From ff3420c69e30d6e03c3d75771df4292bab81519b Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 04:47:06 +0000 Subject: [PATCH] refactor(ri-exchange): share the convertible RI monthly cost formula internal/api and internal/server each carried a verbatim copy of monthlyCostFromConvertibleRI with a bare 730. Move the one implementation to the new internal/ricost package with a named HoursPerMonth constant; both packages' monthlyCostFromConvertibleRI now delegate to it. Results are bit-for-bit unchanged; a table test pins the exact values the old copies returned, including the non-positive duration guard and zero prices. The frontend half of the issue (730 vs 1/720) is left for the UI owner. Refs #337 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_ri_exchange.go | 23 +++------- internal/ricost/monthly_cost.go | 40 ++++++++++++++++++ internal/ricost/monthly_cost_test.go | 58 ++++++++++++++++++++++++++ internal/server/handler_ri_exchange.go | 22 +++------- 4 files changed, 108 insertions(+), 35 deletions(-) create mode 100644 internal/ricost/monthly_cost.go create mode 100644 internal/ricost/monthly_cost_test.go diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 1df14783b..12373aeba 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -30,6 +30,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/credentials" "github.com/LeanerCloud/cloud-commitments-platform/internal/ctxutil" + "github.com/LeanerCloud/cloud-commitments-platform/internal/ricost" ) // reshapeEC2Client is the narrow slice of the EC2 client that @@ -1599,25 +1600,11 @@ func parseThresholdParam(params map[string]string) (float64, error) { return f, nil } -// monthlyCostFromConvertibleRI computes the per-instance per-month -// effective cost from an RI's pricing fields, matching the same -// formula effectiveMonthlyCost uses for offerings: -// -// 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. -// -// 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. +// monthlyCostFromConvertibleRI delegates to ricost.MonthlyCostFromConvertibleRI, +// the single implementation of the convertible RI monthly cost formula shared +// with internal/server. 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) - } - hoursPerTerm := float64(ri.Duration) / 3600 - return ((ri.FixedPrice / hoursPerTerm) + ri.UsagePrice + ri.RecurringHourlyAmount) * 730, nil + return ricost.MonthlyCostFromConvertibleRI(ri) } // convertToExchangeTypes converts provider-specific types to the exchange package types. diff --git a/internal/ricost/monthly_cost.go b/internal/ricost/monthly_cost.go new file mode 100644 index 000000000..cdfe35648 --- /dev/null +++ b/internal/ricost/monthly_cost.go @@ -0,0 +1,40 @@ +// Package ricost holds the convertible Reserved Instance cost formula shared +// by the reshape recommendations API (internal/api) and the auto-exchange and +// ladder paths (internal/server), so the two can't drift apart. +package ricost + +import ( + "fmt" + + ec2svc "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/ec2" +) + +// HoursPerMonth is AWS's canonical hours-per-month convention for monthly +// cost figures: 8760 hours in a 365-day year divided by 12 months. It matches +// hoursPerMonth in cloud-commitments-go's providers/aws/recommendations and +// the 730-hour monthly rates used elsewhere in the platform. +const HoursPerMonth = 730.0 + +// secondsPerHour converts an RI's Duration (seconds) into hours. +const secondsPerHour = 3600 + +// MonthlyCostFromConvertibleRI computes the per-instance per-month effective +// cost from an RI's pricing fields, matching the formula the AWS +// effectiveMonthlyCost function uses for offerings: +// +// monthly = (FixedPrice / hours_per_term + UsagePrice + recurring_hourly) × HoursPerMonth +// +// 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. +// +// Used to populate exchange.RIInfo.MonthlyCost so the cross-family +// dollar-units pre-filter compares 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) + } + hoursPerTerm := float64(ri.Duration) / secondsPerHour + return ((ri.FixedPrice / hoursPerTerm) + ri.UsagePrice + ri.RecurringHourlyAmount) * HoursPerMonth, nil +} diff --git a/internal/ricost/monthly_cost_test.go b/internal/ricost/monthly_cost_test.go new file mode 100644 index 000000000..ba1bc0f44 --- /dev/null +++ b/internal/ricost/monthly_cost_test.go @@ -0,0 +1,58 @@ +package ricost + +import ( + "testing" + + ec2svc "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/ec2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestMonthlyCostFromConvertibleRI pins the exact float64 results the +// duplicated copies in internal/api and internal/server returned before they +// were merged into this package (issue #337). The comparisons are exact on +// purpose: moving the formula here must not change a single bit of any +// dollar figure. +func TestMonthlyCostFromConvertibleRI(t *testing.T) { + const oneYear, threeYears = 31536000, 94608000 + cases := []struct { + name string + ri ec2svc.ConvertibleRI + want float64 + wantErr string + }{ + {name: "1y all upfront", ri: ec2svc.ConvertibleRI{ReservedInstanceID: "ri-a", FixedPrice: 8760, Duration: oneYear}, want: 730}, + {name: "3y all upfront", ri: ec2svc.ConvertibleRI{ReservedInstanceID: "ri-b", FixedPrice: 3000, Duration: threeYears}, want: 83.33333333333333}, + {name: "1y no upfront", ri: ec2svc.ConvertibleRI{ReservedInstanceID: "ri-c", RecurringHourlyAmount: 0.05, Duration: oneYear}, want: 36.5}, + {name: "1y partial upfront", ri: ec2svc.ConvertibleRI{ReservedInstanceID: "ri-d", FixedPrice: 438, RecurringHourlyAmount: 0.025, Duration: oneYear}, want: 54.75000000000001}, + {name: "usage price only", ri: ec2svc.ConvertibleRI{ReservedInstanceID: "ri-e", UsagePrice: 0.04, Duration: oneYear}, want: 29.2}, + {name: "all components 3y", ri: ec2svc.ConvertibleRI{ReservedInstanceID: "ri-f", FixedPrice: 1234.56, UsagePrice: 0.011, RecurringHourlyAmount: 0.0207, Duration: threeYears}, want: 57.43433333333333}, + // Zero prices with a valid term currently compute to 0 without an + // error; this pins today's behavior rather than endorsing it. + {name: "zero prices", ri: ec2svc.ConvertibleRI{ReservedInstanceID: "ri-g", Duration: oneYear}, want: 0}, + {name: "one second term", ri: ec2svc.ConvertibleRI{ReservedInstanceID: "ri-h", FixedPrice: 1, Duration: 1}, want: 2628000}, + {name: "zero duration", ri: ec2svc.ConvertibleRI{ReservedInstanceID: "ri-i", FixedPrice: 100}, + wantErr: "convertible RI ri-i has non-positive duration 0: monthly cost is not computable"}, + {name: "negative duration", ri: ec2svc.ConvertibleRI{ReservedInstanceID: "ri-j", FixedPrice: 100, Duration: -oneYear}, + wantErr: "convertible RI ri-j has non-positive duration -31536000: monthly cost is not computable"}, + {name: "zero duration and zero prices", ri: ec2svc.ConvertibleRI{ReservedInstanceID: "ri-k"}, + wantErr: "convertible RI ri-k has non-positive duration 0: monthly cost is not computable"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := MonthlyCostFromConvertibleRI(tc.ri) + if tc.wantErr != "" { + require.EqualError(t, err, tc.wantErr) + assert.Zero(t, got) + return + } + require.NoError(t, err) + assert.Equal(t, tc.want, got) // exact on purpose: no numeric drift allowed + }) + } +} + +func TestHoursPerMonth(t *testing.T) { + assert.Equal(t, 365*24/12, int(HoursPerMonth)) + assert.Equal(t, 730.0, HoursPerMonth) +} diff --git a/internal/server/handler_ri_exchange.go b/internal/server/handler_ri_exchange.go index e6b2c0b8e..36470c28f 100644 --- a/internal/server/handler_ri_exchange.go +++ b/internal/server/handler_ri_exchange.go @@ -18,6 +18,7 @@ import ( ec2svc "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/ec2" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/email" + "github.com/LeanerCloud/cloud-commitments-platform/internal/ricost" ) // riExchangeClients holds the injectable dependencies for RI exchange operations. @@ -271,24 +272,11 @@ func buildExchangeNotificationData(result *exchange.AutoExchangeResult, dashboar return data } -// monthlyCostFromConvertibleRI computes the per-instance per-month -// effective cost from an RI's pricing fields, matching the same -// formula the AWS effectiveMonthlyCost function uses for offerings: -// -// 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. -// -// Used to populate exchange.RIInfo.MonthlyCost so the cross-family -// dollar-units pre-filter compares apples-to-apples against per-target -// offering costs. +// monthlyCostFromConvertibleRI delegates to ricost.MonthlyCostFromConvertibleRI, +// the single implementation of the convertible RI monthly cost formula shared +// with internal/api. 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) - } - hoursPerTerm := float64(ri.Duration) / 3600 - return ((ri.FixedPrice / hoursPerTerm) + ri.UsagePrice + ri.RecurringHourlyAmount) * 730, nil + return ricost.MonthlyCostFromConvertibleRI(ri) } // convertForAutoExchange converts provider-specific types to exchange package types,