diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b9d9a8..72b2674 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,16 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Added +- **`recommendations.ApplyFamilyNUSizingRDSWithExpiryOptions`:** the + rebuy-window RDS family sizing, plus credit for replacements already bought. + Coverage is a lookback average, so RIs bought inside `CoverageLookbackDays` + are not in it yet; with R the NU of visible active, non-expiring RIs that + started inside the lookback, post-expiry covered is + `max(0, max(min(C, C - E + R), min(C, A - E)))`. It is never below the R = 0 + formula, so it can only buy less, and a partial inventory still never removes + more than expires. `FamilyExpiryReport.ReplacementNU` records R. + `CoverageLookbackDays <= 0` credits none. Additive; the other functions are + unchanged (LeanerCloud/cloud-commitments-cli#2132, #2175). - **`common.SavingsPlanDetails.Currency`:** the ISO 4217 code the hourly commitment is denominated in. Additive and unused until the Azure Savings Plan client starts requiring it (#46). diff --git a/providers/aws/recommendations/family_nu.go b/providers/aws/recommendations/family_nu.go index addbbda..2cc62fd 100644 --- a/providers/aws/recommendations/family_nu.go +++ b/providers/aws/recommendations/family_nu.go @@ -377,6 +377,11 @@ type FamilyExpiryReport struct { // ExpiringNU is the normalized units of active RDS RIs ending inside the // window that were deducted from family coverage. ExpiringNU float64 + // ReplacementNU is the normalized units of visible active, non-expiring RDS + // RIs that started inside the coverage lookback and so are not yet in the + // lagged coverage, summed over families that have expiring RIs. It is only + // non-zero when FamilyExpiryOptions.CoverageLookbackDays > 0. + ReplacementNU 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 @@ -384,6 +389,20 @@ type FamilyExpiryReport struct { Unusable int } +// FamilyExpiryOptions configures ApplyFamilyNUSizingRDSWithExpiryOptions. +type FamilyExpiryOptions struct { + // WindowDays is the rebuy window: active RIs ending within it are treated + // as expiring. <= 0 disables the expiry adjustment. + WindowDays int + // CoverageLookbackDays is the lookback, in days, of the Cost Explorer + // coverage average the caller passed in (the lookbackDays given to + // GetRICoverageMap). Coverage is an average over that period, so an RI that + // started inside it is not yet fully reflected. Visible active, non-expiring + // RIs that started inside it are credited as already-bought replacements. + // <= 0 credits none, which is exactly ApplyFamilyNUSizingRDSWithExpiry. + CoverageLookbackDays 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 @@ -400,32 +419,86 @@ type FamilyExpiryReport struct { // actually expires. // windowDays <= 0 or no commitments leaves coverage unchanged. A family // with several size rows or linked-account rows is deducted once. +// +// This form ignores replacement RIs the account already bought for the +// expiring ones; callers that know the coverage lookback should use +// ApplyFamilyNUSizingRDSWithExpiryOptions. func ApplyFamilyNUSizingRDSWithExpiry( recs []common.Recommendation, coverage PoolCoverageMap, targetPct float64, commitments []common.Commitment, windowDays int, +) (sizedRDS, nonRDS []common.Recommendation, drops FamilyDropCounts, expiry FamilyExpiryReport) { + return ApplyFamilyNUSizingRDSWithExpiryOptions(recs, coverage, targetPct, commitments, + FamilyExpiryOptions{WindowDays: windowDays}) +} + +// ApplyFamilyNUSizingRDSWithExpiryOptions is ApplyFamilyNUSizingRDSWithExpiry +// that also credits replacements already bought. With R = the NU of visible +// active, non-expiring RIs in the family that started within +// opts.CoverageLookbackDays, the family's post-expiry covered NU is +// +// max(0, max(min(C, C - E + R), min(C, A - E))) +// +// where C is CoveredNU, E the expiring NU and A the active NU. R only raises +// the first term, so the result is never below the R = 0 formula (the purchase +// never grows), never above C, and never more than E below C. Replacement RIs +// bought inside the lookback are in the lagged C only in part, so crediting +// them in full can only under-count the gap, never over-count it. With full +// inventory the min(C, A - E) term already counts them. +func ApplyFamilyNUSizingRDSWithExpiryOptions( + recs []common.Recommendation, + coverage PoolCoverageMap, + targetPct float64, + commitments []common.Commitment, + opts FamilyExpiryOptions, ) (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()) + expiry = adjustFamilyCoverageForExpiry(familyCov, commitments, opts, 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 { +// the caller) as described on ApplyFamilyNUSizingRDSWithExpiryOptions. +func adjustFamilyCoverageForExpiry(familyCov map[string]FamilyCoverage, commitments []common.Commitment, opts FamilyExpiryOptions, now time.Time) FamilyExpiryReport { var report FamilyExpiryReport - if windowDays <= 0 || len(commitments) == 0 { + if opts.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) + totals := tallyFamilyCommitments(familyCov, commitments, opts, now, &report) + for fk, exp := range totals.expiring { + fc := familyCov[fk] + rep := totals.replacement[fk] + fc.CoveredNU = math.Max(0, math.Max(math.Min(fc.CoveredNU, fc.CoveredNU-exp+rep), math.Min(fc.CoveredNU, totals.active[fk]-exp))) + familyCov[fk] = fc + report.ExpiringNU += exp + report.ReplacementNU += rep + } + return report +} + +// familyNUTotals holds per-family normalized-unit sums over the inventory. +type familyNUTotals struct { + active map[string]float64 // every active RI + expiring map[string]float64 // active RIs ending inside the rebuy window + replacement map[string]float64 // active, non-expiring RIs started inside the coverage lookback +} + +// tallyFamilyCommitments sums the inventory per family and counts expiring RIs +// it cannot use into report.Unusable. +func tallyFamilyCommitments(familyCov map[string]FamilyCoverage, commitments []common.Commitment, opts FamilyExpiryOptions, now time.Time, report *FamilyExpiryReport) familyNUTotals { + cutoff := now.Add(time.Duration(opts.WindowDays) * 24 * time.Hour) + lagStart := now.Add(-time.Duration(opts.CoverageLookbackDays) * 24 * time.Hour) + t := familyNUTotals{ + active: make(map[string]float64), + expiring: make(map[string]float64), + replacement: make(map[string]float64), + } for i := range commitments { fk, units, expiring, ok := rdsCommitmentFamilyUnits(&commitments[i], cutoff) if !ok { @@ -434,23 +507,20 @@ func adjustFamilyCoverageForExpiry(familyCov map[string]FamilyCoverage, commitme } continue } - activeNU[fk] += units + t.active[fk] += units if !expiring { + if opts.CoverageLookbackDays > 0 && commitments[i].StartDate.After(lagStart) { + t.replacement[fk] += units + } continue } if _, known := familyCov[fk]; !known { report.Unusable++ continue } - expiringNU[fk] += units + t.expiring[fk] += units } - for fk, exp := range expiringNU { - fc := familyCov[fk] - fc.CoveredNU = math.Max(0, math.Max(fc.CoveredNU-exp, math.Min(fc.CoveredNU, activeNU[fk]-exp))) - familyCov[fk] = fc - report.ExpiringNU += exp - } - return report + return t } // rdsCommitmentFamilyUnits maps an active RDS commitment to its family key and diff --git a/providers/aws/recommendations/family_nu_expiry_replacements_test.go b/providers/aws/recommendations/family_nu_expiry_replacements_test.go new file mode 100644 index 0000000..79dffc7 --- /dev/null +++ b/providers/aws/recommendations/family_nu_expiry_replacements_test.go @@ -0,0 +1,250 @@ +package recommendations + +import ( + "math" + "math/rand" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +const replTestLookback = 30 + +func replTestOpts() FamilyExpiryOptions { + return FamilyExpiryOptions{WindowDays: 30, CoverageLookbackDays: replTestLookback} +} + +// replTestStarted returns c started daysAgo days ago. +func replTestStarted(c common.Commitment, daysAgo int) common.Commitment { + c.StartDate = time.Now().Add(-time.Duration(daysAgo) * 24 * time.Hour) + return c +} + +// replTestRepro is cli#2132 F1: lagged coverage 66 of 80 NU (82.5%), account A +// has 4 x large expiring and 4 x large replacements bought 2 days ago; 12 x +// large in other accounts are not in the inventory. True post-expiry gap is 0. +func replTestRepro() (PoolCoverageMap, []common.Recommendation, []common.Commitment) { + cov := expiryTestCov("db.r6g.xlarge", 10, 82.5) + recs := expiryTestRec("db.r6g.large", 10) + commits := []common.Commitment{ + replTestStarted(expiryTestRI("db.r6g.large", "a", 4, 15), 700), + replTestStarted(expiryTestRI("db.r6g.large", "a", 4, 700), 2), + } + return cov, recs, commits +} + +func TestExpiryReplacements_Regression(t *testing.T) { + t.Run("repro: own recent replacements are credited, nothing is bought", func(t *testing.T) { + cov, recs, commits := replTestRepro() + sized, _, _, _ := ApplyFamilyNUSizingRDSWithExpiry(recs, cov, 80, commits, 30) + require.Len(t, sized, 1, "current function over-buys") + assert.Equal(t, 3, sized[0].Count) + + sized, _, drops, rep := ApplyFamilyNUSizingRDSWithExpiryOptions(recs, cov, 80, commits, replTestOpts()) + assert.Empty(t, sized) + assert.Equal(t, 1, drops.AlreadyAtTarget) + assert.Equal(t, 16.0, rep.ExpiringNU) + assert.Equal(t, 16.0, rep.ReplacementNU) + }) + + t.Run("full inventory still buys nothing", func(t *testing.T) { + cov, recs, commits := replTestRepro() + commits = append(commits, replTestStarted(expiryTestRI("db.r6g.large", "b", 12, 700), 700)) + sized, _, drops, _ := ApplyFamilyNUSizingRDSWithExpiryOptions(recs, cov, 80, commits, replTestOpts()) + assert.Empty(t, sized) + assert.Equal(t, 1, drops.AlreadyAtTarget) + }) + + t.Run("go#341 F1 case: unlisted account RIs, no replacements, still buys 2", func(t *testing.T) { + cov, recs, _ := expiryTestScenario() + commits := []common.Commitment{replTestStarted(expiryTestRI("db.r6g.large", "a", 4, 15), 700)} + sized, _, _, rep := ApplyFamilyNUSizingRDSWithExpiryOptions(recs, cov, 80, commits, replTestOpts()) + require.Len(t, sized, 1) + assert.Equal(t, 2, sized[0].Count) + assert.Zero(t, rep.ReplacementNU) + }) + + t.Run("replacements started outside the lookback are not counted", func(t *testing.T) { + cov, recs, commits := replTestRepro() + commits[1] = replTestStarted(commits[1], replTestLookback+10) + sized, _, _, rep := ApplyFamilyNUSizingRDSWithExpiryOptions(recs, cov, 80, commits, replTestOpts()) + require.Len(t, sized, 1) + assert.Equal(t, 3, sized[0].Count) + assert.Zero(t, rep.ReplacementNU) + }) + + t.Run("replacements that are themselves expiring are not counted", func(t *testing.T) { + cov, recs, commits := replTestRepro() + commits[1] = replTestStarted(expiryTestRI("db.r6g.large", "a", 4, 20), 2) + _, _, _, rep := ApplyFamilyNUSizingRDSWithExpiryOptions(recs, cov, 80, commits, replTestOpts()) + assert.Zero(t, rep.ReplacementNU) + assert.Equal(t, 32.0, rep.ExpiringNU) + }) + + t.Run("inactive and other-family replacements are not counted", func(t *testing.T) { + cov, recs, commits := replTestRepro() + retired := replTestStarted(expiryTestRI("db.r6g.large", "a", 4, 700), 2) + retired.State = "retired" + otherFamily := replTestStarted(expiryTestRI("db.r7g.large", "a", 4, 700), 2) + commits[1] = retired + commits = append(commits, otherFamily) + sized, _, _, rep := ApplyFamilyNUSizingRDSWithExpiryOptions(recs, cov, 80, commits, replTestOpts()) + require.Len(t, sized, 1) + assert.Equal(t, 3, sized[0].Count) + assert.Zero(t, rep.ReplacementNU) + }) + + t.Run("mixed sizes are summed in NU", func(t *testing.T) { + cov, recs, commits := replTestRepro() + commits[1] = replTestStarted(expiryTestRI("db.r6g.xlarge", "a", 1, 700), 2) + commits = append(commits, replTestStarted(expiryTestRI("db.r6g.large", "a", 2, 700), 5)) + _, _, _, rep := ApplyFamilyNUSizingRDSWithExpiryOptions(recs, cov, 80, commits, replTestOpts()) + assert.Equal(t, 16.0, rep.ReplacementNU, "8 + 2x4 NU") + }) + + t.Run("lookback 0 credits nothing", func(t *testing.T) { + cov, recs, commits := replTestRepro() + sized, _, _, rep := ApplyFamilyNUSizingRDSWithExpiryOptions(recs, cov, 80, commits, + FamilyExpiryOptions{WindowDays: 30}) + require.Len(t, sized, 1) + assert.Equal(t, 3, sized[0].Count) + assert.Zero(t, rep.ReplacementNU) + }) +} + +// legacyAdjust is the merged (#341) per-family formula, kept verbatim as the +// reference for the R = 0 equivalence test. +func legacyAdjust(c, exp, active float64) float64 { + return math.Max(0, math.Max(c-exp, math.Min(c, active-exp))) +} + +type replWorld struct { + own, all []common.Commitment + cov PoolCoverageMap + c, t float64 +} + +var replSizes = []string{"db.r6g.large", "db.r6g.xlarge", "db.r6g.2xlarge"} + +// replGenWorld builds a random family. Own account: expiring, long-lived and +// recent (replacement) RIs. Other accounts: old RIs, some expiring, invisible +// to the inventory. Lagged coverage C counts every old RI in full plus a +// random fraction f of the recent ones; usage exceeds every RI, so coverage is +// not saturated. +func replGenWorld(rng *rand.Rand) replWorld { + var w replWorld + old, recent := 0.0, 0.0 + add := func(visible bool, endDays, startDaysAgo int, isRecent bool) { + size := replSizes[rng.Intn(len(replSizes))] + cnt := 1 + rng.Intn(4) + c := replTestStarted(expiryTestRI(size, "x", cnt, endDays), startDaysAgo) + nu := float64(cnt) * rdsInstanceNUFromType(size) + if isRecent { + recent += nu + } else { + old += nu + } + if visible { + w.own = append(w.own, c) + } + w.all = append(w.all, c) + } + for i := rng.Intn(4); i > 0; i-- { + add(true, 1+rng.Intn(29), 100+rng.Intn(600), false) + } + for i := rng.Intn(4); i > 0; i-- { + add(true, 100+rng.Intn(600), 100+rng.Intn(600), false) + } + for i := rng.Intn(4); i > 0; i-- { + add(true, 100+rng.Intn(600), rng.Intn(replTestLookback), true) + } + for i := rng.Intn(5); i > 0; i-- { + end := 100 + rng.Intn(600) + if rng.Intn(3) == 0 { + end = 1 + rng.Intn(29) + } + add(false, end, 100+rng.Intn(600), false) + } + w.c = old + rng.Float64()*recent + w.t = old + recent + 1 + rng.Float64()*40 + w.cov = expiryTestCov("db.r6g.large", w.t/4, w.c/w.t*100) + return w +} + +func replFamilyCovered(t *testing.T, w replWorld, commits []common.Commitment, opts FamilyExpiryOptions) (float64, FamilyExpiryReport) { + t.Helper() + fcs := AggregateRDSFamilyCoverage(w.cov) + require.Len(t, fcs, 1) + rep := adjustFamilyCoverageForExpiry(fcs, commits, opts, time.Now()) + for _, fc := range fcs { + return fc.CoveredNU, rep + } + return 0, rep +} + +func replBought(w replWorld, covered float64) int { + fcs := AggregateRDSFamilyCoverage(w.cov) + for k, fc := range fcs { + fc.CoveredNU = covered + fcs[k] = fc + } + sized, _, _ := sizeRDSFamilies(expiryTestRec("db.r6g.xlarge", 40), fcs, 90) + if len(sized) == 0 { + return 0 + } + return sized[0].Count +} + +func TestExpiryReplacements_ZeroLookbackMatchesMergedFormula(t *testing.T) { + rng := rand.New(rand.NewSource(341)) + for seed := 0; seed < 3000; seed++ { + w := replGenWorld(rng) + got, rep := replFamilyCovered(t, w, w.own, FamilyExpiryOptions{WindowDays: 30}) + fc := AggregateRDSFamilyCoverage(w.cov) + var base FamilyCoverage + for _, v := range fc { + base = v + } + exp, active := 0.0, 0.0 + for _, c := range w.own { + nu := float64(c.Count) * rdsInstanceNUFromType(c.ResourceType) + active += nu + if c.EndDate.Before(time.Now().Add(30 * 24 * time.Hour)) { + exp += nu + } + } + want := base.CoveredNU + if exp > 0 { + want = legacyAdjust(base.CoveredNU, exp, active) + } + require.InDelta(t, want, got, 1e-9, "seed %d", seed) + require.Zero(t, rep.ReplacementNU) + } +} + +func TestExpiryReplacements_Properties(t *testing.T) { + rng := rand.New(rand.NewSource(2132)) + creditedWorlds := 0 + for seed := 0; seed < 3000; seed++ { + w := replGenWorld(rng) + base, _ := replFamilyCovered(t, w, nil, replTestOpts()) + old, _ := replFamilyCovered(t, w, w.own, FamilyExpiryOptions{WindowDays: 30}) + got, rep := replFamilyCovered(t, w, w.own, replTestOpts()) + oracle, _ := replFamilyCovered(t, w, w.all, FamilyExpiryOptions{WindowDays: 30}) + + require.GreaterOrEqual(t, got, old-1e-9, "seed %d: never below the R=0 formula", seed) + require.LessOrEqual(t, got, base+1e-9, "seed %d: never above measured coverage", seed) + require.LessOrEqual(t, base-got, rep.ExpiringNU+1e-9, "seed %d: deduction never exceeds expiring NU", seed) + require.GreaterOrEqual(t, got, oracle-1e-9, "seed %d: never below the full-inventory covered NU", seed) + require.LessOrEqual(t, replBought(w, got), replBought(w, oracle), "seed %d: purchase never exceeds the full-inventory gap", seed) + require.LessOrEqual(t, replBought(w, got), replBought(w, old), "seed %d: purchase never exceeds the R=0 purchase", seed) + if rep.ReplacementNU > 0 && got > old { + creditedWorlds++ + } + } + assert.Greater(t, creditedWorlds, 100, "generator must exercise the credit") +}