diff --git a/internal/config/recommendation_identity.go b/internal/config/recommendation_identity.go index c278f85f4..cc42e0372 100644 --- a/internal/config/recommendation_identity.go +++ b/internal/config/recommendation_identity.go @@ -64,3 +64,16 @@ func RecIdentityKey(rec *RecommendationRecord) (string, error) { rec.AvailabilityZone, ci.Platform, ci.Tenancy, }, "\x1f"), nil } + +// SuppressionVariant is the rec's availability zone, platform and tenancy as +// the purchase suppression key carries them (issue #816): the same AZ field and +// the same Details read (ComputeIdentity) as RecIdentityKey, so the write side +// (BuildSuppressions) and the read side (the scheduler's subtraction pass) +// cannot drift from the identity key or from each other. +func (r *RecommendationRecord) SuppressionVariant() (SuppressionVariant, error) { + ci, err := r.ComputeIdentity() + if err != nil { + return SuppressionVariant{}, err + } + return SuppressionVariant{AvailabilityZone: r.AvailabilityZone, Platform: ci.Platform, Tenancy: ci.Tenancy}, nil +} diff --git a/internal/config/store_postgres_suppressions.go b/internal/config/store_postgres_suppressions.go index 6ff09753b..7356c978f 100644 --- a/internal/config/store_postgres_suppressions.go +++ b/internal/config/store_postgres_suppressions.go @@ -1,7 +1,7 @@ package config // store_postgres_suppressions.go — CRUD + transaction-accepting variants -// for the purchase_suppressions table (migration 000037) plus the +// for the purchase_suppressions table (migrations 000037, 000134) plus the // WithTx helper used by executePurchase and cancelExecution to commit // the execution row + suppression rows atomically. @@ -103,16 +103,24 @@ func (s *PostgresStore) CreateSuppressionTx(ctx context.Context, tx pgx.Tx, sup sup.ID = uuid.New().String() } + // A nil Variant writes NULL into all three variant columns (a legacy, + // 6-tuple-wide row); otherwise all three are written, empty strings included + // (migration 000134 keeps them all-NULL or all-set). + var az, platform, tenancy *string + if sup.Variant != nil { + az, platform, tenancy = &sup.Variant.AvailabilityZone, &sup.Variant.Platform, &sup.Variant.Tenancy + } const q = ` INSERT INTO purchase_suppressions ( id, execution_id, account_id, provider, service, region, - resource_type, engine, suppressed_count, expires_at - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) + resource_type, engine, suppressed_count, expires_at, + availability_zone, platform, tenancy + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13) ` if _, err := tx.Exec(ctx, q, sup.ID, sup.ExecutionID, sup.AccountID, sup.Provider, sup.Service, sup.Region, sup.ResourceType, sup.Engine, sup.SuppressedCount, - sup.ExpiresAt, + sup.ExpiresAt, az, platform, tenancy, ); err != nil { return fmt.Errorf("failed to insert purchase suppression: %w", err) } @@ -144,12 +152,15 @@ func (s *PostgresStore) DeleteSuppressionsByExecutionTx(ctx context.Context, tx // ListActiveSuppressions returns every suppression row whose expiry // hasn't passed. The scheduler groups these by the 6-tuple key -// (account, provider, service, region, resource_type, engine) to -// subtract cumulative suppressed counts from its rec-list output. +// (account, provider, service, region, resource_type, engine) and +// matches each row's variant (availability zone, platform, tenancy; +// issue #816) to subtract cumulative suppressed counts from its rec-list +// output. A row with NULL variant columns comes back with a nil Variant. func (s *PostgresStore) ListActiveSuppressions(ctx context.Context) ([]PurchaseSuppression, error) { const q = ` SELECT id, execution_id, account_id, provider, service, region, - resource_type, engine, suppressed_count, expires_at, created_at + resource_type, engine, suppressed_count, expires_at, created_at, + availability_zone, platform, tenancy FROM purchase_suppressions WHERE expires_at > NOW() ` @@ -162,13 +173,16 @@ func (s *PostgresStore) ListActiveSuppressions(ctx context.Context) ([]PurchaseS var out []PurchaseSuppression for rows.Next() { var sup PurchaseSuppression + var az, platform, tenancy *string if err := rows.Scan( &sup.ID, &sup.ExecutionID, &sup.AccountID, &sup.Provider, &sup.Service, &sup.Region, &sup.ResourceType, &sup.Engine, &sup.SuppressedCount, &sup.ExpiresAt, &sup.CreatedAt, + &az, &platform, &tenancy, ); err != nil { return nil, fmt.Errorf("failed to scan suppression row: %w", err) } + sup.Variant = suppressionVariantFromColumns(az, platform, tenancy) out = append(out, sup) } if err := rows.Err(); err != nil { @@ -176,3 +190,14 @@ func (s *PostgresStore) ListActiveSuppressions(ctx context.Context) ([]PurchaseS } return out, nil } + +// suppressionVariantFromColumns rebuilds a row's variant from its nullable +// columns. The 000134 CHECK keeps them all-NULL or all-set; a row with any +// NULL is read as having no variant (a 6-tuple-wide wildcard), which +// over-suppresses rather than over-shows. +func suppressionVariantFromColumns(az, platform, tenancy *string) *SuppressionVariant { + if az == nil || platform == nil || tenancy == nil { + return nil + } + return &SuppressionVariant{AvailabilityZone: *az, Platform: *platform, Tenancy: *tenancy} +} diff --git a/internal/config/store_postgres_suppressions_test.go b/internal/config/store_postgres_suppressions_test.go index 477e69c43..012a316d3 100644 --- a/internal/config/store_postgres_suppressions_test.go +++ b/internal/config/store_postgres_suppressions_test.go @@ -19,7 +19,7 @@ func TestPGXMock_CreateSuppression_RoundTrip(t *testing.T) { mock.ExpectBegin() mock.ExpectExec(`INSERT INTO purchase_suppressions`). WithArgs(pgxmock.AnyArg(), "exec-1", "acct-1", "aws", "ec2", "us-east-1", - "t4g.nano", "", 3, pgxmock.AnyArg()). + "t4g.nano", "", 3, pgxmock.AnyArg(), (*string)(nil), (*string)(nil), (*string)(nil)). WillReturnResult(pgxmock.NewResult("INSERT", 1)) mock.ExpectCommit() @@ -33,6 +33,32 @@ func TestPGXMock_CreateSuppression_RoundTrip(t *testing.T) { assert.NoError(t, mock.ExpectationsWereMet()) } +// TestPGXMock_CreateSuppression_WritesVariant pins issue #816: a row with a +// variant writes all three variant columns, empty strings included (a +// regional, known-empty AZ is not NULL). +func TestPGXMock_CreateSuppression_WritesVariant(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + az, platform, tenancy := "", "Linux/UNIX", "default" + mock.ExpectBegin() + mock.ExpectExec(`INSERT INTO purchase_suppressions`). + WithArgs(pgxmock.AnyArg(), "exec-1", "acct-1", "aws", "ec2", "us-east-1", + "t4g.nano", "", 3, pgxmock.AnyArg(), &az, &platform, &tenancy). + WillReturnResult(pgxmock.NewResult("INSERT", 1)) + mock.ExpectCommit() + + err := store.CreateSuppression(ctx, &PurchaseSuppression{ + ExecutionID: "exec-1", AccountID: "acct-1", Provider: "aws", + Service: "ec2", Region: "us-east-1", ResourceType: "t4g.nano", + SuppressedCount: 3, ExpiresAt: time.Now().Add(time.Hour), + Variant: &SuppressionVariant{Platform: "Linux/UNIX", Tenancy: "default"}, + }) + require.NoError(t, err) + assert.NoError(t, mock.ExpectationsWereMet()) +} + func TestPGXMock_CreateSuppression_Validation(t *testing.T) { mock := newMock(t) store := storeWith(mock) @@ -94,12 +120,14 @@ func TestPGXMock_ListActiveSuppressions(t *testing.T) { cols := []string{ "id", "execution_id", "account_id", "provider", "service", "region", "resource_type", "engine", "suppressed_count", "expires_at", "created_at", + "availability_zone", "platform", "tenancy", } + az, platform, tenancy := "us-east-1a", "Linux/UNIX", "default" rows := pgxmock.NewRows(cols). AddRow("sup-1", "exec-1", "acct-1", "aws", "ec2", "us-east-1", - "t4g.nano", "", 3, future, now). + "t4g.nano", "", 3, future, now, nil, nil, nil). AddRow("sup-2", "exec-2", "acct-1", "aws", "ec2", "us-east-1", - "t4g.nano", "", 2, future, now) + "t4g.nano", "", 2, future, now, &az, &platform, &tenancy) mock.ExpectQuery(`SELECT .* FROM purchase_suppressions WHERE expires_at > NOW\(\)`). WillReturnRows(rows) @@ -108,8 +136,10 @@ func TestPGXMock_ListActiveSuppressions(t *testing.T) { assert.Len(t, sups, 2) assert.Equal(t, "sup-1", sups[0].ID) assert.Equal(t, 3, sups[0].SuppressedCount) + assert.Nil(t, sups[0].Variant, "NULL variant columns read back as a legacy, 6-tuple-wide row") assert.Equal(t, "sup-2", sups[1].ID) assert.Equal(t, 2, sups[1].SuppressedCount) + assert.Equal(t, &SuppressionVariant{AvailabilityZone: "us-east-1a", Platform: "Linux/UNIX", Tenancy: "default"}, sups[1].Variant) assert.NoError(t, mock.ExpectationsWereMet()) } diff --git a/internal/config/suppressions_build.go b/internal/config/suppressions_build.go index 2b91de733..c2af92f0e 100644 --- a/internal/config/suppressions_build.go +++ b/internal/config/suppressions_build.go @@ -1,47 +1,78 @@ package config -import "time" +import ( + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" +) + +// suppressionBucket is the key BuildSuppressions aggregates on: the 6-tuple +// (account, provider, service, region, resource_type, engine) plus the rec's +// variant (availability zone, platform, tenancy; issue #816). unknownVariant +// marks a rec whose Details could not be read. +type suppressionBucket struct { + accountID, provider, service, region, resourceType, engine string + variant SuppressionVariant + unknownVariant bool +} + +// suppressionBucketOf returns rec's bucket. A rec whose Details cannot be read +// gets no variant: its row is then a wildcard over the 6-tuple, the pre-#816 +// 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. +func suppressionBucketOf(rec *RecommendationRecord) suppressionBucket { + accountID := "" + if rec.CloudAccountID != nil { + accountID = *rec.CloudAccountID + } + b := suppressionBucket{ + accountID: accountID, + provider: rec.Provider, + service: rec.Service, + region: rec.Region, + resourceType: rec.ResourceType, + engine: rec.Engine, + } + v, err := rec.SuppressionVariant() + if err != nil { + logging.Warnf("suppression for %s/%s/%s: %v; suppressing the whole 6-tuple", rec.Provider, rec.Region, rec.ResourceType, err) + b.unknownVariant = true + return b + } + b.variant = v + return b +} // BuildSuppressions converts the recs on an executePurchase request into // purchase_suppressions rows, one per unique 6-tuple (account, provider, -// service, region, resource_type, engine). Counts for duplicate tuples -// within the same request are summed (defensive — the UI should collapse -// them client-side, but the backend doesn't trust that). Providers with -// a grace period of 0 (feature disabled) contribute no rows. +// service, region, resource_type, engine) and variant (availability zone, +// platform, tenancy; issue #816), so a purchase of one zonal EC2 variant does +// not hide its sibling AZ, regional, platform or tenancy recs. Counts for +// duplicate keys within the same request are summed (defensive — the UI +// should collapse them client-side, but the backend doesn't trust that). +// Providers with a grace period of 0 (feature disabled) contribute no rows. func BuildSuppressions(recs []RecommendationRecord, executionID string, cfg *GlobalConfig, now time.Time) []PurchaseSuppression { - // Aggregate count per 6-tuple before writing suppression rows so - // the UNIQUE(execution_id, ...6-tuple) constraint can't fire from - // a request that repeated the same tuple. - type key struct { - accountID, provider, service, region, resourceType, engine string - } - agg := map[key]int{} - order := []key{} - for _rvc := range recs { - rec := recs[_rvc] - if rec.Count <= 0 { + // Aggregate count per key before writing suppression rows so the + // UNIQUE(execution_id, ...key) constraint can't fire from a request + // that repeated the same key. + agg := map[suppressionBucket]int{} + order := []suppressionBucket{} + for i := range recs { + if recs[i].Count <= 0 { continue } - accountID := "" - if rec.CloudAccountID != nil { - accountID = *rec.CloudAccountID - } - k := key{ - accountID: accountID, - provider: rec.Provider, - service: rec.Service, - region: rec.Region, - resourceType: rec.ResourceType, - engine: rec.Engine, - } + k := suppressionBucketOf(&recs[i]) if _, seen := agg[k]; !seen { order = append(order, k) } - agg[k] += rec.Count + agg[k] += recs[i].Count } out := make([]PurchaseSuppression, 0, len(order)) - for _, k := range order { + for i := range order { + k := &order[i] graceDays := DefaultGracePeriodDays if cfg != nil { graceDays = cfg.GracePeriodFor(k.provider) @@ -49,7 +80,7 @@ func BuildSuppressions(recs []RecommendationRecord, executionID string, cfg *Glo if graceDays <= 0 { continue // feature disabled for this provider } - out = append(out, PurchaseSuppression{ + sup := PurchaseSuppression{ ExecutionID: executionID, AccountID: k.accountID, Provider: k.provider, @@ -57,9 +88,14 @@ func BuildSuppressions(recs []RecommendationRecord, executionID string, cfg *Glo Region: k.region, ResourceType: k.resourceType, Engine: k.engine, - SuppressedCount: agg[k], + SuppressedCount: agg[*k], ExpiresAt: now.Add(time.Duration(graceDays) * 24 * time.Hour), - }) + } + if !k.unknownVariant { + v := k.variant + sup.Variant = &v + } + out = append(out, sup) } return out } diff --git a/internal/config/suppressions_build_test.go b/internal/config/suppressions_build_test.go index 9ff0a1670..1f42b5681 100644 --- a/internal/config/suppressions_build_test.go +++ b/internal/config/suppressions_build_test.go @@ -68,3 +68,87 @@ func TestBuildSuppressions_SkipsZeroCountRecs(t *testing.T) { assert.Empty(t, sups, "zero-count recs don't produce suppression rows") } + +// TestBuildSuppressions_SeparatesVariants pins issue #816: recs on one 6-tuple +// that differ only by availability zone, platform or tenancy get one row each, +// carrying the rec's variant; recs with the same variant are still summed. +func TestBuildSuppressions_SeparatesVariants(t *testing.T) { + ec2 := func(az, platform, tenancy string, count int) RecommendationRecord { + return RecommendationRecord{ + Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", + Count: count, CloudAccountID: strPtr("acct-1"), AvailabilityZone: az, + Details: []byte(`{"platform":"` + platform + `","tenancy":"` + tenancy + `"}`), + } + } + sups := BuildSuppressions([]RecommendationRecord{ + ec2("us-east-1a", "Linux/UNIX", "default", 1), + ec2("us-east-1a", "Linux/UNIX", "default", 2), + ec2("us-east-1b", "Linux/UNIX", "default", 3), + ec2("", "Linux/UNIX", "default", 4), + ec2("us-east-1a", "Windows", "default", 5), + ec2("us-east-1a", "Linux/UNIX", "dedicated", 6), + }, "exec-1", &GlobalConfig{}, time.Now()) + + require.Len(t, sups, 5) + want := []struct { + variant SuppressionVariant + count int + }{ + {SuppressionVariant{"us-east-1a", "Linux/UNIX", "default"}, 3}, + {SuppressionVariant{"us-east-1b", "Linux/UNIX", "default"}, 3}, + {SuppressionVariant{"", "Linux/UNIX", "default"}, 4}, + {SuppressionVariant{"us-east-1a", "Windows", "default"}, 5}, + {SuppressionVariant{"us-east-1a", "Linux/UNIX", "dedicated"}, 6}, + } + for i, w := range want { + require.NotNil(t, sups[i].Variant, "row %d", i) + assert.Equal(t, w.variant, *sups[i].Variant, "row %d", i) + assert.Equal(t, w.count, sups[i].SuppressedCount, "row %d", i) + } +} + +// TestBuildSuppressions_NonComputeVariantIsKnownEmpty: a rec without platform +// or tenancy (non-EC2, no Details) gets an empty but known variant, not a +// legacy wildcard. +func TestBuildSuppressions_NonComputeVariantIsKnownEmpty(t *testing.T) { + sups := BuildSuppressions([]RecommendationRecord{ + {Provider: "aws", Service: "rds", Region: "us-east-1", ResourceType: "db.t4g.micro", Engine: "postgres", Count: 1}, + }, "exec-1", &GlobalConfig{}, time.Now()) + require.Len(t, sups, 1) + assert.Equal(t, &SuppressionVariant{}, sups[0].Variant) +} + +// TestBuildSuppressions_UnreadableDetailsFallBackToWildcard: a rec whose +// Details cannot be read gets no variant, so its row suppresses the whole +// 6-tuple (pre-#816 behavior) instead of nothing. +func TestBuildSuppressions_UnreadableDetailsFallBackToWildcard(t *testing.T) { + sups := BuildSuppressions([]RecommendationRecord{ + {Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", Count: 2, + AvailabilityZone: "us-east-1a", Details: []byte(`{"platform":42}`)}, + {Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", Count: 1, + AvailabilityZone: "us-east-1a", Details: []byte(`{"platform":"Linux/UNIX","tenancy":"default"}`)}, + }, "exec-1", &GlobalConfig{}, time.Now()) + require.Len(t, sups, 2) + assert.Nil(t, sups[0].Variant) + assert.Equal(t, 2, sups[0].SuppressedCount) + require.NotNil(t, sups[1].Variant) + assert.Equal(t, 1, sups[1].SuppressedCount) +} + +// TestPurchaseSuppression_AppliesTo pins the match rule shared by the +// scheduler: exact variant match, legacy rows and unknown rec variants are +// wildcards. +func TestPurchaseSuppression_AppliesTo(t *testing.T) { + v := SuppressionVariant{AvailabilityZone: "us-east-1a", Platform: "Linux/UNIX", Tenancy: "default"} + scoped := PurchaseSuppression{Variant: &v} + legacy := PurchaseSuppression{} + + assert.True(t, scoped.AppliesTo(&SuppressionVariant{"us-east-1a", "Linux/UNIX", "default"})) + assert.False(t, scoped.AppliesTo(&SuppressionVariant{"us-east-1b", "Linux/UNIX", "default"})) + assert.False(t, scoped.AppliesTo(&SuppressionVariant{"", "Linux/UNIX", "default"})) + assert.False(t, scoped.AppliesTo(&SuppressionVariant{"us-east-1a", "Windows", "default"})) + assert.False(t, scoped.AppliesTo(&SuppressionVariant{"us-east-1a", "Linux/UNIX", "dedicated"})) + assert.True(t, scoped.AppliesTo(nil), "an unreadable rec variant matches every row") + assert.True(t, legacy.AppliesTo(&SuppressionVariant{"us-east-1b", "Windows", "dedicated"}), "a legacy row is a wildcard") + assert.True(t, legacy.AppliesTo(nil)) +} diff --git a/internal/config/types.go b/internal/config/types.go index 18d5c1916..4b43d6a0f 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -679,8 +679,9 @@ type RecommendationRecord struct { CloudAccountID *string `json:"cloud_account_id,omitempty" dynamodbav:"cloud_account_id,omitempty"` SourceAccountID *string `json:"source_account_id,omitempty" dynamodbav:"source_account_id,omitempty"` // SuppressedCount is the cumulative count already committed against - // this recommendation's 6-tuple (account, provider, service, region, - // resource_type, engine) within the active grace window. The + // this recommendation's suppression key (account, provider, service, + // region, resource_type, engine, plus availability zone, platform and + // tenancy, issue #816) within the active grace window. The // scheduler subtracts this from Count before returning the rec to // the frontend; a rec where SuppressedCount ≥ original count is // dropped entirely. Populated by the scheduler — zero on writes. @@ -739,6 +740,30 @@ type PurchaseSuppression struct { SuppressedCount int `json:"suppressed_count"` ExpiresAt time.Time `json:"expires_at"` CreatedAt time.Time `json:"created_at"` + // Variant narrows the row to one availability zone, platform and tenancy + // on its 6-tuple (issue #816, migration 000134). nil means the variant is + // unknown: rows written before 000134 or by code predating #816. Such a + // row is a wildcard over its 6-tuple (see AppliesTo). + Variant *SuppressionVariant `json:"variant,omitempty"` +} + +// SuppressionVariant is the part of the suppression key beyond the 6-tuple: +// the zonal EC2 availability zone and the EC2 platform and tenancy, the same +// three fields the rec identity key carries (issues #792, #806). Empty strings +// are known values (a regional rec, a non-EC2 rec), not wildcards. +type SuppressionVariant struct { + AvailabilityZone string `json:"availability_zone"` + Platform string `json:"platform"` + Tenancy string `json:"tenancy"` +} + +// AppliesTo reports whether the suppression counts against a recommendation +// on the same 6-tuple whose variant is v. A row without a variant (legacy) +// applies to every variant, and a nil v (the rec's Details could not be read) +// matches every row: both fall back to the pre-#816 6-tuple match, which +// over-suppresses (under-buys) rather than over-shows. +func (s *PurchaseSuppression) AppliesTo(v *SuppressionVariant) bool { + return s.Variant == nil || v == nil || *s.Variant == *v } // RecommendationFilter parameterises ListStoredRecommendations and the diff --git a/internal/database/postgres/migrations/000134_purchase_suppressions_variant.down.sql b/internal/database/postgres/migrations/000134_purchase_suppressions_variant.down.sql new file mode 100644 index 000000000..8111cfb06 --- /dev/null +++ b/internal/database/postgres/migrations/000134_purchase_suppressions_variant.down.sql @@ -0,0 +1,50 @@ +-- Rollback of 000134: restore the 6-tuple suppression key. +-- +-- Rows of one execution that differ only by availability zone, platform or +-- tenancy (allowed while the widened key exists) are merged into one row per +-- execution and 6-tuple before the old UNIQUE constraint is recreated. The +-- merged row keeps the summed count and the latest expiry (rows of one +-- execution and provider share one expiry in practice), so the rollback never +-- suppresses less than before; it over-suppresses siblings, which is the +-- pre-#816 behavior that the old code expects. +WITH merged AS ( + SELECT execution_id, account_id, provider, service, region, resource_type, engine, + SUM(suppressed_count)::INT AS total, + MAX(expires_at) AS expires, + MIN(created_at) AS created, + (ARRAY_AGG(id ORDER BY created_at, id))[1] AS keep_id + FROM purchase_suppressions + GROUP BY execution_id, account_id, provider, service, region, resource_type, engine + HAVING COUNT(*) > 1 +), +dropped AS ( + DELETE FROM purchase_suppressions s + USING merged m + WHERE s.execution_id = m.execution_id + AND s.account_id = m.account_id + AND s.provider = m.provider + AND s.service = m.service + AND s.region = m.region + AND s.resource_type = m.resource_type + AND s.engine = m.engine + AND s.id <> m.keep_id +) +UPDATE purchase_suppressions s +SET suppressed_count = m.total, expires_at = m.expires, created_at = m.created +FROM merged m +WHERE s.id = m.keep_id; + +ALTER TABLE purchase_suppressions + DROP CONSTRAINT IF EXISTS purchase_suppressions_execution_variant_key; + +ALTER TABLE purchase_suppressions + DROP CONSTRAINT IF EXISTS purchase_suppressions_variant_all_or_none; + +ALTER TABLE purchase_suppressions + DROP COLUMN IF EXISTS availability_zone, + DROP COLUMN IF EXISTS platform, + DROP COLUMN IF EXISTS tenancy; + +ALTER TABLE purchase_suppressions + ADD CONSTRAINT purchase_suppressions_execution_id_account_id_provider_serv_key + UNIQUE (execution_id, account_id, provider, service, region, resource_type, engine); diff --git a/internal/database/postgres/migrations/000134_purchase_suppressions_variant.up.sql b/internal/database/postgres/migrations/000134_purchase_suppressions_variant.up.sql new file mode 100644 index 000000000..53965e9aa --- /dev/null +++ b/internal/database/postgres/migrations/000134_purchase_suppressions_variant.up.sql @@ -0,0 +1,40 @@ +-- Migration 000134 (issue #816): widen the purchase suppression key with the +-- availability zone, platform and tenancy of the purchased recommendation. +-- +-- The 6-tuple key (account, provider, service, region, resource type, engine) +-- made a purchase of one zonal EC2 variant subtract its count from every +-- sibling recommendation on that tuple (other AZs, the regional rec, other +-- platforms and tenancies). The recommendations natural key, rec ID and +-- identity key already carry these three fields (#792, #806). +-- +-- Expand step, compatible with code that predates #816: +-- * The columns are nullable with no default, so adding them is a catalog +-- change and does not rewrite the table. Old code keeps inserting the +-- 6-tuple only; its rows get NULL in all three columns. +-- * NULL in all three means "variant unknown": the reader treats the row +-- as a wildcard over its 6-tuple, which is the pre-#816 behavior. New code +-- always writes all three (''/'' is a known value, e.g. a regional or a +-- non-EC2 rec). The CHECK keeps the three columns all-NULL or all-set. +-- * The 6-tuple UNIQUE constraint is replaced by one that also covers the +-- three columns, so one execution can hold two rows that differ only by +-- AZ, platform or tenancy. Old code aggregates per 6-tuple before +-- inserting and never relies on the constraint (plain INSERT, no +-- ON CONFLICT), so dropping it is safe under old code. NULLS NOT DISTINCT +-- keeps legacy (all-NULL) rows unique per execution and 6-tuple, as before. +ALTER TABLE purchase_suppressions + ADD COLUMN IF NOT EXISTS availability_zone TEXT, + ADD COLUMN IF NOT EXISTS platform TEXT, + ADD COLUMN IF NOT EXISTS tenancy TEXT; + +ALTER TABLE purchase_suppressions + ADD CONSTRAINT purchase_suppressions_variant_all_or_none + CHECK ((availability_zone IS NULL) = (platform IS NULL) + AND (platform IS NULL) = (tenancy IS NULL)); + +ALTER TABLE purchase_suppressions + DROP CONSTRAINT IF EXISTS purchase_suppressions_execution_id_account_id_provider_serv_key; + +ALTER TABLE purchase_suppressions + ADD CONSTRAINT purchase_suppressions_execution_variant_key + UNIQUE NULLS NOT DISTINCT (execution_id, account_id, provider, service, region, + resource_type, engine, availability_zone, platform, tenancy); diff --git a/internal/database/postgres/migrations/000134_purchase_suppressions_variant_test.go b/internal/database/postgres/migrations/000134_purchase_suppressions_variant_test.go new file mode 100644 index 000000000..81860b7b4 --- /dev/null +++ b/internal/database/postgres/migrations/000134_purchase_suppressions_variant_test.go @@ -0,0 +1,114 @@ +//go:build integration + +package migrations_test + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/jackc/pgx/v5/pgconn" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + exec134A = "00000000-0000-0000-0000-000000134a01" + exec134B = "00000000-0000-0000-0000-000000134b01" + + // insertLegacySuppression134 is the insert pre-#816 code issues: the + // 6-tuple only, never the variant columns. + insertLegacySuppression134 = ` + INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, + resource_type, engine, suppressed_count, expires_at) + VALUES ($1, 'acct', 'aws', 'ec2', 'us-east-1', $2, '', $3, NOW() + INTERVAL '7 days')` + + insertVariantSuppression134 = ` + INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, + resource_type, engine, suppressed_count, expires_at, + availability_zone, platform, tenancy) + VALUES ($1, 'acct', 'aws', 'ec2', 'us-east-1', 'm5.large', '', $2, NOW() + INTERVAL '7 days', $3, $4, $5)` +) + +func requireUniqueViolation134(t *testing.T, err error, constraint string) { + t.Helper() + var pgErr *pgconn.PgError + require.ErrorAs(t, err, &pgErr) + assert.Equal(t, "23505", pgErr.Code) + assert.Equal(t, constraint, pgErr.ConstraintName) +} + +// TestMigration_PurchaseSuppressionsVariant locks down migration 000134 +// (issue #816): nullable availability_zone / platform / tenancy columns, the +// all-or-none CHECK, the widened UNIQUE key (legacy NULL rows stay unique per +// execution and 6-tuple), old-code inserts still working, and a down that +// merges variant rows back into one row per execution and 6-tuple. +func TestMigration_PurchaseSuppressionsVariant(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) + pool, path := container.DB.Pool(), getMigrationsPath() + prev := previousMigrationVersion(t, 134) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) + _, err := pool.Exec(ctx, insertLegacySuppression134, exec134A, "m5.large", 2) + require.NoError(t, err) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 134)) + + // A row from before the migration has no variant (NULL in all three). + var nulls int + require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM purchase_suppressions + WHERE availability_zone IS NULL AND platform IS NULL AND tenancy IS NULL`).Scan(&nulls)) + assert.Equal(t, 1, nulls) + + // Old code keeps working after the migration: its insert leaves the + // variant NULL, and a second legacy row on the same execution and 6-tuple + // is still rejected (NULLS NOT DISTINCT). + _, err = pool.Exec(ctx, insertLegacySuppression134, exec134A, "c5.large", 1) + require.NoError(t, err) + _, err = pool.Exec(ctx, insertLegacySuppression134, exec134A, "c5.large", 1) + requireUniqueViolation134(t, err, "purchase_suppressions_execution_variant_key") + + // One execution can now hold rows that differ only by AZ, platform or + // tenancy, including the known-empty regional AZ. + for _, v := range [][3]string{ + {"us-east-1a", "Linux/UNIX", "default"}, + {"us-east-1b", "Linux/UNIX", "default"}, + {"", "Linux/UNIX", "default"}, + {"us-east-1a", "Windows", "default"}, + {"us-east-1a", "Linux/UNIX", "dedicated"}, + } { + _, err = pool.Exec(ctx, insertVariantSuppression134, exec134B, 1, v[0], v[1], v[2]) + require.NoError(t, err, "variant %v", v) + } + _, err = pool.Exec(ctx, insertVariantSuppression134, exec134B, 1, "us-east-1a", "Linux/UNIX", "default") + requireUniqueViolation134(t, err, "purchase_suppressions_execution_variant_key") + + // The CHECK keeps the three columns all-NULL or all-set. + _, err = pool.Exec(ctx, insertVariantSuppression134, exec134B, 1, "us-east-1c", nil, "default") + var pgErr *pgconn.PgError + require.ErrorAs(t, err, &pgErr) + assert.Equal(t, "23514", pgErr.Code) + assert.Equal(t, "purchase_suppressions_variant_all_or_none", pgErr.ConstraintName) + + // Down merges exec134B's five m5.large variant rows into one 6-tuple row + // with the summed count, and restores the 6-tuple UNIQUE constraint. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) + var rows, total int + require.NoError(t, pool.QueryRow(ctx, `SELECT count(*), COALESCE(SUM(suppressed_count), 0) + FROM purchase_suppressions WHERE execution_id = $1`, exec134B).Scan(&rows, &total)) + assert.Equal(t, 1, rows) + assert.Equal(t, 5, total) + require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM purchase_suppressions WHERE execution_id = $1`, exec134A).Scan(&rows)) + assert.Equal(t, 2, rows, "legacy rows on distinct 6-tuples are untouched") + var cols int + require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM information_schema.columns + WHERE table_name = 'purchase_suppressions' AND column_name IN ('availability_zone', 'platform', 'tenancy')`).Scan(&cols)) + assert.Zero(t, cols, "down drops the variant columns") + _, err = pool.Exec(ctx, insertLegacySuppression134, exec134B, "m5.large", 1) + requireUniqueViolation134(t, err, "purchase_suppressions_execution_id_account_id_provider_serv_key") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 134), "up must re-apply cleanly after down") +} diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index f6d4b4051..f48f5ff8d 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1360,13 +1360,15 @@ func (s *Scheduler) resolveEffectiveCacheTTL(ctx context.Context) (ttl time.Dura return time.Duration(globalCfg.RecommendationsCacheStaleHours) * time.Hour, false } -// suppressionKey is the 6-tuple used to match suppressions to recs. +// suppressionKey is the 6-tuple used to group suppressions. Within a group, +// each row's variant (availability zone, platform, tenancy; issue #816) +// decides whether it applies to a given rec (PurchaseSuppression.AppliesTo). type suppressionKey struct { accountID, provider, service, region, resourceType, engine string } -// suppressionAgg aggregates all active suppression rows for a single -// 6-tuple: cumulative count, earliest expiry (drives the badge's +// suppressionAgg aggregates the active suppression rows that apply to one +// rec: cumulative count, earliest expiry (drives the badge's // "Xd remaining"), and the execution whose suppression contributed // the most (drives the badge deep-link). type suppressionAgg struct { @@ -1377,6 +1379,24 @@ type suppressionAgg struct { primaryExecutionContrib int } +// add folds one suppression row into the aggregate. +func (a *suppressionAgg) add(sup *config.PurchaseSuppression) { + a.suppressedCount += sup.SuppressedCount + if sup.ExpiresAt.Before(a.earliestExpiresAt) { + a.earliestExpiresAt = sup.ExpiresAt + } + // "Most contribution" selects the biggest single suppression + // row; ties go to the most-recently-created row so a fresh + // cancel of an older peer doesn't leave the badge pointing at + // a stale execution. + if sup.SuppressedCount > a.primaryExecutionContrib || + (sup.SuppressedCount == a.primaryExecutionContrib && sup.CreatedAt.After(a.primaryExecutionCreated)) { + a.primaryExecutionID = sup.ExecutionID + a.primaryExecutionCreated = sup.CreatedAt + a.primaryExecutionContrib = sup.SuppressedCount + } +} + // applySuppressions subtracts active purchase_suppressions from each // rec's count and annotates the rec with the SuppressedCount / // SuppressionExpiresAt / PrimarySuppressionExecutionID fields that @@ -1394,42 +1414,49 @@ func (s *Scheduler) applySuppressions(ctx context.Context, recs []config.Recomme return applySuppressionIndex(recs, index), nil } -// aggregateSuppressions groups a raw suppression list by 6-tuple. -func aggregateSuppressions(sups []config.PurchaseSuppression) map[suppressionKey]*suppressionAgg { - index := make(map[suppressionKey]*suppressionAgg, len(sups)) +// aggregateSuppressions groups a raw suppression list by 6-tuple. The +// per-rec aggregate is built in applySuppressionIndex, because which rows +// of a group apply depends on the rec's variant. +func aggregateSuppressions(sups []config.PurchaseSuppression) map[suppressionKey][]config.PurchaseSuppression { + index := make(map[suppressionKey][]config.PurchaseSuppression, len(sups)) for i := range sups { sup := sups[i] k := suppressionKey{ accountID: sup.AccountID, provider: sup.Provider, service: sup.Service, region: sup.Region, resourceType: sup.ResourceType, engine: sup.Engine, } - a, ok := index[k] - if !ok { - a = &suppressionAgg{earliestExpiresAt: sup.ExpiresAt} - index[k] = a - } - a.suppressedCount += sup.SuppressedCount - if sup.ExpiresAt.Before(a.earliestExpiresAt) { - a.earliestExpiresAt = sup.ExpiresAt + index[k] = append(index[k], sup) + } + return index +} + +// matchingSuppressions aggregates the rows of rec's 6-tuple group that apply +// to rec's variant: rows for the same availability zone, platform and +// tenancy, plus legacy rows with no variant (a wildcard over the 6-tuple). +// A rec whose Details cannot be read matches every row of its group, the +// pre-#816 behavior. Returns nil when no row applies. +func matchingSuppressions(rec *config.RecommendationRecord, group []config.PurchaseSuppression) *suppressionAgg { + var recVariant *config.SuppressionVariant + if v, err := rec.SuppressionVariant(); err == nil { + recVariant = &v + } + var a *suppressionAgg + for i := range group { + if !group[i].AppliesTo(recVariant) { + continue } - // "Most contribution" selects the biggest single suppression - // row; ties go to the most-recently-created row so a fresh - // cancel of an older peer doesn't leave the badge pointing at - // a stale execution. - if sup.SuppressedCount > a.primaryExecutionContrib || - (sup.SuppressedCount == a.primaryExecutionContrib && sup.CreatedAt.After(a.primaryExecutionCreated)) { - a.primaryExecutionID = sup.ExecutionID - a.primaryExecutionCreated = sup.CreatedAt - a.primaryExecutionContrib = sup.SuppressedCount + if a == nil { + a = &suppressionAgg{earliestExpiresAt: group[i].ExpiresAt} } + a.add(&group[i]) } - return index + return a } -// applySuppressionIndex walks recs, subtracts the matching aggregate -// count from each rec, drops recs that go to 0 or below, and -// annotates the survivors with the badge fields. -func applySuppressionIndex(recs []config.RecommendationRecord, index map[suppressionKey]*suppressionAgg) []config.RecommendationRecord { +// applySuppressionIndex walks recs, subtracts the count of the +// suppressions that apply to each rec, drops recs that go to 0 or below, +// and annotates the survivors with the badge fields. +func applySuppressionIndex(recs []config.RecommendationRecord, index map[suppressionKey][]config.PurchaseSuppression) []config.RecommendationRecord { // Allocate a fresh backing array so callers that hold a reference to // the original recs slice do not see mutations (05-M1). out := make([]config.RecommendationRecord, 0, len(recs)) @@ -1443,8 +1470,8 @@ func applySuppressionIndex(recs []config.RecommendationRecord, index map[suppres accountID: accountID, provider: rec.Provider, service: rec.Service, region: rec.Region, resourceType: rec.ResourceType, engine: rec.Engine, } - a, ok := index[k] - if !ok { + a := matchingSuppressions(&rec, index[k]) + if a == nil { out = append(out, rec) continue } diff --git a/internal/scheduler/scheduler_suppressions_test.go b/internal/scheduler/scheduler_suppressions_test.go index 6c7f442f9..92d44d95c 100644 --- a/internal/scheduler/scheduler_suppressions_test.go +++ b/internal/scheduler/scheduler_suppressions_test.go @@ -244,3 +244,117 @@ func TestApplySuppressionIndex_DoesNotMutateCallerSlice(t *testing.T) { // Suppress unused-import warning in case this is the only file that imports pgx. var _ pgx.Tx = (pgx.Tx)(nil) + +// ec2VariantRec is a zonal or regional EC2 rec whose platform and tenancy live +// in Details, as the collector stores them. +func ec2VariantRec(id, az, platform, tenancy string) config.RecommendationRecord { + return config.RecommendationRecord{ + ID: id, Provider: "aws", Service: "ec2", Region: "us-east-1", + ResourceType: "m5.large", Count: 5, CloudAccountID: strPtr("acct-1"), + AvailabilityZone: az, + Details: []byte(`{"availability_zone":"` + az + `","platform":"` + platform + `","tenancy":"` + tenancy + `"}`), + } +} + +// TestApplySuppressions_OnlyThePurchasedVariant is the issue #816 regression: +// buying the us-east-1a Linux/default variant must reduce only that rec. Before +// the fix the 6-tuple key subtracted the purchase from the us-east-1b, +// regional, Windows and dedicated siblings too, hiding them for the whole +// grace window. The suppressions come from the real BuildSuppressions, so the +// write and read sides are exercised together. +func TestApplySuppressions_OnlyThePurchasedVariant(t *testing.T) { + ctx := context.Background() + bought := ec2VariantRec("bought", "us-east-1a", "Linux/UNIX", "default") + siblings := []config.RecommendationRecord{ + ec2VariantRec("other-az", "us-east-1b", "Linux/UNIX", "default"), + ec2VariantRec("regional", "", "Linux/UNIX", "default"), + ec2VariantRec("windows", "us-east-1a", "Windows", "default"), + ec2VariantRec("dedicated", "us-east-1a", "Linux/UNIX", "dedicated"), + } + purchase := bought + purchase.Count = 3 + sups := config.BuildSuppressions([]config.RecommendationRecord{purchase}, "e1", + &config.GlobalConfig{GracePeriodDays: map[string]int{"aws": 7}}, time.Now()) + require.Len(t, sups, 1) + + store := &mockSuppressionStore{recs: append([]config.RecommendationRecord{bought}, siblings...), sups: sups} + s := &Scheduler{config: store} + + recs, err := s.ListRecommendations(ctx, config.RecommendationFilter{}) + require.NoError(t, err) + got := map[string]config.RecommendationRecord{} + for _, r := range recs { + got[r.ID] = r + } + require.Len(t, got, 5, "no sibling may be hidden by the purchase") + assert.Equal(t, 2, got["bought"].Count, "5 - 3 on the purchased variant") + assert.Equal(t, 3, got["bought"].SuppressedCount) + for _, sib := range siblings { + assert.Equal(t, 5, got[sib.ID].Count, "%s must keep its full count", sib.ID) + assert.Zero(t, got[sib.ID].SuppressedCount, "%s must carry no suppression badge", sib.ID) + assert.Nil(t, got[sib.ID].PrimarySuppressionExecutionID, sib.ID) + } +} + +// TestApplySuppressions_LegacyRowIsAWildcard pins the expand/contract rule of +// issue #816: a row without a variant (written before migration 000134, or by +// pre-#816 code during a rolling deploy) still applies to every rec on its +// 6-tuple, as before, while a variant row applies only to its own variant and +// adds to the legacy count there. +func TestApplySuppressions_LegacyRowIsAWildcard(t *testing.T) { + future := time.Now().Add(24 * time.Hour) + legacy := config.PurchaseSuppression{ExecutionID: "legacy", AccountID: "acct-1", Provider: "aws", + Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", SuppressedCount: 1, ExpiresAt: future} + scoped := legacy + scoped.ExecutionID = "scoped" + scoped.SuppressedCount = 2 + scoped.Variant = &config.SuppressionVariant{AvailabilityZone: "us-east-1a", Platform: "Linux/UNIX", Tenancy: "default"} + + recs := []config.RecommendationRecord{ + ec2VariantRec("a-linux", "us-east-1a", "Linux/UNIX", "default"), + ec2VariantRec("b-linux", "us-east-1b", "Linux/UNIX", "default"), + } + out := applySuppressionIndex(recs, aggregateSuppressions([]config.PurchaseSuppression{legacy, scoped})) + require.Len(t, out, 2) + assert.Equal(t, 2, out[0].Count, "a-linux: 5 - 1 legacy - 2 scoped") + assert.Equal(t, 3, out[0].SuppressedCount) + require.NotNil(t, out[0].PrimarySuppressionExecutionID) + assert.Equal(t, "scoped", *out[0].PrimarySuppressionExecutionID) + assert.Equal(t, 4, out[1].Count, "b-linux: only the legacy wildcard applies") + require.NotNil(t, out[1].PrimarySuppressionExecutionID) + assert.Equal(t, "legacy", *out[1].PrimarySuppressionExecutionID) +} + +// TestApplySuppressions_NonComputeVariantMatches: a non-EC2 rec has an empty +// variant (empty AZ, platform and tenancy), which is a known value and must match +// the row its own purchase wrote, exactly as the 6-tuple did before #816. +func TestApplySuppressions_NonComputeVariantMatches(t *testing.T) { + rds := config.RecommendationRecord{ID: "rds", Provider: "aws", Service: "rds", Region: "us-east-1", + ResourceType: "db.t4g.micro", Engine: "postgres", Count: 5, CloudAccountID: strPtr("acct-1"), + Details: []byte(`{"engine":"postgres","deployment_option":"single-az"}`)} + purchase := rds + purchase.Count = 2 + sups := config.BuildSuppressions([]config.RecommendationRecord{purchase}, "e1", &config.GlobalConfig{}, time.Now()) + require.Len(t, sups, 1) + require.NotNil(t, sups[0].Variant) + + out := applySuppressionIndex([]config.RecommendationRecord{rds}, aggregateSuppressions(sups)) + require.Len(t, out, 1) + assert.Equal(t, 3, out[0].Count) +} + +// TestApplySuppressions_UnreadableRecDetailsMatchEveryRow: a rec whose Details +// cannot be read has no known variant, so every row on its 6-tuple applies +// (the pre-#816 behavior: over-suppress, never over-show). +func TestApplySuppressions_UnreadableRecDetailsMatchEveryRow(t *testing.T) { + future := time.Now().Add(24 * time.Hour) + sup := config.PurchaseSuppression{ExecutionID: "e1", AccountID: "acct-1", Provider: "aws", + Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", SuppressedCount: 2, ExpiresAt: future, + Variant: &config.SuppressionVariant{AvailabilityZone: "us-east-1a", Platform: "Linux/UNIX", Tenancy: "default"}} + rec := ec2VariantRec("broken", "us-east-1b", "Windows", "default") + rec.Details = []byte(`{"platform":42}`) + + out := applySuppressionIndex([]config.RecommendationRecord{rec}, aggregateSuppressions([]config.PurchaseSuppression{sup})) + require.Len(t, out, 1) + assert.Equal(t, 3, out[0].Count) +} diff --git a/internal/scheduler/suppression_variant_integration_test.go b/internal/scheduler/suppression_variant_integration_test.go new file mode 100644 index 000000000..6c6001e74 --- /dev/null +++ b/internal/scheduler/suppression_variant_integration_test.go @@ -0,0 +1,78 @@ +//go:build integration + +package scheduler + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/google/uuid" + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestSuppressionsScopedToPurchasedVariant_RealPostgres is the issue #816 +// regression on a real database: recs are persisted and listed through the +// real store, the purchase's suppressions are built by BuildSuppressions and +// written by CreateSuppressionTx, and ListRecommendations subtracts them. +// Buying us-east-1a Linux/default must not hide the us-east-1b, regional, +// Windows or dedicated siblings. A row written the pre-#816 way (no variant +// columns, as old code inserts during a rolling deploy) still applies to every +// variant of its 6-tuple. +func TestSuppressionsScopedToPurchasedVariant_RealPostgres(t *testing.T) { + ctx := t.Context() + pg, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, pg.Cleanup(context.Background())) }) + require.NoError(t, migrations.RunMigrations(ctx, pg.DB.Pool(), "../database/postgres/migrations", "", "")) + store := config.NewPostgresStore(pg.DB) + + rec := func(id, resourceType, az, platform, tenancy string) config.RecommendationRecord { + return config.RecommendationRecord{ + ID: id, Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: resourceType, + Count: 5, Term: 1, Payment: "all-upfront", AvailabilityZone: az, + Details: []byte(`{"availability_zone":"` + az + `","platform":"` + platform + `","tenancy":"` + tenancy + `"}`), + } + } + bought := rec("bought", "m5.large", "us-east-1a", "Linux/UNIX", "default") + records := []config.RecommendationRecord{ + bought, + rec("other-az", "m5.large", "us-east-1b", "Linux/UNIX", "default"), + rec("regional", "m5.large", "", "Linux/UNIX", "default"), + rec("windows", "m5.large", "us-east-1a", "Windows", "default"), + rec("dedicated", "m5.large", "us-east-1a", "Linux/UNIX", "dedicated"), + rec("legacy-a", "c5.large", "us-east-1a", "Linux/UNIX", "default"), + rec("legacy-b", "c5.large", "us-east-1b", "Windows", "default"), + } + require.NoError(t, store.UpsertRecommendations(ctx, time.Now(), records, nil)) + + purchase := bought + purchase.Count = 3 + sups := config.BuildSuppressions([]config.RecommendationRecord{purchase}, 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]) })) + + // The insert old code issues: 6-tuple only, variant columns left NULL. + _, err = pg.DB.Pool().Exec(ctx, ` + INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, + resource_type, engine, suppressed_count, expires_at) + VALUES ($1, '', 'aws', 'ec2', 'us-east-1', 'c5.large', '', 1, NOW() + INTERVAL '1 day')`, uuid.NewString()) + require.NoError(t, err) + + s := &Scheduler{config: store, isLambda: true} + got, err := s.ListRecommendations(ctx, config.RecommendationFilter{}) + require.NoError(t, err) + counts := map[string]int{} + for _, r := range got { + counts[r.ID] = r.Count + } + assert.Equal(t, map[string]int{ + "bought": 2, "other-az": 5, "regional": 5, "windows": 5, "dedicated": 5, + "legacy-a": 4, "legacy-b": 4, + }, counts) +}