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.