Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flatten modules into main graph #1781

Merged
merged 34 commits into from
May 5, 2015
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
1152ff5
terraform: add variables as graph nodes (no eval yet)
mitchellh Apr 30, 2015
90cfade
terraform: naive flatten
mitchellh May 1, 2015
15ca84a
terraform: module dependencies in graph use full name (FOR THE FUTURE)
mitchellh May 1, 2015
5e767f6
terraform: move module to its own file
mitchellh May 1, 2015
12c30fe
terraform: start FlattenGraph impl.
mitchellh May 1, 2015
3bfef7c
terraform: wrap variable values and prefix dependencies
mitchellh May 1, 2015
e542d6e
terraform: test to verify config variables are variables
mitchellh May 1, 2015
dd14ce9
terraform: test that variable deps reach out to parent graph
mitchellh May 1, 2015
7e838df
terraform: Graph should overrride Remove to clear lookaside
mitchellh May 1, 2015
a0d9bc0
terraform: outputs connect properly
mitchellh May 1, 2015
f24a153
terraform: GraphNodeProxy
mitchellh May 1, 2015
7d28e98
terraform: proxy uses custom edge
mitchellh May 1, 2015
e544e1d
terraform: hook up the proxies
mitchellh May 1, 2015
9a54ddd
terraform: eval for variables
mitchellh May 1, 2015
f2e7f50
terraform: subpath context setting
mitchellh May 1, 2015
86d07d3
terraform: redo how flattening works
mitchellh May 1, 2015
416e7a2
terraform: missing provider should be flattenable
mitchellh May 1, 2015
94e1bab
terraform: fill in more flat interfaces
mitchellh May 1, 2015
c207bed
terraform: set variables in the proper location
mitchellh May 1, 2015
7fd432b
terraform: providers in flattened graphs should depend on the parent
mitchellh May 1, 2015
8f58367
terraform: missing providers need to do dependencies
mitchellh May 2, 2015
1f10dfe
terraform: remove test
mitchellh May 2, 2015
23b6acc
terraform: add module destroy node to graph
mitchellh May 2, 2015
bbb065d
terraform: add edge for missing providers
mitchellh May 2, 2015
542ddb8
terraform: disable destroy mode for flattened nodes
mitchellh May 2, 2015
6afc149
terraform: destroy transform must happen globally
mitchellh May 3, 2015
cc0c208
terraform: skip outputs for destroy graphs
mitchellh May 3, 2015
2f3f680
terraform: update comment
mitchellh May 4, 2015
13f3daa
terraform: update comment on interpolater stuff
mitchellh May 4, 2015
c3ce23c
core: failing test for a bad module cycle
phinze May 4, 2015
803348d
Merge pull request #1797 from hashicorp/b-flatten-modules-cycle
mitchellh May 5, 2015
6d4969f
terraform: run prune destroy on validate
mitchellh May 5, 2015
d503cc2
terraform: flattenable graphNodeMissingProvisioner
mitchellh May 5, 2015
8c34e9a
terraform: provisionedby prefixed
mitchellh May 5, 2015
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 82 additions & 0 deletions terraform/context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4053,6 +4053,88 @@ func TestContext2Apply_module(t *testing.T) {
}
}

