From ae714be5eef0c3933fa56ab25aa2afa6b067ddd7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 20:38:29 +0200 Subject: [PATCH] fix(recfilter): match EC2 commitments on platform, tenancy and scope when deduplicating AWS EC2 and compute commitments were deduplicated by region and instance type alone, so a Windows or dedicated reservation suppressed a Linux or default-tenancy recommendation. They now match on normalized platform and tenancy (empty on either side is a wildcard) and on scope. Scope is asymmetric: a regional reservation covers regional and zonal demand, a zonal one covers only its own zone, and an unknown scope on either side is a wildcard. Entries are kept in a stable order, fewest wildcards consumed first, and a recommendation is resized once to what remains. Account stays out of the key, so a commitment in another account still suppresses. Every other service keeps its existing key. Refs #323 Co-Authored-By: Claude Opus 5.5 --- pkg/recfilter/dedupe.go | 12 +- pkg/recfilter/dedupe_ec2.go | 189 +++++++++++++++++++++++++++++++ pkg/recfilter/dedupe_ec2_test.go | 189 +++++++++++++++++++++++++++++++ 3 files changed, 387 insertions(+), 3 deletions(-) create mode 100644 pkg/recfilter/dedupe_ec2.go create mode 100644 pkg/recfilter/dedupe_ec2_test.go diff --git a/pkg/recfilter/dedupe.go b/pkg/recfilter/dedupe.go index e3aa9624..0bd528ec 100644 --- a/pkg/recfilter/dedupe.go +++ b/pkg/recfilter/dedupe.go @@ -68,7 +68,7 @@ func (d *DuplicateChecker) AdjustRecommendationsForExisting(ctx context.Context, existingMap := buildExistingCommitmentsMap(recentExisting, d.Logf) d.Logf.printf(" [DuplicateChecker] Existing map has %d unique keys", len(existingMap)) - passed, filtered, err = adjustRecommendationsAgainstExisting(recs, existingMap, d.Logf) + passed, filtered, err = adjustRecommendationsAgainstExisting(recs, existingMap, newEC2Dedupe(recentExisting, d.Logf), d.Logf) if err != nil { return nil, nil, err } @@ -226,13 +226,19 @@ func buildExistingCommitmentsMap(commitments []common.Commitment, logf Logf) map // 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, err error) { +func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existingMap map[string]int, ec2 *ec2Dedupe, logf Logf) (passed, filtered []common.Recommendation, err error) { passed = make([]common.Recommendation, 0, len(recs)) filtered = make([]common.Recommendation, 0) for _rvc := range recs { rec := recs[_rvc] - adjusted, err := adjustSingleRecommendation(rec, existingMap, logf) + var adjusted common.Recommendation + var err error + if ec2.handles(rec) { + adjusted, err = ec2.adjust(rec) + } else { + adjusted, err = adjustSingleRecommendation(rec, existingMap, logf) + } if err != nil { return nil, nil, err } diff --git a/pkg/recfilter/dedupe_ec2.go b/pkg/recfilter/dedupe_ec2.go new file mode 100644 index 00000000..f386be43 --- /dev/null +++ b/pkg/recfilter/dedupe_ec2.go @@ -0,0 +1,189 @@ +package recfilter + +import ( + "fmt" + "sort" + "strings" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +type scopeKind int + +const ( + scopeUnknown scopeKind = iota + scopeRegionalKind + scopeZonalKind +) + +// scopeState is the normalized scope of an EC2 commitment or recommendation. +type scopeState struct { + kind scopeKind + az string +} + +func newScopeState(rawScope, az string) scopeState { + az = strings.ToLower(strings.TrimSpace(az)) + scope := common.NormalizeScopeName(rawScope) + switch { + case az != "" || scope == common.ScopeZonal: + return scopeState{kind: scopeZonalKind, az: az} + case scope == common.ScopeRegional: + return scopeState{kind: scopeRegionalKind} + default: + return scopeState{} + } +} + +// scopeCovers reports whether an entry of the given scope can pay for a +// recommendation of the given scope. A regional reservation covers any zone's +// usage but a zonal one does not cover regional demand, so the rule is +// asymmetric. Unknown scope on either side is a wildcard. +func scopeCovers(entry, rec scopeState) bool { + switch { + case entry.kind == scopeUnknown || rec.kind == scopeUnknown: + return true + case entry.kind == scopeRegionalKind: + return true + case rec.kind != scopeZonalKind: + return false + default: + return entry.az == "" || rec.az == "" || entry.az == rec.az + } +} + +type ec2Entry struct { + platform, tenancy string + scope scopeState + count, wildcards int +} + +func (e *ec2Entry) covers(platform, tenancy string, scope scopeState) bool { + return (e.platform == "" || platform == "" || e.platform == platform) && + (e.tenancy == "" || tenancy == "" || e.tenancy == tenancy) && + scopeCovers(e.scope, scope) +} + +// ec2Dedupe holds the AWS EC2 commitments by region and instance type. Account +// is deliberately not part of the identity: a commitment in another account +// still suppresses, as for every other service. +type ec2Dedupe struct { + entries map[string][]*ec2Entry + warned map[string]bool + logf Logf +} + +func isAWSCompute(p common.ProviderType, s common.ServiceType) bool { + return p == common.ProviderAWS && (s == common.ServiceEC2 || s == common.ServiceCompute) +} + +func ec2BaseKey(resourceType, region string) string { + return fmt.Sprintf("ec2|%s|%s", resourceType, region) +} + +func newEC2Dedupe(commitments []common.Commitment, logf Logf) *ec2Dedupe { + d := &ec2Dedupe{entries: map[string][]*ec2Entry{}, warned: map[string]bool{}, logf: logf} + for i := range commitments { + c := &commitments[i] + if !isAWSCompute(c.Provider, c.Service) { + continue + } + platform, tenancy, scope := d.normalize(c.Platform, c.Tenancy, c.Scope, c.AvailabilityZone) + e := &ec2Entry{platform: platform, tenancy: tenancy, scope: scope, count: c.Count} + e.wildcards = wildcardCount(e) + key := ec2BaseKey(c.ResourceType, c.Region) + d.entries[key] = append(d.entries[key], e) + } + for _, list := range d.entries { + sort.SliceStable(list, func(i, j int) bool { return list[i].wildcards < list[j].wildcards }) + } + return d +} + +func wildcardCount(e *ec2Entry) int { + n := 0 + for _, wild := range []bool{e.platform == "", e.tenancy == "", e.scope.kind == scopeUnknown, + e.scope.kind == scopeZonalKind && e.scope.az == ""} { + if wild { + n++ + } + } + return n +} + +func (d *ec2Dedupe) normalize(rawPlatform, rawTenancy, rawScope, az string) (platform, tenancy string, scope scopeState) { + platform = common.NormalizePlatformName(rawPlatform) + tenancy = common.NormalizeTenancyName(rawTenancy) + d.warnIfUnrecognized("platform", rawPlatform, knownPlatform(platform)) + d.warnIfUnrecognized("tenancy", rawTenancy, tenancy) + d.warnIfUnrecognized("scope", rawScope, common.NormalizeScopeName(rawScope)) + return platform, tenancy, newScopeState(rawScope, az) +} + +// knownPlatform returns platform when it is an EC2 product description, +// else "". NormalizePlatformName never returns "" for non-empty input, so the +// unrecognized case is detected here. +func knownPlatform(platform string) string { + if platform == "linux/unix" || strings.HasPrefix(platform, "windows") || + strings.HasPrefix(platform, "red hat") || strings.HasPrefix(platform, "suse") || strings.HasPrefix(platform, "linux with sql") { + return platform + } + return "" +} + +func (d *ec2Dedupe) warnIfUnrecognized(dim, raw, normalized string) { + raw = strings.TrimSpace(raw) + if raw == "" || normalized != "" || d.warned[dim+"="+raw] { + return + } + d.warned[dim+"="+raw] = true + effect := "treating it as a wildcard" + if dim == "platform" { + effect = "matching it only against the same platform" + } + d.logf.printf(" [DuplicateChecker] WARNING: unrecognized EC2 %s %q; %s", dim, raw, effect) +} + +// ec2DimsFromDetails reads the EC2 dimensions from ComputeDetails, held by +// value, by pointer, or as a typed nil. +func ec2DimsFromDetails(details common.ServiceDetails) (platform, tenancy, scope, az string) { + switch d := details.(type) { + case common.ComputeDetails: + return d.Platform, d.Tenancy, d.Scope, d.AvailabilityZone + case *common.ComputeDetails: + if d != nil { + return d.Platform, d.Tenancy, d.Scope, d.AvailabilityZone + } + } + return "", "", "", "" +} + +func (d *ec2Dedupe) handles(rec common.Recommendation) bool { + return isAWSCompute(rec.Provider, rec.Service) +} + +// adjust consumes matching entries, fewest wildcards first, and resizes the +// recommendation once to what is left. Entries are shared across +// recommendations, so which recommendation is suppressed depends on their +// order. +func (d *ec2Dedupe) adjust(rec common.Recommendation) (common.Recommendation, error) { + rawPlatform, rawTenancy, rawScope, rawAZ := ec2DimsFromDetails(rec.Details) + platform, tenancy, scope := d.normalize(rawPlatform, rawTenancy, rawScope, rawAZ) + remaining := rec.Count + for _, e := range d.entries[ec2BaseKey(rec.ResourceType, rec.Region)] { + if remaining <= 0 { + break + } + if e.count <= 0 || !e.covers(platform, tenancy, scope) { + continue + } + used := min(e.count, remaining) + e.count -= used + remaining -= used + } + if remaining <= 0 { + d.logf.printf(" [DuplicateChecker] SKIP %s: recent commitments cover recommended %d", ec2BaseKey(rec.ResourceType, rec.Region), rec.Count) + return common.Recommendation{Count: 0}, nil + } + return common.ResizeCount(rec, remaining) +} diff --git a/pkg/recfilter/dedupe_ec2_test.go b/pkg/recfilter/dedupe_ec2_test.go new file mode 100644 index 00000000..5f67b04c --- /dev/null +++ b/pkg/recfilter/dedupe_ec2_test.go @@ -0,0 +1,189 @@ +package recfilter + +import ( + "context" + "fmt" + "strings" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestScopeCovers(t *testing.T) { + t.Parallel() + regional, unknown := scopeState{kind: scopeRegionalKind}, scopeState{} + zonalA, zonalB := scopeState{kind: scopeZonalKind, az: "us-east-1a"}, scopeState{kind: scopeZonalKind, az: "us-east-1b"} + zonalAny := scopeState{kind: scopeZonalKind} + tests := []struct { + name string + entry, rec scopeState + want bool + }{ + {"regional covers regional", regional, regional, true}, + {"regional covers zonal", regional, zonalA, true}, + {"regional covers unknown", regional, unknown, true}, + {"zonal a covers zonal a", zonalA, zonalA, true}, + {"zonal a covers unknown", zonalA, unknown, true}, + {"zonal a not zonal b", zonalA, zonalB, false}, + {"zonal a not regional", zonalA, regional, false}, + {"zonal no az covers zonal a", zonalAny, zonalA, true}, + {"zonal no az covers unknown", zonalAny, unknown, true}, + {"zonal no az not regional", zonalAny, regional, false}, + {"unknown covers regional", unknown, regional, true}, + {"unknown covers zonal", unknown, zonalA, true}, + {"unknown covers unknown", unknown, unknown, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, scopeCovers(tt.entry, tt.rec)) + }) + } + assert.Equal(t, zonalA, newScopeState("", " US-East-1A ")) + assert.Equal(t, unknown, newScopeState("bogus", "")) +} + +func ec2Commitment(platform, tenancy, scope, az string, count int) common.Commitment { + return common.Commitment{Provider: common.ProviderAWS, Service: common.ServiceEC2, ResourceType: "m5.large", + Region: "us-east-1", Platform: platform, Tenancy: tenancy, Scope: scope, AvailabilityZone: az, Count: count, + State: common.CommitmentStateActive, StartDate: time.Now(), Account: "acct-a"} +} + +func ec2Rec(platform, tenancy, scope, az string, count int) common.Recommendation { + monthly := 10.0 * float64(count) + return common.Recommendation{Provider: common.ProviderAWS, Service: common.ServiceCompute, ResourceType: "m5.large", + Region: "us-east-1", Count: count, EstimatedSavings: monthly, RecurringMonthlyCost: &monthly, Account: "acct-b", + Details: &common.ComputeDetails{InstanceType: "m5.large", Platform: platform, Tenancy: tenancy, Scope: scope, AvailabilityZone: az}} +} + +func runEC2(t *testing.T, logf Logf, commitments []common.Commitment, recs ...common.Recommendation) []common.Recommendation { + t.Helper() + checker := NewDuplicateChecker(0) + checker.Logf = logf + passed, _, err := checker.AdjustRecommendationsForExisting(context.Background(), recs, &fakeServiceClient{commitments: commitments}) + require.NoError(t, err) + return passed +} + +func TestEC2DedupeMatching(t *testing.T) { + t.Parallel() + tests := []struct { + name string + commitment common.Commitment + rec common.Recommendation + suppressed bool + }{ + {"windows RI does not cover linux", ec2Commitment("Windows", "default", "regional", "", 1), ec2Rec("Linux/UNIX", "default", "regional", "", 1), false}, + {"dedicated does not cover default", ec2Commitment("linux", "dedicated", "regional", "", 1), ec2Rec("linux", "default", "regional", "", 1), false}, + {"zonal RI does not cover regional rec", ec2Commitment("linux", "default", "zonal", "us-east-1a", 1), ec2Rec("linux", "default", "regional", "", 1), false}, + {"zonal RI does not cover other zone", ec2Commitment("linux", "default", "zonal", "us-east-1a", 1), ec2Rec("linux", "default", "zonal", "us-east-1b", 1), false}, + {"regional RI covers zonal rec", ec2Commitment("linux", "default", "regional", "", 1), ec2Rec("linux", "default", "zonal", "us-east-1a", 1), true}, + {"zonal RI covers same zone, case-insensitive", ec2Commitment("linux", "default", "zonal", "US-EAST-1A", 1), ec2Rec("linux", "default", "zonal", "us-east-1a", 1), true}, + {"amazon vpc suffix", ec2Commitment("Linux/UNIX (Amazon VPC)", "default", "regional", "", 1), ec2Rec("Linux/UNIX", "default", "regional", "", 1), true}, + {"shared is default", ec2Commitment("linux", "shared", "regional", "", 1), ec2Rec("linux", "default", "regional", "", 1), true}, + {"Region is regional", ec2Commitment("linux", "default", "Region", "", 1), ec2Rec("linux", "default", "regional", "", 1), true}, + {"commitment platform wildcard", ec2Commitment("", "default", "regional", "", 1), ec2Rec("windows", "default", "regional", "", 1), true}, + {"rec platform wildcard", ec2Commitment("windows", "default", "regional", "", 1), ec2Rec("", "default", "regional", "", 1), true}, + {"commitment tenancy wildcard", ec2Commitment("linux", "", "regional", "", 1), ec2Rec("linux", "dedicated", "regional", "", 1), true}, + {"rec tenancy wildcard", ec2Commitment("linux", "dedicated", "regional", "", 1), ec2Rec("linux", "", "regional", "", 1), true}, + {"commitment scope wildcard", ec2Commitment("linux", "default", "", "", 1), ec2Rec("linux", "default", "regional", "", 1), true}, + {"rec scope wildcard", ec2Commitment("linux", "default", "zonal", "us-east-1a", 1), ec2Rec("linux", "default", "", "", 1), true}, + {"different account still suppresses", ec2Commitment("linux", "default", "regional", "", 1), ec2Rec("linux", "default", "regional", "", 1), true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + passed := runEC2(t, nil, []common.Commitment{tt.commitment}, tt.rec) + assert.Equal(t, !tt.suppressed, len(passed) == 1) + }) + } +} + +func TestEC2DedupeValueDetails(t *testing.T) { + t.Parallel() + rec := ec2Rec("windows", "default", "regional", "", 1) + rec.Details = *rec.Details.(*common.ComputeDetails) + assert.Empty(t, runEC2(t, nil, []common.Commitment{ec2Commitment("windows", "default", "regional", "", 1)}, rec)) + var typedNil *common.ComputeDetails + rec.Details = typedNil + assert.Empty(t, runEC2(t, nil, []common.Commitment{ec2Commitment("windows", "default", "regional", "", 1)}, rec)) +} + +func TestEC2DedupeAzureNotKeyed(t *testing.T) { + t.Parallel() + c := common.Commitment{Provider: common.ProviderAzure, Service: common.ServiceCompute, ResourceType: "Standard_D2s_v5", + Region: "eastus", Platform: "windows", Count: 1, State: common.CommitmentStateActive, StartDate: time.Now()} + rec := common.Recommendation{Provider: common.ProviderAzure, Service: common.ServiceCompute, ResourceType: "Standard_D2s_v5", + Region: "eastus", Count: 1, Details: &common.ComputeDetails{Platform: "linux"}} + assert.Empty(t, runEC2(t, nil, []common.Commitment{c}, rec)) +} + +func TestEC2DedupeWildcardPreference(t *testing.T) { + t.Parallel() + commitments := []common.Commitment{ + ec2Commitment("", "", "", "", 2), + ec2Commitment("linux", "default", "regional", "", 1), + ec2Commitment("linux", "default", "regional", "", 1), + } + for i := 0; i < 5; i++ { + passed := runEC2(t, nil, commitments, ec2Rec("linux", "default", "regional", "", 5)) + require.Len(t, passed, 1) + assert.Equal(t, 1, passed[0].Count) + assert.InDelta(t, 10.0, passed[0].EstimatedSavings, 1e-9) + assert.InDelta(t, 10.0, *passed[0].RecurringMonthlyCost, 1e-9) + } + passed := runEC2(t, nil, commitments, ec2Rec("windows", "default", "regional", "", 1), ec2Rec("windows", "default", "regional", "", 1), ec2Rec("windows", "default", "regional", "", 1)) + assert.Len(t, passed, 1) +} + +func TestEC2DedupeEntryOrder(t *testing.T) { + t.Parallel() + // Fewest wildcards first: linux takes the exact entry, leaving the + // wildcard platform entry for windows. Reversed, windows would pass. + passed := runEC2(t, nil, []common.Commitment{ + ec2Commitment("", "default", "regional", "", 1), + ec2Commitment("linux", "default", "regional", "", 1), + }, ec2Rec("linux", "default", "regional", "", 1), ec2Rec("windows", "default", "regional", "", 1)) + assert.Empty(t, passed) + + // Equal wildcard counts keep input order: the first entry (zonal a) is + // consumed by the first rec, leaving only zone b for the second. + passed = runEC2(t, nil, []common.Commitment{ + ec2Commitment("linux", "default", "zonal", "us-east-1a", 1), + ec2Commitment("linux", "default", "zonal", "us-east-1b", 1), + }, ec2Rec("linux", "default", "", "", 1), ec2Rec("linux", "default", "zonal", "us-east-1a", 1)) + require.Len(t, passed, 1) + assert.Equal(t, "us-east-1a", passed[0].Details.(*common.ComputeDetails).AvailabilityZone) +} + +func TestEC2DedupeNoWarnForSQLServerPlatform(t *testing.T) { + var lines []string + logf := func(format string, args ...any) { lines = append(lines, fmt.Sprintf(format, args...)) } + runEC2(t, logf, []common.Commitment{ec2Commitment("Linux with SQL Server Standard", "default", "regional", "", 1)}, + ec2Rec("Linux with SQL Server Standard", "default", "regional", "", 1)) + for _, l := range lines { + assert.NotContains(t, l, "WARNING") + } +} + +func TestEC2DedupeWarnsOncePerUnrecognizedValue(t *testing.T) { + var lines []string + logf := func(format string, args ...any) { lines = append(lines, fmt.Sprintf(format, args...)) } + runEC2(t, logf, []common.Commitment{ec2Commitment("linux", "default", "weird-scope", "", 1)}, + ec2Rec("beos", "weird-tenancy", "weird-scope", "", 1), ec2Rec("linux", "weird-tenancy", "weird-scope", "", 1)) + var warnings []string + for _, l := range lines { + if strings.Contains(l, "WARNING") { + warnings = append(warnings, l) + } + } + require.Len(t, warnings, 3) + assert.Contains(t, warnings[0], `scope "weird-scope"`) + assert.Contains(t, warnings[0], "treating it as a wildcard") + assert.Contains(t, warnings[1], `platform "beos"`) + assert.Contains(t, warnings[1], "matching it only against the same platform") + assert.Contains(t, warnings[2], `tenancy "weird-tenancy"`) +}