diff --git a/internal/config/suppressions_build.go b/internal/config/suppressions_build.go index c2af92f0..2bf6e7c9 100644 --- a/internal/config/suppressions_build.go +++ b/internal/config/suppressions_build.go @@ -3,6 +3,7 @@ package config import ( "time" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" ) @@ -21,7 +22,8 @@ type suppressionBucket struct { // behavior, which over-suppresses (under-buys) rather than leaving the // purchased capacity on show. The purchase paths reject such recs before // they get here (RecIdentityKey reads the same Details), so this is a -// fallback, not a path in normal use. +// fallback, not a path in normal use. An AWS EC2 rec (service ec2 or compute) with no +// platform or tenancy (legacy, no Details) is treated the same way (issue #1232). func suppressionBucketOf(rec *RecommendationRecord) suppressionBucket { accountID := "" if rec.CloudAccountID != nil { @@ -41,6 +43,15 @@ func suppressionBucketOf(rec *RecommendationRecord) suppressionBucket { b.unknownVariant = true return b } + isAWSEC2 := rec.Provider == string(common.ProviderAWS) && + (rec.Service == string(common.ServiceEC2) || rec.Service == string(common.ServiceCompute)) + if isAWSEC2 && v.Platform == "" && v.Tenancy == "" { + // Saved before Details existed (#453) and retried without re-matching: + // the bought platform/tenancy is unknown, so a known {az,"",""} would + // leave the real variant unsuppressed (double-buy). Wildcard instead. + b.unknownVariant = true + return b + } b.variant = v return b } diff --git a/internal/config/suppressions_build_test.go b/internal/config/suppressions_build_test.go index 1f42b568..e8c85f22 100644 --- a/internal/config/suppressions_build_test.go +++ b/internal/config/suppressions_build_test.go @@ -152,3 +152,54 @@ func TestPurchaseSuppression_AppliesTo(t *testing.T) { assert.True(t, legacy.AppliesTo(&SuppressionVariant{"us-east-1b", "Windows", "dedicated"}), "a legacy row is a wildcard") assert.True(t, legacy.AppliesTo(nil)) } + +// TestBuildSuppressions_LegacyEC2WithoutDetailsIsWildcard: an EC2 rec saved +// before Details existed has an unknown platform and tenancy, so it must +// suppress every variant (issue #1232). Non-EC2 and fully detailed recs keep +// their known variant. +func TestBuildSuppressions_LegacyEC2WithoutDetailsIsWildcard(t *testing.T) { + base := RecommendationRecord{Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", Count: 1, AvailabilityZone: "us-east-1a"} + for name, details := range map[string][]byte{"nil": nil, "empty": {}, "null": []byte(`null`), "no keys": []byte(`{}`)} { + t.Run(name, func(t *testing.T) { + rec := base + rec.Details = details + sups := BuildSuppressions([]RecommendationRecord{rec}, "exec-1", &GlobalConfig{}, time.Now()) + require.Len(t, sups, 1) + assert.Nil(t, sups[0].Variant) + assert.True(t, sups[0].AppliesTo(&SuppressionVariant{"us-east-1a", "Linux/UNIX", "default"})) + }) + } + + full := base + full.Details = []byte(`{"platform":"Linux/UNIX","tenancy":"default"}`) + sups := BuildSuppressions([]RecommendationRecord{full}, "exec-1", &GlobalConfig{}, time.Now()) + require.Len(t, sups, 1) + assert.Equal(t, &SuppressionVariant{"us-east-1a", "Linux/UNIX", "default"}, sups[0].Variant) +} + +// TestBuildSuppressions_ComputeSlugEC2: AWS EC2 recs under the canonical +// "compute" slug follow the same wildcard rule as "ec2"; a rec with only a +// platform or only a tenancy keeps its narrow variant. +func TestBuildSuppressions_ComputeSlugEC2(t *testing.T) { + base := RecommendationRecord{Provider: "aws", Service: "compute", Region: "us-east-1", ResourceType: "m5.large", Count: 1, AvailabilityZone: "us-east-1a"} + sups := BuildSuppressions([]RecommendationRecord{base}, "exec-1", &GlobalConfig{}, time.Now()) + require.Len(t, sups, 1) + assert.Nil(t, sups[0].Variant) + assert.True(t, sups[0].AppliesTo(&SuppressionVariant{"us-east-1a", "Linux/UNIX", "default"})) + + for name, tc := range map[string]struct { + details []byte + want SuppressionVariant + }{ + "platform only": {[]byte(`{"platform":"Linux/UNIX"}`), SuppressionVariant{"us-east-1a", "Linux/UNIX", ""}}, + "tenancy only": {[]byte(`{"tenancy":"default"}`), SuppressionVariant{"us-east-1a", "", "default"}}, + } { + t.Run(name, func(t *testing.T) { + rec := base + rec.Details = tc.details + sups := BuildSuppressions([]RecommendationRecord{rec}, "exec-1", &GlobalConfig{}, time.Now()) + require.Len(t, sups, 1) + assert.Equal(t, &tc.want, sups[0].Variant) + }) + } +} diff --git a/internal/scheduler/suppression_variant_integration_test.go b/internal/scheduler/suppression_variant_integration_test.go index 37728557..8de1f79f 100644 --- a/internal/scheduler/suppression_variant_integration_test.go +++ b/internal/scheduler/suppression_variant_integration_test.go @@ -71,3 +71,34 @@ func TestSuppressionsScopedToPurchasedVariant_RealPostgres(t *testing.T) { "legacy-a": 4, "legacy-b": 4, }, counts) } + +// TestLegacyEC2RetrySuppressesBoughtVariant_RealPostgres is the issue #1232 +// regression: a retried execution persisted before Details existed carries an +// EC2 rec with no platform or tenancy. Its suppression must wildcard the +// 6-tuple, so the later Linux/UNIX recommendation that was actually bought is +// filtered instead of re-offered. +func TestLegacyEC2RetrySuppressesBoughtVariant_RealPostgres(t *testing.T) { + ctx := t.Context() + pg := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") + store := config.NewPostgresStore(pg.DB) + + later := config.RecommendationRecord{ + ID: "later", Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", + Count: 3, Term: 1, Payment: "all-upfront", AvailabilityZone: "us-east-1a", + Details: []byte(`{"availability_zone":"us-east-1a","platform":"Linux/UNIX","tenancy":"default"}`), + } + require.NoError(t, store.UpsertRecommendations(ctx, time.Now(), []config.RecommendationRecord{later}, nil)) + + legacy := later + legacy.ID = "legacy" + legacy.Count = 3 + legacy.Details = nil + sups := config.BuildSuppressions([]config.RecommendationRecord{legacy}, uuid.NewString(), &config.GlobalConfig{}, time.Now()) + require.Len(t, sups, 1) + require.NoError(t, store.WithTx(ctx, func(tx pgx.Tx) error { return store.CreateSuppressionTx(ctx, tx, &sups[0]) })) + + s := &Scheduler{config: store, isLambda: true} + got, err := s.ListRecommendations(ctx, config.RecommendationFilter{}) + require.NoError(t, err) + assert.Empty(t, got, "the bought variant must be fully suppressed") +}