From 774cb8dd9b3eaeb57f86b3c89df2b7d2540afd01 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Fri, 24 Mar 2017 15:26:05 -0700 Subject: [PATCH 1/4] 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 | 6 +++--- client/getter/getter.go | 28 ++++++++++++++++++++++++++-- client/restarts.go | 2 +- client/task_runner.go | 10 +++++----- nomad/node_endpoint.go | 3 +-- nomad/node_endpoint_test.go | 2 +- nomad/structs/structs.go | 19 +++++++++++++++---- nomad/vault_test.go | 6 +++--- 8 files changed, 55 insertions(+), 21 deletions(-) diff --git a/client/driver/docker.go b/client/driver/docker.go index 844c9cff873d..7c50b5558689 100644 --- a/client/driver/docker.go +++ b/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 } diff --git a/client/getter/getter.go b/client/getter/getter.go index 6bd3d53fb47d..3ac2a22d6ea8 100644 --- a/client/getter/getter.go +++ b/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 +} diff --git a/client/restarts.go b/client/restarts.go index 2c52cd1c9001..b6e49e31c702 100644 --- a/client/restarts.go +++ b/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 } diff --git a/client/task_runner.go b/client/task_runner.go index f9d15767c1cc..febe92c17279 100644 --- a/client/task_runner.go +++ b/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 diff --git a/nomad/node_endpoint.go b/nomad/node_endpoint.go index 0dd7768c7223..5075aecfc1d1 100644 --- a/nomad/node_endpoint.go +++ b/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) } diff --git a/nomad/node_endpoint_test.go b/nomad/node_endpoint_test.go index 329e39d141f2..20e313d8023c 100644 --- a/nomad/node_endpoint_test.go +++ b/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) } } diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index c35c1a2aa534..6cd2a6b7ac40 100644 --- a/nomad/structs/structs.go +++ b/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,7 +4189,7 @@ func NewRecoverableError(e error, recoverable bool) error { return &RecoverableError{ Err: e.Error(), - Recoverable: recoverable, + recoverable: recoverable, } } @@ -4197,11 +4197,22 @@ 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 } diff --git a/nomad/vault_test.go b/nomad/vault_test.go index 4e613a8abb87..b19d65976ac4 100644 --- a/nomad/vault_test.go +++ b/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") } } From 76c909bc9ed8d6884b356890bdd218b666ee285a Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Mon, 27 Mar 2017 15:37:15 -0700 Subject: [PATCH 2/4] Add WrapRecoverable helper --- client/driver/docker.go | 9 +++------ client/task_runner.go | 26 +++++++------------------- nomad/node_endpoint.go | 8 ++------ nomad/structs/structs.go | 7 +++++++ 4 files changed, 19 insertions(+), 31 deletions(-) diff --git a/client/driver/docker.go b/client/driver/docker.go index 7c50b5558689..fbfe94113333 100644 --- a/client/driver/docker.go +++ b/client/driver/docker.go @@ -507,13 +507,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.Recoverable); ok { - wrapped := fmt.Errorf("Failed to create container: %v", rerr) - return nil, structs.NewRecoverableError(wrapped, rerr.Recoverable()) - } - return nil, err + return nil, structs.WrapRecoverable(wrapped, err) } d.logger.Printf("[INFO] driver.docker: created container %s", container.ID) diff --git a/client/task_runner.go b/client/task_runner.go index febe92c17279..6f6990103394 100644 --- a/client/task_runner.go +++ b/client/task_runner.go @@ -796,7 +796,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 } } @@ -1171,31 +1171,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.Recoverable); 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.Recoverable); ok { - return structs.NewRecoverableError(wrapped, rerr.Recoverable()) - } - - return wrapped + r.logger.Printf("[WARN] client: %s", wrapped) + return structs.WrapRecoverable(wrapped, err) } diff --git a/nomad/node_endpoint.go b/nomad/node_endpoint.go index 5075aecfc1d1..4cc5ce47633b 100644 --- a/nomad/node_endpoint.go +++ b/nomad/node_endpoint.go @@ -1064,12 +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 structs.IsRecoverable(err) { - 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 diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 6cd2a6b7ac40..3da7919998b8 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -4193,6 +4193,13 @@ 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 } From 6a4e1d1709dc9446395820005373948c332988d7 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Mon, 27 Mar 2017 15:41:35 -0700 Subject: [PATCH 3/4] Recoverable is a method now, not a field --- client/driver/docker_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/driver/docker_test.go b/client/driver/docker_test.go index fe725252fc8f..f5b0e61ec828 100644 --- a/client/driver/docker_test.go +++ b/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.Recoverable() { t.Fatalf("error not recoverable: %+v", err) } } From ae9d49502aa4d061c0f69bd02882508cebc8db90 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Mon, 27 Mar 2017 16:27:24 -0700 Subject: [PATCH 4/4] Recoverable struct field must be exported --- client/driver/docker_test.go | 2 +- client/getter/getter.go | 2 +- nomad/node_endpoint_test.go | 2 +- nomad/structs/structs.go | 14 +++++++------- nomad/vault_test.go | 2 +- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/client/driver/docker_test.go b/client/driver/docker_test.go index f5b0e61ec828..9b6825074e05 100644 --- a/client/driver/docker_test.go +++ b/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) } } diff --git a/client/getter/getter.go b/client/getter/getter.go index 3ac2a22d6ea8..cff9f965eff2 100644 --- a/client/getter/getter.go +++ b/client/getter/getter.go @@ -120,6 +120,6 @@ func (g *GetError) Error() string { return g.Err.Error() } -func (g *GetError) Recoverable() bool { +func (g *GetError) IsRecoverable() bool { return g.recoverable } diff --git a/nomad/node_endpoint_test.go b/nomad/node_endpoint_test.go index 20e313d8023c..68e2b42e7407 100644 --- a/nomad/node_endpoint_test.go +++ b/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) } } diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 3da7919998b8..f85ae1d52721 100644 --- a/nomad/structs/structs.go +++ b/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,7 +4189,7 @@ func NewRecoverableError(e error, recoverable bool) error { return &RecoverableError{ Err: e.Error(), - recoverable: recoverable, + Recoverable: recoverable, } } @@ -4197,29 +4197,29 @@ func NewRecoverableError(e error, recoverable bool) error { // 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)} + return &RecoverableError{Err: msg, Recoverable: IsRecoverable(err)} } func (r *RecoverableError) Error() string { return r.Err } -func (r *RecoverableError) Recoverable() bool { - return r.recoverable +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 - Recoverable() bool + 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.(Recoverable); ok { - return re.Recoverable() + return re.IsRecoverable() } return false } diff --git a/nomad/vault_test.go b/nomad/vault_test.go index b19d65976ac4..b874230b42ae 100644 --- a/nomad/vault_test.go +++ b/nomad/vault_test.go @@ -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") } }