diff --git a/CHANGELOG.md b/CHANGELOG.md index dfe4224e..b2da837f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -279,6 +279,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..3246db06 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,67 @@ 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 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 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 intCount < 0 { - return 0, false, fmt.Errorf("recommended quantity %q is negative", qty) - } - return 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, false, fmt.Errorf("failed to parse quantity '%s' as float or int", qty) + count = float64(intCount) } - // Sscanf %f accepts "NaN"/"Inf"; a non-finite quantity would corrupt the - // purchase count (int(math.Round(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) + if err := checkQuantityRange(raw, count); err != nil { + return 0, 0, false, err } - 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 +} + +// 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. diff --git a/providers/aws/recommendations/parser_ri_test.go b/providers/aws/recommendations/parser_ri_test.go index 3353250c..0a8e3d61 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, }, { @@ -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{ @@ -119,7 +143,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 +165,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 +855,59 @@ 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}, + // 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{ + 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..2b0cb766 100644 --- a/providers/aws/services/ec2/purchase_price_guard_test.go +++ b/providers/aws/services/ec2/purchase_price_guard_test.go @@ -86,3 +86,30 @@ 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.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{}) + require.ErrorContains(t, err, "exceeds the approved upfront") + m.AssertNotCalled(t, "PurchaseReservedInstancesOffering", mock.Anything, mock.Anything) +}