Improve artifact download error message

Fixes #2289

Unfortunately took more RecoverableError hijinx than I would have liked.
There might be a better way.

client/driver/docker.go

@@ -509,9 +509,9 @@ func (d *DockerDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle
 	if err != nil {
 		d.logger.Printf("[ERR] driver.docker: failed to create container: %s", err)
 		pluginClient.Kill()
-		if rerr, ok := err.(*structs.RecoverableError); ok {
-			rerr.Err = fmt.Sprintf("Failed to create container: %s", rerr.Err)
-			return nil, rerr
+		if rerr, ok := err.(structs.Recoverable); ok {
+			wrapped := fmt.Errorf("Failed to create container: %v", rerr)
+			return nil, structs.NewRecoverableError(wrapped, rerr.Recoverable())
 		}
 		return nil, err
 	}

client/getter/getter.go

@@ -88,14 +88,38 @@ func getGetterUrl(taskEnv *env.TaskEnvironment, artifact *structs.TaskArtifact)
 func GetArtifact(taskEnv *env.TaskEnvironment, artifact *structs.TaskArtifact, taskDir string) error {
 	url, err := getGetterUrl(taskEnv, artifact)
 	if err != nil {
-		return err
+		return newGetError(artifact.GetterSource, err, false)
 	}
 
 	// Download the artifact
 	dest := filepath.Join(taskDir, artifact.RelativeDest)
 	if err := getClient(url, dest).Get(); err != nil {
-		return structs.NewRecoverableError(fmt.Errorf("GET error: %v", err), true)
+		return newGetError(url, err, true)
 	}
 
 	return nil
 }
+
+// GetError wraps the underlying artifact fetching error with the URL. It
+// implements the RecoverableError interface.
+type GetError struct {
+	URL         string
+	Err         error
+	recoverable bool
+}
+
+func newGetError(url string, err error, recoverable bool) *GetError {
+	return &GetError{
+		URL:         url,
+		Err:         err,
+		recoverable: recoverable,
+	}
+}
+
+func (g *GetError) Error() string {
+	return g.Err.Error()
+}
+
+func (g *GetError) Recoverable() bool {
+	return g.recoverable
+}

client/restarts.go

@@ -149,7 +149,7 @@ func (r *RestartTracker) GetState() (string, time.Duration) {
 // infinitely try to start a task.
 func (r *RestartTracker) handleStartError() (string, time.Duration) {
 	// If the error is not recoverable, do not restart.
-	if rerr, ok := r.startErr.(*structs.RecoverableError); !(ok && rerr.Recoverable) {
+	if !structs.IsRecoverable(r.startErr) {
 		r.reason = ReasonUnrecoverableErrror
 		return structs.TaskNotRestarting, 0
 	}

client/task_runner.go

@@ -661,7 +661,7 @@ func (r *TaskRunner) deriveVaultToken() (token string, exit bool) {
 		}
 
 		// Check if we can't recover from the error
-		if rerr, ok := err.(*structs.RecoverableError); !ok || !rerr.Recoverable {
+		if !structs.IsRecoverable(err) {
 			r.logger.Printf("[ERR] client: failed to derive Vault token for task %v on alloc %q: %v",
 				r.task.Name, r.alloc.ID, err)
 			r.Kill("vault", fmt.Sprintf("failed to derive token: %v", err), true)
@@ -1176,8 +1176,8 @@ func (r *TaskRunner) startTask() error {
 
 		r.logger.Printf("[WARN] client: error from prestart: %v", wrapped)
 
-		if rerr, ok := err.(*structs.RecoverableError); ok {
-			return structs.NewRecoverableError(wrapped, rerr.Recoverable)
+		if rerr, ok := err.(structs.Recoverable); ok {
+			return structs.NewRecoverableError(wrapped, rerr.Recoverable())
 		}
 
 		return wrapped
@@ -1191,8 +1191,8 @@ func (r *TaskRunner) startTask() error {
 
 		r.logger.Printf("[WARN] client: %v", wrapped)
 
-		if rerr, ok := err.(*structs.RecoverableError); ok {
-			return structs.NewRecoverableError(wrapped, rerr.Recoverable)
+		if rerr, ok := err.(structs.Recoverable); ok {
+			return structs.NewRecoverableError(wrapped, rerr.Recoverable())
 		}
 
 		return wrapped

nomad/node_endpoint.go

@@ -1065,8 +1065,7 @@ func (n *Node) DeriveVaultToken(args *structs.DeriveVaultTokenRequest,
 					secret, err := n.srv.vault.CreateToken(ctx, alloc, task)
 					if err != nil {
 						wrapped := fmt.Errorf("failed to create token for task %q on alloc %q: %v", task, alloc.ID, err)
-						if rerr, ok := err.(*structs.RecoverableError); ok && rerr.Recoverable {
-							// If the error is recoverable, propogate it
+						if structs.IsRecoverable(err) {
 							return structs.NewRecoverableError(wrapped, true)
 						}
 

nomad/node_endpoint_test.go

@@ -2030,7 +2030,7 @@ func TestClientEndpoint_DeriveVaultToken_VaultError(t *testing.T) {
 	if err != nil {
 		t.Fatalf("bad: %v", err)
 	}
-	if resp.Error == nil || !resp.Error.Recoverable {
+	if resp.Error == nil || !resp.Error.Recoverable() {
 		t.Fatalf("bad: %+v", resp.Error)
 	}
 }

nomad/structs/structs.go

@@ -4177,7 +4177,7 @@ type KeyringRequest struct {
 // be retried or it is fatal.
 type RecoverableError struct {
 	Err         string
-	Recoverable bool
+	recoverable bool
 }
 
 // NewRecoverableError is used to wrap an error and mark it as recoverable or
@@ -4189,19 +4189,30 @@ func NewRecoverableError(e error, recoverable bool) error {
 
 	return &RecoverableError{
 		Err:         e.Error(),
-		Recoverable: recoverable,
+		recoverable: recoverable,
 	}
 }
 
 func (r *RecoverableError) Error() string {
 	return r.Err
 }
 
+func (r *RecoverableError) Recoverable() bool {
+	return r.recoverable
+}
+
+// Recoverable is an interface for errors to implement to indicate whether or
+// not they are fatal or recoverable.
+type Recoverable interface {
+	error
+	Recoverable() bool
+}
+
 // IsRecoverable returns true if error is a RecoverableError with
 // Recoverable=true. Otherwise false is returned.
 func IsRecoverable(e error) bool {
-	if re, ok := e.(*RecoverableError); ok {
-		return re.Recoverable
+	if re, ok := e.(Recoverable); ok {
+		return re.Recoverable()
 	}
 	return false
 }

nomad/vault_test.go

@@ -925,9 +925,9 @@ func TestVaultClient_CreateToken_Role_Unrecoverable(t *testing.T) {
 		t.Fatalf("CreateToken should have failed: %v", err)
 	}
 
-	_, ok := err.(*structs.RecoverableError)
+	_, ok := err.(structs.Recoverable)
 	if ok {
-		t.Fatalf("CreateToken should not be a recoverable error type: %v", err)
+		t.Fatalf("CreateToken should not be a recoverable error type: %v (%T)", err, err)
 	}
 }
 
@@ -955,7 +955,7 @@ func TestVaultClient_CreateToken_Prestart(t *testing.T) {
 
 	if rerr, ok := err.(*structs.RecoverableError); !ok {
 		t.Fatalf("Err should have been type recoverable error")
-	} else if ok && !rerr.Recoverable {
+	} else if ok && !rerr.Recoverable() {
 		t.Fatalf("Err should have been recoverable")
 	}
 }