Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 30 additions & 14 deletions providers/aws/ladder/adapters.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,15 @@ type activeSPListAPI interface {
DescribeSavingsPlans(ctx context.Context, params *sdksp.DescribeSavingsPlansInput, optFns ...func(*sdksp.Options)) (*sdksp.DescribeSavingsPlansOutput, error)
}

// spListerStates is the set of Savings Plan states counted as existing
// commitment.
// spListerStates is the single source of the Savings Plan states counted as
// existing commitment; read it only through ExistingSPStates.
var spListerStates = []sptypes.SavingsPlanState{
sptypes.SavingsPlanStateActive,
sptypes.SavingsPlanStatePaymentPending,
}

// ExistingSPStates returns the Savings Plan states counted as existing
// commitment. The slice is a fresh copy, so callers cannot mutate the set.
// - active: currently billing.
// - payment-pending: purchase accepted, first payment in flight. A
// just-purchased SP sits in this state briefly; it MUST count as existing
Expand All @@ -36,14 +43,26 @@ type activeSPListAPI interface {
// started, so counting them would suppress purchases the queued SP will
// not cover until its start date. Revisit with L14 (expiry alignment),
// which can reason about future-dated coverage explicitly.
var spListerStates = []sptypes.SavingsPlanState{
sptypes.SavingsPlanStateActive,
sptypes.SavingsPlanStatePaymentPending,
func ExistingSPStates() []sptypes.SavingsPlanState {
return append([]sptypes.SavingsPlanState(nil), spListerStates...)
}

// CountsAsExistingSP applies the region rule to a Savings Plan already in an
// ExistingSPStates state: EC2Instance SPs bound to another region cover that
// region's usage, not the ladder's, and counting them would inflate
// ExistingUSDPerHour and under-purchase. Compute SPs are global and always
// count. Any other plan type is not filtered here (the ladder's own type
// check handles it). The comparison is exact, case-sensitive. An empty region
// is not special-cased: an EC2Instance plan counts only when
// spRegion == ladderRegion, so two empty regions count. Callers must validate
// both regions first; the adapter rejects an empty SP region in mapActiveSP.
func CountsAsExistingSP(planType sptypes.SavingsPlanType, spRegion, ladderRegion string) bool {
return planType != sptypes.SavingsPlanTypeEc2Instance || spRegion == ladderRegion
}

// spListerAdapter implements the spLister interface using the AWS Savings
// Plans SDK directly. It filters to spListerStates (active + payment-pending;
// see that var for rationale) and scopes EC2Instance SPs to the ladder's
// Plans SDK directly. It filters to ExistingSPStates (active + payment-pending;
// see that func for rationale) and scopes EC2Instance SPs to the ladder's
// region. Commitment strings are parsed with strconv.ParseFloat; parse errors
// fail loud per feedback_strict_int_parse and feedback_no_silent_fallbacks.
type spListerAdapter struct {
Expand All @@ -55,7 +74,7 @@ type spListerAdapter struct {
region string
}

// ListActiveSPs lists the account's Savings Plans in spListerStates and maps
// ListActiveSPs lists the account's Savings Plans in ExistingSPStates and maps
// them to the AWSLadder view, region-scoped:
//
// - EC2Instance SPs are region-bound; entries whose Region differs from the
Expand Down Expand Up @@ -87,7 +106,7 @@ func (a *spListerAdapter) ListActiveSPs(ctx context.Context) ([]ActiveSP, error)
page++

input := &sdksp.DescribeSavingsPlansInput{
States: spListerStates,
States: ExistingSPStates(),
NextToken: nextToken,
MaxResults: aws.Int32(100),
}
Expand Down Expand Up @@ -115,18 +134,15 @@ func (a *spListerAdapter) ListActiveSPs(ctx context.Context) ([]ActiveSP, error)
}

// appendRegionScopedSPs maps one DescribeSavingsPlans page onto the
// accumulated slice, applying the region-scoping rule: EC2Instance SPs bound
// to another region cover that region's usage, not ours; including them
// would inflate ExistingUSDPerHour and under-purchase. Compute SPs are
// global and always kept (see ListActiveSPs doc). Extracted to keep
// accumulated slice, applying CountsAsExistingSP. Extracted to keep
// ListActiveSPs under the cyclomatic complexity limit.
func appendRegionScopedSPs(sps []ActiveSP, page []sptypes.SavingsPlan, region string) ([]ActiveSP, error) {
for i := range page {
entry, err := mapActiveSP(page[i])
if err != nil {
return nil, err
}
if entry.PlanType == spPlanTypeEC2Instance && entry.Region != region {
if !CountsAsExistingSP(sptypes.SavingsPlanType(entry.PlanType), entry.Region, region) {
continue
}
sps = append(sps, entry)
Expand Down
39 changes: 39 additions & 0 deletions providers/aws/ladder/adapters_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -622,3 +622,42 @@ func TestGetLayerStates_RealSPLister_NonZeroExisting(t *testing.T) {
assert.InDelta(t, 2.0, *computeSP.ExistingUSDPerHour, 1e-9,
"ExistingUSDPerHour must equal the SP $2/hr commitment (was 0.0 pre-fix)")
}

func TestExistingSPStates(t *testing.T) {
got := ExistingSPStates()
assert.Equal(t, []sptypes.SavingsPlanState{
sptypes.SavingsPlanStateActive,
sptypes.SavingsPlanStatePaymentPending,
}, got)

got[0] = sptypes.SavingsPlanStateRetired
assert.Equal(t, sptypes.SavingsPlanStateActive, ExistingSPStates()[0], "mutating a returned copy must not affect the package set")
}

func TestCountsAsExistingSP(t *testing.T) {
tests := []struct {
name string
planType sptypes.SavingsPlanType
spRegion string
want bool
}{
{"EC2Instance same region", sptypes.SavingsPlanTypeEc2Instance, "us-east-1", true},
{"EC2Instance other region", sptypes.SavingsPlanTypeEc2Instance, "eu-west-1", false},
{"EC2Instance empty region", sptypes.SavingsPlanTypeEc2Instance, "", false},
{"EC2Instance region case differs", sptypes.SavingsPlanTypeEc2Instance, "US-EAST-1", false},
{"Compute same region", sptypes.SavingsPlanTypeCompute, "us-east-1", true},
{"Compute other region", sptypes.SavingsPlanTypeCompute, "eu-west-1", true},
{"Compute empty region", sptypes.SavingsPlanTypeCompute, "", true},
{"SageMaker other region", sptypes.SavingsPlanTypeSagemaker, "eu-west-1", true},
{"unknown type other region", sptypes.SavingsPlanType("Other"), "eu-west-1", true},
{"plan type case differs", sptypes.SavingsPlanType("ec2instance"), "eu-west-1", true},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.want, CountsAsExistingSP(tt.planType, tt.spRegion, "us-east-1"))
})
}
t.Run("EC2Instance both regions empty", func(t *testing.T) {
assert.True(t, CountsAsExistingSP(sptypes.SavingsPlanTypeEc2Instance, "", ""))
})
}
Loading