diff --git a/internal/api/handler_plans_sp_multistep_test.go b/internal/api/handler_plans_sp_multistep_test.go new file mode 100644 index 000000000..b8d816614 --- /dev/null +++ b/internal/api/handler_plans_sp_multistep_test.go @@ -0,0 +1,95 @@ +package api + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Savings Plans are scoped out of multi-step ramps (platform#631 Q7): create, +// PUT and PATCH (arm) refuse an armed multi-step plan with an SP service with +// a 400, and leave single-step SP and multi-step non-SP plans alone. + +const spPlanID = "11111111-1111-1111-1111-111111111111" + +func spHandler(t *testing.T) (*Handler, *MockConfigStore) { + t.Helper() + ctx := context.Background() + store := new(MockConfigStore) + auth := new(MockAuthService) + auth.On("ValidateSession", ctx, "admin-token").Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil) + auth.grantAdmin() + return &Handler{config: store, auth: auth}, store +} + +func spReq(body string) *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-token"}, Body: body} +} + +func requireSP400(t *testing.T, err error) { + t.Helper() + ce, ok := IsClientError(err) + require.True(t, ok, "want ClientError, got %T: %v", err, err) + assert.Equal(t, 400, ce.code) + assert.Contains(t, ce.Error(), "multi-step ramps do not support Savings Plans") +} + +func TestCreatePlan_RejectsArmedMultiStepSavingsPlan(t *testing.T) { + h, _ := spHandler(t) + body := `{"name":"P","provider":"aws","service":"savingsplans","ramp_schedule":"weekly-25pct","enabled":true,"auto_purchase":true,"target_accounts":["bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"]}` + _, err := h.createPlan(context.Background(), spReq(body)) + requireSP400(t, err) +} + +func TestMergeUpdatedPlan_RejectsArmedMultiStepSavingsPlan(t *testing.T) { + existing := &config.PurchasePlan{Name: "p", RampSchedule: config.RampSchedule{Type: "immediate", PercentPerStep: 100, TotalSteps: 1}} + _, err := mergeUpdatedPlan(existing, &PlanRequest{Name: "p", Provider: "aws", Service: "savingsplans", + RampSchedule: "weekly-25pct", Enabled: true, AutoPurchase: true}, "id") + requireSP400(t, err) +} + +func patchSP(t *testing.T, existing *config.PurchasePlan, body string) (any, error) { + t.Helper() + ctx := context.Background() + h, store := spHandler(t) + store.On("GetPurchasePlan", ctx, spPlanID).Return(existing, nil) + store.On("UpdatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(nil) + return h.patchPlan(ctx, spReq(body), spPlanID) +} + +func spPlan(service string, ramp config.RampSchedule) *config.PurchasePlan { + return &config.PurchasePlan{ID: spPlanID, Name: "p", NotificationDaysBefore: 3, RampSchedule: ramp, + Services: map[string]config.ServiceConfig{"aws/" + service: {Provider: "aws", Service: service, Enabled: true, Term: 3, Payment: "no-upfront", Coverage: 80}}} +} + +func TestPatchPlan_ArmingMultiStepSavingsPlanIs400(t *testing.T) { + weekly := config.RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, TotalSteps: 4} + _, err := patchSP(t, spPlan("savingsplans", weekly), `{"enabled":true,"auto_purchase":true}`) + requireSP400(t, err) +} + +func TestPatchPlan_SingleStepSavingsPlanAndMultiStepNonSPArmFine(t *testing.T) { + immediate := config.RampSchedule{Type: "immediate", PercentPerStep: 100, TotalSteps: 1} + weekly := config.RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, TotalSteps: 4} + for name, plan := range map[string]*config.PurchasePlan{ + "single-step SP": spPlan("savingsplans", immediate), + "multi-step RDS": spPlan("rds", weekly), + "single custom SP (100%)": spPlan("savingsplans", config.RampSchedule{Type: "custom", PercentPerStep: 100, StepIntervalDays: 7, TotalSteps: 1}), + } { + t.Run(name, func(t *testing.T) { + _, err := patchSP(t, plan, `{"enabled":true,"auto_purchase":true}`) + require.NoError(t, err) + }) + } +} + +func TestPatchPlan_UnarmedMultiStepSavingsPlanIsAllowed(t *testing.T) { + weekly := config.RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, TotalSteps: 4} + _, err := patchSP(t, spPlan("savingsplans", weekly), `{"name":"renamed"}`) + require.NoError(t, err) +} diff --git a/internal/config/validation.go b/internal/config/validation.go index 8080dfbcd..96af8a796 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -3,6 +3,7 @@ package config import ( "encoding/json" + "errors" "fmt" "math" "net/mail" @@ -10,6 +11,7 @@ import ( "strconv" "strings" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/ladder" ) @@ -597,6 +599,29 @@ func (p *PurchasePlan) Validate() error { } } + return p.CheckNoMultiStepSavingsPlans() +} + +// ErrMultiStepSavingsPlans marks an armed multi-step plan with an enabled +// Savings Plans service. Step 1 suppresses the whole SP recommendation for 7 +// days, so later steps find nothing and the ramp strands (platform#631, Q7). +// Read-only query for already-armed hits: runbooks/armed-multi-step-savings-plans-plans.md. +var ErrMultiStepSavingsPlans = errors.New("multi-step ramps do not support Savings Plans: use a single-step (immediate) ramp or remove the Savings Plans service") + +// CheckNoMultiStepSavingsPlans refuses an armed (enabled and auto-purchase) +// plan that has more than one ramp step and an enabled Savings Plans service. +// It is the one definition, shared by plan validation (create, update, arm) and +// the step precheck at attach. Single-step plans are unaffected. +func (p *PurchasePlan) CheckNoMultiStepSavingsPlans() error { + if !p.Enabled || !p.AutoPurchase || p.RampSchedule.TotalSteps <= 1 { + return nil + } + for key := range p.Services { + svc := p.Services[key] + if svc.Enabled && common.IsSavingsPlan(common.ServiceType(svc.Service)) { + return fmt.Errorf("%w (service %q, total_steps %d)", ErrMultiStepSavingsPlans, key, p.RampSchedule.TotalSteps) + } + } return nil } diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index 96b3d39f1..2b9301b13 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -1109,3 +1109,39 @@ func TestLadderCapMicros(t *testing.T) { assert.Error(t, err, in) } } + +func TestPurchasePlan_CheckNoMultiStepSavingsPlans(t *testing.T) { + svc := func(name string, enabled bool) map[string]ServiceConfig { + return map[string]ServiceConfig{"aws/" + name: {Provider: "aws", Service: name, Enabled: enabled}} + } + cases := []struct { + name string + enabled bool + auto bool + steps int + svcs map[string]ServiceConfig + wantErr bool + }{ + {"armed 4-step SP", true, true, 4, svc("savingsplans", true), true}, + {"armed 2-step SP (boundary)", true, true, 2, svc("savingsplans", true), true}, + {"armed 1-step SP", true, true, 1, svc("savingsplans", true), false}, + {"armed 0-step SP", true, true, 0, svc("savingsplans", true), false}, + {"armed 4-step compute SP", true, true, 4, svc("savings-plans-compute", true), true}, + {"armed 4-step disabled SP service", true, true, 4, svc("savingsplans", false), false}, + {"armed 4-step non-SP", true, true, 4, svc("rds", true), false}, + {"not auto-purchase", true, false, 4, svc("savingsplans", true), false}, + {"disabled", false, true, 4, svc("savingsplans", true), false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + p := &PurchasePlan{Enabled: tc.enabled, AutoPurchase: tc.auto, Services: tc.svcs, + RampSchedule: RampSchedule{TotalSteps: tc.steps}} + err := p.CheckNoMultiStepSavingsPlans() + if tc.wantErr { + require.ErrorIs(t, err, ErrMultiStepSavingsPlans) + return + } + require.NoError(t, err) + }) + } +} diff --git a/internal/planstep/precheck.go b/internal/planstep/precheck.go index 1926d0519..72d5dd846 100644 --- a/internal/planstep/precheck.go +++ b/internal/planstep/precheck.go @@ -17,7 +17,8 @@ var ErrUnsupportedPlan = errors.New("plan settings are not supported by recommen // Precheck refuses, with a plan-specific error, the plan settings Resolve does // not apply: any Include*/Exclude*/MinCount filter on a service config (the // plan-step request never sets them, so honoring them would buy something other -// than what the plan says), and an immediate ramp whose PercentPerStep is not +// than what the plan says), an armed multi-step plan with a Savings Plans +// service (platform#631 Q7), and an immediate ramp whose PercentPerStep is not // 100 (an immediate ramp ignores that field when validated but Resolve sizes // with it). func Precheck(plan *config.PurchasePlan) error { @@ -36,6 +37,9 @@ func Precheck(plan *config.PurchasePlan) error { return fmt.Errorf("%w: plan %s has an immediate ramp with percent_per_step %v (must be 100)", ErrUnsupportedPlan, plan.ID, plan.RampSchedule.PercentPerStep) } + if err := plan.CheckNoMultiStepSavingsPlans(); err != nil { + return fmt.Errorf("%w: plan %s: %w", ErrUnsupportedPlan, plan.ID, err) + } return nil } diff --git a/internal/planstep/precheck_test.go b/internal/planstep/precheck_test.go index 935a60b91..2138dd337 100644 --- a/internal/planstep/precheck_test.go +++ b/internal/planstep/precheck_test.go @@ -17,6 +17,9 @@ func planWith(svc config.ServiceConfig, ramp config.RampSchedule) *config.Purcha func TestPrecheck(t *testing.T) { ok := config.ServiceConfig{Provider: "aws", Service: "ec2", Enabled: true} weekly := config.RampSchedule{Type: "weekly", PercentPerStep: 25} + sp := config.ServiceConfig{Provider: "aws", Service: "savingsplans", Enabled: true} + weekly4 := config.RampSchedule{Type: "weekly", PercentPerStep: 25, TotalSteps: 4} + armed := func(p *config.PurchasePlan) *config.PurchasePlan { p.Enabled, p.AutoPurchase = true, true; return p } cases := []struct { name string plan *config.PurchasePlan @@ -35,6 +38,9 @@ func TestPrecheck(t *testing.T) { {"exclude types", planWith(withSvc(ok, func(s *config.ServiceConfig) { s.ExcludeTypes = []string{"m5.large"} }), weekly), "exclude_types"}, {"min count", planWith(withSvc(ok, func(s *config.ServiceConfig) { s.MinCount = 1 }), weekly), "min_count"}, {"negative min count", planWith(withSvc(ok, func(s *config.ServiceConfig) { s.MinCount = -1 }), weekly), "min_count"}, + {"armed multi-step savings plan", armed(planWith(sp, weekly4)), "multi-step ramps do not support Savings Plans"}, + {"armed single-step savings plan", armed(planWith(sp, config.RampSchedule{Type: "immediate", PercentPerStep: 100, TotalSteps: 1})), ""}, + {"armed multi-step non-SP", armed(planWith(ok, weekly4)), ""}, {"filter on a disabled service still refuses", planWith(withSvc(ok, func(s *config.ServiceConfig) { s.Enabled = false; s.MinCount = 2 }), weekly), "min_count"}, } for _, tc := range cases { diff --git a/internal/purchase/plan_step_attach.go b/internal/purchase/plan_step_attach.go index f6a0a5ba7..5d289c4b4 100644 --- a/internal/purchase/plan_step_attach.go +++ b/internal/purchase/plan_step_attach.go @@ -153,6 +153,9 @@ func (m *Manager) attachStep(ctx context.Context, tx pgx.Tx, locked *config.Purc if errors.Is(err, planstep.ErrNoMatch) { return m.noMatch(ctx, tx, locked, claim, root) } + if errors.Is(err, planstep.ErrUnsupportedPlan) { + return m.failStep(ctx, tx, locked, claim, root, err.Error()) + } if err != nil { return stepClaim{}, fmt.Errorf("plan %s step %d: %w", locked.ID, claim.number, err) } diff --git a/internal/purchase/plan_step_attach_test.go b/internal/purchase/plan_step_attach_test.go index 390cf1276..1f6b7f593 100644 --- a/internal/purchase/plan_step_attach_test.go +++ b/internal/purchase/plan_step_attach_test.go @@ -237,3 +237,25 @@ func TestPlanUnchanged_ComparesDatesAtMicrosecondPrecision(t *testing.T) { locked.NextExecutionDate = &moved assert.False(t, planUnchanged(listed, locked, providers), "a microsecond is a change") } + +// An already-armed multi-step Savings Plans plan (platform#631 Q7) must produce +// one visible failed row with the reason, not an error on every tick. +func TestAttach_UnsupportedPlanFailsTheStepVisibly(t *testing.T) { + listed := attachPlan(time.Now().Add(48*time.Hour), 1e6) + locked := attachPlan(*listed.NextExecutionDate, 1e6) + for _, p := range []*config.PurchasePlan{listed, locked} { + p.Services = map[string]config.ServiceConfig{"aws/savingsplans": {Enabled: true, Provider: "aws", Service: "savingsplans"}} + p.RampSchedule = config.RampSchedule{Type: "weekly", PercentPerStep: 25, TotalSteps: 4} + } + res := &fakeResolver{err: planstep.Precheck(locked)} + require.ErrorIs(t, res.err, planstep.ErrUnsupportedPlan) + m, store := attachManager(t, listed, locked, res) + store.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil) + + _, _, _, err := m.getOrCreateExecution(context.Background(), listed) + require.ErrorIs(t, err, errExecutionNotNotifiable) + saved := savedRows(store) + require.Len(t, saved, 1) + assert.Equal(t, "failed", saved[0].Status) + assert.Contains(t, saved[0].Error, "multi-step ramps do not support Savings Plans") +} diff --git a/runbooks/armed-multi-step-savings-plans-plans.md b/runbooks/armed-multi-step-savings-plans-plans.md new file mode 100644 index 000000000..32b4637a0 --- /dev/null +++ b/runbooks/armed-multi-step-savings-plans-plans.md @@ -0,0 +1,29 @@ +# Runbook: armed multi-step plans that include Savings Plans + +Multi-step ramps do not support Savings Plans (platform#631). Step 1 suppresses +the whole SP recommendation for 7 days, so later steps find nothing and the ramp +strands. Plan create, update and arm now return HTTP 400 for an armed plan +(enabled and auto_purchase) with `total_steps > 1` and an enabled Savings Plans +service, and an already-armed one fails its next step with a visible failed row. + +## Before deploying this change (read-only, run on DEV and prod) + +Lists the armed plans that would start failing. Decide each hit first: re-create +it as a single-step (immediate) plan, remove the SP service, or disarm it. + +```sql +SELECT p.id, p.name, p.ramp_schedule->>'type' AS ramp_type, + (p.ramp_schedule->>'total_steps')::int AS total_steps, + (p.ramp_schedule->>'current_step')::int AS current_step, + p.next_execution_date, + array_agg(s.key) AS sp_services +FROM purchase_plans p +CROSS JOIN LATERAL jsonb_each(p.services) AS s(key, val) +WHERE p.enabled AND p.auto_purchase + AND COALESCE((p.ramp_schedule->>'total_steps')::int, 0) > 1 + AND COALESCE((s.val->>'enabled')::boolean, false) + AND (s.val->>'service' = 'savingsplans' OR s.val->>'service' LIKE 'savings-plans-%') +GROUP BY p.id, p.name, p.ramp_schedule, p.next_execution_date; +``` + +Zero rows means nothing changes for existing plans.