Stop treating prefixes as magic in DeviceManager

pkg/cloud/devicemanager/allocator.go

@@ -22,8 +22,7 @@ import (
 
 // ExistingNames is a map of assigned device names. Presence of a key with a device
 // name in the map means that the device is allocated. Value is irrelevant and
-// can be used for anything that NameAllocator user wants.  Only the relevant
-// part of device name should be in the map, e.g. "ba" for "/dev/xvdba".
+// can be used for anything that NameAllocator user wants.
 type ExistingNames map[string]string
 
 // On AWS, we should assign new (not yet used) device names to attached volumes.
@@ -36,9 +35,8 @@ type ExistingNames map[string]string
 // device name reuse.
 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)
+	// name. The prefix (such as "/dev/xvd" or "/dev/sd") is passed as a parameter.
+	GetNext(existingNames ExistingNames, prefix string) (name string, err error)
 }
 
 type nameAllocator struct{}
@@ -48,13 +46,13 @@ 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
+//	ba, ..., bz, ca, ..., cz, ..., ..., zz
 //
 // and return the first one that is not used yet.
-func (d *nameAllocator) GetNext(existingNames ExistingNames) (string, error) {
-	for _, c1 := range []string{"b", "c"} {
+func (d *nameAllocator) GetNext(existingNames ExistingNames, prefix string) (string, error) {
+	for c1 := 'b'; c1 <= 'z'; c1++ {
 		for c2 := 'a'; c2 <= 'z'; c2++ {
-			name := fmt.Sprintf("%s%s", c1, string(c2))
+			name := fmt.Sprintf("%s%s%s", prefix, string(c1), string(c2))
 			if _, found := existingNames[name]; !found {
 				return name, nil
 			}

pkg/cloud/devicemanager/allocator_test.go

@@ -37,7 +37,7 @@ func TestNameAllocator(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.expectedName, func(t *testing.T) {
-			actual, err := allocator.GetNext(existingNames)
+			actual, err := allocator.GetNext(existingNames, "")
 			if err != nil {
 				t.Errorf("test %q: unexpected error: %v", test.expectedName, err)
 			}
@@ -53,11 +53,14 @@ func TestNameAllocatorError(t *testing.T) {
 	allocator := nameAllocator{}
 	existingNames := map[string]string{}
 
-	for i := 0; i < 52; i++ {
-		name, _ := allocator.GetNext(existingNames)
+	// 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++ {
+		name, _ := allocator.GetNext(existingNames, "/dev/xvd")
 		existingNames[name] = ""
 	}
-	name, err := allocator.GetNext(existingNames)
+	name, err := allocator.GetNext(existingNames, "/dev/xvd")
 	if err == nil {
 		t.Errorf("expected error, got device  %q", name)
 	}

pkg/cloud/devicemanager/manager.go

@@ -18,15 +18,14 @@ package devicemanager
 
 import (
 	"fmt"
-	"strings"
 	"sync"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"k8s.io/klog/v2"
 )
 
-const devPreffix = "/dev/xvd"
+const devPrefix = "/dev/xvd"
 
 type Device struct {
 	Instance          *ec2.Instance
@@ -127,15 +126,15 @@ func (d *deviceManager) NewDevice(instance *ec2.Instance, volumeID string) (*Dev
 		return nil, err
 	}
 
-	name, err := d.nameAllocator.GetNext(inUse)
+	name, err := d.nameAllocator.GetNext(inUse, devPrefix)
 	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)
 
-	return d.newBlockDevice(instance, volumeID, devPreffix+name, false), nil
+	return d.newBlockDevice(instance, volumeID, name, false), nil
 }
 
 func (d *deviceManager) GetDevice(instance *ec2.Instance, volumeID string) (*Device, error) {
@@ -175,12 +174,7 @@ func (d *deviceManager) release(device *Device) error {
 	d.mux.Lock()
 	defer d.mux.Unlock()
 
-	var name string
-	if len(device.Path) > 2 {
-		name = strings.TrimPrefix(device.Path, devPreffix)
-	}
-
-	existingVolumeID := d.inFlight.GetVolume(nodeID, name)
+	existingVolumeID := d.inFlight.GetVolume(nodeID, device.Path)
 	if len(existingVolumeID) == 0 {
 		// Attaching is not in progress, so there's nothing to release
 		return nil
@@ -195,7 +189,7 @@ func (d *deviceManager) release(device *Device) error {
 	}
 
 	klog.V(5).InfoS("[Debug] Releasing in-process", "attachment entry", device.Path, "volume", device.VolumeID)
-	d.inFlight.Del(nodeID, name)
+	d.inFlight.Del(nodeID, device.Path)
 
 	return nil
 }
@@ -207,13 +201,6 @@ func (d *deviceManager) getDeviceNamesInUse(instance *ec2.Instance) map[string]s
 	inUse := map[string]string{}
 	for _, blockDevice := range instance.BlockDeviceMappings {
 		name := aws.StringValue(blockDevice.DeviceName)
-		// trims /dev/sd or /dev/xvd from device name
-		name = strings.TrimPrefix(name, "/dev/sd")
-		name = strings.TrimPrefix(name, "/dev/xvd")
-
-		if len(name) < 1 || len(name) > 2 {
-			klog.InfoS("Unexpected EBS DeviceName", "DeviceName", aws.StringValue(blockDevice.DeviceName))
-		}
 		inUse[name] = aws.StringValue(blockDevice.Ebs.VolumeId)
 	}
 
@@ -227,7 +214,7 @@ func (d *deviceManager) getDeviceNamesInUse(instance *ec2.Instance) map[string]s
 func (d *deviceManager) getPath(inUse map[string]string, volumeID string) string {
 	for name, volID := range inUse {
 		if volumeID == volID {
-			return devPreffix + name
+			return name
 		}
 	}
 	return ""

pkg/cloud/devicemanager/manager_test.go

@@ -92,6 +92,60 @@ func TestNewDevice(t *testing.T) {
 	}
 }
 
+func TestNewDeviceWithExistingDevice(t *testing.T) {
+	testCases := []struct {
+		name         string
+		existingID   string
+		existingPath string
+		volumeID     string
+		expectedPath string
+	}{
+		{
+			name:         "success: different volumes",
+			existingID:   "vol-1",
+			existingPath: "/dev/xvdba",
+			volumeID:     "vol-2",
+			expectedPath: "/dev/xvdbb",
+		},
+		{
+			name:         "success: same volumes",
+			existingID:   "vol-1",
+			existingPath: "/dev/xvdba",
+			volumeID:     "vol-1",
+			expectedPath: "/dev/xvdba",
+		},
+		{
+			name:         "success: same volumes with /dev/sdX path",
+			existingID:   "vol-3",
+			existingPath: "/dev/sdf",
+			volumeID:     "vol-3",
+			expectedPath: "/dev/sdf",
+		},
+		{
+			name:         "success: same volumes with weird path",
+			existingID:   "vol-42",
+			existingPath: "/weird/path",
+			volumeID:     "vol-42",
+			expectedPath: "/weird/path",
+		},
+	}
+	// Use a shared DeviceManager to make sure that there are no race conditions
+	dm := NewDeviceManager()
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			fakeInstance := newFakeInstance("fake-instance", tc.existingID, tc.existingPath)
+
+			dev, err := dm.NewDevice(fakeInstance, tc.volumeID)
+			assertDevice(t, dev, tc.existingID == tc.volumeID, err)
+
+			if dev.Path != tc.expectedPath {
+				t.Fatalf("Expected path %v got %v", tc.expectedPath, dev.Path)
+			}
+		})
+	}
+}
+
 func TestGetDevice(t *testing.T) {
 	testCases := []struct {
 		name               string