diff --git a/internal/api/handler_purchases_retry_fanout_test.go b/internal/api/handler_purchases_retry_fanout_test.go index 5449c500..2daa6c66 100644 --- a/internal/api/handler_purchases_retry_fanout_test.go +++ b/internal/api/handler_purchases_retry_fanout_test.go @@ -63,6 +63,18 @@ func fanoutRecs() []config.RecommendationRecord { } } +// fanoutRootRecs is a root step's rec set: one rec per plan account, because +// the fan-out buys each account's own recs only (platform#631). +func fanoutRootRecs() []config.RecommendationRecord { + recs := make([]config.RecommendationRecord, 0, 3) + for _, id := range []string{fanoutAcctA, fanoutAcctB, fanoutAcctC} { + rec := fanoutRecs()[0] + rec.CloudAccountID = strPtr(id) + recs = append(recs, rec) + } + return recs +} + // fanoutCredStore resolves a static AWS access-key blob so per-account // credential resolution succeeds without touching the cloud. type fanoutCredStore struct { @@ -329,7 +341,7 @@ func TestRetryOfFailedRootRowStillFansOutToEveryAccount(t *testing.T) { CreatedByUserID: &creator, CapacityPercent: 100, Source: common.PurchaseSourceWeb, - Recommendations: fanoutRecs(), + Recommendations: fanoutRootRecs(), } successor := retryFailedRow(t, failedRoot) diff --git a/internal/purchase/account_scope.go b/internal/purchase/account_scope.go new file mode 100644 index 00000000..d2bcbe65 --- /dev/null +++ b/internal/purchase/account_scope.go @@ -0,0 +1,63 @@ +package purchase + +import ( + "fmt" + "strings" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// scopeExecutionsByAccount splits a plan step's recommendations across the +// plan's accounts so each account buys only its own (platform#631). The +// returned executions are per-account copies of base whose Recommendations +// and money totals cover only that account's recs; accounts with no recs of +// their own are omitted. +// +// It fails loudly rather than dropping anything: a rec with no account, or an +// account outside the plan, would otherwise be silently unbought (or, before +// this scoping existed, bought by every account). An empty step returns +// ErrPlanStepNoRecommendations so it cannot complete and advance the ramp. +func scopeExecutionsByAccount(base *config.PurchaseExecution, accounts []config.CloudAccount) (map[string]*config.PurchaseExecution, error) { + if len(base.Recommendations) == 0 { + return nil, fmt.Errorf("execution %s: %w", base.ExecutionID, ErrPlanStepNoRecommendations) + } + planAccounts := make(map[string]bool, len(accounts)) + for i := range accounts { + planAccounts[accounts[i].ID] = true + } + byAccount := make(map[string][]config.RecommendationRecord, len(accounts)) + var stray []string + for i := range base.Recommendations { + rec := base.Recommendations[i] + if rec.CloudAccountID == nil || !planAccounts[*rec.CloudAccountID] { + stray = append(stray, describeScopeRec(&rec)) + continue + } + byAccount[*rec.CloudAccountID] = append(byAccount[*rec.CloudAccountID], rec) + } + if len(stray) > 0 { + return nil, fmt.Errorf("execution %s: %d recommendation(s) are not attributed to one of the plan's accounts and cannot be bought: %s", + base.ExecutionID, len(stray), strings.Join(stray, ", ")) + } + + scoped := make(map[string]*config.PurchaseExecution, len(byAccount)) + for id, recs := range byAccount { + e := *base + e.Recommendations = recs + e.TotalUpfrontCost, e.EstimatedSavings = 0, 0 + for i := range recs { + e.TotalUpfrontCost += recs[i].UpfrontCost + e.EstimatedSavings += recs[i].Savings + } + scoped[id] = &e + } + return scoped, nil +} + +func describeScopeRec(rec *config.RecommendationRecord) string { + acct := "no account" + if rec.CloudAccountID != nil { + acct = "account " + *rec.CloudAccountID + } + return fmt.Sprintf("%s/%s %s (%s)", rec.Provider, rec.Service, rec.ResourceType, acct) +} diff --git a/internal/purchase/account_scope_test.go b/internal/purchase/account_scope_test.go new file mode 100644 index 00000000..b719695b --- /dev/null +++ b/internal/purchase/account_scope_test.go @@ -0,0 +1,79 @@ +package purchase + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func withAccount(rec config.RecommendationRecord, id string) config.RecommendationRecord { + rec.CloudAccountID = &id + return rec +} + +func scopedTestRec(accountID string) config.RecommendationRecord { + return withAccount(config.RecommendationRecord{ + Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", + Count: 1, UpfrontCost: 300, Savings: 75, Selected: true, + }, accountID) +} + +func scopeFixture() (*config.PurchaseExecution, []config.CloudAccount) { + a, b := scopedTestRec("acct-A"), scopedTestRec("acct-B") + b.ResourceType, b.UpfrontCost, b.Savings = "c5.large", 100, 20 + return &config.PurchaseExecution{ + ExecutionID: "root", PlanID: "plan-x", StepNumber: 1, + TotalUpfrontCost: 400, EstimatedSavings: 95, + Recommendations: []config.RecommendationRecord{a, b}, + }, []config.CloudAccount{{ID: "acct-A"}, {ID: "acct-B"}} +} + +// Each account gets only its own recs, and its row carries its own totals +// (not the step's, which would show N times the money in History). +func TestScopeExecutionsByAccount_OwnRecsAndTotals(t *testing.T) { + base, accounts := scopeFixture() + scoped, err := scopeExecutionsByAccount(base, accounts) + require.NoError(t, err) + require.Len(t, scoped, 2) + assert.Len(t, scoped["acct-A"].Recommendations, 1) + assert.Equal(t, "m5.large", scoped["acct-A"].Recommendations[0].ResourceType) + assert.Equal(t, 300.0, scoped["acct-A"].TotalUpfrontCost) + assert.Equal(t, 75.0, scoped["acct-A"].EstimatedSavings) + assert.Equal(t, 100.0, scoped["acct-B"].TotalUpfrontCost) + assert.Equal(t, 20.0, scoped["acct-B"].EstimatedSavings) + assert.Len(t, base.Recommendations, 2, "base execution must be untouched") +} + +// A rec with no account, or one outside the plan, fails the step loudly. +func TestScopeExecutionsByAccount_StrayRecFailsLoudly(t *testing.T) { + for name, stray := range map[string]config.RecommendationRecord{ + "nil account": {Provider: "azure", Service: "compute", ResourceType: "D2"}, + "foreign account": scopedTestRec("acct-other"), + } { + t.Run(name, func(t *testing.T) { + base, accounts := scopeFixture() + base.Recommendations = append(base.Recommendations, stray) + scoped, err := scopeExecutionsByAccount(base, accounts) + require.Error(t, err) + assert.Nil(t, scoped) + assert.Contains(t, err.Error(), "not attributed to one of the plan's accounts") + }) + } +} + +// executeMultiAccount: an empty step must fail with ErrPlanStepNoRecommendations +// (nil would be recorded as completed and advance the ramp), buying nothing. +func TestExecuteMultiAccount_EmptyStepFailsAndBuysNothing(t *testing.T) { + base, accounts := scopeFixture() + base.Recommendations = nil + store := new(MockConfigStore) + m := &Manager{config: store} + + err := m.executeMultiAccount(context.Background(), base, &config.PurchasePlan{ID: "plan-x"}, accounts) + + require.ErrorIs(t, err, ErrPlanStepNoRecommendations) + store.AssertNotCalled(t, "SavePurchaseExecution") +} diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 7bbb2d3f..a3ac8001 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -191,8 +191,21 @@ var errAllAccountsFailed = errors.New("multi-account execution: all accounts fai // callers ACK rather than redeliver, issue #1014); and errAllAccountsFailed // (wrapping the per-account errors) when no account committed anything. func (m *Manager) executeMultiAccount(ctx context.Context, baseExec *config.PurchaseExecution, plan *config.PurchasePlan, accounts []config.CloudAccount) error { - results := execution.RunForAccountsWithConcurrency(ctx, accounts, func(ctx context.Context, account config.CloudAccount) (bool, error) { - return m.executeForAccount(ctx, baseExec, plan, account) + // Checked before any account runs: every account would otherwise buy the + // whole step, and a step with nothing to buy would return nil below and + // be recorded as completed, advancing the ramp (platform#631). + scoped, scopeErr := scopeExecutionsByAccount(baseExec, accounts) + if scopeErr != nil { + return scopeErr + } + buying := make([]config.CloudAccount, 0, len(scoped)) + for i := range accounts { + if _, ok := scoped[accounts[i].ID]; ok { + buying = append(buying, accounts[i]) + } + } + results := execution.RunForAccountsWithConcurrency(ctx, buying, func(ctx context.Context, account config.CloudAccount) (bool, error) { + return m.executeForAccount(ctx, scoped[account.ID], plan, account) }, getMaxAccountParallelism()) committed := 0 diff --git a/internal/purchase/execution_test.go b/internal/purchase/execution_test.go index de4cd133..7a13ee8b 100644 --- a/internal/purchase/execution_test.go +++ b/internal/purchase/execution_test.go @@ -743,7 +743,7 @@ func TestManager_ExecutePurchase_MultiAccount(t *testing.T) { ExecutionID: "exec-multi", PlanID: "plan-multi", Status: "pending", - Recommendations: []config.RecommendationRecord{rec}, + Recommendations: []config.RecommendationRecord{withAccount(rec, "aaaaaaaa-0000-0000-0000-000000000001"), withAccount(rec, "aaaaaaaa-0000-0000-0000-000000000002")}, } accounts := []config.CloudAccount{ @@ -797,6 +797,14 @@ func TestManager_ExecutePurchase_MultiAccount(t *testing.T) { "aaaaaaaa-0000-0000-0000-000000000002", }, accountIDs) + // platform#631: each account buys only its own rec (2 calls, not 4) and + // its row carries its own totals, not the whole step's. + mockServiceClient.AssertNumberOfCalls(t, "PurchaseCommitment", 2) + for _, e := range savedExecs { + require.Len(t, e.Recommendations, 1) + assert.Equal(t, *e.CloudAccountID, *e.Recommendations[0].CloudAccountID) + } + mockStore.AssertExpectations(t) mockEmail.AssertExpectations(t) mockFactory.AssertExpectations(t) @@ -871,7 +879,7 @@ func TestExecuteForAccount_CredentialFailure_MarksFailed(t *testing.T) { ExecutionID: "exec-credfail", PlanID: "plan-credfail", Status: "pending", - Recommendations: []config.RecommendationRecord{rec}, + Recommendations: []config.RecommendationRecord{withAccount(rec, tt.account.ID)}, } mockStore.On("GetPurchasePlan", ctx, "plan-credfail").Return(plan, nil) @@ -1032,7 +1040,7 @@ func TestExecuteMultiAccount_PartialFailure_IsolatesAccounts(t *testing.T) { ExecutionID: "exec-partial", PlanID: "plan-partial", Status: "pending", - Recommendations: []config.RecommendationRecord{rec}, + Recommendations: []config.RecommendationRecord{withAccount(rec, accountVID), withAccount(rec, accountIID)}, } plan := &config.PurchasePlan{ @@ -1203,7 +1211,7 @@ func TestExecuteMultiAccount_RunsAccountsInParallel(t *testing.T) { ExecutionID: "exec-parallel", PlanID: "plan-parallel", Status: "pending", - Recommendations: []config.RecommendationRecord{rec}, + Recommendations: []config.RecommendationRecord{withAccount(rec, accountAID), withAccount(rec, accountBID)}, } plan := &config.PurchasePlan{ diff --git a/internal/purchase/money_path_regression_test.go b/internal/purchase/money_path_regression_test.go index 043ebcca..6e990919 100644 --- a/internal/purchase/money_path_regression_test.go +++ b/internal/purchase/money_path_regression_test.go @@ -240,7 +240,8 @@ func TestMultiAccountSeedsStablePerAccountKey(t *testing.T) { PlanID: "plan-x", Source: common.PurchaseSourceWeb, Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300, Selected: true}, + scopedTestRec("acct-A"), + scopedTestRec("acct-B"), }, } plan := &config.PurchasePlan{ID: "plan-x", Name: "Plan X"} @@ -385,7 +386,8 @@ func TestMultiAccountPartialSuccessIsAcked(t *testing.T) { Status: "pending", PlanID: "plan-x", Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300, Selected: true}, + scopedTestRec("acct-ok"), + scopedTestRec("acct-bad"), }, } plan := &config.PurchasePlan{ID: "plan-x", Name: "Plan X", AutoPurchase: true} @@ -484,7 +486,8 @@ func TestExecuteForAccount_CommittedButUnsavedIsAcked(t *testing.T) { Status: "pending", PlanID: "plan-y", Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300, Selected: true}, + scopedTestRec("acct-bad"), + scopedTestRec("acct-unsaved"), }, } plan := &config.PurchasePlan{ID: "plan-y", Name: "Plan Y", AutoPurchase: true} @@ -786,7 +789,8 @@ func TestRootKeyStillFansOutThroughExecutePurchase(t *testing.T) { PlanID: "plan-x", Source: common.PurchaseSourceWeb, Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300, Selected: true}, + scopedTestRec("acct-A"), + scopedTestRec("acct-B"), }, } require.NoError(t, manager.executePurchase(ctx, root), diff --git a/internal/purchase/ramp_step_progress_integration_test.go b/internal/purchase/ramp_step_progress_integration_test.go index f2b4b929..0e821c91 100644 --- a/internal/purchase/ramp_step_progress_integration_test.go +++ b/internal/purchase/ramp_step_progress_integration_test.go @@ -199,6 +199,19 @@ func rampStepRecommendation() []config.RecommendationRecord { } } +// rampStepRecommendationsPerAccount is the step's rec set: one rec per plan +// account, since the fan-out buys each account's own recs only (platform#631). +func (f *rampStepFixture) rampStepRecommendationsPerAccount() []config.RecommendationRecord { + recs := make([]config.RecommendationRecord, 0, len(f.accounts)) + for i := range f.accounts { + id := f.accounts[i].ID + rec := rampStepRecommendation()[0] + rec.CloudAccountID = &id + recs = append(recs, rec) + } + return recs +} + // saveRootExecution persists the plan-scheduled root row for ramp step // stepNumber: no cloud_account_id, so the executor fans it out. func (f *rampStepFixture) saveRootExecution(ctx context.Context, t *testing.T, stepNumber int) *config.PurchaseExecution { @@ -210,7 +223,7 @@ func (f *rampStepFixture) saveRootExecution(ctx context.Context, t *testing.T, s Status: "pending", StepNumber: stepNumber, ScheduledDate: time.Now(), - Recommendations: rampStepRecommendation(), + Recommendations: f.rampStepRecommendationsPerAccount(), } require.NoError(t, f.store.SavePurchaseExecution(ctx, exec)) return exec