From 5c408e0fad44c3f195274f3032f3a313e8271610 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Thu, 22 Oct 2026 01:42:41 +0100 Subject: [PATCH 01/19] feat(db): add generated az_config column and 12-column recommendations index (#408) Migration 000140 adds a generated az_config column, read verbatim from payload->'details'->>'az_config' (RDS single-az/multi-az, Azure SQL zone redundancy, '' otherwise), and two unique indexes that include it: the 11-column natural key plus az_config (the new upsert arbiter) and the 000120 source-account key plus az_config. The existing 11-column and 000120 source indexes are kept, so the current upsert keeps working and this can be applied before the store switch. Co-Authored-By: Claude Opus 5.5 --- .../000140_recommendations_az_config.down.sql | 4 + .../000140_recommendations_az_config.up.sql | 44 ++++++ .../000140_recommendations_az_config_test.go | 132 ++++++++++++++++++ 3 files changed, 180 insertions(+) create mode 100644 internal/database/postgres/migrations/000140_recommendations_az_config.down.sql create mode 100644 internal/database/postgres/migrations/000140_recommendations_az_config.up.sql create mode 100644 internal/database/postgres/migrations/000140_recommendations_az_config_test.go diff --git a/internal/database/postgres/migrations/000140_recommendations_az_config.down.sql b/internal/database/postgres/migrations/000140_recommendations_az_config.down.sql new file mode 100644 index 000000000..23050ab14 --- /dev/null +++ b/internal/database/postgres/migrations/000140_recommendations_az_config.down.sql @@ -0,0 +1,4 @@ +DROP INDEX IF EXISTS recommendations_natural_key_source_azc_idx; +DROP INDEX IF EXISTS recommendations_natural_key_az_tp_azc_idx; + +ALTER TABLE recommendations DROP COLUMN IF EXISTS az_config; diff --git a/internal/database/postgres/migrations/000140_recommendations_az_config.up.sql b/internal/database/postgres/migrations/000140_recommendations_az_config.up.sql new file mode 100644 index 000000000..ed47c2790 --- /dev/null +++ b/internal/database/postgres/migrations/000140_recommendations_az_config.up.sql @@ -0,0 +1,44 @@ +-- Migration 000140: az_config generated column and a 12-column unique index on +-- recommendations (#408, expand step of the RDS AZ-config identity work). +-- +-- Cost Explorer reports Single-AZ and Multi-AZ RDS reservations of the same +-- instance class, engine, term and payment as separate recommendations. They +-- are different offerings with different prices, but the 11-column natural key +-- cannot tell them apart, so a batch carrying both fails the whole collection +-- write with SQLSTATE 21000. This migration adds the AZ configuration to the +-- schema; nothing uses it yet. +-- +-- The column is derived verbatim from the stored payload (the JSON of the +-- RecommendationRecord) at payload->'details' (the DatabaseDetails blob), so +-- existing rows are backfilled by the table rewrite with no data migration. +-- No trimming or case-folding. Absent Details, JSON null, a missing key and a +-- null value all become ''. Every non-database row is ''. +-- +-- Two indexes are added: +-- - recommendations_natural_key_az_tp_azc_idx: the 11 columns plus az_config, +-- the new upsert arbiter. +-- - recommendations_natural_key_source_azc_idx: the source-account key of +-- migration 000120 (#799) plus az_config. Every unique index without +-- az_config rejects a Single-AZ/Multi-AZ pair, so the source-aware key has +-- to gain the column too; 000141 retires the 000120 index it replaces. +-- +-- The existing 11-column recommendations_natural_key_az_tp_idx and the +-- 000120 recommendations_natural_key_source_idx are KEPT, so the current +-- ON CONFLICT on the 11 columns keeps working for old and new code. Two rows +-- that differ only by az_config are still rejected by those indexes until +-- migration 000141 drops them (applied only once the store upserts on the +-- 12-column index). +-- +-- Lock impact: ADD COLUMN ... GENERATED ... STORED rewrites the table and the +-- index build blocks writes. The table is a regenerable cache re-populated +-- every collection cycle, so this is acceptable. CREATE INDEX CONCURRENTLY is +-- not possible: the migration runner wraps every migration in a transaction. +ALTER TABLE recommendations + ADD COLUMN IF NOT EXISTS az_config TEXT + GENERATED ALWAYS AS (COALESCE(payload->'details'->>'az_config', '')) STORED; + +CREATE UNIQUE INDEX IF NOT EXISTS recommendations_natural_key_az_tp_azc_idx + ON recommendations (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, az_config); + +CREATE UNIQUE INDEX IF NOT EXISTS recommendations_natural_key_source_azc_idx + ON recommendations (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, az_config, source_account_key); diff --git a/internal/database/postgres/migrations/000140_recommendations_az_config_test.go b/internal/database/postgres/migrations/000140_recommendations_az_config_test.go new file mode 100644 index 000000000..801dc4e9c --- /dev/null +++ b/internal/database/postgres/migrations/000140_recommendations_az_config_test.go @@ -0,0 +1,132 @@ +//go:build integration +// +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 insertRecForMigration140 = ` + INSERT INTO recommendations + (collected_at, provider, service, region, resource_type, engine, payload, + upfront_cost, monthly_savings, term, payment_option) + VALUES (NOW(), 'aws', 'rds', 'us-east-1', $1, 'mysql', $2::jsonb, 0, 0, 1, 'all-upfront') +` + +// TestMigration_RecommendationsAZConfig locks down migration 000140 (issue +// #408, expand step): the generated az_config column is read verbatim from +// payload->'details', the 11-column index is kept so the previous upsert still +// works, the 12-column index is added, and rollback removes only the new +// column and index. The vectors are mirrored by TestDetailsAZConfig_Vectors +// (internal/config) so the Go reader cannot diverge from the column. +func TestMigration_RecommendationsAZConfig(t *testing.T) { + ctx := context.Background() + migrationsPath := getMigrationsPath() + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 140))) + + type vector struct{ payload, azConfig string } + vectors := map[string]vector{ + "details-absent": {`{"service":"rds"}`, ""}, + "details-null": {`{"details":null}`, ""}, + "details-empty": {`{"details":{}}`, ""}, + "az-config-null": {`{"details":{"az_config":null}}`, ""}, + "empty-string": {`{"details":{"az_config":""}}`, ""}, + "rds-single-az": {`{"details":{"az_config":"single-az"}}`, "single-az"}, + "rds-multi-az": {`{"details":{"az_config":"multi-az"}}`, "multi-az"}, + "azure-zone-redundant": {`{"details":{"az_config":"zoneRedundant"}}`, "zoneRedundant"}, + "verbatim-spaces": {`{"details":{"az_config":" Multi-AZ "}}`, " Multi-AZ "}, + "compute-details": {`{"details":{"platform":"Linux/UNIX","tenancy":"default"}}`, ""}, + // A top-level key is NOT consulted: only Details carries the field. + "top-level-ignored": {`{"az_config":"multi-az"}`, ""}, + } + for name, v := range vectors { + _, err = pool.Exec(ctx, insertRecForMigration140, name, v.payload) + require.NoError(t, err) + } + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 140)) + + for name, v := range vectors { + var got string + require.NoError(t, pool.QueryRow(ctx, `SELECT az_config FROM recommendations WHERE resource_type = $1`, name).Scan(&got)) + assert.Equal(t, v.azConfig, got, "az_config of %s", name) + } + + for _, idx := range []string{ + "recommendations_natural_key_az_tp_idx", "recommendations_natural_key_az_tp_azc_idx", + "recommendations_natural_key_source_idx", "recommendations_natural_key_source_azc_idx", + } { + var exists bool + require.NoError(t, pool.QueryRow(ctx, + `SELECT EXISTS (SELECT 1 FROM pg_indexes WHERE tablename = 'recommendations' AND indexname = $1)`, idx).Scan(&exists)) + assert.True(t, exists, "index %s must exist after 000140", idx) + } + var indexDef string + require.NoError(t, pool.QueryRow(ctx, + `SELECT indexdef FROM pg_indexes WHERE indexname = 'recommendations_natural_key_az_tp_azc_idx'`).Scan(&indexDef)) + assert.Contains(t, indexDef, "UNIQUE INDEX") + assert.Contains(t, indexDef, "(account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, az_config)") + require.NoError(t, pool.QueryRow(ctx, + `SELECT indexdef FROM pg_indexes WHERE indexname = 'recommendations_natural_key_source_azc_idx'`).Scan(&indexDef)) + assert.Contains(t, indexDef, "UNIQUE INDEX") + assert.Contains(t, indexDef, "(account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, az_config, source_account_key)") + + // The previous code's upsert (11-column ON CONFLICT) still works and + // updates the existing row in place, so 000140 can be applied before the + // store switch is released. + singleAZ := `{"details":{"az_config":"single-az"}}` + _, err = pool.Exec(ctx, insertRecForMigration140+` + ON CONFLICT (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy) + DO UPDATE SET upfront_cost = 5`, "pair", singleAZ) + require.NoError(t, err) + _, err = pool.Exec(ctx, insertRecForMigration140+` + ON CONFLICT (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy) + DO UPDATE SET upfront_cost = 5`, "pair", singleAZ) + require.NoError(t, err, "the existing 11-column upsert must keep working after 000140") + var n int + require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*) FROM recommendations WHERE resource_type = 'pair'`).Scan(&n)) + assert.Equal(t, 1, n) + + // Known limitation, resolved by 000141: a row differing only by az_config + // is still rejected by the kept indexes without az_config. + _, err = pool.Exec(ctx, insertRecForMigration140, "pair", `{"details":{"az_config":"multi-az"}}`) + var pgErr *pgconn.PgError + require.ErrorAs(t, err, &pgErr) + assert.Equal(t, "23505", pgErr.Code) + assert.Contains(t, []string{"recommendations_natural_key_az_tp_idx", "recommendations_natural_key_source_idx"}, pgErr.ConstraintName) + + // Rollback removes the new column and index only. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 140))) + var colExists bool + require.NoError(t, pool.QueryRow(ctx, + `SELECT EXISTS (SELECT 1 FROM information_schema.columns WHERE table_name = 'recommendations' AND column_name = 'az_config')`).Scan(&colExists)) + assert.False(t, colExists) + for idx, want := range map[string]bool{ + "recommendations_natural_key_az_tp_azc_idx": false, + "recommendations_natural_key_source_azc_idx": false, + "recommendations_natural_key_az_tp_idx": true, + "recommendations_natural_key_source_idx": true, + } { + var exists bool + require.NoError(t, pool.QueryRow(ctx, `SELECT EXISTS (SELECT 1 FROM pg_indexes WHERE indexname = $1)`, idx).Scan(&exists)) + assert.Equal(t, want, exists, "index %s after rollback", idx) + } + require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*) FROM recommendations`).Scan(&n)) + assert.Equal(t, len(vectors)+1, n, "rollback must not delete rows") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 140)) +} From a15aff2abaac198097b0885023c862d1de403d7c Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Thu, 22 Oct 2026 01:42:41 +0100 Subject: [PATCH 02/19] fix(purchase): carry RDS az_config in rec ID, identity key and idempotency key (#408) Single-AZ and Multi-AZ RDS recommendations of the same class, engine, term and payment shared one rec ID and one identity key. Read az_config verbatim from Details (RecommendationRecord.DetailsAZConfig, pinned to the 000140 generated column by shared vectors) and add it to RecIdentityKey, the rec ID (appended only when set, so every other rec keeps its ID), the purchase idempotency key and describeRec. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_purchases.go | 10 ++- internal/api/purchase_azconfig_test.go | 88 +++++++++++++++++++ internal/api/purchase_pricing.go | 3 + internal/config/recommendation_identity.go | 33 ++++++- .../config/recommendation_identity_test.go | 56 ++++++++++++ .../scheduler/recommendation_identity_test.go | 34 +++++++ internal/scheduler/scheduler.go | 11 +++ 7 files changed, 230 insertions(+), 5 deletions(-) create mode 100644 internal/api/purchase_azconfig_test.go diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index af7c61b96..65e10cff4 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -3288,11 +3288,17 @@ func purchaseIdempotencyKey(creatorID string, recs []config.RecommendationRecord if err != nil { return "", err } - tuples = append(tuples, fmt.Sprintf("%s|%s|%s|%s|%s|%s|%d|%d|%s|%s|%s|%s", + // Single-AZ and Multi-AZ RDS buys of the same class are different + // purchases and must not dedupe into one (issue #408). + azConfig, err := r.DetailsAZConfig() + if err != nil { + return "", err + } + tuples = append(tuples, fmt.Sprintf("%s|%s|%s|%s|%s|%s|%d|%d|%s|%s|%s|%s|%s", strings.ToLower(strings.TrimSpace(r.Provider)), r.Service, r.Region, r.ResourceType, r.Engine, acct, r.Count, r.Term, strings.ToLower(strings.TrimSpace(r.Payment)), az, - ci.Platform, ci.Tenancy)) + ci.Platform, ci.Tenancy, azConfig)) } sort.Strings(tuples) h := sha256.New() diff --git a/internal/api/purchase_azconfig_test.go b/internal/api/purchase_azconfig_test.go new file mode 100644 index 000000000..b1572a18e --- /dev/null +++ b/internal/api/purchase_azconfig_test.go @@ -0,0 +1,88 @@ +package api + +import ( + "context" + "encoding/json" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Real RDS Details shape as persisted by the scheduler (issue #408). +func rdsDetails(azConfig string) json.RawMessage { + return json.RawMessage(`{"engine":"mysql","az_config":"` + azConfig + `","instance_class":"db.r5.large"}`) +} + +func rdsAZConfigRec(azConfig string, upfront float64) config.RecommendationRecord { + return config.RecommendationRecord{ + ID: "rec-rds-" + azConfig, Provider: "aws", Service: "rds", Region: "us-east-1", ResourceType: "db.r5.large", + Engine: "mysql", Term: 1, Payment: "all-upfront", Count: 1, UpfrontCost: upfront, Savings: 10, + Details: rdsDetails(azConfig), + } +} + +// Stored Single-AZ and Multi-AZ rows of the same class must both load (no +// "more than one row for identity key" refusal) and the request, which carries +// az_config only in Details, is priced from its own variant's row. +func TestPriceRecommendationsFromStore_PricesFromRequestedAZConfig(t *testing.T) { + for _, want := range []struct { + azConfig string + upfront float64 + }{ + {"single-az", 100}, + {"multi-az", 300}, + } { + t.Run(want.azConfig, func(t *testing.T) { + store := new(MockConfigStore) + t.Cleanup(func() { store.AssertExpectations(t) }) + expectStoredRecs(store, rdsAZConfigRec("single-az", 100), rdsAZConfigRec("multi-az", 300)) + h := &Handler{config: store} + req := []config.RecommendationRecord{{ + Provider: "aws", Service: "rds", Region: "us-east-1", ResourceType: "db.r5.large", Engine: "mysql", + Term: 1, Payment: "all-upfront", Count: 1, Details: rdsDetails(want.azConfig), + }} + require.NoError(t, h.priceRecommendationsFromStore(context.Background(), req)) + assert.Equal(t, want.upfront, req[0].UpfrontCost) + got, err := req[0].DetailsAZConfig() + require.NoError(t, err) + assert.Equal(t, want.azConfig, got) + }) + } +} + +// A request that does not name its az_config cannot be priced off an arbitrary +// RDS variant: it is refused with a 409. +func TestPriceRecommendationsFromStore_RequestWithoutAZConfigRefusedForRDS(t *testing.T) { + store := new(MockConfigStore) + t.Cleanup(func() { store.AssertExpectations(t) }) + expectStoredRecs(store, rdsAZConfigRec("multi-az", 300)) + h := &Handler{config: store} + req := []config.RecommendationRecord{{ + Provider: "aws", Service: "rds", Region: "us-east-1", ResourceType: "db.r5.large", Engine: "mysql", + Term: 1, Payment: "all-upfront", Count: 1, + }} + err := h.priceRecommendationsFromStore(context.Background(), req) + require.Error(t, err) + ce, isClient := IsClientError(err) + require.True(t, isClient) + assert.Equal(t, 409, ce.code) +} + +func TestPurchaseIdempotencyKey_DiffersByAZConfig(t *testing.T) { + t.Parallel() + key := func(azConfig string) string { + return mustPurchaseIdempotencyKey("u", []config.RecommendationRecord{rdsAZConfigRec(azConfig, 1)}, 100) + } + assert.NotEqual(t, key("single-az"), key("multi-az"), + "a Multi-AZ submit right after a Single-AZ one must not dedupe into it") + assert.Equal(t, key("multi-az"), key("multi-az")) +} + +func TestDescribeRec_IncludesAZConfig(t *testing.T) { + r := rdsAZConfigRec("multi-az", 1) + assert.Contains(t, describeRec(&r), `az_config="multi-az"`) + bare := config.RecommendationRecord{Provider: "aws", Service: "rds"} + assert.NotContains(t, describeRec(&bare), "az_config=") +} diff --git a/internal/api/purchase_pricing.go b/internal/api/purchase_pricing.go index 0d3f76610..e270aa0b9 100644 --- a/internal/api/purchase_pricing.go +++ b/internal/api/purchase_pricing.go @@ -363,5 +363,8 @@ func describeRec(rec *config.RecommendationRecord) string { if ci, err := rec.ComputeIdentity(); err == nil && !ci.IsZero() { desc += fmt.Sprintf(" platform=%q tenancy=%q", ci.Platform, ci.Tenancy) } + if azConfig, err := rec.DetailsAZConfig(); err == nil && azConfig != "" { + desc += fmt.Sprintf(" az_config=%q", azConfig) + } return desc } diff --git a/internal/config/recommendation_identity.go b/internal/config/recommendation_identity.go index c278f85f4..86e8a8945 100644 --- a/internal/config/recommendation_identity.go +++ b/internal/config/recommendation_identity.go @@ -41,10 +41,33 @@ func (r *RecommendationRecord) ComputeIdentity() (ComputeIdentity, error) { return ComputeIdentity{Platform: d.Platform, Tenancy: d.Tenancy}, nil } -// RecIdentityKey is the tuple a price is a function of: the same eleven +// DetailsAZConfig reads the database AZ configuration (RDS "single-az" / +// "multi-az", Azure SQL "zoneRedundant" / "none") from the rec's Details +// blob, VERBATIM. Single-AZ and Multi-AZ RDS reservations are different +// offerings with different prices, yet share every other identity field +// (issue #408). It mirrors the generated az_config column of migration 000140 +// (COALESCE(payload->'details'->>'az_config', ”)) and is pinned to it by the +// shared vectors in TestDetailsAZConfig_Vectors: absent Details, JSON null, a +// missing key and a null value all yield "". Only DatabaseDetails carries the +// key. A non-string value is an error. +func (r *RecommendationRecord) DetailsAZConfig() (string, error) { + if len(r.Details) == 0 { + return "", nil + } + var d struct { + AZConfig string `json:"az_config"` + } + if err := json.Unmarshal(r.Details, &d); err != nil { + return "", fmt.Errorf("decode az_config of %s/%s details: %w", r.Region, r.ResourceType, err) + } + return d.AZConfig, nil +} + +// RecIdentityKey is the tuple a price is a function of: the same twelve // components (the ninth is the zonal EC2 availability zone, "" for every // other rec, issue #792; the tenth and eleventh are the EC2 platform and -// tenancy read from Details, "" when absent, issue #806) behind +// tenancy read from Details, "" when absent, issue #806; the twelfth is the +// database az_config read from Details, "" when absent, issue #408) behind // RecommendationRecord.ID, with a nil // and an empty CloudAccountID collapsed together (as purchaseConstraintSets // and the store's account_key both do). Provider and payment are lowercased @@ -54,6 +77,10 @@ func RecIdentityKey(rec *RecommendationRecord) (string, error) { if err != nil { return "", err } + azConfig, err := rec.DetailsAZConfig() + if err != nil { + return "", err + } account := "" if rec.CloudAccountID != nil { account = *rec.CloudAccountID @@ -61,6 +88,6 @@ func RecIdentityKey(rec *RecommendationRecord) (string, error) { return strings.Join([]string{ strings.ToLower(rec.Provider), account, rec.Service, rec.Region, rec.ResourceType, rec.Engine, strconv.Itoa(rec.Term), strings.ToLower(rec.Payment), - rec.AvailabilityZone, ci.Platform, ci.Tenancy, + rec.AvailabilityZone, ci.Platform, ci.Tenancy, azConfig, }, "\x1f"), nil } diff --git a/internal/config/recommendation_identity_test.go b/internal/config/recommendation_identity_test.go index 5f8fcf505..26c1372bf 100644 --- a/internal/config/recommendation_identity_test.go +++ b/internal/config/recommendation_identity_test.go @@ -45,3 +45,59 @@ func TestComputeIdentity_TopLevelKeyIgnoredAndMalformedFailsLoud(t *testing.T) { _, err = bad.ComputeIdentity() assert.Error(t, err) } + +// The vectors mirror TestMigration_RecommendationsAZConfig (migration 000140, +// internal/database/postgres/migrations) so the Go reader and the generated +// az_config column cannot diverge (issue #408). Details here is the details +// blob itself. +func TestDetailsAZConfig_Vectors(t *testing.T) { + vectors := []struct { + name, details, azConfig string + }{ + {"details-absent", ``, ""}, + {"details-null", `null`, ""}, + {"details-empty", `{}`, ""}, + {"az-config-null", `{"az_config":null}`, ""}, + {"empty-string", `{"az_config":""}`, ""}, + {"rds-single-az", `{"engine":"mysql","az_config":"single-az","instance_class":"db.r5.large"}`, "single-az"}, + {"rds-multi-az", `{"engine":"mysql","az_config":"multi-az","instance_class":"db.r5.large"}`, "multi-az"}, + {"azure-zone-redundant", `{"engine":"sqlserver","az_config":"zoneRedundant"}`, "zoneRedundant"}, + {"verbatim-spaces", `{"az_config":" Multi-AZ "}`, " Multi-AZ "}, + {"compute-details", `{"platform":"Linux/UNIX","tenancy":"default","scope":"Region"}`, ""}, + } + for _, v := range vectors { + t.Run(v.name, func(t *testing.T) { + rec := RecommendationRecord{Details: json.RawMessage(v.details)} + got, err := rec.DetailsAZConfig() + require.NoError(t, err) + assert.Equal(t, v.azConfig, got) + }) + } +} + +func TestDetailsAZConfig_MalformedFailsLoud(t *testing.T) { + bad := RecommendationRecord{Details: json.RawMessage(`{"az_config":5}`)} + _, err := bad.DetailsAZConfig() + require.Error(t, err) + _, err = RecIdentityKey(&bad) + require.Error(t, err, "the identity key must not silently drop an unreadable az_config") +} + +// Two otherwise identical RDS rows that differ only by az_config have +// different identity keys; the same az_config yields the same key. +func TestRecIdentityKey_DiffersByAZConfig(t *testing.T) { + rds := func(azConfig string) *RecommendationRecord { + return &RecommendationRecord{ + Provider: "aws", Service: "rds", Region: "us-east-1", ResourceType: "db.r5.large", Engine: "mysql", + Term: 1, Payment: "all-upfront", Details: json.RawMessage(`{"engine":"mysql","az_config":"` + azConfig + `"}`), + } + } + key := func(r *RecommendationRecord) string { + k, err := RecIdentityKey(r) + require.NoError(t, err) + return k + } + assert.NotEqual(t, key(rds("single-az")), key(rds("multi-az"))) + assert.NotEqual(t, key(rds("single-az")), key(rds(""))) + assert.Equal(t, key(rds("multi-az")), key(rds("multi-az"))) +} diff --git a/internal/scheduler/recommendation_identity_test.go b/internal/scheduler/recommendation_identity_test.go index c3d050d6f..8d98e686f 100644 --- a/internal/scheduler/recommendation_identity_test.go +++ b/internal/scheduler/recommendation_identity_test.go @@ -188,3 +188,37 @@ func TestRecommendationID_MalformedDetailsErrorsAndTagAccountDrops(t *testing.T) s := &Scheduler{} assert.Empty(t, s.tagAccount([]config.RecommendationRecord{bad}, "acct")) } + +func rdsAZConfigRec(azConfig string) common.Recommendation { + return common.Recommendation{ + Provider: common.ProviderAWS, Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.r5.large", Count: 1, Term: "1yr", PaymentOption: "all-upfront", + Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: azConfig, InstanceClass: "db.r5.large"}, + } +} + +// Two RDS recs for the same class, engine and term that differ only by +// Single-AZ vs Multi-AZ must get distinct IDs (issue #408); before, both got +// "aws||rds|us-east-1|db.r5.large|mysql|1|all-upfront". A database rec without +// az_config keeps its pre-#408 ID. +func TestConvertRecommendations_RDSAZConfigInID(t *testing.T) { + s := &Scheduler{} + records := s.convertRecommendations([]common.Recommendation{rdsAZConfigRec("single-az"), rdsAZConfigRec("multi-az"), rdsAZConfigRec("")}, "aws") + require.Len(t, records, 3) + assert.Equal(t, "aws||rds|us-east-1|db.r5.large|mysql|1|all-upfront|single-az", records[0].ID) + assert.Equal(t, "aws||rds|us-east-1|db.r5.large|mysql|1|all-upfront|multi-az", records[1].ID) + assert.Equal(t, "aws||rds|us-east-1|db.r5.large|mysql|1|all-upfront", records[2].ID) + + tagged := s.tagAccount(records[1:2], "acct") + require.Len(t, tagged, 1) + assert.Equal(t, "aws|acct|rds|us-east-1|db.r5.large|mysql|1|all-upfront|multi-az", tagged[0].ID) +} + +// An unreadable az_config is dropped loudly, never emitted with an ID that +// collides with its sibling. +func TestRecommendationID_MalformedAZConfigErrors(t *testing.T) { + bad := config.RecommendationRecord{Provider: "aws", Service: "rds", Details: []byte(`{"az_config":5}`)} + _, err := recommendationID(bad) + require.Error(t, err) + s := &Scheduler{} + assert.Empty(t, s.tagAccount([]config.RecommendationRecord{bad}, "acct")) +} diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 1d6953042..59e38cd39 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1150,6 +1150,10 @@ func recommendationID(rec config.RecommendationRecord) (string, error) { if err != nil { return "", err } + azConfig, err := rec.DetailsAZConfig() + if err != nil { + return "", err + } accountID := "" if rec.CloudAccountID != nil { accountID = *rec.CloudAccountID @@ -1169,6 +1173,13 @@ func recommendationID(rec config.RecommendationRecord) (string, error) { // spaces. id += "|" + ci.Platform + "|" + ci.Tenancy } + if azConfig != "" { + // Appended only for database recs whose Details carry it (RDS + // single-az/multi-az, Azure SQL zone redundancy), so every other rec + // keeps its ID (issue #408). Those recs never carry an AZ or a + // platform/tenancy pair, and the service field already differs. + id += "|" + azConfig + } return id, nil } From 913376f68000375146599f5caaa22cbe244673e8 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Thu, 22 Oct 2026 01:42:41 +0100 Subject: [PATCH 03/19] fix(config): upsert recommendations on the 12-column index with az_config (#408) Requires migration 000140. Co-Authored-By: Claude Opus 5.5 --- internal/config/store_postgres_recommendations.go | 10 ++++++---- internal/purchase/azure_pricing_integration_test.go | 6 +++--- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index d95877c47..cee71cf53 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -61,7 +61,8 @@ func (s *PostgresStore) ReplaceRecommendations(ctx context.Context, collectedAt // UpsertRecommendations is the incremental write path: it upserts each row // by natural key (cloud_account_id, provider, service, region, -// resource_type, engine, term, payment_option, availability_zone, platform, tenancy) and then evicts stale rows for the +// resource_type, engine, term, payment_option, availability_zone, platform, +// tenancy, az_config) and then evicts stale rows for the // set of (provider, account) pairs that successfully collected in this run. // Pairs whose collection failed are NOT in successfulCollects and their // stale rows stay — callers see older data with a banner rather than a @@ -71,8 +72,9 @@ func (s *PostgresStore) ReplaceRecommendations(ctx context.Context, collectedAt // so per-rec ON CONFLICT no longer collides on SQLSTATE 21000. Migration // 000109 added availability_zone, a GENERATED column derived from payload that // is never written here. Migration 000111 added the platform and tenancy -// generated columns the same way; the upsert arbiter is the 11-column -// recommendations_natural_key_az_tp_idx (issues #792, #806). +// generated columns the same way, and migration 000140 the az_config one; the +// upsert arbiter is the 12-column recommendations_natural_key_az_tp_azc_idx +// (issues #792, #806, #408). // // The eviction predicate uses (provider, account_key) IN (unnest($2, $3)) // where account_key matches the generated column on the table — nil @@ -226,7 +228,7 @@ func insertRecommendationsBatch(ctx context.Context, tx pgx.Tx, collectedAt time if onConflict { query += ` - ON CONFLICT (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy) + ON CONFLICT (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, az_config) DO UPDATE SET payload = EXCLUDED.payload, upfront_cost = EXCLUDED.upfront_cost, diff --git a/internal/purchase/azure_pricing_integration_test.go b/internal/purchase/azure_pricing_integration_test.go index 0fe2d4dcf..a734724cf 100644 --- a/internal/purchase/azure_pricing_integration_test.go +++ b/internal/purchase/azure_pricing_integration_test.go @@ -102,10 +102,10 @@ func TestApproveAzurePricingRequiresMigrationAndRecollection(t *testing.T) { require.NoError(t, migrations.MigrateToVersion(ctx, pool, rampMigrationsPath(), 106)) t.Run("106 invalidated old prices", func(t *testing.T) { approve(t, ErrStaleAzurePricing, 0) }) rec.UpfrontCost = 700 - // The store's upsert needs the availability_zone, platform and tenancy columns and the - // 11-column index from migration 000111, and resets + // The store's upsert needs the availability_zone, platform, tenancy and az_config columns + // and the 12-column index from migration 000140 (issue #408), and resets // recommendations_state.last_collection_partial, added by migration 000129 (issue #87). - require.NoError(t, migrations.MigrateToVersion(ctx, pool, rampMigrationsPath(), 129)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, rampMigrationsPath(), 140)) require.NoError(t, pricing.UpsertRecommendations(ctx, time.Now().Add(time.Minute), []config.RecommendationRecord{rec}, collects)) t.Run("recollected prices purchase once", func(t *testing.T) { approve(t, nil, 1) }) } From a035c5d52b70d2b59d77adc15af48f1b6ab23806 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Thu, 22 Oct 2026 01:42:41 +0100 Subject: [PATCH 04/19] fix(db): drop the recommendations indexes without az_config (#408) Migration 000141 drops the 11-column natural key and the 000120 source index, both superseded by the 000140 indexes that add az_config. While either exists, a collection batch carrying a Single-AZ and a Multi-AZ RDS recommendation of the same class fails as a whole. The rollback collapses az_config-only duplicates to the newest row and recreates both. Apply only after the 12-column upsert is live everywhere. Co-Authored-By: Claude Opus 5.5 --- ..._postgres_recommendations_azconfig_test.go | 159 ++++++++++++++++++ ...recommendations_11col_natural_key.down.sql | 28 +++ ...p_recommendations_11col_natural_key.up.sql | 26 +++ ..._recommendations_11col_natural_key_test.go | 106 ++++++++++++ 4 files changed, 319 insertions(+) create mode 100644 internal/config/store_postgres_recommendations_azconfig_test.go create mode 100644 internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key.down.sql create mode 100644 internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key.up.sql create mode 100644 internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key_test.go diff --git a/internal/config/store_postgres_recommendations_azconfig_test.go b/internal/config/store_postgres_recommendations_azconfig_test.go new file mode 100644 index 000000000..8eb61c822 --- /dev/null +++ b/internal/config/store_postgres_recommendations_azconfig_test.go @@ -0,0 +1,159 @@ +//go:build integration +// +build integration + +package config_test + +import ( + "context" + "encoding/json" + "sync" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func rdsAZConfigStoreRec(azConfig string, savings float64) config.RecommendationRecord { + rec := awsRec("rds-"+azConfig, "rds", "us-east-1", "db.r5.large", savings) + rec.Engine = "mysql" + rec.Details = json.RawMessage(`{"engine":"mysql","az_config":"` + azConfig + `","instance_class":"db.r5.large"}`) + return rec +} + +// Issue #408: Cost Explorer reports Single-AZ and Multi-AZ RDS reservations of +// the same class, engine, term and payment as separate recommendations. Once +// the store upserts on the 12-column index (with the generated az_config +// column) and migration 000141 drops the 11-column index, both rows upsert in +// ONE batch (the shape persistCollection writes) and both persist. Before, +// the batch failed with SQLSTATE 21000 ("ON CONFLICT DO UPDATE command cannot +// affect row a second time") and took the whole collection write down. +func TestPostgresStore_UpsertRecommendations_AZConfigBatch(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequireMigratedDatabase(ctx, t, getMigrationsPath()) + pool := container.DB.Pool() + store := config.NewPostgresStore(container.DB) + t0 := time.Now().UTC().Truncate(time.Second) + collect := []config.SuccessfulCollect{{Provider: "aws"}} + + var oldIdx bool + require.NoError(t, pool.QueryRow(ctx, `SELECT EXISTS (SELECT 1 FROM pg_indexes WHERE indexname = 'recommendations_natural_key_az_tp_idx')`).Scan(&oldIdx)) + require.False(t, oldIdx, "11-column index must be gone after the full migration chain (000141)") + + savingsByAZConfig := func(t *testing.T) map[string]float64 { + t.Helper() + rows, err := pool.Query(ctx, `SELECT az_config, monthly_savings FROM recommendations`) + require.NoError(t, err) + defer rows.Close() + out := map[string]float64{} + for rows.Next() { + var k string + var v float64 + require.NoError(t, rows.Scan(&k, &v)) + _, dup := out[k] + require.False(t, dup, "two stored rows share az_config %q", k) + out[k] = v + } + require.NoError(t, rows.Err()) + return out + } + reset := func(t *testing.T) { + t.Helper() + _, err := pool.Exec(ctx, `DELETE FROM recommendations`) + require.NoError(t, err) + } + + t.Run("single-az and multi-az in one batch both persist", func(t *testing.T) { + reset(t) + require.NoError(t, store.UpsertRecommendations(ctx, t0, + []config.RecommendationRecord{rdsAZConfigStoreRec("single-az", 10), rdsAZConfigStoreRec("multi-az", 20)}, collect)) + assert.Equal(t, map[string]float64{"single-az": 10, "multi-az": 20}, savingsByAZConfig(t)) + }) + + t.Run("same az_config re-upsert updates in place and leaves the sibling alone", func(t *testing.T) { + require.NoError(t, store.UpsertRecommendations(ctx, t0.Add(time.Hour), + []config.RecommendationRecord{rdsAZConfigStoreRec("single-az", 11), rdsAZConfigStoreRec("multi-az", 20)}, collect)) + assert.Equal(t, map[string]float64{"single-az": 11, "multi-az": 20}, savingsByAZConfig(t)) + }) + + t.Run("cross-run single-az then multi-az both persist", func(t *testing.T) { + reset(t) + require.NoError(t, store.UpsertRecommendations(ctx, t0, []config.RecommendationRecord{rdsAZConfigStoreRec("single-az", 10)}, collect)) + require.NoError(t, store.UpsertRecommendations(ctx, t0.Add(time.Hour), + []config.RecommendationRecord{rdsAZConfigStoreRec("single-az", 10), rdsAZConfigStoreRec("multi-az", 20)}, collect)) + assert.Equal(t, map[string]float64{"single-az": 10, "multi-az": 20}, savingsByAZConfig(t)) + }) + + t.Run("non-database rows alongside are unaffected", func(t *testing.T) { + reset(t) + ec2 := awsRec("e1", "ec2", "us-east-1", "m5.large", 30) + batch := []config.RecommendationRecord{rdsAZConfigStoreRec("single-az", 10), rdsAZConfigStoreRec("multi-az", 20), ec2} + require.NoError(t, store.UpsertRecommendations(ctx, t0, batch, collect)) + assert.Equal(t, map[string]float64{"single-az": 10, "multi-az": 20, "": 30}, savingsByAZConfig(t)) + }) + + t.Run("row written before the switch is found and updated", func(t *testing.T) { + reset(t) + // What the pre-#408 writer stored: az_config only inside details. The + // generated column picks it up, so the new arbiter matches it. + _, err := pool.Exec(ctx, ` + INSERT INTO recommendations + (collected_at, provider, service, region, resource_type, engine, payload, + upfront_cost, monthly_savings, term, payment_option) + VALUES ($1, 'aws', 'rds', 'us-east-1', 'db.r5.large', 'mysql', + '{"details":{"engine":"mysql","az_config":"multi-az"}}'::jsonb, 100, 5, 12, 'no-upfront')`, t0) + require.NoError(t, err) + require.NoError(t, store.UpsertRecommendations(ctx, t0.Add(time.Hour), []config.RecommendationRecord{rdsAZConfigStoreRec("multi-az", 40)}, collect)) + assert.Equal(t, map[string]float64{"multi-az": 40}, savingsByAZConfig(t), "the legacy row must be the conflict target, not duplicated") + }) +} + +// Concurrent collections (overlapping scheduled runs, or a manual refresh +// racing the cron) re-upsert an already stored Single-AZ/Multi-AZ pair at +// once. Every writer must succeed and exactly one row per az_config must +// remain: the 12-column arbiter routes each writer to its own variant's row. +// +// The rows are seeded first on purpose. Two writers inserting the same NEW +// row at the same instant can still fail with 23505 on the non-arbiter +// source-account unique index; that race predates #408 (migration 000120's +// recommendations_natural_key_source_idx on main has it too) and is out of +// this test's scope. +func TestPostgresStore_UpsertRecommendations_AZConfigConcurrent(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequireMigratedDatabase(ctx, t, getMigrationsPath()) + pool := container.DB.Pool() + store := config.NewPostgresStore(container.DB) + t0 := time.Now().UTC().Truncate(time.Second) + collect := []config.SuccessfulCollect{{Provider: "aws"}} + require.NoError(t, store.UpsertRecommendations(ctx, t0, + []config.RecommendationRecord{rdsAZConfigStoreRec("single-az", 1), rdsAZConfigStoreRec("multi-az", 2)}, collect)) + + const writers = 8 + var wg sync.WaitGroup + errs := make([]error, writers) + start := make(chan struct{}) + for i := 0; i < writers; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + <-start + batch := []config.RecommendationRecord{ + rdsAZConfigStoreRec("single-az", float64(10+i)), + rdsAZConfigStoreRec("multi-az", float64(20+i)), + } + errs[i] = store.UpsertRecommendations(ctx, t0.Add(time.Duration(i+1)*time.Second), batch, collect) + }(i) + } + close(start) + wg.Wait() + for i, err := range errs { + require.NoError(t, err, "writer %d", i) + } + + var n, distinct int + require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*), COUNT(DISTINCT az_config) FROM recommendations`).Scan(&n, &distinct)) + assert.Equal(t, 2, n) + assert.Equal(t, 2, distinct) +} diff --git a/internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key.down.sql b/internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key.down.sql new file mode 100644 index 000000000..a5ab50e36 --- /dev/null +++ b/internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key.down.sql @@ -0,0 +1,28 @@ +-- Rollback of 000141: recreate the 11-column unique index exactly as defined +-- by 000111 and the source-account index exactly as defined by 000120. +-- +-- Only safe together with a code rollback to the 11-column upsert. The newer +-- code needs the 12-column arbiter from 000140, which this rollback keeps. The +-- recommendations table is a regenerable cache, so rows that differ only by +-- az_config (allowed while the 11-column index is absent) are collapsed to the +-- most recently collected one before the indexes are rebuilt; the next +-- collection cycle repopulates the rest. Only duplicates under the 11-column +-- key are deleted, which also clears every duplicate under the finer 000120 +-- source key. +DELETE FROM recommendations +WHERE id IN ( + SELECT id FROM ( + SELECT id, ROW_NUMBER() OVER ( + PARTITION BY account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy + ORDER BY collected_at DESC, id DESC + ) AS rn + FROM recommendations + ) ranked + WHERE rn > 1 +); + +CREATE UNIQUE INDEX IF NOT EXISTS recommendations_natural_key_az_tp_idx + ON recommendations (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy); + +CREATE UNIQUE INDEX IF NOT EXISTS recommendations_natural_key_source_idx + ON recommendations (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, source_account_key); diff --git a/internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key.up.sql b/internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key.up.sql new file mode 100644 index 000000000..71422a1fb --- /dev/null +++ b/internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key.up.sql @@ -0,0 +1,26 @@ +-- Migration 000141: drop the 11-column recommendations_natural_key_az_tp_idx +-- and the 12-column recommendations_natural_key_source_idx (#408, contract step +-- of the RDS AZ-config identity work). +-- +-- Migration 000140 added the generated az_config column, the 12-column +-- recommendations_natural_key_az_tp_azc_idx and the 13-column +-- recommendations_natural_key_source_azc_idx, and kept the indexes without +-- az_config so the previous upsert (ON CONFLICT on the 11 columns) kept +-- working. The store now upserts on the 12-column index, so both indexes +-- without az_config only do harm: they reject a Single-AZ and a Multi-AZ RDS +-- row of the same class, engine, term and payment, which aborts the whole +-- collection write. Dropping them lets those rows coexist. The 000120 source +-- index is superseded by recommendations_natural_key_source_azc_idx (the same +-- key plus az_config), so the source-account work of #799 keeps a +-- source-aware unique key. +-- +-- DEPLOY ORDER: apply this only after the code that upserts on the 12-column +-- index is live everywhere. Applying it while older code still runs breaks +-- every collection, because ON CONFLICT on the 11 columns needs that index. +-- database-migration.yml applies ALL pending migrations when `steps` is 0 or +-- empty, so never dispatch it with the default before that code is released. +-- +-- Each replacement index is strictly finer than the one it supersedes, so no +-- existing row can violate it and nothing needs backfilling. +DROP INDEX IF EXISTS recommendations_natural_key_az_tp_idx; +DROP INDEX IF EXISTS recommendations_natural_key_source_idx; diff --git a/internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key_test.go b/internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key_test.go new file mode 100644 index 000000000..20aa2dce5 --- /dev/null +++ b/internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key_test.go @@ -0,0 +1,106 @@ +//go:build integration +// +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/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const insertRecForMigration141 = ` + INSERT INTO recommendations + (collected_at, provider, service, region, resource_type, engine, payload, + upfront_cost, monthly_savings, term, payment_option) + VALUES ($1::timestamptz, 'aws', 'rds', $2, 'db.r5.large', 'mysql', $3::jsonb, 0, 0, 1, 'all-upfront') +` + +// TestMigration_DropRecommendations11ColNaturalKey locks down migration 000141 +// (issue #408, contract step): Single-AZ and Multi-AZ rows of the same class +// coexist once the 11-column and 000120 source indexes are gone, the 12-column index still dedupes, +// and the rollback recreates both indexes after collapsing those +// duplicates to the newest row. +func TestMigration_DropRecommendations11ColNaturalKey(t *testing.T) { + ctx := context.Background() + migrationsPath := getMigrationsPath() + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + indexExists := func(name string) bool { + var exists bool + require.NoError(t, pool.QueryRow(ctx, + `SELECT EXISTS (SELECT 1 FROM pg_indexes WHERE tablename = 'recommendations' AND indexname = $1)`, name).Scan(&exists)) + return exists + } + count := func(region string) int { + var n int + require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*) FROM recommendations WHERE region = $1`, region).Scan(&n)) + return n + } + const ( + single = `{"details":{"engine":"mysql","az_config":"single-az"}}` + multi = `{"details":{"engine":"mysql","az_config":"multi-az"}}` + none = `{"details":{"engine":"mysql"}}` + ) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 141))) + require.True(t, indexExists("recommendations_natural_key_az_tp_idx")) + require.True(t, indexExists("recommendations_natural_key_az_tp_azc_idx")) + require.True(t, indexExists("recommendations_natural_key_source_idx")) + require.True(t, indexExists("recommendations_natural_key_source_azc_idx")) + + _, err = pool.Exec(ctx, insertRecForMigration141, "2026-01-01T00:00:00Z", "us-east-1", single) + require.NoError(t, err) + _, err = pool.Exec(ctx, insertRecForMigration141, "2026-01-01T00:00:00Z", "us-west-2", single) + require.NoError(t, err) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 141)) + assert.False(t, indexExists("recommendations_natural_key_az_tp_idx"), "11-column index must be dropped") + assert.True(t, indexExists("recommendations_natural_key_az_tp_azc_idx"), "12-column index must remain") + assert.False(t, indexExists("recommendations_natural_key_source_idx"), "000120's source index (no az_config) must be dropped") + assert.True(t, indexExists("recommendations_natural_key_source_azc_idx"), "the source-aware key with az_config must remain") + + // Rows differing only by az_config now coexist; an exact duplicate is + // still rejected by the 12-column index. + _, err = pool.Exec(ctx, insertRecForMigration141, "2026-01-02T00:00:00Z", "us-east-1", multi) + require.NoError(t, err) + _, err = pool.Exec(ctx, insertRecForMigration141, "2026-01-03T00:00:00Z", "us-east-1", none) + require.NoError(t, err) + assert.Equal(t, 3, count("us-east-1")) + _, err = pool.Exec(ctx, insertRecForMigration141, "2026-01-04T00:00:00Z", "us-east-1", single) + require.Error(t, err, "the same az_config must still violate the 12-column index") + + // Rollback: az_config-only duplicates collapse to the newest, the other + // region's row is untouched, and the 11-column index is recreated with + // its original shape. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 141))) + assert.Equal(t, 1, count("us-east-1"), "only duplicates under the 11-column key may be deleted") + assert.Equal(t, 1, count("us-west-2")) + var kept string + require.NoError(t, pool.QueryRow(ctx, `SELECT az_config FROM recommendations WHERE region = 'us-east-1'`).Scan(&kept)) + assert.Equal(t, "", kept, "the most recently collected row survives") + + var indexDef string + require.NoError(t, pool.QueryRow(ctx, + `SELECT indexdef FROM pg_indexes WHERE indexname = 'recommendations_natural_key_az_tp_idx'`).Scan(&indexDef)) + assert.Contains(t, indexDef, "UNIQUE INDEX") + assert.Contains(t, indexDef, "(account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy)") + assert.True(t, indexExists("recommendations_natural_key_az_tp_azc_idx")) + require.NoError(t, pool.QueryRow(ctx, + `SELECT indexdef FROM pg_indexes WHERE indexname = 'recommendations_natural_key_source_idx'`).Scan(&indexDef)) + assert.Contains(t, indexDef, "UNIQUE INDEX") + assert.Contains(t, indexDef, "(account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, source_account_key)") + + // Re-applying the up migration after a rollback works. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 141)) + assert.False(t, indexExists("recommendations_natural_key_az_tp_idx")) + assert.False(t, indexExists("recommendations_natural_key_source_idx")) +} From de5e3125f8f2cb31e5d0999c0aeba58018c5d65e Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Wed, 28 Oct 2026 00:19:39 +0000 Subject: [PATCH 05/19] fix(deps): bump golang.org/x/net to v0.62.0 (GO-2026-6860, GO-2026-6866) govulncheck in the Docker image build now fails on these two advisories, both fixed in x/net v0.62.0. Co-Authored-By: Claude Opus 5.5 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 5fb3a2f61..abcf4fb6c 100644 --- a/go.mod +++ b/go.mod @@ -59,7 +59,7 @@ require ( go.opentelemetry.io/otel/metric v1.45.0 // indirect go.opentelemetry.io/otel/trace v1.45.0 // indirect golang.org/x/crypto v0.58.0 - golang.org/x/net v0.61.0 // indirect + golang.org/x/net v0.62.0 // indirect golang.org/x/oauth2 v0.37.0 golang.org/x/sync v0.24.0 golang.org/x/sys v0.49.0 // indirect diff --git a/go.sum b/go.sum index 06deac0b9..83df52e66 100644 --- a/go.sum +++ b/go.sum @@ -367,8 +367,8 @@ golang.org/x/crypto v0.58.0 h1:COkYLr4k7nDI4r6QtMML2AfzeIswWHAJthLS6K/M/54= golang.org/x/crypto v0.58.0/go.mod h1:Xh+kl5A+M0gMiWDytH2Hhr5Z7WbNCpv/eB/EFgfSk+w= golang.org/x/mod v0.42.0 h1:ICeL3t2L30ljSPHHxDnR4fqVcBt/zR2zJAb9juavmQg= golang.org/x/mod v0.42.0/go.mod h1:u5fzIrRInkL8MaO1cbRkUHT64v303YIT5KEqpNr7jRs= -golang.org/x/net v0.61.0 h1:RR+6j/BTBCrggOLugxa+66R8FvoN6MHlzDlZIbN9YFw= -golang.org/x/net v0.61.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI= +golang.org/x/net v0.62.0 h1:18ELalfM5+XNRwzM2nYqXsZQ9ByS1uhj2O/U7+5jqRY= +golang.org/x/net v0.62.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI= golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98= golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58= golang.org/x/sync v0.24.0 h1:iqiDrGoDgyuvAKrhLQthUEzU/R378N78gXbBjxed7oA= From 002d123b6ccf5af398993175f63a2e1bfe88ecaa Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 3 Nov 2026 19:35:05 +0100 Subject: [PATCH 06/19] fix(recommendations): scope az_config identity to AWS RDS (#408) Azure SQL's az_config is a subscription-wide dominant value that is empty when the server lookup is incomplete, so keying the rec ID, identity key and purchase fingerprint on it flipped Azure SQL IDs between collections for no benefit. Add IdentityAZConfig, which returns the Details az_config for AWS RDS and "" for every other rec, and use it in those three places. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_purchases.go | 2 +- internal/api/purchase_azconfig_test.go | 10 ++++++++ internal/config/recommendation_identity.go | 24 ++++++++++++++++++- .../config/recommendation_identity_test.go | 23 +++++++++++++++++- .../scheduler/recommendation_identity_test.go | 19 +++++++++++++++ internal/scheduler/scheduler.go | 10 ++++---- 6 files changed, 80 insertions(+), 8 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index afeadc671..192282073 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -3640,7 +3640,7 @@ func purchaseIdempotencyKey(creatorID string, recs []config.RecommendationRecord } // Single-AZ and Multi-AZ RDS buys of the same class are different // purchases and must not dedupe into one (issue #408). - azConfig, err := r.DetailsAZConfig() + azConfig, err := r.IdentityAZConfig() if err != nil { return "", err } diff --git a/internal/api/purchase_azconfig_test.go b/internal/api/purchase_azconfig_test.go index b1572a18e..b676800e8 100644 --- a/internal/api/purchase_azconfig_test.go +++ b/internal/api/purchase_azconfig_test.go @@ -86,3 +86,13 @@ func TestDescribeRec_IncludesAZConfig(t *testing.T) { bare := config.RecommendationRecord{Provider: "aws", Service: "rds"} assert.NotContains(t, describeRec(&bare), "az_config=") } + +func TestPurchaseIdempotencyKey_IgnoresAZConfigForAzureSQL(t *testing.T) { + t.Parallel() + key := func(azConfig string) string { + r := rdsAZConfigRec(azConfig, 1) + r.Provider, r.Service = "azure", "database" + return mustPurchaseIdempotencyKey("u", []config.RecommendationRecord{r}, 100) + } + assert.Equal(t, key(""), key("zoneRedundant")) +} diff --git a/internal/config/recommendation_identity.go b/internal/config/recommendation_identity.go index 9789b4bd7..f9c809fda 100644 --- a/internal/config/recommendation_identity.go +++ b/internal/config/recommendation_identity.go @@ -5,6 +5,8 @@ import ( "fmt" "strconv" "strings" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" ) // ComputeIdentity is the EC2 platform and tenancy of a recommendation, the two @@ -63,6 +65,26 @@ func (r *RecommendationRecord) DetailsAZConfig() (string, error) { return d.AZConfig, nil } +// isAWSRDS is the scope of the az_config identity component: only AWS RDS has +// Single-AZ and Multi-AZ offerings of one class that Cost Explorer returns as +// separate recs. Azure SQL's az_config is a subscription-wide "dominant" value +// that is "" when the server lookup is incomplete, so keying on it would flip +// IDs between collections with no benefit (issue #408). +func isAWSRDS(provider, service string) bool { + p := strings.ToLower(provider) + return p == "aws" && CanonicalSuppressionService(p, service) == string(common.ServiceRelationalDB) +} + +// IdentityAZConfig is the az_config component of the rec ID, identity key, +// purchase fingerprint and suppression variant: DetailsAZConfig for AWS RDS, +// "" for every other rec. +func (r *RecommendationRecord) IdentityAZConfig() (string, error) { + if !isAWSRDS(r.Provider, r.Service) { + return "", nil + } + return r.DetailsAZConfig() +} + // RecIdentityKey is the tuple a price is a function of: the same twelve // components (the ninth is the zonal EC2 availability zone, "" for every // other rec, issue #792; the tenth and eleventh are the EC2 platform and @@ -77,7 +99,7 @@ func RecIdentityKey(rec *RecommendationRecord) (string, error) { if err != nil { return "", err } - azConfig, err := rec.DetailsAZConfig() + azConfig, err := rec.IdentityAZConfig() if err != nil { return "", err } diff --git a/internal/config/recommendation_identity_test.go b/internal/config/recommendation_identity_test.go index b5d1d18f8..cae908aa3 100644 --- a/internal/config/recommendation_identity_test.go +++ b/internal/config/recommendation_identity_test.go @@ -76,7 +76,7 @@ func TestDetailsAZConfig_Vectors(t *testing.T) { } func TestDetailsAZConfig_MalformedFailsLoud(t *testing.T) { - bad := RecommendationRecord{Details: json.RawMessage(`{"az_config":5}`)} + bad := RecommendationRecord{Provider: "aws", Service: "rds", Details: json.RawMessage(`{"az_config":5}`)} _, err := bad.DetailsAZConfig() require.Error(t, err) _, err = RecIdentityKey(&bad) @@ -101,3 +101,24 @@ func TestRecIdentityKey_DiffersByAZConfig(t *testing.T) { assert.NotEqual(t, key(rds("single-az")), key(rds(""))) assert.Equal(t, key(rds("multi-az")), key(rds("multi-az"))) } + +// Azure SQL's az_config is a subscription-wide value that is "" when the +// server lookup is incomplete; it must not enter the identity key, or the key +// would flip between collections (issue #408). +func TestRecIdentityKey_IgnoresAZConfigOutsideAWSRDS(t *testing.T) { + rec := func(provider, service, azConfig string) *RecommendationRecord { + return &RecommendationRecord{ + Provider: provider, Service: service, Region: "eastus", ResourceType: "GP_Gen5_2", Engine: "sqlserver", + Term: 1, Payment: "upfront", Details: json.RawMessage(`{"engine":"sqlserver","az_config":"` + azConfig + `"}`), + } + } + key := func(r *RecommendationRecord) string { + k, err := RecIdentityKey(r) + require.NoError(t, err) + return k + } + assert.Equal(t, key(rec("azure", "database", "")), key(rec("azure", "database", "zoneRedundant"))) + assert.Equal(t, key(rec("azure", "relational-db", "none")), key(rec("azure", "relational-db", ""))) + assert.NotEqual(t, key(rec("aws", "relational-db", "single-az")), key(rec("aws", "relational-db", "multi-az"))) + assert.NotEqual(t, key(rec("aws", "rds", "single-az")), key(rec("aws", "rds", "multi-az"))) +} diff --git a/internal/scheduler/recommendation_identity_test.go b/internal/scheduler/recommendation_identity_test.go index 8d98e686f..f4e1ba810 100644 --- a/internal/scheduler/recommendation_identity_test.go +++ b/internal/scheduler/recommendation_identity_test.go @@ -222,3 +222,22 @@ func TestRecommendationID_MalformedAZConfigErrors(t *testing.T) { s := &Scheduler{} assert.Empty(t, s.tagAccount([]config.RecommendationRecord{bad}, "acct")) } + +// An Azure SQL rec's ID is stable when its subscription-wide az_config flips +// between "" (incomplete server lookup) and a value (issue #408). +func TestRecommendationID_AzureSQLStableAcrossAZConfig(t *testing.T) { + azure := func(azConfig string) config.RecommendationRecord { + return config.RecommendationRecord{ + Provider: "azure", Service: "database", Region: "eastus", ResourceType: "GP_Gen5_2", + Engine: "sqlserver", Term: 1, Payment: "upfront", + Details: []byte(`{"engine":"sqlserver","az_config":"` + azConfig + `"}`), + } + } + empty, err := recommendationID(azure("")) + require.NoError(t, err) + for _, v := range []string{"zoneRedundant", "none"} { + got, err := recommendationID(azure(v)) + require.NoError(t, err) + assert.Equal(t, empty, got, v) + } +} diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 0e269c475..ad47edf5a 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1161,7 +1161,7 @@ func recommendationID(rec config.RecommendationRecord) (string, error) { if err != nil { return "", err } - azConfig, err := rec.DetailsAZConfig() + azConfig, err := rec.IdentityAZConfig() if err != nil { return "", err } @@ -1185,10 +1185,10 @@ func recommendationID(rec config.RecommendationRecord) (string, error) { id += "|" + ci.Platform + "|" + ci.Tenancy } if azConfig != "" { - // Appended only for database recs whose Details carry it (RDS - // single-az/multi-az, Azure SQL zone redundancy), so every other rec - // keeps its ID (issue #408). Those recs never carry an AZ or a - // platform/tenancy pair, and the service field already differs. + // Appended only for AWS RDS recs whose Details carry it + // (single-az/multi-az), so every other rec keeps its ID (issue #408). + // Azure SQL is excluded: its az_config is a subscription-wide value + // that is "" on an incomplete server lookup, so it would only flip IDs. id += "|" + azConfig } return id, nil From cad5d3fd912b713ba32d61f37966215f8f2b0335 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 3 Nov 2026 19:46:42 +0100 Subject: [PATCH 07/19] feat(db): add az_config to purchase_suppressions variant (#408) Add a nullable az_config column and widen the per-execution UNIQUE key so a Single-AZ and a Multi-AZ RDS suppression of one execution can coexist. Backfill rule: NULL az_config means unknown or not applicable and is a wildcard, so every existing row keeps suppressing both offerings, failing safe against a double purchase. Down merges rows that differ only by az_config (summed count, latest expiry) before restoring the 000134 key. Apply as its own deploy step, before the code that writes the column. Co-Authored-By: Claude Opus 5.5 --- ...6_purchase_suppressions_az_config.down.sql | 52 ++++++++++ ...156_purchase_suppressions_az_config.up.sql | 41 ++++++++ ...56_purchase_suppressions_az_config_test.go | 99 +++++++++++++++++++ 3 files changed, 192 insertions(+) create mode 100644 internal/database/postgres/migrations/000156_purchase_suppressions_az_config.down.sql create mode 100644 internal/database/postgres/migrations/000156_purchase_suppressions_az_config.up.sql create mode 100644 internal/database/postgres/migrations/000156_purchase_suppressions_az_config_test.go diff --git a/internal/database/postgres/migrations/000156_purchase_suppressions_az_config.down.sql b/internal/database/postgres/migrations/000156_purchase_suppressions_az_config.down.sql new file mode 100644 index 000000000..106655e80 --- /dev/null +++ b/internal/database/postgres/migrations/000156_purchase_suppressions_az_config.down.sql @@ -0,0 +1,52 @@ +-- Rollback of 000156: restore the 000134 suppression key without az_config. +-- +-- Rows of one execution that differ only by az_config are merged into one row +-- per execution, 6-tuple and variant before the narrower UNIQUE constraint is +-- recreated. The merged row keeps the summed count and the latest expiry, so +-- the rollback never suppresses less than before; it over-suppresses the +-- sibling offering, which is the pre-#408 behavior old code expects. +WITH merged AS ( + SELECT execution_id, account_id, provider, service, region, resource_type, engine, + availability_zone, platform, tenancy, + 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, + availability_zone, platform, tenancy + 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.availability_zone IS NOT DISTINCT FROM m.availability_zone + AND s.platform IS NOT DISTINCT FROM m.platform + AND s.tenancy IS NOT DISTINCT FROM m.tenancy + 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_azc_key; + +ALTER TABLE purchase_suppressions + DROP CONSTRAINT IF EXISTS purchase_suppressions_az_config_needs_variant; + +ALTER TABLE purchase_suppressions + DROP COLUMN IF EXISTS az_config; + +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/000156_purchase_suppressions_az_config.up.sql b/internal/database/postgres/migrations/000156_purchase_suppressions_az_config.up.sql new file mode 100644 index 000000000..12778d0b1 --- /dev/null +++ b/internal/database/postgres/migrations/000156_purchase_suppressions_az_config.up.sql @@ -0,0 +1,41 @@ +-- Migration 000156 (issue #408): widen the purchase suppression variant with +-- the AWS RDS az_config ("single-az" / "multi-az"). +-- +-- Migration 000155 lets a Single-AZ and a Multi-AZ RDS rec of the same class, +-- engine, term and payment coexist, but the suppression key (000134) cannot +-- tell them apart: buying 2x Single-AZ would write a suppression that also +-- matched, and hid, the Multi-AZ rec. The variant now carries az_config for +-- AWS RDS recs only; every other provider/service writes NULL. +-- +-- Backfill rule (none needed): az_config NULL means "unknown or not +-- applicable" and is a WILDCARD over the row's other variant columns, so every +-- row written before this migration keeps suppressing exactly what it +-- suppressed before (both the Single-AZ and the Multi-AZ rec). That fails safe +-- against a double purchase; the over-suppression ends when the row expires. +-- Rows are deliberately not rewritten: their original az_config is unknowable. +-- +-- Expand step, compatible with code that predates this change: the column is +-- nullable with no default (catalog-only, no table rewrite) and old code's +-- inserts leave it NULL. The UNIQUE key gains the column (NULLS NOT DISTINCT, +-- so NULL rows stay unique per execution, 6-tuple and variant as before). +-- A suppression with an az_config must also carry the rest of the variant. +-- +-- DEPLOY ORDER: apply this before deploying the code that writes az_config +-- (the insert names the column). Apply 000154, 000155 and this migration as +-- separate steps, each after the code it needs is live (see the 000155 header); +-- never dispatch database-migration.yml with the default of all pending steps. +ALTER TABLE purchase_suppressions + ADD COLUMN IF NOT EXISTS az_config TEXT; + +ALTER TABLE purchase_suppressions + ADD CONSTRAINT purchase_suppressions_az_config_needs_variant + CHECK (az_config IS NULL OR availability_zone IS NOT NULL); + +ALTER TABLE purchase_suppressions + DROP CONSTRAINT IF EXISTS purchase_suppressions_execution_variant_key; + +ALTER TABLE purchase_suppressions + ADD CONSTRAINT purchase_suppressions_execution_variant_azc_key + UNIQUE NULLS NOT DISTINCT (execution_id, account_id, provider, service, region, + resource_type, engine, availability_zone, platform, tenancy, + az_config); diff --git a/internal/database/postgres/migrations/000156_purchase_suppressions_az_config_test.go b/internal/database/postgres/migrations/000156_purchase_suppressions_az_config_test.go new file mode 100644 index 000000000..f139a4520 --- /dev/null +++ b/internal/database/postgres/migrations/000156_purchase_suppressions_az_config_test.go @@ -0,0 +1,99 @@ +//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/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + exec156 = "00000000-0000-0000-0000-000000156a01" + + insertRDSSuppression156 = ` + INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, + resource_type, engine, suppressed_count, expires_at, + availability_zone, platform, tenancy, az_config) + VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.r5.large', 'mysql', $2, + NOW() + INTERVAL '7 days', '', '', '', $3)` +) + +// TestMigration_PurchaseSuppressionsAZConfig locks down migration 000156 +// (issue #408): rows seeded before the migration, with and without a 000134 +// variant, survive it with a NULL az_config (the wildcard backfill rule); +// afterwards Single-AZ and Multi-AZ rows of one execution coexist while a +// duplicate and a variant-less az_config are rejected; down merges the +// az_config rows into one row with the summed count, restores the 000134 key +// and keeps the pre-existing rows; up re-applies cleanly. +func TestMigration_PurchaseSuppressionsAZConfig(t *testing.T) { + t.Parallel() + ctx := context.Background() + container := testhelpers.RequireEmptyDatabase(ctx, t) + pool, path := container.DB.Pool(), getMigrationsPath() + prev := previousMigrationVersion(t, 156) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) + // Pre-migration rows: one legacy 6-tuple row (NULL variant) and one RDS + // row with a 000134 variant, as pre-#408 code writes it. + _, err := pool.Exec(ctx, ` + INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, + resource_type, engine, suppressed_count, expires_at) + VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.t3.micro', 'mysql', 1, NOW() + INTERVAL '7 days')`, exec156) + require.NoError(t, err) + _, err = pool.Exec(ctx, ` + 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', 'relational-db', 'us-east-1', 'db.r5.large', 'mysql', 2, NOW() + INTERVAL '7 days', '', '', '')`, exec156) + require.NoError(t, err) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 156)) + + var rows, nulls int + require.NoError(t, pool.QueryRow(ctx, `SELECT count(*), count(*) FILTER (WHERE az_config IS NULL) + FROM purchase_suppressions WHERE execution_id = $1`, exec156).Scan(&rows, &nulls)) + assert.Equal(t, 2, rows, "existing rows survive") + assert.Equal(t, 2, nulls, "existing rows get a NULL az_config: a wildcard") + + // Single-AZ and Multi-AZ rows coexist next to the NULL row of the same variant. + for _, v := range []string{"single-az", "multi-az"} { + _, err = pool.Exec(ctx, insertRDSSuppression156, exec156, 3, v) + require.NoError(t, err, v) + } + _, err = pool.Exec(ctx, insertRDSSuppression156, exec156, 3, "multi-az") + requireUniqueViolation134(t, err, "purchase_suppressions_execution_variant_azc_key") + // An az_config without the rest of the variant is rejected. + _, err = pool.Exec(ctx, ` + INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, + resource_type, engine, suppressed_count, expires_at, az_config) + VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.m5.large', 'mysql', 1, NOW() + INTERVAL '7 days', 'multi-az')`, exec156) + require.Error(t, err) + assert.Contains(t, err.Error(), "purchase_suppressions_az_config_needs_variant") + + // Down: the NULL, single-az and multi-az rows of db.r5.large merge into one + // (2 + 3 + 3); the legacy db.t3.micro row is untouched; the column is gone. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) + var total int + require.NoError(t, pool.QueryRow(ctx, `SELECT count(*), COALESCE(SUM(suppressed_count), 0) + FROM purchase_suppressions WHERE execution_id = $1 AND resource_type = 'db.r5.large'`, exec156).Scan(&rows, &total)) + assert.Equal(t, 1, rows) + assert.Equal(t, 8, total) + require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM purchase_suppressions + WHERE execution_id = $1 AND resource_type = 'db.t3.micro'`, exec156).Scan(&rows)) + assert.Equal(t, 1, rows) + var cols int + require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM information_schema.columns + WHERE table_name = 'purchase_suppressions' AND column_name = 'az_config'`).Scan(&cols)) + assert.Zero(t, cols) + _, err = pool.Exec(ctx, insertVariantSuppression134, exec156, 1, "us-east-1a", "Linux/UNIX", "default") + require.NoError(t, err) + _, err = pool.Exec(ctx, insertVariantSuppression134, exec156, 1, "us-east-1a", "Linux/UNIX", "default") + requireUniqueViolation134(t, err, "purchase_suppressions_execution_variant_key") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 156), "up must re-apply cleanly after down") +} From 8d8159e395477758c94dedd636e295ef13993160 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 3 Nov 2026 19:46:49 +0100 Subject: [PATCH 08/19] fix(suppressions): key purchase suppressions on RDS az_config (#408) Migration 000155 lets Single-AZ and Multi-AZ RDS recs of one class coexist, but the suppression variant lacked az_config, so buying 2x Single-AZ wrote a suppression that also matched, and hid, the Multi-AZ rec (an under-buy). SuppressionVariant now carries AZConfig, set for AWS RDS recs only, and is stored in the new purchase_suppressions.az_config column. An empty AZConfig is a wildcard on either side: suppressions written before migration 000156 and RDS recs without an az_config keep matching both offerings. Regression: TestApplySuppressions_RDSAZConfigScopesToPurchasedOffering failed before (Multi-AZ count 3, want 5) and passes now. Co-Authored-By: Claude Opus 5.5 --- internal/config/recommendation_identity.go | 11 +++- .../config/store_postgres_list_caps_test.go | 4 +- .../config/store_postgres_suppressions.go | 31 ++++++---- .../store_postgres_suppressions_test.go | 47 +++++++++++++-- internal/config/suppressions_build_test.go | 50 ++++++++-------- internal/config/types.go | 17 ++++-- .../scheduler/scheduler_suppressions_test.go | 57 +++++++++++++++++++ .../suppression_variant_integration_test.go | 48 ++++++++++++++++ 8 files changed, 215 insertions(+), 50 deletions(-) diff --git a/internal/config/recommendation_identity.go b/internal/config/recommendation_identity.go index f9c809fda..e4bc8a437 100644 --- a/internal/config/recommendation_identity.go +++ b/internal/config/recommendation_identity.go @@ -114,8 +114,9 @@ func RecIdentityKey(rec *RecommendationRecord) (string, error) { }, "\x1f"), nil } -// SuppressionVariant is the rec's availability zone, platform and tenancy as -// the purchase suppression key carries them (issue #816): the platform and +// SuppressionVariant is the rec's availability zone, platform, tenancy and +// (AWS RDS only) az_config as the purchase suppression key carries them +// (issues #816, #408): the platform and // tenancy come from the same Details read (ComputeIdentity) as RecIdentityKey, // so the write side (BuildSuppressions) and the read side (the scheduler's // subtraction pass) cannot drift from it or from each other. For AWS EC2 only, @@ -126,7 +127,11 @@ func (r *RecommendationRecord) SuppressionVariant() (SuppressionVariant, error) if err != nil { return SuppressionVariant{}, err } - v := SuppressionVariant{AvailabilityZone: r.AvailabilityZone, Platform: ci.Platform, Tenancy: ci.Tenancy} + azConfig, err := r.IdentityAZConfig() + if err != nil { + return SuppressionVariant{}, err + } + v := SuppressionVariant{AvailabilityZone: r.AvailabilityZone, Platform: ci.Platform, Tenancy: ci.Tenancy, AZConfig: azConfig} if isAWSEC2(r.Provider, r.Service) { v.Tenancy = canonicalSuppressionTenancy(v.Tenancy) if v.AvailabilityZone == "" { diff --git a/internal/config/store_postgres_list_caps_test.go b/internal/config/store_postgres_list_caps_test.go index 2b98152ca..f4725c0a0 100644 --- a/internal/config/store_postgres_list_caps_test.go +++ b/internal/config/store_postgres_list_caps_test.go @@ -70,11 +70,11 @@ func suppressionRows(n int) *pgxmock.Rows { rows := pgxmock.NewRows([]string{ "id", "execution_id", "account_id", "provider", "service", "region", "resource_type", "engine", "suppressed_count", "expires_at", "created_at", - "availability_zone", "platform", "tenancy", + "availability_zone", "platform", "tenancy", "az_config", }) for i := 0; i < n; i++ { rows.AddRow("sup", "exec", "acct", "aws", "ec2", "us-east-1", - "t4g.nano", "", 1, now.Add(time.Hour), now, nil, nil, nil) + "t4g.nano", "", 1, now.Add(time.Hour), now, nil, nil, nil, nil) } return rows } diff --git a/internal/config/store_postgres_suppressions.go b/internal/config/store_postgres_suppressions.go index 5395929dd..16562f1ae 100644 --- a/internal/config/store_postgres_suppressions.go +++ b/internal/config/store_postgres_suppressions.go @@ -106,21 +106,25 @@ func (s *PostgresStore) CreateSuppressionTx(ctx context.Context, tx pgx.Tx, sup // 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 + // az_config is NULL when empty: unknown or not applicable (migration 000156). + var az, platform, tenancy, azConfig *string if sup.Variant != nil { az, platform, tenancy = &sup.Variant.AvailabilityZone, &sup.Variant.Platform, &sup.Variant.Tenancy + if sup.Variant.AZConfig != "" { + azConfig = &sup.Variant.AZConfig + } } const q = ` INSERT INTO purchase_suppressions ( id, execution_id, account_id, provider, service, region, 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) + availability_zone, platform, tenancy, az_config + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14) ` 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, az, platform, tenancy, + sup.ExpiresAt, az, platform, tenancy, azConfig, ); err != nil { return fmt.Errorf("failed to insert purchase suppression: %w", err) } @@ -165,7 +169,7 @@ func (s *PostgresStore) ListActiveSuppressions(ctx context.Context) ([]PurchaseS const q = ` SELECT id, execution_id, account_id, provider, service, region, resource_type, engine, suppressed_count, expires_at, created_at, - availability_zone, platform, tenancy + availability_zone, platform, tenancy, az_config FROM purchase_suppressions WHERE expires_at > NOW() ` @@ -178,16 +182,16 @@ func (s *PostgresStore) ListActiveSuppressions(ctx context.Context) ([]PurchaseS var out []PurchaseSuppression for rows.Next() { var sup PurchaseSuppression - var az, platform, tenancy *string + var az, platform, tenancy, azConfig *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, + &az, &platform, &tenancy, &azConfig, ); err != nil { return nil, fmt.Errorf("failed to scan suppression row: %w", err) } - sup.Variant = suppressionVariantFromColumns(az, platform, tenancy) + sup.Variant = suppressionVariantFromColumns(az, platform, tenancy, azConfig) out = append(out, sup) } if err := rows.Err(); err != nil { @@ -199,10 +203,15 @@ func (s *PostgresStore) ListActiveSuppressions(ctx context.Context) ([]PurchaseS // 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 { +// over-suppresses rather than over-shows. A NULL az_config (rows written before +// migration 000156, or a rec with none) is read as "" = any az_config. +func suppressionVariantFromColumns(az, platform, tenancy, azConfig *string) *SuppressionVariant { if az == nil || platform == nil || tenancy == nil { return nil } - return &SuppressionVariant{AvailabilityZone: *az, Platform: *platform, Tenancy: *tenancy} + v := &SuppressionVariant{AvailabilityZone: *az, Platform: *platform, Tenancy: *tenancy} + if azConfig != nil { + v.AZConfig = *azConfig + } + return v } diff --git a/internal/config/store_postgres_suppressions_test.go b/internal/config/store_postgres_suppressions_test.go index 6779f66be..230cac325 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(), (*string)(nil), (*string)(nil), (*string)(nil)). + "t4g.nano", "", 3, pgxmock.AnyArg(), (*string)(nil), (*string)(nil), (*string)(nil), (*string)(nil)). WillReturnResult(pgxmock.NewResult("INSERT", 1)) mock.ExpectCommit() @@ -45,7 +45,7 @@ func TestPGXMock_CreateSuppression_WritesVariant(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(), &az, &platform, &tenancy). + "t4g.nano", "", 3, pgxmock.AnyArg(), &az, &platform, &tenancy, (*string)(nil)). WillReturnResult(pgxmock.NewResult("INSERT", 1)) mock.ExpectCommit() @@ -120,14 +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", + "availability_zone", "platform", "tenancy", "az_config", } 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, nil, nil, nil). + "t4g.nano", "", 3, future, now, nil, nil, nil, nil). AddRow("sup-2", "exec-2", "acct-1", "aws", "ec2", "us-east-1", - "t4g.nano", "", 2, future, now, &az, &platform, &tenancy) + "t4g.nano", "", 2, future, now, &az, &platform, &tenancy, nil) mock.ExpectQuery(`SELECT .* FROM purchase_suppressions WHERE expires_at > NOW\(\)`). WillReturnRows(rows) @@ -175,3 +175,40 @@ func TestPGXMock_WithTx_RollsBackOnError(t *testing.T) { assert.ErrorIs(t, err, assert.AnError) assert.NoError(t, mock.ExpectationsWereMet()) } + +// Issue #408: an RDS suppression writes its az_config, and a NULL az_config +// reads back as "" (a wildcard); a row with a value reads back with it. +func TestPGXMock_Suppression_AZConfigRoundTrip(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + az, platform, tenancy, azConfig := "", "", "", "multi-az" + mock.ExpectBegin() + mock.ExpectExec(`INSERT INTO purchase_suppressions`). + WithArgs(pgxmock.AnyArg(), "exec-1", "acct-1", "aws", "relational-db", "us-east-1", + "db.r5.large", "mysql", 2, pgxmock.AnyArg(), &az, &platform, &tenancy, &azConfig). + WillReturnResult(pgxmock.NewResult("INSERT", 1)) + mock.ExpectCommit() + require.NoError(t, store.CreateSuppression(ctx, &PurchaseSuppression{ + ExecutionID: "exec-1", AccountID: "acct-1", Provider: "aws", Service: "relational-db", + Region: "us-east-1", ResourceType: "db.r5.large", Engine: "mysql", + SuppressedCount: 2, ExpiresAt: time.Now().Add(time.Hour), + Variant: &SuppressionVariant{AZConfig: "multi-az"}, + })) + + now := time.Now().UTC().Truncate(time.Second) + cols := []string{"id", "execution_id", "account_id", "provider", "service", "region", + "resource_type", "engine", "suppressed_count", "expires_at", "created_at", + "availability_zone", "platform", "tenancy", "az_config"} + rows := pgxmock.NewRows(cols). + AddRow("a", "e", "acct-1", "aws", "relational-db", "us-east-1", "db.r5.large", "mysql", 2, now, now, &az, &platform, &tenancy, nil). + AddRow("b", "e", "acct-1", "aws", "relational-db", "us-east-1", "db.r5.large", "mysql", 2, now, now, &az, &platform, &tenancy, &azConfig) + mock.ExpectQuery(`SELECT .* FROM purchase_suppressions`).WillReturnRows(rows) + sups, err := store.ListActiveSuppressions(ctx) + require.NoError(t, err) + require.Len(t, sups, 2) + assert.Equal(t, "", sups[0].Variant.AZConfig) + assert.Equal(t, "multi-az", sups[1].Variant.AZConfig) + assert.NoError(t, mock.ExpectationsWereMet()) +} diff --git a/internal/config/suppressions_build_test.go b/internal/config/suppressions_build_test.go index 6d054c9c1..2215f404f 100644 --- a/internal/config/suppressions_build_test.go +++ b/internal/config/suppressions_build_test.go @@ -94,11 +94,11 @@ func TestBuildSuppressions_SeparatesVariants(t *testing.T) { 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}, + {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) @@ -143,13 +143,13 @@ func TestPurchaseSuppression_AppliesTo(t *testing.T) { 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(&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(&SuppressionVariant{"us-east-1b", "Windows", "dedicated", ""}), "a legacy row is a wildcard") assert.True(t, legacy.AppliesTo(nil)) } @@ -166,7 +166,7 @@ func TestBuildSuppressions_LegacyEC2WithoutDetailsIsWildcard(t *testing.T) { 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"})) + assert.True(t, sups[0].AppliesTo(&SuppressionVariant{"us-east-1a", "Linux/UNIX", "default", ""})) }) } @@ -174,7 +174,7 @@ func TestBuildSuppressions_LegacyEC2WithoutDetailsIsWildcard(t *testing.T) { 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) + assert.Equal(t, &SuppressionVariant{"us-east-1a", "Linux/UNIX", "default", ""}, sups[0].Variant) } // TestBuildSuppressions_ComputeSlugEC2: AWS EC2 recs under the canonical @@ -185,14 +185,14 @@ func TestBuildSuppressions_ComputeSlugEC2(t *testing.T) { 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"})) + 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"}}, + "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 @@ -284,12 +284,12 @@ func TestBuildSuppressions_LegacyShapesCanonicalised(t *testing.T) { rec RecommendationRecord want SuppressionVariant }{ - "legacy shared tenancy": {mk("ec2", "us-east-1a", `{"platform":"Linux/UNIX","tenancy":"shared"}`), SuppressionVariant{"us-east-1a", "Linux/UNIX", "default"}}, - "compute slug shared": {mk("compute", "", `{"platform":"Linux/UNIX","tenancy":"shared"}`), SuppressionVariant{"", "Linux/UNIX", "default"}}, - "AZ only in details": {mk("ec2", "", `{"availability_zone":"us-east-1a","platform":"Linux/UNIX","tenancy":"default"}`), SuppressionVariant{"us-east-1a", "Linux/UNIX", "default"}}, - "field wins over details": {mk("ec2", "us-east-1b", `{"availability_zone":"us-east-1a","platform":"Linux/UNIX","tenancy":"default"}`), SuppressionVariant{"us-east-1b", "Linux/UNIX", "default"}}, - "modern rec unchanged": {mk("ec2", "us-east-1a", `{"availability_zone":"us-east-1a","platform":"Linux/UNIX","tenancy":"dedicated"}`), SuppressionVariant{"us-east-1a", "Linux/UNIX", "dedicated"}}, - "non-EC2 untouched": {RecommendationRecord{Provider: "aws", Service: "rds", Region: "r", ResourceType: "db", Count: 1, Details: []byte(`{"availability_zone":"us-east-1a","tenancy":"shared"}`)}, SuppressionVariant{"", "", "shared"}}, + "legacy shared tenancy": {mk("ec2", "us-east-1a", `{"platform":"Linux/UNIX","tenancy":"shared"}`), SuppressionVariant{"us-east-1a", "Linux/UNIX", "default", ""}}, + "compute slug shared": {mk("compute", "", `{"platform":"Linux/UNIX","tenancy":"shared"}`), SuppressionVariant{"", "Linux/UNIX", "default", ""}}, + "AZ only in details": {mk("ec2", "", `{"availability_zone":"us-east-1a","platform":"Linux/UNIX","tenancy":"default"}`), SuppressionVariant{"us-east-1a", "Linux/UNIX", "default", ""}}, + "field wins over details": {mk("ec2", "us-east-1b", `{"availability_zone":"us-east-1a","platform":"Linux/UNIX","tenancy":"default"}`), SuppressionVariant{"us-east-1b", "Linux/UNIX", "default", ""}}, + "modern rec unchanged": {mk("ec2", "us-east-1a", `{"availability_zone":"us-east-1a","platform":"Linux/UNIX","tenancy":"dedicated"}`), SuppressionVariant{"us-east-1a", "Linux/UNIX", "dedicated", ""}}, + "non-EC2 untouched": {RecommendationRecord{Provider: "aws", Service: "rds", Region: "r", ResourceType: "db", Count: 1, Details: []byte(`{"availability_zone":"us-east-1a","tenancy":"shared"}`)}, SuppressionVariant{"", "", "shared", ""}}, } { t.Run(name, func(t *testing.T) { sups := BuildSuppressions([]RecommendationRecord{tc.rec}, "exec-1", &GlobalConfig{}, time.Now()) @@ -300,8 +300,8 @@ func TestBuildSuppressions_LegacyShapesCanonicalised(t *testing.T) { } func TestPurchaseSuppression_AppliesTo_LegacySharedTenancy(t *testing.T) { - def := &SuppressionVariant{"us-east-1a", "Linux/UNIX", "default"} - variant := &SuppressionVariant{"us-east-1a", "Linux/UNIX", "shared"} + def := &SuppressionVariant{"us-east-1a", "Linux/UNIX", "default", ""} + variant := &SuppressionVariant{"us-east-1a", "Linux/UNIX", "shared", ""} for name, tc := range map[string]struct { provider, service string want bool @@ -317,7 +317,7 @@ func TestPurchaseSuppression_AppliesTo_LegacySharedTenancy(t *testing.T) { }) } ec2 := PurchaseSuppression{Provider: "aws", Service: "ec2", Variant: variant} - assert.False(t, ec2.AppliesTo(&SuppressionVariant{"us-east-1a", "Linux/UNIX", "dedicated"})) + assert.False(t, ec2.AppliesTo(&SuppressionVariant{"us-east-1a", "Linux/UNIX", "dedicated", ""})) } func TestRecommendationRecord_SuppressionVariant_DetailsAZ(t *testing.T) { diff --git a/internal/config/types.go b/internal/config/types.go index 3bc0b71ba..e227aa722 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -774,13 +774,18 @@ type PurchaseSuppression struct { } // 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. +// the zonal EC2 availability zone, the EC2 platform and tenancy, and the AWS +// RDS az_config, the same fields the rec identity key carries (issues #792, +// #806, #408). Empty AvailabilityZone, Platform and Tenancy are known values +// (a regional rec, a non-EC2 rec), not wildcards. AZConfig is the exception: +// "" means "not applicable or unknown" and matches any az_config (see +// AppliesTo), because suppressions written before migration 000156 have none +// and must keep suppressing both the Single-AZ and the Multi-AZ rec. type SuppressionVariant struct { AvailabilityZone string `json:"availability_zone"` Platform string `json:"platform"` Tenancy string `json:"tenancy"` + AZConfig string `json:"az_config,omitempty"` } // awsLegacyServiceSlugs maps the AWS service slugs that rec rows persisted @@ -838,12 +843,16 @@ func (s *PurchaseSuppression) AppliesTo(v *SuppressionVariant) bool { return true } a, b := *s.Variant, *v + // An empty az_config on either side is a wildcard: legacy rows predate it + // and an RDS rec without one cannot say which offering it is (#408). + azConfigOK := a.AZConfig == "" || b.AZConfig == "" || a.AZConfig == b.AZConfig + a.AZConfig, b.AZConfig = "", "" // Only the stored side needs canonicalising: SuppressionVariant already // canonicalises every rec variant, so a raw "shared" never reaches v. if isAWSEC2(s.Provider, s.Service) { a.Tenancy = canonicalSuppressionTenancy(a.Tenancy) } - return a == b + return azConfigOK && a == b } // RecommendationFilter parameterises ListStoredRecommendations and the diff --git a/internal/scheduler/scheduler_suppressions_test.go b/internal/scheduler/scheduler_suppressions_test.go index 818ca9d70..52b6dda37 100644 --- a/internal/scheduler/scheduler_suppressions_test.go +++ b/internal/scheduler/scheduler_suppressions_test.go @@ -444,3 +444,60 @@ func withCount(r config.RecommendationRecord, n int) config.RecommendationRecord r.Count = n return r } + +func rdsAZConfigSuppressionRec(id, azConfig string) config.RecommendationRecord { + return config.RecommendationRecord{ID: id, Provider: "aws", Service: "rds", Region: "us-east-1", + ResourceType: "db.r5.large", Engine: "mysql", Count: 5, Term: 1, Payment: "all-upfront", + CloudAccountID: strPtr("acct-1"), + Details: []byte(`{"engine":"mysql","az_config":"` + azConfig + `"}`)} +} + +// Issue #408: buying 2x Single-AZ db.r5.large mysql must not hide or reduce +// the Multi-AZ rec of the same class, which is a different offering. +func TestApplySuppressions_RDSAZConfigScopesToPurchasedOffering(t *testing.T) { + single := rdsAZConfigSuppressionRec("single", "single-az") + multi := rdsAZConfigSuppressionRec("multi", "multi-az") + purchase := single + purchase.Count = 2 + sups := config.BuildSuppressions([]config.RecommendationRecord{purchase}, "e1", &config.GlobalConfig{}, time.Now()) + require.Len(t, sups, 1) + + out := applySuppressionIndex([]config.RecommendationRecord{single, multi}, aggregateSuppressions(sups)) + require.Len(t, out, 2) + assert.Equal(t, 3, out[0].Count, "single-az: 5 - 2") + assert.Equal(t, 5, out[1].Count, "multi-az must keep its full count") + assert.Zero(t, out[1].SuppressedCount) +} + +// A suppression row written before az_config existed (variant columns set, +// az_config NULL) still suppresses both offerings, as it did before, so the +// fix fails safe against a double purchase. +func TestApplySuppressions_RDSLegacyVariantRowIsAZConfigWildcard(t *testing.T) { + legacy := config.PurchaseSuppression{ExecutionID: "old", AccountID: "acct-1", Provider: "aws", + Service: "rds", Region: "us-east-1", ResourceType: "db.r5.large", Engine: "mysql", + SuppressedCount: 2, ExpiresAt: time.Now().Add(24 * time.Hour), + Variant: &config.SuppressionVariant{}} + out := applySuppressionIndex([]config.RecommendationRecord{ + rdsAZConfigSuppressionRec("single", "single-az"), rdsAZConfigSuppressionRec("multi", "multi-az"), + }, aggregateSuppressions([]config.PurchaseSuppression{legacy})) + require.Len(t, out, 2) + assert.Equal(t, 3, out[0].Count) + assert.Equal(t, 3, out[1].Count) +} + +// Azure SQL keeps az_config out of the suppression variant: a flip of the +// subscription-wide value between collections must not un-suppress the rec. +func TestApplySuppressions_AzureSQLIgnoresAZConfig(t *testing.T) { + azure := func(azConfig string) config.RecommendationRecord { + r := rdsAZConfigSuppressionRec("sql", azConfig) + r.Provider, r.Service, r.Region, r.ResourceType, r.Engine = "azure", "database", "eastus", "GP_Gen5_2", "sqlserver" + return r + } + purchase := azure("zoneRedundant") + purchase.Count = 2 + sups := config.BuildSuppressions([]config.RecommendationRecord{purchase}, "e1", &config.GlobalConfig{GracePeriodDays: map[string]int{"azure": 7}}, time.Now()) + require.Len(t, sups, 1) + out := applySuppressionIndex([]config.RecommendationRecord{azure("")}, aggregateSuppressions(sups)) + 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 index f406cae88..08a6ddf6c 100644 --- a/internal/scheduler/suppression_variant_integration_test.go +++ b/internal/scheduler/suppression_variant_integration_test.go @@ -146,3 +146,51 @@ func TestSuppressionsMatchAcrossEC2Slugs_RealPostgres(t *testing.T) { } assert.Equal(t, map[string]int{"ec2-rec": 3, "compute-rec": 3, "azure-rec": 5}, counts) } + +// TestRDSSuppressionScopedToAZConfig_RealPostgres is the issue #408 +// regression on a real database: after migrations 000154-000156 a Single-AZ +// and a Multi-AZ db.r5.large mysql rec coexist. Buying 2x Single-AZ must leave +// the Multi-AZ rec at its full count, while a suppression row written before +// az_config existed (variant set, az_config NULL) still hides both. +func TestRDSSuppressionScopedToAZConfig_RealPostgres(t *testing.T) { + ctx := t.Context() + pg := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") + store := config.NewPostgresStore(pg.DB) + + rec := func(id, resourceType, azConfig string) config.RecommendationRecord { + return config.RecommendationRecord{ + ID: id, Provider: "aws", Service: "relational-db", Region: "us-east-1", ResourceType: resourceType, + Engine: "mysql", Count: 5, Term: 1, Payment: "all-upfront", + Details: []byte(`{"engine":"mysql","az_config":"` + azConfig + `"}`), + } + } + single := rec("single", "db.r5.large", "single-az") + records := []config.RecommendationRecord{ + single, rec("multi", "db.r5.large", "multi-az"), + rec("old-single", "db.m5.large", "single-az"), rec("old-multi", "db.m5.large", "multi-az"), + } + require.NoError(t, store.UpsertRecommendations(ctx, time.Now(), records, nil)) + + purchase := single + purchase.Count = 2 + 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]) })) + + // Written by code that predates az_config: variant set, az_config 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, + availability_zone, platform, tenancy) + VALUES ($1, '', 'aws', 'relational-db', 'us-east-1', 'db.m5.large', 'mysql', 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{"single": 3, "multi": 5, "old-single": 4, "old-multi": 4}, counts) +} From c684e213cf28e40e42a81f8db31992cb440af272 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 3 Nov 2026 20:49:07 +0100 Subject: [PATCH 09/19] fix(recommendations): scope generated az_config to AWS RDS The az_config column was derived for every provider while the rec ID, identity key and fingerprint only carry it for AWS RDS. A cron collect and a refresh overlapping on an Azure SQL rec could commit it with az_config 'none' and '', the 12-column upsert key inserted a second row and the eviction spared it, so the rec was listed twice and could be bought twice. The column is now '' outside provider aws with service rds or relational-db, mirroring IdentityAZConfig. Edited in 000154 in place: it is unmerged and 000155/000156 do not reference the expression. Refs #408 Co-Authored-By: Claude Opus 5.5 --- internal/config/recommendation_identity.go | 5 ++- ..._postgres_recommendations_azconfig_test.go | 42 +++++++++++++++++++ .../000154_recommendations_az_config.up.sql | 14 ++++++- .../000154_recommendations_az_config_test.go | 42 +++++++++++++------ 4 files changed, 86 insertions(+), 17 deletions(-) diff --git a/internal/config/recommendation_identity.go b/internal/config/recommendation_identity.go index e4bc8a437..8adbc4761 100644 --- a/internal/config/recommendation_identity.go +++ b/internal/config/recommendation_identity.go @@ -47,8 +47,9 @@ func (r *RecommendationRecord) ComputeIdentity() (ComputeIdentity, error) { // "multi-az", Azure SQL "zoneRedundant" / "none") from the rec's Details // blob, VERBATIM. Single-AZ and Multi-AZ RDS reservations are different // offerings with different prices, yet share every other identity field -// (issue #408). It mirrors the generated az_config column of migration 000154 -// (COALESCE(payload->'details'->>'az_config', ”)) and is pinned to it by the +// (issue #408). For AWS RDS rows it mirrors the generated az_config column of +// migration 000154 (COALESCE(payload->'details'->>'az_config', ”); the column +// is ” for every other rec, like IdentityAZConfig) and is pinned to it by the // shared vectors in TestDetailsAZConfig_Vectors: absent Details, JSON null, a // missing key and a null value all yield "". Only DatabaseDetails carries the // key. A non-string value is an error. diff --git a/internal/config/store_postgres_recommendations_azconfig_test.go b/internal/config/store_postgres_recommendations_azconfig_test.go index 49b32bad2..198ec91ff 100644 --- a/internal/config/store_postgres_recommendations_azconfig_test.go +++ b/internal/config/store_postgres_recommendations_azconfig_test.go @@ -157,3 +157,45 @@ func TestPostgresStore_UpsertRecommendations_AZConfigConcurrent(t *testing.T) { assert.Equal(t, 2, n) assert.Equal(t, 2, distinct) } + +func azureSQLStoreRec(azConfig string) config.RecommendationRecord { + rec := config.RecommendationRecord{ + ID: "azsql", Provider: "azure", Service: "relational-db", Region: "eastus", + ResourceType: "GP_Gen5_2", Savings: 10, UpfrontCost: 100, Count: 1, Term: 12, Payment: "no-upfront", + } + rec.Details = json.RawMessage(`{"az_config":"` + azConfig + `"}`) + return rec +} + +// Azure SQL's az_config is outside the rec identity (IdentityAZConfig is +// AWS-RDS only), so the 12-column arbiter must not see it either. A cron +// collect and a refresh overlap and can commit the same Azure SQL rec with +// "none" and "" (an incomplete server lookup); the arbiter used to key on the +// generated column for every provider, insert a second row, and the eviction +// (collected_at < t) spared it, listing the rec twice. +func TestPostgresStore_UpsertRecommendations_AzureSQLAZConfigIsNotAKey(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequireMigratedDatabase(ctx, t, getMigrationsPath()) + pool := container.DB.Pool() + store := config.NewPostgresStore(container.DB) + t0 := time.Now().UTC().Truncate(time.Second) + collect := []config.SuccessfulCollect{{Provider: "azure"}} + + for _, tc := range []struct{ name, first, second string }{ + {"refresh none then earlier cron empty", "none", ""}, + {"refresh empty then earlier cron none", "", "none"}, + {"zoneRedundant then earlier cron none", "zoneRedundant", "none"}, + } { + t.Run(tc.name, func(t *testing.T) { + _, err := pool.Exec(ctx, `DELETE FROM recommendations`) + require.NoError(t, err) + // The refresh started later but commits first; the cron it + // overlapped (older collectedAt) commits second. + require.NoError(t, store.UpsertRecommendations(ctx, t0.Add(time.Minute), []config.RecommendationRecord{azureSQLStoreRec(tc.first)}, collect)) + require.NoError(t, store.UpsertRecommendations(ctx, t0, []config.RecommendationRecord{azureSQLStoreRec(tc.second)}, collect)) + var n int + require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM recommendations`).Scan(&n)) + assert.Equal(t, 1, n, "one Azure SQL rec must be stored once whatever its az_config") + }) + } +} diff --git a/internal/database/postgres/migrations/000154_recommendations_az_config.up.sql b/internal/database/postgres/migrations/000154_recommendations_az_config.up.sql index 65f22d9e8..ca0bdbda3 100644 --- a/internal/database/postgres/migrations/000154_recommendations_az_config.up.sql +++ b/internal/database/postgres/migrations/000154_recommendations_az_config.up.sql @@ -12,7 +12,12 @@ -- RecommendationRecord) at payload->'details' (the DatabaseDetails blob), so -- existing rows are backfilled by the table rewrite with no data migration. -- No trimming or case-folding. Absent Details, JSON null, a missing key and a --- null value all become ''. Every non-database row is ''. +-- null value all become ''. Every row outside AWS RDS is '' too: it mirrors +-- RecommendationRecord.IdentityAZConfig (provider 'aws' and service 'rds' or +-- its canonical slug 'relational-db'). Azure SQL's az_config is a +-- subscription-wide value that flips between collections and is not part of +-- the rec ID, so keying the upsert on it would store the same rec twice when +-- a cron collect and a refresh overlap. -- -- Two indexes are added: -- - recommendations_natural_key_az_tp_azc_idx: the 11 columns plus az_config, @@ -35,7 +40,12 @@ -- not possible: the migration runner wraps every migration in a transaction. ALTER TABLE recommendations ADD COLUMN IF NOT EXISTS az_config TEXT - GENERATED ALWAYS AS (COALESCE(payload->'details'->>'az_config', '')) STORED; + GENERATED ALWAYS AS ( + CASE WHEN lower(provider) = 'aws' AND service IN ('rds', 'relational-db') + THEN COALESCE(payload->'details'->>'az_config', '') + ELSE '' + END + ) STORED; CREATE UNIQUE INDEX IF NOT EXISTS recommendations_natural_key_az_tp_azc_idx ON recommendations (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, az_config); diff --git a/internal/database/postgres/migrations/000154_recommendations_az_config_test.go b/internal/database/postgres/migrations/000154_recommendations_az_config_test.go index 301341ae1..0d2e4a8d5 100644 --- a/internal/database/postgres/migrations/000154_recommendations_az_config_test.go +++ b/internal/database/postgres/migrations/000154_recommendations_az_config_test.go @@ -21,6 +21,13 @@ const insertRecForMigration152 = ` VALUES (NOW(), 'aws', 'rds', 'us-east-1', $1, 'mysql', $2::jsonb, 0, 0, 1, 'all-upfront') ` +const insertRecForMigration154Scoped = ` + INSERT INTO recommendations + (collected_at, provider, service, region, resource_type, engine, payload, + upfront_cost, monthly_savings, term, payment_option) + VALUES (NOW(), $3, $4, 'us-east-1', $1, 'mysql', $2::jsonb, 0, 0, 1, 'all-upfront') +` + // TestMigration_RecommendationsAZConfig locks down migration 000154 (issue // #408, expand step): the generated az_config column is read verbatim from // payload->'details', the 11-column index is kept so the previous upsert still @@ -38,23 +45,32 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 154))) - type vector struct{ payload, azConfig string } + // provider and service default to aws/rds; the column is scoped to AWS RDS + // like IdentityAZConfig, so other providers and services read "". + type vector struct{ payload, azConfig, provider, service string } vectors := map[string]vector{ - "details-absent": {`{"service":"rds"}`, ""}, - "details-null": {`{"details":null}`, ""}, - "details-empty": {`{"details":{}}`, ""}, - "az-config-null": {`{"details":{"az_config":null}}`, ""}, - "empty-string": {`{"details":{"az_config":""}}`, ""}, - "rds-single-az": {`{"details":{"az_config":"single-az"}}`, "single-az"}, - "rds-multi-az": {`{"details":{"az_config":"multi-az"}}`, "multi-az"}, - "azure-zone-redundant": {`{"details":{"az_config":"zoneRedundant"}}`, "zoneRedundant"}, - "verbatim-spaces": {`{"details":{"az_config":" Multi-AZ "}}`, " Multi-AZ "}, - "compute-details": {`{"details":{"platform":"Linux/UNIX","tenancy":"default"}}`, ""}, + "details-absent": {`{"service":"rds"}`, "", "", ""}, + "details-null": {`{"details":null}`, "", "", ""}, + "details-empty": {`{"details":{}}`, "", "", ""}, + "az-config-null": {`{"details":{"az_config":null}}`, "", "", ""}, + "empty-string": {`{"details":{"az_config":""}}`, "", "", ""}, + "rds-single-az": {`{"details":{"az_config":"single-az"}}`, "single-az", "", ""}, + "rds-multi-az": {`{"details":{"az_config":"multi-az"}}`, "multi-az", "", ""}, + "rds-canonical-slug": {`{"details":{"az_config":"multi-az"}}`, "multi-az", "aws", "relational-db"}, + "azure-sql-zone-redundant": {`{"details":{"az_config":"zoneRedundant"}}`, "", "azure", "relational-db"}, + "azure-sql-none": {`{"details":{"az_config":"none"}}`, "", "azure", "relational-db"}, + "aws-ec2-with-key": {`{"details":{"az_config":"multi-az"}}`, "", "aws", "ec2"}, + "gcp-sql": {`{"details":{"az_config":"multi-az"}}`, "", "gcp", "relational-db"}, + "verbatim-spaces": {`{"details":{"az_config":" Multi-AZ "}}`, " Multi-AZ ", "", ""}, + "compute-details": {`{"details":{"platform":"Linux/UNIX","tenancy":"default"}}`, "", "", ""}, // A top-level key is NOT consulted: only Details carries the field. - "top-level-ignored": {`{"az_config":"multi-az"}`, ""}, + "top-level-ignored": {`{"az_config":"multi-az"}`, "", "", ""}, } for name, v := range vectors { - _, err = pool.Exec(ctx, insertRecForMigration152, name, v.payload) + if v.provider == "" { + v.provider, v.service = "aws", "rds" + } + _, err = pool.Exec(ctx, insertRecForMigration154Scoped, name, v.payload, v.provider, v.service) require.NoError(t, err) } From e9dffbff06a8b6052baf8c822e32ac15de164bf7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 3 Nov 2026 20:49:18 +0100 Subject: [PATCH 10/19] test(scheduler): cover Azure SQL zoneRedundant to none flip in suppressions Refs #408 Co-Authored-By: Claude Opus 5.5 --- .../scheduler/scheduler_suppressions_test.go | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/internal/scheduler/scheduler_suppressions_test.go b/internal/scheduler/scheduler_suppressions_test.go index 52b6dda37..99eec4821 100644 --- a/internal/scheduler/scheduler_suppressions_test.go +++ b/internal/scheduler/scheduler_suppressions_test.go @@ -493,11 +493,20 @@ func TestApplySuppressions_AzureSQLIgnoresAZConfig(t *testing.T) { r.Provider, r.Service, r.Region, r.ResourceType, r.Engine = "azure", "database", "eastus", "GP_Gen5_2", "sqlserver" return r } - purchase := azure("zoneRedundant") - purchase.Count = 2 - sups := config.BuildSuppressions([]config.RecommendationRecord{purchase}, "e1", &config.GlobalConfig{GracePeriodDays: map[string]int{"azure": 7}}, time.Now()) - require.Len(t, sups, 1) - out := applySuppressionIndex([]config.RecommendationRecord{azure("")}, aggregateSuppressions(sups)) - require.Len(t, out, 1) - assert.Equal(t, 3, out[0].Count) + // "none" is the real double-buy direction: an empty az_config is a + // wildcard, but "zoneRedundant" vs "none" would un-suppress the rec if the + // variant carried it. + for _, tc := range []struct{ bought, listed string }{ + {"zoneRedundant", ""}, {"zoneRedundant", "none"}, {"none", "zoneRedundant"}, {"none", ""}, + } { + t.Run(tc.bought+" then "+tc.listed, func(t *testing.T) { + purchase := azure(tc.bought) + purchase.Count = 2 + sups := config.BuildSuppressions([]config.RecommendationRecord{purchase}, "e1", &config.GlobalConfig{GracePeriodDays: map[string]int{"azure": 7}}, time.Now()) + require.Len(t, sups, 1) + out := applySuppressionIndex([]config.RecommendationRecord{azure(tc.listed)}, aggregateSuppressions(sups)) + require.Len(t, out, 1) + assert.Equal(t, 3, out[0].Count) + }) + } } From 70d85dbe2db9d1e870a8273e91d13f5025039560 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 18:56:01 +0100 Subject: [PATCH 11/19] fix(migrations): order both az_config expand steps before the contract The suppression az_config migration sorted after the contract migration that drops the 11-column recommendations key, but the contract can only run after the release. New code on a database at the last pre-contract version failed on purchase suppression writes and listing. Renumber so both expands run first: 000154 recommendations az_config, 000155 purchase_suppressions az_config, 000156 contract. Update the cross-references in migration comments and store comments, and add an integration test that runs the new code against a database stopped just below the contract and again after it. Co-Authored-By: Claude Opus 5.5 --- .../azconfig_expand_order_integration_test.go | 113 ++++++++++++++++++ ..._postgres_recommendations_azconfig_test.go | 4 +- .../config/store_postgres_suppressions.go | 4 +- internal/config/types.go | 2 +- .../000154_recommendations_az_config.up.sql | 4 +- .../000154_recommendations_az_config_test.go | 2 +- ..._purchase_suppressions_az_config.down.sql} | 2 +- ...55_purchase_suppressions_az_config.up.sql} | 14 ++- ...5_purchase_suppressions_az_config_test.go} | 32 ++--- ...ecommendations_11col_natural_key.down.sql} | 2 +- ..._recommendations_11col_natural_key.up.sql} | 2 +- ...recommendations_11col_natural_key_test.go} | 10 +- 12 files changed, 153 insertions(+), 38 deletions(-) create mode 100644 internal/api/azconfig_expand_order_integration_test.go rename internal/database/postgres/migrations/{000156_purchase_suppressions_az_config.down.sql => 000155_purchase_suppressions_az_config.down.sql} (97%) rename internal/database/postgres/migrations/{000156_purchase_suppressions_az_config.up.sql => 000155_purchase_suppressions_az_config.up.sql} (77%) rename internal/database/postgres/migrations/{000156_purchase_suppressions_az_config_test.go => 000155_purchase_suppressions_az_config_test.go} (87%) rename internal/database/postgres/migrations/{000155_drop_recommendations_11col_natural_key.down.sql => 000156_drop_recommendations_11col_natural_key.down.sql} (95%) rename internal/database/postgres/migrations/{000155_drop_recommendations_11col_natural_key.up.sql => 000156_drop_recommendations_11col_natural_key.up.sql} (96%) rename internal/database/postgres/migrations/{000155_drop_recommendations_11col_natural_key_test.go => 000156_drop_recommendations_11col_natural_key_test.go} (97%) diff --git a/internal/api/azconfig_expand_order_integration_test.go b/internal/api/azconfig_expand_order_integration_test.go new file mode 100644 index 000000000..fff498245 --- /dev/null +++ b/internal/api/azconfig_expand_order_integration_test.go @@ -0,0 +1,113 @@ +//go:build integration + +package api + +import ( + "context" + "path/filepath" + "strconv" + "strings" + "testing" + "time" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "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" +) + +// contractMigrationVersion is the version of the migration that drops the +// 11-column recommendations key (#408). It can only run after the release that +// upserts on the 12-column key, so every expand step the new code needs must +// sort BEFORE it. +func contractMigrationVersion(t *testing.T) uint { + t.Helper() + paths, err := filepath.Glob(filepath.Join(getMigrationsPath(), "*_drop_recommendations_11col_natural_key.up.sql")) + require.NoError(t, err) + require.Len(t, paths, 1) + n, err := strconv.ParseUint(strings.SplitN(filepath.Base(paths[0]), "_", 2)[0], 10, 64) + require.NoError(t, err) + return uint(n) +} + +// previousVersion returns the highest migration version below version, so the +// test stops at the last migration a deploy applies before the contract step. +func previousVersion(t *testing.T, version uint) uint { + t.Helper() + paths, err := filepath.Glob(filepath.Join(getMigrationsPath(), "*.up.sql")) + require.NoError(t, err) + var best uint + for _, p := range paths { + n, err := strconv.ParseUint(strings.SplitN(filepath.Base(p), "_", 2)[0], 10, 64) + require.NoError(t, err) + if uint(n) < version && uint(n) > best { + best = uint(n) + } + } + require.NotZero(t, best) + return best +} + +func expandOrderRDSRec(azConfig string, upfront float64) config.RecommendationRecord { + rec := rdsAZConfigRec(azConfig, upfront) + rec.ID = "" + return rec +} + +// exerciseNewCode runs the four paths the #408 code touches against whatever +// schema the database currently has. +// Before the contract step the 11-column index is still in place, so a +// Single-AZ and Multi-AZ pair of one class cannot coexist yet (the documented +// limitation the contract step resolves, and the same failure the previous +// release had); only after it may both variants be upserted in one batch. +func exerciseNewCode(ctx context.Context, t *testing.T, store *config.PostgresStore, recs []config.RecommendationRecord, priced string, wantUpfront float64) { + t.Helper() + single := expandOrderRDSRec("single-az", 100) + + require.NoError(t, store.UpsertRecommendations(ctx, time.Now(), recs, []config.SuccessfulCollect{{Provider: "aws"}}), + "upsert of RDS variants") + + purchase := single + purchase.Count = 2 + 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]) }), + "suppression write inside the purchase transaction") + + active, err := store.ListActiveSuppressions(ctx) + require.NoError(t, err, "ListActiveSuppressions") + assert.NotEmpty(t, active) + + h := &Handler{config: store} + req := []config.RecommendationRecord{{ + Provider: "aws", Service: "rds", Region: "us-east-1", ResourceType: "db.r5.large", Engine: "mysql", + Term: 1, Payment: "all-upfront", Count: 1, Details: rdsDetails(priced), + }} + require.NoError(t, h.priceRecommendationsFromStore(ctx, req), "purchase pricing index lookup") + assert.Equal(t, wantUpfront, req[0].UpfrontCost) +} + +// Issue #408 deploy order: the new code ships after the expand migrations and +// before the contract one. A database stopped just below the contract step +// must already serve every new-code path, and still do so once the contract +// step has run. +func TestAZConfigNewCodeWorksBeforeAndAfterContractMigration(t *testing.T) { + ctx := context.Background() + pg := testhelpers.RequireEmptyDatabase(ctx, t) + pool, path := pg.DB.Pool(), getMigrationsPath() + contract := contractMigrationVersion(t) + store := config.NewPostgresStore(pg.DB) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, previousVersion(t, contract))) + single, multi := expandOrderRDSRec("single-az", 100), expandOrderRDSRec("multi-az", 300) + exerciseNewCode(ctx, t, store, []config.RecommendationRecord{single}, "single-az", 100) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, contract)) + _, err := pool.Exec(ctx, `TRUNCATE recommendations, purchase_suppressions`) + require.NoError(t, err) + exerciseNewCode(ctx, t, store, []config.RecommendationRecord{single, multi}, "multi-az", 300) +} diff --git a/internal/config/store_postgres_recommendations_azconfig_test.go b/internal/config/store_postgres_recommendations_azconfig_test.go index 198ec91ff..e65e88c30 100644 --- a/internal/config/store_postgres_recommendations_azconfig_test.go +++ b/internal/config/store_postgres_recommendations_azconfig_test.go @@ -26,7 +26,7 @@ func rdsAZConfigStoreRec(azConfig string, savings float64) config.Recommendation // Issue #408: Cost Explorer reports Single-AZ and Multi-AZ RDS reservations of // the same class, engine, term and payment as separate recommendations. Once // the store upserts on the 12-column index (with the generated az_config -// column) and migration 000155 drops the 11-column index, both rows upsert in +// column) and migration 000156 drops the 11-column index, both rows upsert in // ONE batch (the shape persistCollection writes) and both persist. Before, // the batch failed with SQLSTATE 21000 ("ON CONFLICT DO UPDATE command cannot // affect row a second time") and took the whole collection write down. @@ -40,7 +40,7 @@ func TestPostgresStore_UpsertRecommendations_AZConfigBatch(t *testing.T) { var oldIdx bool require.NoError(t, pool.QueryRow(ctx, `SELECT EXISTS (SELECT 1 FROM pg_indexes WHERE indexname = 'recommendations_natural_key_az_tp_idx')`).Scan(&oldIdx)) - require.False(t, oldIdx, "11-column index must be gone after the full migration chain (000155)") + require.False(t, oldIdx, "11-column index must be gone after the full migration chain (000156)") savingsByAZConfig := func(t *testing.T) map[string]float64 { t.Helper() diff --git a/internal/config/store_postgres_suppressions.go b/internal/config/store_postgres_suppressions.go index 16562f1ae..88dbcf108 100644 --- a/internal/config/store_postgres_suppressions.go +++ b/internal/config/store_postgres_suppressions.go @@ -106,7 +106,7 @@ func (s *PostgresStore) CreateSuppressionTx(ctx context.Context, tx pgx.Tx, sup // 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). - // az_config is NULL when empty: unknown or not applicable (migration 000156). + // az_config is NULL when empty: unknown or not applicable (migration 000155). var az, platform, tenancy, azConfig *string if sup.Variant != nil { az, platform, tenancy = &sup.Variant.AvailabilityZone, &sup.Variant.Platform, &sup.Variant.Tenancy @@ -204,7 +204,7 @@ func (s *PostgresStore) ListActiveSuppressions(ctx context.Context) ([]PurchaseS // 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. A NULL az_config (rows written before -// migration 000156, or a rec with none) is read as "" = any az_config. +// migration 000155, or a rec with none) is read as "" = any az_config. func suppressionVariantFromColumns(az, platform, tenancy, azConfig *string) *SuppressionVariant { if az == nil || platform == nil || tenancy == nil { return nil diff --git a/internal/config/types.go b/internal/config/types.go index b2bda9c18..4c3e88209 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -779,7 +779,7 @@ type PurchaseSuppression struct { // #806, #408). Empty AvailabilityZone, Platform and Tenancy are known values // (a regional rec, a non-EC2 rec), not wildcards. AZConfig is the exception: // "" means "not applicable or unknown" and matches any az_config (see -// AppliesTo), because suppressions written before migration 000156 have none +// AppliesTo), because suppressions written before migration 000155 have none // and must keep suppressing both the Single-AZ and the Multi-AZ rec. type SuppressionVariant struct { AvailabilityZone string `json:"availability_zone"` diff --git a/internal/database/postgres/migrations/000154_recommendations_az_config.up.sql b/internal/database/postgres/migrations/000154_recommendations_az_config.up.sql index ca0bdbda3..9b418c5e2 100644 --- a/internal/database/postgres/migrations/000154_recommendations_az_config.up.sql +++ b/internal/database/postgres/migrations/000154_recommendations_az_config.up.sql @@ -25,13 +25,13 @@ -- - recommendations_natural_key_source_azc_idx: the source-account key of -- migration 000120 (#799) plus az_config. Every unique index without -- az_config rejects a Single-AZ/Multi-AZ pair, so the source-aware key has --- to gain the column too; 000155 retires the 000120 index it replaces. +-- to gain the column too; 000156 retires the 000120 index it replaces. -- -- The existing 11-column recommendations_natural_key_az_tp_idx and the -- 000120 recommendations_natural_key_source_idx are KEPT, so the current -- ON CONFLICT on the 11 columns keeps working for old and new code. Two rows -- that differ only by az_config are still rejected by those indexes until --- migration 000155 drops them (applied only once the store upserts on the +-- migration 000156 drops them (applied only once the store upserts on the -- 12-column index). -- -- Lock impact: ADD COLUMN ... GENERATED ... STORED rewrites the table and the diff --git a/internal/database/postgres/migrations/000154_recommendations_az_config_test.go b/internal/database/postgres/migrations/000154_recommendations_az_config_test.go index 0d2e4a8d5..658b119c0 100644 --- a/internal/database/postgres/migrations/000154_recommendations_az_config_test.go +++ b/internal/database/postgres/migrations/000154_recommendations_az_config_test.go @@ -117,7 +117,7 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*) FROM recommendations WHERE resource_type = 'pair'`).Scan(&n)) assert.Equal(t, 1, n) - // Known limitation, resolved by 000155: a row differing only by az_config + // Known limitation, resolved by 000156: a row differing only by az_config // is still rejected by the kept indexes without az_config. _, err = pool.Exec(ctx, insertRecForMigration152, "pair", `{"details":{"az_config":"multi-az"}}`) var pgErr *pgconn.PgError diff --git a/internal/database/postgres/migrations/000156_purchase_suppressions_az_config.down.sql b/internal/database/postgres/migrations/000155_purchase_suppressions_az_config.down.sql similarity index 97% rename from internal/database/postgres/migrations/000156_purchase_suppressions_az_config.down.sql rename to internal/database/postgres/migrations/000155_purchase_suppressions_az_config.down.sql index 106655e80..8b0feb167 100644 --- a/internal/database/postgres/migrations/000156_purchase_suppressions_az_config.down.sql +++ b/internal/database/postgres/migrations/000155_purchase_suppressions_az_config.down.sql @@ -1,4 +1,4 @@ --- Rollback of 000156: restore the 000134 suppression key without az_config. +-- Rollback of 000155: restore the 000134 suppression key without az_config. -- -- Rows of one execution that differ only by az_config are merged into one row -- per execution, 6-tuple and variant before the narrower UNIQUE constraint is diff --git a/internal/database/postgres/migrations/000156_purchase_suppressions_az_config.up.sql b/internal/database/postgres/migrations/000155_purchase_suppressions_az_config.up.sql similarity index 77% rename from internal/database/postgres/migrations/000156_purchase_suppressions_az_config.up.sql rename to internal/database/postgres/migrations/000155_purchase_suppressions_az_config.up.sql index 12778d0b1..ea72129c7 100644 --- a/internal/database/postgres/migrations/000156_purchase_suppressions_az_config.up.sql +++ b/internal/database/postgres/migrations/000155_purchase_suppressions_az_config.up.sql @@ -1,7 +1,7 @@ --- Migration 000156 (issue #408): widen the purchase suppression variant with +-- Migration 000155 (issue #408): widen the purchase suppression variant with -- the AWS RDS az_config ("single-az" / "multi-az"). -- --- Migration 000155 lets a Single-AZ and a Multi-AZ RDS rec of the same class, +-- Migration 000156 lets a Single-AZ and a Multi-AZ RDS rec of the same class, -- engine, term and payment coexist, but the suppression key (000134) cannot -- tell them apart: buying 2x Single-AZ would write a suppression that also -- matched, and hid, the Multi-AZ rec. The variant now carries az_config for @@ -20,10 +20,12 @@ -- so NULL rows stay unique per execution, 6-tuple and variant as before). -- A suppression with an az_config must also carry the rest of the variant. -- --- DEPLOY ORDER: apply this before deploying the code that writes az_config --- (the insert names the column). Apply 000154, 000155 and this migration as --- separate steps, each after the code it needs is live (see the 000155 header); --- never dispatch database-migration.yml with the default of all pending steps. +-- DEPLOY ORDER: apply this (and 000154) before deploying the code that writes +-- az_config (the insert names the column, and the purchase path and the +-- suppression listing need it). Both are expand steps. 000156 is the contract +-- step and must wait until that code is live everywhere (see the 000156 +-- header); never dispatch database-migration.yml with the default of all +-- pending steps once 000156 is pending. ALTER TABLE purchase_suppressions ADD COLUMN IF NOT EXISTS az_config TEXT; diff --git a/internal/database/postgres/migrations/000156_purchase_suppressions_az_config_test.go b/internal/database/postgres/migrations/000155_purchase_suppressions_az_config_test.go similarity index 87% rename from internal/database/postgres/migrations/000156_purchase_suppressions_az_config_test.go rename to internal/database/postgres/migrations/000155_purchase_suppressions_az_config_test.go index f139a4520..9f19f4dfd 100644 --- a/internal/database/postgres/migrations/000156_purchase_suppressions_az_config_test.go +++ b/internal/database/postgres/migrations/000155_purchase_suppressions_az_config_test.go @@ -13,9 +13,9 @@ import ( ) const ( - exec156 = "00000000-0000-0000-0000-000000156a01" + exec155 = "00000000-0000-0000-0000-000000155a01" - insertRDSSuppression156 = ` + insertRDSSuppression155 = ` INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, resource_type, engine, suppressed_count, expires_at, availability_zone, platform, tenancy, az_config) @@ -23,7 +23,7 @@ const ( NOW() + INTERVAL '7 days', '', '', '', $3)` ) -// TestMigration_PurchaseSuppressionsAZConfig locks down migration 000156 +// TestMigration_PurchaseSuppressionsAZConfig locks down migration 000155 // (issue #408): rows seeded before the migration, with and without a 000134 // variant, survive it with a NULL az_config (the wildcard backfill rule); // afterwards Single-AZ and Multi-AZ rows of one execution coexist while a @@ -35,7 +35,7 @@ func TestMigration_PurchaseSuppressionsAZConfig(t *testing.T) { ctx := context.Background() container := testhelpers.RequireEmptyDatabase(ctx, t) pool, path := container.DB.Pool(), getMigrationsPath() - prev := previousMigrationVersion(t, 156) + prev := previousMigrationVersion(t, 155) require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) // Pre-migration rows: one legacy 6-tuple row (NULL variant) and one RDS @@ -43,35 +43,35 @@ func TestMigration_PurchaseSuppressionsAZConfig(t *testing.T) { _, err := pool.Exec(ctx, ` INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, resource_type, engine, suppressed_count, expires_at) - VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.t3.micro', 'mysql', 1, NOW() + INTERVAL '7 days')`, exec156) + VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.t3.micro', 'mysql', 1, NOW() + INTERVAL '7 days')`, exec155) require.NoError(t, err) _, err = pool.Exec(ctx, ` 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', 'relational-db', 'us-east-1', 'db.r5.large', 'mysql', 2, NOW() + INTERVAL '7 days', '', '', '')`, exec156) + VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.r5.large', 'mysql', 2, NOW() + INTERVAL '7 days', '', '', '')`, exec155) require.NoError(t, err) - require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 156)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 155)) var rows, nulls int require.NoError(t, pool.QueryRow(ctx, `SELECT count(*), count(*) FILTER (WHERE az_config IS NULL) - FROM purchase_suppressions WHERE execution_id = $1`, exec156).Scan(&rows, &nulls)) + FROM purchase_suppressions WHERE execution_id = $1`, exec155).Scan(&rows, &nulls)) assert.Equal(t, 2, rows, "existing rows survive") assert.Equal(t, 2, nulls, "existing rows get a NULL az_config: a wildcard") // Single-AZ and Multi-AZ rows coexist next to the NULL row of the same variant. for _, v := range []string{"single-az", "multi-az"} { - _, err = pool.Exec(ctx, insertRDSSuppression156, exec156, 3, v) + _, err = pool.Exec(ctx, insertRDSSuppression155, exec155, 3, v) require.NoError(t, err, v) } - _, err = pool.Exec(ctx, insertRDSSuppression156, exec156, 3, "multi-az") + _, err = pool.Exec(ctx, insertRDSSuppression155, exec155, 3, "multi-az") requireUniqueViolation134(t, err, "purchase_suppressions_execution_variant_azc_key") // An az_config without the rest of the variant is rejected. _, err = pool.Exec(ctx, ` INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, resource_type, engine, suppressed_count, expires_at, az_config) - VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.m5.large', 'mysql', 1, NOW() + INTERVAL '7 days', 'multi-az')`, exec156) + VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.m5.large', 'mysql', 1, NOW() + INTERVAL '7 days', 'multi-az')`, exec155) require.Error(t, err) assert.Contains(t, err.Error(), "purchase_suppressions_az_config_needs_variant") @@ -80,20 +80,20 @@ func TestMigration_PurchaseSuppressionsAZConfig(t *testing.T) { require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) var total int require.NoError(t, pool.QueryRow(ctx, `SELECT count(*), COALESCE(SUM(suppressed_count), 0) - FROM purchase_suppressions WHERE execution_id = $1 AND resource_type = 'db.r5.large'`, exec156).Scan(&rows, &total)) + FROM purchase_suppressions WHERE execution_id = $1 AND resource_type = 'db.r5.large'`, exec155).Scan(&rows, &total)) assert.Equal(t, 1, rows) assert.Equal(t, 8, total) require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM purchase_suppressions - WHERE execution_id = $1 AND resource_type = 'db.t3.micro'`, exec156).Scan(&rows)) + WHERE execution_id = $1 AND resource_type = 'db.t3.micro'`, exec155).Scan(&rows)) assert.Equal(t, 1, rows) var cols int require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM information_schema.columns WHERE table_name = 'purchase_suppressions' AND column_name = 'az_config'`).Scan(&cols)) assert.Zero(t, cols) - _, err = pool.Exec(ctx, insertVariantSuppression134, exec156, 1, "us-east-1a", "Linux/UNIX", "default") + _, err = pool.Exec(ctx, insertVariantSuppression134, exec155, 1, "us-east-1a", "Linux/UNIX", "default") require.NoError(t, err) - _, err = pool.Exec(ctx, insertVariantSuppression134, exec156, 1, "us-east-1a", "Linux/UNIX", "default") + _, err = pool.Exec(ctx, insertVariantSuppression134, exec155, 1, "us-east-1a", "Linux/UNIX", "default") requireUniqueViolation134(t, err, "purchase_suppressions_execution_variant_key") - require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 156), "up must re-apply cleanly after down") + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 155), "up must re-apply cleanly after down") } diff --git a/internal/database/postgres/migrations/000155_drop_recommendations_11col_natural_key.down.sql b/internal/database/postgres/migrations/000156_drop_recommendations_11col_natural_key.down.sql similarity index 95% rename from internal/database/postgres/migrations/000155_drop_recommendations_11col_natural_key.down.sql rename to internal/database/postgres/migrations/000156_drop_recommendations_11col_natural_key.down.sql index 4a9b3bd28..52b3f43ba 100644 --- a/internal/database/postgres/migrations/000155_drop_recommendations_11col_natural_key.down.sql +++ b/internal/database/postgres/migrations/000156_drop_recommendations_11col_natural_key.down.sql @@ -1,4 +1,4 @@ --- Rollback of 000155: recreate the 11-column unique index exactly as defined +-- Rollback of 000156: recreate the 11-column unique index exactly as defined -- by 000111 and the source-account index exactly as defined by 000120. -- -- Only safe together with a code rollback to the 11-column upsert. The newer diff --git a/internal/database/postgres/migrations/000155_drop_recommendations_11col_natural_key.up.sql b/internal/database/postgres/migrations/000156_drop_recommendations_11col_natural_key.up.sql similarity index 96% rename from internal/database/postgres/migrations/000155_drop_recommendations_11col_natural_key.up.sql rename to internal/database/postgres/migrations/000156_drop_recommendations_11col_natural_key.up.sql index 619c49d04..a6bb12332 100644 --- a/internal/database/postgres/migrations/000155_drop_recommendations_11col_natural_key.up.sql +++ b/internal/database/postgres/migrations/000156_drop_recommendations_11col_natural_key.up.sql @@ -1,4 +1,4 @@ --- Migration 000155: drop the 11-column recommendations_natural_key_az_tp_idx +-- Migration 000156: drop the 11-column recommendations_natural_key_az_tp_idx -- and the 12-column recommendations_natural_key_source_idx (#408, contract step -- of the RDS AZ-config identity work). -- diff --git a/internal/database/postgres/migrations/000155_drop_recommendations_11col_natural_key_test.go b/internal/database/postgres/migrations/000156_drop_recommendations_11col_natural_key_test.go similarity index 97% rename from internal/database/postgres/migrations/000155_drop_recommendations_11col_natural_key_test.go rename to internal/database/postgres/migrations/000156_drop_recommendations_11col_natural_key_test.go index 2115fb967..e78565eac 100644 --- a/internal/database/postgres/migrations/000155_drop_recommendations_11col_natural_key_test.go +++ b/internal/database/postgres/migrations/000156_drop_recommendations_11col_natural_key_test.go @@ -20,7 +20,7 @@ const insertRecForMigration153 = ` VALUES ($1::timestamptz, 'aws', 'rds', $2, 'db.r5.large', 'mysql', $3::jsonb, 0, 0, 1, 'all-upfront') ` -// TestMigration_DropRecommendations11ColNaturalKey locks down migration 000155 +// TestMigration_DropRecommendations11ColNaturalKey locks down migration 000156 // (issue #408, contract step): Single-AZ and Multi-AZ rows of the same class // coexist once the 11-column and 000120 source indexes are gone, the 12-column index still dedupes, // and the rollback recreates both indexes after collapsing those @@ -51,7 +51,7 @@ func TestMigration_DropRecommendations11ColNaturalKey(t *testing.T) { none = `{"details":{"engine":"mysql"}}` ) - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 155))) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 156))) require.True(t, indexExists("recommendations_natural_key_az_tp_idx")) require.True(t, indexExists("recommendations_natural_key_az_tp_azc_idx")) require.True(t, indexExists("recommendations_natural_key_source_idx")) @@ -62,7 +62,7 @@ func TestMigration_DropRecommendations11ColNaturalKey(t *testing.T) { _, err = pool.Exec(ctx, insertRecForMigration153, "2026-01-01T00:00:00Z", "us-west-2", single) require.NoError(t, err) - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 155)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 156)) assert.False(t, indexExists("recommendations_natural_key_az_tp_idx"), "11-column index must be dropped") assert.True(t, indexExists("recommendations_natural_key_az_tp_azc_idx"), "12-column index must remain") assert.False(t, indexExists("recommendations_natural_key_source_idx"), "000120's source index (no az_config) must be dropped") @@ -81,7 +81,7 @@ func TestMigration_DropRecommendations11ColNaturalKey(t *testing.T) { // Rollback: az_config-only duplicates collapse to the newest, the other // region's row is untouched, and the 11-column index is recreated with // its original shape. - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 155))) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 156))) assert.Equal(t, 1, count("us-east-1"), "only duplicates under the 11-column key may be deleted") assert.Equal(t, 1, count("us-west-2")) var kept string @@ -100,7 +100,7 @@ func TestMigration_DropRecommendations11ColNaturalKey(t *testing.T) { assert.Contains(t, indexDef, "(account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, source_account_key)") // Re-applying the up migration after a rollback works. - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 155)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 156)) assert.False(t, indexExists("recommendations_natural_key_az_tp_idx")) assert.False(t, indexExists("recommendations_natural_key_source_idx")) } From 908e738eff39b1923b173be1c346879f704b0c19 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 19:22:18 +0100 Subject: [PATCH 12/19] feat(config): gate az_config code on the expand migrations New code that writes az_config failed mid-transaction on a database without migrations 000154 and 000155. Generalise checkAzurePricingMigration into checkSchemaVersion and add CheckAZConfigMigration, which returns the typed ErrAZConfigMigrationNotReady naming the missing migrations. The purchase path refuses with a 503 before it opens its transaction. Recommendation writes and suppression listing return the same error, wrapped with what was skipped, instead of a SQL failure. Co-Authored-By: Claude Opus 5.5 --- internal/api/az_config_gate.go | 35 ++++++++++ .../api/azconfig_gate_integration_test.go | 67 +++++++++++++++++++ internal/api/handler_purchases.go | 6 ++ internal/config/errors.go | 5 ++ .../config/store_postgres_list_caps_test.go | 1 + .../config/store_postgres_pgxmock_test.go | 7 ++ .../config/store_postgres_recommendations.go | 29 ++++++-- .../config/store_postgres_rollback_test.go | 23 ++++--- .../config/store_postgres_suppressions.go | 3 + .../store_postgres_suppressions_test.go | 2 + 10 files changed, 166 insertions(+), 12 deletions(-) create mode 100644 internal/api/az_config_gate.go create mode 100644 internal/api/azconfig_gate_integration_test.go diff --git a/internal/api/az_config_gate.go b/internal/api/az_config_gate.go new file mode 100644 index 000000000..012244e76 --- /dev/null +++ b/internal/api/az_config_gate.go @@ -0,0 +1,35 @@ +package api + +import ( + "context" + "errors" + "fmt" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// azConfigSchemaChecker is implemented by the Postgres store. Stores without a +// schema (test doubles) have nothing to check. +type azConfigSchemaChecker interface { + CheckAZConfigMigration(ctx context.Context) error +} + +// requireAZConfigSchema refuses a purchase submission before its transaction +// opens when the az_config expand migrations (000154, 000155) are missing: the +// suppression insert names az_config, so it would fail mid-transaction. +func (h *Handler) requireAZConfigSchema(ctx context.Context) error { + checker, ok := h.config.(azConfigSchemaChecker) + if !ok { + return nil + } + err := checker.CheckAZConfigMigration(ctx) + if errors.Is(err, config.ErrAZConfigMigrationNotReady) { + return NewClientErrorWithDetails(503, + fmt.Sprintf("purchases are unavailable until database migrations 000154 and 000155 are applied: %v", err), + map[string]any{}) + } + if err != nil { + return fmt.Errorf("check az_config migration: %w", err) + } + return nil +} diff --git a/internal/api/azconfig_gate_integration_test.go b/internal/api/azconfig_gate_integration_test.go new file mode 100644 index 000000000..8b92f395e --- /dev/null +++ b/internal/api/azconfig_gate_integration_test.go @@ -0,0 +1,67 @@ +//go:build integration + +package api + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "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" +) + +// txCountingStore counts the transactions the purchase path opens. +type txCountingStore struct { + *config.PostgresStore + txs int +} + +func (s *txCountingStore) WithTx(ctx context.Context, fn func(pgx.Tx) error) error { + s.txs++ + return s.PostgresStore.WithTx(ctx, fn) +} + +// Without the az_config expand migrations the new code must refuse loudly: +// the purchase path before it opens a transaction, collection and suppression +// listing with the typed error, not a SQL failure. +func TestAZConfigGate_RefusesBeforeExpandMigrations(t *testing.T) { + ctx := context.Background() + pg := testhelpers.RequireEmptyDatabase(ctx, t) + pool, path := pg.DB.Pool(), getMigrationsPath() + store := &txCountingStore{PostgresStore: config.NewPostgresStore(pg.DB)} + h := &Handler{config: store} + exec := &config.PurchaseExecution{ExecutionID: uuid.NewString()} + + for _, version := range []uint{153, 154} { // before both expands, then after only the first + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) + + _, err := h.persistExecutionAndSuppressions(ctx, exec, nil, "creator", "key") + ce, ok := IsClientError(err) + require.True(t, ok, "version %d: %v", version, err) + assert.Equal(t, 503, ce.code) + assert.Contains(t, ce.message, "000154 and 000155") + assert.Zero(t, store.txs, "no transaction may open before the gate") + + rec := expandOrderRDSRec("single-az", 100) + err = store.UpsertRecommendations(ctx, time.Now(), []config.RecommendationRecord{rec}, []config.SuccessfulCollect{{Provider: "aws"}}) + assert.True(t, errors.Is(err, config.ErrAZConfigMigrationNotReady), "collection: %v", err) + + _, err = store.ListActiveSuppressions(ctx) + assert.True(t, errors.Is(err, config.ErrAZConfigMigrationNotReady), "suppression listing: %v", err) + } + + for _, version := range []uint{155, 156} { + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) + require.NoError(t, h.requireAZConfigSchema(ctx), "version %d", version) + _, err := store.ListActiveSuppressions(ctx) + require.NoError(t, err, "version %d", version) + } +} diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index df6de50b5..b8d8569ef 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2805,6 +2805,9 @@ func (h *Handler) persistRetryExecution(ctx context.Context, failedExec *config. // The successor is a web-source pending row for the retrying user, so it // also goes through the submit dedupe (issue #760): see writeRetrySuccessorTx. func (h *Handler) persistRetrySuccessorTx(ctx context.Context, failedExec, newExecution *config.PurchaseExecution, suppressions []config.PurchaseSuppression) error { + if err := h.requireAZConfigSchema(ctx); err != nil { + return err + } err := h.config.WithTx(ctx, func(tx pgx.Tx) error { return h.writeRetrySuccessorTx(ctx, tx, failedExec, newExecution, suppressions) }) @@ -3639,6 +3642,9 @@ func (h *Handler) persistExecutionAndSuppressions( suppressions []config.PurchaseSuppression, creatorID, idempotencyKey string, ) (dup *config.PurchaseExecution, err error) { + if err := h.requireAZConfigSchema(ctx); err != nil { + return nil, err + } txErr := h.config.WithTx(ctx, func(tx pgx.Tx) error { // Duplicate check inside the tx, after taking the creator's submit // lock: a concurrent identical submit waits for this tx to commit, diff --git a/internal/config/errors.go b/internal/config/errors.go index 32dcb0901..9c72fda93 100644 --- a/internal/config/errors.go +++ b/internal/config/errors.go @@ -4,6 +4,11 @@ import "errors" var ErrAzurePricingNotReady = errors.New("azure pricing migration not ready") +// ErrAZConfigMigrationNotReady means migrations 000154 (recommendations +// az_config) and 000155 (purchase_suppressions az_config) are not both applied. +// Apply them before running a release that reads or writes az_config. +var ErrAZConfigMigrationNotReady = errors.New("az_config migrations 000154 and 000155 are not applied") + // ErrNotFound is returned when a requested config-store row does not exist. var ErrNotFound = errors.New("not found") diff --git a/internal/config/store_postgres_list_caps_test.go b/internal/config/store_postgres_list_caps_test.go index f4725c0a0..cfc44b89b 100644 --- a/internal/config/store_postgres_list_caps_test.go +++ b/internal/config/store_postgres_list_caps_test.go @@ -84,6 +84,7 @@ func suppressionRows(n int) *pgxmock.Rows { func TestPGXMock_ListActiveSuppressions_ReturnsEveryRowAboveMaxListLimit(t *testing.T) { mock := newMock(t) store := storeWith(mock) + expectAZConfigReady(mock) mock.ExpectQuery(`(?s)FROM purchase_suppressions\s+WHERE expires_at > NOW\(\)\s*$`). WillReturnRows(suppressionRows(MaxListLimit + 5)) diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index ab640f0ac..435586a8d 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -29,6 +29,13 @@ func newMock(t *testing.T) pgxmock.PgxPoolIface { return mock } +// expectAZConfigReady primes the schema gate that UpsertRecommendations and +// ListActiveSuppressions run first, reporting the fully migrated version. +func expectAZConfigReady(mock pgxmock.PgxPoolIface) { + mock.ExpectQuery("SELECT version, dirty FROM schema_migrations"). + WillReturnRows(pgxmock.NewRows([]string{"version", "dirty"}).AddRow(156, false)) +} + // anyArgs returns n AnyArg() matchers. func anyArgsCfg(n int) []interface{} { args := make([]interface{}, n) diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index 0cf627b1c..de745b803 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -23,6 +23,10 @@ const recommendationsBatchSize = 500 const azurePricingMigrationVersion = 106 +// azConfigExpandMigrationVersion is the last az_config expand migration +// (000155); the contract step 000156 is not required. +const azConfigExpandMigrationVersion = 155 + // UpsertRecommendations is the incremental write path: it upserts each row // by natural key (cloud_account_id, provider, service, region, // resource_type, engine, term, payment_option, availability_zone, platform, @@ -56,6 +60,9 @@ func (s *PostgresStore) UpsertRecommendations(ctx context.Context, collectedAt t // carryAzureHold), so overlapping collections cannot drop each other's hold // and a failed read or write rolls the rows back with the marker. func (s *PostgresStore) UpsertRecommendationsWithPartial(ctx context.Context, collectedAt time.Time, recs []RecommendationRecord, successfulCollects []SuccessfulCollect, partial *RecommendationsCollectionPartial) error { + if err := s.CheckAZConfigMigration(ctx); err != nil { + return fmt.Errorf("recommendation write skipped: %w", err) + } tx, err := s.db.Begin(ctx) if err != nil { return fmt.Errorf("failed to begin tx: %w", err) @@ -409,17 +416,31 @@ func (s *PostgresStore) checkAzurePricingMigration(ctx context.Context, required if !required { return nil } + return s.checkSchemaVersion(ctx, azurePricingMigrationVersion, ErrAzurePricingNotReady, "Azure pricing") +} + +// CheckAZConfigMigration returns ErrAZConfigMigrationNotReady (wrapped, with +// the schema version found) until both az_config expand migrations (000154 +// recommendations, 000155 purchase_suppressions) are applied. +func (s *PostgresStore) CheckAZConfigMigration(ctx context.Context) error { + return s.checkSchemaVersion(ctx, azConfigExpandMigrationVersion, ErrAZConfigMigrationNotReady, "az_config") +} + +// checkSchemaVersion fails with notReady when schema_migrations is empty, +// dirty or below minVersion. Migrations apply in order, so the version alone +// says every earlier one ran. +func (s *PostgresStore) checkSchemaVersion(ctx context.Context, minVersion int, notReady error, what string) error { var version int var dirty bool err := s.db.QueryRow(ctx, `SELECT version, dirty FROM schema_migrations`).Scan(&version, &dirty) if errors.Is(err, pgx.ErrNoRows) { - return ErrAzurePricingNotReady + return notReady } if err != nil { - return fmt.Errorf("check Azure pricing migration: %w", err) + return fmt.Errorf("check %s migration: %w", what, err) } - if version < azurePricingMigrationVersion || dirty { - return ErrAzurePricingNotReady + if version < minVersion || dirty { + return fmt.Errorf("%w: schema version %d (dirty=%t), need >= %d", notReady, version, dirty, minVersion) } return nil } diff --git a/internal/config/store_postgres_rollback_test.go b/internal/config/store_postgres_rollback_test.go index 98790a205..3c7e10553 100644 --- a/internal/config/store_postgres_rollback_test.go +++ b/internal/config/store_postgres_rollback_test.go @@ -38,23 +38,24 @@ func captureConfigLogs(t *testing.T, level logging.Level) *bytes.Buffer { // is primed with only Begin and Rollback, so the first statement inside the // transaction fails as unexpected and the deferred rollback runs. var rollbackSites = []struct { - name string - op string - run func(ctx context.Context, s *PostgresStore) error + name string + op string + gated bool // runs the az_config schema gate before Begin + run func(ctx context.Context, s *PostgresStore) error }{ - {"DeleteCloudAccount", "DeleteCloudAccount", func(ctx context.Context, s *PostgresStore) error { + {"DeleteCloudAccount", "DeleteCloudAccount", false, func(ctx context.Context, s *PostgresStore) error { return s.DeleteCloudAccount(ctx, "11111111-1111-1111-1111-111111111111") }}, - {"SetPlanAccounts", "SetPlanAccounts", func(ctx context.Context, s *PostgresStore) error { + {"SetPlanAccounts", "SetPlanAccounts", false, func(ctx context.Context, s *PostgresStore) error { return s.SetPlanAccounts(ctx, "22222222-2222-2222-2222-222222222222", nil) }}, - {"UpsertRecommendations", "UpsertRecommendations", func(ctx context.Context, s *PostgresStore) error { + {"UpsertRecommendations", "UpsertRecommendations", true, func(ctx context.Context, s *PostgresStore) error { return s.UpsertRecommendations(ctx, time.Now(), nil, nil) }}, - {"WithTx", "WithTx", func(ctx context.Context, s *PostgresStore) error { + {"WithTx", "WithTx", false, func(ctx context.Context, s *PostgresStore) error { return s.WithTx(ctx, func(pgx.Tx) error { return errors.New("user code failed") }) }}, - {"withLadderConfigLock", "ladder config lock cfg-1", func(ctx context.Context, s *PostgresStore) error { + {"withLadderConfigLock", "ladder config lock cfg-1", false, func(ctx context.Context, s *PostgresStore) error { return s.withLadderConfigLock(ctx, "cfg-1", func(pgx.Tx) error { return nil }) }}, } @@ -65,6 +66,9 @@ func TestTxRollbackFailureIsLogged(t *testing.T) { // Capture at Warn so a downgrade of the log level is caught. buf := captureConfigLogs(t, logging.LevelWarn) mock := newMock(t) + if tc.gated { + expectAZConfigReady(mock) + } mock.ExpectBegin() rbErr := errors.New("conn reset by peer") mock.ExpectRollback().WillReturnError(rbErr) @@ -87,6 +91,9 @@ func TestTxRollbackAfterCloseIsQuiet(t *testing.T) { t.Run(tc.name, func(t *testing.T) { buf := captureConfigLogs(t, logging.LevelDebug) mock := newMock(t) + if tc.gated { + expectAZConfigReady(mock) + } mock.ExpectBegin() mock.ExpectRollback().WillReturnError(pgx.ErrTxClosed) diff --git a/internal/config/store_postgres_suppressions.go b/internal/config/store_postgres_suppressions.go index 88dbcf108..369a66f7f 100644 --- a/internal/config/store_postgres_suppressions.go +++ b/internal/config/store_postgres_suppressions.go @@ -166,6 +166,9 @@ func (s *PostgresStore) DeleteSuppressionsByExecutionTx(ctx context.Context, tx // days, so normal use can exceed MaxListLimit. A capped read would either // under-suppress (re-offering capacity just bought) or fail every caller. func (s *PostgresStore) ListActiveSuppressions(ctx context.Context) ([]PurchaseSuppression, error) { + if err := s.CheckAZConfigMigration(ctx); err != nil { + return nil, fmt.Errorf("suppression listing skipped: %w", err) + } const q = ` SELECT id, execution_id, account_id, provider, service, region, resource_type, engine, suppressed_count, expires_at, created_at, diff --git a/internal/config/store_postgres_suppressions_test.go b/internal/config/store_postgres_suppressions_test.go index 230cac325..26c0f208e 100644 --- a/internal/config/store_postgres_suppressions_test.go +++ b/internal/config/store_postgres_suppressions_test.go @@ -128,6 +128,7 @@ func TestPGXMock_ListActiveSuppressions(t *testing.T) { "t4g.nano", "", 3, future, now, nil, nil, nil, nil). AddRow("sup-2", "exec-2", "acct-1", "aws", "ec2", "us-east-1", "t4g.nano", "", 2, future, now, &az, &platform, &tenancy, nil) + expectAZConfigReady(mock) mock.ExpectQuery(`SELECT .* FROM purchase_suppressions WHERE expires_at > NOW\(\)`). WillReturnRows(rows) @@ -204,6 +205,7 @@ func TestPGXMock_Suppression_AZConfigRoundTrip(t *testing.T) { rows := pgxmock.NewRows(cols). AddRow("a", "e", "acct-1", "aws", "relational-db", "us-east-1", "db.r5.large", "mysql", 2, now, now, &az, &platform, &tenancy, nil). AddRow("b", "e", "acct-1", "aws", "relational-db", "us-east-1", "db.r5.large", "mysql", 2, now, now, &az, &platform, &tenancy, &azConfig) + expectAZConfigReady(mock) mock.ExpectQuery(`SELECT .* FROM purchase_suppressions`).WillReturnRows(rows) sups, err := store.ListActiveSuppressions(ctx) require.NoError(t, err) From 341be900ad8f6833c4d72e247f03bb272cd8b754 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 20:03:34 +0100 Subject: [PATCH 13/19] fix(config): gate suppression writes on the az_config expand CreateSuppressionTx inserts az_config, so every caller (the plan step claim, attach recommendations, CreateSuppression) failed with a raw SQL error below migration 000155. Check the schema version at the top of CreateSuppressionTx and return the typed ErrAZConfigMigrationNotReady, so the transaction rolls back cleanly for all callers. The handler keeps its pre-transaction 503. Co-Authored-By: Claude Opus 5.5 --- internal/api/azconfig_gate_integration_test.go | 8 ++++++++ internal/config/store_postgres_suppressions.go | 3 +++ internal/config/store_postgres_suppressions_test.go | 3 +++ 3 files changed, 14 insertions(+) diff --git a/internal/api/azconfig_gate_integration_test.go b/internal/api/azconfig_gate_integration_test.go index 8b92f395e..a5800324a 100644 --- a/internal/api/azconfig_gate_integration_test.go +++ b/internal/api/azconfig_gate_integration_test.go @@ -56,6 +56,14 @@ func TestAZConfigGate_RefusesBeforeExpandMigrations(t *testing.T) { _, err = store.ListActiveSuppressions(ctx) assert.True(t, errors.Is(err, config.ErrAZConfigMigrationNotReady), "suppression listing: %v", err) + + sup := &config.PurchaseSuppression{ExecutionID: uuid.NewString(), Provider: "aws", Service: "rds", Region: "us-east-1", + ResourceType: "db.r5.large", SuppressedCount: 1, ExpiresAt: time.Now().Add(time.Hour)} + err = store.CreateSuppression(ctx, sup) + assert.True(t, errors.Is(err, config.ErrAZConfigMigrationNotReady), "CreateSuppression: %v", err) + err = store.WithTx(ctx, func(tx pgx.Tx) error { return store.CreateSuppressionTx(ctx, tx, sup) }) + assert.True(t, errors.Is(err, config.ErrAZConfigMigrationNotReady), "CreateSuppressionTx: %v", err) + store.txs = 0 // the direct WithTx above is not the purchase path } for _, version := range []uint{155, 156} { diff --git a/internal/config/store_postgres_suppressions.go b/internal/config/store_postgres_suppressions.go index 369a66f7f..b6dee8cde 100644 --- a/internal/config/store_postgres_suppressions.go +++ b/internal/config/store_postgres_suppressions.go @@ -96,6 +96,9 @@ func validateSuppression(sup *PurchaseSuppression) error { // transaction. sup.ID is populated from the Postgres-generated UUID if // the caller left it blank. func (s *PostgresStore) CreateSuppressionTx(ctx context.Context, tx pgx.Tx, sup *PurchaseSuppression) error { + if err := s.CheckAZConfigMigration(ctx); err != nil { + return fmt.Errorf("suppression write refused: %w", err) + } if err := validateSuppression(sup); err != nil { return err } diff --git a/internal/config/store_postgres_suppressions_test.go b/internal/config/store_postgres_suppressions_test.go index 26c0f208e..a7feb1252 100644 --- a/internal/config/store_postgres_suppressions_test.go +++ b/internal/config/store_postgres_suppressions_test.go @@ -17,6 +17,7 @@ func TestPGXMock_CreateSuppression_RoundTrip(t *testing.T) { ctx := context.Background() mock.ExpectBegin() + expectAZConfigReady(mock) mock.ExpectExec(`INSERT INTO purchase_suppressions`). WithArgs(pgxmock.AnyArg(), "exec-1", "acct-1", "aws", "ec2", "us-east-1", "t4g.nano", "", 3, pgxmock.AnyArg(), (*string)(nil), (*string)(nil), (*string)(nil), (*string)(nil)). @@ -43,6 +44,7 @@ func TestPGXMock_CreateSuppression_WritesVariant(t *testing.T) { az, platform, tenancy := "", "Linux/UNIX", "default" mock.ExpectBegin() + expectAZConfigReady(mock) 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, (*string)(nil)). @@ -186,6 +188,7 @@ func TestPGXMock_Suppression_AZConfigRoundTrip(t *testing.T) { az, platform, tenancy, azConfig := "", "", "", "multi-az" mock.ExpectBegin() + expectAZConfigReady(mock) mock.ExpectExec(`INSERT INTO purchase_suppressions`). WithArgs(pgxmock.AnyArg(), "exec-1", "acct-1", "aws", "relational-db", "us-east-1", "db.r5.large", "mysql", 2, pgxmock.AnyArg(), &az, &platform, &tenancy, &azConfig). From 0321b8fd01f44157d7fa21a2cc097c20467eb649 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 20:31:52 +0100 Subject: [PATCH 14/19] test(purchase): migrate the Azure pricing fixture past the az_config expand The fixture stopped at 000154, but the recommendations upsert is now gated until the 000155 expand, so the re-collection step was refused. Migrate to 000155 and say why in the comment. Co-Authored-By: Claude Opus 5.5 --- internal/purchase/azure_pricing_integration_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/purchase/azure_pricing_integration_test.go b/internal/purchase/azure_pricing_integration_test.go index df64d7ad8..d953dcb96 100644 --- a/internal/purchase/azure_pricing_integration_test.go +++ b/internal/purchase/azure_pricing_integration_test.go @@ -98,9 +98,10 @@ func TestApproveAzurePricingRequiresMigrationAndRecollection(t *testing.T) { t.Run("106 invalidated old prices", func(t *testing.T) { approve(t, ErrStaleAzurePricing, 0) }) rec.UpfrontCost = 700 // The store's upsert needs the availability_zone, platform, tenancy and az_config columns - // and the 12-column index from migration 000154 (issue #408), and resets + // and the 12-column index from migration 000154 (issue #408), and is gated until the + // 000155 expand (CheckAZConfigMigration). It also resets // recommendations_state.last_collection_partial, added by migration 000129 (issue #87). - require.NoError(t, migrations.MigrateToVersion(ctx, pool, rampMigrationsPath(), 154)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, rampMigrationsPath(), 155)) require.NoError(t, pricing.UpsertRecommendations(ctx, time.Now().Add(time.Minute), []config.RecommendationRecord{rec}, collects)) t.Run("recollected prices purchase once", func(t *testing.T) { approve(t, nil, 1) }) } From ea6975f9419ea0554c6a64ee5f7c775f4f648e5d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 13:41:37 +0100 Subject: [PATCH 15/19] fix(config): run the suppression-write schema check on the caller's transaction CreateSuppressionTx queried the pool for the az_config schema version while the caller's transaction was open, taking a second pool connection per suppression row. Under the plan-step claim's advisory and row locks and a pool of 3 that can exhaust the pool. Take the check through a rowQuerier satisfied by both the pool and pgx.Tx, and run it on the tx. Co-Authored-By: Claude Opus 5.5 --- .../config/store_postgres_recommendations.go | 19 +++++++++--- .../config/store_postgres_suppressions.go | 2 +- .../store_postgres_suppressions_test.go | 30 +++++++++++++++++++ 3 files changed, 46 insertions(+), 5 deletions(-) diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index de745b803..a2250a789 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -416,23 +416,34 @@ func (s *PostgresStore) checkAzurePricingMigration(ctx context.Context, required if !required { return nil } - return s.checkSchemaVersion(ctx, azurePricingMigrationVersion, ErrAzurePricingNotReady, "Azure pricing") + return checkSchemaVersion(ctx, s.db, azurePricingMigrationVersion, ErrAzurePricingNotReady, "Azure pricing") } // CheckAZConfigMigration returns ErrAZConfigMigrationNotReady (wrapped, with // the schema version found) until both az_config expand migrations (000154 // recommendations, 000155 purchase_suppressions) are applied. func (s *PostgresStore) CheckAZConfigMigration(ctx context.Context) error { - return s.checkSchemaVersion(ctx, azConfigExpandMigrationVersion, ErrAZConfigMigrationNotReady, "az_config") + return checkSchemaVersion(ctx, s.db, azConfigExpandMigrationVersion, ErrAZConfigMigrationNotReady, "az_config") +} + +// checkAZConfigMigrationTx is CheckAZConfigMigration on the caller's open +// transaction, so a write inside a tx does not take a second pool connection. +func checkAZConfigMigrationTx(ctx context.Context, tx pgx.Tx) error { + return checkSchemaVersion(ctx, tx, azConfigExpandMigrationVersion, ErrAZConfigMigrationNotReady, "az_config") +} + +// rowQuerier is satisfied by both the pool and pgx.Tx. +type rowQuerier interface { + QueryRow(ctx context.Context, sql string, args ...any) pgx.Row } // checkSchemaVersion fails with notReady when schema_migrations is empty, // dirty or below minVersion. Migrations apply in order, so the version alone // says every earlier one ran. -func (s *PostgresStore) checkSchemaVersion(ctx context.Context, minVersion int, notReady error, what string) error { +func checkSchemaVersion(ctx context.Context, q rowQuerier, minVersion int, notReady error, what string) error { var version int var dirty bool - err := s.db.QueryRow(ctx, `SELECT version, dirty FROM schema_migrations`).Scan(&version, &dirty) + err := q.QueryRow(ctx, `SELECT version, dirty FROM schema_migrations`).Scan(&version, &dirty) if errors.Is(err, pgx.ErrNoRows) { return notReady } diff --git a/internal/config/store_postgres_suppressions.go b/internal/config/store_postgres_suppressions.go index b6dee8cde..a79dc6077 100644 --- a/internal/config/store_postgres_suppressions.go +++ b/internal/config/store_postgres_suppressions.go @@ -96,7 +96,7 @@ func validateSuppression(sup *PurchaseSuppression) error { // transaction. sup.ID is populated from the Postgres-generated UUID if // the caller left it blank. func (s *PostgresStore) CreateSuppressionTx(ctx context.Context, tx pgx.Tx, sup *PurchaseSuppression) error { - if err := s.CheckAZConfigMigration(ctx); err != nil { + if err := checkAZConfigMigrationTx(ctx, tx); err != nil { return fmt.Errorf("suppression write refused: %w", err) } if err := validateSuppression(sup); err != nil { diff --git a/internal/config/store_postgres_suppressions_test.go b/internal/config/store_postgres_suppressions_test.go index a7feb1252..2add294c0 100644 --- a/internal/config/store_postgres_suppressions_test.go +++ b/internal/config/store_postgres_suppressions_test.go @@ -217,3 +217,33 @@ func TestPGXMock_Suppression_AZConfigRoundTrip(t *testing.T) { assert.Equal(t, "multi-az", sups[1].Variant.AZConfig) assert.NoError(t, mock.ExpectationsWereMet()) } + +// poolQueryForbidden fails the test if the pool itself is queried: the +// suppression-write schema check must run on the caller's transaction, not take +// a second pool connection while the tx is open. +type poolQueryForbidden struct { + pgxmock.PgxPoolIface + t *testing.T +} + +func (p poolQueryForbidden) QueryRow(ctx context.Context, sql string, args ...any) pgx.Row { + p.t.Errorf("pool QueryRow used inside an open transaction: %s", sql) + return p.PgxPoolIface.QueryRow(ctx, sql, args...) +} + +func TestPGXMock_CreateSuppressionTx_SchemaCheckRunsOnTx(t *testing.T) { + mock := newMock(t) + store := &PostgresStore{db: poolQueryForbidden{PgxPoolIface: mock, t: t}} + mock.ExpectBegin() + expectAZConfigReady(mock) + mock.ExpectExec(`INSERT INTO purchase_suppressions`).WithArgs(anyArgsCfg(14)...). + WillReturnResult(pgxmock.NewResult("INSERT", 1)) + mock.ExpectCommit() + + err := store.CreateSuppression(context.Background(), &PurchaseSuppression{ + ExecutionID: "exec-1", Provider: "aws", Service: "ec2", SuppressedCount: 1, + ExpiresAt: time.Now().Add(time.Hour), + }) + require.NoError(t, err) + assert.NoError(t, mock.ExpectationsWereMet()) +} From a3784f4a8712f5dffaf0596284d580cdb43960ea Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 17:08:47 +0100 Subject: [PATCH 16/19] fix(db): renumber the az_config migrations to 000155-000157 after #1762 Main now has 000154_purchase_executions_first_dispatch_at. Move the recommendations az_config expand to 000155, the purchase_suppressions expand to 000156 and the contract to 000157, and update every reference: SQL headers, store comments, fixtures and gate test versions. The gate error and 503 message now build the migration numbers from two constants. Co-Authored-By: Claude Opus 5.5 --- internal/api/az_config_gate.go | 28 ++++++++++------ .../api/azconfig_gate_integration_test.go | 6 ++-- internal/config/errors.go | 21 +++++++++--- internal/config/recommendation_identity.go | 2 +- .../config/recommendation_identity_test.go | 2 +- .../config/store_postgres_recommendations.go | 10 +++--- ..._postgres_recommendations_azconfig_test.go | 4 +-- .../config/store_postgres_suppressions.go | 4 +-- internal/config/types.go | 2 +- ...000155_recommendations_az_config.down.sql} | 0 ...> 000155_recommendations_az_config.up.sql} | 6 ++-- ... 000155_recommendations_az_config_test.go} | 22 ++++++------- ..._purchase_suppressions_az_config.down.sql} | 2 +- ...56_purchase_suppressions_az_config.up.sql} | 12 +++---- ...6_purchase_suppressions_az_config_test.go} | 32 +++++++++---------- ...ecommendations_11col_natural_key.down.sql} | 4 +-- ..._recommendations_11col_natural_key.up.sql} | 4 +-- ...recommendations_11col_natural_key_test.go} | 10 +++--- .../azure_pricing_integration_test.go | 6 ++-- .../suppression_variant_integration_test.go | 2 +- 20 files changed, 100 insertions(+), 79 deletions(-) rename internal/database/postgres/migrations/{000154_recommendations_az_config.down.sql => 000155_recommendations_az_config.down.sql} (100%) rename internal/database/postgres/migrations/{000154_recommendations_az_config.up.sql => 000155_recommendations_az_config.up.sql} (94%) rename internal/database/postgres/migrations/{000154_recommendations_az_config_test.go => 000155_recommendations_az_config_test.go} (94%) rename internal/database/postgres/migrations/{000155_purchase_suppressions_az_config.down.sql => 000156_purchase_suppressions_az_config.down.sql} (97%) rename internal/database/postgres/migrations/{000155_purchase_suppressions_az_config.up.sql => 000156_purchase_suppressions_az_config.up.sql} (87%) rename internal/database/postgres/migrations/{000155_purchase_suppressions_az_config_test.go => 000156_purchase_suppressions_az_config_test.go} (87%) rename internal/database/postgres/migrations/{000156_drop_recommendations_11col_natural_key.down.sql => 000157_drop_recommendations_11col_natural_key.down.sql} (91%) rename internal/database/postgres/migrations/{000156_drop_recommendations_11col_natural_key.up.sql => 000157_drop_recommendations_11col_natural_key.up.sql} (92%) rename internal/database/postgres/migrations/{000156_drop_recommendations_11col_natural_key_test.go => 000157_drop_recommendations_11col_natural_key_test.go} (97%) diff --git a/internal/api/az_config_gate.go b/internal/api/az_config_gate.go index 012244e76..c2c799f13 100644 --- a/internal/api/az_config_gate.go +++ b/internal/api/az_config_gate.go @@ -14,22 +14,32 @@ type azConfigSchemaChecker interface { CheckAZConfigMigration(ctx context.Context) error } +// azConfigNotReadyError maps ErrAZConfigMigrationNotReady to the 503 every +// az_config-dependent path returns, and passes any other error through. +func azConfigNotReadyError(err error) error { + if !errors.Is(err, config.ErrAZConfigMigrationNotReady) { + return err + } + return NewClientErrorWithDetails(503, + fmt.Sprintf("database migrations %06d and %06d are not applied; recommendations and purchases are unavailable until they are", + config.AZConfigRecommendationsMigration, config.AZConfigSuppressionsMigration), + map[string]any{}) +} + // requireAZConfigSchema refuses a purchase submission before its transaction -// opens when the az_config expand migrations (000154, 000155) are missing: the -// suppression insert names az_config, so it would fail mid-transaction. +// opens when the az_config expand migrations are missing: the suppression +// insert names az_config, so it would fail mid-transaction. func (h *Handler) requireAZConfigSchema(ctx context.Context) error { checker, ok := h.config.(azConfigSchemaChecker) if !ok { return nil } err := checker.CheckAZConfigMigration(ctx) - if errors.Is(err, config.ErrAZConfigMigrationNotReady) { - return NewClientErrorWithDetails(503, - fmt.Sprintf("purchases are unavailable until database migrations 000154 and 000155 are applied: %v", err), - map[string]any{}) + if err == nil { + return nil } - if err != nil { - return fmt.Errorf("check az_config migration: %w", err) + if errors.Is(err, config.ErrAZConfigMigrationNotReady) { + return azConfigNotReadyError(err) } - return nil + return fmt.Errorf("check az_config migration: %w", err) } diff --git a/internal/api/azconfig_gate_integration_test.go b/internal/api/azconfig_gate_integration_test.go index a5800324a..9e865ed78 100644 --- a/internal/api/azconfig_gate_integration_test.go +++ b/internal/api/azconfig_gate_integration_test.go @@ -40,14 +40,14 @@ func TestAZConfigGate_RefusesBeforeExpandMigrations(t *testing.T) { h := &Handler{config: store} exec := &config.PurchaseExecution{ExecutionID: uuid.NewString()} - for _, version := range []uint{153, 154} { // before both expands, then after only the first + for _, version := range []uint{154, 155} { // before both expands, then after only the first require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) _, err := h.persistExecutionAndSuppressions(ctx, exec, nil, "creator", "key") ce, ok := IsClientError(err) require.True(t, ok, "version %d: %v", version, err) assert.Equal(t, 503, ce.code) - assert.Contains(t, ce.message, "000154 and 000155") + assert.Contains(t, ce.message, "000155 and 000156") assert.Zero(t, store.txs, "no transaction may open before the gate") rec := expandOrderRDSRec("single-az", 100) @@ -66,7 +66,7 @@ func TestAZConfigGate_RefusesBeforeExpandMigrations(t *testing.T) { store.txs = 0 // the direct WithTx above is not the purchase path } - for _, version := range []uint{155, 156} { + for _, version := range []uint{156, 157} { require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) require.NoError(t, h.requireAZConfigSchema(ctx), "version %d", version) _, err := store.ListActiveSuppressions(ctx) diff --git a/internal/config/errors.go b/internal/config/errors.go index 9c72fda93..88241327f 100644 --- a/internal/config/errors.go +++ b/internal/config/errors.go @@ -1,13 +1,24 @@ package config -import "errors" +import ( + "errors" + "fmt" +) var ErrAzurePricingNotReady = errors.New("azure pricing migration not ready") -// ErrAZConfigMigrationNotReady means migrations 000154 (recommendations -// az_config) and 000155 (purchase_suppressions az_config) are not both applied. -// Apply them before running a release that reads or writes az_config. -var ErrAZConfigMigrationNotReady = errors.New("az_config migrations 000154 and 000155 are not applied") +// The az_config expand migrations (issue #408). A later renumber is an edit of +// these two constants (plus the migration files). +const ( + AZConfigRecommendationsMigration = 155 + AZConfigSuppressionsMigration = 156 +) + +// ErrAZConfigMigrationNotReady means the recommendations and +// purchase_suppressions az_config migrations are not both applied. Apply them +// before running a release that reads or writes az_config. +var ErrAZConfigMigrationNotReady = fmt.Errorf("az_config migrations %06d and %06d are not applied", + AZConfigRecommendationsMigration, AZConfigSuppressionsMigration) // ErrNotFound is returned when a requested config-store row does not exist. var ErrNotFound = errors.New("not found") diff --git a/internal/config/recommendation_identity.go b/internal/config/recommendation_identity.go index 8adbc4761..80629c5d8 100644 --- a/internal/config/recommendation_identity.go +++ b/internal/config/recommendation_identity.go @@ -48,7 +48,7 @@ func (r *RecommendationRecord) ComputeIdentity() (ComputeIdentity, error) { // blob, VERBATIM. Single-AZ and Multi-AZ RDS reservations are different // offerings with different prices, yet share every other identity field // (issue #408). For AWS RDS rows it mirrors the generated az_config column of -// migration 000154 (COALESCE(payload->'details'->>'az_config', ”); the column +// migration 000155 (COALESCE(payload->'details'->>'az_config', ”); the column // is ” for every other rec, like IdentityAZConfig) and is pinned to it by the // shared vectors in TestDetailsAZConfig_Vectors: absent Details, JSON null, a // missing key and a null value all yield "". Only DatabaseDetails carries the diff --git a/internal/config/recommendation_identity_test.go b/internal/config/recommendation_identity_test.go index cae908aa3..4ae1f0d26 100644 --- a/internal/config/recommendation_identity_test.go +++ b/internal/config/recommendation_identity_test.go @@ -46,7 +46,7 @@ func TestComputeIdentity_TopLevelKeyIgnoredAndMalformedFailsLoud(t *testing.T) { assert.Error(t, err) } -// The vectors mirror TestMigration_RecommendationsAZConfig (migration 000154, +// The vectors mirror TestMigration_RecommendationsAZConfig (migration 000155, // internal/database/postgres/migrations) so the Go reader and the generated // az_config column cannot diverge (issue #408). Details here is the details // blob itself. diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index a2250a789..76c0ab76e 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -24,8 +24,8 @@ const recommendationsBatchSize = 500 const azurePricingMigrationVersion = 106 // azConfigExpandMigrationVersion is the last az_config expand migration -// (000155); the contract step 000156 is not required. -const azConfigExpandMigrationVersion = 155 +// (AZConfigSuppressionsMigration); the contract step after it is not required. +const azConfigExpandMigrationVersion = AZConfigSuppressionsMigration // UpsertRecommendations is the incremental write path: it upserts each row // by natural key (cloud_account_id, provider, service, region, @@ -40,7 +40,7 @@ const azConfigExpandMigrationVersion = 155 // so per-rec ON CONFLICT no longer collides on SQLSTATE 21000. Migration // 000109 added availability_zone, a GENERATED column derived from payload that // is never written here. Migration 000111 added the platform and tenancy -// generated columns the same way, and migration 000154 the az_config one; the +// generated columns the same way, and migration 000155 the az_config one; the // upsert arbiter is the 12-column recommendations_natural_key_az_tp_azc_idx // (issues #792, #806, #408). // @@ -420,8 +420,8 @@ func (s *PostgresStore) checkAzurePricingMigration(ctx context.Context, required } // CheckAZConfigMigration returns ErrAZConfigMigrationNotReady (wrapped, with -// the schema version found) until both az_config expand migrations (000154 -// recommendations, 000155 purchase_suppressions) are applied. +// the schema version found) until both az_config expand migrations (000155 +// recommendations, 000156 purchase_suppressions) are applied. func (s *PostgresStore) CheckAZConfigMigration(ctx context.Context) error { return checkSchemaVersion(ctx, s.db, azConfigExpandMigrationVersion, ErrAZConfigMigrationNotReady, "az_config") } diff --git a/internal/config/store_postgres_recommendations_azconfig_test.go b/internal/config/store_postgres_recommendations_azconfig_test.go index e65e88c30..fd6f91855 100644 --- a/internal/config/store_postgres_recommendations_azconfig_test.go +++ b/internal/config/store_postgres_recommendations_azconfig_test.go @@ -26,7 +26,7 @@ func rdsAZConfigStoreRec(azConfig string, savings float64) config.Recommendation // Issue #408: Cost Explorer reports Single-AZ and Multi-AZ RDS reservations of // the same class, engine, term and payment as separate recommendations. Once // the store upserts on the 12-column index (with the generated az_config -// column) and migration 000156 drops the 11-column index, both rows upsert in +// column) and migration 000157 drops the 11-column index, both rows upsert in // ONE batch (the shape persistCollection writes) and both persist. Before, // the batch failed with SQLSTATE 21000 ("ON CONFLICT DO UPDATE command cannot // affect row a second time") and took the whole collection write down. @@ -40,7 +40,7 @@ func TestPostgresStore_UpsertRecommendations_AZConfigBatch(t *testing.T) { var oldIdx bool require.NoError(t, pool.QueryRow(ctx, `SELECT EXISTS (SELECT 1 FROM pg_indexes WHERE indexname = 'recommendations_natural_key_az_tp_idx')`).Scan(&oldIdx)) - require.False(t, oldIdx, "11-column index must be gone after the full migration chain (000156)") + require.False(t, oldIdx, "11-column index must be gone after the full migration chain (000157)") savingsByAZConfig := func(t *testing.T) map[string]float64 { t.Helper() diff --git a/internal/config/store_postgres_suppressions.go b/internal/config/store_postgres_suppressions.go index a79dc6077..15c4ea8e5 100644 --- a/internal/config/store_postgres_suppressions.go +++ b/internal/config/store_postgres_suppressions.go @@ -109,7 +109,7 @@ func (s *PostgresStore) CreateSuppressionTx(ctx context.Context, tx pgx.Tx, sup // 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). - // az_config is NULL when empty: unknown or not applicable (migration 000155). + // az_config is NULL when empty: unknown or not applicable (migration 000156). var az, platform, tenancy, azConfig *string if sup.Variant != nil { az, platform, tenancy = &sup.Variant.AvailabilityZone, &sup.Variant.Platform, &sup.Variant.Tenancy @@ -210,7 +210,7 @@ func (s *PostgresStore) ListActiveSuppressions(ctx context.Context) ([]PurchaseS // 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. A NULL az_config (rows written before -// migration 000155, or a rec with none) is read as "" = any az_config. +// migration 000156, or a rec with none) is read as "" = any az_config. func suppressionVariantFromColumns(az, platform, tenancy, azConfig *string) *SuppressionVariant { if az == nil || platform == nil || tenancy == nil { return nil diff --git a/internal/config/types.go b/internal/config/types.go index 7f950eebb..6f941097a 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -800,7 +800,7 @@ type PurchaseSuppression struct { // #806, #408). Empty AvailabilityZone, Platform and Tenancy are known values // (a regional rec, a non-EC2 rec), not wildcards. AZConfig is the exception: // "" means "not applicable or unknown" and matches any az_config (see -// AppliesTo), because suppressions written before migration 000155 have none +// AppliesTo), because suppressions written before migration 000156 have none // and must keep suppressing both the Single-AZ and the Multi-AZ rec. type SuppressionVariant struct { AvailabilityZone string `json:"availability_zone"` diff --git a/internal/database/postgres/migrations/000154_recommendations_az_config.down.sql b/internal/database/postgres/migrations/000155_recommendations_az_config.down.sql similarity index 100% rename from internal/database/postgres/migrations/000154_recommendations_az_config.down.sql rename to internal/database/postgres/migrations/000155_recommendations_az_config.down.sql diff --git a/internal/database/postgres/migrations/000154_recommendations_az_config.up.sql b/internal/database/postgres/migrations/000155_recommendations_az_config.up.sql similarity index 94% rename from internal/database/postgres/migrations/000154_recommendations_az_config.up.sql rename to internal/database/postgres/migrations/000155_recommendations_az_config.up.sql index 9b418c5e2..592c37834 100644 --- a/internal/database/postgres/migrations/000154_recommendations_az_config.up.sql +++ b/internal/database/postgres/migrations/000155_recommendations_az_config.up.sql @@ -1,4 +1,4 @@ --- Migration 000154: az_config generated column and a 12-column unique index on +-- Migration 000155: az_config generated column and a 12-column unique index on -- recommendations (#408, expand step of the RDS AZ-config identity work). -- -- Cost Explorer reports Single-AZ and Multi-AZ RDS reservations of the same @@ -25,13 +25,13 @@ -- - recommendations_natural_key_source_azc_idx: the source-account key of -- migration 000120 (#799) plus az_config. Every unique index without -- az_config rejects a Single-AZ/Multi-AZ pair, so the source-aware key has --- to gain the column too; 000156 retires the 000120 index it replaces. +-- to gain the column too; 000157 retires the 000120 index it replaces. -- -- The existing 11-column recommendations_natural_key_az_tp_idx and the -- 000120 recommendations_natural_key_source_idx are KEPT, so the current -- ON CONFLICT on the 11 columns keeps working for old and new code. Two rows -- that differ only by az_config are still rejected by those indexes until --- migration 000156 drops them (applied only once the store upserts on the +-- migration 000157 drops them (applied only once the store upserts on the -- 12-column index). -- -- Lock impact: ADD COLUMN ... GENERATED ... STORED rewrites the table and the diff --git a/internal/database/postgres/migrations/000154_recommendations_az_config_test.go b/internal/database/postgres/migrations/000155_recommendations_az_config_test.go similarity index 94% rename from internal/database/postgres/migrations/000154_recommendations_az_config_test.go rename to internal/database/postgres/migrations/000155_recommendations_az_config_test.go index 658b119c0..5f5d4dfe8 100644 --- a/internal/database/postgres/migrations/000154_recommendations_az_config_test.go +++ b/internal/database/postgres/migrations/000155_recommendations_az_config_test.go @@ -21,14 +21,14 @@ const insertRecForMigration152 = ` VALUES (NOW(), 'aws', 'rds', 'us-east-1', $1, 'mysql', $2::jsonb, 0, 0, 1, 'all-upfront') ` -const insertRecForMigration154Scoped = ` +const insertRecForMigration155Scoped = ` INSERT INTO recommendations (collected_at, provider, service, region, resource_type, engine, payload, upfront_cost, monthly_savings, term, payment_option) VALUES (NOW(), $3, $4, 'us-east-1', $1, 'mysql', $2::jsonb, 0, 0, 1, 'all-upfront') ` -// TestMigration_RecommendationsAZConfig locks down migration 000154 (issue +// TestMigration_RecommendationsAZConfig locks down migration 000155 (issue // #408, expand step): the generated az_config column is read verbatim from // payload->'details', the 11-column index is kept so the previous upsert still // works, the 12-column index is added, and rollback removes only the new @@ -43,7 +43,7 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { defer container.Cleanup(ctx) pool := container.DB.Pool() - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 154))) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 155))) // provider and service default to aws/rds; the column is scoped to AWS RDS // like IdentityAZConfig, so other providers and services read "". @@ -70,11 +70,11 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { if v.provider == "" { v.provider, v.service = "aws", "rds" } - _, err = pool.Exec(ctx, insertRecForMigration154Scoped, name, v.payload, v.provider, v.service) + _, err = pool.Exec(ctx, insertRecForMigration155Scoped, name, v.payload, v.provider, v.service) require.NoError(t, err) } - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 154)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 155)) for name, v := range vectors { var got string @@ -89,7 +89,7 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { var exists bool require.NoError(t, pool.QueryRow(ctx, `SELECT EXISTS (SELECT 1 FROM pg_indexes WHERE tablename = 'recommendations' AND indexname = $1)`, idx).Scan(&exists)) - assert.True(t, exists, "index %s must exist after 000154", idx) + assert.True(t, exists, "index %s must exist after 000155", idx) } var indexDef string require.NoError(t, pool.QueryRow(ctx, @@ -102,7 +102,7 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { assert.Contains(t, indexDef, "(account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, az_config, source_account_key)") // The previous code's upsert (11-column ON CONFLICT) still works and - // updates the existing row in place, so 000154 can be applied before the + // updates the existing row in place, so 000155 can be applied before the // store switch is released. singleAZ := `{"details":{"az_config":"single-az"}}` _, err = pool.Exec(ctx, insertRecForMigration152+` @@ -112,12 +112,12 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { _, err = pool.Exec(ctx, insertRecForMigration152+` ON CONFLICT (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy) DO UPDATE SET upfront_cost = 5`, "pair", singleAZ) - require.NoError(t, err, "the existing 11-column upsert must keep working after 000154") + require.NoError(t, err, "the existing 11-column upsert must keep working after 000155") var n int require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*) FROM recommendations WHERE resource_type = 'pair'`).Scan(&n)) assert.Equal(t, 1, n) - // Known limitation, resolved by 000156: a row differing only by az_config + // Known limitation, resolved by 000157: a row differing only by az_config // is still rejected by the kept indexes without az_config. _, err = pool.Exec(ctx, insertRecForMigration152, "pair", `{"details":{"az_config":"multi-az"}}`) var pgErr *pgconn.PgError @@ -126,7 +126,7 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { assert.Contains(t, []string{"recommendations_natural_key_az_tp_idx", "recommendations_natural_key_source_idx"}, pgErr.ConstraintName) // Rollback removes the new column and index only. - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 154))) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 155))) var colExists bool require.NoError(t, pool.QueryRow(ctx, `SELECT EXISTS (SELECT 1 FROM information_schema.columns WHERE table_name = 'recommendations' AND column_name = 'az_config')`).Scan(&colExists)) @@ -144,5 +144,5 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*) FROM recommendations`).Scan(&n)) assert.Equal(t, len(vectors)+1, n, "rollback must not delete rows") - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 154)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 155)) } diff --git a/internal/database/postgres/migrations/000155_purchase_suppressions_az_config.down.sql b/internal/database/postgres/migrations/000156_purchase_suppressions_az_config.down.sql similarity index 97% rename from internal/database/postgres/migrations/000155_purchase_suppressions_az_config.down.sql rename to internal/database/postgres/migrations/000156_purchase_suppressions_az_config.down.sql index 8b0feb167..106655e80 100644 --- a/internal/database/postgres/migrations/000155_purchase_suppressions_az_config.down.sql +++ b/internal/database/postgres/migrations/000156_purchase_suppressions_az_config.down.sql @@ -1,4 +1,4 @@ --- Rollback of 000155: restore the 000134 suppression key without az_config. +-- Rollback of 000156: restore the 000134 suppression key without az_config. -- -- Rows of one execution that differ only by az_config are merged into one row -- per execution, 6-tuple and variant before the narrower UNIQUE constraint is diff --git a/internal/database/postgres/migrations/000155_purchase_suppressions_az_config.up.sql b/internal/database/postgres/migrations/000156_purchase_suppressions_az_config.up.sql similarity index 87% rename from internal/database/postgres/migrations/000155_purchase_suppressions_az_config.up.sql rename to internal/database/postgres/migrations/000156_purchase_suppressions_az_config.up.sql index ea72129c7..92b3df7d1 100644 --- a/internal/database/postgres/migrations/000155_purchase_suppressions_az_config.up.sql +++ b/internal/database/postgres/migrations/000156_purchase_suppressions_az_config.up.sql @@ -1,7 +1,7 @@ --- Migration 000155 (issue #408): widen the purchase suppression variant with +-- Migration 000156 (issue #408): widen the purchase suppression variant with -- the AWS RDS az_config ("single-az" / "multi-az"). -- --- Migration 000156 lets a Single-AZ and a Multi-AZ RDS rec of the same class, +-- Migration 000157 lets a Single-AZ and a Multi-AZ RDS rec of the same class, -- engine, term and payment coexist, but the suppression key (000134) cannot -- tell them apart: buying 2x Single-AZ would write a suppression that also -- matched, and hid, the Multi-AZ rec. The variant now carries az_config for @@ -20,12 +20,12 @@ -- so NULL rows stay unique per execution, 6-tuple and variant as before). -- A suppression with an az_config must also carry the rest of the variant. -- --- DEPLOY ORDER: apply this (and 000154) before deploying the code that writes +-- DEPLOY ORDER: apply this (and 000155) before deploying the code that writes -- az_config (the insert names the column, and the purchase path and the --- suppression listing need it). Both are expand steps. 000156 is the contract --- step and must wait until that code is live everywhere (see the 000156 +-- suppression listing need it). Both are expand steps. 000157 is the contract +-- step and must wait until that code is live everywhere (see the 000157 -- header); never dispatch database-migration.yml with the default of all --- pending steps once 000156 is pending. +-- pending steps once 000157 is pending. ALTER TABLE purchase_suppressions ADD COLUMN IF NOT EXISTS az_config TEXT; diff --git a/internal/database/postgres/migrations/000155_purchase_suppressions_az_config_test.go b/internal/database/postgres/migrations/000156_purchase_suppressions_az_config_test.go similarity index 87% rename from internal/database/postgres/migrations/000155_purchase_suppressions_az_config_test.go rename to internal/database/postgres/migrations/000156_purchase_suppressions_az_config_test.go index 9f19f4dfd..4e6bdf3ae 100644 --- a/internal/database/postgres/migrations/000155_purchase_suppressions_az_config_test.go +++ b/internal/database/postgres/migrations/000156_purchase_suppressions_az_config_test.go @@ -13,9 +13,9 @@ import ( ) const ( - exec155 = "00000000-0000-0000-0000-000000155a01" + exec156 = "00000000-0000-0000-0000-000000155a01" - insertRDSSuppression155 = ` + insertRDSSuppression156 = ` INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, resource_type, engine, suppressed_count, expires_at, availability_zone, platform, tenancy, az_config) @@ -23,7 +23,7 @@ const ( NOW() + INTERVAL '7 days', '', '', '', $3)` ) -// TestMigration_PurchaseSuppressionsAZConfig locks down migration 000155 +// TestMigration_PurchaseSuppressionsAZConfig locks down migration 000156 // (issue #408): rows seeded before the migration, with and without a 000134 // variant, survive it with a NULL az_config (the wildcard backfill rule); // afterwards Single-AZ and Multi-AZ rows of one execution coexist while a @@ -35,7 +35,7 @@ func TestMigration_PurchaseSuppressionsAZConfig(t *testing.T) { ctx := context.Background() container := testhelpers.RequireEmptyDatabase(ctx, t) pool, path := container.DB.Pool(), getMigrationsPath() - prev := previousMigrationVersion(t, 155) + prev := previousMigrationVersion(t, 156) require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) // Pre-migration rows: one legacy 6-tuple row (NULL variant) and one RDS @@ -43,35 +43,35 @@ func TestMigration_PurchaseSuppressionsAZConfig(t *testing.T) { _, err := pool.Exec(ctx, ` INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, resource_type, engine, suppressed_count, expires_at) - VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.t3.micro', 'mysql', 1, NOW() + INTERVAL '7 days')`, exec155) + VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.t3.micro', 'mysql', 1, NOW() + INTERVAL '7 days')`, exec156) require.NoError(t, err) _, err = pool.Exec(ctx, ` 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', 'relational-db', 'us-east-1', 'db.r5.large', 'mysql', 2, NOW() + INTERVAL '7 days', '', '', '')`, exec155) + VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.r5.large', 'mysql', 2, NOW() + INTERVAL '7 days', '', '', '')`, exec156) require.NoError(t, err) - require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 155)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 156)) var rows, nulls int require.NoError(t, pool.QueryRow(ctx, `SELECT count(*), count(*) FILTER (WHERE az_config IS NULL) - FROM purchase_suppressions WHERE execution_id = $1`, exec155).Scan(&rows, &nulls)) + FROM purchase_suppressions WHERE execution_id = $1`, exec156).Scan(&rows, &nulls)) assert.Equal(t, 2, rows, "existing rows survive") assert.Equal(t, 2, nulls, "existing rows get a NULL az_config: a wildcard") // Single-AZ and Multi-AZ rows coexist next to the NULL row of the same variant. for _, v := range []string{"single-az", "multi-az"} { - _, err = pool.Exec(ctx, insertRDSSuppression155, exec155, 3, v) + _, err = pool.Exec(ctx, insertRDSSuppression156, exec156, 3, v) require.NoError(t, err, v) } - _, err = pool.Exec(ctx, insertRDSSuppression155, exec155, 3, "multi-az") + _, err = pool.Exec(ctx, insertRDSSuppression156, exec156, 3, "multi-az") requireUniqueViolation134(t, err, "purchase_suppressions_execution_variant_azc_key") // An az_config without the rest of the variant is rejected. _, err = pool.Exec(ctx, ` INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, resource_type, engine, suppressed_count, expires_at, az_config) - VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.m5.large', 'mysql', 1, NOW() + INTERVAL '7 days', 'multi-az')`, exec155) + VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.m5.large', 'mysql', 1, NOW() + INTERVAL '7 days', 'multi-az')`, exec156) require.Error(t, err) assert.Contains(t, err.Error(), "purchase_suppressions_az_config_needs_variant") @@ -80,20 +80,20 @@ func TestMigration_PurchaseSuppressionsAZConfig(t *testing.T) { require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) var total int require.NoError(t, pool.QueryRow(ctx, `SELECT count(*), COALESCE(SUM(suppressed_count), 0) - FROM purchase_suppressions WHERE execution_id = $1 AND resource_type = 'db.r5.large'`, exec155).Scan(&rows, &total)) + FROM purchase_suppressions WHERE execution_id = $1 AND resource_type = 'db.r5.large'`, exec156).Scan(&rows, &total)) assert.Equal(t, 1, rows) assert.Equal(t, 8, total) require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM purchase_suppressions - WHERE execution_id = $1 AND resource_type = 'db.t3.micro'`, exec155).Scan(&rows)) + WHERE execution_id = $1 AND resource_type = 'db.t3.micro'`, exec156).Scan(&rows)) assert.Equal(t, 1, rows) var cols int require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM information_schema.columns WHERE table_name = 'purchase_suppressions' AND column_name = 'az_config'`).Scan(&cols)) assert.Zero(t, cols) - _, err = pool.Exec(ctx, insertVariantSuppression134, exec155, 1, "us-east-1a", "Linux/UNIX", "default") + _, err = pool.Exec(ctx, insertVariantSuppression134, exec156, 1, "us-east-1a", "Linux/UNIX", "default") require.NoError(t, err) - _, err = pool.Exec(ctx, insertVariantSuppression134, exec155, 1, "us-east-1a", "Linux/UNIX", "default") + _, err = pool.Exec(ctx, insertVariantSuppression134, exec156, 1, "us-east-1a", "Linux/UNIX", "default") requireUniqueViolation134(t, err, "purchase_suppressions_execution_variant_key") - require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 155), "up must re-apply cleanly after down") + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 156), "up must re-apply cleanly after down") } diff --git a/internal/database/postgres/migrations/000156_drop_recommendations_11col_natural_key.down.sql b/internal/database/postgres/migrations/000157_drop_recommendations_11col_natural_key.down.sql similarity index 91% rename from internal/database/postgres/migrations/000156_drop_recommendations_11col_natural_key.down.sql rename to internal/database/postgres/migrations/000157_drop_recommendations_11col_natural_key.down.sql index 52b3f43ba..5fe3a5a3e 100644 --- a/internal/database/postgres/migrations/000156_drop_recommendations_11col_natural_key.down.sql +++ b/internal/database/postgres/migrations/000157_drop_recommendations_11col_natural_key.down.sql @@ -1,8 +1,8 @@ --- Rollback of 000156: recreate the 11-column unique index exactly as defined +-- Rollback of 000157: recreate the 11-column unique index exactly as defined -- by 000111 and the source-account index exactly as defined by 000120. -- -- Only safe together with a code rollback to the 11-column upsert. The newer --- code needs the 12-column arbiter from 000154, which this rollback keeps. The +-- code needs the 12-column arbiter from 000155, which this rollback keeps. The -- recommendations table is a regenerable cache, so rows that differ only by -- az_config (allowed while the 11-column index is absent) are collapsed to the -- most recently collected one before the indexes are rebuilt; the next diff --git a/internal/database/postgres/migrations/000156_drop_recommendations_11col_natural_key.up.sql b/internal/database/postgres/migrations/000157_drop_recommendations_11col_natural_key.up.sql similarity index 92% rename from internal/database/postgres/migrations/000156_drop_recommendations_11col_natural_key.up.sql rename to internal/database/postgres/migrations/000157_drop_recommendations_11col_natural_key.up.sql index a6bb12332..21ec8ab05 100644 --- a/internal/database/postgres/migrations/000156_drop_recommendations_11col_natural_key.up.sql +++ b/internal/database/postgres/migrations/000157_drop_recommendations_11col_natural_key.up.sql @@ -1,8 +1,8 @@ --- Migration 000156: drop the 11-column recommendations_natural_key_az_tp_idx +-- Migration 000157: drop the 11-column recommendations_natural_key_az_tp_idx -- and the 12-column recommendations_natural_key_source_idx (#408, contract step -- of the RDS AZ-config identity work). -- --- Migration 000154 added the generated az_config column, the 12-column +-- Migration 000155 added the generated az_config column, the 12-column -- recommendations_natural_key_az_tp_azc_idx and the 13-column -- recommendations_natural_key_source_azc_idx, and kept the indexes without -- az_config so the previous upsert (ON CONFLICT on the 11 columns) kept diff --git a/internal/database/postgres/migrations/000156_drop_recommendations_11col_natural_key_test.go b/internal/database/postgres/migrations/000157_drop_recommendations_11col_natural_key_test.go similarity index 97% rename from internal/database/postgres/migrations/000156_drop_recommendations_11col_natural_key_test.go rename to internal/database/postgres/migrations/000157_drop_recommendations_11col_natural_key_test.go index e78565eac..088b667c6 100644 --- a/internal/database/postgres/migrations/000156_drop_recommendations_11col_natural_key_test.go +++ b/internal/database/postgres/migrations/000157_drop_recommendations_11col_natural_key_test.go @@ -20,7 +20,7 @@ const insertRecForMigration153 = ` VALUES ($1::timestamptz, 'aws', 'rds', $2, 'db.r5.large', 'mysql', $3::jsonb, 0, 0, 1, 'all-upfront') ` -// TestMigration_DropRecommendations11ColNaturalKey locks down migration 000156 +// TestMigration_DropRecommendations11ColNaturalKey locks down migration 000157 // (issue #408, contract step): Single-AZ and Multi-AZ rows of the same class // coexist once the 11-column and 000120 source indexes are gone, the 12-column index still dedupes, // and the rollback recreates both indexes after collapsing those @@ -51,7 +51,7 @@ func TestMigration_DropRecommendations11ColNaturalKey(t *testing.T) { none = `{"details":{"engine":"mysql"}}` ) - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 156))) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 157))) require.True(t, indexExists("recommendations_natural_key_az_tp_idx")) require.True(t, indexExists("recommendations_natural_key_az_tp_azc_idx")) require.True(t, indexExists("recommendations_natural_key_source_idx")) @@ -62,7 +62,7 @@ func TestMigration_DropRecommendations11ColNaturalKey(t *testing.T) { _, err = pool.Exec(ctx, insertRecForMigration153, "2026-01-01T00:00:00Z", "us-west-2", single) require.NoError(t, err) - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 156)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 157)) assert.False(t, indexExists("recommendations_natural_key_az_tp_idx"), "11-column index must be dropped") assert.True(t, indexExists("recommendations_natural_key_az_tp_azc_idx"), "12-column index must remain") assert.False(t, indexExists("recommendations_natural_key_source_idx"), "000120's source index (no az_config) must be dropped") @@ -81,7 +81,7 @@ func TestMigration_DropRecommendations11ColNaturalKey(t *testing.T) { // Rollback: az_config-only duplicates collapse to the newest, the other // region's row is untouched, and the 11-column index is recreated with // its original shape. - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 156))) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 157))) assert.Equal(t, 1, count("us-east-1"), "only duplicates under the 11-column key may be deleted") assert.Equal(t, 1, count("us-west-2")) var kept string @@ -100,7 +100,7 @@ func TestMigration_DropRecommendations11ColNaturalKey(t *testing.T) { assert.Contains(t, indexDef, "(account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, source_account_key)") // Re-applying the up migration after a rollback works. - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 156)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 157)) assert.False(t, indexExists("recommendations_natural_key_az_tp_idx")) assert.False(t, indexExists("recommendations_natural_key_source_idx")) } diff --git a/internal/purchase/azure_pricing_integration_test.go b/internal/purchase/azure_pricing_integration_test.go index d953dcb96..aa294d72b 100644 --- a/internal/purchase/azure_pricing_integration_test.go +++ b/internal/purchase/azure_pricing_integration_test.go @@ -98,10 +98,10 @@ func TestApproveAzurePricingRequiresMigrationAndRecollection(t *testing.T) { t.Run("106 invalidated old prices", func(t *testing.T) { approve(t, ErrStaleAzurePricing, 0) }) rec.UpfrontCost = 700 // The store's upsert needs the availability_zone, platform, tenancy and az_config columns - // and the 12-column index from migration 000154 (issue #408), and is gated until the - // 000155 expand (CheckAZConfigMigration). It also resets + // and the 12-column index from migration 000155 (issue #408), and is gated until the + // 000156 expand (CheckAZConfigMigration). It also resets // recommendations_state.last_collection_partial, added by migration 000129 (issue #87). - require.NoError(t, migrations.MigrateToVersion(ctx, pool, rampMigrationsPath(), 155)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, rampMigrationsPath(), 156)) require.NoError(t, pricing.UpsertRecommendations(ctx, time.Now().Add(time.Minute), []config.RecommendationRecord{rec}, collects)) t.Run("recollected prices purchase once", func(t *testing.T) { approve(t, nil, 1) }) } diff --git a/internal/scheduler/suppression_variant_integration_test.go b/internal/scheduler/suppression_variant_integration_test.go index 08a6ddf6c..d22affc3e 100644 --- a/internal/scheduler/suppression_variant_integration_test.go +++ b/internal/scheduler/suppression_variant_integration_test.go @@ -148,7 +148,7 @@ func TestSuppressionsMatchAcrossEC2Slugs_RealPostgres(t *testing.T) { } // TestRDSSuppressionScopedToAZConfig_RealPostgres is the issue #408 -// regression on a real database: after migrations 000154-000156 a Single-AZ +// regression on a real database: after migrations 000155-000157 a Single-AZ // and a Multi-AZ db.r5.large mysql rec coexist. Buying 2x Single-AZ must leave // the Multi-AZ rec at its full count, while a suppression row written before // az_config existed (variant set, az_config NULL) still hides both. From 1561dbab0158cd0854289efc7ea8b32a21288364 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 17:09:15 +0100 Subject: [PATCH 17/19] fix(api): answer the recommendations list with a 503 below the az_config expand The list returned a generic 500 when the az_config migrations were missing. Map ErrAZConfigMigrationNotReady to the same 503 the purchase paths return, through one shared helper. Co-Authored-By: Claude Opus 5.5 --- internal/api/az_config_gate_test.go | 47 +++++++++++++++++++++++++ internal/api/handler_recommendations.go | 5 +++ 2 files changed, 52 insertions(+) create mode 100644 internal/api/az_config_gate_test.go diff --git a/internal/api/az_config_gate_test.go b/internal/api/az_config_gate_test.go new file mode 100644 index 000000000..3f8b5463a --- /dev/null +++ b/internal/api/az_config_gate_test.go @@ -0,0 +1,47 @@ +package api + +import ( + "context" + "fmt" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// A recommendations list below the az_config expand migrations answers the same +// 503 as the purchase paths, not a generic 500. +func TestGetRecommendations_MapsMissingAZConfigMigrationTo503(t *testing.T) { + ctx := context.Background() + + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + mockAuth.On("HasAPIKeyPermissionAPI", ctx, "cudly-user-key", "view", "recommendations"). + Return("user-123", "key-abc", true, nil) + // Account scoping resolves against the key's owning user. + mockAuth.On("GetAllowedAccountsAPI", ctx, "user-123"). + Return([]string(nil), nil) + // The unrestricted read probes whether it may kick a collection. + mockAuth.On("HasAPIKeyPermissionForConstraintsAPI", ctx, "key-abc", "user-123", "view", "recommendations", mock.Anything). + Return(true, nil) + + mockScheduler := new(MockScheduler) + t.Cleanup(func() { mockScheduler.AssertExpectations(t) }) + mockScheduler.On("ListRecommendations", mock.Anything, mock.Anything). + Return([]config.RecommendationRecord(nil), fmt.Errorf("suppression listing skipped: %w", config.ErrAZConfigMigrationNotReady)) + + handler := &Handler{auth: mockAuth, scheduler: mockScheduler} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "cudly-user-key"}, + } + + _, err := handler.getRecommendations(ctx, req, map[string]string{}) + ce, ok := IsClientError(err) + require.True(t, ok, "got %v", err) + assert.Equal(t, 503, ce.code) + assert.Contains(t, ce.message, "000155 and 000156") +} diff --git a/internal/api/handler_recommendations.go b/internal/api/handler_recommendations.go index af1bf2924..cfcc03eed 100644 --- a/internal/api/handler_recommendations.go +++ b/internal/api/handler_recommendations.go @@ -85,6 +85,11 @@ func parseRecommendationFilter(params map[string]string) (config.RecommendationF // Recommendations handlers. func (h *Handler) getRecommendations(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (*RecommendationsResponse, error) { + resp, err := h.listRecommendations(ctx, req, params) + return resp, azConfigNotReadyError(err) +} + +func (h *Handler) listRecommendations(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (*RecommendationsResponse, error) { // Require view:recommendations permission session, err := h.requirePermission(ctx, req, "view", "recommendations") if err != nil { From c3bdec54c7d52a979ff94af43428b663630f134a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 17:12:02 +0100 Subject: [PATCH 18/19] fix(db): renumber the az_config migrations to 000157-000159 after #1241 Main now has 000155 and 000156 from #1241. Move the recommendations az_config expand to 000157, the purchase_suppressions expand to 000158 and the contract to 000159, with the gate constants, error text, fixtures and comments. Co-Authored-By: Claude Opus 5.5 --- internal/api/az_config_gate_test.go | 2 +- .../api/azconfig_gate_integration_test.go | 6 ++-- internal/config/errors.go | 4 +-- internal/config/recommendation_identity.go | 2 +- .../config/recommendation_identity_test.go | 2 +- .../config/store_postgres_pgxmock_test.go | 2 +- .../config/store_postgres_recommendations.go | 2 +- ..._postgres_recommendations_azconfig_test.go | 4 +-- .../config/store_postgres_suppressions.go | 4 +-- internal/config/types.go | 2 +- ...000157_recommendations_az_config.down.sql} | 0 ...> 000157_recommendations_az_config.up.sql} | 6 ++-- ... 000157_recommendations_az_config_test.go} | 22 ++++++------- ..._purchase_suppressions_az_config.down.sql} | 2 +- ...58_purchase_suppressions_az_config.up.sql} | 12 +++---- ...8_purchase_suppressions_az_config_test.go} | 32 +++++++++---------- ...ecommendations_11col_natural_key.down.sql} | 4 +-- ..._recommendations_11col_natural_key.up.sql} | 4 +-- ...recommendations_11col_natural_key_test.go} | 10 +++--- .../azure_pricing_integration_test.go | 6 ++-- .../suppression_variant_integration_test.go | 2 +- 21 files changed, 65 insertions(+), 65 deletions(-) rename internal/database/postgres/migrations/{000155_recommendations_az_config.down.sql => 000157_recommendations_az_config.down.sql} (100%) rename internal/database/postgres/migrations/{000155_recommendations_az_config.up.sql => 000157_recommendations_az_config.up.sql} (94%) rename internal/database/postgres/migrations/{000155_recommendations_az_config_test.go => 000157_recommendations_az_config_test.go} (94%) rename internal/database/postgres/migrations/{000156_purchase_suppressions_az_config.down.sql => 000158_purchase_suppressions_az_config.down.sql} (97%) rename internal/database/postgres/migrations/{000156_purchase_suppressions_az_config.up.sql => 000158_purchase_suppressions_az_config.up.sql} (87%) rename internal/database/postgres/migrations/{000156_purchase_suppressions_az_config_test.go => 000158_purchase_suppressions_az_config_test.go} (87%) rename internal/database/postgres/migrations/{000157_drop_recommendations_11col_natural_key.down.sql => 000159_drop_recommendations_11col_natural_key.down.sql} (91%) rename internal/database/postgres/migrations/{000157_drop_recommendations_11col_natural_key.up.sql => 000159_drop_recommendations_11col_natural_key.up.sql} (92%) rename internal/database/postgres/migrations/{000157_drop_recommendations_11col_natural_key_test.go => 000159_drop_recommendations_11col_natural_key_test.go} (97%) diff --git a/internal/api/az_config_gate_test.go b/internal/api/az_config_gate_test.go index 3f8b5463a..c22f948ff 100644 --- a/internal/api/az_config_gate_test.go +++ b/internal/api/az_config_gate_test.go @@ -43,5 +43,5 @@ func TestGetRecommendations_MapsMissingAZConfigMigrationTo503(t *testing.T) { ce, ok := IsClientError(err) require.True(t, ok, "got %v", err) assert.Equal(t, 503, ce.code) - assert.Contains(t, ce.message, "000155 and 000156") + assert.Contains(t, ce.message, "000157 and 000158") } diff --git a/internal/api/azconfig_gate_integration_test.go b/internal/api/azconfig_gate_integration_test.go index 9e865ed78..c9d9314cb 100644 --- a/internal/api/azconfig_gate_integration_test.go +++ b/internal/api/azconfig_gate_integration_test.go @@ -40,14 +40,14 @@ func TestAZConfigGate_RefusesBeforeExpandMigrations(t *testing.T) { h := &Handler{config: store} exec := &config.PurchaseExecution{ExecutionID: uuid.NewString()} - for _, version := range []uint{154, 155} { // before both expands, then after only the first + for _, version := range []uint{156, 157} { // before both expands, then after only the first require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) _, err := h.persistExecutionAndSuppressions(ctx, exec, nil, "creator", "key") ce, ok := IsClientError(err) require.True(t, ok, "version %d: %v", version, err) assert.Equal(t, 503, ce.code) - assert.Contains(t, ce.message, "000155 and 000156") + assert.Contains(t, ce.message, "000157 and 000158") assert.Zero(t, store.txs, "no transaction may open before the gate") rec := expandOrderRDSRec("single-az", 100) @@ -66,7 +66,7 @@ func TestAZConfigGate_RefusesBeforeExpandMigrations(t *testing.T) { store.txs = 0 // the direct WithTx above is not the purchase path } - for _, version := range []uint{156, 157} { + for _, version := range []uint{158, 159} { require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) require.NoError(t, h.requireAZConfigSchema(ctx), "version %d", version) _, err := store.ListActiveSuppressions(ctx) diff --git a/internal/config/errors.go b/internal/config/errors.go index 88241327f..4a95b539b 100644 --- a/internal/config/errors.go +++ b/internal/config/errors.go @@ -10,8 +10,8 @@ var ErrAzurePricingNotReady = errors.New("azure pricing migration not ready") // The az_config expand migrations (issue #408). A later renumber is an edit of // these two constants (plus the migration files). const ( - AZConfigRecommendationsMigration = 155 - AZConfigSuppressionsMigration = 156 + AZConfigRecommendationsMigration = 157 + AZConfigSuppressionsMigration = 158 ) // ErrAZConfigMigrationNotReady means the recommendations and diff --git a/internal/config/recommendation_identity.go b/internal/config/recommendation_identity.go index 80629c5d8..8e303a238 100644 --- a/internal/config/recommendation_identity.go +++ b/internal/config/recommendation_identity.go @@ -48,7 +48,7 @@ func (r *RecommendationRecord) ComputeIdentity() (ComputeIdentity, error) { // blob, VERBATIM. Single-AZ and Multi-AZ RDS reservations are different // offerings with different prices, yet share every other identity field // (issue #408). For AWS RDS rows it mirrors the generated az_config column of -// migration 000155 (COALESCE(payload->'details'->>'az_config', ”); the column +// migration 000157 (COALESCE(payload->'details'->>'az_config', ”); the column // is ” for every other rec, like IdentityAZConfig) and is pinned to it by the // shared vectors in TestDetailsAZConfig_Vectors: absent Details, JSON null, a // missing key and a null value all yield "". Only DatabaseDetails carries the diff --git a/internal/config/recommendation_identity_test.go b/internal/config/recommendation_identity_test.go index 4ae1f0d26..2edfbca39 100644 --- a/internal/config/recommendation_identity_test.go +++ b/internal/config/recommendation_identity_test.go @@ -46,7 +46,7 @@ func TestComputeIdentity_TopLevelKeyIgnoredAndMalformedFailsLoud(t *testing.T) { assert.Error(t, err) } -// The vectors mirror TestMigration_RecommendationsAZConfig (migration 000155, +// The vectors mirror TestMigration_RecommendationsAZConfig (migration 000157, // internal/database/postgres/migrations) so the Go reader and the generated // az_config column cannot diverge (issue #408). Details here is the details // blob itself. diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 0fb88b8ea..03a43fd95 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -33,7 +33,7 @@ func newMock(t *testing.T) pgxmock.PgxPoolIface { // ListActiveSuppressions run first, reporting the fully migrated version. func expectAZConfigReady(mock pgxmock.PgxPoolIface) { mock.ExpectQuery("SELECT version, dirty FROM schema_migrations"). - WillReturnRows(pgxmock.NewRows([]string{"version", "dirty"}).AddRow(156, false)) + WillReturnRows(pgxmock.NewRows([]string{"version", "dirty"}).AddRow(159, false)) } // anyArgs returns n AnyArg() matchers. diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index 4ac05170e..b40014d33 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -39,7 +39,7 @@ const azConfigExpandMigrationVersion = AZConfigSuppressionsMigration // so per-rec ON CONFLICT no longer collides on SQLSTATE 21000. Migration // 000109 added availability_zone, a GENERATED column derived from payload that // is never written here. Migration 000111 added the platform and tenancy -// generated columns the same way, and migration 000155 the az_config one; the +// generated columns the same way, and migration 000157 the az_config one; the // upsert arbiter is the 12-column recommendations_natural_key_az_tp_azc_idx // (issues #792, #806, #408). // diff --git a/internal/config/store_postgres_recommendations_azconfig_test.go b/internal/config/store_postgres_recommendations_azconfig_test.go index fd6f91855..a9390acfc 100644 --- a/internal/config/store_postgres_recommendations_azconfig_test.go +++ b/internal/config/store_postgres_recommendations_azconfig_test.go @@ -26,7 +26,7 @@ func rdsAZConfigStoreRec(azConfig string, savings float64) config.Recommendation // Issue #408: Cost Explorer reports Single-AZ and Multi-AZ RDS reservations of // the same class, engine, term and payment as separate recommendations. Once // the store upserts on the 12-column index (with the generated az_config -// column) and migration 000157 drops the 11-column index, both rows upsert in +// column) and migration 000159 drops the 11-column index, both rows upsert in // ONE batch (the shape persistCollection writes) and both persist. Before, // the batch failed with SQLSTATE 21000 ("ON CONFLICT DO UPDATE command cannot // affect row a second time") and took the whole collection write down. @@ -40,7 +40,7 @@ func TestPostgresStore_UpsertRecommendations_AZConfigBatch(t *testing.T) { var oldIdx bool require.NoError(t, pool.QueryRow(ctx, `SELECT EXISTS (SELECT 1 FROM pg_indexes WHERE indexname = 'recommendations_natural_key_az_tp_idx')`).Scan(&oldIdx)) - require.False(t, oldIdx, "11-column index must be gone after the full migration chain (000157)") + require.False(t, oldIdx, "11-column index must be gone after the full migration chain (000159)") savingsByAZConfig := func(t *testing.T) map[string]float64 { t.Helper() diff --git a/internal/config/store_postgres_suppressions.go b/internal/config/store_postgres_suppressions.go index 15c4ea8e5..94c0d7d53 100644 --- a/internal/config/store_postgres_suppressions.go +++ b/internal/config/store_postgres_suppressions.go @@ -109,7 +109,7 @@ func (s *PostgresStore) CreateSuppressionTx(ctx context.Context, tx pgx.Tx, sup // 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). - // az_config is NULL when empty: unknown or not applicable (migration 000156). + // az_config is NULL when empty: unknown or not applicable (migration 000158). var az, platform, tenancy, azConfig *string if sup.Variant != nil { az, platform, tenancy = &sup.Variant.AvailabilityZone, &sup.Variant.Platform, &sup.Variant.Tenancy @@ -210,7 +210,7 @@ func (s *PostgresStore) ListActiveSuppressions(ctx context.Context) ([]PurchaseS // 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. A NULL az_config (rows written before -// migration 000156, or a rec with none) is read as "" = any az_config. +// migration 000158, or a rec with none) is read as "" = any az_config. func suppressionVariantFromColumns(az, platform, tenancy, azConfig *string) *SuppressionVariant { if az == nil || platform == nil || tenancy == nil { return nil diff --git a/internal/config/types.go b/internal/config/types.go index 6f951f3d6..e0ba2de4b 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -800,7 +800,7 @@ type PurchaseSuppression struct { // #806, #408). Empty AvailabilityZone, Platform and Tenancy are known values // (a regional rec, a non-EC2 rec), not wildcards. AZConfig is the exception: // "" means "not applicable or unknown" and matches any az_config (see -// AppliesTo), because suppressions written before migration 000156 have none +// AppliesTo), because suppressions written before migration 000158 have none // and must keep suppressing both the Single-AZ and the Multi-AZ rec. type SuppressionVariant struct { AvailabilityZone string `json:"availability_zone"` diff --git a/internal/database/postgres/migrations/000155_recommendations_az_config.down.sql b/internal/database/postgres/migrations/000157_recommendations_az_config.down.sql similarity index 100% rename from internal/database/postgres/migrations/000155_recommendations_az_config.down.sql rename to internal/database/postgres/migrations/000157_recommendations_az_config.down.sql diff --git a/internal/database/postgres/migrations/000155_recommendations_az_config.up.sql b/internal/database/postgres/migrations/000157_recommendations_az_config.up.sql similarity index 94% rename from internal/database/postgres/migrations/000155_recommendations_az_config.up.sql rename to internal/database/postgres/migrations/000157_recommendations_az_config.up.sql index 592c37834..61e8b3450 100644 --- a/internal/database/postgres/migrations/000155_recommendations_az_config.up.sql +++ b/internal/database/postgres/migrations/000157_recommendations_az_config.up.sql @@ -1,4 +1,4 @@ --- Migration 000155: az_config generated column and a 12-column unique index on +-- Migration 000157: az_config generated column and a 12-column unique index on -- recommendations (#408, expand step of the RDS AZ-config identity work). -- -- Cost Explorer reports Single-AZ and Multi-AZ RDS reservations of the same @@ -25,13 +25,13 @@ -- - recommendations_natural_key_source_azc_idx: the source-account key of -- migration 000120 (#799) plus az_config. Every unique index without -- az_config rejects a Single-AZ/Multi-AZ pair, so the source-aware key has --- to gain the column too; 000157 retires the 000120 index it replaces. +-- to gain the column too; 000159 retires the 000120 index it replaces. -- -- The existing 11-column recommendations_natural_key_az_tp_idx and the -- 000120 recommendations_natural_key_source_idx are KEPT, so the current -- ON CONFLICT on the 11 columns keeps working for old and new code. Two rows -- that differ only by az_config are still rejected by those indexes until --- migration 000157 drops them (applied only once the store upserts on the +-- migration 000159 drops them (applied only once the store upserts on the -- 12-column index). -- -- Lock impact: ADD COLUMN ... GENERATED ... STORED rewrites the table and the diff --git a/internal/database/postgres/migrations/000155_recommendations_az_config_test.go b/internal/database/postgres/migrations/000157_recommendations_az_config_test.go similarity index 94% rename from internal/database/postgres/migrations/000155_recommendations_az_config_test.go rename to internal/database/postgres/migrations/000157_recommendations_az_config_test.go index 5f5d4dfe8..7fc249ea1 100644 --- a/internal/database/postgres/migrations/000155_recommendations_az_config_test.go +++ b/internal/database/postgres/migrations/000157_recommendations_az_config_test.go @@ -21,14 +21,14 @@ const insertRecForMigration152 = ` VALUES (NOW(), 'aws', 'rds', 'us-east-1', $1, 'mysql', $2::jsonb, 0, 0, 1, 'all-upfront') ` -const insertRecForMigration155Scoped = ` +const insertRecForMigration157Scoped = ` INSERT INTO recommendations (collected_at, provider, service, region, resource_type, engine, payload, upfront_cost, monthly_savings, term, payment_option) VALUES (NOW(), $3, $4, 'us-east-1', $1, 'mysql', $2::jsonb, 0, 0, 1, 'all-upfront') ` -// TestMigration_RecommendationsAZConfig locks down migration 000155 (issue +// TestMigration_RecommendationsAZConfig locks down migration 000157 (issue // #408, expand step): the generated az_config column is read verbatim from // payload->'details', the 11-column index is kept so the previous upsert still // works, the 12-column index is added, and rollback removes only the new @@ -43,7 +43,7 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { defer container.Cleanup(ctx) pool := container.DB.Pool() - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 155))) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 157))) // provider and service default to aws/rds; the column is scoped to AWS RDS // like IdentityAZConfig, so other providers and services read "". @@ -70,11 +70,11 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { if v.provider == "" { v.provider, v.service = "aws", "rds" } - _, err = pool.Exec(ctx, insertRecForMigration155Scoped, name, v.payload, v.provider, v.service) + _, err = pool.Exec(ctx, insertRecForMigration157Scoped, name, v.payload, v.provider, v.service) require.NoError(t, err) } - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 155)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 157)) for name, v := range vectors { var got string @@ -89,7 +89,7 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { var exists bool require.NoError(t, pool.QueryRow(ctx, `SELECT EXISTS (SELECT 1 FROM pg_indexes WHERE tablename = 'recommendations' AND indexname = $1)`, idx).Scan(&exists)) - assert.True(t, exists, "index %s must exist after 000155", idx) + assert.True(t, exists, "index %s must exist after 000157", idx) } var indexDef string require.NoError(t, pool.QueryRow(ctx, @@ -102,7 +102,7 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { assert.Contains(t, indexDef, "(account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, az_config, source_account_key)") // The previous code's upsert (11-column ON CONFLICT) still works and - // updates the existing row in place, so 000155 can be applied before the + // updates the existing row in place, so 000157 can be applied before the // store switch is released. singleAZ := `{"details":{"az_config":"single-az"}}` _, err = pool.Exec(ctx, insertRecForMigration152+` @@ -112,12 +112,12 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { _, err = pool.Exec(ctx, insertRecForMigration152+` ON CONFLICT (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy) DO UPDATE SET upfront_cost = 5`, "pair", singleAZ) - require.NoError(t, err, "the existing 11-column upsert must keep working after 000155") + require.NoError(t, err, "the existing 11-column upsert must keep working after 000157") var n int require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*) FROM recommendations WHERE resource_type = 'pair'`).Scan(&n)) assert.Equal(t, 1, n) - // Known limitation, resolved by 000157: a row differing only by az_config + // Known limitation, resolved by 000159: a row differing only by az_config // is still rejected by the kept indexes without az_config. _, err = pool.Exec(ctx, insertRecForMigration152, "pair", `{"details":{"az_config":"multi-az"}}`) var pgErr *pgconn.PgError @@ -126,7 +126,7 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { assert.Contains(t, []string{"recommendations_natural_key_az_tp_idx", "recommendations_natural_key_source_idx"}, pgErr.ConstraintName) // Rollback removes the new column and index only. - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 155))) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 157))) var colExists bool require.NoError(t, pool.QueryRow(ctx, `SELECT EXISTS (SELECT 1 FROM information_schema.columns WHERE table_name = 'recommendations' AND column_name = 'az_config')`).Scan(&colExists)) @@ -144,5 +144,5 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*) FROM recommendations`).Scan(&n)) assert.Equal(t, len(vectors)+1, n, "rollback must not delete rows") - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 155)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 157)) } diff --git a/internal/database/postgres/migrations/000156_purchase_suppressions_az_config.down.sql b/internal/database/postgres/migrations/000158_purchase_suppressions_az_config.down.sql similarity index 97% rename from internal/database/postgres/migrations/000156_purchase_suppressions_az_config.down.sql rename to internal/database/postgres/migrations/000158_purchase_suppressions_az_config.down.sql index 106655e80..3fc4e19d0 100644 --- a/internal/database/postgres/migrations/000156_purchase_suppressions_az_config.down.sql +++ b/internal/database/postgres/migrations/000158_purchase_suppressions_az_config.down.sql @@ -1,4 +1,4 @@ --- Rollback of 000156: restore the 000134 suppression key without az_config. +-- Rollback of 000158: restore the 000134 suppression key without az_config. -- -- Rows of one execution that differ only by az_config are merged into one row -- per execution, 6-tuple and variant before the narrower UNIQUE constraint is diff --git a/internal/database/postgres/migrations/000156_purchase_suppressions_az_config.up.sql b/internal/database/postgres/migrations/000158_purchase_suppressions_az_config.up.sql similarity index 87% rename from internal/database/postgres/migrations/000156_purchase_suppressions_az_config.up.sql rename to internal/database/postgres/migrations/000158_purchase_suppressions_az_config.up.sql index 92b3df7d1..5045f2948 100644 --- a/internal/database/postgres/migrations/000156_purchase_suppressions_az_config.up.sql +++ b/internal/database/postgres/migrations/000158_purchase_suppressions_az_config.up.sql @@ -1,7 +1,7 @@ --- Migration 000156 (issue #408): widen the purchase suppression variant with +-- Migration 000158 (issue #408): widen the purchase suppression variant with -- the AWS RDS az_config ("single-az" / "multi-az"). -- --- Migration 000157 lets a Single-AZ and a Multi-AZ RDS rec of the same class, +-- Migration 000159 lets a Single-AZ and a Multi-AZ RDS rec of the same class, -- engine, term and payment coexist, but the suppression key (000134) cannot -- tell them apart: buying 2x Single-AZ would write a suppression that also -- matched, and hid, the Multi-AZ rec. The variant now carries az_config for @@ -20,12 +20,12 @@ -- so NULL rows stay unique per execution, 6-tuple and variant as before). -- A suppression with an az_config must also carry the rest of the variant. -- --- DEPLOY ORDER: apply this (and 000155) before deploying the code that writes +-- DEPLOY ORDER: apply this (and 000157) before deploying the code that writes -- az_config (the insert names the column, and the purchase path and the --- suppression listing need it). Both are expand steps. 000157 is the contract --- step and must wait until that code is live everywhere (see the 000157 +-- suppression listing need it). Both are expand steps. 000159 is the contract +-- step and must wait until that code is live everywhere (see the 000159 -- header); never dispatch database-migration.yml with the default of all --- pending steps once 000157 is pending. +-- pending steps once 000159 is pending. ALTER TABLE purchase_suppressions ADD COLUMN IF NOT EXISTS az_config TEXT; diff --git a/internal/database/postgres/migrations/000156_purchase_suppressions_az_config_test.go b/internal/database/postgres/migrations/000158_purchase_suppressions_az_config_test.go similarity index 87% rename from internal/database/postgres/migrations/000156_purchase_suppressions_az_config_test.go rename to internal/database/postgres/migrations/000158_purchase_suppressions_az_config_test.go index 4e6bdf3ae..d2f91166f 100644 --- a/internal/database/postgres/migrations/000156_purchase_suppressions_az_config_test.go +++ b/internal/database/postgres/migrations/000158_purchase_suppressions_az_config_test.go @@ -13,9 +13,9 @@ import ( ) const ( - exec156 = "00000000-0000-0000-0000-000000155a01" + exec158 = "00000000-0000-0000-0000-000000158a01" - insertRDSSuppression156 = ` + insertRDSSuppression158 = ` INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, resource_type, engine, suppressed_count, expires_at, availability_zone, platform, tenancy, az_config) @@ -23,7 +23,7 @@ const ( NOW() + INTERVAL '7 days', '', '', '', $3)` ) -// TestMigration_PurchaseSuppressionsAZConfig locks down migration 000156 +// TestMigration_PurchaseSuppressionsAZConfig locks down migration 000158 // (issue #408): rows seeded before the migration, with and without a 000134 // variant, survive it with a NULL az_config (the wildcard backfill rule); // afterwards Single-AZ and Multi-AZ rows of one execution coexist while a @@ -35,7 +35,7 @@ func TestMigration_PurchaseSuppressionsAZConfig(t *testing.T) { ctx := context.Background() container := testhelpers.RequireEmptyDatabase(ctx, t) pool, path := container.DB.Pool(), getMigrationsPath() - prev := previousMigrationVersion(t, 156) + prev := previousMigrationVersion(t, 158) require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) // Pre-migration rows: one legacy 6-tuple row (NULL variant) and one RDS @@ -43,35 +43,35 @@ func TestMigration_PurchaseSuppressionsAZConfig(t *testing.T) { _, err := pool.Exec(ctx, ` INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, resource_type, engine, suppressed_count, expires_at) - VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.t3.micro', 'mysql', 1, NOW() + INTERVAL '7 days')`, exec156) + VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.t3.micro', 'mysql', 1, NOW() + INTERVAL '7 days')`, exec158) require.NoError(t, err) _, err = pool.Exec(ctx, ` 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', 'relational-db', 'us-east-1', 'db.r5.large', 'mysql', 2, NOW() + INTERVAL '7 days', '', '', '')`, exec156) + VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.r5.large', 'mysql', 2, NOW() + INTERVAL '7 days', '', '', '')`, exec158) require.NoError(t, err) - require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 156)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 158)) var rows, nulls int require.NoError(t, pool.QueryRow(ctx, `SELECT count(*), count(*) FILTER (WHERE az_config IS NULL) - FROM purchase_suppressions WHERE execution_id = $1`, exec156).Scan(&rows, &nulls)) + FROM purchase_suppressions WHERE execution_id = $1`, exec158).Scan(&rows, &nulls)) assert.Equal(t, 2, rows, "existing rows survive") assert.Equal(t, 2, nulls, "existing rows get a NULL az_config: a wildcard") // Single-AZ and Multi-AZ rows coexist next to the NULL row of the same variant. for _, v := range []string{"single-az", "multi-az"} { - _, err = pool.Exec(ctx, insertRDSSuppression156, exec156, 3, v) + _, err = pool.Exec(ctx, insertRDSSuppression158, exec158, 3, v) require.NoError(t, err, v) } - _, err = pool.Exec(ctx, insertRDSSuppression156, exec156, 3, "multi-az") + _, err = pool.Exec(ctx, insertRDSSuppression158, exec158, 3, "multi-az") requireUniqueViolation134(t, err, "purchase_suppressions_execution_variant_azc_key") // An az_config without the rest of the variant is rejected. _, err = pool.Exec(ctx, ` INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, resource_type, engine, suppressed_count, expires_at, az_config) - VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.m5.large', 'mysql', 1, NOW() + INTERVAL '7 days', 'multi-az')`, exec156) + VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.m5.large', 'mysql', 1, NOW() + INTERVAL '7 days', 'multi-az')`, exec158) require.Error(t, err) assert.Contains(t, err.Error(), "purchase_suppressions_az_config_needs_variant") @@ -80,20 +80,20 @@ func TestMigration_PurchaseSuppressionsAZConfig(t *testing.T) { require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) var total int require.NoError(t, pool.QueryRow(ctx, `SELECT count(*), COALESCE(SUM(suppressed_count), 0) - FROM purchase_suppressions WHERE execution_id = $1 AND resource_type = 'db.r5.large'`, exec156).Scan(&rows, &total)) + FROM purchase_suppressions WHERE execution_id = $1 AND resource_type = 'db.r5.large'`, exec158).Scan(&rows, &total)) assert.Equal(t, 1, rows) assert.Equal(t, 8, total) require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM purchase_suppressions - WHERE execution_id = $1 AND resource_type = 'db.t3.micro'`, exec156).Scan(&rows)) + WHERE execution_id = $1 AND resource_type = 'db.t3.micro'`, exec158).Scan(&rows)) assert.Equal(t, 1, rows) var cols int require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM information_schema.columns WHERE table_name = 'purchase_suppressions' AND column_name = 'az_config'`).Scan(&cols)) assert.Zero(t, cols) - _, err = pool.Exec(ctx, insertVariantSuppression134, exec156, 1, "us-east-1a", "Linux/UNIX", "default") + _, err = pool.Exec(ctx, insertVariantSuppression134, exec158, 1, "us-east-1a", "Linux/UNIX", "default") require.NoError(t, err) - _, err = pool.Exec(ctx, insertVariantSuppression134, exec156, 1, "us-east-1a", "Linux/UNIX", "default") + _, err = pool.Exec(ctx, insertVariantSuppression134, exec158, 1, "us-east-1a", "Linux/UNIX", "default") requireUniqueViolation134(t, err, "purchase_suppressions_execution_variant_key") - require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 156), "up must re-apply cleanly after down") + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 158), "up must re-apply cleanly after down") } diff --git a/internal/database/postgres/migrations/000157_drop_recommendations_11col_natural_key.down.sql b/internal/database/postgres/migrations/000159_drop_recommendations_11col_natural_key.down.sql similarity index 91% rename from internal/database/postgres/migrations/000157_drop_recommendations_11col_natural_key.down.sql rename to internal/database/postgres/migrations/000159_drop_recommendations_11col_natural_key.down.sql index 5fe3a5a3e..74e3b41cb 100644 --- a/internal/database/postgres/migrations/000157_drop_recommendations_11col_natural_key.down.sql +++ b/internal/database/postgres/migrations/000159_drop_recommendations_11col_natural_key.down.sql @@ -1,8 +1,8 @@ --- Rollback of 000157: recreate the 11-column unique index exactly as defined +-- Rollback of 000159: recreate the 11-column unique index exactly as defined -- by 000111 and the source-account index exactly as defined by 000120. -- -- Only safe together with a code rollback to the 11-column upsert. The newer --- code needs the 12-column arbiter from 000155, which this rollback keeps. The +-- code needs the 12-column arbiter from 000157, which this rollback keeps. The -- recommendations table is a regenerable cache, so rows that differ only by -- az_config (allowed while the 11-column index is absent) are collapsed to the -- most recently collected one before the indexes are rebuilt; the next diff --git a/internal/database/postgres/migrations/000157_drop_recommendations_11col_natural_key.up.sql b/internal/database/postgres/migrations/000159_drop_recommendations_11col_natural_key.up.sql similarity index 92% rename from internal/database/postgres/migrations/000157_drop_recommendations_11col_natural_key.up.sql rename to internal/database/postgres/migrations/000159_drop_recommendations_11col_natural_key.up.sql index 21ec8ab05..921780d72 100644 --- a/internal/database/postgres/migrations/000157_drop_recommendations_11col_natural_key.up.sql +++ b/internal/database/postgres/migrations/000159_drop_recommendations_11col_natural_key.up.sql @@ -1,8 +1,8 @@ --- Migration 000157: drop the 11-column recommendations_natural_key_az_tp_idx +-- Migration 000159: drop the 11-column recommendations_natural_key_az_tp_idx -- and the 12-column recommendations_natural_key_source_idx (#408, contract step -- of the RDS AZ-config identity work). -- --- Migration 000155 added the generated az_config column, the 12-column +-- Migration 000157 added the generated az_config column, the 12-column -- recommendations_natural_key_az_tp_azc_idx and the 13-column -- recommendations_natural_key_source_azc_idx, and kept the indexes without -- az_config so the previous upsert (ON CONFLICT on the 11 columns) kept diff --git a/internal/database/postgres/migrations/000157_drop_recommendations_11col_natural_key_test.go b/internal/database/postgres/migrations/000159_drop_recommendations_11col_natural_key_test.go similarity index 97% rename from internal/database/postgres/migrations/000157_drop_recommendations_11col_natural_key_test.go rename to internal/database/postgres/migrations/000159_drop_recommendations_11col_natural_key_test.go index 088b667c6..7dc570c9c 100644 --- a/internal/database/postgres/migrations/000157_drop_recommendations_11col_natural_key_test.go +++ b/internal/database/postgres/migrations/000159_drop_recommendations_11col_natural_key_test.go @@ -20,7 +20,7 @@ const insertRecForMigration153 = ` VALUES ($1::timestamptz, 'aws', 'rds', $2, 'db.r5.large', 'mysql', $3::jsonb, 0, 0, 1, 'all-upfront') ` -// TestMigration_DropRecommendations11ColNaturalKey locks down migration 000157 +// TestMigration_DropRecommendations11ColNaturalKey locks down migration 000159 // (issue #408, contract step): Single-AZ and Multi-AZ rows of the same class // coexist once the 11-column and 000120 source indexes are gone, the 12-column index still dedupes, // and the rollback recreates both indexes after collapsing those @@ -51,7 +51,7 @@ func TestMigration_DropRecommendations11ColNaturalKey(t *testing.T) { none = `{"details":{"engine":"mysql"}}` ) - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 157))) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 159))) require.True(t, indexExists("recommendations_natural_key_az_tp_idx")) require.True(t, indexExists("recommendations_natural_key_az_tp_azc_idx")) require.True(t, indexExists("recommendations_natural_key_source_idx")) @@ -62,7 +62,7 @@ func TestMigration_DropRecommendations11ColNaturalKey(t *testing.T) { _, err = pool.Exec(ctx, insertRecForMigration153, "2026-01-01T00:00:00Z", "us-west-2", single) require.NoError(t, err) - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 157)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 159)) assert.False(t, indexExists("recommendations_natural_key_az_tp_idx"), "11-column index must be dropped") assert.True(t, indexExists("recommendations_natural_key_az_tp_azc_idx"), "12-column index must remain") assert.False(t, indexExists("recommendations_natural_key_source_idx"), "000120's source index (no az_config) must be dropped") @@ -81,7 +81,7 @@ func TestMigration_DropRecommendations11ColNaturalKey(t *testing.T) { // Rollback: az_config-only duplicates collapse to the newest, the other // region's row is untouched, and the 11-column index is recreated with // its original shape. - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 157))) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 159))) assert.Equal(t, 1, count("us-east-1"), "only duplicates under the 11-column key may be deleted") assert.Equal(t, 1, count("us-west-2")) var kept string @@ -100,7 +100,7 @@ func TestMigration_DropRecommendations11ColNaturalKey(t *testing.T) { assert.Contains(t, indexDef, "(account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, source_account_key)") // Re-applying the up migration after a rollback works. - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 157)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 159)) assert.False(t, indexExists("recommendations_natural_key_az_tp_idx")) assert.False(t, indexExists("recommendations_natural_key_source_idx")) } diff --git a/internal/purchase/azure_pricing_integration_test.go b/internal/purchase/azure_pricing_integration_test.go index aa294d72b..24baa3566 100644 --- a/internal/purchase/azure_pricing_integration_test.go +++ b/internal/purchase/azure_pricing_integration_test.go @@ -98,10 +98,10 @@ func TestApproveAzurePricingRequiresMigrationAndRecollection(t *testing.T) { t.Run("106 invalidated old prices", func(t *testing.T) { approve(t, ErrStaleAzurePricing, 0) }) rec.UpfrontCost = 700 // The store's upsert needs the availability_zone, platform, tenancy and az_config columns - // and the 12-column index from migration 000155 (issue #408), and is gated until the - // 000156 expand (CheckAZConfigMigration). It also resets + // and the 12-column index from migration 000157 (issue #408), and is gated until the + // 000158 expand (CheckAZConfigMigration). It also resets // recommendations_state.last_collection_partial, added by migration 000129 (issue #87). - require.NoError(t, migrations.MigrateToVersion(ctx, pool, rampMigrationsPath(), 156)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, rampMigrationsPath(), 158)) require.NoError(t, pricing.UpsertRecommendations(ctx, time.Now().Add(time.Minute), []config.RecommendationRecord{rec}, collects)) t.Run("recollected prices purchase once", func(t *testing.T) { approve(t, nil, 1) }) } diff --git a/internal/scheduler/suppression_variant_integration_test.go b/internal/scheduler/suppression_variant_integration_test.go index d22affc3e..df53f179f 100644 --- a/internal/scheduler/suppression_variant_integration_test.go +++ b/internal/scheduler/suppression_variant_integration_test.go @@ -148,7 +148,7 @@ func TestSuppressionsMatchAcrossEC2Slugs_RealPostgres(t *testing.T) { } // TestRDSSuppressionScopedToAZConfig_RealPostgres is the issue #408 -// regression on a real database: after migrations 000155-000157 a Single-AZ +// regression on a real database: after migrations 000157-000159 a Single-AZ // and a Multi-AZ db.r5.large mysql rec coexist. Buying 2x Single-AZ must leave // the Multi-AZ rec at its full count, while a suppression row written before // az_config existed (variant set, az_config NULL) still hides both. From bd7f82044359d25189b12fcb0ef99694533084fc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 19:24:42 +0100 Subject: [PATCH 19/19] chore(db): renumber the az_config migrations to 000158-000160 #1253 took 000157 on main. Number-only: the files, the gate constants, the message checks, the migration-version literals and comments move by one. Co-Authored-By: Claude Opus 5.5 --- internal/api/az_config_gate_test.go | 2 +- .../api/azconfig_gate_integration_test.go | 6 ++-- internal/config/errors.go | 4 +-- internal/config/recommendation_identity.go | 2 +- .../config/recommendation_identity_test.go | 2 +- .../config/store_postgres_pgxmock_test.go | 2 +- .../config/store_postgres_recommendations.go | 2 +- ..._postgres_recommendations_azconfig_test.go | 4 +-- .../config/store_postgres_suppressions.go | 4 +-- internal/config/types.go | 2 +- ...000158_recommendations_az_config.down.sql} | 0 ...> 000158_recommendations_az_config.up.sql} | 6 ++-- ... 000158_recommendations_az_config_test.go} | 22 ++++++------- ..._purchase_suppressions_az_config.down.sql} | 2 +- ...59_purchase_suppressions_az_config.up.sql} | 12 +++---- ...9_purchase_suppressions_az_config_test.go} | 32 +++++++++---------- ...ecommendations_11col_natural_key.down.sql} | 4 +-- ..._recommendations_11col_natural_key.up.sql} | 4 +-- ...recommendations_11col_natural_key_test.go} | 10 +++--- .../azure_pricing_integration_test.go | 6 ++-- .../suppression_variant_integration_test.go | 2 +- 21 files changed, 65 insertions(+), 65 deletions(-) rename internal/database/postgres/migrations/{000157_recommendations_az_config.down.sql => 000158_recommendations_az_config.down.sql} (100%) rename internal/database/postgres/migrations/{000157_recommendations_az_config.up.sql => 000158_recommendations_az_config.up.sql} (94%) rename internal/database/postgres/migrations/{000157_recommendations_az_config_test.go => 000158_recommendations_az_config_test.go} (94%) rename internal/database/postgres/migrations/{000158_purchase_suppressions_az_config.down.sql => 000159_purchase_suppressions_az_config.down.sql} (97%) rename internal/database/postgres/migrations/{000158_purchase_suppressions_az_config.up.sql => 000159_purchase_suppressions_az_config.up.sql} (87%) rename internal/database/postgres/migrations/{000158_purchase_suppressions_az_config_test.go => 000159_purchase_suppressions_az_config_test.go} (87%) rename internal/database/postgres/migrations/{000159_drop_recommendations_11col_natural_key.down.sql => 000160_drop_recommendations_11col_natural_key.down.sql} (91%) rename internal/database/postgres/migrations/{000159_drop_recommendations_11col_natural_key.up.sql => 000160_drop_recommendations_11col_natural_key.up.sql} (92%) rename internal/database/postgres/migrations/{000159_drop_recommendations_11col_natural_key_test.go => 000160_drop_recommendations_11col_natural_key_test.go} (97%) diff --git a/internal/api/az_config_gate_test.go b/internal/api/az_config_gate_test.go index c22f948ff..d548aecb3 100644 --- a/internal/api/az_config_gate_test.go +++ b/internal/api/az_config_gate_test.go @@ -43,5 +43,5 @@ func TestGetRecommendations_MapsMissingAZConfigMigrationTo503(t *testing.T) { ce, ok := IsClientError(err) require.True(t, ok, "got %v", err) assert.Equal(t, 503, ce.code) - assert.Contains(t, ce.message, "000157 and 000158") + assert.Contains(t, ce.message, "000158 and 000159") } diff --git a/internal/api/azconfig_gate_integration_test.go b/internal/api/azconfig_gate_integration_test.go index c9d9314cb..e19927b25 100644 --- a/internal/api/azconfig_gate_integration_test.go +++ b/internal/api/azconfig_gate_integration_test.go @@ -40,14 +40,14 @@ func TestAZConfigGate_RefusesBeforeExpandMigrations(t *testing.T) { h := &Handler{config: store} exec := &config.PurchaseExecution{ExecutionID: uuid.NewString()} - for _, version := range []uint{156, 157} { // before both expands, then after only the first + for _, version := range []uint{157, 158} { // before both expands, then after only the first require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) _, err := h.persistExecutionAndSuppressions(ctx, exec, nil, "creator", "key") ce, ok := IsClientError(err) require.True(t, ok, "version %d: %v", version, err) assert.Equal(t, 503, ce.code) - assert.Contains(t, ce.message, "000157 and 000158") + assert.Contains(t, ce.message, "000158 and 000159") assert.Zero(t, store.txs, "no transaction may open before the gate") rec := expandOrderRDSRec("single-az", 100) @@ -66,7 +66,7 @@ func TestAZConfigGate_RefusesBeforeExpandMigrations(t *testing.T) { store.txs = 0 // the direct WithTx above is not the purchase path } - for _, version := range []uint{158, 159} { + for _, version := range []uint{159, 160} { require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) require.NoError(t, h.requireAZConfigSchema(ctx), "version %d", version) _, err := store.ListActiveSuppressions(ctx) diff --git a/internal/config/errors.go b/internal/config/errors.go index 4a95b539b..1e2890b99 100644 --- a/internal/config/errors.go +++ b/internal/config/errors.go @@ -10,8 +10,8 @@ var ErrAzurePricingNotReady = errors.New("azure pricing migration not ready") // The az_config expand migrations (issue #408). A later renumber is an edit of // these two constants (plus the migration files). const ( - AZConfigRecommendationsMigration = 157 - AZConfigSuppressionsMigration = 158 + AZConfigRecommendationsMigration = 158 + AZConfigSuppressionsMigration = 159 ) // ErrAZConfigMigrationNotReady means the recommendations and diff --git a/internal/config/recommendation_identity.go b/internal/config/recommendation_identity.go index 8e303a238..b390902c9 100644 --- a/internal/config/recommendation_identity.go +++ b/internal/config/recommendation_identity.go @@ -48,7 +48,7 @@ func (r *RecommendationRecord) ComputeIdentity() (ComputeIdentity, error) { // blob, VERBATIM. Single-AZ and Multi-AZ RDS reservations are different // offerings with different prices, yet share every other identity field // (issue #408). For AWS RDS rows it mirrors the generated az_config column of -// migration 000157 (COALESCE(payload->'details'->>'az_config', ”); the column +// migration 000158 (COALESCE(payload->'details'->>'az_config', ”); the column // is ” for every other rec, like IdentityAZConfig) and is pinned to it by the // shared vectors in TestDetailsAZConfig_Vectors: absent Details, JSON null, a // missing key and a null value all yield "". Only DatabaseDetails carries the diff --git a/internal/config/recommendation_identity_test.go b/internal/config/recommendation_identity_test.go index 2edfbca39..c83712179 100644 --- a/internal/config/recommendation_identity_test.go +++ b/internal/config/recommendation_identity_test.go @@ -46,7 +46,7 @@ func TestComputeIdentity_TopLevelKeyIgnoredAndMalformedFailsLoud(t *testing.T) { assert.Error(t, err) } -// The vectors mirror TestMigration_RecommendationsAZConfig (migration 000157, +// The vectors mirror TestMigration_RecommendationsAZConfig (migration 000158, // internal/database/postgres/migrations) so the Go reader and the generated // az_config column cannot diverge (issue #408). Details here is the details // blob itself. diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 03a43fd95..fd39a4d0b 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -33,7 +33,7 @@ func newMock(t *testing.T) pgxmock.PgxPoolIface { // ListActiveSuppressions run first, reporting the fully migrated version. func expectAZConfigReady(mock pgxmock.PgxPoolIface) { mock.ExpectQuery("SELECT version, dirty FROM schema_migrations"). - WillReturnRows(pgxmock.NewRows([]string{"version", "dirty"}).AddRow(159, false)) + WillReturnRows(pgxmock.NewRows([]string{"version", "dirty"}).AddRow(160, false)) } // anyArgs returns n AnyArg() matchers. diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index b40014d33..71b095e40 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -39,7 +39,7 @@ const azConfigExpandMigrationVersion = AZConfigSuppressionsMigration // so per-rec ON CONFLICT no longer collides on SQLSTATE 21000. Migration // 000109 added availability_zone, a GENERATED column derived from payload that // is never written here. Migration 000111 added the platform and tenancy -// generated columns the same way, and migration 000157 the az_config one; the +// generated columns the same way, and migration 000158 the az_config one; the // upsert arbiter is the 12-column recommendations_natural_key_az_tp_azc_idx // (issues #792, #806, #408). // diff --git a/internal/config/store_postgres_recommendations_azconfig_test.go b/internal/config/store_postgres_recommendations_azconfig_test.go index a9390acfc..8385eb931 100644 --- a/internal/config/store_postgres_recommendations_azconfig_test.go +++ b/internal/config/store_postgres_recommendations_azconfig_test.go @@ -26,7 +26,7 @@ func rdsAZConfigStoreRec(azConfig string, savings float64) config.Recommendation // Issue #408: Cost Explorer reports Single-AZ and Multi-AZ RDS reservations of // the same class, engine, term and payment as separate recommendations. Once // the store upserts on the 12-column index (with the generated az_config -// column) and migration 000159 drops the 11-column index, both rows upsert in +// column) and migration 000160 drops the 11-column index, both rows upsert in // ONE batch (the shape persistCollection writes) and both persist. Before, // the batch failed with SQLSTATE 21000 ("ON CONFLICT DO UPDATE command cannot // affect row a second time") and took the whole collection write down. @@ -40,7 +40,7 @@ func TestPostgresStore_UpsertRecommendations_AZConfigBatch(t *testing.T) { var oldIdx bool require.NoError(t, pool.QueryRow(ctx, `SELECT EXISTS (SELECT 1 FROM pg_indexes WHERE indexname = 'recommendations_natural_key_az_tp_idx')`).Scan(&oldIdx)) - require.False(t, oldIdx, "11-column index must be gone after the full migration chain (000159)") + require.False(t, oldIdx, "11-column index must be gone after the full migration chain (000160)") savingsByAZConfig := func(t *testing.T) map[string]float64 { t.Helper() diff --git a/internal/config/store_postgres_suppressions.go b/internal/config/store_postgres_suppressions.go index 94c0d7d53..01adc065d 100644 --- a/internal/config/store_postgres_suppressions.go +++ b/internal/config/store_postgres_suppressions.go @@ -109,7 +109,7 @@ func (s *PostgresStore) CreateSuppressionTx(ctx context.Context, tx pgx.Tx, sup // 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). - // az_config is NULL when empty: unknown or not applicable (migration 000158). + // az_config is NULL when empty: unknown or not applicable (migration 000159). var az, platform, tenancy, azConfig *string if sup.Variant != nil { az, platform, tenancy = &sup.Variant.AvailabilityZone, &sup.Variant.Platform, &sup.Variant.Tenancy @@ -210,7 +210,7 @@ func (s *PostgresStore) ListActiveSuppressions(ctx context.Context) ([]PurchaseS // 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. A NULL az_config (rows written before -// migration 000158, or a rec with none) is read as "" = any az_config. +// migration 000159, or a rec with none) is read as "" = any az_config. func suppressionVariantFromColumns(az, platform, tenancy, azConfig *string) *SuppressionVariant { if az == nil || platform == nil || tenancy == nil { return nil diff --git a/internal/config/types.go b/internal/config/types.go index 6a9d58482..e55729b5c 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -806,7 +806,7 @@ type PurchaseSuppression struct { // #806, #408). Empty AvailabilityZone, Platform and Tenancy are known values // (a regional rec, a non-EC2 rec), not wildcards. AZConfig is the exception: // "" means "not applicable or unknown" and matches any az_config (see -// AppliesTo), because suppressions written before migration 000158 have none +// AppliesTo), because suppressions written before migration 000159 have none // and must keep suppressing both the Single-AZ and the Multi-AZ rec. type SuppressionVariant struct { AvailabilityZone string `json:"availability_zone"` diff --git a/internal/database/postgres/migrations/000157_recommendations_az_config.down.sql b/internal/database/postgres/migrations/000158_recommendations_az_config.down.sql similarity index 100% rename from internal/database/postgres/migrations/000157_recommendations_az_config.down.sql rename to internal/database/postgres/migrations/000158_recommendations_az_config.down.sql diff --git a/internal/database/postgres/migrations/000157_recommendations_az_config.up.sql b/internal/database/postgres/migrations/000158_recommendations_az_config.up.sql similarity index 94% rename from internal/database/postgres/migrations/000157_recommendations_az_config.up.sql rename to internal/database/postgres/migrations/000158_recommendations_az_config.up.sql index 61e8b3450..3b282e9bc 100644 --- a/internal/database/postgres/migrations/000157_recommendations_az_config.up.sql +++ b/internal/database/postgres/migrations/000158_recommendations_az_config.up.sql @@ -1,4 +1,4 @@ --- Migration 000157: az_config generated column and a 12-column unique index on +-- Migration 000158: az_config generated column and a 12-column unique index on -- recommendations (#408, expand step of the RDS AZ-config identity work). -- -- Cost Explorer reports Single-AZ and Multi-AZ RDS reservations of the same @@ -25,13 +25,13 @@ -- - recommendations_natural_key_source_azc_idx: the source-account key of -- migration 000120 (#799) plus az_config. Every unique index without -- az_config rejects a Single-AZ/Multi-AZ pair, so the source-aware key has --- to gain the column too; 000159 retires the 000120 index it replaces. +-- to gain the column too; 000160 retires the 000120 index it replaces. -- -- The existing 11-column recommendations_natural_key_az_tp_idx and the -- 000120 recommendations_natural_key_source_idx are KEPT, so the current -- ON CONFLICT on the 11 columns keeps working for old and new code. Two rows -- that differ only by az_config are still rejected by those indexes until --- migration 000159 drops them (applied only once the store upserts on the +-- migration 000160 drops them (applied only once the store upserts on the -- 12-column index). -- -- Lock impact: ADD COLUMN ... GENERATED ... STORED rewrites the table and the diff --git a/internal/database/postgres/migrations/000157_recommendations_az_config_test.go b/internal/database/postgres/migrations/000158_recommendations_az_config_test.go similarity index 94% rename from internal/database/postgres/migrations/000157_recommendations_az_config_test.go rename to internal/database/postgres/migrations/000158_recommendations_az_config_test.go index 7fc249ea1..dd94a95c5 100644 --- a/internal/database/postgres/migrations/000157_recommendations_az_config_test.go +++ b/internal/database/postgres/migrations/000158_recommendations_az_config_test.go @@ -21,14 +21,14 @@ const insertRecForMigration152 = ` VALUES (NOW(), 'aws', 'rds', 'us-east-1', $1, 'mysql', $2::jsonb, 0, 0, 1, 'all-upfront') ` -const insertRecForMigration157Scoped = ` +const insertRecForMigration158Scoped = ` INSERT INTO recommendations (collected_at, provider, service, region, resource_type, engine, payload, upfront_cost, monthly_savings, term, payment_option) VALUES (NOW(), $3, $4, 'us-east-1', $1, 'mysql', $2::jsonb, 0, 0, 1, 'all-upfront') ` -// TestMigration_RecommendationsAZConfig locks down migration 000157 (issue +// TestMigration_RecommendationsAZConfig locks down migration 000158 (issue // #408, expand step): the generated az_config column is read verbatim from // payload->'details', the 11-column index is kept so the previous upsert still // works, the 12-column index is added, and rollback removes only the new @@ -43,7 +43,7 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { defer container.Cleanup(ctx) pool := container.DB.Pool() - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 157))) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 158))) // provider and service default to aws/rds; the column is scoped to AWS RDS // like IdentityAZConfig, so other providers and services read "". @@ -70,11 +70,11 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { if v.provider == "" { v.provider, v.service = "aws", "rds" } - _, err = pool.Exec(ctx, insertRecForMigration157Scoped, name, v.payload, v.provider, v.service) + _, err = pool.Exec(ctx, insertRecForMigration158Scoped, name, v.payload, v.provider, v.service) require.NoError(t, err) } - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 157)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 158)) for name, v := range vectors { var got string @@ -89,7 +89,7 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { var exists bool require.NoError(t, pool.QueryRow(ctx, `SELECT EXISTS (SELECT 1 FROM pg_indexes WHERE tablename = 'recommendations' AND indexname = $1)`, idx).Scan(&exists)) - assert.True(t, exists, "index %s must exist after 000157", idx) + assert.True(t, exists, "index %s must exist after 000158", idx) } var indexDef string require.NoError(t, pool.QueryRow(ctx, @@ -102,7 +102,7 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { assert.Contains(t, indexDef, "(account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, az_config, source_account_key)") // The previous code's upsert (11-column ON CONFLICT) still works and - // updates the existing row in place, so 000157 can be applied before the + // updates the existing row in place, so 000158 can be applied before the // store switch is released. singleAZ := `{"details":{"az_config":"single-az"}}` _, err = pool.Exec(ctx, insertRecForMigration152+` @@ -112,12 +112,12 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { _, err = pool.Exec(ctx, insertRecForMigration152+` ON CONFLICT (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy) DO UPDATE SET upfront_cost = 5`, "pair", singleAZ) - require.NoError(t, err, "the existing 11-column upsert must keep working after 000157") + require.NoError(t, err, "the existing 11-column upsert must keep working after 000158") var n int require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*) FROM recommendations WHERE resource_type = 'pair'`).Scan(&n)) assert.Equal(t, 1, n) - // Known limitation, resolved by 000159: a row differing only by az_config + // Known limitation, resolved by 000160: a row differing only by az_config // is still rejected by the kept indexes without az_config. _, err = pool.Exec(ctx, insertRecForMigration152, "pair", `{"details":{"az_config":"multi-az"}}`) var pgErr *pgconn.PgError @@ -126,7 +126,7 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { assert.Contains(t, []string{"recommendations_natural_key_az_tp_idx", "recommendations_natural_key_source_idx"}, pgErr.ConstraintName) // Rollback removes the new column and index only. - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 157))) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 158))) var colExists bool require.NoError(t, pool.QueryRow(ctx, `SELECT EXISTS (SELECT 1 FROM information_schema.columns WHERE table_name = 'recommendations' AND column_name = 'az_config')`).Scan(&colExists)) @@ -144,5 +144,5 @@ func TestMigration_RecommendationsAZConfig(t *testing.T) { require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*) FROM recommendations`).Scan(&n)) assert.Equal(t, len(vectors)+1, n, "rollback must not delete rows") - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 157)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 158)) } diff --git a/internal/database/postgres/migrations/000158_purchase_suppressions_az_config.down.sql b/internal/database/postgres/migrations/000159_purchase_suppressions_az_config.down.sql similarity index 97% rename from internal/database/postgres/migrations/000158_purchase_suppressions_az_config.down.sql rename to internal/database/postgres/migrations/000159_purchase_suppressions_az_config.down.sql index 3fc4e19d0..6eb82a9ec 100644 --- a/internal/database/postgres/migrations/000158_purchase_suppressions_az_config.down.sql +++ b/internal/database/postgres/migrations/000159_purchase_suppressions_az_config.down.sql @@ -1,4 +1,4 @@ --- Rollback of 000158: restore the 000134 suppression key without az_config. +-- Rollback of 000159: restore the 000134 suppression key without az_config. -- -- Rows of one execution that differ only by az_config are merged into one row -- per execution, 6-tuple and variant before the narrower UNIQUE constraint is diff --git a/internal/database/postgres/migrations/000158_purchase_suppressions_az_config.up.sql b/internal/database/postgres/migrations/000159_purchase_suppressions_az_config.up.sql similarity index 87% rename from internal/database/postgres/migrations/000158_purchase_suppressions_az_config.up.sql rename to internal/database/postgres/migrations/000159_purchase_suppressions_az_config.up.sql index 5045f2948..96e274a5f 100644 --- a/internal/database/postgres/migrations/000158_purchase_suppressions_az_config.up.sql +++ b/internal/database/postgres/migrations/000159_purchase_suppressions_az_config.up.sql @@ -1,7 +1,7 @@ --- Migration 000158 (issue #408): widen the purchase suppression variant with +-- Migration 000159 (issue #408): widen the purchase suppression variant with -- the AWS RDS az_config ("single-az" / "multi-az"). -- --- Migration 000159 lets a Single-AZ and a Multi-AZ RDS rec of the same class, +-- Migration 000160 lets a Single-AZ and a Multi-AZ RDS rec of the same class, -- engine, term and payment coexist, but the suppression key (000134) cannot -- tell them apart: buying 2x Single-AZ would write a suppression that also -- matched, and hid, the Multi-AZ rec. The variant now carries az_config for @@ -20,12 +20,12 @@ -- so NULL rows stay unique per execution, 6-tuple and variant as before). -- A suppression with an az_config must also carry the rest of the variant. -- --- DEPLOY ORDER: apply this (and 000157) before deploying the code that writes +-- DEPLOY ORDER: apply this (and 000158) before deploying the code that writes -- az_config (the insert names the column, and the purchase path and the --- suppression listing need it). Both are expand steps. 000159 is the contract --- step and must wait until that code is live everywhere (see the 000159 +-- suppression listing need it). Both are expand steps. 000160 is the contract +-- step and must wait until that code is live everywhere (see the 000160 -- header); never dispatch database-migration.yml with the default of all --- pending steps once 000159 is pending. +-- pending steps once 000160 is pending. ALTER TABLE purchase_suppressions ADD COLUMN IF NOT EXISTS az_config TEXT; diff --git a/internal/database/postgres/migrations/000158_purchase_suppressions_az_config_test.go b/internal/database/postgres/migrations/000159_purchase_suppressions_az_config_test.go similarity index 87% rename from internal/database/postgres/migrations/000158_purchase_suppressions_az_config_test.go rename to internal/database/postgres/migrations/000159_purchase_suppressions_az_config_test.go index d2f91166f..8cf13f8bd 100644 --- a/internal/database/postgres/migrations/000158_purchase_suppressions_az_config_test.go +++ b/internal/database/postgres/migrations/000159_purchase_suppressions_az_config_test.go @@ -13,9 +13,9 @@ import ( ) const ( - exec158 = "00000000-0000-0000-0000-000000158a01" + exec159 = "00000000-0000-0000-0000-000000159a01" - insertRDSSuppression158 = ` + insertRDSSuppression159 = ` INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, resource_type, engine, suppressed_count, expires_at, availability_zone, platform, tenancy, az_config) @@ -23,7 +23,7 @@ const ( NOW() + INTERVAL '7 days', '', '', '', $3)` ) -// TestMigration_PurchaseSuppressionsAZConfig locks down migration 000158 +// TestMigration_PurchaseSuppressionsAZConfig locks down migration 000159 // (issue #408): rows seeded before the migration, with and without a 000134 // variant, survive it with a NULL az_config (the wildcard backfill rule); // afterwards Single-AZ and Multi-AZ rows of one execution coexist while a @@ -35,7 +35,7 @@ func TestMigration_PurchaseSuppressionsAZConfig(t *testing.T) { ctx := context.Background() container := testhelpers.RequireEmptyDatabase(ctx, t) pool, path := container.DB.Pool(), getMigrationsPath() - prev := previousMigrationVersion(t, 158) + prev := previousMigrationVersion(t, 159) require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) // Pre-migration rows: one legacy 6-tuple row (NULL variant) and one RDS @@ -43,35 +43,35 @@ func TestMigration_PurchaseSuppressionsAZConfig(t *testing.T) { _, err := pool.Exec(ctx, ` INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, resource_type, engine, suppressed_count, expires_at) - VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.t3.micro', 'mysql', 1, NOW() + INTERVAL '7 days')`, exec158) + VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.t3.micro', 'mysql', 1, NOW() + INTERVAL '7 days')`, exec159) require.NoError(t, err) _, err = pool.Exec(ctx, ` 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', 'relational-db', 'us-east-1', 'db.r5.large', 'mysql', 2, NOW() + INTERVAL '7 days', '', '', '')`, exec158) + VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.r5.large', 'mysql', 2, NOW() + INTERVAL '7 days', '', '', '')`, exec159) require.NoError(t, err) - require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 158)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 159)) var rows, nulls int require.NoError(t, pool.QueryRow(ctx, `SELECT count(*), count(*) FILTER (WHERE az_config IS NULL) - FROM purchase_suppressions WHERE execution_id = $1`, exec158).Scan(&rows, &nulls)) + FROM purchase_suppressions WHERE execution_id = $1`, exec159).Scan(&rows, &nulls)) assert.Equal(t, 2, rows, "existing rows survive") assert.Equal(t, 2, nulls, "existing rows get a NULL az_config: a wildcard") // Single-AZ and Multi-AZ rows coexist next to the NULL row of the same variant. for _, v := range []string{"single-az", "multi-az"} { - _, err = pool.Exec(ctx, insertRDSSuppression158, exec158, 3, v) + _, err = pool.Exec(ctx, insertRDSSuppression159, exec159, 3, v) require.NoError(t, err, v) } - _, err = pool.Exec(ctx, insertRDSSuppression158, exec158, 3, "multi-az") + _, err = pool.Exec(ctx, insertRDSSuppression159, exec159, 3, "multi-az") requireUniqueViolation134(t, err, "purchase_suppressions_execution_variant_azc_key") // An az_config without the rest of the variant is rejected. _, err = pool.Exec(ctx, ` INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, resource_type, engine, suppressed_count, expires_at, az_config) - VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.m5.large', 'mysql', 1, NOW() + INTERVAL '7 days', 'multi-az')`, exec158) + VALUES ($1, 'acct', 'aws', 'relational-db', 'us-east-1', 'db.m5.large', 'mysql', 1, NOW() + INTERVAL '7 days', 'multi-az')`, exec159) require.Error(t, err) assert.Contains(t, err.Error(), "purchase_suppressions_az_config_needs_variant") @@ -80,20 +80,20 @@ func TestMigration_PurchaseSuppressionsAZConfig(t *testing.T) { require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) var total int require.NoError(t, pool.QueryRow(ctx, `SELECT count(*), COALESCE(SUM(suppressed_count), 0) - FROM purchase_suppressions WHERE execution_id = $1 AND resource_type = 'db.r5.large'`, exec158).Scan(&rows, &total)) + FROM purchase_suppressions WHERE execution_id = $1 AND resource_type = 'db.r5.large'`, exec159).Scan(&rows, &total)) assert.Equal(t, 1, rows) assert.Equal(t, 8, total) require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM purchase_suppressions - WHERE execution_id = $1 AND resource_type = 'db.t3.micro'`, exec158).Scan(&rows)) + WHERE execution_id = $1 AND resource_type = 'db.t3.micro'`, exec159).Scan(&rows)) assert.Equal(t, 1, rows) var cols int require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM information_schema.columns WHERE table_name = 'purchase_suppressions' AND column_name = 'az_config'`).Scan(&cols)) assert.Zero(t, cols) - _, err = pool.Exec(ctx, insertVariantSuppression134, exec158, 1, "us-east-1a", "Linux/UNIX", "default") + _, err = pool.Exec(ctx, insertVariantSuppression134, exec159, 1, "us-east-1a", "Linux/UNIX", "default") require.NoError(t, err) - _, err = pool.Exec(ctx, insertVariantSuppression134, exec158, 1, "us-east-1a", "Linux/UNIX", "default") + _, err = pool.Exec(ctx, insertVariantSuppression134, exec159, 1, "us-east-1a", "Linux/UNIX", "default") requireUniqueViolation134(t, err, "purchase_suppressions_execution_variant_key") - require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 158), "up must re-apply cleanly after down") + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 159), "up must re-apply cleanly after down") } diff --git a/internal/database/postgres/migrations/000159_drop_recommendations_11col_natural_key.down.sql b/internal/database/postgres/migrations/000160_drop_recommendations_11col_natural_key.down.sql similarity index 91% rename from internal/database/postgres/migrations/000159_drop_recommendations_11col_natural_key.down.sql rename to internal/database/postgres/migrations/000160_drop_recommendations_11col_natural_key.down.sql index 74e3b41cb..9d09ea7d5 100644 --- a/internal/database/postgres/migrations/000159_drop_recommendations_11col_natural_key.down.sql +++ b/internal/database/postgres/migrations/000160_drop_recommendations_11col_natural_key.down.sql @@ -1,8 +1,8 @@ --- Rollback of 000159: recreate the 11-column unique index exactly as defined +-- Rollback of 000160: recreate the 11-column unique index exactly as defined -- by 000111 and the source-account index exactly as defined by 000120. -- -- Only safe together with a code rollback to the 11-column upsert. The newer --- code needs the 12-column arbiter from 000157, which this rollback keeps. The +-- code needs the 12-column arbiter from 000158, which this rollback keeps. The -- recommendations table is a regenerable cache, so rows that differ only by -- az_config (allowed while the 11-column index is absent) are collapsed to the -- most recently collected one before the indexes are rebuilt; the next diff --git a/internal/database/postgres/migrations/000159_drop_recommendations_11col_natural_key.up.sql b/internal/database/postgres/migrations/000160_drop_recommendations_11col_natural_key.up.sql similarity index 92% rename from internal/database/postgres/migrations/000159_drop_recommendations_11col_natural_key.up.sql rename to internal/database/postgres/migrations/000160_drop_recommendations_11col_natural_key.up.sql index 921780d72..548d5d749 100644 --- a/internal/database/postgres/migrations/000159_drop_recommendations_11col_natural_key.up.sql +++ b/internal/database/postgres/migrations/000160_drop_recommendations_11col_natural_key.up.sql @@ -1,8 +1,8 @@ --- Migration 000159: drop the 11-column recommendations_natural_key_az_tp_idx +-- Migration 000160: drop the 11-column recommendations_natural_key_az_tp_idx -- and the 12-column recommendations_natural_key_source_idx (#408, contract step -- of the RDS AZ-config identity work). -- --- Migration 000157 added the generated az_config column, the 12-column +-- Migration 000158 added the generated az_config column, the 12-column -- recommendations_natural_key_az_tp_azc_idx and the 13-column -- recommendations_natural_key_source_azc_idx, and kept the indexes without -- az_config so the previous upsert (ON CONFLICT on the 11 columns) kept diff --git a/internal/database/postgres/migrations/000159_drop_recommendations_11col_natural_key_test.go b/internal/database/postgres/migrations/000160_drop_recommendations_11col_natural_key_test.go similarity index 97% rename from internal/database/postgres/migrations/000159_drop_recommendations_11col_natural_key_test.go rename to internal/database/postgres/migrations/000160_drop_recommendations_11col_natural_key_test.go index 7dc570c9c..8eeaaf335 100644 --- a/internal/database/postgres/migrations/000159_drop_recommendations_11col_natural_key_test.go +++ b/internal/database/postgres/migrations/000160_drop_recommendations_11col_natural_key_test.go @@ -20,7 +20,7 @@ const insertRecForMigration153 = ` VALUES ($1::timestamptz, 'aws', 'rds', $2, 'db.r5.large', 'mysql', $3::jsonb, 0, 0, 1, 'all-upfront') ` -// TestMigration_DropRecommendations11ColNaturalKey locks down migration 000159 +// TestMigration_DropRecommendations11ColNaturalKey locks down migration 000160 // (issue #408, contract step): Single-AZ and Multi-AZ rows of the same class // coexist once the 11-column and 000120 source indexes are gone, the 12-column index still dedupes, // and the rollback recreates both indexes after collapsing those @@ -51,7 +51,7 @@ func TestMigration_DropRecommendations11ColNaturalKey(t *testing.T) { none = `{"details":{"engine":"mysql"}}` ) - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 159))) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 160))) require.True(t, indexExists("recommendations_natural_key_az_tp_idx")) require.True(t, indexExists("recommendations_natural_key_az_tp_azc_idx")) require.True(t, indexExists("recommendations_natural_key_source_idx")) @@ -62,7 +62,7 @@ func TestMigration_DropRecommendations11ColNaturalKey(t *testing.T) { _, err = pool.Exec(ctx, insertRecForMigration153, "2026-01-01T00:00:00Z", "us-west-2", single) require.NoError(t, err) - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 159)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 160)) assert.False(t, indexExists("recommendations_natural_key_az_tp_idx"), "11-column index must be dropped") assert.True(t, indexExists("recommendations_natural_key_az_tp_azc_idx"), "12-column index must remain") assert.False(t, indexExists("recommendations_natural_key_source_idx"), "000120's source index (no az_config) must be dropped") @@ -81,7 +81,7 @@ func TestMigration_DropRecommendations11ColNaturalKey(t *testing.T) { // Rollback: az_config-only duplicates collapse to the newest, the other // region's row is untouched, and the 11-column index is recreated with // its original shape. - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 159))) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 160))) assert.Equal(t, 1, count("us-east-1"), "only duplicates under the 11-column key may be deleted") assert.Equal(t, 1, count("us-west-2")) var kept string @@ -100,7 +100,7 @@ func TestMigration_DropRecommendations11ColNaturalKey(t *testing.T) { assert.Contains(t, indexDef, "(account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, source_account_key)") // Re-applying the up migration after a rollback works. - require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 159)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 160)) assert.False(t, indexExists("recommendations_natural_key_az_tp_idx")) assert.False(t, indexExists("recommendations_natural_key_source_idx")) } diff --git a/internal/purchase/azure_pricing_integration_test.go b/internal/purchase/azure_pricing_integration_test.go index 24baa3566..508dac5f8 100644 --- a/internal/purchase/azure_pricing_integration_test.go +++ b/internal/purchase/azure_pricing_integration_test.go @@ -98,10 +98,10 @@ func TestApproveAzurePricingRequiresMigrationAndRecollection(t *testing.T) { t.Run("106 invalidated old prices", func(t *testing.T) { approve(t, ErrStaleAzurePricing, 0) }) rec.UpfrontCost = 700 // The store's upsert needs the availability_zone, platform, tenancy and az_config columns - // and the 12-column index from migration 000157 (issue #408), and is gated until the - // 000158 expand (CheckAZConfigMigration). It also resets + // and the 12-column index from migration 000158 (issue #408), and is gated until the + // 000159 expand (CheckAZConfigMigration). It also resets // recommendations_state.last_collection_partial, added by migration 000129 (issue #87). - require.NoError(t, migrations.MigrateToVersion(ctx, pool, rampMigrationsPath(), 158)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, rampMigrationsPath(), 159)) require.NoError(t, pricing.UpsertRecommendations(ctx, time.Now().Add(time.Minute), []config.RecommendationRecord{rec}, collects)) t.Run("recollected prices purchase once", func(t *testing.T) { approve(t, nil, 1) }) } diff --git a/internal/scheduler/suppression_variant_integration_test.go b/internal/scheduler/suppression_variant_integration_test.go index df53f179f..f00af4aaf 100644 --- a/internal/scheduler/suppression_variant_integration_test.go +++ b/internal/scheduler/suppression_variant_integration_test.go @@ -148,7 +148,7 @@ func TestSuppressionsMatchAcrossEC2Slugs_RealPostgres(t *testing.T) { } // TestRDSSuppressionScopedToAZConfig_RealPostgres is the issue #408 -// regression on a real database: after migrations 000157-000159 a Single-AZ +// regression on a real database: after migrations 000158-000160 a Single-AZ // and a Multi-AZ db.r5.large mysql rec coexist. Buying 2x Single-AZ must leave // the Multi-AZ rec at its full count, while a suppression row written before // az_config existed (variant set, az_config NULL) still hides both.