Stop treating prefixes as magic in DeviceManager

DM treats the prefixes "/dev/xvd" and "/dev/sd" as magic. This leads to
a bug where volumes with a prefix starting with "/dev/sd" (for example,
"/dev/sdf") will get incorrectly mangled into a "/dev/xvd" form (i.e.
converting "/dev/sdf" to "/dev/xvdf"). This will cause a volume that is
already mounted and prefixed with "/dev/sd" (for example, a volume
manually mounted via the AWS console) to get stuck in an infinite loop
as the node will believe it is mounted at the wrong path.

With this change, instead of storing only the end of the path in the
inflight and existing volumes map, we instead store the entire device
path. The previously leaky abstraction from the allocator to store only
the end of the path is contained to the allocator, which has a new
parameter for the desired prefix.

This commit also vastly increases the range of paths the allocator can
generate from 52 simultaneously mounted volumes to 650.

Signed-off-by: Connor Catlett <[email protected]>

pkg/cloud/devicemanager/allocator.go

@@ -22,9 +22,8 @@ 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".
-type ExistingNames map[string]string
+// can be used for anything that NameAllocator user wants.
+type ExistingNames map[string]struct{}
 
 // On AWS, we should assign new (not yet used) device names to attached volumes.
 // If we reuse a previously used name, we may get the volume "attaching" forever,
@@ -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 ""