Refactor device path allocator

pkg/cloud/devicemanager/allocator.go

@@ -18,8 +18,6 @@ package devicemanager
 
 import (
 	"fmt"
-	"sort"
-	"sync"
 )
 
 // ExistingNames is a map of assigned device names. Presence of a key with a device
@@ -36,84 +34,30 @@ type ExistingNames map[string]string
 // device name to the previously assigned one (from previous NameAllocator
 // call), so all available device names are used eventually and it minimizes
 // device name reuse.
-// All these allocations are in-memory, nothing is written to / read from
-// /dev directory.
 type NameAllocator interface {
 	// GetNext returns a free device name or error when there is no free device
 	// name. Only the device name is returned, e.g. "ba" for "/dev/xvdba".
 	// It's up to the called to add appropriate "/dev/sd" or "/dev/xvd" prefix.
 	GetNext(existingNames ExistingNames) (name string, err error)
-
-	// Deprioritize the device name so as it can't be used immediately again
-	Deprioritize(chosen string)
 }
 
-type nameAllocator struct {
-	possibleNames map[string]int
-	counter       int
-	mux           sync.Mutex
-}
+type nameAllocator struct{}
 
 var _ NameAllocator = &nameAllocator{}
 
-type namePair struct {
-	name  string
-	index int
-}
-
-type namePairList []namePair
-
-func (p namePairList) Len() int           { return len(p) }
-func (p namePairList) Less(i, j int) bool { return p[i].index < p[j].index }
-func (p namePairList) Swap(i, j int)      { p[i], p[j] = p[j], p[i] }
-
-// 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 NewNameAllocator() NameAllocator {
-	possibleNames := make(map[string]int)
-	for _, firstChar := range []rune{'b', 'c'} {
-		for i := 'a'; i <= 'z'; i++ {
-			name := string([]rune{firstChar, i})
-			possibleNames[name] = 0
-		}
-	}
-	return &nameAllocator{
-		possibleNames: possibleNames,
-		counter:       0,
-	}
-}
-
-// GetNext gets next available device from the pool, this function assumes that caller
-// holds the necessary lock on nameAllocator
+// GetNext gets next available device given existing names that are being used
+// This function iterate through the device names in deterministic order of:
+//     a, b, ... , z, aa, ab, ... , az
+// and return the first one that is not used yet.
 func (d *nameAllocator) GetNext(existingNames ExistingNames) (string, error) {
-	d.mux.Lock()
-	defer d.mux.Unlock()
-
-	for _, namePair := range d.sortByCount() {
-		if _, found := existingNames[namePair.name]; !found {
-			return namePair.name, nil
+	for _, c1 := range []string{"", "a"} {
+		for c2 := 'a'; c2 <= 'z'; c2++ {
+			name := fmt.Sprintf("%s%s", c1, string(c2))
+			if _, found := existingNames[name]; !found {
+				return name, nil
+			}
 		}
 	}
-	return "", fmt.Errorf("there are no names available")
-}
-
-// Deprioritize the name so as it can't be used immediately again
-func (d *nameAllocator) Deprioritize(chosen string) {
-	d.mux.Lock()
-	defer d.mux.Unlock()
-
-	if _, ok := d.possibleNames[chosen]; ok {
-		d.counter++
-		d.possibleNames[chosen] = d.counter
-	}
-}
 
-func (d *nameAllocator) sortByCount() namePairList {
-	npl := make(namePairList, 0)
-	for name, index := range d.possibleNames {
-		npl = append(npl, namePair{name, index})
-	}
-	sort.Sort(npl)
-	return npl
+	return "", fmt.Errorf("there are no names available")
 }

pkg/cloud/devicemanager/allocator_test.go

@@ -21,65 +21,44 @@ import (
 )
 
 func TestNameAllocator(t *testing.T) {
+	existingNames := map[string]string{}
+	allocator := nameAllocator{}
+
 	tests := []struct {
-		name           string
-		existingNames  ExistingNames
-		deviceMap      map[string]int
-		expectedOutput string
+		expectedName string
 	}{
-		{
-			"empty device list with wrap",
-			ExistingNames{},
-			generateUnsortedNameList(),
-			"bd", // next to 'cz' is the first one, 'ba'
-		},
+		{"a"}, {"b"}, {"c"}, {"d"}, {"e"}, {"f"}, {"g"}, {"h"}, {"i"}, {"j"},
+		{"k"}, {"l"}, {"m"}, {"n"}, {"o"}, {"p"}, {"q"}, {"r"}, {"s"}, {"t"},
+		{"u"}, {"v"}, {"w"}, {"x"}, {"y"}, {"z"},
+		{"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"},
 	}
 
 	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			allocator := NewNameAllocator().(*nameAllocator)
-			for k, v := range test.deviceMap {
-				allocator.possibleNames[k] = v
-			}
-
-			got, err := allocator.GetNext(test.existingNames)
+		t.Run(test.expectedName, func(t *testing.T) {
+			actual, err := allocator.GetNext(existingNames)
 			if err != nil {
-				t.Errorf("text %q: unexpected error: %v", test.name, err)
+				t.Errorf("test %q: unexpected error: %v", test.expectedName, err)
 			}
-			if got != test.expectedOutput {
-				t.Errorf("text %q: expected %q, got %q", test.name, test.expectedOutput, got)
+			if actual != test.expectedName {
+				t.Errorf("test %q: expected %q, got %q", test.expectedName, test.expectedName, actual)
 			}
+			existingNames[actual] = ""
 		})
 	}
 }
 
-func generateUnsortedNameList() map[string]int {
-	possibleNames := make(map[string]int)
-	for _, firstChar := range []rune{'b', 'c'} {
-		for i := 'a'; i <= 'z'; i++ {
-			dev := string([]rune{firstChar, i})
-			possibleNames[dev] = 3
-		}
-	}
-	possibleNames["bd"] = 0
-	return possibleNames
-}
-
 func TestNameAllocatorError(t *testing.T) {
-	allocator := NewNameAllocator().(*nameAllocator)
-	existingNames := ExistingNames{}
+	allocator := nameAllocator{}
+	existingNames := map[string]string{}
 
-	// make all devices used
-	var first, second byte
-	for first = 'b'; first <= 'c'; first++ {
-		for second = 'a'; second <= 'z'; second++ {
-			device := [2]byte{first, second}
-			existingNames[string(device[:])] = "used"
-		}
+	for i := 0; i < 52; i++ {
+		name, _ := allocator.GetNext(existingNames)
+		existingNames[name] = ""
 	}
-
-	device, err := allocator.GetNext(existingNames)
+	name, err := allocator.GetNext(existingNames)
 	if err == nil {
-		t.Errorf("expected error, got device  %q", device)
+		t.Errorf("expected error, got device  %q", name)
 	}
 }

pkg/cloud/devicemanager/manager.go

@@ -62,8 +62,8 @@ type DeviceManager interface {
 }
 
 type deviceManager struct {
-	// nameAllocators holds the state of a device allocator for each node.
-	nameAllocators map[string]NameAllocator
+	// nameAllocator assigns new device name
+	nameAllocator NameAllocator
 
 	// We keep an active list of devices we have assigned but not yet
 	// attached, to avoid a race condition where we assign a device mapping
@@ -101,59 +101,48 @@ func (i inFlightAttaching) GetVolume(nodeID, name string) string {
 
 func NewDeviceManager() DeviceManager {
 	return &deviceManager{
-		nameAllocators: make(map[string]NameAllocator),
-		inFlight:       make(inFlightAttaching),
+		nameAllocator: &nameAllocator{},
+		inFlight:      make(inFlightAttaching),
 	}
 }
 
 func (d *deviceManager) NewDevice(instance *ec2.Instance, volumeID string) (*Device, error) {
-	nodeID, err := getInstanceID(instance)
-	if err != nil {
-		return nil, err
-	}
-
 	d.mux.Lock()
 	defer d.mux.Unlock()
 
+	if instance == nil {
+		return nil, fmt.Errorf("instance is nil")
+	}
+
 	// Get device names being attached and already attached to this instance
-	inUse := d.getDeviceNamesInUse(instance, nodeID)
+	inUse := d.getDeviceNamesInUse(instance)
 
 	// Check if this volume is already assigned a device on this machine
 	if path := d.getPath(inUse, volumeID); path != "" {
 		return d.newBlockDevice(instance, volumeID, path, true), nil
 	}
 
-	// Find the next unused device name
-	nameAllocator := d.nameAllocators[nodeID]
-	if nameAllocator == nil {
-		nameAllocator = NewNameAllocator()
-		d.nameAllocators[nodeID] = nameAllocator
+	nodeID, err := getInstanceID(instance)
+	if err != nil {
+		return nil, err
 	}
 
-	name, err := nameAllocator.GetNext(inUse)
+	name, err := d.nameAllocator.GetNext(inUse)
 	if err != nil {
 		return nil, fmt.Errorf("could not get a free device name to assign to node %s", nodeID)
 	}
 
 	// Add the chosen device and volume to the "attachments in progress" map
 	d.inFlight.Add(nodeID, volumeID, name)
 
-	// Deprioritize this name so it's not picked again right away.
-	nameAllocator.Deprioritize(name)
-
 	return d.newBlockDevice(instance, volumeID, devPreffix+name, false), nil
 }
 
 func (d *deviceManager) GetDevice(instance *ec2.Instance, volumeID string) (*Device, error) {
-	nodeID, err := getInstanceID(instance)
-	if err != nil {
-		return nil, err
-	}
-
 	d.mux.Lock()
 	defer d.mux.Unlock()
 
-	inUse := d.getDeviceNamesInUse(instance, nodeID)
+	inUse := d.getDeviceNamesInUse(instance)
 
 	if path := d.getPath(inUse, volumeID); path != "" {
 		return d.newBlockDevice(instance, volumeID, path, true), nil
@@ -188,7 +177,7 @@ func (d *deviceManager) release(device *Device) error {
 
 	var name string
 	if len(device.Path) > 2 {
-		name = device.Path[len(device.Path)-2:]
+		name = strings.TrimPrefix(device.Path, devPreffix)
 	}
 
 	existingVolumeID := d.inFlight.GetVolume(nodeID, name)
@@ -211,7 +200,10 @@ func (d *deviceManager) release(device *Device) error {
 	return nil
 }
 
-func (d *deviceManager) getDeviceNamesInUse(instance *ec2.Instance, nodeID string) map[string]string {
+// getDeviceNamesInUse returns the device to volume ID mapping
+// the mapping includes both already attached and being attached volumes
+func (d *deviceManager) getDeviceNamesInUse(instance *ec2.Instance) map[string]string {
+	nodeID := aws.StringValue(instance.InstanceId)
 	inUse := map[string]string{}
 	for _, blockDevice := range instance.BlockDeviceMappings {
 		name := aws.StringValue(blockDevice.DeviceName)

pkg/cloud/devicemanager/manager_test.go

@@ -57,10 +57,7 @@ func TestNewDevice(t *testing.T) {
 	dm := NewDeviceManager()
 
 	for _, tc := range testCases {
-		tc := tc // capture tc
 		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()
-
 			// Should fail if instance is nil
 			dev1, err := dm.NewDevice(nil, tc.volumeID)
 			if err == nil {
@@ -83,12 +80,12 @@ func TestNewDevice(t *testing.T) {
 				t.Fatalf("Expected equal paths, got %v and %v", dev1.Path, dev2.Path)
 			}
 
-			// Should create new Device with a different path after releasing
+			// Should create new Device with the same path after releasing
 			dev2.Release(false)
 			dev3, err := dm.NewDevice(fakeInstance, tc.volumeID)
 			assertDevice(t, dev3, false, err)
-			if dev3.Path == dev1.Path {
-				t.Fatalf("Expected equal paths, got %v and %v", dev1.Path, dev2.Path)
+			if dev3.Path != dev1.Path {
+				t.Fatalf("Expected equal paths, got %v and %v", dev1.Path, dev3.Path)
 			}
 			dev3.Release(false)
 		})
@@ -172,56 +169,6 @@ func TestReleaseDevice(t *testing.T) {
 	}
 }
 
-func TestExaustDevices(t *testing.T) {
-	testCases := []struct {
-		name               string
-		instanceID         string
-		existingDevicePath string
-		existingVolumeID   string
-		volumeID           string
-	}{
-		{
-			name:               "success: normal",
-			instanceID:         "instance-1",
-			existingDevicePath: "",
-			existingVolumeID:   "",
-			volumeID:           "vol-2",
-		},
-	}
-
-	dm := NewDeviceManager()
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			fakeInstance := newFakeInstance(tc.instanceID, tc.existingVolumeID, tc.existingDevicePath)
-
-			// Create one device and save it for later
-			dev, err := dm.NewDevice(fakeInstance, tc.volumeID)
-			assertDevice(t, dev, false /*IsAlreadyAssigned*/, err)
-			dev.Release(true)
-
-			// The maximum number of the ring is 52, so create enough devices
-			// to circle back to the first device gotten, i.e., dev
-			for i := 0; i < 51; i++ {
-				d, err := dm.NewDevice(fakeInstance, tc.volumeID)
-				assertDevice(t, d, false, err)
-				// Make sure none of them have the same path as the first device created
-				if d.Path == dev.Path {
-					t.Fatalf("Expected different device paths, got equals %q", d.Path)
-				}
-				d.Release(true)
-			}
-
-			dev2, err := dm.NewDevice(fakeInstance, tc.volumeID)
-			assertDevice(t, dev2, false /*IsAlreadyAssigned*/, err)
-
-			//Should be equal to the first device created
-			if dev2.Path != dev.Path {
-				t.Fatalf("Expected %q, got %q", dev2.Path, dev.Path)
-			}
-		})
-	}
-}
-
 func newFakeInstance(instanceID, volumeID, devicePath string) *ec2.Instance {
 	return &ec2.Instance{
 		InstanceId: aws.String(instanceID),