diff --git a/cmd/helpers.go b/cmd/helpers.go index 22a7970c0..4d23354e8 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -5,14 +5,12 @@ import ( "context" "fmt" "log" - "math" "os" "strings" "sync" - "time" "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" @@ -20,11 +18,11 @@ import ( // Constants for purchase processing. const ( - // DefaultDuplicateCheckLookbackHours is the default lookback period for checking recent purchases. - DefaultDuplicateCheckLookbackHours = 24 - // PurchaseDelaySeconds is the delay between consecutive purchases to avoid rate limiting. PurchaseDelaySeconds = 2 + + // DefaultDuplicateCheckLookbackHours is re-exported from pkg/recfilter. + DefaultDuplicateCheckLookbackHours = recfilter.DefaultDuplicateCheckLookbackHours ) // AppLogger is a simple logger for application output. @@ -114,366 +112,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. @@ -572,216 +227,22 @@ func ConfirmPurchase(totalInstances int, totalSavings float64, skipConfirmation return response == "yes" || response == "y" } -// CheckAuditLogWritable opens the audit log file in append mode to verify it is writable. -// Returns an error if the path cannot be opened for writing. -func CheckAuditLogWritable(path string) error { - f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600) // #nosec G304 -- audit log path is operator-configured; value is not reachable from user input - if err != nil { - return fmt.Errorf("audit log %q not writable: %w", path, err) - } - return f.Close() -} +// CheckAuditLogWritable reports whether the audit log at path can be appended to. +// Thin wrapper over common.CheckAuditLogWritable; kept so cmd's existing call sites +// and tests are unchanged. +func CheckAuditLogWritable(path string) error { return common.CheckAuditLogWritable(path) } -// DuplicateChecker checks for existing commitments to avoid duplicates. -type DuplicateChecker struct { - LookbackHours int // How many hours to look back for recent purchases -} +// DuplicateChecker is re-exported from pkg/recfilter so cmd's existing call +// sites and tests are unchanged. +type DuplicateChecker = recfilter.DuplicateChecker -// NewDuplicateChecker creates a new duplicate checker. Pass 0 to use the default lookback period. +// NewDuplicateChecker creates a new duplicate checker. Pass 0 to use the +// default lookback period. Logf is wired to log.Printf so the CLI's +// decision trail keeps going to stderr exactly as it does today. func NewDuplicateChecker(hours int) *DuplicateChecker { - if hours <= 0 { - hours = DefaultDuplicateCheckLookbackHours - } - return &DuplicateChecker{ - LookbackHours: hours, - } -} - -// AdjustRecommendationsForExisting adjusts recommendations based on existing commitments -// This checks for recently purchased RIs (within LookbackHours) to avoid duplicate purchases. -// Note: This is designed to prevent re-purchasing something you just bought, not to prevent -// purchasing RIs in other accounts that happen to have the same characteristics. -func (d *DuplicateChecker) AdjustRecommendationsForExisting(ctx context.Context, recs []common.Recommendation, client provider.ServiceClient) (passed, filtered []common.Recommendation, err error) { - existing, err := client.GetExistingCommitments(ctx) - if err != nil { - return recs, nil, err - } - - log.Printf(" [DuplicateChecker] Found %d total existing commitments", len(existing)) - - recentExisting := d.filterRecentCommitments(existing) - log.Printf(" [DuplicateChecker] Found %d recent commitments (purchased in last %d hours)", len(recentExisting), d.LookbackHours) - - if len(recentExisting) == 0 { - return recs, nil, nil - } - - existingMap := buildExistingCommitmentsMap(recentExisting) - log.Printf(" [DuplicateChecker] Existing map has %d unique keys", len(existingMap)) - - passed, filtered = adjustRecommendationsAgainstExisting(recs, existingMap) - - if len(filtered) > 0 { - log.Printf(" [DuplicateChecker] Result: %d recommendations kept out of %d (avoided %d duplicates)", - len(passed), len(recs), len(filtered)) - } - return passed, filtered, nil -} - -// filterRecentCommitments filters commitments to only recent purchases within the lookback window. -func (d *DuplicateChecker) filterRecentCommitments(existing []common.Commitment) []common.Commitment { - cutoffTime := time.Now().Add(-time.Duration(d.LookbackHours) * time.Hour) - recentExisting := make([]common.Commitment, 0) - - for _rvc := range existing { - c := existing[_rvc] - if isRecentActiveCommitment(c, cutoffTime) { - recentExisting = append(recentExisting, c) - } - } - - return recentExisting -} - -// isRecentActiveCommitment checks if a commitment is active and purchased after the cutoff time. -func isRecentActiveCommitment(c common.Commitment, cutoffTime time.Time) bool { - return (c.State == "active" || c.State == "payment-pending") && c.StartDate.After(cutoffTime) -} - -// buildExistingCommitmentsMap builds a map of commitments by resource type, region, and engine. -func buildExistingCommitmentsMap(commitments []common.Commitment) map[string]int { - existingMap := make(map[string]int) - - for _rvc := range commitments { - c := commitments[_rvc] - normalizedEngine := normalizeEngineName(c.Engine) - key := fmt.Sprintf("%s|%s|%s", c.ResourceType, c.Region, normalizedEngine) - existingMap[key] += c.Count - log.Printf(" [DuplicateChecker] Recent RI: key=%s count=%d startDate=%s (raw engine=%s)", - key, c.Count, c.StartDate.Format("2006-01-02 15:04:05"), c.Engine) - } - - return existingMap -} - -// adjustRecommendationsAgainstExisting adjusts recommendations based on existing commitments. -// Returns (passed, filtered) where filtered contains recs whose count was reduced to zero. -func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existingMap map[string]int) (passed, filtered []common.Recommendation) { - passed = make([]common.Recommendation, 0, len(recs)) - filtered = make([]common.Recommendation, 0) - - for _rvc := range recs { - rec := recs[_rvc] - adjusted := adjustSingleRecommendation(rec, existingMap) - if adjusted.Count > 0 { - passed = append(passed, adjusted) - } else { - filtered = append(filtered, rec) - } - } - - return passed, filtered -} - -// adjustSingleRecommendation adjusts a single recommendation based on existing commitments. -func adjustSingleRecommendation(rec common.Recommendation, existingMap map[string]int) common.Recommendation { - engine := getEngineFromRecommendation(rec) - key := fmt.Sprintf("%s|%s|%s", rec.ResourceType, rec.Region, engine) - existingCount := existingMap[key] - - if existingCount >= rec.Count { - // All of this recommendation is covered by recent RIs. - // Return a zero-value Recommendation (Count=0) as a sentinel; the caller - // (adjustRecommendationsAgainstExisting) filters out recommendations with Count <= 0. - log.Printf(" [DuplicateChecker] SKIP %s: recent %d >= recommended %d", key, existingCount, rec.Count) - existingMap[key] -= rec.Count - return common.Recommendation{Count: 0} - } - - // Partial or no coverage by recent RIs - adjusted := rec - if existingCount > 0 { - adjusted.Count = rec.Count - existingCount - existingMap[key] = 0 - log.Printf(" [DuplicateChecker] PARTIAL %s: adjusted count from %d to %d", key, rec.Count, adjusted.Count) - } - - return adjusted -} - -// getEngineFromRecommendation extracts the engine from recommendation details. -// DatabaseDetails/CacheDetails are always pointers (every producer -- AWS, -// Azure, the CSV loader, and the JSON codec -- constructs them that way); see -// pkg/common/service_details_codec.go's package doc for the pointer -// invariant. The nil-pointer guards are not dead code: `rec.Details == nil` -// only catches an untyped nil, so a typed nil (a (*common.DatabaseDetails)(nil) -// stored in the interface) reaches the switch and would panic on the field -// read. Returning blank matches the sibling helpers (extractDeployment and -// extractEngine in multi_service_csv.go, rdsEngineDeploymentFromRec in the -// AWS coverage package), which already guard the same way. -func getEngineFromRecommendation(rec common.Recommendation) string { - if rec.Details == nil { - return "" - } - var engine string - switch details := rec.Details.(type) { - case *common.DatabaseDetails: - if details == nil { - return "" - } - engine = details.Engine - case *common.CacheDetails: - if details == nil { - return "" - } - engine = details.Engine - default: - return "" - } - return normalizeEngineName(engine) -} - -// engineNameMap maps database engine names to a consistent normalized format. -// AWS RIs use: "aurora-postgresql", "aurora-mysql", "mysql", "postgres" -// Cost Explorer uses: "Aurora PostgreSQL", "Aurora MySQL", "MySQL", "PostgreSQL". -var engineNameMap = map[string]string{ - // Cost Explorer format -> normalized - "Aurora PostgreSQL": "aurora-postgresql", - "Aurora MySQL": "aurora-mysql", - "MySQL": "mysql", - "PostgreSQL": "postgresql", - "MariaDB": "mariadb", - "Oracle": "oracle", - "SQL Server": "sqlserver", - // Already normalized (from AWS RIs) - "aurora-postgresql": "aurora-postgresql", - "aurora-mysql": "aurora-mysql", - "mysql": "mysql", - "postgresql": "postgresql", - "postgres": "postgresql", - "mariadb": "mariadb", - "oracle-se": "oracle", - "oracle-se1": "oracle", - "oracle-se2": "oracle", - "oracle-ee": "oracle", - "sqlserver-se": "sqlserver", - "sqlserver-ee": "sqlserver", - "sqlserver-ex": "sqlserver", - "sqlserver-web": "sqlserver", -} - -// normalizeEngineName normalizes database engine names to a consistent format. -func normalizeEngineName(engine string) string { - if normalized, ok := engineNameMap[engine]; ok { - return normalized - } - // Return lowercase as fallback - return strings.ToLower(engine) -} - -// AdjustRecommendationsForExistingRIs is an alias for AdjustRecommendationsForExisting. -func (d *DuplicateChecker) AdjustRecommendationsForExistingRIs(ctx context.Context, recs []common.Recommendation, client provider.ServiceClient) (passed, filtered []common.Recommendation, err error) { - return d.AdjustRecommendationsForExisting(ctx, recs, client) + d := recfilter.NewDuplicateChecker(hours) + d.Logf = log.Printf + return d } // GetRecommendationDescription returns a human-readable description. diff --git a/cmd/helpers_test.go b/cmd/helpers_test.go index 82f9de211..357d01292 100644 --- a/cmd/helpers_test.go +++ b/cmd/helpers_test.go @@ -583,7 +583,7 @@ func TestNormalizeEngineName(t *testing.T) { for _, tt := range tests { t.Run(tt.input, func(t *testing.T) { - result := normalizeEngineName(tt.input) + result := common.NormalizeEngineName(tt.input) assert.Equal(t, tt.expected, result) }) } @@ -649,7 +649,7 @@ func TestGetEngineFromRecommendation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := getEngineFromRecommendation(tt.rec) + result := common.EngineFromDetails(tt.rec.Details) assert.Equal(t, tt.expected, result) }) } diff --git a/cmd/multi_service_filters.go b/cmd/multi_service_filters.go index 2d652baf4..66eb43e7d 100644 --- a/cmd/multi_service_filters.go +++ b/cmd/multi_service_filters.go @@ -1,34 +1,39 @@ package main import ( - "fmt" "log" - "slices" "strings" "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/pkg/recfilter" awsprovider "github.com/LeanerCloud/CUDly/providers/aws" ) +// filtersFromConfig maps the CLI Config's dimension-filter and min-pool-size +// fields onto a recfilter.Filters value. Account filtering stays cmd-only +// (see shouldIncludeAccount) so it is not part of recfilter.Filters. +func filtersFromConfig(cfg *Config) recfilter.Filters { + return recfilter.Filters{ + IncludeRegions: cfg.IncludeRegions, + ExcludeRegions: cfg.ExcludeRegions, + IncludeInstanceTypes: cfg.IncludeInstanceTypes, + ExcludeInstanceTypes: cfg.ExcludeInstanceTypes, + IncludeEngines: cfg.IncludeEngines, + ExcludeEngines: cfg.ExcludeEngines, + MinPoolSize: cfg.MinPoolSize, + } +} + // applyFilters applies region, instance type, engine, and engine version filters to recommendations. // currentRegion is the region being processed in the current loop iteration; if non-empty, only // recommendations for that region are included. // drops accumulates per-reason drop counts for the end-of-run summary; pass nil to skip tracking. func applyFilters(recs []common.Recommendation, cfg *Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string, drops *common.DropSummary) []common.Recommendation { - var filtered []common.Recommendation - var poolDropCount int - var poolDropInstances float64 + survivors := filtersFromConfig(cfg).ApplyMinPoolSize(recs, log.Printf, drops) - for i := range recs { - if cfg.MinPoolSize > 0 && !shouldIncludePoolSize(&recs[i], cfg) { - poolDropInstances += recs[i].AverageInstancesUsedPerHour - label := fmt.Sprintf("%s/%s/%s", recs[i].Service, recs[i].Region, recs[i].ResourceType) - log.Printf("INFO: --min-pool-size=%.1f dropped %s (avg=%.2f < threshold)", cfg.MinPoolSize, label, recs[i].AverageInstancesUsedPerHour) - poolDropCount++ - drops.Add(common.DropMinPoolSize, 1) - continue - } - adjusted, include, dropReason := processRecommendation(&recs[i], cfg, instanceVersions, versionInfo, currentRegion) + var filtered []common.Recommendation + for i := range survivors { + adjusted, include, dropReason := processRecommendation(&survivors[i], cfg, instanceVersions, versionInfo, currentRegion) if include { filtered = append(filtered, adjusted) } else if dropReason != "" { @@ -36,10 +41,6 @@ func applyFilters(recs []common.Recommendation, cfg *Config, instanceVersions ma } } - if poolDropCount > 0 { - log.Printf("INFO: --min-pool-size dropped %d recommendation(s) (%.2f avg instances/hr total)", poolDropCount, poolDropInstances) - } - return filtered } @@ -86,6 +87,11 @@ func processRecommendation(rec *common.Recommendation, cfg *Config, instanceVers // each function's cyclomatic complexity under the gocyclo limit; the // dimension filters here are pure functions of rec + cfg with no side // effects. Pool-size filtering is handled with logging in applyFilters. +// +// Region and account stay here rather than moving into recfilter: the +// region-agnostic handling (#1881) needs providers/aws, which the pkg module +// cannot import, and account filtering is name-substring matching backed by +// AccountAliasCache. Only the instance-type and engine checks are portable. func passesDimensionFilters(rec *common.Recommendation, cfg *Config) bool { if !shouldIncludeRecommendationRegion(rec, cfg) { return false @@ -96,31 +102,14 @@ func passesDimensionFilters(rec *common.Recommendation, cfg *Config) bool { if !shouldIncludeEngine(rec, cfg) { return false } - if !shouldIncludeAccount(rec.AccountName, cfg) { - return false - } - return true + return shouldIncludeAccount(rec.AccountName, cfg) } -// shouldIncludePoolSize filters out RI recommendations for pools whose -// AverageInstancesUsedPerHour is below cfg.MinPoolSize. The purpose is to -// drop tiny pools where integer-arithmetic sizing forces 100% coverage -// regardless of --target-coverage (e.g. avg=1 with target=80% -> floor(0.8)=0 -// drops, ceil(0.8)=1 over-covers). Setting --min-pool-size=2 keeps pools -// where target can be meaningfully approximated. -// -// Pass-through cases: filter disabled (MinPoolSize<=0), or rec has no -// per-hour signal (avg<=0 -- SPs and recs CE didn't return usage for). -// Those pools aren't sized via the per-hour formula so the filter doesn't -// apply to them. +// shouldIncludePoolSize checks if a recommendation's pool size meets cfg.MinPoolSize. +// Thin wrapper over recfilter.Filters.IncludesPoolSize; kept so cmd's existing call sites +// and tests are unchanged. func shouldIncludePoolSize(rec *common.Recommendation, cfg *Config) bool { - if cfg.MinPoolSize <= 0 { - return true - } - if rec.AverageInstancesUsedPerHour <= 0 { - return true - } - return rec.AverageInstancesUsedPerHour >= cfg.MinPoolSize + return filtersFromConfig(cfg).IncludesPoolSize(rec) } // shouldIncludeRecommendationRegion applies the region filters to a whole @@ -143,71 +132,24 @@ func shouldIncludeRecommendationRegion(rec *common.Recommendation, cfg *Config) } // shouldIncludeRegion checks if a region should be included based on filters. +// Thin wrapper over recfilter.Filters.IncludesRegion; kept so cmd's existing call sites +// and tests are unchanged. func shouldIncludeRegion(region string, cfg *Config) bool { - // If include list is specified, region must be in it. - if len(cfg.IncludeRegions) > 0 && !slices.Contains(cfg.IncludeRegions, region) { - return false - } - - // If exclude list is specified, region must not be in it. - if slices.Contains(cfg.ExcludeRegions, region) { - return false - } - - return true + return filtersFromConfig(cfg).IncludesRegion(region) } // shouldIncludeInstanceType checks if an instance type should be included based on filters. +// Thin wrapper over recfilter.Filters.IncludesInstanceType; kept so cmd's existing call sites +// and tests are unchanged. func shouldIncludeInstanceType(instanceType string, cfg *Config) bool { - // If include list is specified, instance type must be in it. - if len(cfg.IncludeInstanceTypes) > 0 && !slices.Contains(cfg.IncludeInstanceTypes, instanceType) { - return false - } - - // If exclude list is specified, instance type must not be in it. - if slices.Contains(cfg.ExcludeInstanceTypes, instanceType) { - return false - } - - return true + return filtersFromConfig(cfg).IncludesInstanceType(instanceType) } // shouldIncludeEngine checks if a recommendation should be included based on engine filters. +// Thin wrapper over recfilter.Filters.IncludesEngine; kept so cmd's existing call sites +// and tests are unchanged. func shouldIncludeEngine(rec *common.Recommendation, cfg *Config) bool { - // Extract engine from recommendation. - engine := getEngineFromRecommendation(*rec) - if engine == "" { - // If no engine info, include by default unless there's an include list. - return len(cfg.IncludeEngines) == 0 - } - - // Normalize engine name to lowercase for comparison. - engine = strings.ToLower(engine) - - // If include list is specified, engine must be in it. - if len(cfg.IncludeEngines) > 0 { - found := false - for _, e := range cfg.IncludeEngines { - if strings.EqualFold(e, engine) { - found = true - break - } - } - if !found { - return false - } - } - - // If exclude list is specified, engine must not be in it. - if len(cfg.ExcludeEngines) > 0 { - for _, e := range cfg.ExcludeEngines { - if strings.EqualFold(e, engine) { - return false - } - } - } - - return true + return filtersFromConfig(cfg).IncludesEngine(rec) } // shouldIncludeAccount checks if an account should be included based on filters. diff --git a/cmd/multi_service_filters_test.go b/cmd/multi_service_filters_test.go index f742435ec..58a666130 100644 --- a/cmd/multi_service_filters_test.go +++ b/cmd/multi_service_filters_test.go @@ -1,11 +1,14 @@ package main import ( + "fmt" + "log" "testing" "time" "github.com/LeanerCloud/CUDly/pkg/common" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestApplyFilters(t *testing.T) { @@ -629,3 +632,124 @@ func TestApplyFilters_SavingsPlansRegionFilters(t *testing.T) { }) } } + +// applyFiltersPreExtraction is the pre-refactor applyFilters implementation, +// kept verbatim (from origin/main, before the recfilter.Filters.ApplyMinPoolSize +// extraction) as a differential oracle. It applies the --min-pool-size check +// inline, in the same loop and same order as processRecommendation, rather +// than as a separate pass over the full slice. +func applyFiltersPreExtraction(recs []common.Recommendation, cfg *Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string, drops *common.DropSummary) []common.Recommendation { + var filtered []common.Recommendation + var poolDropCount int + var poolDropInstances float64 + + for i := range recs { + if cfg.MinPoolSize > 0 && !shouldIncludePoolSize(&recs[i], cfg) { + poolDropInstances += recs[i].AverageInstancesUsedPerHour + label := fmt.Sprintf("%s/%s/%s", recs[i].Service, recs[i].Region, recs[i].ResourceType) + log.Printf("INFO: --min-pool-size=%.1f dropped %s (avg=%.2f < threshold)", cfg.MinPoolSize, label, recs[i].AverageInstancesUsedPerHour) + poolDropCount++ + drops.Add(common.DropMinPoolSize, 1) + continue + } + adjusted, include, dropReason := processRecommendation(&recs[i], cfg, instanceVersions, versionInfo, currentRegion) + if include { + filtered = append(filtered, adjusted) + } else if dropReason != "" { + drops.Add(dropReason, 1) + } + } + + if poolDropCount > 0 { + log.Printf("INFO: --min-pool-size dropped %d recommendation(s) (%.2f avg instances/hr total)", poolDropCount, poolDropInstances) + } + + return filtered +} + +// TestApplyFilters_MinPoolSizeMultiRegionMatchesPreExtractionBehaviour is a +// differential regression test against a reviewer's claim that extracting +// the --min-pool-size check into recfilter.Filters.ApplyMinPoolSize changed +// its ordering relative to the currentRegion guard in processRecommendation +// (inflating common.DropMinPoolSize in multi-region runs). It runs the +// current applyFilters and the pre-extraction oracle above side by side, +// once per region, over the same multi-region recommendation set, and +// asserts both the survivors and the drop accounting match exactly. +func TestApplyFilters_MinPoolSizeMultiRegionMatchesPreExtractionBehaviour(t *testing.T) { + origCfg := toolCfg + defer func() { toolCfg = origCfg }() + + toolCfg = Config{MinPoolSize: 2.0} + + regions := []string{"us-east-1", "eu-west-1", "ap-southeast-1"} + const distinctBelowThreshold = 3 // one below-threshold rec per region below + + // Fresh copies per call: applyFilters mutates nothing in place today, but + // the oracle and the refactored code must each see their own slice so a + // hypothetical future in-place adjustment on one side can't leak into the + // other's input and mask a real divergence. + makeRecs := func() []common.Recommendation { + return []common.Recommendation{ + {Region: "us-east-1", ResourceType: "db.t3.micro", Count: 3, AverageInstancesUsedPerHour: 5.0}, + {Region: "us-east-1", ResourceType: "db.t3.small", Count: 3, AverageInstancesUsedPerHour: 1.0}, // below threshold + {Region: "eu-west-1", ResourceType: "db.t3.micro", Count: 3, AverageInstancesUsedPerHour: 5.0}, + {Region: "eu-west-1", ResourceType: "db.t3.small", Count: 3, AverageInstancesUsedPerHour: 1.0}, // below threshold + {Region: "ap-southeast-1", ResourceType: "db.t3.micro", Count: 3, AverageInstancesUsedPerHour: 5.0}, + {Region: "ap-southeast-1", ResourceType: "db.t3.small", Count: 3, AverageInstancesUsedPerHour: 1.0}, // below threshold + {Region: "us-east-1", ResourceType: "db.t3.large", Count: 3, AverageInstancesUsedPerHour: 0}, // no-signal passthrough + { + Service: common.ServiceSavingsPlansCompute, + ResourceType: "ec2-instance", + Count: 10, + AverageInstancesUsedPerHour: 5.0, // account-level: bypasses the currentRegion guard + }, + } + } + + var totalDropMinPoolSizeNew, totalDropMinPoolSizeOld int + + for _, region := range regions { + t.Run(region, func(t *testing.T) { + dropsNew := common.NewDropSummary() + dropsOld := common.NewDropSummary() + + resultNew := applyFilters(makeRecs(), &toolCfg, make(map[string][]InstanceEngineVersion), make(map[string]MajorEngineVersionInfo), region, dropsNew) + resultOld := applyFiltersPreExtraction(makeRecs(), &toolCfg, make(map[string][]InstanceEngineVersion), make(map[string]MajorEngineVersionInfo), region, dropsOld) + + assert.Equal(t, resultOld, resultNew, + "region %s: refactored applyFilters diverged from the pre-extraction oracle -- the ApplyMinPoolSize extraction changed observable CLI filtering behavior", region) + assert.Equal(t, dropsOld.FormatOneLine(), dropsNew.FormatOneLine(), + "region %s: drop summaries diverged between pre- and post-extraction implementations", region) + assert.Equal(t, dropsOld.Total(), dropsNew.Total(), + "region %s: total drop counts diverged between pre- and post-extraction implementations", region) + + // The fixture is built so --min-pool-size is the ONLY drop reason + // either implementation can record, which is what lets the summed + // Total() below stand in for the min-pool count specifically. + // Asserting the exact per-pass count keeps that guarantee honest: + // if some other filter started dropping rows, Total() would exceed + // the below-threshold count and this would fail. + require.Contains(t, dropsNew.FormatOneLine(), common.DropMinPoolSize, + "region %s: fixture must exercise the --min-pool-size drop path", region) + require.Equal(t, distinctBelowThreshold, dropsNew.Total(), + "region %s: --min-pool-size must be the only drop reason this fixture records", region) + + totalDropMinPoolSizeNew += dropsNew.Total() + totalDropMinPoolSizeOld += dropsOld.Total() + }) + } + + require.Equal(t, totalDropMinPoolSizeOld, totalDropMinPoolSizeNew, + "old and new implementations must agree on the summed multi-region --min-pool-size drop count") + + // Pre-existing behavior, identical in both implementations (not introduced + // by the ApplyMinPoolSize extraction): each per-region call re-scans the + // FULL recommendation set passed to it, not a per-region subset, so a + // below-threshold recommendation from one region is re-counted as dropped + // during every other region's pass too. Summed across N region passes this + // is distinctBelowThreshold * N, not distinctBelowThreshold. This assertion + // pins today's (inflated) value rather than an aspirational deduplicated + // one -- see the test's summary report for the actual vs. distinct counts. + assert.Equal(t, distinctBelowThreshold*len(regions), totalDropMinPoolSizeNew, + "summed multi-region --min-pool-size drop count should match today's pre-existing inflation factor") +} diff --git a/pkg/common/audit.go b/pkg/common/audit.go index 457914bd7..edecf9308 100644 --- a/pkg/common/audit.go +++ b/pkg/common/audit.go @@ -37,6 +37,16 @@ func WriteAuditRecord(record AuditRecord, path string) error { return nil } +// CheckAuditLogWritable opens the audit log file in append mode to verify it is writable. +// Returns an error if the path cannot be opened for writing. +func CheckAuditLogWritable(path string) error { + f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) // #nosec G302,G304 -- 0644 intentionally matches WriteAuditRecord; path is operator-configured. + if err != nil { + return fmt.Errorf("audit log %q not writable: %w", path, err) + } + return f.Close() +} + // NewAuditRecord constructs an AuditRecord from a Recommendation and a PurchaseResult. // status must be one of: "success", "error", "skipped" (dry-run), "skipped_covered" (idempotency). // source is the CUDly surface that triggered the run — copied into the JSONL so CLI diff --git a/pkg/common/audit_permissions_unix_test.go b/pkg/common/audit_permissions_unix_test.go new file mode 100644 index 000000000..272242483 --- /dev/null +++ b/pkg/common/audit_permissions_unix_test.go @@ -0,0 +1,44 @@ +//go:build unix + +package common + +import ( + "os" + "path/filepath" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCheckAuditLogWritable_CreatesMissingAuditLogReadableByDownstreamTools(t *testing.T) { + old := syscall.Umask(0) + t.Cleanup(func() { syscall.Umask(old) }) + + path := filepath.Join(t.TempDir(), "audit.jsonl") + + require.NoError(t, CheckAuditLogWritable(path)) + + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o644), info.Mode().Perm()) +} + +func TestCheckAuditLogWritable_DoesNotChmodExistingAuditLog(t *testing.T) { + old := syscall.Umask(0) + t.Cleanup(func() { syscall.Umask(old) }) + + path := filepath.Join(t.TempDir(), "audit.jsonl") + require.NoError(t, os.WriteFile(path, []byte("line1\n"), 0o600)) + + require.NoError(t, CheckAuditLogWritable(path)) + + data, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "line1\n", string(data)) + + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) +} diff --git a/pkg/common/audit_test.go b/pkg/common/audit_test.go index ae9eaf4df..3a13950d5 100644 --- a/pkg/common/audit_test.go +++ b/pkg/common/audit_test.go @@ -141,6 +141,41 @@ func TestNewAuditRecord_Fields(t *testing.T) { assert.WithinDuration(t, time.Now().UTC(), ar.Timestamp, 5*time.Second) } +func TestCheckAuditLogWritable_WritablePath(t *testing.T) { + t.Parallel() + dir := t.TempDir() + path := filepath.Join(dir, "audit.jsonl") + + assert.NoError(t, CheckAuditLogWritable(path)) +} + +// The unwritable path is a child of a regular file rather than a 0555 +// directory: a root process ignores the permission bits, so a chmod-based +// case would pass vacuously in root-based CI. +func TestCheckAuditLogWritable_UnwritablePath(t *testing.T) { + t.Parallel() + notADir := filepath.Join(t.TempDir(), "regular-file") + require.NoError(t, os.WriteFile(notADir, []byte("x"), 0600)) + + path := filepath.Join(notADir, "audit.jsonl") + err := CheckAuditLogWritable(path) + assert.Error(t, err) + assert.Contains(t, err.Error(), path) +} + +func TestCheckAuditLogWritable_DoesNotTruncateExistingContent(t *testing.T) { + t.Parallel() + dir := t.TempDir() + path := filepath.Join(dir, "audit.jsonl") + require.NoError(t, os.WriteFile(path, []byte("line1\n"), 0644)) + + require.NoError(t, CheckAuditLogWritable(path)) + + data, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "line1\n", string(data)) +} + func TestTermMonths(t *testing.T) { t.Parallel() cases := []struct { diff --git a/pkg/common/deployment.go b/pkg/common/deployment.go new file mode 100644 index 000000000..577f63067 --- /dev/null +++ b/pkg/common/deployment.go @@ -0,0 +1,42 @@ +package common + +import "strings" + +// NormalizeDeploymentName canonicalises an RDS deployment-option string +// (e.g. Cost Explorer's "Multi-AZ" or the parser's "multi-az") to a single +// lowercase, no-separator form. Mirrors +// providers/aws/recommendations/coverage.go's normaliseDeployment, which +// lives in the root module and can't be imported here (pkg is a separate +// Go module); keep the two in sync by hand if the vocabulary changes. +// +// An empty input stays "" rather than becoming a synthetic bucket, so +// non-RDS commitments/recommendations (which never populate a deployment) +// collapse onto the same key instead of colliding with a real +// single-az/multi-az bucket. +func NormalizeDeploymentName(deployment string) string { + s := strings.ToLower(deployment) + s = strings.ReplaceAll(s, " ", "") + s = strings.ReplaceAll(s, "-", "") + s = strings.ReplaceAll(s, "_", "") + s = strings.ReplaceAll(s, "(", "") + s = strings.ReplaceAll(s, ")", "") + return s +} + +// DeploymentFromDetails extracts the RDS deployment option (AZConfig) from +// recommendation details. Returns "" for non-database service types. +// +// The `d == nil` check after the type assertion is not redundant with the +// `details == nil` check above it: the first only catches an untyped nil, so +// an interface holding a typed nil pointer reaches the assertion, satisfies +// it, and would panic on the field read. Same guard as EngineFromDetails. +func DeploymentFromDetails(details ServiceDetails) string { + if details == nil { + return "" + } + d, ok := details.(*DatabaseDetails) + if !ok || d == nil { + return "" + } + return d.AZConfig +} diff --git a/pkg/common/deployment_test.go b/pkg/common/deployment_test.go new file mode 100644 index 000000000..53bc94d67 --- /dev/null +++ b/pkg/common/deployment_test.go @@ -0,0 +1,61 @@ +package common + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNormalizeDeploymentName(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + want string + }{ + {"Cost Explorer spelling", "Multi-AZ", "multiaz"}, + {"parser spelling", "multi-az", "multiaz"}, + {"single-az variants agree", "Single-AZ", "singleaz"}, + {"underscores", "multi_az", "multiaz"}, + {"spaces", "Multi AZ", "multiaz"}, + {"parentheses", "Multi-AZ (readable standby)", "multiazreadablestandby"}, + // Empty stays empty rather than becoming a synthetic bucket, so a + // non-RDS commitment and a non-RDS recommendation land on one key. + {"empty stays empty", "", ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, NormalizeDeploymentName(tt.input)) + }) + } +} + +// The two spellings the RDS path actually produces must collapse together; +// that agreement is what the duplicate-identity key depends on. +func TestNormalizeDeploymentName_SpellingsCollapse(t *testing.T) { + t.Parallel() + assert.Equal(t, NormalizeDeploymentName("Multi-AZ"), NormalizeDeploymentName("multi-az")) + assert.NotEqual(t, NormalizeDeploymentName("Multi-AZ"), NormalizeDeploymentName("Single-AZ")) +} + +func TestDeploymentFromDetails(t *testing.T) { + t.Parallel() + + assert.Equal(t, "multi-az", DeploymentFromDetails(&DatabaseDetails{AZConfig: "multi-az"})) + assert.Empty(t, DeploymentFromDetails(nil), "untyped nil details") + assert.Empty(t, DeploymentFromDetails(&CacheDetails{Engine: "redis"}), "non-database details") + assert.Empty(t, DeploymentFromDetails(&DatabaseDetails{}), "database details with no AZConfig") +} + +// A typed nil stored in the interface satisfies the type assertion, so +// without the type-specific nil guard this panics rather than returning "". +func TestDeploymentFromDetails_TypedNilDoesNotPanic(t *testing.T) { + t.Parallel() + var typedNil *DatabaseDetails + assert.NotPanics(t, func() { + assert.Empty(t, DeploymentFromDetails(typedNil)) + }) +} diff --git a/pkg/common/engine.go b/pkg/common/engine.go index b9c99ca80..5277267bb 100644 --- a/pkg/common/engine.go +++ b/pkg/common/engine.go @@ -7,20 +7,17 @@ import "strings" // Cost Explorer uses: "Aurora PostgreSQL", "Aurora MySQL", "MySQL", "PostgreSQL" var engineNameMap = map[string]string{ // Cost Explorer format -> normalized - "Aurora PostgreSQL": "aurora-postgresql", - "Aurora MySQL": "aurora-mysql", - "MySQL": "mysql", - "PostgreSQL": "postgresql", - "MariaDB": "mariadb", - "Oracle": "oracle", - "SQL Server": "sqlserver", + "aurora postgresql": "aurora-postgresql", + "aurora mysql": "aurora-mysql", + "mysql": "mysql", + "postgresql": "postgresql", + "mariadb": "mariadb", + "oracle": "oracle", + "sql server": "sqlserver", // Already normalized (from AWS RIs) "aurora-postgresql": "aurora-postgresql", "aurora-mysql": "aurora-mysql", - "mysql": "mysql", - "postgresql": "postgresql", "postgres": "postgresql", - "mariadb": "mariadb", "oracle-se": "oracle", "oracle-se1": "oracle", "oracle-se2": "oracle", @@ -34,10 +31,11 @@ var engineNameMap = map[string]string{ // NormalizeEngineName normalizes database engine names to a consistent format. // Returns lowercase of the input as a fallback when the engine is not recognized. func NormalizeEngineName(engine string) string { - if normalized, ok := engineNameMap[engine]; ok { - return normalized + normalized := strings.ToLower(engine) + if normalizedEngine, ok := engineNameMap[normalized]; ok { + return normalizedEngine } - return strings.ToLower(engine) + return normalized } // EngineFromDetails extracts and normalizes the engine name from diff --git a/pkg/common/engine_test.go b/pkg/common/engine_test.go index 19347424e..8efa76f49 100644 --- a/pkg/common/engine_test.go +++ b/pkg/common/engine_test.go @@ -6,6 +6,33 @@ import ( "github.com/stretchr/testify/assert" ) +func TestNormalizeEngineName_CaseInsensitiveRecognizedAliases(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + engine string + want string + }{ + {name: "uppercase aurora postgresql", engine: "AURORA POSTGRESQL", want: "aurora-postgresql"}, + {name: "title case aurora postgresql", engine: "Aurora PostgreSQL", want: "aurora-postgresql"}, + {name: "uppercase postgres alias", engine: "POSTGRES", want: "postgresql"}, + {name: "title case postgres alias", engine: "Postgres", want: "postgresql"}, + {name: "uppercase sql server", engine: "SQL SERVER", want: "sqlserver"}, + {name: "uppercase oracle ee", engine: "ORACLE-EE", want: "oracle"}, + {name: "uppercase sqlserver web", engine: "SQLSERVER-WEB", want: "sqlserver"}, + {name: "unknown engine preserves fallback shape", engine: " Custom Engine ", want: " custom engine "}, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, NormalizeEngineName(tt.engine)) + }) + } +} + // TestEngineFromDetails covers the pointer-only dispatch documented in // service_details_codec.go's package doc, plus the typed-nil guard. // diff --git a/pkg/common/matches_test.go b/pkg/common/matches_test.go index 954545775..0d61e27ac 100644 --- a/pkg/common/matches_test.go +++ b/pkg/common/matches_test.go @@ -70,6 +70,27 @@ func TestMatches_NormalizedEngine(t *testing.T) { assert.True(t, Matches(rec, c)) } +func TestMatches_UppercaseRecognizedCommitmentAlias(t *testing.T) { + t.Parallel() + + rec := Recommendation{ + Provider: ProviderAWS, + Region: "us-east-1", + Service: ServiceRDS, + ResourceType: "db.r5.large", + Details: &DatabaseDetails{Engine: "aurora-postgresql"}, + } + c := Commitment{ + Provider: ProviderAWS, + Region: "us-east-1", + Service: ServiceRDS, + ResourceType: "db.r5.large", + Engine: "AURORA POSTGRESQL", + } + + assert.True(t, Matches(rec, c)) +} + func TestMatches_NoDetails(t *testing.T) { t.Parallel() // Compute recommendations have no engine — both sides normalize to "" diff --git a/pkg/recfilter/dedupe.go b/pkg/recfilter/dedupe.go new file mode 100644 index 000000000..14fb175c5 --- /dev/null +++ b/pkg/recfilter/dedupe.go @@ -0,0 +1,163 @@ +package recfilter + +import ( + "context" + "fmt" + "time" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/pkg/provider" +) + +// DefaultDuplicateCheckLookbackHours is the default lookback period for checking recent purchases. +const DefaultDuplicateCheckLookbackHours = 24 + +// DuplicateChecker checks for existing commitments to avoid duplicates. +type DuplicateChecker struct { + LookbackHours int // How many hours to look back for recent purchases + + // Logf receives the per-commitment decision trail. Nil is silent. + // recfilter never logs through a package-level logger: cmd's AppLogger + // writes to stdout, which the MCP server owns as its protocol transport. + Logf Logf +} + +// NewDuplicateChecker creates a new duplicate checker. Pass 0 to use the default lookback period. +func NewDuplicateChecker(hours int) *DuplicateChecker { + if hours <= 0 { + hours = DefaultDuplicateCheckLookbackHours + } + return &DuplicateChecker{ + LookbackHours: hours, + } +} + +// AdjustRecommendationsForExisting adjusts recommendations based on existing commitments +// This checks for recently purchased RIs (within LookbackHours) to avoid duplicate purchases. +// Note: This is designed to prevent re-purchasing something you just bought, not to prevent +// purchasing RIs in other accounts that happen to have the same characteristics. +func (d *DuplicateChecker) AdjustRecommendationsForExisting(ctx context.Context, recs []common.Recommendation, client provider.ServiceClient) (passed, filtered []common.Recommendation, err error) { + existing, err := client.GetExistingCommitments(ctx) + if err != nil { + return recs, nil, err + } + + d.Logf.printf(" [DuplicateChecker] Found %d total existing commitments", len(existing)) + + recentExisting := d.filterRecentCommitments(existing) + d.Logf.printf(" [DuplicateChecker] Found %d recent commitments (purchased in last %d hours)", len(recentExisting), d.LookbackHours) + + if len(recentExisting) == 0 { + return recs, nil, nil + } + + existingMap := buildExistingCommitmentsMap(recentExisting, d.Logf) + d.Logf.printf(" [DuplicateChecker] Existing map has %d unique keys", len(existingMap)) + + passed, filtered = adjustRecommendationsAgainstExisting(recs, existingMap, d.Logf) + + if len(filtered) > 0 { + d.Logf.printf(" [DuplicateChecker] Result: %d recommendations kept out of %d (avoided %d duplicates)", + len(passed), len(recs), len(filtered)) + } + return passed, filtered, nil +} + +// filterRecentCommitments filters commitments to only recent purchases within the lookback window. +func (d *DuplicateChecker) filterRecentCommitments(existing []common.Commitment) []common.Commitment { + cutoffTime := time.Now().Add(-time.Duration(d.LookbackHours) * time.Hour) + recentExisting := make([]common.Commitment, 0) + + for _rvc := range existing { + c := existing[_rvc] + if isRecentActiveCommitment(c, cutoffTime) { + recentExisting = append(recentExisting, c) + } + } + + return recentExisting +} + +// isRecentActiveCommitment checks if a commitment is active and purchased after the cutoff time. +func isRecentActiveCommitment(c common.Commitment, cutoffTime time.Time) bool { + return (c.State == "active" || c.State == "payment-pending") && c.StartDate.After(cutoffTime) +} + +// dedupeKey builds the duplicate-identity key shared by +// buildExistingCommitmentsMap and adjustSingleRecommendation. Both call +// sites MUST build the key through this function: a Single-AZ and a +// Multi-AZ RDS commitment/recommendation are priced and provisioned +// differently and do not cover each other's demand, so deployment has to +// be part of the identity or the two keys can drift and silently +// suppress (or fail to suppress) the wrong recommendation. +func dedupeKey(resourceType, region, engine, deployment string) string { + return fmt.Sprintf("%s|%s|%s|%s", resourceType, region, engine, deployment) +} + +// buildExistingCommitmentsMap builds a map of commitments by resource type, region, engine, and deployment. +func buildExistingCommitmentsMap(commitments []common.Commitment, logf Logf) map[string]int { + existingMap := make(map[string]int) + + for _rvc := range commitments { + c := commitments[_rvc] + normalizedEngine := common.NormalizeEngineName(c.Engine) + normalizedDeployment := common.NormalizeDeploymentName(c.Deployment) + key := dedupeKey(c.ResourceType, c.Region, normalizedEngine, normalizedDeployment) + existingMap[key] += c.Count + logf.printf(" [DuplicateChecker] Recent RI: key=%s count=%d startDate=%s (raw engine=%s)", + key, c.Count, c.StartDate.Format("2006-01-02 15:04:05"), c.Engine) + } + + return existingMap +} + +// adjustRecommendationsAgainstExisting adjusts recommendations based on existing commitments. +// Returns (passed, filtered) where filtered contains recs whose count was reduced to zero. +func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existingMap map[string]int, logf Logf) (passed, filtered []common.Recommendation) { + passed = make([]common.Recommendation, 0, len(recs)) + filtered = make([]common.Recommendation, 0) + + for _rvc := range recs { + rec := recs[_rvc] + adjusted := adjustSingleRecommendation(rec, existingMap, logf) + if adjusted.Count > 0 { + passed = append(passed, adjusted) + } else { + filtered = append(filtered, rec) + } + } + + return passed, filtered +} + +// adjustSingleRecommendation adjusts a single recommendation based on existing commitments. +func adjustSingleRecommendation(rec common.Recommendation, existingMap map[string]int, logf Logf) common.Recommendation { + engine := common.EngineFromDetails(rec.Details) + deployment := common.NormalizeDeploymentName(common.DeploymentFromDetails(rec.Details)) + key := dedupeKey(rec.ResourceType, rec.Region, engine, deployment) + existingCount := existingMap[key] + + if existingCount >= rec.Count { + // All of this recommendation is covered by recent RIs. + // Return a zero-value Recommendation (Count=0) as a sentinel; the caller + // (adjustRecommendationsAgainstExisting) filters out recommendations with Count <= 0. + logf.printf(" [DuplicateChecker] SKIP %s: recent %d >= recommended %d", key, existingCount, rec.Count) + existingMap[key] -= rec.Count + return common.Recommendation{Count: 0} + } + + // Partial or no coverage by recent RIs + adjusted := rec + if existingCount > 0 { + adjusted.Count = rec.Count - existingCount + existingMap[key] = 0 + logf.printf(" [DuplicateChecker] PARTIAL %s: adjusted count from %d to %d", key, rec.Count, adjusted.Count) + } + + return adjusted +} + +// AdjustRecommendationsForExistingRIs is an alias for AdjustRecommendationsForExisting. +func (d *DuplicateChecker) AdjustRecommendationsForExistingRIs(ctx context.Context, recs []common.Recommendation, client provider.ServiceClient) (passed, filtered []common.Recommendation, err error) { + return d.AdjustRecommendationsForExisting(ctx, recs, client) +} diff --git a/pkg/recfilter/dedupe_test.go b/pkg/recfilter/dedupe_test.go new file mode 100644 index 000000000..c9f97960c --- /dev/null +++ b/pkg/recfilter/dedupe_test.go @@ -0,0 +1,335 @@ +package recfilter + +import ( + "context" + "errors" + "fmt" + "strings" + "testing" + "time" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeServiceClient is a minimal provider.ServiceClient implementing only +// GetExistingCommitments, which is all AdjustRecommendationsForExisting uses. +type fakeServiceClient struct { + commitments []common.Commitment + err error +} + +func (f *fakeServiceClient) GetServiceType() common.ServiceType { return "" } +func (f *fakeServiceClient) GetRegion() string { return "" } +func (f *fakeServiceClient) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { + return nil, nil +} +func (f *fakeServiceClient) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { + return f.commitments, f.err +} +func (f *fakeServiceClient) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { + return common.PurchaseResult{}, nil +} +func (f *fakeServiceClient) ValidateOffering(ctx context.Context, rec common.Recommendation) error { + return nil +} +func (f *fakeServiceClient) GetOfferingDetails(ctx context.Context, rec common.Recommendation) (*common.OfferingDetails, error) { + return nil, nil +} +func (f *fakeServiceClient) GetValidResourceTypes(ctx context.Context) ([]string, error) { + return nil, nil +} + +func TestFilterRecentCommitments_StateAndWindow(t *testing.T) { + t.Parallel() + now := time.Now() + commitments := []common.Commitment{ + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 1, State: "active", StartDate: now.Add(-25 * time.Hour)}, // too old + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 2, State: "retired", StartDate: now.Add(-1 * time.Hour)}, // wrong state + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 3, State: "cancelled", StartDate: now.Add(-1 * time.Hour)}, // wrong state + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 4, State: "payment-pending", StartDate: now.Add(-1 * time.Hour)}, + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 5, State: "active", StartDate: now.Add(-1 * time.Hour)}, + } + + d := NewDuplicateChecker(DefaultDuplicateCheckLookbackHours) + recent := d.filterRecentCommitments(commitments) + + require.Len(t, recent, 2) + counts := []int{recent[0].Count, recent[1].Count} + assert.ElementsMatch(t, []int{4, 5}, counts) +} + +func TestAdjustRecommendationsForExisting_EngineNormalizationCollides(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "db.r5.large", Region: "us-east-1", Engine: "Aurora PostgreSQL", Count: 5, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + rec := common.Recommendation{ + ResourceType: "db.r5.large", Region: "us-east-1", Count: 5, + Details: &common.DatabaseDetails{Engine: "aurora-postgresql"}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec}, client) + + require.NoError(t, err) + assert.Empty(t, passed) + assert.Len(t, filtered, 1) +} + +func TestAdjustRecommendationsForExisting_UppercaseRecognizedAliasCollides(t *testing.T) { + t.Parallel() + + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + { + ResourceType: "db.r5.large", + Region: "us-east-1", + Engine: "AURORA POSTGRESQL", + Deployment: "single-az", + Count: 1, + State: "active", + StartDate: time.Now().Add(-1 * time.Hour), + }, + }} + rec := common.Recommendation{ + ResourceType: "db.r5.large", + Region: "us-east-1", + Count: 1, + Details: &common.DatabaseDetails{Engine: "aurora-postgresql", AZConfig: "single-az"}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec}, client) + + require.NoError(t, err) + assert.Empty(t, passed) + assert.Len(t, filtered, 1) +} + +func TestAdjustRecommendationsForExisting_FullCoverageDrops(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 5, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + rec := common.Recommendation{ + ResourceType: "db.t3.small", Region: "us-east-1", Count: 5, + Details: &common.DatabaseDetails{Engine: "mysql"}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec}, client) + + require.NoError(t, err) + assert.Empty(t, passed) + require.Len(t, filtered, 1) + assert.Equal(t, rec, filtered[0]) +} + +func TestAdjustRecommendationsForExisting_PartialCoverageConsumesBudget(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 2, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + recs := []common.Recommendation{ + {ResourceType: "db.t3.small", Region: "us-east-1", Count: 5, Details: &common.DatabaseDetails{Engine: "mysql"}}, + {ResourceType: "db.t3.small", Region: "us-east-1", Count: 3, Details: &common.DatabaseDetails{Engine: "mysql"}}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, recs, client) + + require.NoError(t, err) + assert.Empty(t, filtered) + require.Len(t, passed, 2) + assert.Equal(t, 3, passed[0].Count) // 5 - 2 = 3, budget consumed + assert.Equal(t, 3, passed[1].Count) // no existing coverage left, unchanged +} + +func TestAdjustRecommendationsForExisting_ClientErrorReturnsOriginal(t *testing.T) { + t.Parallel() + ctx := context.Background() + wantErr := errors.New("boom") + client := &fakeServiceClient{err: wantErr} + recs := []common.Recommendation{ + {ResourceType: "db.t3.small", Region: "us-east-1", Count: 5, Details: &common.DatabaseDetails{Engine: "mysql"}}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, recs, client) + + assert.Equal(t, recs, passed) + assert.Nil(t, filtered) + assert.Equal(t, wantErr, err) +} + +func TestAdjustRecommendationsForExisting_NoRecentCommitmentsPassesThrough(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{} + recs := []common.Recommendation{ + {ResourceType: "db.t3.small", Region: "us-east-1", Count: 5, Details: &common.DatabaseDetails{Engine: "mysql"}}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, recs, client) + + require.NoError(t, err) + assert.Nil(t, filtered) + require.Len(t, passed, 1) + // No reallocation/reordering: passed IS recs, not a copy. + assert.Same(t, &recs[0], &passed[0]) +} + +func TestNewDuplicateChecker_DefaultAndCustomLookback(t *testing.T) { + t.Parallel() + assert.Equal(t, DefaultDuplicateCheckLookbackHours, NewDuplicateChecker(0).LookbackHours) + assert.Equal(t, DefaultDuplicateCheckLookbackHours, NewDuplicateChecker(-1).LookbackHours) + assert.Equal(t, 48, NewDuplicateChecker(48).LookbackHours) +} + +func TestAdjustRecommendationsForExisting_NilLogfDoesNotPanic(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 2, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + recs := []common.Recommendation{ + {ResourceType: "db.t3.small", Region: "us-east-1", Count: 5, Details: &common.DatabaseDetails{Engine: "mysql"}}, + } + + d := NewDuplicateChecker(0) + assert.NotPanics(t, func() { + _, _, err := d.AdjustRecommendationsForExisting(ctx, recs, client) + require.NoError(t, err) + }) +} + +func TestAdjustRecommendationsForExisting_LogfReceivesDecisionTrail(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 5, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + recs := []common.Recommendation{ + {ResourceType: "db.t3.small", Region: "us-east-1", Count: 5, Details: &common.DatabaseDetails{Engine: "mysql"}}, + } + + var lines []string + d := NewDuplicateChecker(0) + d.Logf = func(format string, args ...any) { + lines = append(lines, fmt.Sprintf(format, args...)) + } + + _, _, err := d.AdjustRecommendationsForExisting(ctx, recs, client) + require.NoError(t, err) + + require.NotEmpty(t, lines) + found := false + for _, l := range lines { + if strings.Contains(l, "[DuplicateChecker]") { + found = true + break + } + } + assert.True(t, found) +} + +// TestAdjustRecommendationsForExisting_SingleAZCommitmentDoesNotSuppressMultiAZRec +// is a regression test for a duplicate-identity key that omitted the RDS +// deployment dimension. A recent Single-AZ commitment must not reduce or +// drop a Multi-AZ recommendation for the same instance type/region/engine: +// the two are priced and provisioned differently and do not cover each +// other's demand. +func TestAdjustRecommendationsForExisting_SingleAZCommitmentDoesNotSuppressMultiAZRec(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "db.r5.large", Region: "us-east-1", Engine: "mysql", Deployment: "single-az", Count: 5, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + rec := common.Recommendation{ + ResourceType: "db.r5.large", Region: "us-east-1", Count: 5, + Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: "multi-az"}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec}, client) + + require.NoError(t, err) + assert.Empty(t, filtered) + require.Len(t, passed, 1) + assert.Equal(t, rec.Count, passed[0].Count) +} + +// TestAdjustRecommendationsForExisting_MultiAZCommitmentDoesNotSuppressSingleAZRec +// is the mirror direction of the above: a recent Multi-AZ commitment must +// not suppress a Single-AZ recommendation. +func TestAdjustRecommendationsForExisting_MultiAZCommitmentDoesNotSuppressSingleAZRec(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "db.r5.large", Region: "us-east-1", Engine: "mysql", Deployment: "multi-az", Count: 5, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + rec := common.Recommendation{ + ResourceType: "db.r5.large", Region: "us-east-1", Count: 5, + Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: "single-az"}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec}, client) + + require.NoError(t, err) + assert.Empty(t, filtered) + require.Len(t, passed, 1) + assert.Equal(t, rec.Count, passed[0].Count) +} + +// TestAdjustRecommendationsForExisting_SingleAZCommitmentStillSuppressesSingleAZRec +// is the positive control: matching deployment on both sides must still +// dedupe, so the fix above does not simply disable deduplication for RDS. +func TestAdjustRecommendationsForExisting_SingleAZCommitmentStillSuppressesSingleAZRec(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "db.r5.large", Region: "us-east-1", Engine: "mysql", Deployment: "single-az", Count: 5, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + rec := common.Recommendation{ + ResourceType: "db.r5.large", Region: "us-east-1", Count: 5, + Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: "single-az"}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec}, client) + + require.NoError(t, err) + assert.Empty(t, passed) + require.Len(t, filtered, 1) +} + +// TestAdjustRecommendationsForExisting_NonRDSCommitmentStillDeduplicates +// guards the non-RDS path: a commitment/recommendation pair with no +// deployment dimension at all (e.g. EC2) must still land on the same key +// and dedupe, so adding deployment to the key doesn't regress non-RDS +// resource types that never populate it. +func TestAdjustRecommendationsForExisting_NonRDSCommitmentStillDeduplicates(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "m5.large", Region: "us-east-1", Count: 5, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + rec := common.Recommendation{ + ResourceType: "m5.large", Region: "us-east-1", Count: 5, + Details: &common.ComputeDetails{Platform: "linux"}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec}, client) + + require.NoError(t, err) + assert.Empty(t, passed) + require.Len(t, filtered, 1) +} diff --git a/pkg/recfilter/filters.go b/pkg/recfilter/filters.go new file mode 100644 index 000000000..8bd538436 --- /dev/null +++ b/pkg/recfilter/filters.go @@ -0,0 +1,155 @@ +// Package recfilter holds the stateless dimension filters (region, instance +// type, engine) and the minimum-pool-size stage shared between the cmd CLI +// and the MCP server. +package recfilter + +import ( + "fmt" + "slices" + "strings" + + "github.com/LeanerCloud/CUDly/pkg/common" +) + +// Logf is an injected logging sink. Package recfilter never logs through a +// package-level logger: cmd's AppLogger writes to stdout, which the MCP +// server owns as its protocol transport. A nil Logf disables logging. +type Logf func(format string, args ...any) + +// printf calls l with format/args, or no-ops if l is nil. +func (l Logf) printf(format string, args ...any) { + if l == nil { + return + } + l(format, args...) +} + +// Filters holds the stateless include/exclude dimension filters plus the +// minimum-pool-size threshold. Zero value = allow everything. +type Filters struct { + IncludeRegions []string + ExcludeRegions []string + IncludeInstanceTypes []string + ExcludeInstanceTypes []string + IncludeEngines []string + ExcludeEngines []string + MinPoolSize float64 +} + +// IncludesRegion checks if a region should be included based on filters. +func (f Filters) IncludesRegion(region string) bool { + // If include list is specified, region must be in it. + if len(f.IncludeRegions) > 0 && !slices.Contains(f.IncludeRegions, region) { + return false + } + + // If exclude list is specified, region must not be in it. + if slices.Contains(f.ExcludeRegions, region) { + return false + } + + return true +} + +// IncludesInstanceType checks if an instance type should be included based on filters. +func (f Filters) IncludesInstanceType(instanceType string) bool { + // If include list is specified, instance type must be in it. + if len(f.IncludeInstanceTypes) > 0 && !slices.Contains(f.IncludeInstanceTypes, instanceType) { + return false + } + + // If exclude list is specified, instance type must not be in it. + if slices.Contains(f.ExcludeInstanceTypes, instanceType) { + return false + } + + return true +} + +// IncludesEngine checks if a recommendation should be included based on engine +// filters. +// +// Both sides of the comparison are normalized. Normalizing only the +// recommendation would leave the filter list holding whatever spelling the +// operator typed, so --include-engines=postgres would silently fail to match a +// recommendation whose engine normalizes to "postgresql". NormalizeEngineName +// lowercases anything it does not recognize, so unknown engines keep the +// case-insensitive matching this had before. +func (f Filters) IncludesEngine(rec *common.Recommendation) bool { + engine := common.EngineFromDetails(rec.Details) + if engine == "" { + // No engine info: include by default unless there's an include list. + return len(f.IncludeEngines) == 0 + } + engine = strings.ToLower(engine) + + if len(f.IncludeEngines) > 0 && !matchesEngine(f.IncludeEngines, engine) { + return false + } + return !matchesEngine(f.ExcludeEngines, engine) +} + +// matchesEngine reports whether any filter entry normalizes to engine, which +// the caller has already normalized. +func matchesEngine(filters []string, engine string) bool { + for _, e := range filters { + if common.NormalizeEngineName(e) == engine { + return true + } + } + return false +} + +// IncludesPoolSize filters out RI recommendations for pools whose +// AverageInstancesUsedPerHour is below f.MinPoolSize. The purpose is to +// drop tiny pools where integer-arithmetic sizing forces 100% coverage +// regardless of --target-coverage (e.g. avg=1 with target=80% -> floor(0.8)=0 +// drops, ceil(0.8)=1 over-covers). Setting --min-pool-size=2 keeps pools +// where target can be meaningfully approximated. +// +// Pass-through cases: filter disabled (MinPoolSize<=0), or rec has no +// per-hour signal (avg<=0 -- SPs and recs CE didn't return usage for). +// Those pools aren't sized via the per-hour formula so the filter doesn't +// apply to them. +func (f Filters) IncludesPoolSize(rec *common.Recommendation) bool { + if f.MinPoolSize <= 0 { + return true + } + if rec.AverageInstancesUsedPerHour <= 0 { + return true + } + return rec.AverageInstancesUsedPerHour >= f.MinPoolSize +} + +// ApplyMinPoolSize drops recommendations whose AverageInstancesUsedPerHour +// is below f.MinPoolSize, logging a per-recommendation line via logf (no-op +// if nil) and recording each drop in drops under common.DropMinPoolSize. +// Returns recs unchanged, with no logging or allocation, when the filter is +// disabled (f.MinPoolSize <= 0). +func (f Filters) ApplyMinPoolSize(recs []common.Recommendation, logf Logf, drops *common.DropSummary) []common.Recommendation { + if f.MinPoolSize <= 0 { + return recs + } + + var kept []common.Recommendation + var poolDropCount int + var poolDropInstances float64 + + for i := range recs { + if f.IncludesPoolSize(&recs[i]) { + kept = append(kept, recs[i]) + continue + } + poolDropInstances += recs[i].AverageInstancesUsedPerHour + label := fmt.Sprintf("%s/%s/%s", recs[i].Service, recs[i].Region, recs[i].ResourceType) + logf.printf("INFO: --min-pool-size=%.1f dropped %s (avg=%.2f < threshold)", f.MinPoolSize, label, recs[i].AverageInstancesUsedPerHour) + poolDropCount++ + drops.Add(common.DropMinPoolSize, 1) + } + + if poolDropCount > 0 { + logf.printf("INFO: --min-pool-size dropped %d recommendation(s) (%.2f avg instances/hr total)", poolDropCount, poolDropInstances) + } + + return kept +} diff --git a/pkg/recfilter/filters_test.go b/pkg/recfilter/filters_test.go new file mode 100644 index 000000000..3927a4309 --- /dev/null +++ b/pkg/recfilter/filters_test.go @@ -0,0 +1,217 @@ +package recfilter + +import ( + "fmt" + "testing" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/stretchr/testify/assert" +) + +func TestIncludesEngine_CEAndRISpellingsBothMatch(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + recEngine string + includeEngines []string + expected bool + }{ + {"CE spelling rec, RI spelling filter", "Aurora PostgreSQL", []string{"aurora-postgresql"}, true}, + {"RI spelling rec, CE-normalized filter", "aurora-postgresql", []string{"aurora-postgresql"}, true}, + {"postgres rec, postgresql filter", "postgres", []string{"postgresql"}, true}, + {"PostgreSQL rec, postgresql filter", "PostgreSQL", []string{"postgresql"}, true}, + // The reciprocal direction: the filter carries the alias and the + // recommendation the canonical name. Matching only after normalizing + // the recommendation would miss every one of these. + {"postgresql rec, postgres filter", "postgresql", []string{"postgres"}, true}, + {"aurora-postgresql rec, CE spelling filter", "aurora-postgresql", []string{"Aurora PostgreSQL"}, true}, + {"sqlserver rec, sqlserver-se filter", "sqlserver", []string{"sqlserver-se"}, true}, + {"oracle rec, oracle-ee filter", "oracle", []string{"oracle-ee"}, true}, + {"unrecognized engine still matches case-insensitively", "Db2", []string{"DB2"}, true}, + {"unrelated engine does not match", "mysql", []string{"postgres"}, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := Filters{IncludeEngines: tt.includeEngines} + rec := common.Recommendation{Details: &common.DatabaseDetails{Engine: tt.recEngine}} + assert.Equal(t, tt.expected, f.IncludesEngine(&rec)) + }) + } +} + +func TestIncludesEngine_UppercaseRecognizedAliasFilters(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + recEngine string + includeEngines []string + excludeEngines []string + want bool + }{ + { + name: "uppercase aurora postgresql include matches canonical recommendation", + recEngine: "aurora-postgresql", + includeEngines: []string{"AURORA POSTGRESQL"}, + want: true, + }, + { + name: "uppercase aurora postgresql exclude removes canonical recommendation", + recEngine: "aurora-postgresql", + excludeEngines: []string{"AURORA POSTGRESQL"}, + want: false, + }, + { + name: "uppercase postgres short alias include matches postgresql recommendation", + recEngine: "postgresql", + includeEngines: []string{"POSTGRES"}, + want: true, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + f := Filters{IncludeEngines: tt.includeEngines, ExcludeEngines: tt.excludeEngines} + rec := common.Recommendation{Details: &common.DatabaseDetails{Engine: tt.recEngine}} + assert.Equal(t, tt.want, f.IncludesEngine(&rec)) + }) + } +} + +func TestIncludesRegion_EmptyIncludeListAllowsAll(t *testing.T) { + f := Filters{} + assert.True(t, f.IncludesRegion("us-east-1")) + assert.True(t, f.IncludesRegion("eu-west-1")) +} + +func TestIncludesInstanceType_EmptyIncludeListAllowsAll(t *testing.T) { + f := Filters{} + assert.True(t, f.IncludesInstanceType("db.t3.micro")) + assert.True(t, f.IncludesInstanceType("cache.r5.large")) +} + +func TestIncludesEngine_EmptyIncludeListAllowsAll(t *testing.T) { + f := Filters{} + rec := common.Recommendation{Details: &common.DatabaseDetails{Engine: "mysql"}} + assert.True(t, f.IncludesEngine(&rec)) +} + +func TestIncludesRegion_ExcludeBeatsInclude(t *testing.T) { + f := Filters{IncludeRegions: []string{"us-east-1"}, ExcludeRegions: []string{"us-east-1"}} + assert.False(t, f.IncludesRegion("us-east-1")) +} + +func TestIncludesInstanceType_ExcludeBeatsInclude(t *testing.T) { + f := Filters{IncludeInstanceTypes: []string{"db.t3.micro"}, ExcludeInstanceTypes: []string{"db.t3.micro"}} + assert.False(t, f.IncludesInstanceType("db.t3.micro")) +} + +// Exclude entries are normalized on the same axis as include entries: an +// operator excluding "postgres" must not still get "postgresql" rows. +func TestIncludesEngine_ExcludeNormalizesAliases(t *testing.T) { + f := Filters{ExcludeEngines: []string{"postgres"}} + rec := common.Recommendation{Details: &common.DatabaseDetails{Engine: "PostgreSQL"}} + assert.False(t, f.IncludesEngine(&rec)) + + other := common.Recommendation{Details: &common.DatabaseDetails{Engine: "mysql"}} + assert.True(t, f.IncludesEngine(&other)) +} + +func TestIncludesEngine_ExcludeBeatsInclude(t *testing.T) { + f := Filters{IncludeEngines: []string{"mysql"}, ExcludeEngines: []string{"mysql"}} + rec := common.Recommendation{Details: &common.DatabaseDetails{Engine: "mysql"}} + assert.False(t, f.IncludesEngine(&rec)) +} + +func TestIncludesPoolSize(t *testing.T) { + tests := []struct { + name string + avg float64 + minPool float64 + expected bool + }{ + {"filter disabled", 0.5, 0, true}, + {"no per-hour signal passes through", 0, 2.0, true}, + {"below threshold", 1.5, 2.0, false}, + {"at threshold", 2.0, 2.0, true}, + {"above threshold", 3.0, 2.0, true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := Filters{MinPoolSize: tt.minPool} + rec := common.Recommendation{AverageInstancesUsedPerHour: tt.avg} + assert.Equal(t, tt.expected, f.IncludesPoolSize(&rec)) + }) + } +} + +func TestApplyMinPoolSize_NilLogfSafeAndRecordsDrop(t *testing.T) { + f := Filters{MinPoolSize: 2.0} + recs := []common.Recommendation{ + {Region: "us-east-1", ResourceType: "db.t3.micro", AverageInstancesUsedPerHour: 1.5}, + } + drops := common.NewDropSummary() + + assert.NotPanics(t, func() { + result := f.ApplyMinPoolSize(recs, nil, drops) + assert.Empty(t, result) + }) + assert.Equal(t, 1, drops.Total()) + assert.Contains(t, drops.FormatOneLine(), common.DropMinPoolSize) +} + +func TestApplyMinPoolSize_DisabledReturnsUnchangedNoDrops(t *testing.T) { + f := Filters{MinPoolSize: 0} + recs := []common.Recommendation{ + {Region: "us-east-1", ResourceType: "db.t3.micro", AverageInstancesUsedPerHour: 0.1}, + } + drops := common.NewDropSummary() + + result := f.ApplyMinPoolSize(recs, nil, drops) + + assert.Equal(t, recs, result) + assert.Equal(t, 0, drops.Total()) +} + +func TestApplyMinPoolSize_LogfReceivesExpectedLines(t *testing.T) { + f := Filters{MinPoolSize: 2.0} + recs := []common.Recommendation{ + {Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.micro", AverageInstancesUsedPerHour: 1.5}, + {Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.small", AverageInstancesUsedPerHour: 5.0}, + } + drops := common.NewDropSummary() + + var lines []string + logf := func(format string, args ...any) { + lines = append(lines, fmt.Sprintf(format, args...)) + } + + result := f.ApplyMinPoolSize(recs, logf, drops) + + assert.Len(t, result, 1) + assert.Len(t, lines, 2) + assert.Contains(t, lines[0], "--min-pool-size=2.0 dropped") + assert.Contains(t, lines[1], "--min-pool-size dropped 1 recommendation(s)") +} + +// The dimension predicates are deliberately independent rather than bundled +// behind one PassesDimensions helper: region resolution is provider-specific +// (a Savings Plan's effective region lives in Details, see #1881) and needs +// providers/aws, which the pkg module cannot import. Callers compose the +// portable checks with their own region handling. +func TestDimensionPredicates_IgnoreAccount(t *testing.T) { + f := Filters{} + rec := common.Recommendation{ + Region: "us-east-1", + ResourceType: "db.t3.micro", + AccountName: "some-restricted-account", + } + assert.True(t, f.IncludesRegion(rec.Region)) + assert.True(t, f.IncludesInstanceType(rec.ResourceType)) + assert.True(t, f.IncludesEngine(&rec)) +} diff --git a/pkg/recfilter/sizing.go b/pkg/recfilter/sizing.go new file mode 100644 index 000000000..3d1357060 --- /dev/null +++ b/pkg/recfilter/sizing.go @@ -0,0 +1,375 @@ +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 { + // No nil guard: DropSummary.Add is nil-receiver safe, and every + // other drop site in this package relies on that. + 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) +// avg = AverageInstancesUsedPerHour (instances) +// n_target = floor(avg * gap / 100) +// +// The buy is anchored on the pool's own average demand and the absolute +// gap to target: target%-existing% of avg instances. For example with +// avg=10, existing=50% and target=80%: gap=30, so n_target=floor(3)=3. +// +// An earlier version anchored on AWS's rec.Count +// (floor(rec.Count * gap / (100-existing))). That under-bought when AWS +// sized rec.Count for less than full coverage, and when CE's org-wide +// ExistingCoveragePct disagreed with rec.Count's per-account derivation. +// Both inputs of the current formula come from GetReservationCoverage, so +// the buy lines up with the AWS console's reservations-coverage report. +// rec.Count survives only as the denominator of the cost-scaling ratio. +// +// 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(avg * target/100) — i.e. plain target% of the pool's average +// hourly demand. +// 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..ef3db120b --- /dev/null +++ b/pkg/recfilter/sizing_test.go @@ -0,0 +1,395 @@ +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) + }) + + t.Run("Nonzero existing coverage keeps only the remaining target gap", func(t *testing.T) { + t.Parallel() + // avg=10, target=80, existing=50: gap=30, nTarget=floor(10*30/100)=3. + // The kept recommendation should size only the uncovered gap. + rec := mkRI(10, 10, 50) + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil, nil) + require.Len(t, out, 1) + assert.Equal(t, 3, out[0].Count) + assert.Equal(t, 80.0, out[0].ProjectedCoverage) + assert.InDelta(t, 300.0, out[0].CommitmentCost, 0.001) + }) +} + +// 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 + }) +}