diff --git a/cmd/main.go b/cmd/main.go index f6c4c9cc7..118141e61 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -11,6 +11,7 @@ import ( "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/provider" _ "github.com/LeanerCloud/CUDly/providers/aws" + "github.com/LeanerCloud/CUDly/providers/aws/recommendations" "github.com/LeanerCloud/CUDly/providers/aws/services/ec2" "github.com/LeanerCloud/CUDly/providers/aws/services/elasticache" "github.com/LeanerCloud/CUDly/providers/aws/services/memorydb" @@ -69,6 +70,12 @@ type Config struct { ActualPurchase bool DryRun bool SkipConfirmation bool + // RecLookbackPeriod controls the LookbackPeriodInDays passed to + // GetReservationPurchaseRecommendation. Valid values: "7d", "30d", "60d" + // (recommendations.DefaultRecLookbackPeriod is the shared default). + // A longer window smooths seasonal spikes; a shorter window weights + // recent demand more heavily. + RecLookbackPeriod string } func main() { @@ -146,6 +153,10 @@ func init() { "below this threshold. Useful with --target-coverage to skip tiny pools "+ "that integer arithmetic forces above target (e.g. avg=1 cannot hit 80%%). "+ "Default 0 = no filter.") + rootCmd.Flags().StringVar(&toolCfg.RecLookbackPeriod, "rec-lookback-period", recommendations.DefaultRecLookbackPeriod, + "Historical window for GetReservationPurchaseRecommendation. "+ + "Valid values: 7d, 30d, 60d. A longer window smooths seasonal spikes; "+ + "a shorter window weights recent demand more heavily. Default 7d.") } // Package-level Config that cobra flags bind to. diff --git a/cmd/multi_service.go b/cmd/multi_service.go index eb3939cbd..9c4cf5e94 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -117,6 +117,9 @@ func runToolMultiService(ctx context.Context, cfg Config) { accountCache := NewAccountAliasCache(awsCfg) recClient := awsprovider.NewRecommendationsClient(awsCfg) + if adapter, ok := recClient.(*awsprovider.RecommendationsClientAdapter); ok && cfg.RecLookbackPeriod != "" { + adapter.SetRecLookbackPeriod(cfg.RecLookbackPeriod) + } engineData := fetchEngineVersionData(ctx, cfg) // Fetch existing-RI coverage so --target-coverage can subtract what @@ -137,7 +140,14 @@ func runToolMultiService(ctx context.Context, cfg Config) { return } - // Phase 3: confirm (skipped in dry-run). + // Phases 3-4: confirm, purchase, and produce summary outputs. + runPurchaseAndReport(ctx, awsCfg, scoredResult, isDryRun, cfg) +} + +// runPurchaseAndReport handles the confirm, execute, and report phases of +// the multi-service pipeline. It is a separate function to keep +// runToolMultiService within the cyclomatic-complexity limit. +func runPurchaseAndReport(ctx context.Context, awsCfg aws.Config, scoredResult scorer.ScoredResult, isDryRun bool, cfg Config) { runID := uuid.New().String() if !isDryRun { totalInstances, totalSavings := sumPassedRecs(scoredResult.Passed) @@ -147,10 +157,8 @@ func runToolMultiService(ctx context.Context, cfg Config) { } } - // Phase 4: purchase each recommendation and write audit records. allResults := executePurchasePipeline(ctx, awsCfg, scoredResult.Passed, isDryRun, runID, cfg) - // Produce summary outputs. serviceStats := buildServiceStats(scoredResult.Passed, allResults) finalCSVOutput := generateCSVFilename(isDryRun, cfg) if err := writeMultiServiceCSVReport(allResults, finalCSVOutput); err != nil { diff --git a/cmd/multi_service_coverage_test.go b/cmd/multi_service_coverage_test.go index 833c93d24..2fa4a676c 100644 --- a/cmd/multi_service_coverage_test.go +++ b/cmd/multi_service_coverage_test.go @@ -797,9 +797,11 @@ func TestFetchExistingCoverage_LookbackDays(t *testing.T) { assert.Nil(t, got, "non-AWS provider must return nil (no CE integration)") }) - t.Run("custom lookback stored in Config", func(t *testing.T) { - cfg := Config{TargetCoverage: 80, CoverageLookbackDays: 60, Regions: []string{"us-east-1"}} - // CoverageLookbackDays field value is preserved in the struct. - assert.Equal(t, 60, cfg.CoverageLookbackDays) - }) + // The previous "custom lookback stored in Config" subcase asserted only that + // a struct field equals what was just assigned -- a tautology that passes even + // if CoverageLookbackDays is never forwarded to GetRICoverageMap. The real + // assertion -- that the lookback value reaches the CE TimePeriod -- is covered + // by TestGetRICoverageMap_LookbackWindowWidth in + // providers/aws/recommendations/coverage_test.go, which directly verifies + // end-start == lookbackDays on the actual CE input. No redundant subcase here. } diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index 387fa58c4..581f4200c 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -423,12 +423,16 @@ func fetchRecommendationsForRegion( termStr = "3yr" } + lookback := cfg.RecLookbackPeriod + if lookback == "" { + lookback = recommendations.DefaultRecLookbackPeriod + } params := common.RecommendationParams{ Service: service, Region: region, PaymentOption: cfg.PaymentOption, Term: termStr, - LookbackPeriod: "7d", + LookbackPeriod: lookback, // Savings Plans specific filters IncludeSPTypes: cfg.IncludeSPTypes, ExcludeSPTypes: cfg.ExcludeSPTypes, diff --git a/cmd/validators.go b/cmd/validators.go index 9277e6836..a97e71bcc 100644 --- a/cmd/validators.go +++ b/cmd/validators.go @@ -29,6 +29,19 @@ func validateFlags(cmd *cobra.Command, args []string) error { return err } + if err := validateRecLookbackPeriod(); err != nil { + return err + } + + return nil +} + +// validateRecLookbackPeriod validates the --rec-lookback-period flag. +func validateRecLookbackPeriod() error { + valid := map[string]bool{"7d": true, "30d": true, "60d": true} + if !valid[toolCfg.RecLookbackPeriod] { + return fmt.Errorf("invalid rec-lookback-period %q: must be one of 7d, 30d, 60d", toolCfg.RecLookbackPeriod) + } return nil } diff --git a/cmd/validators_test.go b/cmd/validators_test.go index 98dcd5728..05c1b817c 100644 --- a/cmd/validators_test.go +++ b/cmd/validators_test.go @@ -537,3 +537,39 @@ func TestValidateCoverageLookbackDays(t *testing.T) { }) } } + +// TestValidateRecLookbackPeriod verifies that validateRecLookbackPeriod accepts +// the three valid values and rejects anything else, including empty string. +func TestValidateRecLookbackPeriod(t *testing.T) { + tests := []struct { + name string + period string + wantErr bool + errSubstr string + }{ + {name: "7d valid", period: "7d", wantErr: false}, + {name: "30d valid", period: "30d", wantErr: false}, + {name: "60d valid", period: "60d", wantErr: false}, + {name: "empty rejected", period: "", wantErr: true, errSubstr: "invalid rec-lookback-period"}, + {name: "14d rejected", period: "14d", wantErr: true, errSubstr: "invalid rec-lookback-period"}, + {name: "90d rejected", period: "90d", wantErr: true, errSubstr: "invalid rec-lookback-period"}, + {name: "SEVEN_DAYS rejected", period: "SEVEN_DAYS", wantErr: true, errSubstr: "invalid rec-lookback-period"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + origCfg := toolCfg + defer func() { toolCfg = origCfg }() + toolCfg.RecLookbackPeriod = tt.period + err := validateRecLookbackPeriod() + if tt.wantErr { + if err == nil { + t.Errorf("validateRecLookbackPeriod() expected error containing %q, got nil", tt.errSubstr) + } else if tt.errSubstr != "" && !strings.Contains(err.Error(), tt.errSubstr) { + t.Errorf("validateRecLookbackPeriod() error = %v, want substring %q", err, tt.errSubstr) + } + } else if err != nil { + t.Errorf("validateRecLookbackPeriod() unexpected error = %v", err) + } + }) + } +} diff --git a/providers/aws/recommendations/client.go b/providers/aws/recommendations/client.go index 7a1274f7f..8ea6c2bea 100644 --- a/providers/aws/recommendations/client.go +++ b/providers/aws/recommendations/client.go @@ -22,6 +22,16 @@ import ( // payer org we have seen. Exceeding the cap returns a diagnostic error (issue #692). const maxRecommendationPages = 20 +// DefaultRecLookbackPeriod is the LookbackPeriod string forwarded to +// GetReservationPurchaseRecommendation when --rec-lookback-period is not +// specified. Kept in the recommendations package so the cmd flag default, +// the cmd-side fallback, and the client-side fallback all refer to a single +// source of truth (avoids the magic-value duplication called out by +// feedback_no_hardcoded_magic_values.md). Valid CE values are 7d/30d/60d +// (see convertLookbackPeriodE); 7d matches the prior hardcoded behaviour +// from before --rec-lookback-period existed. +const DefaultRecLookbackPeriod = "7d" + // CostExplorerAPI defines the interface for Cost Explorer operations type CostExplorerAPI interface { GetReservationPurchaseRecommendation(ctx context.Context, params *costexplorer.GetReservationPurchaseRecommendationInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetReservationPurchaseRecommendationOutput, error) @@ -55,6 +65,10 @@ type Client struct { // lazily once per Client lifetime via sync.Once (one DescribeInstanceTypes // fan-out per scheduler tick). skuCatalog skuCatalog + + // recLookbackPeriod is forwarded to GetReservationPurchaseRecommendation + // as LookbackPeriodInDays. Defaults to "7d" when empty. + recLookbackPeriod string } // NewClient creates a new recommendations client @@ -100,7 +114,7 @@ func (c *Client) SetInstanceTypePagerFactory(f func() InstanceTypePager) { // instanceTypeLookup returns the cached SKU entry for instanceType. // On the first call the catalogue is built by calling the pager factory. // ok=false when no factory is configured, the catalogue fetch failed, or -// the instance type was not in the catalogue — the caller falls back to +// the instance type was not in the catalogue -- the caller falls back to // VCPU=0/MemoryGB=0 (graceful-degradation contract from Azure PR #810). func (c *Client) instanceTypeLookup(ctx context.Context, instanceType string) (instanceTypeSKUEntry, bool) { if c.instanceTypePagerFactory == nil { @@ -109,6 +123,13 @@ func (c *Client) instanceTypeLookup(ctx context.Context, instanceType string) (i return c.skuCatalog.lookup(ctx, instanceType, c.instanceTypePagerFactory) } +// SetRecLookbackPeriod configures the LookbackPeriodInDays used by +// GetRecommendationsForService. Valid values: "7d", "30d", "60d". +// An empty or unrecognised value falls back to "7d" at call time. +func (c *Client) SetRecLookbackPeriod(period string) { + c.recLookbackPeriod = period +} + // GetRecommendations fetches Reserved Instance recommendations for any service func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { // Handle Savings Plans separately — they use a different Cost Explorer API @@ -231,9 +252,47 @@ var defaultDiscoveryTerms = []string{"1yr", "3yr"} // rows and render as distinct UI rows for free. var defaultDiscoveryPaymentOptions = []string{"all-upfront", "partial-upfront", "no-upfront"} +// fetchSingleComboRecs fetches recommendations for one (term, payment) pair. +// If the context is already done before the call, it returns (nil, ctx.Err()). +// If GetRecommendations returns an error after ctx cancellation, it also +// returns (nil, ctx.Err()) so the caller exits the sweep immediately. Per-combo +// errors (throttle, 5xx) return (nil, err) with ctx.Err() == nil, signalling +// skip-and-continue tolerance in the outer loop. +func (c *Client) fetchSingleComboRecs(ctx context.Context, service common.ServiceType, term string, payment string) ([]common.Recommendation, error) { + if ctx.Err() != nil { + return nil, ctx.Err() + } + lookback := c.recLookbackPeriod + if lookback == "" { + lookback = DefaultRecLookbackPeriod + } + params := common.RecommendationParams{ + Service: service, + PaymentOption: payment, + Term: term, + LookbackPeriod: lookback, + Region: "", + } + recs, err := c.GetRecommendations(ctx, params) + if err != nil { + // A canceled / deadline-exceeded ctx is NOT a per-combo + // failure to be tolerated -- every subsequent combo + // would just hit the same dead context and waste time + // while we accumulate "failures" that hide the real + // reason. Short-circuit so the caller sees the ctx + // error verbatim. Per-combo errors (throttle, 5xx) + // keep the existing skip-and-continue tolerance. + if ctx.Err() != nil { + return nil, ctx.Err() + } + return nil, err + } + return recs, nil +} + // GetRecommendationsForService fetches recommendations for a specific -// service across the full Cartesian product of defaultDiscoveryTerms × -// defaultDiscoveryPaymentOptions (currently 2 × 3 = 6 Cost Explorer +// service across the full Cartesian product of defaultDiscoveryTerms x +// defaultDiscoveryPaymentOptions (currently 2 x 3 = 6 Cost Explorer // calls per service). Each call returns the recs for that single // (term, payment) cell and the parser tags them with params.Term / // params.PaymentOption so the resulting slice contains every combo @@ -250,26 +309,9 @@ func (c *Client) GetRecommendationsForService(ctx context.Context, service commo attempts := 0 for _, term := range defaultDiscoveryTerms { for _, payment := range defaultDiscoveryPaymentOptions { - if ctx.Err() != nil { - return nil, ctx.Err() - } attempts++ - params := common.RecommendationParams{ - Service: service, - PaymentOption: payment, - Term: term, - LookbackPeriod: "7d", - Region: "", - } - recs, err := c.GetRecommendations(ctx, params) + recs, err := c.fetchSingleComboRecs(ctx, service, term, payment) if err != nil { - // A canceled / deadline-exceeded ctx is NOT a per-combo - // failure to be tolerated — every subsequent combo - // would just hit the same dead context and waste time - // while we accumulate "failures" that hide the real - // reason. Short-circuit so the caller sees the ctx - // error verbatim. Per-combo errors (throttle, 5xx) - // keep the existing skip-and-continue tolerance. if ctx.Err() != nil { return nil, ctx.Err() } diff --git a/providers/aws/recommendations/client_test.go b/providers/aws/recommendations/client_test.go index 54beed2de..66551ce6b 100644 --- a/providers/aws/recommendations/client_test.go +++ b/providers/aws/recommendations/client_test.go @@ -917,3 +917,37 @@ func TestMergeServiceResults_AllFailIsError(t *testing.T) { require.NoError(t, err) assert.Empty(t, recs) } + +// TestSetRecLookbackPeriod_ReachesGetReservationPurchaseRecommendation asserts +// that SetRecLookbackPeriod propagates the chosen period into the +// LookbackPeriodInDays field of every GetReservationPurchaseRecommendation +// call issued by GetRecommendationsForService (refs #360). This test is +// intentionally discriminating: it would fail if recLookbackPeriod were +// ignored and the hardcoded "7d" default were sent instead. +func TestSetRecLookbackPeriod_ReachesGetReservationPurchaseRecommendation(t *testing.T) { + cases := []struct { + period string + wantEnum types.LookbackPeriodInDays + }{ + {"7d", types.LookbackPeriodInDaysSevenDays}, + {"30d", types.LookbackPeriodInDaysThirtyDays}, + {"60d", types.LookbackPeriodInDaysSixtyDays}, + } + for _, tc := range cases { + t.Run(tc.period, func(t *testing.T) { + mock := &mockCostExplorerAPI{ + riRecommendations: &costexplorer.GetReservationPurchaseRecommendationOutput{}, + } + client := NewClientWithAPI(mock, "us-east-1") + client.SetRecLookbackPeriod(tc.period) + + _, err := client.GetRecommendationsForService(context.Background(), common.ServiceEC2) + require.NoError(t, err) + require.NotEmpty(t, mock.riCalls, "GetReservationPurchaseRecommendation must have been called") + for i, call := range mock.riCalls { + assert.Equal(t, tc.wantEnum, call.LookbackPeriodInDays, + "call[%d]: LookbackPeriodInDays must match --rec-lookback-period=%s", i, tc.period) + } + }) + } +} diff --git a/providers/aws/recommendations/coverage_test.go b/providers/aws/recommendations/coverage_test.go index 685d20802..c346fc8a7 100644 --- a/providers/aws/recommendations/coverage_test.go +++ b/providers/aws/recommendations/coverage_test.go @@ -3,6 +3,7 @@ package recommendations import ( "context" "testing" + "time" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/costexplorer" @@ -15,15 +16,21 @@ import ( // mockCoverageCE extends the test mock with a configurable GetReservationCoverage // response so the coverage path can be exercised without hitting AWS. +// lastTimePeriod captures the TimePeriod from the most recent call so tests can +// assert that the CE window matches the requested lookbackDays. type mockCoverageCE struct { mockCostExplorerAPI coverageOutput *costexplorer.GetReservationCoverageOutput coverageError error coverageCalls int + lastTimePeriod *types.DateInterval } func (m *mockCoverageCE) GetReservationCoverage(ctx context.Context, params *costexplorer.GetReservationCoverageInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetReservationCoverageOutput, error) { m.coverageCalls++ + if params != nil { + m.lastTimePeriod = params.TimePeriod + } if m.coverageError != nil { return nil, m.coverageError } @@ -118,6 +125,55 @@ func TestGetRICoverageMap_LookbackDefault(t *testing.T) { assert.Equal(t, wantCalls, mock.coverageCalls) } +// TestGetRICoverageMap_LookbackWindowWidth asserts that the CE TimePeriod +// sent to GetReservationCoverage spans exactly lookbackDays calendar days. +// This test is deliberately discriminating: it parses the YYYY-MM-DD Start +// and End strings and verifies end-start == N days so a regression in the +// window math (e.g. reverting to a hardcoded 30) causes a test failure. +func TestGetRICoverageMap_LookbackWindowWidth(t *testing.T) { + cases := []struct { + name string + lookbackDays int + }{ + {"14-day window", 14}, + {"60-day window", 60}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + mock := &mockCoverageCE{coverageOutput: &costexplorer.GetReservationCoverageOutput{}} + client := NewClientWithAPI(mock, "us-east-1") + + before := time.Now().UTC() + _, err := client.GetRICoverageMap(context.Background(), tc.lookbackDays, []string{"us-east-1"}) + after := time.Now().UTC() + require.NoError(t, err) + require.NotNil(t, mock.lastTimePeriod, "GetReservationCoverage must have been called with a TimePeriod") + require.NotNil(t, mock.lastTimePeriod.Start) + require.NotNil(t, mock.lastTimePeriod.End) + + start, err := time.Parse("2006-01-02", aws.ToString(mock.lastTimePeriod.Start)) + require.NoError(t, err, "TimePeriod.Start must be a valid YYYY-MM-DD date") + end, err := time.Parse("2006-01-02", aws.ToString(mock.lastTimePeriod.End)) + require.NoError(t, err, "TimePeriod.End must be a valid YYYY-MM-DD date") + + // CE dates are truncated to day; allow for midnight-boundary + // skew of at most 1 day when the test runs near UTC midnight. + spanDays := int(end.Sub(start).Hours() / 24) + assert.Equal(t, tc.lookbackDays, spanDays, + "CE TimePeriod must span exactly lookbackDays days (start=%s end=%s)", + aws.ToString(mock.lastTimePeriod.Start), + aws.ToString(mock.lastTimePeriod.End)) + + // Sanity: the End date must fall within the test's execution window. + endUTC := end + assert.True(t, !endUTC.Before(before.Truncate(24*time.Hour)), + "TimePeriod.End must not be before test start") + assert.True(t, !endUTC.After(after.Add(24*time.Hour)), + "TimePeriod.End must not be more than a day after test completion") + }) + } +} + // TestApplyCoverageMapToRecommendations covers the org-wide pool matching: // recs look up by (region, instance_type, [engine, deployment]) so any // linked account in the org sees the same coverage % for the same pool diff --git a/providers/aws/service_client.go b/providers/aws/service_client.go index a9d5acff9..902187860 100644 --- a/providers/aws/service_client.go +++ b/providers/aws/service_client.go @@ -174,6 +174,12 @@ func (r *RecommendationsClientAdapter) GetRICoverageMap(ctx context.Context, loo return r.client.GetRICoverageMap(ctx, lookbackDays, regions) } +// SetRecLookbackPeriod configures the LookbackPeriodInDays forwarded to +// GetReservationPurchaseRecommendation. Valid values: "7d", "30d", "60d". +func (r *RecommendationsClientAdapter) SetRecLookbackPeriod(period string) { + r.client.SetRecLookbackPeriod(period) +} + // NewRecommendationsClientDirect creates a new recommendations client returning the concrete type // (needed for GetRIUtilization which is not part of the generic provider interface). func NewRecommendationsClientDirect(cfg aws.Config) *RecommendationsClientAdapter {