Fix issue with --target deletion dependent calculation (#8360)

* Fix issue with --target deletion dependant calculation

The code that computed --target deletion dependants was not correct.
It used parent/child component relationships, but did not respect actual
DAG dependencies. As a result, it could erroneously leave hanging
references to resources that no longer exist after performing a
`pulumi destroy --target X` operation. This manifested in bugs like
https://github.com/pulumi/pulumi/issues/6283, which is fixed by this
change. The solution is to compute the (transitive!) dependency graph
correctly, factoring in both parent/child, as well as explicit and
implicit, dependencies. The existing logic does the correct thing once
we do this. I've also added tests for this area, including regression
tests that cover transitive dependency relationships, as well as ones
that would cause an infinite loop given a naive implementation.

* Add a changelog entry

* Fix failing test to include all destroyed targets

Unless I'm missing something, the entire tree should be deleted
in this test case because A is the ancestor for the entire tree.

* Use DependencyGraph to compute dependents

Per code review feedback from @pgavlin.

Co-authored-by: Anton Tayanovskyy <anton@pulumi.com>

CHANGELOG_PENDING.md

@@ -4,3 +4,6 @@
   [#8338](https://github.com/pulumi/pulumi/pull/8338)
 
 ### Bug Fixes
+
+- [engine] - Compute dependents correctly during targeted deletes.
+  [#8360](https://github.com/pulumi/pulumi/pull/8360)

pkg/engine/lifeycletest/target_test.go

@@ -33,15 +33,21 @@ func TestDestroyTarget(t *testing.T) {
 	destroySpecificTargets(
 		t, []string{"A"}, true, /*targetDependents*/
 		func(urns []resource.URN, deleted map[resource.URN]bool) {
-			// when deleting 'A' we expect A, B, C, E, F, and K to be deleted
+			// when deleting 'A' we expect A, B, C, D, E, F, G, H, I, J, K, and L to be deleted
 			names := complexTestDependencyGraphNames
 			assert.Equal(t, map[resource.URN]bool{
 				pickURN(t, urns, names, "A"): true,
 				pickURN(t, urns, names, "B"): true,
 				pickURN(t, urns, names, "C"): true,
+				pickURN(t, urns, names, "D"): true,
 				pickURN(t, urns, names, "E"): true,
 				pickURN(t, urns, names, "F"): true,
+				pickURN(t, urns, names, "G"): true,
+				pickURN(t, urns, names, "H"): true,
+				pickURN(t, urns, names, "I"): true,
+				pickURN(t, urns, names, "J"): true,
 				pickURN(t, urns, names, "K"): true,
+				pickURN(t, urns, names, "L"): true,
 			}, deleted)
 		})
 

pkg/resource/deploy/step_generator.go

@@ -772,23 +772,44 @@ func (sg *stepGenerator) GenerateDeletes(targetsOpt map[resource.URN]bool) ([]St
 	return dels, nil
 }
 
-func (sg *stepGenerator) getTargetIncludingChildren(target resource.URN) map[resource.URN]bool {
-	allTargets := make(map[resource.URN]bool)
-	allTargets[target] = true
-
-	// The list of resources is a topological sort of the reverse-dependency graph, so any
-	// resource R will appear in a list before its dependents and its children. We can use this
-	// to our advantage here and find all of a resource's children via a linear scan of the list
-	// starting from R.
+// getTargetDependents returns the (transitive) set of dependents on the target resources.
+// This includes both implicit and explicit dependents in the DAG itself, as well as children.
+func (sg *stepGenerator) getTargetDependents(targetsOpt map[resource.URN]bool) map[resource.URN]bool {
+	// Seed the list with the initial set of targets.
+	var frontier []*resource.State
 	for _, res := range sg.deployment.prev.Resources {
-		if _, has := allTargets[res.Parent]; has {
-			allTargets[res.URN] = true
+		if _, has := targetsOpt[res.URN]; has {
+			frontier = append(frontier, res)
 		}
 	}
 
-	return allTargets
+	// Produce a dependency graph of resources.
+	dg := graph.NewDependencyGraph(sg.deployment.prev.Resources)
+
+	// Now accumulate a list of targets that are implicated because they depend upon the targets.
+	targets := make(map[resource.URN]bool)
+	for len(frontier) > 0 {
+		// Pop the next to explore, mark it, and skip any we've already seen.
+		next := frontier[0]
+		frontier = frontier[1:]
+		if _, has := targets[next.URN]; has {
+			continue
+		}
+		targets[next.URN] = true
+
+		// Compute the set of resources depending on this one, either implicitly, explicitly,
+		// or because it is a child resource. Add them to the frontier to keep exploring.
+		deps := dg.DependingOn(next, targets, true)
+		frontier = append(frontier, deps...)
+	}
+
+	return targets
 }
 
+// determineAllowedResourcesToDeleteFromTargets computes the full (transitive) closure of resources
+// that need to be deleted to permit the full list of targetsOpt resources to be deleted. This list
+// will include the targetsOpt resources, but may contain more than just that, if there are dependent
+// or child resources that require the targets to exist (and so are implicated in the deletion).
 func (sg *stepGenerator) determineAllowedResourcesToDeleteFromTargets(
 	targetsOpt map[resource.URN]bool) (map[resource.URN]bool, result.Result) {
 
@@ -797,21 +818,14 @@ func (sg *stepGenerator) determineAllowedResourcesToDeleteFromTargets(
 		return nil, nil
 	}
 
-	targetsIncludingChildren := make(map[resource.URN]bool)
-
-	// Include all the children of each target.
-	for target := range targetsOpt {
-		allTargets := sg.getTargetIncludingChildren(target)
-		for child := range allTargets {
-			targetsIncludingChildren[child] = true
-		}
-	}
-
+	// Produce a map of targets and their dependents, including explicit and implicit
+	// DAG dependencies, as well as children (transitively).
+	targets := sg.getTargetDependents(targetsOpt)
 	logging.V(7).Infof("Planner was asked to only delete/update '%v'", targetsOpt)
 	resourcesToDelete := make(map[resource.URN]bool)
 
 	// Now actually use all the requested targets to figure out the exact set to delete.
-	for target := range targetsIncludingChildren {
+	for target := range targets {
 		current := sg.deployment.olds[target]
 		if current == nil {
 			// user specified a target that didn't exist.  they will have already gotten a warning
@@ -1395,7 +1409,7 @@ func (sg *stepGenerator) calculateDependentReplacements(root *resource.State) ([
 	// that have already been registered must not depend on the root. Thus, we ignore these resources if they are
 	// encountered while walking the old dependency graph to determine the set of dependents.
 	impossibleDependents := sg.urns
-	for _, d := range sg.deployment.depGraph.DependingOn(root, impossibleDependents) {
+	for _, d := range sg.deployment.depGraph.DependingOn(root, impossibleDependents, false) {
 		replace, keys, res := requiresReplacement(d)
 		if res != nil {
 			return nil, res

pkg/resource/edit/operations.go

@@ -41,7 +41,7 @@ func DeleteResource(snapshot *deploy.Snapshot, condemnedRes *resource.State) err
 	}
 
 	dg := graph.NewDependencyGraph(snapshot.Resources)
-	dependencies := dg.DependingOn(condemnedRes, nil)
+	dependencies := dg.DependingOn(condemnedRes, nil, false)
 	if len(dependencies) != 0 {
 		return ResourceHasDependenciesError{Condemned: condemnedRes, Dependencies: dependencies}
 	}

pkg/resource/graph/dependency_graph.go

@@ -20,7 +20,8 @@ type DependencyGraph struct {
 // order with respect to the snapshot dependency graph.
 //
 // The time complexity of DependingOn is linear with respect to the number of resources.
-func (dg *DependencyGraph) DependingOn(res *resource.State, ignore map[resource.URN]bool) []*resource.State {
+func (dg *DependencyGraph) DependingOn(res *resource.State,
+	ignore map[resource.URN]bool, includeChildren bool) []*resource.State {
 	// This implementation relies on the detail that snapshots are stored in a valid
 	// topological order.
 	var dependents []*resource.State
@@ -34,6 +35,14 @@ func (dg *DependencyGraph) DependingOn(res *resource.State, ignore map[resource.
 		if ignore[candidate.URN] {
 			return false
 		}
+		if includeChildren && candidate.Parent == res.URN {
+			return true
+		}
+		for _, dependency := range candidate.Dependencies {
+			if dependentSet[dependency] {
+				return true
+			}
+		}
 		if candidate.Provider != "" {
 			ref, err := providers.ParseReference(candidate.Provider)
 			contract.Assert(err == nil)
@@ -41,11 +50,6 @@ func (dg *DependencyGraph) DependingOn(res *resource.State, ignore map[resource.
 				return true
 			}
 		}
-		for _, dependency := range candidate.Dependencies {
-			if dependentSet[dependency] {
-				return true
-			}
-		}
 		return false
 	}
 

pkg/resource/graph/dependency_graph_test.go

@@ -63,58 +63,58 @@ func TestBasicGraph(t *testing.T) {
 
 	assert.Equal(t, []*resource.State{
 		a, b, pB, c, d,
-	}, dg.DependingOn(pA, nil))
+	}, dg.DependingOn(pA, nil, false))
 
 	assert.Equal(t, []*resource.State{
 		b, pB, c, d,
-	}, dg.DependingOn(a, nil))
+	}, dg.DependingOn(a, nil, false))
 
 	assert.Equal(t, []*resource.State{
 		pB, c, d,
-	}, dg.DependingOn(b, nil))
+	}, dg.DependingOn(b, nil, false))
 
 	assert.Equal(t, []*resource.State{
 		c,
-	}, dg.DependingOn(pB, nil))
+	}, dg.DependingOn(pB, nil, false))
 
-	assert.Nil(t, dg.DependingOn(c, nil))
-	assert.Nil(t, dg.DependingOn(d, nil))
+	assert.Nil(t, dg.DependingOn(c, nil, false))
+	assert.Nil(t, dg.DependingOn(d, nil, false))
 
 	assert.Nil(t, dg.DependingOn(pA, map[resource.URN]bool{
 		a.URN: true,
 		b.URN: true,
-	}))
+	}, false))
 
 	assert.Equal(t, []*resource.State{
 		a, pB, c,
 	}, dg.DependingOn(pA, map[resource.URN]bool{
 		b.URN: true,
-	}))
+	}, false))
 
 	assert.Equal(t, []*resource.State{
 		b, pB, c, d,
 	}, dg.DependingOn(pA, map[resource.URN]bool{
 		a.URN: true,
-	}))
+	}, false))
 
 	assert.Equal(t, []*resource.State{
 		c,
 	}, dg.DependingOn(a, map[resource.URN]bool{
 		b.URN:  true,
 		pB.URN: true,
-	}))
+	}, false))
 
 	assert.Equal(t, []*resource.State{
 		pB, c,
 	}, dg.DependingOn(a, map[resource.URN]bool{
 		b.URN: true,
-	}))
+	}, false))
 
 	assert.Equal(t, []*resource.State{
 		d,
 	}, dg.DependingOn(b, map[resource.URN]bool{
 		pB.URN: true,
-	}))
+	}, false))
 }
 
 // Tests that we don't add the same node to the DependingOn set twice.
