diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 58b613b13..bd5394a5c 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -217,13 +217,42 @@ func scoreLimitAndDisplay(recs []common.Recommendation, cfg Config, drops *commo MinCount: cfg.MinCount, } result := scorer.Score(recs, scorerCfg) - result.Passed = applyGlobalInstanceLimit(result.Passed, cfg, drops) + result.Passed = applyGlobalInstanceLimit(result.Passed, cfg, rankBySavingsPercentage, drops) fmt.Print(reporter.RenderTable(result)) fmt.Print(reporter.RenderExcluded(result)) fmt.Print(reporter.RenderSummary(result)) return result } +// rankingRule names the ordering --max-instances consumes, so the cap can say +// which rule decided what survived. The two paths rank on different keys +// because they carry different data, and an operator reading the drop list has +// to know which one applied. Typed rather than a bare string so the call sites +// cannot invent a third wording that does not match any implemented ordering. +type rankingRule string + +const ( + // rankBySavingsPercentage is the recommendation-driven path: scorer.Score + // sorts on SavingsPercentage, an intensive rate, before the cap runs. + rankBySavingsPercentage rankingRule = "highest savings-percentage recommendations first" + // rankBySavingsPerInstance is the --input-csv path: a CSV row carries no + // savings percentage, so sortBySavingsPerInstance derives the rate from + // the EstimatedSavings and Count columns it does carry. + rankBySavingsPerInstance rankingRule = "highest savings-per-instance rows first (a CSV row carries no savings percentage)" +) + +// capBinds reports whether --max-instances will actually remove instances from +// recs, rather than being unset or already satisfied. +// +// applyGlobalInstanceLimit and requireRankingSignal have to agree on this +// exactly: the second refuses precisely the runs whose outcome the first would +// otherwise decide on an ordering that carries no information. If the two +// conditions drift apart, either a run is refused for a cap that changes +// nothing, or an unrankable run reaches the cap after all. +func capBinds(recs []common.Recommendation, cfg Config) bool { + return cfg.MaxInstances > 0 && CalculateTotalInstances(recs) > int(cfg.MaxInstances) +} + // applyGlobalInstanceLimit enforces --max-instances once across the entire run. // // The flag is documented as a hard cap on the total number of instances @@ -232,9 +261,11 @@ func scoreLimitAndDisplay(recs []common.Recommendation, cfg Config, drops *commo // each (service, region) pair independently and multiplies the operator's cap // by the number of pairs. // -// passed must already be ordered best-first: ApplyInstanceLimit consumes the -// slice in order and drops the tail, so the ordering decides which commitments -// survive. scorer.Score guarantees that ordering. +// passed must already be ordered best-first by rule: ApplyInstanceLimit +// consumes the slice in order and drops the tail, so the ordering decides +// which commitments survive. scorer.Score guarantees the +// rankBySavingsPercentage ordering; scoreAndLimitCSVRecs establishes the +// rankBySavingsPerInstance one. // // Truncation can push a recommendation under --min-count, which is a hard // floor rather than advice, so dropTruncatedBelowMinCount removes any such @@ -242,18 +273,15 @@ func scoreLimitAndDisplay(recs []common.Recommendation, cfg Config, drops *commo // // Nothing is truncated silently. Every reduced or dropped recommendation is // named on stdout, and the drops are counted into the end-of-run summary. -func applyGlobalInstanceLimit(passed []common.Recommendation, cfg Config, drops *common.DropSummary) []common.Recommendation { - if cfg.MaxInstances <= 0 { - return passed - } - totalBefore := CalculateTotalInstances(passed) - if totalBefore <= int(cfg.MaxInstances) { +func applyGlobalInstanceLimit(passed []common.Recommendation, cfg Config, rule rankingRule, drops *common.DropSummary) []common.Recommendation { + if !capBinds(passed, cfg) { return passed } + totalBefore := CalculateTotalInstances(passed) limited := ApplyInstanceLimit(passed, cfg.MaxInstances) limited, belowMin := dropTruncatedBelowMinCount(limited, cfg.MinCount) - reportInstanceLimit(passed, limited, len(belowMin), totalBefore, cfg.MaxInstances, drops) + reportInstanceLimit(passed, limited, len(belowMin), totalBefore, cfg.MaxInstances, rule, drops) reportMinCountDrops(belowMin, cfg.MinCount, drops) return limited } @@ -311,15 +339,19 @@ func reportMinCountDrops(removed []common.Recommendation, minCount int, drops *c // after must be the prefix of before produced by ApplyInstanceLimit, so // after[i] and before[i] describe the same recommendation. // +// rule is the ordering the caller sorted before by, and is printed verbatim: +// the drop list is unreadable without knowing which key decided it, and the +// two paths do not rank on the same key. +// // belowMinCount is how many of the missing entries were removed by the // --min-count floor rather than by the budget. They are still listed here as // dropped (they were), but they are attributed to --min-count-after-cap by // reportMinCountDrops, so excluding them from this tally keeps each dropped // recommendation counted exactly once in the end-of-run summary. -func reportInstanceLimit(before, after []common.Recommendation, belowMinCount, totalBefore int, maxInstances int32, drops *common.DropSummary) { +func reportInstanceLimit(before, after []common.Recommendation, belowMinCount, totalBefore int, maxInstances int32, rule rankingRule, drops *common.DropSummary) { AppLogger.Printf("\nšŸ”’ --max-instances=%d caps the whole run: the %d recommendations that passed scoring total %d instances.\n", maxInstances, len(before), totalBefore) - AppLogger.Printf(" Keeping the highest savings-percentage recommendations first. The following are reduced or dropped:\n") + AppLogger.Printf(" Keeping the %s. The following are reduced or dropped:\n", rule) reduced, dropped := 0, 0 for i := range before { @@ -454,20 +486,17 @@ func runToolFromCSV(ctx context.Context, cfg Config) error { AppLogger.Printf("āœ… Loaded %d recommendations from CSV\n", len(recs)) // Filter and adjust recommendations - recs = filterAndAdjustRecommendations(recs, csvModeCoverage, cfg) + recs, err = filterAndAdjustRecommendations(recs, csvModeCoverage, cfg) + if err != nil { + return err + } if len(recs) == 0 { AppLogger.Println("āš ļø No recommendations to process after filtering") return nil } - // Load AWS configuration - var configOptions []func(*awsconfig.LoadOptions) error - configOptions = append(configOptions, awsconfig.WithRegion("us-east-1")) - if cfg.Profile != "" { - configOptions = append(configOptions, awsconfig.WithSharedConfigProfile(cfg.Profile)) - } - awsCfg, err := awsconfig.LoadDefaultConfig(ctx, configOptions...) + awsCfg, err := loadAWSConfig(ctx, cfg) if err != nil { return fmt.Errorf("failed to load AWS config: %w", err) } @@ -518,7 +547,13 @@ func runToolFromCSV(ctx context.Context, cfg Config) error { AppLogger.Printf(" āš ļø Warning: Could not check for existing RIs: %v\n", err) adjustedRecs = recs // Continue with original recommendations if check fails } - recs = adjustedRecs + // Deducting existing commitments shrinks Count, which can push a + // row that cleared the floor in filterAndAdjustRecommendations back + // under it (--min-count 5, a row of 6, and 5 matching recent + // commitments would otherwise be purchased at 1). --min-count is a + // floor on what gets bought, so it is re-applied to whatever the + // deduction left, not only to the pre-deduction counts. + recs = applyMinCountFloor(adjustedRecs, cfg.MinCount) serviceRecs = append(serviceRecs, recs...) allAdjustedRecs = append(allAdjustedRecs, recs...) @@ -553,8 +588,13 @@ func runToolFromCSV(ctx context.Context, cfg Config) error { return nil } -// filterAndAdjustRecommendations applies filters, coverage, count override, and instance limits to recommendations. -func filterAndAdjustRecommendations(recs []common.Recommendation, csvModeCoverage float64, cfg Config) []common.Recommendation { +// filterAndAdjustRecommendations applies filters, coverage, count override, +// the --min-count floor and the run-wide --max-instances cap to +// recommendations loaded from --input-csv. +// +// It returns an error when the cap cannot be enforced honestly; see +// requireRankingSignal. +func filterAndAdjustRecommendations(recs []common.Recommendation, csvModeCoverage float64, cfg Config) ([]common.Recommendation, error) { // Query running instances for engine version validation log.Printf("šŸ” Querying running RDS instances across all regions to validate engine versions...") instanceVersions, err := queryRunningInstanceEngineVersions(context.Background(), cfg) @@ -604,16 +644,8 @@ func filterAndAdjustRecommendations(recs []common.Recommendation, csvModeCoverag recs = ApplyCountOverride(recs, cfg.OverrideCount) } - // Apply instance limit if specified - if cfg.MaxInstances > 0 { - beforeLimit := len(recs) - recs = ApplyInstanceLimit(recs, cfg.MaxInstances) - if len(recs) < beforeLimit { - AppLogger.Printf("šŸ”’ Applied instance limit: %d recs after limiting to %d instances\n", len(recs), cfg.MaxInstances) - } - } - - return recs + // Enforce --min-count and the run-wide --max-instances cap. + return scoreAndLimitCSVRecs(recs, cfg) } // processService processes a single service and returns recommendations and results. diff --git a/cmd/multi_service_coverage_test.go b/cmd/multi_service_coverage_test.go index bfe0644d8..ad82791f8 100644 --- a/cmd/multi_service_coverage_test.go +++ b/cmd/multi_service_coverage_test.go @@ -723,7 +723,8 @@ func TestFilterAndAdjustRecommendations_ZeroCoverage(t *testing.T) { toolCfg.MaxInstances = 0 toolCfg.OverrideCount = 0 - result := filterAndAdjustRecommendations(recommendations, 0.0, toolCfg) + result, err := filterAndAdjustRecommendations(recommendations, 0.0, toolCfg) + require.NoError(t, err) // 0% coverage should return empty assert.Empty(t, result) @@ -749,7 +750,8 @@ func TestFilterAndAdjustRecommendations_WithEngineVersionFiltering(t *testing.T) toolCfg.OverrideCount = 0 toolCfg.IncludeExtendedSupport = false - result := filterAndAdjustRecommendations(recommendations, 100.0, toolCfg) + result, err := filterAndAdjustRecommendations(recommendations, 100.0, toolCfg) + require.NoError(t, err) // Should return recommendations (engine version filtering is done inside the function) assert.NotEmpty(t, result) @@ -760,14 +762,18 @@ func TestFilterAndAdjustRecommendations_MaxInstancesApplied(t *testing.T) { defer saved.restore() recommendations := []common.Recommendation{ - {Service: common.ServiceRDS, ResourceType: "db.t3.small", Count: 20}, - {Service: common.ServiceRDS, ResourceType: "db.t3.medium", Count: 20}, + // EstimatedSavings is set because a binding --max-instances has to rank + // the rows it chooses between; requireRankingSignal refuses a run whose + // rows carry no savings value rather than capping them by name. + {Service: common.ServiceRDS, ResourceType: "db.t3.small", Count: 20, EstimatedSavings: 100}, + {Service: common.ServiceRDS, ResourceType: "db.t3.medium", Count: 20, EstimatedSavings: 200}, } toolCfg.MaxInstances = 15 toolCfg.OverrideCount = 0 - result := filterAndAdjustRecommendations(recommendations, 100.0, toolCfg) + result, err := filterAndAdjustRecommendations(recommendations, 100.0, toolCfg) + require.NoError(t, err) // Total instances should not exceed maxInstances totalInstances := 0 @@ -789,7 +795,8 @@ func TestFilterAndAdjustRecommendations_OverrideCountApplied(t *testing.T) { toolCfg.MaxInstances = 0 toolCfg.OverrideCount = 5 - result := filterAndAdjustRecommendations(recommendations, 100.0, toolCfg) + result, err := filterAndAdjustRecommendations(recommendations, 100.0, toolCfg) + require.NoError(t, err) // All recommendations should have count = OverrideCount for _, rec := range result { diff --git a/cmd/multi_service_csv_cap.go b/cmd/multi_service_csv_cap.go new file mode 100644 index 000000000..76d360a9c --- /dev/null +++ b/cmd/multi_service_csv_cap.go @@ -0,0 +1,159 @@ +package main + +import ( + "fmt" + "sort" + "strings" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/pkg/scorer" +) + +// scoreAndLimitCSVRecs enforces --min-count and the run-wide --max-instances +// cap on recommendations loaded from --input-csv, so both spend guards behave +// on the CSV path as they do on the recommendation-driven path. Both are +// documented in docs/cli/filtering.md as flags of the tool, not of a mode. +// +// Previously the CSV path handed the load-ordered slice straight to +// ApplyInstanceLimit, which consumes its input in slice order and drops the +// tail: whichever rows appeared first in the file spent the budget, and +// --min-count was never consulted, so a row the cap truncated below the floor +// was purchased short. +// +// Only MinCount is enforced here. A CSV row carries no savings percentage and +// no break-even figure (writeMultiServiceCSVReport emits neither column and +// parseCSVRecord reads neither), so both fields load as zero and gating on +// them would reject every row of every file. --min-savings-pct and +// --max-break-even-months are therefore refused up front by +// validateCSVModeFilterFlags rather than accepted and ignored here; #1819 +// tracks teaching the CSV format to carry those columns. +// +// scorer.Score's own ordering is useless here: SavingsPercentage is uniformly +// zero on loaded rows, so it resolves on EstimatedSavings, a whole-row dollar +// total, while --max-instances is a budget in instances. Ranking a total +// against a per-instance budget spends the whole budget on whichever row is +// merely biggest, not on the rows that return the most per instance bought. +// sortBySavingsPerInstance therefore re-orders the survivors on the rate +// derived from the two columns a CSV does carry, which is the greedy the flag +// implies, and applyGlobalInstanceLimit consumes that order: the cap keeps the +// best-value rows, names every row it reduces or drops, and drops rather than +// shortens anything truncated below --min-count. +func scoreAndLimitCSVRecs(recs []common.Recommendation, cfg Config) ([]common.Recommendation, error) { + passed := applyMinCountFloor(recs, cfg.MinCount) + if err := requireRankingSignal(passed, cfg); err != nil { + return nil, err + } + sortBySavingsPerInstance(passed) + return applyGlobalInstanceLimit(passed, cfg, rankBySavingsPerInstance, nil), nil +} + +// applyMinCountFloor drops recommendations below the --min-count floor and +// names each drop on stdout. A floor of 0 disables the flag, and returns recs +// untouched rather than re-ordering them for nothing. +// +// The floor itself is scorer.Score's, so the CSV path and the +// recommendation-driven path reject on the identical predicate and reason +// string. MinCount is the only scorer.Config field this helper ever sets, so +// the "--min-count dropped" prefix cannot come to describe some other filter. +func applyMinCountFloor(recs []common.Recommendation, minCount int) []common.Recommendation { + if minCount <= 0 { + return recs + } + scored := scorer.Score(recs, scorer.Config{MinCount: minCount}) + for i := range scored.Filtered { + f := scored.Filtered[i] + AppLogger.Printf("šŸ”’ --min-count dropped %s %s %s: %s\n", + f.Recommendation.Service, f.Recommendation.Region, f.Recommendation.ResourceType, f.FilterReason) + } + return scored.Passed +} + +// savingsPerInstance is the ranking key for CSV rows: the row's monthly +// savings divided by the instances it would buy. --max-instances is a budget +// in instances, so the rows worth keeping are the ones returning the most per +// instance, not the ones whose total happens to be largest. +// +// A non-positive Count buys nothing and has no rate, so it ranks last rather +// than dividing by zero. ApplyInstanceLimit already refuses to credit budget +// back for such a row. +func savingsPerInstance(rec common.Recommendation) float64 { + if rec.Count <= 0 { + return 0 + } + return rec.EstimatedSavings / float64(rec.Count) +} + +// sortBySavingsPerInstance orders recs best-value-first, in place. +// +// Rows with equal rates fall back to the same Service|Region|ResourceType key +// scorer.Score tie-breaks on, so the selection is deterministic whatever order +// the file listed them in. The tie-break is spelled out here rather than +// inherited from an upstream sort because --min-count 0 skips the scorer +// entirely, which would otherwise leave file order deciding between equals on +// exactly the path #1741 is about. +// +// It must run before the cap, never after: ApplyInstanceLimit truncates Count +// without rescaling EstimatedSavings (#1830), so a post-cap row's rate is +// inflated by exactly the amount the cap removed. +func sortBySavingsPerInstance(recs []common.Recommendation) { + sort.SliceStable(recs, func(i, j int) bool { + a, b := recs[i], recs[j] + if rateA, rateB := savingsPerInstance(a), savingsPerInstance(b); rateA != rateB { + return rateA > rateB + } + keyA := string(a.Service) + "|" + a.Region + "|" + a.ResourceType + keyB := string(b.Service) + "|" + b.Region + "|" + b.ResourceType + return keyA < keyB + }) +} + +// maxNamedUnrankableRows bounds how many offending rows requireRankingSignal +// names before summarizing the rest, so a large file produces a readable error. +const maxNamedUnrankableRows = 5 + +// requireRankingSignal refuses a run whose --max-instances cap has to choose +// between rows it cannot rank. +// +// parseCSVFloat leaves EstimatedSavings at zero for a blank cell, and +// getCSVField returns "" for a column that is not in the header at all, so a +// CSV written without an EstimatedSavings column loads every row at zero. +// Nothing downstream can tell that apart from a row genuinely worth $0: the +// value is absent, not zero. With every rate equal, sortBySavingsPerInstance +// and scorer.Score both fall through to the Service|Region|ResourceType +// tie-break, and the cap silently buys by instance-type name while stdout and +// docs/cli/filtering.md both promise it is buying by savings. +// +// Ranking only decides anything when the cap actually binds, so that is the +// only case this refuses; a file with no savings column still runs uncapped, +// and so does one whose total already fits. Money paths in this project fail +// loud rather than picking a defensible-looking default, and #1741's own +// framing is that silent partial enforcement of a spend guard is worse than +// not offering the guard. +func requireRankingSignal(recs []common.Recommendation, cfg Config) error { + if !capBinds(recs, cfg) { + return nil + } + + unrankable := make([]string, 0) + for i := range recs { + if recs[i].EstimatedSavings <= 0 { + unrankable = append(unrankable, fmt.Sprintf("%s %s %s", + recs[i].Service, recs[i].Region, recs[i].ResourceType)) + } + } + if len(unrankable) == 0 { + return nil + } + + named := unrankable + suffix := "" + if len(named) > maxNamedUnrankableRows { + named = named[:maxNamedUnrankableRows] + suffix = fmt.Sprintf(" (and %d more)", len(unrankable)-maxNamedUnrankableRows) + } + return fmt.Errorf( + "--max-instances=%d has to choose which of %d recommendations to buy, but %d row(s) of %s carry no usable EstimatedSavings value: %s%s. "+ + "A blank or missing EstimatedSavings cell is indistinguishable from $0 of savings, so capping on it would pick by instance-type name rather than by value. "+ + "Populate EstimatedSavings for every row, or drop --max-instances and cap the file itself", + cfg.MaxInstances, len(recs), len(unrankable), cfg.CSVInput, strings.Join(named, ", "), suffix) +} diff --git a/cmd/multi_service_max_instances_test.go b/cmd/multi_service_max_instances_test.go index b1f70e711..f0befa28c 100644 --- a/cmd/multi_service_max_instances_test.go +++ b/cmd/multi_service_max_instances_test.go @@ -2,6 +2,7 @@ package main import ( "context" + "path/filepath" "testing" "github.com/LeanerCloud/CUDly/pkg/common" @@ -361,7 +362,7 @@ func TestApplyGlobalInstanceLimit(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { drops := common.NewDropSummary() - got := applyGlobalInstanceLimit(recs, Config{MaxInstances: tt.maxInstances}, drops) + got := applyGlobalInstanceLimit(recs, Config{MaxInstances: tt.maxInstances}, rankBySavingsPercentage, drops) assert.Len(t, got, tt.expectedLen) assert.Equal(t, tt.expectedTotal, CalculateTotalInstances(got)) @@ -414,7 +415,7 @@ func TestReportInstanceLimitNamesEveryChange(t *testing.T) { drops := common.NewDropSummary() out := captureAppOutput(t, func() { - reportInstanceLimit(before, after, 0, CalculateTotalInstances(before), 7, drops) + reportInstanceLimit(before, after, 0, CalculateTotalInstances(before), 7, rankBySavingsPercentage, drops) }) assert.Contains(t, out, "--max-instances=7") @@ -423,3 +424,477 @@ func TestReportInstanceLimitNamesEveryChange(t *testing.T) { assert.Contains(t, out, "dropped") assert.Contains(t, out, "m5.xlarge") } + +// ==================== --input-csv path (#1741) ==================== + +// csvFixtureHeader matches writeMultiServiceCSVReport's column names for the +// fields these tests exercise, so the fixtures parse the way a real +// round-tripped report does. +const csvFixtureHeader = "Service,Region,ResourceType,Engine,Count,EstimatedSavings,Term,PaymentOption,Account\n" + +// csvNoSavingsHeader is csvFixtureHeader without the EstimatedSavings column, +// the shape of a hand-written minimal CSV. getCSVField returns "" for a column +// that is not in the header and parseCSVFloat leaves the field at zero, so +// every row of such a file loads with EstimatedSavings == 0. +const csvNoSavingsHeader = "Service,Region,ResourceType,Engine,Count,Term,PaymentOption,Account\n" + +// csvRecsFrom writes rows to a temporary recommendations CSV, loads them back +// through the production parser, and returns the recommendations exactly as a +// --input-csv run sees them. +// +// Building the fixture through loadRecommendationsFromCSV rather than by hand +// keeps these tests honest about what a CSV row actually carries: neither +// writeMultiServiceCSVReport nor parseCSVRecord knows a savings-percentage +// column, so every loaded row has SavingsPercentage == 0 and any +// savings-ordered selection has to resolve on EstimatedSavings. A hand-built +// fixture could quietly set SavingsPercentage and prove nothing about the real +// input. +func csvRecsFrom(t *testing.T, rows string) []common.Recommendation { + t.Helper() + return csvRecsFromHeader(t, csvFixtureHeader, rows) +} + +// csvRecsFromHeader is csvRecsFrom over an arbitrary header, for fixtures that +// exercise a CSV missing a column the selection depends on. +func csvRecsFromHeader(t *testing.T, header, rows string) []common.Recommendation { + t.Helper() + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, header+rows)) + require.NoError(t, err) + require.NotEmpty(t, recs) + for i := range recs { + require.Zero(t, recs[i].SavingsPercentage, + "a CSV row carries no savings percentage; the ordering under test must come from EstimatedSavings") + } + return recs +} + +// TestCSVCapKeepsHighestSavingsNotFileOrder is the selection-order regression +// for #1741. +// +// filterAndAdjustRecommendations used to hand the load-ordered slice straight +// to ApplyInstanceLimit, which consumes its input in slice order and drops the +// tail, so whichever rows appeared first in the file spent the whole budget. +// Both the old and the new behavior respect the cap total, so only a fixture +// whose best-value rows are *not* first can tell them apart: the $10 row leads +// the file and the $500 row sits in the middle. +func TestCSVCapKeepsHighestSavingsNotFileOrder(t *testing.T) { + isolateAWSEnv(t) + const ( + maxInstances = 10 + countPerRow = 6 + ) + + recs := csvRecsFrom(t, `rds,us-east-1,db.t3.small,postgres,6,10.00,1yr,All Upfront,123456789012 +rds,us-east-1,db.t3.medium,postgres,6,500.00,1yr,All Upfront,123456789012 +rds,us-east-1,db.t3.large,postgres,6,100.00,1yr,All Upfront,123456789012 +`) + + var ( + got []common.Recommendation + err error + ) + out := captureAppOutput(t, func() { + got, err = filterAndAdjustRecommendations(recs, 100.0, Config{MaxInstances: maxInstances}) + }) + require.NoError(t, err) + + // Asserted only to keep the fixture honest: a file-ordered cap satisfies + // the total too, so the total on its own proves nothing here. + require.Equal(t, maxInstances, CalculateTotalInstances(got)) + + // The property under test is the selection ORDER. The budget must go to + // the highest-savings rows, best first, not to the rows the file happened + // to list first. + require.Len(t, got, 2) + assert.Equal(t, "db.t3.medium", got[0].ResourceType, + "the cap must consume savings order, not file order; got %s first", got[0].ResourceType) + assert.InDelta(t, 500.00, got[0].EstimatedSavings, 0.001) + assert.Equal(t, countPerRow, got[0].Count) + assert.Equal(t, "db.t3.large", got[1].ResourceType) + assert.InDelta(t, 100.00, got[1].EstimatedSavings, 0.001) + assert.Equal(t, maxInstances-countPerRow, got[1].Count) + + // Nothing shrinks silently: the row the cap dropped is named. + assert.Contains(t, out, "db.t3.small") +} + +// TestCSVCapDropsTruncationBelowMinCount pins that the CSV path cannot deliver +// a purchase smaller than the operator's --min-count floor, the property #1608 +// and #1725 established on the recommendation-driven path. +// +// Fixture: two rows of 6 instances, cap 10, floor 5. The budget leaves room +// for 6 + 4, and 4 is under the floor, so the second row is dropped rather +// than purchased short. The run buys 6. +func TestCSVCapDropsTruncationBelowMinCount(t *testing.T) { + isolateAWSEnv(t) + const ( + maxInstances = 10 + minCount = 5 + countPerRow = 6 + ) + + recs := csvRecsFrom(t, `rds,us-east-1,db.t3.large,postgres,6,100.00,1yr,All Upfront,123456789012 +rds,us-east-1,db.t3.medium,postgres,6,500.00,1yr,All Upfront,123456789012 +`) + + var ( + got []common.Recommendation + err error + ) + out := captureAppOutput(t, func() { + got, err = filterAndAdjustRecommendations(recs, 100.0, Config{MaxInstances: maxInstances, MinCount: minCount}) + }) + require.NoError(t, err) + + for i := range got { + assert.GreaterOrEqual(t, got[i].Count, minCount, + "--min-count %d was set, but %s would be purchased at count=%d", + minCount, got[i].ResourceType, got[i].Count) + } + + require.Len(t, got, 1) + assert.Equal(t, "db.t3.medium", got[0].ResourceType) + assert.Equal(t, countPerRow, got[0].Count) + assert.Equal(t, countPerRow, CalculateTotalInstances(got)) + assert.Contains(t, out, "below --min-count 5") +} + +// TestCSVMinCountDropsRowsUnderTheFloor pins the floor with no cap in play: a +// CSV row that is already under --min-count is dropped, not purchased. The +// higher-value row is the one under the floor, so a selection that sorts by +// savings without gating on the floor still buys it. +func TestCSVMinCountDropsRowsUnderTheFloor(t *testing.T) { + isolateAWSEnv(t) + const minCount = 5 + + recs := csvRecsFrom(t, `rds,us-east-1,db.t3.small,postgres,2,500.00,1yr,All Upfront,123456789012 +rds,us-east-1,db.t3.medium,postgres,6,100.00,1yr,All Upfront,123456789012 +`) + + var ( + got []common.Recommendation + err error + ) + out := captureAppOutput(t, func() { + got, err = filterAndAdjustRecommendations(recs, 100.0, Config{MinCount: minCount}) + }) + require.NoError(t, err) + + require.Len(t, got, 1, "the row at count=2 is below --min-count 5 and must not be purchased") + assert.Equal(t, "db.t3.medium", got[0].ResourceType) + assert.Equal(t, 6, got[0].Count) + assert.Contains(t, out, "--min-count dropped") + assert.Contains(t, out, "db.t3.small") +} + +// TestRunToolFromCSVEnforcesMinCountAndCap drives the guards end to end +// through the real --input-csv entry point and asserts on the purchase report, +// in both directions: the guards bind when they should, and a legitimate run +// under the same flags still purchases every row at its full count. +func TestRunToolFromCSVEnforcesMinCountAndCap(t *testing.T) { + origCfg := toolCfg + t.Cleanup(func() { toolCfg = origCfg }) + isolateAWSEnv(t) + + // Worst-value row first, so file order and savings order disagree. + csvPath := writeTestRecommendationsCSV(t, csvFixtureHeader+ + `rds,us-east-1,db.t3.small,postgres,6,10.00,1yr,All Upfront,123456789012 +rds,us-east-1,db.t3.medium,postgres,6,500.00,1yr,All Upfront,123456789012 +rds,us-east-1,db.t3.large,postgres,6,100.00,1yr,All Upfront,123456789012 +`) + + tests := []struct { + name string + wantRows []string + maxInstances int32 + minCount int + }{ + { + // Budget 10 across rows of 6: the best row takes 6, the next is + // truncated to 4, and 4 is under the floor, so it is dropped. + // A file-ordered cap would instead buy db.t3.small at 6. + name: "cap and floor bind", + maxInstances: 10, + minCount: 5, + wantRows: []string{"db.t3.medium@6"}, + }, + { + // Same floor, cap above the natural total: nothing is dropped and + // every row is purchased at its CSV count. A guard that + // over-blocks is as wrong as one that never fires. + name: "legitimate run purchases every row", + maxInstances: 100, + minCount: 5, + wantRows: []string{"db.t3.small@6", "db.t3.medium@6", "db.t3.large@6"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reportPath := filepath.Join(t.TempDir(), "report.csv") + toolCfg.CSVInput = csvPath + toolCfg.CSVOutput = reportPath + toolCfg.ActualPurchase = false + toolCfg.Coverage = 100.0 + toolCfg.TargetCoverage = 0 + toolCfg.OverrideCount = 0 + toolCfg.MaxInstances = tt.maxInstances + toolCfg.MinCount = tt.minCount + + require.NoError(t, runToolFromCSV(context.Background(), toolCfg)) + + header, rows := readPurchaseReport(t, reportPath) + assert.ElementsMatch(t, tt.wantRows, reportRowPairs(t, header, rows)) + }) + } +} + +// reportRowPairs pairs each purchase-report row's ResourceType with its Count. +// Asserting the two columns independently would pass a result that put the +// right counts against the wrong rows, which is precisely what a wrong +// selection order produces. +func reportRowPairs(t *testing.T, header []string, rows [][]string) []string { + t.Helper() + types := reportColumn(t, header, rows, "ResourceType") + counts := reportColumn(t, header, rows, "Count") + require.Len(t, counts, len(types)) + + pairs := make([]string, 0, len(types)) + for i := range types { + pairs = append(pairs, types[i]+"@"+counts[i]) + } + return pairs +} + +// TestCSVCapRanksBySavingsPerInstance pins the ranking key the cap consumes. +// +// --max-instances is a budget in instances, so the rows worth keeping are the +// ones returning the most savings per instance bought. Ranking on the +// EstimatedSavings column directly ranks a whole-row dollar total against a +// per-instance budget: the fixture's bulk row is larger in total ($600) and +// worse per instance ($6) than the rich row ($500 total, $83 each), so a +// total-ranked cap spends the entire 10-instance budget on bulk for about $60 +// of real value and drops rich outright. +// +// Every earlier fixture in this file uses equal counts, where ranking by total +// and ranking by rate are indistinguishable. The counts here are deliberately +// unequal, which is the only way to tell the two rules apart. +func TestCSVCapRanksBySavingsPerInstance(t *testing.T) { + isolateAWSEnv(t) + const maxInstances = 10 + + recs := csvRecsFrom(t, `rds,us-east-1,db.bulk.large,postgres,100,600.00,1yr,All Upfront,123456789012 +rds,us-east-1,db.rich.large,postgres,6,500.00,1yr,All Upfront,123456789012 +`) + + var ( + got []common.Recommendation + err error + ) + out := captureAppOutput(t, func() { + got, err = filterAndAdjustRecommendations(recs, 100.0, Config{MaxInstances: maxInstances}) + }) + require.NoError(t, err) + + require.Equal(t, maxInstances, CalculateTotalInstances(got), + "the cap total is respected by both the right and the wrong ranking, and is asserted only to keep the fixture honest") + + // The substance: ORDER, and the count each row is bought at. The + // best-value row must be taken whole before the bulk row gets the + // remainder. + require.Len(t, got, 2) + assert.Equal(t, "db.rich.large", got[0].ResourceType, + "the budget must go to the highest savings-per-instance row first; got %s", got[0].ResourceType) + assert.Equal(t, 6, got[0].Count) + assert.Equal(t, "db.bulk.large", got[1].ResourceType) + assert.Equal(t, maxInstances-6, got[1].Count, "the bulk row takes only the remaining budget") + + assert.Contains(t, out, "savings-per-instance", + "the cap must name the ranking rule it applied, not claim a savings percentage a CSV row does not carry") +} + +// TestCSVCapRefusesRowsWithoutARankingSignal covers the absent-vs-zero trap. +// +// A CSV with no EstimatedSavings column, or with a blank cell in it, loads +// every affected row at zero savings. Nothing downstream can tell that apart +// from a row genuinely worth $0, so a binding cap would rank on a value that +// is not there and silently fall through to the Service|Region|ResourceType +// tie-break, buying by instance-type name while stdout promises it is buying +// by savings. The run is refused instead. +// +// Both directions are covered: the two refusal cases, a case proving the +// refusal is scoped to a cap that actually binds, and a legitimate file that +// still ranks and caps normally. +func TestCSVCapRefusesRowsWithoutARankingSignal(t *testing.T) { + isolateAWSEnv(t) + + t.Run("no EstimatedSavings column at all", func(t *testing.T) { + // Worst alphabetically first, so a name-ordered cap is visible: it + // keeps db.aaa.large and drops db.zzz.large regardless of file order. + recs := csvRecsFromHeader(t, csvNoSavingsHeader, + `rds,us-east-1,db.zzz.large,postgres,6,1yr,All Upfront,123456789012 +rds,us-east-1,db.aaa.large,postgres,6,1yr,All Upfront,123456789012 +`) + + var err error + captureAppOutput(t, func() { + _, err = filterAndAdjustRecommendations(recs, 100.0, Config{MaxInstances: 6}) + }) + + require.Error(t, err, "a binding cap with nothing to rank on must refuse, not pick by name") + assert.Contains(t, err.Error(), "EstimatedSavings") + assert.Contains(t, err.Error(), "db.zzz.large", "the error must name the rows it cannot rank") + assert.Contains(t, err.Error(), "db.aaa.large") + }) + + t.Run("one blank EstimatedSavings cell among populated rows", func(t *testing.T) { + // The partial case is the worse one: the blank row loads as $0, ranks + // last, and is dropped first, with nothing saying the value was absent + // rather than genuinely zero. + recs := csvRecsFrom(t, `rds,us-east-1,db.t3.medium,postgres,6,500.00,1yr,All Upfront,123456789012 +rds,us-east-1,db.t3.small,postgres,6,,1yr,All Upfront,123456789012 +`) + + var err error + captureAppOutput(t, func() { + _, err = filterAndAdjustRecommendations(recs, 100.0, Config{MaxInstances: 10}) + }) + + require.Error(t, err) + assert.Contains(t, err.Error(), "db.t3.small", "the blank row must be named") + assert.NotContains(t, err.Error(), "db.t3.medium", "a row with a real savings value is rankable and must not be blamed") + }) + + t.Run("no savings column but the cap does not bind", func(t *testing.T) { + // Nothing has to be chosen between, so nothing has to be ranked. A + // guard that over-blocks is as wrong as one that never fires. + recs := csvRecsFromHeader(t, csvNoSavingsHeader, + `rds,us-east-1,db.zzz.large,postgres,6,1yr,All Upfront,123456789012 +rds,us-east-1,db.aaa.large,postgres,6,1yr,All Upfront,123456789012 +`) + + var ( + got []common.Recommendation + err error + ) + captureAppOutput(t, func() { + got, err = filterAndAdjustRecommendations(recs, 100.0, Config{MaxInstances: 12}) + }) + + require.NoError(t, err) + assert.Len(t, got, 2) + assert.Equal(t, 12, CalculateTotalInstances(got)) + }) + + t.Run("populated savings column still ranks and caps", func(t *testing.T) { + recs := csvRecsFrom(t, `rds,us-east-1,db.t3.small,postgres,6,10.00,1yr,All Upfront,123456789012 +rds,us-east-1,db.t3.medium,postgres,6,500.00,1yr,All Upfront,123456789012 +`) + + var ( + got []common.Recommendation + err error + ) + captureAppOutput(t, func() { + got, err = filterAndAdjustRecommendations(recs, 100.0, Config{MaxInstances: 6}) + }) + + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, "db.t3.medium", got[0].ResourceType) + assert.Equal(t, 6, got[0].Count) + }) +} + +// TestApplyMinCountFloorAfterDuplicateAdjustment covers the floor's second +// call site, the one inside runToolFromCSV's purchase loop. +// +// adjustRecsForDuplicates subtracts recent matching commitments from Count +// after filterAndAdjustRecommendations has already applied the floor, so a row +// that cleared --min-count 5 at Count 6 with 5 existing commitments arrives at +// the purchase loop at Count 1. Without a second application the operator's +// floor is defeated on the very path #1741 is about. +// +// This exercises applyMinCountFloor on the post-deduction counts rather than +// through runToolFromCSV, because createServiceClient is not injectable: there +// is no seam to drive a fake GetExistingCommitments through the CSV entry +// point, and with invalid credentials adjustRecsForDuplicates errors and falls +// back to the unadjusted slice. +func TestApplyMinCountFloorAfterDuplicateAdjustment(t *testing.T) { + const minCount = 5 + + // Counts as adjustRecsForDuplicates leaves them: the first row had 6 and + // 5 existing commitments deducted, the second was untouched. + adjusted := []common.Recommendation{ + {Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.large", Count: 1, EstimatedSavings: 500}, + {Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.medium", Count: 6, EstimatedSavings: 100}, + } + + // Snapshot before the first call. applyMinCountFloor returns its input slice + // untouched at a floor of 0, and scorer.Score may reorder adjusted below, so + // asserting against adjusted itself would compare the result to itself. + original := append([]common.Recommendation(nil), adjusted...) + + var got []common.Recommendation + out := captureAppOutput(t, func() { + got = applyMinCountFloor(adjusted, minCount) + }) + + require.Len(t, got, 1, "the row the deduction left at 1 is below --min-count 5 and must not be purchased") + assert.Equal(t, "db.t3.medium", got[0].ResourceType) + assert.Equal(t, 6, got[0].Count) + assert.Contains(t, out, "db.t3.large", "the drop must be named") + assert.Contains(t, out, "--min-count") + + // A floor of 0 disables the flag: every row survives, in the order given. + var unfiltered []common.Recommendation + quiet := captureAppOutput(t, func() { + unfiltered = applyMinCountFloor(adjusted, 0) + }) + assert.Equal(t, original, unfiltered, "--min-count 0 must not drop or reorder anything") + assert.Empty(t, quiet) +} + +// TestCSVCapOrderIsIndependentOfFileOrder pins that equal-rate rows are not +// separated by where they sit in the file. +// +// --min-count 0 skips the scorer, so the per-instance sort is the only +// ordering the cap sees. If it left equal rates in input order, file order +// would still decide which of two equally-valuable rows the budget buys, which +// is the defect #1741 reported, surviving one layer down. +func TestCSVCapOrderIsIndependentOfFileOrder(t *testing.T) { + isolateAWSEnv(t) + + // Two rows at the identical rate ($100 for 6 instances), listed + // worst-alphabetically-first. + rows := []string{ + "rds,us-east-1,db.t3.zulu,postgres,6,100.00,1yr,All Upfront,123456789012\n", + "rds,us-east-1,db.t3.alpha,postgres,6,100.00,1yr,All Upfront,123456789012\n", + } + + selection := func(t *testing.T, rows []string) []string { + t.Helper() + recs := csvRecsFrom(t, rows[0]+rows[1]) + var ( + got []common.Recommendation + err error + ) + captureAppOutput(t, func() { + got, err = filterAndAdjustRecommendations(recs, 100.0, Config{MaxInstances: 6}) + }) + require.NoError(t, err) + + names := make([]string, 0, len(got)) + for i := range got { + names = append(names, got[i].ResourceType) + } + return names + } + + forward := selection(t, rows) + reversed := selection(t, []string{rows[1], rows[0]}) + + assert.Equal(t, []string{"db.t3.alpha"}, forward, + "equal rates must tie-break on Service|Region|ResourceType, not on file position") + assert.Equal(t, forward, reversed, "reversing the file must not change what the cap buys") +} diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index d696d319f..eb8ae8880 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -1615,10 +1615,14 @@ func TestFilterAndAdjustRecommendations(t *testing.T) { }, { name: "Instance limit applied", + // EstimatedSavings is set because a binding --max-instances has to + // rank the rows it chooses between; requireRankingSignal refuses a + // run whose rows carry no savings value rather than capping them by + // name. recommendations: []common.Recommendation{ - {Service: common.ServiceRDS, ResourceType: "db.t3.small", Count: 10}, - {Service: common.ServiceRDS, ResourceType: "db.t3.medium", Count: 10}, - {Service: common.ServiceRDS, ResourceType: "db.t3.large", Count: 10}, + {Service: common.ServiceRDS, ResourceType: "db.t3.small", Count: 10, EstimatedSavings: 100}, + {Service: common.ServiceRDS, ResourceType: "db.t3.medium", Count: 10, EstimatedSavings: 200}, + {Service: common.ServiceRDS, ResourceType: "db.t3.large", Count: 10, EstimatedSavings: 300}, }, coverage: 100.0, setupFilters: func() { @@ -1638,7 +1642,8 @@ func TestFilterAndAdjustRecommendations(t *testing.T) { // Suppress logger // Logger output disabled for testing - result := filterAndAdjustRecommendations(tt.recommendations, tt.coverage, toolCfg) + result, err := filterAndAdjustRecommendations(tt.recommendations, tt.coverage, toolCfg) + require.NoError(t, err) // Verify result is within expected range assert.GreaterOrEqual(t, len(result), tt.expectedMin) @@ -1812,10 +1817,14 @@ func TestRunToolFromCSV_WithMaxInstances(t *testing.T) { defer func() { toolCfg = origCfg }() isolateAWSEnv(t) - csvPath := writeTestRecommendationsCSV(t, `Service,Region,ResourceType,Engine,Count,Term,PaymentOption,Account -rds,us-east-1,db.t3.small,postgres,10,1yr,All Upfront,123456789012 -rds,us-east-1,db.t3.medium,mysql,10,1yr,All Upfront,123456789012 -rds,us-east-1,db.t3.large,postgres,10,1yr,All Upfront,123456789012 + // EstimatedSavings is populated because a binding --max-instances has to + // rank the rows it chooses between. A file without that column is refused + // rather than capped by name; that case is covered by + // TestCSVCapRefusesRowsWithoutARankingSignal. + csvPath := writeTestRecommendationsCSV(t, `Service,Region,ResourceType,Engine,Count,EstimatedSavings,Term,PaymentOption,Account +rds,us-east-1,db.t3.small,postgres,10,100.00,1yr,All Upfront,123456789012 +rds,us-east-1,db.t3.medium,mysql,10,200.00,1yr,All Upfront,123456789012 +rds,us-east-1,db.t3.large,postgres,10,300.00,1yr,All Upfront,123456789012 `) reportPath := filepath.Join(t.TempDir(), "report.csv") diff --git a/cmd/validators.go b/cmd/validators.go index 25634f49b..36b22ec1f 100644 --- a/cmd/validators.go +++ b/cmd/validators.go @@ -29,6 +29,10 @@ func validateFlags(cmd *cobra.Command, args []string) error { return err } + if err := validateCSVModeFilterFlags(); err != nil { + return err + } + if err := validateRecLookbackPeriod(); err != nil { return err } @@ -167,6 +171,34 @@ func warnRDS3YearNoUpfront() error { return nil } +// validateCSVModeFilterFlags refuses the scorer thresholds that --input-csv +// cannot enforce, instead of accepting them and buying as if they were unset. +// +// A recommendations CSV carries neither a savings-percentage column nor a +// break-even column (writeMultiServiceCSVReport emits neither and +// parseCSVRecord reads neither), so both fields load as zero on every row. +// Gating on a zero field would reject every row of every file, and skipping +// the gate silently drops a spend guard the operator believes is in force. +// Refusing the combination is the only honest option until #1819 teaches the +// format to carry the columns. +// +// Both flags default to 0, so this can only fire when one was set explicitly. +func validateCSVModeFilterFlags() error { + if toolCfg.CSVInput == "" { + return nil + } + + if toolCfg.MinSavingsPct > 0 { + return fmt.Errorf("--min-savings-pct cannot be applied to --input-csv runs: a recommendations CSV carries no savings-percentage column, so the threshold would be silently ignored (see #1819). Remove --min-savings-pct, or filter the CSV before passing it in") + } + + if toolCfg.MaxBreakEvenMonths > 0 { + return fmt.Errorf("--max-break-even-months cannot be applied to --input-csv runs: a recommendations CSV carries no break-even column, so the threshold would be silently ignored (see #1819). Remove --max-break-even-months, or filter the CSV before passing it in") + } + + return nil +} + // containsService checks if a service exists in the slice. func containsService(services []common.ServiceType, service common.ServiceType) bool { for _, svc := range services { diff --git a/cmd/validators_test.go b/cmd/validators_test.go index 979c02717..201e7bbd1 100644 --- a/cmd/validators_test.go +++ b/cmd/validators_test.go @@ -619,3 +619,70 @@ func TestValidateRecLookbackPeriod(t *testing.T) { }) } } + +// TestValidateCSVModeFilterFlags pins that --input-csv refuses the two scorer +// thresholds it cannot enforce, instead of accepting them and buying as if +// they were never set. +// +// A recommendations CSV carries neither a savings-percentage column nor a +// break-even column, so both fields load as zero on every row and any gate on +// them is either vacuous or rejects the whole file. #1741's complaint is +// exactly that silent partial enforcement of a spend guard is worse than not +// offering the guard. +func TestValidateCSVModeFilterFlags(t *testing.T) { + tests := []struct { + name string + csvInput string + errSubstr string + minSavingsPct float64 + maxBreakEvenMonths int + }{ + { + name: "min-savings-pct refused in CSV mode", + csvInput: "recs.csv", + minSavingsPct: 20, + errSubstr: "--min-savings-pct cannot be applied to --input-csv", + }, + { + name: "max-break-even-months refused in CSV mode", + csvInput: "recs.csv", + maxBreakEvenMonths: 12, + errSubstr: "--max-break-even-months cannot be applied to --input-csv", + }, + { + // Both flags default to 0, so an ordinary CSV run is unaffected. + name: "CSV mode without the thresholds is accepted", + csvInput: "recs.csv", + }, + { + // The thresholds are enforced normally off the CSV path. + name: "thresholds accepted without --input-csv", + minSavingsPct: 20, + maxBreakEvenMonths: 12, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + origCfg := toolCfg + defer func() { toolCfg = origCfg }() + toolCfg.CSVInput = tt.csvInput + toolCfg.MinSavingsPct = tt.minSavingsPct + toolCfg.MaxBreakEvenMonths = tt.maxBreakEvenMonths + + err := validateCSVModeFilterFlags() + if tt.errSubstr == "" { + if err != nil { + t.Errorf("validateCSVModeFilterFlags() unexpected error = %v", err) + } + return + } + if err == nil { + t.Fatalf("validateCSVModeFilterFlags() expected error containing %q, got nil", tt.errSubstr) + } + if !strings.Contains(err.Error(), tt.errSubstr) { + t.Errorf("validateCSVModeFilterFlags() error = %v, want substring %q", err, tt.errSubstr) + } + }) + } +} diff --git a/docs/cli/README.md b/docs/cli/README.md index 9e3feac7c..a3656f844 100644 --- a/docs/cli/README.md +++ b/docs/cli/README.md @@ -36,7 +36,7 @@ All flags belong to the root command unless noted otherwise. | `--target-coverage` | `-u` | `0` (disabled) | Target percentage (0-100) of historical average hourly usage to cover with commitments. Sizes each recommendation to `floor(avg * target/100)`, leaving the remainder on-demand. Overrides `--coverage` when non-zero. Pairs with `--rebuy-window-days` and `--min-pool-size` (see [filtering.md](filtering.md)). | | `--coverage-lookback-days` | | `30` | Calendar days of historical demand fed to `GetReservationCoverage` when computing the existing-RI coverage map for `--target-coverage` sizing. Match this to your AWS console coverage report window to reconcile cudly's `ExistingCoverage` column against the console export. Only affects `--target-coverage`. | | `--override-count` | | `0` (disabled) | Replace every recommendation's count with this fixed number. Useful when testing a specific purchase size. | -| `--max-instances` | | `0` (no limit) | Hard cap on the total number of instances purchased across all recommendations. Applied after coverage scaling, once per run across every service and region (not per service or per region). On default runs the survivors are the highest-savings recommendations run-wide; on `--input-csv` runs they are taken in file order. A recommendation the cap would truncate below `--min-count` is dropped instead. See [filtering.md](filtering.md). | +| `--max-instances` | | `0` (no limit) | Hard cap on the total number of instances purchased across all recommendations. Applied after coverage scaling, once per run across every service and region (not per service or per region). The cap is run-wide on both paths, but the two rank on different keys: recommendation-driven runs keep the highest savings *percentage*, while `--input-csv` rows are ranked on savings *per instance* (`EstimatedSavings / Count`), never on file order. A binding cap is refused if any row lacks an `EstimatedSavings` value to rank on. A recommendation the cap would truncate below `--min-count` is dropped instead. See [filtering.md](filtering.md). | ### Purchase terms diff --git a/docs/cli/filtering.md b/docs/cli/filtering.md index 6d3993ce8..97d4c21fe 100644 --- a/docs/cli/filtering.md +++ b/docs/cli/filtering.md @@ -134,6 +134,8 @@ Threshold filters drop individual recommendations that fall below a quality or s Drop recommendations where the adjusted instance count (after coverage scaling) is below this number. Useful for ignoring one-off or very small recommendations that are not worth the operational overhead. +The floor applies on both paths, including `--input-csv`: a row already below it is dropped, and a row `--max-instances` would truncate below it is dropped rather than purchased short. On `--input-csv` runs it is re-checked after existing matching commitments are deducted, so a row whose remaining count falls under the floor is dropped rather than purchased at the smaller size. + ```bash # Only purchase recommendations for 3 or more instances cudly --services rds --min-count 3 @@ -149,6 +151,8 @@ Drop recommendations whose estimated savings percentage falls below this thresho **Important naming distinction:** `--min-savings-pct` is a **percentage** value (e.g. `10` means "at least 10% savings"). This is different from the GUI and API `min_savings` parameter, which filters by **dollar amount**. A value of `30` in `--min-savings-pct` means "30% savings", but `min_savings=30` in the API means "$30 in savings". Mixing up the two by copying a CLI value into the GUI filter (or vice versa) produces silent, incorrect filtering. +**Refused on `--input-csv` runs.** A recommendations CSV carries no savings-percentage column, so this filter has nothing to read. Combining the two flags fails at startup rather than running with the threshold silently unenforced; drop the flag, or filter the CSV before passing it in. Teaching the CSV format to carry the column is tracked in [#1819](https://github.com/LeanerCloud/CUDly/issues/1819). + ```bash # Only recommendations with at least 20% projected savings cudly --services rds,elasticache --min-savings-pct 20 @@ -162,6 +166,8 @@ cudly --services rds,elasticache --min-savings-pct 20 Drop recommendations where the break-even period exceeds this many months. A break-even period is the number of months until the upfront cost of the RI is recovered by the recurring discount. Recommendations without a computable break-even period (e.g. no-upfront) pass through unfiltered. +**Refused on `--input-csv` runs.** A recommendations CSV carries no break-even column, so this filter has nothing to read. Combining the two flags fails at startup rather than running with the threshold silently unenforced; drop the flag, or filter the CSV before passing it in. Teaching the CSV format to carry the column is tracked in [#1819](https://github.com/LeanerCloud/CUDly/issues/1819). + ```bash # Only commitments that break even within 18 months cudly --services ec2 --max-break-even-months 18 @@ -180,9 +186,11 @@ The cap covers the whole run on both code paths: it is applied once to the combi Which recommendations survive depends on the path: - **Default (recommendation-driven) runs** apply the cap after scoring, so the surviving instances are the highest-savings-percentage ones across every service and region. Recommendations the cap reduces or drops are listed by name before the confirmation prompt and counted in the end-of-run drop summary, so a capped run never shrinks silently. -- **`--input-csv` runs** are not scored, so the cap consumes the file in row order and keeps rows from the top until the budget is exhausted. Order the CSV deliberately if you expect the cap to bind. +- **`--input-csv` runs** apply the same cap but rank on a different key, because a CSV row carries no savings percentage. Rows are ordered by **savings per instance** (`EstimatedSavings / Count`), so the budget buys the rows returning the most per instance rather than the rows whose dollar total happens to be largest, and file position is irrelevant. Reduced and dropped rows are named on stdout, alongside the ranking rule that selected them. + + Because that ordering is the only thing deciding what gets bought, a run whose cap actually binds is **refused** when any surviving row has no usable `EstimatedSavings` value. A blank cell, or a file written without the column at all, loads as `0` and is indistinguishable from a row genuinely worth $0: capping on it would silently select by instance-type name. Populate `EstimatedSavings` on every row, or drop `--max-instances` and cap the file itself. A cap that does not bind chooses nothing and is never refused. -If the cap would truncate a recommendation below `--min-count`, that recommendation is dropped rather than purchased at the smaller size, because `--min-count` is a floor rather than a preference. The freed budget is not reallocated to the next recommendation. +If the cap would truncate a recommendation below `--min-count`, that recommendation is dropped rather than purchased at the smaller size, because `--min-count` is a floor rather than a preference. The freed budget is not reallocated to the next recommendation. This holds on both paths. ```bash # Never purchase more than 100 instances in a single run