From 95f30451d96b6b77a0ccea0de83818891229cd02 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 28 Oct 2020 14:12:52 -0400 Subject: [PATCH] get rid of EvalEarlyExitError This was mostly unused now, since we no longer needed to interrupt a series of eval node executions. The exception was the stopHook, which is still used to halt execution when there's an interrupt. Since interrupting execution should not complete successfully, we use a normal opaque error to halt everything, and return it to the UI. We can work on coalescing or hiding these if necessary in a separate PR. --- terraform/context_apply_test.go | 37 ++++++++++++++++--- terraform/eval_context_builtin.go | 2 +- terraform/eval_error.go | 7 ---- terraform/graph_walk_context.go | 45 +---------------------- terraform/hook_stop.go | 7 +--- terraform/node_resource_apply_instance.go | 3 -- terraform/node_resource_destroy.go | 2 - 7 files changed, 37 insertions(+), 66 deletions(-) delete mode 100644 terraform/eval_error.go diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 8d76edc6bb..8b7254f0bd 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -1734,8 +1734,16 @@ func TestContext2Apply_cancel(t *testing.T) { }() state := <-stateCh - if applyDiags.HasErrors() { - t.Fatalf("unexpected errors: %s", applyDiags.Err()) + // only expecting an early exit error + if !applyDiags.HasErrors() { + t.Fatal("expected early exit error") + } + + for _, d := range applyDiags { + desc := d.Description() + if desc.Summary != "execution halted" { + t.Fatalf("unexpected error: %v", applyDiags.Err()) + } } actual := strings.TrimSpace(state.String()) @@ -1812,8 +1820,16 @@ func TestContext2Apply_cancelBlock(t *testing.T) { // Wait for apply to complete state := <-stateCh - if applyDiags.HasErrors() { - t.Fatalf("unexpected error: %s", applyDiags.Err()) + // only expecting an early exit error + if !applyDiags.HasErrors() { + t.Fatal("expected early exit error") + } + + for _, d := range applyDiags { + desc := d.Description() + if desc.Summary != "execution halted" { + t.Fatalf("unexpected error: %v", applyDiags.Err()) + } } checkStateString(t, state, ` @@ -1882,7 +1898,18 @@ func TestContext2Apply_cancelProvisioner(t *testing.T) { // Wait for completion state := <-stateCh - assertNoErrors(t, applyDiags) + + // we are expecting only an early exit error + if !applyDiags.HasErrors() { + t.Fatal("expected early exit error") + } + + for _, d := range applyDiags { + desc := d.Description() + if desc.Summary != "execution halted" { + t.Fatalf("unexpected error: %v", applyDiags.Err()) + } + } checkStateString(t, state, ` aws_instance.foo: (tainted) diff --git a/terraform/eval_context_builtin.go b/terraform/eval_context_builtin.go index 67264948ef..63f87cc274 100644 --- a/terraform/eval_context_builtin.go +++ b/terraform/eval_context_builtin.go @@ -107,7 +107,7 @@ func (ctx *BuiltinEvalContext) Hook(fn func(Hook) (HookAction, error)) error { case HookActionHalt: // Return an early exit error to trigger an early exit log.Printf("[WARN] Early exit triggered by hook: %T", h) - return EvalEarlyExitError{} + return nil } } diff --git a/terraform/eval_error.go b/terraform/eval_error.go deleted file mode 100644 index 853ea2cc8e..0000000000 --- a/terraform/eval_error.go +++ /dev/null @@ -1,7 +0,0 @@ -package terraform - -// EvalEarlyExitError is a special error return value that can be returned -// by eval nodes that does an early exit. -type EvalEarlyExitError struct{} - -func (EvalEarlyExitError) Error() string { return "early exit" } diff --git a/terraform/graph_walk_context.go b/terraform/graph_walk_context.go index 907842f252..70cf4aff41 100644 --- a/terraform/graph_walk_context.go +++ b/terraform/graph_walk_context.go @@ -2,7 +2,6 @@ package terraform import ( "context" - "errors" "sync" "github.com/zclconf/go-cty/cty" @@ -36,7 +35,6 @@ type ContextGraphWalker struct { // is in progress. NonFatalDiagnostics tfdiags.Diagnostics - errorLock sync.Mutex once sync.Once contexts map[string]*BuiltinEvalContext contextLock sync.Mutex @@ -124,46 +122,7 @@ func (w *ContextGraphWalker) init() { func (w *ContextGraphWalker) Execute(ctx EvalContext, n GraphNodeExecutable) tfdiags.Diagnostics { // Acquire a lock on the semaphore w.Context.parallelSem.Acquire() + defer w.Context.parallelSem.Release() - diags := n.Execute(ctx, w.Operation) - - // Release the semaphore - w.Context.parallelSem.Release() - - if !diags.HasErrors() { - return diags - } - - // Acquire the lock because anything is going to require a lock. - w.errorLock.Lock() - defer w.errorLock.Unlock() - - err := diags.Err() - // Handle a simple early exit error - if errors.Is(err, EvalEarlyExitError{}) { - return nil - } - - // we need to see if this wraps only EarlyExitErrors - if wrapper, ok := err.(interface{ WrappedErrors() []error }); ok { - //WrappedErrors only returns native error values, so we can't extract - //them from diagnostics. Just return the whole thing if we have a - //combination. - errs := wrapper.WrappedErrors() - earlyExit := true - for _, err := range errs { - if err != (EvalEarlyExitError{}) { - earlyExit = false - } - } - if len(errs) > 0 && earlyExit { - 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 - // error, causing the graph walk to halt. - return diags + return n.Execute(ctx, w.Operation) } diff --git a/terraform/hook_stop.go b/terraform/hook_stop.go index 811fb337cc..86f2211426 100644 --- a/terraform/hook_stop.go +++ b/terraform/hook_stop.go @@ -1,6 +1,7 @@ package terraform import ( + "errors" "sync/atomic" "github.com/zclconf/go-cty/cty" @@ -76,11 +77,7 @@ func (h *stopHook) PostStateUpdate(new *states.State) (HookAction, error) { func (h *stopHook) hook() (HookAction, error) { if h.Stopped() { - // FIXME: This should really return an error since stopping partway - // through is not a successful run-to-completion, but we'll need to - // introduce that cautiously since existing automation solutions may - // be depending on this behavior. - return HookActionHalt, nil + return HookActionHalt, errors.New("execution halted") } return HookActionContinue, nil diff --git a/terraform/node_resource_apply_instance.go b/terraform/node_resource_apply_instance.go index 90085997b5..bd6a528967 100644 --- a/terraform/node_resource_apply_instance.go +++ b/terraform/node_resource_apply_instance.go @@ -148,7 +148,6 @@ func (n *NodeApplyableResourceInstance) dataResourceExecute(ctx EvalContext) (di } // Stop early if we don't actually have a diff if change == nil { - diags = diags.Append(EvalEarlyExitError{}) return diags } @@ -223,7 +222,6 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) // We don't want to do any destroys // (these are handled by NodeDestroyResourceInstance instead) if diffApply == nil || diffApply.Action == plans.Delete { - diags = diags.Append(EvalEarlyExitError{}) return diags } @@ -325,7 +323,6 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext) // into a NoOp if it only requires destroying, since destroying // is handled by NodeDestroyResourceInstance. if diffApply == nil || diffApply.Action == plans.NoOp { - diags = diags.Append(EvalEarlyExitError{}) return diags } diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index 9e9bfe87db..163aa24dad 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -163,7 +163,6 @@ func (n *NodeDestroyResourceInstance) Execute(ctx EvalContext, op walkOperation) // EvalReduceDiff may have simplified our planned change // into a NoOp if it does not require destroying. if changeApply == nil || changeApply.Action == plans.NoOp { - diags = diags.Append(EvalEarlyExitError{}) return diags } @@ -175,7 +174,6 @@ func (n *NodeDestroyResourceInstance) Execute(ctx EvalContext, op walkOperation) // Exit early if the state object is null after reading the state if state == nil || state.Value.IsNull() { - diags = diags.Append(EvalEarlyExitError{}) return diags }