diff --git a/cmd/helpers.go b/cmd/helpers.go index 09ea2e02c..0aae0c12b 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -501,7 +501,16 @@ func ApplyCountOverride(recs []common.Recommendation, overrideCount int32) []com return result } -// ApplyInstanceLimit limits the total number of instances. +// 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 +// applying it to a subset (one service, one region) caps that subset only and +// multiplies the effective cap by the number of subsets. See +// applyGlobalInstanceLimit in multi_service.go for the run-wide call site. +// +// Recommendations are consumed in slice order, so the caller controls which +// ones survive by ordering the slice (the main path caps the scorer's +// savings-sorted output, keeping the highest-value commitments). func ApplyInstanceLimit(recs []common.Recommendation, maxInstances int32) []common.Recommendation { if maxInstances <= 0 { return recs @@ -520,7 +529,12 @@ func ApplyInstanceLimit(recs []common.Recommendation, maxInstances int32) []comm adjusted.Count = remaining } result = append(result, adjusted) - remaining -= adjusted.Count + // Only a positive Count consumes budget. Subtracting a non-positive + // Count would credit budget back and let later recommendations push + // the run past the cap. + if adjusted.Count > 0 { + remaining -= adjusted.Count + } } return result } diff --git a/cmd/multi_service.go b/cmd/multi_service.go index b445632ee..58b613b13 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -136,8 +136,8 @@ func runToolMultiService(ctx context.Context, cfg Config) { AppLogger.Printf("\nšŸ“„ Fetching recommendations from all services...\n") allRecs, drops := fetchAllRecs(ctx, awsCfg, recClient, accountCache, servicesToProcess, engineData, cfg, coverageMap) - // Phase 2: score and display. - scoredResult := scoreAndDisplay(allRecs, cfg) + // Phase 2: score, enforce the run-wide instance cap, and display. + scoredResult := scoreLimitAndDisplay(allRecs, cfg, drops) if len(scoredResult.Passed) == 0 { printDropSummary(drops) AppLogger.Printf("\nā„¹ļø No recommendations passed filters. Nothing to purchase.\n") @@ -202,20 +202,149 @@ func loadAWSConfig(ctx context.Context, cfg Config) (aws.Config, error) { return awsconfig.LoadDefaultConfig(ctx, opts...) } -// scoreAndDisplay runs the scorer on recs and prints the scored table and summary. -func scoreAndDisplay(recs []common.Recommendation, cfg Config) scorer.ScoredResult { +// scoreLimitAndDisplay runs the scorer on recs, enforces the run-wide +// --max-instances cap on the survivors, and prints the scored table and +// summary. +// +// The cap runs between scoring and rendering so the table, the confirmation +// prompt and the purchase loop all describe the same post-cap set, and so the +// instances that survive are the highest-savings ones run-wide (scorer.Score +// sorts Passed by savings percentage descending). +func scoreLimitAndDisplay(recs []common.Recommendation, cfg Config, drops *common.DropSummary) scorer.ScoredResult { scorerCfg := scorer.Config{ MinSavingsPct: cfg.MinSavingsPct, MaxBreakEvenMonths: cfg.MaxBreakEvenMonths, MinCount: cfg.MinCount, } result := scorer.Score(recs, scorerCfg) + result.Passed = applyGlobalInstanceLimit(result.Passed, cfg, drops) fmt.Print(reporter.RenderTable(result)) fmt.Print(reporter.RenderExcluded(result)) fmt.Print(reporter.RenderSummary(result)) return result } +// applyGlobalInstanceLimit enforces --max-instances once across the entire run. +// +// The flag is documented as a hard cap on the total number of instances +// purchased across all recommendations, so it has to see every service and +// every region together. Applying it inside the per-region fetch instead caps +// 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. +// +// Truncation can push a recommendation under --min-count, which is a hard +// floor rather than advice, so dropTruncatedBelowMinCount removes any such +// recommendation instead of purchasing it short. +// +// 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) { + return passed + } + + limited := ApplyInstanceLimit(passed, cfg.MaxInstances) + limited, belowMin := dropTruncatedBelowMinCount(limited, cfg.MinCount) + reportInstanceLimit(passed, limited, len(belowMin), totalBefore, cfg.MaxInstances, drops) + reportMinCountDrops(belowMin, cfg.MinCount, drops) + return limited +} + +// dropTruncatedBelowMinCount removes recommendations that the cap truncated to +// fewer instances than --min-count allows, returning the survivors and the +// removed recommendations at their truncated counts. +// +// --min-count is a hard floor everywhere else in the codebase, never advice: +// the scorer rejects recommendations under it outright +// (scorer.filterReason, "count %d below minimum %d"), the scheduler's +// meetsMinCount drops them, and both `docs/cli/filtering.md` and +// `docs/cli/README.md` describe it as dropping recommendations below the +// number. filtering.md applies it to "the adjusted instance count (after +// coverage scaling)", so the floor is meant to gate the *sized* count, and +// truncation by --max-instances is another form of sizing. +// +// Buying a commitment smaller than the operator's stated minimum can be worse +// than buying nothing, which is the whole reason the floor exists, so a +// truncated recommendation is dropped rather than purchased short. The freed +// budget is deliberately not redistributed: the next recommendation would have +// to fit in an even smaller remainder and would fail the same floor. +// +// Removal is always from the tail. ApplyInstanceLimit reduces at most one +// recommendation (the one where the budget runs out, which is the last it +// keeps), and every earlier one still carries the full count that already +// cleared the scorer's floor. Taking only from the tail keeps the result a +// prefix of the input, which reportInstanceLimit relies on. +func dropTruncatedBelowMinCount(limited []common.Recommendation, minCount int) (kept, removed []common.Recommendation) { + if minCount <= 0 { + return limited, nil + } + kept = limited + for len(kept) > 0 && kept[len(kept)-1].Count < minCount { + removed = append(removed, kept[len(kept)-1]) + kept = kept[:len(kept)-1] + } + return kept, removed +} + +// reportMinCountDrops names each recommendation the --min-count floor rejected +// after --max-instances truncated it. It continues the reportInstanceLimit +// listing, where these already appear as dropped, and explains why they were +// not simply purchased at the reduced count. +func reportMinCountDrops(removed []common.Recommendation, minCount int, drops *common.DropSummary) { + for i := range removed { + rec := removed[i] + AppLogger.Printf(" ā†³ %s %s %s: the cap left room for only %d instances, below --min-count %d, so it is dropped rather than purchased short\n", + rec.Service, rec.Region, rec.ResourceType, rec.Count, minCount) + } + drops.Add(common.DropMinCountAfterCap, len(removed)) +} + +// reportInstanceLimit prints what --max-instances removed from the run. +// after must be the prefix of before produced by ApplyInstanceLimit, so +// after[i] and before[i] describe the same recommendation. +// +// 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) { + 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") + + reduced, dropped := 0, 0 + for i := range before { + rec := before[i] + kept := 0 + if i < len(after) { + kept = after[i].Count + } + switch { + case kept == rec.Count: + continue + case kept > 0: + reduced++ + AppLogger.Printf(" ā€¢ reduced: %s %s %s %d ā†’ %d instances\n", rec.Service, rec.Region, rec.ResourceType, rec.Count, kept) + default: + dropped++ + AppLogger.Printf(" ā€¢ dropped: %s %s %s (%d instances)\n", rec.Service, rec.Region, rec.ResourceType, rec.Count) + } + } + + drops.Add(common.DropMaxInstances, dropped-belowMinCount) + AppLogger.Printf(" Proceeding with %d instances across %d recommendations (%d reduced, %d dropped).\n", + CalculateTotalInstances(after), len(after), reduced, dropped) +} + // sumPassedRecs returns total instance count and total estimated savings for passed recs. func sumPassedRecs(recs []common.Recommendation) (total int, totalSavings float64) { for _rvc := range recs { diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index 6c999afc4..ca5df213e 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -396,6 +396,25 @@ func processRegionRecommendations( result.recommendations = filteredRecs + // --max-instances is a run-wide cap, and this legacy per-region entry point + // has no view of the other services and regions in the run, so it cannot + // evaluate the cap. Refuse to spend rather than purchase uncapped: an + // over-purchase of reserved capacity is not reversible. Dry runs continue + // so the recommendations are still reported. + // + // This is deliberately the first thing after the recommendations are + // recorded, ahead of building the service client and of the duplicate + // check. The decision depends only on cfg.MaxInstances and isDryRun, both + // already known, so reaching it through a cloud API call would be work + // done to arrive at an answer that was already determined -- and it would + // make the refusal path fail differently depending on whether the describe + // call happened to succeed. + if cfg.MaxInstances > 0 && !isDryRun { + log.Printf("āŒ Refusing to purchase %s/%s: --max-instances is a run-wide cap and cannot be enforced on the per-region path. Use the multi-service pipeline (the default entry point).", + getServiceDisplayName(service), region) + return result + } + // Get service client and process purchases regionalCfg := awsCfg.Copy() regionalCfg.Region = region @@ -407,8 +426,8 @@ func processRegionRecommendations( return result } - // Check for duplicate RIs and apply instance limit. Drop tracking skipped (nil). - adjustedRecs := checkDuplicatesAndApplyLimit(ctx, filteredRecs, serviceClient, cfg, nil) + // Check for duplicate RIs. Drop tracking skipped (nil). + adjustedRecs := checkDuplicates(ctx, filteredRecs, serviceClient, nil) // Process purchases regionResults := processPurchaseLoop(ctx, adjustedRecs, region, isDryRun, serviceClient, cfg) @@ -548,13 +567,20 @@ func applyCoverageAndOverrides(recs []common.Recommendation, cfg Config, coverag return filteredRecs } -// checkDuplicatesAndApplyLimit checks for duplicate RIs and applies instance limits. +// checkDuplicates adjusts recommendations against already-owned RIs so the run +// does not double-purchase existing capacity. +// +// It deliberately does NOT apply --max-instances. This function runs once per +// (service, region), and capping here caps each region independently, which +// multiplies the operator's cap by the number of service/region pairs. The cap +// is applied once run-wide instead, after every region has been fetched +// (applyGlobalInstanceLimit in multi_service.go). +// // drops accumulates per-reason drop counts for the end-of-run summary; pass nil to skip. -func checkDuplicatesAndApplyLimit( +func checkDuplicates( ctx context.Context, filteredRecs []common.Recommendation, serviceClient provider.ServiceClient, - cfg Config, drops *common.DropSummary, ) []common.Recommendation { // Check for duplicate RIs to avoid double purchasing @@ -574,15 +600,6 @@ func checkDuplicatesAndApplyLimit( filteredRecs = adjustedRecs } - // Apply instance limit if specified - if cfg.MaxInstances > 0 { - beforeLimit := len(filteredRecs) - filteredRecs = ApplyInstanceLimit(filteredRecs, cfg.MaxInstances) - if len(filteredRecs) < beforeLimit { - AppLogger.Printf(" šŸ”’ Applied instance limit: %d recommendations after limiting to %d instances\n", len(filteredRecs), cfg.MaxInstances) - } - } - return filteredRecs } @@ -643,8 +660,10 @@ func fetchAndFilterRegionRecs( recs = applyCoverageAndOverrides(recs, cfg, coverageMap, expiringCommitments, drops) // Deduplication: skip recs matching recently-purchased commitments. + // --max-instances is NOT applied here; it is enforced once run-wide by the + // caller so the cap covers every service and region together. if serviceClient != nil { - recs = checkDuplicatesAndApplyLimit(ctx, recs, serviceClient, cfg, drops) + recs = checkDuplicates(ctx, recs, serviceClient, drops) } return recs diff --git a/cmd/multi_service_max_instances_test.go b/cmd/multi_service_max_instances_test.go new file mode 100644 index 000000000..b1f70e711 --- /dev/null +++ b/cmd/multi_service_max_instances_test.go @@ -0,0 +1,425 @@ +package main + +import ( + "context" + "testing" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// maxInstancesFixtureServices and maxInstancesFixtureRegions describe the +// multi-service, multi-region fan-out the run-wide cap has to survive. A +// single-service or single-region fixture stays green with the per-region bug +// present, so both dimensions are needed. +var ( + maxInstancesFixtureServices = []common.ServiceType{common.ServiceRDS, common.ServiceElastiCache} + maxInstancesFixtureRegions = []string{"us-east-1", "us-west-2", "eu-west-1"} +) + +// countPerFixtureRec is the instance count each (service, region) pair returns. +const countPerFixtureRec = 8 + +// newMaxInstancesMockClientWith returns a recommendations client that answers +// one recommendation per (service, region) pair. savingsFor maps the fan-out +// position (service index, region index) to a savings percentage, which lets a +// test decide whether the best recommendations are fetched first or last. +func newMaxInstancesMockClientWith(count int, savingsFor func(serviceIdx, regionIdx int) float64) *MockRecommendationsClient { + mockClient := &MockRecommendationsClient{} + for i, svc := range maxInstancesFixtureServices { + for j, region := range maxInstancesFixtureRegions { + svc, region := svc, region + savings := savingsFor(i, j) + rec := common.Recommendation{ + Service: svc, + Region: region, + ResourceType: "db.t3.small", + Count: count, + EstimatedSavings: savings * 10, + SavingsPercentage: savings, + } + mockClient.On("GetRecommendations", mock.Anything, + mock.MatchedBy(func(p *common.RecommendationParams) bool { + return p != nil && p.Service == svc && p.Region == region + }), + ).Return([]common.Recommendation{rec}, nil).Once() + } + } + return mockClient +} + +// newMaxInstancesMockClient answers with savings descending in fan-out order +// (50, 45, 40, ...), so the best recommendations are also the first fetched. +func newMaxInstancesMockClient() *MockRecommendationsClient { + return newMaxInstancesMockClientWith(countPerFixtureRec, func(i, j int) float64 { + return 50.0 - 5*float64(i*len(maxInstancesFixtureRegions)+j) + }) +} + +// TestMaxInstancesCapsWholeRunAcrossServicesAndRegions is the regression test +// for #1608. +// +// --max-instances is documented (cmd/main.go, docs/cli/README.md, +// docs/cli/filtering.md) as a hard cap on the *total* number of instances +// purchased across all recommendations. It used to be applied inside the +// per-(service, region) fetch, so every pair independently kept up to +// MaxInstances and the run bought roughly cap x services x regions. +// +// This is the issue's scenario in miniature: 2 services x 3 regions, each +// answering with 8 instances (48 natural total), capped at 10. Pre-fix the run +// kept all 48 (4.8x the cap); post-fix the sum across every service and region +// is exactly 10. +func TestMaxInstancesCapsWholeRunAcrossServicesAndRegions(t *testing.T) { + const maxInstances = 10 + + ctx := context.Background() + awsCfg := aws.Config{Region: "us-east-1"} + + origCfg := toolCfg + t.Cleanup(func() { toolCfg = origCfg }) + + toolCfg.Coverage = 100.0 + toolCfg.PaymentOption = "partial-upfront" + toolCfg.TermYears = 1 + toolCfg.Regions = maxInstancesFixtureRegions + toolCfg.MaxInstances = maxInstances + + mockClient := newMaxInstancesMockClient() + t.Cleanup(func() { mockClient.AssertExpectations(t) }) + + accountCache := NewAccountAliasCache(awsCfg) + allRecs, drops := fetchAllRecs(ctx, awsCfg, mockClient, accountCache, + maxInstancesFixtureServices, engineVersionData{}, toolCfg, nil) + + pairs := len(maxInstancesFixtureServices) * len(maxInstancesFixtureRegions) + naturalTotal := pairs * countPerFixtureRec + + // The fetch stage must hand the *uncapped* set to the run-wide cap. If this + // fails, the cap has been pushed back down into the per-region path. + require.Len(t, allRecs, pairs, "fetch stage must not drop recommendations") + require.Equal(t, naturalTotal, CalculateTotalInstances(allRecs), + "fetch stage must not apply the cap per region") + + scored := scoreLimitAndDisplay(allRecs, toolCfg, drops) + + total := CalculateTotalInstances(scored.Passed) + assert.LessOrEqual(t, total, maxInstances, + "--max-instances must cap the sum across every service and region, got %d instances from %d service/region pairs", + total, pairs) + // The cap is a budget to spend, not just a ceiling: with 48 instances + // available it should be consumed exactly. + assert.Equal(t, maxInstances, total) + + // Highest-savings recommendations survive, run-wide: the 50% rec keeps all + // 8 instances and the 45% rec is reduced to the remaining 2. + require.Len(t, scored.Passed, 2) + assert.InDelta(t, 50.0, scored.Passed[0].SavingsPercentage, 0.001) + assert.Equal(t, countPerFixtureRec, scored.Passed[0].Count) + assert.InDelta(t, 45.0, scored.Passed[1].SavingsPercentage, 0.001) + assert.Equal(t, maxInstances-countPerFixtureRec, scored.Passed[1].Count) + + // No silent clamping: the four fully-dropped recommendations are counted + // into the end-of-run summary. + assert.Contains(t, drops.FormatOneLine(), common.DropMaxInstances+"=4") +} + +// TestMaxInstancesKeepsHighestSavingsNotFirstFetched pins the *selection* +// property of the cap, which TestMaxInstancesCapsWholeRunAcrossServicesAndRegions +// cannot see. +// +// ApplyInstanceLimit consumes its input in slice order and drops the tail, so +// whatever ordering it is handed decides which commitments are bought. Capping +// the merged set before scoring and capping the scorer's savings-sorted output +// both satisfy "total <= cap", so the total-focused test passes either way. +// Only this fixture distinguishes them: the first-fetched service and region +// carry the *worst* recommendations, so a cap that consumes in fetch order +// spends the entire budget on them and leaves the best ones unbought. +// +// RDS is fetched first (index 0 of maxInstancesFixtureServices) with 10-12% +// savings; ElastiCache is fetched second with 40-50%. The cap admits 10 of the +// 36 available instances, and every one of them must come from ElastiCache. +func TestMaxInstancesKeepsHighestSavingsNotFirstFetched(t *testing.T) { + const ( + maxInstances = 10 + countPerRec = 6 + ) + + ctx := context.Background() + awsCfg := aws.Config{Region: "us-east-1"} + + origCfg := toolCfg + t.Cleanup(func() { toolCfg = origCfg }) + + toolCfg.Coverage = 100.0 + toolCfg.PaymentOption = "partial-upfront" + toolCfg.TermYears = 1 + toolCfg.Regions = maxInstancesFixtureRegions + toolCfg.MaxInstances = maxInstances + toolCfg.MinSavingsPct = 0 // the low-savings recs must reach the cap, not be scored out + + // Worst-first: the first-fetched service gets 10/11/12%, the second gets + // 40/45/50%. Best-value selection and fetch-order selection now disagree. + mockClient := newMaxInstancesMockClientWith(countPerRec, func(i, j int) float64 { + if i == 0 { + return 10.0 + float64(j) + } + return 40.0 + 5*float64(j) + }) + t.Cleanup(func() { mockClient.AssertExpectations(t) }) + + accountCache := NewAccountAliasCache(awsCfg) + allRecs, drops := fetchAllRecs(ctx, awsCfg, mockClient, accountCache, + maxInstancesFixtureServices, engineVersionData{}, toolCfg, nil) + require.Len(t, allRecs, len(maxInstancesFixtureServices)*len(maxInstancesFixtureRegions)) + + scored := scoreLimitAndDisplay(allRecs, toolCfg, drops) + + // The total is respected under either placement, so it proves nothing on + // its own here. It is asserted only to keep the fixture honest. + require.Equal(t, maxInstances, CalculateTotalInstances(scored.Passed)) + + // The property under test: every purchased instance comes from the + // highest-savings recommendations run-wide, not from the ones fetched + // first. A pre-scoring cap spends the whole budget on the 10-11% RDS recs. + for i := range scored.Passed { + rec := scored.Passed[i] + assert.Equal(t, common.ServiceElastiCache, rec.Service, + "the cap must consume the scorer's savings-sorted order, not fetch order; "+ + "%s %s at %.0f%% savings was bought while better recommendations were dropped", + rec.Service, rec.Region, rec.SavingsPercentage) + assert.GreaterOrEqual(t, rec.SavingsPercentage, 40.0) + } + + // Exact survivors: the 50% rec keeps all 6 instances, the 45% rec is + // reduced to the remaining 4, and everything at or below 40% is dropped. + require.Len(t, scored.Passed, 2) + assert.InDelta(t, 50.0, scored.Passed[0].SavingsPercentage, 0.001) + assert.Equal(t, countPerRec, scored.Passed[0].Count) + assert.InDelta(t, 45.0, scored.Passed[1].SavingsPercentage, 0.001) + assert.Equal(t, maxInstances-countPerRec, scored.Passed[1].Count) +} + +// TestMaxInstancesNeverPurchasesBelowMinCount pins that the cap cannot deliver +// a purchase smaller than the operator's --min-count floor. +// +// Moving the cap downstream of the scorer means truncation is no longer +// re-filtered by the scorer's --min-count gate, so a survivor the cap trims to +// fit the budget could land under a floor the operator explicitly set. Asking +// for "at least 5" and being sold 2 is wrong regardless of which direction it +// errs in, and a commitment below the minimum can be worse than none at all. +// +// Fixture: 6 recommendations of 6 instances each, cap 10, --min-count 5. The +// budget leaves room for 6 + 4, and that 4 is below the floor, so the second +// recommendation must be dropped rather than purchased short. The run buys 6. +func TestMaxInstancesNeverPurchasesBelowMinCount(t *testing.T) { + const ( + maxInstances = 10 + minCount = 5 + countPerRec = 6 + ) + + ctx := context.Background() + awsCfg := aws.Config{Region: "us-east-1"} + + origCfg := toolCfg + t.Cleanup(func() { toolCfg = origCfg }) + + toolCfg.Coverage = 100.0 + toolCfg.PaymentOption = "partial-upfront" + toolCfg.TermYears = 1 + toolCfg.Regions = maxInstancesFixtureRegions + toolCfg.MaxInstances = maxInstances + toolCfg.MinCount = minCount + + mockClient := newMaxInstancesMockClientWith(countPerRec, func(i, j int) float64 { + return 50.0 - 5*float64(i*len(maxInstancesFixtureRegions)+j) + }) + t.Cleanup(func() { mockClient.AssertExpectations(t) }) + + accountCache := NewAccountAliasCache(awsCfg) + allRecs, drops := fetchAllRecs(ctx, awsCfg, mockClient, accountCache, + maxInstancesFixtureServices, engineVersionData{}, toolCfg, nil) + require.Len(t, allRecs, len(maxInstancesFixtureServices)*len(maxInstancesFixtureRegions)) + + scored := scoreLimitAndDisplay(allRecs, toolCfg, drops) + + // The property under test. Every purchased recommendation honors the + // floor; none is bought at a truncated count below it. + for i := range scored.Passed { + assert.GreaterOrEqual(t, scored.Passed[i].Count, minCount, + "--min-count %d was set, but %s %s would be purchased at count=%d", + minCount, scored.Passed[i].Service, scored.Passed[i].Region, scored.Passed[i].Count) + } + + // The truncated second recommendation is dropped, not reduced to 4. + require.Len(t, scored.Passed, 1) + assert.Equal(t, countPerRec, scored.Passed[0].Count) + assert.Equal(t, countPerRec, CalculateTotalInstances(scored.Passed)) + + // Dropped for the min-count reason, not silently, and counted exactly + // once: 5 of the 6 scored recommendations are gone, 1 of them to the + // floor and 4 to the budget. Double-counting the floor drop under both + // reasons would report 6 drops for 5 recommendations. + summary := drops.FormatOneLine() + assert.Contains(t, summary, common.DropMinCountAfterCap+"=1") + assert.Contains(t, summary, common.DropMaxInstances+"=4") + assert.Equal(t, 5, drops.Total()) +} + +// TestDropTruncatedBelowMinCount covers the floor helper directly, including +// the boundary (a truncated count exactly equal to the floor is kept) and the +// disabled case. +func TestDropTruncatedBelowMinCount(t *testing.T) { + tests := []struct { + name string + counts []int + minCount int + wantKept []int + wantRemoved int + }{ + {name: "floor disabled keeps everything", counts: []int{5, 2}, minCount: 0, wantKept: []int{5, 2}}, + {name: "truncated tail below floor is dropped", counts: []int{6, 4}, minCount: 5, wantKept: []int{6}, wantRemoved: 1}, + {name: "truncated tail exactly at floor is kept", counts: []int{6, 5}, minCount: 5, wantKept: []int{6, 5}}, + {name: "tail above floor is kept", counts: []int{6, 6}, minCount: 5, wantKept: []int{6, 6}}, + {name: "every rec below floor leaves nothing", counts: []int{2}, minCount: 5, wantKept: []int{}, wantRemoved: 1}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + recs := make([]common.Recommendation, len(tt.counts)) + for i, c := range tt.counts { + recs[i] = common.Recommendation{Service: common.ServiceRDS, Region: "us-east-1", Count: c} + } + + kept, removed := dropTruncatedBelowMinCount(recs, tt.minCount) + + gotCounts := make([]int, len(kept)) + for i := range kept { + gotCounts[i] = kept[i].Count + } + assert.Equal(t, tt.wantKept, gotCounts) + assert.Len(t, removed, tt.wantRemoved) + }) + } +} + +// TestMaxInstancesNotAppliedWhenUnset guards the other direction: with the flag +// unset the fan-out is purchased in full, so the cap cannot silently shrink a +// run that never asked for one. +func TestMaxInstancesNotAppliedWhenUnset(t *testing.T) { + ctx := context.Background() + awsCfg := aws.Config{Region: "us-east-1"} + + origCfg := toolCfg + t.Cleanup(func() { toolCfg = origCfg }) + + toolCfg.Coverage = 100.0 + toolCfg.PaymentOption = "partial-upfront" + toolCfg.TermYears = 1 + toolCfg.Regions = maxInstancesFixtureRegions + toolCfg.MaxInstances = 0 + + mockClient := newMaxInstancesMockClient() + t.Cleanup(func() { mockClient.AssertExpectations(t) }) + + accountCache := NewAccountAliasCache(awsCfg) + allRecs, drops := fetchAllRecs(ctx, awsCfg, mockClient, accountCache, + maxInstancesFixtureServices, engineVersionData{}, toolCfg, nil) + + scored := scoreLimitAndDisplay(allRecs, toolCfg, drops) + + pairs := len(maxInstancesFixtureServices) * len(maxInstancesFixtureRegions) + assert.Len(t, scored.Passed, pairs) + assert.Equal(t, pairs*countPerFixtureRec, CalculateTotalInstances(scored.Passed)) + assert.NotContains(t, drops.FormatOneLine(), common.DropMaxInstances) +} + +func TestApplyGlobalInstanceLimit(t *testing.T) { + recs := []common.Recommendation{ + {Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.small", Count: 5}, + {Service: common.ServiceEC2, Region: "us-west-2", ResourceType: "m5.large", Count: 4}, + {Service: common.ServiceEC2, Region: "eu-west-1", ResourceType: "m5.xlarge", Count: 3}, + } + + tests := []struct { + name string + maxInstances int32 + expectedTotal int + expectedLen int + expectedDrops int + }{ + {name: "unset leaves the run untouched", maxInstances: 0, expectedTotal: 12, expectedLen: 3}, + {name: "cap above the total leaves the run untouched", maxInstances: 99, expectedTotal: 12, expectedLen: 3}, + {name: "cap truncates the tail", maxInstances: 7, expectedTotal: 7, expectedLen: 2, expectedDrops: 1}, + {name: "cap below the first rec keeps one reduced rec", maxInstances: 2, expectedTotal: 2, expectedLen: 1, expectedDrops: 2}, + {name: "cap equal to the total leaves the run untouched", maxInstances: 12, expectedTotal: 12, expectedLen: 3}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + drops := common.NewDropSummary() + got := applyGlobalInstanceLimit(recs, Config{MaxInstances: tt.maxInstances}, drops) + + assert.Len(t, got, tt.expectedLen) + assert.Equal(t, tt.expectedTotal, CalculateTotalInstances(got)) + + if tt.expectedDrops == 0 { + assert.Empty(t, drops.FormatOneLine()) + return + } + assert.Contains(t, drops.FormatOneLine(), common.DropMaxInstances) + assert.Equal(t, tt.expectedDrops, drops.Total()) + }) + } + + // The input slice must not be mutated: the caller still renders it. + assert.Equal(t, 5, recs[0].Count) + assert.Equal(t, 4, recs[1].Count) + assert.Equal(t, 3, recs[2].Count) +} + +// TestApplyInstanceLimitNonPositiveCountDoesNotCreditBudget pins that a +// non-positive Count cannot raise the remaining budget and let later +// recommendations push the run past the cap. +func TestApplyInstanceLimitNonPositiveCountDoesNotCreditBudget(t *testing.T) { + recs := []common.Recommendation{ + {ResourceType: "a", Count: 4}, + {ResourceType: "b", Count: -10}, + {ResourceType: "c", Count: 100}, + } + + got := ApplyInstanceLimit(recs, 5) + + total := 0 + for i := range got { + if got[i].Count > 0 { + total += got[i].Count + } + } + assert.LessOrEqual(t, total, 5, "a negative Count must not raise the remaining budget") +} + +// TestReportInstanceLimitNamesEveryChange checks the no-silent-clamping +// contract: each reduced and each dropped recommendation is named on stdout. +func TestReportInstanceLimitNamesEveryChange(t *testing.T) { + before := []common.Recommendation{ + {Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.small", Count: 5}, + {Service: common.ServiceEC2, Region: "us-west-2", ResourceType: "m5.large", Count: 4}, + {Service: common.ServiceEC2, Region: "eu-west-1", ResourceType: "m5.xlarge", Count: 3}, + } + after := ApplyInstanceLimit(before, 7) + + drops := common.NewDropSummary() + out := captureAppOutput(t, func() { + reportInstanceLimit(before, after, 0, CalculateTotalInstances(before), 7, drops) + }) + + assert.Contains(t, out, "--max-instances=7") + assert.Contains(t, out, "reduced") + assert.Contains(t, out, "m5.large") + assert.Contains(t, out, "dropped") + assert.Contains(t, out, "m5.xlarge") +} diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index b30394a91..2c09d8ddd 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -351,10 +351,15 @@ func TestProcessService_SavingsPlansAccountLevel(t *testing.T) { } func TestProcessService_WithInstanceLimit(t *testing.T) { - // Note: This test verifies that processService runs without error when MaxInstances is set. - // The actual instance limiting logic is tested in TestApplyInstanceLimit. - // In processService, the limit is applied per-region after duplicate checking, - // so without a real service client, the behavior may differ from production. + // --max-instances is a run-wide cap, so the per-region path deliberately + // does NOT apply it (that was #1608: applying it here capped each + // service/region pair independently). This legacy entry point therefore + // returns the uncapped recommendations; the cap is enforced once by + // scoreLimitAndDisplay on the real pipeline, covered by + // TestMaxInstancesCapsWholeRunAcrossServicesAndRegions. + // + // This run is a dry run, so the fail-closed purchase guard does not fire; + // TestProcessService_InstanceLimitRefusesRealPurchase covers that. ctx := context.Background() awsCfg := aws.Config{Region: "us-east-1"} @@ -377,17 +382,74 @@ func TestProcessService_WithInstanceLimit(t *testing.T) { mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return(mockRecs, nil) accountCache := NewAccountAliasCache(awsCfg) - recs, _ := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceRDS, true, toolCfg, engineVersionData{}) + recs, results := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceRDS, true, toolCfg, engineVersionData{}) - // Verify the function runs without error and returns recommendations - assert.NotEmpty(t, recs, "Should return recommendations") + assert.Len(t, recs, 2, "Should return recommendations") - // Note: The actual instance limit enforcement happens inside the function - // but may not be reflected in the results due to missing service client for duplicate checking + // Assert on the *results*, not on recs. processRegionRecommendations assigns + // result.recommendations before the cap block and did so pre-fix too, so a + // count taken from recs passes either way and would be a vacuous guard. + // The dry-run results carry the recommendations that were actually handed + // to processPurchaseLoop, which is what the cap used to shrink: pre-fix + // these totalled 15 (10 + 5 truncated), post-fix they total the full 20. + resultInstances := 0 + for i := range results { + resultInstances += results[i].Recommendation.Count + } + assert.Len(t, results, 2) + assert.Equal(t, 20, resultInstances, + "the per-region path must not apply --max-instances; the cap is run-wide") mockClient.AssertExpectations(t) } +// TestProcessService_InstanceLimitRefusesRealPurchase pins the fail-closed +// contract on the legacy per-region entry point: it cannot see the other +// services and regions in the run, so it cannot evaluate the run-wide +// --max-instances cap. Rather than purchase uncapped it must make no purchases +// at all. An uncapped over-purchase of reserved capacity is not reversible. +func TestProcessService_InstanceLimitRefusesRealPurchase(t *testing.T) { + ctx := context.Background() + awsCfg := aws.Config{Region: "us-east-1"} + + origCfg := toolCfg + t.Cleanup(func() { toolCfg = origCfg }) + + toolCfg.Coverage = 100.0 + toolCfg.PaymentOption = "partial-upfront" + toolCfg.TermYears = 1 + toolCfg.Regions = []string{"us-east-1"} + toolCfg.MaxInstances = 15 + + mockClient := &MockRecommendationsClient{} + mockRecs := []common.Recommendation{ + {ResourceType: "db.t3.micro", Count: 10, Region: "us-east-1", EstimatedSavings: 100}, + {ResourceType: "db.t3.small", Count: 10, Region: "us-east-1", EstimatedSavings: 200}, + } + mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return(mockRecs, nil) + t.Cleanup(func() { mockClient.AssertExpectations(t) }) + + accountCache := NewAccountAliasCache(awsCfg) + + var recs []common.Recommendation + var results []common.PurchaseResult + out := captureAppOutput(t, func() { + recs, results = processService(ctx, awsCfg, mockClient, accountCache, common.ServiceRDS, false, toolCfg, engineVersionData{}) + }) + + assert.NotEmpty(t, recs, "recommendations are still reported") + assert.Empty(t, results, "no purchase may be attempted when the cap cannot be evaluated") + + // The refusal must be reached without touching AWS. The duplicate check is + // the only cloud call on this path, and without credentials it logs this + // warning, so its absence is positive evidence that the guard returned + // before any client was built. Before the guard was hoisted above + // createServiceClient this assertion failed: the describe ran, 403'd, and + // emitted the warning on the way to a refusal that was already decided. + assert.NotContains(t, out, "Could not check for existing RIs", + "the refusal path must not reach a cloud API call") +} + func TestProcessService_WithOverrideCount(t *testing.T) { ctx := context.Background() awsCfg := aws.Config{Region: "us-east-1"} diff --git a/docs/cli/README.md b/docs/cli/README.md index 30a83af09..9e3feac7c 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. 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). 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). | ### Purchase terms diff --git a/docs/cli/filtering.md b/docs/cli/filtering.md index 53dd1c167..6d3993ce8 100644 --- a/docs/cli/filtering.md +++ b/docs/cli/filtering.md @@ -175,6 +175,15 @@ cudly --services ec2 --max-break-even-months 18 Cap the total number of instances purchased across all recommendations after coverage scaling. Applied as a final cut after all other filters. This is a safety net to prevent unexpectedly large batch purchases, not a primary sizing control. +The cap covers the whole run on both code paths: it is applied once to the combined set of recommendations, never once per service or per region. + +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. + +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. + ```bash # Never purchase more than 100 instances in a single run cudly --services rds --max-instances 100 diff --git a/pkg/common/drop_summary.go b/pkg/common/drop_summary.go index 165c5c1eb..edf34ea82 100644 --- a/pkg/common/drop_summary.go +++ b/pkg/common/drop_summary.go @@ -17,6 +17,8 @@ const ( DropFamilyNoNUSignal = "family-nu-no-nu-signal" DropFamilySizedToZero = "family-nu-sized-to-zero" DropDuplicateDedup = "duplicate-dedup" + DropMaxInstances = "--max-instances" + DropMinCountAfterCap = "--min-count-after-cap" ) // DropSummary accumulates the count of recommendations dropped per reason