From 0e01fc799255faa9eafe2eba2bee8b0dab7fd0fc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 00:14:05 +0200 Subject: [PATCH 1/4] fix(cli): apply --max-instances once run-wide, not per service and region --max-instances is documented in three places (cmd/main.go flag help, docs/cli/README.md, docs/cli/filtering.md) as a hard cap on the total number of instances purchased across all recommendations. On the main purchase path it was applied inside checkDuplicatesAndApplyLimit, which runs once per (service, region) pair, so every pair independently kept up to MaxInstances and the merged set was never re-capped. A run of `--all-services --purchase --max-instances 10` fans out to 6 RI services across every opted-in region, so the operator's cap of 10 could authorise on the order of 1500 instances. The per-region log line ("limiting to 10 instances", printed once per region) read exactly like the cap was holding. Changes: - Remove the cap from the per-region path and rename the helper to checkDuplicates, so its name matches what it does. - Apply the cap once in scoreLimitAndDisplay, between scoring and rendering. Running it after the scorer means the survivors are the highest-savings recommendations run-wide rather than the first ones fetched, and running it before rendering means the table, the confirmation prompt and the purchase loop all describe the same post-cap set. - Report every reduced and dropped recommendation by name, and count the drops into the end-of-run summary under a new --max-instances reason. A capped run must never shrink silently. - Fail closed on the legacy per-region entry point: it cannot see the rest of the run, so it cannot evaluate a run-wide cap. It now refuses to purchase when --max-instances is set instead of purchasing uncapped. An over-purchase of reserved capacity is not reversible. - Do not credit a non-positive Count back to the remaining budget in ApplyInstanceLimit. Moving the cap post-merge also closes a fail-open case: the cap was previously skipped entirely for any region where createServiceClient returned nil, because it lived behind that nil check. The --input-csv path already applied the cap globally and is unchanged. Regression test: TestMaxInstancesCapsWholeRunAcrossServicesAndRegions drives fetchAllRecs plus scoreLimitAndDisplay over 2 services x 3 regions, each answering 8 instances (48 natural total) with the cap at 10, and asserts the sum across every service and region. Against the pre-fix behaviour it fails with "48 is not less than or equal to 10"; it passes after. A single-service or single-region fixture stays green with the bug present, which is why it fans out on both dimensions. Closes #1608 --- cmd/helpers.go | 18 +- cmd/multi_service.go | 76 +++++++- cmd/multi_service_helpers.go | 41 ++-- cmd/multi_service_max_instances_test.go | 238 ++++++++++++++++++++++++ cmd/multi_service_test.go | 56 +++++- docs/cli/filtering.md | 2 + pkg/common/drop_summary.go | 1 + 7 files changed, 402 insertions(+), 30 deletions(-) create mode 100644 cmd/multi_service_max_instances_test.go 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..c02bb4ab0 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,88 @@ 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. +// +// 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) + reportInstanceLimit(passed, limited, totalBefore, cfg.MaxInstances, drops) + return limited +} + +// 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. +func reportInstanceLimit(before, after []common.Recommendation, 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 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) + 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..fc9b14a16 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -407,8 +407,19 @@ 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) + + // --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. + 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 + } // Process purchases regionResults := processPurchaseLoop(ctx, adjustedRecs, region, isDryRun, serviceClient, cfg) @@ -548,13 +559,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 +592,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 +652,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..82f6fcdbf --- /dev/null +++ b/cmd/multi_service_max_instances_test.go @@ -0,0 +1,238 @@ +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 + +// newMaxInstancesMockClient returns a recommendations client that answers one +// recommendation per (service, region) pair, with distinct savings percentages +// so the scorer's ordering is deterministic. +func newMaxInstancesMockClient() *MockRecommendationsClient { + mockClient := &MockRecommendationsClient{} + for i, svc := range maxInstancesFixtureServices { + for j, region := range maxInstancesFixtureRegions { + svc, region := svc, region + // Descending in fan-out order: 50, 45, 40, ... so the scorer's + // savings-first ordering is unambiguous. + savings := 50.0 - 5*float64(i*len(maxInstancesFixtureRegions)+j) + rec := common.Recommendation{ + Service: svc, + Region: region, + ResourceType: "db.t3.small", + Count: countPerFixtureRec, + 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 +} + +// 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") +} + +// 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, 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..2c3f86e51 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"} @@ -379,15 +384,48 @@ func TestProcessService_WithInstanceLimit(t *testing.T) { accountCache := NewAccountAliasCache(awsCfg) recs, _ := 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") - - // 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 + // The per-region path must hand back the full 20 instances, not 15: capping + // here would re-introduce the per-(service, region) multiplication. + assert.Len(t, recs, 2, "Should return recommendations") + assert.Equal(t, 20, CalculateTotalInstances(recs), + "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) + 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") +} + func TestProcessService_WithOverrideCount(t *testing.T) { ctx := context.Background() awsCfg := aws.Config{Region: "us-east-1"} diff --git a/docs/cli/filtering.md b/docs/cli/filtering.md index 53dd1c167..baffef7b3 100644 --- a/docs/cli/filtering.md +++ b/docs/cli/filtering.md @@ -175,6 +175,8 @@ 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: it is applied once to the combined set of recommendations from every service and every region, not once per service or per region. It is applied after scoring, so the instances that survive are the highest-savings ones run-wide. Recommendations that 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 silently shrinks. + ```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..a85d60f5e 100644 --- a/pkg/common/drop_summary.go +++ b/pkg/common/drop_summary.go @@ -17,6 +17,7 @@ const ( DropFamilyNoNUSignal = "family-nu-no-nu-signal" DropFamilySizedToZero = "family-nu-sized-to-zero" DropDuplicateDedup = "duplicate-dedup" + DropMaxInstances = "--max-instances" ) // DropSummary accumulates the count of recommendations dropped per reason From f6c0821ec8f95d56b87442c63bd3e86ee943a8fa Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 00:47:43 +0200 Subject: [PATCH 2/4] test(cli): pin the --max-instances selection order, not just the total ApplyInstanceLimit consumes its input in slice order and drops the tail, so whatever ordering it is handed decides which commitments get bought. Capping the merged set before scoring and capping the scorer's savings-sorted output both satisfy "total <= cap", so TestMaxInstancesCapsWholeRunAcrossServicesAndRegions passes under either placement. The ordering contract existed only in comments, which is one refactor away from being violated with a green suite. TestMaxInstancesKeepsHighestSavingsNotFirstFetched closes that gap. Its fixture makes best-value selection and fetch-order selection disagree: the first-fetched service (RDS) carries 10-12% savings and the second-fetched (ElastiCache) carries 40-50%, with a cap admitting 10 of 36 available instances. Every purchased instance must come from ElastiCache. Verified by mutation. Moving the cap in scoreLimitAndDisplay from result.Passed to the pre-scoring input: TestMaxInstancesKeepsHighestSavingsNotFirstFetched FAIL expected: "elasticache" actual : "rds" the cap must consume the scorer's savings-sorted order, not fetch order; rds us-east-1 at 10% savings was bought while better recommendations were dropped TestMaxInstancesCapsWholeRunAcrossServicesAndRegions PASS That contrast is the point: the pre-existing test cannot see the defect the new one catches. Both pass once the cap is back after scoring. The shared mock builder is parameterised over the savings-by-fan-out position so both fixtures share one construction path; no production code changes. --- cmd/multi_service_max_instances_test.go | 99 +++++++++++++++++++++++-- 1 file changed, 91 insertions(+), 8 deletions(-) diff --git a/cmd/multi_service_max_instances_test.go b/cmd/multi_service_max_instances_test.go index 82f6fcdbf..f79ee4564 100644 --- a/cmd/multi_service_max_instances_test.go +++ b/cmd/multi_service_max_instances_test.go @@ -23,22 +23,21 @@ var ( // countPerFixtureRec is the instance count each (service, region) pair returns. const countPerFixtureRec = 8 -// newMaxInstancesMockClient returns a recommendations client that answers one -// recommendation per (service, region) pair, with distinct savings percentages -// so the scorer's ordering is deterministic. -func newMaxInstancesMockClient() *MockRecommendationsClient { +// 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 - // Descending in fan-out order: 50, 45, 40, ... so the scorer's - // savings-first ordering is unambiguous. - savings := 50.0 - 5*float64(i*len(maxInstancesFixtureRegions)+j) + savings := savingsFor(i, j) rec := common.Recommendation{ Service: svc, Region: region, ResourceType: "db.t3.small", - Count: countPerFixtureRec, + Count: count, EstimatedSavings: savings * 10, SavingsPercentage: savings, } @@ -52,6 +51,14 @@ func newMaxInstancesMockClient() *MockRecommendationsClient { 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. // @@ -119,6 +126,82 @@ func TestMaxInstancesCapsWholeRunAcrossServicesAndRegions(t *testing.T) { 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) +} + // 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. From 326112eca52cadf710b988e55228fdb7616c06ef Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 01:44:06 +0200 Subject: [PATCH 3/4] fix(cli): drop recommendations --max-instances truncates below --min-count Moving the cap downstream of the scorer left truncation unfiltered by the scorer's --min-count gate. A survivor trimmed to fit the budget could land under a floor the operator explicitly set: --min-count 5 with --max-instances 10 purchased a second recommendation at count=2. --min-count is a hard floor everywhere else, never advice. The scorer rejects recommendations under it outright (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 gates the sized count, and truncation by the cap is another form of sizing. Asking for at least 5 and being sold 2 is wrong in either direction: a commitment below the minimum can be worse than no commitment, which is why the floor exists. A recommendation the cap truncates below --min-count is now dropped rather than purchased short, named on stdout with that reason, and counted under a new --min-count-after-cap drop reason. The freed budget is not redistributed; the next recommendation would face an even smaller remainder and the same floor. Removal is always from the tail, since ApplyInstanceLimit reduces at most one recommendation and every earlier one still carries the full count that already cleared the scorer's floor. That keeps the result a prefix of the input, which reportInstanceLimit relies on for positional correspondence. Blast radius is exactly this one flag. The scorer's other filters are scale-invariant: MinSavingsPct reads SavingsPercentage and MaxBreakEvenMonths reads BreakEvenMonths, both ratios unchanged by scaling Count, and the service filter does not look at Count at all. Each dropped recommendation is counted exactly once in the end-of-run summary: reportInstanceLimit excludes the floor drops from its --max-instances tally so they are attributed only to --min-count-after-cap. Also corrects two claims that were wrong rather than merely imprecise: - TestProcessService_WithInstanceLimit asserted on the returned recommendations, which processRegionRecommendations assigns before the cap block and assigned in the same place pre-fix, so the assertion passed on pre-fix code and guarded nothing. It now asserts on the dry-run results, which carry the recommendations actually handed to processPurchaseLoop: 20 instances post-fix, 15 pre-fix. - The --max-instances docs described main-path behaviour only. runToolFromCSV never calls scorer.Score (it has exactly one caller), so --input-csv runs cap in file order. Both paths are now described, in filtering.md and in the README flag table. Regression test: TestMaxInstancesNeverPurchasesBelowMinCount runs 6 recommendations of 6 instances with --max-instances 10 and --min-count 5, asserting no survivor is below the floor. Without the floor re-applied it fails with "4 is not greater than or equal to 5" (rds us-west-2 would be purchased at count=4); the total-focused and ordering tests both stay green under that mutation, which is why neither could catch it. TestDropTruncatedBelowMinCount covers the helper's boundary directly, including a truncated count exactly equal to the floor. Refs #1608 --- cmd/multi_service.go | 69 ++++++++++++++- cmd/multi_service_max_instances_test.go | 106 +++++++++++++++++++++++- cmd/multi_service_test.go | 18 +++- docs/cli/README.md | 2 +- docs/cli/filtering.md | 9 +- pkg/common/drop_summary.go | 1 + 6 files changed, 194 insertions(+), 11 deletions(-) diff --git a/cmd/multi_service.go b/cmd/multi_service.go index c02bb4ab0..58b613b13 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -236,6 +236,10 @@ func scoreLimitAndDisplay(recs []common.Recommendation, cfg Config, drops *commo // 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 { @@ -248,17 +252,74 @@ func applyGlobalInstanceLimit(passed []common.Recommendation, cfg Config, drops } limited := ApplyInstanceLimit(passed, cfg.MaxInstances) - reportInstanceLimit(passed, limited, totalBefore, cfg.MaxInstances, drops) + 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. -func reportInstanceLimit(before, after []common.Recommendation, totalBefore int, maxInstances int32, drops *common.DropSummary) { +// +// 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 recommendations first. The following are reduced or dropped:\n") + AppLogger.Printf(" Keeping the highest savings-percentage recommendations first. The following are reduced or dropped:\n") reduced, dropped := 0, 0 for i := range before { @@ -279,7 +340,7 @@ func reportInstanceLimit(before, after []common.Recommendation, totalBefore int, } } - drops.Add(common.DropMaxInstances, dropped) + 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) } diff --git a/cmd/multi_service_max_instances_test.go b/cmd/multi_service_max_instances_test.go index f79ee4564..b1f70e711 100644 --- a/cmd/multi_service_max_instances_test.go +++ b/cmd/multi_service_max_instances_test.go @@ -202,6 +202,110 @@ func TestMaxInstancesKeepsHighestSavingsNotFirstFetched(t *testing.T) { 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. @@ -310,7 +414,7 @@ func TestReportInstanceLimitNamesEveryChange(t *testing.T) { drops := common.NewDropSummary() out := captureAppOutput(t, func() { - reportInstanceLimit(before, after, CalculateTotalInstances(before), 7, drops) + reportInstanceLimit(before, after, 0, CalculateTotalInstances(before), 7, drops) }) assert.Contains(t, out, "--max-instances=7") diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index 2c3f86e51..7662e218c 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -382,12 +382,22 @@ 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{}) - // The per-region path must hand back the full 20 instances, not 15: capping - // here would re-introduce the per-(service, region) multiplication. assert.Len(t, recs, 2, "Should return recommendations") - assert.Equal(t, 20, CalculateTotalInstances(recs), + + // 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) 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 baffef7b3..6d3993ce8 100644 --- a/docs/cli/filtering.md +++ b/docs/cli/filtering.md @@ -175,7 +175,14 @@ 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: it is applied once to the combined set of recommendations from every service and every region, not once per service or per region. It is applied after scoring, so the instances that survive are the highest-savings ones run-wide. Recommendations that 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 silently shrinks. +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 diff --git a/pkg/common/drop_summary.go b/pkg/common/drop_summary.go index a85d60f5e..edf34ea82 100644 --- a/pkg/common/drop_summary.go +++ b/pkg/common/drop_summary.go @@ -18,6 +18,7 @@ const ( 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 From 93fe75e5ffee53d74312edea45f2c0e55b9b1acc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 02:14:18 +0200 Subject: [PATCH 4/4] refactor(cli): hoist the --max-instances refusal above the duplicate check The fail-closed guard on the legacy per-region path sat after createServiceClient and checkDuplicates, so reaching a refusal that depends only on cfg.MaxInstances and isDryRun cost a DescribeReserved* call first. Both inputs are known as soon as the recommendations are recorded, so the guard now runs there. Two things improve. The refusal no longer spends a cloud API call to arrive at an answer that was already determined, and it no longer fails differently depending on whether that describe happened to succeed: the error branch and the success branch of the duplicate check both led to the same refusal, but only after doing the work. TestProcessService_InstanceLimitRefusesRealPurchase now captures the run's output and asserts the duplicate-check warning is absent, which is positive evidence that no client was built. Measured, not inferred: before the hoist the test emitted one "Could not check for existing RIs" warning from a real 403 and took 0.59s; after, zero warnings and 0.00s. Moving the guard back makes the new assertion fail with the 403 visible in the captured output, so it is not vacuous. This also removes the last network dependency from that test, which previously reached AWS to decide something it did not need AWS for. Refs #1608 --- cmd/multi_service_helpers.go | 30 +++++++++++++++++++----------- cmd/multi_service_test.go | 16 +++++++++++++++- 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index fc9b14a16..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 @@ -410,17 +429,6 @@ func processRegionRecommendations( // Check for duplicate RIs. Drop tracking skipped (nil). adjustedRecs := checkDuplicates(ctx, filteredRecs, serviceClient, nil) - // --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. - 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 - } - // Process purchases regionResults := processPurchaseLoop(ctx, adjustedRecs, region, isDryRun, serviceClient, cfg) result.results = regionResults diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index 7662e218c..2c09d8ddd 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -430,10 +430,24 @@ func TestProcessService_InstanceLimitRefusesRealPurchase(t *testing.T) { t.Cleanup(func() { mockClient.AssertExpectations(t) }) accountCache := NewAccountAliasCache(awsCfg) - recs, results := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceRDS, false, toolCfg, engineVersionData{}) + + 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) {