From 0e4a6e3e89aac9f82f3c7fe870649bc96ec3c98b Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 3 Dec 2016 23:44:09 -0800 Subject: [PATCH] terraform: apply resource must depend on destroy deps Fixes #10440 This updates the behavior of "apply" resources to depend on the destroy versions of their dependencies. We make an exception to this behavior when the "apply" resource is CBD. This is odd and not 100% correct, but it mimics the behavior of the legacy graphs and avoids us having to do major core work to support the 100% correct solution. I'll explain this in examples... Given the following configuration: resource "null_resource" "a" { count = "${var.count}" } resource "null_resource" "b" { triggers { key = "${join(",", null_resource.a.*.id)}" } } Assume we've successfully created this configuration with count = 2. When going from count = 2 to count = 1, `null_resource.b` should wait for `null_resource.a.1` to destroy. If it doesn't, then it is a race: depending when we interpolate the `triggers.key` attribute of `null_resource.b`, we may get 1 value or 2. If `null_resource.a.1` is destroyed, we'll get 1. Otherwise, we'll get 2. This was the root cause of #10440 In the legacy graphs, `null_resource.b` would depend on the destruction of any `null_resource.a` (orphans, tainted, anything!). This would ensure proper ordering. We mimic that behavior here. The difference is CBD. If `null_resource.b` has CBD enabled, then the ordering **in the legacy graph** becomes: 1. null_resource.b (create) 2. null_resource.b (destroy) 3. null_resource.a (destroy) In this case, the update would always have 2 values for `triggers.key`, even though we were destroying a resource later! This scenario required two `terraform apply` operations. This is what the CBD check is for in this PR. We do this to mimic the behavior of the legacy graph. The correct solution to do one day is to allow splat references (`null_resource.a.*.id`) to happen in parallel and only read up to to the `count` amount in the state. This requires some fairly significant work close to the 0.8 release date, so we can defer this to later and adopt the 0.7.x behavior for now. --- terraform/context_apply_test.go | 101 +++++++++++++ terraform/graph_builder_apply_test.go | 140 ++++++++++++++++++ terraform/node_resource_apply.go | 30 ++++ .../apply-multi-var-count-dec/main.tf | 10 ++ .../graph-builder-apply-count/main.tf | 7 + .../graph-builder-apply-double-cbd/main.tf | 9 ++ terraform/transform_destroy_edge_test.go | 3 + 7 files changed, 300 insertions(+) create mode 100644 terraform/test-fixtures/apply-multi-var-count-dec/main.tf create mode 100644 terraform/test-fixtures/graph-builder-apply-count/main.tf create mode 100644 terraform/test-fixtures/graph-builder-apply-double-cbd/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 1601a0acf..6458b82d7 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -2885,6 +2885,107 @@ func TestContext2Apply_multiVarOrderInterp(t *testing.T) { } } +// Based on GH-10440 where a graph edge wasn't properly being created +// between a modified resource and a count instance being destroyed. +func TestContext2Apply_multiVarCountDec(t *testing.T) { + var s *State + + // First create resources. Nothing sneaky here. + { + m := testModule(t, "apply-multi-var-count-dec") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + Variables: map[string]interface{}{ + "count": "2", + }, + }) + + if _, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } + + state, err := ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + t.Logf("Step 1 state: %s", state) + + s = state + } + + // Decrease the count by 1 and verify that everything happens in the + // right order. + { + m := testModule(t, "apply-multi-var-count-dec") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + + // Verify that aws_instance.bar is modified first and nothing + // else happens at the same time. + var checked bool + var called int32 + var lock sync.Mutex + p.ApplyFn = func( + info *InstanceInfo, + is *InstanceState, + id *InstanceDiff) (*InstanceState, error) { + lock.Lock() + defer lock.Unlock() + + if info.HumanId() == "aws_instance.bar" { + checked = true + + // Sleep to allow parallel execution + time.Sleep(50 * time.Millisecond) + + // Verify that called is 0 (dep not called) + if atomic.LoadInt32(&called) != 1 { + return nil, fmt.Errorf("nothing else should be called") + } + } + + atomic.AddInt32(&called, 1) + return testApplyFn(info, is, id) + } + + ctx := testContext2(t, &ContextOpts{ + State: s, + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + Variables: map[string]interface{}{ + "count": "1", + }, + }) + + if _, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } + + state, err := ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + if !checked { + t.Fatal("apply never called") + } + + t.Logf("Step 2 state: %s", state) + + s = state + } +} + func TestContext2Apply_nilDiff(t *testing.T) { m := testModule(t, "apply-good") p := testProvider("aws") diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index 28599602e..57307a8f3 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -88,6 +88,110 @@ func TestApplyGraphBuilder(t *testing.T) { } } +// This tests the ordering of two resources that are both CBD that +// require destroy/create. +func TestApplyGraphBuilder_doubleCBD(t *testing.T) { + diff := &Diff{ + Modules: []*ModuleDiff{ + &ModuleDiff{ + Path: []string{"root"}, + Resources: map[string]*InstanceDiff{ + "aws_instance.A": &InstanceDiff{ + Destroy: true, + Attributes: map[string]*ResourceAttrDiff{ + "name": &ResourceAttrDiff{ + Old: "", + New: "foo", + }, + }, + }, + + "aws_instance.B": &InstanceDiff{ + Destroy: true, + Attributes: map[string]*ResourceAttrDiff{ + "name": &ResourceAttrDiff{ + Old: "", + New: "foo", + }, + }, + }, + }, + }, + }, + } + + b := &ApplyGraphBuilder{ + Module: testModule(t, "graph-builder-apply-double-cbd"), + Diff: diff, + Providers: []string{"aws"}, + Provisioners: []string{"exec"}, + DisableReduce: true, + } + + g, err := b.Build(RootModulePath) + if err != nil { + t.Fatalf("err: %s", err) + } + + if !reflect.DeepEqual(g.Path, RootModulePath) { + t.Fatalf("bad: %#v", g.Path) + } + + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(testApplyGraphBuilderDoubleCBDStr) + if actual != expected { + t.Fatalf("bad: %s", actual) + } +} + +// This tests the ordering of destroying a single count of a resource. +func TestApplyGraphBuilder_destroyCount(t *testing.T) { + diff := &Diff{ + Modules: []*ModuleDiff{ + &ModuleDiff{ + Path: []string{"root"}, + Resources: map[string]*InstanceDiff{ + "aws_instance.A.1": &InstanceDiff{ + Destroy: true, + }, + + "aws_instance.B": &InstanceDiff{ + Attributes: map[string]*ResourceAttrDiff{ + "name": &ResourceAttrDiff{ + Old: "", + New: "foo", + }, + }, + }, + }, + }, + }, + } + + b := &ApplyGraphBuilder{ + Module: testModule(t, "graph-builder-apply-count"), + Diff: diff, + Providers: []string{"aws"}, + Provisioners: []string{"exec"}, + DisableReduce: true, + } + + g, err := b.Build(RootModulePath) + if err != nil { + t.Fatalf("err: %s", err) + } + + if !reflect.DeepEqual(g.Path, RootModulePath) { + t.Fatalf("bad: %#v", g.Path) + } + + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(testApplyGraphBuilderDestroyCountStr) + if actual != expected { + t.Fatalf("bad: %s", actual) + } +} + const testApplyGraphBuilderStr = ` aws_instance.create provider.aws @@ -113,3 +217,39 @@ module.child.provider.aws module.child.provisioner.exec provider.aws ` + +const testApplyGraphBuilderDoubleCBDStr = ` +aws_instance.A + provider.aws +aws_instance.A (destroy) + aws_instance.A + aws_instance.B + aws_instance.B (destroy) + provider.aws +aws_instance.B + aws_instance.A + provider.aws +aws_instance.B (destroy) + aws_instance.B + provider.aws +meta.count-boundary (count boundary fixup) + aws_instance.A + aws_instance.A (destroy) + aws_instance.B + aws_instance.B (destroy) + provider.aws +provider.aws +` + +const testApplyGraphBuilderDestroyCountStr = ` +aws_instance.A[1] (destroy) + provider.aws +aws_instance.B + aws_instance.A[1] (destroy) + provider.aws +meta.count-boundary (count boundary fixup) + aws_instance.A[1] (destroy) + aws_instance.B + provider.aws +provider.aws +` diff --git a/terraform/node_resource_apply.go b/terraform/node_resource_apply.go index 811aa55eb..a16683642 100644 --- a/terraform/node_resource_apply.go +++ b/terraform/node_resource_apply.go @@ -17,6 +17,36 @@ func (n *NodeApplyableResource) CreateAddr() *ResourceAddress { return n.NodeAbstractResource.Addr } +// GraphNodeReferencer, overriding NodeAbstractResource +func (n *NodeApplyableResource) References() []string { + result := n.NodeAbstractResource.References() + + // The "apply" side of a resource generally also depends on the + // destruction of its dependencies as well. For example, if a LB + // references a set of VMs with ${vm.foo.*.id}, then we must wait for + // the destruction so we get the newly updated list of VMs. + // + // The exception here is CBD. When CBD is set, we don't do this since + // it would create a cycle. By not creating a cycle, we require two + // applies since the first apply the creation step will use the OLD + // values (pre-destroy) and the second step will update. + // + // This is how Terraform behaved with "legacy" graphs (TF <= 0.7.x). + // We mimic that behavior here now and can improve upon it in the future. + // + // This behavior is tested in graph_build_apply_test.go to test ordering. + cbd := n.Config != nil && n.Config.Lifecycle.CreateBeforeDestroy + if !cbd { + // The "apply" side of a resource always depends on the destruction + // of all its dependencies in addition to the creation. + for _, v := range result { + result = append(result, v+".destroy") + } + } + + return result +} + // GraphNodeEvalable func (n *NodeApplyableResource) EvalTree() EvalNode { addr := n.NodeAbstractResource.Addr diff --git a/terraform/test-fixtures/apply-multi-var-count-dec/main.tf b/terraform/test-fixtures/apply-multi-var-count-dec/main.tf new file mode 100644 index 000000000..ab820b5cf --- /dev/null +++ b/terraform/test-fixtures/apply-multi-var-count-dec/main.tf @@ -0,0 +1,10 @@ +variable "count" {} + +resource "aws_instance" "foo" { + count = "${var.count}" + value = "foo" +} + +resource "aws_instance" "bar" { + value = "${join(",", aws_instance.foo.*.id)}" +} diff --git a/terraform/test-fixtures/graph-builder-apply-count/main.tf b/terraform/test-fixtures/graph-builder-apply-count/main.tf new file mode 100644 index 000000000..982021bf9 --- /dev/null +++ b/terraform/test-fixtures/graph-builder-apply-count/main.tf @@ -0,0 +1,7 @@ +resource "aws_instance" "A" { + count = 1 +} + +resource "aws_instance" "B" { + value = ["${aws_instance.A.*.id}"] +} diff --git a/terraform/test-fixtures/graph-builder-apply-double-cbd/main.tf b/terraform/test-fixtures/graph-builder-apply-double-cbd/main.tf new file mode 100644 index 000000000..bc121744d --- /dev/null +++ b/terraform/test-fixtures/graph-builder-apply-double-cbd/main.tf @@ -0,0 +1,9 @@ +resource "aws_instance" "A" { + lifecycle { create_before_destroy = true } +} + +resource "aws_instance" "B" { + value = ["${aws_instance.A.*.id}"] + + lifecycle { create_before_destroy = true } +} diff --git a/terraform/transform_destroy_edge_test.go b/terraform/transform_destroy_edge_test.go index cf8e4c704..1f7b1a5d6 100644 --- a/terraform/transform_destroy_edge_test.go +++ b/terraform/transform_destroy_edge_test.go @@ -98,6 +98,7 @@ func TestDestroyEdgeTransformer_module(t *testing.T) { type graphNodeCreatorTest struct { AddrString string + Refs []string } func (n *graphNodeCreatorTest) Name() string { return n.CreateAddr().String() } @@ -110,6 +111,8 @@ func (n *graphNodeCreatorTest) CreateAddr() *ResourceAddress { return addr } +func (n *graphNodeCreatorTest) References() []string { return n.Refs } + type graphNodeDestroyerTest struct { AddrString string CBD bool