From 98b1c08d1b5fd09c707af1f54bf9f833c5659b55 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 23:52:03 +0200 Subject: [PATCH 1/2] fix(cli): log dropped recs from --min-pool-size filter (closes #359) applyFilters now emits one INFO line per dropped rec: INFO: --min-pool-size=2.0 dropped rds/us-east-1/db.t3.micro (avg=0.80 < threshold) and a summary line after all drops are counted: INFO: --min-pool-size dropped N recommendation(s) (X avg instances/hr total) The pool-size check is lifted out of passesDimensionFilters and evaluated first in applyFilters so drops can be observed and counted without adding side effects to the pure shouldIncludePoolSize predicate. Adds TestShouldIncludePoolSize (5 cases) and TestApplyFilters_PoolSizeLogs to assert both the per-rec and summary log lines. --- cmd/multi_service_filters.go | 15 ++++++++ cmd/multi_service_filters_test.go | 61 +++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/cmd/multi_service_filters.go b/cmd/multi_service_filters.go index 88320917c..36d589807 100644 --- a/cmd/multi_service_filters.go +++ b/cmd/multi_service_filters.go @@ -1,6 +1,8 @@ package main import ( + "fmt" + "log" "slices" "strings" @@ -11,14 +13,27 @@ import ( // 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 poolDrops []string + 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) + poolDrops = append(poolDrops, label) + continue + } adjusted, include := processRecommendation(&recs[i], cfg, instanceVersions, versionInfo, currentRegion) if include { filtered = append(filtered, adjusted) } } + if len(poolDrops) > 0 { + log.Printf("INFO: --min-pool-size dropped %d recommendation(s) (%.2f avg instances/hr total)", len(poolDrops), poolDropInstances) + } + return filtered } diff --git a/cmd/multi_service_filters_test.go b/cmd/multi_service_filters_test.go index ec2bc5f72..b907535f2 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)") +} From 44480b66973eef4e0f43c72849c118a1895d5ae2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 19 Jun 2026 23:35:45 +0200 Subject: [PATCH 2/2] fix(cli): validate min-pool-size, remove duplicate filter call, lint fixes - Reject fractional and negative --min-pool-size values in validateNumericRanges (instance counts are integers; 1.5 is nonsensical as a threshold) - Add test cases for negative, fractional, zero, and whole-number inputs - Remove redundant shouldIncludePoolSize call from passesDimensionFilters; applyFilters already handles it with logging, so the silent re-check was dead code that obscured the observable path - Replace []string accumulator (poolDrops) with an int counter since labels are already logged per-rec and the summary only needs a count - Fix rangeValCopy: use index loop to avoid copying 360-byte Recommendation - Fix equalFold: use strings.EqualFold instead of ToLower+== in engine checks - Fix godot: add missing periods to exported comments in filters and validators --- cmd/multi_service_filters.go | 54 +++++++++++++++---------------- cmd/multi_service_filters_test.go | 4 +-- cmd/validators.go | 16 +++++++++ cmd/validators_test.go | 46 ++++++++++++++++++++++++++ 4 files changed, 90 insertions(+), 30 deletions(-) diff --git a/cmd/multi_service_filters.go b/cmd/multi_service_filters.go index 36d589807..66a26f7f9 100644 --- a/cmd/multi_service_filters.go +++ b/cmd/multi_service_filters.go @@ -10,10 +10,11 @@ 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 poolDrops []string + var poolDropCount int var poolDropInstances float64 for i := range recs { @@ -21,7 +22,7 @@ func applyFilters(recs []common.Recommendation, cfg *Config, instanceVersions ma 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) - poolDrops = append(poolDrops, label) + poolDropCount++ continue } adjusted, include := processRecommendation(&recs[i], cfg, instanceVersions, versionInfo, currentRegion) @@ -30,8 +31,8 @@ func applyFilters(recs []common.Recommendation, cfg *Config, instanceVersions ma } } - if len(poolDrops) > 0 { - log.Printf("INFO: --min-pool-size dropped %d recommendation(s) (%.2f avg instances/hr total)", len(poolDrops), poolDropInstances) + if poolDropCount > 0 { + log.Printf("INFO: --min-pool-size dropped %d recommendation(s) (%.2f avg instances/hr total)", poolDropCount, poolDropInstances) } return filtered @@ -42,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 } @@ -53,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 } @@ -67,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 @@ -85,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 } @@ -99,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 { @@ -114,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 } @@ -129,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 } @@ -144,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 { @@ -168,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) { @@ -182,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 b907535f2..4e912dda9 100644 --- a/cmd/multi_service_filters_test.go +++ b/cmd/multi_service_filters_test.go @@ -434,7 +434,7 @@ func TestShouldIncludePoolSize(t *testing.T) { 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)) + assert.Equal(t, tt.expected, shouldIncludePoolSize(&rec, &cfg)) }) } } @@ -462,7 +462,7 @@ func TestApplyFilters_PoolSizeLogs(t *testing.T) { } cfg := Config{MinPoolSize: 2.0} - result := applyFilters(recs, cfg, nil, nil, "") + result := applyFilters(recs, &cfg, nil, nil, "") // Only the rec above threshold passes through. assert.Len(t, result, 1) 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 {