From 68bfe9c5ae2dc73ceed0bf4e9b71181e0146a9e3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 03:50:54 +0100 Subject: [PATCH 1/3] fix(recfilter): dedupe each term and payment variant against the full held quantity The held budget was keyed without term or payment, so one held commitment reduced only the first variant of a recommendation in input order. Each term|payment group now starts from the full held quantity and consumes it in input order. An empty term or payment is its own group. Closes #703 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + pkg/recfilter/dedupe.go | 18 +++++- pkg/recfilter/dedupe_variants_test.go | 87 +++++++++++++++++++++++++++ 3 files changed, 105 insertions(+), 1 deletion(-) create mode 100644 pkg/recfilter/dedupe_variants_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 866981b9..e24d32da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -307,6 +307,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- **Duplicate check:** a recent purchase now reduces every term and payment variant of a recommendation, not only the first one in input order. Each `term|payment` group starts from the full held quantity and consumes it in input order, so one held unit against four variants of 2 yields 1, 1, 1, 1 (was 1, 2, 2, 2). Fixes #703. - **Breaking (source):** `managedredis.ReservationsDetailsPager` and `(*managedredis.Client).SetReservationsPager` are removed; tests inject reservation inventory with `SetInventoryFactories` instead (#699). - **`azure` Redis:** `managedredis.GetExistingCommitments` now reads held reservations from the Reservations API (purchase date, quantity, location, state), so a Redis reservation bought in the last day reduces the matching recommendation instead of being invisible to the duplicate guard (#699). An incomplete live reservation row (missing SKU, location, quantity or purchase date, an unattributable scope, or an expiry falling today) now fails the lookup with an error instead of being skipped, as `azure` compute already does. - **`aws` RI recommendations:** a fractional Cost Explorer quantity now rounds diff --git a/pkg/recfilter/dedupe.go b/pkg/recfilter/dedupe.go index 84ac919d..eb3985a4 100644 --- a/pkg/recfilter/dedupe.go +++ b/pkg/recfilter/dedupe.go @@ -3,6 +3,7 @@ package recfilter import ( "context" "fmt" + "maps" "strings" "time" @@ -252,6 +253,7 @@ func buildExistingCommitmentsMap(commitments []common.Commitment, logf Logf) map 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) + budgets := map[string]map[string]int{} for _rvc := range recs { rec := recs[_rvc] @@ -260,7 +262,7 @@ func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existing if ec2.handles(rec) { adjusted, err = ec2.adjust(rec) } else { - adjusted, err = adjustSingleRecommendation(rec, existingMap, logf) + adjusted, err = adjustSingleRecommendation(rec, variantBudget(budgets, existingMap, rec), logf) } if err != nil { return nil, nil, err @@ -275,6 +277,20 @@ func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existing return passed, filtered, nil } +// variantBudget returns the held budget for rec's term and payment variant. +// Variants are alternative ways to buy the same resource, so each group starts +// from the full held quantity; recommendations of one variant share it in +// input order. An empty term or payment is a group of its own (#703). +func variantBudget(budgets map[string]map[string]int, held map[string]int, rec common.Recommendation) map[string]int { + variant := strings.ToLower(strings.TrimSpace(rec.Term)) + "|" + strings.ToLower(strings.TrimSpace(rec.PaymentOption)) + if b, ok := budgets[variant]; ok { + return b + } + b := maps.Clone(held) + budgets[variant] = b + return b +} + // adjustSingleRecommendation adjusts a single recommendation based on existing // commitments. A reduced count scales the money fields with it. func adjustSingleRecommendation(rec common.Recommendation, existingMap map[string]int, logf Logf) (common.Recommendation, error) { diff --git a/pkg/recfilter/dedupe_variants_test.go b/pkg/recfilter/dedupe_variants_test.go new file mode 100644 index 00000000..dde8a419 --- /dev/null +++ b/pkg/recfilter/dedupe_variants_test.go @@ -0,0 +1,87 @@ +package recfilter + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func variantRec(term, payment string) common.Recommendation { + return common.Recommendation{ResourceType: "Premium_P1", Region: "eastus", Count: 2, Term: term, PaymentOption: payment} +} + +func adjustCounts(t *testing.T, held []common.Commitment, recs []common.Recommendation) []int { + t.Helper() + d := NewDuplicateChecker(0) + passed, _, err := d.AdjustRecommendationsForExisting(context.Background(), recs, &fakeServiceClient{commitments: held}) + require.NoError(t, err) + counts := make([]int, len(passed)) + for i, r := range passed { + counts[i] = r.Count + } + return counts +} + +func heldOne() []common.Commitment { + return []common.Commitment{{ResourceType: "Premium_P1", Region: "eastus", Count: 1, State: "active", StartDate: time.Now().Add(-time.Hour)}} +} + +// #703: term and payment variants are alternatives for the same resource, so +// each variant sees the full held quantity. +func TestDedupe_VariantsEachSeeFullHeldQuantity(t *testing.T) { + t.Parallel() + recs := []common.Recommendation{ + variantRec("1yr", "upfront"), variantRec("1yr", "monthly"), + variantRec("3yr", "upfront"), variantRec("3yr", "monthly"), + } + assert.Equal(t, []int{1, 1, 1, 1}, adjustCounts(t, heldOne(), recs)) +} + +func TestDedupe_SameVariantStillSharesBudgetInOrder(t *testing.T) { + t.Parallel() + recs := []common.Recommendation{ + variantRec("1yr", "upfront"), variantRec("1yr", "upfront"), + variantRec("3yr", "upfront"), + } + assert.Equal(t, []int{1, 2, 1}, adjustCounts(t, heldOne(), recs)) +} + +func TestDedupe_VariantsWithDifferentKeysUnaffected(t *testing.T) { + t.Parallel() + other := variantRec("1yr", "monthly") + other.ResourceType = "Premium_P2" + recs := []common.Recommendation{variantRec("1yr", "upfront"), other} + assert.Equal(t, []int{1, 2}, adjustCounts(t, heldOne(), recs)) +} + +// An empty term or payment is its own group: it is not folded into a named +// variant, because an unknown variant cannot be assumed to be the same offer. +func TestDedupe_EmptyTermOrPaymentIsOwnGroup(t *testing.T) { + t.Parallel() + recs := []common.Recommendation{ + variantRec("", ""), variantRec("", ""), + variantRec("1yr", ""), variantRec("", "upfront"), + } + assert.Equal(t, []int{1, 2, 1, 1}, adjustCounts(t, heldOne(), recs)) +} + +func TestDedupe_ElastiCacheCoverWithVariants(t *testing.T) { + t.Parallel() + held := []common.Commitment{{ + Provider: common.ProviderAWS, Service: common.ServiceElastiCache, ResourceType: "cache.r6g.large", + Region: "us-east-1", Engine: "redis", Count: 4, State: "active", StartDate: time.Now().Add(-time.Hour), + }} + mk := func(term string) common.Recommendation { + return common.Recommendation{ + Provider: common.ProviderAWS, Service: common.ServiceElastiCache, ResourceType: "cache.r6g.large", + Region: "us-east-1", Count: 6, Term: term, PaymentOption: "no-upfront", + Details: &common.CacheDetails{Engine: "valkey"}, + } + } + // 4 Redis nodes cover 5 Valkey nodes, once per variant: 6 -> 1, 1. + assert.Equal(t, []int{1, 1}, adjustCounts(t, held, []common.Recommendation{mk("1yr"), mk("3yr")})) +} From cc1dae1aec44f8cd81c8e4b7a529331e4abce8b5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 03:58:47 +0100 Subject: [PATCH 2/3] fix(recfilter): give EC2 each term and payment variant its own held budget ec2Dedupe spent shared entries across variants, so one held m5.large still reduced only the first EC2 variant. Build one ec2Dedupe per variant group, sharing the group key with the other services. Adds the EC2 probe and normalization tests; the changelog now covers EC2. Refs #703 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 +- pkg/recfilter/dedupe.go | 39 ++++++++++++++++++--------- pkg/recfilter/dedupe_ec2.go | 4 --- pkg/recfilter/dedupe_variants_test.go | 29 ++++++++++++++++++++ 4 files changed, 56 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e24d32da..5bdcf4c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -307,7 +307,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed -- **Duplicate check:** a recent purchase now reduces every term and payment variant of a recommendation, not only the first one in input order. Each `term|payment` group starts from the full held quantity and consumes it in input order, so one held unit against four variants of 2 yields 1, 1, 1, 1 (was 1, 2, 2, 2). Fixes #703. +- **Duplicate check:** a recent purchase now reduces every term and payment variant of a recommendation, not only the first one in input order, for EC2 as well as every other service. Each `term|payment` group starts from the full held quantity and consumes it in input order, so one held unit against four variants of 2 yields 1, 1, 1, 1 (was 1, 2, 2, 2). Providers pick this up on their next `pkg` pin bump. Fixes #703. - **Breaking (source):** `managedredis.ReservationsDetailsPager` and `(*managedredis.Client).SetReservationsPager` are removed; tests inject reservation inventory with `SetInventoryFactories` instead (#699). - **`azure` Redis:** `managedredis.GetExistingCommitments` now reads held reservations from the Reservations API (purchase date, quantity, location, state), so a Redis reservation bought in the last day reduces the matching recommendation instead of being invisible to the duplicate guard (#699). An incomplete live reservation row (missing SKU, location, quantity or purchase date, an unattributable scope, or an expiry falling today) now fails the lookup with an error instead of being skipped, as `azure` compute already does. - **`aws` RI recommendations:** a fractional Cost Explorer quantity now rounds diff --git a/pkg/recfilter/dedupe.go b/pkg/recfilter/dedupe.go index eb3985a4..12b58f20 100644 --- a/pkg/recfilter/dedupe.go +++ b/pkg/recfilter/dedupe.go @@ -78,7 +78,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, newEC2Dedupe(recentExisting, d.Logf), d.Logf) + passed, filtered, err = adjustRecommendationsAgainstExisting(recs, existingMap, recentExisting, d.Logf) if err != nil { return nil, nil, err } @@ -250,19 +250,20 @@ 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, ec2 *ec2Dedupe, logf Logf) (passed, filtered []common.Recommendation, err error) { +func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existingMap map[string]int, recent []common.Commitment, logf Logf) (passed, filtered []common.Recommendation, err error) { passed = make([]common.Recommendation, 0, len(recs)) filtered = make([]common.Recommendation, 0) - budgets := map[string]map[string]int{} + variants := &variantGroups{held: existingMap, recent: recent, logf: logf, groups: map[string]*variantGroup{}} for _rvc := range recs { rec := recs[_rvc] var adjusted common.Recommendation var err error - if ec2.handles(rec) { - adjusted, err = ec2.adjust(rec) + group := variants.of(rec) + if isAWSCompute(rec.Provider, rec.Service) { + adjusted, err = group.ec2.adjust(rec) } else { - adjusted, err = adjustSingleRecommendation(rec, variantBudget(budgets, existingMap, rec), logf) + adjusted, err = adjustSingleRecommendation(rec, group.held, logf) } if err != nil { return nil, nil, err @@ -277,18 +278,30 @@ func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existing return passed, filtered, nil } -// variantBudget returns the held budget for rec's term and payment variant. +// variantGroups holds the held budgets of each term and payment variant. // Variants are alternative ways to buy the same resource, so each group starts // from the full held quantity; recommendations of one variant share it in // input order. An empty term or payment is a group of its own (#703). -func variantBudget(budgets map[string]map[string]int, held map[string]int, rec common.Recommendation) map[string]int { +type variantGroups struct { + groups map[string]*variantGroup + held map[string]int + logf Logf + recent []common.Commitment +} + +type variantGroup struct { + held map[string]int + ec2 *ec2Dedupe +} + +func (v *variantGroups) of(rec common.Recommendation) *variantGroup { variant := strings.ToLower(strings.TrimSpace(rec.Term)) + "|" + strings.ToLower(strings.TrimSpace(rec.PaymentOption)) - if b, ok := budgets[variant]; ok { - return b + g, ok := v.groups[variant] + if !ok { + g = &variantGroup{held: maps.Clone(v.held), ec2: newEC2Dedupe(v.recent, v.logf)} + v.groups[variant] = g } - b := maps.Clone(held) - budgets[variant] = b - return b + return g } // adjustSingleRecommendation adjusts a single recommendation based on existing diff --git a/pkg/recfilter/dedupe_ec2.go b/pkg/recfilter/dedupe_ec2.go index 6577338c..a28b9bce 100644 --- a/pkg/recfilter/dedupe_ec2.go +++ b/pkg/recfilter/dedupe_ec2.go @@ -189,10 +189,6 @@ func ec2DimsFromDetails(details common.ServiceDetails) (platform, tenancy, scope 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 diff --git a/pkg/recfilter/dedupe_variants_test.go b/pkg/recfilter/dedupe_variants_test.go index dde8a419..e05a323e 100644 --- a/pkg/recfilter/dedupe_variants_test.go +++ b/pkg/recfilter/dedupe_variants_test.go @@ -85,3 +85,32 @@ func TestDedupe_ElastiCacheCoverWithVariants(t *testing.T) { // 4 Redis nodes cover 5 Valkey nodes, once per variant: 6 -> 1, 1. assert.Equal(t, []int{1, 1}, adjustCounts(t, held, []common.Recommendation{mk("1yr"), mk("3yr")})) } + +func TestDedupe_EC2VariantsEachSeeFullHeldQuantity(t *testing.T) { + t.Parallel() + held := []common.Commitment{{ + Provider: common.ProviderAWS, Service: common.ServiceEC2, ResourceType: "m5.large", Region: "us-east-1", + Platform: "Linux/UNIX", Tenancy: "default", Scope: "Region", Count: 1, State: "active", StartDate: time.Now().Add(-time.Hour), + }} + mk := func(term, payment string) common.Recommendation { + return common.Recommendation{ + Provider: common.ProviderAWS, Service: common.ServiceEC2, ResourceType: "m5.large", Region: "us-east-1", + Count: 2, Term: term, PaymentOption: payment, + Details: &common.ComputeDetails{Platform: "Linux/UNIX", Tenancy: "default", Scope: "region"}, + } + } + got := adjustCounts(t, held, []common.Recommendation{ + mk("1yr", "all-upfront"), mk("1yr", "no-upfront"), mk("3yr", "all-upfront"), mk("1yr", "all-upfront"), + }) + assert.Equal(t, []int{1, 1, 1, 2}, got) +} + +func TestDedupe_VariantNormalization(t *testing.T) { + t.Parallel() + recs := []common.Recommendation{variantRec("1yr", "upfront"), variantRec(" 1YR ", " UPFRONT ")} + assert.Equal(t, []int{1, 2}, adjustCounts(t, heldOne(), recs), "same variant after trim and case folding shares the budget") + recs = []common.Recommendation{variantRec("1yr", "upfront"), variantRec(" 1YR ", "monthly")} + assert.Equal(t, []int{1, 1}, adjustCounts(t, heldOne(), recs), "term folds case, payment differs") + recs = []common.Recommendation{variantRec("1yr", "monthly"), variantRec("1yr", " MONTHLY ")} + assert.Equal(t, []int{1, 2}, adjustCounts(t, heldOne(), recs), "payment folds case") +} From 011267d4da495867b8d182686532aaf29479f9ab Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 03:59:11 +0100 Subject: [PATCH 3/3] test(azure): drop the managedredis variant pin that asserted the #703 bug TestHeldReservation_VariantPin pinned the shared-budget over-buy. CI runs providers/azure both with the local go.work (new pkg) and with GOWORK=off (old pin), so no single assertion passes both. It returns asserting 1,1,1,1 together with the azure pkg re-pin in a follow-up. Refs #703 Co-Authored-By: Claude Opus 5.5 --- .../held_reservation_dedupe_test.go | 27 ------------------- 1 file changed, 27 deletions(-) diff --git a/providers/azure/services/managedredis/held_reservation_dedupe_test.go b/providers/azure/services/managedredis/held_reservation_dedupe_test.go index 4a6a37f3..bd7b48e4 100644 --- a/providers/azure/services/managedredis/held_reservation_dedupe_test.go +++ b/providers/azure/services/managedredis/held_reservation_dedupe_test.go @@ -201,33 +201,6 @@ func TestHeldReservation_Table(t *testing.T) { } } -// TestHeldReservation_VariantPin pins today's shared-budget behavior of the -// recent-purchase guard on GetRecommendations-shaped output. The held -// quantity is consumed by the first matching variant in input order, so the P1Y -// upfront variant takes it and every other variant is untouched; the P1Y rec -// comes before the P3Y rec in the input and the pin depends on that order. -// The key has no term or payment option, so this is a known over-buy that this -// test pins and does not endorse; tracked in go#703. -func TestHeldReservation_VariantPin(t *testing.T) { - t.Parallel() - c := NewClient(nil, "sub", "eastus") - recs := variantsOf(t, c, legacyRedisRec("P1Y"), legacyRedisRec("P3Y")) - require.Len(t, recs, 4) - c.SetInventoryFactories(factoriesRows(nil, redisRow("held", "Premium_P1", "eastus", 1))) - - passed, err := adjust(t, c, recs) - require.NoError(t, err) - type variant struct{ term, payment string } - got := map[variant]int{} - for _, r := range passed { - got[variant{r.Term, r.PaymentOption}] = r.Count - } - assert.Equal(t, map[variant]int{ - {"1yr", "upfront"}: 1, {"1yr", "monthly"}: 2, - {"3yr", "upfront"}: 2, {"3yr", "monthly"}: 2, - }, got) -} - func TestNewClient_DefaultInventoryFactories(t *testing.T) { t.Parallel() for name, c := range map[string]*Client{