@@ -133,7 +133,7 @@ func TestGraphNoDuplicates(t *testing.T) {
 
 	assert.Equal(t, []*resource.State{
 		b, c, d,
-	}, dg.DependingOn(a, nil))
+	}, dg.DependingOn(a, nil, false))
 }
 
 func TestDependenciesOf(t *testing.T) {

tests/integration/targets/delete_targets_many_deps/Pulumi.yaml

@@ -0,0 +1,2 @@
+name: delete_targets_many_deps
+runtime: nodejs

tests/integration/targets/delete_targets_many_deps/index.ts

@@ -0,0 +1,47 @@
+// Copyright 2021, Pulumi Corporation.  All rights reserved.
+
+import * as pulumi from "@pulumi/pulumi";
+
+interface MyResourceArgs {
+    input?: pulumi.Input<string> | undefined;
+}
+
+class MyResource extends pulumi.dynamic.Resource {
+    static latestId: number = 0;
+
+    constructor(name, args?: MyResourceArgs, opts?: pulumi.CustomResourceOptions) {
+        super({
+            async create(inputs: any) {
+                return { id: (MyResource.latestId++).toString() };
+            },
+        }, name, args || {}, opts);
+    }
+}
+
+// Create a chain of resources, such that attempting to delete
+// one will fail due to numerous dependency violations. This includes
+// both implicit and explicit, as well as parent/child, dependencies.
+
+// A
+// B (impl depends on A)
+// C (expl depends on A)
+// D (impl depends on B)
+// E (expl depends on B)
+// F (child of A)
+// G (child of B)
+// H (expl depends on A, B, impl depends on C, D, child of F)
+
+const a = new MyResource("a");
+const b = new MyResource("b", { input: a.urn });
+const c = new MyResource("c", { }, { dependsOn: a });
+const d = new MyResource("d", { input: b.urn });
+const e = new MyResource("e", { }, { dependsOn: b });
+const f = new MyResource("f", { }, { parent: a });
+const g = new MyResource("g", { }, { parent: b });
+const h = new MyResource("h",
+    { input: pulumi.all(([c.urn, d.urn])).apply(([curn, _])=>curn) },
+    {
+        dependsOn: [a, b],
+        parent: f,
+    },
+);

tests/integration/targets/delete_targets_many_deps/package.json

@@ -0,0 +1,12 @@
+{
+    "name": "delete_targets_many_deps",
+    "license": "Apache-2.0",
+    "main": "bin/index.js",
+    "typings": "bin/index.d.ts",
+    "devDependencies": {
+        "typescript": "^2.5.3"
+    },
+    "peerDependencies": {
+        "@pulumi/pulumi": "latest"
+    }
+}

tests/integration/targets/targets_test.go

@@ -3,6 +3,7 @@
 package ints
 
 import (
+	"fmt"
 	"os"
 	"path"
 	"strings"
@@ -10,8 +11,10 @@ import (
 
 	"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
 	ptesting "github.com/pulumi/pulumi/sdk/v3/go/common/testing"
+	"github.com/pulumi/pulumi/sdk/v3/go/common/tokens"
 	"github.com/pulumi/pulumi/sdk/v3/go/common/util/contract"
 	"github.com/pulumi/pulumi/sdk/v3/go/common/util/fsutil"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestUntargetedCreateDuringTargetedUpdate(t *testing.T) {
@@ -48,3 +51,56 @@ func TestUntargetedCreateDuringTargetedUpdate(t *testing.T) {
 	e.RunCommand("pulumi", "destroy", "--skip-preview", "--non-interactive", "--yes")
 	e.RunCommand("pulumi", "stack", "rm", "--yes")
 }
+
+func TestDeleteManyTargets(t *testing.T) {
+	if os.Getenv("PULUMI_ACCESS_TOKEN") == "" {
+		t.Skipf("Skipping: PULUMI_ACCESS_TOKEN is not set")
+	}
+
+	e := ptesting.NewEnvironment(t)
+	defer func() {
+		if !t.Failed() {
+			e.DeleteEnvironment()
+		}
+	}()
+
+	// First just spin up the project.
+	projName := "delete_targets_many_deps"
+	stackName, err := resource.NewUniqueHex("test-", 8, -1)
+	contract.AssertNoErrorf(err, "resource.NewUniqueHex should not fail with no maximum length is set")
+	e.ImportDirectory(projName)
+	e.RunCommand("pulumi", "stack", "init", stackName)
+	e.RunCommand("yarn", "install")
+	e.RunCommand("yarn", "link", "@pulumi/pulumi")
+	e.RunCommand("pulumi", "up", "--non-interactive", "--skip-preview", "--yes")
+
+	// Create a handy mkURN func to create URNs for dynamic resources in this project/stack.
+	resourceType := tokens.Type("pulumi-nodejs:dynamic:Resource")
+	mkURNStr := func(resourceName tokens.QName, parentType tokens.Type) string {
+		return string(resource.NewURN(
+			tokens.QName(stackName), tokens.PackageName(projName), parentType, resourceType, resourceName))
+	}
+
+	// Attempt to destroy the root-most node. It should fail and the error text should
+	// mention every one of the nodes in the entire graph (since they all transitively depend on a).
+	stdout, _ := e.RunCommandExpectError("pulumi", "destroy", "--skip-preview", "--yes", "--non-interactive",
+		"--target", mkURNStr("a", ""))
+	assert.Contains(t, stdout, mkURNStr("b", ""))
+	assert.Contains(t, stdout, mkURNStr("c", ""))
+	assert.Contains(t, stdout, mkURNStr("d", ""))
+	assert.Contains(t, stdout, mkURNStr("e", ""))
+	assert.Contains(t, stdout, mkURNStr("f", resourceType))
+	assert.Contains(t, stdout, mkURNStr("g", resourceType))
+
+	// Destroy the leaf-most node. This should work just fine.
+	e.RunCommand("pulumi", "destroy", "--skip-preview", "--yes", "--non-interactive",
+		"--target", mkURNStr("h", tokens.Type(fmt.Sprintf("%[1]s$%[1]s", resourceType))))
+
+	// Finally, go back and try to delete the root-most node, but clean up the transitive closure.
+	e.RunCommand("pulumi", "destroy", "--skip-preview", "--yes", "--non-interactive",
+		"--target", mkURNStr("a", ""), "--target-dependents")
+
+	// Finally clean up the entire stack.
+	e.RunCommand("pulumi", "destroy", "--skip-preview", "--yes", "--non-interactive")
+	e.RunCommand("pulumi", "stack", "rm", "--yes")
+}