From 55d58cb8be2c3b6a2d042725f3a8f346541f9bbf Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Wed, 16 Sep 2020 11:33:55 -0400 Subject: [PATCH] terraform: NodeDestroyResourceInstance refactor (#26246) * terraform: remove unused eval node * add UpdateStateHook function to replace EvalUpdateStateHook * early exit error isn't * terraform: NodeDestroyResourceInstance refactor This PR refactor's NodeDestroyResourceInstance EvalTree() into an Execute() node. EvalRequireState and evalWriteEmptyState were used only by this node, and they have been removed in favor of straight code. There are still many calls to someEvalNode.Eval() in NodeDestroyResourceInstance: I plan on refactoring the remaining EvalTree()s before tacking those Eval()s (all of which are used by many graph nodes) I've added a new function, UpdateStateHook, that is effectively the same as EvalUpdateStateHook. The latter will be removed when the larger EvalNode refactor project is complete. --- terraform/eval_state.go | 68 +++----- terraform/eval_state_test.go | 76 +++------ terraform/graph_walk_context.go | 5 + terraform/node_data_refresh.go | 5 +- terraform/node_resource_destroy.go | 261 +++++++++++++++-------------- 5 files changed, 187 insertions(+), 228 deletions(-) diff --git a/terraform/eval_state.go b/terraform/eval_state.go index eca680569..3adabba4a 100644 --- a/terraform/eval_state.go +++ b/terraform/eval_state.go @@ -143,25 +143,6 @@ func (n *EvalReadStateDeposed) Eval(ctx EvalContext) (interface{}, error) { return obj, nil } -// EvalRequireState is an EvalNode implementation that exits early if the given -// object is null. -type EvalRequireState struct { - State **states.ResourceInstanceObject -} - -func (n *EvalRequireState) Eval(ctx EvalContext) (interface{}, error) { - if n.State == nil { - return nil, EvalEarlyExitError{} - } - - state := *n.State - if state == nil || state.Value.IsNull() { - return nil, EvalEarlyExitError{} - } - - return nil, nil -} - // EvalUpdateStateHook is an EvalNode implementation that calls the // PostStateUpdate hook with the current state. type EvalUpdateStateHook struct{} @@ -187,6 +168,27 @@ func (n *EvalUpdateStateHook) Eval(ctx EvalContext) (interface{}, error) { return nil, nil } +// UpdateStateHook calls the PostStateUpdate hook with the current state. +// +// TODO: UpdateStateHook will eventually replace EvalUpdateStateHook, at which +// point EvalUpdateStateHook can be removed and this comment updated. +func UpdateStateHook(ctx EvalContext) error { + // In principle we could grab the lock here just long enough to take a + // deep copy and then pass that to our hooks below, but we'll instead + // hold the hook for the duration to avoid the potential confusing + // situation of us racing to call PostStateUpdate concurrently with + // different state snapshots. + stateSync := ctx.State() + state := stateSync.Lock().DeepCopy() + defer stateSync.Unlock() + + // Call the hook + err := ctx.Hook(func(h Hook) (HookAction, error) { + return h.PostStateUpdate(state) + }) + return err +} + // evalWriteEmptyState wraps EvalWriteState to specifically record an empty // state for a particular object. type evalWriteEmptyState struct { @@ -510,34 +512,6 @@ func (n *EvalWriteResourceState) Eval(ctx EvalContext) (interface{}, error) { return nil, nil } -// EvalForgetResourceState is an EvalNode implementation that prunes out an -// empty resource-level state for a given resource address, or produces an -// error if it isn't empty after all. -// -// This should be the last action taken for a resource that has been removed -// from the configuration altogether, to clean up the leftover husk of the -// resource in the state after other EvalNodes have destroyed and removed -// all of the instances and instance objects beneath it. -type EvalForgetResourceState struct { - Addr addrs.Resource -} - -func (n *EvalForgetResourceState) Eval(ctx EvalContext) (interface{}, error) { - absAddr := n.Addr.Absolute(ctx.Path()) - state := ctx.State() - - pruned := state.RemoveResourceIfEmpty(absAddr) - if !pruned { - // If this produces an error, it indicates a bug elsewhere in Terraform - // -- probably missing graph nodes, graph edges, or - // incorrectly-implemented evaluation steps. - return nil, fmt.Errorf("orphan resource %s still has a non-empty state after apply; this is a bug in Terraform", absAddr) - } - log.Printf("[TRACE] EvalForgetResourceState: Pruned husk of %s from state", absAddr) - - return nil, nil -} - // EvalRefreshDependencies is an EvalNode implementation that appends any newly // found dependencies to those saved in the state. The existing dependencies // are retained, as they may be missing from the config, and will be required diff --git a/terraform/eval_state_test.go b/terraform/eval_state_test.go index 8cc771f76..6d6425a54 100644 --- a/terraform/eval_state_test.go +++ b/terraform/eval_state_test.go @@ -12,57 +12,6 @@ import ( "github.com/hashicorp/terraform/states" ) -func TestEvalRequireState(t *testing.T) { - ctx := new(MockEvalContext) - - cases := []struct { - State *states.ResourceInstanceObject - Exit bool - }{ - { - nil, - true, - }, - { - &states.ResourceInstanceObject{ - Value: cty.NullVal(cty.Object(map[string]cty.Type{ - "id": cty.String, - })), - Status: states.ObjectReady, - }, - true, - }, - { - &states.ResourceInstanceObject{ - Value: cty.ObjectVal(map[string]cty.Value{ - "id": cty.StringVal("foo"), - }), - Status: states.ObjectReady, - }, - false, - }, - } - - var exitVal EvalEarlyExitError - for _, tc := range cases { - node := &EvalRequireState{State: &tc.State} - _, err := node.Eval(ctx) - if tc.Exit { - if err != exitVal { - t.Fatalf("should've exited: %#v", tc.State) - } - - continue - } - if !tc.Exit && err != nil { - t.Fatalf("shouldn't exit: %#v", tc.State) - } - if err != nil { - t.Fatalf("err: %s", err) - } - } -} - func TestEvalUpdateStateHook(t *testing.T) { mockHook := new(MockHook) @@ -275,3 +224,28 @@ aws_instance.foo: (1 deposed) Deposed ID 1 = i-abc123 `) } + +// Same test as TestEvalUpdateStateHook, similar function, slightly different +// signature. The EvalUpdateStateHook test and function will be removed when the +// EvalNode Removal is complete. +func TestUpdateStateHook(t *testing.T) { + mockHook := new(MockHook) + + state := states.NewState() + state.Module(addrs.RootModuleInstance).SetLocalValue("foo", cty.StringVal("hello")) + + ctx := new(MockEvalContext) + ctx.HookHook = mockHook + ctx.StateState = state.SyncWrapper() + + if err := UpdateStateHook(ctx); err != nil { + t.Fatalf("err: %s", err) + } + + if !mockHook.PostStateUpdateCalled { + t.Fatal("should call PostStateUpdate") + } + if mockHook.PostStateUpdateState.LocalValue(addrs.LocalValue{Name: "foo"}.Absolute(addrs.RootModuleInstance)) != cty.StringVal("hello") { + t.Fatalf("wrong state passed to hook: %s", spew.Sdump(mockHook.PostStateUpdateState)) + } +} diff --git a/terraform/graph_walk_context.go b/terraform/graph_walk_context.go index 4a2e26841..c842c0af4 100644 --- a/terraform/graph_walk_context.go +++ b/terraform/graph_walk_context.go @@ -188,6 +188,11 @@ func (w *ContextGraphWalker) Execute(ctx EvalContext, n GraphNodeExecutable) tfd return nil } + // If we early exit, it isn't an error. + if _, isEarlyExit := err.(EvalEarlyExitError); isEarlyExit { + return nil + } + // Otherwise, we'll let our usual diagnostics machinery figure out how to // unpack this as one or more diagnostic messages and return that. If we // get down here then the returned diagnostics will contain at least one diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index f9377bf76..740af0fbe 100644 --- a/terraform/node_data_refresh.go +++ b/terraform/node_data_refresh.go @@ -251,12 +251,11 @@ func (n *NodeRefreshableDataResourceInstance) Execute(ctx EvalContext, op walkOp return err } - // EvalUpdateStateHook - updateStateHookReq := EvalUpdateStateHook{} - _, err = updateStateHookReq.Eval(ctx) + err = UpdateStateHook(ctx) if err != nil { return err } + } else { // EvalWriteDiff writeDiffReq := &EvalWriteDiff{ diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index 558d958fc..70533ca7f 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -5,7 +5,6 @@ import ( "log" "github.com/hashicorp/terraform/plans" - "github.com/hashicorp/terraform/providers" "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" @@ -31,7 +30,7 @@ var ( _ GraphNodeDestroyerCBD = (*NodeDestroyResourceInstance)(nil) _ GraphNodeReferenceable = (*NodeDestroyResourceInstance)(nil) _ GraphNodeReferencer = (*NodeDestroyResourceInstance)(nil) - _ GraphNodeEvalable = (*NodeDestroyResourceInstance)(nil) + _ GraphNodeExecutable = (*NodeDestroyResourceInstance)(nil) _ GraphNodeProviderConsumer = (*NodeDestroyResourceInstance)(nil) _ GraphNodeProvisionerConsumer = (*NodeDestroyResourceInstance)(nil) ) @@ -122,8 +121,8 @@ func (n *NodeDestroyResourceInstance) References() []*addrs.Reference { return nil } -// GraphNodeEvalable -func (n *NodeDestroyResourceInstance) EvalTree() EvalNode { +// GraphNodeExecutable +func (n *NodeDestroyResourceInstance) Execute(ctx EvalContext, op walkOperation) error { addr := n.ResourceInstanceAddr() // Get our state @@ -132,142 +131,150 @@ func (n *NodeDestroyResourceInstance) EvalTree() EvalNode { log.Printf("[WARN] NodeDestroyResourceInstance for %s with no state", addr) } + // These vars are updated through pointers at various stages below. var changeApply *plans.ResourceInstanceChange - var provider providers.Interface - var providerSchema *ProviderSchema var state *states.ResourceInstanceObject - var err error - return &EvalOpFilter{ - Ops: []walkOperation{walkApply, walkDestroy}, - Node: &EvalSequence{ - Nodes: []EvalNode{ - &EvalGetProvider{ - Addr: n.ResolvedProvider, - Output: &provider, - Schema: &providerSchema, - }, + var provisionerErr error - // Get the saved diff for apply - &EvalReadDiff{ - Addr: addr.Resource, - ProviderSchema: &providerSchema, - Change: &changeApply, - }, + switch op { + case walkApply, walkDestroy: + provider, providerSchema, err := GetProvider(ctx, n.ResolvedProvider) + if err != nil { + return err + } - &EvalReduceDiff{ - Addr: addr.Resource, - InChange: &changeApply, - Destroy: true, - OutChange: &changeApply, - }, + evalReadDiff := &EvalReadDiff{ + Addr: addr.Resource, + ProviderSchema: &providerSchema, + Change: &changeApply, + } + _, err = evalReadDiff.Eval(ctx) + if err != nil { + return err + } - // EvalReduceDiff may have simplified our planned change - // into a NoOp if it does not require destroying. - &EvalIf{ - If: func(ctx EvalContext) (bool, error) { - if changeApply == nil || changeApply.Action == plans.NoOp { - return true, EvalEarlyExitError{} - } - return true, nil - }, - Then: EvalNoop{}, - }, + evalReduceDiff := &EvalReduceDiff{ + Addr: addr.Resource, + InChange: &changeApply, + Destroy: true, + OutChange: &changeApply, + } + _, err = evalReduceDiff.Eval(ctx) + if err != nil { + return err + } - &EvalReadState{ - Addr: addr.Resource, - Output: &state, - Provider: &provider, - ProviderSchema: &providerSchema, - }, - &EvalRequireState{ - State: &state, - }, + // EvalReduceDiff may have simplified our planned change + // into a NoOp if it does not require destroying. + if changeApply == nil || changeApply.Action == plans.NoOp { + return EvalEarlyExitError{} + } - // Call pre-apply hook - &EvalApplyPre{ - Addr: addr.Resource, - State: &state, - Change: &changeApply, - }, + evalReadState := &EvalReadState{ + Addr: addr.Resource, + Output: &state, + Provider: &provider, + ProviderSchema: &providerSchema, + } + _, err = evalReadState.Eval(ctx) + if err != nil { + return err + } - // Run destroy provisioners if not tainted - &EvalIf{ - If: func(ctx EvalContext) (bool, error) { - if state != nil && state.Status == states.ObjectTainted { - return false, nil - } + // Exit early if the state object is null after reading the state + if state == nil || state.Value.IsNull() { + return EvalEarlyExitError{} + } - return true, nil - }, - - Then: &EvalApplyProvisioners{ - Addr: addr.Resource, - State: &state, - ResourceConfig: n.Config, - Error: &err, - When: configs.ProvisionerWhenDestroy, - }, - }, + evalApplyPre := &EvalApplyPre{ + Addr: addr.Resource, + State: &state, + Change: &changeApply, + } + _, err = evalApplyPre.Eval(ctx) + if err != nil { + return err + } + // Run destroy provisioners if not tainted + if state != nil && state.Status != states.ObjectTainted { + evalApplyProvisioners := &EvalApplyProvisioners{ + Addr: addr.Resource, + State: &state, + ResourceConfig: n.Config, + Error: &provisionerErr, + When: configs.ProvisionerWhenDestroy, + } + _, err := evalApplyProvisioners.Eval(ctx) + if err != nil { + return err + } + if provisionerErr != nil { // If we have a provisioning error, then we just call // the post-apply hook now. - &EvalIf{ - If: func(ctx EvalContext) (bool, error) { - return err != nil, nil - }, - - Then: &EvalApplyPost{ - Addr: addr.Resource, - State: &state, - Error: &err, - }, - }, - - // Managed resources need to be destroyed, while data sources - // are only removed from state. - &EvalIf{ - If: func(ctx EvalContext) (bool, error) { - return addr.Resource.Resource.Mode == addrs.ManagedResourceMode, nil - }, - - Then: &EvalSequence{ - Nodes: []EvalNode{ - &EvalApply{ - Addr: addr.Resource, - Config: nil, // No configuration because we are destroying - State: &state, - Change: &changeApply, - Provider: &provider, - ProviderAddr: n.ResolvedProvider, - ProviderMetas: n.ProviderMetas, - ProviderSchema: &providerSchema, - Output: &state, - Error: &err, - }, - &EvalWriteState{ - Addr: addr.Resource, - ProviderAddr: n.ResolvedProvider, - ProviderSchema: &providerSchema, - State: &state, - }, - }, - }, - Else: &evalWriteEmptyState{ - EvalWriteState{ - Addr: addr.Resource, - ProviderAddr: n.ResolvedProvider, - ProviderSchema: &providerSchema, - }, - }, - }, - - &EvalApplyPost{ + evalApplyPost := &EvalApplyPost{ Addr: addr.Resource, State: &state, - Error: &err, - }, - &EvalUpdateStateHook{}, - }, - }, + Error: &provisionerErr, + } + _, err = evalApplyPost.Eval(ctx) + if err != nil { + return err + } + } + } + + // Managed resources need to be destroyed, while data sources + // are only removed from state. + if addr.Resource.Resource.Mode == addrs.ManagedResourceMode { + evalApply := &EvalApply{ + Addr: addr.Resource, + Config: nil, // No configuration because we are destroying + State: &state, + Change: &changeApply, + Provider: &provider, + ProviderAddr: n.ResolvedProvider, + ProviderMetas: n.ProviderMetas, + ProviderSchema: &providerSchema, + Output: &state, + Error: &provisionerErr, + } + _, err = evalApply.Eval(ctx) + if err != nil { + return err + } + + evalWriteState := &EvalWriteState{ + Addr: addr.Resource, + ProviderAddr: n.ResolvedProvider, + ProviderSchema: &providerSchema, + State: &state, + } + _, err = evalWriteState.Eval(ctx) + if err != nil { + return err + } + } else { + log.Printf("[TRACE] NodeDestroyResourceInstance: removing state object for %s", n.Addr) + state := ctx.State() + state.SetResourceInstanceCurrent(n.Addr, nil, n.ResolvedProvider) + } + + evalApplyPost := &EvalApplyPost{ + Addr: addr.Resource, + State: &state, + Error: &provisionerErr, + } + _, err = evalApplyPost.Eval(ctx) + if err != nil { + return err + } + + err = UpdateStateHook(ctx) + if err != nil { + return err + } } + + return nil }