Merge pull request #2183 from ConnorJC3/fix-attachvolume-spam

Only bail to AttachVolume if volume is detached

pkg/cloud/cloud.go

@@ -1080,7 +1080,7 @@ func (c *cloud) WaitForAttachmentState(ctx context.Context, volumeID, expectedSt
 		// if we expected volume to be attached and it was reported as already attached via DescribeInstance call
 		// but DescribeVolume told us volume is detached, we will short-circuit this long wait loop and return error
 		// so as AttachDisk can be retried without waiting for 20 minutes.
-		if (expectedState == volumeAttachedState) && alreadyAssigned && (attachmentState != expectedState) {
+		if (expectedState == volumeAttachedState) && alreadyAssigned && (attachmentState == volumeDetachedState) {
 			request := &ec2.AttachVolumeInput{
 				Device:     aws.String(expectedDevice),
 				InstanceId: aws.String(expectedInstance),

pkg/cloud/cloud_test.go

@@ -3301,14 +3301,23 @@ func TestWaitForAttachmentState(t *testing.T) {
 			expectError:      true,
 		},
 		{
-			name:             "failure: already assigned but wrong state",
+			name:             "failure: already assigned but detached state",
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeAttachedState,
 			expectedInstance: "1234",
 			expectedDevice:   defaultPath,
 			alreadyAssigned:  true,
 			expectError:      true,
 		},
+		{
+			name:             "failure: already assigned but attaching state",
+			volumeID:         "vol-test-1234",
+			expectedState:    volumeAttachedState,
+			expectedInstance: "1234",
+			expectedDevice:   defaultPath,
+			alreadyAssigned:  true,
+			expectError:      false,
+		},
 		{
 			name:             "failure: multiple attachments with Multi-Attach disabled",
 			volumeID:         "vol-test-1234",
@@ -3353,9 +3362,12 @@ func TestWaitForAttachmentState(t *testing.T) {
 			switch tc.name {
 			case "success: detached":
 				mockEC2.EXPECT().DescribeVolumes(gomock.Any(), gomock.Any()).Return(&ec2.DescribeVolumesOutput{Volumes: []types.Volume{detachedVol}}, nil).AnyTimes()
-			case "failure: already assigned but wrong state":
+			case "failure: already assigned but detached state":
 				mockEC2.EXPECT().DescribeVolumes(gomock.Any(), gomock.Any()).Return(&ec2.DescribeVolumesOutput{Volumes: []types.Volume{detachedVol}}, nil)
 				mockEC2.EXPECT().AttachVolume(gomock.Any(), gomock.Any()).Return(nil, nil)
+			case "failure: already assigned but attaching state":
+				mockEC2.EXPECT().DescribeVolumes(gomock.Any(), gomock.Any()).Return(&ec2.DescribeVolumesOutput{Volumes: []types.Volume{attachingVol}}, nil)
+				mockEC2.EXPECT().DescribeVolumes(gomock.Any(), gomock.Any()).Return(&ec2.DescribeVolumesOutput{Volumes: []types.Volume{attachedVol}}, nil)
 			case "success: disk not found, assumed detached", "failure: disk not found, expected attached":
 				mockEC2.EXPECT().DescribeVolumes(gomock.Any(), gomock.Any()).Return(nil, &smithy.GenericAPIError{
 					Code:    "InvalidVolume.NotFound",