Merge pull request #910 from nmeyerhans/fix-error-type

Fix type assertion bug

agent/engine/docker_container_engine.go

@@ -552,7 +552,7 @@ func (dg *dockerGoClient) StopContainer(dockerID string, timeout time.Duration)
 		if err == context.DeadlineExceeded {
 			return DockerContainerMetadata{Error: &DockerTimeoutError{timeout, "stopped"}}
 		}
-		return DockerContainerMetadata{Error: &CannotStopContainerError{err}}
+		return DockerContainerMetadata{Error: CannotStopContainerError{err}}
 	}
 }
 

agent/engine/docker_task_engine_test.go

@@ -901,7 +901,7 @@ func TestTaskTransitionWhenStopContainerReturnsTransientErrorBeforeSucceeding(t
 	client.EXPECT().ContainerEvents(gomock.Any()).Return(eventStream, nil)
 	mockTime.EXPECT().After(gomock.Any()).AnyTimes()
 	containerStoppingError := DockerContainerMetadata{
-		Error: &CannotStopContainerError{errors.New("Error stopping container")},
+		Error: CannotStopContainerError{errors.New("Error stopping container")},
 	}
 	for _, container := range sleepTask.Containers {
 		gomock.InOrder(

agent/engine/errors.go

@@ -30,6 +30,11 @@ type engineError interface {
 	ErrorName() string
 }
 
+type cannotStopContainerError interface {
+	engineError
+	IsRetriableError() bool
+}
+
 // impossibleTransitionError is an error that occurs when an event causes a
 // container to try and transition to a state that it cannot be moved to
 type impossibleTransitionError struct {
@@ -154,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

@@ -254,8 +254,8 @@ 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() {
+			cannotStopContainerError, ok := event.Error.(cannotStopContainerError)
+			if ok && cannotStopContainerError.IsRetriableError() {
 				seelog.Infof("Error stopping the container, ignoring state change; error: %s, task: %v",
 					cannotStopContainerError.Error(), mtask.Task)
 				container.SetKnownStatus(currentKnownStatus)
@@ -266,8 +266,9 @@ func (mtask *managedTask) handleContainerChange(containerChange dockerContainerC
 			// clearly can't just continue trying to transition it to stopped
 			// again and again... In this case, assume it's stopped (or close
 			// enough) and get on with it
-			// This actually happens a lot for the case of stopping something that was not running.
-			llog.Info("Error for 'docker stop' of container; assuming it's stopped anyways", "err", event.Error)
+			// This can happen in cases where the container we tried to stop
+			// was already stopped or did not exist at all.
+			seelog.Warnf("'docker stop' returned %s: %s", event.Error.ErrorName(), event.Error.Error())
 			container.SetKnownStatus(api.ContainerStopped)
 			container.SetDesiredStatus(api.ContainerStopped)
 		} else if event.Status == api.ContainerPulled {