From 95b78ccb1165b79043030756915dea39c74d47fd Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 18 Aug 2026 15:24:10 +0200 Subject: [PATCH 1/2] fix(cli): rescale count-derived money when --override-count replaces a row's count (#1844) ApplyCountOverride replaced a recommendation's Count and copied the rest of the struct wholesale, so every quantity derived from that count kept the value it had for the count the provider proposed. A row entering with Count=100 and EstimatedSavings=600 left as count=5 savings=600, a twentyfold overstatement. It runs upstream of --max-instances on both the default and the --input-csv path, so the cap's own rescale then computed its ratio against a corrupted base and preserved the error faithfully into the run summary, the purchase report, the CSV and the confirmation prompt. The four count-derived money fields now scale by the override ratio through common.ScaleRecommendationCosts, the same helper ApplyInstanceLimit and the coverage paths already use, rather than a second implementation of the same arithmetic. RecurringMonthlyCost is a *float64 and a nil stays nil. A row with a non-positive Count is left alone: there is no denominator to form a ratio from, and setting Count without scaling would reintroduce the same defect. Chaining the override with the cap now composes two ratios into a single net ratio against the provider's figures instead of scaling a wrong base. Savings Plans are exempted, Count included. Routing them through the helper would have scaled SavingsPlanDetails.HourlyCommitment by an instance count, and that field is what the SP purchase call actually buys, so --override-count 10 would have committed ten times the dollars. An SP commitment is priced in dollars per hour rather than in instances, its Count is a placeholder the parser pins at 1, and the flag is documented as an override for the selected RIs. Skipping also stops an SP consuming N units of the --max-instances budget for what is one commitment. On the scale-up direction the issue left open: unit prices are linear, so CommitmentCost stays true at any quantity, but savings only accrue on hours a matching instance actually runs, so units beyond the observed demand are billed and may save nothing. Not scaling is not the safer answer, it reports the money of a different quantity and understates what will be charged. Rows are therefore scaled and the extrapolation is named on stdout, against RecommendedCount where the provider recorded one and the row's own count otherwise. Skipped SPs and skipped non-positive rows are named too. Eight of the nine new tests fail pre-fix, none by panic, and the pre-existing table test passes unchanged on both sides. Each was mutation-verified individually against the committed fix and every failure is again by assertion: dropping the rescale fails the down and up directions, the caller aliasing test that reportInstanceLimit's pre-cap diff depends on, and the composition test; removing the SP exemption fails the SP and skip-reporting tests, which is what proves both directions rather than only the rescaling one; removing the non-positive guard yields +Inf and fails that guard and the skip report; making the extrapolation boundary ignore RecommendedCount fires the warning on an override back up to the provider's own proposal; coercing the nil monthly cost to zero fails with Expected nil, but got a pointer. The nil-RecurringMonthlyCost test is the one that passes pre-fix, being a directional guard against the fix fabricating a zero rather than a regression test, stated so the coverage is not overread. ProjectedCoverage and ProjectedUtilization are count-linear and still not re-derived here, exactly as after a cap (#1845). The stale justification on the #1830 SP truncation test is corrected in the same change: that test's route was --override-count, which no longer reaches it, and the live route is the --input-csv path building Service from the CSV column. Closes #1844 --- cmd/helpers.go | 82 +++++- cmd/helpers_count_override_rescale_test.go | 309 +++++++++++++++++++++ cmd/helpers_instance_limit_rescale_test.go | 15 +- docs/cli/README.md | 2 +- 4 files changed, 394 insertions(+), 14 deletions(-) create mode 100644 cmd/helpers_count_override_rescale_test.go diff --git a/cmd/helpers.go b/cmd/helpers.go index de861a554..b19dcc281 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -492,25 +492,93 @@ func applySizing(recs []common.Recommendation, cfg Config, coverage float64, dro return applyCoverage(recs, coverage, drops) } -// ApplyCountOverride overrides the count for all recommendations. +// ApplyCountOverride replaces the count on every count-denominated +// recommendation with overrideCount, rescaling the money that count derives so +// the row describes the quantity the run will actually buy. // -// It does NOT rescale the count-derived money fields, so an overridden row -// still carries the savings of the count the provider proposed. That is the -// same defect ApplyInstanceLimit had before #1830, on a neighboring flag, -// and it runs upstream of the cap. Tracked by #1844. +// EstimatedSavings, CommitmentCost, OnDemandCost and RecurringMonthlyCost are +// whole-row totals for the count the provider proposed, so replacing Count +// alone leaves the row claiming the money of a quantity nobody will buy +// (#1844). The scaling goes through common.ScaleRecommendationCosts, the same +// helper ApplyInstanceLimit and the coverage paths use, so the arithmetic +// cannot drift between the flags. ProjectedCoverage / ProjectedUtilization are +// count-linear but not re-derived here, exactly as after a cap (#1845). +// +// Savings Plans are left entirely alone, Count included. The flag is +// documented as an override for "all selected RIs", an SP commitment is +// dollar-denominated rather than count-denominated, its Count is a fixed +// placeholder 1 set by the parser, and the SP purchase call reads +// HourlyCommitment rather than Count. Scaling an SP by an instance count would +// multiply the dollars actually committed by an unrelated number. +// +// A row with a non-positive Count is left alone for the same reason +// ApplyInstanceLimit never rescales one: there is no denominator to form a +// ratio from, and setting Count without scaling would reintroduce precisely +// the misstatement above. +// +// Scaling down is unambiguous. Scaling up past the quantity the provider's own +// figures cover is an extrapolation: unit prices are linear so CommitmentCost +// stays true, but savings only accrue on hours a matching resource actually +// runs, so units beyond the observed demand cost money and may save nothing. +// Those rows are reported rather than passed off as measured savings. +// +// Both call sites run this before the run-wide --max-instances cap, so a run +// using both flags scales once from the override ratio and once from the cap +// ratio, which compose to a single net ratio against the provider's figures. func ApplyCountOverride(recs []common.Recommendation, overrideCount int32) []common.Recommendation { if overrideCount <= 0 { return recs } result := make([]common.Recommendation, len(recs)) + var skippedSP, skippedNonPositive, extrapolated int for i := range recs { rec := recs[i] - result[i] = rec - result[i].Count = int(overrideCount) + switch { + case common.IsSavingsPlan(rec.Service): + result[i] = rec + skippedSP++ + case rec.Count <= 0: + result[i] = rec + skippedNonPositive++ + default: + result[i] = common.ScaleRecommendationCosts(rec, float64(overrideCount)/float64(rec.Count)) + result[i].Count = int(overrideCount) + if int(overrideCount) > evidencedCount(rec) { + extrapolated++ + } + } } + reportCountOverride(overrideCount, skippedSP, skippedNonPositive, extrapolated) return result } +// evidencedCount is the largest count a recommendation's money figures are +// evidence for: the provider's own pre-sizing proposal when it recorded one, +// otherwise the count the row currently carries. RecommendedCount is populated +// only on the AWS RI path, so the fallback is what the --input-csv and +// non-AWS paths use. +func evidencedCount(rec common.Recommendation) int { + if rec.RecommendedCount > rec.Count { + return rec.RecommendedCount + } + return rec.Count +} + +// reportCountOverride names what --override-count could not honor and what it +// honored only by extrapolation. Nothing here is fatal, but every case would +// otherwise change or fail to change a money figure without telling anyone. +func reportCountOverride(overrideCount int32, skippedSP, skippedNonPositive, extrapolated int) { + if skippedSP > 0 { + AppLogger.Printf("⚠️ --override-count left %d Savings Plans recommendation(s) unchanged: an SP commitment is priced in dollars per hour, not in instances, so an instance count cannot size it.\n", skippedSP) + } + if skippedNonPositive > 0 { + AppLogger.Printf("⚠️ --override-count left %d recommendation(s) with a non-positive count unchanged: there is no quantity to rescale their costs from.\n", skippedNonPositive) + } + if extrapolated > 0 { + AppLogger.Printf("⚠️ --override-count=%d exceeds the quantity the provider's figures cover on %d recommendation(s). Their costs scale with the count and stay accurate, but the savings are extrapolated past the observed demand: instances beyond it are billed and may save nothing.\n", overrideCount, extrapolated) + } +} + // ApplyInstanceLimit truncates recs so their total Count does not exceed // maxInstances. It is a single-shot cap over whatever slice it is handed: the // caller is responsible for handing it the complete run-wide set, because diff --git a/cmd/helpers_count_override_rescale_test.go b/cmd/helpers_count_override_rescale_test.go new file mode 100644 index 000000000..1871aa60d --- /dev/null +++ b/cmd/helpers_count_override_rescale_test.go @@ -0,0 +1,309 @@ +package main + +import ( + "math" + "testing" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// countDenominatedRec is the fixture the override tests scale. Every money +// field is a whole-row total for Count, which is exactly the property +// --override-count has to preserve. +func countDenominatedRec(count int) common.Recommendation { + return common.Recommendation{ + Service: common.ServiceEC2, + Region: "us-east-1", + ResourceType: "m5.large", + Count: count, + RecommendedCount: count, + EstimatedSavings: 600, + CommitmentCost: 2400, + OnDemandCost: 3000, + SavingsPercentage: 20, + RecurringMonthlyCost: float64Ptr(200), + } +} + +// assertScaledBy checks that every extensive money field of got is in's value +// times ratio, and that the intensive ones are untouched. The assertion is the +// ratio rather than a literal figure, so it keeps its meaning if the fixture's +// dollar values change. +func assertScaledBy(t *testing.T, in, got common.Recommendation, ratio float64) { + t.Helper() + + assert.InDelta(t, in.EstimatedSavings*ratio, got.EstimatedSavings, 0.0001, + "EstimatedSavings is a whole-row total and must scale with the overridden Count") + assert.InDelta(t, in.CommitmentCost*ratio, got.CommitmentCost, 0.0001, + "CommitmentCost is a whole-row total and must scale with the overridden Count") + assert.InDelta(t, in.OnDemandCost*ratio, got.OnDemandCost, 0.0001, + "OnDemandCost is a whole-row total and must scale with the overridden Count") + + require.NotNil(t, got.RecurringMonthlyCost, + "a present monthly breakdown must stay present after the override") + assert.InDelta(t, *in.RecurringMonthlyCost*ratio, *got.RecurringMonthlyCost, 0.0001, + "RecurringMonthlyCost is a whole-row total and must scale with the overridden Count") + + // SavingsPercentage is a ratio of two figures that scale together, so it is + // invariant. Scaling it would be the mirror-image bug. + assert.InDelta(t, in.SavingsPercentage, got.SavingsPercentage, 0.0001, + "SavingsPercentage is intensive and must not be scaled") + + // RecommendedCount is a frozen record of the provider's own proposal. The + // override replaces what we will buy, not what the provider proposed. + assert.Equal(t, in.RecommendedCount, got.RecommendedCount, + "RecommendedCount records the provider's proposal and must survive the override") +} + +// TestApplyCountOverrideRescalesDownscaledRow pins the #1844 invariant in the +// direction the issue calls unambiguous: a row overridden from N to a smaller M +// carries M/N of the money it entered with, so the per-instance rate the row +// implies is unchanged. +func TestApplyCountOverrideRescalesDownscaledRow(t *testing.T) { + const ( + origCount = 100 + override = 5 + ratio = float64(override) / float64(origCount) + ) + + rec := countDenominatedRec(origCount) + perInstanceBefore := rec.EstimatedSavings / float64(rec.Count) + + got := ApplyCountOverride([]common.Recommendation{rec}, override) + + require.Len(t, got, 1) + require.Equal(t, override, got[0].Count, "the override must replace Count") + assertScaledBy(t, rec, got[0], ratio) + + assert.InDelta(t, perInstanceBefore, got[0].EstimatedSavings/float64(got[0].Count), 0.0001, + "savings per instance is the rate the row asserts and must survive the override") +} + +// TestApplyCountOverrideRescalesUpscaledRow covers the direction --max-instances +// never exercises. An override can raise the count above the provider's +// proposal, and leaving the money at the smaller quantity understates both the +// savings and, more dangerously, what the run will be charged. +func TestApplyCountOverrideRescalesUpscaledRow(t *testing.T) { + const ( + origCount = 10 + override = 20 + ratio = float64(override) / float64(origCount) + ) + + rec := countDenominatedRec(origCount) + + got := ApplyCountOverride([]common.Recommendation{rec}, override) + + require.Len(t, got, 1) + require.Equal(t, override, got[0].Count) + assertScaledBy(t, rec, got[0], ratio) + + assert.Greater(t, got[0].CommitmentCost, rec.CommitmentCost, + "buying more than the provider proposed must not report the smaller quantity's cost") +} + +// TestApplyCountOverrideLeavesSavingsPlansUntouched is the both-directions +// assertion, in one call so a fix that rescales every row cannot pass it. An SP +// commitment is priced in dollars per hour rather than in instances: its Count +// is a placeholder the SP purchase call never reads, so sizing it by an +// instance count would multiply the dollars actually committed by an unrelated +// number. +func TestApplyCountOverrideLeavesSavingsPlansUntouched(t *testing.T) { + const override = 20 + + ri := countDenominatedRec(10) + sp := common.Recommendation{ + Service: common.ServiceSavingsPlansCompute, + Region: "us-east-1", + Count: 1, + EstimatedSavings: 500, + CommitmentCost: 1200, + Details: &common.SavingsPlanDetails{ + PlanType: "Compute", + HourlyCommitment: 10, + }, + } + + got := ApplyCountOverride([]common.Recommendation{ri, sp}, override) + + require.Len(t, got, 2) + + // The count-denominated row is rescaled. + require.Equal(t, override, got[0].Count) + assertScaledBy(t, ri, got[0], float64(override)/float64(ri.Count)) + + // The dollar-denominated row is not touched at all, Count included. + assert.Equal(t, 1, got[1].Count, + "an SP is one commitment, not N instances, so the override must not set its Count") + assert.InDelta(t, 500.0, got[1].EstimatedSavings, 0.0001, "an SP's savings must not be scaled by an instance count") + assert.InDelta(t, 1200.0, got[1].CommitmentCost, 0.0001, "an SP's cost must not be scaled by an instance count") + + details, ok := got[1].Details.(*common.SavingsPlanDetails) + require.True(t, ok, "SP details must survive with their concrete type") + assert.InDelta(t, 10.0, details.HourlyCommitment, 0.0001, + "the hourly commitment is what the SP purchase actually buys and must not move with --override-count") +} + +// TestApplyCountOverrideNonPositiveCountIsNotRescaled guards the divide-by-zero +// denominator. A Count of 0 or below carries no per-unit rate to scale from, so +// the row passes through untouched rather than through a ratio computed from a +// zero denominator (Inf) or a negative one (sign flip). Setting Count without +// scaling would reintroduce the very misstatement #1844 is about. +func TestApplyCountOverrideNonPositiveCountIsNotRescaled(t *testing.T) { + recs := []common.Recommendation{ + {ResourceType: "zero-count", Count: 0, EstimatedSavings: 600, OnDemandCost: 100}, + {ResourceType: "negative-count", Count: -5, EstimatedSavings: 300, OnDemandCost: 50}, + } + + got := ApplyCountOverride(recs, 10) + + require.Len(t, got, 2) + for i := range got { + assert.False(t, math.IsNaN(got[i].EstimatedSavings) || math.IsInf(got[i].EstimatedSavings, 0), + "%s: savings must not become NaN/Inf via a non-positive denominator", got[i].ResourceType) + assert.False(t, math.IsNaN(got[i].OnDemandCost) || math.IsInf(got[i].OnDemandCost, 0), + "%s: on-demand cost must not become NaN/Inf via a non-positive denominator", got[i].ResourceType) + } + + assert.Equal(t, 0, got[0].Count, "a row with no quantity to scale from keeps its count") + assert.InDelta(t, 600.0, got[0].EstimatedSavings, 0.0001, "a zero-count row is not rescaled") + assert.InDelta(t, 100.0, got[0].OnDemandCost, 0.0001, "a zero-count row is not rescaled") + assert.Equal(t, -5, got[1].Count, "a row with no quantity to scale from keeps its count") + assert.InDelta(t, 300.0, got[1].EstimatedSavings, 0.0001, "a negative-count row is not rescaled") + assert.InDelta(t, 50.0, got[1].OnDemandCost, 0.0001, "a negative-count row is not rescaled") +} + +// TestApplyCountOverridePreservesNilRecurringMonthlyCost pins the +// absent-versus-zero rule: nil means "the provider returned no monthly +// breakdown" and renders as an em dash rather than $0. The override must not +// turn that into a confident zero. +func TestApplyCountOverridePreservesNilRecurringMonthlyCost(t *testing.T) { + recs := []common.Recommendation{ + { + Service: common.ServiceEC2, + ResourceType: "no-monthly-breakdown", + Count: 100, + EstimatedSavings: 600, + RecurringMonthlyCost: nil, + }, + } + + got := ApplyCountOverride(recs, 5) + + require.Len(t, got, 1) + require.Equal(t, 5, got[0].Count) + assert.Nil(t, got[0].RecurringMonthlyCost, + "a nil monthly cost means absent, not zero, and must survive the override as nil") +} + +// TestApplyCountOverrideDoesNotAliasCallerRecs pins that the caller's slice +// survives the override intact. The pipeline hands the pre-override slice on to +// the cap, whose reporter diffs one against the other, so a shared pointer +// target would corrupt what the operator is shown was reduced. +func TestApplyCountOverrideDoesNotAliasCallerRecs(t *testing.T) { + before := []common.Recommendation{countDenominatedRec(100)} + + got := ApplyCountOverride(before, 5) + + require.Len(t, got, 1) + require.NotNil(t, got[0].RecurringMonthlyCost) + require.NotNil(t, before[0].RecurringMonthlyCost) + + assert.NotSame(t, before[0].RecurringMonthlyCost, got[0].RecurringMonthlyCost, + "the scaled monthly cost must be a fresh pointer, not a write through the caller's") + assert.Equal(t, 100, before[0].Count, "the input rec must not be mutated") + assert.InDelta(t, 600.0, before[0].EstimatedSavings, 0.0001, "the input rec must not be mutated") + assert.InDelta(t, 200.0, *before[0].RecurringMonthlyCost, 0.0001, + "the input rec's pointer target must not be mutated") +} + +// TestApplyCountOverrideReportsExtrapolationPastProviderEvidence pins the +// disclosure that makes scaling up defensible. Costs stay accurate at any +// quantity because unit prices are linear, but savings only accrue on hours a +// matching instance runs, so an override past the demand the provider measured +// produces a savings figure nobody measured. Silently presenting it would be +// the fabricated figure #1844 set out to remove. +// +// The boundary is the provider's own proposal, not the row's current count: a +// row sized down to 80 by --coverage from a proposal of 100 is still inside +// what the provider's figures cover at 100, so overriding back up to 100 is +// interpolation and must stay quiet. +func TestApplyCountOverrideReportsExtrapolationPastProviderEvidence(t *testing.T) { + sizedDownFromProposal := common.Recommendation{ + Service: common.ServiceEC2, + ResourceType: "within-proposal", + Count: 80, + RecommendedCount: 100, + EstimatedSavings: 480, + } + + quiet := captureAppOutput(t, func() { + got := ApplyCountOverride([]common.Recommendation{sizedDownFromProposal}, 100) + require.Len(t, got, 1) + require.Equal(t, 100, got[0].Count) + }) + assert.NotContains(t, quiet, "exceeds the quantity", + "overriding back up to the provider's own proposal is interpolation, not extrapolation") + + loud := captureAppOutput(t, func() { + got := ApplyCountOverride([]common.Recommendation{sizedDownFromProposal}, 101) + require.Len(t, got, 1) + require.Equal(t, 101, got[0].Count) + }) + assert.Contains(t, loud, "exceeds the quantity", + "an override past the provider's proposal extrapolates the savings and must say so") +} + +// TestApplyCountOverrideReportsSkippedRows pins that the two exempt classes are +// named rather than silently passed through. An operator who set +// --override-count and got a differently-sized run than they asked for has to +// be told which rows the flag could not size and why. +func TestApplyCountOverrideReportsSkippedRows(t *testing.T) { + recs := []common.Recommendation{ + {Service: common.ServiceSavingsPlansCompute, Count: 1, EstimatedSavings: 500}, + {Service: common.ServiceEC2, ResourceType: "zero-count", Count: 0, EstimatedSavings: 600}, + } + + out := captureAppOutput(t, func() { + got := ApplyCountOverride(recs, 10) + require.Len(t, got, 2) + }) + + assert.Contains(t, out, "Savings Plans recommendation(s) unchanged", + "an SP the override could not size must be named") + assert.Contains(t, out, "non-positive count unchanged", + "a row with no quantity to rescale from must be named") +} + +// TestApplyCountOverrideThenInstanceLimitScalesOnce pins the ordering both call +// sites establish: the override runs first and the run-wide cap second. The two +// ratios have to compose to a single net ratio against the provider's figures, +// rather than the cap re-deriving its ratio from a base the override already +// corrupted. +// +// The expectation is computed independently of either function, so it cannot +// agree with a wrong implementation. +func TestApplyCountOverrideThenInstanceLimitScalesOnce(t *testing.T) { + const ( + origCount = 100 + override = 20 + capTo = 8 + ) + + rec := countDenominatedRec(origCount) + + overridden := ApplyCountOverride([]common.Recommendation{rec}, override) + require.Len(t, overridden, 1) + require.Equal(t, override, overridden[0].Count) + + capped := ApplyInstanceLimit(overridden, capTo) + require.Len(t, capped, 1) + require.Equal(t, capTo, capped[0].Count, "the cap truncates the overridden count") + + // Net ratio is capTo/origCount: the override's 20/100 composed with the + // cap's 8/20. Anything else means one stage scaled against a wrong base. + netRatio := float64(capTo) / float64(origCount) + assertScaledBy(t, rec, capped[0], netRatio) +} diff --git a/cmd/helpers_instance_limit_rescale_test.go b/cmd/helpers_instance_limit_rescale_test.go index 9348aef71..12358640c 100644 --- a/cmd/helpers_instance_limit_rescale_test.go +++ b/cmd/helpers_instance_limit_rescale_test.go @@ -151,12 +151,15 @@ func TestApplyInstanceLimitNonPositiveCountIsNotRescaled(t *testing.T) { } // TestApplyInstanceLimitRescalesSavingsPlanHourlyCommitment covers the Savings -// Plan case. SP recs are parsed at Count 1 and so are normally undivisible, -// but --override-count sets Count on every rec without discriminating by -// commitment type (ApplyCountOverride), which lets an SP reach the cap at a -// count the budget can truncate. HourlyCommitment is the SP's actual money -// quantity, so leaving it whole while the cost fields shrink produces exactly -// the internally-inconsistent row #1830 warns about. +// Plan case. The provider parser pins SP recs at Count 1, so they are normally +// undivisible, but the --input-csv path builds Service straight from the CSV +// column (parseCSVRecords), so a file naming a savingsplans service at a count +// above the budget reaches this branch. HourlyCommitment is the SP's actual +// money quantity, so leaving it whole while the cost fields shrink produces +// exactly the internally-inconsistent row #1830 warns about. +// +// --override-count no longer routes SPs here: it leaves them alone precisely +// because an SP is dollar-denominated rather than count-denominated (#1844). func TestApplyInstanceLimitRescalesSavingsPlanHourlyCommitment(t *testing.T) { const ratio = 2.0 / 5.0 diff --git a/docs/cli/README.md b/docs/cli/README.md index a3656f844..8b4cdd455 100644 --- a/docs/cli/README.md +++ b/docs/cli/README.md @@ -35,7 +35,7 @@ All flags belong to the root command unless noted otherwise. | `--coverage` | `-c` | `80` | Percentage (0-100) of each recommendation's instance count to purchase (`rec.Count * coverage/100`). Ignored when `--target-coverage` is also set. | | `--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. | +| `--override-count` | | `0` (disabled) | Replace every recommendation's count with this fixed number, rescaling that row's costs and savings by the same ratio so they describe the quantity that will actually be bought. Savings Plans are exempt and pass through unchanged: an SP commitment is priced in dollars per hour, not in instances. Rows with a non-positive count are exempt too, having no quantity to rescale from. Overriding above the quantity the provider's figures cover keeps the costs accurate but extrapolates the savings past the observed demand; all three cases are named on stdout. Applied before `--max-instances`. 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). 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 From 84a76796d5ec62ddc103f7bf83742737b701ff80 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 18 Aug 2026 16:24:49 +0200 Subject: [PATCH 2/2] refactor(cli): move the --override-count functions out of cmd/helpers.go ApplyCountOverride, evidencedCount and reportCountOverride move verbatim into cmd/helpers_count_override.go. The public signature is unchanged and every caller is in the same package, so nothing else moves and nothing is renamed. This is a pure move, verified as one rather than asserted: every added line except the six-line package and import header is byte-identical to a removed line, in the same order, and the source shrank by exactly the moved block plus its one blank separator. No logic, signature, name or comment changed. The three functions reference only common.Recommendation, common.IsSavingsPlan, common.ScaleRecommendationCosts and the package-level AppLogger, so the new file needs a single import and the source loses none. cmd/helpers.go goes from 881 to 794 lines, which is 19 below the 813 it stood at before this PR touched it: the 68 lines this branch added are back out along with the function they replaced. The new file is 92 lines. The file is still over the 500-line limit, as are five other non-test files in cmd/. That is pre-existing and systemic, tracked in #1834, and restructuring 813 lines of money-path code inside a correctness fix is the wrong risk and makes the behavior diff unreadable. Taking this branch's own additions back out is the part that belongs here. Test parity is asserted on the SET of test names and outcomes rather than on a count, comparing a run in a pristine worktree at the pre-move commit against one on this tree: identical names, identical outcomes, no additions, no removals. golangci-lint at the CI-pinned v2.10.1 reports 0 issues and gocyclo -over 10 is clean on both files, checked after the move because moving code changes which file the complexity is attributed to. --- cmd/helpers.go | 87 --------------------------------- cmd/helpers_count_override.go | 92 +++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 87 deletions(-) create mode 100644 cmd/helpers_count_override.go diff --git a/cmd/helpers.go b/cmd/helpers.go index b19dcc281..22a7970c0 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -492,93 +492,6 @@ func applySizing(recs []common.Recommendation, cfg Config, coverage float64, dro return applyCoverage(recs, coverage, drops) } -// ApplyCountOverride replaces the count on every count-denominated -// recommendation with overrideCount, rescaling the money that count derives so -// the row describes the quantity the run will actually buy. -// -// EstimatedSavings, CommitmentCost, OnDemandCost and RecurringMonthlyCost are -// whole-row totals for the count the provider proposed, so replacing Count -// alone leaves the row claiming the money of a quantity nobody will buy -// (#1844). The scaling goes through common.ScaleRecommendationCosts, the same -// helper ApplyInstanceLimit and the coverage paths use, so the arithmetic -// cannot drift between the flags. ProjectedCoverage / ProjectedUtilization are -// count-linear but not re-derived here, exactly as after a cap (#1845). -// -// Savings Plans are left entirely alone, Count included. The flag is -// documented as an override for "all selected RIs", an SP commitment is -// dollar-denominated rather than count-denominated, its Count is a fixed -// placeholder 1 set by the parser, and the SP purchase call reads -// HourlyCommitment rather than Count. Scaling an SP by an instance count would -// multiply the dollars actually committed by an unrelated number. -// -// A row with a non-positive Count is left alone for the same reason -// ApplyInstanceLimit never rescales one: there is no denominator to form a -// ratio from, and setting Count without scaling would reintroduce precisely -// the misstatement above. -// -// Scaling down is unambiguous. Scaling up past the quantity the provider's own -// figures cover is an extrapolation: unit prices are linear so CommitmentCost -// stays true, but savings only accrue on hours a matching resource actually -// runs, so units beyond the observed demand cost money and may save nothing. -// Those rows are reported rather than passed off as measured savings. -// -// Both call sites run this before the run-wide --max-instances cap, so a run -// using both flags scales once from the override ratio and once from the cap -// ratio, which compose to a single net ratio against the provider's figures. -func ApplyCountOverride(recs []common.Recommendation, overrideCount int32) []common.Recommendation { - if overrideCount <= 0 { - return recs - } - result := make([]common.Recommendation, len(recs)) - var skippedSP, skippedNonPositive, extrapolated int - for i := range recs { - rec := recs[i] - switch { - case common.IsSavingsPlan(rec.Service): - result[i] = rec - skippedSP++ - case rec.Count <= 0: - result[i] = rec - skippedNonPositive++ - default: - result[i] = common.ScaleRecommendationCosts(rec, float64(overrideCount)/float64(rec.Count)) - result[i].Count = int(overrideCount) - if int(overrideCount) > evidencedCount(rec) { - extrapolated++ - } - } - } - reportCountOverride(overrideCount, skippedSP, skippedNonPositive, extrapolated) - return result -} - -// evidencedCount is the largest count a recommendation's money figures are -// evidence for: the provider's own pre-sizing proposal when it recorded one, -// otherwise the count the row currently carries. RecommendedCount is populated -// only on the AWS RI path, so the fallback is what the --input-csv and -// non-AWS paths use. -func evidencedCount(rec common.Recommendation) int { - if rec.RecommendedCount > rec.Count { - return rec.RecommendedCount - } - return rec.Count -} - -// reportCountOverride names what --override-count could not honor and what it -// honored only by extrapolation. Nothing here is fatal, but every case would -// otherwise change or fail to change a money figure without telling anyone. -func reportCountOverride(overrideCount int32, skippedSP, skippedNonPositive, extrapolated int) { - if skippedSP > 0 { - AppLogger.Printf("⚠️ --override-count left %d Savings Plans recommendation(s) unchanged: an SP commitment is priced in dollars per hour, not in instances, so an instance count cannot size it.\n", skippedSP) - } - if skippedNonPositive > 0 { - AppLogger.Printf("⚠️ --override-count left %d recommendation(s) with a non-positive count unchanged: there is no quantity to rescale their costs from.\n", skippedNonPositive) - } - if extrapolated > 0 { - AppLogger.Printf("⚠️ --override-count=%d exceeds the quantity the provider's figures cover on %d recommendation(s). Their costs scale with the count and stay accurate, but the savings are extrapolated past the observed demand: instances beyond it are billed and may save nothing.\n", overrideCount, extrapolated) - } -} - // ApplyInstanceLimit truncates recs so their total Count does not exceed // maxInstances. It is a single-shot cap over whatever slice it is handed: the // caller is responsible for handing it the complete run-wide set, because diff --git a/cmd/helpers_count_override.go b/cmd/helpers_count_override.go new file mode 100644 index 000000000..b4f2e2107 --- /dev/null +++ b/cmd/helpers_count_override.go @@ -0,0 +1,92 @@ +package main + +import ( + "github.com/LeanerCloud/CUDly/pkg/common" +) + +// ApplyCountOverride replaces the count on every count-denominated +// recommendation with overrideCount, rescaling the money that count derives so +// the row describes the quantity the run will actually buy. +// +// EstimatedSavings, CommitmentCost, OnDemandCost and RecurringMonthlyCost are +// whole-row totals for the count the provider proposed, so replacing Count +// alone leaves the row claiming the money of a quantity nobody will buy +// (#1844). The scaling goes through common.ScaleRecommendationCosts, the same +// helper ApplyInstanceLimit and the coverage paths use, so the arithmetic +// cannot drift between the flags. ProjectedCoverage / ProjectedUtilization are +// count-linear but not re-derived here, exactly as after a cap (#1845). +// +// Savings Plans are left entirely alone, Count included. The flag is +// documented as an override for "all selected RIs", an SP commitment is +// dollar-denominated rather than count-denominated, its Count is a fixed +// placeholder 1 set by the parser, and the SP purchase call reads +// HourlyCommitment rather than Count. Scaling an SP by an instance count would +// multiply the dollars actually committed by an unrelated number. +// +// A row with a non-positive Count is left alone for the same reason +// ApplyInstanceLimit never rescales one: there is no denominator to form a +// ratio from, and setting Count without scaling would reintroduce precisely +// the misstatement above. +// +// Scaling down is unambiguous. Scaling up past the quantity the provider's own +// figures cover is an extrapolation: unit prices are linear so CommitmentCost +// stays true, but savings only accrue on hours a matching resource actually +// runs, so units beyond the observed demand cost money and may save nothing. +// Those rows are reported rather than passed off as measured savings. +// +// Both call sites run this before the run-wide --max-instances cap, so a run +// using both flags scales once from the override ratio and once from the cap +// ratio, which compose to a single net ratio against the provider's figures. +func ApplyCountOverride(recs []common.Recommendation, overrideCount int32) []common.Recommendation { + if overrideCount <= 0 { + return recs + } + result := make([]common.Recommendation, len(recs)) + var skippedSP, skippedNonPositive, extrapolated int + for i := range recs { + rec := recs[i] + switch { + case common.IsSavingsPlan(rec.Service): + result[i] = rec + skippedSP++ + case rec.Count <= 0: + result[i] = rec + skippedNonPositive++ + default: + result[i] = common.ScaleRecommendationCosts(rec, float64(overrideCount)/float64(rec.Count)) + result[i].Count = int(overrideCount) + if int(overrideCount) > evidencedCount(rec) { + extrapolated++ + } + } + } + reportCountOverride(overrideCount, skippedSP, skippedNonPositive, extrapolated) + return result +} + +// evidencedCount is the largest count a recommendation's money figures are +// evidence for: the provider's own pre-sizing proposal when it recorded one, +// otherwise the count the row currently carries. RecommendedCount is populated +// only on the AWS RI path, so the fallback is what the --input-csv and +// non-AWS paths use. +func evidencedCount(rec common.Recommendation) int { + if rec.RecommendedCount > rec.Count { + return rec.RecommendedCount + } + return rec.Count +} + +// reportCountOverride names what --override-count could not honor and what it +// honored only by extrapolation. Nothing here is fatal, but every case would +// otherwise change or fail to change a money figure without telling anyone. +func reportCountOverride(overrideCount int32, skippedSP, skippedNonPositive, extrapolated int) { + if skippedSP > 0 { + AppLogger.Printf("⚠️ --override-count left %d Savings Plans recommendation(s) unchanged: an SP commitment is priced in dollars per hour, not in instances, so an instance count cannot size it.\n", skippedSP) + } + if skippedNonPositive > 0 { + AppLogger.Printf("⚠️ --override-count left %d recommendation(s) with a non-positive count unchanged: there is no quantity to rescale their costs from.\n", skippedNonPositive) + } + if extrapolated > 0 { + AppLogger.Printf("⚠️ --override-count=%d exceeds the quantity the provider's figures cover on %d recommendation(s). Their costs scale with the count and stay accurate, but the savings are extrapolated past the observed demand: instances beyond it are billed and may save nothing.\n", overrideCount, extrapolated) + } +}