Prevent allocation of devices after /dev/xvddx

EBS has undocumented validation on the device that prevents the use of device names after /dev/xvddx. Also, expand the beginning of the device pool to account for this smaller pool of device names. Signed-off-by: Connor Catlett <[email protected]>
kubernetes-sigs · Mar 10, 2023 · 42e61c7 · 42e61c7
1 parent 58fc07e
commit 42e61c7
Show file tree

Hide file tree

Showing 5 changed files with 37 additions and 27 deletions.
diff --git a/Makefile b/Makefile
@@ -35,8 +35,8 @@ OS?=linux
 ARCH?=amd64
 OSVERSION?=amazon
 
-ALL_OS?=linux windows
-ALL_ARCH_linux?=amd64 arm64
+ALL_OS?=linux
+ALL_ARCH_linux?=amd64
 ALL_OSVERSION_linux?=amazon
 ALL_OS_ARCH_OSVERSION_linux=$(foreach arch, $(ALL_ARCH_linux), $(foreach osversion, ${ALL_OSVERSION_linux}, linux-$(arch)-${osversion}))
 

diff --git a/pkg/cloud/cloud_test.go b/pkg/cloud/cloud_test.go
@@ -721,7 +721,7 @@ func TestAttachDisk(t *testing.T) {
 
 			vol := &ec2.Volume{
 				VolumeId:    aws.String(tc.volumeID),
-				Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdba"), InstanceId: aws.String("node-1234"), State: aws.String("attached")}},
+				Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdaa"), InstanceId: aws.String("node-1234"), State: aws.String("attached")}},
 			}
 
 			ctx := context.Background()
@@ -1614,7 +1614,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeAttachedState,
 			expectedInstance: "1234",
-			expectedDevice:   "/dev/xvdba",
+			expectedDevice:   "/dev/xvdaa",
 			alreadyAssigned:  false,
 			expectError:      false,
 		},
@@ -1623,7 +1623,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeDetachedState,
 			expectedInstance: "1234",
-			expectedDevice:   "/dev/xvdba",
+			expectedDevice:   "/dev/xvdaa",
 			alreadyAssigned:  false,
 			expectError:      false,
 		},
@@ -1632,7 +1632,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeDetachedState,
 			expectedInstance: "1234",
-			expectedDevice:   "/dev/xvdba",
+			expectedDevice:   "/dev/xvdaa",
 			alreadyAssigned:  false,
 			expectError:      false,
 		},
@@ -1641,7 +1641,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeAttachedState,
 			expectedInstance: "1234",
-			expectedDevice:   "/dev/xvdba",
+			expectedDevice:   "/dev/xvdaa",
 			alreadyAssigned:  false,
 			expectError:      true,
 		},
@@ -1650,7 +1650,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeAttachedState,
 			expectedInstance: "1234",
-			expectedDevice:   "/dev/xvdbb",
+			expectedDevice:   "/dev/xvdab",
 			alreadyAssigned:  false,
 			expectError:      true,
 		},
@@ -1659,7 +1659,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeAttachedState,
 			expectedInstance: "1235",
-			expectedDevice:   "/dev/xvdba",
+			expectedDevice:   "/dev/xvdaa",
 			alreadyAssigned:  false,
 			expectError:      true,
 		},
@@ -1668,7 +1668,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeAttachedState,
 			expectedInstance: "1234",
-			expectedDevice:   "/dev/xvdba",
+			expectedDevice:   "/dev/xvdaa",
 			alreadyAssigned:  true,
 			expectError:      true,
 		},
@@ -1677,7 +1677,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeAttachedState,
 			expectedInstance: "1234",
-			expectedDevice:   "/dev/xvdba",
+			expectedDevice:   "/dev/xvdaa",
 			alreadyAssigned:  false,
 			expectError:      false,
 		},
@@ -1686,7 +1686,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeAttachedState,
 			expectedInstance: "1234",
-			expectedDevice:   "/dev/xvdba",
+			expectedDevice:   "/dev/xvdaa",
 			alreadyAssigned:  false,
 			expectError:      true,
 		},
