Replace CannotStopContainerError.IsUnretryableError with CannotStopCo…

…ntainerError.IsUnretryableError

The only place this method was used was in a negated test. Negating negatives
requires unnecessarily mental work on the part of the reader. IsRetryableError
is quicker to understand.

agent/engine/errors.go

@@ -32,7 +32,7 @@ type engineError interface {
 
 type cannotStopContainerError interface {
 	engineError
-	IsUnretriableError() bool
+	IsRetriableError() bool
 }
 
 // impossibleTransitionError is an error that occurs when an event causes a
@@ -159,15 +159,21 @@ func (err CannotStopContainerError) ErrorName() string {
 	return "CannotStopContainerError"
 }
 
-func (err CannotStopContainerError) IsUnretriableError() bool {
+// IsRetriableError returns a boolean indicating whether the call that
+// generated the error can be retried.
+// When stopping a container, most errors that we can get should be
+// considered retriable. However, in the case where the container is
+// already stopped or doesn't exist at all, there's no sense in
+// retrying.
+func (err CannotStopContainerError) IsRetriableError() bool {
 	if _, ok := err.fromError.(*docker.NoSuchContainer); ok {
-		return true
+		return false
 	}
 	if _, ok := err.fromError.(*docker.ContainerNotRunning); ok {
-		return true
+		return false
 	}
 
-	return false
+	return true
 }
 
 // CannotPullContainerError indicates any error when trying to pull

agent/engine/errors_test.go

@@ -22,17 +22,17 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestUnretriableErrorReturnsTrueForNoSuchContainer(t *testing.T) {
+func TestRetriableErrorReturnsFalseForNoSuchContainer(t *testing.T) {
 	err := CannotStopContainerError{&docker.NoSuchContainer{}}
-	assert.True(t, err.IsUnretriableError(), "No such container error should be treated as unretriable docker error")
+	assert.False(t, err.IsRetriableError(), "No such container error should be treated as unretriable docker error")
 }
 
-func TestUnretriableErrorReturnsTrueForContainerNotRunning(t *testing.T) {
+func TestRetriableErrorReturnsFalseForContainerNotRunning(t *testing.T) {
 	err := CannotStopContainerError{&docker.ContainerNotRunning{}}
-	assert.True(t, err.IsUnretriableError(), "ContainerNotRunning error should be treated as unretriable docker error")
+	assert.False(t, err.IsRetriableError(), "ContainerNotRunning error should be treated as unretriable docker error")
 }
 
-func TestUnretriableErrorReturnsFalse(t *testing.T) {
+func TestRetriableErrorReturnsTrue(t *testing.T) {
 	err := CannotStopContainerError{errors.New("error")}
-	assert.False(t, err.IsUnretriableError(), "Non unretriable error treated as unretriable docker error")
+	assert.True(t, err.IsRetriableError(), "Non unretriable error treated as unretriable docker error")
 }

agent/engine/task_manager.go

@@ -255,7 +255,7 @@ func (mtask *managedTask) handleContainerChange(containerChange dockerContainerC
 			// If docker returned a transient error while trying to stop a container,
 			// reset the known status to the current status and return
 			cannotStopContainerError, ok := event.Error.(cannotStopContainerError)
-			if ok && !cannotStopContainerError.IsUnretriableError() {
+			if ok && cannotStopContainerError.IsRetriableError() {
 				seelog.Infof("Error stopping the container, ignoring state change; error: %s, task: %v",
 					cannotStopContainerError.Error(), mtask.Task)
 				container.SetKnownStatus(currentKnownStatus)