diff --git a/CHANGELOG.md b/CHANGELOG.md index 974b897c..38b84ff0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -312,6 +312,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, 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. - **`common`:** a `Recommendation`, and a `PurchaseResult` carrying one (with no `Error`), now decodes from JSON when `details` holds an object. `details` is decoded into the typed pointer for the recommendation's service through diff --git a/pkg/recfilter/dedupe.go b/pkg/recfilter/dedupe.go index 84ac919d..12b58f20 100644 --- a/pkg/recfilter/dedupe.go +++ b/pkg/recfilter/dedupe.go @@ -3,6 +3,7 @@ package recfilter import ( "context" "fmt" + "maps" "strings" "time" @@ -77,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 } @@ -249,18 +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) + 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, existingMap, logf) + adjusted, err = adjustSingleRecommendation(rec, group.held, logf) } if err != nil { return nil, nil, err @@ -275,6 +278,32 @@ func adjustRecommendationsAgainstExisting(recs []common.Recommendation, existing return passed, filtered, nil } +// 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). +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)) + g, ok := v.groups[variant] + if !ok { + g = &variantGroup{held: maps.Clone(v.held), ec2: newEC2Dedupe(v.recent, v.logf)} + v.groups[variant] = g + } + return g +} + // 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_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 new file mode 100644 index 00000000..e05a323e --- /dev/null +++ b/pkg/recfilter/dedupe_variants_test.go @@ -0,0 +1,116 @@ +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")})) +} + +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") +} 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{