@@ -1702,22 +1702,22 @@ func TestWaitForAttachmentState(t *testing.T) {
 
 			attachedVol := &ec2.Volume{
 				VolumeId:    aws.String(tc.volumeID),
-				Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdba"), InstanceId: aws.String("1234"), State: aws.String("attached")}},
+				Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdaa"), InstanceId: aws.String("1234"), State: aws.String("attached")}},
 			}
 
 			attachingVol := &ec2.Volume{
 				VolumeId:    aws.String(tc.volumeID),
-				Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdba"), InstanceId: aws.String("1234"), State: aws.String("attaching")}},
+				Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdaa"), InstanceId: aws.String("1234"), State: aws.String("attaching")}},
 			}
 
 			detachedVol := &ec2.Volume{
 				VolumeId:    aws.String(tc.volumeID),
-				Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdba"), InstanceId: aws.String("1234"), State: aws.String("detached")}},
+				Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdaa"), InstanceId: aws.String("1234"), State: aws.String("detached")}},
 			}
 
 			multipleAttachmentsVol := &ec2.Volume{
 				VolumeId:    aws.String(tc.volumeID),
-				Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdba"), InstanceId: aws.String("1235"), State: aws.String("attached")}, {Device: aws.String("/dev/xvdba"), InstanceId: aws.String("1234"), State: aws.String("attached")}},
+				Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdaa"), InstanceId: aws.String("1235"), State: aws.String("attached")}, {Device: aws.String("/dev/xvdaa"), InstanceId: aws.String("1234"), State: aws.String("attached")}},
 			}
 
 			ctx := context.Background()

diff --git a/pkg/cloud/devicemanager/allocator.go b/pkg/cloud/devicemanager/allocator.go
@@ -46,12 +46,18 @@ var _ NameAllocator = &nameAllocator{}
 // GetNext gets next available device given existing names that are being used
 // This function iterate through the device names in deterministic order of:
 //
-//	ba, ..., bz, ca, ..., cz, ..., ..., zz
+//	aa, ..., az, ba, ..., bz, ..., ..., dx
 //
 // and return the first one that is not used yet.
+// We stop at dx because EBS performs undocumented validation on the device
+// name that refuses to mount devices after /dev/xvddx
 func (d *nameAllocator) GetNext(existingNames ExistingNames, prefix string) (string, error) {
-	for c1 := 'b'; c1 <= 'z'; c1++ {
-		for c2 := 'a'; c2 <= 'z'; c2++ {
+	for c1 := 'a'; c1 <= 'd'; c1++ {
+		c2end := 'z'
+		if c1 == 'd' {
+			c2end = 'x'
+		}
+		for c2 := 'a'; c2 <= c2end; c2++ {
 			name := fmt.Sprintf("%s%s%s", prefix, string(c1), string(c2))
 			if _, found := existingNames[name]; !found {
 				return name, nil

diff --git a/pkg/cloud/devicemanager/allocator_test.go b/pkg/cloud/devicemanager/allocator_test.go
@@ -27,12 +27,18 @@ func TestNameAllocator(t *testing.T) {
 	tests := []struct {
 		expectedName string
 	}{
+		{"aa"}, {"ab"}, {"ac"}, {"ad"}, {"ae"}, {"af"}, {"ag"}, {"ah"}, {"ai"}, {"aj"},
+		{"ak"}, {"al"}, {"am"}, {"an"}, {"ao"}, {"ap"}, {"aq"}, {"ar"}, {"as"}, {"at"},
+		{"au"}, {"av"}, {"aw"}, {"ax"}, {"ay"}, {"az"},
 		{"ba"}, {"bb"}, {"bc"}, {"bd"}, {"be"}, {"bf"}, {"bg"}, {"bh"}, {"bi"}, {"bj"},
 		{"bk"}, {"bl"}, {"bm"}, {"bn"}, {"bo"}, {"bp"}, {"bq"}, {"br"}, {"bs"}, {"bt"},
 		{"bu"}, {"bv"}, {"bw"}, {"bx"}, {"by"}, {"bz"},
 		{"ca"}, {"cb"}, {"cc"}, {"cd"}, {"ce"}, {"cf"}, {"cg"}, {"ch"}, {"ci"}, {"cj"},
 		{"ck"}, {"cl"}, {"cm"}, {"cn"}, {"co"}, {"cp"}, {"cq"}, {"cr"}, {"cs"}, {"ct"},
 		{"cu"}, {"cv"}, {"cw"}, {"cx"}, {"cy"}, {"cz"},
+		{"da"}, {"db"}, {"dc"}, {"dd"}, {"de"}, {"df"}, {"dg"}, {"dh"}, {"di"}, {"dj"},
+		{"dk"}, {"dl"}, {"dm"}, {"dn"}, {"do"}, {"dp"}, {"dq"}, {"dr"}, {"ds"}, {"dt"},
+		{"du"}, {"dv"}, {"dw"}, {"dx"},
 	}
 
 	for _, test := range tests {
@@ -53,10 +59,8 @@ func TestNameAllocatorError(t *testing.T) {
 	allocator := nameAllocator{}
 	existingNames := map[string]string{}
 
-	// Allocator goes from ba, bb, bc, ..., bz, ca, ..., cz, ..., ..., zz
-	// Thus 25*26 for the maximum number of allocatable volumes (25 for the
-	// first letter as it starts on 'b'
-	for i := 0; i < 25*26; i++ {
+	// 102 == number of allocations from aa ... dx (see allocator.go for why we stop at dx)
+	for i := 0; i < 102; i++ {
 		name, _ := allocator.GetNext(existingNames, "/dev/xvd")
 		existingNames[name] = ""
 	}

diff --git a/pkg/cloud/devicemanager/manager_test.go b/pkg/cloud/devicemanager/manager_test.go
@@ -103,16 +103,16 @@ func TestNewDeviceWithExistingDevice(t *testing.T) {
 		{
 			name:         "success: different volumes",
 			existingID:   "vol-1",
-			existingPath: "/dev/xvdba",
+			existingPath: "/dev/xvdaa",
 			volumeID:     "vol-2",
-			expectedPath: "/dev/xvdbb",
+			expectedPath: "/dev/xvdab",
 		},
 		{
 			name:         "success: same volumes",
 			existingID:   "vol-1",
-			existingPath: "/dev/xvdba",
+			existingPath: "/dev/xvdcc",
 			volumeID:     "vol-1",
-			expectedPath: "/dev/xvdba",
+			expectedPath: "/dev/xvdcc",
 		},
 		{
 			name:         "success: same volumes with /dev/sdX path",