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) 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") diff --git a/terraform/graph.go b/terraform/graph.go index 0e561c882..ff1e32ffb 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,32 +1650,30 @@ 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) - // 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 // 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) @@ -1727,13 +1727,70 @@ 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()] + } + } + } + + // Initialize a default state if not available if state == nil { state = &ResourceState{ Type: r.Type, } } - flags := FlagPrimary + // 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! + flags := n.Resource.Flags if len(state.Tainted) > 0 { flags |= FlagHasTainted } @@ -1743,6 +1800,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{ @@ -1763,6 +1821,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, @@ -1790,8 +1849,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 +1861,23 @@ 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.Flags&FlagOrphan != 0 || + 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 +1886,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