diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 44a6955c..73460727 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -3391,11 +3391,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 00000000..b1572a18 --- /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 721da06d..04b37c8c 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 cc42e037..69122120 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,7 +88,7 @@ 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 5f8fcf50..26c1372b 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/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index 18870490..d4cee221 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -25,7 +25,8 @@ const azurePricingMigrationVersion = 106 // 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 @@ -35,8 +36,9 @@ const azurePricingMigrationVersion = 106 // 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 @@ -184,7 +186,7 @@ func insertRecommendationsBatch(ctx context.Context, tx pgx.Tx, collectedAt time resource_type, engine, payload, upfront_cost, monthly_savings, term, payment_option) VALUES %s - 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/config/store_postgres_recommendations_azconfig_test.go b/internal/config/store_postgres_recommendations_azconfig_test.go new file mode 100644 index 00000000..8eb61c82 --- /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/000140_recommendations_az_config.down.sql b/internal/database/postgres/migrations/000140_recommendations_az_config.down.sql new file mode 100644 index 00000000..23050ab1 --- /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 00000000..ed47c279 --- /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 00000000..801dc4e9 --- /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)) +} 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 00000000..a5ab50e3 --- /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 00000000..71422a1f --- /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 00000000..20aa2dce --- /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")) +} diff --git a/internal/purchase/azure_pricing_integration_test.go b/internal/purchase/azure_pricing_integration_test.go index 59fb3e30..cc531058 100644 --- a/internal/purchase/azure_pricing_integration_test.go +++ b/internal/purchase/azure_pricing_integration_test.go @@ -97,10 +97,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) }) } diff --git a/internal/scheduler/recommendation_identity_test.go b/internal/scheduler/recommendation_identity_test.go index c3d050d6..8d98e686 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 c7514c95..6db3361b 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 }