func TestContext2Apply_moduleDestroyOrder(t *testing.T) {
m := testModule(t, "apply-module-destroy-order")
p := testProvider("aws")
p.DiffFn = testDiffFn

// Create a custom apply function to track the order they were destroyed
var order []string
var orderLock sync.Mutex
p.ApplyFn = func(
info *InstanceInfo,
is *InstanceState,
id *InstanceDiff) (*InstanceState, error) {
orderLock.Lock()
defer orderLock.Unlock()

order = append(order, is.ID)
return nil, nil
}

state := &State{
Modules: []*ModuleState{
&ModuleState{
Path: rootModulePath,
Resources: map[string]*ResourceState{
"aws_instance.b": &ResourceState{
Type: "aws_instance",
Primary: &InstanceState{
ID: "b",
},
},
},
},

&ModuleState{
Path: []string{"root", "child"},
Resources: map[string]*ResourceState{
"aws_instance.a": &ResourceState{
Type: "aws_instance",
Primary: &InstanceState{
ID: "a",
},
},
},
Outputs: map[string]string{
"a_output": "a",
},
},
},
}

ctx := testContext2(t, &ContextOpts{
Module: m,
Providers: map[string]ResourceProviderFactory{
"aws": testProviderFuncFixed(p),
},
State: state,
Destroy: true,
})

if _, err := ctx.Plan(); err != nil {
t.Fatalf("err: %s", err)
}

state, err := ctx.Apply()
if err != nil {
t.Fatalf("err: %s", err)
}

expected := []string{"b", "a"}
if !reflect.DeepEqual(order, expected) {
t.Fatalf("bad: %#v", order)
}

{
actual := strings.TrimSpace(state.String())
expected := strings.TrimSpace(testTerraformApplyModuleDestroyOrderStr)
if actual != expected {
t.Fatalf("bad: \n%s", actual)
}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love how the big overarching test comes first in the diff. 😄

"WE GON' MAKE THIS WORK!"

func TestContext2Apply_moduleVarResourceCount(t *testing.T) {
m := testModule(t, "apply-module-var-resource-count")
p := testProvider("aws")
Expand Down
8 changes: 4 additions & 4 deletions terraform/eval_context.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ type EvalContext interface {
// that is currently being acted upon.
Interpolate(*config.RawConfig, *Resource) (*ResourceConfig, error)

// SetVariables sets the variables for interpolation. These variables
// should not have a "var." prefix. For example: "var.foo" should be
// "foo" as the key.
SetVariables(map[string]string)
// SetVariables sets the variables for the module within
// this context with the name n. This function call is additive:
// the second parameter is merged with any previous call.
SetVariables(string, map[string]string)

// Diff returns the global diff as well as the lock that should
// be used to modify that diff.
Expand Down
20 changes: 18 additions & 2 deletions terraform/eval_context_builtin.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import (
type BuiltinEvalContext struct {
PathValue []string
Interpolater *Interpolater
InterpolaterVars map[string]map[string]string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love a quick high level explanation for the Interp vars stuff. I can see the pieces but it'd be helpful to hear the overall story.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added a comment on the new diff.

InterpolaterVarLock *sync.Mutex
Hooks []Hook
InputValue UIInput
Providers map[string]ResourceProviderFactory
Expand Down Expand Up @@ -237,9 +239,23 @@ func (ctx *BuiltinEvalContext) Path() []string {
return ctx.PathValue
}

func (ctx *BuiltinEvalContext) SetVariables(vs map[string]string) {
func (ctx *BuiltinEvalContext) SetVariables(n string, vs map[string]string) {
ctx.InterpolaterVarLock.Lock()
defer ctx.InterpolaterVarLock.Unlock()

path := make([]string, len(ctx.Path())+1)
copy(path, ctx.Path())
path[len(path)-1] = n
key := PathCacheKey(path)

vars := ctx.InterpolaterVars[key]
if vars == nil {
vars = make(map[string]string)
ctx.InterpolaterVars[key] = vars
}

for k, v := range vs {
ctx.Interpolater.Variables[k] = v
vars[k] = v
}
}

Expand Down
4 changes: 3 additions & 1 deletion terraform/eval_context_mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ type MockEvalContext struct {
PathPath []string

SetVariablesCalled bool
SetVariablesModule string
SetVariablesVariables map[string]string

DiffCalled bool
Expand Down Expand Up @@ -162,8 +163,9 @@ func (c *MockEvalContext) Path() []string {
return c.PathPath
}

func (c *MockEvalContext) SetVariables(vs map[string]string) {
func (c *MockEvalContext) SetVariables(n string, vs map[string]string) {
c.SetVariablesCalled = true
c.SetVariablesModule = n
c.SetVariablesVariables = vs
}

Expand Down
3 changes: 2 additions & 1 deletion terraform/eval_variable.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,13 @@ import (
// EvalSetVariables is an EvalNode implementation that sets the variables
// explicitly for interpolation later.
type EvalSetVariables struct {
Module *string
Variables map[string]string
}

// TODO: test
func (n *EvalSetVariables) Eval(ctx EvalContext) (interface{}, error) {
ctx.SetVariables(n.Variables)
ctx.SetVariables(*n.Module, n.Variables)
return nil, nil
}

Expand Down
44 changes: 40 additions & 4 deletions terraform/graph.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,33 @@ func (g *Graph) Add(v dag.Vertex) dag.Vertex {
return v
}

// Remove is the same as dag.Graph.Remove
func (g *Graph) Remove(v dag.Vertex) dag.Vertex {
g.once.Do(g.init)

// If this is a depend-able node, then remove the lookaside info
if dv, ok := v.(GraphNodeDependable); ok {
for _, n := range dv.DependableName() {
delete(g.dependableMap, n)
}
}

// Call upwards to remove it from the actual graph
return g.Graph.Remove(v)
}

// Replace is the same as dag.Graph.Replace
func (g *Graph) Replace(o, n dag.Vertex) bool {
// Go through and update our lookaside to point to the new vertex
for k, v := range g.dependableMap {
if v == o {
g.dependableMap[k] = n
}
}

return g.Graph.Replace(o, n)
}

// ConnectDependent connects a GraphNodeDependent to all of its
// GraphNodeDependables. It returns the list of dependents it was
// unable to connect to.
Expand Down Expand Up @@ -129,8 +156,8 @@ func (g *Graph) init() {

func (g *Graph) walk(walker GraphWalker) error {
// The callbacks for enter/exiting a graph
ctx := walker.EnterGraph(g)
defer walker.ExitGraph(g)
ctx := walker.EnterPath(g.Path)
defer walker.ExitPath(g.Path)

// Get the path for logs
path := strings.Join(ctx.Path(), ".")
Expand All @@ -143,6 +170,15 @@ func (g *Graph) walk(walker GraphWalker) error {
walker.EnterVertex(v)
defer func() { walker.ExitVertex(v, rerr) }()

// vertexCtx is the context that we use when evaluating. This
// is normally the context of our graph but can be overridden
// with a GraphNodeSubPath impl.
vertexCtx := ctx
if pn, ok := v.(GraphNodeSubPath); ok && len(pn.Path()) > 0 {
vertexCtx = walker.EnterPath(pn.Path())
defer walker.ExitPath(pn.Path())
}

// If the node is eval-able, then evaluate it.
if ev, ok := v.(GraphNodeEvalable); ok {
tree := ev.EvalTree()
Expand All @@ -155,7 +191,7 @@ func (g *Graph) walk(walker GraphWalker) error {
// then callback with the output.
log.Printf("[DEBUG] vertex %s.%s: evaluating", path, dag.VertexName(v))
tree = walker.EnterEvalTree(v, tree)
output, err := Eval(tree, ctx)
output, err := Eval(tree, vertexCtx)
if rerr = walker.ExitEvalTree(v, output, err); rerr != nil {
return
}
Expand All @@ -167,7 +203,7 @@ func (g *Graph) walk(walker GraphWalker) error {
"[DEBUG] vertex %s.%s: expanding/walking dynamic subgraph",
path,
dag.VertexName(v))
g, err := ev.DynamicExpand(ctx)
g, err := ev.DynamicExpand(vertexCtx)
if err != nil {
rerr = err
return
Expand Down
45 changes: 31 additions & 14 deletions terraform/graph_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ type BuiltinGraphBuilder struct {
// Build builds the graph according to the steps returned by Steps.
func (b *BuiltinGraphBuilder) Build(path []string) (*Graph, error) {
basic := &BasicGraphBuilder{
Steps: b.Steps(),
Steps: b.Steps(path),
Validate: b.Validate,
}

Expand All @@ -100,7 +100,7 @@ func (b *BuiltinGraphBuilder) Build(path []string) (*Graph, error) {

// Steps returns the ordered list of GraphTransformers that must be executed
// to build a complete graph.
func (b *BuiltinGraphBuilder) Steps() []GraphTransformer {
func (b *BuiltinGraphBuilder) Steps(path []string) []GraphTransformer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed this too for module targeting. I'll rebase on top of this.

steps := []GraphTransformer{
// Create all our resources from the configuration and state
&ConfigTransformer{Module: b.Root},
Expand Down Expand Up @@ -134,24 +134,41 @@ func (b *BuiltinGraphBuilder) Steps() []GraphTransformer {
},
},

// Flatten stuff
&FlattenTransformer{},

// Make sure all the connections that are proxies are connected through
&ProxyTransformer{},

// Optionally reduces the graph to a user-specified list of targets and
// their dependencies.
&TargetsTransformer{Targets: b.Targets, Destroy: b.Destroy},

// Create the destruction nodes
&DestroyTransformer{},
&CreateBeforeDestroyTransformer{},
b.conditional(&conditionalOpts{
If: func() bool { return !b.Verbose },
Then: &PruneDestroyTransformer{Diff: b.Diff, State: b.State},
}),

// Make sure we create one root
// Make sure we have a single root
&RootTransformer{},
}

// Perform the transitive reduction to make our graph a bit
// more sane if possible (it usually is possible).
&TransitiveReductionTransformer{},
// If we're on the root path, then we do a bunch of other stuff.
// We don't do the following for modules.
if len(path) <= 1 {
steps = append(steps,
// Create the destruction nodes
&DestroyTransformer{},
&CreateBeforeDestroyTransformer{},
b.conditional(&conditionalOpts{
If: func() bool { return !b.Verbose },
Then: &PruneDestroyTransformer{Diff: b.Diff, State: b.State},
}),

// Make sure we have a single root after the above changes.
// This is the 2nd root transformer. In practice this shouldn't
// actually matter as the RootTransformer is idempotent.
&RootTransformer{},

// Perform the transitive reduction to make our graph a bit
// more sane if possible (it usually is possible).
&TransitiveReductionTransformer{},
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah this is funky, but agreed with top-level description that it's better to land it working and then refactor.

}

// Remove nils
Expand Down
Loading