diff --git a/agent/taskresource/volume/dockervolume.go b/agent/taskresource/volume/dockervolume.go index 0a480eb89b7..2a90966655e 100644 --- a/agent/taskresource/volume/dockervolume.go +++ b/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 diff --git a/agent/taskresource/volume/dockervolume_test.go b/agent/taskresource/volume/dockervolume_test.go index bf26ec2e84b..b56cc088158 100644 --- a/agent/taskresource/volume/dockervolume_test.go +++ b/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,7 +78,7 @@ 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()) @@ -86,6 +86,8 @@ func TestCreateError(t *testing.T) { 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,9 +113,10 @@ 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" @@ -121,7 +124,9 @@ func TestCleanupError(t *testing.T) { 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) }