diff --git a/cmd/multi_service_filters.go b/cmd/multi_service_filters.go index 88320917c..66a26f7f9 100644 --- a/cmd/multi_service_filters.go +++ b/cmd/multi_service_filters.go @@ -1,6 +1,8 @@ package main import ( + "fmt" + "log" "slices" "strings" @@ -8,17 +10,31 @@ import ( ) // applyFilters applies region, instance type, engine, and engine version filters to recommendations. -// currentRegion is the region being processed in the current loop iteration - if non-empty, only recommendations for that region are included. +// currentRegion is the region being processed in the current loop iteration; if non-empty, only +// recommendations for that region are included. func applyFilters(recs []common.Recommendation, cfg *Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string) []common.Recommendation { var filtered []common.Recommendation + var poolDropCount int + var poolDropInstances float64 for i := range recs { + if cfg.MinPoolSize > 0 && !shouldIncludePoolSize(&recs[i], cfg) { + poolDropInstances += recs[i].AverageInstancesUsedPerHour + label := fmt.Sprintf("%s/%s/%s", recs[i].Service, recs[i].Region, recs[i].ResourceType) + log.Printf("INFO: --min-pool-size=%.1f dropped %s (avg=%.2f < threshold)", cfg.MinPoolSize, label, recs[i].AverageInstancesUsedPerHour) + poolDropCount++ + continue + } adjusted, include := processRecommendation(&recs[i], cfg, instanceVersions, versionInfo, currentRegion) if include { filtered = append(filtered, adjusted) } } + if poolDropCount > 0 { + log.Printf("INFO: --min-pool-size dropped %d recommendation(s) (%.2f avg instances/hr total)", poolDropCount, poolDropInstances) + } + return filtered } @@ -27,9 +43,9 @@ func applyFilters(recs []common.Recommendation, cfg *Config, instanceVersions ma // boolean-filter checks are delegated to passesDimensionFilters to keep // this function under gocyclo's complexity threshold. func processRecommendation(rec *common.Recommendation, cfg *Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string) (common.Recommendation, bool) { - // Filter to only recommendations for the current region being processed - // This prevents duplicating recommendations across all regions - // Skip this filter for Savings Plans as they are account-level, not regional + // Filter to only recommendations for the current region being processed. + // This prevents duplicating recommendations across all regions. + // Skip this filter for Savings Plans as they are account-level, not regional. if currentRegion != "" && rec.Region != currentRegion && !common.IsSavingsPlan(rec.Service) { return *rec, false } @@ -38,10 +54,10 @@ func processRecommendation(rec *common.Recommendation, cfg *Config, instanceVers return *rec, false } - // Apply engine version filters - adjust instance count by subtracting extended support versions + // Apply engine version filters - adjust instance count by subtracting extended support versions. if !cfg.IncludeExtendedSupport { adjusted := adjustRecommendationForExcludedVersions(*rec, instanceVersions, versionInfo) - // Skip if all instances were excluded (count reduced to 0) + // Skip if all instances were excluded (count reduced to 0). if adjusted.Count <= 0 { return adjusted, false } @@ -52,11 +68,11 @@ func processRecommendation(rec *common.Recommendation, cfg *Config, instanceVers } // passesDimensionFilters runs the stateless include/exclude checks on -// region, instance type, engine, account, and pool size. Returns false on +// region, instance type, engine, and account. Returns false on // the first failing filter. Split out of processRecommendation to keep // each function's cyclomatic complexity under the gocyclo limit; the // dimension filters here are pure functions of rec + cfg with no side -// effects. +// effects. Pool-size filtering is handled with logging in applyFilters. func passesDimensionFilters(rec *common.Recommendation, cfg *Config) bool { if !shouldIncludeRegion(rec.Region, cfg) { return false @@ -70,9 +86,6 @@ func passesDimensionFilters(rec *common.Recommendation, cfg *Config) bool { if !shouldIncludeAccount(rec.AccountName, cfg) { return false } - if !shouldIncludePoolSize(rec, cfg) { - return false - } return true } @@ -84,7 +97,7 @@ func passesDimensionFilters(rec *common.Recommendation, cfg *Config) bool { // where target can be meaningfully approximated. // // Pass-through cases: filter disabled (MinPoolSize<=0), or rec has no -// per-hour signal (avg<=0 - SPs and recs CE didn't return usage for). +// per-hour signal (avg<=0 -- SPs and recs CE didn't return usage for). // Those pools aren't sized via the per-hour formula so the filter doesn't // apply to them. func shouldIncludePoolSize(rec *common.Recommendation, cfg *Config) bool { @@ -99,12 +112,12 @@ func shouldIncludePoolSize(rec *common.Recommendation, cfg *Config) bool { // shouldIncludeRegion checks if a region should be included based on filters. func shouldIncludeRegion(region string, cfg *Config) bool { - // If include list is specified, region must be in it + // If include list is specified, region must be in it. if len(cfg.IncludeRegions) > 0 && !slices.Contains(cfg.IncludeRegions, region) { return false } - // If exclude list is specified, region must not be in it + // If exclude list is specified, region must not be in it. if slices.Contains(cfg.ExcludeRegions, region) { return false } @@ -114,12 +127,12 @@ func shouldIncludeRegion(region string, cfg *Config) bool { // shouldIncludeInstanceType checks if an instance type should be included based on filters. func shouldIncludeInstanceType(instanceType string, cfg *Config) bool { - // If include list is specified, instance type must be in it + // If include list is specified, instance type must be in it. if len(cfg.IncludeInstanceTypes) > 0 && !slices.Contains(cfg.IncludeInstanceTypes, instanceType) { return false } - // If exclude list is specified, instance type must not be in it + // If exclude list is specified, instance type must not be in it. if slices.Contains(cfg.ExcludeInstanceTypes, instanceType) { return false } @@ -129,17 +142,17 @@ func shouldIncludeInstanceType(instanceType string, cfg *Config) bool { // shouldIncludeEngine checks if a recommendation should be included based on engine filters. func shouldIncludeEngine(rec *common.Recommendation, cfg *Config) bool { - // Extract engine from recommendation + // Extract engine from recommendation. engine := getEngineFromRecommendation(*rec) if engine == "" { - // If no engine info, include by default unless there's an include list + // If no engine info, include by default unless there's an include list. return len(cfg.IncludeEngines) == 0 } - // Normalize engine name to lowercase for comparison + // Normalize engine name to lowercase for comparison. engine = strings.ToLower(engine) - // If include list is specified, engine must be in it + // If include list is specified, engine must be in it. if len(cfg.IncludeEngines) > 0 { found := false for _, e := range cfg.IncludeEngines { @@ -153,7 +166,7 @@ func shouldIncludeEngine(rec *common.Recommendation, cfg *Config) bool { } } - // If exclude list is specified, engine must not be in it + // If exclude list is specified, engine must not be in it. if len(cfg.ExcludeEngines) > 0 { for _, e := range cfg.ExcludeEngines { if strings.EqualFold(e, engine) { @@ -167,19 +180,19 @@ func shouldIncludeEngine(rec *common.Recommendation, cfg *Config) bool { // shouldIncludeAccount checks if an account should be included based on filters. func shouldIncludeAccount(accountName string, cfg *Config) bool { - // If account name is empty and there are filters, skip it (unless include list is empty) + // If account name is empty and there are filters, skip it (unless include list is empty). if accountName == "" { return len(cfg.IncludeAccounts) == 0 && len(cfg.ExcludeAccounts) == 0 } accountLower := strings.ToLower(accountName) - // Check include list + // Check include list. if !checkIncludeList(accountLower, cfg.IncludeAccounts) { return false } - // Check exclude list + // Check exclude list. if checkExcludeList(accountLower, cfg.ExcludeAccounts) { return false } diff --git a/cmd/multi_service_filters_test.go b/cmd/multi_service_filters_test.go index ec2bc5f72..4e912dda9 100644 --- a/cmd/multi_service_filters_test.go +++ b/cmd/multi_service_filters_test.go @@ -1,6 +1,8 @@ package main import ( + "bytes" + "log" "testing" "github.com/LeanerCloud/CUDly/pkg/common" @@ -413,3 +415,62 @@ func TestShouldIncludeAccount(t *testing.T) { }) } } + +func TestShouldIncludePoolSize(t *testing.T) { + tests := []struct { + name string + avg float64 + minPool float64 + expected bool + }{ + {"filter disabled (0)", 0.5, 0, true}, + {"avg=0 passes through", 0, 2.0, true}, + {"avg below threshold", 1.5, 2.0, false}, + {"avg equal to threshold", 2.0, 2.0, true}, + {"avg above threshold", 3.0, 2.0, true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rec := common.Recommendation{AverageInstancesUsedPerHour: tt.avg} + cfg := Config{MinPoolSize: tt.minPool} + assert.Equal(t, tt.expected, shouldIncludePoolSize(&rec, &cfg)) + }) + } +} + +// TestApplyFilters_PoolSizeLogs verifies that applyFilters emits per-rec and +// summary log lines when --min-pool-size drops recommendations. +func TestApplyFilters_PoolSizeLogs(t *testing.T) { + origCfg := toolCfg + defer func() { toolCfg = origCfg }() + + // Capture log output. + var buf bytes.Buffer + origFlags := log.Flags() + origWriter := log.Writer() + log.SetOutput(&buf) + log.SetFlags(0) // strip timestamps so the assertions are stable + defer func() { + log.SetOutput(origWriter) + log.SetFlags(origFlags) + }() + + recs := []common.Recommendation{ + {Service: "rds", Region: "us-east-1", ResourceType: "db.t3.micro", AverageInstancesUsedPerHour: 0.8, Count: 1}, + {Service: "rds", Region: "us-east-1", ResourceType: "db.r5.large", AverageInstancesUsedPerHour: 3.5, Count: 2}, + } + cfg := Config{MinPoolSize: 2.0} + + result := applyFilters(recs, &cfg, nil, nil, "") + + // Only the rec above threshold passes through. + assert.Len(t, result, 1) + assert.Equal(t, "db.r5.large", result[0].ResourceType) + + output := buf.String() + // Per-rec line present. + assert.Contains(t, output, "--min-pool-size=2.0 dropped rds/us-east-1/db.t3.micro (avg=0.80 < threshold)") + // Summary line present. + assert.Contains(t, output, "--min-pool-size dropped 1 recommendation(s)") +} diff --git a/cmd/validators.go b/cmd/validators.go index a97e71bcc..25634f49b 100644 --- a/cmd/validators.go +++ b/cmd/validators.go @@ -82,6 +82,22 @@ func validateNumericRanges(cmd *cobra.Command) error { return fmt.Errorf("override-count (%d) exceeds reasonable limit of %d", toolCfg.OverrideCount, MaxReasonableInstances) } + if err := validateMinPoolSize(); err != nil { + return err + } + + return nil +} + +// validateMinPoolSize checks that --min-pool-size is a non-negative whole number. +// Instance counts are integers, so a fractional threshold is nonsensical. +func validateMinPoolSize() error { + if toolCfg.MinPoolSize < 0 { + return fmt.Errorf("min-pool-size must be 0 (disabled) or a positive whole number, got: %.2f", toolCfg.MinPoolSize) + } + if toolCfg.MinPoolSize > 0 && toolCfg.MinPoolSize != float64(int64(toolCfg.MinPoolSize)) { + return fmt.Errorf("min-pool-size must be a whole number (instance counts are integers), got: %.2f", toolCfg.MinPoolSize) + } return nil } diff --git a/cmd/validators_test.go b/cmd/validators_test.go index 05c1b817c..979c02717 100644 --- a/cmd/validators_test.go +++ b/cmd/validators_test.go @@ -87,6 +87,52 @@ func TestValidateNumericRanges(t *testing.T) { wantErr: true, errMsg: "override-count", }, + { + name: "min-pool-size negative", + setupFunc: func() { + toolCfg.Coverage = 80.0 + toolCfg.MaxInstances = 0 + toolCfg.OverrideCount = 0 + toolCfg.CoverageLookbackDays = 30 + toolCfg.MinPoolSize = -1.0 + }, + wantErr: true, + errMsg: "min-pool-size must be 0 (disabled) or a positive whole number", + }, + { + name: "min-pool-size fractional rejected", + setupFunc: func() { + toolCfg.Coverage = 80.0 + toolCfg.MaxInstances = 0 + toolCfg.OverrideCount = 0 + toolCfg.CoverageLookbackDays = 30 + toolCfg.MinPoolSize = 1.5 + }, + wantErr: true, + errMsg: "min-pool-size must be a whole number", + }, + { + name: "min-pool-size disabled (zero)", + setupFunc: func() { + toolCfg.Coverage = 80.0 + toolCfg.MaxInstances = 0 + toolCfg.OverrideCount = 0 + toolCfg.CoverageLookbackDays = 30 + toolCfg.MinPoolSize = 0 + }, + wantErr: false, + }, + { + name: "min-pool-size whole number accepted", + setupFunc: func() { + toolCfg.Coverage = 80.0 + toolCfg.MaxInstances = 0 + toolCfg.OverrideCount = 0 + toolCfg.CoverageLookbackDays = 30 + toolCfg.MinPoolSize = 2.0 + }, + wantErr: false, + }, } for _, tt := range tests {