From d7443fde4ab00aba6afa877a3857c9b63c6b5296 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 01:22:39 +0100 Subject: [PATCH 1/2] fix(aws): round fractional RI quantities down and scale their costs Cost Explorer can recommend a fractional instance count. The parser rounded it to the nearest integer and kept CE's figures for the fractional quantity, so a 2.5 recommendation became Count 3 with the upfront for 2.5, and the approved-upfront check refused the purchase. Per the owner's option C, the quantity now rounds down, so a purchase never exceeds CE's quantity, and the upfront, recurring monthly cost, monthly savings and savings percentage scale by count/quantity. The on-demand baseline, break-even months and usage signals are unchanged. A quantity below 1 keeps its existing Count 0 and warning. Closes #635 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 7 +++ providers/aws/recommendations/parser_ri.go | 56 ++++++++++++----- .../aws/recommendations/parser_ri_test.go | 62 +++++++++++++++++-- .../services/ec2/purchase_price_guard_test.go | 24 +++++++ 4 files changed, 129 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99209924..3cebb374 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -269,6 +269,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- **`aws` RI recommendations:** a fractional Cost Explorer quantity now rounds + down instead of to the nearest integer, so a purchase never exceeds CE's + quantity. The upfront, recurring monthly cost, monthly savings and savings + percentage scale by `count / quantity`, so they describe the instances + actually bought. Before, 2.5 became 3 with the upfront for 2.5, and the + approved-upfront check refused the purchase. The on-demand baseline and + break-even months are unchanged (#635). - **`azure`:** an error from a reservation purchase retry (attempt 2 or later) now also wraps `common.ErrOutcomeUnknown`. That covers a currency refusal from the retry's calculatePrice, a failed idempotency re-check, a non-retryable purchase failure, a cancelled retry wait and exhausted attempts. A retry runs only after a purchase that failed retryably and may have committed, and the re-check can miss that order, so none of these errors may read as "nothing was bought". The cause stays in the chain, so `errors.Is(err, common.ErrCurrencyNotAccepted)` still matches. The `ErrCurrencyNotAccepted` doc now says it is pre-purchase only without `ErrOutcomeUnknown` (#671). - **Azure purchases check the recommendation currency.** Azure reservation purchases compare the calculatePrice billing currency with the recommendation currency diff --git a/providers/aws/recommendations/parser_ri.go b/providers/aws/recommendations/parser_ri.go index 113edd73..61a51c2e 100644 --- a/providers/aws/recommendations/parser_ri.go +++ b/providers/aws/recommendations/parser_ri.go @@ -73,7 +73,7 @@ func (c *Client) parseRecommendationDetail(ctx context.Context, details *types.R // count so the CSV can show what AWS proposed alongside what --coverage / // --target-coverage chose; Count is the working value the sizing step // mutates. - count, roundedFromFraction, err := c.parseRecommendedQuantity(details) + count, qty, roundedFromFraction, err := c.parseRecommendedQuantity(details) if err != nil { return nil, fmt.Errorf("failed to parse recommended quantity: %w", err) } @@ -107,6 +107,8 @@ func (c *Client) parseRecommendationDetail(ctx context.Context, details *types.R return nil, fmt.Errorf("failed to parse RI utilization signals: %w", err) } + scaleToRoundedCount(rec, count, qty) + // Parse service-specific details if err := c.parseServiceSpecificDetails(ctx, rec, details, params.Service); err != nil { return nil, err @@ -143,35 +145,57 @@ func (c *Client) parseRIUtilizationSignals(rec *common.Recommendation, details * return nil } -// parseRecommendedQuantity extracts the recommended quantity from details. The -// bool reports that a positive fractional quantity rounded down to 0; the -// caller warns once the recommendation's identity is known. -func (c *Client) parseRecommendedQuantity(details *types.ReservationPurchaseRecommendationDetail) (rounded int, fractionRoundedToZero bool, err error) { +// scaleToRoundedCount makes the figures CE reports for its (fractional) +// quantity agree with the rounded-down count that is actually bought (go#635 +// option C): the upfront, the recurring monthly charge, the monthly savings and +// the savings percentage scale by count/qty. The on-demand baseline, break-even +// (upfront over savings) and the usage signals do not depend on the quantity. +// A quantity that rounds to 0 is left unscaled; the caller warns about it. +func scaleToRoundedCount(rec *common.Recommendation, count int, qty float64) { + if count == 0 || float64(count) == qty { + return + } + f := float64(count) / qty + rec.CommitmentCost *= f + if rec.RecurringMonthlyCost != nil { + v := *rec.RecurringMonthlyCost * f + rec.RecurringMonthlyCost = &v + } + rec.EstimatedSavings *= f + rec.SavingsPercentage *= f +} + +// parseRecommendedQuantity extracts the recommended quantity from details, +// rounded down so a purchase never exceeds CE's quantity (go#635 option C), +// and also returns the parsed quantity for scaling. The bool reports that a +// positive fractional quantity rounded down to 0; the caller warns once the +// recommendation's identity is known. +func (c *Client) parseRecommendedQuantity(details *types.ReservationPurchaseRecommendationDetail) (rounded int, qty float64, fractionRoundedToZero bool, err error) { if details.RecommendedNumberOfInstancesToPurchase == nil { - return 0, false, fmt.Errorf("recommended quantity not found") + return 0, 0, false, fmt.Errorf("recommended quantity not found") } - qty := *details.RecommendedNumberOfInstancesToPurchase + raw := *details.RecommendedNumberOfInstancesToPurchase var count float64 - if _, err = fmt.Sscanf(qty, "%f", &count); err != nil { - if intCount, atoiErr := strconv.Atoi(qty); atoiErr == nil { + if _, err = fmt.Sscanf(raw, "%f", &count); err != nil { + if intCount, atoiErr := strconv.Atoi(raw); atoiErr == nil { if intCount < 0 { - return 0, false, fmt.Errorf("recommended quantity %q is negative", qty) + return 0, 0, false, fmt.Errorf("recommended quantity %q is negative", raw) } - return intCount, false, nil + return intCount, float64(intCount), false, nil } - return 0, false, fmt.Errorf("failed to parse quantity '%s' as float or int", qty) + return 0, 0, false, fmt.Errorf("failed to parse quantity '%s' as float or int", raw) } // Sscanf %f accepts "NaN"/"Inf"; a non-finite quantity would corrupt the - // purchase count (int(math.Round(NaN)) is undefined). A negative count is + // purchase count (int(math.Floor(NaN)) is undefined). A negative count is // likewise invalid for a purchase quantity. Fail loud on both. if math.IsNaN(count) || math.IsInf(count, 0) || count < 0 { - return 0, false, fmt.Errorf("recommended quantity %q is not a finite non-negative number", qty) + return 0, 0, false, fmt.Errorf("recommended quantity %q is not a finite non-negative number", raw) } - rounded = int(math.Round(count)) - return rounded, rounded == 0 && count > 0, nil + rounded = int(math.Floor(count)) + return rounded, count, rounded == 0 && count > 0, nil } // parseCostInformation extracts cost and savings information. diff --git a/providers/aws/recommendations/parser_ri_test.go b/providers/aws/recommendations/parser_ri_test.go index 3353250c..6c8269dd 100644 --- a/providers/aws/recommendations/parser_ri_test.go +++ b/providers/aws/recommendations/parser_ri_test.go @@ -36,11 +36,11 @@ func TestParseRecommendedQuantity(t *testing.T) { expectError: false, }, { - name: "Float quantity rounds to nearest", + name: "Float quantity rounds down (go#635 option C)", details: &types.ReservationPurchaseRecommendationDetail{ RecommendedNumberOfInstancesToPurchase: aws.String("3.8"), }, - expected: 4, // math.Round(3.8) = 4 + expected: 3, // never buy above CE's quantity expectError: false, }, { @@ -119,7 +119,7 @@ func TestParseRecommendedQuantity(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result, _, err := client.parseRecommendedQuantity(tt.details) + result, _, _, err := client.parseRecommendedQuantity(tt.details) if tt.expectError { assert.Error(t, err) @@ -141,7 +141,9 @@ func TestParseRecommendationDetail_WarnsWhenFractionRoundsToZero(t *testing.T) { {"0.05", 0, true}, {"0.4", 0, true}, {"0.4999", 0, true}, - {"0.5", 1, false}, // math.Round rounds half away from zero + {"0.5", 0, true}, + {"0.9999", 0, true}, // rounds down, never up (go#635) + {"1.5", 1, false}, {"2.4", 2, false}, {"0", 0, false}, {"0.0", 0, false}, @@ -829,3 +831,55 @@ func TestParseRecommendations_InvalidAverageInstancesIsReportedNotZeroed(t *test assert.Zero(t, recs[0].AverageInstancesUsedPerHour) }) } + +// go#635 option C: a fractional quantity rounds down, and the figures CE gives +// for that quantity scale to the bought count, so Count and the costs agree. +func TestParseRecommendationDetail_FloorsFractionalQuantityAndScalesCosts(t *testing.T) { + client := &Client{} + for _, tt := range []struct { + qty string + count int + scale float64 + }{ + {"2.5", 2, 2 / 2.5}, + {"2.4", 2, 2 / 2.4}, + {"3.99", 3, 3 / 3.99}, + {"3", 3, 1}, + {"3.0", 3, 1}, + {"0.6", 0, 1}, // rounds to 0: warned and left unscaled, as before + } { + t.Run(tt.qty, func(t *testing.T) { + rec, err := client.parseRecommendationDetail(context.Background(), &types.ReservationPurchaseRecommendationDetail{ + RecommendedNumberOfInstancesToPurchase: aws.String(tt.qty), + AccountId: aws.String("123456789012"), + CurrencyCode: aws.String("USD"), + UpfrontCost: aws.String("2500"), + RecurringStandardMonthlyCost: aws.String("100"), + EstimatedMonthlySavingsAmount: aws.String("50"), + EstimatedMonthlySavingsPercentage: aws.String("40"), + EstimatedMonthlyOnDemandCost: aws.String("125"), + EstimatedBreakEvenInMonths: aws.String("7"), + AverageNumberOfInstancesUsedPerHour: aws.String("2.7"), + InstanceDetails: &types.InstanceDetails{ + EC2InstanceDetails: &types.EC2InstanceDetails{ + InstanceType: aws.String("m5.large"), + Platform: aws.String("Linux/UNIX"), + Region: aws.String("us-east-1"), + Tenancy: aws.String("shared"), + }, + }, + }, common.RecommendationParams{Service: common.ServiceEC2, PaymentOption: "partial-upfront", Term: "1yr", LookbackPeriod: "7d"}) + require.NoError(t, err) + assert.Equal(t, tt.count, rec.Count) + assert.Equal(t, tt.count, rec.RecommendedCount) + assert.InDelta(t, 2500*tt.scale, rec.CommitmentCost, 1e-9, "upfront") + require.NotNil(t, rec.RecurringMonthlyCost) + assert.InDelta(t, 100*tt.scale, *rec.RecurringMonthlyCost, 1e-9, "recurring") + assert.InDelta(t, 50*tt.scale, rec.EstimatedSavings, 1e-9, "savings") + assert.InDelta(t, 40*tt.scale, rec.SavingsPercentage, 1e-9, "savings %% of an unchanged on-demand baseline") + assert.InDelta(t, 125, rec.OnDemandCost, 1e-9, "on-demand spend does not depend on the quantity bought") + assert.InDelta(t, 7, rec.BreakEvenMonths, 1e-9, "upfront/savings is unchanged by scaling") + assert.InDelta(t, 2.7, rec.AverageInstancesUsedPerHour, 1e-9, "demand is not scaled") + }) + } +} diff --git a/providers/aws/services/ec2/purchase_price_guard_test.go b/providers/aws/services/ec2/purchase_price_guard_test.go index 595ecca5..8a862dee 100644 --- a/providers/aws/services/ec2/purchase_price_guard_test.go +++ b/providers/aws/services/ec2/purchase_price_guard_test.go @@ -86,3 +86,27 @@ func TestCheckApprovedUpfront_RefusesMissingPriceAndNonUSDOffering(t *testing.T) eur.CurrencyCode = types.CurrencyCodeValues("EUR") require.ErrorContains(t, checkApprovedUpfront(pricedRec(), &eur), "USD only") } + +// go#635: CE recommends 2.5 instances at a $2500 upfront for that quantity. The +// parser floors it to 2 and scales the upfront to $2000 (pinned in +// recommendations.TestParseRecommendationDetail_FloorsFractionalQuantityAndScalesCosts), +// so a $1000 offering (2 x 1000) is bought, and one above the 5% tolerance is +// still refused. +func TestPurchaseCommitment_FlooredFractionalQuantity(t *testing.T) { + t.Parallel() + floored := func() common.Recommendation { + r := pricedRec() + r.Count = 2 + r.CommitmentCost = 2500 * 2 / 2.5 + return r + } + m := offeringsMock(pricedOffering(1000)) + res, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), floored(), common.PurchaseOptions{}) + require.NoError(t, err) + assert.True(t, res.Success) + + m = offeringsMock(pricedOffering(1051)) // 2102 > 2000 x 1.05 + _, err = (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), floored(), common.PurchaseOptions{}) + require.ErrorContains(t, err, "exceeds the approved upfront") + m.AssertNotCalled(t, "PurchaseReservedInstancesOffering", mock.Anything, mock.Anything) +} From f43140ab2f0fb2d867f33fd7fae1cd96c719919d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 15:20:48 +0100 Subject: [PATCH 2/2] fix(aws): scale a zero-count RI recommendation to zero cost and cap the quantity A fractional quantity below 1 floors to Count 0, but its upfront, recurring and savings kept the figures for the fractional quantity, so the row showed a full upfront for nothing bought and its savings counted toward estimated savings. Option C scales the costs to the bought count, which is 0 here. A quantity above MaxInt32 is now refused instead of saturating the count and shrinking the costs, since the purchase APIs take an int32. The EC2 floored-quantity test also asserts InstanceCount. Co-Authored-By: Claude Opus 5.5 --- providers/aws/recommendations/parser_ri.go | 38 ++++++++++++------- .../aws/recommendations/parser_ri_test.go | 30 ++++++++++++++- .../services/ec2/purchase_price_guard_test.go | 3 ++ 3 files changed, 56 insertions(+), 15 deletions(-) diff --git a/providers/aws/recommendations/parser_ri.go b/providers/aws/recommendations/parser_ri.go index 61a51c2e..3246db06 100644 --- a/providers/aws/recommendations/parser_ri.go +++ b/providers/aws/recommendations/parser_ri.go @@ -150,9 +150,10 @@ func (c *Client) parseRIUtilizationSignals(rec *common.Recommendation, details * // option C): the upfront, the recurring monthly charge, the monthly savings and // the savings percentage scale by count/qty. The on-demand baseline, break-even // (upfront over savings) and the usage signals do not depend on the quantity. -// A quantity that rounds to 0 is left unscaled; the caller warns about it. +// A quantity that rounds to 0 scales every cost to 0, so the row never carries +// an upfront or savings for instances it does not buy; the caller warns. func scaleToRoundedCount(rec *common.Recommendation, count int, qty float64) { - if count == 0 || float64(count) == qty { + if float64(count) == qty { return } f := float64(count) / qty @@ -178,26 +179,35 @@ func (c *Client) parseRecommendedQuantity(details *types.ReservationPurchaseReco raw := *details.RecommendedNumberOfInstancesToPurchase var count float64 - if _, err = fmt.Sscanf(raw, "%f", &count); err != nil { - if intCount, atoiErr := strconv.Atoi(raw); atoiErr == nil { - if intCount < 0 { - return 0, 0, false, fmt.Errorf("recommended quantity %q is negative", raw) - } - return intCount, float64(intCount), false, nil + if _, scanErr := fmt.Sscanf(raw, "%f", &count); scanErr != nil { + intCount, atoiErr := strconv.Atoi(raw) + if atoiErr != nil { + return 0, 0, false, fmt.Errorf("failed to parse quantity '%s' as float or int", raw) } - return 0, 0, false, fmt.Errorf("failed to parse quantity '%s' as float or int", raw) + count = float64(intCount) } - // Sscanf %f accepts "NaN"/"Inf"; a non-finite quantity would corrupt the - // purchase count (int(math.Floor(NaN)) is undefined). A negative count is - // likewise invalid for a purchase quantity. Fail loud on both. - if math.IsNaN(count) || math.IsInf(count, 0) || count < 0 { - return 0, 0, false, fmt.Errorf("recommended quantity %q is not a finite non-negative number", raw) + if err := checkQuantityRange(raw, count); err != nil { + return 0, 0, false, err } rounded = int(math.Floor(count)) return rounded, count, rounded == 0 && count > 0, nil } +// checkQuantityRange refuses a quantity that cannot be a purchase count. +// Sscanf %f accepts "NaN"/"Inf" (int(math.Floor(NaN)) is undefined), negatives +// are meaningless, and purchase APIs take an int32 count, so a larger value +// would saturate or truncate. +func checkQuantityRange(raw string, count float64) error { + if math.IsNaN(count) || math.IsInf(count, 0) || count < 0 { + return fmt.Errorf("recommended quantity %q is not a finite non-negative number", raw) + } + if count > math.MaxInt32 { + return fmt.Errorf("recommended quantity %q exceeds %d", raw, math.MaxInt32) + } + return nil +} + // parseCostInformation extracts cost and savings information. // // EstimatedMonthlySavingsAmount represents the savings from buying the full diff --git a/providers/aws/recommendations/parser_ri_test.go b/providers/aws/recommendations/parser_ri_test.go index 6c8269dd..0a8e3d61 100644 --- a/providers/aws/recommendations/parser_ri_test.go +++ b/providers/aws/recommendations/parser_ri_test.go @@ -107,6 +107,30 @@ func TestParseRecommendedQuantity(t *testing.T) { expected: 0, expectError: true, }, + { + name: "float quantity above MaxInt32 rejected", + details: &types.ReservationPurchaseRecommendationDetail{ + RecommendedNumberOfInstancesToPurchase: aws.String("1e10"), + }, + expected: 0, + expectError: true, + }, + { + name: "int quantity above MaxInt32 rejected", + details: &types.ReservationPurchaseRecommendationDetail{ + RecommendedNumberOfInstancesToPurchase: aws.String("3000000000"), + }, + expected: 0, + expectError: true, + }, + { + name: "MaxInt32 accepted", + details: &types.ReservationPurchaseRecommendationDetail{ + RecommendedNumberOfInstancesToPurchase: aws.String("2147483647"), + }, + expected: 2147483647, + expectError: false, + }, { name: "negative int quantity rejected", details: &types.ReservationPurchaseRecommendationDetail{ @@ -846,7 +870,11 @@ func TestParseRecommendationDetail_FloorsFractionalQuantityAndScalesCosts(t *tes {"3.99", 3, 3 / 3.99}, {"3", 3, 1}, {"3.0", 3, 1}, - {"0.6", 0, 1}, // rounds to 0: warned and left unscaled, as before + // Rounds to 0: option C scales the costs to 0 as well, so a Count-0 + // row never carries a full upfront or counts toward savings. + {"0.4", 0, 0}, + {"0.5", 0, 0}, + {"0.6", 0, 0}, } { t.Run(tt.qty, func(t *testing.T) { rec, err := client.parseRecommendationDetail(context.Background(), &types.ReservationPurchaseRecommendationDetail{ diff --git a/providers/aws/services/ec2/purchase_price_guard_test.go b/providers/aws/services/ec2/purchase_price_guard_test.go index 8a862dee..2b0cb766 100644 --- a/providers/aws/services/ec2/purchase_price_guard_test.go +++ b/providers/aws/services/ec2/purchase_price_guard_test.go @@ -104,6 +104,9 @@ func TestPurchaseCommitment_FlooredFractionalQuantity(t *testing.T) { res, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), floored(), common.PurchaseOptions{}) require.NoError(t, err) assert.True(t, res.Success) + m.AssertCalled(t, "PurchaseReservedInstancesOffering", mock.Anything, mock.MatchedBy(func(in *ec2.PurchaseReservedInstancesOfferingInput) bool { + return in.InstanceCount != nil && *in.InstanceCount == 2 + }), mock.Anything) m = offeringsMock(pricedOffering(1051)) // 2102 > 2000 x 1.05 _, err = (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), floored(), common.PurchaseOptions{})