From 1a652a3f4e2b5987a7fa9ab46f6eb9ca3732cdd6 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Tue, 14 Feb 2017 21:37:20 -0500 Subject: [PATCH] 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. --- pkg/cloudprovider/providers/aws/aws.go | 7 +-- .../providers/aws/device_allocator.go | 54 ++++++++----------- .../providers/aws/device_allocator_test.go | 45 +++++----------- 3 files changed, 40 insertions(+), 66 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index 50cbbf0802dd..901bd43ed627 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/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 - // /dev/xvdba (leaving xvda - xvdz and xvdaa-xvdaz to the system) - deviceAllocator = NewDeviceAllocator(2, "ba") + // we want device names with two significant characters, starting with /dev/xvdbb + // the allowed range is /dev/xvd[b-c][a-z] + // http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/device_naming.html + deviceAllocator = NewDeviceAllocator(0) c.deviceAllocators[i.nodeName] = deviceAllocator } chosen, err := deviceAllocator.GetNext(deviceMappings) diff --git a/pkg/cloudprovider/providers/aws/device_allocator.go b/pkg/cloudprovider/providers/aws/device_allocator.go index ad78f931ad17..482d99bf5765 100644 --- a/pkg/cloudprovider/providers/aws/device_allocator.go +++ b/pkg/cloudprovider/providers/aws/device_allocator.go @@ -43,53 +43,45 @@ type DeviceAllocator interface { } type deviceAllocator struct { - firstDevice mountDevice - lastAssignedDevice mountDevice - length int + possibleDevices []mountDevice + lastIndex int } -// NewDeviceAllocator creates new DeviceAlllocator that allocates device names -// of given length ("aaa" for length 3) and with given first device, so all -// devices before the first device are left to the operating system. -// With length 2 and firstDevice "ba", it will allocate device names -// ba, bb, ..., bz, ca, ... cz, ..., da, ... zz, so a..z and aa..az can be used -// by the operating system. -func NewDeviceAllocator(length int, firstDevice mountDevice) DeviceAllocator { - lastDevice := make([]byte, length) - for i := 0; i < length; i++ { - lastDevice[i] = 'z' +// Allocates device names according to scheme ba..bz, ca..cz +// it moves along the ring and always picks next device until +// device list is exhausted. +func NewDeviceAllocator(lastIndex int) DeviceAllocator { + possibleDevices := []mountDevice{} + for _, firstChar := range []rune{'b', 'c'} { + for i := 'a'; i <= 'z'; i++ { + dev := mountDevice([]rune{firstChar, i}) + possibleDevices = append(possibleDevices, dev) + } } return &deviceAllocator{ - firstDevice: firstDevice, - lastAssignedDevice: mountDevice(lastDevice), - length: length, + possibleDevices: possibleDevices, + lastIndex: lastIndex, } } 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 { - dev := []byte(device) - for i := d.length - 1; i >= 0; i-- { - if dev[i] != 'z' { - dev[i]++ - return mountDevice(dev) - } - dev[i] = 'a' +func (d *deviceAllocator) nextDevice(nextIndex int) (mountDevice, int) { + if nextIndex < len(d.possibleDevices) { + return d.possibleDevices[nextIndex], nextIndex } - // all parts of device were 'z', jump to the first device - return d.firstDevice + return d.possibleDevices[0], 0 } diff --git a/pkg/cloudprovider/providers/aws/device_allocator_test.go b/pkg/cloudprovider/providers/aws/device_allocator_test.go index 801e2c4ebedf..aa377c7b488c 100644 --- a/pkg/cloudprovider/providers/aws/device_allocator_test.go +++ b/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 - firstDevice mountDevice - lastAllocated mountDevice + lastIndex int expectedOutput mountDevice }{ { "empty device list", ExistingDevices{}, - 2, - "aa", - "aa", - "ab", + 0, + "bb", }, { "empty device list with wrap", ExistingDevices{}, - 2, - "ba", - "zz", + 51, "ba", // next to 'zz' is the first one, 'ba' }, { "device list", - ExistingDevices{"aa": "used", "ab": "used", "ac": "used"}, - 2, - "aa", - "aa", - "ad", // all up to "ac" are used + ExistingDevices{"ba": "used", "bb": "used", "bc": "used"}, + 0, + "bd", }, { "device list with wrap", - ExistingDevices{"zy": "used", "zz": "used", "ba": "used"}, - 2, - "ba", - "zx", - "bb", // "zy", "zz" and "ba" are used - }, - { - "three characters with wrap", - ExistingDevices{"zzy": "used", "zzz": "used", "baa": "used"}, - 3, - "baa", - "zzx", - "bab", + ExistingDevices{"cy": "used", "cz": "used", "ba": "used"}, + 49, + "bb", // "cy", "cz" and "ba" are used }, } for _, test := range tests { - allocator := NewDeviceAllocator(test.length, test.firstDevice).(*deviceAllocator) - allocator.lastAssignedDevice = test.lastAllocated + allocator := NewDeviceAllocator(test.lastIndex).(*deviceAllocator) 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"