From 6550f564bf3ad8f2611c54681bdcb54b977baf76 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 17 Jul 2015 16:18:55 -0700 Subject: [PATCH 1/4] terraform: PruneNoopTransformer --- terraform/transform_noop.go | 100 +++++++++++++++++++++++++++++++ terraform/transform_noop_test.go | 54 +++++++++++++++++ 2 files changed, 154 insertions(+) create mode 100644 terraform/transform_noop.go create mode 100644 terraform/transform_noop_test.go diff --git a/terraform/transform_noop.go b/terraform/transform_noop.go new file mode 100644 index 000000000000..982f755ce495 --- /dev/null +++ b/terraform/transform_noop.go @@ -0,0 +1,100 @@ +package terraform + +import ( + "github.com/hashicorp/terraform/dag" +) + +// GraphNodeNoopPrunable can be implemented by nodes that can be +// pruned if they are noops. +type GraphNodeNoopPrunable interface { + Noop(*NoopOpts) bool +} + +// NoopOpts are the options available to determine if your node is a noop. +type NoopOpts struct { + Graph *Graph + Vertex dag.Vertex + Diff *ModuleDiff + State *ModuleState +} + +// PruneNoopTransformer is a graph transform that prunes nodes that +// consider themselves no-ops. This is done to both simplify the graph +// as well as to remove graph nodes that might otherwise cause problems +// during the graph run. Therefore, this transformer isn't completely +// an optimization step, and can instead be considered critical to +// Terraform operations. +// +// Example of the above case: variables for modules interpolate their values. +// Interpolation will fail on destruction (since attributes are being deleted), +// but variables shouldn't even eval if there is nothing that will consume +// the variable. Therefore, variables can note that they can be omitted +// safely in this case. +// +// The PruneNoopTransformer will prune nodes depth first, and will automatically +// create connect through the dependencies of pruned nodes. For example, +// if we have a graph A => B => C (A depends on B, etc.), and B decides to +// be removed, we'll still be left with A => C; the edge will be properly +// connected. +type PruneNoopTransformer struct { + Diff *Diff + State *State +} + +func (t *PruneNoopTransformer) Transform(g *Graph) error { + // Find the leaves. + leaves := make([]dag.Vertex, 0, 10) + for _, v := range g.Vertices() { + if g.DownEdges(v).Len() == 0 { + leaves = append(leaves, v) + } + } + + // Do a depth first walk from the leaves and remove things. + return g.ReverseDepthFirstWalk(leaves, func(v dag.Vertex, depth int) error { + // We need a prunable + pn, ok := v.(GraphNodeNoopPrunable) + if !ok { + return nil + } + + // Start building the noop opts + path := g.Path + if pn, ok := v.(GraphNodeSubPath); ok { + path = pn.Path() + } + + var modDiff *ModuleDiff + var modState *ModuleState + if t.Diff != nil { + modDiff = t.Diff.ModuleByPath(path) + } + if t.State != nil { + modState = t.State.ModuleByPath(path) + } + + // Determine if its a noop. If it isn't, just return + noop := pn.Noop(&NoopOpts{ + Graph: g, + Vertex: v, + Diff: modDiff, + State: modState, + }) + if !noop { + return nil + } + + // It is a noop! We first preserve edges. + up := g.UpEdges(v).List() + for _, downV := range g.DownEdges(v).List() { + for _, upV := range up { + g.Connect(dag.BasicEdge(upV, downV)) + } + } + + // Then remove it + g.Remove(v) + + return nil + }) +} diff --git a/terraform/transform_noop_test.go b/terraform/transform_noop_test.go new file mode 100644 index 000000000000..65db95fda15b --- /dev/null +++ b/terraform/transform_noop_test.go @@ -0,0 +1,54 @@ +package terraform + +import ( + "strings" + "testing" + + "github.com/hashicorp/terraform/dag" +) + +func TestPruneNoopTransformer(t *testing.T) { + g := Graph{Path: RootModulePath} + + a := &testGraphNodeNoop{NameValue: "A"} + b := &testGraphNodeNoop{NameValue: "B", Value: true} + c := &testGraphNodeNoop{NameValue: "C"} + + g.Add(a) + g.Add(b) + g.Add(c) + g.Connect(dag.BasicEdge(a, b)) + g.Connect(dag.BasicEdge(b, c)) + + { + tf := &PruneNoopTransformer{} + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(testTransformPruneNoopStr) + if actual != expected { + t.Fatalf("bad:\n\n%s", actual) + } +} + +const testTransformPruneNoopStr = ` +A + C +C +` + +type testGraphNodeNoop struct { + NameValue string + Value bool +} + +func (v *testGraphNodeNoop) Name() string { + return v.NameValue +} + +func (v *testGraphNodeNoop) Noop(*NoopOpts) bool { + return v.Value +} From 4d361c839eeb9697cd801a5c8198369625d6f3a6 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 17 Jul 2015 16:52:15 -0700 Subject: [PATCH 2/4] terraform: prune resources and variables --- terraform/context_apply_test.go | 43 +++++++++++++++++++ terraform/graph_builder.go | 3 ++ terraform/graph_config_node_resource.go | 32 ++++++++++++++ terraform/graph_config_node_variable.go | 22 ++++++++++ terraform/interpolate.go | 4 +- .../apply-destroy-computed/child/main.tf | 5 +++ .../apply-destroy-computed/main.tf | 6 +++ terraform/transform_noop.go | 21 +++++---- terraform/transform_root.go | 4 +- 9 files changed, 129 insertions(+), 11 deletions(-) create mode 100644 terraform/test-fixtures/apply-destroy-computed/child/main.tf create mode 100644 terraform/test-fixtures/apply-destroy-computed/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index a5e12e2d3eca..ac252abd2624 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -215,6 +215,49 @@ func TestContext2Apply_createBeforeDestroyUpdate(t *testing.T) { } } +func TestContext2Apply_destroyComputed(t *testing.T) { + m := testModule(t, "apply-destroy-computed") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + state := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Resources: map[string]*ResourceState{ + "aws_instance.foo": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "foo", + Attributes: map[string]string{ + "output": "value", + }, + }, + }, + }, + }, + }, + } + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + State: state, + Destroy: true, + }) + + if p, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } else { + t.Logf(p.String()) + } + + if _, err := ctx.Apply(); err != nil { + t.Fatalf("err: %s", err) + } +} + func TestContext2Apply_minimal(t *testing.T) { m := testModule(t, "apply-minimal") p := testProvider("aws") diff --git a/terraform/graph_builder.go b/terraform/graph_builder.go index 0ec3f8b20ae7..2190be15ec86 100644 --- a/terraform/graph_builder.go +++ b/terraform/graph_builder.go @@ -164,6 +164,9 @@ func (b *BuiltinGraphBuilder) Steps(path []string) []GraphTransformer { Then: &PruneDestroyTransformer{Diff: b.Diff, State: b.State}, }), + // Remove the noop nodes + &PruneNoopTransformer{Diff: b.Diff, State: b.State}, + // Insert nodes to close opened plugin connections &CloseProviderTransformer{}, &CloseProvisionerTransformer{}, diff --git a/terraform/graph_config_node_resource.go b/terraform/graph_config_node_resource.go index 2235f4925646..7ddb23826aa6 100644 --- a/terraform/graph_config_node_resource.go +++ b/terraform/graph_config_node_resource.go @@ -249,6 +249,38 @@ func (n *GraphNodeConfigResource) DestroyNode(mode GraphNodeDestroyMode) GraphNo return result } +// GraphNodeNoopPrunable +func (n *GraphNodeConfigResource) Noop(opts *NoopOpts) bool { + // We don't have any noop optimizations for destroy nodes yet + if n.DestroyMode != DestroyNone { + return false + } + + // If there is no diff, then we aren't a noop + if opts.Diff == nil || opts.Diff.Empty() { + return false + } + + // If we have no module diff, we're certainly a noop + if opts.ModDiff == nil || opts.ModDiff.Empty() { + return true + } + + // Grab the ID which is the prefix (in the case count > 0 at some point) + prefix := n.Resource.Id() + + // Go through the diff and if there are any with our name on it, keep us + found := false + for k, _ := range opts.ModDiff.Resources { + if strings.HasPrefix(k, prefix) { + found = true + break + } + } + + return !found +} + // Same as GraphNodeConfigResource, but for flattening type GraphNodeConfigResourceFlat struct { *GraphNodeConfigResource diff --git a/terraform/graph_config_node_variable.go b/terraform/graph_config_node_variable.go index 91f126c49046..9b3d77dbc4b1 100644 --- a/terraform/graph_config_node_variable.go +++ b/terraform/graph_config_node_variable.go @@ -75,6 +75,28 @@ func (n *GraphNodeConfigVariable) DestroyEdgeInclude(v dag.Vertex) bool { return false } +// GraphNodeNoopPrunable +func (n *GraphNodeConfigVariable) Noop(opts *NoopOpts) bool { + // 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() { + return false + } + + 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 { + continue + } + + return false + } + + return true +} + // GraphNodeProxy impl. func (n *GraphNodeConfigVariable) Proxy() bool { return true diff --git a/terraform/interpolate.go b/terraform/interpolate.go index 40a4645f85da..6d103cd80289 100644 --- a/terraform/interpolate.go +++ b/terraform/interpolate.go @@ -370,7 +370,7 @@ MISSING: // be unknown. Instead, we return that the value is computed so // that the graph can continue to refresh other nodes. It doesn't // matter because the config isn't interpolated anyways. - if i.Operation == walkRefresh { + if i.Operation == walkRefresh || i.Operation == walkPlanDestroy { return config.UnknownVariableValue, nil } @@ -446,7 +446,7 @@ func (i *Interpolater) computeResourceMultiVariable( // be unknown. Instead, we return that the value is computed so // that the graph can continue to refresh other nodes. It doesn't // matter because the config isn't interpolated anyways. - if i.Operation == walkRefresh { + if i.Operation == walkRefresh || i.Operation == walkPlanDestroy { return config.UnknownVariableValue, nil } diff --git a/terraform/test-fixtures/apply-destroy-computed/child/main.tf b/terraform/test-fixtures/apply-destroy-computed/child/main.tf new file mode 100644 index 000000000000..5cd1f02b666c --- /dev/null +++ b/terraform/test-fixtures/apply-destroy-computed/child/main.tf @@ -0,0 +1,5 @@ +variable "value" {} + +resource "aws_instance" "bar" { + value = "${var.value}" +} diff --git a/terraform/test-fixtures/apply-destroy-computed/main.tf b/terraform/test-fixtures/apply-destroy-computed/main.tf new file mode 100644 index 000000000000..768c9680d801 --- /dev/null +++ b/terraform/test-fixtures/apply-destroy-computed/main.tf @@ -0,0 +1,6 @@ +resource "aws_instance" "foo" {} + +module "child" { + source = "./child" + value = "${aws_instance.foo.output}" +} diff --git a/terraform/transform_noop.go b/terraform/transform_noop.go index 982f755ce495..95964ea1d649 100644 --- a/terraform/transform_noop.go +++ b/terraform/transform_noop.go @@ -12,10 +12,12 @@ type GraphNodeNoopPrunable interface { // NoopOpts are the options available to determine if your node is a noop. type NoopOpts struct { - Graph *Graph - Vertex dag.Vertex - Diff *ModuleDiff - State *ModuleState + Graph *Graph + Vertex dag.Vertex + Diff *Diff + State *State + ModDiff *ModuleDiff + ModState *ModuleState } // PruneNoopTransformer is a graph transform that prunes nodes that @@ -52,6 +54,7 @@ func (t *PruneNoopTransformer) Transform(g *Graph) error { // Do a depth first walk from the leaves and remove things. return g.ReverseDepthFirstWalk(leaves, func(v dag.Vertex, depth int) error { + println("NAME: " + v.(dag.NamedVertex).Name()) // We need a prunable pn, ok := v.(GraphNodeNoopPrunable) if !ok { @@ -75,10 +78,12 @@ func (t *PruneNoopTransformer) Transform(g *Graph) error { // Determine if its a noop. If it isn't, just return noop := pn.Noop(&NoopOpts{ - Graph: g, - Vertex: v, - Diff: modDiff, - State: modState, + Graph: g, + Vertex: v, + Diff: t.Diff, + State: t.State, + ModDiff: modDiff, + ModState: modState, }) if !noop { return nil diff --git a/terraform/transform_root.go b/terraform/transform_root.go index 15de14754ae4..7a422b8264f3 100644 --- a/terraform/transform_root.go +++ b/terraform/transform_root.go @@ -2,6 +2,8 @@ package terraform import "github.com/hashicorp/terraform/dag" +const rootNodeName = "root" + // RootTransformer is a GraphTransformer that adds a root to the graph. type RootTransformer struct{} @@ -32,7 +34,7 @@ func (t *RootTransformer) Transform(g *Graph) error { type graphNodeRoot struct{} func (n graphNodeRoot) Name() string { - return "root" + return rootNodeName } func (n graphNodeRoot) Flatten(p []string) (dag.Vertex, error) { From 696d5ef94ffa48b8b0c81210dd98787502f8107d Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 20 Jul 2015 08:54:42 -0700 Subject: [PATCH 3/4] terraform: clarify comment --- terraform/graph_config_node_resource.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/terraform/graph_config_node_resource.go b/terraform/graph_config_node_resource.go index 7ddb23826aa6..dfc9587146b3 100644 --- a/terraform/graph_config_node_resource.go +++ b/terraform/graph_config_node_resource.go @@ -256,12 +256,15 @@ func (n *GraphNodeConfigResource) Noop(opts *NoopOpts) bool { return false } - // If there is no diff, then we aren't a noop + // 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() { return false } - // If we have no module diff, we're certainly a noop + // If we have no module diff, we're certainly a noop. This is because + // 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 } From 8d29f274c8130834c790a851dab9650a67f2625b Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 20 Jul 2015 08:55:57 -0700 Subject: [PATCH 4/4] terraform: remove print --- terraform/transform_noop.go | 1 - 1 file changed, 1 deletion(-) diff --git a/terraform/transform_noop.go b/terraform/transform_noop.go index 95964ea1d649..e36b619377ed 100644 --- a/terraform/transform_noop.go +++ b/terraform/transform_noop.go @@ -54,7 +54,6 @@ func (t *PruneNoopTransformer) Transform(g *Graph) error { // Do a depth first walk from the leaves and remove things. return g.ReverseDepthFirstWalk(leaves, func(v dag.Vertex, depth int) error { - println("NAME: " + v.(dag.NamedVertex).Name()) // We need a prunable pn, ok := v.(GraphNodeNoopPrunable) if !ok {