diff --git a/providers/aws/ladder/commitment_rates.go b/providers/aws/ladder/commitment_rates.go index 34922a98..5f9059d3 100644 --- a/providers/aws/ladder/commitment_rates.go +++ b/providers/aws/ladder/commitment_rates.go @@ -38,10 +38,15 @@ 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") } - // 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. + // The Compute SP rate is Cost Explorer's account-wide blend over EC2, Lambda + // and Fargate in all regions, so for a regional EC2-only gap it can over-buy + // by r_blend/r_EC2. Accepted by the owner (#349, option A). Here only + // MaxHourlyCommitPerRun bounds it, and only when set; the platform plans a + // required per-sweep cap (cloud-commitments-platform#70). Region is required + // by the source, not a filter. layers := []rateLayer{ {layer: ladder.LayerEC2InstanceSP, plan: types.SupportedSavingsPlansTypeEc2InstanceSp, region: a.cfg.Region}, + {layer: ladder.LayerComputeSP, plan: types.SupportedSavingsPlansTypeComputeSp, region: a.cfg.Region}, } return fetchLayerRates(ctx, a.spRates, layers, recommendations.SPCommitmentRateParams{ AccountID: a.cfg.AccountID, diff --git a/providers/aws/ladder/commitment_rates_sdk_test.go b/providers/aws/ladder/commitment_rates_sdk_test.go index 768d1fd6..77df3316 100644 --- a/providers/aws/ladder/commitment_rates_sdk_test.go +++ b/providers/aws/ladder/commitment_rates_sdk_test.go @@ -61,11 +61,12 @@ func ratePage(details, token string) string { return fmt.Sprintf(`{"NextPageToken":%q,"SavingsPlansPurchaseRecommendation":{"SavingsPlansPurchaseRecommendationDetails":[%s]}}`, token, details) } -func sdkRateLadder(t *testing.T, pages []string) *awsladder.AWSLadder { +// sdkRateLadder serves pages per Savings Plans type and asserts that exactly +// those pages are requested for each type. +func sdkRateLadder(t *testing.T, pagesByType map[string][]string) *awsladder.AWSLadder { t.Helper() - var calls atomic.Int32 + calls := map[string]*atomic.Int32{"EC2_INSTANCE_SP": {}, "COMPUTE_SP": {}} srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - page := int(calls.Add(1)) - 1 assert.Equal(t, http.MethodPost, r.Method) assert.Equal(t, "AWSInsightsIndexService.GetSavingsPlansPurchaseRecommendation", r.Header.Get("X-Amz-Target")) var input map[string]any @@ -73,7 +74,15 @@ func sdkRateLadder(t *testing.T, pages []string) *awsladder.AWSLadder { w.WriteHeader(http.StatusBadRequest) return } - want := map[string]any{"SavingsPlansType": "EC2_INSTANCE_SP", "AccountScope": "LINKED", + planType, _ := input["SavingsPlansType"].(string) + counter, known := calls[planType] + if !assert.True(t, known, "unexpected plan type %q", planType) { + w.WriteHeader(http.StatusBadRequest) + return + } + pages := pagesByType[planType] + page := int(counter.Add(1)) - 1 + want := map[string]any{"SavingsPlansType": planType, "AccountScope": "LINKED", "TermInYears": "THREE_YEARS", "PaymentOption": "PARTIAL_UPFRONT", "LookbackPeriodInDays": "SEVEN_DAYS"} if page > 0 { want["NextPageToken"] = "second" @@ -90,7 +99,9 @@ func sdkRateLadder(t *testing.T, pages []string) *awsladder.AWSLadder { })) t.Cleanup(func() { srv.Close() - assert.Equal(t, int32(len(pages)), calls.Load()) + for planType, counter := range calls { + assert.Equal(t, int32(len(pagesByType[planType])), counter.Load(), planType) + } }) endpoint, err := url.Parse(srv.URL) require.NoError(t, err) @@ -106,44 +117,61 @@ func sdkRateLadder(t *testing.T, pages []string) *awsladder.AWSLadder { func TestCommitmentRates_PublicSDKConsumer(t *testing.T) { t.Parallel() - a := sdkRateLadder(t, []string{ - ratePage(rateDetail("fixture-account", "eu-west-1", "m5", "10", "10")+","+ - rateDetail("other-fixture-account", "eu-west-1", "m5", "999", "1")+","+ - rateDetail("fixture-account", "us-east-1", "m5", "999", "1"), "second"), - ratePage(rateDetail("fixture-account", "eu-west-1", "c5", "60", "20"), ""), + a := sdkRateLadder(t, map[string][]string{ + "EC2_INSTANCE_SP": { + ratePage(rateDetail("fixture-account", "eu-west-1", "m5", "10", "10")+","+ + rateDetail("other-fixture-account", "eu-west-1", "m5", "999", "1")+","+ + rateDetail("fixture-account", "us-east-1", "m5", "999", "1"), "second"), + ratePage(rateDetail("fixture-account", "eu-west-1", "c5", "60", "20"), ""), + }, + // Compute SP is account-wide: both regions count, other accounts do not. + // The regions have different ratios (0.5 and 0.75), so only the blend of + // both gives 0.625; a regional filter would give 0.5. + "COMPUTE_SP": {ratePage(rateDetail("fixture-account", "eu-west-1", "", "20", "20")+","+ + rateDetail("fixture-account", "us-east-1", "", "30", "10")+","+ + rateDetail("other-fixture-account", "eu-west-1", "", "999", "1"), "")}, }) scope := ladder.Scope{Provider: common.ProviderAWS, AccountID: "fixture-account"} rates, err := a.GetCommitmentRates(context.Background(), scope, ladder.Term3Year, ladder.PaymentPartialUpfront) require.NoError(t, err) - require.Equal(t, map[ladder.LayerType]float64{ladder.LayerEC2InstanceSP: 0.7}, rates) + require.Equal(t, map[ladder.LayerType]float64{ladder.LayerEC2InstanceSP: 0.7, ladder.LayerComputeSP: 0.625}, rates) in := consumerInput(a, scope, rates) result, err := ladder.Allocate(in) require.NoError(t, err) - require.Len(t, result.Allocations, 1) - allocation := result.Allocations[0] - assert.Equal(t, ladder.LayerEC2InstanceSP, allocation.Layer) - assert.Equal(t, 0, allocation.OnDemandEquivalentUSDPerHour.Cmp(big.NewRat(10, 1))) - assert.InDelta(t, 7, ratFloat(allocation.GapUSDPerHour), 1e-12) - assertMissingRateHold(t, result, ladder.LayerComputeSP) + // With both rates present, the base gap buys EC2 Instance SP at 0.7 and the + // flex gap buys Compute SP at the account-wide 0.625 (#349 option A). + require.Len(t, result.Allocations, 2) + byLayer := map[ladder.LayerType]ladder.Allocation{} + for _, a := range result.Allocations { + byLayer[a.Layer] = a + } + ec2, compute := byLayer[ladder.LayerEC2InstanceSP], byLayer[ladder.LayerComputeSP] + assert.Equal(t, 0, ec2.OnDemandEquivalentUSDPerHour.Cmp(big.NewRat(10, 1))) + assert.InDelta(t, 7, ratFloat(ec2.GapUSDPerHour), 1e-12) + assert.Equal(t, 0, compute.OnDemandEquivalentUSDPerHour.Cmp(big.NewRat(10, 1))) + assert.InDelta(t, 6.25, ratFloat(compute.GapUSDPerHour), 1e-12) + nextID := 0 tranches, err := ladder.BuildTranches(&ladder.TrancheInput{ - Config: &in.Config, RunID: "synthetic-run", Term: ladder.Term3Year, - PaymentOption: ladder.PaymentPartialUpfront, NewID: func() string { return "synthetic-tranche" }, - Now: in.Now, Allocations: result.Allocations, + Config: &in.Config, RunID: "synthetic-run", Term: ladder.Term3Year, PaymentOption: ladder.PaymentPartialUpfront, + NewID: func() string { nextID++; return fmt.Sprintf("synthetic-tranche-%d", nextID) }, + Now: in.Now, Allocations: result.Allocations, }) require.NoError(t, err) - require.Len(t, tranches.Tranches, 1) - tr := tranches.Tranches[0] - assert.Equal(t, allocation.GapUSDPerHour.RatString(), tr.AmountUSDPerHour) - assert.Equal(t, allocation.Rate.RatString(), tr.CommitmentRate) - storedAmount, ok := new(big.Rat).SetString(tr.AmountUSDPerHour) - require.True(t, ok) - storedRate, ok := new(big.Rat).SetString(tr.CommitmentRate) - require.True(t, ok) - in.InFlight = []ladder.InFlightTranche{{Layer: tr.Layer, CommitmentUSDPerHour: ratFloat(storedAmount), Rate: ratFloat(storedRate)}} + require.Len(t, tranches.Tranches, 2) + in.InFlight = []ladder.InFlightTranche{} + for _, tr := range tranches.Tranches { + alloc := byLayer[tr.Layer] + assert.Equal(t, alloc.GapUSDPerHour.RatString(), tr.AmountUSDPerHour, tr.Layer) + assert.Equal(t, alloc.Rate.RatString(), tr.CommitmentRate, tr.Layer) + storedAmount, ok := new(big.Rat).SetString(tr.AmountUSDPerHour) + require.True(t, ok) + storedRate, ok := new(big.Rat).SetString(tr.CommitmentRate) + require.True(t, ok) + in.InFlight = append(in.InFlight, ladder.InFlightTranche{Layer: tr.Layer, CommitmentUSDPerHour: ratFloat(storedAmount), Rate: ratFloat(storedRate)}) + } replay, err := ladder.Allocate(in) require.NoError(t, err) assert.True(t, replay.IsNoOp()) - assertMissingRateHold(t, replay, ladder.LayerComputeSP) } func consumerInput(a *awsladder.AWSLadder, scope ladder.Scope, rates map[ladder.LayerType]float64) *ladder.AllocationInput { @@ -184,7 +212,11 @@ func TestCommitmentRates_SDKMissingAndMalformed(t *testing.T) { if cost == "" { detail = `{"AccountId":"fixture-account","SavingsPlansDetails":{"Region":"eu-west-1"}}` } - a := sdkRateLadder(t, []string{ratePage(detail, "")}) + pages := map[string][]string{"EC2_INSTANCE_SP": {ratePage(detail, "")}} + if cost == "" { + pages["COMPUTE_SP"] = []string{ratePage(detail, "")} + } + a := sdkRateLadder(t, pages) scope := ladder.Scope{Provider: common.ProviderAWS, AccountID: "fixture-account"} rates, err := a.GetCommitmentRates(context.Background(), scope, ladder.Term3Year, ladder.PaymentPartialUpfront) if cost != "" { @@ -199,6 +231,7 @@ func TestCommitmentRates_SDKMissingAndMalformed(t *testing.T) { require.NoError(t, err) assert.True(t, result.IsNoOp()) assertMissingRateHold(t, result, ladder.LayerEC2InstanceSP) + assertMissingRateHold(t, result, ladder.LayerComputeSP) }) } } diff --git a/providers/aws/ladder/commitment_rates_test.go b/providers/aws/ladder/commitment_rates_test.go index 30564db9..89c4b35a 100644 --- a/providers/aws/ladder/commitment_rates_test.go +++ b/providers/aws/ladder/commitment_rates_test.go @@ -28,9 +28,16 @@ type fakeSPRateSource struct { params []recommendations.SPCommitmentRateParams } +// ec2Fake answers EC2 Instance SP with the given result and Compute SP with a +// fixed usable rate of 0.8. func ec2Fake(rate float64, err error) *fakeSPRateSource { + return layersFake(fakeRateResult{rate, err}, fakeRateResult{rate: 0.8}) +} + +func layersFake(ec2, compute fakeRateResult) *fakeSPRateSource { return &fakeSPRateSource{results: map[types.SupportedSavingsPlansType]fakeRateResult{ - types.SupportedSavingsPlansTypeEc2InstanceSp: {rate, err}, + types.SupportedSavingsPlansTypeEc2InstanceSp: ec2, + types.SupportedSavingsPlansTypeComputeSp: compute, }} } @@ -85,22 +92,40 @@ func TestGetCommitmentRates_ValidatesBeforeSource(t *testing.T) { 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 := 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) - assert.Equal(t, map[ladder.LayerType]float64{ladder.LayerEC2InstanceSP: 0.7}, rates) - if days == 0 { - days = DefaultLookbackDays - } - assert.Equal(t, []recommendations.SPCommitmentRateParams{{ - PlanType: types.SupportedSavingsPlansTypeEc2InstanceSp, - AccountID: testScope().AccountID, Region: "eu-west-1", - Term: "3yr", PaymentOption: "partial-upfront", LookbackPeriod: fmt.Sprintf("%dd", days), - }}, f.params) - }) + // Every requested term and payment must reach Cost Explorer as is, for both + // layers: a 1yr no-upfront ladder sized at a 3yr rate would over-buy. + selections := []struct { + term ladder.Term + payment ladder.PaymentOption + wantT string + wantP string + }{ + {ladder.Term1Year, ladder.PaymentNoUpfront, "1yr", "no-upfront"}, + {ladder.Term3Year, ladder.PaymentAllUpfront, "3yr", "all-upfront"}, + {ladder.Term3Year, ladder.PaymentPartialUpfront, "3yr", "partial-upfront"}, + } + for _, sel := range selections { + for _, days := range []int{0, 7, 60} { + t.Run(fmt.Sprintf("%s/%s/%d", sel.wantT, sel.wantP, days), func(t *testing.T) { + 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(), sel.term, sel.payment) + require.NoError(t, err) + assert.Equal(t, map[ladder.LayerType]float64{ladder.LayerEC2InstanceSP: 0.7, ladder.LayerComputeSP: 0.8}, rates) + wantDays := days + if wantDays == 0 { + wantDays = DefaultLookbackDays + } + base := recommendations.SPCommitmentRateParams{ + AccountID: testScope().AccountID, Region: "eu-west-1", + Term: sel.wantT, PaymentOption: sel.wantP, LookbackPeriod: fmt.Sprintf("%dd", wantDays), + } + ec2, compute := base, base + ec2.PlanType = types.SupportedSavingsPlansTypeEc2InstanceSp + compute.PlanType = types.SupportedSavingsPlansTypeComputeSp + assert.Equal(t, []recommendations.SPCommitmentRateParams{ec2, compute}, f.params) + }) + } } } @@ -113,7 +138,7 @@ func TestGetCommitmentRates_AbsenceAndErrors(t *testing.T) { want error }{ {"unwired", nil, nil}, - {"absent", ec2Fake(0, fmt.Errorf("wrapped: %w", recommendations.ErrNoSPCommitmentRate)), nil}, + {"absent", layersFake(fakeRateResult{err: fmt.Errorf("wrapped: %w", recommendations.ErrNoSPCommitmentRate)}, fakeRateResult{err: recommendations.ErrNoSPCommitmentRate}), nil}, {"hard", ec2Fake(0, hard), hard}, } { t.Run(tc.name, func(t *testing.T) { @@ -156,6 +181,39 @@ func TestGetCommitmentRates_HardErrorNamesLayer(t *testing.T) { assert.ErrorContains(t, err, string(ladder.LayerEC2InstanceSP)) } +func TestGetCommitmentRates_ComputeLayer(t *testing.T) { + t.Parallel() + missing := fmt.Errorf("none: %w", recommendations.ErrNoSPCommitmentRate) + hard := errors.New("source failed") + ec2, compute := fakeRateResult{rate: 0.7}, fakeRateResult{rate: 0.8} + for _, tc := range []struct { + name string + ec2 fakeRateResult + compute fakeRateResult + want map[ladder.LayerType]float64 + wantErr error + }{ + {"both", ec2, compute, map[ladder.LayerType]float64{ladder.LayerEC2InstanceSP: 0.7, ladder.LayerComputeSP: 0.8}, nil}, + {"ec2 missing", fakeRateResult{err: missing}, compute, map[ladder.LayerType]float64{ladder.LayerComputeSP: 0.8}, nil}, + {"compute missing", ec2, fakeRateResult{err: missing}, map[ladder.LayerType]float64{ladder.LayerEC2InstanceSP: 0.7}, nil}, + {"compute hard", ec2, fakeRateResult{err: hard}, nil, hard}, + } { + t.Run(tc.name, func(t *testing.T) { + f := layersFake(tc.ec2, tc.compute) + a := &AWSLadder{cfg: Config{Region: "eu-west-1", AccountID: testScope().AccountID}, spRates: f} + got, err := a.GetCommitmentRates(context.Background(), testScope(), ladder.Term1Year, ladder.PaymentNoUpfront) + if tc.wantErr != nil { + require.ErrorIs(t, err, tc.wantErr) + assert.ErrorContains(t, err, string(ladder.LayerComputeSP)) + assert.Nil(t, got) + return + } + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} + func TestFetchLayerRates_LayersAreIndependent(t *testing.T) { t.Parallel() const (