From be5c4afb361e7595a94a946eef2b639c382796fb Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 12 Oct 2026 21:41:51 +0200 Subject: [PATCH] fix(aws/ladder): scope EC2 coverage report to EC2 pools computeEC2CoveragePct blended ElastiCache, OpenSearch, Redshift and MemoryDB pools (same region:instance_type key shape) into the reported EC2 RI coverage. Add Client.GetEC2RICoverageMap (one EC2-filtered CE call per region) and use it for the ladder. GetRICoverageMap is unchanged for its multi-service callers. Closes #37 Co-Authored-By: Claude Sonnet 5.5 --- providers/aws/ladder/factory.go | 2 +- providers/aws/ladder/interfaces.go | 7 +- providers/aws/ladder/ladder_test.go | 2 +- providers/aws/ladder/layer_states.go | 9 +-- providers/aws/recommendations/coverage.go | 49 +++++++++---- .../aws/recommendations/coverage_test.go | 70 +++++++++++++++++++ providers/aws/service_client.go | 6 ++ 7 files changed, 123 insertions(+), 22 deletions(-) diff --git a/providers/aws/ladder/factory.go b/providers/aws/ladder/factory.go index 86115bdd..e132409c 100644 --- a/providers/aws/ladder/factory.go +++ b/providers/aws/ladder/factory.go @@ -28,7 +28,7 @@ import ( // // - riLister : ec2svc.Client.ListConvertibleReservedInstances // - spLister : spListerAdapter wrapping *sdksp.Client.DescribeSavingsPlans -// - riCoverageSource : recommendations.Client.GetRICoverageMap +// - riCoverageSource : recommendations.Client.GetEC2RICoverageMap // - onDemandSeries : onDemandSeriesAdapter wrapping recommendations.Client.GetOnDemandSeries // (CE GetCostAndUsage; maps recommendations.DailyCost to DailyPoint) // - utilizationSource : recommendations.Client.GetRIUtilization diff --git a/providers/aws/ladder/interfaces.go b/providers/aws/ladder/interfaces.go index f494e365..329e8c76 100644 --- a/providers/aws/ladder/interfaces.go +++ b/providers/aws/ladder/interfaces.go @@ -68,13 +68,14 @@ type spLister interface { } // riCoverageSource is the narrow interface for RI coverage data, consumed by -// GetLayerStates. GetRICoverageMap returns the per-pool org-wide RI coverage -// map (keyed by "region:instance_type" for EC2) for the given lookback window +// GetLayerStates. GetEC2RICoverageMap returns the per-pool org-wide EC2 RI +// coverage map (keyed by "region:instance_type", EC2 pools only) for the +// given lookback window // and regions. Kept single-method (interface segregation) so implementations // that only provide coverage need not stub the on-demand series and vice versa; // one concrete adapter may still implement both. type riCoverageSource interface { - GetRICoverageMap(ctx context.Context, lookbackDays int, regions []string) (recommendations.PoolCoverageMap, error) + GetEC2RICoverageMap(ctx context.Context, lookbackDays int, regions []string) (recommendations.PoolCoverageMap, error) } // DailyPoint is a single calendar-day entry in the on-demand cost series. diff --git a/providers/aws/ladder/ladder_test.go b/providers/aws/ladder/ladder_test.go index 7686f8db..d3cc09b7 100644 --- a/providers/aws/ladder/ladder_test.go +++ b/providers/aws/ladder/ladder_test.go @@ -52,7 +52,7 @@ type fakeCoverageSource struct { onDemandPoints []DailyPoint } -func (f *fakeCoverageSource) GetRICoverageMap(_ context.Context, _ int, _ []string) (recommendations.PoolCoverageMap, error) { +func (f *fakeCoverageSource) GetEC2RICoverageMap(_ context.Context, _ int, _ []string) (recommendations.PoolCoverageMap, error) { return f.coverageMap, f.coverageErr } diff --git a/providers/aws/ladder/layer_states.go b/providers/aws/ladder/layer_states.go index 58bf69e3..526c8ea4 100644 --- a/providers/aws/ladder/layer_states.go +++ b/providers/aws/ladder/layer_states.go @@ -49,7 +49,7 @@ func (a *AWSLadder) GetLayerStates(ctx context.Context, scope ladder.Scope) (map return nil, fmt.Errorf("GetLayerStates: SP listing failed: %w", err) } - coverageMap, covErr := a.riCoverage.GetRICoverageMap(ctx, a.cfg.lookbackDays(), []string{a.cfg.Region}) + coverageMap, covErr := a.riCoverage.GetEC2RICoverageMap(ctx, a.cfg.lookbackDays(), []string{a.cfg.Region}) // covErr is checked per-layer below; a coverage failure does not fail the // whole snapshot — it degrades CoveragePct to nil. @@ -89,7 +89,7 @@ func (a *AWSLadder) riLayerState( // CoveragePct stays nil (unmeasured). Log so a persistently failing CE // call is visible — silent degradation would quietly disable reshape // triggering downstream. - log.Printf("WARNING: AWSLadder GetLayerStates: RI coverage degraded to nil (layer=%s, source=GetRICoverageMap, region=%s): %v", + log.Printf("WARNING: AWSLadder GetLayerStates: RI coverage degraded to nil (layer=%s, source=GetEC2RICoverageMap, region=%s): %v", ladder.LayerConvertibleRI, a.cfg.Region, covErr) case len(coverageMap) > 0: state.CoveragePct = computeEC2CoveragePct(coverageMap, a.cfg.Region) @@ -267,8 +267,9 @@ func sumExpiringSPHourlyCost(sps []ActiveSP, planType string, horizon time.Time) } // computeEC2CoveragePct derives an aggregate EC2 RI coverage percentage from -// the PoolCoverageMap for the given region. Only EC2 pools (keys matching -// "region:*" with a non-zero AvgInstancesPerHour or Pct) are considered. +// the PoolCoverageMap for the given region. The map must come from +// GetEC2RICoverageMap: other services share the "region:instance_type" key +// shape and cannot be told apart from EC2 by key. // // Weighting: when any pool has a non-zero AvgInstancesPerHour, coverage is // a weighted average (weight = AvgInstancesPerHour). When all pools have diff --git a/providers/aws/recommendations/coverage.go b/providers/aws/recommendations/coverage.go index 2fc2af53..654e3def 100644 --- a/providers/aws/recommendations/coverage.go +++ b/providers/aws/recommendations/coverage.go @@ -27,13 +27,16 @@ import ( // across engines via the DATABASE_ENGINE filter and pull DEPLOYMENT_OPTION // into the GroupBy alongside INSTANCE_TYPE. var coverageServiceFilters = []string{ - "Amazon Elastic Compute Cloud - Compute", // EC2 - "Amazon ElastiCache", // ElastiCache - "Amazon OpenSearch Service", // OpenSearch - "Amazon Redshift", // Redshift - "Amazon MemoryDB", // MemoryDB + ec2CoverageService, // EC2 + "Amazon ElastiCache", // ElastiCache + "Amazon OpenSearch Service", // OpenSearch + "Amazon Redshift", // Redshift + "Amazon MemoryDB", // MemoryDB } +// ec2CoverageService is CE's SERVICE dimension value for EC2. +const ec2CoverageService = "Amazon Elastic Compute Cloud - Compute" + // rdsCoverageEngines enumerates CE's DATABASE_ENGINE dimension values for // RDS recommendations. CUDly's parser normalises engine names to lowercase // shorthand (e.g. "aurora-postgresql") for rec.Details.Engine; CE expects @@ -177,14 +180,7 @@ func normaliseDeployment(deployment string) string { // --target-coverage sizing drops (target-coverage-unknown) rather than // treating them as 0% covered. func (c *Client) GetRICoverageMap(ctx context.Context, lookbackDays int, regions []string) (PoolCoverageMap, error) { - if lookbackDays <= 0 { - lookbackDays = 30 - } - end := time.Now().UTC() - start := end.AddDate(0, 0, -lookbackDays) - startStr := start.Format("2006-01-02") - endStr := end.Format("2006-01-02") - windowHours := float64(lookbackDays * 24) + startStr, endStr, windowHours := coverageWindow(lookbackDays) out := make(PoolCoverageMap) // Cost: one CE call per (service, region) for non-RDS; one CE call per @@ -206,6 +202,33 @@ func (c *Client) GetRICoverageMap(ctx context.Context, lookbackDays int, regions return out, nil } +// GetEC2RICoverageMap is GetRICoverageMap restricted to EC2: one CE call per +// region. Non-EC2 services (ElastiCache, OpenSearch, Redshift, MemoryDB) +// share the "region:instance_type" key shape, so a caller that must report +// EC2-only coverage cannot recover it from the all-services map by key. +func (c *Client) GetEC2RICoverageMap(ctx context.Context, lookbackDays int, regions []string) (PoolCoverageMap, error) { + startStr, endStr, windowHours := coverageWindow(lookbackDays) + + out := make(PoolCoverageMap) + for _, region := range regions { + if err := c.fetchCoverageForServiceRegion(ctx, startStr, endStr, windowHours, ec2CoverageService, region, out); err != nil { + return nil, fmt.Errorf("fetching EC2 coverage for region %q: %w", region, err) + } + } + return out, nil +} + +// coverageWindow resolves the CE date range and window length in hours for +// lookbackDays, defaulting a non-positive value to 30. +func coverageWindow(lookbackDays int) (startStr, endStr string, windowHours float64) { + if lookbackDays <= 0 { + lookbackDays = 30 + } + end := time.Now().UTC() + start := end.AddDate(0, 0, -lookbackDays) + return start.Format("2006-01-02"), end.Format("2006-01-02"), float64(lookbackDays * 24) +} + // fetchCoverageForRDSEngineRegion fetches coverage for one (RDS engine, // region) tuple. Writes entries keyed by // "region:instance_type:engine:deployment" so the apply helper looks up diff --git a/providers/aws/recommendations/coverage_test.go b/providers/aws/recommendations/coverage_test.go index 1b222d77..9ad2968d 100644 --- a/providers/aws/recommendations/coverage_test.go +++ b/providers/aws/recommendations/coverage_test.go @@ -492,3 +492,73 @@ func TestApplyCoverageMap_UnmatchedPool_DroppedByTargetCoverage(t *testing.T) { assert.Empty(t, got) assert.Equal(t, "Dropped 1 recs: target-coverage-unknown=1", drops.FormatOneLine()) } + +// serviceRoutedCoverageCE answers GetReservationCoverage per SERVICE filter, +// recording every call, so tests can prove which services were queried. +type serviceRoutedCoverageCE struct { + mockCostExplorerAPI + byService map[string]*costexplorer.GetReservationCoverageOutput + inputs []*costexplorer.GetReservationCoverageInput +} + +func (m *serviceRoutedCoverageCE) GetReservationCoverage(_ context.Context, params *costexplorer.GetReservationCoverageInput, _ ...func(*costexplorer.Options)) (*costexplorer.GetReservationCoverageOutput, error) { + m.inputs = append(m.inputs, params) + for _, e := range params.Filter.And { + if e.Dimensions != nil && e.Dimensions.Key == types.DimensionService { + if out, ok := m.byService[e.Dimensions.Values[0]]; ok { + return out, nil + } + } + } + return &costexplorer.GetReservationCoverageOutput{}, nil +} + +func coverageOutputFor(instanceType, pct string) *costexplorer.GetReservationCoverageOutput { + return &costexplorer.GetReservationCoverageOutput{CoveragesByTime: []types.CoverageByTime{{ + Groups: []types.ReservationCoverageGroup{{ + Attributes: map[string]string{"instanceType": instanceType}, + Coverage: &types.Coverage{CoverageHours: &types.CoverageHours{ + CoverageHoursPercentage: aws.String(pct), + TotalRunningHours: aws.String("7200"), + }}, + }}, + }}} +} + +// TestGetEC2RICoverageMap_QueriesOnlyEC2 is the go#37 regression: the +// all-services map blends ElastiCache/Redshift/OpenSearch/MemoryDB pools +// (same one-colon key shape) into what the ladder reports as EC2 coverage. +// The EC2 fetch must issue exactly one EC2-filtered CE call per region and +// return only EC2 pools. +func TestGetEC2RICoverageMap_QueriesOnlyEC2(t *testing.T) { + mock := &serviceRoutedCoverageCE{byService: map[string]*costexplorer.GetReservationCoverageOutput{ + ec2CoverageService: coverageOutputFor("m5.large", "80.0"), + "Amazon ElastiCache": coverageOutputFor("cache.m5.large", "0.0"), + "Amazon Redshift": coverageOutputFor("dc2.large", "0.0"), + "Amazon OpenSearch Service": coverageOutputFor("m5.large.search", "0.0"), + "Amazon MemoryDB": coverageOutputFor("db.r6g.large", "0.0"), + }} + client := NewClientWithAPI(mock, "us-east-1") + + got, err := client.GetEC2RICoverageMap(context.Background(), 30, []string{"us-east-1"}) + require.NoError(t, err) + + require.Len(t, mock.inputs, 1, "one CE call per region, EC2 only") + assert.Equal(t, []string{ec2CoverageService}, mock.inputs[0].Filter.And[0].Dimensions.Values) + assert.Equal(t, PoolCoverageMap{poolKey("us-east-1", "m5.large"): {Pct: 80, AvgInstancesPerHour: 10}}, got) + + // Control: the all-services fetch still returns the other services' pools + // for its multi-service callers (target-coverage sizing). + all, err := client.GetRICoverageMap(context.Background(), 30, []string{"us-east-1"}) + require.NoError(t, err) + assert.Contains(t, all, poolKey("us-east-1", "cache.m5.large")) + assert.Contains(t, all, poolKey("us-east-1", "dc2.large")) +} + +func TestGetEC2RICoverageMap_ErrorPropagates(t *testing.T) { + mock := &mockCoverageCE{coverageError: assert.AnError} + client := NewClientWithAPI(mock, "us-east-1") + got, err := client.GetEC2RICoverageMap(context.Background(), 30, []string{"us-east-1"}) + require.Error(t, err) + assert.Nil(t, got) +} diff --git a/providers/aws/service_client.go b/providers/aws/service_client.go index a93cd602..616f29a2 100644 --- a/providers/aws/service_client.go +++ b/providers/aws/service_client.go @@ -308,6 +308,12 @@ func (r *RecommendationsClientAdapter) GetRICoverageMap(ctx context.Context, loo return r.client.GetRICoverageMap(ctx, lookbackDays, regions) } +// GetEC2RICoverageMap is GetRICoverageMap restricted to EC2 pools (one CE +// call per region); used by the ladder's EC2 coverage report. +func (r *RecommendationsClientAdapter) GetEC2RICoverageMap(ctx context.Context, lookbackDays int, regions []string) (recommendations.PoolCoverageMap, error) { + return r.client.GetEC2RICoverageMap(ctx, lookbackDays, regions) +} + // SetRecLookbackPeriod configures the LookbackPeriodInDays forwarded to // GetReservationPurchaseRecommendation. Valid values: "7d", "30d", "60d". func (r *RecommendationsClientAdapter) SetRecLookbackPeriod(period string) {