From 648e1c5be57f617fc3d604825961a41cd7836d6f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 12 Oct 2026 19:50:40 +0200 Subject: [PATCH 1/2] feat(aws): add ApplyFamilyNUSizingRDSWithExpiry for rebuy-window RDS sizing Folds expiring active RDS RIs into family coverage in normalized units before family sizing, bounded by min(covered, active - expiring), and reports expiring RIs that cannot be deducted. Existing exports unchanged. Refs LeanerCloud/cloud-commitments-cli#2132 Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 7 + providers/aws/recommendations/family_nu.go | 108 ++++++++++++- .../recommendations/family_nu_expiry_test.go | 142 ++++++++++++++++++ 3 files changed, 256 insertions(+), 1 deletion(-) create mode 100644 providers/aws/recommendations/family_nu_expiry_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 2681a702..484d7daf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Added +- **`recommendations.ApplyFamilyNUSizingRDSWithExpiry`:** family-NU sizing + that deducts RDS RIs expiring inside the rebuy window from family coverage + in normalized units (`min(covered, active - expiring)`), and reports + expiring RIs it could not use (`FamilyExpiryReport.Unusable`). Additive; + `ApplyFamilyNUSizingRDS` is unchanged. Consumers that pass a rebuy window + will see higher RDS counts and costs for families with expiring RIs + (LeanerCloud/cloud-commitments-cli#2132). - **`insurance.ContractTerms()`:** returns the documented Archera `contract_term` values that `Comparison` accepts and the decoder returns, as a new sorted slice on every call (#316). The order is lexicographic, not by diff --git a/providers/aws/recommendations/family_nu.go b/providers/aws/recommendations/family_nu.go index 8cbd3c7f..3dd7384d 100644 --- a/providers/aws/recommendations/family_nu.go +++ b/providers/aws/recommendations/family_nu.go @@ -3,6 +3,7 @@ package recommendations import ( "math" "strings" + "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" ) @@ -188,7 +189,17 @@ func ApplyFamilyNUSizingRDS( if targetPct <= 0 || targetPct > 100 { return nil, recs, drops } - familyCov := AggregateRDSFamilyCoverage(coverage) + return sizeRDSFamilies(recs, AggregateRDSFamilyCoverage(coverage), targetPct) +} + +// sizeRDSFamilies is the shared body of ApplyFamilyNUSizingRDS and +// ApplyFamilyNUSizingRDSWithExpiry: it sizes each RDS family against the +// supplied family coverage view. +func sizeRDSFamilies( + recs []common.Recommendation, + familyCov map[string]FamilyCoverage, + targetPct float64, +) (sizedRDS, nonRDS []common.Recommendation, drops FamilyDropCounts) { familyIdx, nonRDS := partitionRDSRecsByFamily(recs) for fk, indices := range familyIdx { sized, familyDrops := sizeRDSFamilyRecs(recs, indices, familyCov[fk], targetPct) @@ -359,3 +370,98 @@ func isRDSRIRec(rec common.Recommendation) bool { } return rec.Service == common.ServiceRDS || rec.Service == common.ServiceRelationalDB } + +// FamilyExpiryReport describes how commitment expiry was folded into family +// coverage by ApplyFamilyNUSizingRDSWithExpiry. +type FamilyExpiryReport struct { + // ExpiringNU is the normalized units of active RDS RIs ending inside the + // window that were deducted from family coverage. + ExpiringNU float64 + // Unusable counts active RDS RIs expiring inside the window that could not + // be deducted: an unparsable instance class (NU 0) or a family key with no + // coverage family (e.g. an engine spelled differently from the coverage + // report). Callers must surface this; it never means "no expiry". + Unusable int +} + +// ApplyFamilyNUSizingRDSWithExpiry is ApplyFamilyNUSizingRDS with rebuy-window +// expiry folded into family coverage first, in normalized units. RDS RIs are +// size-flexible and Cost Explorer reports coverage under the size actually +// running, so the deduction is made per family (region, family, engine, +// deployment), not per size pool. For each family with expiring active RIs: +// +// post-expiry covered NU = min(CoveredNU, activeNU - expiringNU), floored at 0 +// +// activeNU sums count x NU over every active RI in the family. The min() keeps +// underutilized RIs (more RI units than usage) from overstating the gap. +// windowDays <= 0 or no commitments leaves coverage unchanged. A family +// with several size rows or linked-account rows is deducted once. +func ApplyFamilyNUSizingRDSWithExpiry( + recs []common.Recommendation, + coverage PoolCoverageMap, + targetPct float64, + commitments []common.Commitment, + windowDays int, +) (sizedRDS, nonRDS []common.Recommendation, drops FamilyDropCounts, expiry FamilyExpiryReport) { + if targetPct <= 0 || targetPct > 100 { + return nil, recs, drops, expiry + } + familyCov := AggregateRDSFamilyCoverage(coverage) + expiry = adjustFamilyCoverageForExpiry(familyCov, commitments, windowDays, time.Now()) + sizedRDS, nonRDS, drops = sizeRDSFamilies(recs, familyCov, targetPct) + return sizedRDS, nonRDS, drops, expiry +} + +// adjustFamilyCoverageForExpiry mutates familyCov (a fresh aggregate owned by +// the caller) as described on ApplyFamilyNUSizingRDSWithExpiry. +func adjustFamilyCoverageForExpiry(familyCov map[string]FamilyCoverage, commitments []common.Commitment, windowDays int, now time.Time) FamilyExpiryReport { + var report FamilyExpiryReport + if windowDays <= 0 || len(commitments) == 0 { + return report + } + cutoff := now.Add(time.Duration(windowDays) * 24 * time.Hour) + activeNU := make(map[string]float64) + expiringNU := make(map[string]float64) + for _, c := range commitments { + fk, units, expiring, ok := rdsCommitmentFamilyUnits(c, cutoff) + if !ok { + if expiring { + report.Unusable++ + } + continue + } + activeNU[fk] += units + if !expiring { + continue + } + if _, known := familyCov[fk]; !known { + report.Unusable++ + continue + } + expiringNU[fk] += units + } + for fk, exp := range expiringNU { + fc := familyCov[fk] + fc.CoveredNU = math.Min(fc.CoveredNU, math.Max(0, activeNU[fk]-exp)) + familyCov[fk] = fc + report.ExpiringNU += exp + } + return report +} + +// rdsCommitmentFamilyUnits maps an active RDS commitment to its family key and +// normalized units. ok is false when the commitment is not an active RDS RI +// (expiring is then false) or its class cannot be parsed (expiring is then +// true when it ends inside the window, so the caller can report it). +func rdsCommitmentFamilyUnits(c common.Commitment, cutoff time.Time) (fk string, units float64, expiring, ok bool) { + if (c.Service != common.ServiceRDS && c.Service != common.ServiceRelationalDB) || !commitmentIsActive(c) { + return "", 0, false, false + } + expiring = !c.EndDate.IsZero() && !c.EndDate.After(cutoff) + family := rdsFamilyFromType(c.ResourceType) + nu := rdsInstanceNUFromType(c.ResourceType) + if family == "" || nu == 0 { + return "", 0, expiring, false + } + return rdsFamilyKey(c.Region, family, c.Engine, c.Deployment), float64(c.Count) * nu, expiring, true +} diff --git a/providers/aws/recommendations/family_nu_expiry_test.go b/providers/aws/recommendations/family_nu_expiry_test.go new file mode 100644 index 00000000..39b7e60e --- /dev/null +++ b/providers/aws/recommendations/family_nu_expiry_test.go @@ -0,0 +1,142 @@ +package recommendations + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +func expiryTestCov(usageType string, avg, pct float64) PoolCoverageMap { + return PoolCoverageMap{ + rdsPoolKey("us-east-1", usageType, "MySQL", "Single-AZ"): {Pct: pct, AvgInstancesPerHour: avg}, + } +} + +func expiryTestRec(size string, count int) []common.Recommendation { + return []common.Recommendation{{ + Service: common.ServiceRDS, + CommitmentType: common.CommitmentReservedInstance, + Region: "us-east-1", + ResourceType: size, + Count: count, + CommitmentCost: 1000, OnDemandCost: 2000, EstimatedSavings: 500, + Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: "single-az"}, + }} +} + +func expiryTestRI(size, account string, count, endDays int) common.Commitment { + return common.Commitment{ + Service: common.ServiceRDS, Region: "us-east-1", ResourceType: size, + Engine: "mysql", Deployment: "single-az", Account: account, + Count: count, State: "active", + EndDate: time.Now().Add(time.Duration(endDays) * 24 * time.Hour), + } +} + +// scenario: usage 10 x db.r6g.xlarge (80 NU) at 80% covered (64 NU); active +// RIs 4 x large expiring (16 NU) + 6 x xlarge long-lived (48 NU) = 64 NU. +func expiryTestScenario() (PoolCoverageMap, []common.Recommendation, []common.Commitment) { + return expiryTestCov("db.r6g.xlarge", 10, 80), expiryTestRec("db.r6g.xlarge", 5), + []common.Commitment{expiryTestRI("db.r6g.large", "a", 4, 15), expiryTestRI("db.r6g.xlarge", "a", 6, 300)} +} + +func TestApplyFamilyNUSizingRDSWithExpiry(t *testing.T) { + t.Run("expiring large RI against xlarge usage: parent drops family, fix sizes it", func(t *testing.T) { + cov, recs, commits := expiryTestScenario() + _, _, parent := ApplyFamilyNUSizingRDS(recs, cov, 80) + assert.Equal(t, 1, parent.AlreadyAtTarget, "parent sees 80% and drops the family") + + sized, _, drops, rep := ApplyFamilyNUSizingRDSWithExpiry(recs, cov, 80, commits, 30) + // covered 64 -> min(64, 64-16) = 48 => 60%; gap 20% of 80 NU = 16 NU; + // rec 5 x xlarge = 40 NU; scale 0.4 => count 2, cost x0.4. + require.Len(t, sized, 1) + assert.Equal(t, 0, drops.AlreadyAtTarget) + assert.Equal(t, 2, sized[0].Count) + assert.InDelta(t, 400.0, sized[0].CommitmentCost, 1e-9) + assert.InDelta(t, 60.0, sized[0].ExistingCoveragePct, 1e-9) + assert.Equal(t, 16.0, rep.ExpiringNU) + assert.Zero(t, rep.Unusable) + }) + + t.Run("linked-account rows are deducted once", func(t *testing.T) { + cov, recs, _ := expiryTestScenario() + commits := []common.Commitment{ + expiryTestRI("db.r6g.large", "a", 2, 15), expiryTestRI("db.r6g.large", "b", 2, 15), + expiryTestRI("db.r6g.xlarge", "a", 6, 300), + } + cov[rdsPoolKey("us-east-1", "db.r6g.large", "MySQL", "Single-AZ")] = PoolCoverage{Pct: 0, AvgInstancesPerHour: 0} + sized, _, _, rep := ApplyFamilyNUSizingRDSWithExpiry(recs, cov, 80, commits, 30) + require.Len(t, sized, 1) + assert.Equal(t, 2, sized[0].Count, "a double deduction would give count 4") + assert.Equal(t, 16.0, rep.ExpiringNU) + }) + + t.Run("RIs exceeding usage do not inflate the gap", func(t *testing.T) { + // covered 40 NU of 80; active 10 x xlarge = 80 NU, 2 expiring (16). + // min(40, 64) = 40: unchanged 50%. Naive subtraction would give 24 NU. + cov := expiryTestCov("db.r6g.xlarge", 10, 50) + commits := []common.Commitment{expiryTestRI("db.r6g.xlarge", "a", 2, 15), expiryTestRI("db.r6g.xlarge", "a", 8, 300)} + sized, _, _, _ := ApplyFamilyNUSizingRDSWithExpiry(expiryTestRec("db.r6g.xlarge", 10), cov, 80, commits, 30) + require.Len(t, sized, 1) + assert.InDelta(t, 50.0, sized[0].ExistingCoveragePct, 1e-9) + assert.Equal(t, 3, sized[0].Count, "gap 30% of 80 NU = 24 NU over 80 NU rec") + }) + + t.Run("mixed sizes: min is in NU, not counts", func(t *testing.T) { + // active 2 x large (8 NU) + 1 expiring xlarge (8 NU) = 16 NU; covered + // 12 NU of 16; post-expiry covered = min(12, 16-8) = 8 => 50%. + cov := expiryTestCov("db.r6g.xlarge", 2, 75) + commits := []common.Commitment{expiryTestRI("db.r6g.large", "a", 2, 300), expiryTestRI("db.r6g.xlarge", "a", 1, 15)} + sized, _, _, rep := ApplyFamilyNUSizingRDSWithExpiry(expiryTestRec("db.r6g.large", 4), cov, 80, commits, 30) + require.Len(t, sized, 1) + assert.InDelta(t, 50.0, sized[0].ExistingCoveragePct, 1e-9) + assert.Equal(t, 1, sized[0].Count, "gap 30% of 16 NU = 4.8 NU over a 16 NU rec: floor(1.2)") + assert.Equal(t, 8.0, rep.ExpiringNU) + }) + + t.Run("control: outside window", func(t *testing.T) { + cov, recs, _ := expiryTestScenario() + commits := []common.Commitment{expiryTestRI("db.r6g.large", "a", 4, 180), expiryTestRI("db.r6g.xlarge", "a", 6, 300)} + sized, _, drops, rep := ApplyFamilyNUSizingRDSWithExpiry(recs, cov, 80, commits, 30) + assert.Empty(t, sized) + assert.Equal(t, 1, drops.AlreadyAtTarget) + assert.Zero(t, rep) + }) + + t.Run("control: non-RDS, inactive, zero window and no commitments are ignored", func(t *testing.T) { + cov, recs, commits := expiryTestScenario() + ec2 := expiryTestRI("m5.large", "a", 4, 15) + ec2.Service = common.ServiceEC2 + retired := expiryTestRI("db.r6g.large", "a", 4, 15) + retired.State = "retired" + for name, c := range map[string][]common.Commitment{"ec2": {ec2}, "retired": {retired}, "none": nil} { + _, _, drops, rep := ApplyFamilyNUSizingRDSWithExpiry(recs, cov, 80, c, 30) + assert.Equal(t, 1, drops.AlreadyAtTarget, name) + assert.Zero(t, rep, name) + } + _, _, drops, _ := ApplyFamilyNUSizingRDSWithExpiry(recs, cov, 80, commits, 0) + assert.Equal(t, 1, drops.AlreadyAtTarget, "window 0") + }) + + t.Run("unusable expiring RIs are counted, never read as no expiry", func(t *testing.T) { + cov, recs, _ := expiryTestScenario() + unparsable := expiryTestRI("db.r6g.weird", "a", 1, 15) + engineMismatch := expiryTestRI("db.r6g.large", "a", 1, 15) + engineMismatch.Engine = "aurora" + _, _, drops, rep := ApplyFamilyNUSizingRDSWithExpiry(recs, cov, 80, []common.Commitment{unparsable, engineMismatch}, 30) + assert.Equal(t, 2, rep.Unusable) + assert.Zero(t, rep.ExpiringNU) + assert.Equal(t, 1, drops.AlreadyAtTarget, "no deduction was possible") + }) + + t.Run("invalid target returns recs untouched", func(t *testing.T) { + cov, recs, commits := expiryTestScenario() + sized, nonRDS, _, _ := ApplyFamilyNUSizingRDSWithExpiry(recs, cov, 0, commits, 30) + assert.Nil(t, sized) + assert.Len(t, nonRDS, 1) + }) +} From 707ca12e24174216a67f2a16500a1af6bbe2eb8a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 12 Oct 2026 19:58:06 +0200 Subject: [PATCH 2/2] fix(aws): cap family expiry deduction at expiring NU for partial inventories Coverage is org-wide while the supplied commitments may cover only some accounts, so min(covered, active - expiring) could remove more than expires. Cap the deduction at expiring NU; iterate by index for gocritic. Refs LeanerCloud/cloud-commitments-cli#2132 Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 3 ++- providers/aws/recommendations/family_nu.go | 20 +++++++++++-------- .../recommendations/family_nu_expiry_test.go | 12 +++++++++++ 3 files changed, 26 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 484d7daf..134ff3c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,7 +34,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). - **`recommendations.ApplyFamilyNUSizingRDSWithExpiry`:** family-NU sizing that deducts RDS RIs expiring inside the rebuy window from family coverage - in normalized units (`min(covered, active - expiring)`), and reports + in normalized units (`max(covered - expiring, min(covered, active - expiring))`, + so a partial RI inventory never removes more than expires), and reports expiring RIs it could not use (`FamilyExpiryReport.Unusable`). Additive; `ApplyFamilyNUSizingRDS` is unchanged. Consumers that pass a rebuy window will see higher RDS counts and costs for families with expiring RIs diff --git a/providers/aws/recommendations/family_nu.go b/providers/aws/recommendations/family_nu.go index 3dd7384d..addbbdaf 100644 --- a/providers/aws/recommendations/family_nu.go +++ b/providers/aws/recommendations/family_nu.go @@ -390,10 +390,14 @@ type FamilyExpiryReport struct { // running, so the deduction is made per family (region, family, engine, // deployment), not per size pool. For each family with expiring active RIs: // -// post-expiry covered NU = min(CoveredNU, activeNU - expiringNU), floored at 0 +// post-expiry covered NU = max(CoveredNU - expiringNU, min(CoveredNU, activeNU - expiringNU)), floored at 0 // -// activeNU sums count x NU over every active RI in the family. The min() keeps -// underutilized RIs (more RI units than usage) from overstating the gap. +// activeNU sums count x NU over every active RI in the family the caller +// supplied. The min() keeps underutilized RIs (more RI units than usage) from +// overstating the gap. activeNU may be a subset of the org-wide coverage +// (the CLI inventory is single-account, cli#2131), so the outer max() caps the +// deduction at expiringNU: missing RIs must never remove more coverage than +// actually expires. // windowDays <= 0 or no commitments leaves coverage unchanged. A family // with several size rows or linked-account rows is deducted once. func ApplyFamilyNUSizingRDSWithExpiry( @@ -422,8 +426,8 @@ func adjustFamilyCoverageForExpiry(familyCov map[string]FamilyCoverage, commitme cutoff := now.Add(time.Duration(windowDays) * 24 * time.Hour) activeNU := make(map[string]float64) expiringNU := make(map[string]float64) - for _, c := range commitments { - fk, units, expiring, ok := rdsCommitmentFamilyUnits(c, cutoff) + for i := range commitments { + fk, units, expiring, ok := rdsCommitmentFamilyUnits(&commitments[i], cutoff) if !ok { if expiring { report.Unusable++ @@ -442,7 +446,7 @@ func adjustFamilyCoverageForExpiry(familyCov map[string]FamilyCoverage, commitme } for fk, exp := range expiringNU { fc := familyCov[fk] - fc.CoveredNU = math.Min(fc.CoveredNU, math.Max(0, activeNU[fk]-exp)) + fc.CoveredNU = math.Max(0, math.Max(fc.CoveredNU-exp, math.Min(fc.CoveredNU, activeNU[fk]-exp))) familyCov[fk] = fc report.ExpiringNU += exp } @@ -453,8 +457,8 @@ func adjustFamilyCoverageForExpiry(familyCov map[string]FamilyCoverage, commitme // normalized units. ok is false when the commitment is not an active RDS RI // (expiring is then false) or its class cannot be parsed (expiring is then // true when it ends inside the window, so the caller can report it). -func rdsCommitmentFamilyUnits(c common.Commitment, cutoff time.Time) (fk string, units float64, expiring, ok bool) { - if (c.Service != common.ServiceRDS && c.Service != common.ServiceRelationalDB) || !commitmentIsActive(c) { +func rdsCommitmentFamilyUnits(c *common.Commitment, cutoff time.Time) (fk string, units float64, expiring, ok bool) { + if (c.Service != common.ServiceRDS && c.Service != common.ServiceRelationalDB) || !commitmentIsActive(*c) { return "", 0, false, false } expiring = !c.EndDate.IsZero() && !c.EndDate.After(cutoff) diff --git a/providers/aws/recommendations/family_nu_expiry_test.go b/providers/aws/recommendations/family_nu_expiry_test.go index 39b7e60e..ebc0bf38 100644 --- a/providers/aws/recommendations/family_nu_expiry_test.go +++ b/providers/aws/recommendations/family_nu_expiry_test.go @@ -75,6 +75,18 @@ func TestApplyFamilyNUSizingRDSWithExpiry(t *testing.T) { assert.Equal(t, 16.0, rep.ExpiringNU) }) + t.Run("partial inventory: missing accounts' RIs never remove more than expires", func(t *testing.T) { + // Coverage is org-wide (64 NU covered) but only account A's expiring + // 16 NU is supplied; account B's 48 NU long-lived RIs are not (cli#2131). + // Covered must drop by exactly the expiring 16 NU (60%), not to 0. + cov, recs, _ := expiryTestScenario() + commits := []common.Commitment{expiryTestRI("db.r6g.large", "a", 4, 15)} + sized, _, _, _ := ApplyFamilyNUSizingRDSWithExpiry(recs, cov, 80, commits, 30) + require.Len(t, sized, 1) + assert.InDelta(t, 60.0, sized[0].ExistingCoveragePct, 1e-9) + assert.Equal(t, 2, sized[0].Count, "uncapped deduction would size 8") + }) + t.Run("RIs exceeding usage do not inflate the gap", func(t *testing.T) { // covered 40 NU of 80; active 10 x xlarge = 80 NU, 2 expiring (16). // min(40, 64) = 40: unchanged 50%. Naive subtraction would give 24 NU.