From 6c93fbb85dfec574d9b5089d7a45b5fc5fb7313d Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Wed, 4 Mar 2015 12:15:53 -0600 Subject: [PATCH] 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 | 139 ++++++++++++++++++ terraform/eval_error.go | 4 + terraform/eval_state.go | 37 +++-- terraform/eval_state_test.go | 18 ++- terraform/state.go | 30 ++-- terraform/terraform_test.go | 4 +- .../main.tf | 8 + terraform/transform_deposed.go | 38 ++--- terraform/transform_resource.go | 8 +- terraform/transform_tainted.go | 16 +- 10 files changed, 243 insertions(+), 59 deletions(-) create mode 100644 terraform/test-fixtures/apply-multi-depose-create-before-destroy/main.tf diff --git a/terraform/context_test.go b/terraform/context_test.go index aaeda9159..9050d4b96 100644 --- a/terraform/context_test.go +++ b/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 diff --git a/terraform/eval_error.go b/terraform/eval_error.go index 690fe28e9..470f798b7 100644 --- a/terraform/eval_error.go +++ b/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 } diff --git a/terraform/eval_state.go b/terraform/eval_state.go index 2fa4b8d06..b7480aa6f 100644 --- a/terraform/eval_state.go +++ b/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.Deposed = nil + rs.Primary = rs.Deposed[len(rs.Deposed)-1] + rs.Deposed = rs.Deposed[:len(rs.Deposed)-1] return nil, nil } diff --git a/terraform/eval_state_test.go b/terraform/eval_state_test.go index 168955c0a..e31a69132 100644 --- a/terraform/eval_state_test.go +++ b/terraform/eval_state_test.go @@ -97,21 +97,24 @@ func TestEvalReadState(t *testing.T) { }, }, Node: &EvalReadStateTainted{ - Name: "aws_instance.bar", - Output: &output, - TaintedIndex: 0, + Name: "aws_instance.bar", + Output: &output, + 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) } } diff --git a/terraform/state.go b/terraform/state.go index 9c9dd7e24..a600e211d 100644 --- a/terraform/state.go +++ b/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 { - deposedStr = fmt.Sprintf(" (1 deposed)") + if len(rs.Deposed) > 0 { + 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 { - buf.WriteString(fmt.Sprintf(" Deposed ID = %s\n", rs.Deposed.ID)) + for idx, t := range rs.Deposed { + 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 == "" { - r.Deposed = nil + n = len(r.Deposed) + 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() { diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index 4070475c0..94664791f 100644 --- a/terraform/terraform_test.go +++ b/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 = ` diff --git a/terraform/test-fixtures/apply-multi-depose-create-before-destroy/main.tf b/terraform/test-fixtures/apply-multi-depose-create-before-destroy/main.tf new file mode 100644 index 000000000..ac7ba4b9b --- /dev/null +++ b/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 + } +} diff --git a/terraform/transform_deposed.go b/terraform/transform_deposed.go index 9a86dd5df..4e66d81c1 100644 --- a/terraform/transform_deposed.go +++ b/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{ - ResourceName: k, - ResourceType: rs.Type, - }) + for i, _ := range deposed { + g.Add(&graphNodeDeposedResource{ + 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 - // successfully destroyed it will be pruned. If it did not, it will - // remain tainted. - &EvalWriteStateTainted{ - Name: n.ResourceName, - ResourceType: n.ResourceType, - State: &state, - TaintedIndex: -1, - }, - // Then clear the deposed state. + // Always write the resource back to the state deposed... if it + // was successfully destroyed it will be pruned. If it was not, it will + // be caught on the next run. &EvalWriteStateDeposed{ Name: n.ResourceName, ResourceType: n.ResourceType, - State: &emptyState, + State: &state, + Index: n.Index, }, &EvalReturnError{ Error: &err, diff --git a/terraform/transform_resource.go b/terraform/transform_resource.go index aa5e17998..8c2a00c78 100644 --- a/terraform/transform_resource.go +++ b/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(), - Output: &state, - TaintedIndex: -1, + Name: n.stateId(), + Output: &state, + Index: -1, }, Else: &EvalReadState{ Name: n.stateId(), diff --git a/terraform/transform_tainted.go b/terraform/transform_tainted.go index 9fbc53b14..c88516ade 100644 --- a/terraform/transform_tainted.go +++ b/terraform/transform_tainted.go @@ -88,9 +88,9 @@ func (n *graphNodeTaintedResource) EvalTree() EvalNode { Output: &provider, }, &EvalReadStateTainted{ - Name: n.ResourceName, - TaintedIndex: n.Index, - Output: &state, + Name: n.ResourceName, + Index: n.Index, + 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, - TaintedIndex: n.Index, - Output: &state, + Name: n.ResourceName, + Index: n.Index, + 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{}, },