From a4002e7f9fdcd16f5252c520819e556fd7570c0e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 21:09:50 +0100 Subject: [PATCH] fix(aws/ladder): declare the residual basis on the usage baseline The AWS baseline series counts only on-demand usage that existing commitments left uncovered, but GetUsageBaseline returned it with an empty Basis, so Allocate read it as total demand and subtracted existing commitment a second time. A $4/hr uncovered series with a $6/hr Compute SP held as "target already met" while $4/hr was uncovered. Return BaselineBasisResidual, add the replay through the real baseline and Allocate, and correct the comments that described the double netting as intended. Part of #282 Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 9 ++- providers/aws/ladder/baseline.go | 16 ++--- providers/aws/ladder/ladder_test.go | 59 +++++++++++++++++++ .../aws/recommendations/ondemand_series.go | 7 ++- 4 files changed, 79 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 272894f9..fff956cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -487,8 +487,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). met". A residual baseline with a target below 100% and existing commitment is held (the gap needs the covered usage the residual excludes). Expiring commitment is not added back on that basis. Unknown basis values are an - error. Additive: total-basis results are unchanged. The AWS ladder adopts the - residual basis in a follow-up once this pkg version is published. + error. Additive: total-basis results are unchanged. The AWS ladder baseline + now declares the residual basis (its series counts only uncovered On Demand + Instances usage), so a partly covered AWS account is no longer held as + "target already met"; with a target below 100% and existing commitment it is + held ("residual baseline cannot size a partial target") instead of being + undersized by subtracting that commitment from the residual again. Part of + #282. - **`azure/savingsplans`:** the commitment currency is no longer hardcoded to USD. `SavingsPlanDetails.Currency` (new, additive) is sent in the purchase and diff --git a/providers/aws/ladder/baseline.go b/providers/aws/ladder/baseline.go index bef87186..06ebb287 100644 --- a/providers/aws/ladder/baseline.go +++ b/providers/aws/ladder/baseline.go @@ -37,16 +37,17 @@ const maxSeriesAgeDays = 3 // GetUsageBaseline computes a statistical low-water-mark from a daily // on-demand-equivalent USD/hour series returned by the injected onDemandSeriesSource. // -// Series semantics: each element is the average on-demand-equivalent USD/hour -// for one calendar day over the lookback window, ordered oldest-to-newest. -// The series is sourced from onDemandSeriesSource.GetOnDemandSeries, which is wired -// in a later PR to call CE GetCostAndUsage (Granularity=Daily, on-demand -// usage-type filter). Until that wiring lands, callers receive a data-source -// error from GetOnDemandSeries. +// Series semantics: each element is the average USD/hour of on-demand usage +// left uncovered by existing commitments for one calendar day over the lookback +// window, ordered oldest-to-newest. The series is sourced from +// onDemandSeriesSource.GetOnDemandSeries (CE GetCostAndUsage, Granularity=Daily, +// "On Demand Instances" purchase type), so covered RI and Savings Plan usage is +// excluded. The returned baseline therefore declares BaselineBasisResidual: +// Allocate must not subtract existing commitment from it a second time (go#282). // // Limitations: // - The series covers only the on-demand costs reported by the coverage -// source. Until the CE GetCostAndUsage wiring lands, this will be an error. +// source. // - The series is EC2-scoped (the initial coverage source implementation // covers EC2 on-demand cost only, not RDS/ElastiCache/etc.). // - LowWaterUSDPerHour is the nearest-rank percentile of the daily series @@ -127,6 +128,7 @@ func (a *AWSLadder) GetUsageBaseline(ctx context.Context, scope ladder.Scope, lo return ladder.UsageBaseline{ LowWaterUSDPerHour: ptr(lowWater), StableUSDPerHour: nil, + Basis: ladder.BaselineBasisResidual, Series: series, LookbackDays: lookbackDays, Percentile: percentile, diff --git a/providers/aws/ladder/ladder_test.go b/providers/aws/ladder/ladder_test.go index 9815d348..2c38b190 100644 --- a/providers/aws/ladder/ladder_test.go +++ b/providers/aws/ladder/ladder_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "math" + "math/big" "testing" "time" @@ -784,6 +785,64 @@ func TestGetUsageBaseline_SingleDaySeries_ReturnsThatValue(t *testing.T) { assert.Nil(t, bl.StableUSDPerHour) assert.Equal(t, 7, bl.LookbackDays) assert.InDelta(t, 5.0, bl.Percentile, 1e-9) + // The series counts only usage existing commitments left uncovered. + assert.Equal(t, ladder.BaselineBasisResidual, bl.Basis) +} + +// The go#282 replay through the real baseline: a $4/hr uncovered series with a +// $6/hr Compute SP already held must still buy, not "target already met" +// (4 - 6 = -2). The control zeroes Basis to show it is what changes the result. +func TestGetUsageBaseline_PartlyCoveredAccountStillAllocates(t *testing.T) { + t.Parallel() + cov := &fakeCoverageSource{onDemandPoints: makeConstantPoints(7, 4.0)} + a := newTestLadder(t, &fakeRILister{}, &fakeSPLister{}, cov, &fakeUtilizationSource{}) + bl, err := a.GetUsageBaseline(context.Background(), testScope(), 7, 5.0) + require.NoError(t, err) + + input := func(basis ladder.BaselineBasis, target float64) *ladder.AllocationInput { + zero, six, rate := 0.0, 6.0, 1.0 + states := map[ladder.LayerType]ladder.LayerState{} + rates := map[ladder.LayerType]float64{} + for _, spec := range a.SupportedLayers() { + existing := &zero + if spec.Type == ladder.LayerComputeSP { + existing = &six + } + states[spec.Type] = ladder.LayerState{Layer: spec.Type, ExistingUSDPerHour: existing, ExpiringUSDPerHour: &zero} + rates[spec.Type] = rate + } + b := bl + b.Basis = basis + return &ladder.AllocationInput{ + Config: ladder.LadderConfig{Scope: testScope(), Mode: ladder.ModeEmailApproval, Cadence: ladder.CadenceWeekly, + TargetCoveragePct: target, BaselinePercentile: 5, LookbackDays: 7, MaxActionsPerRun: 10, + BufferUtilizationThresholdPct: ladder.DefaultBufferUtilizationThresholdPct, + Ramp: ladder.RampSchedule{Steps: []ladder.RampStep{{AfterDays: 0, Fraction: 1}}}}, + Layers: a.SupportedLayers(), LayerStates: states, CommitmentRates: rates, InFlight: []ladder.InFlightTranche{}, + Baseline: b, Now: time.Date(2026, 10, 15, 0, 0, 0, 0, time.UTC), DataSources: []string{"constructed"}, + } + } + + res, err := ladder.Allocate(input(bl.Basis, 100)) + require.NoError(t, err) + require.Len(t, res.Allocations, 1) + assert.Equal(t, ladder.LayerComputeSP, res.Allocations[0].Layer) + assert.Equal(t, 0, res.Allocations[0].OnDemandEquivalentUSDPerHour.Cmp(big.NewRat(4, 1))) + + before, err := ladder.Allocate(input(ladder.BaselineBasisTotal, 100)) + require.NoError(t, err) + require.True(t, before.IsNoOp()) + require.Len(t, before.Holds, 1) + assert.Contains(t, before.Holds[0].Rationale, "target already met") + assert.Contains(t, before.Holds[0].Rationale, "gap=$-2.0000/hr") + + // A target below 100% with existing commitment is held by design: the gap + // needs covered usage the residual series excludes. + partial, err := ladder.Allocate(input(bl.Basis, 80)) + require.NoError(t, err) + assert.True(t, partial.IsNoOp()) + require.Len(t, partial.Holds, 1) + assert.Contains(t, partial.Holds[0].Rationale, "residual baseline cannot size a partial target") } func TestGetUsageBaseline_P5OfVariedSeries(t *testing.T) { diff --git a/providers/aws/recommendations/ondemand_series.go b/providers/aws/recommendations/ondemand_series.go index d1c35767..136aa4b1 100644 --- a/providers/aws/recommendations/ondemand_series.go +++ b/providers/aws/recommendations/ondemand_series.go @@ -21,9 +21,10 @@ const ec2ComputeService = "Amazon Elastic Compute Cloud - Compute" // purchaseTypeOnDemand is the CE PURCHASE_TYPE dimension value for on-demand // instances. Filters to usage billed at on-demand rates (excludes RI-covered, -// SP-covered, and Spot). Using the uncovered on-demand spend as the baseline -// is conservative: existing coverage is separately counted by GetLayerStates, -// so the engine correctly nets it into the gap calculation. +// SP-covered, and Spot). The series is therefore a residual: existing coverage +// is already excluded from it, so the ladder baseline declares +// BaselineBasisResidual and Allocate does not subtract existing commitment from +// it again (go#282). const purchaseTypeOnDemand = "On Demand Instances" // onDemandMetric is the CE metric name for GetCostAndUsage. UnblendedCost is