Merge pull request #28165 from hashicorp/jbardin/destroy-update-dep

Destroy then update dependency ordering

docs/destroying.md

@@ -114,6 +114,7 @@ digraph replacement {
     }
 
     a -> a_d;
+    a -> b_d [style=dotted];
     b -> a_d [style=dotted];
     b -> b_d;
 }
@@ -158,6 +159,28 @@ While the dependency edge from `B update` to `A destroy` isn't necessary in
 these examples, it is shown here as an implementation detail which will be
 mentioned later on.
 
+A final example based on the replacement graph; starting with the above
+configuration where `B` depends on `A`. The graph is reduced to an update of
+`A` while only destroying `B`. The interesting feature here is the remaining
+dependency of `A update` on `B destroy`. We can derive this ordering of
+operations from the full replacement example above, by replacing `A create`
+with `A update` and removing the unused nodes.
+
+![Replace All](./images/destroy_then_update.png)
+<!--
+digraph destroy_then_update {
+    subgraph update {
+        rank=same;
+        a [label="A update"];
+    }
+    subgraph destroy {
+        rank=same;
+        b_d [label="B destroy"];
+    }
+
+    a -> b_d;
+}
+-->
 ## Create Before Destroy
 
 Currently, the only user-controllable method for changing the ordering of

terraform/context_apply2_test.go

@@ -3,7 +3,9 @@ package terraform
 import (
 	"errors"
 	"fmt"
+	"sync"
 	"testing"
+	"time"
 
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/providers"
@@ -177,3 +179,72 @@ output "data" {
 		t.Fatal(diags.Err())
 	}
 }
