From 32ff7fdb5b3a0d47b1346d16825dbf1f95500a88 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 18:46:43 +0100 Subject: [PATCH] refactor(ladder): fetch commitment rates per layer so one missing rate holds only its layer GetCommitmentRates returned an empty map as soon as the EC2 Instance SP rate was missing. That is harmless with one layer, but would drop every other layer once more are added. The fetch is now a loop over an explicit layer list (layer, plan type, region): a missing rate holds only that layer, any other error fails the call naming the layer, and a rate that is not finite or not positive is rejected instead of stored. The list still holds only EC2 Instance SP. Compute SP stays absent until the owner decides; its account-wide blend over EC2, Lambda and Fargate biases sizing of a regional EC2-only gap. Part of #349 Co-Authored-By: Claude Sonnet 5.5 --- providers/aws/ladder/commitment_rates.go | 50 +++++++--- providers/aws/ladder/commitment_rates_test.go | 92 +++++++++++++++++-- 2 files changed, 123 insertions(+), 19 deletions(-) diff --git a/providers/aws/ladder/commitment_rates.go b/providers/aws/ladder/commitment_rates.go index f7908d1e..34922a98 100644 --- a/providers/aws/ladder/commitment_rates.go +++ b/providers/aws/ladder/commitment_rates.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "math" "github.com/aws/aws-sdk-go-v2/service/costexplorer/types" @@ -15,8 +16,15 @@ type spCommitmentRateSource interface { GetSPCommitmentRate(context.Context, recommendations.SPCommitmentRateParams) (float64, error) } -// GetCommitmentRates leaves Compute absent pending the caller's sizing policy; -// regional EC2 Instance recommendations are not a Compute pricing proxy. +// rateLayer is one layer whose commitment rate is fetched independently. +type rateLayer struct { + layer ladder.LayerType + plan types.SupportedSavingsPlansType + region string +} + +// GetCommitmentRates fetches one rate per layer; a layer without a rate is +// absent (hold) and never affects the others. func (a *AWSLadder) GetCommitmentRates(ctx context.Context, scope ladder.Scope, term ladder.Term, payment ladder.PaymentOption) (map[ladder.LayerType]float64, error) { if err := a.validateScope(scope); err != nil { return nil, err @@ -30,17 +38,35 @@ func (a *AWSLadder) GetCommitmentRates(ctx context.Context, scope ladder.Scope, if a.spRates == nil { return nil, errors.New("GetCommitmentRates: Savings Plan rate source not wired") } - rate, err := a.spRates.GetSPCommitmentRate(ctx, recommendations.SPCommitmentRateParams{ - PlanType: types.SupportedSavingsPlansTypeEc2InstanceSp, - AccountID: a.cfg.AccountID, Region: a.cfg.Region, - Term: string(term), PaymentOption: string(payment), + // Compute SP stays absent until the owner decides (#349). Its account-wide + // blend over EC2+Lambda+Fargate biases sizing of a regional EC2-only gap. + layers := []rateLayer{ + {layer: ladder.LayerEC2InstanceSP, plan: types.SupportedSavingsPlansTypeEc2InstanceSp, region: a.cfg.Region}, + } + return fetchLayerRates(ctx, a.spRates, layers, recommendations.SPCommitmentRateParams{ + AccountID: a.cfg.AccountID, + Term: string(term), PaymentOption: string(payment), LookbackPeriod: fmt.Sprintf("%dd", a.cfg.lookbackDays()), }) - if errors.Is(err, recommendations.ErrNoSPCommitmentRate) { - return map[ladder.LayerType]float64{}, nil - } - if err != nil { - return nil, fmt.Errorf("GetCommitmentRates: %w", err) +} + +// fetchLayerRates sets PlanType and Region from each layer on a copy of base. +func fetchLayerRates(ctx context.Context, src spCommitmentRateSource, layers []rateLayer, base recommendations.SPCommitmentRateParams) (map[ladder.LayerType]float64, error) { + rates := make(map[ladder.LayerType]float64, len(layers)) + for _, l := range layers { + p := base + p.PlanType, p.Region = l.plan, l.region + rate, err := src.GetSPCommitmentRate(ctx, p) + if errors.Is(err, recommendations.ErrNoSPCommitmentRate) { + continue + } + if err != nil { + return nil, fmt.Errorf("GetCommitmentRates: layer %s: %w", l.layer, err) + } + if math.IsNaN(rate) || math.IsInf(rate, 0) || rate <= 0 { + return nil, fmt.Errorf("GetCommitmentRates: layer %s: unusable rate %v", l.layer, rate) + } + rates[l.layer] = rate } - return map[ladder.LayerType]float64{ladder.LayerEC2InstanceSP: rate}, nil + return rates, nil } diff --git a/providers/aws/ladder/commitment_rates_test.go b/providers/aws/ladder/commitment_rates_test.go index 261477b3..30564db9 100644 --- a/providers/aws/ladder/commitment_rates_test.go +++ b/providers/aws/ladder/commitment_rates_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "math" "testing" "github.com/aws/aws-sdk-go-v2/aws" @@ -16,9 +17,21 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/providers/aws/recommendations" ) +type fakeRateResult struct { + rate float64 + err error +} + +// fakeSPRateSource answers per plan type; an unlisted plan type is a test bug. type fakeSPRateSource struct { - err error - params []recommendations.SPCommitmentRateParams + results map[types.SupportedSavingsPlansType]fakeRateResult + params []recommendations.SPCommitmentRateParams +} + +func ec2Fake(rate float64, err error) *fakeSPRateSource { + return &fakeSPRateSource{results: map[types.SupportedSavingsPlansType]fakeRateResult{ + types.SupportedSavingsPlansTypeEc2InstanceSp: {rate, err}, + }} } func TestNewFromConfig_ValidatesAndScopesClients(t *testing.T) { @@ -39,7 +52,11 @@ func TestNewFromConfig_ValidatesAndScopesClients(t *testing.T) { func (f *fakeSPRateSource) GetSPCommitmentRate(_ context.Context, p recommendations.SPCommitmentRateParams) (float64, error) { f.params = append(f.params, p) - return 0.7, f.err + r, ok := f.results[p.PlanType] + if !ok { + return 0, fmt.Errorf("unexpected plan type %s", p.PlanType) + } + return r.rate, r.err } func TestGetCommitmentRates_ValidatesBeforeSource(t *testing.T) { @@ -56,7 +73,7 @@ func TestGetCommitmentRates_ValidatesBeforeSource(t *testing.T) { {"payment", testScope(), ladder.Term1Year, ladder.PaymentOption("bad")}, } { t.Run(tc.name, func(t *testing.T) { - f := &fakeSPRateSource{} + f := ec2Fake(0.7, nil) a := &AWSLadder{cfg: Config{Region: "us-east-1", AccountID: testScope().AccountID}, spRates: f} rates, err := a.GetCommitmentRates(context.Background(), tc.scope, tc.term, tc.payment) require.Error(t, err) @@ -70,7 +87,7 @@ func TestGetCommitmentRates_ForwardsSelection(t *testing.T) { t.Parallel() for _, days := range []int{0, 7, 60} { t.Run(fmt.Sprint(days), func(t *testing.T) { - f := &fakeSPRateSource{} + f := ec2Fake(0.7, nil) a := &AWSLadder{cfg: Config{Region: "eu-west-1", AccountID: testScope().AccountID, LookbackDays: days}, spRates: f} rates, err := a.GetCommitmentRates(context.Background(), testScope(), ladder.Term3Year, ladder.PaymentPartialUpfront) require.NoError(t, err) @@ -96,8 +113,8 @@ func TestGetCommitmentRates_AbsenceAndErrors(t *testing.T) { want error }{ {"unwired", nil, nil}, - {"absent", &fakeSPRateSource{err: fmt.Errorf("wrapped: %w", recommendations.ErrNoSPCommitmentRate)}, nil}, - {"hard", &fakeSPRateSource{err: hard}, hard}, + {"absent", ec2Fake(0, fmt.Errorf("wrapped: %w", recommendations.ErrNoSPCommitmentRate)), nil}, + {"hard", ec2Fake(0, hard), hard}, } { t.Run(tc.name, func(t *testing.T) { a := newTestLadder(t, &fakeRILister{}, &fakeSPLister{}, &fakeCoverageSource{}, &fakeUtilizationSource{}) @@ -117,3 +134,64 @@ func TestGetCommitmentRates_AbsenceAndErrors(t *testing.T) { }) } } + +func TestGetCommitmentRates_RejectsUnusableRate(t *testing.T) { + t.Parallel() + for name, rate := range map[string]float64{"nan": math.NaN(), "inf": math.Inf(1), "zero": 0, "negative": -0.3} { + t.Run(name, func(t *testing.T) { + a := &AWSLadder{cfg: Config{Region: "us-east-1", AccountID: testScope().AccountID}, spRates: ec2Fake(rate, nil)} + rates, err := a.GetCommitmentRates(context.Background(), testScope(), ladder.Term1Year, ladder.PaymentNoUpfront) + require.ErrorContains(t, err, string(ladder.LayerEC2InstanceSP)) + assert.Nil(t, rates) + }) + } +} + +func TestGetCommitmentRates_HardErrorNamesLayer(t *testing.T) { + t.Parallel() + hard := errors.New("source failed") + a := &AWSLadder{cfg: Config{Region: "us-east-1", AccountID: testScope().AccountID}, spRates: ec2Fake(0, hard)} + _, err := a.GetCommitmentRates(context.Background(), testScope(), ladder.Term1Year, ladder.PaymentNoUpfront) + require.ErrorIs(t, err, hard) + assert.ErrorContains(t, err, string(ladder.LayerEC2InstanceSP)) +} + +func TestFetchLayerRates_LayersAreIndependent(t *testing.T) { + t.Parallel() + const ( + planA = types.SupportedSavingsPlansTypeEc2InstanceSp + planB = types.SupportedSavingsPlansTypeComputeSp + ) + layers := []rateLayer{ + {layer: ladder.LayerEC2InstanceSP, plan: planA, region: "eu-west-1"}, + {layer: ladder.LayerComputeSP, plan: planB, region: ""}, + } + missing := fmt.Errorf("none: %w", recommendations.ErrNoSPCommitmentRate) + hard := errors.New("source failed") + for _, tc := range []struct { + name string + a, b fakeRateResult + want map[ladder.LayerType]float64 + wantErr error + }{ + {"first missing second present", fakeRateResult{err: missing}, fakeRateResult{rate: 0.8}, map[ladder.LayerType]float64{ladder.LayerComputeSP: 0.8}, nil}, + {"first present second hard", fakeRateResult{rate: 0.7}, fakeRateResult{err: hard}, nil, hard}, + {"both missing", fakeRateResult{err: missing}, fakeRateResult{err: missing}, map[ladder.LayerType]float64{}, nil}, + {"both present", fakeRateResult{rate: 0.7}, fakeRateResult{rate: 0.8}, map[ladder.LayerType]float64{ladder.LayerEC2InstanceSP: 0.7, ladder.LayerComputeSP: 0.8}, nil}, + } { + t.Run(tc.name, func(t *testing.T) { + f := &fakeSPRateSource{results: map[types.SupportedSavingsPlansType]fakeRateResult{planA: tc.a, planB: tc.b}} + got, err := fetchLayerRates(context.Background(), f, layers, recommendations.SPCommitmentRateParams{AccountID: "acct"}) + if tc.wantErr != nil { + require.ErrorIs(t, err, tc.wantErr) + assert.Nil(t, got) + return + } + require.NoError(t, err) + assert.Equal(t, tc.want, got) + require.Len(t, f.params, 2) + assert.Equal(t, "eu-west-1", f.params[0].Region) + assert.Equal(t, planB, f.params[1].PlanType) + }) + } +}