taskresources: set terminal reason msg for all errors (#2004)

* taskresources: set terminal reason msg for all errors

closes #1631

return a default terminal reason

remove terminalReason code for cgroups based on PR review

Test that volume errors are propogated

* fix cleanup error test and add string check

agent/taskresource/volume/dockervolume.go

@@ -34,11 +34,10 @@ const (
 	// SharedScope indicates that the volume's lifecycle is outside the scope of task
 	SharedScope = "shared"
 	// DockerLocalVolumeDriver is the name of the docker default volume driver
-	DockerLocalVolumeDriver = "local"
+	DockerLocalVolumeDriver   = "local"
+	resourceProvisioningError = "VolumeError: Agent could not create task's volume resources"
 )
 
-const resourceProvisioningError = "VolumeError: Agent could not create task's volume resources"
-
 // VolumeResource represents volume resource
 type VolumeResource struct {
 	// Name is the name of the docker volume
@@ -56,6 +55,12 @@ type VolumeResource struct {
 	statusToTransitions map[resourcestatus.ResourceStatus]func() error
 	client              dockerapi.DockerClient
 	ctx                 context.Context
+
+	// terminalReason should be set for resource creation failures. This ensures
+	// the resource object carries some context for why provisoning failed.
+	terminalReason     string
+	terminalReasonOnce sync.Once
+
 	// lock is used for fields that are accessed and updated concurrently
 	lock sync.RWMutex
 }
@@ -147,7 +152,17 @@ func (vol *VolumeResource) DesiredTerminal() bool {
 // GetTerminalReason returns an error string to propagate up through to task
 // state change messages
 func (vol *VolumeResource) GetTerminalReason() string {
-	return resourceProvisioningError
+	if vol.terminalReason == "" {
+		return resourceProvisioningError
+	}
+	return vol.terminalReason
+}
+
+func (vol *VolumeResource) setTerminalReason(reason string) {
+	vol.terminalReasonOnce.Do(func() {
+		seelog.Infof("Volume Resource [%s]: setting terminal reason for volume resource", vol.Name)
+		vol.terminalReason = reason
+	})
 }
 
 // SetDesiredStatus safely sets the desired status of the resource
@@ -210,8 +225,10 @@ func (vol *VolumeResource) SteadyState() resourcestatus.ResourceStatus {
 func (vol *VolumeResource) ApplyTransition(nextState resourcestatus.ResourceStatus) error {
 	transitionFunc, ok := vol.statusToTransitions[nextState]
 	if !ok {
-		return errors.Errorf("volume [%s]: transition to %s impossible", vol.Name,
+		errW := errors.Errorf("volume [%s]: transition to %s impossible", vol.Name,
 			vol.StatusString(nextState))
+		vol.setTerminalReason(errW.Error())
+		return errW
 	}
 	return transitionFunc()
 }
@@ -289,6 +306,7 @@ func (vol *VolumeResource) Create() error {
 		dockerclient.CreateVolumeTimeout)
 
 	if volumeResponse.Error != nil {
+		vol.setTerminalReason(volumeResponse.Error.Error())
 		return volumeResponse.Error
 	}
 
@@ -309,6 +327,7 @@ func (vol *VolumeResource) Cleanup() error {
 	err := vol.client.RemoveVolume(vol.ctx, vol.VolumeConfig.DockerVolumeName, dockerclient.RemoveVolumeTimeout)
 
 	if err != nil {
+		vol.setTerminalReason(err.Error())
 		return err
 	}
 	return nil

agent/taskresource/volume/dockervolume_test.go

@@ -24,7 +24,7 @@ import (
 
 	"github.com/aws/amazon-ecs-agent/agent/dockerclient"
 	"github.com/aws/amazon-ecs-agent/agent/dockerclient/dockerapi"
-	"github.com/aws/amazon-ecs-agent/agent/dockerclient/dockerapi/mocks"
+	mock_dockerapi "github.com/aws/amazon-ecs-agent/agent/dockerclient/dockerapi/mocks"
 	resourcestatus "github.com/aws/amazon-ecs-agent/agent/taskresource/status"
 
 	"github.com/docker/docker/api/types"
@@ -78,14 +78,16 @@ func TestCreateError(t *testing.T) {
 	mockClient.EXPECT().CreateVolume(gomock.Any(), name, driver, nil, labels, dockerclient.CreateVolumeTimeout).Return(
 		dockerapi.SDKVolumeResponse{
 			DockerVolume: nil,
-			Error:        errors.New("some error"),
+			Error:        errors.New("Test this error is propogated"),
 		})
 
 	ctx, cancel := context.WithCancel(context.TODO())
 	defer cancel()
 	volume, _ := NewVolumeResource(ctx, name, name, scope, autoprovision, driver, nil, labels, mockClient)
 	err := volume.Create()
 	assert.NotNil(t, err)
+	assert.Equal(t, "Test this error is propogated", err.Error())
+	assert.Equal(t, "Test this error is propogated", volume.GetTerminalReason())
 }
 
 func TestCleanupSuccess(t *testing.T) {
@@ -111,17 +113,20 @@ func TestCleanupError(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
 	mockClient := mock_dockerapi.NewMockDockerClient(ctrl)
+	mockClient.EXPECT().RemoveVolume(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("Test this is propogated"))
 
 	name := "volumeName"
-	scope := "shared"
+	scope := "task"
 	autoprovision := false
 	driver := "driver"
 
 	ctx, cancel := context.WithCancel(context.TODO())
 	defer cancel()
 	volume, _ := NewVolumeResource(ctx, name, name, scope, autoprovision, driver, nil, nil, mockClient)
 	err := volume.Cleanup()
-	assert.Nil(t, err)
+	assert.Error(t, err)
+	assert.Equal(t, "Test this is propogated", err.Error())
+	assert.Equal(t, "Test this is propogated", volume.GetTerminalReason())
 }
 
 func TestApplyTransitionForTaskScopeVolume(t *testing.T) {
@@ -234,7 +239,7 @@ func TestNewVolumeResource(t *testing.T) {
 		fail          bool
 	}{
 		{
-			"task scoped volume can be non-auto provisioned",
+			"task scoped volume can not be auto provisioned",
 			"task",
 			true,
 			true,
@@ -262,10 +267,12 @@ func TestNewVolumeResource(t *testing.T) {
 	for _, testcase := range testCases {
 		t.Run(fmt.Sprintf("%s,scope %s, autoprovision: %v", testcase.description,
 			testcase.scope, testcase.autoprovision), func(t *testing.T) {
-			_, err := NewVolumeResource(nil, "volume", "dockerVolume",
+			vol, err := NewVolumeResource(nil, "volume", "dockerVolume",
 				testcase.scope, testcase.autoprovision, "", nil, nil, nil)
 			if testcase.fail {
 				assert.Error(t, err)
+				assert.Nil(t, vol)
+				assert.Contains(t, err.Error(), "task scoped volume could not be autoprovisioned")
 			} else {
 				assert.NoError(t, err)
 			}