From b1532c0f0441a658c4edef624a34d9e1a36dd27b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 6 Apr 2020 12:52:25 -0400 Subject: [PATCH 1/3] make the module closer referenceable This is all that is required to make module reference ordering work during apply, by adding and edge to the nodeCloseModule node, which will be the last node evaluated in the module. --- terraform/node_module_expand.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/terraform/node_module_expand.go b/terraform/node_module_expand.go index f77989890..e159f7c3b 100644 --- a/terraform/node_module_expand.go +++ b/terraform/node_module_expand.go @@ -99,6 +99,23 @@ type nodeCloseModule struct { Addr addrs.Module } +var ( + _ graphNodeModuleCloser = (*nodeCloseModule)(nil) + _ GraphNodeReferenceable = (*nodeCloseModule)(nil) +) + +func (n *nodeCloseModule) ModulePath() addrs.Module { + mod, _ := n.Addr.Call() + return mod +} + +func (n *nodeCloseModule) ReferenceableAddrs() []addrs.Referenceable { + _, call := n.Addr.Call() + return []addrs.Referenceable{ + call, + } +} + func (n *nodeCloseModule) Name() string { if len(n.Addr) == 0 { return "root" From 700e20de5d2215131d97c44b719fd8912502d0da Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 6 Apr 2020 15:21:15 -0400 Subject: [PATCH 2/3] connect references to the module closer NodeModuleRemoved is redundant now with the concept of nodeCloseModule, so we can replace it within the graph. This does mean that nodeCloseModule needs to know if it's evaluating an orphaned module that can't be expanded, but the overhead to checking this isn't too bad. Now that nodeModuleClose is referenceable, and we can ensure it's always in the graph at the correct time, we can eliminate the need to connect each resource to every single node within a module it references, and instead connect only to the nodeModuleClose, which acts as the module root. Since module expansion can cause exponential growth in the number of edges in graphs, this will help with performance problems when transforming and reducing these graphs by eliminating the bulk of redundant edges. This will also help with general debugging, making the graphs easier to read. --- terraform/node_module_expand.go | 33 ++++++--- terraform/node_module_removed.go | 97 ------------------------- terraform/transform_module_expansion.go | 41 ++++++++--- terraform/transform_reference.go | 11 --- terraform/transform_removed_modules.go | 16 +++- 5 files changed, 67 insertions(+), 131 deletions(-) delete mode 100644 terraform/node_module_removed.go diff --git a/terraform/node_module_expand.go b/terraform/node_module_expand.go index e159f7c3b..a0669b7d4 100644 --- a/terraform/node_module_expand.go +++ b/terraform/node_module_expand.go @@ -97,6 +97,10 @@ 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 ( @@ -140,7 +144,8 @@ func (n *nodeCloseModule) EvalTree() EvalNode { &EvalOpFilter{ Ops: []walkOperation{walkApply, walkDestroy}, Node: &evalCloseModule{ - Addr: n.Addr, + Addr: n.Addr, + orphaned: n.orphaned, }, }, }, @@ -148,7 +153,8 @@ func (n *nodeCloseModule) EvalTree() EvalNode { } type evalCloseModule struct { - Addr addrs.Module + Addr addrs.Module + orphaned bool } func (n *evalCloseModule) Eval(ctx EvalContext) (interface{}, error) { @@ -158,7 +164,11 @@ func (n *evalCloseModule) Eval(ctx EvalContext) (interface{}, error) { defer ctx.State().Unlock() expander := ctx.InstanceExpander() - currentModuleInstances := expander.ExpandModule(n.Addr) + var currentModuleInstances []addrs.ModuleInstance + // we can't expand if we're just removing + if !n.orphaned { + currentModuleInstances = expander.ExpandModule(n.Addr) + } for modKey, mod := range state.Modules { if !n.Addr.Equal(mod.Addr.Module()) { @@ -172,13 +182,18 @@ func (n *evalCloseModule) Eval(ctx EvalContext) (interface{}, error) { } } - // if this instance is not in the current expansion, remove it from the - // state found := false - for _, current := range currentModuleInstances { - if current.Equal(mod.Addr) { - found = true - break + 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 + } } } diff --git a/terraform/node_module_removed.go b/terraform/node_module_removed.go deleted file mode 100644 index b6ea00bff..000000000 --- a/terraform/node_module_removed.go +++ /dev/null @@ -1,97 +0,0 @@ -package terraform - -import ( - "fmt" - - "github.com/hashicorp/terraform/addrs" -) - -// NodeModuleRemoved represents a module that is no longer in the -// config. -type NodeModuleRemoved struct { - Addr addrs.ModuleInstance -} - -var ( - _ GraphNodeModuleInstance = (*NodeModuleRemoved)(nil) - _ RemovableIfNotTargeted = (*NodeModuleRemoved)(nil) - _ GraphNodeEvalable = (*NodeModuleRemoved)(nil) - _ GraphNodeReferencer = (*NodeModuleRemoved)(nil) - _ GraphNodeReferenceOutside = (*NodeModuleRemoved)(nil) -) - -func (n *NodeModuleRemoved) Name() string { - return fmt.Sprintf("%s (removed)", n.Addr.String()) -} - -// GraphNodeModuleInstance -func (n *NodeModuleRemoved) Path() addrs.ModuleInstance { - return n.Addr -} - -// GraphNodeModulePath implementation -func (n *NodeModuleRemoved) ModulePath() addrs.Module { - // This node represents the module call within a module, - // so return the CallerAddr as the path, as the module - // call may expand into multiple child instances - return n.Addr.Module() -} - -// GraphNodeEvalable -func (n *NodeModuleRemoved) EvalTree() EvalNode { - return &EvalOpFilter{ - Ops: []walkOperation{walkRefresh, walkApply, walkDestroy}, - Node: &EvalCheckModuleRemoved{ - Addr: n.Addr, - }, - } -} - -func (n *NodeModuleRemoved) ReferenceOutside() (selfPath, referencePath addrs.Module) { - // Our "References" implementation indicates that this node depends on - // the call to the module it represents, which implicitly depends on - // everything inside the module. That reference must therefore be - // interpreted in terms of our parent module. - return n.Addr.Module(), n.Addr.Parent().Module() -} - -func (n *NodeModuleRemoved) References() []*addrs.Reference { - // We depend on the call to the module we represent, because that - // implicitly then depends on everything inside that module. - // Our ReferenceOutside implementation causes this to be interpreted - // within the parent module. - - _, call := n.Addr.CallInstance() - return []*addrs.Reference{ - { - Subject: call, - - // No source range here, because there's nothing reasonable for - // us to return. - }, - } -} - -// RemovableIfNotTargeted -func (n *NodeModuleRemoved) RemoveIfNotTargeted() bool { - // We need to add this so that this node will be removed if - // it isn't targeted or a dependency of a target. - return true -} - -// EvalCheckModuleRemoved is an EvalNode implementation that verifies that -// a module has been removed from the state as expected. -type EvalCheckModuleRemoved struct { - Addr addrs.ModuleInstance -} - -func (n *EvalCheckModuleRemoved) Eval(ctx EvalContext) (interface{}, error) { - mod := ctx.State().Module(n.Addr) - if mod != nil { - // If we get here then that indicates a bug either in the states - // module or in an earlier step of the graph walk, since we should've - // pruned out the module when the last resource was removed from it. - return nil, fmt.Errorf("leftover module %s in state that should have been removed; this is a bug in Terraform and should be reported", n.Addr) - } - return nil, nil -} diff --git a/terraform/transform_module_expansion.go b/terraform/transform_module_expansion.go index 4e688718f..f26bc8385 100644 --- a/terraform/transform_module_expansion.go +++ b/terraform/transform_module_expansion.go @@ -21,9 +21,12 @@ type ModuleExpansionTransformer struct { // Concrete allows injection of a wrapped module node by the graph builder // to alter the evaluation behavior. Concrete ConcreteModuleNodeFunc + + closers map[string]*nodeCloseModule } func (t *ModuleExpansionTransformer) Transform(g *Graph) error { + t.closers = make(map[string]*nodeCloseModule) // The root module is always a singleton and so does not need expansion // processing, but any descendent modules do. We'll process them // recursively using t.transform. @@ -33,6 +36,26 @@ func (t *ModuleExpansionTransformer) Transform(g *Graph) error { return err } } + + // Now go through and connect all nodes to their respective module closers. + // This is done all at once here, because orphaned modules were already + // handled by the RemovedModuleTransformer, and those module closers are in + // the graph already, and need to be connected to their parent closers. + for _, v := range g.Vertices() { + // any node that executes within the scope of a module should be a + // GraphNodeModulePath + pather, ok := v.(GraphNodeModulePath) + if !ok { + continue + } + if closer, ok := t.closers[pather.ModulePath().String()]; ok { + // The module root depends on each child resource instance, since + // during apply the module expansion will complete before the + // individual instances are applied. + g.Connect(dag.BasicEdge(closer, v)) + } + } + return nil } @@ -58,16 +81,15 @@ func (t *ModuleExpansionTransformer) transform(g *Graph, c *configs.Config, pare g.Connect(dag.BasicEdge(v, parentNode)) } - // Add the root module node to provide a single exit point for the expanded - // module. - moduleRoot := &nodeCloseModule{ + // Add the closer (which acts as the root module node) to provide a + // single exit point for the expanded module. + closer := &nodeCloseModule{ Addr: c.Path, } - g.Add(moduleRoot) - g.Connect(dag.BasicEdge(moduleRoot, v)) + g.Add(closer) + g.Connect(dag.BasicEdge(closer, v)) + t.closers[c.Path.String()] = closer - // Connect any node that reports this module as its Path to ensure that - // the module expansion will be handled before that node. for _, childV := range g.Vertices() { pather, ok := childV.(GraphNodeModulePath) if !ok { @@ -76,11 +98,6 @@ func (t *ModuleExpansionTransformer) transform(g *Graph, c *configs.Config, pare if pather.ModulePath().Equal(c.Path) { log.Printf("[TRACE] ModuleExpansionTransformer: %s must wait for expansion of %s", dag.VertexName(childV), c.Path) g.Connect(dag.BasicEdge(childV, v)) - - // The module root also depends on each child instance, since - // during apply the module expansion will complete before the - // individual instances are applied. - g.Connect(dag.BasicEdge(moduleRoot, childV)) } } diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index d92783a32..74df6628a 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -365,17 +365,6 @@ func NewReferenceMap(vs []dag.Vertex) *ReferenceMap { key := m.mapKey(path, addr) vertices[key] = append(vertices[key], v) } - - // Any node can be referenced by the address of the module it belongs - // to or any of that module's ancestors. - for _, addr := range path.Ancestors()[1:] { - // Can be referenced either as the specific call instance (with - // an instance key) or as the bare module call itself (the "module" - // block in the parent module that created the instance). - callPath, call := addr.Call() - callKey := m.mapKey(callPath, call) - vertices[callKey] = append(vertices[callKey], v) - } } m.vertices = vertices diff --git a/terraform/transform_removed_modules.go b/terraform/transform_removed_modules.go index ee71387e2..7b2464790 100644 --- a/terraform/transform_removed_modules.go +++ b/terraform/transform_removed_modules.go @@ -3,6 +3,7 @@ package terraform import ( "log" + "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" "github.com/hashicorp/terraform/states" ) @@ -20,14 +21,25 @@ func (t *RemovedModuleTransformer) Transform(g *Graph) error { return nil } + removed := map[string]addrs.Module{} + for _, m := range t.State.Modules { cc := t.Config.DescendentForInstance(m.Addr) if cc != nil { continue } - + removed[m.Addr.Module().String()] = m.Addr.Module() log.Printf("[DEBUG] %s is no longer in configuration\n", m.Addr) - g.Add(&NodeModuleRemoved{Addr: m.Addr}) } + + // add closers to collect any module instances we're removing + for _, modAddr := range removed { + closer := &nodeCloseModule{ + Addr: modAddr, + orphaned: true, + } + g.Add(closer) + } + return nil } From 43d93b20368a0697eff60de37d9475f7db79a7a9 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 6 Apr 2020 15:30:23 -0400 Subject: [PATCH 3/3] remove excess logging from dag The dag Update messages were not particularly helpful in the debugging of terraform, and since we're going to be relying on DynamicExpand to an even greater extent, this will eliminate a log of extra log output. --- dag/walk.go | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/dag/walk.go b/dag/walk.go index 8fb60f23a..d7b202d71 100644 --- a/dag/walk.go +++ b/dag/walk.go @@ -148,7 +148,6 @@ func (w *Walker) Wait() tfdiags.Diagnostics { // Multiple Updates can be called in parallel. Update can be called at any // time during a walk. func (w *Walker) Update(g *AcyclicGraph) { - log.Print("[TRACE] dag/walk: updating graph") w.init() v := make(Set) e := make(Set) @@ -181,7 +180,6 @@ func (w *Walker) Update(g *AcyclicGraph) { w.wait.Add(1) // Add to our own set so we know about it already - log.Printf("[TRACE] dag/walk: added new vertex: %q", VertexName(v)) w.vertices.Add(raw) // Initialize the vertex info @@ -212,8 +210,6 @@ func (w *Walker) Update(g *AcyclicGraph) { // Delete it out of the map delete(w.vertexMap, v) - - log.Printf("[TRACE] dag/walk: removed vertex: %q", VertexName(v)) w.vertices.Delete(raw) } @@ -242,10 +238,6 @@ func (w *Walker) Update(g *AcyclicGraph) { // Record that the deps changed for this waiter changedDeps.Add(waiter) - - log.Printf( - "[TRACE] dag/walk: added edge: %q waiting on %q", - VertexName(waiter), VertexName(dep)) w.edges.Add(raw) } @@ -266,10 +258,6 @@ func (w *Walker) Update(g *AcyclicGraph) { // Record that the deps changed for this waiter changedDeps.Add(waiter) - - log.Printf( - "[TRACE] dag/walk: removed edge: %q waiting on %q", - VertexName(waiter), VertexName(dep)) w.edges.Delete(raw) } @@ -310,10 +298,6 @@ func (w *Walker) Update(g *AcyclicGraph) { } info.depsCancelCh = cancelCh - log.Printf( - "[TRACE] dag/walk: dependencies changed for %q, sending new deps", - VertexName(v)) - // Start the waiter go w.waitDeps(v, deps, doneCh, cancelCh) }