DependingOn transitivity

Per code review feedback from @pgavlin.

CHANGELOG_PENDING.md

@@ -5,3 +5,6 @@
 
 - [programgen/go] - Don't change imported resource names.
   [#8353](https://github.com/pulumi/pulumi/pull/8353)
+
+- [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
@@ -31,17 +32,22 @@ func (dg *DependencyGraph) DependingOn(res *resource.State, ignore map[resource.
 	dependentSet[res.URN] = true
 
 	isDependent := func(candidate *resource.State) bool {
-		if ignore[candidate.URN] {
-			return false
-		}
+		// Direct deps include explicit `Dependencies`,
+		// provider, and parent (under `includeChildren=true`
+		// semantic).
+		directDeps := candidate.Dependencies
 		if candidate.Provider != "" {
 			ref, err := providers.ParseReference(candidate.Provider)
 			contract.Assert(err == nil)
-			if dependentSet[ref.URN()] {
-				return true
-			}
+			directDeps = append(directDeps, ref.URN())
 		}
-		for _, dependency := range candidate.Dependencies {
+		if includeChildren && candidate.Parent != "" {
+			directDeps = append(directDeps, candidate.Parent)
+		}
+
+		// We are computing a transitive closure of direct
+		// deps; therefore check in `dependentSet`.
+		for _, dependency := range directDeps {
 			if dependentSet[dependency] {
 				return true
 			}

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) {
@@ -229,3 +229,41 @@ func TestDependenciesOfRemoteComponentsNoCycle(t *testing.T) {
 	assert.True(t, rDependencies[parent])
 	assert.False(t, rDependencies[child])
 }
+
+func NewCustomResource(name string, provider *resource.State, deps ...resource.URN) *resource.State {
+	r := NewResource(name, provider, deps...)
+	r.Custom = true
+	return r
+}
+
+func TestDependingOnIndirect(t *testing.T) {
+	var noProvider *resource.State
+
+	d2 := NewCustomResource("d2", noProvider)
+	d3 := NewCustomResource("d3", noProvider, d2.URN)
+	c3 := NewCustomResource("c3", noProvider)
+	c3.Parent = d3.URN
+
+	dg := NewDependencyGraph([]*resource.State{
+		d2, d3, c3,
+	})
+
+	for r := range dg.DependenciesOf(c3) {
+		t.Logf("dg.DependenciesOf(c3) includes %v", r.URN)
+	}
+
+	for r := range dg.DependenciesOf(d3) {
+		t.Logf("dg.DependenciesOf(d3) includes %v", r.URN)
+	}
+
+	var foundC3 bool
+	for _, r := range dg.DependingOn(d2, nil, true) {
+		if r == c3 {
+			foundC3 = true
+		}
+		t.Logf("DependingOn(d2) includes %v", r.URN)
+	}
+	if !foundC3 {
+		t.Errorf("DependingOn(d2, nil) should have included c3")
+	}
+}

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")
+}