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

@@ -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 n.orphaned {
-			if current.Equal(mod.Addr) {
+			// we're removing the entire module, so all instances must go
-				found = true
+			found = true
-				break
+		} 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
+				}
 			}
 		}
 

terraform/node_module_removed.go

@@ -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
-}

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
+	// Add the closer (which acts as the root module node) to provide a
-	// module.
+	// single exit point for the expanded module.
-	moduleRoot := &nodeCloseModule{
+	closer := &nodeCloseModule{
 		Addr: c.Path,
 	}
-	g.Add(moduleRoot)
+	g.Add(closer)
-	g.Connect(dag.BasicEdge(moduleRoot, v))
+	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))
 		}
 	}
 

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

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
 }