Merge pull request #578 from hashicorp/f-sub-graph

Fixing dynamic expansion issues

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)
 

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")

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
+	// Do the initial expansion of the nodes, attaching diffs if
-	// nodes to create, return that.
+	// applicable
-	n.expand(g, count)
+	n.expand(g, count, n.Diff)
-
-	// Add in the diff if we have it
-	if n.Diff != nil {
-		if err := graphAddDiff(g, n.Diff); err != nil {
-			return nil, err
-		}
-	}
 
 	// Add all the variable dependencies
 	graphAddVariableDeps(g)
 
-	// If we're just expanding the apply, then filter those out and
+	// Filter the nodes depending on the expansion type
-	// return them now.
+	switch n.ExpandMode {
-	if n.ExpandMode == ResourceExpandApply {
+	case ResourceExpandApply:
-		return n.finalizeGraph(g, false)
+		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(
+// filterResources is used to remove resources from the sub-graph based
-	g *depgraph.Graph, destroy bool) (*depgraph.Graph, error) {
+// 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
+		if destroy {
-		// in that case.
+			if rn.Resource.Diff != nil && rn.Resource.Diff.Destroy {
-		if rn.Resource.Diff == nil {
-			if !destroy {
 				result = append(result, n)
 			}
-
 			continue
 		}
 
-		// If we are destroying, append it only if we care about destroys
+		if rn.Resource.Flags&FlagOrphan != 0 ||
-		if rn.Resource.Diff.Destroy {
+			rn.Resource.Diff == nil || !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 {
 			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 nil
-	return g, nil
 }
 
 // matchingPrefixes takes a resource type and a set of resource