From 561260d6fedb2805e6eb1fd560c693c49bf78abe Mon Sep 17 00:00:00 2001
From: Michael Schurter <schmichael@hashicorp.com>
Date: Fri, 29 Jun 2018 15:39:54 -0700
Subject: [PATCH] tr: skip error/success saving

All hooks only need to be run once.
Since only one hook can fail per run there's no need to
track errors on a per hook basis.
---
 .../allocrunnerv2/taskrunner/state/state.go   |  4 +--
 .../taskrunner/task_runner_hooks.go           | 30 +++++--------------
 2 files changed, 9 insertions(+), 25 deletions(-)

diff --git a/client/allocrunnerv2/taskrunner/state/state.go b/client/allocrunnerv2/taskrunner/state/state.go
index c46ab8ace..5c079a9e1 100644
--- a/client/allocrunnerv2/taskrunner/state/state.go
+++ b/client/allocrunnerv2/taskrunner/state/state.go
@@ -34,9 +34,7 @@ func (s *State) Copy() *State {
 }
 
 type HookState struct {
-	SuccessfulOnce bool
-	Data           map[string]string
-	LastError      error
+	Data map[string]string
 }
 
 func (h *HookState) Copy() *HookState {
diff --git a/client/allocrunnerv2/taskrunner/task_runner_hooks.go b/client/allocrunnerv2/taskrunner/task_runner_hooks.go
index 6daae44e9..3be92e94c 100644
--- a/client/allocrunnerv2/taskrunner/task_runner_hooks.go
+++ b/client/allocrunnerv2/taskrunner/task_runner_hooks.go
@@ -28,6 +28,8 @@ func (tr *TaskRunner) initHooks() {
 
 // prerun is used to run the runners prerun hooks.
 func (tr *TaskRunner) prerun() error {
+	//XXX is this necessary? maybe we should have a generic cancelletion
+	//    method instead of peeking into the alloc
 	// Determine if the allocation is terminaland we should avoid running
 	// pre-run hooks.
 	alloc := tr.Alloc()
@@ -68,23 +70,19 @@ func (tr *TaskRunner) prerun() error {
 		tr.state.RLock()
 		hookState := tr.state.Hooks[name]
 		if hookState != nil {
-			req.HookData = hookState.Data
+			// Hook already ran, skip
+			tr.state.RUnlock()
+			continue
 		}
 
 		req.VaultToken = tr.state.VaultToken
 		tr.state.RUnlock()
 
-		//XXX Can we assume everything only wants to be run until
-		//successful and simply keep track of which hooks have yet to
-		//run on failures+retries?
-		if hookState != nil && hookState.SuccessfulOnce {
-			tr.logger.Trace("skipping hook since it was successfully run once", "name", name)
-			continue
-		}
-
 		// Run the pre-run hook
 		var resp interfaces.TaskPrerunResponse
-		err := pre.Prerun(&req, &resp)
+		if err := pre.Prerun(&req, &resp); err != nil {
+			return structs.WrapRecoverable(fmt.Sprintf("pre-run hook %q failed: %v", name, err), err)
+		}
 
 		// Store the hook state
 		{
@@ -95,15 +93,10 @@ func (tr *TaskRunner) prerun() error {
 				tr.state.Hooks[name] = hookState
 			}
 
-			hookState.LastError = err
 			if resp.HookData != nil {
 				hookState.Data = resp.HookData
 			}
 
-			if resp.DoOnce && err != nil {
-				hookState.SuccessfulOnce = true
-			}
-
 			// XXX Detect if state has changed so that we can signal to the
 			// alloc runner precisly
 			/*
@@ -114,10 +107,6 @@ func (tr *TaskRunner) prerun() error {
 			tr.state.Unlock()
 		}
 
-		if err != nil {
-			return structs.WrapRecoverable(fmt.Sprintf("pre-run hook %q failed: %v", name, err), err)
-		}
-
 		// Store the environment variables returned by the hook
 		if len(resp.Env) != 0 {
 			tr.envBuilder.SetGenericEnv(resp.Env)
@@ -283,9 +272,6 @@ func (h *artifactHook) Prerun(req *interfaces.TaskPrerunRequest, resp *interface
 		}
 	}
 
-	//XXX Should this be managed by task runner directly? Seems silly to
-	//make every hook specify it
-	resp.DoOnce = true
 	return nil
 }