From 2351fa776fa8202fbc0b793040cb1056011d0068 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 20 Oct 2026 08:44:40 +0100 Subject: [PATCH 1/3] fix(ri-exchange): exclude convertible RIs with no pricing from cost math monthlyCostFromConvertibleRI returned 0 for a convertible RI whose FixedPrice, UsagePrice and RecurringHourlyAmount are all zero. reshape's pricing gate treats MonthlyCost <= 0 as "no source pricing" and passes every alternative, and costComponent scores it as neutral, so such an RI read as free. A real convertible RI always carries an upfront or hourly charge, so an all-zero price set means missing pricing data. The function now also returns an error when the computed monthly cost is not positive (all-zero, a negative sum, or NaN), the same way #1187 handles a non-positive duration: convertToExchangeTypes (api) reports the RI in excluded_ris with a pricing-specific reason and convertForAutoExchange (server) logs a warning and drops it. No Upfront RIs (FixedPrice 0 with an hourly charge) are unaffected. The reshape notice now shows each excluded RI's reason instead of always saying AWS returned no term length. Test fixtures that modelled RIs with no pricing now carry a price. Closes #1188 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/riexchange.test.ts | 17 ++- frontend/src/riexchange.ts | 4 +- internal/api/handler_ri_exchange.go | 35 +++++- .../handler_ri_exchange_integration_test.go | 20 +++- internal/api/handler_ri_exchange_test.go | 106 +++++++++++++++++- internal/api/openapi.yaml | 9 +- internal/server/handler_ri_exchange.go | 15 ++- ...i_exchange_reservation_integration_test.go | 2 +- internal/server/handler_ri_exchange_test.go | 49 ++++++++ internal/server/ri_exchange_zone_test.go | 5 +- 10 files changed, 238 insertions(+), 24 deletions(-) diff --git a/frontend/src/__tests__/riexchange.test.ts b/frontend/src/__tests__/riexchange.test.ts index e9f42d41c..e3089e40d 100644 --- a/frontend/src/__tests__/riexchange.test.ts +++ b/frontend/src/__tests__/riexchange.test.ts @@ -1382,12 +1382,25 @@ describe('excluded RIs notice', () => { await loadReshapeRecommendations(); const notice = document.getElementById('ri-exchange-recommendations-excluded'); - expect(notice?.textContent).toContain('2 convertible RIs were left out because AWS returned no term length'); - expect(notice?.textContent).toContain('ri-0'); + expect(notice?.textContent).toContain('2 convertible RIs were left out of the analysis'); + expect(notice?.textContent).toContain('ri-0: no term'); expect(notice?.textContent).toContain(''); expect(notice?.querySelector('img')).toBeNull(); }); + it('shows each RI with its own reason, not a fixed term-length message (issue #1188)', () => { + renderExcludedRIsNotice([ + { ri_id: 'ri-noterm', reason: 'AWS returned no term length (duration), so its monthly cost cannot be computed' }, + { ri_id: 'ri-unpriced', reason: 'AWS returned no pricing (fixed, usage and recurring charges total zero or less), so its monthly cost cannot be computed' }, + ]); + const text = document.getElementById('ri-exchange-recommendations-excluded')?.textContent ?? ''; + expect(text).toContain('ri-noterm: AWS returned no term length'); + expect(text).toContain('ri-unpriced: AWS returned no pricing'); + + renderExcludedRIsNotice([{ ri_id: 'ri-unpriced', reason: 'AWS returned no pricing' }]); + expect(document.getElementById('ri-exchange-recommendations-excluded')?.textContent).not.toContain('term length'); + }); + it('shows no notice when excluded_ris is empty or absent, and clears a stale one', () => { renderExcludedRIsNotice([{ ri_id: 'ri-0', reason: 'x' }]); expect(document.getElementById('ri-exchange-recommendations-excluded')?.textContent).toContain('1 convertible RI was left out'); diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index 4b0855f7b..1290c9b7b 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -569,7 +569,9 @@ export function renderExcludedRIsNotice(excluded: ExcludedRI[] | undefined): voi } const n = excluded.length; notice.className = 'freshness-banner warning'; - notice.textContent = `${n} convertible RI${n === 1 ? ' was' : 's were'} left out because AWS returned no term length: ${excluded.map((e) => e.ri_id).join(', ')}`; + // The backend names why each RI was left out (no term length, no pricing), + // so show its reason rather than assuming one cause for all of them. + notice.textContent = `${n} convertible RI${n === 1 ? ' was' : 's were'} left out of the analysis. ${excluded.map((e) => `${e.ri_id}: ${e.reason}`).join('; ')}`; } /** diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index ed6ee02d5..1cd279814 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -1599,18 +1599,43 @@ func parseThresholdParam(params map[string]string) (float64, error) { // monthly = (FixedPrice / hours_per_term + UsagePrice + recurring_hourly) × 730 // // 730 is AWS's canonical hours-per-month constant. Returns an error when -// Duration is non-positive: a zero cost would read as a free RI and skip -// reshape's dollar-units pricing gate, so callers must exclude the RI. +// Duration is non-positive or when the computed cost is not positive (for +// example FixedPrice, UsagePrice and RecurringHourlyAmount all zero, which +// means AWS returned no pricing, not a free RI): a zero cost would read as a +// free RI and skip reshape's dollar-units pricing gate, so callers must +// exclude the RI. The error wraps errRINonPositiveDuration or errRINoPricing. // // Used to populate exchange.RIInfo.MonthlyCost so the cross-family // dollar-units pre-filter can compare against per-target offering // costs computed with the same formula. func monthlyCostFromConvertibleRI(ri ec2svc.ConvertibleRI) (float64, error) { if ri.Duration <= 0 { - return 0, fmt.Errorf("convertible RI %s has non-positive duration %d: monthly cost is not computable", ri.ReservedInstanceID, ri.Duration) + return 0, fmt.Errorf("convertible RI %s has %w %d: monthly cost is not computable", ri.ReservedInstanceID, errRINonPositiveDuration, ri.Duration) } hoursPerTerm := float64(ri.Duration) / 3600 - return ((ri.FixedPrice / hoursPerTerm) + ri.UsagePrice + ri.RecurringHourlyAmount) * 730, nil + cost := ((ri.FixedPrice / hoursPerTerm) + ri.UsagePrice + ri.RecurringHourlyAmount) * 730 + // !(cost > 0) also rejects NaN, which is no usable price either. + if !(cost > 0) { + return 0, fmt.Errorf("convertible RI %s has %w (fixed %v, usage %v, recurring hourly %v): monthly cost is not computable", + ri.ReservedInstanceID, errRINoPricing, ri.FixedPrice, ri.UsagePrice, ri.RecurringHourlyAmount) + } + return cost, nil +} + +// Sentinel causes for monthlyCostFromConvertibleRI, used to pick the reason +// shown for an RI excluded from reshape analysis. +var ( + errRINonPositiveDuration = errors.New("non-positive duration") + errRINoPricing = errors.New("no positive price") +) + +// excludedRIReason is the user-facing reason for an RI left out of reshape +// analysis because monthlyCostFromConvertibleRI returned err. +func excludedRIReason(err error) string { + if errors.Is(err, errRINoPricing) { + return "AWS returned no pricing (fixed, usage and recurring charges total zero or less), so its monthly cost cannot be computed" + } + return "AWS returned no term length (duration), so its monthly cost cannot be computed" } // convertToExchangeTypes converts provider-specific types to the exchange package types. @@ -1624,7 +1649,7 @@ func convertToExchangeTypes(instances []ec2svc.ConvertibleRI, utilData []recomme monthlyCost, err := monthlyCostFromConvertibleRI(inst) if err != nil { logging.Warnf("convertToExchangeTypes: excluding RI from reshape: %v", err) - excluded = append(excluded, ExcludedRI{RIID: inst.ReservedInstanceID, Reason: "AWS returned no term length (duration), so its monthly cost cannot be computed"}) + excluded = append(excluded, ExcludedRI{RIID: inst.ReservedInstanceID, Reason: excludedRIReason(err)}) continue } riInfos = append(riInfos, exchange.RIInfo{ diff --git a/internal/api/handler_ri_exchange_integration_test.go b/internal/api/handler_ri_exchange_integration_test.go index 13b71e06a..b920875c4 100644 --- a/internal/api/handler_ri_exchange_integration_test.go +++ b/internal/api/handler_ri_exchange_integration_test.go @@ -109,6 +109,12 @@ func seedRecsForRegion(ctx context.Context, t *testing.T, store *config.Postgres "seeding recommendations into the test container failed") } +// reshapeIntegrationFixedPrice gives fixture RIs a real price (an unpriced +// RI is excluded, issue #1188) small enough that the dollar-units gate keeps +// the seeded cross-family recs: 100 over 1y is ~8.33/month, and NF 8 x 8.33 +// stays under the c5.large / r5.large recs' NF 4 x 50 and NF 4 x 60. +const reshapeIntegrationFixedPrice = 100 + // TestReshapeRecommendations_Integration_EndToEnd exercises the full // path: auth → cache cold-fetch → AnalyzeReshapingWithRecs (recs-driven // alternatives) → JSON response. Asserts the response carries @@ -135,8 +141,9 @@ func TestReshapeRecommendations_Integration_EndToEnd(t *testing.T) { ec2Fake := &fakeReshapeEC2{ instances: []ec2svc.ConvertibleRI{ - {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000}, + {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000, FixedPrice: reshapeIntegrationFixedPrice}, {ReservedInstanceID: "ri-noterm", InstanceType: "m5.xlarge", InstanceCount: 1}, + {ReservedInstanceID: "ri-unpriced", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000}, }, } recsFake := &fakeReshapeRecs{ @@ -154,8 +161,11 @@ func TestReshapeRecommendations_Integration_EndToEnd(t *testing.T) { rec := body.Recommendations[0] require.Equal(t, "ri-1", rec.SourceRIID) require.Equal(t, "m5.large", rec.TargetInstanceType) - require.Len(t, body.ExcludedRIs, 1, "an RI with no term length must be reported, not silently skipped") + require.Len(t, body.ExcludedRIs, 2, "RIs with no term length or no pricing must be reported, not silently skipped") require.Equal(t, "ri-noterm", body.ExcludedRIs[0].RIID) + require.Contains(t, body.ExcludedRIs[0].Reason, "term length") + require.Equal(t, "ri-unpriced", body.ExcludedRIs[1].RIID) + require.Contains(t, body.ExcludedRIs[1].Reason, "no pricing") // Same-family m5 stripped; c5 + r5 surface ascending by cost. require.Len(t, rec.AlternativeTargets, 2, @@ -200,7 +210,7 @@ func TestReshapeRecommendations_Integration_SecondCallHitsCache(t *testing.T) { ec2Fake := &fakeReshapeEC2{ instances: []ec2svc.ConvertibleRI{ - {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000}, + {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000, FixedPrice: reshapeIntegrationFixedPrice}, }, } recsFake := &fakeReshapeRecs{ @@ -236,7 +246,7 @@ func TestReshapeRecommendations_Integration_NoCachedRecsReturnsPrimaryOnly(t *te ec2Fake := &fakeReshapeEC2{ instances: []ec2svc.ConvertibleRI{ - {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000}, + {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000, FixedPrice: reshapeIntegrationFixedPrice}, }, } recsFake := &fakeReshapeRecs{ @@ -337,7 +347,7 @@ func TestReshapeRecommendations_Integration_ScopedAccount_FiltersToAccount(t *te ec2Fake := &fakeReshapeEC2{ instances: []ec2svc.ConvertibleRI{ - {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000}, + {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000, FixedPrice: reshapeIntegrationFixedPrice}, }, } recsFake := &fakeReshapeRecs{ diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 499923dd2..1a733e73e 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "math" "math/big" "strings" "testing" @@ -974,7 +975,7 @@ func TestGetReshapeRecommendations_EmptyRegionUsesConfigRegion(t *testing.T) { reshapeEC2Factory: func(_ aws.Config) reshapeEC2Client { return &fakeReshapeEC2Stub{ instances: []ec2svc.ConvertibleRI{ - {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, CurrencyCode: "USD", Duration: 31536000}, + {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, CurrencyCode: "USD", Duration: 31536000, FixedPrice: 876}, }, } }, @@ -1059,8 +1060,11 @@ func TestGetReshapeRecommendations_AlternativeCurrencyFromRIs(t *testing.T) { config: mockStore, auth: &mockAuthForExchange{}, reshapeEC2Factory: func(_ aws.Config) reshapeEC2Client { + // FixedPrice 100 over 1y is ~8.33/month; NF 8 x 8.33 stays + // under the c6i.large rec's NF 4 x 40, so the dollar-units + // gate keeps the alternative this test is about. return &fakeReshapeEC2Stub{instances: []ec2svc.ConvertibleRI{ - {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, CurrencyCode: tc.riCurrency, Duration: 31536000}, + {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, CurrencyCode: tc.riCurrency, Duration: 31536000, FixedPrice: 100}, }} }, reshapeRecsFactory: func(_ aws.Config) reshapeRecsClient { @@ -2803,3 +2807,101 @@ func TestMonthlyCostFromConvertibleRI_NonPositiveDurationErrors(t *testing.T) { require.NoError(t, err) assert.InDelta(t, 730.0, cost, 1e-6) } + +// A convertible RI whose FixedPrice, UsagePrice and RecurringHourlyAmount are +// all zero has no pricing data; it is not free. MonthlyCost=0 would make +// reshape's pricing gate pass every alternative and score the RI as neutral, +// so it must be excluded (issue #1188). A No Upfront RI (FixedPrice 0 with an +// hourly charge) is real pricing and must be kept. +func TestConvertToExchangeTypes_ExcludesRIWithoutPricing(t *testing.T) { + const oneYear = 31536000 + instances := []ec2svc.ConvertibleRI{ + {ReservedInstanceID: "ri-unpriced", InstanceType: "m5.large", InstanceCount: 1, Duration: oneYear}, + {ReservedInstanceID: "ri-no-upfront", InstanceType: "m5.large", InstanceCount: 1, RecurringHourlyAmount: 0.05, Duration: oneYear}, + {ReservedInstanceID: "ri-usage-only", InstanceType: "m5.large", InstanceCount: 1, UsagePrice: 0.04, Duration: oneYear}, + {ReservedInstanceID: "ri-all-upfront", InstanceType: "m5.large", InstanceCount: 1, FixedPrice: 876, Duration: oneYear}, + {ReservedInstanceID: "ri-negative", InstanceType: "c5.large", InstanceCount: 1, FixedPrice: -876, Duration: oneYear}, + } + + riInfos, _, excluded := convertToExchangeTypes(instances, nil) + + require.Len(t, riInfos, 3) + want := map[string]float64{"ri-no-upfront": 0.05 * 730, "ri-usage-only": 0.04 * 730, "ri-all-upfront": 876.0 / 8760 * 730} + for _, ri := range riInfos { + cost, ok := want[ri.ID] + require.True(t, ok, "unexpected RI %s kept", ri.ID) + assert.InDelta(t, cost, ri.MonthlyCost, 1e-6, ri.ID) + assert.Greater(t, ri.MonthlyCost, 0.0, ri.ID) + } + require.Len(t, excluded, 2) + assert.Equal(t, "ri-unpriced", excluded[0].RIID) + assert.Equal(t, "ri-negative", excluded[1].RIID) + for _, e := range excluded { + assert.Contains(t, e.Reason, "no pricing") + assert.NotContains(t, e.Reason, "term length") + } +} + +// End to end through the reshape handler: an unpriced RI is reported in +// excluded_ris with the pricing reason and gets no recommendation, while a +// priced RI in the same account still does. +func TestGetReshapeRecommendations_ReportsUnpricedRIs(t *testing.T) { + mockStore := &MockConfigStore{} + mockStore.On("ListStoredRecommendations", mock.Anything, mock.Anything). + Return([]config.RecommendationRecord(nil), nil) + h := &Handler{ + config: mockStore, + auth: &mockAuthForExchange{}, + reshapeEC2Factory: func(_ aws.Config) reshapeEC2Client { + return &fakeReshapeEC2Stub{instances: []ec2svc.ConvertibleRI{ + {ReservedInstanceID: "ri-unpriced", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000, CurrencyCode: "USD"}, + {ReservedInstanceID: "ri-ok", InstanceType: "m5.xlarge", InstanceCount: 1, FixedPrice: 8760, Duration: 31536000, CurrencyCode: "USD"}, + }} + }, + reshapeRecsFactory: func(_ aws.Config) reshapeRecsClient { + return &fakeReshapeRecsStub{utilization: []recommendations.RIUtilization{ + {ReservedInstanceID: "ri-unpriced", UtilizationPercent: 50.0}, + {ReservedInstanceID: "ri-ok", UtilizationPercent: 50.0}, + }} + }, + reshapeAccountResolver: func(_ context.Context) (string, error) { return "", nil }, + } + h.awsCfgOnce.Do(func() { h.awsCfg = aws.Config{Region: "us-east-1"} }) + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer test-token"}, + QueryStringParameters: map[string]string{"lookback_days": "30", "threshold": "95.0"}, + } + out, err := h.getReshapeRecommendations(context.Background(), req) + require.NoError(t, err) + resp, ok := out.(*ReshapeRecommendationsResponse) + require.True(t, ok) + + require.Len(t, resp.ExcludedRIs, 1) + assert.Equal(t, "ri-unpriced", resp.ExcludedRIs[0].RIID) + assert.Contains(t, resp.ExcludedRIs[0].Reason, "no pricing") + require.NotEmpty(t, resp.Recommendations) + for _, r := range resp.Recommendations { + assert.NotEqual(t, "ri-unpriced", r.SourceRIID) + } +} + +func TestMonthlyCostFromConvertibleRI_NoPositivePriceErrors(t *testing.T) { + for name, ri := range map[string]ec2svc.ConvertibleRI{ + "all zero": {ReservedInstanceID: "ri-x", Duration: 31536000}, + "negative sum": {ReservedInstanceID: "ri-x", FixedPrice: -876, Duration: 31536000}, + "NaN": {ReservedInstanceID: "ri-x", UsagePrice: math.NaN(), Duration: 31536000}, + } { + cost, err := monthlyCostFromConvertibleRI(ri) + require.Error(t, err, name) + assert.Zero(t, cost, name) + assert.Contains(t, err.Error(), "ri-x", name) + assert.ErrorIs(t, err, errRINoPricing, name) + assert.NotErrorIs(t, err, errRINonPositiveDuration, name) + } + _, err := monthlyCostFromConvertibleRI(ec2svc.ConvertibleRI{ReservedInstanceID: "ri-y"}) + assert.ErrorIs(t, err, errRINonPositiveDuration, "duration is checked before pricing") + cost, err := monthlyCostFromConvertibleRI(ec2svc.ConvertibleRI{RecurringHourlyAmount: 0.05, Duration: 31536000}) + require.NoError(t, err) + assert.InDelta(t, 36.5, cost, 1e-6) +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 31f3dbd43..d0531fd39 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -1333,9 +1333,12 @@ paths: excluded_ris: type: array description: > - Convertible RIs left out of cost-based analysis (for - example AWS returned no term length), so they have no - recommendation. Omitted when empty. + Convertible RIs left out of cost-based analysis + because their monthly cost cannot be computed (AWS + returned no term length, or no pricing: fixed, usage + and recurring charges total zero or less), so they have + no recommendation. reason says which. Omitted when + empty. items: type: object properties: diff --git a/internal/server/handler_ri_exchange.go b/internal/server/handler_ri_exchange.go index 86d18605f..4fbcf0d05 100644 --- a/internal/server/handler_ri_exchange.go +++ b/internal/server/handler_ri_exchange.go @@ -244,8 +244,11 @@ func buildExchangeNotificationData(result *exchange.AutoExchangeResult, dashboar // // monthly = (FixedPrice / hours_per_term + UsagePrice + recurring_hourly) × 730 // -// Returns an error when Duration is non-positive: a zero cost would read -// as a free RI and skip the pricing gate, so callers must exclude the RI. +// Returns an error when Duration is non-positive or when the computed cost +// is not positive (for example FixedPrice, UsagePrice and +// RecurringHourlyAmount all zero, which means AWS returned no pricing, not a +// free RI): a zero cost would read as a free RI and skip the pricing gate, +// so callers must exclude the RI. // // Used to populate exchange.RIInfo.MonthlyCost so the cross-family // dollar-units pre-filter compares apples-to-apples against per-target @@ -255,7 +258,13 @@ func monthlyCostFromConvertibleRI(ri ec2svc.ConvertibleRI) (float64, error) { return 0, fmt.Errorf("convertible RI %s has non-positive duration %d: monthly cost is not computable", ri.ReservedInstanceID, ri.Duration) } hoursPerTerm := float64(ri.Duration) / 3600 - return ((ri.FixedPrice / hoursPerTerm) + ri.UsagePrice + ri.RecurringHourlyAmount) * 730, nil + cost := ((ri.FixedPrice / hoursPerTerm) + ri.UsagePrice + ri.RecurringHourlyAmount) * 730 + // !(cost > 0) also rejects NaN, which is no usable price either. + if !(cost > 0) { + return 0, fmt.Errorf("convertible RI %s has no positive price (fixed %v, usage %v, recurring hourly %v): monthly cost is not computable", + ri.ReservedInstanceID, ri.FixedPrice, ri.UsagePrice, ri.RecurringHourlyAmount) + } + return cost, nil } // convertForAutoExchange converts provider-specific types to exchange package types, diff --git a/internal/server/handler_ri_exchange_reservation_integration_test.go b/internal/server/handler_ri_exchange_reservation_integration_test.go index 058e17415..82a74d6a7 100644 --- a/internal/server/handler_ri_exchange_reservation_integration_test.go +++ b/internal/server/handler_ri_exchange_reservation_integration_test.go @@ -74,7 +74,7 @@ func TestRIExchangeAutoWorkers_ConcreteStoreAndFixtureClient(t *testing.T) { } ri := func(id string) []ec2svc.ConvertibleRI { return []ec2svc.ConvertibleRI{{ReservedInstanceID: id, InstanceType: "c5.xlarge", InstanceCount: 1, NormalizationFactor: 8, - ProductDescription: "Linux/UNIX", InstanceTenancy: "default", Scope: "Region", Duration: 31536000}} + ProductDescription: "Linux/UNIX", InstanceTenancy: "default", Scope: "Region", Duration: 31536000, FixedPrice: 876}} } util := func(id string) []recommendations.RIUtilization { return []recommendations.RIUtilization{{ReservedInstanceID: id, UtilizationPercent: 30}} diff --git a/internal/server/handler_ri_exchange_test.go b/internal/server/handler_ri_exchange_test.go index 6d4d998bc..b5bd1890c 100644 --- a/internal/server/handler_ri_exchange_test.go +++ b/internal/server/handler_ri_exchange_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "math" "math/big" "strings" "testing" @@ -140,6 +141,7 @@ func TestConvertForAutoExchange(t *testing.T) { ProductDescription: "Linux/UNIX", InstanceTenancy: "default", Scope: "Region", + FixedPrice: 876, Duration: 31536000, }, { @@ -150,6 +152,7 @@ func TestConvertForAutoExchange(t *testing.T) { ProductDescription: "Windows", InstanceTenancy: "dedicated", Scope: "Availability Zone", + FixedPrice: 876, Duration: 94608000, }, } @@ -568,6 +571,7 @@ func TestExecuteRIExchangeReshape_ManualMode(t *testing.T) { ProductDescription: "Linux/UNIX", InstanceTenancy: "default", Scope: "Region", + FixedPrice: 876, Duration: 31536000, }, }, nil @@ -675,6 +679,7 @@ func TestExecuteRIExchangeReshape_AutoMode(t *testing.T) { ProductDescription: "Linux/UNIX", InstanceTenancy: "default", Scope: "Region", + FixedPrice: 876, Duration: 31536000, }, }, nil @@ -783,6 +788,7 @@ func TestExecuteRIExchangeReshape_DailyCapHitMidRun(t *testing.T) { ProductDescription: "Linux/UNIX", InstanceTenancy: "default", Scope: "Region", + FixedPrice: 876, Duration: 31536000, }, { @@ -793,6 +799,7 @@ func TestExecuteRIExchangeReshape_DailyCapHitMidRun(t *testing.T) { ProductDescription: "Linux/UNIX", InstanceTenancy: "default", Scope: "Region", + FixedPrice: 876, Duration: 31536000, }, }, nil @@ -1106,3 +1113,45 @@ func TestMonthlyCostFromConvertibleRI_NonPositiveDurationErrors(t *testing.T) { t.Fatalf("valid RI: cost=%v err=%v", cost, err) } } + +// A convertible RI whose FixedPrice, UsagePrice and RecurringHourlyAmount are +// all zero has no pricing data; it is not free. It must be excluded like a +// non-positive duration (issue #1188), while a No Upfront RI (FixedPrice 0 +// with an hourly charge) is real pricing and must be kept. +func TestConvertForAutoExchange_ExcludesRIWithoutPricing(t *testing.T) { + const oneYear = 31536000 + instances := []ec2svc.ConvertibleRI{ + {ReservedInstanceID: "ri-unpriced", InstanceType: "m5.large", InstanceCount: 1, Duration: oneYear}, + {ReservedInstanceID: "ri-no-upfront", InstanceType: "m5.large", InstanceCount: 1, RecurringHourlyAmount: 0.05, Duration: oneYear}, + {ReservedInstanceID: "ri-negative", InstanceType: "c5.large", InstanceCount: 1, FixedPrice: -876, Duration: oneYear}, + } + + riInfos, _, meta := convertForAutoExchange(instances, nil) + + if len(riInfos) != 1 || riInfos[0].ID != "ri-no-upfront" { + t.Fatalf("want only ri-no-upfront, got %+v", riInfos) + } + if want := 0.05 * 730; riInfos[0].MonthlyCost < want-1e-6 || riInfos[0].MonthlyCost > want+1e-6 { + t.Fatalf("MonthlyCost = %v, want %v", riInfos[0].MonthlyCost, want) + } + if _, ok := meta["ri-no-upfront"]; !ok || len(meta) != 1 { + t.Fatalf("metadata must cover only ri-no-upfront, got %+v", meta) + } +} + +func TestMonthlyCostFromConvertibleRI_NoPositivePriceErrors(t *testing.T) { + for name, ri := range map[string]ec2svc.ConvertibleRI{ + "all zero": {ReservedInstanceID: "ri-x", Duration: 31536000}, + "negative sum": {ReservedInstanceID: "ri-x", FixedPrice: -876, Duration: 31536000}, + "NaN": {ReservedInstanceID: "ri-x", UsagePrice: math.NaN(), Duration: 31536000}, + } { + cost, err := monthlyCostFromConvertibleRI(ri) + if err == nil || cost != 0 || !strings.Contains(err.Error(), "ri-x") || !strings.Contains(err.Error(), "no positive price") { + t.Fatalf("%s: want error naming ri-x and the missing price, got cost=%v err=%v", name, cost, err) + } + } + cost, err := monthlyCostFromConvertibleRI(ec2svc.ConvertibleRI{RecurringHourlyAmount: 0.05, Duration: 31536000}) + if err != nil || cost < 36.5-1e-6 || cost > 36.5+1e-6 { + t.Fatalf("No Upfront RI: cost=%v err=%v", cost, err) + } +} diff --git a/internal/server/ri_exchange_zone_test.go b/internal/server/ri_exchange_zone_test.go index 1515fd590..d98888695 100644 --- a/internal/server/ri_exchange_zone_test.go +++ b/internal/server/ri_exchange_zone_test.go @@ -58,6 +58,7 @@ func zoneTestRI(scope ec2types.Scope, az string) ec2types.ReservedInstances { InstanceTenancy: ec2types.TenancyDefault, Scope: scope, Duration: aws.Int64(31536000), + FixedPrice: aws.Float32(876), OfferingClass: ec2types.OfferingClassTypeConvertible, CurrencyCode: ec2types.CurrencyCodeValuesUsd, } @@ -179,8 +180,8 @@ func TestNewConvertibleOfferingLookup_MapsEveryField(t *testing.T) { func TestConvertForAutoExchange_CarriesAvailabilityZone(t *testing.T) { _, _, meta := convertForAutoExchange([]ec2svc.ConvertibleRI{ - {ReservedInstanceID: "ri-zonal", Scope: "Availability Zone", AvailabilityZone: "us-east-1b", Duration: 31536000}, - {ReservedInstanceID: "ri-regional", Scope: "Region", Duration: 31536000}, + {ReservedInstanceID: "ri-zonal", Scope: "Availability Zone", AvailabilityZone: "us-east-1b", Duration: 31536000, FixedPrice: 876}, + {ReservedInstanceID: "ri-regional", Scope: "Region", Duration: 31536000, FixedPrice: 876}, }, nil) assert.Equal(t, "us-east-1b", meta["ri-zonal"].AvailabilityZone) assert.Empty(t, meta["ri-regional"].AvailabilityZone) From e93092fc4c6b266286a491fd4959f173ccb7b14b Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 20 Oct 2026 08:45:52 +0100 Subject: [PATCH 2/3] revert(frontend): leave the excluded-RI notice to the UI owner The backend now names each excluded RI's reason in excluded_ris; the notice text change is left for the frontend owner (see PR body). Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/riexchange.test.ts | 17 ++--------------- frontend/src/riexchange.ts | 4 +--- 2 files changed, 3 insertions(+), 18 deletions(-) diff --git a/frontend/src/__tests__/riexchange.test.ts b/frontend/src/__tests__/riexchange.test.ts index e3089e40d..e9f42d41c 100644 --- a/frontend/src/__tests__/riexchange.test.ts +++ b/frontend/src/__tests__/riexchange.test.ts @@ -1382,25 +1382,12 @@ describe('excluded RIs notice', () => { await loadReshapeRecommendations(); const notice = document.getElementById('ri-exchange-recommendations-excluded'); - expect(notice?.textContent).toContain('2 convertible RIs were left out of the analysis'); - expect(notice?.textContent).toContain('ri-0: no term'); + expect(notice?.textContent).toContain('2 convertible RIs were left out because AWS returned no term length'); + expect(notice?.textContent).toContain('ri-0'); expect(notice?.textContent).toContain(''); expect(notice?.querySelector('img')).toBeNull(); }); - it('shows each RI with its own reason, not a fixed term-length message (issue #1188)', () => { - renderExcludedRIsNotice([ - { ri_id: 'ri-noterm', reason: 'AWS returned no term length (duration), so its monthly cost cannot be computed' }, - { ri_id: 'ri-unpriced', reason: 'AWS returned no pricing (fixed, usage and recurring charges total zero or less), so its monthly cost cannot be computed' }, - ]); - const text = document.getElementById('ri-exchange-recommendations-excluded')?.textContent ?? ''; - expect(text).toContain('ri-noterm: AWS returned no term length'); - expect(text).toContain('ri-unpriced: AWS returned no pricing'); - - renderExcludedRIsNotice([{ ri_id: 'ri-unpriced', reason: 'AWS returned no pricing' }]); - expect(document.getElementById('ri-exchange-recommendations-excluded')?.textContent).not.toContain('term length'); - }); - it('shows no notice when excluded_ris is empty or absent, and clears a stale one', () => { renderExcludedRIsNotice([{ ri_id: 'ri-0', reason: 'x' }]); expect(document.getElementById('ri-exchange-recommendations-excluded')?.textContent).toContain('1 convertible RI was left out'); diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index 1290c9b7b..4b0855f7b 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -569,9 +569,7 @@ export function renderExcludedRIsNotice(excluded: ExcludedRI[] | undefined): voi } const n = excluded.length; notice.className = 'freshness-banner warning'; - // The backend names why each RI was left out (no term length, no pricing), - // so show its reason rather than assuming one cause for all of them. - notice.textContent = `${n} convertible RI${n === 1 ? ' was' : 's were'} left out of the analysis. ${excluded.map((e) => `${e.ri_id}: ${e.reason}`).join('; ')}`; + notice.textContent = `${n} convertible RI${n === 1 ? ' was' : 's were'} left out because AWS returned no term length: ${excluded.map((e) => e.ri_id).join(', ')}`; } /** From 9df76b650e4e61888bed69cf9a38332cd2bd98ef Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Wed, 21 Oct 2026 08:25:57 +0100 Subject: [PATCH 3/3] docs(ri-exchange): cross-reference the two monthlyCostFromConvertibleRI copies Note in each copy that the other exists and how they differ (only the api copy wraps the sentinels), and say that convertForAutoExchange also serves the ladder write path, naming both in its exclusion log line. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_ri_exchange.go | 4 ++++ internal/server/handler_ri_exchange.go | 11 +++++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index c0d5cc8de..0a4644552 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -1614,6 +1614,10 @@ func parseThresholdParam(params map[string]string) (float64, error) { // Used to populate exchange.RIInfo.MonthlyCost so the cross-family // dollar-units pre-filter can compare against per-target offering // costs computed with the same formula. +// +// Keep in sync with the copy in internal/server/handler_ri_exchange.go +// (auto-exchange and ladder path), which applies the same checks but does +// not wrap the sentinels. func monthlyCostFromConvertibleRI(ri ec2svc.ConvertibleRI) (float64, error) { if ri.Duration <= 0 { return 0, fmt.Errorf("convertible RI %s has %w %d: monthly cost is not computable", ri.ReservedInstanceID, errRINonPositiveDuration, ri.Duration) diff --git a/internal/server/handler_ri_exchange.go b/internal/server/handler_ri_exchange.go index 4fbcf0d05..b79760e58 100644 --- a/internal/server/handler_ri_exchange.go +++ b/internal/server/handler_ri_exchange.go @@ -253,6 +253,11 @@ func buildExchangeNotificationData(result *exchange.AutoExchangeResult, dashboar // Used to populate exchange.RIInfo.MonthlyCost so the cross-family // dollar-units pre-filter compares apples-to-apples against per-target // offering costs. +// +// Keep in sync with the copy in internal/api/handler_ri_exchange.go. That +// copy also wraps errRINonPositiveDuration / errRINoPricing so the API can +// report a per-RI reason; this one does not, because the callers here only +// log and drop the RI. func monthlyCostFromConvertibleRI(ri ec2svc.ConvertibleRI) (float64, error) { if ri.Duration <= 0 { return 0, fmt.Errorf("convertible RI %s has non-positive duration %d: monthly cost is not computable", ri.ReservedInstanceID, ri.Duration) @@ -268,7 +273,9 @@ func monthlyCostFromConvertibleRI(ri ec2svc.ConvertibleRI) (float64, error) { } // convertForAutoExchange converts provider-specific types to exchange package types, -// including RI metadata needed for offering lookup. +// including RI metadata needed for offering lookup. It serves both the +// auto-exchange run and the ladder write path (ladder_write.go); an RI whose +// monthly cost is not computable is logged and dropped on both. func convertForAutoExchange(instances []ec2svc.ConvertibleRI, utilData []recommendations.RIUtilization) ([]exchange.RIInfo, []exchange.UtilizationInfo, map[string]exchange.RIMetadataInfo) { riInfos := make([]exchange.RIInfo, 0, len(instances)) riMetadata := make(map[string]exchange.RIMetadataInfo, len(instances)) @@ -277,7 +284,7 @@ func convertForAutoExchange(instances []ec2svc.ConvertibleRI, utilData []recomme inst := instances[i] monthlyCost, err := monthlyCostFromConvertibleRI(inst) if err != nil { - logging.Warnf("convertForAutoExchange: excluding RI from auto-exchange: %v", err) + logging.Warnf("convertForAutoExchange: excluding RI from auto-exchange and ladder reshaping: %v", err) continue } riInfos = append(riInfos, exchange.RIInfo{