From 1482df932119ff70936ac16bee8d0fa31dd7183e Mon Sep 17 00:00:00 2001 From: Fraser Waters Date: Mon, 10 Oct 2022 11:18:57 +0100 Subject: [PATCH] Fix update plans with dependent replacements We weren't correctly handling the case where a resource was marked for deletion due to one of it's dependencies being deleted. We would add an entry to it's "Ops" list, but then overwrite that "Ops" list when we came to generate the recreation step. Fixes https://github.com/pulumi/pulumi/issues/10924 --- .../20221013--engine--update-plan-dbr.yaml | 4 + pkg/engine/lifecycletest/update_plan_test.go | 96 +++++++++++++++++++ pkg/resource/deploy/deployment.go | 4 + pkg/resource/deploy/step_generator.go | 28 ++++-- 4 files changed, 126 insertions(+), 6 deletions(-) create mode 100644 changelog/pending/20221013--engine--update-plan-dbr.yaml diff --git a/changelog/pending/20221013--engine--update-plan-dbr.yaml b/changelog/pending/20221013--engine--update-plan-dbr.yaml new file mode 100644 index 000000000000..6ee5093932e5 --- /dev/null +++ b/changelog/pending/20221013--engine--update-plan-dbr.yaml @@ -0,0 +1,4 @@ +changes: +- type: fix + scope: engine + description: Fix a bug in update plans handling resources being replaced due to other resources being deleted before replacement. diff --git a/pkg/engine/lifecycletest/update_plan_test.go b/pkg/engine/lifecycletest/update_plan_test.go index 875bd52b2a9a..803f758418f5 100644 --- a/pkg/engine/lifecycletest/update_plan_test.go +++ b/pkg/engine/lifecycletest/update_plan_test.go @@ -17,6 +17,7 @@ package lifecycletest import ( "regexp" + "strings" "testing" "github.com/blang/semver" @@ -1507,3 +1508,98 @@ func TestProviderDeterministicPreview(t *testing.T) { assert.NotEqual(t, expectedName, snap.Resources[1].Inputs["name"]) assert.NotEqual(t, expectedName, snap.Resources[1].Outputs["name"]) } + +func TestPlannedUpdateWithDependentDelete(t *testing.T) { + t.Parallel() + + var diffResult *plugin.DiffResult + + loaders := []*deploytest.ProviderLoader{ + deploytest.NewProviderLoader("pkgA", semver.MustParse("1.0.0"), func() (plugin.Provider, error) { + return &deploytest.Provider{ + CreateF: func(urn resource.URN, news resource.PropertyMap, timeout float64, + preview bool) (resource.ID, resource.PropertyMap, resource.Status, error) { + return resource.ID("created-id-" + urn.Name()), news, resource.StatusOK, nil + }, + UpdateF: func(urn resource.URN, id resource.ID, olds, news resource.PropertyMap, timeout float64, + ignoreChanges []string, preview bool) (resource.PropertyMap, resource.Status, error) { + return news, resource.StatusOK, nil + }, + CheckF: func(urn resource.URN, + olds, news resource.PropertyMap, _ []byte) (resource.PropertyMap, []plugin.CheckFailure, error) { + return news, nil, nil + }, + DiffF: func(urn resource.URN, + id resource.ID, olds, news resource.PropertyMap, ignoreChanges []string) (plugin.DiffResult, error) { + if strings.Contains(string(urn), "resA") || strings.Contains(string(urn), "resB") { + assert.NotNil(t, diffResult, "Diff was called but diffResult wasn't set") + return *diffResult, nil + } + return plugin.DiffResult{}, nil + }, + }, nil + }), + } + + var ins resource.PropertyMap + program := deploytest.NewLanguageRuntime(func(_ plugin.RunInfo, monitor *deploytest.ResourceMonitor) error { + resA, _, outs, err := monitor.RegisterResource("pkgA:m:typA", "resA", true, deploytest.ResourceOptions{ + Inputs: ins, + }) + assert.NoError(t, err) + + _, _, _, err = monitor.RegisterResource("pkgA:m:typB", "resB", true, deploytest.ResourceOptions{ + Inputs: outs, + Dependencies: []resource.URN{resA}, + }) + assert.NoError(t, err) + + return nil + }) + host := deploytest.NewPluginHost(nil, nil, program, loaders...) + + p := &TestPlan{ + Options: UpdateOptions{Host: host, GeneratePlan: true}, + } + + project := p.GetProject() + + // Create an initial ResA and resB + ins = resource.NewPropertyMapFromMap(map[string]interface{}{ + "foo": "bar", + "zed": "baz", + }) + snap, res := TestOp(Update).Run(project, p.GetTarget(t, nil), p.Options, false, p.BackendClient, nil) + assert.NotNil(t, snap) + assert.Nil(t, res) + + // Update the input and mark it as a replace, check that both A and B are marked as replacements + ins = resource.NewPropertyMapFromMap(map[string]interface{}{ + "foo": "frob", + "zed": "baz", + }) + diffResult = &plugin.DiffResult{ + Changes: plugin.DiffSome, + ReplaceKeys: []resource.PropertyKey{"foo"}, + StableKeys: []resource.PropertyKey{"zed"}, + DetailedDiff: map[string]plugin.PropertyDiff{ + "foo": { + Kind: plugin.DiffUpdateReplace, + InputDiff: true, + }, + }, + DeleteBeforeReplace: true, + } + plan, res := TestOp(Update).Plan(project, p.GetTarget(t, snap), p.Options, p.BackendClient, nil) + assert.NotNil(t, plan) + assert.Nil(t, res) + + assert.Equal(t, 3, len(plan.ResourcePlans["urn:pulumi:test::test::pkgA:m:typA::resA"].Ops)) + assert.Equal(t, 3, len(plan.ResourcePlans["urn:pulumi:test::test::pkgA:m:typB::resB"].Ops)) + + // Now try and run with the plan + p.Options.Plan = plan.Clone() + snap, res = TestOp(Update).Run(project, p.GetTarget(t, snap), p.Options, false, p.BackendClient, nil) + assert.NotNil(t, snap) + assert.Nil(t, res) +} diff --git a/pkg/resource/deploy/deployment.go b/pkg/resource/deploy/deployment.go index 3129066f098d..165c19fa5666 100644 --- a/pkg/resource/deploy/deployment.go +++ b/pkg/resource/deploy/deployment.go @@ -151,6 +151,10 @@ func (m *resourcePlans) set(urn resource.URN, plan *ResourcePlan) { m.m.Lock() defer m.m.Unlock() + if _, ok := m.plans.ResourcePlans[urn]; ok { + panic(fmt.Sprintf("tried to set resource plan for %s but it's already been set", urn)) + } + m.plans.ResourcePlans[urn] = plan } diff --git a/pkg/resource/deploy/step_generator.go b/pkg/resource/deploy/step_generator.go index 746d071dc9f8..8022ed3e7d8f 100644 --- a/pkg/resource/deploy/step_generator.go +++ b/pkg/resource/deploy/step_generator.go @@ -253,16 +253,21 @@ func (sg *stepGenerator) GenerateSteps(event RegisterResourceEvent) ([]Step, res // Check each proposed step against the relevant resource plan, if any for _, s := range steps { + logging.V(5).Infof("Checking step %s for %s", s.Op(), s.URN()) + if sg.deployment.plan != nil { if resourcePlan, ok := sg.deployment.plan.ResourcePlans[s.URN()]; ok { if len(resourcePlan.Ops) == 0 { return nil, result.Errorf("%v is not allowed by the plan: no more steps were expected for this resource", s.Op()) } constraint := resourcePlan.Ops[0] + // We remove the Op from the list before doing the constraint check. + // This is because we look at Ops at the end to see if any expected operations didn't attempt to happen. + // This op has been attempted, it just might fail its constraint. + resourcePlan.Ops = resourcePlan.Ops[1:] if !ConstrainedTo(s.Op(), constraint) { return nil, result.Errorf("%v is not allowed by the plan: this resource is constrained to %v", s.Op(), constraint) } - resourcePlan.Ops = resourcePlan.Ops[1:] } else { if !ConstrainedTo(s.Op(), OpSame) { return nil, result.Errorf("%v is not allowed by the plan: no steps were expected for this resource", s.Op()) @@ -601,11 +606,22 @@ func (sg *stepGenerator) generateSteps(event RegisterResourceEvent) ([]Step, res } inputDiff := oldInputs.Diff(inputs) - // Generate the output goal plan - newResourcePlan := &ResourcePlan{ - Seed: randomSeed, - Goal: NewGoalPlan(inputDiff, goal)} - sg.deployment.newPlans.set(urn, newResourcePlan) + // Generate the output goal plan, if we're recreating this it should already exist + if recreating { + plan, ok := sg.deployment.newPlans.get(urn) + if !ok { + return nil, result.FromError(fmt.Errorf("no plan for resource %v", urn)) + } + // The plan will have had it's Ops already partially filled in for the delete operation, but we + // now have the information needed to fill in Seed and Goal. + plan.Seed = randomSeed + plan.Goal = NewGoalPlan(inputDiff, goal) + } else { + newResourcePlan := &ResourcePlan{ + Seed: randomSeed, + Goal: NewGoalPlan(inputDiff, goal)} + sg.deployment.newPlans.set(urn, newResourcePlan) + } } // If there is a plan for this resource, validate that the program goal conforms to the plan.