From 8223e606df9ca3695883302b8b229150268a411d Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Tue, 10 May 2016 15:11:48 -0500 Subject: [PATCH 1/2] terraform: add debug logging to Noop impls This sort of output is needed to debug these codepaths anyways, so I figured might as well leave it in. --- terraform/graph_config_node_resource.go | 14 ++++++++------ terraform/graph_config_node_variable.go | 5 +++++ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/terraform/graph_config_node_resource.go b/terraform/graph_config_node_resource.go index 41fc7e515..7c189b605 100644 --- a/terraform/graph_config_node_resource.go +++ b/terraform/graph_config_node_resource.go @@ -2,6 +2,7 @@ package terraform import ( "fmt" + "log" "strings" "github.com/hashicorp/terraform/config" @@ -272,14 +273,17 @@ func (n *GraphNodeConfigResource) DestroyNode(mode GraphNodeDestroyMode) GraphNo // GraphNodeNoopPrunable func (n *GraphNodeConfigResource) Noop(opts *NoopOpts) bool { + log.Printf("[DEBUG] Checking resource noop: %s", n.Name()) // We don't have any noop optimizations for destroy nodes yet if n.DestroyMode != DestroyNone { + log.Printf("[DEBUG] Destroy node, not a noop") return false } // If there is no diff, then we aren't a noop since something needs to // be done (such as a plan). We only check if we're a noop in a diff. if opts.Diff == nil || opts.Diff.Empty() { + log.Printf("[DEBUG] No diff, not a noop") return false } @@ -287,6 +291,7 @@ func (n *GraphNodeConfigResource) Noop(opts *NoopOpts) bool { // we need to be sure to evaluate the count so that splat variables work // later (which need to know the full count). if len(n.Resource.RawCount.Interpolations) > 0 { + log.Printf("[DEBUG] Count has interpolations, not a noop") return false } @@ -294,12 +299,7 @@ func (n *GraphNodeConfigResource) Noop(opts *NoopOpts) bool { // it means there is a diff, and that the module we're in just isn't // in it, meaning we're not doing anything. if opts.ModDiff == nil || opts.ModDiff.Empty() { - return true - } - - // If the whole module is being destroyed, then the resource nodes in that - // module are irrelevant - we only need to keep the destroy nodes. - if opts.ModDiff != nil && opts.ModDiff.Destroy == true { + log.Printf("[DEBUG] No mod diff, treating resource as a noop") return true } @@ -310,11 +310,13 @@ func (n *GraphNodeConfigResource) Noop(opts *NoopOpts) bool { found := false for k, _ := range opts.ModDiff.Resources { if strings.HasPrefix(k, prefix) { + log.Printf("[DEBUG] Diff has %s, resource is not a noop", k) found = true break } } + log.Printf("[DEBUG] Final noop value: %t", !found) return !found } diff --git a/terraform/graph_config_node_variable.go b/terraform/graph_config_node_variable.go index 389d7babf..41aedff9e 100644 --- a/terraform/graph_config_node_variable.go +++ b/terraform/graph_config_node_variable.go @@ -2,6 +2,7 @@ package terraform import ( "fmt" + "log" "github.com/hashicorp/terraform/config" "github.com/hashicorp/terraform/config/module" @@ -79,11 +80,13 @@ func (n *GraphNodeConfigVariable) DestroyEdgeInclude(v dag.Vertex) bool { // GraphNodeNoopPrunable func (n *GraphNodeConfigVariable) Noop(opts *NoopOpts) bool { + log.Printf("[DEBUG] Checking variable noop: %s", n.Name()) // If we have no diff, always keep this in the graph. We have to do // this primarily for validation: we want to validate that variable // interpolations are valid even if there are no resources that // depend on them. if opts.Diff == nil || opts.Diff.Empty() { + log.Printf("[DEBUG] No diff, not a noop") return false } @@ -93,9 +96,11 @@ func (n *GraphNodeConfigVariable) Noop(opts *NoopOpts) bool { continue } + log.Printf("[DEBUG] Found up edge to %s, var is not noop", dag.VertexName(v)) return false } + log.Printf("[DEBUG] No up edges, treating variable as a noop") return true } From 559f017ebb7649d4db425f9f2412c1463a36f97d Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Tue, 10 May 2016 15:25:58 -0500 Subject: [PATCH 2/2] terraform: Correct fix for destroy interp errors The fix that landed in #6557 was unfortunately the wrong subset of the work I had been doing locally, and users of the attached bugs are still reporting problems with Terraform v0.6.16. At the very last step, I attempted to scope down both the failing test and the implementation to their bare essentials, but ended up with a test that did not exercise the root of the problem and a subset of the implementation that was insufficient for a full bugfix. The key thing I removed from the test was a _referencing output_ for the module, which is what breaks down the #6557 solution. I've re-tested the examples in #5440 and #3268 to verify this solution does indeed solve the problem. --- terraform/context_apply_test.go | 15 ++++++--------- terraform/graph_config_node_variable.go | 10 ++++++++++ .../apply-destroy-module-with-attrs/child/main.tf | 4 ++++ .../apply-destroy-module-with-attrs/main.tf | 6 +++++- 4 files changed, 25 insertions(+), 10 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index fe030b2c6..ca172ca1f 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -2549,19 +2549,16 @@ func TestContext2Apply_destroyModuleWithAttrsReferencingResource(t *testing.T) { Providers: map[string]ResourceProviderFactory{ "aws": testProviderFuncFixed(p), }, - Variables: map[string]string{ - "key_name": "foobarkey", - }, }) // First plan and apply a create operation if _, err := ctx.Plan(); err != nil { - t.Fatalf("err: %s", err) + t.Fatalf("plan err: %s", err) } state, err = ctx.Apply() if err != nil { - t.Fatalf("err: %s", err) + t.Fatalf("apply err: %s", err) } } @@ -2585,17 +2582,17 @@ func TestContext2Apply_destroyModuleWithAttrsReferencingResource(t *testing.T) { // First plan and apply a create operation plan, err := ctx.Plan() if err != nil { - t.Fatalf("err: %s", err) + t.Fatalf("destroy plan err: %s", err) } var buf bytes.Buffer if err := WritePlan(plan, &buf); err != nil { - t.Fatalf("err: %s", err) + t.Fatalf("plan write err: %s", err) } planFromFile, err := ReadPlan(&buf) if err != nil { - t.Fatalf("err: %s", err) + t.Fatalf("plan read err: %s", err) } ctx, err = planFromFile.Context(&ContextOpts{ @@ -2609,7 +2606,7 @@ func TestContext2Apply_destroyModuleWithAttrsReferencingResource(t *testing.T) { state, err = ctx.Apply() if err != nil { - t.Fatalf("err: %s", err) + t.Fatalf("destroy apply err: %s", err) } } diff --git a/terraform/graph_config_node_variable.go b/terraform/graph_config_node_variable.go index 41aedff9e..f08520532 100644 --- a/terraform/graph_config_node_variable.go +++ b/terraform/graph_config_node_variable.go @@ -90,6 +90,16 @@ func (n *GraphNodeConfigVariable) Noop(opts *NoopOpts) bool { return false } + // We have to find our our module diff since we do funky things with + // the flat node's implementation of Path() below. + modDiff := opts.Diff.ModuleByPath(n.ModulePath) + + // If we're destroying, we have no need of variables. + if modDiff != nil && modDiff.Destroy { + log.Printf("[DEBUG] Destroy diff, treating variable as a noop") + return true + } + for _, v := range opts.Graph.UpEdges(opts.Vertex).List() { // This is terrible, but I can't think of a better way to do this. if dag.VertexName(v) == rootNodeName { diff --git a/terraform/test-fixtures/apply-destroy-module-with-attrs/child/main.tf b/terraform/test-fixtures/apply-destroy-module-with-attrs/child/main.tf index 2aa1e1045..95c701268 100644 --- a/terraform/test-fixtures/apply-destroy-module-with-attrs/child/main.tf +++ b/terraform/test-fixtures/apply-destroy-module-with-attrs/child/main.tf @@ -3,3 +3,7 @@ variable "vpc_id" {} resource "aws_instance" "child" { vpc_id = "${var.vpc_id}" } + +output "modout" { + value = "${aws_instance.child.id}" +} diff --git a/terraform/test-fixtures/apply-destroy-module-with-attrs/main.tf b/terraform/test-fixtures/apply-destroy-module-with-attrs/main.tf index 0f26089ff..d369873bf 100644 --- a/terraform/test-fixtures/apply-destroy-module-with-attrs/main.tf +++ b/terraform/test-fixtures/apply-destroy-module-with-attrs/main.tf @@ -1,6 +1,10 @@ -resource "aws_instance" "vpc" { } +resource "aws_instance" "vpc" { } module "child" { source = "./child" vpc_id = "${aws_instance.vpc.id}" } + +output "out" { + value = "${module.child.modout}" +}