Merge pull request #1675 from ConnorJC3/fix-device-names-non-nitro

Reorder device names to prevent bad behavior on non-nitro instance types
kubernetes-sigs · Jul 12, 2023 · f9cc230 · f9cc230
2 parents 52a655b + d841964
commit f9cc230
Show file tree

Hide file tree

Showing 2 changed files with 73 additions and 65 deletions.
diff --git a/pkg/cloud/cloud_test.go b/pkg/cloud/cloud_test.go
@@ -41,7 +41,7 @@ const (
 	snowZone        = "snow"
 	defaultVolumeID = "vol-test-1234"
 	defaultNodeID   = "node-1234"
-	defaultPath     = "/dev/xvdb"
+	defaultPath     = "/dev/xvdaa"
 )
 
 func TestCreateDisk(t *testing.T) {
@@ -1892,7 +1892,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeAttachedState,
 			expectedInstance: "1234",
-			expectedDevice:   "/dev/xvdb",
+			expectedDevice:   defaultPath,
 			alreadyAssigned:  false,
 			expectError:      false,
 		},
@@ -1901,7 +1901,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeDetachedState,
 			expectedInstance: "1234",
-			expectedDevice:   "/dev/xvdb",
+			expectedDevice:   defaultPath,
 			alreadyAssigned:  false,
 			expectError:      false,
 		},
@@ -1910,7 +1910,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeDetachedState,
 			expectedInstance: "1234",
-			expectedDevice:   "/dev/xvdb",
+			expectedDevice:   defaultPath,
 			alreadyAssigned:  false,
 			expectError:      false,
 		},
@@ -1919,7 +1919,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeAttachedState,
 			expectedInstance: "1234",
-			expectedDevice:   "/dev/xvdb",
+			expectedDevice:   defaultPath,
 			alreadyAssigned:  false,
 			expectError:      true,
 		},
@@ -1928,7 +1928,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeAttachedState,
 			expectedInstance: "1234",
-			expectedDevice:   "/dev/xvdc",
+			expectedDevice:   "/dev/xvdab",
 			alreadyAssigned:  false,
 			expectError:      true,
 		},
@@ -1937,7 +1937,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeAttachedState,
 			expectedInstance: "1235",
-			expectedDevice:   "/dev/xvdb",
+			expectedDevice:   defaultPath,
 			alreadyAssigned:  false,
 			expectError:      true,
 		},
@@ -1946,7 +1946,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeAttachedState,
 			expectedInstance: "1234",
-			expectedDevice:   "/dev/xvdb",
+			expectedDevice:   defaultPath,
 			alreadyAssigned:  true,
 			expectError:      true,
 		},
@@ -1955,7 +1955,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeAttachedState,
 			expectedInstance: "1234",
-			expectedDevice:   "/dev/xvdb",
+			expectedDevice:   defaultPath,
 			alreadyAssigned:  false,
 			expectError:      false,
 		},
@@ -1964,7 +1964,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeAttachedState,
 			expectedInstance: "1234",
-			expectedDevice:   "/dev/xvdb",
+			expectedDevice:   defaultPath,
 			alreadyAssigned:  false,
 			expectError:      true,
 		},
@@ -1973,7 +1973,7 @@ func TestWaitForAttachmentState(t *testing.T) {
 			volumeID:         "vol-test-1234",
 			expectedState:    volumeAttachedState,
 			expectedInstance: "1234",
-			expectedDevice:   "/dev/xvdb",
+			expectedDevice:   defaultPath,
 			alreadyAssigned:  false,
 			expectError:      true,
 		},
