Repository: kubernetes-sigs/aws-ebs-csi-driver
Pull request #2183 (2024-10-14 14:48:11)

Only bail to AttachVolume if volume is detached
**Is this a bug fix or adding new feature?**

Bug fix

**What is this PR about? / Why do we need it?**

In `WaitForAttachmentState`, we bail to calling `AttachVolume` and immediately returning an error if the volume was reported as attached by `DescribeInstances` (i.e. if `alreadyAssigned` is true), but the volume returns a state other than `attached`. This check exists to guard against the case where `DescribeInstances` is incorrect, bailing early allows us to get the attachment moving and avoid sitting in an infinite loop until a timeout occurs.

However, this is incorrect - we should only perform this action if the volume is `detached`, not all states other than `attached`. This is for two reasons:

1. Making an `AttachVolume` call when the volume is in a state other than `detached` will almost always fail.
2. Due to eventual consistency, it is reasonably common some volumes will (correctly) report as attached via `DescribeInstances` but still report as `attaching` via `DescribeVolumes` for a short period of time. In this case, we repeatedly error out at the earliest possible opportunity, preventing `WaitForAttachmentState`'s exponential backoff from working and peppering the AWS API with spurious `AttachVolume` calls.

**What testing is done?** 

Added new unit test. Note that although depending on the test name is a horrible practice we should remove from this test, I have not spent the significant effort that would be required in this small PR to fix it.


Patch:
diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go
index da71ef13..8a483c93 100644
--- a/pkg/cloud/cloud.go
+++ b/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),


Test patch:
diff --git a/pkg/cloud/cloud_test.go b/pkg/cloud/cloud_test.go
index 4f43a1bc..84100c85 100644
--- a/pkg/cloud/cloud_test.go
+++ b/pkg/cloud/cloud_test.go
@@ -3301,7 +3301,7 @@ 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",
@@ -3309,6 +3309,15 @@ func TestWaitForAttachmentState(t *testing.T) {
 			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",