From 1b9e8da7e7328a460d0bfe8b579d48169bc280aa Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 25 Aug 2026 03:33:44 +0200 Subject: [PATCH] refactor(recfilter): extract coverage and target-coverage sizing to pkg ApplyCoverage and ApplyTargetCoverage (with its RI and SP branches) lived in package main, so the MCP search tool could not size recommendations the way the CLI does. A model reimplementing --coverage by multiplying costs by 0.8 gets the money wrong by up to ~50%: the RI path scales cost-bearing fields by the DISCRETE ratio newCount/Count, not the requested ratio. Extracting the real implementation is the only way both surfaces agree. Both functions move into pkg/recfilter/sizing.go and take an injected Logf, since cmd's AppLogger writes to os.Stdout and the MCP server owns stdout as its protocol transport. cmd keeps thin wrappers under the existing names passing AppLogger.Printf, so its sizing tests run untouched. The exported ApplyCoverage/applyCoverage pair collapses into one recfilter function taking drops; the split only existed to give the exported form a shorter signature. Refs #1883 --- cmd/helpers.go | 365 +-------------------------------- pkg/recfilter/sizing.go | 368 +++++++++++++++++++++++++++++++++ pkg/recfilter/sizing_test.go | 383 +++++++++++++++++++++++++++++++++++ 3 files changed, 762 insertions(+), 354 deletions(-) create mode 100644 pkg/recfilter/sizing.go create mode 100644 pkg/recfilter/sizing_test.go diff --git a/cmd/helpers.go b/cmd/helpers.go index 309599edd..911708112 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -5,7 +5,6 @@ import ( "context" "fmt" "log" - "math" "os" "strings" "sync" @@ -13,6 +12,7 @@ import ( "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/provider" + "github.com/LeanerCloud/CUDly/pkg/recfilter" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/organizations" "golang.org/x/term" @@ -114,366 +114,23 @@ func CalculateTotalInstances(recs []common.Recommendation) int { return total } -// ApplyCoverage applies coverage percentage to recommendations. -// -// All cost-bearing fields (CommitmentCost, OnDemandCost, EstimatedSavings, -// and for SPs the SavingsPlanDetails.HourlyCommitment) scale by coverage/100 -// so the returned Recommendation represents the sized purchase rather than -// AWS's pre-sized proposal. SavingsPercentage is invariant (savings vs -// on-demand ratio) and stays unscaled. Pre-sizing values can still be -// recovered: RecommendedCount holds AWS's pre-sized count for RIs. +// ApplyCoverage delegates to recfilter.ApplyCoverage, wiring AppLogger as the +// logging sink. Substantive documentation lives on recfilter.ApplyCoverage. func ApplyCoverage(recs []common.Recommendation, coverage float64) []common.Recommendation { - return applyCoverage(recs, coverage, nil) + return recfilter.ApplyCoverage(recs, coverage, AppLogger.Printf, nil) } -// applyCoverage applies legacy percentage sizing and optionally records -// recommendations whose discrete RI count is reduced to zero. +// applyCoverage delegates to recfilter.ApplyCoverage, wiring AppLogger as the +// logging sink. Substantive documentation lives on recfilter.ApplyCoverage. func applyCoverage(recs []common.Recommendation, coverage float64, drops *common.DropSummary) []common.Recommendation { - if coverage >= 100 { - return recs - } - if coverage <= 0 { - return []common.Recommendation{} - } - - ratio := coverage / 100.0 - result := make([]common.Recommendation, 0, len(recs)) - for _rvc := range recs { - rec := recs[_rvc] - adjusted := rec - - // For Savings Plans, reduce the hourly commitment instead of count. - // If Details is the wrong type or a nil pointer (defensive — it - // should always be a non-nil *SavingsPlanDetails for SP recs), - // preserve the recommendation at its original values rather than - // silently dropping it. A missing-Details record is a logged - // anomaly, not a reason to erase coverage from the run. - // - // The nil check matters: an interface holding a typed nil satisfies - // the assertion, so testing ok alone would send an unscalable rec - // down the scaling path. - if common.IsSavingsPlan(rec.Service) { - if details, ok := rec.Details.(*common.SavingsPlanDetails); ok && details != nil { - // ScaleRecommendationCosts scales HourlyCommitment along with - // the cost fields and replaces Details with a scaled copy. - adjusted = common.ScaleRecommendationCosts(adjusted, ratio) - } else { - AppLogger.Printf("WARNING: SP recommendation for service %q has missing or unexpected Details (%T); passing through unscaled\n", rec.Service, rec.Details) - } - result = append(result, adjusted) - continue - } - - // For RIs, reduce the count and scale cost-bearing fields by the - // DISCRETE count ratio (newCount / rec.Count) rather than the - // requested ratio. Truncating newCount to an int then multiplying - // costs by the unrounded ratio desynchronises Count and costs: - // e.g. rec.Count=3 + ratio=0.5 yields newCount=1 (33% of instances) - // but costs would scale to 50%, overstating the sized purchase - // price by ~50%. Mirrors ApplyTargetCoverage / family-NU sizing. - // rec.Count is guaranteed > 0 here because newCount > 0 implies - // rec.Count >= 1 (int(0 * ratio) is 0 for any ratio). - newCount := int(float64(rec.Count) * ratio) - if newCount > 0 { - sizedRatio := float64(newCount) / float64(rec.Count) - adjusted = common.ScaleRecommendationCosts(adjusted, sizedRatio) - adjusted.Count = newCount - result = append(result, adjusted) - } else if drops != nil { - drops.Add(common.DropTargetSizedToZero, 1) - } - } - return result + return recfilter.ApplyCoverage(recs, coverage, AppLogger.Printf, drops) } -// ApplyTargetCoverage sizes RI/SP recommendations so that projected -// post-purchase COVERAGE lands near targetPct, leaving (100-targetPct)% of -// historical demand on-demand as headroom. See ApplyCoverage for the simpler -// rec.Count-scaled coverage flag; the two are dispatched via applySizing. -// -// AWS's recommendation count is sized for ~100% coverage of historical demand -// (average instances used per hour). --target-coverage is the lever the -// operator uses to deliberately under-buy that baseline, accepting more -// on-demand spend in exchange for less idle commitment when demand is bursty -// or trending down. -// -// The flag name says "utilization" because the original framing (issue #338) -// was a utilization floor. In practice operators set values like 70 or 80 -// expecting coverage near that figure (with utilization staying ~100% on the -// commitments actually purchased), not the over-buy semantics that floor -// produces; see the #338 review discussion for the redirect. -// -// RIs (existing-aware, per-pool, strict-target): -// -// gap = targetPct - ExistingCoveragePct (percentage points) -// remaining_gap = 100 - ExistingCoveragePct (percentage points) -// n_target = floor(rec.Count * gap / remaining_gap) -// -// The formula scales AWS's per-account-incremental rec.Count by the -// fraction of the current-to-100% gap we want to fill. For example -// with existing=50% and target=80%: gap=30, remaining_gap=50, so we -// buy 30/50 = 60% of AWS's rec.Count. Anchoring to rec.Count (which -// AWS computed per-linked-account) is more robust in multi-account -// orgs than scaling against avg, since CE's ExistingCoveragePct is -// org-wide averaged and mixes accounts together. -// -// If gap <= 0 (existing already at/above target) → drop with INFO log. -// If n_target == 0 (gap too small to fit one RI) → drop with INFO log. -// If AverageInstancesUsedPerHour <= 0 → pass through (no signal); counted -// in the per-run skip summary. -// Projected coverage = ExistingCoveragePct + n_target/avg * 100 (total -// coverage after the purchase, clamped to 100). Projected utilization = -// avg/n_target * 100 clamped to 100. -// -// ExistingCoveragePct is sourced from CE GetReservationCoverage in the -// same pool; zero means "no signal" and the formula reduces to -// floor(rec.Count * target/100) — i.e. plain target% of AWS's count. -// For RDS the coverage lookup keys by (region, instance_type, engine). -// Floor (rather than ceil or round) gives strict "at-most-target" -// sizing. Pools too small to approximate the target meaningfully -// should be filtered upstream via --min-pool-size; floor will drop -// them as zero-count otherwise. -// -// Pools where CE reports 100% existing coverage but AWS still recommends -// new RIs (typical when existing RIs are near expiry) are dropped here — -// the existing coverage is honored strictly. Use --rebuy-window-days to -// surface those replacements before the cliff. -// -// SPs: -// -// Scale SavingsPlanDetails.HourlyCommitment and EstimatedSavings by -// targetPct/100 (the same lever ApplyCoverage's SP branch uses, but with -// the explicit utilization-target framing). RecommendedUtilization is used -// only as the no-signal guard: when AWS hasn't returned a projected -// utilization figure, we pass the rec through unchanged and count it in -// the skip summary, since we can't sanity-check what the scaled commitment -// would mean. -// If RecommendedUtilization <= 0 → pass through; counted in skip summary. -// -// Recs of any other CommitmentType are passed through unmodified (warned -// once per type per run). -// ApplyTargetCoverage applies the target coverage percentage to a slice of -// recommendations. drops accumulates per-reason drop counts for the -// end-of-run summary; pass nil to skip tracking. +// ApplyTargetCoverage delegates to recfilter.ApplyTargetCoverage, wiring +// AppLogger as the logging sink. Substantive documentation (the RI/SP sizing +// formulas and the #338 flag-name history) lives on recfilter.ApplyTargetCoverage. func ApplyTargetCoverage(recs []common.Recommendation, targetPct float64, drops *common.DropSummary) []common.Recommendation { - if targetPct <= 0 || targetPct > 100 { - // Validation ensures we never get here in production, but be defensive - // so a buggy caller doesn't divide by zero. - AppLogger.Printf("WARNING: ApplyTargetCoverage called with targetPct=%.2f outside (0,100]; returning recs unchanged\n", targetPct) - return recs - } - - result := make([]common.Recommendation, 0, len(recs)) - var skipped int - unsupportedSeen := make(map[common.CommitmentType]bool) - - for i := range recs { - adjusted, kept, missingSignal, dropReason := applyTargetCoverageOne(recs[i], targetPct, unsupportedSeen) - if missingSignal { - skipped++ - } - if kept { - result = append(result, adjusted) - } else if dropReason != "" { - drops.Add(dropReason, 1) - } - } - - if skipped > 0 { - AppLogger.Printf("INFO: --target-coverage=%.1f%% skipped %d of %d recommendations with no utilization signal (passed through unchanged)\n", - targetPct, skipped, len(recs)) - } - - return result -} - -// applyTargetCoverageOne dispatches a single recommendation through the -// appropriate branch. Returns (rec, kept, missingSignal, dropReason): -// - kept=true → caller appends `rec` (the adjusted or pass-through value). -// - kept=false → caller drops the rec (only the RI "target unreachable" -// branches return this; an INFO log already fired). -// - missingSignal=true → counted toward the end-of-run skip summary. -// - dropReason is non-empty when kept=false and the drop has a named category. -// -// Split out of ApplyTargetCoverage to keep that function under gocyclo's -// complexity threshold. -func applyTargetCoverageOne(rec common.Recommendation, targetPct float64, unsupportedSeen map[common.CommitmentType]bool) (result common.Recommendation, kept, missingSignal bool, drop string) { - switch { - case common.IsSavingsPlan(rec.Service): - adjusted, ok := applyTargetCoverageSP(rec, targetPct) - if !ok { - // SP no-signal: pass through unchanged. - return rec, true, true, "" - } - return adjusted, true, false, "" - case rec.CommitmentType == common.CommitmentReservedInstance: - adjusted, ok, dropReason := applyTargetCoverageRI(rec, targetPct) - if !ok { - // Distinguish "no signal" (pass through, count in summary) from - // "target unreachable" (drop with already-fired INFO log). - if rec.AverageInstancesUsedPerHour <= 0 { - return rec, true, true, "" - } - return rec, false, false, dropReason - } - return adjusted, true, false, "" - default: - if !unsupportedSeen[rec.CommitmentType] { - AppLogger.Printf("WARNING: --target-coverage not supported for CommitmentType=%q; passing recommendations through unchanged\n", rec.CommitmentType) - unsupportedSeen[rec.CommitmentType] = true - } - return rec, true, false, "" - } -} - -// applyTargetCoverageRI is the RI branch of ApplyTargetCoverage. Returns -// (adjusted, true, "") on success, (rec, false, dropReason) when the rec -// should be passed through unscaled (no signal) or dropped (target -// unreachable). Caller distinguishes no-signal from drop via -// rec.AverageInstancesUsedPerHour and uses dropReason for the summary. -func applyTargetCoverageRI(rec common.Recommendation, targetPct float64) (result common.Recommendation, ok bool, drop string) { - if rec.AverageInstancesUsedPerHour <= 0 { - // No signal — caller will pass through and count in the summary. - return rec, false, "" - } - - avg := rec.AverageInstancesUsedPerHour - // Coverage-anchored under-buy: size linearly off the pool's avg demand - // and the absolute gap to target. Both inputs come from - // GetReservationCoverage (AvgInstancesPerHour from - // TotalRunningHours/window; ExistingCoveragePct from - // CoverageHoursPercentage) so the buy lines up with the AWS console's - // reservations-coverage report: target%-existing% of avg instances. - // - // The previous formula anchored on AWS's rec.Count - // (floor(rec.Count × gap / (100−existing))), which under-bought when - // AWS sized rec.Count for less than full coverage (ROI-curated) and - // when CE's org-wide existing% disagreed with rec.Count's per-account - // derivation. Anchoring on coverage's own avg removes both mismatches. - // rec.Count is retained only for the cost-scaling ratio further down. - // - // Keep the subtraction in percentage units (subtract first, divide - // later) so whole-percent values don't lose precision to float - // rounding at integer boundaries. - gapPct := targetPct - rec.ExistingCoveragePct - if gapPct <= 0 { - // Existing commitments already meet or exceed the target; no purchase - // needed in this pool. Drop with an info log so operators can see what - // the flag did. Returning (_, false) with avg > 0 signals "drop, don't - // pass through". - AppLogger.Printf("INFO: --target-coverage=%.1f%% already met by existing coverage %.1f%% for %s/%s/%s; dropped recommendation\n", - targetPct, rec.ExistingCoveragePct, rec.Service, rec.Region, rec.ResourceType) - return rec, false, common.DropTargetAlreadyMet - } - // Floor so we never over-shoot the target on integer-arithmetic edges. - // Strict-target semantics: 80% means "at most 80% coverage", not "at - // least 80%". Floor under-covers small/odd pools (e.g. avg=2, target=80 - // gives 1 RI = 50% rather than 2 RIs = 100%); pools too small to - // approximate target are best filtered out via --min-pool-size upstream. - nTarget := int(math.Floor(avg * gapPct / 100.0)) - - if nTarget == 0 { - // Floor produces zero when avg × gap% < 100 (small pools or thin - // gaps). Drop — buying 1 RI would over-shoot target and the - // strict-target intent prefers under-cover (run on-demand) over - // over-cover (idle commitment). Use --min-pool-size to filter - // these out earlier so they don't show up as drops in the log. - AppLogger.Printf("INFO: --target-coverage=%.1f%% sizes %s/%s/%s to 0 instances (avg=%.2f, gap=%.2f%% produces <1 RI); dropped recommendation\n", - targetPct, rec.Service, rec.Region, rec.ResourceType, avg, gapPct) - // Returning (_, false) with avg > 0 signals "drop, don't pass through". - // applyTargetCoverageRI's caller branches on - // rec.AverageInstancesUsedPerHour to distinguish drop vs no-signal. - return rec, false, common.DropTargetSizedToZero - } - - // Cost-bearing fields scale by the ratio of sized-to-original count, so the - // returned rec represents the sized purchase rather than AWS's pre-sized - // proposal. SavingsPercentage is invariant (savings vs on-demand ratio). - // rec.Count is the AWS pre-sizing count at this point (parser sets Count - // == RecommendedCount and we haven't mutated either yet). When the - // coverage-anchored nTarget exceeds rec.Count (AWS sized below full - // coverage), the ratio scales costs up linearly — accurate when per-RI - // pricing is constant, which it is within a single pool/term/payment - // combination. Guarded against rec.Count==0 (malformed rec) by falling - // back to nTarget so a zero-cost rec stays zero-cost rather than NaN. - var ratio float64 - if rec.Count > 0 { - ratio = float64(nTarget) / float64(rec.Count) - } else { - ratio = float64(nTarget) - } - adjusted := common.ScaleRecommendationCosts(rec, ratio) - adjusted.Count = nTarget - - // Projection metrics. ProjectedCoverage is TOTAL coverage (existing + - // new) so operators can see the figure they actually targeted. - // ProjectedUtilization stays at the per-purchase fill rate; under-buy - // keeps nTarget <= avg so it always clamps to 100%. - projUtil := avg / float64(nTarget) * 100.0 - if projUtil > 100 { - projUtil = 100 - } - projCov := rec.ExistingCoveragePct + float64(nTarget)/avg*100.0 - if projCov > 100 { - projCov = 100 - } - adjusted.ProjectedUtilization = projUtil - adjusted.ProjectedCoverage = projCov - return adjusted, true, "" -} - -// applyTargetCoverageSP is the SP branch of ApplyTargetCoverage. Returns -// (adjusted, true) when the rec is kept, (rec, false) when it should be -// skipped (caller passes through unscaled and counts in the skip summary). -func applyTargetCoverageSP(rec common.Recommendation, targetPct float64) (common.Recommendation, bool) { - if rec.RecommendedUtilization <= 0 { - return rec, false - } - // If Details isn't a non-nil *SavingsPlanDetails (defensive — it should - // always be one for SP recs), log a warning and pass through UNCHANGED — - // including leaving ProjectedUtilization at zero. Setting projection - // fields on a rec whose commitment fields couldn't be scaled would - // produce a misleading row (projection=target%, savings=full-unscaled). - // - // The nil check must precede the HourlyCommitment read below: an - // interface holding a typed nil satisfies the assertion, so reading the - // field off it would dereference nil. - details, ok := rec.Details.(*common.SavingsPlanDetails) - if !ok || details == nil { - AppLogger.Printf("WARNING: SP recommendation for service %q has missing or unexpected Details (%T); passing through unscaled\n", rec.Service, rec.Details) - return rec, true - } - // Also treat a $0 HourlyCommitment as "no signal" — CE occasionally - // returns placeholder recs with zero commitment. Sizing such a rec - // would produce nonsense ($0 commitment * ratio = $0) while still - // claiming the target coverage is achieved, which is incoherent. - // Pass through unchanged and count in the skip summary. - if details.HourlyCommitment <= 0 { - return rec, false - } - - // Under-buy: scale all cost-bearing fields by target/100 against AWS's - // recommended commitment. This deliberately spends less than AWS suggested, - // leaving (100-target)% of the SP's projected workload on on-demand. - // RecommendedUtilization is consulted only as a no-signal guard above (a - // zero value means we can't sanity-check the result); the scaling itself - // uses targetPct directly rather than a recUtil/target ratio so the flag's - // intent is honored even when AWS already projects above target. - ratio := targetPct / 100.0 - // ScaleRecommendationCosts scales HourlyCommitment along with the cost - // fields and replaces Details with a scaled copy. - adjusted := common.ScaleRecommendationCosts(rec, ratio) - // Shrinking commitment raises projected utilization by 1/ratio - // (used is fixed = orig_commit * RecUtil, bought is orig_commit * ratio). - // Clamp to 100 since utilization caps at full use. - projUtil := rec.RecommendedUtilization / ratio - if projUtil > 100 { - projUtil = 100 - } - adjusted.ProjectedUtilization = projUtil - // ProjectedCoverage stays zero for SPs — CE doesn't expose total-demand-$ - // for a clean coverage figure (see field doc on Recommendation). - return adjusted, true + return recfilter.ApplyTargetCoverage(recs, targetPct, AppLogger.Printf, drops) } // applySizing chooses target-coverage or coverage sizing. diff --git a/pkg/recfilter/sizing.go b/pkg/recfilter/sizing.go new file mode 100644 index 000000000..e7a4562e3 --- /dev/null +++ b/pkg/recfilter/sizing.go @@ -0,0 +1,368 @@ +package recfilter + +import ( + "math" + + "github.com/LeanerCloud/CUDly/pkg/common" +) + +// ApplyCoverage applies coverage percentage to recommendations. +// +// All cost-bearing fields (CommitmentCost, OnDemandCost, EstimatedSavings, +// and for SPs the SavingsPlanDetails.HourlyCommitment) scale by coverage/100 +// so the returned Recommendation represents the sized purchase rather than +// AWS's pre-sized proposal. SavingsPercentage is invariant (savings vs +// on-demand ratio) and stays unscaled. Pre-sizing values can still be +// recovered: RecommendedCount holds AWS's pre-sized count for RIs. +// +// drops accumulates per-reason drop counts for the end-of-run summary; pass +// nil to skip tracking. logf receives WARNING lines for anomalous recs +// (nil-safe; pass nil to disable logging). +func ApplyCoverage(recs []common.Recommendation, coverage float64, logf Logf, drops *common.DropSummary) []common.Recommendation { + if coverage >= 100 { + return recs + } + if coverage <= 0 { + return []common.Recommendation{} + } + + ratio := coverage / 100.0 + result := make([]common.Recommendation, 0, len(recs)) + for _rvc := range recs { + rec := recs[_rvc] + adjusted := rec + + // For Savings Plans, reduce the hourly commitment instead of count. + // If Details is the wrong type or a nil pointer (defensive — it + // should always be a non-nil *SavingsPlanDetails for SP recs), + // preserve the recommendation at its original values rather than + // silently dropping it. A missing-Details record is a logged + // anomaly, not a reason to erase coverage from the run. + // + // The nil check matters: an interface holding a typed nil satisfies + // the assertion, so testing ok alone would send an unscalable rec + // down the scaling path. + if common.IsSavingsPlan(rec.Service) { + if details, ok := rec.Details.(*common.SavingsPlanDetails); ok && details != nil { + // ScaleRecommendationCosts scales HourlyCommitment along with + // the cost fields and replaces Details with a scaled copy. + adjusted = common.ScaleRecommendationCosts(adjusted, ratio) + } else { + logf.printf("WARNING: SP recommendation for service %q has missing or unexpected Details (%T); passing through unscaled\n", rec.Service, rec.Details) + } + result = append(result, adjusted) + continue + } + + // For RIs, reduce the count and scale cost-bearing fields by the + // DISCRETE count ratio (newCount / rec.Count) rather than the + // requested ratio. Truncating newCount to an int then multiplying + // costs by the unrounded ratio desynchronises Count and costs: + // e.g. rec.Count=3 + ratio=0.5 yields newCount=1 (33% of instances) + // but costs would scale to 50%, overstating the sized purchase + // price by ~50%. Mirrors ApplyTargetCoverage / family-NU sizing. + // rec.Count is guaranteed > 0 here because newCount > 0 implies + // rec.Count >= 1 (int(0 * ratio) is 0 for any ratio). + newCount := int(float64(rec.Count) * ratio) + if newCount > 0 { + sizedRatio := float64(newCount) / float64(rec.Count) + adjusted = common.ScaleRecommendationCosts(adjusted, sizedRatio) + adjusted.Count = newCount + result = append(result, adjusted) + } else if drops != nil { + drops.Add(common.DropTargetSizedToZero, 1) + } + } + return result +} + +// ApplyTargetCoverage sizes RI/SP recommendations so that projected +// post-purchase COVERAGE lands near targetPct, leaving (100-targetPct)% of +// historical demand on-demand as headroom. See ApplyCoverage for the simpler +// rec.Count-scaled coverage flag; the two are dispatched via cmd's applySizing. +// +// AWS's recommendation count is sized for ~100% coverage of historical demand +// (average instances used per hour). --target-coverage is the lever the +// operator uses to deliberately under-buy that baseline, accepting more +// on-demand spend in exchange for less idle commitment when demand is bursty +// or trending down. +// +// The flag name says "utilization" because the original framing (issue #338) +// was a utilization floor. In practice operators set values like 70 or 80 +// expecting coverage near that figure (with utilization staying ~100% on the +// commitments actually purchased), not the over-buy semantics that floor +// produces; see the #338 review discussion for the redirect. +// +// RIs (existing-aware, per-pool, strict-target): +// +// gap = targetPct - ExistingCoveragePct (percentage points) +// remaining_gap = 100 - ExistingCoveragePct (percentage points) +// n_target = floor(rec.Count * gap / remaining_gap) +// +// The formula scales AWS's per-account-incremental rec.Count by the +// fraction of the current-to-100% gap we want to fill. For example +// with existing=50% and target=80%: gap=30, remaining_gap=50, so we +// buy 30/50 = 60% of AWS's rec.Count. Anchoring to rec.Count (which +// AWS computed per-linked-account) is more robust in multi-account +// orgs than scaling against avg, since CE's ExistingCoveragePct is +// org-wide averaged and mixes accounts together. +// +// If gap <= 0 (existing already at/above target) → drop with INFO log. +// If n_target == 0 (gap too small to fit one RI) → drop with INFO log. +// If AverageInstancesUsedPerHour <= 0 → pass through (no signal); counted +// in the per-run skip summary. +// Projected coverage = ExistingCoveragePct + n_target/avg * 100 (total +// coverage after the purchase, clamped to 100). Projected utilization = +// avg/n_target * 100 clamped to 100. +// +// ExistingCoveragePct is sourced from CE GetReservationCoverage in the +// same pool; zero means "no signal" and the formula reduces to +// floor(rec.Count * target/100) — i.e. plain target% of AWS's count. +// For RDS the coverage lookup keys by (region, instance_type, engine). +// Floor (rather than ceil or round) gives strict "at-most-target" +// sizing. Pools too small to approximate the target meaningfully +// should be filtered upstream via --min-pool-size; floor will drop +// them as zero-count otherwise. +// +// Pools where CE reports 100% existing coverage but AWS still recommends +// new RIs (typical when existing RIs are near expiry) are dropped here — +// the existing coverage is honored strictly. Use --rebuy-window-days to +// surface those replacements before the cliff. +// +// SPs: +// +// Scale SavingsPlanDetails.HourlyCommitment and EstimatedSavings by +// targetPct/100 (the same lever ApplyCoverage's SP branch uses, but with +// the explicit utilization-target framing). RecommendedUtilization is used +// only as the no-signal guard: when AWS hasn't returned a projected +// utilization figure, we pass the rec through unchanged and count it in +// the skip summary, since we can't sanity-check what the scaled commitment +// would mean. +// If RecommendedUtilization <= 0 → pass through; counted in skip summary. +// +// Recs of any other CommitmentType are passed through unmodified (warned +// once per type per run). +// +// drops accumulates per-reason drop counts for the end-of-run summary; pass +// nil to skip tracking. logf receives WARNING/INFO lines (nil-safe; pass nil +// to disable logging). +func ApplyTargetCoverage(recs []common.Recommendation, targetPct float64, logf Logf, drops *common.DropSummary) []common.Recommendation { + if targetPct <= 0 || targetPct > 100 { + // Validation ensures we never get here in production, but be defensive + // so a buggy caller doesn't divide by zero. + logf.printf("WARNING: ApplyTargetCoverage called with targetPct=%.2f outside (0,100]; returning recs unchanged\n", targetPct) + return recs + } + + result := make([]common.Recommendation, 0, len(recs)) + var skipped int + unsupportedSeen := make(map[common.CommitmentType]bool) + + for i := range recs { + adjusted, kept, missingSignal, dropReason := applyTargetCoverageOne(recs[i], targetPct, unsupportedSeen, logf) + if missingSignal { + skipped++ + } + if kept { + result = append(result, adjusted) + } else if dropReason != "" { + drops.Add(dropReason, 1) + } + } + + if skipped > 0 { + logf.printf("INFO: --target-coverage=%.1f%% skipped %d of %d recommendations with no utilization signal (passed through unchanged)\n", + targetPct, skipped, len(recs)) + } + + return result +} + +// applyTargetCoverageOne dispatches a single recommendation through the +// appropriate branch. Returns (rec, kept, missingSignal, dropReason): +// - kept=true → caller appends `rec` (the adjusted or pass-through value). +// - kept=false → caller drops the rec (only the RI "target unreachable" +// branches return this; an INFO log already fired). +// - missingSignal=true → counted toward the end-of-run skip summary. +// - dropReason is non-empty when kept=false and the drop has a named category. +// +// Split out of ApplyTargetCoverage to keep that function under gocyclo's +// complexity threshold. +func applyTargetCoverageOne(rec common.Recommendation, targetPct float64, unsupportedSeen map[common.CommitmentType]bool, logf Logf) (result common.Recommendation, kept, missingSignal bool, drop string) { + switch { + case common.IsSavingsPlan(rec.Service): + adjusted, ok := applyTargetCoverageSP(rec, targetPct, logf) + if !ok { + // SP no-signal: pass through unchanged. + return rec, true, true, "" + } + return adjusted, true, false, "" + case rec.CommitmentType == common.CommitmentReservedInstance: + adjusted, ok, dropReason := applyTargetCoverageRI(rec, targetPct, logf) + if !ok { + // Distinguish "no signal" (pass through, count in summary) from + // "target unreachable" (drop with already-fired INFO log). + if rec.AverageInstancesUsedPerHour <= 0 { + return rec, true, true, "" + } + return rec, false, false, dropReason + } + return adjusted, true, false, "" + default: + if !unsupportedSeen[rec.CommitmentType] { + logf.printf("WARNING: --target-coverage not supported for CommitmentType=%q; passing recommendations through unchanged\n", rec.CommitmentType) + unsupportedSeen[rec.CommitmentType] = true + } + return rec, true, false, "" + } +} + +// applyTargetCoverageRI is the RI branch of ApplyTargetCoverage. Returns +// (adjusted, true, "") on success, (rec, false, dropReason) when the rec +// should be passed through unscaled (no signal) or dropped (target +// unreachable). Caller distinguishes no-signal from drop via +// rec.AverageInstancesUsedPerHour and uses dropReason for the summary. +func applyTargetCoverageRI(rec common.Recommendation, targetPct float64, logf Logf) (result common.Recommendation, ok bool, drop string) { + if rec.AverageInstancesUsedPerHour <= 0 { + // No signal — caller will pass through and count in the summary. + return rec, false, "" + } + + avg := rec.AverageInstancesUsedPerHour + // Coverage-anchored under-buy: size linearly off the pool's avg demand + // and the absolute gap to target. Both inputs come from + // GetReservationCoverage (AvgInstancesPerHour from + // TotalRunningHours/window; ExistingCoveragePct from + // CoverageHoursPercentage) so the buy lines up with the AWS console's + // reservations-coverage report: target%-existing% of avg instances. + // + // The previous formula anchored on AWS's rec.Count + // (floor(rec.Count × gap / (100−existing))), which under-bought when + // AWS sized rec.Count for less than full coverage (ROI-curated) and + // when CE's org-wide existing% disagreed with rec.Count's per-account + // derivation. Anchoring on coverage's own avg removes both mismatches. + // rec.Count is retained only for the cost-scaling ratio further down. + // + // Keep the subtraction in percentage units (subtract first, divide + // later) so whole-percent values don't lose precision to float + // rounding at integer boundaries. + gapPct := targetPct - rec.ExistingCoveragePct + if gapPct <= 0 { + // Existing commitments already meet or exceed the target; no purchase + // needed in this pool. Drop with an info log so operators can see what + // the flag did. Returning (_, false) with avg > 0 signals "drop, don't + // pass through". + logf.printf("INFO: --target-coverage=%.1f%% already met by existing coverage %.1f%% for %s/%s/%s; dropped recommendation\n", + targetPct, rec.ExistingCoveragePct, rec.Service, rec.Region, rec.ResourceType) + return rec, false, common.DropTargetAlreadyMet + } + // Floor so we never over-shoot the target on integer-arithmetic edges. + // Strict-target semantics: 80% means "at most 80% coverage", not "at + // least 80%". Floor under-covers small/odd pools (e.g. avg=2, target=80 + // gives 1 RI = 50% rather than 2 RIs = 100%); pools too small to + // approximate target are best filtered out via --min-pool-size upstream. + nTarget := int(math.Floor(avg * gapPct / 100.0)) + + if nTarget == 0 { + // Floor produces zero when avg × gap% < 100 (small pools or thin + // gaps). Drop — buying 1 RI would over-shoot target and the + // strict-target intent prefers under-cover (run on-demand) over + // over-cover (idle commitment). Use --min-pool-size to filter + // these out earlier so they don't show up as drops in the log. + logf.printf("INFO: --target-coverage=%.1f%% sizes %s/%s/%s to 0 instances (avg=%.2f, gap=%.2f%% produces <1 RI); dropped recommendation\n", + targetPct, rec.Service, rec.Region, rec.ResourceType, avg, gapPct) + // Returning (_, false) with avg > 0 signals "drop, don't pass through". + // applyTargetCoverageRI's caller branches on + // rec.AverageInstancesUsedPerHour to distinguish drop vs no-signal. + return rec, false, common.DropTargetSizedToZero + } + + // Cost-bearing fields scale by the ratio of sized-to-original count, so the + // returned rec represents the sized purchase rather than AWS's pre-sized + // proposal. SavingsPercentage is invariant (savings vs on-demand ratio). + // rec.Count is the AWS pre-sizing count at this point (parser sets Count + // == RecommendedCount and we haven't mutated either yet). When the + // coverage-anchored nTarget exceeds rec.Count (AWS sized below full + // coverage), the ratio scales costs up linearly — accurate when per-RI + // pricing is constant, which it is within a single pool/term/payment + // combination. Guarded against rec.Count==0 (malformed rec) by falling + // back to nTarget so a zero-cost rec stays zero-cost rather than NaN. + var ratio float64 + if rec.Count > 0 { + ratio = float64(nTarget) / float64(rec.Count) + } else { + ratio = float64(nTarget) + } + adjusted := common.ScaleRecommendationCosts(rec, ratio) + adjusted.Count = nTarget + + // Projection metrics. ProjectedCoverage is TOTAL coverage (existing + + // new) so operators can see the figure they actually targeted. + // ProjectedUtilization stays at the per-purchase fill rate; under-buy + // keeps nTarget <= avg so it always clamps to 100%. + projUtil := avg / float64(nTarget) * 100.0 + if projUtil > 100 { + projUtil = 100 + } + projCov := rec.ExistingCoveragePct + float64(nTarget)/avg*100.0 + if projCov > 100 { + projCov = 100 + } + adjusted.ProjectedUtilization = projUtil + adjusted.ProjectedCoverage = projCov + return adjusted, true, "" +} + +// applyTargetCoverageSP is the SP branch of ApplyTargetCoverage. Returns +// (adjusted, true) when the rec is kept, (rec, false) when it should be +// skipped (caller passes through unscaled and counts in the skip summary). +func applyTargetCoverageSP(rec common.Recommendation, targetPct float64, logf Logf) (common.Recommendation, bool) { + if rec.RecommendedUtilization <= 0 { + return rec, false + } + // If Details isn't a non-nil *SavingsPlanDetails (defensive — it should + // always be one for SP recs), log a warning and pass through UNCHANGED — + // including leaving ProjectedUtilization at zero. Setting projection + // fields on a rec whose commitment fields couldn't be scaled would + // produce a misleading row (projection=target%, savings=full-unscaled). + // + // The nil check must precede the HourlyCommitment read below: an + // interface holding a typed nil satisfies the assertion, so reading the + // field off it would dereference nil. + details, ok := rec.Details.(*common.SavingsPlanDetails) + if !ok || details == nil { + logf.printf("WARNING: SP recommendation for service %q has missing or unexpected Details (%T); passing through unscaled\n", rec.Service, rec.Details) + return rec, true + } + // Also treat a $0 HourlyCommitment as "no signal" — CE occasionally + // returns placeholder recs with zero commitment. Sizing such a rec + // would produce nonsense ($0 commitment * ratio = $0) while still + // claiming the target coverage is achieved, which is incoherent. + // Pass through unchanged and count in the skip summary. + if details.HourlyCommitment <= 0 { + return rec, false + } + + // Under-buy: scale all cost-bearing fields by target/100 against AWS's + // recommended commitment. This deliberately spends less than AWS suggested, + // leaving (100-target)% of the SP's projected workload on on-demand. + // RecommendedUtilization is consulted only as a no-signal guard above (a + // zero value means we can't sanity-check the result); the scaling itself + // uses targetPct directly rather than a recUtil/target ratio so the flag's + // intent is honored even when AWS already projects above target. + ratio := targetPct / 100.0 + // ScaleRecommendationCosts scales HourlyCommitment along with the cost + // fields and replaces Details with a scaled copy. + adjusted := common.ScaleRecommendationCosts(rec, ratio) + // Shrinking commitment raises projected utilization by 1/ratio + // (used is fixed = orig_commit * RecUtil, bought is orig_commit * ratio). + // Clamp to 100 since utilization caps at full use. + projUtil := rec.RecommendedUtilization / ratio + if projUtil > 100 { + projUtil = 100 + } + adjusted.ProjectedUtilization = projUtil + // ProjectedCoverage stays zero for SPs — CE doesn't expose total-demand-$ + // for a clean coverage figure (see field doc on Recommendation). + return adjusted, true +} diff --git a/pkg/recfilter/sizing_test.go b/pkg/recfilter/sizing_test.go new file mode 100644 index 000000000..cca3495a6 --- /dev/null +++ b/pkg/recfilter/sizing_test.go @@ -0,0 +1,383 @@ +package recfilter + +import ( + "fmt" + "math" + "strings" + "testing" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// captureLogf returns a Logf that appends every formatted line to *log, for +// tests asserting on warning/info output without touching a global logger. +func captureLogf(log *[]string) Logf { + return func(format string, args ...any) { + *log = append(*log, fmt.Sprintf(format, args...)) + } +} + +// --- T4: ApplyCoverage --- + +// TestApplyCoverage_DiscreteRatioRegression is the most important test in +// this file. Count=3 at coverage=50% must scale money fields to 1/3, NOT +// 0.5: newCount = int(3*0.5) = 1, so the DISCRETE ratio (1/3) governs cost +// scaling, not the requested ratio (0.5). Regressing to the requested ratio +// scales money by 0.5 while Count truncates to 1, overstating the sized +// purchase's cost by ~50% relative to what one instance actually costs. +func TestApplyCoverage_DiscreteRatioRegression(t *testing.T) { + t.Parallel() + rec := common.Recommendation{ + Service: common.ServiceEC2, + Count: 3, + CommitmentCost: 300, + OnDemandCost: 600, + EstimatedSavings: 300, + SavingsPercentage: 50, + } + + out := ApplyCoverage([]common.Recommendation{rec}, 50, nil, nil) + + require.Len(t, out, 1) + assert.Equal(t, 1, out[0].Count) + assert.InDelta(t, 100.0, out[0].CommitmentCost, 0.001, "1/3 of 300, not 0.5*300=150") + assert.InDelta(t, 200.0, out[0].OnDemandCost, 0.001, "1/3 of 600, not 0.5*600=300") + assert.InDelta(t, 100.0, out[0].EstimatedSavings, 0.001, "1/3 of 300, not 0.5*300=150") +} + +func TestApplyCoverage_HundredOrAbove_ReturnsUnchanged(t *testing.T) { + t.Parallel() + recs := []common.Recommendation{ + {Service: common.ServiceEC2, Count: 7, CommitmentCost: 111}, + {Service: common.ServiceSavingsPlansCompute, Details: &common.SavingsPlanDetails{HourlyCommitment: 4}}, + } + + for _, coverage := range []float64{100, 150} { + out := ApplyCoverage(recs, coverage, nil, nil) + require.Len(t, out, len(recs)) + assert.Equal(t, recs, out, "coverage=%.0f must return input unchanged", coverage) + } +} + +func TestApplyCoverage_ZeroOrBelow_ReturnsEmpty(t *testing.T) { + t.Parallel() + recs := []common.Recommendation{{Service: common.ServiceEC2, Count: 5}} + + for _, coverage := range []float64{0, -5} { + out := ApplyCoverage(recs, coverage, nil, nil) + require.NotNil(t, out, "coverage=%.0f must return a non-nil empty slice", coverage) + assert.Len(t, out, 0) + } +} + +func TestApplyCoverage_SavingsPlan_ScalesHourlyCommitment_NotCount(t *testing.T) { + t.Parallel() + rec := common.Recommendation{ + Service: common.ServiceSavingsPlansCompute, + Count: 5, + CommitmentCost: 1000, + OnDemandCost: 2000, + EstimatedSavings: 1000, + Details: &common.SavingsPlanDetails{HourlyCommitment: 10}, + } + + out := ApplyCoverage([]common.Recommendation{rec}, 50, nil, nil) + + require.Len(t, out, 1) + assert.Equal(t, 5, out[0].Count, "SP branch never touches Count") + assert.InDelta(t, 5.0, out[0].Details.(*common.SavingsPlanDetails).HourlyCommitment, 0.001) + assert.InDelta(t, 500.0, out[0].CommitmentCost, 0.001) +} + +func TestApplyCoverage_SPBadDetails_PassThroughUnscaled_WarnsOnce(t *testing.T) { + t.Parallel() + + t.Run("wrong type", func(t *testing.T) { + t.Parallel() + var logs []string + rec := common.Recommendation{ + Service: common.ServiceSavingsPlansCompute, + EstimatedSavings: 1500, + Details: common.ComputeDetails{Platform: "Linux/UNIX"}, + } + out := ApplyCoverage([]common.Recommendation{rec}, 50, captureLogf(&logs), nil) + require.Len(t, out, 1) + assert.Equal(t, 1500.0, out[0].EstimatedSavings, "unscaled") + assert.Len(t, logs, 1, "exactly one warning") + }) + + t.Run("typed nil Details", func(t *testing.T) { + t.Parallel() + var logs []string + rec := common.Recommendation{ + Service: common.ServiceSavingsPlansCompute, + EstimatedSavings: 1500, + Details: (*common.SavingsPlanDetails)(nil), + } + // An interface holding a typed nil satisfies the type assertion with + // ok==true, so this case exercises the `ok && details != nil` guard + // specifically — a plain `ok` check would send this down the + // scaling path and panic on the nil dereference. + out := ApplyCoverage([]common.Recommendation{rec}, 50, captureLogf(&logs), nil) + require.Len(t, out, 1) + assert.Equal(t, 1500.0, out[0].EstimatedSavings, "unscaled") + assert.Len(t, logs, 1, "exactly one warning") + }) +} + +func TestApplyCoverage_RISizedToZero_DropsAndRecords(t *testing.T) { + t.Parallel() + rec := common.Recommendation{Service: common.ServiceEC2, Count: 1, CommitmentCost: 100} + + t.Run("with drops summary", func(t *testing.T) { + t.Parallel() + d := common.NewDropSummary() + out := ApplyCoverage([]common.Recommendation{rec}, 10, nil, d) + assert.Empty(t, out) + assert.Equal(t, 1, d.Total()) + assert.Contains(t, d.FormatOneLine(), common.DropTargetSizedToZero) + }) + + t.Run("with nil drops", func(t *testing.T) { + t.Parallel() + assert.NotPanics(t, func() { + out := ApplyCoverage([]common.Recommendation{rec}, 10, nil, nil) + assert.Empty(t, out) + }) + }) +} + +func TestApplyCoverage_NilLogfSafe(t *testing.T) { + t.Parallel() + rec := common.Recommendation{ + Service: common.ServiceSavingsPlansCompute, + Details: common.ComputeDetails{Platform: "Linux/UNIX"}, // wrong type -> warning path + } + assert.NotPanics(t, func() { + ApplyCoverage([]common.Recommendation{rec}, 50, nil, nil) + }) +} + +// --- T5: ApplyTargetCoverage --- + +func mkRI(count int, avg, existingCov float64) common.Recommendation { + return common.Recommendation{ + Service: common.ServiceEC2, + Region: "us-east-1", + ResourceType: "t3.medium", + Count: count, + CommitmentType: common.CommitmentReservedInstance, + CommitmentCost: 1000, + OnDemandCost: 2000, + EstimatedSavings: 500, + AverageInstancesUsedPerHour: avg, + ExistingCoveragePct: existingCov, + } +} + +func mkSP(recUtil, hourlyCommitment float64) common.Recommendation { + return common.Recommendation{ + Service: common.ServiceSavingsPlansCompute, + CommitmentType: common.CommitmentSavingsPlan, + CommitmentCost: 1000, + OnDemandCost: 5000, + EstimatedSavings: 1500, + RecommendedUtilization: recUtil, + Details: &common.SavingsPlanDetails{HourlyCommitment: hourlyCommitment}, + } +} + +func TestApplyTargetCoverage_GapAlreadyMet_Drops(t *testing.T) { + t.Parallel() + rec := mkRI(5, 8.0, 90) // existing=90 >= target=80 + d := common.NewDropSummary() + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil, d) + assert.Empty(t, out) + assert.Equal(t, 1, d.Total()) + assert.Contains(t, d.FormatOneLine(), common.DropTargetAlreadyMet) +} + +func TestApplyTargetCoverage_FloorSizesToZero_Drops(t *testing.T) { + t.Parallel() + // avg=0.5, target=80, existing=0: floor(0.5*80/100)=floor(0.4)=0. + rec := mkRI(1, 0.5, 0) + d := common.NewDropSummary() + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil, d) + assert.Empty(t, out) + assert.Equal(t, 1, d.Total()) + assert.Contains(t, d.FormatOneLine(), common.DropTargetSizedToZero) +} + +func TestApplyTargetCoverage_NoSignal_PassesThroughUnchanged(t *testing.T) { + t.Parallel() + rec := mkRI(5, 0, 0) // AverageInstancesUsedPerHour <= 0 -> no signal + d := common.NewDropSummary() + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil, d) + require.Len(t, out, 1) + assert.Equal(t, rec, out[0], "passed through unchanged") + assert.True(t, d.IsEmpty(), "no-signal is not a drop") +} + +// TestApplyTargetCoverage_ProjectionsClampTo100 covers the clamp on +// ProjectedUtilization (easy to overflow: avg/nTarget*100 grows unbounded as +// nTarget shrinks relative to avg) and documents why ProjectedCoverage's +// clamp is a defensive boundary check rather than a reachable overflow: floor +// guarantees nTarget/avg*100 <= gapPct, so projCov <= existing+gapPct == +// target <= 100 in exact arithmetic. The target=100 boundary case below +// exercises that bound exactly. +func TestApplyTargetCoverage_ProjectionsClampTo100(t *testing.T) { + t.Parallel() + + t.Run("ProjectedUtilization clamps", func(t *testing.T) { + t.Parallel() + // avg=100, target=10, existing=0: gap=10, nTarget=floor(100*10/100)=10. + // Unclamped projUtil = 100/10*100 = 1000%. + rec := mkRI(200, 100, 0) + out := ApplyTargetCoverage([]common.Recommendation{rec}, 10, nil, nil) + require.Len(t, out, 1) + assert.Equal(t, 100.0, out[0].ProjectedUtilization) + }) + + t.Run("ProjectedCoverage stays at the target boundary", func(t *testing.T) { + t.Parallel() + // avg=10, target=100, existing=0: gap=100, nTarget=floor(10)=10. + // projCov = 0 + 10/10*100 = 100.0 exactly at the clamp boundary. + rec := mkRI(10, 10, 0) + out := ApplyTargetCoverage([]common.Recommendation{rec}, 100, nil, nil) + require.Len(t, out, 1) + assert.LessOrEqual(t, out[0].ProjectedCoverage, 100.0) + assert.Equal(t, 100.0, out[0].ProjectedCoverage) + }) +} + +// TestApplyTargetCoverage_CountZeroNoNaNOrInf covers the rec.Count==0 +// fallback (`ratio = float64(nTarget)` instead of nTarget/rec.Count) that +// guards against a division by zero producing NaN/Inf in every scaled money +// field. +func TestApplyTargetCoverage_CountZeroNoNaNOrInf(t *testing.T) { + t.Parallel() + monthly := 20.0 + rec := mkRI(0, 10, 0) // Count=0, avg=10, existing=0 + rec.RecurringMonthlyCost = &monthly + + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil, nil) + + require.Len(t, out, 1) + got := out[0] + for name, v := range map[string]float64{ + "CommitmentCost": got.CommitmentCost, + "OnDemandCost": got.OnDemandCost, + "EstimatedSavings": got.EstimatedSavings, + "RecurringMonthlyCost": *got.RecurringMonthlyCost, + "ProjectedUtilization": got.ProjectedUtilization, + "ProjectedCoverage": got.ProjectedCoverage, + } { + assert.False(t, math.IsNaN(v), "%s is NaN", name) + assert.False(t, math.IsInf(v, 0), "%s is Inf", name) + } +} + +func TestApplyTargetCoverage_SPEdgePassthroughs(t *testing.T) { + t.Parallel() + + t.Run("RecommendedUtilization <= 0 passes through unchanged", func(t *testing.T) { + t.Parallel() + rec := mkSP(0, 2.0) + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil, nil) + require.Len(t, out, 1) + assert.Equal(t, rec, out[0]) + }) + + t.Run("HourlyCommitment <= 0 passes through unchanged", func(t *testing.T) { + t.Parallel() + rec := mkSP(50, 0) + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil, nil) + require.Len(t, out, 1) + assert.Equal(t, rec, out[0]) + }) + + t.Run("wrong-type Details passes through with warning, ProjectedUtilization stays zero", func(t *testing.T) { + t.Parallel() + var logs []string + rec := mkSP(50, 2.0) + rec.Details = common.ComputeDetails{Platform: "Linux/UNIX"} + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, captureLogf(&logs), nil) + require.Len(t, out, 1) + assert.Equal(t, rec, out[0]) + assert.Equal(t, 0.0, out[0].ProjectedUtilization, "scaling failed, so projection must not be set") + assert.Len(t, logs, 1) + }) + + t.Run("typed-nil Details passes through with warning, ProjectedUtilization stays zero", func(t *testing.T) { + t.Parallel() + var logs []string + rec := mkSP(50, 2.0) + rec.Details = (*common.SavingsPlanDetails)(nil) + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, captureLogf(&logs), nil) + require.Len(t, out, 1) + assert.Equal(t, 0.0, out[0].ProjectedUtilization) + assert.Len(t, logs, 1) + }) +} + +// TestApplyTargetCoverage_UnsupportedType_WarnsOncePerType covers a slice +// containing several recs of the same unsupported CommitmentType: the +// warning must fire exactly once per distinct type across the whole slice, +// not once per rec, and every rec passes through unchanged regardless. +func TestApplyTargetCoverage_UnsupportedType_WarnsOncePerType(t *testing.T) { + t.Parallel() + var logs []string + recs := []common.Recommendation{ + {Service: common.ServiceCompute, CommitmentType: common.CommitmentCUD, Count: 1}, + {Service: common.ServiceCompute, CommitmentType: common.CommitmentCUD, Count: 2}, + {Service: common.ServiceCompute, CommitmentType: common.CommitmentReservedCapacity, Count: 3}, + } + + out := ApplyTargetCoverage(recs, 80, captureLogf(&logs), nil) + + require.Len(t, out, 3) + assert.Equal(t, recs, out, "unsupported types pass through unchanged") + + cudWarnings, capacityWarnings := 0, 0 + for _, l := range logs { + switch { + case strings.Contains(l, string(common.CommitmentCUD)): + cudWarnings++ + case strings.Contains(l, string(common.CommitmentReservedCapacity)): + capacityWarnings++ + } + } + assert.Equal(t, 1, cudWarnings, "CommitmentCUD warns exactly once despite 2 recs") + assert.Equal(t, 1, capacityWarnings, "CommitmentReservedCapacity warns exactly once") +} + +func TestApplyTargetCoverage_TargetPctOutOfRange_PassesThroughWithWarning(t *testing.T) { + t.Parallel() + recs := []common.Recommendation{mkRI(5, 8, 0)} + + for _, targetPct := range []float64{0, -1, 101} { + var logs []string + out := ApplyTargetCoverage(recs, targetPct, captureLogf(&logs), nil) + assert.Equal(t, recs, out, "targetPct=%.0f", targetPct) + assert.Len(t, logs, 1, "targetPct=%.0f", targetPct) + } +} + +func TestApplyTargetCoverage_NilLogfSafe(t *testing.T) { + t.Parallel() + recs := []common.Recommendation{ + mkRI(5, 8, 90), // gapPct <= 0 -> INFO log + mkRI(1, 0.5, 0), // sized to zero -> INFO log + mkRI(5, 0, 0), // no signal, no log + mkSP(50, 0), // no signal, no log + {Service: common.ServiceCompute, CommitmentType: common.CommitmentCUD}, // unsupported -> WARNING log + } + assert.NotPanics(t, func() { + ApplyTargetCoverage(recs, 80, nil, nil) + }) + assert.NotPanics(t, func() { + ApplyTargetCoverage(recs, 0, nil, nil) // out-of-range -> WARNING log + }) +}