From a2ba45edf58398de4c233341a44aca914dd45c5f Mon Sep 17 00:00:00 2001 From: Armon Dadgar Date: Mon, 17 Nov 2014 15:50:26 -0800 Subject: [PATCH 1/5] terraform: Simplify sub-graph finalization --- terraform/graph.go | 61 +++++++++++++++++----------------------------- 1 file changed, 23 insertions(+), 38 deletions(-) diff --git a/terraform/graph.go b/terraform/graph.go index 0e561c882..ef4b55ee2 100644 --- a/terraform/graph.go +++ b/terraform/graph.go @@ -1662,13 +1662,18 @@ func (n *GraphNodeResource) Expand() (*depgraph.Graph, error) { // Add all the variable dependencies graphAddVariableDeps(g) - // If we're just expanding the apply, then filter those out and - // return them now. - if n.ExpandMode == ResourceExpandApply { - return n.finalizeGraph(g, false) + // Filter the nodes depending on the expansion type + switch n.ExpandMode { + case ResourceExpandApply: + n.filterResources(g, false) + case ResourceExpandDestroy: + n.filterResources(g, true) + default: + panic(fmt.Sprintf("Unhandled expansion mode %d", n.ExpandMode)) } - return n.finalizeGraph(g, true) + // Return the finalized graph + return g, n.finalizeGraph(g) } // expand expands this resource and adds the resources to the graph. It @@ -1790,8 +1795,11 @@ func (n *GraphNodeResource) copyResource(id string) *Resource { return &resource } -func (n *GraphNodeResource) finalizeGraph( - g *depgraph.Graph, destroy bool) (*depgraph.Graph, error) { +// filterResources is used to remove resources from the sub-graph based +// on the ExpandMode. This is because there is a Destroy sub-graph, and +// Apply sub-graph, and we cannot includes the same instances in both +// sub-graphs. +func (n *GraphNodeResource) filterResources(g *depgraph.Graph, destroy bool) { result := make([]*depgraph.Noun, 0, len(g.Nouns)) for _, n := range g.Nouns { rn, ok := n.Meta.(*GraphNodeResource) @@ -1799,44 +1807,22 @@ func (n *GraphNodeResource) finalizeGraph( continue } - // If the diff is nil, then we're not destroying, so append only - // in that case. - if rn.Resource.Diff == nil { - if !destroy { + if destroy { + if rn.Resource.Diff != nil && rn.Resource.Diff.Destroy { result = append(result, n) } - continue } - // If we are destroying, append it only if we care about destroys - if rn.Resource.Diff.Destroy { - if destroy { - result = append(result, n) - } - - continue - } - - // If this is an oprhan, we only care about it if we're destroying. - if rn.Resource.Flags&FlagOrphan != 0 { - if destroy { - result = append(result, n) - } - - continue - } - - // If we're not destroying, then add it only if we don't - // care about deploys. - if !destroy { + if rn.Resource.Diff == nil || !rn.Resource.Diff.Destroy { result = append(result, n) } } - - // Set the nouns to be only those we care about g.Nouns = result +} +// finalizeGraph is used to ensure the generated graph is valid +func (n *GraphNodeResource) finalizeGraph(g *depgraph.Graph) error { // Remove the dependencies that don't exist graphRemoveInvalidDeps(g) @@ -1845,10 +1831,9 @@ func (n *GraphNodeResource) finalizeGraph( // Validate if err := g.Validate(); err != nil { - return nil, err + return err } - - return g, nil + return nil } // matchingPrefixes takes a resource type and a set of resource From 507b75449f916f7d0c7682d2f25b963e4ba0c0f0 Mon Sep 17 00:00:00 2001 From: Armon Dadgar Date: Tue, 18 Nov 2014 15:10:18 -0800 Subject: [PATCH 2/5] terraform: Move diff handling during dynamic expansion --- terraform/graph.go | 63 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 50 insertions(+), 13 deletions(-) diff --git a/terraform/graph.go b/terraform/graph.go index ef4b55ee2..6762ed8fa 100644 --- a/terraform/graph.go +++ b/terraform/graph.go @@ -539,6 +539,8 @@ func graphAddDiff(g *depgraph.Graph, d *ModuleDiff) error { rn.Diff = d } + // If we are not expanding, then we assign the + // instance diff to the resource. var rd *InstanceDiff if rn.ExpandMode == ResourceExpandNone { rd = diffs[0] @@ -1648,16 +1650,9 @@ func (n *GraphNodeResource) Expand() (*depgraph.Graph, error) { ModulePath: n.Resource.Info.ModulePath, } - // Determine the nodes to create. If we're just looking for the - // nodes to create, return that. - n.expand(g, count) - - // Add in the diff if we have it - if n.Diff != nil { - if err := graphAddDiff(g, n.Diff); err != nil { - return nil, err - } - } + // Do the initial expansion of the nodes, attaching diffs if + // applicable + n.expand(g, count, n.Diff) // Add all the variable dependencies graphAddVariableDeps(g) @@ -1678,7 +1673,7 @@ func (n *GraphNodeResource) Expand() (*depgraph.Graph, error) { // expand expands this resource and adds the resources to the graph. It // adds both create and destroy resources. -func (n *GraphNodeResource) expand(g *depgraph.Graph, count int) { +func (n *GraphNodeResource) expand(g *depgraph.Graph, count int, diff *ModuleDiff) { // Create the list of nouns result := make([]*depgraph.Noun, 0, count) @@ -1732,13 +1727,52 @@ func (n *GraphNodeResource) expand(g *depgraph.Graph, count int) { } } + // Add in the diff if we have it + var inDiff *InstanceDiff + if diff != nil { + // Looup the instance diff + if d, ok := diff.Resources[name]; ok { + inDiff = d + } + + if inDiff == nil { + if count == 1 { + // If the count is one, check the state for ".0" + // appended, which might exist if we go from + // count > 1 to count == 1. + k := r.Id() + ".0" + inDiff = diff.Resources[k] + } else if i == 0 { + // If count is greater than one, check for state + // with just the ID, which might exist if we go + // from count == 1 to count > 1 + inDiff = diff.Resources[r.Id()] + } + } + } + if state == nil { state = &ResourceState{ Type: r.Type, } } - flags := FlagPrimary + if inDiff != nil && n.ExpandMode != ResourceExpandDestroy { + // Disable Destroy if we aren't doing a destroy expansion. + // There is a seperate expansion for the destruction action. + d := new(InstanceDiff) + *d = *inDiff + inDiff = d + inDiff.Destroy = false + } else if inDiff != nil && inDiff.Destroy && n.ExpandMode == ResourceExpandDestroy { + // If we are doing a destroy, make sure it is exclusively + // a destroy, since there is a seperate expansion for the apply + inDiff = new(InstanceDiff) + inDiff.Destroy = true + } + + // Inherit the existing flags! + flags := n.Resource.Flags if len(state.Tainted) > 0 { flags |= FlagHasTainted } @@ -1748,6 +1782,7 @@ func (n *GraphNodeResource) expand(g *depgraph.Graph, count int) { resource.CountIndex = i resource.State = state.Primary resource.Flags = flags + resource.Diff = inDiff // Add the result result = append(result, &depgraph.Noun{ @@ -1768,6 +1803,7 @@ func (n *GraphNodeResource) expand(g *depgraph.Graph, count int) { resource.Config = NewResourceConfig(nil) resource.State = rs.Primary resource.Flags = FlagOrphan + resource.Diff = &InstanceDiff{Destroy: true} noun := &depgraph.Noun{ Name: k, @@ -1814,7 +1850,8 @@ func (n *GraphNodeResource) filterResources(g *depgraph.Graph, destroy bool) { continue } - if rn.Resource.Diff == nil || !rn.Resource.Diff.Destroy { + if rn.Resource.Flags&FlagOrphan != 0 || + rn.Resource.Diff == nil || !rn.Resource.Diff.Destroy { result = append(result, n) } } From 01db1ff8bdeddc66917539f342c906109ef41ac0 Mon Sep 17 00:00:00 2001 From: Armon Dadgar Date: Tue, 18 Nov 2014 15:38:40 -0800 Subject: [PATCH 3/5] terraform: diff handler in expansion avoids duplicate destroy --- terraform/graph.go | 42 ++++++++++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/terraform/graph.go b/terraform/graph.go index 6762ed8fa..ff1e32ffb 100644 --- a/terraform/graph.go +++ b/terraform/graph.go @@ -1751,24 +1751,42 @@ func (n *GraphNodeResource) expand(g *depgraph.Graph, count int, diff *ModuleDif } } + // Initialize a default state if not available if state == nil { state = &ResourceState{ Type: r.Type, } } - if inDiff != nil && n.ExpandMode != ResourceExpandDestroy { - // Disable Destroy if we aren't doing a destroy expansion. - // There is a seperate expansion for the destruction action. - d := new(InstanceDiff) - *d = *inDiff - inDiff = d - inDiff.Destroy = false - } else if inDiff != nil && inDiff.Destroy && n.ExpandMode == ResourceExpandDestroy { - // If we are doing a destroy, make sure it is exclusively - // a destroy, since there is a seperate expansion for the apply - inDiff = new(InstanceDiff) - inDiff.Destroy = true + // Prepare the diff if it exists + if inDiff != nil { + switch n.ExpandMode { + case ResourceExpandApply: + // Disable Destroy if we aren't doing a destroy expansion. + // There is a seperate expansion for the destruction action. + d := new(InstanceDiff) + *d = *inDiff + inDiff = d + inDiff.Destroy = false + + // If we require a new resource, there is a seperate delete + // phase, so the create phase must not have access to the ID. + if inDiff.RequiresNew() { + s := new(ResourceState) + *s = *state + state = s + state.Primary = nil + } + + case ResourceExpandDestroy: + // If we are doing a destroy, make sure it is exclusively + // a destroy, since there is a seperate expansion for the apply + inDiff = new(InstanceDiff) + inDiff.Destroy = true + + default: + panic(fmt.Sprintf("Unhandled expansion mode %d", n.ExpandMode)) + } } // Inherit the existing flags! From e29e364c890402de0803643fce83a5677f499f4f Mon Sep 17 00:00:00 2001 From: Armon Dadgar Date: Tue, 18 Nov 2014 15:38:54 -0800 Subject: [PATCH 4/5] terraform: Testing duplicate delete issue --- terraform/context_test.go | 89 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 88 insertions(+), 1 deletion(-) diff --git a/terraform/context_test.go b/terraform/context_test.go index 5ad260763..dcfc69d3a 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -2656,7 +2656,7 @@ func TestContextApply_createBefore_depends(t *testing.T) { // Test that things were managed _in the right order_ order := h.States diffs := h.Diffs - if order[0].ID != "bar" || diffs[0].Destroy { + if order[0].ID != "" || diffs[0].Destroy { t.Fatalf("should create new instance first: %#v", order) } @@ -2669,6 +2669,93 @@ func TestContextApply_createBefore_depends(t *testing.T) { } } +func TestContextApply_singleDestroy(t *testing.T) { + m := testModule(t, "apply-depends-create-before") + h := new(HookRecordApplyOrder) + p := testProvider("aws") + + invokeCount := 0 + p.ApplyFn = func(info *InstanceInfo, s *InstanceState, d *InstanceDiff) (*InstanceState, error) { + invokeCount++ + switch invokeCount { + case 1: + if d.Destroy { + t.Fatalf("should not destroy") + } + if s.ID != "" { + t.Fatalf("should not have ID") + } + case 2: + if d.Destroy { + t.Fatalf("should not destroy") + } + if s.ID != "baz" { + t.Fatalf("should have id") + } + case 3: + if !d.Destroy { + t.Fatalf("should destroy") + } + if s.ID == "" { + t.Fatalf("should have ID") + } + default: + t.Fatalf("bad invoke count %d", invokeCount) + } + return testApplyFn(info, s, d) + } + p.DiffFn = testDiffFn + state := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Resources: map[string]*ResourceState{ + "aws_instance.web": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "bar", + Attributes: map[string]string{ + "require_new": "ami-old", + }, + }, + }, + "aws_instance.lb": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "baz", + Attributes: map[string]string{ + "instance": "bar", + }, + }, + }, + }, + }, + }, + } + ctx := testContext(t, &ContextOpts{ + Module: m, + Hooks: []Hook{h}, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + State: state, + }) + + if _, err := ctx.Plan(nil); err != nil { + t.Fatalf("err: %s", err) + } + + h.Active = true + state, err := ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + if invokeCount != 3 { + t.Fatalf("bad: %d", invokeCount) + } +} + func TestContextPlan(t *testing.T) { m := testModule(t, "plan-good") p := testProvider("aws") From 07fff364a5c7e878598ed71eb30b473926ff6e7d Mon Sep 17 00:00:00 2001 From: Armon Dadgar Date: Tue, 18 Nov 2014 15:46:59 -0800 Subject: [PATCH 5/5] CHANGELOG updates --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50b5a41c2..43e3de9ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,12 @@ ## 0.3.2 (unreleased) +BUG FIXES: + * core: Fixed issue causing double delete. [GH-555] + * core: Fixed issue with create-before-destroy not being respected in + some circumstances. + * core: Fixing issue with count expansion with non-homogenous instance + plans. ## 0.3.1 (October 21, 2014)