diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index 1d4cf6bd..f5422c07 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -487,6 +487,9 @@ func (m *mockConfigStore) GetRecommendationsFreshness(_ context.Context) (*confi func (m *mockConfigStore) SetRecommendationsCollectionError(_ context.Context, _ string) error { return nil } +func (m *mockConfigStore) SetRecommendationsCollectionPartial(_ context.Context, _ *config.RecommendationsCollectionPartial) error { + return nil +} func (m *mockConfigStore) MarkCollectionStarted(_ context.Context) (string, bool, error) { return "mock-owner-token", true, nil } diff --git a/internal/api/handler_recommendations_test.go b/internal/api/handler_recommendations_test.go index 87fed7b0..6395e124 100644 --- a/internal/api/handler_recommendations_test.go +++ b/internal/api/handler_recommendations_test.go @@ -140,6 +140,54 @@ func TestHandler_getRecommendationsFreshness(t *testing.T) { assert.WithinDuration(t, now, *got.LastCollectedAt, time.Second) }) + // Issue #87: the freshness payload is what the dashboard reads, so a + // partial sweep must reach it as a structured, sanitized field. + t.Run("surfaces last_collection_partial", func(t *testing.T) { + mockStore := new(MockConfigStore) + now := time.Now().UTC() + partial := &config.RecommendationsCollectionPartial{ + Providers: []config.PartialProviderSweep{{ + Provider: "azure", + IncompleteAccounts: 1, + AttemptedScopes: 10, + FailedScopes: 9, + Reasons: map[string]int{config.PartialReasonThrottled: 9}, + }}, + } + mockStore.On("GetRecommendationsFreshness", ctx). + Return(&config.RecommendationsFreshness{LastCollectedAt: &now, LastCollectionPartial: partial}, nil) + + handler := &Handler{config: mockStore, apiKey: "test-key"} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "test-key"}, + } + + got, err := handler.getRecommendationsFreshness(ctx, req) + require.NoError(t, err) + body, err := json.Marshal(got) + require.NoError(t, err) + assert.JSONEq(t, `{"providers":[{"provider":"azure","incomplete_accounts":1,"attempted_scopes":10,`+ + `"failed_scopes":9,"failed_details":0,"reasons":{"throttled":9}}]}`, + string(mustField(t, body, "last_collection_partial"))) + }) + + t.Run("complete collection serializes last_collection_partial as null", func(t *testing.T) { + mockStore := new(MockConfigStore) + mockStore.On("GetRecommendationsFreshness", ctx).Return(&config.RecommendationsFreshness{}, nil) + + handler := &Handler{config: mockStore, apiKey: "test-key"} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "test-key"}, + } + + got, err := handler.getRecommendationsFreshness(ctx, req) + require.NoError(t, err) + body, err := json.Marshal(got) + require.NoError(t, err) + assert.Equal(t, "null", string(mustField(t, body, "last_collection_partial")), + "the key must always be present so the frontend can rely on it") + }) + t.Run("surfaces store error", func(t *testing.T) { mockStore := new(MockConfigStore) mockStore.On("GetRecommendationsFreshness", ctx).Return(nil, errors.New("db down")) @@ -766,3 +814,13 @@ func TestConfidenceBucketFor(t *testing.T) { }) } } + +// mustField returns the raw JSON of a top-level key, failing if it is absent. +func mustField(t *testing.T, body []byte, key string) json.RawMessage { + t.Helper() + var fields map[string]json.RawMessage + require.NoError(t, json.Unmarshal(body, &fields)) + raw, ok := fields[key] + require.True(t, ok, "response must carry %q", key) + return raw +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index ccf28ba9..5ad33ce0 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -255,6 +255,29 @@ paths: '403': $ref: '#/components/responses/Forbidden' + /api/recommendations/freshness: + get: + operationId: getRecommendationsFreshness + tags: [Recommendations] + summary: Get the recommendations cache freshness and last collection outcome + description: > + Returns when recommendations were last collected, the most recent + collection error, whether a collection is in flight, and whether the + last collection was partial (some subscriptions, projects or scopes + could not be queried, so the stored recommendations under-state the + real opportunity). + responses: + '200': + description: Freshness state + content: + application/json: + schema: + $ref: '#/components/schemas/RecommendationsFreshness' + '401': + $ref: '#/components/responses/Unauthorized' + '403': + $ref: '#/components/responses/Forbidden' + # ---- Purchase Plans ----------------------------------------------------- /api/plans: get: @@ -3196,6 +3219,75 @@ components: items: type: string + RecommendationsFreshness: + type: object + required: [last_collected_at, last_collection_error, last_collection_started_at, last_collection_partial] + properties: + last_collected_at: + type: string + format: date-time + nullable: true + description: When recommendations were last persisted; null on a cold start. + last_collection_error: + type: string + nullable: true + description: Error from the most recent collection when a provider failed; null otherwise. + last_collection_started_at: + type: string + format: date-time + nullable: true + description: Set while a collection is in flight. + last_collection_partial: + nullable: true + description: > + Non-null when the most recent persisted collection kept rows from + at least one incomplete sweep. Cleared by the next collection whose + sweeps are all complete. Carries counts and reason categories only. + allOf: + - $ref: '#/components/schemas/RecommendationsCollectionPartial' + + RecommendationsCollectionPartial: + type: object + required: [providers] + properties: + providers: + type: array + items: + $ref: '#/components/schemas/PartialProviderSweep' + + PartialProviderSweep: + type: object + required: [provider, incomplete_accounts, attempted_scopes, failed_scopes, failed_details, reasons] + properties: + provider: + type: string + enum: [aws, azure, gcp] + incomplete_accounts: + type: integer + description: Accounts (or the ambient host identity) whose sweep was incomplete. + attempted_scopes: + type: integer + description: > + Scopes (Azure subscriptions, GCP projects) the incomplete sweeps + were asked to cover; 0 when the provider does not report it (AWS). + failed_scopes: + type: integer + description: Scopes that could not be queried. + failed_details: + type: integer + description: Individual recommendation details rejected inside answering scopes (AWS). + reasons: + type: object + description: > + Failure count per reason category. Keys are drawn from a closed + set: permission_denied, throttled, timeout, canceled, not_found, + provider_unavailable, other. + additionalProperties: + type: integer + example: + permission_denied: 2 + throttled: 1 + # -- Plans -------------------------------------------------------------- PlansResponse: type: object diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 5f237b29..6483836d 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -540,6 +540,10 @@ type StoreInterface interface { ListStoredRecommendations(ctx context.Context, filter RecommendationFilter) ([]RecommendationRecord, error) GetRecommendationsFreshness(ctx context.Context) (*RecommendationsFreshness, error) SetRecommendationsCollectionError(ctx context.Context, errMsg string) error + // SetRecommendationsCollectionPartial records (or, with nil, clears) the + // most recent collection's incomplete-sweep summary. Upsert/Replace + // clear it, so a later complete collection drops the marker. + SetRecommendationsCollectionPartial(ctx context.Context, partial *RecommendationsCollectionPartial) error // MarkCollectionStarted atomically sets last_collection_started_at = now // only when no in-flight collection is running (last_collection_started_at IS NULL // OR older than 5 minutes), and stamps a freshly generated owner token diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index f0937c05..d95877c4 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -45,8 +45,9 @@ func (s *PostgresStore) ReplaceRecommendations(ctx context.Context, collectedAt if _, err := tx.Exec(ctx, ` UPDATE recommendations_state - SET last_collected_at = $1, - last_collection_error = NULL + SET last_collected_at = $1, + last_collection_error = NULL, + last_collection_partial = NULL WHERE id = 1 `, collectedAt); err != nil { return fmt.Errorf("failed to update state: %w", err) @@ -109,8 +110,9 @@ func (s *PostgresStore) UpsertRecommendations(ctx context.Context, collectedAt t if _, err := tx.Exec(ctx, ` UPDATE recommendations_state - SET last_collected_at = $1, - last_collection_error = NULL + SET last_collected_at = $1, + last_collection_error = NULL, + last_collection_partial = NULL WHERE id = 1 `, collectedAt); err != nil { return fmt.Errorf("failed to update state: %w", err) @@ -430,18 +432,20 @@ func (s *PostgresStore) ListStoredRecommendations(ctx context.Context, filter Re // GetRecommendationsFreshness returns the singleton freshness row. The // table is seeded with id=1 by the migration so a row always exists; -// LastCollectedAt, LastCollectionError, and LastCollectionStartedAt can be NULL. +// LastCollectedAt, LastCollectionError, LastCollectionStartedAt and +// LastCollectionPartial can be NULL. func (s *PostgresStore) GetRecommendationsFreshness(ctx context.Context) (*RecommendationsFreshness, error) { var ( lastCollectedAt *time.Time lastCollectionError *string lastCollectionStartedAt *time.Time + lastCollectionPartial []byte ) err := s.db.QueryRow(ctx, ` - SELECT last_collected_at, last_collection_error, last_collection_started_at + SELECT last_collected_at, last_collection_error, last_collection_started_at, last_collection_partial FROM recommendations_state WHERE id = 1 - `).Scan(&lastCollectedAt, &lastCollectionError, &lastCollectionStartedAt) + `).Scan(&lastCollectedAt, &lastCollectionError, &lastCollectionStartedAt, &lastCollectionPartial) if err != nil { return nil, fmt.Errorf("failed to read recommendations_state: %w", err) } @@ -449,9 +453,28 @@ func (s *PostgresStore) GetRecommendationsFreshness(ctx context.Context) (*Recom LastCollectedAt: lastCollectedAt, LastCollectionError: lastCollectionError, LastCollectionStartedAt: lastCollectionStartedAt, + LastCollectionPartial: decodeCollectionPartial(lastCollectionPartial), }, nil } +// decodeCollectionPartial decodes the last_collection_partial JSONB column. +// NULL decodes to nil (the last collection was complete). The column is only +// ever written by SetRecommendationsCollectionPartial via json.Marshal, so an +// undecodable value means the row was edited by hand; it is logged and read +// as nil rather than failing the whole freshness read, which the scheduler's +// cache-TTL check and every dashboard load depend on. +func decodeCollectionPartial(raw []byte) *RecommendationsCollectionPartial { + if len(raw) == 0 { + return nil + } + var partial RecommendationsCollectionPartial + if err := json.Unmarshal(raw, &partial); err != nil { + logging.Warnf("recommendations_state.last_collection_partial is not valid JSON; ignoring it: %v", err) + return nil + } + return &partial +} + // SetRecommendationsCollectionError records the most recent collection's // error message without touching last_collected_at or // last_collection_started_at. Used by the scheduler when a collect fails @@ -478,6 +501,32 @@ func (s *PostgresStore) SetRecommendationsCollectionError(ctx context.Context, e return nil } +// SetRecommendationsCollectionPartial records the most recent collection's +// incomplete-sweep summary (issue #87), or clears it when partial is nil. +// Like SetRecommendationsCollectionError it is called by the scheduler AFTER +// UpsertRecommendations has reset the column to NULL, so a complete +// collection leaves it cleared and only a partial one re-sets it. It touches +// no other column; in particular it must not clear last_collection_started_at +// (see SetRecommendationsCollectionError, issue #261). +func (s *PostgresStore) SetRecommendationsCollectionPartial(ctx context.Context, partial *RecommendationsCollectionPartial) error { + var raw []byte + if partial != nil { + encoded, err := json.Marshal(partial) + if err != nil { + return fmt.Errorf("failed to encode collection partial: %w", err) + } + raw = encoded + } + if _, err := s.db.Exec(ctx, ` + UPDATE recommendations_state + SET last_collection_partial = $1::jsonb + WHERE id = 1 + `, raw); err != nil { + return fmt.Errorf("failed to set collection partial: %w", err) + } + return nil +} + // MarkCollectionStarted atomically sets last_collection_started_at = NOW() // only when no in-flight collection is currently running. The WHERE clause // treats a started_at older than 5 minutes as stale (the scheduler Lambda diff --git a/internal/config/store_postgres_recommendations_partial_test.go b/internal/config/store_postgres_recommendations_partial_test.go new file mode 100644 index 00000000..86d7ea4d --- /dev/null +++ b/internal/config/store_postgres_recommendations_partial_test.go @@ -0,0 +1,22 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDecodeCollectionPartial(t *testing.T) { + assert.Nil(t, decodeCollectionPartial(nil), "NULL means the last collection was complete") + + got := decodeCollectionPartial([]byte(`{"providers":[{"provider":"azure","incomplete_accounts":1,"attempted_scopes":3,"failed_scopes":2,"failed_details":0,"reasons":{"throttled":2}}]}`)) + require.NotNil(t, got) + assert.Equal(t, []PartialProviderSweep{{ + Provider: "azure", IncompleteAccounts: 1, AttemptedScopes: 3, FailedScopes: 2, + Reasons: map[string]int{PartialReasonThrottled: 2}, + }}, got.Providers) + + assert.Nil(t, decodeCollectionPartial([]byte(`not json`)), + "an undecodable diagnostic must not fail the freshness read") +} diff --git a/internal/config/store_postgres_recommendations_test.go b/internal/config/store_postgres_recommendations_test.go index 82af25db..d9d0d953 100644 --- a/internal/config/store_postgres_recommendations_test.go +++ b/internal/config/store_postgres_recommendations_test.go @@ -227,6 +227,63 @@ func TestPostgresStore_Freshness_RoundTrip(t *testing.T) { assert.Equal(t, "aws failed", *fr.LastCollectionError) } +// TestPostgresStore_CollectionPartial_RecordedAndCleared pins issue #87: a +// partial sweep's summary round-trips through recommendations_state, survives +// an error write (both can be true of one run), and is cleared by the next +// Upsert or Replace -- i.e. by the next persisted collection, which re-sets +// it only if that collection was partial too. +func TestPostgresStore_CollectionPartial_RecordedAndCleared(t *testing.T) { + ctx := context.Background() + store, cleanup := setupRecommendationsStore(ctx, t) + defer cleanup() + + fr, err := store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + assert.Nil(t, fr.LastCollectionPartial, "a fresh deployment reads as complete") + + partial := &config.RecommendationsCollectionPartial{ + Providers: []config.PartialProviderSweep{{ + Provider: "azure", + IncompleteAccounts: 1, + AttemptedScopes: 10, + FailedScopes: 9, + Reasons: map[string]int{config.PartialReasonPermissionDenied: 9}, + }}, + } + recordPartial := func() { + t.Helper() + require.NoError(t, store.SetRecommendationsCollectionPartial(ctx, partial)) + got, getErr := store.GetRecommendationsFreshness(ctx) + require.NoError(t, getErr) + require.Equal(t, partial, got.LastCollectionPartial) + } + + recordPartial() + require.NoError(t, store.SetRecommendationsCollectionError(ctx, "gcp: failed")) + fr, err = store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + assert.Equal(t, partial, fr.LastCollectionPartial, "recording an error must not drop the partial marker") + + now := time.Now().UTC().Truncate(time.Second) + require.NoError(t, store.UpsertRecommendations(ctx, now, nil, nil)) + fr, err = store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + assert.Nil(t, fr.LastCollectionPartial, "the next persisted collection must clear the marker") + assert.Nil(t, fr.LastCollectionError) + + recordPartial() + require.NoError(t, store.ReplaceRecommendations(ctx, now.Add(time.Minute), nil)) + fr, err = store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + assert.Nil(t, fr.LastCollectionPartial, "a full resync must clear the marker too") + + recordPartial() + require.NoError(t, store.SetRecommendationsCollectionPartial(ctx, nil)) + fr, err = store.GetRecommendationsFreshness(ctx) + require.NoError(t, err) + assert.Nil(t, fr.LastCollectionPartial, "a nil partial clears the column") +} + // TestPostgresStore_UpsertRecommendations_StoresAllTermVariants pins // the broadened-natural-key behavior from migration 000032: when // Azure returns multiple `(term, payment)` variants for the same diff --git a/internal/config/types.go b/internal/config/types.go index 300a86dd..433c9f93 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -778,10 +778,63 @@ type PurchasePlanFilter struct { // the scheduler clears it on completion (success or failure). A value older // than 5 minutes means the scheduler crashed mid-run — the refresh handler // treats it as stale and allows a new collection. +// LastCollectionPartial is non-nil when the most recent persisted collection +// kept rows from a sweep that did not cover everything it was asked to (for +// example 9 of 10 Azure subscriptions answered). It is cleared by the next +// collection whose every sweep is complete. type RecommendationsFreshness struct { - LastCollectedAt *time.Time `json:"last_collected_at"` - LastCollectionError *string `json:"last_collection_error"` - LastCollectionStartedAt *time.Time `json:"last_collection_started_at"` + LastCollectedAt *time.Time `json:"last_collected_at"` + LastCollectionError *string `json:"last_collection_error"` + LastCollectionStartedAt *time.Time `json:"last_collection_started_at"` + LastCollectionPartial *RecommendationsCollectionPartial `json:"last_collection_partial"` +} + +// RecommendationsCollectionPartial records that the most recent collection +// was incomplete without having failed: its rows were kept and stored, but +// some subscriptions, projects or collection scopes were never queried, so +// the stored recommendations under-state the real opportunity (issue #87). +// +// It is deliberately aggregate-only. recommendations_state is one global row +// that every user with view:recommendations reads, while accounts are +// access-scoped, so it carries counts and a fixed set of reason categories -- +// never raw provider error text or subscription/project identifiers. The +// failed identifiers are named in the scheduler's WARN log for operators. +type RecommendationsCollectionPartial struct { + // Providers lists one entry per provider with at least one incomplete + // sweep, in the configured provider order. + Providers []PartialProviderSweep `json:"providers"` +} + +// Reason categories for PartialProviderSweep.Reasons. The set is closed so +// the API never echoes provider error text. +const ( + PartialReasonPermissionDenied = "permission_denied" + PartialReasonThrottled = "throttled" + PartialReasonTimeout = "timeout" + PartialReasonCanceled = "canceled" + PartialReasonNotFound = "not_found" + PartialReasonProviderUnavailable = "provider_unavailable" + PartialReasonOther = "other" +) + +// PartialProviderSweep summarizes one provider's incomplete sweeps in a +// collection. A "scope" is the unit a sweep fails in: a subscription for +// Azure, a project for GCP, and a service/region collection scope for AWS. +type PartialProviderSweep struct { + Provider string `json:"provider"` + // IncompleteAccounts is how many accounts (registered, or the single + // ambient host identity) returned rows from an incomplete sweep. + IncompleteAccounts int `json:"incomplete_accounts"` + // AttemptedScopes is how many scopes the incomplete sweeps were asked to + // cover. Zero when the provider does not report it (AWS). + AttemptedScopes int `json:"attempted_scopes"` + // FailedScopes is how many of those scopes could not be queried. + FailedScopes int `json:"failed_scopes"` + // FailedDetails is how many individual recommendation details were + // rejected inside scopes that otherwise answered (AWS only). + FailedDetails int `json:"failed_details"` + // Reasons counts the failure causes by PartialReason* category. + Reasons map[string]int `json:"reasons"` } // SuccessfulCollect identifies a (provider, account) pair whose collection diff --git a/internal/database/postgres/migrations/000129_recommendations_state_partial.down.sql b/internal/database/postgres/migrations/000129_recommendations_state_partial.down.sql new file mode 100644 index 00000000..2c9d3edc --- /dev/null +++ b/internal/database/postgres/migrations/000129_recommendations_state_partial.down.sql @@ -0,0 +1,2 @@ +ALTER TABLE recommendations_state + DROP COLUMN IF EXISTS last_collection_partial; diff --git a/internal/database/postgres/migrations/000129_recommendations_state_partial.up.sql b/internal/database/postgres/migrations/000129_recommendations_state_partial.up.sql new file mode 100644 index 00000000..91c3e3ab --- /dev/null +++ b/internal/database/postgres/migrations/000129_recommendations_state_partial.up.sql @@ -0,0 +1,13 @@ +-- Add last_collection_partial to recommendations_state. +-- +-- Issue #87: a multi-subscription sweep that loses some subscriptions keeps +-- the rows of the ones that answered (tolerateIncompleteSweep), but the +-- incompleteness only reached the logs, so the dashboard rendered 1 of 10 +-- subscriptions exactly like full coverage. This column records an +-- aggregate, sanitized summary of the most recent collection's incomplete +-- sweeps (per-provider counts and reason categories, never raw provider +-- errors or subscription IDs). NULL means the last persisted collection was +-- complete; UpsertRecommendations/ReplaceRecommendations reset it to NULL +-- and the scheduler re-sets it when a sweep was partial. +ALTER TABLE recommendations_state + ADD COLUMN IF NOT EXISTS last_collection_partial JSONB; diff --git a/internal/database/postgres/migrations/000129_recommendations_state_partial_test.go b/internal/database/postgres/migrations/000129_recommendations_state_partial_test.go new file mode 100644 index 00000000..d2763d9e --- /dev/null +++ b/internal/database/postgres/migrations/000129_recommendations_state_partial_test.go @@ -0,0 +1,74 @@ +//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" +) + +// TestMigration_RecommendationsStatePartial locks down migration 000129: +// recommendations_state gains a nullable last_collection_partial JSONB +// column (issue #87: incomplete multi-subscription sweeps visible outside +// the logs), the seeded singleton row starts out NULL (= complete), and the +// down migration removes the column cleanly. +func TestMigration_RecommendationsStatePartial(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() + + const version = 129 + // Migration numbers are not contiguous, so pin at the real predecessor + // rather than version-1. + prev := previousMigrationVersion(t, version) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, prev)) + + columnExists := func() bool { + t.Helper() + var exists bool + require.NoError(t, pool.QueryRow(ctx, ` + SELECT EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_name = 'recommendations_state' + AND column_name = 'last_collection_partial' + ) + `).Scan(&exists)) + return exists + } + assert.False(t, columnExists(), "last_collection_partial must not exist before 000129") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, version)) + + var dataType, isNullable string + err = pool.QueryRow(ctx, ` + SELECT data_type, is_nullable + FROM information_schema.columns + WHERE table_name = 'recommendations_state' + AND column_name = 'last_collection_partial' + `).Scan(&dataType, &isNullable) + require.NoError(t, err, "last_collection_partial must exist after 000129") + assert.Equal(t, "jsonb", dataType) + assert.Equal(t, "YES", isNullable) + + var partialIsNull bool + require.NoError(t, pool.QueryRow(ctx, + `SELECT last_collection_partial IS NULL FROM recommendations_state WHERE id = 1`).Scan(&partialIsNull)) + assert.True(t, partialIsNull, "an existing deployment must read as complete until a partial sweep is recorded") + + // Rollback restores the pre-migration schema, even with a value stored. + _, err = pool.Exec(ctx, + `UPDATE recommendations_state SET last_collection_partial = '{"providers":[]}'::jsonb WHERE id = 1`) + require.NoError(t, err) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, prev)) + assert.False(t, columnExists(), "last_collection_partial must be dropped after rollback") +} diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 0e0be017..a4c9546f 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -1613,6 +1613,15 @@ func (m *MockConfigStore) SetRecommendationsCollectionError(ctx context.Context, return args.Error(0) } +func (m *MockConfigStore) SetRecommendationsCollectionPartial(ctx context.Context, partial *config.RecommendationsCollectionPartial) error { + m.record("SetRecommendationsCollectionPartial", ctx, partial) + if !isExpected(&m.Mock, "SetRecommendationsCollectionPartial") { + return nil + } + args := m.Called(ctx, partial) + return args.Error(0) +} + func (m *MockConfigStore) GetRIUtilizationCache(ctx context.Context, region string, lookbackDays int) (*config.RIUtilizationCacheEntry, error) { m.record("GetRIUtilizationCache", ctx, region, lookbackDays) if !isExpected(&m.Mock, "GetRIUtilizationCache") { diff --git a/internal/purchase/azure_pricing_integration_test.go b/internal/purchase/azure_pricing_integration_test.go index f9d3b692..99c6b1be 100644 --- a/internal/purchase/azure_pricing_integration_test.go +++ b/internal/purchase/azure_pricing_integration_test.go @@ -103,8 +103,9 @@ 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 and tenancy columns and the - // 11-column index from migration 000111. - require.NoError(t, migrations.MigrateToVersion(ctx, pool, rampMigrationsPath(), 111)) + // 11-column index from migration 000111, 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, 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/list_accounts_error_test.go b/internal/scheduler/list_accounts_error_test.go index 6bbf2822..c56c9009 100644 --- a/internal/scheduler/list_accounts_error_test.go +++ b/internal/scheduler/list_accounts_error_test.go @@ -19,15 +19,15 @@ func TestCollectRecommendations_ListAccountsErrorFailsClosed(t *testing.T) { tests := []struct { name string env map[string]string - collect func(*Scheduler) ([]config.RecommendationRecord, []string, error) + collect func(*Scheduler) ([]config.RecommendationRecord, []string, *sweepGap, error) }{ - {"aws", nil, func(s *Scheduler) ([]config.RecommendationRecord, []string, error) { + {"aws", nil, func(s *Scheduler) ([]config.RecommendationRecord, []string, *sweepGap, error) { return s.collectAWSRecommendations(context.Background(), &config.GlobalConfig{}) }}, - {"azure", map[string]string{"AZURE_SUBSCRIPTION_ID": "sub-a"}, func(s *Scheduler) ([]config.RecommendationRecord, []string, error) { + {"azure", map[string]string{"AZURE_SUBSCRIPTION_ID": "sub-a"}, func(s *Scheduler) ([]config.RecommendationRecord, []string, *sweepGap, error) { return s.collectAzureRecommendations(context.Background(), &config.GlobalConfig{}) }}, - {"gcp", map[string]string{"GCP_PROJECT_ID": "proj-a"}, func(s *Scheduler) ([]config.RecommendationRecord, []string, error) { + {"gcp", map[string]string{"GCP_PROJECT_ID": "proj-a"}, func(s *Scheduler) ([]config.RecommendationRecord, []string, *sweepGap, error) { return s.collectGCPRecommendations(context.Background(), &config.GlobalConfig{}) }}, } @@ -42,12 +42,13 @@ func TestCollectRecommendations_ListAccountsErrorFailsClosed(t *testing.T) { factory := new(MockProviderFactory) s := &Scheduler{config: store, providerFactory: factory} - recs, succeeded, err := tt.collect(s) + recs, succeeded, gap, err := tt.collect(s) require.Error(t, err) assert.ErrorIs(t, err, listErr) assert.Empty(t, recs) assert.Empty(t, succeeded, "a failed listing must not make any account eligible for eviction") + assert.Nil(t, gap, "a failed listing is a failure, not a partial sweep") factory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) }) } diff --git a/internal/scheduler/partial_sweep.go b/internal/scheduler/partial_sweep.go new file mode 100644 index 00000000..516a1621 --- /dev/null +++ b/internal/scheduler/partial_sweep.go @@ -0,0 +1,222 @@ +package scheduler + +import ( + "context" + "errors" + "net/http" + "strings" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + awsrecommendations "github.com/LeanerCloud/cloud-commitments-go/providers/aws/recommendations" + azureprovider "github.com/LeanerCloud/cloud-commitments-go/providers/azure" + gcpprovider "github.com/LeanerCloud/cloud-commitments-go/providers/gcp" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "google.golang.org/api/googleapi" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +// sweepGap describes what an incomplete recommendation sweep missed, in the +// sanitized form that is persisted to recommendations_state and served to the +// dashboard (issue #87). A nil *sweepGap means the sweep was complete. +// +// It carries counts and reason categories only. The raw provider errors and +// the failed subscription/project IDs stay in the scheduler's WARN log: the +// state row is global and readable by every view:recommendations user, while +// accounts are access-scoped, so naming them there would leak which +// subscriptions exist to users not authorized for them. +type sweepGap struct { + incompleteAccounts int + attemptedScopes int + failedScopes int + failedDetails int + reasons map[string]int +} + +// gapFromAWSIncomplete summarizes an AWS IncompleteRecommendationsError. +// AWS does not report how many scopes it attempted, so attemptedScopes is 0. +func gapFromAWSIncomplete(e *awsrecommendations.IncompleteRecommendationsError) *sweepGap { + g := &sweepGap{ + failedScopes: e.FailedScopes, + failedDetails: e.FailedDetails, + reasons: map[string]int{}, + } + for _, cause := range e.Causes { + g.reasons[classifySweepCause(cause)]++ + } + return g +} + +// gapFromPartialSubscriptions summarizes a multi-subscription fan-out that +// lost some subscriptions (Azure subscriptions, or GCP projects routed +// through the same error type). +func gapFromPartialSubscriptions(p *azureprovider.PartialSubscriptionFailureError) *sweepGap { + g := &sweepGap{ + attemptedScopes: p.Attempted, + failedScopes: max(p.Attempted-p.Succeeded, len(p.Failed)), + reasons: map[string]int{}, + } + for _, f := range p.Failed { + g.reasons[classifySweepCause(f.Err)]++ + } + return g +} + +// forAccount marks the gap as one account's incomplete sweep. Scope-level +// gaps (for example the AWS primary fetch and its fallback for the same +// account) are merged first and counted as an account once, here. +func (g *sweepGap) forAccount() *sweepGap { + if g == nil { + return nil + } + out := mergeSweepGaps(nil, g) + out.incompleteAccounts = 1 + return out +} + +// mergeSweepGaps returns the sum of two gaps without mutating either. nil is +// the identity: merging two complete sweeps stays complete. +func mergeSweepGaps(a, b *sweepGap) *sweepGap { + if a == nil && b == nil { + return nil + } + out := &sweepGap{reasons: map[string]int{}} + for _, g := range []*sweepGap{a, b} { + if g == nil { + continue + } + out.incompleteAccounts += g.incompleteAccounts + out.attemptedScopes += g.attemptedScopes + out.failedScopes += g.failedScopes + out.failedDetails += g.failedDetails + for reason, n := range g.reasons { + out.reasons[reason] += n + } + } + return out +} + +// toProviderSweep converts the gap into its persisted, API-facing shape. +func (g *sweepGap) toProviderSweep(providerName string) config.PartialProviderSweep { + reasons := make(map[string]int, len(g.reasons)) + for reason, n := range g.reasons { + reasons[reason] = n + } + return config.PartialProviderSweep{ + Provider: providerName, + IncompleteAccounts: g.incompleteAccounts, + AttemptedScopes: g.attemptedScopes, + FailedScopes: g.failedScopes, + FailedDetails: g.failedDetails, + Reasons: reasons, + } +} + +// classifySweepCause maps one failed scope's error to a closed reason +// category so the API never echoes provider error text (which can carry +// ARNs, subscription IDs or request IDs). +func classifySweepCause(err error) string { + switch { + case err == nil: + return config.PartialReasonOther + case errors.Is(err, context.Canceled): + return config.PartialReasonCanceled + case errors.Is(err, context.DeadlineExceeded): + return config.PartialReasonTimeout + case gcpprovider.IsPermissionError(err): + return config.PartialReasonPermissionDenied + } + // AWS throttling and access errors arrive as HTTP 400 with a telling + // error code, so the code is checked before the status. + if reason := reasonForErrorCode(err); reason != "" { + return reason + } + if code, ok := httpStatusOf(err); ok { + return reasonForHTTPStatus(code) + } + if s, ok := status.FromError(err); ok { + return reasonForGRPCCode(s.Code()) + } + return config.PartialReasonOther +} + +// reasonForErrorCode classifies AWS (smithy APIError) and Azure error codes. +// It returns "" when the code says nothing classifiable. +func reasonForErrorCode(err error) string { + var code string + var apiErr interface{ ErrorCode() string } + var azErr *azcore.ResponseError + switch { + case errors.As(err, &apiErr): + code = apiErr.ErrorCode() + case errors.As(err, &azErr): + code = azErr.ErrorCode + default: + return "" + } + code = strings.ToLower(code) + switch { + case strings.Contains(code, "throttl"), strings.Contains(code, "toomanyrequests"), + strings.Contains(code, "requestlimitexceeded"): + return config.PartialReasonThrottled + case strings.Contains(code, "accessdenied"), strings.Contains(code, "unauthorized"), + strings.Contains(code, "authorizationfailed"), strings.Contains(code, "forbidden"): + return config.PartialReasonPermissionDenied + default: + return "" + } +} + +// httpStatusOf extracts an HTTP status from the Azure, GCP REST or AWS +// error types in err's chain. +func httpStatusOf(err error) (int, bool) { + var azErr *azcore.ResponseError + if errors.As(err, &azErr) { + return azErr.StatusCode, true + } + var gErr *googleapi.Error + if errors.As(err, &gErr) { + return gErr.Code, true + } + var awsErr interface{ HTTPStatusCode() int } + if errors.As(err, &awsErr) { + return awsErr.HTTPStatusCode(), true + } + return 0, false +} + +func reasonForHTTPStatus(code int) string { + switch { + case code == http.StatusUnauthorized, code == http.StatusForbidden: + return config.PartialReasonPermissionDenied + case code == http.StatusNotFound: + return config.PartialReasonNotFound + case code == http.StatusRequestTimeout, code == http.StatusGatewayTimeout: + return config.PartialReasonTimeout + case code == http.StatusTooManyRequests: + return config.PartialReasonThrottled + case code >= http.StatusInternalServerError: + return config.PartialReasonProviderUnavailable + default: + return config.PartialReasonOther + } +} + +func reasonForGRPCCode(code codes.Code) string { + switch code { + case codes.PermissionDenied, codes.Unauthenticated: + return config.PartialReasonPermissionDenied + case codes.ResourceExhausted: + return config.PartialReasonThrottled + case codes.DeadlineExceeded: + return config.PartialReasonTimeout + case codes.Canceled: + return config.PartialReasonCanceled + case codes.NotFound: + return config.PartialReasonNotFound + case codes.Unavailable, codes.Internal: + return config.PartialReasonProviderUnavailable + default: + return config.PartialReasonOther + } +} diff --git a/internal/scheduler/partial_sweep_eviction_test.go b/internal/scheduler/partial_sweep_eviction_test.go index 19e59b81..4f142a88 100644 --- a/internal/scheduler/partial_sweep_eviction_test.go +++ b/internal/scheduler/partial_sweep_eviction_test.go @@ -33,8 +33,9 @@ func TestAWSIncompleteSweep(t *testing.T) { {name: "deadline", err: context.DeadlineExceeded, fatal: true}, } { t.Run(tc.name, func(t *testing.T) { - complete, err := tolerateIncompleteSweep("aws", tc.err) - require.Equal(t, tc.complete, complete) + gap, err := tolerateIncompleteSweep("aws", tc.err) + require.Equal(t, tc.complete || tc.fatal, gap == nil, + "only a tolerated incomplete sweep carries a gap") if tc.fatal { require.ErrorIs(t, err, tc.err) } else { @@ -73,11 +74,13 @@ func TestAWSIncompleteFallbackCannotAuthorizeEviction(t *testing.T) { prov := new(MockProvider) prov.On("GetRecommendationsClient", mock.Anything).Return(client, nil).Once() s := &Scheduler{} - recs, complete, err := s.fetchAndConvert(context.Background(), prov, "aws", nil, "", + recs, gap, err := s.fetchAndConvert(context.Background(), prov, "aws", nil, "", &config.GlobalConfig{DefaultTerm: 1, DefaultPayment: "no-upfront", RecommendationsLookbackDays: 30}) require.NoError(t, err) require.Len(t, recs, 1) - require.False(t, complete) + require.NotNil(t, gap, "an incomplete primary or fallback fetch must leave the account incomplete") + assert.Zero(t, gap.incompleteAccounts, + "fetchAndConvert merges scope-level gaps; the account is counted once, by its caller") client.AssertExpectations(t) prov.AssertExpectations(t) }) @@ -130,23 +133,70 @@ func partialSweepErr(failedSubscriptionIDs ...string) *azureprovider.PartialSubs } } -// newPartialSweepScheduler wires a Scheduler whose Azure provider returns -// recs plus a partial-subscription failure, exercising the ambient path -// (no registered accounts + AZURE_SUBSCRIPTION_ID set). -func newPartialSweepScheduler(t *testing.T, sweepErr error) *Scheduler { +// partialSweepCase is one provider's ambient collection path. Every case +// used to drive Azure only, so the GCP and AWS wiring of the gap returned by +// tolerateIncompleteSweep was uncovered (audit finding A16-007): a partial +// GCP sweep could enter the eviction roster with the whole suite green. +type partialSweepCase struct { + provider string + env map[string]string + partialErr error + collect func(*Scheduler) ([]config.RecommendationRecord, []string, *sweepGap, error) +} + +func partialSweepCases() []partialSweepCase { + return []partialSweepCase{ + { + provider: "aws", + partialErr: &awsrecommendations.IncompleteRecommendationsError{ + FailedScopes: 2, Causes: []error{errors.New("scope one failed"), errors.New("scope two failed")}, + }, + collect: func(s *Scheduler) ([]config.RecommendationRecord, []string, *sweepGap, error) { + return s.collectAWSRecommendations(context.Background(), &config.GlobalConfig{}) + }, + }, + { + provider: "azure", + env: map[string]string{"AZURE_SUBSCRIPTION_ID": "sub-a"}, + partialErr: partialSweepErr("sub-b", "sub-c"), + collect: func(s *Scheduler) ([]config.RecommendationRecord, []string, *sweepGap, error) { + return s.collectAzureRecommendations(context.Background(), &config.GlobalConfig{}) + }, + }, + { + provider: "gcp", + env: map[string]string{"GCP_PROJECT_ID": "proj-a"}, + partialErr: partialSweepErr("proj-b", "proj-c"), + collect: func(s *Scheduler) ([]config.RecommendationRecord, []string, *sweepGap, error) { + return s.collectGCPRecommendations(context.Background(), &config.GlobalConfig{}) + }, + }, + } +} + +// partialSweepRec is the one recommendation the surviving scope returns. +func partialSweepRec() common.Recommendation { + return common.Recommendation{ + Service: common.ServiceEC2, + Region: "westeurope", + ResourceType: "Standard_D2s_v3", + Count: 3, + Term: "1yr", + PaymentOption: "all-upfront", + } +} + +// newPartialSweepScheduler wires a Scheduler whose providerName provider +// returns one recommendation plus sweepErr, exercising the ambient path (no +// registered accounts; AZURE_SUBSCRIPTION_ID / GCP_PROJECT_ID set by the +// caller). It returns the store so callers can add expectations. +func newPartialSweepScheduler(t *testing.T, providerName string, sweepErr error) (*Scheduler, *MockConfigStore) { t.Helper() recClient := new(MockRecommendationsClient) t.Cleanup(func() { recClient.AssertExpectations(t) }) recClient.On("GetAllRecommendations", mock.Anything). - Return([]common.Recommendation{{ - Service: common.ServiceEC2, - Region: "westeurope", - ResourceType: "Standard_D2s_v3", - Count: 3, - Term: "1yr", - PaymentOption: "all-upfront", - }}, sweepErr).Once() + Return([]common.Recommendation{partialSweepRec()}, sweepErr).Once() prov := new(MockProvider) t.Cleanup(func() { prov.AssertExpectations(t) }) @@ -154,59 +204,77 @@ func newPartialSweepScheduler(t *testing.T, sweepErr error) *Scheduler { factory := new(MockProviderFactory) t.Cleanup(func() { factory.AssertExpectations(t) }) - factory.On("CreateAndValidateProvider", mock.Anything, "azure", mock.Anything). + factory.On("CreateAndValidateProvider", mock.Anything, providerName, mock.Anything). Return(prov, nil).Once() store := new(MockConfigStore) t.Cleanup(func() { store.AssertExpectations(t) }) - // No registered Azure accounts -> ambient path. + // No registered accounts -> ambient path. store.On("ListCloudAccounts", mock.Anything, mock.Anything). Return([]config.CloudAccount{}, nil).Maybe() // Ambient account resolution is best-effort; leave it unresolved so the // roster carries the nil-CloudAccountID sentinel and the assertion is // about roster membership rather than which ID it holds. - store.On("GetCloudAccountByExternalID", mock.Anything, "azure", mock.Anything). + store.On("GetCloudAccountByExternalID", mock.Anything, providerName, mock.Anything). Return(nil, nil).Maybe() - return &Scheduler{config: store, providerFactory: factory} + return &Scheduler{config: store, providerFactory: factory}, store } -// TestCollectAzure_PartialSweepIsNotEvictionEligible is the regression for -// #1652. It fails on the pre-fix code, where tolerateIncompleteSweep returns -// nil and the account is reported as fully succeeded. -func TestCollectAzure_PartialSweepIsNotEvictionEligible(t *testing.T) { - t.Setenv("AZURE_SUBSCRIPTION_ID", "sub-a") +func setEnv(t *testing.T, env map[string]string) { + t.Helper() + for k, v := range env { + t.Setenv(k, v) + } +} - s := newPartialSweepScheduler(t, partialSweepErr("sub-b", "sub-c")) +// TestCollectProvider_PartialSweepIsNotEvictionEligible is the regression +// for #1652, now per provider. It fails on the pre-#1652 code, where +// tolerateIncompleteSweep returns nil and the account is reported as fully +// succeeded, and on any provider whose collector drops the gap (#87). +func TestCollectProvider_PartialSweepIsNotEvictionEligible(t *testing.T) { + for _, tc := range partialSweepCases() { + t.Run(tc.provider, func(t *testing.T) { + setEnv(t, tc.env) + s, _ := newPartialSweepScheduler(t, tc.provider, tc.partialErr) - recs, succeededAccountIDs, err := s.collectAzureRecommendations(context.Background(), &config.GlobalConfig{}) + recs, succeededAccountIDs, gap, err := tc.collect(s) - require.NoError(t, err, - "a partial sweep must not fail the account: one flaky subscription cannot blank the whole account's dashboard") - assert.NotEmpty(t, recs, - "the subscriptions that DID succeed must still contribute rows to be upserted") - assert.Emptyf(t, succeededAccountIDs, - "a partial sweep must not enter the eviction roster, got %#v: 2 subscription(s) were never "+ - "queried, so their previous-cycle rows would be deleted by the (provider, account_key) DELETE", - succeededAccountIDs) + require.NoError(t, err, + "a partial sweep must not fail the account: one flaky subscription cannot blank the whole account's dashboard") + assert.NotEmpty(t, recs, + "the subscriptions that DID succeed must still contribute rows to be upserted") + assert.Emptyf(t, succeededAccountIDs, + "a partial sweep must not enter the eviction roster, got %#v: 2 scope(s) were never "+ + "queried, so their previous-cycle rows would be deleted by the (provider, account_key) DELETE", + succeededAccountIDs) + require.NotNil(t, gap, "the partial sweep must reach the provider outcome so it can be persisted (#87)") + assert.Equal(t, 1, gap.incompleteAccounts, "the ambient identity is one incomplete account") + assert.Equal(t, 2, gap.failedScopes) + }) + } } -// TestCollectAzure_CompleteSweepStaysEvictionEligible is the control. The +// TestCollectProvider_CompleteSweepStaysEvictionEligible is the control. The // fix must withhold eviction ONLY for partial sweeps -- if a clean sweep // also stopped evicting, stale rows would accumulate forever and rows for // genuinely-gone resources would never disappear. Under-evicting is a // quieter bug than over-evicting, so it gets its own assertion. -func TestCollectAzure_CompleteSweepStaysEvictionEligible(t *testing.T) { - t.Setenv("AZURE_SUBSCRIPTION_ID", "sub-a") +func TestCollectProvider_CompleteSweepStaysEvictionEligible(t *testing.T) { + for _, tc := range partialSweepCases() { + t.Run(tc.provider, func(t *testing.T) { + setEnv(t, tc.env) + s, _ := newPartialSweepScheduler(t, tc.provider, nil) - s := newPartialSweepScheduler(t, nil) + recs, succeededAccountIDs, gap, err := tc.collect(s) - recs, succeededAccountIDs, err := s.collectAzureRecommendations(context.Background(), &config.GlobalConfig{}) - - require.NoError(t, err) - assert.NotEmpty(t, recs) - assert.NotEmpty(t, succeededAccountIDs, - "a complete sweep must still authorize eviction, otherwise stale rows accumulate forever") + require.NoError(t, err) + assert.NotEmpty(t, recs) + assert.NotEmpty(t, succeededAccountIDs, + "a complete sweep must still authorize eviction, otherwise stale rows accumulate forever") + assert.Nil(t, gap, "a complete sweep must not be marked partial") + }) + } } // TestFanOutPerAccount_PartialSweepNotInSucceededAccountIDs covers the @@ -221,10 +289,13 @@ func TestFanOutPerAccount_PartialSweepNotInSucceededAccountIDs(t *testing.T) { {ID: "acct-complete", Name: "acct-complete", ExternalID: "e-ok"}, {ID: "acct-partial", Name: "acct-partial", ExternalID: "e-partial"}, } - fn := func(_ context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, bool, error) { + fn := func(_ context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, *sweepGap, error) { // Both return rows; only one swept every subscription. - return []config.RecommendationRecord{{ID: acct.ID, Provider: "azure"}}, - acct.ID == "acct-complete", nil + var gap *sweepGap + if acct.ID != "acct-complete" { + gap = gapFromPartialSubscriptions(partialSweepErr("sub-b", "sub-c")) + } + return []config.RecommendationRecord{{ID: acct.ID, Provider: "azure"}}, gap, nil } recs, outcome := fanOutPerAccount(context.Background(), "Azure", accounts, fn) @@ -237,18 +308,26 @@ func TestFanOutPerAccount_PartialSweepNotInSucceededAccountIDs(t *testing.T) { assert.Equal(t, 2, outcome.SucceededCount, "a partial sweep still counts as a success, so one flaky subscription cannot trip the all-accounts-failed guard") assert.Zero(t, outcome.FailedCount) + require.NotNil(t, outcome.Gap, "the partial account's gap must reach the provider outcome (#87)") + assert.Equal(t, 1, outcome.Gap.incompleteAccounts) + assert.Equal(t, 3, outcome.Gap.attemptedScopes) + assert.Equal(t, 2, outcome.Gap.failedScopes) } -// TestCollectAzure_HardSweepErrorStillFailsTheAccount pins that the fix did -// not soften genuine failures into partial ones: a non-partial error must +// TestCollectProvider_HardSweepErrorStillFailsTheAccount pins that the fix +// did not soften genuine failures into partial ones: a non-partial error must // still propagate, keeping the account out of the roster via the error path. -func TestCollectAzure_HardSweepErrorStillFailsTheAccount(t *testing.T) { - t.Setenv("AZURE_SUBSCRIPTION_ID", "sub-a") - - s := newPartialSweepScheduler(t, errors.New("429 too many requests")) +func TestCollectProvider_HardSweepErrorStillFailsTheAccount(t *testing.T) { + for _, tc := range partialSweepCases() { + t.Run(tc.provider, func(t *testing.T) { + setEnv(t, tc.env) + s, _ := newPartialSweepScheduler(t, tc.provider, errors.New("429 too many requests")) - _, succeededAccountIDs, err := s.collectAzureRecommendations(context.Background(), &config.GlobalConfig{}) + _, succeededAccountIDs, gap, err := tc.collect(s) - require.Error(t, err, "a hard collection error must still fail the account") - assert.Empty(t, succeededAccountIDs) + require.Error(t, err, "a hard collection error must still fail the account") + assert.Empty(t, succeededAccountIDs) + assert.Nil(t, gap, "a hard failure is reported as an error, never as a partial sweep") + }) + } } diff --git a/internal/scheduler/partial_sweep_test.go b/internal/scheduler/partial_sweep_test.go new file mode 100644 index 00000000..4448a52a --- /dev/null +++ b/internal/scheduler/partial_sweep_test.go @@ -0,0 +1,274 @@ +package scheduler + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/url" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + azureprovider "github.com/LeanerCloud/cloud-commitments-go/providers/azure" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/smithy-go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "google.golang.org/api/googleapi" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +// hostExternalID is the synthetic AWS host identity the self-account +// collector verifies against STS. +const hostExternalID = "111111111111" + +func azResponseError(statusCode int, code string) *azcore.ResponseError { + return &azcore.ResponseError{ + StatusCode: statusCode, + ErrorCode: code, + RawResponse: &http.Response{ + StatusCode: statusCode, + Request: &http.Request{Method: http.MethodGet, URL: &url.URL{Scheme: "https", Host: "management.azure.com"}}, + }, + } +} + +func TestClassifySweepCause(t *testing.T) { + for _, tc := range []struct { + name string + err error + want string + }{ + {"nil", nil, config.PartialReasonOther}, + {"plain error", errors.New("something odd for subscription 0000"), config.PartialReasonOther}, + {"canceled", fmt.Errorf("sub-1: %w", context.Canceled), config.PartialReasonCanceled}, + {"deadline", fmt.Errorf("sub-1: %w", context.DeadlineExceeded), config.PartialReasonTimeout}, + {"azure 403", fmt.Errorf("query: %w", azResponseError(403, "")), config.PartialReasonPermissionDenied}, + {"azure AuthorizationFailed code", azResponseError(400, "AuthorizationFailed"), config.PartialReasonPermissionDenied}, + {"azure 429", azResponseError(429, ""), config.PartialReasonThrottled}, + {"azure 404", azResponseError(404, "SubscriptionNotFound"), config.PartialReasonNotFound}, + {"azure 503", azResponseError(503, ""), config.PartialReasonProviderUnavailable}, + {"azure 400 unknown code", azResponseError(400, "BadRequest"), config.PartialReasonOther}, + {"gcp REST 403", &googleapi.Error{Code: 403}, config.PartialReasonPermissionDenied}, + {"gcp REST 429", fmt.Errorf("x: %w", &googleapi.Error{Code: 429}), config.PartialReasonThrottled}, + {"gcp gRPC permission", status.Error(codes.PermissionDenied, "denied"), config.PartialReasonPermissionDenied}, + {"gcp gRPC exhausted", status.Error(codes.ResourceExhausted, "quota"), config.PartialReasonThrottled}, + {"gcp gRPC unavailable", status.Error(codes.Unavailable, "down"), config.PartialReasonProviderUnavailable}, + {"aws throttling code", fmt.Errorf("op: %w", &smithy.GenericAPIError{Code: "ThrottlingException"}), config.PartialReasonThrottled}, + {"aws access denied code", &smithy.GenericAPIError{Code: "AccessDeniedException"}, config.PartialReasonPermissionDenied}, + } { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, classifySweepCause(tc.err)) + }) + } +} + +func TestMergeSweepGaps(t *testing.T) { + assert.Nil(t, mergeSweepGaps(nil, nil), "two complete sweeps stay complete") + assert.Nil(t, (*sweepGap)(nil).forAccount(), "a complete sweep is not an incomplete account") + + a := &sweepGap{attemptedScopes: 3, failedScopes: 1, reasons: map[string]int{config.PartialReasonThrottled: 1}} + b := &sweepGap{failedScopes: 2, failedDetails: 4, reasons: map[string]int{config.PartialReasonThrottled: 1, config.PartialReasonOther: 1}} + + got := mergeSweepGaps(a, b) + assert.Equal(t, &sweepGap{ + attemptedScopes: 3, failedScopes: 3, failedDetails: 4, + reasons: map[string]int{config.PartialReasonThrottled: 2, config.PartialReasonOther: 1}, + }, got) + assert.Equal(t, 1, a.failedScopes, "merge must not mutate its inputs") + assert.Equal(t, 1, a.reasons[config.PartialReasonThrottled], "merge must not mutate its inputs") + + acct := got.forAccount() + assert.Equal(t, 1, acct.incompleteAccounts) + assert.Zero(t, got.incompleteAccounts, "forAccount must not mutate the scope-level gap") + + two := mergeSweepGaps(acct, b.forAccount()) + assert.Equal(t, 2, two.incompleteAccounts) +} + +// The persisted summary is served to every view:recommendations user, so it +// must not carry the failed subscription IDs or raw provider error text. +func TestSweepGap_ProviderSweepIsSanitized(t *testing.T) { + partial := &azureprovider.PartialSubscriptionFailureError{ + Attempted: 4, + Succeeded: 2, + Failed: []azureprovider.SubscriptionFailure{ + {SubscriptionID: "sub-secret-a", Err: fmt.Errorf("token for tenant tenant-xyz rejected: %w", azResponseError(403, ""))}, + {SubscriptionID: "sub-secret-b", Err: errors.New("raw provider text arn:aws:iam::role/x")}, + }, + } + sweep := gapFromPartialSubscriptions(partial).forAccount().toProviderSweep("azure") + + assert.Equal(t, config.PartialProviderSweep{ + Provider: "azure", + IncompleteAccounts: 1, + AttemptedScopes: 4, + FailedScopes: 2, + Reasons: map[string]int{config.PartialReasonPermissionDenied: 1, config.PartialReasonOther: 1}, + }, sweep) + + encoded, err := json.Marshal(sweep) + require.NoError(t, err) + for _, leak := range []string{"sub-secret-a", "sub-secret-b", "tenant-xyz", "arn:aws", "raw provider text"} { + assert.NotContains(t, string(encoded), leak) + } +} + +// TestCollectProvider_RegisteredAccountPartialSweep drives the registered- +// account path (the common production shape) for the providers whose +// per-account collector can be wired without real credentials: AWS +// self-account and GCP application-default. Hardcoding a nil gap in +// collectAWSForAccount or collectGCPForAccount fails this test (A16-007). +func TestCollectProvider_RegisteredAccountPartialSweep(t *testing.T) { + for _, tc := range []struct { + provider string + account config.CloudAccount + partialErr error + collect func(*Scheduler) ([]config.RecommendationRecord, []string, *sweepGap, error) + }{ + { + provider: "aws", + account: config.CloudAccount{ID: "acct-aws", Name: "aws", Provider: "aws", AWSAuthMode: "role_arn", + ExternalID: hostExternalID, Enabled: true}, + partialErr: partialSweepCases()[0].partialErr, + collect: func(s *Scheduler) ([]config.RecommendationRecord, []string, *sweepGap, error) { + return s.collectAWSRecommendations(context.Background(), &config.GlobalConfig{}) + }, + }, + { + provider: "gcp", + account: config.CloudAccount{ID: "acct-gcp", Name: "gcp", Provider: "gcp", GCPAuthMode: "application_default", + GCPProjectID: "proj-a", ExternalID: "proj-a", Enabled: true}, + partialErr: partialSweepErr("proj-b", "proj-c"), + collect: func(s *Scheduler) ([]config.RecommendationRecord, []string, *sweepGap, error) { + return s.collectGCPRecommendations(context.Background(), &config.GlobalConfig{}) + }, + }, + } { + for _, partial := range []bool{true, false} { + t.Run(fmt.Sprintf("%s/partial=%v", tc.provider, partial), func(t *testing.T) { + var sweepErr error + if partial { + sweepErr = tc.partialErr + } + s, store := newPartialSweepScheduler(t, tc.provider, sweepErr) + s.stsClient = &fakeSTSClient{accountID: hostExternalID} + store.ExpectedCalls = nil // replace the ambient-path listing with one registered account + store.On("ListCloudAccounts", mock.Anything, mock.Anything). + Return([]config.CloudAccount{tc.account}, nil).Once() + + recs, succeeded, gap, err := tc.collect(s) + + require.NoError(t, err) + assert.NotEmpty(t, recs, "the scopes that answered must still be upserted") + if !partial { + assert.Equal(t, []string{tc.account.ID}, succeeded) + assert.Nil(t, gap) + return + } + assert.Empty(t, succeeded, "a partially-swept account must not authorize eviction") + require.NotNil(t, gap) + assert.Equal(t, 1, gap.incompleteAccounts) + assert.Equal(t, 2, gap.failedScopes) + }) + } + } +} + +// expectScheduledCollection wires the store calls CollectRecommendations +// makes on the scheduled (cron, tokenless) path and returns pointers that +// capture what was persisted. +func expectScheduledCollection(store *MockConfigStore, providers ...string) (*[]config.RecommendationRecord, **config.RecommendationsCollectionPartial) { + store.On("GetGlobalConfig", mock.Anything). + Return(&config.GlobalConfig{EnabledProviders: providers}, nil).Once() + + var upserted []config.RecommendationRecord + store.On("UpsertRecommendations", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + upserted = args.Get(2).([]config.RecommendationRecord) + }).Return(nil).Once() + + // Registered as Maybe so an unwanted call is RECORDED: MockConfigStore + // short-circuits unexpected methods before mock.Called, which would + // make AssertNotCalled pass vacuously. + var recorded *config.RecommendationsCollectionPartial + store.On("SetRecommendationsCollectionPartial", mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + recorded = args.Get(1).(*config.RecommendationsCollectionPartial) + }).Return(nil).Maybe() + return &upserted, &recorded +} + +// TestCollectRecommendations_PartialSweepRecordedInState is the scheduled- +// path regression for #87: a sweep where a strict subset of subscriptions +// fails must persist BOTH the surviving recommendations and the +// partial-sweep marker the freshness API serves. +func TestCollectRecommendations_PartialSweepRecordedInState(t *testing.T) { + t.Setenv("AZURE_SUBSCRIPTION_ID", "sub-a") + s, store := newPartialSweepScheduler(t, "azure", partialSweepErr("sub-b", "sub-c")) + upserted, recorded := expectScheduledCollection(store, "azure") + + result, err := s.CollectRecommendations(context.Background(), "") + + require.NoError(t, err) + assert.Equal(t, []string{"azure"}, result.SuccessfulProviders, "a partial sweep is not a failed provider") + assert.Len(t, *upserted, 1, "the surviving subscription's recommendation must still be stored") + require.NotNil(t, *recorded, "the partial sweep must be recorded in recommendations_state") + assert.Equal(t, &config.RecommendationsCollectionPartial{ + Providers: []config.PartialProviderSweep{{ + Provider: "azure", + IncompleteAccounts: 1, + AttemptedScopes: 3, + FailedScopes: 2, + Reasons: map[string]int{config.PartialReasonOther: 2}, + }}, + }, *recorded) + store.AssertNotCalled(t, "SetRecommendationsCollectionError", mock.Anything, mock.Anything) +} + +// TestCollectRecommendations_CompleteSweepLeavesPartialCleared: a complete +// sweep records no partial marker. UpsertRecommendations resets the column +// (see the store integration test), so the previous run's marker clears. +func TestCollectRecommendations_CompleteSweepLeavesPartialCleared(t *testing.T) { + t.Setenv("AZURE_SUBSCRIPTION_ID", "sub-a") + s, store := newPartialSweepScheduler(t, "azure", nil) + upserted, _ := expectScheduledCollection(store, "azure") + + _, err := s.CollectRecommendations(context.Background(), "") + + require.NoError(t, err) + assert.Len(t, *upserted, 1) + store.AssertNotCalled(t, "SetRecommendationsCollectionPartial", mock.Anything, mock.Anything) +} + +// TestCollectRecommendations_TotalFailureUnchanged: a hard provider failure +// keeps the pre-#87 behavior -- the provider is failed and named in +// last_collection_error -- and is never reported as a partial sweep. +func TestCollectRecommendations_TotalFailureUnchanged(t *testing.T) { + t.Setenv("AZURE_SUBSCRIPTION_ID", "sub-a") + s, store := newPartialSweepScheduler(t, "azure", errors.New("credentials expired")) + expectScheduledCollection(store, "azure") + var recordedErr string + store.On("SetRecommendationsCollectionError", mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { recordedErr = args.String(1) }).Return(nil).Once() + + result, err := s.CollectRecommendations(context.Background(), "") + + require.NoError(t, err) + assert.Contains(t, result.FailedProviders, "azure") + assert.Contains(t, recordedErr, "azure: ") + store.AssertNotCalled(t, "SetRecommendationsCollectionPartial", mock.Anything, mock.Anything) +} + +// A partial provider and a complete one: only the partial provider appears. +func TestAppendPartialProvider(t *testing.T) { + assert.Nil(t, appendPartialProvider(nil, "aws", nil)) + got := appendPartialProvider(nil, "azure", &sweepGap{failedScopes: 1, reasons: map[string]int{}}) + got = appendPartialProvider(got, "gcp", nil) + require.Len(t, got.Providers, 1) + assert.Equal(t, "azure", got.Providers[0].Provider) + assert.NotNil(t, got.Providers[0].Reasons, "reasons must encode as {} rather than null") +} diff --git a/internal/scheduler/recommendation_completeness_integration_test.go b/internal/scheduler/recommendation_completeness_integration_test.go index fd698d70..785d7340 100644 --- a/internal/scheduler/recommendation_completeness_integration_test.go +++ b/internal/scheduler/recommendation_completeness_integration_test.go @@ -271,7 +271,7 @@ func TestAWSRecommendationCompletenessPersistence(t *testing.T) { if tc.fallback != nil { globalCfg = &config.GlobalConfig{DefaultTerm: 1, DefaultPayment: "no-upfront", RecommendationsLookbackDays: 30} } - recs, ids, err := s.collectAWSRecommendations(ctx, globalCfg) + recs, ids, _, err := s.collectAWSRecommendations(ctx, globalCfg) require.NoError(t, err) wantRows := tc.wantRows switch { @@ -288,7 +288,7 @@ func TestAWSRecommendationCompletenessPersistence(t *testing.T) { require.Empty(t, ids) } require.Len(t, recs, wantRows) - s.persistCollection(ctx, recs, expandSuccessfulCollects("aws", ids), nil) + s.persistCollection(ctx, recs, expandSuccessfulCollects("aws", ids), nil, nil) rows, err := store.ListStoredRecommendations(ctx, config.RecommendationFilter{}) require.NoError(t, err) byID := make(map[string]config.RecommendationRecord, len(rows)) diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 408e2cbd..f6d4b405 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -221,7 +221,7 @@ func (s *Scheduler) CollectRecommendations(ctx context.Context, ownerToken strin // deterministic regardless of goroutine completion order. After Wait, ctx // cancellation is propagated. No concurrency cap — the universe is at // most 3 providers. - allRecommendations, totalSavings, successfulProviders, successfulCollects, failedProviders, err := s.collectAllProviders(ctx, globalCfg) + allRecommendations, totalSavings, successfulProviders, successfulCollects, failedProviders, partial, err := s.collectAllProviders(ctx, globalCfg) if err != nil { return nil, err } @@ -229,7 +229,7 @@ func (s *Scheduler) CollectRecommendations(ctx context.Context, ownerToken strin logging.Infof("Collected %d recommendations with $%.2f/month potential savings", len(allRecommendations), totalSavings) - s.persistCollection(ctx, allRecommendations, successfulCollects, failedProviders) + s.persistCollection(ctx, allRecommendations, successfulCollects, failedProviders, partial) // Send notification if we have recommendations if len(allRecommendations) > 0 && totalSavings > 0 { @@ -270,28 +270,18 @@ func (s *Scheduler) CollectRecommendations(ctx context.Context, ownerToken strin }, nil } -// persistCollection writes the collection results to the recommendations -// cache via UpsertRecommendations, which: -// -// - Upserts each row by natural key so re-collected recommendations -// reuse the same PK across runs (stable IDs for frontend). -// - Evicts stale rows (collected_at < $now) but ONLY for the -// (provider, account) pairs that successfully collected in this run. -// Failed pairs' rows stay visible so the dashboard isn't blanked out -// by transient cloud-API failures, even when one of several accounts -// under the same provider succeeded. -// -// On any failure (including partial), the collection error is additionally -// recorded in recommendations_state so the frontend banner renders while -// the user still sees the valid rows we managed to upsert. // providerOutcome bundles a single provider's collection outcome for the // deterministic merge in collectAllProviders. Only one of recs/err is // meaningful per outcome — err != nil means the provider failed entirely // (mirrors the pre-fan-out `continue` branch); err == nil means the provider // succeeded (possibly partially — partial-account-failure semantics live in // fanOutPerAccount, not here). +// +// gap is non-nil when at least one of the provider's sweeps was incomplete; +// it is surfaced in recommendations_state.last_collection_partial (#87). type providerOutcome struct { err error + gap *sweepGap recs []config.RecommendationRecord succeededAccountIDs []string } @@ -314,6 +304,7 @@ func (s *Scheduler) collectAllProviders(ctx context.Context, globalCfg *config.G successfulProviders []string, successfulCollects []config.SuccessfulCollect, failedProviders map[string]string, + partial *config.RecommendationsCollectionPartial, err error, ) { failedProviders = map[string]string{} @@ -329,11 +320,12 @@ func (s *Scheduler) collectAllProviders(ctx context.Context, globalCfg *config.G providerName := providerName // capture per-iteration g.Go(func() error { logging.Infof("Collecting recommendations from %s...", providerName) - recs, succeededAccountIDs, perr := s.collectProviderRecommendations(gctx, providerName, globalCfg) + recs, succeededAccountIDs, gap, perr := s.collectProviderRecommendations(gctx, providerName, globalCfg) mu.Lock() outcomes[providerName] = providerOutcome{ recs: recs, succeededAccountIDs: succeededAccountIDs, + gap: gap, err: perr, } mu.Unlock() @@ -350,7 +342,7 @@ func (s *Scheduler) collectAllProviders(ctx context.Context, globalCfg *config.G logging.Warnf("collectAllProviders: errgroup.Wait returned unexpected error: %v", waitErr) } if cerr := ctx.Err(); cerr != nil { - return nil, 0, nil, nil, nil, cerr + return nil, 0, nil, nil, nil, nil, cerr } // Deterministic merge: walk EnabledProviders in config order so @@ -373,8 +365,24 @@ func (s *Scheduler) collectAllProviders(ctx context.Context, globalCfg *config.G totalSavings += rec.Savings } allRecommendations = append(allRecommendations, out.recs...) + partial = appendPartialProvider(partial, providerName, out.gap) } - return allRecommendations, totalSavings, successfulProviders, successfulCollects, failedProviders, nil + return allRecommendations, totalSavings, successfulProviders, successfulCollects, failedProviders, partial, nil +} + +// appendPartialProvider adds providerName's incomplete-sweep summary to +// partial, allocating it on first use. A complete provider (nil gap) leaves +// partial unchanged, so a collection whose every sweep was complete yields +// nil and persistCollection leaves the column cleared. +func appendPartialProvider(partial *config.RecommendationsCollectionPartial, providerName string, gap *sweepGap) *config.RecommendationsCollectionPartial { + if gap == nil { + return partial + } + if partial == nil { + partial = &config.RecommendationsCollectionPartial{} + } + partial.Providers = append(partial.Providers, gap.toProviderSweep(providerName)) + return partial } // clearCollectionStartedBestEffort clears last_collection_started_at on the @@ -395,7 +403,27 @@ func (s *Scheduler) clearCollectionStartedBestEffort(ctx context.Context, ownerT } } -func (s *Scheduler) persistCollection(ctx context.Context, recs []config.RecommendationRecord, successfulCollects []config.SuccessfulCollect, failedProviders map[string]string) { +// persistCollection writes the collection results to the recommendations +// cache via UpsertRecommendations, which: +// +// - Upserts each row by natural key so re-collected recommendations +// reuse the same PK across runs (stable IDs for frontend). +// - Evicts stale rows (collected_at < $now) but ONLY for the +// (provider, account) pairs that successfully collected in this run. +// Failed pairs' rows stay visible so the dashboard isn't blanked out +// by transient cloud-API failures, even when one of several accounts +// under the same provider succeeded. +// +// On any failure (including partial), the collection error is additionally +// recorded in recommendations_state so the frontend banner renders while +// the user still sees the valid rows we managed to upsert. +// +// UpsertRecommendations also resets last_collection_partial, so a complete +// collection clears the previous run's partial marker; a non-nil partial +// re-records it straight after (issue #87). The two writes mirror the +// existing error-column pattern and are not atomic: if the second write +// fails, the run is logged and reads as complete until the next collection. +func (s *Scheduler) persistCollection(ctx context.Context, recs []config.RecommendationRecord, successfulCollects []config.SuccessfulCollect, failedProviders map[string]string, partial *config.RecommendationsCollectionPartial) { if err := s.config.UpsertRecommendations(ctx, time.Now().UTC(), recs, successfulCollects); err != nil { logging.Errorf("Failed to persist recommendations: %v", err) if setErr := s.config.SetRecommendationsCollectionError(ctx, err.Error()); setErr != nil { @@ -409,6 +437,12 @@ func (s *Scheduler) persistCollection(ctx context.Context, recs []config.Recomme logging.Errorf("Failed to record collection error: %v", err) } } + + if partial != nil { + if err := s.config.SetRecommendationsCollectionPartial(ctx, partial); err != nil { + logging.Errorf("Failed to record partial collection: %v", err) + } + } } // joinProviderErrors formats a "provider: err; provider: err" string for @@ -428,8 +462,10 @@ func joinProviderErrors(failed map[string]string) string { // []config.SuccessfulCollect for account-scoped eviction. For the AWS // ambient path (no registered accounts) the slice contains a single empty // string sentinel that expandSuccessfulCollects translates into a nil -// CloudAccountID. Provider-level errors propagate unchanged. -func (s *Scheduler) collectProviderRecommendations(ctx context.Context, providerName string, globalCfg *config.GlobalConfig) ([]config.RecommendationRecord, []string, error) { +// CloudAccountID. The *sweepGap is non-nil when some account's sweep was +// incomplete (its rows are kept; see tolerateIncompleteSweep). Provider-level +// errors propagate unchanged. +func (s *Scheduler) collectProviderRecommendations(ctx context.Context, providerName string, globalCfg *config.GlobalConfig) ([]config.RecommendationRecord, []string, *sweepGap, error) { switch providerName { case "aws": return s.collectAWSRecommendations(ctx, globalCfg) @@ -439,7 +475,7 @@ func (s *Scheduler) collectProviderRecommendations(ctx context.Context, provider return s.collectGCPRecommendations(ctx, globalCfg) default: logging.Warnf("Unknown provider: %s", providerName) - return nil, nil, nil + return nil, nil, nil, nil } } @@ -483,17 +519,17 @@ func expandSuccessfulCollects(providerName string, accountIDs []string) []config // Returns the merged recommendations + the IDs of accounts that succeeded // (or [""] for the ambient path so the caller can synthesize a nil // CloudAccountID for eviction). -func (s *Scheduler) collectAWSRecommendations(ctx context.Context, globalCfg *config.GlobalConfig) ([]config.RecommendationRecord, []string, error) { +func (s *Scheduler) collectAWSRecommendations(ctx context.Context, globalCfg *config.GlobalConfig) ([]config.RecommendationRecord, []string, *sweepGap, error) { accounts, err := s.enabledAccounts(ctx, "aws") if err != nil { - return nil, nil, err + return nil, nil, nil, err } // Backward-compatible fallback: no registered accounts → ambient credentials if len(accounts) == 0 { - recs, complete, err := s.collectAWSAmbient(ctx, globalCfg) + recs, gap, err := s.collectAWSAmbient(ctx, globalCfg) if err != nil { - return nil, nil, err + return nil, nil, nil, err } // Issue #604: if the Lambda's STS identity matches a registered // (possibly disabled) cloud_accounts row, tag every rec with that @@ -503,18 +539,18 @@ func (s *Scheduler) collectAWSRecommendations(ctx context.Context, globalCfg *co // in those cases. if acctID := s.resolveAmbientHostAccountID(ctx); acctID != "" { recs = s.tagAccount(recs, acctID) - return recs, ambientEvictionRoster(complete, acctID), nil + return recs, ambientEvictionRoster(gap == nil, acctID), gap.forAccount(), nil } - return recs, ambientEvictionRoster(complete, ""), nil + return recs, ambientEvictionRoster(gap == nil, ""), gap.forAccount(), nil } - recs, outcome := fanOutPerAccount(ctx, "AWS", accounts, func(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, bool, error) { + recs, outcome := fanOutPerAccount(ctx, "AWS", accounts, func(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, *sweepGap, error) { return s.collectAWSForAccount(ctx, globalCfg, acct) }) if outcome.FailedCount == len(accounts) && len(accounts) > 0 { - return nil, nil, errAllAccountsFailed("AWS", outcome) + return nil, nil, nil, errAllAccountsFailed("AWS", outcome) } - return recs, outcome.SucceededAccountIDs, nil + return recs, outcome.SucceededAccountIDs, outcome.Gap, nil } // accountOutcome is the per-provider summary returned by @@ -541,7 +577,12 @@ func (s *Scheduler) collectAWSRecommendations(ctx context.Context, globalCfg *co // account while a sweep fails per subscription; evicting on a partial // sweep deletes the un-queried subscriptions' previous rows. See // tolerateIncompleteSweep and issue #1652. +// +// Gap is the merged incomplete-sweep summary across IncompleteAccountIDs +// (nil when every succeeded account swept completely), surfaced in +// recommendations_state.last_collection_partial (issue #87). type accountOutcome struct { + Gap *sweepGap LastErr string SucceededAccountIDs []string IncompleteAccountIDs []string @@ -563,7 +604,7 @@ func fanOutPerAccount( ctx context.Context, providerLabel string, accounts []config.CloudAccount, - fn func(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, bool, error), + fn func(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, *sweepGap, error), ) ([]config.RecommendationRecord, accountOutcome) { g, gctx := errgroup.WithContext(ctx) g.SetLimit(execution.ConcurrencyFromEnv()) @@ -577,7 +618,7 @@ func fanOutPerAccount( for _rvc := range accounts { acct := accounts[_rvc] g.Go(func() error { - recs, complete, err := fn(gctx, acct) + recs, gap, err := fn(gctx, acct) if err != nil { if isAccountPermissionError(providerLabel, err) { // Operator-fixable misconfiguration (missing IAM role @@ -604,10 +645,11 @@ func fanOutPerAccount( // SucceededAccountIDs, the roster that authorizes stale-row // eviction. See tolerateIncompleteSweep for why (issue #1652). outcome.SucceededCount++ - if complete { + if gap == nil { outcome.SucceededAccountIDs = append(outcome.SucceededAccountIDs, acct.ID) } else { outcome.IncompleteAccountIDs = append(outcome.IncompleteAccountIDs, acct.ID) + outcome.Gap = mergeSweepGaps(outcome.Gap, gap.forAccount()) } mu.Unlock() return nil @@ -651,10 +693,10 @@ func errAllAccountsFailed(providerLabel string, outcome accountOutcome) error { providerLabel, outcome.FailedCount, outcome.LastErr) } -func (s *Scheduler) collectAWSAmbient(ctx context.Context, globalCfg *config.GlobalConfig) ([]config.RecommendationRecord, bool, error) { +func (s *Scheduler) collectAWSAmbient(ctx context.Context, globalCfg *config.GlobalConfig) ([]config.RecommendationRecord, *sweepGap, error) { prov, err := s.providerFactory.CreateAndValidateProvider(ctx, "aws", nil) if err != nil { - return nil, false, fmt.Errorf("failed to create AWS provider: %w", err) + return nil, nil, fmt.Errorf("failed to create AWS provider: %w", err) } return s.fetchAndConvert(ctx, prov, "aws", nil, s.hostAWSAccountID(ctx), globalCfg) } @@ -735,46 +777,46 @@ func (s *Scheduler) resolveAmbientAccountID(ctx context.Context, providerName, e // fallback path in collectAzureRecommendations when no accounts are registered. // subscriptionID is passed explicitly to avoid an unnecessary Azure API round-trip // to auto-discover subscriptions — the caller already resolved it from env. -func (s *Scheduler) collectAzureAmbient(ctx context.Context, subscriptionID string) ([]config.RecommendationRecord, bool, error) { +func (s *Scheduler) collectAzureAmbient(ctx context.Context, subscriptionID string) ([]config.RecommendationRecord, *sweepGap, error) { prov, err := s.providerFactory.CreateAndValidateProvider(ctx, "azure", &provider.ProviderConfig{ AzureSubscriptionID: subscriptionID, }) if err != nil { - return nil, false, fmt.Errorf("create ambient Azure provider: %w", err) + return nil, nil, fmt.Errorf("create ambient Azure provider: %w", err) } recClient, err := prov.GetRecommendationsClient(ctx) if err != nil { - return nil, false, fmt.Errorf("get Azure recommendations client: %w", err) + return nil, nil, fmt.Errorf("get Azure recommendations client: %w", err) } recs, err := recClient.GetAllRecommendations(ctx) - complete, err := tolerateIncompleteSweep("azure", err) + gap, err := tolerateIncompleteSweep("azure", err) if err != nil { - return nil, false, fmt.Errorf("get Azure recommendations: %w", err) + return nil, nil, fmt.Errorf("get Azure recommendations: %w", err) } - return s.convertRecommendations(recs, "azure"), complete, nil + return s.convertRecommendations(recs, "azure"), gap, nil } // collectGCPAmbient collects GCP recommendations using Application Default // Credentials. Used by the ambient fallback path in collectGCPRecommendations // when no accounts are registered. -func (s *Scheduler) collectGCPAmbient(ctx context.Context) ([]config.RecommendationRecord, bool, error) { +func (s *Scheduler) collectGCPAmbient(ctx context.Context) ([]config.RecommendationRecord, *sweepGap, error) { prov, err := s.providerFactory.CreateAndValidateProvider(ctx, "gcp", nil) if err != nil { - return nil, false, fmt.Errorf("create ambient GCP provider: %w", err) + return nil, nil, fmt.Errorf("create ambient GCP provider: %w", err) } recClient, err := prov.GetRecommendationsClient(ctx) if err != nil { - return nil, false, fmt.Errorf("get GCP recommendations client: %w", err) + return nil, nil, fmt.Errorf("get GCP recommendations client: %w", err) } recs, err := recClient.GetAllRecommendations(ctx) - complete, err := tolerateIncompleteSweep("gcp", err) + gap, err := tolerateIncompleteSweep("gcp", err) if err != nil { - return nil, false, fmt.Errorf("get GCP recommendations: %w", err) + return nil, nil, fmt.Errorf("get GCP recommendations: %w", err) } - return s.convertRecommendations(recs, "gcp"), complete, nil + return s.convertRecommendations(recs, "gcp"), gap, nil } -func (s *Scheduler) collectAWSForAccount(ctx context.Context, globalCfg *config.GlobalConfig, acct config.CloudAccount) ([]config.RecommendationRecord, bool, error) { +func (s *Scheduler) collectAWSForAccount(ctx context.Context, globalCfg *config.GlobalConfig, acct config.CloudAccount) ([]config.RecommendationRecord, *sweepGap, error) { // Self-account: role_arn mode with no role ARN means "use the CUDly // Lambda's own (ambient) credentials to access this account" -- see // credentials.resolveRoleARNProvider, which documents the identical @@ -789,22 +831,22 @@ func (s *Scheduler) collectAWSForAccount(ctx context.Context, globalCfg *config. // host-identity check instead (issue #402). if acct.AWSAuthMode == "role_arn" && acct.AWSRoleARN == "" { if err := credentials.VerifyHostAccount(ctx, &acct, s.stsClient); err != nil { - return nil, false, err + return nil, nil, err } prov, err := s.providerFactory.CreateAndValidateProvider(ctx, "aws", nil) if err != nil { - return nil, false, fmt.Errorf("create ambient provider: %w", err) + return nil, nil, fmt.Errorf("create ambient provider: %w", err) } return s.fetchAndConvert(ctx, prov, "aws", &acct.ID, acct.ExternalID, globalCfg) } awsCreds, err := credentials.ResolveAWSCredentialProvider(ctx, &acct, s.credStore, s.assumeRoleSTS) if err != nil { - return nil, false, fmt.Errorf("resolve credentials: %w", err) + return nil, nil, fmt.Errorf("resolve credentials: %w", err) } cfg := &provider.ProviderConfig{Name: "aws", AWSCredentialsProvider: awsCreds} prov, err := s.providerFactory.CreateAndValidateProvider(ctx, "aws", cfg) if err != nil { - return nil, false, fmt.Errorf("create provider: %w", err) + return nil, nil, fmt.Errorf("create provider: %w", err) } return s.fetchAndConvert(ctx, prov, "aws", &acct.ID, acct.ExternalID, globalCfg) } @@ -815,10 +857,10 @@ func (s *Scheduler) collectAWSForAccount(ctx context.Context, globalCfg *config. // running natively on Azure with managed identity), falls back to ambient // credentials and tags recommendations with the registered subscription's // UUID if found in cloud_accounts. -func (s *Scheduler) collectAzureRecommendations(ctx context.Context, _ *config.GlobalConfig) ([]config.RecommendationRecord, []string, error) { +func (s *Scheduler) collectAzureRecommendations(ctx context.Context, _ *config.GlobalConfig) ([]config.RecommendationRecord, []string, *sweepGap, error) { accounts, err := s.enabledAccounts(ctx, "azure") if err != nil { - return nil, nil, err + return nil, nil, nil, err } if len(accounts) == 0 { // Issue #662: mirror the AWS ambient-path tagging fix for Azure. @@ -828,27 +870,27 @@ func (s *Scheduler) collectAzureRecommendations(ctx context.Context, _ *config.G subscriptionID := os.Getenv("AZURE_SUBSCRIPTION_ID") if subscriptionID == "" { logging.Info("No enabled Azure accounts — skipping Azure recommendations") - return nil, nil, nil + return nil, nil, nil, nil } - recs, complete, err := s.collectAzureAmbient(ctx, subscriptionID) + recs, gap, err := s.collectAzureAmbient(ctx, subscriptionID) if err != nil { - return nil, nil, err + return nil, nil, nil, err } if acctID := s.resolveAmbientAccountID(ctx, "azure", subscriptionID); acctID != "" { recs = s.tagAccount(recs, acctID) - return recs, ambientEvictionRoster(complete, acctID), nil + return recs, ambientEvictionRoster(gap == nil, acctID), gap.forAccount(), nil } - return recs, ambientEvictionRoster(complete, ""), nil + return recs, ambientEvictionRoster(gap == nil, ""), gap.forAccount(), nil } recs, outcome := fanOutPerAccount(ctx, "Azure", accounts, s.collectAzureForAccount) if outcome.FailedCount == len(accounts) && len(accounts) > 0 { - return nil, nil, errAllAccountsFailed("Azure", outcome) + return nil, nil, nil, errAllAccountsFailed("Azure", outcome) } - return recs, outcome.SucceededAccountIDs, nil + return recs, outcome.SucceededAccountIDs, outcome.Gap, nil } -func (s *Scheduler) collectAzureForAccount(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, bool, error) { +func (s *Scheduler) collectAzureForAccount(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, *sweepGap, error) { // Every recommendation returned below is tagged with THIS account's UUID // (see tagAccount at the end of this function), so the provider must be // pinned to this account's subscription. An empty AzureSubscriptionID @@ -857,7 +899,7 @@ func (s *Scheduler) collectAzureForAccount(ctx context.Context, acct config.Clou // other subscriptions' recommendations under this account and expose them // to anyone authorized for it. Fail loud instead; the row is misconfigured. if acct.AzureSubscriptionID == "" { - return nil, false, fmt.Errorf("cloud account %s has no azure_subscription_id configured", acct.ID) + return nil, nil, fmt.Errorf("cloud account %s has no azure_subscription_id configured", acct.ID) } azCred, err := credentials.ResolveAzureTokenCredentialWithOpts(ctx, &acct, s.credStore, credentials.AzureResolveOptions{ @@ -865,24 +907,24 @@ func (s *Scheduler) collectAzureForAccount(ctx context.Context, acct config.Clou IssuerURL: s.oidcIssuerURL, }) if err != nil { - return nil, false, fmt.Errorf("resolve credentials: %w", err) + return nil, nil, fmt.Errorf("resolve credentials: %w", err) } azProv, err := azureprovider.NewAzureProvider(&provider.ProviderConfig{Profile: acct.AzureSubscriptionID}) if err != nil { - return nil, false, fmt.Errorf("create provider: %w", err) + return nil, nil, fmt.Errorf("create provider: %w", err) } azProv.SetCredential(azCred) recClient, err := azProv.GetRecommendationsClient(ctx) if err != nil { - return nil, false, fmt.Errorf("get recommendations client: %w", err) + return nil, nil, fmt.Errorf("get recommendations client: %w", err) } recs, err := recClient.GetAllRecommendations(ctx) - complete, err := tolerateIncompleteSweep("azure", err) + gap, err := tolerateIncompleteSweep("azure", err) if err != nil { - return nil, false, fmt.Errorf("get recommendations: %w", err) + return nil, nil, fmt.Errorf("get recommendations: %w", err) } - return s.tagAccount(s.convertRecommendations(recs, "azure"), acct.ID), complete, nil + return s.tagAccount(s.convertRecommendations(recs, "azure"), acct.ID), gap, nil } // collectGCPRecommendations fans out across all enabled GCP accounts, @@ -891,10 +933,10 @@ func (s *Scheduler) collectAzureForAccount(ctx context.Context, acct config.Clou // running natively on GCP with ADC), falls back to ambient credentials // and tags recommendations with the registered project's UUID if found // in cloud_accounts. -func (s *Scheduler) collectGCPRecommendations(ctx context.Context, _ *config.GlobalConfig) ([]config.RecommendationRecord, []string, error) { +func (s *Scheduler) collectGCPRecommendations(ctx context.Context, _ *config.GlobalConfig) ([]config.RecommendationRecord, []string, *sweepGap, error) { accounts, err := s.enabledAccounts(ctx, "gcp") if err != nil { - return nil, nil, err + return nil, nil, nil, err } if len(accounts) == 0 { // Issue #662: mirror the AWS ambient-path tagging fix for GCP. @@ -904,33 +946,33 @@ func (s *Scheduler) collectGCPRecommendations(ctx context.Context, _ *config.Glo projectID := os.Getenv("GCP_PROJECT_ID") if projectID == "" { logging.Info("No enabled GCP accounts — skipping GCP recommendations") - return nil, nil, nil + return nil, nil, nil, nil } - recs, complete, err := s.collectGCPAmbient(ctx) + recs, gap, err := s.collectGCPAmbient(ctx) if err != nil { - return nil, nil, err + return nil, nil, nil, err } if acctID := s.resolveAmbientAccountID(ctx, "gcp", projectID); acctID != "" { recs = s.tagAccount(recs, acctID) - return recs, ambientEvictionRoster(complete, acctID), nil + return recs, ambientEvictionRoster(gap == nil, acctID), gap.forAccount(), nil } - return recs, ambientEvictionRoster(complete, ""), nil + return recs, ambientEvictionRoster(gap == nil, ""), gap.forAccount(), nil } recs, outcome := fanOutPerAccount(ctx, "GCP", accounts, s.collectGCPForAccount) if outcome.FailedCount == len(accounts) && len(accounts) > 0 { - return nil, nil, errAllAccountsFailed("GCP", outcome) + return nil, nil, nil, errAllAccountsFailed("GCP", outcome) } - return recs, outcome.SucceededAccountIDs, nil + return recs, outcome.SucceededAccountIDs, outcome.Gap, nil } -func (s *Scheduler) collectGCPForAccount(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, bool, error) { +func (s *Scheduler) collectGCPForAccount(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, *sweepGap, error) { gcpTS, err := credentials.ResolveGCPTokenSourceWithOpts(ctx, &acct, s.credStore, credentials.GCPResolveOptions{ Signer: s.oidcSigner, IssuerURL: s.oidcIssuerURL, }) if err != nil { - return nil, false, fmt.Errorf("resolve credentials: %w", err) + return nil, nil, fmt.Errorf("resolve credentials: %w", err) } var prov provider.Provider @@ -940,21 +982,21 @@ func (s *Scheduler) collectGCPForAccount(ctx context.Context, acct config.CloudA // ADC mode (application_default): use ambient credentials created, createErr := s.providerFactory.CreateAndValidateProvider(ctx, "gcp", nil) if createErr != nil { - return nil, false, fmt.Errorf("create ambient GCP provider: %w", createErr) + return nil, nil, fmt.Errorf("create ambient GCP provider: %w", createErr) } prov = created } recClient, err := prov.GetRecommendationsClient(ctx) if err != nil { - return nil, false, fmt.Errorf("get recommendations client: %w", err) + return nil, nil, fmt.Errorf("get recommendations client: %w", err) } recs, err := recClient.GetAllRecommendations(ctx) - complete, err := tolerateIncompleteSweep("gcp", err) + gap, err := tolerateIncompleteSweep("gcp", err) if err != nil { - return nil, false, fmt.Errorf("get recommendations: %w", err) + return nil, nil, fmt.Errorf("get recommendations: %w", err) } - return s.tagAccount(s.convertRecommendations(recs, "gcp"), acct.ID), complete, nil + return s.tagAccount(s.convertRecommendations(recs, "gcp"), acct.ID), gap, nil } // enabledAccounts returns all enabled cloud accounts for the given provider. @@ -991,8 +1033,9 @@ func (s *Scheduler) enabledAccounts(ctx context.Context, providerName string) ([ // other error is returned unchanged, preserving the existing fail-loud // behavior. // -// The complete return value is what stops a tolerated sweep from being -// mistaken for a whole one. It is false only for a partial sweep, and callers +// The gap return value is what stops a tolerated sweep from being mistaken +// for a whole one. It is nil for a complete sweep (and for a hard error, which +// callers handle via err first); non-nil only for a partial sweep, and callers // must keep such an account OUT of the succeeded-account roster: that roster // drives UpsertRecommendations' stale-row eviction, which deletes with // @@ -1011,45 +1054,45 @@ func (s *Scheduler) enabledAccounts(ctx context.Context, providerName string) ([ // flaky subscription, the opposite failure and the one the succeeded-account // roster exists to prevent. // -// NOTE: the incompleteness still reaches the state table only as a log line; -// UpsertRecommendations continues to clear last_collection_error. That column -// is a single global row (recommendations_state WHERE id = 1), not per -// account, so representing one account's partial sweep in it is a separate -// design decision and is left as follow-up work. With eviction withheld the -// residue is observability, not lost rows. -func tolerateIncompleteSweep(providerName string, err error) (complete bool, _ error) { +// The gap is also how the incompleteness leaves the logs (issue #87): it is +// threaded up through fanOutPerAccount and collectAllProviders into +// recommendations_state.last_collection_partial, which the freshness API +// serves, so the dashboard can tell 1-of-10 coverage from full coverage. It +// is sanitized (counts and reason categories); the failed subscription IDs +// and raw errors stay in this WARN log only. See sweepGap. +func tolerateIncompleteSweep(providerName string, err error) (*sweepGap, error) { var incomplete *awsrecommendations.IncompleteRecommendationsError if errors.As(err, &incomplete) { logging.Warnf("%s recommendations incomplete: failed_details=%d failed_scopes=%d; %v; keeping collected rows and withholding stale-row eviction", providerName, incomplete.FailedDetails, incomplete.FailedScopes, incomplete) - return false, nil + return gapFromAWSIncomplete(incomplete), nil } partial := azureprovider.AsPartialSubscriptionFailure(err) if partial == nil { - return err == nil, err + return nil, err } logging.Warnf( "%s recommendations incomplete: %d of %d subscriptions succeeded; not queried: %s (keeping the %d that did, "+ "and holding back stale-row eviction so the un-queried subscriptions' previous rows survive)", providerName, partial.Succeeded, partial.Attempted, strings.Join(partial.FailedSubscriptionIDs(), ", "), partial.Succeeded) - return false, nil + return gapFromPartialSubscriptions(partial), nil } // fetchAndConvert is a convenience for the AWS ambient path. // -// The bool reports whether the sweep covered everything it was asked to; see +// The *sweepGap is nil when the sweep covered everything it was asked to; see // tolerateIncompleteSweep for why an incomplete one must not authorize // stale-row eviction. -func (s *Scheduler) fetchAndConvert(ctx context.Context, prov provider.Provider, providerName string, accountID *string, expectedAccount string, globalCfg *config.GlobalConfig) ([]config.RecommendationRecord, bool, error) { +func (s *Scheduler) fetchAndConvert(ctx context.Context, prov provider.Provider, providerName string, accountID *string, expectedAccount string, globalCfg *config.GlobalConfig) ([]config.RecommendationRecord, *sweepGap, error) { recClient, err := prov.GetRecommendationsClient(ctx) if err != nil { - return nil, false, fmt.Errorf("failed to get %s recommendations client: %w", providerName, err) + return nil, nil, fmt.Errorf("failed to get %s recommendations client: %w", providerName, err) } recs, err := recClient.GetAllRecommendations(ctx) - complete, err := tolerateIncompleteSweep(providerName, err) + gap, err := tolerateIncompleteSweep(providerName, err) if err != nil { - return nil, false, fmt.Errorf("failed to get %s recommendations: %w", providerName, err) + return nil, nil, fmt.Errorf("failed to get %s recommendations: %w", providerName, err) } recs = dropForeignZonalEC2(recs, expectedAccount) if len(recs) == 0 && globalCfg != nil { @@ -1065,20 +1108,20 @@ func (s *Scheduler) fetchAndConvert(ctx context.Context, prov provider.Provider, LookbackPeriod: fmt.Sprintf("%dd", lookbackDays), } fallbackRecs, recErr := recClient.GetRecommendations(ctx, ¶ms) - fallbackComplete, recErr := tolerateIncompleteSweep(providerName, recErr) + fallbackGap, recErr := tolerateIncompleteSweep(providerName, recErr) if recErr != nil { - return nil, false, fmt.Errorf("failed to get %s recommendations with default term/payment fallback (service=%s, term=%s, payment=%s, lookback=%s): %w", + return nil, nil, fmt.Errorf("failed to get %s recommendations with default term/payment fallback (service=%s, term=%s, payment=%s, lookback=%s): %w", providerName, params.Service, params.Term, params.PaymentOption, params.LookbackPeriod, recErr) } recs = append(recs, dropForeignZonalEC2(fallbackRecs, expectedAccount)...) - complete = complete && fallbackComplete + gap = mergeSweepGaps(gap, fallbackGap) } } result := s.convertRecommendations(recs, providerName) if accountID != nil { result = s.tagAccount(result, *accountID) } - return result, complete, nil + return result, gap, nil } // tagAccount assigns the stored account identity and updates its recommendation ID. diff --git a/internal/scheduler/scheduler_global_filters_integration_test.go b/internal/scheduler/scheduler_global_filters_integration_test.go index 4805e14c..c1270326 100644 --- a/internal/scheduler/scheduler_global_filters_integration_test.go +++ b/internal/scheduler/scheduler_global_filters_integration_test.go @@ -132,7 +132,7 @@ func TestPersistCollectionMultiAZInOneBatch(t *testing.T) { } s := &Scheduler{config: store} - s.persistCollection(ctx, recs, []config.SuccessfulCollect{{Provider: "aws"}}, nil) + s.persistCollection(ctx, recs, []config.SuccessfulCollect{{Provider: "aws"}}, nil, nil) rows, err := store.ListStoredRecommendations(ctx, config.RecommendationFilter{}) require.NoError(t, err) @@ -171,7 +171,7 @@ func TestPersistCollectionPlatformTenancyInOneBatch(t *testing.T) { } } persist := func(recs ...common.Recommendation) { - s.persistCollection(ctx, s.convertRecommendations(recs, "aws"), []config.SuccessfulCollect{{Provider: "aws"}}, nil) + s.persistCollection(ctx, s.convertRecommendations(recs, "aws"), []config.SuccessfulCollect{{Provider: "aws"}}, nil, nil) } keys := func() map[string]bool { rows, err := pg.DB.Pool().Query(ctx, `SELECT platform || '|' || tenancy FROM recommendations`) diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 3d1d5e67..9fcf5169 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -509,7 +509,7 @@ func TestScheduler_CollectAWSRecommendations(t *testing.T) { providerFactory: mockFactory, } - recs, _, err := scheduler.collectAWSRecommendations(ctx, globalCfg) + recs, _, _, err := scheduler.collectAWSRecommendations(ctx, globalCfg) require.Error(t, err) // Should error due to mock provider failing assert.Nil(t, recs) } @@ -527,7 +527,7 @@ func TestScheduler_CollectAzureRecommendations_NoAccounts(t *testing.T) { scheduler := &Scheduler{config: mockStore} - recs, _, err := scheduler.collectAzureRecommendations(ctx, globalCfg) + recs, _, _, err := scheduler.collectAzureRecommendations(ctx, globalCfg) require.NoError(t, err) assert.Empty(t, recs) } @@ -545,7 +545,7 @@ func TestScheduler_CollectGCPRecommendations_NoAccounts_Alt(t *testing.T) { scheduler := &Scheduler{config: mockStore} - recs, _, err := scheduler.collectGCPRecommendations(ctx, globalCfg) + recs, _, _, err := scheduler.collectGCPRecommendations(ctx, globalCfg) require.NoError(t, err) assert.Empty(t, recs) } @@ -583,7 +583,7 @@ func TestScheduler_CollectProviderRecommendations(t *testing.T) { for _, tt := range tests { t.Run(tt.provider, func(t *testing.T) { - recs, _, err := scheduler.collectProviderRecommendations(ctx, tt.provider, globalCfg) + recs, _, _, err := scheduler.collectProviderRecommendations(ctx, tt.provider, globalCfg) if tt.expectError { require.Error(t, err) } else { @@ -755,14 +755,14 @@ func TestScheduler_FetchAndConvert_KeepsPartialSweepData(t *testing.T) { // globalCfg nil so the zero-results fallback branch stays out of the way; // collected is non-empty anyway. - recs, complete, err := s.fetchAndConvert(ctx, prov, "azure", nil, "", nil) + recs, gap, err := s.fetchAndConvert(ctx, prov, "azure", nil, "", nil) require.NoError(t, err, "a partial multi-subscription sweep must not fail the whole collection") require.Len(t, recs, 1, "the subscriptions that succeeded must still be persisted, not discarded") assert.Equal(t, 3, recs[0].Count, "the surviving subscription's recommendation must be intact") - assert.False(t, complete, + assert.NotNil(t, gap, "a partial sweep must report itself incomplete so the caller withholds stale-row eviction (#1652)") } @@ -781,17 +781,16 @@ func TestScheduler_FetchAndConvert_RealErrorStillFailsLoud(t *testing.T) { s := &Scheduler{config: new(MockConfigStore)} - recs, complete, err := s.fetchAndConvert(ctx, prov, "azure", nil, "", nil) + recs, _, err := s.fetchAndConvert(ctx, prov, "azure", nil, "", nil) require.Error(t, err, "a genuine error must still fail the collection") assert.Contains(t, err.Error(), "credentials expired") assert.Nil(t, recs) - assert.False(t, complete, "a failed sweep is never eviction-eligible") } func TestTolerateIncompleteSweep(t *testing.T) { // Swallowing the error is only half the contract. The other half is - // reporting complete=false, which is what keeps the account out of the + // reporting a non-nil gap, which is what keeps the account out of the // eviction roster; asserting only NoError here would encode the #1652 // hazard as intended behavior. t.Run("partial failure is swallowed but reported as incomplete", func(t *testing.T) { @@ -799,23 +798,25 @@ func TestTolerateIncompleteSweep(t *testing.T) { Attempted: 2, Succeeded: 1, Failed: []azureprovider.SubscriptionFailure{{SubscriptionID: "sub-1", Err: errors.New("boom")}}, } - complete, err := tolerateIncompleteSweep("azure", partial) + gap, err := tolerateIncompleteSweep("azure", partial) assert.NoError(t, err, "the collected subscriptions' data must be kept") - assert.False(t, complete, + require.NotNil(t, gap, "an incomplete sweep must not authorize stale-row eviction for the account (#1652)") + assert.Equal(t, 2, gap.attemptedScopes) + assert.Equal(t, 1, gap.failedScopes) }) t.Run("other errors pass through unchanged", func(t *testing.T) { boom := errors.New("boom") - complete, err := tolerateIncompleteSweep("azure", boom) + gap, err := tolerateIncompleteSweep("azure", boom) assert.Same(t, boom, err) - assert.False(t, complete, "a failed sweep is never eviction-eligible") + assert.Nil(t, gap, "a hard failure is an error, not a partial sweep") }) t.Run("nil stays nil and is complete", func(t *testing.T) { - complete, err := tolerateIncompleteSweep("azure", nil) + gap, err := tolerateIncompleteSweep("azure", nil) assert.NoError(t, err) - assert.True(t, complete, + assert.Nil(t, gap, "a clean sweep must stay eviction-eligible, otherwise stale rows accumulate forever") }) } @@ -1311,13 +1312,13 @@ func TestFanOutPerAccount_RespectsParallelismLimit(t *testing.T) { } } - fn := func(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, bool, error) { + fn := func(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, *sweepGap, error) { cur := inflight.Add(1) updatePeak(cur) // Small sleep so concurrent workers genuinely overlap. time.Sleep(5 * time.Millisecond) inflight.Add(-1) - return []config.RecommendationRecord{{ID: acct.ID}}, true, nil + return []config.RecommendationRecord{{ID: acct.ID}}, nil, nil } out, outcome := fanOutPerAccount(context.Background(), "Test", accounts, fn) @@ -1339,8 +1340,8 @@ func TestFanOutPerAccount_AllAccountsFail(t *testing.T) { {ID: "acct-2", Name: "acct-2", ExternalID: "ext-2"}, {ID: "acct-3", Name: "acct-3", ExternalID: "ext-3"}, } - fn := func(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, bool, error) { - return nil, false, fmt.Errorf("cred error for %s", acct.ID) + fn := func(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, *sweepGap, error) { + return nil, nil, fmt.Errorf("cred error for %s", acct.ID) } recs, outcome := fanOutPerAccount(context.Background(), "Test", accounts, fn) @@ -1363,11 +1364,11 @@ func TestFanOutPerAccount_PartialSuccess(t *testing.T) { {ID: "acct-ok-2", Name: "acct-ok-2", ExternalID: "e2"}, {ID: "acct-bad", Name: "acct-bad", ExternalID: "ebad"}, } - fn := func(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, bool, error) { + fn := func(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, *sweepGap, error) { if acct.ID == "acct-bad" { - return nil, false, fmt.Errorf("transient") + return nil, nil, fmt.Errorf("transient") } - return []config.RecommendationRecord{{ID: acct.ID, Provider: "test"}}, true, nil + return []config.RecommendationRecord{{ID: acct.ID, Provider: "test"}}, nil, nil } recs, outcome := fanOutPerAccount(context.Background(), "Test", accounts, fn) @@ -1390,11 +1391,11 @@ func TestFanOutPerAccount_FailedCollectionNotInSucceededAccountIDs(t *testing.T) {ID: "acct-ok", Name: "acct-ok", ExternalID: "e-ok"}, {ID: "acct-all-services-failed", Name: "acct-all-services-failed", ExternalID: "e-bad"}, } - fn := func(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, bool, error) { + fn := func(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, *sweepGap, error) { if acct.ID == "acct-all-services-failed" { - return nil, false, errors.New("all 6 Azure recommendation services failed: 429 too many requests") + return nil, nil, errors.New("all 6 Azure recommendation services failed: 429 too many requests") } - return []config.RecommendationRecord{{ID: acct.ID, Provider: "azure"}}, true, nil + return []config.RecommendationRecord{{ID: acct.ID, Provider: "azure"}}, nil, nil } _, outcome := fanOutPerAccount(context.Background(), "Azure", accounts, fn) @@ -1409,9 +1410,9 @@ func TestFanOutPerAccount_FailedCollectionNotInSucceededAccountIDs(t *testing.T) // correctly skips the all-failed error path. func TestFanOutPerAccount_ZeroAccounts(t *testing.T) { recs, outcome := fanOutPerAccount(context.Background(), "Test", nil, - func(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, bool, error) { + func(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, *sweepGap, error) { t.Fatalf("fn must not be called for zero-accounts input") - return nil, false, nil + return nil, nil, nil }) assert.Empty(t, recs) assert.Zero(t, outcome.SucceededCount) @@ -1442,7 +1443,7 @@ func TestScheduler_persistCollection_PartialFailure(t *testing.T) { }) s := &Scheduler{config: mockStore} - s.persistCollection(ctx, recs, successful, failed) + s.persistCollection(ctx, recs, successful, failed, nil) mockStore.AssertExpectations(t) } @@ -1463,7 +1464,7 @@ func TestScheduler_persistCollection_FullSuccess(t *testing.T) { mockStore.On("UpsertRecommendations", ctx, mock.Anything, recs, successful).Return(nil) s := &Scheduler{config: mockStore} - s.persistCollection(ctx, recs, successful, nil) + s.persistCollection(ctx, recs, successful, nil, nil) mockStore.AssertExpectations(t) mockStore.AssertNotCalled(t, "SetRecommendationsCollectionError", mock.Anything, mock.Anything) @@ -1830,7 +1831,7 @@ func TestScheduler_CollectAWSRecommendations_Success(t *testing.T) { providerFactory: mockFactory, } - recs, _, err := scheduler.collectAWSRecommendations(ctx, globalCfg) + recs, _, _, err := scheduler.collectAWSRecommendations(ctx, globalCfg) require.NoError(t, err) assert.Len(t, recs, 1) assert.Equal(t, "ec2", recs[0].Service) @@ -1856,7 +1857,7 @@ func TestScheduler_CollectAWSRecommendations_RecClientError(t *testing.T) { providerFactory: mockFactory, } - recs, _, err := scheduler.collectAWSRecommendations(ctx, globalCfg) + recs, _, _, err := scheduler.collectAWSRecommendations(ctx, globalCfg) require.Error(t, err) assert.Nil(t, recs) } @@ -1883,7 +1884,7 @@ func TestScheduler_CollectAWSRecommendations_GetRecsError(t *testing.T) { providerFactory: mockFactory, } - recs, _, err := scheduler.collectAWSRecommendations(ctx, globalCfg) + recs, _, _, err := scheduler.collectAWSRecommendations(ctx, globalCfg) require.Error(t, err) assert.Nil(t, recs) } @@ -1928,7 +1929,7 @@ func TestScheduler_CollectAzureRecommendations_AllAccountsFailLoud(t *testing.T) config: mockStore, } - recs, succeeded, err := scheduler.collectAzureRecommendations(ctx, globalCfg) + recs, succeeded, _, err := scheduler.collectAzureRecommendations(ctx, globalCfg) require.Error(t, err, "collectAzureRecommendations must fail loud when the only account has unusable credentials (COR-03)") assert.Contains(t, err.Error(), "Azure: all 1 accounts failed", "the per-account fan-out must surface the all-accounts-failed shape so the scheduler keeps stale rows and records last_collection_error") @@ -1955,7 +1956,7 @@ func TestScheduler_CollectAzureForAccount_MissingSubscriptionIDFailsLoud(t *test ctx := context.Background() scheduler := &Scheduler{config: new(MockConfigStore)} - recs, complete, err := scheduler.collectAzureForAccount(ctx, config.CloudAccount{ + recs, gap, err := scheduler.collectAzureForAccount(ctx, config.CloudAccount{ ID: "az-no-sub", Provider: "azure", AzureAuthMode: "managed_identity", @@ -1965,7 +1966,7 @@ func TestScheduler_CollectAzureForAccount_MissingSubscriptionIDFailsLoud(t *test require.Error(t, err) assert.Nil(t, recs) - assert.False(t, complete, "a rejected account is never eviction-eligible") + assert.Nil(t, gap, "a rejected account is an error, not a partial sweep") assert.Contains(t, err.Error(), "no azure_subscription_id configured", "an unpinned Azure account must be rejected by name, not fall through to an unscoped org-wide collection") assert.Contains(t, err.Error(), "az-no-sub", "the error must identify the misconfigured account") @@ -1987,7 +1988,7 @@ func TestScheduler_CollectAWSForAccount_EmptyRoleARNWrongModeFailsLoud(t *testin globalCfg := &config.GlobalConfig{DefaultTerm: 3, DefaultPayment: "all-upfront"} - recs, complete, err := scheduler.collectAWSForAccount(ctx, globalCfg, config.CloudAccount{ + recs, gap, err := scheduler.collectAWSForAccount(ctx, globalCfg, config.CloudAccount{ ID: "aws-member-pending-review", Provider: "aws", ExternalID: "222222222222", @@ -1999,7 +2000,7 @@ func TestScheduler_CollectAWSForAccount_EmptyRoleARNWrongModeFailsLoud(t *testin require.Error(t, err, "an empty aws_auth_mode must fail loud, never fall back to ambient (host) credentials") assert.Contains(t, err.Error(), "unsupported aws_auth_mode") - assert.False(t, complete, "a rejected account is never eviction-eligible") + assert.Nil(t, gap, "a rejected account is an error, not a partial sweep") assert.Nil(t, recs, "zero recommendation rows must be produced for a rejected account") } @@ -2036,7 +2037,7 @@ func TestScheduler_CollectAWSForAccount_SelfAccountRoleARNModeUsesAmbient(t *tes stsClient: &fakeSTSClient{accountID: "111111111111"}, } - recs, complete, err := scheduler.collectAWSForAccount(ctx, globalCfg, config.CloudAccount{ + recs, gap, err := scheduler.collectAWSForAccount(ctx, globalCfg, config.CloudAccount{ ID: "aws-self-account", Provider: "aws", AWSAuthMode: "role_arn", @@ -2046,7 +2047,7 @@ func TestScheduler_CollectAWSForAccount_SelfAccountRoleARNModeUsesAmbient(t *tes }) require.NoError(t, err) - assert.True(t, complete) + assert.Nil(t, gap) require.Len(t, recs, 1) assert.Equal(t, "aws-self-account", *recs[0].CloudAccountID) } @@ -2063,7 +2064,7 @@ func TestScheduler_CollectAWSForAccount_RoleARNWithoutARNOnNonHostFailsClosed(t mockFactory := new(MockProviderFactory) scheduler := &Scheduler{config: new(MockConfigStore), providerFactory: mockFactory, stsClient: stsClient} - recs, complete, err := scheduler.collectAWSForAccount(context.Background(), &config.GlobalConfig{}, config.CloudAccount{ + recs, gap, err := scheduler.collectAWSForAccount(context.Background(), &config.GlobalConfig{}, config.CloudAccount{ ID: "tenant-account", Provider: "aws", AWSAuthMode: "role_arn", @@ -2072,7 +2073,7 @@ func TestScheduler_CollectAWSForAccount_RoleARNWithoutARNOnNonHostFailsClosed(t }) require.ErrorIs(t, err, credentials.ErrNotHostAccount) - assert.False(t, complete) + assert.Nil(t, gap) assert.Nil(t, recs) mockFactory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) }) @@ -2095,7 +2096,7 @@ func TestScheduler_CollectGCPRecommendations_NoAccounts(t *testing.T) { config: mockStore, } - recs, _, err := scheduler.collectGCPRecommendations(ctx, globalCfg) + recs, _, _, err := scheduler.collectGCPRecommendations(ctx, globalCfg) require.NoError(t, err) assert.Len(t, recs, 0) } @@ -2201,7 +2202,7 @@ func TestScheduler_CollectAWSRecommendations_FallbackToFiltered(t *testing.T) { providerFactory: mockFactory, } - recs, _, err := scheduler.collectAWSRecommendations(ctx, globalCfg) + recs, _, _, err := scheduler.collectAWSRecommendations(ctx, globalCfg) require.NoError(t, err) assert.Len(t, recs, 1) mockRecClient.AssertExpectations(t) @@ -2242,7 +2243,7 @@ func TestScheduler_CollectAWSRecommendations_FallbackError(t *testing.T) { providerFactory: mockFactory, } - recs, _, err := scheduler.collectAWSRecommendations(ctx, globalCfg) + recs, _, _, err := scheduler.collectAWSRecommendations(ctx, globalCfg) require.Error(t, err, "fallback GetRecommendations failure must propagate, not be swallowed") require.ErrorIs(t, err, fallbackErr) assert.Contains(t, err.Error(), "default term/payment fallback", "error must identify the fallback path") @@ -2332,7 +2333,7 @@ func TestScheduler_CollectAWSRecommendations_AmbientTagging_HappyPath(t *testing stsClient: &fakeSTSClient{accountID: "123456789012"}, } - recs, acctIDs, err := scheduler.collectAWSRecommendations(ctx, globalCfg) + recs, acctIDs, _, err := scheduler.collectAWSRecommendations(ctx, globalCfg) require.NoError(t, err) require.Len(t, recs, 1) require.NotNil(t, recs[0].CloudAccountID, "rec must be tagged with registered account UUID, not nil") @@ -2372,7 +2373,7 @@ func TestScheduler_CollectAWSRecommendations_AmbientTagging_NoRegisteredAccount( stsClient: &fakeSTSClient{accountID: "999888777666"}, } - recs, acctIDs, err := scheduler.collectAWSRecommendations(ctx, globalCfg) + recs, acctIDs, _, err := scheduler.collectAWSRecommendations(ctx, globalCfg) require.NoError(t, err) require.Len(t, recs, 1) assert.Nil(t, recs[0].CloudAccountID, "truly-orphan ambient deployment must keep CloudAccountID = nil") @@ -2406,7 +2407,7 @@ func TestScheduler_CollectAWSRecommendations_AmbientTagging_STSFailure(t *testin stsClient: &fakeSTSClient{err: errors.New("sts unreachable")}, } - recs, acctIDs, err := scheduler.collectAWSRecommendations(ctx, globalCfg) + recs, acctIDs, _, err := scheduler.collectAWSRecommendations(ctx, globalCfg) require.NoError(t, err, "STS failure must NOT fail the collection") require.Len(t, recs, 1) assert.Nil(t, recs[0].CloudAccountID, "STS failure must leave the pre-fix nil tagging in place") @@ -2516,7 +2517,7 @@ func TestScheduler_CollectAzureRecommendations_AmbientTagging_HappyPath(t *testi providerFactory: mockFactory, } - recs, acctIDs, err := sched.collectAzureRecommendations(ctx, nil) + recs, acctIDs, _, err := sched.collectAzureRecommendations(ctx, nil) require.NoError(t, err) require.Len(t, recs, 1) require.NotNil(t, recs[0].CloudAccountID, "rec must be tagged with registered Azure account UUID, not nil") @@ -2556,7 +2557,7 @@ func TestScheduler_CollectAzureRecommendations_AmbientTagging_NoRegisteredAccoun providerFactory: mockFactory, } - recs, acctIDs, err := sched.collectAzureRecommendations(ctx, nil) + recs, acctIDs, _, err := sched.collectAzureRecommendations(ctx, nil) require.NoError(t, err) require.Len(t, recs, 1) assert.Nil(t, recs[0].CloudAccountID, "truly-orphan Azure deployment must keep CloudAccountID = nil") @@ -2593,7 +2594,7 @@ func TestScheduler_CollectAzureRecommendations_AmbientTagging_StoreError(t *test providerFactory: mockFactory, } - recs, acctIDs, err := sched.collectAzureRecommendations(ctx, nil) + recs, acctIDs, _, err := sched.collectAzureRecommendations(ctx, nil) require.NoError(t, err, "store error must NOT fail the Azure collection") require.Len(t, recs, 1) assert.Nil(t, recs[0].CloudAccountID, "store error must leave the pre-fix nil tagging in place") @@ -2617,7 +2618,7 @@ func TestScheduler_CollectAzureRecommendations_NoEnvVar_Skips(t *testing.T) { providerFactory: mockFactory, } - recs, acctIDs, err := sched.collectAzureRecommendations(ctx, nil) + recs, acctIDs, _, err := sched.collectAzureRecommendations(ctx, nil) require.NoError(t, err) assert.Empty(t, recs) assert.Empty(t, acctIDs) @@ -2669,7 +2670,7 @@ func TestScheduler_CollectGCPRecommendations_AmbientTagging_HappyPath(t *testing providerFactory: mockFactory, } - recs, acctIDs, err := sched.collectGCPRecommendations(ctx, nil) + recs, acctIDs, _, err := sched.collectGCPRecommendations(ctx, nil) require.NoError(t, err) require.Len(t, recs, 1) require.NotNil(t, recs[0].CloudAccountID, "rec must be tagged with registered GCP account UUID, not nil") @@ -2707,7 +2708,7 @@ func TestScheduler_CollectGCPRecommendations_AmbientTagging_NoRegisteredAccount( providerFactory: mockFactory, } - recs, acctIDs, err := sched.collectGCPRecommendations(ctx, nil) + recs, acctIDs, _, err := sched.collectGCPRecommendations(ctx, nil) require.NoError(t, err) require.Len(t, recs, 1) assert.Nil(t, recs[0].CloudAccountID, "truly-orphan GCP deployment must keep CloudAccountID = nil") @@ -2742,7 +2743,7 @@ func TestScheduler_CollectGCPRecommendations_AmbientTagging_StoreError(t *testin providerFactory: mockFactory, } - recs, acctIDs, err := sched.collectGCPRecommendations(ctx, nil) + recs, acctIDs, _, err := sched.collectGCPRecommendations(ctx, nil) require.NoError(t, err, "store error must NOT fail the GCP collection") require.Len(t, recs, 1) assert.Nil(t, recs[0].CloudAccountID, "store error must leave the pre-fix nil tagging in place") @@ -2765,7 +2766,7 @@ func TestScheduler_CollectGCPRecommendations_NoEnvVar_Skips(t *testing.T) { providerFactory: mockFactory, } - recs, acctIDs, err := sched.collectGCPRecommendations(ctx, nil) + recs, acctIDs, _, err := sched.collectGCPRecommendations(ctx, nil) require.NoError(t, err) assert.Empty(t, recs) assert.Empty(t, acctIDs) diff --git a/internal/scheduler/zonal_recs_test.go b/internal/scheduler/zonal_recs_test.go index a9c149d7..8fd3f4ee 100644 --- a/internal/scheduler/zonal_recs_test.go +++ b/internal/scheduler/zonal_recs_test.go @@ -88,9 +88,9 @@ func mixedAccountRecs(own string) []common.Recommendation { return []common.Recommendation{zonalRec(own, "us-east-1a"), zonalRec("222222222222", "us-east-1b"), memberRegional} } -func assertOnlyMemberZonalDropped(t *testing.T, recs []config.RecommendationRecord, complete bool) { +func assertOnlyMemberZonalDropped(t *testing.T, recs []config.RecommendationRecord, gap *sweepGap) { t.Helper() - assert.True(t, complete, "dropping a foreign zonal rec must not mark the collection incomplete") + assert.Nil(t, gap, "dropping a foreign zonal rec must not mark the collection incomplete") require.Len(t, recs, 2) azs := []string{recs[0].AvailabilityZone, recs[1].AvailabilityZone} assert.ElementsMatch(t, []string{"us-east-1a", ""}, azs, "own zonal and member regional kept; member zonal dropped") @@ -106,9 +106,9 @@ func TestFetchAndConvert_DropsForeignZonalEC2_Primary(t *testing.T) { client := new(MockRecommendationsClient) client.On("GetAllRecommendations", mock.Anything).Return(mixedAccountRecs("111111111111"), nil) s := &Scheduler{} - recs, complete, err := s.fetchAndConvert(context.Background(), newRecProvider(client), "aws", nil, "111111111111", nil) + recs, gap, err := s.fetchAndConvert(context.Background(), newRecProvider(client), "aws", nil, "111111111111", nil) require.NoError(t, err) - assertOnlyMemberZonalDropped(t, recs, complete) + assertOnlyMemberZonalDropped(t, recs, gap) } func TestFetchAndConvert_DropsForeignZonalEC2_LookbackFallback(t *testing.T) { @@ -121,10 +121,10 @@ func TestFetchAndConvert_DropsForeignZonalEC2_LookbackFallback(t *testing.T) { return p.Service != common.ServiceEC2 })).Return([]common.Recommendation{}, nil) s := &Scheduler{} - recs, complete, err := s.fetchAndConvert(context.Background(), newRecProvider(client), "aws", nil, "111111111111", + recs, gap, err := s.fetchAndConvert(context.Background(), newRecProvider(client), "aws", nil, "111111111111", &config.GlobalConfig{DefaultTerm: 1, DefaultPayment: "no-upfront", RecommendationsLookbackDays: 30}) require.NoError(t, err) - assertOnlyMemberZonalDropped(t, recs, complete) + assertOnlyMemberZonalDropped(t, recs, gap) } func TestFetchAndConvert_NilExpectedAccountKeepsForeignZonal(t *testing.T) { @@ -144,9 +144,9 @@ func TestCollectAWSAmbient_PassesHostAccount(t *testing.T) { client.On("GetAllRecommendations", mock.Anything).Return(mixedAccountRecs("111111111111"), nil) factory.On("CreateAndValidateProvider", ctx, "aws", (*provider.ProviderConfig)(nil)).Return(newRecProvider(client), nil) s := &Scheduler{providerFactory: factory, stsClient: &fakeSTSClient{accountID: "111111111111"}} - recs, complete, err := s.collectAWSAmbient(ctx, nil) + recs, gap, err := s.collectAWSAmbient(ctx, nil) require.NoError(t, err) - assertOnlyMemberZonalDropped(t, recs, complete) + assertOnlyMemberZonalDropped(t, recs, gap) } // A registered self account must compare against its ExternalID. @@ -157,11 +157,11 @@ func TestCollectAWSForAccount_SelfAccountPassesExternalID(t *testing.T) { client.On("GetAllRecommendations", mock.Anything).Return(mixedAccountRecs("111111111111"), nil) factory.On("CreateAndValidateProvider", ctx, "aws", (*provider.ProviderConfig)(nil)).Return(newRecProvider(client), nil) s := &Scheduler{config: new(MockConfigStore), providerFactory: factory, stsClient: &fakeSTSClient{accountID: "111111111111"}} - recs, complete, err := s.collectAWSForAccount(ctx, nil, config.CloudAccount{ + recs, gap, err := s.collectAWSForAccount(ctx, nil, config.CloudAccount{ ID: "self", Provider: "aws", AWSAuthMode: "role_arn", ExternalID: "111111111111", Enabled: true, }) require.NoError(t, err) - assertOnlyMemberZonalDropped(t, recs, complete) + assertOnlyMemberZonalDropped(t, recs, gap) } // A member account reached through an assumed role must compare against its @@ -173,12 +173,12 @@ func TestCollectAWSForAccount_AssumedRolePassesExternalID(t *testing.T) { client.On("GetAllRecommendations", mock.Anything).Return(mixedAccountRecs("333333333333"), nil) factory.On("CreateAndValidateProvider", ctx, "aws", mock.AnythingOfType("*provider.ProviderConfig")).Return(newRecProvider(client), nil) s := &Scheduler{config: new(MockConfigStore), providerFactory: factory, stsClient: &fakeSTSClient{accountID: "111111111111"}} - recs, complete, err := s.collectAWSForAccount(ctx, nil, config.CloudAccount{ + recs, gap, err := s.collectAWSForAccount(ctx, nil, config.CloudAccount{ ID: "member", Provider: "aws", AWSAuthMode: "role_arn", AWSRoleARN: "arn:aws:iam::333333333333:role/cudly", ExternalID: "333333333333", Enabled: true, }) require.NoError(t, err) - assert.True(t, complete) + assert.Nil(t, gap) require.Len(t, recs, 2) // 333333333333 owns the first zonal rec; the 222222222222 zonal rec is dropped. assert.ElementsMatch(t, []string{"us-east-1a", ""}, []string{recs[0].AvailabilityZone, recs[1].AvailabilityZone}) diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index 6a9398a8..bf5d1ee9 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -338,6 +338,9 @@ func (m *mockConfigStoreForHealth) GetRecommendationsFreshness(_ context.Context func (m *mockConfigStoreForHealth) SetRecommendationsCollectionError(_ context.Context, _ string) error { return nil } +func (m *mockConfigStoreForHealth) SetRecommendationsCollectionPartial(_ context.Context, _ *config.RecommendationsCollectionPartial) error { + return nil +} func (m *mockConfigStoreForHealth) MarkCollectionStarted(_ context.Context) (string, bool, error) { return "mock-owner-token", true, nil }