+
+func TestContext2Apply_destroyThenUpdate(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+resource "test_instance" "a" {
+	value = "udpated"
+}
+`,
+	})
+
+	p := testProvider("test")
+	p.PlanResourceChangeFn = testDiffFn
+
+	var orderMu sync.Mutex
+	var order []string
+	p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) (resp providers.ApplyResourceChangeResponse) {
+		id := req.PriorState.GetAttr("id").AsString()
+		if id == "b" {
+			// slow down the b destroy, since a should wait for it
+			time.Sleep(100 * time.Millisecond)
+		}
+
+		orderMu.Lock()
+		order = append(order, id)
+		orderMu.Unlock()
+
+		resp.NewState = req.PlannedState
+		return resp
+	}
+
+	addrA := mustResourceInstanceAddr(`test_instance.a`)
+	addrB := mustResourceInstanceAddr(`test_instance.b`)
+
+	state := states.BuildState(func(s *states.SyncState) {
+		s.SetResourceInstanceCurrent(addrA, &states.ResourceInstanceObjectSrc{
+			AttrsJSON: []byte(`{"id":"a","value":"old","type":"test"}`),
+			Status:    states.ObjectReady,
+		}, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`))
+
+		// test_instance.b depended on test_instance.a, and therefor should be
+		// destroyed before any changes to test_instance.a
+		s.SetResourceInstanceCurrent(addrB, &states.ResourceInstanceObjectSrc{
+			AttrsJSON:    []byte(`{"id":"b"}`),
+			Status:       states.ObjectReady,
+			Dependencies: []addrs.ConfigResource{addrA.ContainingResource().Config()},
+		}, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`))
+	})
+
+	ctx := testContext2(t, &ContextOpts{
+		Config: m,
+		State:  state,
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+	})
+
+	if _, diags := ctx.Plan(); diags.HasErrors() {
+		t.Fatal(diags.Err())
+	}
+
+	_, diags := ctx.Apply()
+	if diags.HasErrors() {
+		t.Fatal(diags.Err())
+	}
+
+	if order[0] != "b" {
+		t.Fatalf("expected apply order [b, a], got: %v\n", order)
+	}
+}

terraform/transform_destroy_edge.go

@@ -59,7 +59,7 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 
 	// Record the creators, which will need to depend on the destroyers if they
 	// are only being updated.
-	creators := make(map[string]GraphNodeCreator)
+	creators := make(map[string][]GraphNodeCreator)
 
 	// destroyersByResource records each destroyer by the ConfigResource
 	// address.  We use this because dependencies are only referenced as
@@ -83,8 +83,8 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 			resAddr := addr.ContainingResource().Config().String()
 			destroyersByResource[resAddr] = append(destroyersByResource[resAddr], n)
 		case GraphNodeCreator:
-			addr := n.CreateAddr()
-			creators[addr.String()] = n
+			addr := n.CreateAddr().ContainingResource().Config().String()
+			creators[addr] = append(creators[addr], n)
 		}
 	}
 
@@ -111,24 +111,38 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 						log.Printf("[TRACE] DestroyEdgeTransformer: skipping %s => %s inter-module-instance dependency\n", dag.VertexName(desDep), dag.VertexName(des))
 					}
 				}
+
+				// We can have some create or update nodes which were
+				// dependents of the destroy node. If they have no destroyer
+				// themselves, make the connection directly from the creator.
+				for _, createDep := range creators[resAddr.String()] {
+					if !graphNodesAreResourceInstancesInDifferentInstancesOfSameModule(createDep, des) {
+						log.Printf("[DEBUG] DestroyEdgeTransformer: %s has stored dependency of %s\n", dag.VertexName(createDep), dag.VertexName(des))
+						g.Connect(dag.BasicEdge(createDep, des))
+					} else {
+						log.Printf("[TRACE] DestroyEdgeTransformer: skipping %s => %s inter-module-instance dependency\n", dag.VertexName(createDep), dag.VertexName(des))
+					}
+				}
 			}
 		}
 	}
 
 	// connect creators to any destroyers on which they may depend
-	for _, c := range creators {
-		ri, ok := c.(GraphNodeResourceInstance)
-		if !ok {
-			continue
-		}
+	for _, cs := range creators {
+		for _, c := range cs {
+			ri, ok := c.(GraphNodeResourceInstance)
+			if !ok {
+				continue
+			}
 
-		for _, resAddr := range ri.StateDependencies() {
-			for _, desDep := range destroyersByResource[resAddr.String()] {
-				if !graphNodesAreResourceInstancesInDifferentInstancesOfSameModule(c, desDep) {
-					log.Printf("[TRACE] DestroyEdgeTransformer: %s has stored dependency of %s\n", dag.VertexName(c), dag.VertexName(desDep))
-					g.Connect(dag.BasicEdge(c, desDep))
-				} else {
-					log.Printf("[TRACE] DestroyEdgeTransformer: skipping %s => %s inter-module-instance dependency\n", dag.VertexName(c), dag.VertexName(desDep))
+			for _, resAddr := range ri.StateDependencies() {
+				for _, desDep := range destroyersByResource[resAddr.String()] {
+					if !graphNodesAreResourceInstancesInDifferentInstancesOfSameModule(c, desDep) {
+						log.Printf("[TRACE] DestroyEdgeTransformer: %s has stored dependency of %s\n", dag.VertexName(c), dag.VertexName(desDep))
+						g.Connect(dag.BasicEdge(c, desDep))
+					} else {
+						log.Printf("[TRACE] DestroyEdgeTransformer: skipping %s => %s inter-module-instance dependency\n", dag.VertexName(c), dag.VertexName(desDep))
+					}
 				}
 			}
 		}

terraform/transform_destroy_edge_test.go

@@ -260,14 +260,74 @@ module.child[1].test_object.c (destroy)
 	}
 }
 
+func TestDestroyEdgeTransformer_destroyThenUpdate(t *testing.T) {
+	g := Graph{Path: addrs.RootModuleInstance}
+	g.Add(testUpdateNode("test_object.A"))
+	g.Add(testDestroyNode("test_object.B"))
+
+	state := states.NewState()
+	root := state.EnsureModule(addrs.RootModuleInstance)
+	root.SetResourceInstanceCurrent(
+		mustResourceInstanceAddr("test_object.A").Resource,
+		&states.ResourceInstanceObjectSrc{
+			Status:    states.ObjectReady,
+			AttrsJSON: []byte(`{"id":"A","test_string":"old"}`),
+		},
+		mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
+	)
+	root.SetResourceInstanceCurrent(
+		mustResourceInstanceAddr("test_object.B").Resource,
+		&states.ResourceInstanceObjectSrc{
+			Status:       states.ObjectReady,
+			AttrsJSON:    []byte(`{"id":"B","test_string":"x"}`),
+			Dependencies: []addrs.ConfigResource{mustConfigResourceAddr("test_object.A")},
+		},
+		mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
+	)
+
+	if err := (&AttachStateTransformer{State: state}).Transform(&g); err != nil {
+		t.Fatal(err)
+	}
+
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+resource "test_instance" "a" {
+	test_string = "udpated"
+}
+`,
+	})
+	tf := &DestroyEdgeTransformer{
+		Config:  m,
+		Schemas: simpleTestSchemas(),
+	}
+	if err := tf.Transform(&g); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	expected := strings.TrimSpace(`
+test_object.A
+  test_object.B (destroy)
+test_object.B (destroy)
+`)
+	actual := strings.TrimSpace(g.String())
+
+	if actual != expected {
+		t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected)
+	}
+}
+
 func testDestroyNode(addrString string) GraphNodeDestroyer {
 	instAddr := mustResourceInstanceAddr(addrString)
-
 	inst := NewNodeAbstractResourceInstance(instAddr)
-
 	return &NodeDestroyResourceInstance{NodeAbstractResourceInstance: inst}
 }
 
+func testUpdateNode(addrString string) GraphNodeCreator {
+	instAddr := mustResourceInstanceAddr(addrString)
+	inst := NewNodeAbstractResourceInstance(instAddr)
+	return &NodeApplyableResourceInstance{NodeAbstractResourceInstance: inst}
+}
+
 const testTransformDestroyEdgeBasicStr = `
 test_object.A (destroy)
   test_object.B (destroy)