Merge pull request #41455 from gnufied/fix-aws-device-allocator

Automatic merge from submit-queue

Fix AWS device allocator to only use valid device names

According to
http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/device_naming.html
we can only use /dev/xvd[b-c][a-z] as device names - so we can only
allocate upto 52 ebs volumes on a node.

fixes #41453 

cc @justinsb  @kubernetes/sig-storage-pr-reviews

pkg/cloudprovider/providers/aws/aws.go

@@ -1223,9 +1223,10 @@ func (c *Cloud) getMountDevice(
 	// Find the next unused device name
 	deviceAllocator := c.deviceAllocators[i.nodeName]
 	if deviceAllocator == nil {
-		// we want device names with two significant characters, starting with
+		// we want device names with two significant characters, starting with /dev/xvdbb
-		// /dev/xvdba (leaving xvda - xvdz and xvdaa-xvdaz to the system)
+		// the allowed range is /dev/xvd[b-c][a-z]
-		deviceAllocator = NewDeviceAllocator(2, "ba")
+		// http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/device_naming.html
+		deviceAllocator = NewDeviceAllocator(0)
 		c.deviceAllocators[i.nodeName] = deviceAllocator
 	}
 	chosen, err := deviceAllocator.GetNext(deviceMappings)

pkg/cloudprovider/providers/aws/device_allocator.go

@@ -43,53 +43,45 @@ type DeviceAllocator interface {
 }
 
 type deviceAllocator struct {
-	firstDevice        mountDevice
+	possibleDevices []mountDevice
-	lastAssignedDevice mountDevice
+	lastIndex       int
-	length             int
 }
 
-// NewDeviceAllocator creates new DeviceAlllocator that allocates device names
+// Allocates device names according to scheme ba..bz, ca..cz
-// of given length ("aaa" for length 3) and with given first device, so all
+// it moves along the ring and always picks next device until
-// devices before the first device are left to the operating system.
+// device list is exhausted.
-// With length 2 and firstDevice "ba", it will allocate device names
+func NewDeviceAllocator(lastIndex int) DeviceAllocator {
-// ba, bb, ..., bz, ca, ... cz, ..., da, ... zz, so a..z and aa..az can be used
+	possibleDevices := []mountDevice{}
-// by the operating system.
+	for _, firstChar := range []rune{'b', 'c'} {
-func NewDeviceAllocator(length int, firstDevice mountDevice) DeviceAllocator {
+		for i := 'a'; i <= 'z'; i++ {
-	lastDevice := make([]byte, length)
+			dev := mountDevice([]rune{firstChar, i})
-	for i := 0; i < length; i++ {
+			possibleDevices = append(possibleDevices, dev)
-		lastDevice[i] = 'z'
+		}
 	}
 	return &deviceAllocator{
-		firstDevice:        firstDevice,
+		possibleDevices: possibleDevices,
-		lastAssignedDevice: mountDevice(lastDevice),
+		lastIndex:       lastIndex,
-		length:             length,
 	}
 }
 
 func (d *deviceAllocator) GetNext(existingDevices ExistingDevices) (mountDevice, error) {
-	candidate := d.lastAssignedDevice
+	var candidate mountDevice
-
+	foundIndex := d.lastIndex
 	for {
-		candidate = d.nextDevice(candidate)
+		candidate, foundIndex = d.nextDevice(foundIndex + 1)
 		if _, found := existingDevices[candidate]; !found {
-			d.lastAssignedDevice = candidate
+			d.lastIndex = foundIndex
 			return candidate, nil
 		}
-		if candidate == d.lastAssignedDevice {
+		if foundIndex == d.lastIndex {
 			return "", fmt.Errorf("no devices are available")
 		}
 	}
 }
 
-func (d *deviceAllocator) nextDevice(device mountDevice) mountDevice {
+func (d *deviceAllocator) nextDevice(nextIndex int) (mountDevice, int) {
-	dev := []byte(device)
+	if nextIndex < len(d.possibleDevices) {
-	for i := d.length - 1; i >= 0; i-- {
+		return d.possibleDevices[nextIndex], nextIndex
-		if dev[i] != 'z' {
-			dev[i]++
-			return mountDevice(dev)
 	}
-		dev[i] = 'a'
+	return d.possibleDevices[0], 0
-	}
-	// all parts of device were 'z', jump to the first device
-	return d.firstDevice
 }

pkg/cloudprovider/providers/aws/device_allocator_test.go

@@ -22,56 +22,37 @@ func TestDeviceAllocator(t *testing.T) {
 	tests := []struct {
 		name            string
 		existingDevices ExistingDevices
-		length          int
+		lastIndex       int
-		firstDevice     mountDevice
-		lastAllocated   mountDevice
 		expectedOutput  mountDevice
 	}{
 		{
 			"empty device list",
 			ExistingDevices{},
-			2,
+			0,
-			"aa",
+			"bb",
-			"aa",
-			"ab",
 		},
 		{
 			"empty device list with wrap",
 			ExistingDevices{},
-			2,
+			51,
-			"ba",
-			"zz",
 			"ba", // next to 'zz' is the first one, 'ba'
 		},
 		{
 			"device list",
-			ExistingDevices{"aa": "used", "ab": "used", "ac": "used"},
+			ExistingDevices{"ba": "used", "bb": "used", "bc": "used"},
-			2,
+			0,
-			"aa",
+			"bd",
-			"aa",
-			"ad", // all up to "ac" are used
 		},
 		{
 			"device list with wrap",
-			ExistingDevices{"zy": "used", "zz": "used", "ba": "used"},
+			ExistingDevices{"cy": "used", "cz": "used", "ba": "used"},
-			2,
+			49,
-			"ba",
+			"bb", // "cy", "cz" and "ba" are used
-			"zx",
-			"bb", // "zy", "zz" and "ba" are used
-		},
-		{
-			"three characters with wrap",
-			ExistingDevices{"zzy": "used", "zzz": "used", "baa": "used"},
-			3,
-			"baa",
-			"zzx",
-			"bab",
 		},
 	}
 
 	for _, test := range tests {
-		allocator := NewDeviceAllocator(test.length, test.firstDevice).(*deviceAllocator)
+		allocator := NewDeviceAllocator(test.lastIndex).(*deviceAllocator)
-		allocator.lastAssignedDevice = test.lastAllocated
 
 		got, err := allocator.GetNext(test.existingDevices)
 		if err != nil {
@@ -84,12 +65,12 @@ func TestDeviceAllocator(t *testing.T) {
 }
 
 func TestDeviceAllocatorError(t *testing.T) {
-	allocator := NewDeviceAllocator(2, "ba").(*deviceAllocator)
+	allocator := NewDeviceAllocator(0).(*deviceAllocator)
 	existingDevices := ExistingDevices{}
 
 	// make all devices used
 	var first, second byte
-	for first = 'b'; first <= 'z'; first++ {
+	for first = 'b'; first <= 'c'; first++ {
 		for second = 'a'; second <= 'z'; second++ {
 			device := [2]byte{first, second}
 			existingDevices[mountDevice(device[:])] = "used"