core: [refactor] store Deposed resource instances as a list

Deposed instances need to be stored as a list for certain pathological
cases where destroys fail for some reason (e.g. upstream API failure,
Terraform interrupted mid-run). Terraform needs to be able to remember
all Deposed nodes so that it can clean them up properly in subsequent
runs.

Deposed instances will now never touch the Tainted list - they're fully
managed from within their own list.

Added a "multiDepose" test case that walks through a scenario to
exercise this.

terraform/context_test.go

@@ -3845,6 +3845,136 @@ func TestContext2Apply_errorDestroy_createBeforeDestroy(t *testing.T) {
 	}
 }
 
+func TestContext2Apply_multiDepose_createBeforeDestroy(t *testing.T) {
+	m := testModule(t, "apply-multi-depose-create-before-destroy")
+	p := testProvider("aws")
+	p.DiffFn = testDiffFn
+	ps := map[string]ResourceProviderFactory{"aws": testProviderFuncFixed(p)}
+	state := &State{
+		Modules: []*ModuleState{
+			&ModuleState{
+				Path: rootModulePath,
+				Resources: map[string]*ResourceState{
+					"aws_instance.web": &ResourceState{
+						Type:    "aws_instance",
+						Primary: &InstanceState{ID: "foo"},
+					},
+				},
+			},
+		},
+	}
+
+	ctx := testContext2(t, &ContextOpts{
+		Module:    m,
+		Providers: ps,
+		State:     state,
+	})
+	createdInstanceId := "bar"
+	// Create works
+	createFunc := func(is *InstanceState) (*InstanceState, error) {
+		return &InstanceState{ID: createdInstanceId}, nil
+	}
+	// Destroy starts broken
+	destroyFunc := func(is *InstanceState) (*InstanceState, error) {
+		return is, fmt.Errorf("destroy failed")
+	}
+	p.ApplyFn = func(info *InstanceInfo, is *InstanceState, id *InstanceDiff) (*InstanceState, error) {
+		if id.Destroy {
+			return destroyFunc(is)
+		} else {
+			return createFunc(is)
+		}
+	}
+
+	if _, err := ctx.Plan(nil); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	// Destroy is broken, so even though CBD successfully replaces the instance,
+	// we'll have to save the Deposed instance to destroy later
+	state, err := ctx.Apply()
+	if err == nil {
+		t.Fatal("should have error")
+	}
+
+	checkStateString(t, state, `
+aws_instance.web: (1 deposed)
+  ID = bar
+  Deposed ID 1 = foo
+	`)
+
+	createdInstanceId = "baz"
+	ctx = testContext2(t, &ContextOpts{
+		Module:    m,
+		Providers: ps,
+		State:     state,
+	})
+
+	if _, err := ctx.Plan(nil); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	// We're replacing the primary instance once again. Destroy is _still_
+	// broken, so the Deposed list gets longer
+	state, err = ctx.Apply()
+	if err == nil {
+		t.Fatal("should have error")
+	}
+
+	checkStateString(t, state, `
+aws_instance.web: (2 deposed)
+  ID = baz
+  Deposed ID 1 = foo
+  Deposed ID 2 = bar
+	`)
+
+	// Destroy partially fixed!
+	destroyFunc = func(is *InstanceState) (*InstanceState, error) {
+		if is.ID == "foo" || is.ID == "baz" {
+			return nil, nil
+		} else {
+			return is, fmt.Errorf("destroy partially failed")
+		}
+	}
+
+	createdInstanceId = "qux"
+	if _, err := ctx.Plan(nil); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+	state, err = ctx.Apply()
+	// Expect error because 1/2 of Deposed destroys failed
+	if err == nil {
+		t.Fatal("should have error")
+	}
+
+	// foo and baz are now gone, bar sticks around
+	checkStateString(t, state, `
+aws_instance.web: (1 deposed)
+  ID = qux
+  Deposed ID 1 = bar
+	`)
+
+	// Destroy working fully!
+	destroyFunc = func(is *InstanceState) (*InstanceState, error) {
+		return nil, nil
+	}
+
+	createdInstanceId = "quux"
+	if _, err := ctx.Plan(nil); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+	state, err = ctx.Apply()
+	if err != nil {
+		t.Fatal("should not have error:", err)
+	}
+
+	// And finally the state is clean
+	checkStateString(t, state, `
+aws_instance.web:
+  ID = quux
+	`)
+}
+
 func TestContext2Apply_provisionerResourceRef(t *testing.T) {
 	m := testModule(t, "apply-provisioner-resource-ref")
 	p := testProvider("aws")
@@ -5343,6 +5473,15 @@ func testProvisioner() *MockResourceProvisioner {
 	return p
 }
 
+func checkStateString(t *testing.T, state *State, expected string) {
+	actual := strings.TrimSpace(state.String())
+	expected = strings.TrimSpace(expected)
+
+	if actual != expected {
+		t.Fatalf("state does not match! actual:\n%s\n\nexpected:\n%s", actual, expected)
+	}
+}
+
 const testContextGraph = `
 root: root
 aws_instance.bar

terraform/eval_error.go

@@ -12,5 +12,9 @@ type EvalReturnError struct {
 }
 
 func (n *EvalReturnError) Eval(ctx EvalContext) (interface{}, error) {
+	if n.Error == nil {
+		return nil, nil
+	}
+
 	return nil, *n.Error
 }

terraform/eval_state.go

@@ -25,13 +25,13 @@ type EvalReadStateTainted struct {
 
 	// Tainted is a per-resource list, this index determines which item in the
 	// list we are addressing
-	TaintedIndex int
+	Index int
 }
 
 func (n *EvalReadStateTainted) Eval(ctx EvalContext) (interface{}, error) {
 	return readInstanceFromState(ctx, n.Name, n.Output, func(rs *ResourceState) (*InstanceState, error) {
 		// Get the index. If it is negative, then we get the last one
-		idx := n.TaintedIndex
+		idx := n.Index
 		if idx < 0 {
 			idx = len(rs.Tainted) - 1
 		}
@@ -48,11 +48,21 @@ func (n *EvalReadStateTainted) Eval(ctx EvalContext) (interface{}, error) {
 type EvalReadStateDeposed struct {
 	Name   string
 	Output **InstanceState
+	Index  int
 }
 
 func (n *EvalReadStateDeposed) Eval(ctx EvalContext) (interface{}, error) {
 	return readInstanceFromState(ctx, n.Name, n.Output, func(rs *ResourceState) (*InstanceState, error) {
-		return rs.Deposed, nil
+		// Get the index. If it is negative, then we get the last one
+		idx := n.Index
+		if idx < 0 {
+			idx = len(rs.Deposed) - 1
+		}
+		if idx >= 0 && idx < len(rs.Deposed) {
+			return rs.Deposed[idx], nil
+		} else {
+			return nil, fmt.Errorf("bad deposed index: %d, for resource: %#v", idx, rs)
+		}
 	})
 }
 
@@ -183,7 +193,7 @@ type EvalWriteStateTainted struct {
 	ResourceType string
 	Dependencies []string
 	State        **InstanceState
-	TaintedIndex int
+	Index        int
 }
 
 func (n *EvalWriteStateTainted) Eval(ctx EvalContext) (interface{}, error) {
@@ -212,10 +222,10 @@ func (n *EvalWriteStateTainted) Eval(ctx EvalContext) (interface{}, error) {
 	rs.Type = n.ResourceType
 	rs.Dependencies = n.Dependencies
 
-	if n.TaintedIndex == -1 {
+	if n.Index == -1 {
 		rs.Tainted = append(rs.Tainted, *n.State)
 	} else {
-		rs.Tainted[n.TaintedIndex] = *n.State
+		rs.Tainted[n.Index] = *n.State
 	}
 
 	return nil, nil
@@ -226,6 +236,7 @@ type EvalWriteStateDeposed struct {
 	ResourceType string
 	Dependencies []string
 	State        **InstanceState
+	Index        int
 }
 
 func (n *EvalWriteStateDeposed) Eval(ctx EvalContext) (interface{}, error) {
@@ -254,7 +265,11 @@ func (n *EvalWriteStateDeposed) Eval(ctx EvalContext) (interface{}, error) {
 	rs.Type = n.ResourceType
 	rs.Dependencies = n.Dependencies
 
-	rs.Deposed = *n.State
+	if n.Index == -1 {
+		rs.Deposed = append(rs.Deposed, *n.State)
+	} else {
+		rs.Deposed[n.Index] = *n.State
+	}
 
 	return nil, nil
 }
@@ -324,7 +339,7 @@ func (n *EvalDeposeState) Eval(ctx EvalContext) (interface{}, error) {
 	}
 
 	// Depose
-	rs.Deposed = rs.Primary
+	rs.Deposed = append(rs.Deposed, rs.Primary)
 	rs.Primary = nil
 
 	return nil, nil
@@ -357,13 +372,13 @@ func (n *EvalUndeposeState) Eval(ctx EvalContext) (interface{}, error) {
 	}
 
 	// If we don't have any desposed resource, then we don't have anything to do
-	if rs.Deposed == nil {
+	if len(rs.Deposed) == 0 {
 		return nil, nil
 	}
 
 	// Undepose
-	rs.Primary = rs.Deposed
+	rs.Primary = rs.Deposed[len(rs.Deposed)-1]
-	rs.Deposed = nil
+	rs.Deposed = rs.Deposed[:len(rs.Deposed)-1]
 
 	return nil, nil
 }

terraform/eval_state_test.go

@@ -97,21 +97,24 @@ func TestEvalReadState(t *testing.T) {
 				},
 			},
 			Node: &EvalReadStateTainted{
-				Name:         "aws_instance.bar",
+				Name:   "aws_instance.bar",
-				Output:       &output,
+				Output: &output,
-				TaintedIndex: 0,
+				Index:  0,
 			},
 			ExpectedInstanceId: "i-abc123",
 		},
 		"ReadStateDeposed gets deposed instance": {
 			Resources: map[string]*ResourceState{
 				"aws_instance.bar": &ResourceState{
-					Deposed: &InstanceState{ID: "i-abc123"},
+					Deposed: []*InstanceState{
+						&InstanceState{ID: "i-abc123"},
+					},
 				},
 			},
 			Node: &EvalReadStateDeposed{
 				Name:   "aws_instance.bar",
 				Output: &output,
+				Index:  0,
 			},
 			ExpectedInstanceId: "i-abc123",
 		},
@@ -187,7 +190,7 @@ func TestEvalWriteStateTainted(t *testing.T) {
 		Name:         "restype.resname",
 		ResourceType: "restype",
 		State:        &is,
-		TaintedIndex: -1,
+		Index:        -1,
 	}
 	_, err := node.Eval(ctx)
 	if err != nil {
@@ -195,7 +198,7 @@ func TestEvalWriteStateTainted(t *testing.T) {
 	}
 
 	rs := state.ModuleByPath(ctx.Path()).Resources["restype.resname"]
-	if len(rs.Tainted) == 1 && rs.Tainted[0].ID != "i-abc123" {
+	if len(rs.Tainted) != 1 || rs.Tainted[0].ID != "i-abc123" {
 		t.Fatalf("expected tainted instance to have ID 'i-abc123': %#v", rs)
 	}
 }
@@ -212,6 +215,7 @@ func TestEvalWriteStateDeposed(t *testing.T) {
 		Name:         "restype.resname",
 		ResourceType: "restype",
 		State:        &is,
+		Index:        -1,
 	}
 	_, err := node.Eval(ctx)
 	if err != nil {
@@ -219,7 +223,7 @@ func TestEvalWriteStateDeposed(t *testing.T) {
 	}
 
 	rs := state.ModuleByPath(ctx.Path()).Resources["restype.resname"]
-	if rs.Deposed == nil || rs.Deposed.ID != "i-abc123" {
+	if len(rs.Deposed) != 1 || rs.Deposed[0].ID != "i-abc123" {
 		t.Fatalf("expected deposed instance to have ID 'i-abc123': %#v", rs)
 	}
 }

terraform/state.go

@@ -498,7 +498,7 @@ func (m *ModuleState) prune() {
 	for k, v := range m.Resources {
 		v.prune()
 
-		if (v.Primary == nil || v.Primary.ID == "") && len(v.Tainted) == 0 {
+		if (v.Primary == nil || v.Primary.ID == "") && len(v.Tainted) == 0 && len(v.Deposed) == 0 {
 			delete(m.Resources, k)
 		}
 	}
@@ -549,8 +549,8 @@ func (m *ModuleState) String() string {
 		}
 
 		deposedStr := ""
-		if rs.Deposed != nil {
+		if len(rs.Deposed) > 0 {
-			deposedStr = fmt.Sprintf(" (1 deposed)")
+			deposedStr = fmt.Sprintf(" (%d deposed)", len(rs.Deposed))
 		}
 
 		buf.WriteString(fmt.Sprintf("%s:%s%s\n", k, taintStr, deposedStr))
@@ -579,8 +579,8 @@ func (m *ModuleState) String() string {
 			buf.WriteString(fmt.Sprintf("  Tainted ID %d = %s\n", idx+1, t.ID))
 		}
 
-		if rs.Deposed != nil {
+		for idx, t := range rs.Deposed {
-			buf.WriteString(fmt.Sprintf("  Deposed ID = %s\n", rs.Deposed.ID))
+			buf.WriteString(fmt.Sprintf("  Deposed ID %d = %s\n", idx+1, t.ID))
 		}
 
 		if len(rs.Dependencies) > 0 {
@@ -660,7 +660,7 @@ type ResourceState struct {
 	// Deposed instance is cleaned up. If there were problems creating the
 	// replacement, we mark the replacement as Tainted and Undepose the former
 	// Primary.
-	Deposed *InstanceState `json:"deposed,omitempty"`
+	Deposed []*InstanceState `json:"deposed,omitempty"`
 }
 
 // Equal tests whether two ResourceStates are equal.
@@ -762,7 +762,10 @@ func (r *ResourceState) deepcopy() *ResourceState {
 		}
 	}
 	if r.Deposed != nil {
-		n.Deposed = r.Deposed.deepcopy()
+		n.Deposed = make([]*InstanceState, 0, len(r.Deposed))
+		for _, inst := range r.Deposed {
+			n.Deposed = append(n.Deposed, inst.deepcopy())
+		}
 	}
 
 	return n
@@ -783,9 +786,18 @@ func (r *ResourceState) prune() {
 
 	r.Tainted = r.Tainted[:n]
 
-	if r.Deposed != nil && r.Deposed.ID == "" {
+	n = len(r.Deposed)
-		r.Deposed = nil
+	for i := 0; i < n; i++ {
+		inst := r.Deposed[i]
+		if inst == nil || inst.ID == "" {
+			copy(r.Deposed[i:], r.Deposed[i+1:])
+			r.Deposed[n-1] = nil
+			n--
+			i--
+		}
 	}
+
+	r.Deposed = r.Deposed[:n]
 }
 
 func (r *ResourceState) sort() {

terraform/terraform_test.go

@@ -443,9 +443,9 @@ aws_instance.bar:
 `
 
 const testTerraformApplyErrorDestroyCreateBeforeDestroyStr = `
-aws_instance.bar: (1 tainted)
+aws_instance.bar: (1 deposed)
   ID = foo
-  Tainted ID 1 = bar
+  Deposed ID 1 = bar
 `
 
 const testTerraformApplyErrorPartialStr = `

terraform/test-fixtures/apply-multi-depose-create-before-destroy/main.tf

@@ -0,0 +1,8 @@
+resource "aws_instance" "web" {
+    // require_new is a special attribute recognized by testDiffFn that forces
+    // a new resource on every apply
+    require_new = "yes"
+    lifecycle {
+        create_before_destroy = true
+    }
+}

terraform/transform_deposed.go

@@ -29,14 +29,19 @@ func (t *DeposedTransformer) Transform(g *Graph) error {
 
 	// Go through all the resources in our state to look for deposed resources
 	for k, rs := range state.Resources {
-		if rs.Deposed == nil {
+		// If we have no deposed resources, then move on
+		if len(rs.Deposed) == 0 {
 			continue
 		}
+		deposed := rs.Deposed
 
-		g.Add(&graphNodeDeposedResource{
+		for i, _ := range deposed {
-			ResourceName: k,
+			g.Add(&graphNodeDeposedResource{
-			ResourceType: rs.Type,
+				Index:        i,
-		})
+				ResourceName: k,
+				ResourceType: rs.Type,
+			})
+		}
 	}
 
 	return nil
@@ -44,12 +49,13 @@ func (t *DeposedTransformer) Transform(g *Graph) error {
 
 // graphNodeDeposedResource is the graph vertex representing a deposed resource.
 type graphNodeDeposedResource struct {
+	Index        int
 	ResourceName string
 	ResourceType string
 }
 
 func (n *graphNodeDeposedResource) Name() string {
-	return fmt.Sprintf("%s (deposed)", n.ResourceName)
+	return fmt.Sprintf("%s (deposed #%d)", n.ResourceName, n.Index)
 }
 
 func (n *graphNodeDeposedResource) ProvidedBy() []string {
@@ -79,6 +85,7 @@ func (n *graphNodeDeposedResource) EvalTree() EvalNode {
 				&EvalReadStateDeposed{
 					Name:   n.ResourceName,
 					Output: &state,
+					Index:  n.Index,
 				},
 				&EvalRefresh{
 					Info:     info,
@@ -90,6 +97,7 @@ func (n *graphNodeDeposedResource) EvalTree() EvalNode {
 					Name:         n.ResourceName,
 					ResourceType: n.ResourceType,
 					State:        &state,
+					Index:        n.Index,
 				},
 			},
 		},
@@ -98,7 +106,6 @@ func (n *graphNodeDeposedResource) EvalTree() EvalNode {
 	// Apply
 	var diff *InstanceDiff
 	var err error
-	var emptyState *InstanceState
 	seq.Nodes = append(seq.Nodes, &EvalOpFilter{
 		Ops: []walkOperation{walkApply},
 		Node: &EvalSequence{
@@ -110,6 +117,7 @@ func (n *graphNodeDeposedResource) EvalTree() EvalNode {
 				&EvalReadStateDeposed{
 					Name:   n.ResourceName,
 					Output: &state,
+					Index:  n.Index,
 				},
 				&EvalDiffDestroy{
 					Info:   info,
@@ -124,20 +132,14 @@ func (n *graphNodeDeposedResource) EvalTree() EvalNode {
 					Output:   &state,
 					Error:    &err,
 				},
-				// Always write the resource back to the state tainted... if it
+				// Always write the resource back to the state deposed... if it
-				// successfully destroyed it will be pruned. If it did not, it will
+				// was successfully destroyed it will be pruned. If it was not, it will
-				// remain tainted.
+				// be caught on the next run.
-				&EvalWriteStateTainted{
-					Name:         n.ResourceName,
-					ResourceType: n.ResourceType,
-					State:        &state,
-					TaintedIndex: -1,
-				},
-				// Then clear the deposed state.
 				&EvalWriteStateDeposed{
 					Name:         n.ResourceName,
 					ResourceType: n.ResourceType,
-					State:        &emptyState,
+					State:        &state,
+					Index:        n.Index,
 				},
 				&EvalReturnError{
 					Error: &err,

terraform/transform_resource.go

@@ -406,7 +406,7 @@ func (n *graphNodeExpandedResource) EvalTree() EvalNode {
 								ResourceType: n.Resource.Type,
 								Dependencies: n.DependentOn(),
 								State:        &state,
-								TaintedIndex: -1,
+								Index:        -1,
 							},
 							&EvalIf{
 								If: func(ctx EvalContext) (bool, error) {
@@ -513,9 +513,9 @@ func (n *graphNodeExpandedResourceDestroy) EvalTree() EvalNode {
 						return n.Resource.Lifecycle.CreateBeforeDestroy, nil
 					},
 					Then: &EvalReadStateTainted{
-						Name:         n.stateId(),
+						Name:   n.stateId(),
-						Output:       &state,
+						Output: &state,
-						TaintedIndex: -1,
+						Index:  -1,
 					},
 					Else: &EvalReadState{
 						Name:   n.stateId(),

terraform/transform_tainted.go

@@ -88,9 +88,9 @@ func (n *graphNodeTaintedResource) EvalTree() EvalNode {
 					Output: &provider,
 				},
 				&EvalReadStateTainted{
-					Name:         n.ResourceName,
+					Name:   n.ResourceName,
-					TaintedIndex: n.Index,
+					Index:  n.Index,
-					Output:       &state,
+					Output: &state,
 				},
 				&EvalRefresh{
 					Info:     info,
@@ -102,7 +102,7 @@ func (n *graphNodeTaintedResource) EvalTree() EvalNode {
 					Name:         n.ResourceName,
 					ResourceType: n.ResourceType,
 					State:        &state,
-					TaintedIndex: n.Index,
+					Index:        n.Index,
 				},
 			},
 		},
@@ -119,9 +119,9 @@ func (n *graphNodeTaintedResource) EvalTree() EvalNode {
 					Output: &provider,
 				},
 				&EvalReadStateTainted{
-					Name:         n.ResourceName,
+					Name:   n.ResourceName,
-					TaintedIndex: n.Index,
+					Index:  n.Index,
-					Output:       &state,
+					Output: &state,
 				},
 				&EvalDiffDestroy{
 					Info:   info,
@@ -139,7 +139,7 @@ func (n *graphNodeTaintedResource) EvalTree() EvalNode {
 					Name:         n.ResourceName,
 					ResourceType: n.ResourceType,
 					State:        &state,
-					TaintedIndex: n.Index,
+					Index:        n.Index,
 				},
 				&EvalUpdateStateHook{},
 			},