diff --git a/CHANGELOG.md b/CHANGELOG.md index 3623a02b6..4e1bd23a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). - `--target-coverage` with `--rebuy-window-days` no longer buys replacement EC2 and other non-RDS RIs again when they were already bought inside the coverage lookback: the expiring RIs are netted against those replacements (#2235). +- `--min-savings-pct` and `--max-break-even-months` now apply on `--input-csv` + runs instead of being refused. The CSV report gains `SavingsPercentage` and + `BreakEvenMonths` columns; replaying a report that lacks a column a set flag + needs (or has it blank) fails with an error naming the flag, never reading the + missing figure as 0. When present, the `SavingsPercentage` and + `BreakEvenMonths` columns are now parsed strictly (an invalid value fails the + load) even when the threshold flags are unset. `--max-instances` still ranks + on savings per instance (#1819). - Recommendations with no account ID or name are now reported in the drop summary when an account filter (`--include-accounts` or `--exclude-accounts`) removes them (#1613). diff --git a/cmd/multi_service.go b/cmd/multi_service.go index bdab60015..3f7cb312d 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -373,10 +373,11 @@ 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)" + // rankBySavingsPerInstance is the --input-csv path: the cap is a budget in + // instances, and a CSV may lack the savings percentage (older reports), so + // sortBySavingsPerInstance derives the rate from the EstimatedSavings and + // Count columns every CSV carries. + rankBySavingsPerInstance rankingRule = "highest savings-per-instance rows first (EstimatedSavings / Count, not savings percentage)" ) // capBinds reports whether --max-instances will actually remove instances from @@ -704,7 +705,7 @@ func prepareCSVPurchaseRun(ctx context.Context, cfg Config, csvModeCoverage floa } AppLogger.Printf("📄 Reading recommendations from CSV: %s\n", cfg.CSVInput) - recs, err = loadRecommendationsFromCSV(cfg.CSVInput) + recs, err = loadCSVRecommendations(cfg.CSVInput, csvThresholdNeedsFor(cfg)) if err != nil { return nil, aws.Config{}, "", fmt.Errorf("failed to read CSV file: %w", err) } diff --git a/cmd/multi_service_csv.go b/cmd/multi_service_csv.go index 6f553cfdf..7e99e0c73 100644 --- a/cmd/multi_service_csv.go +++ b/cmd/multi_service_csv.go @@ -33,6 +33,12 @@ func determineCSVCoverage(cfg Config) float64 { // loadRecommendationsFromCSV reads and returns recommendations from a CSV file. func loadRecommendationsFromCSV(csvPath string) ([]common.Recommendation, error) { + return loadCSVRecommendations(csvPath, csvThresholdNeeds{}) +} + +// loadCSVRecommendations is loadRecommendationsFromCSV for a run whose flags +// need threshold figures from the file (see csvThresholdNeeds). +func loadCSVRecommendations(csvPath string, need csvThresholdNeeds) ([]common.Recommendation, error) { file, err := os.Open(csvPath) // #nosec G304 -- CLI tool: csvPath is an operator-supplied command-line argument if err != nil { return nil, fmt.Errorf("failed to open CSV file: %w", err) @@ -58,9 +64,12 @@ func loadRecommendationsFromCSV(csvPath string) ([]common.Recommendation, error) if err != nil { return nil, err } + if colErr := requireThresholdColumns(colIdx, need); colErr != nil { + return nil, colErr + } // Parse all records - parsed, err := parseCSVRecords(reader, colIdx) + parsed, err := parseCSVRecords(reader, colIdx, need) if err != nil { return nil, err } @@ -125,7 +134,7 @@ func buildColumnIndexMap(header []string) (map[string]int, error) { } // parseCSVRecords reads and parses all CSV records. -func parseCSVRecords(reader *csv.Reader, colIdx map[string]int) ([]common.Recommendation, error) { +func parseCSVRecords(reader *csv.Reader, colIdx map[string]int, need csvThresholdNeeds) ([]common.Recommendation, error) { var recs []common.Recommendation for { @@ -149,7 +158,7 @@ func parseCSVRecords(reader *csv.Reader, colIdx map[string]int) ([]common.Recomm continue } - rec, err := parseCSVRecord(record, colIdx) + rec, err := parseCSVRecord(record, colIdx, need) if err != nil { line, _ := reader.FieldPos(0) return nil, fmt.Errorf("CSV line %d: %w", line, err) @@ -162,7 +171,7 @@ func parseCSVRecords(reader *csv.Reader, colIdx map[string]int) ([]common.Recomm } // parseCSVRecord parses a single CSV record into a Recommendation. -func parseCSVRecord(record []string, colIdx map[string]int) (common.Recommendation, error) { +func parseCSVRecord(record []string, colIdx map[string]int, need csvThresholdNeeds) (common.Recommendation, error) { rec := common.Recommendation{Provider: common.ProviderAWS} // Parse string fields @@ -193,6 +202,10 @@ func parseCSVRecord(record []string, colIdx map[string]int) (common.Recommendati return rec, err } + if cellErr := parseCSVThresholds(record, colIdx, need, &rec); cellErr != nil { + return rec, cellErr + } + // Details come from the Engine/Deployment columns and the appended detail // columns (see csvDetails). details, err := csvDetails(rec, record, colIdx) @@ -320,7 +333,7 @@ func writeMultiServiceCSV(w io.Writer, results []common.PurchaseResult) error { "ExistingCoverage", "ProjectedCoverage", } // Appended, never reordered: existing readers look columns up by name. - header := slices.Concat(baseHeader, csvDetailColumns) + header := slices.Concat(baseHeader, csvDetailColumns, csvThresholdColumns) if err := writer.Write(header); err != nil { return fmt.Errorf("failed to write CSV header: %w", err) } @@ -370,7 +383,7 @@ func writeMultiServiceCSV(w io.Writer, results []common.PurchaseResult) error { formatExistingCoverage(rec), formatPercentOrBlank(rec.ProjectedCoverage), } - row := slices.Concat(baseRow, detailCells(rec)) + row := slices.Concat(baseRow, detailCells(rec), thresholdCells(rec)) if err := writer.Write(row); err != nil { return fmt.Errorf("failed to write CSV row: %w", err) } @@ -432,7 +445,7 @@ func buildTotalRow(results []common.PurchaseResult) []string { "", "", "", "", // CommitmentID, Success, Error, Timestamp "", "", // ExistingCoverage, ProjectedCoverage } - return slices.Concat(totals, make([]string, len(csvDetailColumns))) // detail columns do not aggregate + return slices.Concat(totals, make([]string, len(csvDetailColumns)+len(csvThresholdColumns))) // detail and threshold columns do not aggregate } // formatIntOrBlank renders an int as its decimal string when non-zero, "" diff --git a/cmd/multi_service_csv_cap.go b/cmd/multi_service_csv_cap.go index d482779e9..3113fc6d9 100644 --- a/cmd/multi_service_csv_cap.go +++ b/cmd/multi_service_csv_cap.go @@ -20,26 +20,23 @@ import ( // --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. +// --min-savings-pct and --max-break-even-months are enforced here too, on the +// SavingsPercentage and BreakEvenMonths columns the report writer emits. The +// loader refuses a file that lacks a column a set flag needs (see +// csvThresholdNeeds), so a gate never reads a coerced zero. // -// 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. +// scorer.Score's own ordering is useless here: it sorts on SavingsPercentage and +// then 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) + passed := applyThresholdGates(applyMinCountFloor(recs, cfg.MinCount), cfg) if err := requireRankingSignal(passed, cfg); err != nil { return nil, err } @@ -68,6 +65,23 @@ func applyMinCountFloor(recs []common.Recommendation, minCount int) []common.Rec return scored.Passed } +// applyThresholdGates drops recommendations that fail --min-savings-pct or +// --max-break-even-months and names each drop on stdout with scorer.Score's own +// reason, so the CSV path and the recommendation-driven path reject on the +// identical predicates. With both at 0 it returns recs untouched. +func applyThresholdGates(recs []common.Recommendation, cfg Config) []common.Recommendation { + if cfg.MinSavingsPct <= 0 && cfg.MaxBreakEvenMonths <= 0 { + return recs + } + scored := scorer.Score(recs, scorer.Config{MinSavingsPct: cfg.MinSavingsPct, MaxBreakEvenMonths: cfg.MaxBreakEvenMonths}) + for i := range scored.Filtered { + f := scored.Filtered[i] + AppLogger.Printf("🔒 --min-savings-pct/--max-break-even-months 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 diff --git a/cmd/multi_service_csv_thresholds.go b/cmd/multi_service_csv_thresholds.go new file mode 100644 index 000000000..df2ed48e9 --- /dev/null +++ b/cmd/multi_service_csv_thresholds.go @@ -0,0 +1,90 @@ +package main + +import ( + "fmt" + "math" + "strconv" + "strings" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +// csvThresholdColumns carry the two figures --min-savings-pct and +// --max-break-even-months gate on, so a report replayed with --input-csv can +// be filtered the way a live run is. They are appended after csvDetailColumns, +// never reordered: readers look columns up by name. +// +// SavingsPercentage is a percent (25.5 means 25.5%), the unit of the flag and +// of common.Recommendation.SavingsPercentage. BreakEvenMonths is months. +// Both are written with the shortest exact decimal, so a write then a read +// yields the same float64. +const ( + csvSavingsPctColumn = "SavingsPercentage" + csvBreakEvenColumn = "BreakEvenMonths" +) + +var csvThresholdColumns = []string{csvSavingsPctColumn, csvBreakEvenColumn} + +// csvThresholdNeeds names the threshold figures a run reads from the CSV. +// A figure that is needed but absent fails the load: common.Recommendation +// holds both as plain float64, so an absent cell would read as 0, which the +// savings gate rejects and the break-even gate passes. +type csvThresholdNeeds struct { + savingsPct bool + breakEven bool +} + +func csvThresholdNeedsFor(cfg Config) csvThresholdNeeds { + return csvThresholdNeeds{savingsPct: cfg.MinSavingsPct > 0, breakEven: cfg.MaxBreakEvenMonths > 0} +} + +// thresholdCells renders rec for csvThresholdColumns, in that order. The +// break-even cell is blank when it is unknown or never pays back; "0" is a +// real value (no upfront payment). +func thresholdCells(rec common.Recommendation) []string { + cells := []string{strconv.FormatFloat(rec.SavingsPercentage, 'f', -1, 64), ""} + if months, known := common.BreakEven(rec); known && !math.IsInf(months, 0) { + cells[1] = strconv.FormatFloat(months, 'f', -1, 64) + } + return cells +} + +// requireThresholdColumns refuses a CSV whose header lacks a column a set flag +// needs (a report written before the columns existed). +func requireThresholdColumns(colIdx map[string]int, need csvThresholdNeeds) error { + if need.savingsPct { + if _, ok := colIdx[csvSavingsPctColumn]; !ok { + return missingThresholdColumn(csvSavingsPctColumn, "--min-savings-pct") + } + } + if need.breakEven { + if _, ok := colIdx[csvBreakEvenColumn]; !ok { + return missingThresholdColumn(csvBreakEvenColumn, "--max-break-even-months") + } + } + return nil +} + +func missingThresholdColumn(column, flag string) error { + return fmt.Errorf("CSV has no %q column, so %s cannot be applied to it (reports written before #1819 lack it). Regenerate the report with this version, add the column, or drop %s", column, flag, flag) +} + +// parseCSVThresholds reads the threshold cells of one row into rec. A cell is +// parsed whenever it holds a value, and a blank one is an error only when the +// matching flag needs it. +func parseCSVThresholds(record []string, colIdx map[string]int, need csvThresholdNeeds, rec *common.Recommendation) error { + if err := parseThresholdCell(record, colIdx, csvSavingsPctColumn, need.savingsPct, "--min-savings-pct", &rec.SavingsPercentage); err != nil { + return err + } + return parseThresholdCell(record, colIdx, csvBreakEvenColumn, need.breakEven, "--max-break-even-months", &rec.BreakEvenMonths) +} + +func parseThresholdCell(record []string, colIdx map[string]int, column string, required bool, flag string, target *float64) error { + if strings.TrimSpace(getCSVField(record, colIdx, column)) == "" { + if required { + return fmt.Errorf("column %q is blank, so %s cannot be applied to this row", column, flag) + } + return nil + } + return parseCSVFloat(record, colIdx, column, target) +} diff --git a/cmd/multi_service_csv_thresholds_test.go b/cmd/multi_service_csv_thresholds_test.go new file mode 100644 index 000000000..7049c0f47 --- /dev/null +++ b/cmd/multi_service_csv_thresholds_test.go @@ -0,0 +1,201 @@ +package main + +import ( + "bytes" + "context" + "math" + "os" + "os/exec" + "path/filepath" + "sort" + "strings" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + csvThresholdArgsEnv = "CUDLY_CSV_THRESHOLD_TEST_ARGS" + csvThresholdCSVEnv = "CUDLY_CSV_THRESHOLD_TEST_CSV" + csvThresholdWantEnv = "CUDLY_CSV_THRESHOLD_TEST_WANT" +) + +const thresholdCSVHeader = "Service,Region,ResourceType,Engine,Count,Term,PaymentOption,Account,EstimatedSavings,SavingsPercentage,BreakEvenMonths\n" + +var thresholdCSVBodies = map[string]string{ + "full": thresholdCSVHeader + + "rds,us-east-1,db.t3.small,postgres,2,1yr,all-upfront,111111111111,100,10,3\n" + + "rds,us-east-1,db.t3.medium,postgres,2,1yr,all-upfront,111111111111,200,30,14\n" + + "rds,us-east-1,db.t3.large,postgres,2,1yr,all-upfront,111111111111,300,50,8\n", + "old": "Service,Region,ResourceType,Engine,Count,Term,PaymentOption,Account,EstimatedSavings\n" + + "rds,us-east-1,db.t3.small,postgres,2,1yr,all-upfront,111111111111,100\n", + "blank": thresholdCSVHeader + + "rds,us-east-1,db.t3.small,postgres,2,1yr,all-upfront,111111111111,100,,\n", +} + +// TestCSVThresholdFlagsThroughRootCommand drives --min-savings-pct and +// --max-break-even-months through the real command (cobra, validateFlags, +// runTool, the --input-csv dry run) for #1819. Each case runs in a child +// process so cobra's flag state never leaks between cases. +func TestCSVThresholdFlagsThroughRootCommand(t *testing.T) { + if args, ok := os.LookupEnv(csvThresholdArgsEnv); ok { + runCSVThresholdChild(t, args, os.Getenv(csvThresholdCSVEnv), os.Getenv(csvThresholdWantEnv)) + return + } + tests := []struct { + name string + csv string + args []string + want string + }{ + {"no thresholds keeps every row", "full", nil, "ok:db.t3.large,db.t3.medium,db.t3.small"}, + {"min-savings-pct filters rows below it", "full", []string{"--min-savings-pct", "25"}, "ok:db.t3.large,db.t3.medium"}, + {"max-break-even-months filters rows above it", "full", []string{"--max-break-even-months", "10"}, "ok:db.t3.large,db.t3.small"}, + {"both thresholds combine", "full", []string{"--min-savings-pct", "25", "--max-break-even-months", "10"}, "ok:db.t3.large"}, + {"older CSV without the columns runs when no flag needs them", "old", nil, "ok:db.t3.small"}, + {"older CSV with --min-savings-pct is refused", "old", []string{"--min-savings-pct", "25"}, `err:no "SavingsPercentage" column`}, + {"older CSV with --max-break-even-months is refused", "old", []string{"--max-break-even-months", "10"}, `err:no "BreakEvenMonths" column`}, + {"blank cell with --min-savings-pct is refused", "blank", []string{"--min-savings-pct", "25"}, `err:column "SavingsPercentage" is blank`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + child := exec.CommandContext(ctx, os.Args[0], "-test.run=^TestCSVThresholdFlagsThroughRootCommand$", "-test.v") + child.Env = []string{ + csvThresholdArgsEnv + "=" + strings.Join(tt.args, "\x1f"), + csvThresholdCSVEnv + "=" + tt.csv, + csvThresholdWantEnv + "=" + tt.want, + } + output, err := child.CombinedOutput() + if substr, isErr := strings.CutPrefix(tt.want, "err:"); isErr { + // runTool reports a refused load with log.Fatalf, so the run + // ends the process non-zero before any report is written. + require.Error(t, err, "%s", output) + assert.Contains(t, string(output), substr) + return + } + require.NoError(t, err, "%s", output) + assert.Contains(t, string(output), "--- PASS: TestCSVThresholdFlagsThroughRootCommand") + }) + } +} + +func runCSVThresholdChild(t *testing.T, joinedArgs, csvKey, want string) { + isolateAWSEnv(t) + dir := t.TempDir() + csvPath := writeTestRecommendationsCSV(t, thresholdCSVBodies[csvKey]) + reportPath := filepath.Join(dir, "report.csv") + args := []string{"--input-csv", csvPath, "--output", reportPath, "--audit-log", filepath.Join(dir, "audit.jsonl")} + if joinedArgs != "" { + args = append(args, strings.Split(joinedArgs, "\x1f")...) + } + rootCmd.SetArgs(args) + err := rootCmd.Execute() + require.NoError(t, err) + header, rows := readPurchaseReport(t, reportPath) + got := reportColumn(t, header, rows, "ResourceType") + sort.Strings(got) + assert.Equal(t, strings.Split(strings.TrimPrefix(want, "ok:"), ","), got) +} + +// TestCSVThresholdColumnsRoundTrip pins that the writer emits both columns and +// the parser reads back exactly what was written. +func TestCSVThresholdColumnsRoundTrip(t *testing.T) { + recs := []common.Recommendation{ + {Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.small", Count: 2, Term: "1yr", PaymentOption: "all-upfront", + CommitmentCost: 1200, EstimatedSavings: 50, SavingsPercentage: 33.333333333333336}, + {Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.medium", Count: 1, Term: "1yr", PaymentOption: "no-upfront", + EstimatedSavings: 20, SavingsPercentage: 12.5}, + } + results := make([]common.PurchaseResult, len(recs)) + for i, r := range recs { + results[i] = common.PurchaseResult{Recommendation: r, Success: true, Timestamp: time.Unix(0, 0).UTC()} + } + var buf bytes.Buffer + require.NoError(t, writeMultiServiceCSV(&buf, results)) + + path := writeTestRecommendationsCSV(t, buf.String()) + loaded, err := loadCSVRecommendations(path, csvThresholdNeeds{savingsPct: true, breakEven: true}) + require.NoError(t, err) + byType := map[string]common.Recommendation{} + for _, r := range loaded { + byType[r.ResourceType] = r + } + assert.Equal(t, 33.333333333333336, byType["db.t3.small"].SavingsPercentage, "exact decimal round trip") + assert.Equal(t, 8.0, byType["db.t3.small"].BreakEvenMonths) + assert.Equal(t, 12.5, byType["db.t3.medium"].SavingsPercentage) + assert.Zero(t, byType["db.t3.medium"].BreakEvenMonths, "no upfront breaks even at 0 months") + + header := strings.Split(strings.SplitN(buf.String(), "\n", 2)[0], ",") + assert.Equal(t, csvThresholdColumns, header[len(header)-len(csvThresholdColumns):], "threshold columns are appended last") +} + +// TestThresholdCellsBlankWhenBreakEvenUnknownOrInfinite pins that a break-even +// that cannot be stated as a finite number is written blank (absent), never as +// "+Inf" or a fabricated 0. +func TestThresholdCellsBlankWhenBreakEvenUnknownOrInfinite(t *testing.T) { + never := common.Recommendation{Term: "1yr", CommitmentCost: 1200, EstimatedSavings: -100, SavingsPercentage: 1} + months, known := common.BreakEven(never) + require.True(t, known) + require.True(t, math.IsInf(months, 1)) + assert.Equal(t, []string{"1", ""}, thresholdCells(never)) + + badTerm := common.Recommendation{Term: "5yr", CommitmentCost: 1200, EstimatedSavings: 50} + assert.Equal(t, []string{"0", ""}, thresholdCells(badTerm)) +} + +// TestCSVThresholdHeaderStaysStrict pins that carrying the new columns did not +// loosen the required-column check. +func TestCSVThresholdHeaderStaysStrict(t *testing.T) { + _, err := loadCSVRecommendations(writeTestRecommendationsCSV(t, + "Service,Region,ResourceType,Term,PaymentOption,SavingsPercentage,BreakEvenMonths\nrds,us-east-1,db.t3.small,1yr,all-upfront,10,3\n"), + csvThresholdNeeds{savingsPct: true, breakEven: true}) + require.Error(t, err) + assert.Contains(t, err.Error(), "CSV header missing required columns: Count") +} + +// TestCSVThresholdWhitespaceCellIsBlank pins that a whitespace-only cell is +// blank: refused when its flag is set, ignored (left 0) when it is not. +func TestCSVThresholdWhitespaceCellIsBlank(t *testing.T) { + const row = "rds,us-east-1,db.t3.small,postgres,2,1yr,all-upfront,111111111111,100,%s,%s\n" + for _, blank := range []string{" ", "\t"} { + for _, col := range []struct { + name, flag string + cells [2]string + need csvThresholdNeeds + }{ + {csvSavingsPctColumn, "--min-savings-pct", [2]string{blank, "3"}, csvThresholdNeeds{savingsPct: true}}, + {csvBreakEvenColumn, "--max-break-even-months", [2]string{"10", blank}, csvThresholdNeeds{breakEven: true}}, + } { + path := writeTestRecommendationsCSV(t, thresholdCSVHeader+strings.Replace(strings.Replace(row, "%s", col.cells[0], 1), "%s", col.cells[1], 1)) + + _, err := loadCSVRecommendations(path, col.need) + require.Error(t, err, "%q in %s", blank, col.name) + assert.Contains(t, err.Error(), col.name) + assert.Contains(t, err.Error(), col.flag) + assert.Contains(t, err.Error(), "CSV line 2") + + recs, err := loadCSVRecommendations(path, csvThresholdNeeds{}) + require.NoError(t, err, "%q in %s with no flag", blank, col.name) + require.Len(t, recs, 1) + } + } +} + +// TestCSVThresholdOrderingStaysOnSavingsPerInstance pins that populating +// SavingsPercentage does not re-rank --max-instances: the cap still buys the +// best savings-per-instance row, not the highest percentage. +func TestCSVThresholdOrderingStaysOnSavingsPerInstance(t *testing.T) { + recs := []common.Recommendation{ + {Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "high-pct", Count: 4, EstimatedSavings: 40, SavingsPercentage: 90}, + {Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "high-rate", Count: 4, EstimatedSavings: 400, SavingsPercentage: 20}, + } + got, err := scoreAndLimitCSVRecs(recs, Config{MaxInstances: 4, MinSavingsPct: 10}) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, "high-rate", got[0].ResourceType) +} diff --git a/cmd/multi_service_max_instances_test.go b/cmd/multi_service_max_instances_test.go index 1dff4d972..3ffdf31e8 100644 --- a/cmd/multi_service_max_instances_test.go +++ b/cmd/multi_service_max_instances_test.go @@ -466,7 +466,7 @@ func csvRecsFromHeader(t *testing.T, header, rows string) []common.Recommendatio 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") + "SavingsPercentage must stay zero here; the ordering under test ranks on EstimatedSavings / Count") } return recs } diff --git a/cmd/savings_plan_purchase_test.go b/cmd/savings_plan_purchase_test.go index 998bf24f1..aa5cfc76a 100644 --- a/cmd/savings_plan_purchase_test.go +++ b/cmd/savings_plan_purchase_test.go @@ -397,8 +397,9 @@ func TestCSVWriter_AppendsDetailColumnsWithoutReorderingExistingOnes(t *testing. rows, err := csv.NewReader(f).ReadAll() require.NoError(t, err) header := rows[0] - n := len(header) - len(csvDetailColumns) - assert.Equal(t, csvDetailColumns, header[n:]) + n := len(header) - len(csvDetailColumns) - len(csvThresholdColumns) + assert.Equal(t, csvDetailColumns, header[n:n+len(csvDetailColumns)]) + assert.Equal(t, csvThresholdColumns, header[n+len(csvDetailColumns):]) assert.Equal(t, "Service", header[0]) assert.Equal(t, "ProjectedCoverage", header[n-1]) for _, row := range rows { diff --git a/cmd/validators.go b/cmd/validators.go index 3ec27991d..66ce09e23 100644 --- a/cmd/validators.go +++ b/cmd/validators.go @@ -35,10 +35,6 @@ func validateFlags(cmd *cobra.Command, args []string) error { return err } - if err := validateCSVModeFilterFlags(); err != nil { - return err - } - if err := validateRecLookbackPeriod(); err != nil { return err } @@ -251,34 +247,6 @@ 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 2bb7b8660..24f6d157c 100644 --- a/cmd/validators_test.go +++ b/cmd/validators_test.go @@ -675,73 +675,6 @@ 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) - } - }) - } -} - // TestValidateIdempotencyWindow covers #1262: the flag used to be accepted // unparsed, so "banana" or "90m" ran silently with a hardcoded 24h window. func TestValidateIdempotencyWindow(t *testing.T) { diff --git a/docs/cli/filtering.md b/docs/cli/filtering.md index c8c4206a8..3d31ec06b 100644 --- a/docs/cli/filtering.md +++ b/docs/cli/filtering.md @@ -157,7 +157,7 @@ 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). +**On `--input-csv` runs** the filter reads the `SavingsPercentage` column of the report (a percent, so `25.5` is 25.5%), which the CSV report writer emits. A CSV without that column (a report written by an older version), or a row with that cell blank, fails the run with an error naming the flag instead of treating the missing figure as 0%. Add the column or drop the flag. Dropped rows are named on stdout. ```bash # Only recommendations with at least 20% projected savings @@ -179,7 +179,7 @@ break-even months = upfront / gross monthly savings `EstimatedSavings` is already net of the amortized upfront, so the term amortization is added back before dividing. A provider-supplied break-even wins over the formula. No-upfront recommendations (upfront 0) have a break-even of 0 and always pass. A recommendation with an upfront payment whose break-even cannot be computed (negative or non-finite money, or a term other than 1 or 3 years) is dropped, with the reason in the drop summary, because the limit cannot be enforced on it. If gross monthly savings are not positive (net `EstimatedSavings` at or below `-upfront / term months`) the break-even is infinite and the recommendation is dropped. Net savings of 0 give a break-even equal to the term. -**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). +**On `--input-csv` runs** the filter reads the `BreakEvenMonths` column of the report (months; `0` for a no-upfront row), which the CSV report writer emits. A blank cell means the writer could not state a finite break-even. A CSV without the column (a report written by an older version), or a row with that cell blank, fails the run with an error naming the flag instead of treating the missing figure as passing. Add the column or drop the flag. Dropped rows are named on stdout. ```bash # Only commitments that break even within 18 months @@ -199,7 +199,7 @@ 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** 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. +- **`--input-csv` runs** apply the same cap but rank on a different key, because the cap is a budget in instances and older CSVs carry no savings percentage. Rows are ordered by **savings per instance** (`EstimatedSavings / Count`), whatever `SavingsPercentage` holds, 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.