From c638252210a894985a822dc13e6cfa343602ec30 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 20 May 2020 13:45:31 -0400 Subject: [PATCH] pruneUnusedNodesTransformer Create a single transformer to remove all unused nodes from the apply graph. This is similar to the combination of the resource pruning done in the destroy edge transformer, and the unused values transformer. In addition to resources, variables, locals, and outputs, we now need to remove unused module expansion nodes as well. Since these can all be interdependent, we need to process them as whole in a single transformation. --- terraform/graph_builder_apply.go | 7 +- terraform/node_module_expand.go | 59 +-------- terraform/transform_destroy_edge.go | 163 +++++++++++++----------- terraform/transform_module_expansion.go | 2 +- terraform/transform_removed_modules.go | 3 +- 5 files changed, 98 insertions(+), 136 deletions(-) diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 3736fd3d7..f40827859 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -163,9 +163,10 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { // Create a destroy node for outputs to remove them from the state. &DestroyOutputTransformer{Destroy: b.Destroy}, - // Prune unreferenced values, which may have interpolations that can't - // be resolved. - &pruneUnusedExpanderTransformer{}, + // We need to remove configuration nodes that are not used at all, as + // they may not be able to evaluate, especially during destroy. + // These include variables, locals, and instance expanders. + &pruneUnusedNodesTransformer{}, // Add the node to fix the state count boundaries &CountBoundaryTransformer{ diff --git a/terraform/node_module_expand.go b/terraform/node_module_expand.go index 7f9f3c3b9..5168508ee 100644 --- a/terraform/node_module_expand.go +++ b/terraform/node_module_expand.go @@ -1,7 +1,6 @@ package terraform import ( - "fmt" "log" "github.com/hashicorp/terraform/addrs" @@ -10,12 +9,6 @@ import ( "github.com/hashicorp/terraform/lang" ) -// graphNodeModuleCloser is an interface implemented by nodes that finalize the -// evaluation of modules. -type graphNodeModuleCloser interface { - CloseModule() addrs.Module -} - type ConcreteModuleNodeFunc func(n *nodeExpandModule) dag.Vertex // nodeExpandModule represents a module call in the configuration that @@ -126,14 +119,9 @@ func (n *nodeExpandModule) EvalTree() EvalNode { // empty resources and modules from the state. type nodeCloseModule struct { Addr addrs.Module - - // orphaned indicates that this module has no expansion, because it no - // longer exists in the configuration - orphaned bool } var ( - _ graphNodeModuleCloser = (*nodeCloseModule)(nil) _ GraphNodeReferenceable = (*nodeCloseModule)(nil) _ GraphNodeReferenceOutside = (*nodeCloseModule)(nil) ) @@ -160,10 +148,6 @@ func (n *nodeCloseModule) Name() string { return n.Addr.String() + " (close)" } -func (n *nodeCloseModule) CloseModule() addrs.Module { - return n.Addr -} - // RemovableIfNotTargeted implementation func (n *nodeCloseModule) RemoveIfNotTargeted() bool { // We need to add this so that this node will be removed if @@ -177,8 +161,7 @@ func (n *nodeCloseModule) EvalTree() EvalNode { &EvalOpFilter{ Ops: []walkOperation{walkApply, walkDestroy}, Node: &evalCloseModule{ - Addr: n.Addr, - orphaned: n.orphaned, + Addr: n.Addr, }, }, }, @@ -186,8 +169,7 @@ func (n *nodeCloseModule) EvalTree() EvalNode { } type evalCloseModule struct { - Addr addrs.Module - orphaned bool + Addr addrs.Module } func (n *evalCloseModule) Eval(ctx EvalContext) (interface{}, error) { @@ -196,20 +178,6 @@ func (n *evalCloseModule) Eval(ctx EvalContext) (interface{}, error) { state := ctx.State().Lock() defer ctx.State().Unlock() - expander := ctx.InstanceExpander() - var currentModuleInstances []addrs.ModuleInstance - // we can't expand if we're just removing - if !n.orphaned { - func() { - // FIXME: we need to "turn off" closers if their expander has been removed - defer func() { - recover() - n.orphaned = true - }() - currentModuleInstances = expander.ExpandModule(n.Addr) - }() - } - for modKey, mod := range state.Modules { if !n.Addr.Equal(mod.Addr.Module()) { continue @@ -222,27 +190,8 @@ func (n *evalCloseModule) Eval(ctx EvalContext) (interface{}, error) { } } - found := false - if n.orphaned { - // we're removing the entire module, so all instances must go - found = true - } else { - // if this instance is not in the current expansion, remove it from - // the state - for _, current := range currentModuleInstances { - if current.Equal(mod.Addr) { - found = true - break - } - } - } - - if !found { - if len(mod.Resources) > 0 { - // FIXME: add more info to this error - return nil, fmt.Errorf("module %q still contains resources in state", mod.Addr) - } - + // empty child modules are always removed + if len(mod.Resources) == 0 && !mod.Addr.IsRoot() { delete(state.Modules, modKey) } } diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index 7f54f0c3e..0d9a06ddf 100644 --- a/terraform/transform_destroy_edge.go +++ b/terraform/transform_destroy_edge.go @@ -167,74 +167,87 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { return nil } -// Nodes that register instances in the instances.Expander are not needed -// during apply if there are no instances that will lookup the expansion. This -// is the case when a module tree is removed or during a full destroy, and we -// may not be able to evaluate the expansion expression. -type pruneUnusedExpanderTransformer struct { +// Remove any nodes that aren't needed when destroying modules. +// Variables, outputs, locals, and expanders may not be able to evaluate +// correctly, so we can remove these if nothing depends on them. The module +// closers also need to disable their use of expansion if the module itself is +// no longer present. +type pruneUnusedNodesTransformer struct { } -func (t *pruneUnusedExpanderTransformer) Transform(g *Graph) error { - // We need a reverse depth first walk of modules, but it needs to be - // recursive so that we can process the lead modules first. +func (t *pruneUnusedNodesTransformer) Transform(g *Graph) error { + // We need a reverse depth first walk of modules, processing them in order + // from the leaf modules to the root. This allows us to remove unneeded + // dependencies from child modules, freeing up nodes in the parent module + // to also be removed. - // collect all nodes into their containing module - type mod struct { - addr addrs.Module - nodes []dag.Vertex - } - - // first collect the nodes into their respective modules - moduleMap := make(map[string]*mod) + // First collect the nodes into their respective modules based on + // configuration path. + moduleMap := make(map[string]pruneUnusedNodesMod) for _, v := range g.Vertices() { var path addrs.Module switch v := v.(type) { case instanceExpander: path = v.expandsInstances() - case graphNodeModuleCloser: - // module closers are connected like module calls, and report - // their parent module address - path = v.CloseModule() - case GraphNodeModulePath: path = v.ModulePath() + default: + continue } - m, ok := moduleMap[path.String()] - if !ok { - m = &mod{} - moduleMap[path.String()] = m - } - + m := moduleMap[path.String()] m.addr = path m.nodes = append(m.nodes, v) + + // We need to keep track of the closers, to make sure they don't look + // for an expansion if there's nothing being expanded. + if c, ok := v.(*nodeCloseModule); ok { + m.closer = c + } + moduleMap[path.String()] = m } // now we need to restructure the modules so we can sort them - var modules []*mod + var modules []pruneUnusedNodesMod for _, mod := range moduleMap { modules = append(modules, mod) } - // Sort them by path length, longest first, so that we process the deepest - // modules first. The order of modules at the same tree level doesn't - // matter, we just need to ensure that child modules are processed before - // parent modules. + // Sort them by path length, longest first, so that start with the deepest + // modules. The order of modules at the same tree level doesn't matter, we + // just need to ensure that child modules are processed before parent + // modules. sort.Slice(modules, func(i, j int) bool { return len(modules[i].addr) > len(modules[j].addr) }) - for _, module := range modules { - t.removeUnused(module.nodes, g) + for _, mod := range modules { + mod.removeUnused(g) } return nil } -func (t *pruneUnusedExpanderTransformer) removeUnused(nodes []dag.Vertex, g *Graph) { +// pruneUnusedNodesMod is a container to hold the nodes that belong to a +// particular configuration module for the pruneUnusedNodesTransformer +type pruneUnusedNodesMod struct { + addr addrs.Module + nodes []dag.Vertex + closer *nodeCloseModule +} + +// Remove any unused locals, variables, outputs and expanders. Since module +// closers can also lookup expansion info to detect orphaned instances, disable +// them if their associated expander is removed. +func (m *pruneUnusedNodesMod) removeUnused(g *Graph) { + // We modify the nodes slice during processing here. + // Make a copy so no one is surprised by this changing in the future. + nodes := make([]dag.Vertex, len(m.nodes)) + copy(nodes, m.nodes) + // since we have no defined structure within the module, just cycle through - // the nodes until there are no more removals + // the nodes in each module until there are no more removals removed := true for { if !removed { @@ -242,51 +255,51 @@ func (t *pruneUnusedExpanderTransformer) removeUnused(nodes []dag.Vertex, g *Gra } removed = false - last := len(nodes) - 1 - - NEXT: for i := 0; i < len(nodes); i++ { - n := nodes[i] - switch n.(type) { - case graphNodeTemporaryValue: - if n, ok := n.(GraphNodeModulePath); ok { - // root outputs always have a dependency on remote state - if n.ModulePath().IsRoot() { - continue NEXT + // run this in a closure, so we can return early rather than + // dealing with complex looping and labels + func() { + n := nodes[i] + switch n.(type) { + case graphNodeTemporaryValue: + // temporary value, which consist of variables, locals, and + // outputs, must be kept if anything refers to them. + if n, ok := n.(GraphNodeModulePath); ok { + // root outputs always have an implicit dependency on + // remote state. + if n.ModulePath().IsRoot() { + return + } } - } - for _, vv := range g.UpEdges(n) { - if _, ok := vv.(GraphNodeReferencer); ok { - continue NEXT + for _, vv := range g.UpEdges(n) { + // keep any value which is connected through a + // reference + if _, ok := vv.(GraphNodeReferencer); ok { + return + } } + + case instanceExpander: + // Any nodes that expand instances are kept when their + // instances may need to be evaluated. + for _, vv := range g.UpEdges(n) { + if _, ok := vv.(requiresInstanceExpansion); ok { + return + } + } + + default: + return } - case instanceExpander: - for _, vv := range g.UpEdges(n) { - if _, ok := vv.(requiresInstanceExpansion); ok { - continue NEXT - } - } + g.Remove(n) + removed = true - default: - continue NEXT - } - - removed = true - - //// connect through edges - //for _, d := range g.DownEdges(n) { - // for _, u := range g.UpEdges(n) { - // g.Connect(dag.BasicEdge(u, d)) - // } - //} - - g.Remove(n) - - // remove the node from our iteration as well - nodes[i], nodes[last] = nodes[last], nodes[i] - nodes = nodes[:last] - last-- + // remove the node from our iteration as well + last := len(nodes) - 1 + nodes[i], nodes[last] = nodes[last], nodes[i] + nodes = nodes[:last] + }() } } } diff --git a/terraform/transform_module_expansion.go b/terraform/transform_module_expansion.go index 8d41465b7..e6c961c32 100644 --- a/terraform/transform_module_expansion.go +++ b/terraform/transform_module_expansion.go @@ -43,7 +43,7 @@ func (t *ModuleExpansionTransformer) Transform(g *Graph) error { // the graph already, and need to be connected to their parent closers. for _, v := range g.Vertices() { // skip closers so they don't attach to themselves - if _, ok := v.(graphNodeModuleCloser); ok { + if _, ok := v.(*nodeCloseModule); ok { continue } // any node that executes within the scope of a module should be a diff --git a/terraform/transform_removed_modules.go b/terraform/transform_removed_modules.go index 7b2464790..7c354cdfe 100644 --- a/terraform/transform_removed_modules.go +++ b/terraform/transform_removed_modules.go @@ -35,8 +35,7 @@ func (t *RemovedModuleTransformer) Transform(g *Graph) error { // add closers to collect any module instances we're removing for _, modAddr := range removed { closer := &nodeCloseModule{ - Addr: modAddr, - orphaned: true, + Addr: modAddr, } g.Add(closer) }