diff --git a/CHANGELOG.md b/CHANGELOG.md index 012dad5a8..1a2f57d5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). `--min-count` floor no longer drives the tally negative (which panicked in `DropSummary.Add`), and removed zero-Count entries are now counted and named (#1742). +- `--target-coverage` RDS recommendations now report a `ProjectedCoverage` that + matches the final count after `--max-instances`, `--count-override` or the + duplicate-RI check lowered it, instead of the pre-cap family value (#2214). - `--max-instances` and `--min-count` no longer apply to Savings Plans, whose Count of 1 is not an instance count: a Savings Plan neither uses up the instance cap nor is dropped by the floor (#1613). diff --git a/cmd/helpers.go b/cmd/helpers.go index 7fef0217e..03b68a0fb 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -308,7 +308,9 @@ func mapRecs(recs []common.Recommendation, fn func(common.Recommendation) common // usage signal (zero, negative, NaN or infinite avg) and rows without a positive // Count are returned unchanged. // RDS ProjectedCoverage is a family aggregate set by family-NU sizing, which a -// per-row formula would corrupt, so only RDS ProjectedUtilization is refreshed. +// per-row formula would corrupt, so only RDS ProjectedUtilization is refreshed; +// the family coverage is re-derived once on the final set by +// rederiveRDSFamilyCoverage (#2214). func rederiveProjections(rec common.Recommendation) common.Recommendation { avg := rec.AverageInstancesUsedPerHour if common.IsSavingsPlan(rec.Service) || rec.Count <= 0 || !(avg > 0) || math.IsInf(avg, 1) { diff --git a/cmd/instance_limit_savings_plan_test.go b/cmd/instance_limit_savings_plan_test.go index 3e769ce19..423fa494c 100644 --- a/cmd/instance_limit_savings_plan_test.go +++ b/cmd/instance_limit_savings_plan_test.go @@ -101,7 +101,7 @@ func assertSameRelativeOrder(t *testing.T, in, got []common.Recommendation) { // scoreLimitAndDisplay hands MinCount to scorer.Score, which must exempt // Savings Plans (Count 1 is a placeholder) at the pinned library version. func TestScoreLimitAndDisplay_MinCountKeepsSavingsPlan(t *testing.T) { - result := scoreLimitAndDisplay([]common.Recommendation{mkSP(), mkRI("a", 3)}, Config{MinCount: 2}, nil) + result := scoreLimitAndDisplay([]common.Recommendation{mkSP(), mkRI("a", 3)}, Config{MinCount: 2}, nil, nil) types := make([]common.CommitmentType, 0, len(result.Passed)) for _, r := range result.Passed { diff --git a/cmd/multi_service.go b/cmd/multi_service.go index bf1a39ccc..eba5be7d5 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -194,7 +194,7 @@ func runToolMultiService(ctx context.Context, cfg Config) { exitIfInterrupted(ctx, "fetching recommendations") // Phase 2: score, enforce the run-wide instance cap, and display. - scoredResult := scoreLimitAndDisplay(allRecs, cfg, drops) + scoredResult := scoreLimitAndDisplay(allRecs, cfg, drops, coverageMap) if len(scoredResult.Passed) == 0 { printDropSummary(drops) AppLogger.Printf("\nℹ️ No recommendations passed filters. Nothing to purchase.\n") @@ -348,7 +348,12 @@ func loadAWSConfig(ctx context.Context, cfg Config) (aws.Config, error) { // 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 { +// +// RDS family coverage is re-derived after the cap (see rederiveRDSFamilyCoverage), +// because the cap, --count-override and the duplicate check all change Count +// after family-NU sizing wrote ProjectedCoverage (#2214). coverageMap must be +// the map the sizing stage used. +func scoreLimitAndDisplay(recs []common.Recommendation, cfg Config, drops *common.DropSummary, coverageMap recommendations.PoolCoverageMap) scorer.ScoredResult { scorerCfg := scorer.Config{ MinSavingsPct: cfg.MinSavingsPct, MaxBreakEvenMonths: cfg.MaxBreakEvenMonths, @@ -356,12 +361,25 @@ func scoreLimitAndDisplay(recs []common.Recommendation, cfg Config, drops *commo } result := scorer.Score(recs, scorerCfg) result.Passed = applyGlobalInstanceLimit(result.Passed, cfg, rankBySavingsPercentage, drops) + result.Passed = rederiveRDSFamilyCoverage(result.Passed, cfg, coverageMap) fmt.Print(reporter.RenderTable(result)) fmt.Print(reporter.RenderExcluded(result)) fmt.Print(reporter.RenderSummary(result)) return result } +// rederiveRDSFamilyCoverage refreshes the family-wide RDS ProjectedCoverage +// from the final counts. It only runs for --target-coverage, the sole path +// where ApplyFamilyNUSizingRDS wrote that field; on the legacy --coverage path +// the field is a per-pool value that RederiveFamilyCoverage would overwrite. +// The CSV path has no coverage map and does not call it. +func rederiveRDSFamilyCoverage(recs []common.Recommendation, cfg Config, coverageMap recommendations.PoolCoverageMap) []common.Recommendation { + if cfg.TargetCoverage <= 0 { + return recs + } + return recommendations.RederiveFamilyCoverage(recs, coverageMap) +} + // rankingRule names the ordering --max-instances consumes, so the cap can say // which rule decided what survived. The two paths rank on different keys // because they carry different data, and an operator reading the drop list has diff --git a/cmd/multi_service_max_instances_test.go b/cmd/multi_service_max_instances_test.go index 9a781e834..1e624f76a 100644 --- a/cmd/multi_service_max_instances_test.go +++ b/cmd/multi_service_max_instances_test.go @@ -103,7 +103,7 @@ func TestMaxInstancesCapsWholeRunAcrossServicesAndRegions(t *testing.T) { require.Equal(t, naturalTotal, CalculateTotalInstances(allRecs), "fetch stage must not apply the cap per region") - scored := scoreLimitAndDisplay(allRecs, toolCfg, drops) + scored := scoreLimitAndDisplay(allRecs, toolCfg, drops, nil) total := CalculateTotalInstances(scored.Passed) assert.LessOrEqual(t, total, maxInstances, @@ -176,7 +176,7 @@ func TestMaxInstancesKeepsHighestSavingsNotFirstFetched(t *testing.T) { awsStub.requireDuplicateChecks(t, len(maxInstancesFixtureServices)*len(maxInstancesFixtureRegions)) require.Len(t, allRecs, len(maxInstancesFixtureServices)*len(maxInstancesFixtureRegions)) - scored := scoreLimitAndDisplay(allRecs, toolCfg, drops) + scored := scoreLimitAndDisplay(allRecs, toolCfg, drops, nil) // The total is respected under either placement, so it proves nothing on // its own here. It is asserted only to keep the fixture honest. @@ -246,7 +246,7 @@ func TestMaxInstancesNeverPurchasesBelowMinCount(t *testing.T) { awsStub.requireDuplicateChecks(t, len(maxInstancesFixtureServices)*len(maxInstancesFixtureRegions)) require.Len(t, allRecs, len(maxInstancesFixtureServices)*len(maxInstancesFixtureRegions)) - scored := scoreLimitAndDisplay(allRecs, toolCfg, drops) + scored := scoreLimitAndDisplay(allRecs, toolCfg, drops, nil) // The property under test. Every purchased recommendation honors the // floor; none is bought at a truncated count below it. @@ -333,7 +333,7 @@ func TestMaxInstancesNotAppliedWhenUnset(t *testing.T) { maxInstancesFixtureServices, engineVersionData{}, toolCfg, nil) awsStub.requireDuplicateChecks(t, len(maxInstancesFixtureServices)*len(maxInstancesFixtureRegions)) - scored := scoreLimitAndDisplay(allRecs, toolCfg, drops) + scored := scoreLimitAndDisplay(allRecs, toolCfg, drops, nil) pairs := len(maxInstancesFixtureServices) * len(maxInstancesFixtureRegions) assert.Len(t, scored.Passed, pairs) diff --git a/cmd/multi_service_rederive_coverage_test.go b/cmd/multi_service_rederive_coverage_test.go new file mode 100644 index 000000000..c70a78592 --- /dev/null +++ b/cmd/multi_service_rederive_coverage_test.go @@ -0,0 +1,66 @@ +package main + +import ( + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/recommendations" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func rederiveRDSRec(count int) common.Recommendation { + return common.Recommendation{ + Service: common.ServiceRDS, CommitmentType: common.CommitmentReservedInstance, + Region: "us-east-1", ResourceType: "db.r7g.large", Count: count, + CommitmentCost: float64(count) * 100, EstimatedSavings: 100, + AverageInstancesUsedPerHour: 3, + Details: &common.DatabaseDetails{Engine: "aurora-mysql", AZConfig: "single-az"}, + } +} + +var rederiveCoverage = recommendations.PoolCoverageMap{ + "us-east-1:db.r7g.large:auroramysql:singleaz": {Pct: 20, AvgInstancesPerHour: 40}, +} + +// sizedRederiveRec returns one family-NU sized RDS rec (the --target-coverage +// output) and its ProjectedCoverage. +func sizedRederiveRec(t *testing.T) common.Recommendation { + t.Helper() + sized, _, _ := recommendations.ApplyFamilyNUSizingRDS([]common.Recommendation{rederiveRDSRec(30)}, rederiveCoverage, 90) + require.Len(t, sized, 1) + require.Greater(t, sized[0].Count, 5) + return sized[0] +} + +func TestScoreLimitAndDisplay_TargetCoverageRederivesRDSCoverageAfterCap(t *testing.T) { + sized := sizedRederiveRec(t) + uncapped := sized.ProjectedCoverage + + got := scoreLimitAndDisplay([]common.Recommendation{sized}, Config{TargetCoverage: 90, MaxInstances: 5}, nil, rederiveCoverage) + + require.Len(t, got.Passed, 1) + assert.Equal(t, 5, got.Passed[0].Count) + // existing 20% + 5 instances of 40 = 12.5% + assert.InDelta(t, 32.5, got.Passed[0].ProjectedCoverage, 1e-6) + assert.Less(t, got.Passed[0].ProjectedCoverage, uncapped) +} + +func TestScoreLimitAndDisplay_LegacyCoverageLeavesProjectedCoverage(t *testing.T) { + rec := rederiveRDSRec(30) + rec.ExistingCoverageKnown, rec.ExistingCoveragePct, rec.ProjectedCoverage = true, 20, 77 + + got := scoreLimitAndDisplay([]common.Recommendation{rec}, Config{Coverage: 80, MaxInstances: 5}, nil, rederiveCoverage) + + require.Len(t, got.Passed, 1) + assert.Equal(t, 77.0, got.Passed[0].ProjectedCoverage) +} + +func TestScoreLimitAndDisplay_CSVPathNilCoverageUnchanged(t *testing.T) { + sized := sizedRederiveRec(t) + + got := scoreLimitAndDisplay([]common.Recommendation{sized}, Config{TargetCoverage: 90}, nil, nil) + + require.Len(t, got.Passed, 1) + assert.Equal(t, sized.ProjectedCoverage, got.Passed[0].ProjectedCoverage) +}