Merge pull request #2482 from hashicorp/f-2289-better-artifact-err

Improve artifact download error message

client/driver/docker.go

@@ -508,13 +508,10 @@ func (d *DockerDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle
 
 	container, err := d.createContainer(config)
 	if err != nil {
-		d.logger.Printf("[ERR] driver.docker: failed to create container: %s", err)
+		wrapped := fmt.Sprintf("Failed to create container: %v", err)
+		d.logger.Printf("[ERR] driver.docker: %s", wrapped)
 		pluginClient.Kill()
-		if rerr, ok := err.(*structs.RecoverableError); ok {
-			rerr.Err = fmt.Sprintf("Failed to create container: %s", rerr.Err)
-			return nil, rerr
-		}
-		return nil, err
+		return nil, structs.WrapRecoverable(wrapped, err)
 	}
 
 	d.logger.Printf("[INFO] driver.docker: created container %s", container.ID)

client/driver/docker_test.go

@@ -364,7 +364,7 @@ func TestDockerDriver_Start_BadPull_Recoverable(t *testing.T) {
 
 	if rerr, ok := err.(*structs.RecoverableError); !ok {
 		t.Fatalf("want recoverable error: %+v", err)
-	} else if !rerr.Recoverable {
+	} else if !rerr.IsRecoverable() {
 		t.Fatalf("error not recoverable: %+v", 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) IsRecoverable() 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

@@ -665,7 +665,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)
@@ -800,7 +800,7 @@ func (r *TaskRunner) prestart(resultCh chan bool) {
 					r.logger.Printf("[DEBUG] client: %v", wrapped)
 					r.setState(structs.TaskStatePending,
 						structs.NewTaskEvent(structs.TaskArtifactDownloadFailed).SetDownloadError(wrapped))
-					r.restartTracker.SetStartError(structs.NewRecoverableError(wrapped, structs.IsRecoverable(err)))
+					r.restartTracker.SetStartError(structs.WrapRecoverable(wrapped.Error(), err))
 					goto RESTART
 				}
 			}
@@ -1195,31 +1195,19 @@ func (r *TaskRunner) startTask() error {
 	r.createdResourcesLock.Unlock()
 
 	if err != nil {
-		wrapped := fmt.Errorf("failed to initialize task %q for alloc %q: %v",
+		wrapped := fmt.Sprintf("failed to initialize task %q for alloc %q: %v",
 			r.task.Name, r.alloc.ID, err)
-
-		r.logger.Printf("[WARN] client: error from prestart: %v", wrapped)
-
-		if rerr, ok := err.(*structs.RecoverableError); ok {
-			return structs.NewRecoverableError(wrapped, rerr.Recoverable)
-		}
-
-		return wrapped
+		r.logger.Printf("[WARN] client: error from prestart: %s", wrapped)
+		return structs.WrapRecoverable(wrapped, err)
 	}
 
 	// Start the job
 	handle, err := drv.Start(ctx, r.task)
 	if err != nil {
-		wrapped := fmt.Errorf("failed to start task %q for alloc %q: %v",
+		wrapped := fmt.Sprintf("failed to start task %q for alloc %q: %v",
 			r.task.Name, r.alloc.ID, err)
-
-		r.logger.Printf("[WARN] client: %v", wrapped)
-
-		if rerr, ok := err.(*structs.RecoverableError); ok {
-			return structs.NewRecoverableError(wrapped, rerr.Recoverable)
-		}
-
-		return wrapped
+		r.logger.Printf("[WARN] client: %s", wrapped)
+		return structs.WrapRecoverable(wrapped, err)
 
 	}
 

nomad/node_endpoint.go

@@ -1064,13 +1064,8 @@ 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
-							return structs.NewRecoverableError(wrapped, true)
-						}
-
-						return wrapped
+						wrapped := fmt.Sprintf("failed to create token for task %q on alloc %q: %v", task, alloc.ID, err)
+						return structs.WrapRecoverable(wrapped, err)
 					}
 
 					results[task] = secret

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.IsRecoverable() {
 		t.Fatalf("bad: %+v", resp.Error)
 	}
 }

nomad/structs/structs.go

@@ -4193,15 +4193,33 @@ func NewRecoverableError(e error, recoverable bool) error {
 	}
 }
 
+// WrapRecoverable wraps an existing error in a new RecoverableError with a new
+// message. If the error was recoverable before the returned error is as well;
+// otherwise it is unrecoverable.
+func WrapRecoverable(msg string, err error) error {
+	return &RecoverableError{Err: msg, Recoverable: IsRecoverable(err)}
+}
+
 func (r *RecoverableError) Error() string {
 	return r.Err
 }
 
+func (r *RecoverableError) IsRecoverable() 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
+	IsRecoverable() 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.IsRecoverable()
 	}
 	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.IsRecoverable() {
 		t.Fatalf("Err should have been recoverable")
 	}
 }