From f265ebad3ba39d29f0dca8b65625e7672925958f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 25 Aug 2026 03:01:21 +0200 Subject: [PATCH 01/12] refactor(common): move CheckAuditLogWritable into pkg/common The audit-log writability probe lived in package main under cmd/, so the MCP server could not reuse it for its own startup check. Move it next to WriteAuditRecord/NewAuditRecord in pkg/common/audit.go; cmd keeps a thin delegating wrapper under the same name so its call sites and tests are unchanged. Behaviour is identical, including the 0600 create mode. Refs #1883 --- cmd/helpers.go | 13 ++++--------- pkg/common/audit.go | 10 ++++++++++ pkg/common/audit_test.go | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 9 deletions(-) diff --git a/cmd/helpers.go b/cmd/helpers.go index 22a7970c0..083641184 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -572,15 +572,10 @@ func ConfirmPurchase(totalInstances int, totalSavings float64, skipConfirmation return response == "yes" || response == "y" } -// CheckAuditLogWritable opens the audit log file in append mode to verify it is writable. -// Returns an error if the path cannot be opened for writing. -func CheckAuditLogWritable(path string) error { - f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600) // #nosec G304 -- audit log path is operator-configured; value is not reachable from user input - if err != nil { - return fmt.Errorf("audit log %q not writable: %w", path, err) - } - return f.Close() -} +// CheckAuditLogWritable reports whether the audit log at path can be appended to. +// Thin wrapper over common.CheckAuditLogWritable; kept so cmd's existing call sites +// and tests are unchanged. +func CheckAuditLogWritable(path string) error { return common.CheckAuditLogWritable(path) } // DuplicateChecker checks for existing commitments to avoid duplicates. type DuplicateChecker struct { diff --git a/pkg/common/audit.go b/pkg/common/audit.go index 457914bd7..1b951a97b 100644 --- a/pkg/common/audit.go +++ b/pkg/common/audit.go @@ -37,6 +37,16 @@ func WriteAuditRecord(record AuditRecord, path string) error { return nil } +// CheckAuditLogWritable opens the audit log file in append mode to verify it is writable. +// Returns an error if the path cannot be opened for writing. +func CheckAuditLogWritable(path string) error { + f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600) // #nosec G304 -- audit log path is operator-configured; value is not reachable from user input + if err != nil { + return fmt.Errorf("audit log %q not writable: %w", path, err) + } + return f.Close() +} + // NewAuditRecord constructs an AuditRecord from a Recommendation and a PurchaseResult. // status must be one of: "success", "error", "skipped" (dry-run), "skipped_covered" (idempotency). // source is the CUDly surface that triggered the run — copied into the JSONL so CLI diff --git a/pkg/common/audit_test.go b/pkg/common/audit_test.go index ae9eaf4df..ded139722 100644 --- a/pkg/common/audit_test.go +++ b/pkg/common/audit_test.go @@ -141,6 +141,39 @@ func TestNewAuditRecord_Fields(t *testing.T) { assert.WithinDuration(t, time.Now().UTC(), ar.Timestamp, 5*time.Second) } +func TestCheckAuditLogWritable_WritablePath(t *testing.T) { + t.Parallel() + dir := t.TempDir() + path := filepath.Join(dir, "audit.jsonl") + + assert.NoError(t, CheckAuditLogWritable(path)) +} + +func TestCheckAuditLogWritable_UnwritablePath(t *testing.T) { + t.Parallel() + dir := t.TempDir() + require.NoError(t, os.Chmod(dir, 0555)) + t.Cleanup(func() { _ = os.Chmod(dir, 0755) }) + + path := filepath.Join(dir, "audit.jsonl") + err := CheckAuditLogWritable(path) + assert.Error(t, err) + assert.Contains(t, err.Error(), path) +} + +func TestCheckAuditLogWritable_DoesNotTruncateExistingContent(t *testing.T) { + t.Parallel() + dir := t.TempDir() + path := filepath.Join(dir, "audit.jsonl") + require.NoError(t, os.WriteFile(path, []byte("line1\n"), 0644)) + + require.NoError(t, CheckAuditLogWritable(path)) + + data, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "line1\n", string(data)) +} + func TestTermMonths(t *testing.T) { t.Parallel() cases := []struct { From 0a69be170c5eebb4aa4043ebb255c5581e9ded63 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 25 Aug 2026 03:03:06 +0200 Subject: [PATCH 02/12] refactor(recfilter): extract dimension filters and min-pool-size to pkg The region/instance-type/engine filters and the --min-pool-size stage lived in package main, so the MCP server could not reuse them and would have had to hand-roll engine matching for its search tool. Move them into a new pkg/recfilter behind a Filters struct; cmd keeps thin delegating wrappers under the existing names so its filter tests run untouched against the extracted code. cmd/helpers.go declares AppLogger on os.Stdout, which the MCP server owns as its protocol transport, so ApplyMinPoolSize takes an injected Logf instead of reaching for a package-level logger. A nil Logf is silent. Account filtering stays in cmd: it matches account *names* via AccountAliasCache (organizations:DescribeAccount) and is deliberately not part of the MCP surface. cmd's private engineNameMap, normalizeEngineName and getEngineFromRecommendation are deleted as byte-equivalent duplicates of common.NormalizeEngineName and common.EngineFromDetails. Refs #1883 --- cmd/helpers.go | 74 +--------------- cmd/helpers_test.go | 4 +- cmd/multi_service_filters.go | 136 +++++++++-------------------- pkg/recfilter/filters.go | 159 ++++++++++++++++++++++++++++++++++ pkg/recfilter/filters_test.go | 154 ++++++++++++++++++++++++++++++++ 5 files changed, 356 insertions(+), 171 deletions(-) create mode 100644 pkg/recfilter/filters.go create mode 100644 pkg/recfilter/filters_test.go diff --git a/cmd/helpers.go b/cmd/helpers.go index 083641184..309599edd 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -649,7 +649,7 @@ func buildExistingCommitmentsMap(commitments []common.Commitment) map[string]int for _rvc := range commitments { c := commitments[_rvc] - normalizedEngine := normalizeEngineName(c.Engine) + normalizedEngine := common.NormalizeEngineName(c.Engine) key := fmt.Sprintf("%s|%s|%s", c.ResourceType, c.Region, normalizedEngine) existingMap[key] += c.Count log.Printf(" [DuplicateChecker] Recent RI: key=%s count=%d startDate=%s (raw engine=%s)", @@ -680,7 +680,7 @@ func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existing // adjustSingleRecommendation adjusts a single recommendation based on existing commitments. func adjustSingleRecommendation(rec common.Recommendation, existingMap map[string]int) common.Recommendation { - engine := getEngineFromRecommendation(rec) + engine := common.EngineFromDetails(rec.Details) key := fmt.Sprintf("%s|%s|%s", rec.ResourceType, rec.Region, engine) existingCount := existingMap[key] @@ -704,76 +704,6 @@ func adjustSingleRecommendation(rec common.Recommendation, existingMap map[strin return adjusted } -// getEngineFromRecommendation extracts the engine from recommendation details. -// DatabaseDetails/CacheDetails are always pointers (every producer -- AWS, -// Azure, the CSV loader, and the JSON codec -- constructs them that way); see -// pkg/common/service_details_codec.go's package doc for the pointer -// invariant. The nil-pointer guards are not dead code: `rec.Details == nil` -// only catches an untyped nil, so a typed nil (a (*common.DatabaseDetails)(nil) -// stored in the interface) reaches the switch and would panic on the field -// read. Returning blank matches the sibling helpers (extractDeployment and -// extractEngine in multi_service_csv.go, rdsEngineDeploymentFromRec in the -// AWS coverage package), which already guard the same way. -func getEngineFromRecommendation(rec common.Recommendation) string { - if rec.Details == nil { - return "" - } - var engine string - switch details := rec.Details.(type) { - case *common.DatabaseDetails: - if details == nil { - return "" - } - engine = details.Engine - case *common.CacheDetails: - if details == nil { - return "" - } - engine = details.Engine - default: - return "" - } - return normalizeEngineName(engine) -} - -// engineNameMap maps database engine names to a consistent normalized format. -// AWS RIs use: "aurora-postgresql", "aurora-mysql", "mysql", "postgres" -// Cost Explorer uses: "Aurora PostgreSQL", "Aurora MySQL", "MySQL", "PostgreSQL". -var engineNameMap = map[string]string{ - // Cost Explorer format -> normalized - "Aurora PostgreSQL": "aurora-postgresql", - "Aurora MySQL": "aurora-mysql", - "MySQL": "mysql", - "PostgreSQL": "postgresql", - "MariaDB": "mariadb", - "Oracle": "oracle", - "SQL Server": "sqlserver", - // Already normalized (from AWS RIs) - "aurora-postgresql": "aurora-postgresql", - "aurora-mysql": "aurora-mysql", - "mysql": "mysql", - "postgresql": "postgresql", - "postgres": "postgresql", - "mariadb": "mariadb", - "oracle-se": "oracle", - "oracle-se1": "oracle", - "oracle-se2": "oracle", - "oracle-ee": "oracle", - "sqlserver-se": "sqlserver", - "sqlserver-ee": "sqlserver", - "sqlserver-ex": "sqlserver", - "sqlserver-web": "sqlserver", -} - -// normalizeEngineName normalizes database engine names to a consistent format. -func normalizeEngineName(engine string) string { - if normalized, ok := engineNameMap[engine]; ok { - return normalized - } - // Return lowercase as fallback - return strings.ToLower(engine) -} - // AdjustRecommendationsForExistingRIs is an alias for AdjustRecommendationsForExisting. func (d *DuplicateChecker) AdjustRecommendationsForExistingRIs(ctx context.Context, recs []common.Recommendation, client provider.ServiceClient) (passed, filtered []common.Recommendation, err error) { return d.AdjustRecommendationsForExisting(ctx, recs, client) diff --git a/cmd/helpers_test.go b/cmd/helpers_test.go index 82f9de211..357d01292 100644 --- a/cmd/helpers_test.go +++ b/cmd/helpers_test.go @@ -583,7 +583,7 @@ func TestNormalizeEngineName(t *testing.T) { for _, tt := range tests { t.Run(tt.input, func(t *testing.T) { - result := normalizeEngineName(tt.input) + result := common.NormalizeEngineName(tt.input) assert.Equal(t, tt.expected, result) }) } @@ -649,7 +649,7 @@ func TestGetEngineFromRecommendation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := getEngineFromRecommendation(tt.rec) + result := common.EngineFromDetails(tt.rec.Details) assert.Equal(t, tt.expected, result) }) } diff --git a/cmd/multi_service_filters.go b/cmd/multi_service_filters.go index 2d652baf4..66eb43e7d 100644 --- a/cmd/multi_service_filters.go +++ b/cmd/multi_service_filters.go @@ -1,34 +1,39 @@ package main import ( - "fmt" "log" - "slices" "strings" "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/pkg/recfilter" awsprovider "github.com/LeanerCloud/CUDly/providers/aws" ) +// filtersFromConfig maps the CLI Config's dimension-filter and min-pool-size +// fields onto a recfilter.Filters value. Account filtering stays cmd-only +// (see shouldIncludeAccount) so it is not part of recfilter.Filters. +func filtersFromConfig(cfg *Config) recfilter.Filters { + return recfilter.Filters{ + IncludeRegions: cfg.IncludeRegions, + ExcludeRegions: cfg.ExcludeRegions, + IncludeInstanceTypes: cfg.IncludeInstanceTypes, + ExcludeInstanceTypes: cfg.ExcludeInstanceTypes, + IncludeEngines: cfg.IncludeEngines, + ExcludeEngines: cfg.ExcludeEngines, + MinPoolSize: cfg.MinPoolSize, + } +} + // 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. // drops accumulates per-reason drop counts for the end-of-run summary; pass nil to skip tracking. func applyFilters(recs []common.Recommendation, cfg *Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string, drops *common.DropSummary) []common.Recommendation { - var filtered []common.Recommendation - var poolDropCount int - var poolDropInstances float64 + survivors := filtersFromConfig(cfg).ApplyMinPoolSize(recs, log.Printf, drops) - 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++ - drops.Add(common.DropMinPoolSize, 1) - continue - } - adjusted, include, dropReason := processRecommendation(&recs[i], cfg, instanceVersions, versionInfo, currentRegion) + var filtered []common.Recommendation + for i := range survivors { + adjusted, include, dropReason := processRecommendation(&survivors[i], cfg, instanceVersions, versionInfo, currentRegion) if include { filtered = append(filtered, adjusted) } else if dropReason != "" { @@ -36,10 +41,6 @@ func applyFilters(recs []common.Recommendation, cfg *Config, instanceVersions ma } } - if poolDropCount > 0 { - log.Printf("INFO: --min-pool-size dropped %d recommendation(s) (%.2f avg instances/hr total)", poolDropCount, poolDropInstances) - } - return filtered } @@ -86,6 +87,11 @@ func processRecommendation(rec *common.Recommendation, cfg *Config, instanceVers // each function's cyclomatic complexity under the gocyclo limit; the // dimension filters here are pure functions of rec + cfg with no side // effects. Pool-size filtering is handled with logging in applyFilters. +// +// Region and account stay here rather than moving into recfilter: the +// region-agnostic handling (#1881) needs providers/aws, which the pkg module +// cannot import, and account filtering is name-substring matching backed by +// AccountAliasCache. Only the instance-type and engine checks are portable. func passesDimensionFilters(rec *common.Recommendation, cfg *Config) bool { if !shouldIncludeRecommendationRegion(rec, cfg) { return false @@ -96,31 +102,14 @@ func passesDimensionFilters(rec *common.Recommendation, cfg *Config) bool { if !shouldIncludeEngine(rec, cfg) { return false } - if !shouldIncludeAccount(rec.AccountName, cfg) { - return false - } - return true + return shouldIncludeAccount(rec.AccountName, cfg) } -// shouldIncludePoolSize filters out RI recommendations for pools whose -// AverageInstancesUsedPerHour is below cfg.MinPoolSize. The purpose is to -// drop tiny pools where integer-arithmetic sizing forces 100% coverage -// regardless of --target-coverage (e.g. avg=1 with target=80% -> floor(0.8)=0 -// drops, ceil(0.8)=1 over-covers). Setting --min-pool-size=2 keeps pools -// 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). -// Those pools aren't sized via the per-hour formula so the filter doesn't -// apply to them. +// shouldIncludePoolSize checks if a recommendation's pool size meets cfg.MinPoolSize. +// Thin wrapper over recfilter.Filters.IncludesPoolSize; kept so cmd's existing call sites +// and tests are unchanged. func shouldIncludePoolSize(rec *common.Recommendation, cfg *Config) bool { - if cfg.MinPoolSize <= 0 { - return true - } - if rec.AverageInstancesUsedPerHour <= 0 { - return true - } - return rec.AverageInstancesUsedPerHour >= cfg.MinPoolSize + return filtersFromConfig(cfg).IncludesPoolSize(rec) } // shouldIncludeRecommendationRegion applies the region filters to a whole @@ -143,71 +132,24 @@ func shouldIncludeRecommendationRegion(rec *common.Recommendation, cfg *Config) } // shouldIncludeRegion checks if a region should be included based on filters. +// Thin wrapper over recfilter.Filters.IncludesRegion; kept so cmd's existing call sites +// and tests are unchanged. func shouldIncludeRegion(region string, cfg *Config) bool { - // 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 slices.Contains(cfg.ExcludeRegions, region) { - return false - } - - return true + return filtersFromConfig(cfg).IncludesRegion(region) } // shouldIncludeInstanceType checks if an instance type should be included based on filters. +// Thin wrapper over recfilter.Filters.IncludesInstanceType; kept so cmd's existing call sites +// and tests are unchanged. func shouldIncludeInstanceType(instanceType string, cfg *Config) bool { - // 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 slices.Contains(cfg.ExcludeInstanceTypes, instanceType) { - return false - } - - return true + return filtersFromConfig(cfg).IncludesInstanceType(instanceType) } // shouldIncludeEngine checks if a recommendation should be included based on engine filters. +// Thin wrapper over recfilter.Filters.IncludesEngine; kept so cmd's existing call sites +// and tests are unchanged. func shouldIncludeEngine(rec *common.Recommendation, cfg *Config) bool { - // Extract engine from recommendation. - engine := getEngineFromRecommendation(*rec) - if engine == "" { - // 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. - engine = strings.ToLower(engine) - - // If include list is specified, engine must be in it. - if len(cfg.IncludeEngines) > 0 { - found := false - for _, e := range cfg.IncludeEngines { - if strings.EqualFold(e, engine) { - found = true - break - } - } - if !found { - return false - } - } - - // 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) { - return false - } - } - } - - return true + return filtersFromConfig(cfg).IncludesEngine(rec) } // shouldIncludeAccount checks if an account should be included based on filters. diff --git a/pkg/recfilter/filters.go b/pkg/recfilter/filters.go new file mode 100644 index 000000000..489864e26 --- /dev/null +++ b/pkg/recfilter/filters.go @@ -0,0 +1,159 @@ +// Package recfilter holds the stateless dimension filters (region, instance +// type, engine) and the minimum-pool-size stage shared between the cmd CLI +// and the MCP server. +package recfilter + +import ( + "fmt" + "slices" + "strings" + + "github.com/LeanerCloud/CUDly/pkg/common" +) + +// Logf is an injected logging sink. Package recfilter never logs through a +// package-level logger: cmd's AppLogger writes to stdout, which the MCP +// server owns as its protocol transport. A nil Logf disables logging. +type Logf func(format string, args ...any) + +// printf calls l with format/args, or no-ops if l is nil. +func (l Logf) printf(format string, args ...any) { + if l == nil { + return + } + l(format, args...) +} + +// Filters holds the stateless include/exclude dimension filters plus the +// minimum-pool-size threshold. Zero value = allow everything. +type Filters struct { + IncludeRegions []string + ExcludeRegions []string + IncludeInstanceTypes []string + ExcludeInstanceTypes []string + IncludeEngines []string + ExcludeEngines []string + MinPoolSize float64 +} + +// IncludesRegion checks if a region should be included based on filters. +func (f Filters) IncludesRegion(region string) bool { + // If include list is specified, region must be in it. + if len(f.IncludeRegions) > 0 && !slices.Contains(f.IncludeRegions, region) { + return false + } + + // If exclude list is specified, region must not be in it. + if slices.Contains(f.ExcludeRegions, region) { + return false + } + + return true +} + +// IncludesInstanceType checks if an instance type should be included based on filters. +func (f Filters) IncludesInstanceType(instanceType string) bool { + // If include list is specified, instance type must be in it. + if len(f.IncludeInstanceTypes) > 0 && !slices.Contains(f.IncludeInstanceTypes, instanceType) { + return false + } + + // If exclude list is specified, instance type must not be in it. + if slices.Contains(f.ExcludeInstanceTypes, instanceType) { + return false + } + + return true +} + +// IncludesEngine checks if a recommendation should be included based on engine filters. +func (f Filters) IncludesEngine(rec *common.Recommendation) bool { + // Extract engine from recommendation. + engine := common.EngineFromDetails(rec.Details) + if engine == "" { + // If no engine info, include by default unless there's an include list. + return len(f.IncludeEngines) == 0 + } + + // Normalize engine name to lowercase for comparison. + engine = strings.ToLower(engine) + + // If include list is specified, engine must be in it. + if len(f.IncludeEngines) > 0 { + found := false + for _, e := range f.IncludeEngines { + if strings.EqualFold(e, engine) { + found = true + break + } + } + if !found { + return false + } + } + + // If exclude list is specified, engine must not be in it. + if len(f.ExcludeEngines) > 0 { + for _, e := range f.ExcludeEngines { + if strings.EqualFold(e, engine) { + return false + } + } + } + + return true +} + +// IncludesPoolSize filters out RI recommendations for pools whose +// AverageInstancesUsedPerHour is below f.MinPoolSize. The purpose is to +// drop tiny pools where integer-arithmetic sizing forces 100% coverage +// regardless of --target-coverage (e.g. avg=1 with target=80% -> floor(0.8)=0 +// drops, ceil(0.8)=1 over-covers). Setting --min-pool-size=2 keeps pools +// 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). +// Those pools aren't sized via the per-hour formula so the filter doesn't +// apply to them. +func (f Filters) IncludesPoolSize(rec *common.Recommendation) bool { + if f.MinPoolSize <= 0 { + return true + } + if rec.AverageInstancesUsedPerHour <= 0 { + return true + } + return rec.AverageInstancesUsedPerHour >= f.MinPoolSize +} + +// ApplyMinPoolSize drops recommendations whose AverageInstancesUsedPerHour +// is below f.MinPoolSize, logging a per-recommendation line via logf (no-op +// if nil) and recording each drop in drops under common.DropMinPoolSize. +// Returns recs unchanged, with no logging or allocation, when the filter is +// disabled (f.MinPoolSize <= 0). +func (f Filters) ApplyMinPoolSize(recs []common.Recommendation, logf Logf, drops *common.DropSummary) []common.Recommendation { + if f.MinPoolSize <= 0 { + return recs + } + + var kept []common.Recommendation + var poolDropCount int + var poolDropInstances float64 + + for i := range recs { + if f.IncludesPoolSize(&recs[i]) { + kept = append(kept, recs[i]) + continue + } + poolDropInstances += recs[i].AverageInstancesUsedPerHour + label := fmt.Sprintf("%s/%s/%s", recs[i].Service, recs[i].Region, recs[i].ResourceType) + logf.printf("INFO: --min-pool-size=%.1f dropped %s (avg=%.2f < threshold)", f.MinPoolSize, label, recs[i].AverageInstancesUsedPerHour) + poolDropCount++ + drops.Add(common.DropMinPoolSize, 1) + } + + if poolDropCount > 0 { + logf.printf("INFO: --min-pool-size dropped %d recommendation(s) (%.2f avg instances/hr total)", poolDropCount, poolDropInstances) + } + + return kept +} diff --git a/pkg/recfilter/filters_test.go b/pkg/recfilter/filters_test.go new file mode 100644 index 000000000..af648aaf6 --- /dev/null +++ b/pkg/recfilter/filters_test.go @@ -0,0 +1,154 @@ +package recfilter + +import ( + "fmt" + "testing" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/stretchr/testify/assert" +) + +func TestIncludesEngine_CEAndRISpellingsBothMatch(t *testing.T) { + tests := []struct { + name string + recEngine string + includeEngines []string + expected bool + }{ + {"CE spelling rec, RI spelling filter", "Aurora PostgreSQL", []string{"aurora-postgresql"}, true}, + {"RI spelling rec, CE-normalized filter", "aurora-postgresql", []string{"aurora-postgresql"}, true}, + {"postgres rec, postgresql filter", "postgres", []string{"postgresql"}, true}, + {"PostgreSQL rec, postgresql filter", "PostgreSQL", []string{"postgresql"}, true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := Filters{IncludeEngines: tt.includeEngines} + rec := common.Recommendation{Details: &common.DatabaseDetails{Engine: tt.recEngine}} + assert.Equal(t, tt.expected, f.IncludesEngine(&rec)) + }) + } +} + +func TestIncludesRegion_EmptyIncludeListAllowsAll(t *testing.T) { + f := Filters{} + assert.True(t, f.IncludesRegion("us-east-1")) + assert.True(t, f.IncludesRegion("eu-west-1")) +} + +func TestIncludesInstanceType_EmptyIncludeListAllowsAll(t *testing.T) { + f := Filters{} + assert.True(t, f.IncludesInstanceType("db.t3.micro")) + assert.True(t, f.IncludesInstanceType("cache.r5.large")) +} + +func TestIncludesEngine_EmptyIncludeListAllowsAll(t *testing.T) { + f := Filters{} + rec := common.Recommendation{Details: &common.DatabaseDetails{Engine: "mysql"}} + assert.True(t, f.IncludesEngine(&rec)) +} + +func TestIncludesRegion_ExcludeBeatsInclude(t *testing.T) { + f := Filters{IncludeRegions: []string{"us-east-1"}, ExcludeRegions: []string{"us-east-1"}} + assert.False(t, f.IncludesRegion("us-east-1")) +} + +func TestIncludesInstanceType_ExcludeBeatsInclude(t *testing.T) { + f := Filters{IncludeInstanceTypes: []string{"db.t3.micro"}, ExcludeInstanceTypes: []string{"db.t3.micro"}} + assert.False(t, f.IncludesInstanceType("db.t3.micro")) +} + +func TestIncludesEngine_ExcludeBeatsInclude(t *testing.T) { + f := Filters{IncludeEngines: []string{"mysql"}, ExcludeEngines: []string{"mysql"}} + rec := common.Recommendation{Details: &common.DatabaseDetails{Engine: "mysql"}} + assert.False(t, f.IncludesEngine(&rec)) +} + +func TestIncludesPoolSize(t *testing.T) { + tests := []struct { + name string + avg float64 + minPool float64 + expected bool + }{ + {"filter disabled", 0.5, 0, true}, + {"no per-hour signal passes through", 0, 2.0, true}, + {"below threshold", 1.5, 2.0, false}, + {"at threshold", 2.0, 2.0, true}, + {"above threshold", 3.0, 2.0, true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := Filters{MinPoolSize: tt.minPool} + rec := common.Recommendation{AverageInstancesUsedPerHour: tt.avg} + assert.Equal(t, tt.expected, f.IncludesPoolSize(&rec)) + }) + } +} + +func TestApplyMinPoolSize_NilLogfSafeAndRecordsDrop(t *testing.T) { + f := Filters{MinPoolSize: 2.0} + recs := []common.Recommendation{ + {Region: "us-east-1", ResourceType: "db.t3.micro", AverageInstancesUsedPerHour: 1.5}, + } + drops := common.NewDropSummary() + + assert.NotPanics(t, func() { + result := f.ApplyMinPoolSize(recs, nil, drops) + assert.Empty(t, result) + }) + assert.Equal(t, 1, drops.Total()) + assert.Contains(t, drops.FormatOneLine(), common.DropMinPoolSize) +} + +func TestApplyMinPoolSize_DisabledReturnsUnchangedNoDrops(t *testing.T) { + f := Filters{MinPoolSize: 0} + recs := []common.Recommendation{ + {Region: "us-east-1", ResourceType: "db.t3.micro", AverageInstancesUsedPerHour: 0.1}, + } + drops := common.NewDropSummary() + + result := f.ApplyMinPoolSize(recs, nil, drops) + + assert.Equal(t, recs, result) + assert.Equal(t, 0, drops.Total()) +} + +func TestApplyMinPoolSize_LogfReceivesExpectedLines(t *testing.T) { + f := Filters{MinPoolSize: 2.0} + recs := []common.Recommendation{ + {Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.micro", AverageInstancesUsedPerHour: 1.5}, + {Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.small", AverageInstancesUsedPerHour: 5.0}, + } + drops := common.NewDropSummary() + + var lines []string + logf := func(format string, args ...any) { + lines = append(lines, fmt.Sprintf(format, args...)) + } + + result := f.ApplyMinPoolSize(recs, logf, drops) + + assert.Len(t, result, 1) + assert.Len(t, lines, 2) + assert.Contains(t, lines[0], "--min-pool-size=2.0 dropped") + assert.Contains(t, lines[1], "--min-pool-size dropped 1 recommendation(s)") +} + +// The dimension predicates are deliberately independent rather than bundled +// behind one PassesDimensions helper: region resolution is provider-specific +// (a Savings Plan's effective region lives in Details, see #1881) and needs +// providers/aws, which the pkg module cannot import. Callers compose the +// portable checks with their own region handling. +func TestDimensionPredicates_IgnoreAccount(t *testing.T) { + f := Filters{} + rec := common.Recommendation{ + Region: "us-east-1", + ResourceType: "db.t3.micro", + AccountName: "some-restricted-account", + } + assert.True(t, f.IncludesRegion(rec.Region)) + assert.True(t, f.IncludesInstanceType(rec.ResourceType)) + assert.True(t, f.IncludesEngine(&rec)) +} From cb0970c4bae208df887240dd661184095e5b0960 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 25 Aug 2026 03:33:44 +0200 Subject: [PATCH 03/12] refactor(recfilter): extract coverage and target-coverage sizing to pkg ApplyCoverage and ApplyTargetCoverage (with its RI and SP branches) lived in package main, so the MCP search tool could not size recommendations the way the CLI does. A model reimplementing --coverage by multiplying costs by 0.8 gets the money wrong by up to ~50%: the RI path scales cost-bearing fields by the DISCRETE ratio newCount/Count, not the requested ratio. Extracting the real implementation is the only way both surfaces agree. Both functions move into pkg/recfilter/sizing.go and take an injected Logf, since cmd's AppLogger writes to os.Stdout and the MCP server owns stdout as its protocol transport. cmd keeps thin wrappers under the existing names passing AppLogger.Printf, so its sizing tests run untouched. The exported ApplyCoverage/applyCoverage pair collapses into one recfilter function taking drops; the split only existed to give the exported form a shorter signature. Refs #1883 --- cmd/helpers.go | 365 +-------------------------------- pkg/recfilter/sizing.go | 368 +++++++++++++++++++++++++++++++++ pkg/recfilter/sizing_test.go | 383 +++++++++++++++++++++++++++++++++++ 3 files changed, 762 insertions(+), 354 deletions(-) create mode 100644 pkg/recfilter/sizing.go create mode 100644 pkg/recfilter/sizing_test.go diff --git a/cmd/helpers.go b/cmd/helpers.go index 309599edd..911708112 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -5,7 +5,6 @@ import ( "context" "fmt" "log" - "math" "os" "strings" "sync" @@ -13,6 +12,7 @@ import ( "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/provider" + "github.com/LeanerCloud/CUDly/pkg/recfilter" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/organizations" "golang.org/x/term" @@ -114,366 +114,23 @@ func CalculateTotalInstances(recs []common.Recommendation) int { return total } -// ApplyCoverage applies coverage percentage to recommendations. -// -// All cost-bearing fields (CommitmentCost, OnDemandCost, EstimatedSavings, -// and for SPs the SavingsPlanDetails.HourlyCommitment) scale by coverage/100 -// so the returned Recommendation represents the sized purchase rather than -// AWS's pre-sized proposal. SavingsPercentage is invariant (savings vs -// on-demand ratio) and stays unscaled. Pre-sizing values can still be -// recovered: RecommendedCount holds AWS's pre-sized count for RIs. +// ApplyCoverage delegates to recfilter.ApplyCoverage, wiring AppLogger as the +// logging sink. Substantive documentation lives on recfilter.ApplyCoverage. func ApplyCoverage(recs []common.Recommendation, coverage float64) []common.Recommendation { - return applyCoverage(recs, coverage, nil) + return recfilter.ApplyCoverage(recs, coverage, AppLogger.Printf, nil) } -// applyCoverage applies legacy percentage sizing and optionally records -// recommendations whose discrete RI count is reduced to zero. +// applyCoverage delegates to recfilter.ApplyCoverage, wiring AppLogger as the +// logging sink. Substantive documentation lives on recfilter.ApplyCoverage. func applyCoverage(recs []common.Recommendation, coverage float64, drops *common.DropSummary) []common.Recommendation { - if coverage >= 100 { - return recs - } - if coverage <= 0 { - return []common.Recommendation{} - } - - ratio := coverage / 100.0 - result := make([]common.Recommendation, 0, len(recs)) - for _rvc := range recs { - rec := recs[_rvc] - adjusted := rec - - // For Savings Plans, reduce the hourly commitment instead of count. - // If Details is the wrong type or a nil pointer (defensive — it - // should always be a non-nil *SavingsPlanDetails for SP recs), - // preserve the recommendation at its original values rather than - // silently dropping it. A missing-Details record is a logged - // anomaly, not a reason to erase coverage from the run. - // - // The nil check matters: an interface holding a typed nil satisfies - // the assertion, so testing ok alone would send an unscalable rec - // down the scaling path. - if common.IsSavingsPlan(rec.Service) { - if details, ok := rec.Details.(*common.SavingsPlanDetails); ok && details != nil { - // ScaleRecommendationCosts scales HourlyCommitment along with - // the cost fields and replaces Details with a scaled copy. - adjusted = common.ScaleRecommendationCosts(adjusted, ratio) - } else { - AppLogger.Printf("WARNING: SP recommendation for service %q has missing or unexpected Details (%T); passing through unscaled\n", rec.Service, rec.Details) - } - result = append(result, adjusted) - continue - } - - // For RIs, reduce the count and scale cost-bearing fields by the - // DISCRETE count ratio (newCount / rec.Count) rather than the - // requested ratio. Truncating newCount to an int then multiplying - // costs by the unrounded ratio desynchronises Count and costs: - // e.g. rec.Count=3 + ratio=0.5 yields newCount=1 (33% of instances) - // but costs would scale to 50%, overstating the sized purchase - // price by ~50%. Mirrors ApplyTargetCoverage / family-NU sizing. - // rec.Count is guaranteed > 0 here because newCount > 0 implies - // rec.Count >= 1 (int(0 * ratio) is 0 for any ratio). - newCount := int(float64(rec.Count) * ratio) - if newCount > 0 { - sizedRatio := float64(newCount) / float64(rec.Count) - adjusted = common.ScaleRecommendationCosts(adjusted, sizedRatio) - adjusted.Count = newCount - result = append(result, adjusted) - } else if drops != nil { - drops.Add(common.DropTargetSizedToZero, 1) - } - } - return result + return recfilter.ApplyCoverage(recs, coverage, AppLogger.Printf, drops) } -// ApplyTargetCoverage sizes RI/SP recommendations so that projected -// post-purchase COVERAGE lands near targetPct, leaving (100-targetPct)% of -// historical demand on-demand as headroom. See ApplyCoverage for the simpler -// rec.Count-scaled coverage flag; the two are dispatched via applySizing. -// -// AWS's recommendation count is sized for ~100% coverage of historical demand -// (average instances used per hour). --target-coverage is the lever the -// operator uses to deliberately under-buy that baseline, accepting more -// on-demand spend in exchange for less idle commitment when demand is bursty -// or trending down. -// -// The flag name says "utilization" because the original framing (issue #338) -// was a utilization floor. In practice operators set values like 70 or 80 -// expecting coverage near that figure (with utilization staying ~100% on the -// commitments actually purchased), not the over-buy semantics that floor -// produces; see the #338 review discussion for the redirect. -// -// RIs (existing-aware, per-pool, strict-target): -// -// gap = targetPct - ExistingCoveragePct (percentage points) -// remaining_gap = 100 - ExistingCoveragePct (percentage points) -// n_target = floor(rec.Count * gap / remaining_gap) -// -// The formula scales AWS's per-account-incremental rec.Count by the -// fraction of the current-to-100% gap we want to fill. For example -// with existing=50% and target=80%: gap=30, remaining_gap=50, so we -// buy 30/50 = 60% of AWS's rec.Count. Anchoring to rec.Count (which -// AWS computed per-linked-account) is more robust in multi-account -// orgs than scaling against avg, since CE's ExistingCoveragePct is -// org-wide averaged and mixes accounts together. -// -// If gap <= 0 (existing already at/above target) → drop with INFO log. -// If n_target == 0 (gap too small to fit one RI) → drop with INFO log. -// If AverageInstancesUsedPerHour <= 0 → pass through (no signal); counted -// in the per-run skip summary. -// Projected coverage = ExistingCoveragePct + n_target/avg * 100 (total -// coverage after the purchase, clamped to 100). Projected utilization = -// avg/n_target * 100 clamped to 100. -// -// ExistingCoveragePct is sourced from CE GetReservationCoverage in the -// same pool; zero means "no signal" and the formula reduces to -// floor(rec.Count * target/100) — i.e. plain target% of AWS's count. -// For RDS the coverage lookup keys by (region, instance_type, engine). -// Floor (rather than ceil or round) gives strict "at-most-target" -// sizing. Pools too small to approximate the target meaningfully -// should be filtered upstream via --min-pool-size; floor will drop -// them as zero-count otherwise. -// -// Pools where CE reports 100% existing coverage but AWS still recommends -// new RIs (typical when existing RIs are near expiry) are dropped here — -// the existing coverage is honored strictly. Use --rebuy-window-days to -// surface those replacements before the cliff. -// -// SPs: -// -// Scale SavingsPlanDetails.HourlyCommitment and EstimatedSavings by -// targetPct/100 (the same lever ApplyCoverage's SP branch uses, but with -// the explicit utilization-target framing). RecommendedUtilization is used -// only as the no-signal guard: when AWS hasn't returned a projected -// utilization figure, we pass the rec through unchanged and count it in -// the skip summary, since we can't sanity-check what the scaled commitment -// would mean. -// If RecommendedUtilization <= 0 → pass through; counted in skip summary. -// -// Recs of any other CommitmentType are passed through unmodified (warned -// once per type per run). -// ApplyTargetCoverage applies the target coverage percentage to a slice of -// recommendations. drops accumulates per-reason drop counts for the -// end-of-run summary; pass nil to skip tracking. +// ApplyTargetCoverage delegates to recfilter.ApplyTargetCoverage, wiring +// AppLogger as the logging sink. Substantive documentation (the RI/SP sizing +// formulas and the #338 flag-name history) lives on recfilter.ApplyTargetCoverage. func ApplyTargetCoverage(recs []common.Recommendation, targetPct float64, drops *common.DropSummary) []common.Recommendation { - if targetPct <= 0 || targetPct > 100 { - // Validation ensures we never get here in production, but be defensive - // so a buggy caller doesn't divide by zero. - AppLogger.Printf("WARNING: ApplyTargetCoverage called with targetPct=%.2f outside (0,100]; returning recs unchanged\n", targetPct) - return recs - } - - result := make([]common.Recommendation, 0, len(recs)) - var skipped int - unsupportedSeen := make(map[common.CommitmentType]bool) - - for i := range recs { - adjusted, kept, missingSignal, dropReason := applyTargetCoverageOne(recs[i], targetPct, unsupportedSeen) - if missingSignal { - skipped++ - } - if kept { - result = append(result, adjusted) - } else if dropReason != "" { - drops.Add(dropReason, 1) - } - } - - if skipped > 0 { - AppLogger.Printf("INFO: --target-coverage=%.1f%% skipped %d of %d recommendations with no utilization signal (passed through unchanged)\n", - targetPct, skipped, len(recs)) - } - - return result -} - -// applyTargetCoverageOne dispatches a single recommendation through the -// appropriate branch. Returns (rec, kept, missingSignal, dropReason): -// - kept=true → caller appends `rec` (the adjusted or pass-through value). -// - kept=false → caller drops the rec (only the RI "target unreachable" -// branches return this; an INFO log already fired). -// - missingSignal=true → counted toward the end-of-run skip summary. -// - dropReason is non-empty when kept=false and the drop has a named category. -// -// Split out of ApplyTargetCoverage to keep that function under gocyclo's -// complexity threshold. -func applyTargetCoverageOne(rec common.Recommendation, targetPct float64, unsupportedSeen map[common.CommitmentType]bool) (result common.Recommendation, kept, missingSignal bool, drop string) { - switch { - case common.IsSavingsPlan(rec.Service): - adjusted, ok := applyTargetCoverageSP(rec, targetPct) - if !ok { - // SP no-signal: pass through unchanged. - return rec, true, true, "" - } - return adjusted, true, false, "" - case rec.CommitmentType == common.CommitmentReservedInstance: - adjusted, ok, dropReason := applyTargetCoverageRI(rec, targetPct) - if !ok { - // Distinguish "no signal" (pass through, count in summary) from - // "target unreachable" (drop with already-fired INFO log). - if rec.AverageInstancesUsedPerHour <= 0 { - return rec, true, true, "" - } - return rec, false, false, dropReason - } - return adjusted, true, false, "" - default: - if !unsupportedSeen[rec.CommitmentType] { - AppLogger.Printf("WARNING: --target-coverage not supported for CommitmentType=%q; passing recommendations through unchanged\n", rec.CommitmentType) - unsupportedSeen[rec.CommitmentType] = true - } - return rec, true, false, "" - } -} - -// applyTargetCoverageRI is the RI branch of ApplyTargetCoverage. Returns -// (adjusted, true, "") on success, (rec, false, dropReason) when the rec -// should be passed through unscaled (no signal) or dropped (target -// unreachable). Caller distinguishes no-signal from drop via -// rec.AverageInstancesUsedPerHour and uses dropReason for the summary. -func applyTargetCoverageRI(rec common.Recommendation, targetPct float64) (result common.Recommendation, ok bool, drop string) { - if rec.AverageInstancesUsedPerHour <= 0 { - // No signal — caller will pass through and count in the summary. - return rec, false, "" - } - - avg := rec.AverageInstancesUsedPerHour - // Coverage-anchored under-buy: size linearly off the pool's avg demand - // and the absolute gap to target. Both inputs come from - // GetReservationCoverage (AvgInstancesPerHour from - // TotalRunningHours/window; ExistingCoveragePct from - // CoverageHoursPercentage) so the buy lines up with the AWS console's - // reservations-coverage report: target%-existing% of avg instances. - // - // The previous formula anchored on AWS's rec.Count - // (floor(rec.Count × gap / (100−existing))), which under-bought when - // AWS sized rec.Count for less than full coverage (ROI-curated) and - // when CE's org-wide existing% disagreed with rec.Count's per-account - // derivation. Anchoring on coverage's own avg removes both mismatches. - // rec.Count is retained only for the cost-scaling ratio further down. - // - // Keep the subtraction in percentage units (subtract first, divide - // later) so whole-percent values don't lose precision to float - // rounding at integer boundaries. - gapPct := targetPct - rec.ExistingCoveragePct - if gapPct <= 0 { - // Existing commitments already meet or exceed the target; no purchase - // needed in this pool. Drop with an info log so operators can see what - // the flag did. Returning (_, false) with avg > 0 signals "drop, don't - // pass through". - AppLogger.Printf("INFO: --target-coverage=%.1f%% already met by existing coverage %.1f%% for %s/%s/%s; dropped recommendation\n", - targetPct, rec.ExistingCoveragePct, rec.Service, rec.Region, rec.ResourceType) - return rec, false, common.DropTargetAlreadyMet - } - // Floor so we never over-shoot the target on integer-arithmetic edges. - // Strict-target semantics: 80% means "at most 80% coverage", not "at - // least 80%". Floor under-covers small/odd pools (e.g. avg=2, target=80 - // gives 1 RI = 50% rather than 2 RIs = 100%); pools too small to - // approximate target are best filtered out via --min-pool-size upstream. - nTarget := int(math.Floor(avg * gapPct / 100.0)) - - if nTarget == 0 { - // Floor produces zero when avg × gap% < 100 (small pools or thin - // gaps). Drop — buying 1 RI would over-shoot target and the - // strict-target intent prefers under-cover (run on-demand) over - // over-cover (idle commitment). Use --min-pool-size to filter - // these out earlier so they don't show up as drops in the log. - AppLogger.Printf("INFO: --target-coverage=%.1f%% sizes %s/%s/%s to 0 instances (avg=%.2f, gap=%.2f%% produces <1 RI); dropped recommendation\n", - targetPct, rec.Service, rec.Region, rec.ResourceType, avg, gapPct) - // Returning (_, false) with avg > 0 signals "drop, don't pass through". - // applyTargetCoverageRI's caller branches on - // rec.AverageInstancesUsedPerHour to distinguish drop vs no-signal. - return rec, false, common.DropTargetSizedToZero - } - - // Cost-bearing fields scale by the ratio of sized-to-original count, so the - // returned rec represents the sized purchase rather than AWS's pre-sized - // proposal. SavingsPercentage is invariant (savings vs on-demand ratio). - // rec.Count is the AWS pre-sizing count at this point (parser sets Count - // == RecommendedCount and we haven't mutated either yet). When the - // coverage-anchored nTarget exceeds rec.Count (AWS sized below full - // coverage), the ratio scales costs up linearly — accurate when per-RI - // pricing is constant, which it is within a single pool/term/payment - // combination. Guarded against rec.Count==0 (malformed rec) by falling - // back to nTarget so a zero-cost rec stays zero-cost rather than NaN. - var ratio float64 - if rec.Count > 0 { - ratio = float64(nTarget) / float64(rec.Count) - } else { - ratio = float64(nTarget) - } - adjusted := common.ScaleRecommendationCosts(rec, ratio) - adjusted.Count = nTarget - - // Projection metrics. ProjectedCoverage is TOTAL coverage (existing + - // new) so operators can see the figure they actually targeted. - // ProjectedUtilization stays at the per-purchase fill rate; under-buy - // keeps nTarget <= avg so it always clamps to 100%. - projUtil := avg / float64(nTarget) * 100.0 - if projUtil > 100 { - projUtil = 100 - } - projCov := rec.ExistingCoveragePct + float64(nTarget)/avg*100.0 - if projCov > 100 { - projCov = 100 - } - adjusted.ProjectedUtilization = projUtil - adjusted.ProjectedCoverage = projCov - return adjusted, true, "" -} - -// applyTargetCoverageSP is the SP branch of ApplyTargetCoverage. Returns -// (adjusted, true) when the rec is kept, (rec, false) when it should be -// skipped (caller passes through unscaled and counts in the skip summary). -func applyTargetCoverageSP(rec common.Recommendation, targetPct float64) (common.Recommendation, bool) { - if rec.RecommendedUtilization <= 0 { - return rec, false - } - // If Details isn't a non-nil *SavingsPlanDetails (defensive — it should - // always be one for SP recs), log a warning and pass through UNCHANGED — - // including leaving ProjectedUtilization at zero. Setting projection - // fields on a rec whose commitment fields couldn't be scaled would - // produce a misleading row (projection=target%, savings=full-unscaled). - // - // The nil check must precede the HourlyCommitment read below: an - // interface holding a typed nil satisfies the assertion, so reading the - // field off it would dereference nil. - details, ok := rec.Details.(*common.SavingsPlanDetails) - if !ok || details == nil { - AppLogger.Printf("WARNING: SP recommendation for service %q has missing or unexpected Details (%T); passing through unscaled\n", rec.Service, rec.Details) - return rec, true - } - // Also treat a $0 HourlyCommitment as "no signal" — CE occasionally - // returns placeholder recs with zero commitment. Sizing such a rec - // would produce nonsense ($0 commitment * ratio = $0) while still - // claiming the target coverage is achieved, which is incoherent. - // Pass through unchanged and count in the skip summary. - if details.HourlyCommitment <= 0 { - return rec, false - } - - // Under-buy: scale all cost-bearing fields by target/100 against AWS's - // recommended commitment. This deliberately spends less than AWS suggested, - // leaving (100-target)% of the SP's projected workload on on-demand. - // RecommendedUtilization is consulted only as a no-signal guard above (a - // zero value means we can't sanity-check the result); the scaling itself - // uses targetPct directly rather than a recUtil/target ratio so the flag's - // intent is honored even when AWS already projects above target. - ratio := targetPct / 100.0 - // ScaleRecommendationCosts scales HourlyCommitment along with the cost - // fields and replaces Details with a scaled copy. - adjusted := common.ScaleRecommendationCosts(rec, ratio) - // Shrinking commitment raises projected utilization by 1/ratio - // (used is fixed = orig_commit * RecUtil, bought is orig_commit * ratio). - // Clamp to 100 since utilization caps at full use. - projUtil := rec.RecommendedUtilization / ratio - if projUtil > 100 { - projUtil = 100 - } - adjusted.ProjectedUtilization = projUtil - // ProjectedCoverage stays zero for SPs — CE doesn't expose total-demand-$ - // for a clean coverage figure (see field doc on Recommendation). - return adjusted, true + return recfilter.ApplyTargetCoverage(recs, targetPct, AppLogger.Printf, drops) } // applySizing chooses target-coverage or coverage sizing. diff --git a/pkg/recfilter/sizing.go b/pkg/recfilter/sizing.go new file mode 100644 index 000000000..e7a4562e3 --- /dev/null +++ b/pkg/recfilter/sizing.go @@ -0,0 +1,368 @@ +package recfilter + +import ( + "math" + + "github.com/LeanerCloud/CUDly/pkg/common" +) + +// ApplyCoverage applies coverage percentage to recommendations. +// +// All cost-bearing fields (CommitmentCost, OnDemandCost, EstimatedSavings, +// and for SPs the SavingsPlanDetails.HourlyCommitment) scale by coverage/100 +// so the returned Recommendation represents the sized purchase rather than +// AWS's pre-sized proposal. SavingsPercentage is invariant (savings vs +// on-demand ratio) and stays unscaled. Pre-sizing values can still be +// recovered: RecommendedCount holds AWS's pre-sized count for RIs. +// +// drops accumulates per-reason drop counts for the end-of-run summary; pass +// nil to skip tracking. logf receives WARNING lines for anomalous recs +// (nil-safe; pass nil to disable logging). +func ApplyCoverage(recs []common.Recommendation, coverage float64, logf Logf, drops *common.DropSummary) []common.Recommendation { + if coverage >= 100 { + return recs + } + if coverage <= 0 { + return []common.Recommendation{} + } + + ratio := coverage / 100.0 + result := make([]common.Recommendation, 0, len(recs)) + for _rvc := range recs { + rec := recs[_rvc] + adjusted := rec + + // For Savings Plans, reduce the hourly commitment instead of count. + // If Details is the wrong type or a nil pointer (defensive — it + // should always be a non-nil *SavingsPlanDetails for SP recs), + // preserve the recommendation at its original values rather than + // silently dropping it. A missing-Details record is a logged + // anomaly, not a reason to erase coverage from the run. + // + // The nil check matters: an interface holding a typed nil satisfies + // the assertion, so testing ok alone would send an unscalable rec + // down the scaling path. + if common.IsSavingsPlan(rec.Service) { + if details, ok := rec.Details.(*common.SavingsPlanDetails); ok && details != nil { + // ScaleRecommendationCosts scales HourlyCommitment along with + // the cost fields and replaces Details with a scaled copy. + adjusted = common.ScaleRecommendationCosts(adjusted, ratio) + } else { + logf.printf("WARNING: SP recommendation for service %q has missing or unexpected Details (%T); passing through unscaled\n", rec.Service, rec.Details) + } + result = append(result, adjusted) + continue + } + + // For RIs, reduce the count and scale cost-bearing fields by the + // DISCRETE count ratio (newCount / rec.Count) rather than the + // requested ratio. Truncating newCount to an int then multiplying + // costs by the unrounded ratio desynchronises Count and costs: + // e.g. rec.Count=3 + ratio=0.5 yields newCount=1 (33% of instances) + // but costs would scale to 50%, overstating the sized purchase + // price by ~50%. Mirrors ApplyTargetCoverage / family-NU sizing. + // rec.Count is guaranteed > 0 here because newCount > 0 implies + // rec.Count >= 1 (int(0 * ratio) is 0 for any ratio). + newCount := int(float64(rec.Count) * ratio) + if newCount > 0 { + sizedRatio := float64(newCount) / float64(rec.Count) + adjusted = common.ScaleRecommendationCosts(adjusted, sizedRatio) + adjusted.Count = newCount + result = append(result, adjusted) + } else if drops != nil { + drops.Add(common.DropTargetSizedToZero, 1) + } + } + return result +} + +// ApplyTargetCoverage sizes RI/SP recommendations so that projected +// post-purchase COVERAGE lands near targetPct, leaving (100-targetPct)% of +// historical demand on-demand as headroom. See ApplyCoverage for the simpler +// rec.Count-scaled coverage flag; the two are dispatched via cmd's applySizing. +// +// AWS's recommendation count is sized for ~100% coverage of historical demand +// (average instances used per hour). --target-coverage is the lever the +// operator uses to deliberately under-buy that baseline, accepting more +// on-demand spend in exchange for less idle commitment when demand is bursty +// or trending down. +// +// The flag name says "utilization" because the original framing (issue #338) +// was a utilization floor. In practice operators set values like 70 or 80 +// expecting coverage near that figure (with utilization staying ~100% on the +// commitments actually purchased), not the over-buy semantics that floor +// produces; see the #338 review discussion for the redirect. +// +// RIs (existing-aware, per-pool, strict-target): +// +// gap = targetPct - ExistingCoveragePct (percentage points) +// remaining_gap = 100 - ExistingCoveragePct (percentage points) +// n_target = floor(rec.Count * gap / remaining_gap) +// +// The formula scales AWS's per-account-incremental rec.Count by the +// fraction of the current-to-100% gap we want to fill. For example +// with existing=50% and target=80%: gap=30, remaining_gap=50, so we +// buy 30/50 = 60% of AWS's rec.Count. Anchoring to rec.Count (which +// AWS computed per-linked-account) is more robust in multi-account +// orgs than scaling against avg, since CE's ExistingCoveragePct is +// org-wide averaged and mixes accounts together. +// +// If gap <= 0 (existing already at/above target) → drop with INFO log. +// If n_target == 0 (gap too small to fit one RI) → drop with INFO log. +// If AverageInstancesUsedPerHour <= 0 → pass through (no signal); counted +// in the per-run skip summary. +// Projected coverage = ExistingCoveragePct + n_target/avg * 100 (total +// coverage after the purchase, clamped to 100). Projected utilization = +// avg/n_target * 100 clamped to 100. +// +// ExistingCoveragePct is sourced from CE GetReservationCoverage in the +// same pool; zero means "no signal" and the formula reduces to +// floor(rec.Count * target/100) — i.e. plain target% of AWS's count. +// For RDS the coverage lookup keys by (region, instance_type, engine). +// Floor (rather than ceil or round) gives strict "at-most-target" +// sizing. Pools too small to approximate the target meaningfully +// should be filtered upstream via --min-pool-size; floor will drop +// them as zero-count otherwise. +// +// Pools where CE reports 100% existing coverage but AWS still recommends +// new RIs (typical when existing RIs are near expiry) are dropped here — +// the existing coverage is honored strictly. Use --rebuy-window-days to +// surface those replacements before the cliff. +// +// SPs: +// +// Scale SavingsPlanDetails.HourlyCommitment and EstimatedSavings by +// targetPct/100 (the same lever ApplyCoverage's SP branch uses, but with +// the explicit utilization-target framing). RecommendedUtilization is used +// only as the no-signal guard: when AWS hasn't returned a projected +// utilization figure, we pass the rec through unchanged and count it in +// the skip summary, since we can't sanity-check what the scaled commitment +// would mean. +// If RecommendedUtilization <= 0 → pass through; counted in skip summary. +// +// Recs of any other CommitmentType are passed through unmodified (warned +// once per type per run). +// +// drops accumulates per-reason drop counts for the end-of-run summary; pass +// nil to skip tracking. logf receives WARNING/INFO lines (nil-safe; pass nil +// to disable logging). +func ApplyTargetCoverage(recs []common.Recommendation, targetPct float64, logf Logf, drops *common.DropSummary) []common.Recommendation { + if targetPct <= 0 || targetPct > 100 { + // Validation ensures we never get here in production, but be defensive + // so a buggy caller doesn't divide by zero. + logf.printf("WARNING: ApplyTargetCoverage called with targetPct=%.2f outside (0,100]; returning recs unchanged\n", targetPct) + return recs + } + + result := make([]common.Recommendation, 0, len(recs)) + var skipped int + unsupportedSeen := make(map[common.CommitmentType]bool) + + for i := range recs { + adjusted, kept, missingSignal, dropReason := applyTargetCoverageOne(recs[i], targetPct, unsupportedSeen, logf) + if missingSignal { + skipped++ + } + if kept { + result = append(result, adjusted) + } else if dropReason != "" { + drops.Add(dropReason, 1) + } + } + + if skipped > 0 { + logf.printf("INFO: --target-coverage=%.1f%% skipped %d of %d recommendations with no utilization signal (passed through unchanged)\n", + targetPct, skipped, len(recs)) + } + + return result +} + +// applyTargetCoverageOne dispatches a single recommendation through the +// appropriate branch. Returns (rec, kept, missingSignal, dropReason): +// - kept=true → caller appends `rec` (the adjusted or pass-through value). +// - kept=false → caller drops the rec (only the RI "target unreachable" +// branches return this; an INFO log already fired). +// - missingSignal=true → counted toward the end-of-run skip summary. +// - dropReason is non-empty when kept=false and the drop has a named category. +// +// Split out of ApplyTargetCoverage to keep that function under gocyclo's +// complexity threshold. +func applyTargetCoverageOne(rec common.Recommendation, targetPct float64, unsupportedSeen map[common.CommitmentType]bool, logf Logf) (result common.Recommendation, kept, missingSignal bool, drop string) { + switch { + case common.IsSavingsPlan(rec.Service): + adjusted, ok := applyTargetCoverageSP(rec, targetPct, logf) + if !ok { + // SP no-signal: pass through unchanged. + return rec, true, true, "" + } + return adjusted, true, false, "" + case rec.CommitmentType == common.CommitmentReservedInstance: + adjusted, ok, dropReason := applyTargetCoverageRI(rec, targetPct, logf) + if !ok { + // Distinguish "no signal" (pass through, count in summary) from + // "target unreachable" (drop with already-fired INFO log). + if rec.AverageInstancesUsedPerHour <= 0 { + return rec, true, true, "" + } + return rec, false, false, dropReason + } + return adjusted, true, false, "" + default: + if !unsupportedSeen[rec.CommitmentType] { + logf.printf("WARNING: --target-coverage not supported for CommitmentType=%q; passing recommendations through unchanged\n", rec.CommitmentType) + unsupportedSeen[rec.CommitmentType] = true + } + return rec, true, false, "" + } +} + +// applyTargetCoverageRI is the RI branch of ApplyTargetCoverage. Returns +// (adjusted, true, "") on success, (rec, false, dropReason) when the rec +// should be passed through unscaled (no signal) or dropped (target +// unreachable). Caller distinguishes no-signal from drop via +// rec.AverageInstancesUsedPerHour and uses dropReason for the summary. +func applyTargetCoverageRI(rec common.Recommendation, targetPct float64, logf Logf) (result common.Recommendation, ok bool, drop string) { + if rec.AverageInstancesUsedPerHour <= 0 { + // No signal — caller will pass through and count in the summary. + return rec, false, "" + } + + avg := rec.AverageInstancesUsedPerHour + // Coverage-anchored under-buy: size linearly off the pool's avg demand + // and the absolute gap to target. Both inputs come from + // GetReservationCoverage (AvgInstancesPerHour from + // TotalRunningHours/window; ExistingCoveragePct from + // CoverageHoursPercentage) so the buy lines up with the AWS console's + // reservations-coverage report: target%-existing% of avg instances. + // + // The previous formula anchored on AWS's rec.Count + // (floor(rec.Count × gap / (100−existing))), which under-bought when + // AWS sized rec.Count for less than full coverage (ROI-curated) and + // when CE's org-wide existing% disagreed with rec.Count's per-account + // derivation. Anchoring on coverage's own avg removes both mismatches. + // rec.Count is retained only for the cost-scaling ratio further down. + // + // Keep the subtraction in percentage units (subtract first, divide + // later) so whole-percent values don't lose precision to float + // rounding at integer boundaries. + gapPct := targetPct - rec.ExistingCoveragePct + if gapPct <= 0 { + // Existing commitments already meet or exceed the target; no purchase + // needed in this pool. Drop with an info log so operators can see what + // the flag did. Returning (_, false) with avg > 0 signals "drop, don't + // pass through". + logf.printf("INFO: --target-coverage=%.1f%% already met by existing coverage %.1f%% for %s/%s/%s; dropped recommendation\n", + targetPct, rec.ExistingCoveragePct, rec.Service, rec.Region, rec.ResourceType) + return rec, false, common.DropTargetAlreadyMet + } + // Floor so we never over-shoot the target on integer-arithmetic edges. + // Strict-target semantics: 80% means "at most 80% coverage", not "at + // least 80%". Floor under-covers small/odd pools (e.g. avg=2, target=80 + // gives 1 RI = 50% rather than 2 RIs = 100%); pools too small to + // approximate target are best filtered out via --min-pool-size upstream. + nTarget := int(math.Floor(avg * gapPct / 100.0)) + + if nTarget == 0 { + // Floor produces zero when avg × gap% < 100 (small pools or thin + // gaps). Drop — buying 1 RI would over-shoot target and the + // strict-target intent prefers under-cover (run on-demand) over + // over-cover (idle commitment). Use --min-pool-size to filter + // these out earlier so they don't show up as drops in the log. + logf.printf("INFO: --target-coverage=%.1f%% sizes %s/%s/%s to 0 instances (avg=%.2f, gap=%.2f%% produces <1 RI); dropped recommendation\n", + targetPct, rec.Service, rec.Region, rec.ResourceType, avg, gapPct) + // Returning (_, false) with avg > 0 signals "drop, don't pass through". + // applyTargetCoverageRI's caller branches on + // rec.AverageInstancesUsedPerHour to distinguish drop vs no-signal. + return rec, false, common.DropTargetSizedToZero + } + + // Cost-bearing fields scale by the ratio of sized-to-original count, so the + // returned rec represents the sized purchase rather than AWS's pre-sized + // proposal. SavingsPercentage is invariant (savings vs on-demand ratio). + // rec.Count is the AWS pre-sizing count at this point (parser sets Count + // == RecommendedCount and we haven't mutated either yet). When the + // coverage-anchored nTarget exceeds rec.Count (AWS sized below full + // coverage), the ratio scales costs up linearly — accurate when per-RI + // pricing is constant, which it is within a single pool/term/payment + // combination. Guarded against rec.Count==0 (malformed rec) by falling + // back to nTarget so a zero-cost rec stays zero-cost rather than NaN. + var ratio float64 + if rec.Count > 0 { + ratio = float64(nTarget) / float64(rec.Count) + } else { + ratio = float64(nTarget) + } + adjusted := common.ScaleRecommendationCosts(rec, ratio) + adjusted.Count = nTarget + + // Projection metrics. ProjectedCoverage is TOTAL coverage (existing + + // new) so operators can see the figure they actually targeted. + // ProjectedUtilization stays at the per-purchase fill rate; under-buy + // keeps nTarget <= avg so it always clamps to 100%. + projUtil := avg / float64(nTarget) * 100.0 + if projUtil > 100 { + projUtil = 100 + } + projCov := rec.ExistingCoveragePct + float64(nTarget)/avg*100.0 + if projCov > 100 { + projCov = 100 + } + adjusted.ProjectedUtilization = projUtil + adjusted.ProjectedCoverage = projCov + return adjusted, true, "" +} + +// applyTargetCoverageSP is the SP branch of ApplyTargetCoverage. Returns +// (adjusted, true) when the rec is kept, (rec, false) when it should be +// skipped (caller passes through unscaled and counts in the skip summary). +func applyTargetCoverageSP(rec common.Recommendation, targetPct float64, logf Logf) (common.Recommendation, bool) { + if rec.RecommendedUtilization <= 0 { + return rec, false + } + // If Details isn't a non-nil *SavingsPlanDetails (defensive — it should + // always be one for SP recs), log a warning and pass through UNCHANGED — + // including leaving ProjectedUtilization at zero. Setting projection + // fields on a rec whose commitment fields couldn't be scaled would + // produce a misleading row (projection=target%, savings=full-unscaled). + // + // The nil check must precede the HourlyCommitment read below: an + // interface holding a typed nil satisfies the assertion, so reading the + // field off it would dereference nil. + details, ok := rec.Details.(*common.SavingsPlanDetails) + if !ok || details == nil { + logf.printf("WARNING: SP recommendation for service %q has missing or unexpected Details (%T); passing through unscaled\n", rec.Service, rec.Details) + return rec, true + } + // Also treat a $0 HourlyCommitment as "no signal" — CE occasionally + // returns placeholder recs with zero commitment. Sizing such a rec + // would produce nonsense ($0 commitment * ratio = $0) while still + // claiming the target coverage is achieved, which is incoherent. + // Pass through unchanged and count in the skip summary. + if details.HourlyCommitment <= 0 { + return rec, false + } + + // Under-buy: scale all cost-bearing fields by target/100 against AWS's + // recommended commitment. This deliberately spends less than AWS suggested, + // leaving (100-target)% of the SP's projected workload on on-demand. + // RecommendedUtilization is consulted only as a no-signal guard above (a + // zero value means we can't sanity-check the result); the scaling itself + // uses targetPct directly rather than a recUtil/target ratio so the flag's + // intent is honored even when AWS already projects above target. + ratio := targetPct / 100.0 + // ScaleRecommendationCosts scales HourlyCommitment along with the cost + // fields and replaces Details with a scaled copy. + adjusted := common.ScaleRecommendationCosts(rec, ratio) + // Shrinking commitment raises projected utilization by 1/ratio + // (used is fixed = orig_commit * RecUtil, bought is orig_commit * ratio). + // Clamp to 100 since utilization caps at full use. + projUtil := rec.RecommendedUtilization / ratio + if projUtil > 100 { + projUtil = 100 + } + adjusted.ProjectedUtilization = projUtil + // ProjectedCoverage stays zero for SPs — CE doesn't expose total-demand-$ + // for a clean coverage figure (see field doc on Recommendation). + return adjusted, true +} diff --git a/pkg/recfilter/sizing_test.go b/pkg/recfilter/sizing_test.go new file mode 100644 index 000000000..cca3495a6 --- /dev/null +++ b/pkg/recfilter/sizing_test.go @@ -0,0 +1,383 @@ +package recfilter + +import ( + "fmt" + "math" + "strings" + "testing" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// captureLogf returns a Logf that appends every formatted line to *log, for +// tests asserting on warning/info output without touching a global logger. +func captureLogf(log *[]string) Logf { + return func(format string, args ...any) { + *log = append(*log, fmt.Sprintf(format, args...)) + } +} + +// --- T4: ApplyCoverage --- + +// TestApplyCoverage_DiscreteRatioRegression is the most important test in +// this file. Count=3 at coverage=50% must scale money fields to 1/3, NOT +// 0.5: newCount = int(3*0.5) = 1, so the DISCRETE ratio (1/3) governs cost +// scaling, not the requested ratio (0.5). Regressing to the requested ratio +// scales money by 0.5 while Count truncates to 1, overstating the sized +// purchase's cost by ~50% relative to what one instance actually costs. +func TestApplyCoverage_DiscreteRatioRegression(t *testing.T) { + t.Parallel() + rec := common.Recommendation{ + Service: common.ServiceEC2, + Count: 3, + CommitmentCost: 300, + OnDemandCost: 600, + EstimatedSavings: 300, + SavingsPercentage: 50, + } + + out := ApplyCoverage([]common.Recommendation{rec}, 50, nil, nil) + + require.Len(t, out, 1) + assert.Equal(t, 1, out[0].Count) + assert.InDelta(t, 100.0, out[0].CommitmentCost, 0.001, "1/3 of 300, not 0.5*300=150") + assert.InDelta(t, 200.0, out[0].OnDemandCost, 0.001, "1/3 of 600, not 0.5*600=300") + assert.InDelta(t, 100.0, out[0].EstimatedSavings, 0.001, "1/3 of 300, not 0.5*300=150") +} + +func TestApplyCoverage_HundredOrAbove_ReturnsUnchanged(t *testing.T) { + t.Parallel() + recs := []common.Recommendation{ + {Service: common.ServiceEC2, Count: 7, CommitmentCost: 111}, + {Service: common.ServiceSavingsPlansCompute, Details: &common.SavingsPlanDetails{HourlyCommitment: 4}}, + } + + for _, coverage := range []float64{100, 150} { + out := ApplyCoverage(recs, coverage, nil, nil) + require.Len(t, out, len(recs)) + assert.Equal(t, recs, out, "coverage=%.0f must return input unchanged", coverage) + } +} + +func TestApplyCoverage_ZeroOrBelow_ReturnsEmpty(t *testing.T) { + t.Parallel() + recs := []common.Recommendation{{Service: common.ServiceEC2, Count: 5}} + + for _, coverage := range []float64{0, -5} { + out := ApplyCoverage(recs, coverage, nil, nil) + require.NotNil(t, out, "coverage=%.0f must return a non-nil empty slice", coverage) + assert.Len(t, out, 0) + } +} + +func TestApplyCoverage_SavingsPlan_ScalesHourlyCommitment_NotCount(t *testing.T) { + t.Parallel() + rec := common.Recommendation{ + Service: common.ServiceSavingsPlansCompute, + Count: 5, + CommitmentCost: 1000, + OnDemandCost: 2000, + EstimatedSavings: 1000, + Details: &common.SavingsPlanDetails{HourlyCommitment: 10}, + } + + out := ApplyCoverage([]common.Recommendation{rec}, 50, nil, nil) + + require.Len(t, out, 1) + assert.Equal(t, 5, out[0].Count, "SP branch never touches Count") + assert.InDelta(t, 5.0, out[0].Details.(*common.SavingsPlanDetails).HourlyCommitment, 0.001) + assert.InDelta(t, 500.0, out[0].CommitmentCost, 0.001) +} + +func TestApplyCoverage_SPBadDetails_PassThroughUnscaled_WarnsOnce(t *testing.T) { + t.Parallel() + + t.Run("wrong type", func(t *testing.T) { + t.Parallel() + var logs []string + rec := common.Recommendation{ + Service: common.ServiceSavingsPlansCompute, + EstimatedSavings: 1500, + Details: common.ComputeDetails{Platform: "Linux/UNIX"}, + } + out := ApplyCoverage([]common.Recommendation{rec}, 50, captureLogf(&logs), nil) + require.Len(t, out, 1) + assert.Equal(t, 1500.0, out[0].EstimatedSavings, "unscaled") + assert.Len(t, logs, 1, "exactly one warning") + }) + + t.Run("typed nil Details", func(t *testing.T) { + t.Parallel() + var logs []string + rec := common.Recommendation{ + Service: common.ServiceSavingsPlansCompute, + EstimatedSavings: 1500, + Details: (*common.SavingsPlanDetails)(nil), + } + // An interface holding a typed nil satisfies the type assertion with + // ok==true, so this case exercises the `ok && details != nil` guard + // specifically — a plain `ok` check would send this down the + // scaling path and panic on the nil dereference. + out := ApplyCoverage([]common.Recommendation{rec}, 50, captureLogf(&logs), nil) + require.Len(t, out, 1) + assert.Equal(t, 1500.0, out[0].EstimatedSavings, "unscaled") + assert.Len(t, logs, 1, "exactly one warning") + }) +} + +func TestApplyCoverage_RISizedToZero_DropsAndRecords(t *testing.T) { + t.Parallel() + rec := common.Recommendation{Service: common.ServiceEC2, Count: 1, CommitmentCost: 100} + + t.Run("with drops summary", func(t *testing.T) { + t.Parallel() + d := common.NewDropSummary() + out := ApplyCoverage([]common.Recommendation{rec}, 10, nil, d) + assert.Empty(t, out) + assert.Equal(t, 1, d.Total()) + assert.Contains(t, d.FormatOneLine(), common.DropTargetSizedToZero) + }) + + t.Run("with nil drops", func(t *testing.T) { + t.Parallel() + assert.NotPanics(t, func() { + out := ApplyCoverage([]common.Recommendation{rec}, 10, nil, nil) + assert.Empty(t, out) + }) + }) +} + +func TestApplyCoverage_NilLogfSafe(t *testing.T) { + t.Parallel() + rec := common.Recommendation{ + Service: common.ServiceSavingsPlansCompute, + Details: common.ComputeDetails{Platform: "Linux/UNIX"}, // wrong type -> warning path + } + assert.NotPanics(t, func() { + ApplyCoverage([]common.Recommendation{rec}, 50, nil, nil) + }) +} + +// --- T5: ApplyTargetCoverage --- + +func mkRI(count int, avg, existingCov float64) common.Recommendation { + return common.Recommendation{ + Service: common.ServiceEC2, + Region: "us-east-1", + ResourceType: "t3.medium", + Count: count, + CommitmentType: common.CommitmentReservedInstance, + CommitmentCost: 1000, + OnDemandCost: 2000, + EstimatedSavings: 500, + AverageInstancesUsedPerHour: avg, + ExistingCoveragePct: existingCov, + } +} + +func mkSP(recUtil, hourlyCommitment float64) common.Recommendation { + return common.Recommendation{ + Service: common.ServiceSavingsPlansCompute, + CommitmentType: common.CommitmentSavingsPlan, + CommitmentCost: 1000, + OnDemandCost: 5000, + EstimatedSavings: 1500, + RecommendedUtilization: recUtil, + Details: &common.SavingsPlanDetails{HourlyCommitment: hourlyCommitment}, + } +} + +func TestApplyTargetCoverage_GapAlreadyMet_Drops(t *testing.T) { + t.Parallel() + rec := mkRI(5, 8.0, 90) // existing=90 >= target=80 + d := common.NewDropSummary() + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil, d) + assert.Empty(t, out) + assert.Equal(t, 1, d.Total()) + assert.Contains(t, d.FormatOneLine(), common.DropTargetAlreadyMet) +} + +func TestApplyTargetCoverage_FloorSizesToZero_Drops(t *testing.T) { + t.Parallel() + // avg=0.5, target=80, existing=0: floor(0.5*80/100)=floor(0.4)=0. + rec := mkRI(1, 0.5, 0) + d := common.NewDropSummary() + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil, d) + assert.Empty(t, out) + assert.Equal(t, 1, d.Total()) + assert.Contains(t, d.FormatOneLine(), common.DropTargetSizedToZero) +} + +func TestApplyTargetCoverage_NoSignal_PassesThroughUnchanged(t *testing.T) { + t.Parallel() + rec := mkRI(5, 0, 0) // AverageInstancesUsedPerHour <= 0 -> no signal + d := common.NewDropSummary() + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil, d) + require.Len(t, out, 1) + assert.Equal(t, rec, out[0], "passed through unchanged") + assert.True(t, d.IsEmpty(), "no-signal is not a drop") +} + +// TestApplyTargetCoverage_ProjectionsClampTo100 covers the clamp on +// ProjectedUtilization (easy to overflow: avg/nTarget*100 grows unbounded as +// nTarget shrinks relative to avg) and documents why ProjectedCoverage's +// clamp is a defensive boundary check rather than a reachable overflow: floor +// guarantees nTarget/avg*100 <= gapPct, so projCov <= existing+gapPct == +// target <= 100 in exact arithmetic. The target=100 boundary case below +// exercises that bound exactly. +func TestApplyTargetCoverage_ProjectionsClampTo100(t *testing.T) { + t.Parallel() + + t.Run("ProjectedUtilization clamps", func(t *testing.T) { + t.Parallel() + // avg=100, target=10, existing=0: gap=10, nTarget=floor(100*10/100)=10. + // Unclamped projUtil = 100/10*100 = 1000%. + rec := mkRI(200, 100, 0) + out := ApplyTargetCoverage([]common.Recommendation{rec}, 10, nil, nil) + require.Len(t, out, 1) + assert.Equal(t, 100.0, out[0].ProjectedUtilization) + }) + + t.Run("ProjectedCoverage stays at the target boundary", func(t *testing.T) { + t.Parallel() + // avg=10, target=100, existing=0: gap=100, nTarget=floor(10)=10. + // projCov = 0 + 10/10*100 = 100.0 exactly at the clamp boundary. + rec := mkRI(10, 10, 0) + out := ApplyTargetCoverage([]common.Recommendation{rec}, 100, nil, nil) + require.Len(t, out, 1) + assert.LessOrEqual(t, out[0].ProjectedCoverage, 100.0) + assert.Equal(t, 100.0, out[0].ProjectedCoverage) + }) +} + +// TestApplyTargetCoverage_CountZeroNoNaNOrInf covers the rec.Count==0 +// fallback (`ratio = float64(nTarget)` instead of nTarget/rec.Count) that +// guards against a division by zero producing NaN/Inf in every scaled money +// field. +func TestApplyTargetCoverage_CountZeroNoNaNOrInf(t *testing.T) { + t.Parallel() + monthly := 20.0 + rec := mkRI(0, 10, 0) // Count=0, avg=10, existing=0 + rec.RecurringMonthlyCost = &monthly + + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil, nil) + + require.Len(t, out, 1) + got := out[0] + for name, v := range map[string]float64{ + "CommitmentCost": got.CommitmentCost, + "OnDemandCost": got.OnDemandCost, + "EstimatedSavings": got.EstimatedSavings, + "RecurringMonthlyCost": *got.RecurringMonthlyCost, + "ProjectedUtilization": got.ProjectedUtilization, + "ProjectedCoverage": got.ProjectedCoverage, + } { + assert.False(t, math.IsNaN(v), "%s is NaN", name) + assert.False(t, math.IsInf(v, 0), "%s is Inf", name) + } +} + +func TestApplyTargetCoverage_SPEdgePassthroughs(t *testing.T) { + t.Parallel() + + t.Run("RecommendedUtilization <= 0 passes through unchanged", func(t *testing.T) { + t.Parallel() + rec := mkSP(0, 2.0) + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil, nil) + require.Len(t, out, 1) + assert.Equal(t, rec, out[0]) + }) + + t.Run("HourlyCommitment <= 0 passes through unchanged", func(t *testing.T) { + t.Parallel() + rec := mkSP(50, 0) + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil, nil) + require.Len(t, out, 1) + assert.Equal(t, rec, out[0]) + }) + + t.Run("wrong-type Details passes through with warning, ProjectedUtilization stays zero", func(t *testing.T) { + t.Parallel() + var logs []string + rec := mkSP(50, 2.0) + rec.Details = common.ComputeDetails{Platform: "Linux/UNIX"} + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, captureLogf(&logs), nil) + require.Len(t, out, 1) + assert.Equal(t, rec, out[0]) + assert.Equal(t, 0.0, out[0].ProjectedUtilization, "scaling failed, so projection must not be set") + assert.Len(t, logs, 1) + }) + + t.Run("typed-nil Details passes through with warning, ProjectedUtilization stays zero", func(t *testing.T) { + t.Parallel() + var logs []string + rec := mkSP(50, 2.0) + rec.Details = (*common.SavingsPlanDetails)(nil) + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, captureLogf(&logs), nil) + require.Len(t, out, 1) + assert.Equal(t, 0.0, out[0].ProjectedUtilization) + assert.Len(t, logs, 1) + }) +} + +// TestApplyTargetCoverage_UnsupportedType_WarnsOncePerType covers a slice +// containing several recs of the same unsupported CommitmentType: the +// warning must fire exactly once per distinct type across the whole slice, +// not once per rec, and every rec passes through unchanged regardless. +func TestApplyTargetCoverage_UnsupportedType_WarnsOncePerType(t *testing.T) { + t.Parallel() + var logs []string + recs := []common.Recommendation{ + {Service: common.ServiceCompute, CommitmentType: common.CommitmentCUD, Count: 1}, + {Service: common.ServiceCompute, CommitmentType: common.CommitmentCUD, Count: 2}, + {Service: common.ServiceCompute, CommitmentType: common.CommitmentReservedCapacity, Count: 3}, + } + + out := ApplyTargetCoverage(recs, 80, captureLogf(&logs), nil) + + require.Len(t, out, 3) + assert.Equal(t, recs, out, "unsupported types pass through unchanged") + + cudWarnings, capacityWarnings := 0, 0 + for _, l := range logs { + switch { + case strings.Contains(l, string(common.CommitmentCUD)): + cudWarnings++ + case strings.Contains(l, string(common.CommitmentReservedCapacity)): + capacityWarnings++ + } + } + assert.Equal(t, 1, cudWarnings, "CommitmentCUD warns exactly once despite 2 recs") + assert.Equal(t, 1, capacityWarnings, "CommitmentReservedCapacity warns exactly once") +} + +func TestApplyTargetCoverage_TargetPctOutOfRange_PassesThroughWithWarning(t *testing.T) { + t.Parallel() + recs := []common.Recommendation{mkRI(5, 8, 0)} + + for _, targetPct := range []float64{0, -1, 101} { + var logs []string + out := ApplyTargetCoverage(recs, targetPct, captureLogf(&logs), nil) + assert.Equal(t, recs, out, "targetPct=%.0f", targetPct) + assert.Len(t, logs, 1, "targetPct=%.0f", targetPct) + } +} + +func TestApplyTargetCoverage_NilLogfSafe(t *testing.T) { + t.Parallel() + recs := []common.Recommendation{ + mkRI(5, 8, 90), // gapPct <= 0 -> INFO log + mkRI(1, 0.5, 0), // sized to zero -> INFO log + mkRI(5, 0, 0), // no signal, no log + mkSP(50, 0), // no signal, no log + {Service: common.ServiceCompute, CommitmentType: common.CommitmentCUD}, // unsupported -> WARNING log + } + assert.NotPanics(t, func() { + ApplyTargetCoverage(recs, 80, nil, nil) + }) + assert.NotPanics(t, func() { + ApplyTargetCoverage(recs, 0, nil, nil) // out-of-range -> WARNING log + }) +} From 71b1ce6ed9da7c6867b6bf3beee964de6349fe8d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 25 Aug 2026 03:54:10 +0200 Subject: [PATCH 04/12] refactor(recfilter): extract DuplicateChecker to pkg The 24h recent-purchase guard lived in package main, so the MCP server had no way to see capacity the CLI bought minutes earlier. Today an MCP purchase can land on top of a CLI purchase 10 minutes old; wiring that guard in needs the checker importable first. DuplicateChecker, its helpers and DefaultDuplicateCheckLookbackHours move into pkg/recfilter/dedupe.go. The decision trail routes through an injected Logf so the MCP server can run the checker silently; cmd's NewDuplicateChecker wires log.Printf, keeping the CLI's stderr output unchanged. cmd re-exports DuplicateChecker as a type alias (not a defined type, so method calls still resolve) and DefaultDuplicateCheckLookbackHours as a const, leaving its existing tests untouched. Refs #1883 --- cmd/helpers.go | 145 ++---------------------- pkg/recfilter/dedupe.go | 150 +++++++++++++++++++++++++ pkg/recfilter/dedupe_test.go | 210 +++++++++++++++++++++++++++++++++++ 3 files changed, 372 insertions(+), 133 deletions(-) create mode 100644 pkg/recfilter/dedupe.go create mode 100644 pkg/recfilter/dedupe_test.go diff --git a/cmd/helpers.go b/cmd/helpers.go index 911708112..4d23354e8 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -8,10 +8,8 @@ import ( "os" "strings" "sync" - "time" "github.com/LeanerCloud/CUDly/pkg/common" - "github.com/LeanerCloud/CUDly/pkg/provider" "github.com/LeanerCloud/CUDly/pkg/recfilter" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/organizations" @@ -20,11 +18,11 @@ import ( // Constants for purchase processing. const ( - // DefaultDuplicateCheckLookbackHours is the default lookback period for checking recent purchases. - DefaultDuplicateCheckLookbackHours = 24 - // PurchaseDelaySeconds is the delay between consecutive purchases to avoid rate limiting. PurchaseDelaySeconds = 2 + + // DefaultDuplicateCheckLookbackHours is re-exported from pkg/recfilter. + DefaultDuplicateCheckLookbackHours = recfilter.DefaultDuplicateCheckLookbackHours ) // AppLogger is a simple logger for application output. @@ -234,136 +232,17 @@ func ConfirmPurchase(totalInstances int, totalSavings float64, skipConfirmation // and tests are unchanged. func CheckAuditLogWritable(path string) error { return common.CheckAuditLogWritable(path) } -// DuplicateChecker checks for existing commitments to avoid duplicates. -type DuplicateChecker struct { - LookbackHours int // How many hours to look back for recent purchases -} +// DuplicateChecker is re-exported from pkg/recfilter so cmd's existing call +// sites and tests are unchanged. +type DuplicateChecker = recfilter.DuplicateChecker -// NewDuplicateChecker creates a new duplicate checker. Pass 0 to use the default lookback period. +// NewDuplicateChecker creates a new duplicate checker. Pass 0 to use the +// default lookback period. Logf is wired to log.Printf so the CLI's +// decision trail keeps going to stderr exactly as it does today. func NewDuplicateChecker(hours int) *DuplicateChecker { - if hours <= 0 { - hours = DefaultDuplicateCheckLookbackHours - } - return &DuplicateChecker{ - LookbackHours: hours, - } -} - -// AdjustRecommendationsForExisting adjusts recommendations based on existing commitments -// This checks for recently purchased RIs (within LookbackHours) to avoid duplicate purchases. -// Note: This is designed to prevent re-purchasing something you just bought, not to prevent -// purchasing RIs in other accounts that happen to have the same characteristics. -func (d *DuplicateChecker) AdjustRecommendationsForExisting(ctx context.Context, recs []common.Recommendation, client provider.ServiceClient) (passed, filtered []common.Recommendation, err error) { - existing, err := client.GetExistingCommitments(ctx) - if err != nil { - return recs, nil, err - } - - log.Printf(" [DuplicateChecker] Found %d total existing commitments", len(existing)) - - recentExisting := d.filterRecentCommitments(existing) - log.Printf(" [DuplicateChecker] Found %d recent commitments (purchased in last %d hours)", len(recentExisting), d.LookbackHours) - - if len(recentExisting) == 0 { - return recs, nil, nil - } - - existingMap := buildExistingCommitmentsMap(recentExisting) - log.Printf(" [DuplicateChecker] Existing map has %d unique keys", len(existingMap)) - - passed, filtered = adjustRecommendationsAgainstExisting(recs, existingMap) - - if len(filtered) > 0 { - log.Printf(" [DuplicateChecker] Result: %d recommendations kept out of %d (avoided %d duplicates)", - len(passed), len(recs), len(filtered)) - } - return passed, filtered, nil -} - -// filterRecentCommitments filters commitments to only recent purchases within the lookback window. -func (d *DuplicateChecker) filterRecentCommitments(existing []common.Commitment) []common.Commitment { - cutoffTime := time.Now().Add(-time.Duration(d.LookbackHours) * time.Hour) - recentExisting := make([]common.Commitment, 0) - - for _rvc := range existing { - c := existing[_rvc] - if isRecentActiveCommitment(c, cutoffTime) { - recentExisting = append(recentExisting, c) - } - } - - return recentExisting -} - -// isRecentActiveCommitment checks if a commitment is active and purchased after the cutoff time. -func isRecentActiveCommitment(c common.Commitment, cutoffTime time.Time) bool { - return (c.State == "active" || c.State == "payment-pending") && c.StartDate.After(cutoffTime) -} - -// buildExistingCommitmentsMap builds a map of commitments by resource type, region, and engine. -func buildExistingCommitmentsMap(commitments []common.Commitment) map[string]int { - existingMap := make(map[string]int) - - for _rvc := range commitments { - c := commitments[_rvc] - normalizedEngine := common.NormalizeEngineName(c.Engine) - key := fmt.Sprintf("%s|%s|%s", c.ResourceType, c.Region, normalizedEngine) - existingMap[key] += c.Count - log.Printf(" [DuplicateChecker] Recent RI: key=%s count=%d startDate=%s (raw engine=%s)", - key, c.Count, c.StartDate.Format("2006-01-02 15:04:05"), c.Engine) - } - - return existingMap -} - -// adjustRecommendationsAgainstExisting adjusts recommendations based on existing commitments. -// Returns (passed, filtered) where filtered contains recs whose count was reduced to zero. -func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existingMap map[string]int) (passed, filtered []common.Recommendation) { - passed = make([]common.Recommendation, 0, len(recs)) - filtered = make([]common.Recommendation, 0) - - for _rvc := range recs { - rec := recs[_rvc] - adjusted := adjustSingleRecommendation(rec, existingMap) - if adjusted.Count > 0 { - passed = append(passed, adjusted) - } else { - filtered = append(filtered, rec) - } - } - - return passed, filtered -} - -// adjustSingleRecommendation adjusts a single recommendation based on existing commitments. -func adjustSingleRecommendation(rec common.Recommendation, existingMap map[string]int) common.Recommendation { - engine := common.EngineFromDetails(rec.Details) - key := fmt.Sprintf("%s|%s|%s", rec.ResourceType, rec.Region, engine) - existingCount := existingMap[key] - - if existingCount >= rec.Count { - // All of this recommendation is covered by recent RIs. - // Return a zero-value Recommendation (Count=0) as a sentinel; the caller - // (adjustRecommendationsAgainstExisting) filters out recommendations with Count <= 0. - log.Printf(" [DuplicateChecker] SKIP %s: recent %d >= recommended %d", key, existingCount, rec.Count) - existingMap[key] -= rec.Count - return common.Recommendation{Count: 0} - } - - // Partial or no coverage by recent RIs - adjusted := rec - if existingCount > 0 { - adjusted.Count = rec.Count - existingCount - existingMap[key] = 0 - log.Printf(" [DuplicateChecker] PARTIAL %s: adjusted count from %d to %d", key, rec.Count, adjusted.Count) - } - - return adjusted -} - -// AdjustRecommendationsForExistingRIs is an alias for AdjustRecommendationsForExisting. -func (d *DuplicateChecker) AdjustRecommendationsForExistingRIs(ctx context.Context, recs []common.Recommendation, client provider.ServiceClient) (passed, filtered []common.Recommendation, err error) { - return d.AdjustRecommendationsForExisting(ctx, recs, client) + d := recfilter.NewDuplicateChecker(hours) + d.Logf = log.Printf + return d } // GetRecommendationDescription returns a human-readable description. diff --git a/pkg/recfilter/dedupe.go b/pkg/recfilter/dedupe.go new file mode 100644 index 000000000..98b050c06 --- /dev/null +++ b/pkg/recfilter/dedupe.go @@ -0,0 +1,150 @@ +package recfilter + +import ( + "context" + "fmt" + "time" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/pkg/provider" +) + +// DefaultDuplicateCheckLookbackHours is the default lookback period for checking recent purchases. +const DefaultDuplicateCheckLookbackHours = 24 + +// DuplicateChecker checks for existing commitments to avoid duplicates. +type DuplicateChecker struct { + LookbackHours int // How many hours to look back for recent purchases + + // Logf receives the per-commitment decision trail. Nil is silent. + // recfilter never logs through a package-level logger: cmd's AppLogger + // writes to stdout, which the MCP server owns as its protocol transport. + Logf Logf +} + +// NewDuplicateChecker creates a new duplicate checker. Pass 0 to use the default lookback period. +func NewDuplicateChecker(hours int) *DuplicateChecker { + if hours <= 0 { + hours = DefaultDuplicateCheckLookbackHours + } + return &DuplicateChecker{ + LookbackHours: hours, + } +} + +// AdjustRecommendationsForExisting adjusts recommendations based on existing commitments +// This checks for recently purchased RIs (within LookbackHours) to avoid duplicate purchases. +// Note: This is designed to prevent re-purchasing something you just bought, not to prevent +// purchasing RIs in other accounts that happen to have the same characteristics. +func (d *DuplicateChecker) AdjustRecommendationsForExisting(ctx context.Context, recs []common.Recommendation, client provider.ServiceClient) (passed, filtered []common.Recommendation, err error) { + existing, err := client.GetExistingCommitments(ctx) + if err != nil { + return recs, nil, err + } + + d.Logf.printf(" [DuplicateChecker] Found %d total existing commitments", len(existing)) + + recentExisting := d.filterRecentCommitments(existing) + d.Logf.printf(" [DuplicateChecker] Found %d recent commitments (purchased in last %d hours)", len(recentExisting), d.LookbackHours) + + if len(recentExisting) == 0 { + return recs, nil, nil + } + + existingMap := buildExistingCommitmentsMap(recentExisting, d.Logf) + d.Logf.printf(" [DuplicateChecker] Existing map has %d unique keys", len(existingMap)) + + passed, filtered = adjustRecommendationsAgainstExisting(recs, existingMap, d.Logf) + + if len(filtered) > 0 { + d.Logf.printf(" [DuplicateChecker] Result: %d recommendations kept out of %d (avoided %d duplicates)", + len(passed), len(recs), len(filtered)) + } + return passed, filtered, nil +} + +// filterRecentCommitments filters commitments to only recent purchases within the lookback window. +func (d *DuplicateChecker) filterRecentCommitments(existing []common.Commitment) []common.Commitment { + cutoffTime := time.Now().Add(-time.Duration(d.LookbackHours) * time.Hour) + recentExisting := make([]common.Commitment, 0) + + for _rvc := range existing { + c := existing[_rvc] + if isRecentActiveCommitment(c, cutoffTime) { + recentExisting = append(recentExisting, c) + } + } + + return recentExisting +} + +// isRecentActiveCommitment checks if a commitment is active and purchased after the cutoff time. +func isRecentActiveCommitment(c common.Commitment, cutoffTime time.Time) bool { + return (c.State == "active" || c.State == "payment-pending") && c.StartDate.After(cutoffTime) +} + +// buildExistingCommitmentsMap builds a map of commitments by resource type, region, and engine. +func buildExistingCommitmentsMap(commitments []common.Commitment, logf Logf) map[string]int { + existingMap := make(map[string]int) + + for _rvc := range commitments { + c := commitments[_rvc] + normalizedEngine := common.NormalizeEngineName(c.Engine) + key := fmt.Sprintf("%s|%s|%s", c.ResourceType, c.Region, normalizedEngine) + existingMap[key] += c.Count + logf.printf(" [DuplicateChecker] Recent RI: key=%s count=%d startDate=%s (raw engine=%s)", + key, c.Count, c.StartDate.Format("2006-01-02 15:04:05"), c.Engine) + } + + return existingMap +} + +// adjustRecommendationsAgainstExisting adjusts recommendations based on existing commitments. +// Returns (passed, filtered) where filtered contains recs whose count was reduced to zero. +func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existingMap map[string]int, logf Logf) (passed, filtered []common.Recommendation) { + passed = make([]common.Recommendation, 0, len(recs)) + filtered = make([]common.Recommendation, 0) + + for _rvc := range recs { + rec := recs[_rvc] + adjusted := adjustSingleRecommendation(rec, existingMap, logf) + if adjusted.Count > 0 { + passed = append(passed, adjusted) + } else { + filtered = append(filtered, rec) + } + } + + return passed, filtered +} + +// adjustSingleRecommendation adjusts a single recommendation based on existing commitments. +func adjustSingleRecommendation(rec common.Recommendation, existingMap map[string]int, logf Logf) common.Recommendation { + engine := common.EngineFromDetails(rec.Details) + key := fmt.Sprintf("%s|%s|%s", rec.ResourceType, rec.Region, engine) + existingCount := existingMap[key] + + if existingCount >= rec.Count { + // All of this recommendation is covered by recent RIs. + // Return a zero-value Recommendation (Count=0) as a sentinel; the caller + // (adjustRecommendationsAgainstExisting) filters out recommendations with Count <= 0. + logf.printf(" [DuplicateChecker] SKIP %s: recent %d >= recommended %d", key, existingCount, rec.Count) + existingMap[key] -= rec.Count + return common.Recommendation{Count: 0} + } + + // Partial or no coverage by recent RIs + adjusted := rec + if existingCount > 0 { + adjusted.Count = rec.Count - existingCount + existingMap[key] = 0 + logf.printf(" [DuplicateChecker] PARTIAL %s: adjusted count from %d to %d", key, rec.Count, adjusted.Count) + } + + return adjusted +} + +// AdjustRecommendationsForExistingRIs is an alias for AdjustRecommendationsForExisting. +func (d *DuplicateChecker) AdjustRecommendationsForExistingRIs(ctx context.Context, recs []common.Recommendation, client provider.ServiceClient) (passed, filtered []common.Recommendation, err error) { + return d.AdjustRecommendationsForExisting(ctx, recs, client) +} diff --git a/pkg/recfilter/dedupe_test.go b/pkg/recfilter/dedupe_test.go new file mode 100644 index 000000000..fbd9b0e80 --- /dev/null +++ b/pkg/recfilter/dedupe_test.go @@ -0,0 +1,210 @@ +package recfilter + +import ( + "context" + "errors" + "fmt" + "strings" + "testing" + "time" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeServiceClient is a minimal provider.ServiceClient implementing only +// GetExistingCommitments, which is all AdjustRecommendationsForExisting uses. +type fakeServiceClient struct { + commitments []common.Commitment + err error +} + +func (f *fakeServiceClient) GetServiceType() common.ServiceType { return "" } +func (f *fakeServiceClient) GetRegion() string { return "" } +func (f *fakeServiceClient) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { + return nil, nil +} +func (f *fakeServiceClient) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { + return f.commitments, f.err +} +func (f *fakeServiceClient) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { + return common.PurchaseResult{}, nil +} +func (f *fakeServiceClient) ValidateOffering(ctx context.Context, rec common.Recommendation) error { + return nil +} +func (f *fakeServiceClient) GetOfferingDetails(ctx context.Context, rec common.Recommendation) (*common.OfferingDetails, error) { + return nil, nil +} +func (f *fakeServiceClient) GetValidResourceTypes(ctx context.Context) ([]string, error) { + return nil, nil +} + +func TestFilterRecentCommitments_StateAndWindow(t *testing.T) { + t.Parallel() + now := time.Now() + commitments := []common.Commitment{ + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 1, State: "active", StartDate: now.Add(-25 * time.Hour)}, // too old + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 2, State: "retired", StartDate: now.Add(-1 * time.Hour)}, // wrong state + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 3, State: "cancelled", StartDate: now.Add(-1 * time.Hour)}, // wrong state + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 4, State: "payment-pending", StartDate: now.Add(-1 * time.Hour)}, + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 5, State: "active", StartDate: now.Add(-1 * time.Hour)}, + } + + d := NewDuplicateChecker(DefaultDuplicateCheckLookbackHours) + recent := d.filterRecentCommitments(commitments) + + require.Len(t, recent, 2) + counts := []int{recent[0].Count, recent[1].Count} + assert.ElementsMatch(t, []int{4, 5}, counts) +} + +func TestAdjustRecommendationsForExisting_EngineNormalizationCollides(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "db.r5.large", Region: "us-east-1", Engine: "Aurora PostgreSQL", Count: 5, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + rec := common.Recommendation{ + ResourceType: "db.r5.large", Region: "us-east-1", Count: 5, + Details: &common.DatabaseDetails{Engine: "aurora-postgresql"}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec}, client) + + require.NoError(t, err) + assert.Empty(t, passed) + assert.Len(t, filtered, 1) +} + +func TestAdjustRecommendationsForExisting_FullCoverageDrops(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 5, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + rec := common.Recommendation{ + ResourceType: "db.t3.small", Region: "us-east-1", Count: 5, + Details: &common.DatabaseDetails{Engine: "mysql"}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec}, client) + + require.NoError(t, err) + assert.Empty(t, passed) + require.Len(t, filtered, 1) + assert.Equal(t, rec, filtered[0]) +} + +func TestAdjustRecommendationsForExisting_PartialCoverageConsumesBudget(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 2, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + recs := []common.Recommendation{ + {ResourceType: "db.t3.small", Region: "us-east-1", Count: 5, Details: &common.DatabaseDetails{Engine: "mysql"}}, + {ResourceType: "db.t3.small", Region: "us-east-1", Count: 3, Details: &common.DatabaseDetails{Engine: "mysql"}}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, recs, client) + + require.NoError(t, err) + assert.Empty(t, filtered) + require.Len(t, passed, 2) + assert.Equal(t, 3, passed[0].Count) // 5 - 2 = 3, budget consumed + assert.Equal(t, 3, passed[1].Count) // no existing coverage left, unchanged +} + +func TestAdjustRecommendationsForExisting_ClientErrorReturnsOriginal(t *testing.T) { + t.Parallel() + ctx := context.Background() + wantErr := errors.New("boom") + client := &fakeServiceClient{err: wantErr} + recs := []common.Recommendation{ + {ResourceType: "db.t3.small", Region: "us-east-1", Count: 5, Details: &common.DatabaseDetails{Engine: "mysql"}}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, recs, client) + + assert.Equal(t, recs, passed) + assert.Nil(t, filtered) + assert.Equal(t, wantErr, err) +} + +func TestAdjustRecommendationsForExisting_NoRecentCommitmentsPassesThrough(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{} + recs := []common.Recommendation{ + {ResourceType: "db.t3.small", Region: "us-east-1", Count: 5, Details: &common.DatabaseDetails{Engine: "mysql"}}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, recs, client) + + require.NoError(t, err) + assert.Nil(t, filtered) + require.Len(t, passed, 1) + // No reallocation/reordering: passed IS recs, not a copy. + assert.Same(t, &recs[0], &passed[0]) +} + +func TestNewDuplicateChecker_DefaultAndCustomLookback(t *testing.T) { + t.Parallel() + assert.Equal(t, DefaultDuplicateCheckLookbackHours, NewDuplicateChecker(0).LookbackHours) + assert.Equal(t, DefaultDuplicateCheckLookbackHours, NewDuplicateChecker(-1).LookbackHours) + assert.Equal(t, 48, NewDuplicateChecker(48).LookbackHours) +} + +func TestAdjustRecommendationsForExisting_NilLogfDoesNotPanic(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 2, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + recs := []common.Recommendation{ + {ResourceType: "db.t3.small", Region: "us-east-1", Count: 5, Details: &common.DatabaseDetails{Engine: "mysql"}}, + } + + d := NewDuplicateChecker(0) + assert.NotPanics(t, func() { + _, _, err := d.AdjustRecommendationsForExisting(ctx, recs, client) + require.NoError(t, err) + }) +} + +func TestAdjustRecommendationsForExisting_LogfReceivesDecisionTrail(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", Count: 5, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + recs := []common.Recommendation{ + {ResourceType: "db.t3.small", Region: "us-east-1", Count: 5, Details: &common.DatabaseDetails{Engine: "mysql"}}, + } + + var lines []string + d := NewDuplicateChecker(0) + d.Logf = func(format string, args ...any) { + lines = append(lines, fmt.Sprintf(format, args...)) + } + + _, _, err := d.AdjustRecommendationsForExisting(ctx, recs, client) + require.NoError(t, err) + + require.NotEmpty(t, lines) + found := false + for _, l := range lines { + if strings.Contains(l, "[DuplicateChecker]") { + found = true + break + } + } + assert.True(t, found) +} From 494616bf6b869f733f41dfc0b64604cd94bdc9f4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 25 Aug 2026 04:36:03 +0200 Subject: [PATCH 05/12] fix(recfilter): normalize engine filter entries before matching Only the recommendation side of the engine comparison was normalized, so a filter entry kept whatever spelling the operator typed: --include-engines postgres never matched a recommendation whose engine normalizes to postgresql, and the same held for oracle-ee, sqlserver-se and the Cost Explorer spellings. Both sides now go through common.NormalizeEngineName. Unrecognized engines fall back to lowercase, so the previous case-insensitive behaviour survives. This is a deliberate behaviour change rather than part of the extraction: handing the MCP search tool a filter that silently under-matches was the worse option. Also drop the redundant drops != nil guard in ApplyCoverage (common.DropSummary.Add is nil-receiver safe, and every other drop site in the package relies on that), and make the audit-log failure test independent of process privileges: a 0555 directory is a no-op for root, so the path now sits under a regular file. Refs #1883 --- pkg/common/audit_test.go | 10 +++++--- pkg/recfilter/filters.go | 46 ++++++++++++++++------------------- pkg/recfilter/filters_test.go | 20 +++++++++++++++ pkg/recfilter/sizing.go | 4 ++- 4 files changed, 50 insertions(+), 30 deletions(-) diff --git a/pkg/common/audit_test.go b/pkg/common/audit_test.go index ded139722..3a13950d5 100644 --- a/pkg/common/audit_test.go +++ b/pkg/common/audit_test.go @@ -149,13 +149,15 @@ func TestCheckAuditLogWritable_WritablePath(t *testing.T) { assert.NoError(t, CheckAuditLogWritable(path)) } +// The unwritable path is a child of a regular file rather than a 0555 +// directory: a root process ignores the permission bits, so a chmod-based +// case would pass vacuously in root-based CI. func TestCheckAuditLogWritable_UnwritablePath(t *testing.T) { t.Parallel() - dir := t.TempDir() - require.NoError(t, os.Chmod(dir, 0555)) - t.Cleanup(func() { _ = os.Chmod(dir, 0755) }) + notADir := filepath.Join(t.TempDir(), "regular-file") + require.NoError(t, os.WriteFile(notADir, []byte("x"), 0600)) - path := filepath.Join(dir, "audit.jsonl") + path := filepath.Join(notADir, "audit.jsonl") err := CheckAuditLogWritable(path) assert.Error(t, err) assert.Contains(t, err.Error(), path) diff --git a/pkg/recfilter/filters.go b/pkg/recfilter/filters.go index 489864e26..8bd538436 100644 --- a/pkg/recfilter/filters.go +++ b/pkg/recfilter/filters.go @@ -66,42 +66,38 @@ func (f Filters) IncludesInstanceType(instanceType string) bool { return true } -// IncludesEngine checks if a recommendation should be included based on engine filters. +// IncludesEngine checks if a recommendation should be included based on engine +// filters. +// +// Both sides of the comparison are normalized. Normalizing only the +// recommendation would leave the filter list holding whatever spelling the +// operator typed, so --include-engines=postgres would silently fail to match a +// recommendation whose engine normalizes to "postgresql". NormalizeEngineName +// lowercases anything it does not recognize, so unknown engines keep the +// case-insensitive matching this had before. func (f Filters) IncludesEngine(rec *common.Recommendation) bool { - // Extract engine from recommendation. engine := common.EngineFromDetails(rec.Details) if engine == "" { - // If no engine info, include by default unless there's an include list. + // No engine info: include by default unless there's an include list. return len(f.IncludeEngines) == 0 } - - // Normalize engine name to lowercase for comparison. engine = strings.ToLower(engine) - // If include list is specified, engine must be in it. - if len(f.IncludeEngines) > 0 { - found := false - for _, e := range f.IncludeEngines { - if strings.EqualFold(e, engine) { - found = true - break - } - } - if !found { - return false - } + if len(f.IncludeEngines) > 0 && !matchesEngine(f.IncludeEngines, engine) { + return false } + return !matchesEngine(f.ExcludeEngines, engine) +} - // If exclude list is specified, engine must not be in it. - if len(f.ExcludeEngines) > 0 { - for _, e := range f.ExcludeEngines { - if strings.EqualFold(e, engine) { - return false - } +// matchesEngine reports whether any filter entry normalizes to engine, which +// the caller has already normalized. +func matchesEngine(filters []string, engine string) bool { + for _, e := range filters { + if common.NormalizeEngineName(e) == engine { + return true } } - - return true + return false } // IncludesPoolSize filters out RI recommendations for pools whose diff --git a/pkg/recfilter/filters_test.go b/pkg/recfilter/filters_test.go index af648aaf6..12015c259 100644 --- a/pkg/recfilter/filters_test.go +++ b/pkg/recfilter/filters_test.go @@ -19,6 +19,15 @@ func TestIncludesEngine_CEAndRISpellingsBothMatch(t *testing.T) { {"RI spelling rec, CE-normalized filter", "aurora-postgresql", []string{"aurora-postgresql"}, true}, {"postgres rec, postgresql filter", "postgres", []string{"postgresql"}, true}, {"PostgreSQL rec, postgresql filter", "PostgreSQL", []string{"postgresql"}, true}, + // The reciprocal direction: the filter carries the alias and the + // recommendation the canonical name. Matching only after normalizing + // the recommendation would miss every one of these. + {"postgresql rec, postgres filter", "postgresql", []string{"postgres"}, true}, + {"aurora-postgresql rec, CE spelling filter", "aurora-postgresql", []string{"Aurora PostgreSQL"}, true}, + {"sqlserver rec, sqlserver-se filter", "sqlserver", []string{"sqlserver-se"}, true}, + {"oracle rec, oracle-ee filter", "oracle", []string{"oracle-ee"}, true}, + {"unrecognized engine still matches case-insensitively", "Db2", []string{"DB2"}, true}, + {"unrelated engine does not match", "mysql", []string{"postgres"}, false}, } for _, tt := range tests { @@ -58,6 +67,17 @@ func TestIncludesInstanceType_ExcludeBeatsInclude(t *testing.T) { assert.False(t, f.IncludesInstanceType("db.t3.micro")) } +// Exclude entries are normalized on the same axis as include entries: an +// operator excluding "postgres" must not still get "postgresql" rows. +func TestIncludesEngine_ExcludeNormalizesAliases(t *testing.T) { + f := Filters{ExcludeEngines: []string{"postgres"}} + rec := common.Recommendation{Details: &common.DatabaseDetails{Engine: "PostgreSQL"}} + assert.False(t, f.IncludesEngine(&rec)) + + other := common.Recommendation{Details: &common.DatabaseDetails{Engine: "mysql"}} + assert.True(t, f.IncludesEngine(&other)) +} + func TestIncludesEngine_ExcludeBeatsInclude(t *testing.T) { f := Filters{IncludeEngines: []string{"mysql"}, ExcludeEngines: []string{"mysql"}} rec := common.Recommendation{Details: &common.DatabaseDetails{Engine: "mysql"}} diff --git a/pkg/recfilter/sizing.go b/pkg/recfilter/sizing.go index e7a4562e3..45c3164d4 100644 --- a/pkg/recfilter/sizing.go +++ b/pkg/recfilter/sizing.go @@ -69,7 +69,9 @@ func ApplyCoverage(recs []common.Recommendation, coverage float64, logf Logf, dr adjusted = common.ScaleRecommendationCosts(adjusted, sizedRatio) adjusted.Count = newCount result = append(result, adjusted) - } else if drops != nil { + } else { + // No nil guard: DropSummary.Add is nil-receiver safe, and every + // other drop site in this package relies on that. drops.Add(common.DropTargetSizedToZero, 1) } } From f79a123dbed2f967e5328cc3c76dc16266123a46 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 27 Aug 2026 14:17:41 +0200 Subject: [PATCH 06/12] test(cmd): pin min-pool-size behaviour against the pre-extraction oracle Review raised that moving the --min-pool-size check into recfilter.ApplyMinPoolSize reordered it relative to processRecommendation's currentRegion guard, inflating DropMinPoolSize on multi-region runs. Rather than argue it, this test keeps the pre-extraction single-loop implementation verbatim as a differential oracle and runs both side by side, once per region, over a multi-region set that mixes above-threshold, below-threshold, no-signal and Savings Plan recommendations. Survivors and drop accounting match exactly in every region, so the extraction is behaviour-preserving on that axis. The test also pins the summed multi-region drop count. It is 3x the distinct below-threshold count, because applyFilters re-scans whatever slice it is handed rather than a region-scoped subset. That is pre-existing and identical in both implementations; production reaches applyFilters through fetchAndFilterRegionRecs, which fetches per region first, so the inflation does not fire there. Refs #1883 --- cmd/multi_service_filters_test.go | 124 ++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/cmd/multi_service_filters_test.go b/cmd/multi_service_filters_test.go index f742435ec..33298f12b 100644 --- a/cmd/multi_service_filters_test.go +++ b/cmd/multi_service_filters_test.go @@ -1,11 +1,14 @@ package main import ( + "fmt" + "log" "testing" "time" "github.com/LeanerCloud/CUDly/pkg/common" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestApplyFilters(t *testing.T) { @@ -629,3 +632,124 @@ func TestApplyFilters_SavingsPlansRegionFilters(t *testing.T) { }) } } + +// applyFiltersPreExtraction is the pre-refactor applyFilters implementation, +// kept verbatim (from origin/main, before the recfilter.Filters.ApplyMinPoolSize +// extraction) as a differential oracle. It applies the --min-pool-size check +// inline, in the same loop and same order as processRecommendation, rather +// than as a separate pass over the full slice. +func applyFiltersPreExtraction(recs []common.Recommendation, cfg *Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string, drops *common.DropSummary) []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++ + drops.Add(common.DropMinPoolSize, 1) + continue + } + adjusted, include, dropReason := processRecommendation(&recs[i], cfg, instanceVersions, versionInfo, currentRegion) + if include { + filtered = append(filtered, adjusted) + } else if dropReason != "" { + drops.Add(dropReason, 1) + } + } + + if poolDropCount > 0 { + log.Printf("INFO: --min-pool-size dropped %d recommendation(s) (%.2f avg instances/hr total)", poolDropCount, poolDropInstances) + } + + return filtered +} + +// TestApplyFilters_MinPoolSizeMultiRegionMatchesPreExtractionBehaviour is a +// differential regression test against a reviewer's claim that extracting +// the --min-pool-size check into recfilter.Filters.ApplyMinPoolSize changed +// its ordering relative to the currentRegion guard in processRecommendation +// (inflating common.DropMinPoolSize in multi-region runs). It runs the +// current applyFilters and the pre-extraction oracle above side by side, +// once per region, over the same multi-region recommendation set, and +// asserts both the survivors and the drop accounting match exactly. +func TestApplyFilters_MinPoolSizeMultiRegionMatchesPreExtractionBehaviour(t *testing.T) { + origCfg := toolCfg + defer func() { toolCfg = origCfg }() + + toolCfg = Config{MinPoolSize: 2.0} + + regions := []string{"us-east-1", "eu-west-1", "ap-southeast-1"} + const distinctBelowThreshold = 3 // one below-threshold rec per region below + + // Fresh copies per call: applyFilters mutates nothing in place today, but + // the oracle and the refactored code must each see their own slice so a + // hypothetical future in-place adjustment on one side can't leak into the + // other's input and mask a real divergence. + makeRecs := func() []common.Recommendation { + return []common.Recommendation{ + {Region: "us-east-1", ResourceType: "db.t3.micro", Count: 3, AverageInstancesUsedPerHour: 5.0}, + {Region: "us-east-1", ResourceType: "db.t3.small", Count: 3, AverageInstancesUsedPerHour: 1.0}, // below threshold + {Region: "eu-west-1", ResourceType: "db.t3.micro", Count: 3, AverageInstancesUsedPerHour: 5.0}, + {Region: "eu-west-1", ResourceType: "db.t3.small", Count: 3, AverageInstancesUsedPerHour: 1.0}, // below threshold + {Region: "ap-southeast-1", ResourceType: "db.t3.micro", Count: 3, AverageInstancesUsedPerHour: 5.0}, + {Region: "ap-southeast-1", ResourceType: "db.t3.small", Count: 3, AverageInstancesUsedPerHour: 1.0}, // below threshold + {Region: "us-east-1", ResourceType: "db.t3.large", Count: 3, AverageInstancesUsedPerHour: 0}, // no-signal passthrough + { + Service: common.ServiceSavingsPlansCompute, + ResourceType: "ec2-instance", + Count: 10, + AverageInstancesUsedPerHour: 5.0, // account-level: bypasses the currentRegion guard + }, + } + } + + var totalDropMinPoolSizeNew, totalDropMinPoolSizeOld int + + for _, region := range regions { + t.Run(region, func(t *testing.T) { + dropsNew := common.NewDropSummary() + dropsOld := common.NewDropSummary() + + resultNew := applyFilters(makeRecs(), &toolCfg, make(map[string][]InstanceEngineVersion), make(map[string]MajorEngineVersionInfo), region, dropsNew) + resultOld := applyFiltersPreExtraction(makeRecs(), &toolCfg, make(map[string][]InstanceEngineVersion), make(map[string]MajorEngineVersionInfo), region, dropsOld) + + assert.Equal(t, resultOld, resultNew, + "region %s: refactored applyFilters diverged from the pre-extraction oracle -- the ApplyMinPoolSize extraction changed observable CLI filtering behaviour", region) + assert.Equal(t, dropsOld.FormatOneLine(), dropsNew.FormatOneLine(), + "region %s: drop summaries diverged between pre- and post-extraction implementations", region) + assert.Equal(t, dropsOld.Total(), dropsNew.Total(), + "region %s: total drop counts diverged between pre- and post-extraction implementations", region) + + // The fixture is built so --min-pool-size is the ONLY drop reason + // either implementation can record, which is what lets the summed + // Total() below stand in for the min-pool count specifically. + // Asserting the exact per-pass count keeps that guarantee honest: + // if some other filter started dropping rows, Total() would exceed + // the below-threshold count and this would fail. + require.Contains(t, dropsNew.FormatOneLine(), common.DropMinPoolSize, + "region %s: fixture must exercise the --min-pool-size drop path", region) + require.Equal(t, distinctBelowThreshold, dropsNew.Total(), + "region %s: --min-pool-size must be the only drop reason this fixture records", region) + + totalDropMinPoolSizeNew += dropsNew.Total() + totalDropMinPoolSizeOld += dropsOld.Total() + }) + } + + require.Equal(t, totalDropMinPoolSizeOld, totalDropMinPoolSizeNew, + "old and new implementations must agree on the summed multi-region --min-pool-size drop count") + + // Pre-existing behaviour, identical in both implementations (not introduced + // by the ApplyMinPoolSize extraction): each per-region call re-scans the + // FULL recommendation set passed to it, not a per-region subset, so a + // below-threshold recommendation from one region is re-counted as dropped + // during every other region's pass too. Summed across N region passes this + // is distinctBelowThreshold * N, not distinctBelowThreshold. This assertion + // pins today's (inflated) value rather than an aspirational deduplicated + // one -- see the test's summary report for the actual vs. distinct counts. + assert.Equal(t, distinctBelowThreshold*len(regions), totalDropMinPoolSizeNew, + "summed multi-region --min-pool-size drop count should match today's pre-existing inflation factor") +} From 16c6683f60edec6197d0dcbff1a4d41350c55f2f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 27 Aug 2026 15:24:06 +0200 Subject: [PATCH 07/12] fix(recfilter): include RDS deployment in the duplicate-identity key The duplicate checker keyed on resourceType|region|engine, so a recent Single-AZ RDS commitment suppressed a Multi-AZ recommendation for the same instance type and engine, and vice versa. Those are separately priced and provisioned and do not cover each other's demand, so the guard was dropping purchases that were never duplicates. Both key constructions now route through one dedupeKey helper and include a normalized deployment, so the commitment side and the recommendation side cannot drift. Empty stays empty, keeping non-RDS commitments on a single shared bucket. The recommendation side reads AZConfig via the new common.DeploymentFromDetails, which mirrors EngineFromDetails including its typed-nil guard. common.NormalizeDeploymentName mirrors the provider-side normaliseDeployment; the pkg module cannot import providers/aws, so the two are kept in sync by hand and the doc comment says so. Found by review on #1885. Regression tests cover both mismatch directions plus two controls: a matching-deployment RDS pair still deduplicates, and a non-RDS commitment still deduplicates. Refs #1883 --- pkg/common/deployment.go | 42 ++++++++++++++++ pkg/common/deployment_test.go | 61 ++++++++++++++++++++++ pkg/recfilter/dedupe.go | 19 +++++-- pkg/recfilter/dedupe_test.go | 95 +++++++++++++++++++++++++++++++++++ 4 files changed, 214 insertions(+), 3 deletions(-) create mode 100644 pkg/common/deployment.go create mode 100644 pkg/common/deployment_test.go diff --git a/pkg/common/deployment.go b/pkg/common/deployment.go new file mode 100644 index 000000000..577f63067 --- /dev/null +++ b/pkg/common/deployment.go @@ -0,0 +1,42 @@ +package common + +import "strings" + +// NormalizeDeploymentName canonicalises an RDS deployment-option string +// (e.g. Cost Explorer's "Multi-AZ" or the parser's "multi-az") to a single +// lowercase, no-separator form. Mirrors +// providers/aws/recommendations/coverage.go's normaliseDeployment, which +// lives in the root module and can't be imported here (pkg is a separate +// Go module); keep the two in sync by hand if the vocabulary changes. +// +// An empty input stays "" rather than becoming a synthetic bucket, so +// non-RDS commitments/recommendations (which never populate a deployment) +// collapse onto the same key instead of colliding with a real +// single-az/multi-az bucket. +func NormalizeDeploymentName(deployment string) string { + s := strings.ToLower(deployment) + s = strings.ReplaceAll(s, " ", "") + s = strings.ReplaceAll(s, "-", "") + s = strings.ReplaceAll(s, "_", "") + s = strings.ReplaceAll(s, "(", "") + s = strings.ReplaceAll(s, ")", "") + return s +} + +// DeploymentFromDetails extracts the RDS deployment option (AZConfig) from +// recommendation details. Returns "" for non-database service types. +// +// The `d == nil` check after the type assertion is not redundant with the +// `details == nil` check above it: the first only catches an untyped nil, so +// an interface holding a typed nil pointer reaches the assertion, satisfies +// it, and would panic on the field read. Same guard as EngineFromDetails. +func DeploymentFromDetails(details ServiceDetails) string { + if details == nil { + return "" + } + d, ok := details.(*DatabaseDetails) + if !ok || d == nil { + return "" + } + return d.AZConfig +} diff --git a/pkg/common/deployment_test.go b/pkg/common/deployment_test.go new file mode 100644 index 000000000..53bc94d67 --- /dev/null +++ b/pkg/common/deployment_test.go @@ -0,0 +1,61 @@ +package common + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNormalizeDeploymentName(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + want string + }{ + {"Cost Explorer spelling", "Multi-AZ", "multiaz"}, + {"parser spelling", "multi-az", "multiaz"}, + {"single-az variants agree", "Single-AZ", "singleaz"}, + {"underscores", "multi_az", "multiaz"}, + {"spaces", "Multi AZ", "multiaz"}, + {"parentheses", "Multi-AZ (readable standby)", "multiazreadablestandby"}, + // Empty stays empty rather than becoming a synthetic bucket, so a + // non-RDS commitment and a non-RDS recommendation land on one key. + {"empty stays empty", "", ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, NormalizeDeploymentName(tt.input)) + }) + } +} + +// The two spellings the RDS path actually produces must collapse together; +// that agreement is what the duplicate-identity key depends on. +func TestNormalizeDeploymentName_SpellingsCollapse(t *testing.T) { + t.Parallel() + assert.Equal(t, NormalizeDeploymentName("Multi-AZ"), NormalizeDeploymentName("multi-az")) + assert.NotEqual(t, NormalizeDeploymentName("Multi-AZ"), NormalizeDeploymentName("Single-AZ")) +} + +func TestDeploymentFromDetails(t *testing.T) { + t.Parallel() + + assert.Equal(t, "multi-az", DeploymentFromDetails(&DatabaseDetails{AZConfig: "multi-az"})) + assert.Empty(t, DeploymentFromDetails(nil), "untyped nil details") + assert.Empty(t, DeploymentFromDetails(&CacheDetails{Engine: "redis"}), "non-database details") + assert.Empty(t, DeploymentFromDetails(&DatabaseDetails{}), "database details with no AZConfig") +} + +// A typed nil stored in the interface satisfies the type assertion, so +// without the type-specific nil guard this panics rather than returning "". +func TestDeploymentFromDetails_TypedNilDoesNotPanic(t *testing.T) { + t.Parallel() + var typedNil *DatabaseDetails + assert.NotPanics(t, func() { + assert.Empty(t, DeploymentFromDetails(typedNil)) + }) +} diff --git a/pkg/recfilter/dedupe.go b/pkg/recfilter/dedupe.go index 98b050c06..14fb175c5 100644 --- a/pkg/recfilter/dedupe.go +++ b/pkg/recfilter/dedupe.go @@ -83,14 +83,26 @@ func isRecentActiveCommitment(c common.Commitment, cutoffTime time.Time) bool { return (c.State == "active" || c.State == "payment-pending") && c.StartDate.After(cutoffTime) } -// buildExistingCommitmentsMap builds a map of commitments by resource type, region, and engine. +// dedupeKey builds the duplicate-identity key shared by +// buildExistingCommitmentsMap and adjustSingleRecommendation. Both call +// sites MUST build the key through this function: a Single-AZ and a +// Multi-AZ RDS commitment/recommendation are priced and provisioned +// differently and do not cover each other's demand, so deployment has to +// be part of the identity or the two keys can drift and silently +// suppress (or fail to suppress) the wrong recommendation. +func dedupeKey(resourceType, region, engine, deployment string) string { + return fmt.Sprintf("%s|%s|%s|%s", resourceType, region, engine, deployment) +} + +// buildExistingCommitmentsMap builds a map of commitments by resource type, region, engine, and deployment. func buildExistingCommitmentsMap(commitments []common.Commitment, logf Logf) map[string]int { existingMap := make(map[string]int) for _rvc := range commitments { c := commitments[_rvc] normalizedEngine := common.NormalizeEngineName(c.Engine) - key := fmt.Sprintf("%s|%s|%s", c.ResourceType, c.Region, normalizedEngine) + normalizedDeployment := common.NormalizeDeploymentName(c.Deployment) + key := dedupeKey(c.ResourceType, c.Region, normalizedEngine, normalizedDeployment) existingMap[key] += c.Count logf.printf(" [DuplicateChecker] Recent RI: key=%s count=%d startDate=%s (raw engine=%s)", key, c.Count, c.StartDate.Format("2006-01-02 15:04:05"), c.Engine) @@ -121,7 +133,8 @@ func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existing // adjustSingleRecommendation adjusts a single recommendation based on existing commitments. func adjustSingleRecommendation(rec common.Recommendation, existingMap map[string]int, logf Logf) common.Recommendation { engine := common.EngineFromDetails(rec.Details) - key := fmt.Sprintf("%s|%s|%s", rec.ResourceType, rec.Region, engine) + deployment := common.NormalizeDeploymentName(common.DeploymentFromDetails(rec.Details)) + key := dedupeKey(rec.ResourceType, rec.Region, engine, deployment) existingCount := existingMap[key] if existingCount >= rec.Count { diff --git a/pkg/recfilter/dedupe_test.go b/pkg/recfilter/dedupe_test.go index fbd9b0e80..2f9d7f6b9 100644 --- a/pkg/recfilter/dedupe_test.go +++ b/pkg/recfilter/dedupe_test.go @@ -208,3 +208,98 @@ func TestAdjustRecommendationsForExisting_LogfReceivesDecisionTrail(t *testing.T } assert.True(t, found) } + +// TestAdjustRecommendationsForExisting_SingleAZCommitmentDoesNotSuppressMultiAZRec +// is a regression test for a duplicate-identity key that omitted the RDS +// deployment dimension. A recent Single-AZ commitment must not reduce or +// drop a Multi-AZ recommendation for the same instance type/region/engine: +// the two are priced and provisioned differently and do not cover each +// other's demand. +func TestAdjustRecommendationsForExisting_SingleAZCommitmentDoesNotSuppressMultiAZRec(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "db.r5.large", Region: "us-east-1", Engine: "mysql", Deployment: "single-az", Count: 5, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + rec := common.Recommendation{ + ResourceType: "db.r5.large", Region: "us-east-1", Count: 5, + Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: "multi-az"}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec}, client) + + require.NoError(t, err) + assert.Empty(t, filtered) + require.Len(t, passed, 1) + assert.Equal(t, rec.Count, passed[0].Count) +} + +// TestAdjustRecommendationsForExisting_MultiAZCommitmentDoesNotSuppressSingleAZRec +// is the mirror direction of the above: a recent Multi-AZ commitment must +// not suppress a Single-AZ recommendation. +func TestAdjustRecommendationsForExisting_MultiAZCommitmentDoesNotSuppressSingleAZRec(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "db.r5.large", Region: "us-east-1", Engine: "mysql", Deployment: "multi-az", Count: 5, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + rec := common.Recommendation{ + ResourceType: "db.r5.large", Region: "us-east-1", Count: 5, + Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: "single-az"}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec}, client) + + require.NoError(t, err) + assert.Empty(t, filtered) + require.Len(t, passed, 1) + assert.Equal(t, rec.Count, passed[0].Count) +} + +// TestAdjustRecommendationsForExisting_SingleAZCommitmentStillSuppressesSingleAZRec +// is the positive control: matching deployment on both sides must still +// dedupe, so the fix above does not simply disable deduplication for RDS. +func TestAdjustRecommendationsForExisting_SingleAZCommitmentStillSuppressesSingleAZRec(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "db.r5.large", Region: "us-east-1", Engine: "mysql", Deployment: "single-az", Count: 5, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + rec := common.Recommendation{ + ResourceType: "db.r5.large", Region: "us-east-1", Count: 5, + Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: "single-az"}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec}, client) + + require.NoError(t, err) + assert.Empty(t, passed) + require.Len(t, filtered, 1) +} + +// TestAdjustRecommendationsForExisting_NonRDSCommitmentStillDeduplicates +// guards the non-RDS path: a commitment/recommendation pair with no +// deployment dimension at all (e.g. EC2) must still land on the same key +// and dedupe, so adding deployment to the key doesn't regress non-RDS +// resource types that never populate it. +func TestAdjustRecommendationsForExisting_NonRDSCommitmentStillDeduplicates(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + {ResourceType: "m5.large", Region: "us-east-1", Count: 5, State: "active", StartDate: time.Now().Add(-1 * time.Hour)}, + }} + rec := common.Recommendation{ + ResourceType: "m5.large", Region: "us-east-1", Count: 5, + Details: &common.ComputeDetails{Platform: "linux"}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec}, client) + + require.NoError(t, err) + assert.Empty(t, passed) + require.Len(t, filtered, 1) +} From 48d1f297145c2131e2c7709b8ffbe3048053f0c6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 27 Aug 2026 15:25:19 +0200 Subject: [PATCH 08/12] docs(recfilter): correct the RI target-coverage formula in the doc block The doc block still described n_target = floor(rec.Count * gap / remaining_gap), the superseded rec.Count-anchored formula. The code computes floor(avg * gap / 100), anchored on AverageInstancesUsedPerHour, and the no-signal case reduces to floor(avg * target/100) rather than target% of AWS's count. Documentation for a money-sizing formula that contradicts the code is worse than none, since a reader checking the arithmetic would confirm the wrong thing. Carried over stale in the extraction; corrected here, with a note on why the anchor changed. Refs #1883 --- pkg/recfilter/sizing.go | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/pkg/recfilter/sizing.go b/pkg/recfilter/sizing.go index 45c3164d4..3d1357060 100644 --- a/pkg/recfilter/sizing.go +++ b/pkg/recfilter/sizing.go @@ -97,17 +97,21 @@ func ApplyCoverage(recs []common.Recommendation, coverage float64, logf Logf, dr // // RIs (existing-aware, per-pool, strict-target): // -// gap = targetPct - ExistingCoveragePct (percentage points) -// remaining_gap = 100 - ExistingCoveragePct (percentage points) -// n_target = floor(rec.Count * gap / remaining_gap) +// gap = targetPct - ExistingCoveragePct (percentage points) +// avg = AverageInstancesUsedPerHour (instances) +// n_target = floor(avg * gap / 100) // -// The formula scales AWS's per-account-incremental rec.Count by the -// fraction of the current-to-100% gap we want to fill. For example -// with existing=50% and target=80%: gap=30, remaining_gap=50, so we -// buy 30/50 = 60% of AWS's rec.Count. Anchoring to rec.Count (which -// AWS computed per-linked-account) is more robust in multi-account -// orgs than scaling against avg, since CE's ExistingCoveragePct is -// org-wide averaged and mixes accounts together. +// The buy is anchored on the pool's own average demand and the absolute +// gap to target: target%-existing% of avg instances. For example with +// avg=10, existing=50% and target=80%: gap=30, so n_target=floor(3)=3. +// +// An earlier version anchored on AWS's rec.Count +// (floor(rec.Count * gap / (100-existing))). That under-bought when AWS +// sized rec.Count for less than full coverage, and when CE's org-wide +// ExistingCoveragePct disagreed with rec.Count's per-account derivation. +// Both inputs of the current formula come from GetReservationCoverage, so +// the buy lines up with the AWS console's reservations-coverage report. +// rec.Count survives only as the denominator of the cost-scaling ratio. // // If gap <= 0 (existing already at/above target) → drop with INFO log. // If n_target == 0 (gap too small to fit one RI) → drop with INFO log. @@ -119,7 +123,8 @@ func ApplyCoverage(recs []common.Recommendation, coverage float64, logf Logf, dr // // ExistingCoveragePct is sourced from CE GetReservationCoverage in the // same pool; zero means "no signal" and the formula reduces to -// floor(rec.Count * target/100) — i.e. plain target% of AWS's count. +// floor(avg * target/100) — i.e. plain target% of the pool's average +// hourly demand. // For RDS the coverage lookup keys by (region, instance_type, engine). // Floor (rather than ceil or round) gives strict "at-most-target" // sizing. Pools too small to approximate the target meaningfully From 359e7b73274b08387f046a7af7dea111a51ef792 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 28 Aug 2026 21:56:03 +0200 Subject: [PATCH 09/12] fix(common): normalize engine aliases case-insensitively --- pkg/common/engine.go | 24 +++++++++---------- pkg/common/engine_test.go | 27 ++++++++++++++++++++++ pkg/common/matches_test.go | 21 +++++++++++++++++ pkg/recfilter/dedupe_test.go | 30 ++++++++++++++++++++++++ pkg/recfilter/filters_test.go | 43 +++++++++++++++++++++++++++++++++++ 5 files changed, 132 insertions(+), 13 deletions(-) diff --git a/pkg/common/engine.go b/pkg/common/engine.go index b9c99ca80..5277267bb 100644 --- a/pkg/common/engine.go +++ b/pkg/common/engine.go @@ -7,20 +7,17 @@ import "strings" // Cost Explorer uses: "Aurora PostgreSQL", "Aurora MySQL", "MySQL", "PostgreSQL" var engineNameMap = map[string]string{ // Cost Explorer format -> normalized - "Aurora PostgreSQL": "aurora-postgresql", - "Aurora MySQL": "aurora-mysql", - "MySQL": "mysql", - "PostgreSQL": "postgresql", - "MariaDB": "mariadb", - "Oracle": "oracle", - "SQL Server": "sqlserver", + "aurora postgresql": "aurora-postgresql", + "aurora mysql": "aurora-mysql", + "mysql": "mysql", + "postgresql": "postgresql", + "mariadb": "mariadb", + "oracle": "oracle", + "sql server": "sqlserver", // Already normalized (from AWS RIs) "aurora-postgresql": "aurora-postgresql", "aurora-mysql": "aurora-mysql", - "mysql": "mysql", - "postgresql": "postgresql", "postgres": "postgresql", - "mariadb": "mariadb", "oracle-se": "oracle", "oracle-se1": "oracle", "oracle-se2": "oracle", @@ -34,10 +31,11 @@ var engineNameMap = map[string]string{ // NormalizeEngineName normalizes database engine names to a consistent format. // Returns lowercase of the input as a fallback when the engine is not recognized. func NormalizeEngineName(engine string) string { - if normalized, ok := engineNameMap[engine]; ok { - return normalized + normalized := strings.ToLower(engine) + if normalizedEngine, ok := engineNameMap[normalized]; ok { + return normalizedEngine } - return strings.ToLower(engine) + return normalized } // EngineFromDetails extracts and normalizes the engine name from diff --git a/pkg/common/engine_test.go b/pkg/common/engine_test.go index 19347424e..8efa76f49 100644 --- a/pkg/common/engine_test.go +++ b/pkg/common/engine_test.go @@ -6,6 +6,33 @@ import ( "github.com/stretchr/testify/assert" ) +func TestNormalizeEngineName_CaseInsensitiveRecognizedAliases(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + engine string + want string + }{ + {name: "uppercase aurora postgresql", engine: "AURORA POSTGRESQL", want: "aurora-postgresql"}, + {name: "title case aurora postgresql", engine: "Aurora PostgreSQL", want: "aurora-postgresql"}, + {name: "uppercase postgres alias", engine: "POSTGRES", want: "postgresql"}, + {name: "title case postgres alias", engine: "Postgres", want: "postgresql"}, + {name: "uppercase sql server", engine: "SQL SERVER", want: "sqlserver"}, + {name: "uppercase oracle ee", engine: "ORACLE-EE", want: "oracle"}, + {name: "uppercase sqlserver web", engine: "SQLSERVER-WEB", want: "sqlserver"}, + {name: "unknown engine preserves fallback shape", engine: " Custom Engine ", want: " custom engine "}, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, NormalizeEngineName(tt.engine)) + }) + } +} + // TestEngineFromDetails covers the pointer-only dispatch documented in // service_details_codec.go's package doc, plus the typed-nil guard. // diff --git a/pkg/common/matches_test.go b/pkg/common/matches_test.go index 954545775..0d61e27ac 100644 --- a/pkg/common/matches_test.go +++ b/pkg/common/matches_test.go @@ -70,6 +70,27 @@ func TestMatches_NormalizedEngine(t *testing.T) { assert.True(t, Matches(rec, c)) } +func TestMatches_UppercaseRecognizedCommitmentAlias(t *testing.T) { + t.Parallel() + + rec := Recommendation{ + Provider: ProviderAWS, + Region: "us-east-1", + Service: ServiceRDS, + ResourceType: "db.r5.large", + Details: &DatabaseDetails{Engine: "aurora-postgresql"}, + } + c := Commitment{ + Provider: ProviderAWS, + Region: "us-east-1", + Service: ServiceRDS, + ResourceType: "db.r5.large", + Engine: "AURORA POSTGRESQL", + } + + assert.True(t, Matches(rec, c)) +} + func TestMatches_NoDetails(t *testing.T) { t.Parallel() // Compute recommendations have no engine — both sides normalize to "" diff --git a/pkg/recfilter/dedupe_test.go b/pkg/recfilter/dedupe_test.go index 2f9d7f6b9..c9f97960c 100644 --- a/pkg/recfilter/dedupe_test.go +++ b/pkg/recfilter/dedupe_test.go @@ -79,6 +79,36 @@ func TestAdjustRecommendationsForExisting_EngineNormalizationCollides(t *testing assert.Len(t, filtered, 1) } +func TestAdjustRecommendationsForExisting_UppercaseRecognizedAliasCollides(t *testing.T) { + t.Parallel() + + ctx := context.Background() + client := &fakeServiceClient{commitments: []common.Commitment{ + { + ResourceType: "db.r5.large", + Region: "us-east-1", + Engine: "AURORA POSTGRESQL", + Deployment: "single-az", + Count: 1, + State: "active", + StartDate: time.Now().Add(-1 * time.Hour), + }, + }} + rec := common.Recommendation{ + ResourceType: "db.r5.large", + Region: "us-east-1", + Count: 1, + Details: &common.DatabaseDetails{Engine: "aurora-postgresql", AZConfig: "single-az"}, + } + + d := NewDuplicateChecker(0) + passed, filtered, err := d.AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec}, client) + + require.NoError(t, err) + assert.Empty(t, passed) + assert.Len(t, filtered, 1) +} + func TestAdjustRecommendationsForExisting_FullCoverageDrops(t *testing.T) { t.Parallel() ctx := context.Background() diff --git a/pkg/recfilter/filters_test.go b/pkg/recfilter/filters_test.go index 12015c259..3927a4309 100644 --- a/pkg/recfilter/filters_test.go +++ b/pkg/recfilter/filters_test.go @@ -9,6 +9,8 @@ import ( ) func TestIncludesEngine_CEAndRISpellingsBothMatch(t *testing.T) { + t.Parallel() + tests := []struct { name string recEngine string @@ -39,6 +41,47 @@ func TestIncludesEngine_CEAndRISpellingsBothMatch(t *testing.T) { } } +func TestIncludesEngine_UppercaseRecognizedAliasFilters(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + recEngine string + includeEngines []string + excludeEngines []string + want bool + }{ + { + name: "uppercase aurora postgresql include matches canonical recommendation", + recEngine: "aurora-postgresql", + includeEngines: []string{"AURORA POSTGRESQL"}, + want: true, + }, + { + name: "uppercase aurora postgresql exclude removes canonical recommendation", + recEngine: "aurora-postgresql", + excludeEngines: []string{"AURORA POSTGRESQL"}, + want: false, + }, + { + name: "uppercase postgres short alias include matches postgresql recommendation", + recEngine: "postgresql", + includeEngines: []string{"POSTGRES"}, + want: true, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + f := Filters{IncludeEngines: tt.includeEngines, ExcludeEngines: tt.excludeEngines} + rec := common.Recommendation{Details: &common.DatabaseDetails{Engine: tt.recEngine}} + assert.Equal(t, tt.want, f.IncludesEngine(&rec)) + }) + } +} + func TestIncludesRegion_EmptyIncludeListAllowsAll(t *testing.T) { f := Filters{} assert.True(t, f.IncludesRegion("us-east-1")) From d73b93fbaf73241869b57340ee0d760a0e219627 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 28 Aug 2026 21:59:44 +0200 Subject: [PATCH 10/12] test(cmd): use CI spelling in min-pool filter test --- cmd/multi_service_filters_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/multi_service_filters_test.go b/cmd/multi_service_filters_test.go index 33298f12b..58a666130 100644 --- a/cmd/multi_service_filters_test.go +++ b/cmd/multi_service_filters_test.go @@ -717,7 +717,7 @@ func TestApplyFilters_MinPoolSizeMultiRegionMatchesPreExtractionBehaviour(t *tes resultOld := applyFiltersPreExtraction(makeRecs(), &toolCfg, make(map[string][]InstanceEngineVersion), make(map[string]MajorEngineVersionInfo), region, dropsOld) assert.Equal(t, resultOld, resultNew, - "region %s: refactored applyFilters diverged from the pre-extraction oracle -- the ApplyMinPoolSize extraction changed observable CLI filtering behaviour", region) + "region %s: refactored applyFilters diverged from the pre-extraction oracle -- the ApplyMinPoolSize extraction changed observable CLI filtering behavior", region) assert.Equal(t, dropsOld.FormatOneLine(), dropsNew.FormatOneLine(), "region %s: drop summaries diverged between pre- and post-extraction implementations", region) assert.Equal(t, dropsOld.Total(), dropsNew.Total(), @@ -742,7 +742,7 @@ func TestApplyFilters_MinPoolSizeMultiRegionMatchesPreExtractionBehaviour(t *tes require.Equal(t, totalDropMinPoolSizeOld, totalDropMinPoolSizeNew, "old and new implementations must agree on the summed multi-region --min-pool-size drop count") - // Pre-existing behaviour, identical in both implementations (not introduced + // Pre-existing behavior, identical in both implementations (not introduced // by the ApplyMinPoolSize extraction): each per-region call re-scans the // FULL recommendation set passed to it, not a per-region subset, so a // below-threshold recommendation from one region is re-counted as dropped From 8de4a5bb5263142bd3900085ca484376a3a5dde9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 29 Aug 2026 01:04:52 +0200 Subject: [PATCH 11/12] fix(common): create audit preflight logs with readable mode --- pkg/common/audit.go | 2 +- pkg/common/audit_permissions_unix_test.go | 44 +++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 pkg/common/audit_permissions_unix_test.go diff --git a/pkg/common/audit.go b/pkg/common/audit.go index 1b951a97b..edecf9308 100644 --- a/pkg/common/audit.go +++ b/pkg/common/audit.go @@ -40,7 +40,7 @@ func WriteAuditRecord(record AuditRecord, path string) error { // CheckAuditLogWritable opens the audit log file in append mode to verify it is writable. // Returns an error if the path cannot be opened for writing. func CheckAuditLogWritable(path string) error { - f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600) // #nosec G304 -- audit log path is operator-configured; value is not reachable from user input + f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) // #nosec G302,G304 -- 0644 intentionally matches WriteAuditRecord; path is operator-configured. if err != nil { return fmt.Errorf("audit log %q not writable: %w", path, err) } diff --git a/pkg/common/audit_permissions_unix_test.go b/pkg/common/audit_permissions_unix_test.go new file mode 100644 index 000000000..272242483 --- /dev/null +++ b/pkg/common/audit_permissions_unix_test.go @@ -0,0 +1,44 @@ +//go:build unix + +package common + +import ( + "os" + "path/filepath" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCheckAuditLogWritable_CreatesMissingAuditLogReadableByDownstreamTools(t *testing.T) { + old := syscall.Umask(0) + t.Cleanup(func() { syscall.Umask(old) }) + + path := filepath.Join(t.TempDir(), "audit.jsonl") + + require.NoError(t, CheckAuditLogWritable(path)) + + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o644), info.Mode().Perm()) +} + +func TestCheckAuditLogWritable_DoesNotChmodExistingAuditLog(t *testing.T) { + old := syscall.Umask(0) + t.Cleanup(func() { syscall.Umask(old) }) + + path := filepath.Join(t.TempDir(), "audit.jsonl") + require.NoError(t, os.WriteFile(path, []byte("line1\n"), 0o600)) + + require.NoError(t, CheckAuditLogWritable(path)) + + data, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "line1\n", string(data)) + + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) +} From 26693a7231828009e6eabaf228992bb8d6720910 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 29 Aug 2026 01:09:25 +0200 Subject: [PATCH 12/12] test(recfilter): cover existing-coverage target sizing --- pkg/recfilter/sizing_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pkg/recfilter/sizing_test.go b/pkg/recfilter/sizing_test.go index cca3495a6..ef3db120b 100644 --- a/pkg/recfilter/sizing_test.go +++ b/pkg/recfilter/sizing_test.go @@ -250,6 +250,18 @@ func TestApplyTargetCoverage_ProjectionsClampTo100(t *testing.T) { assert.LessOrEqual(t, out[0].ProjectedCoverage, 100.0) assert.Equal(t, 100.0, out[0].ProjectedCoverage) }) + + t.Run("Nonzero existing coverage keeps only the remaining target gap", func(t *testing.T) { + t.Parallel() + // avg=10, target=80, existing=50: gap=30, nTarget=floor(10*30/100)=3. + // The kept recommendation should size only the uncovered gap. + rec := mkRI(10, 10, 50) + out := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil, nil) + require.Len(t, out, 1) + assert.Equal(t, 3, out[0].Count) + assert.Equal(t, 80.0, out[0].ProjectedCoverage) + assert.InDelta(t, 300.0, out[0].CommitmentCost, 0.001) + }) } // TestApplyTargetCoverage_CountZeroNoNaNOrInf covers the rec.Count==0