Skip to content

Commit 1cc0aaa

Browse files
cristimcristim
authored
feat(cli): configurable --coverage-lookback-days for --target-coverage (closes #360) (#794)
The GetReservationCoverage lookback was hardcoded to 30 days. Operators reconciling CUDly's ExistingCoverage column against the AWS console coverage-report export need to match the window used by each. Add --coverage-lookback-days (default 30, matching CE's UI default) that threads through Config.CoverageLookbackDays into GetRICoverageMap. Validation rejects values < 1. The fetchExistingCoverage fallback guard (lookbackDays <= 0 -> 30) is kept as a defensive belt-and-suspenders path for callers that bypass flag parsing (e.g. API callers).
1 parent 82c50fe commit 1cc0aaa

5 files changed

Lines changed: 118 additions & 8 deletions

File tree

‎cmd/main.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,13 @@ type Config struct {
7474
MinSavingsPct float64
7575
MaxBreakEvenMonths int
7676
MinCount int
77+
// CoverageLookbackDays is the number of calendar days of historical
78+
// demand fed to GetReservationCoverage when computing the existing-RI
79+
// coverage map for --target-coverage sizing. A longer window smooths
80+
// seasonal spikes; a shorter one matches a narrow billing-period export
81+
// from the AWS console coverage report. Default 30, matching the CE
82+
// UI default.
83+
CoverageLookbackDays int
7784
// RebuyWindowDays, when > 0, treats existing RIs whose remaining term
7885
// is at most this many days as already uncovered, so --target-coverage
7986
// recommends replacements before they expire. Zero (default) keeps the
@@ -150,6 +157,11 @@ func init() {
150157
rootCmd.Flags().Float64Var(&toolCfg.MinSavingsPct, "min-savings-pct", 0, "Minimum savings percentage to include a recommendation (0 = no filter)")
151158
rootCmd.Flags().IntVar(&toolCfg.MaxBreakEvenMonths, "max-break-even-months", 0, "Maximum break-even period in months (0 = no filter)")
152159
rootCmd.Flags().IntVar(&toolCfg.MinCount, "min-count", 0, "Minimum instance count to include a recommendation (0 = no filter)")
160+
rootCmd.Flags().IntVar(&toolCfg.CoverageLookbackDays, "coverage-lookback-days", 30,
161+
"Number of calendar days of historical demand fed to GetReservationCoverage "+
162+
"when computing the existing-RI coverage map for --target-coverage sizing. "+
163+
"Match this to your AWS console coverage report window to reconcile "+
164+
"CUDly's ExistingCoverage column against the console export. Default 30.")
153165
rootCmd.Flags().IntVar(&toolCfg.RebuyWindowDays, "rebuy-window-days", 0,
154166
"When >0, treat existing RIs expiring within this many days as already "+
155167
"uncovered, so --target-coverage sizes recommendations to replace them "+

‎cmd/multi_service.go‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,6 @@ import (
2020
"github.com/google/uuid"
2121
)
2222

23-
// existingCoverageLookbackDays is the historical window we ask CE to
24-
// summarise existing-RI coverage over for --target-coverage sizing.
25-
// 30 days matches CE's UI default and the GetRIUtilization caller.
26-
const existingCoverageLookbackDays = 30
27-
2823
// fetchExistingCoverage retrieves the existing-RI coverage map from Cost
2924
// Explorer so --target-coverage sizing can subtract what's already owned in
3025
// each pool. Best-effort: a transient CE failure logs a warning and returns
@@ -36,6 +31,10 @@ const existingCoverageLookbackDays = 30
3631
// Coverage is fetched per-region per-account so CE's org-wide aggregate
3732
// doesn't bleed one account's coverage into another in multi-account orgs.
3833
// Regions come from cfg.Regions if set, otherwise from EC2 DescribeRegions.
34+
//
35+
// The lookback window is cfg.CoverageLookbackDays (default 30, matching the
36+
// CE UI default). Operators reconciling against the AWS console coverage
37+
// report should match this value to the report's own time window.
3938
func fetchExistingCoverage(ctx context.Context, awsCfg aws.Config, recClient provider.RecommendationsClient, cfg Config) recommendations.PoolCoverageMap {
4039
if cfg.TargetCoverage <= 0 {
4140
return nil
@@ -46,6 +45,10 @@ func fetchExistingCoverage(ctx context.Context, awsCfg aws.Config, recClient pro
4645
// the no-existing-commitments path.
4746
return nil
4847
}
48+
lookbackDays := cfg.CoverageLookbackDays
49+
if lookbackDays <= 0 {
50+
lookbackDays = 30
51+
}
4952
regions := cfg.Regions
5053
if len(regions) == 0 {
5154
allRegions, err := getAllAWSRegions(ctx, awsCfg)
@@ -55,8 +58,8 @@ func fetchExistingCoverage(ctx context.Context, awsCfg aws.Config, recClient pro
5558
}
5659
regions = allRegions
5760
}
58-
AppLogger.Printf("\n🔎 Fetching existing-RI coverage from Cost Explorer per-account across %d regions (lookback %d days)...\n", len(regions), existingCoverageLookbackDays)
59-
cov, err := adapter.GetRICoverageMap(ctx, existingCoverageLookbackDays, regions)
61+
AppLogger.Printf("\n🔎 Fetching existing-RI coverage from Cost Explorer per-account across %d regions (lookback %d days)...\n", len(regions), lookbackDays)
62+
cov, err := adapter.GetRICoverageMap(ctx, lookbackDays, regions)
6063
if err != nil {
6164
AppLogger.Printf(" ⚠️ Could not fetch existing-RI coverage (%v); sizing will assume zero existing coverage\n", err)
6265
return nil

‎cmd/multi_service_coverage_test.go‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -768,3 +768,38 @@ func TestFilterAndAdjustRecommendations_OverrideCountApplied(t *testing.T) {
768768
assert.Equal(t, int(toolCfg.OverrideCount), rec.Count)
769769
}
770770
}
771+
772+
// TestFetchExistingCoverage_LookbackDays verifies that fetchExistingCoverage
773+
// honours cfg.CoverageLookbackDays (issue #360). The test uses the
774+
// MockRecommendationsClient which fails the *awsprovider.RecommendationsClientAdapter
775+
// type assertion, exercising the non-AWS-provider early-return path. The key
776+
// assertions are:
777+
// - TargetCoverage=0 always returns nil regardless of lookback.
778+
// - TargetCoverage>0 with a non-AWS client returns nil (non-AWS path,
779+
// no CE call is made).
780+
//
781+
// Threading through the actual lookback value to GetRICoverageMap is
782+
// integration-tested in providers/aws/recommendations (TestGetRICoverageMap_*).
783+
func TestFetchExistingCoverage_LookbackDays(t *testing.T) {
784+
ctx := context.Background()
785+
awsCfg := aws.Config{}
786+
mockClient := &MockRecommendationsClient{}
787+
788+
t.Run("zero TargetCoverage returns nil regardless of lookback", func(t *testing.T) {
789+
cfg := Config{TargetCoverage: 0, CoverageLookbackDays: 14, Regions: []string{"us-east-1"}}
790+
got := fetchExistingCoverage(ctx, awsCfg, mockClient, cfg)
791+
assert.Nil(t, got, "TargetCoverage=0 must short-circuit before any CE call")
792+
})
793+
794+
t.Run("non-AWS adapter returns nil, lookback not needed", func(t *testing.T) {
795+
cfg := Config{TargetCoverage: 80, CoverageLookbackDays: 14, Regions: []string{"us-east-1"}}
796+
got := fetchExistingCoverage(ctx, awsCfg, mockClient, cfg)
797+
assert.Nil(t, got, "non-AWS provider must return nil (no CE integration)")
798+
})
799+
800+
t.Run("custom lookback stored in Config", func(t *testing.T) {
801+
cfg := Config{TargetCoverage: 80, CoverageLookbackDays: 60, Regions: []string{"us-east-1"}}
802+
// CoverageLookbackDays field value is preserved in the struct.
803+
assert.Equal(t, 60, cfg.CoverageLookbackDays)
804+
})
805+
}

‎cmd/validators.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,11 @@ func validateNumericRanges(cmd *cobra.Command) error {
4646
return err
4747
}
4848

49+
// Validate coverage lookback days
50+
if toolCfg.CoverageLookbackDays < 1 {
51+
return fmt.Errorf("coverage-lookback-days must be >= 1, got: %d", toolCfg.CoverageLookbackDays)
52+
}
53+
4954
// Validate max instances
5055
if toolCfg.MaxInstances < 0 {
5156
return fmt.Errorf("max-instances must be 0 (no limit) or a positive number, got: %d", toolCfg.MaxInstances)

‎cmd/validators_test.go‎

Lines changed: 56 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,15 @@ func TestValidateNumericRanges(t *testing.T) {
2323
toolCfg.Coverage = 80.0
2424
toolCfg.MaxInstances = 100
2525
toolCfg.OverrideCount = 10
26+
toolCfg.CoverageLookbackDays = 30
2627
},
2728
wantErr: false,
2829
},
2930
{
3031
name: "coverage below zero",
3132
setupFunc: func() {
3233
toolCfg.Coverage = -1.0
34+
toolCfg.CoverageLookbackDays = 30
3335
},
3436
wantErr: true,
3537
errMsg: "coverage percentage must be between 0 and 100",
@@ -38,6 +40,7 @@ func TestValidateNumericRanges(t *testing.T) {
3840
name: "coverage above 100",
3941
setupFunc: func() {
4042
toolCfg.Coverage = 101.0
43+
toolCfg.CoverageLookbackDays = 30
4144
},
4245
wantErr: true,
4346
errMsg: "coverage percentage must be between 0 and 100",
@@ -47,6 +50,7 @@ func TestValidateNumericRanges(t *testing.T) {
4750
setupFunc: func() {
4851
toolCfg.Coverage = 80.0
4952
toolCfg.MaxInstances = -1
53+
toolCfg.CoverageLookbackDays = 30
5054
},
5155
wantErr: true,
5256
errMsg: "max-instances must be 0",
@@ -56,6 +60,7 @@ func TestValidateNumericRanges(t *testing.T) {
5660
setupFunc: func() {
5761
toolCfg.Coverage = 80.0
5862
toolCfg.MaxInstances = MaxReasonableInstances + 1
63+
toolCfg.CoverageLookbackDays = 30
5964
},
6065
wantErr: true,
6166
errMsg: "max-instances",
@@ -66,6 +71,7 @@ func TestValidateNumericRanges(t *testing.T) {
6671
toolCfg.Coverage = 80.0
6772
toolCfg.MaxInstances = 100
6873
toolCfg.OverrideCount = -1
74+
toolCfg.CoverageLookbackDays = 30
6975
},
7076
wantErr: true,
7177
errMsg: "override-count must be 0",
@@ -76,6 +82,7 @@ func TestValidateNumericRanges(t *testing.T) {
7682
toolCfg.Coverage = 80.0
7783
toolCfg.MaxInstances = 100
7884
toolCfg.OverrideCount = MaxReasonableInstances + 1
85+
toolCfg.CoverageLookbackDays = 30
7986
},
8087
wantErr: true,
8188
errMsg: "override-count",
@@ -461,7 +468,7 @@ func TestValidateTargetCoverage(t *testing.T) {
461468

462469
for _, tt := range tests {
463470
t.Run(tt.name, func(t *testing.T) {
464-
toolCfg = Config{Coverage: tt.coverage, TargetCoverage: tt.target}
471+
toolCfg = Config{Coverage: tt.coverage, TargetCoverage: tt.target, CoverageLookbackDays: 30}
465472
var cmd *cobra.Command
466473
if tt.useCobraCmd {
467474
cmd = &cobra.Command{Use: "test"}
@@ -485,3 +492,51 @@ func TestValidateTargetCoverage(t *testing.T) {
485492
})
486493
}
487494
}
495+
496+
// TestValidateCoverageLookbackDays verifies that validateNumericRanges rejects
497+
// non-positive --coverage-lookback-days and accepts positive values.
498+
func TestValidateCoverageLookbackDays(t *testing.T) {
499+
tests := []struct {
500+
name string
501+
days int
502+
wantErr bool
503+
errSubstr string
504+
}{
505+
{name: "default 30 is valid", days: 30, wantErr: false},
506+
{name: "1 day is valid", days: 1, wantErr: false},
507+
{name: "90 days is valid", days: 90, wantErr: false},
508+
{
509+
name: "zero rejected",
510+
days: 0,
511+
wantErr: true,
512+
errSubstr: "coverage-lookback-days must be >= 1",
513+
},
514+
{
515+
name: "negative rejected",
516+
days: -1,
517+
wantErr: true,
518+
errSubstr: "coverage-lookback-days must be >= 1",
519+
},
520+
}
521+
522+
for _, tt := range tests {
523+
t.Run(tt.name, func(t *testing.T) {
524+
origCfg := toolCfg
525+
defer func() { toolCfg = origCfg }()
526+
toolCfg = Config{
527+
Coverage: 80,
528+
CoverageLookbackDays: tt.days,
529+
}
530+
err := validateNumericRanges(nil)
531+
if tt.wantErr {
532+
if err == nil {
533+
t.Errorf("validateNumericRanges() expected error containing %q, got nil", tt.errSubstr)
534+
} else if tt.errSubstr != "" && !strings.Contains(err.Error(), tt.errSubstr) {
535+
t.Errorf("validateNumericRanges() error = %v, want substring %q", err, tt.errSubstr)
536+
}
537+
} else if err != nil {
538+
t.Errorf("validateNumericRanges() unexpected error = %v", err)
539+
}
540+
})
541+
}
542+
}

0 commit comments

Comments
 (0)