@@ -1989,22 +1989,22 @@ func TestWaitForAttachmentState(t *testing.T) {
 
 			attachedVol := &ec2.Volume{
 				VolumeId:    aws.String(tc.volumeID),
-				Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdb"), InstanceId: aws.String("1234"), State: aws.String("attached")}},
+				Attachments: []*ec2.VolumeAttachment{{Device: aws.String(defaultPath), InstanceId: aws.String("1234"), State: aws.String("attached")}},
 			}
 
 			attachingVol := &ec2.Volume{
 				VolumeId:    aws.String(tc.volumeID),
-				Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdb"), InstanceId: aws.String("1234"), State: aws.String("attaching")}},
+				Attachments: []*ec2.VolumeAttachment{{Device: aws.String(defaultPath), InstanceId: aws.String("1234"), State: aws.String("attaching")}},
 			}
 
 			detachedVol := &ec2.Volume{
 				VolumeId:    aws.String(tc.volumeID),
-				Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdb"), InstanceId: aws.String("1234"), State: aws.String("detached")}},
+				Attachments: []*ec2.VolumeAttachment{{Device: aws.String(defaultPath), InstanceId: aws.String("1234"), State: aws.String("detached")}},
 			}
 
 			multipleAttachmentsVol := &ec2.Volume{
 				VolumeId:    aws.String(tc.volumeID),
-				Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdb"), InstanceId: aws.String("1235"), State: aws.String("attached")}, {Device: aws.String("/dev/xvdb"), InstanceId: aws.String("1234"), State: aws.String("attached")}},
+				Attachments: []*ec2.VolumeAttachment{{Device: aws.String(defaultPath), InstanceId: aws.String("1235"), State: aws.String("attached")}, {Device: aws.String(defaultPath), InstanceId: aws.String("1234"), State: aws.String("attached")}},
 			}
 
 			ctx, cancel := context.WithCancel(context.Background())

diff --git a/pkg/cloud/devicemanager/device_names.go b/pkg/cloud/devicemanager/device_names.go
@@ -21,57 +21,15 @@ package devicemanager
 // /dev/xvda is broken on Windows (despite the API allowing it)
 // /dev/xvddx is the last allowed device name in the /dev/xvd{a-z}{a-z} series
 // /dev/xvdc{a-z} don't work on some Windows instance types
+//
+// These names are ordered such that /dev/xvda{a-z} and /dev/xvdb{a-z} are
+// the first 52 names in the list. This is intentional, so that those names
+// are always chosen on non-nitro instances. Non-nitro instances have weird
+// behavior with /dev/sd{a-z} and /dev/xvd{a-z} that cause such names to
+// conflict and prevent each other from mounting. Because non-nitro instances
+// are limited to 39 volumes, and are long-term deprecated in favor of nitro,
+// this should be long-term safe.
 var deviceNames = []string{
-	"/dev/xvdb",
-	"/dev/xvdc",
-	"/dev/xvdd",
-	"/dev/xvde",
-	"/dev/xvdf",
-	"/dev/xvdg",
-	"/dev/xvdh",
-	"/dev/xvdi",
-	"/dev/xvdj",
-	"/dev/xvdk",
-	"/dev/xvdl",
-	"/dev/xvdm",
-	"/dev/xvdn",
-	"/dev/xvdo",
-	"/dev/xvdp",
-	"/dev/xvdq",
-	"/dev/xvdr",
-	"/dev/xvds",
-	"/dev/xvdt",
-	"/dev/xvdu",
-	"/dev/xvdv",
-	"/dev/xvdw",
-	"/dev/xvdx",
-	"/dev/xvdy",
-	"/dev/xvdz",
-	"/dev/sdb",
-	"/dev/sdc",
-	"/dev/sdd",
-	"/dev/sde",
-	"/dev/sdf",
-	"/dev/sdg",
-	"/dev/sdh",
-	"/dev/sdi",
-	"/dev/sdj",
-	"/dev/sdk",
-	"/dev/sdl",
-	"/dev/sdm",
-	"/dev/sdn",
-	"/dev/sdo",
-	"/dev/sdp",
-	"/dev/sdq",
-	"/dev/sdr",
-	"/dev/sds",
-	"/dev/sdt",
-	"/dev/sdu",
-	"/dev/sdv",
-	"/dev/sdw",
-	"/dev/sdx",
-	"/dev/sdy",
-	"/dev/sdz",
 	"/dev/xvdaa",
 	"/dev/xvdab",
 	"/dev/xvdac",
@@ -148,5 +106,55 @@ var deviceNames = []string{
 	"/dev/xvddv",
 	"/dev/xvddw",
 	"/dev/xvddx",
+	"/dev/xvdb",
+	"/dev/xvdc",
+	"/dev/xvdd",
+	"/dev/xvde",
+	"/dev/xvdf",
+	"/dev/xvdg",
+	"/dev/xvdh",
+	"/dev/xvdi",
+	"/dev/xvdj",
+	"/dev/xvdk",
+	"/dev/xvdl",
+	"/dev/xvdm",
+	"/dev/xvdn",
+	"/dev/xvdo",
+	"/dev/xvdp",
+	"/dev/xvdq",
+	"/dev/xvdr",
+	"/dev/xvds",
+	"/dev/xvdt",
+	"/dev/xvdu",
+	"/dev/xvdv",
+	"/dev/xvdw",
+	"/dev/xvdx",
+	"/dev/xvdy",
+	"/dev/xvdz",
+	"/dev/sdb",
+	"/dev/sdc",
+	"/dev/sdd",
+	"/dev/sde",
+	"/dev/sdf",
+	"/dev/sdg",
+	"/dev/sdh",
+	"/dev/sdi",
+	"/dev/sdj",
+	"/dev/sdk",
+	"/dev/sdl",
+	"/dev/sdm",
+	"/dev/sdn",
+	"/dev/sdo",
+	"/dev/sdp",
+	"/dev/sdq",
+	"/dev/sdr",
+	"/dev/sds",
+	"/dev/sdt",
+	"/dev/sdu",
+	"/dev/sdv",
+	"/dev/sdw",
+	"/dev/sdx",
+	"/dev/sdy",
+	"/dev/sdz",
 	"/dev/sda2",
 }