From bba5b11f5b5f51bd4b02f9016159fe538c276c15 Mon Sep 17 00:00:00 2001 From: Connor Catlett Date: Thu, 2 Mar 2023 08:16:57 +0000 Subject: [PATCH] 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 --- pkg/cloud/devicemanager/allocator.go | 18 ++++++++-------- pkg/cloud/devicemanager/allocator_test.go | 11 ++++++---- pkg/cloud/devicemanager/manager.go | 25 ++++++----------------- 3 files changed, 21 insertions(+), 33 deletions(-) diff --git a/pkg/cloud/devicemanager/allocator.go b/pkg/cloud/devicemanager/allocator.go index e30cc90a99..ebb8677134 100644 --- a/pkg/cloud/devicemanager/allocator.go +++ b/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 } diff --git a/pkg/cloud/devicemanager/allocator_test.go b/pkg/cloud/devicemanager/allocator_test.go index 607030ee06..9995efdf89 100644 --- a/pkg/cloud/devicemanager/allocator_test.go +++ b/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) } diff --git a/pkg/cloud/devicemanager/manager.go b/pkg/cloud/devicemanager/manager.go index 0292b048b9..534f55af3f 100644 --- a/pkg/cloud/devicemanager/manager.go +++ b/pkg/cloud/devicemanager/manager.go @@ -18,7 +18,6 @@ package devicemanager import ( "fmt" - "strings" "sync" "github.com/aws/aws-sdk-go/aws" @@ -26,7 +25,7 @@ import ( "k8s.io/klog/v2" ) -const devPreffix = "/dev/xvd" +const devPrefix = "/dev/xvd" type Device struct { Instance *ec2.Instance @@ -127,7 +126,7 @@ 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) } @@ -135,7 +134,7 @@ func (d *deviceManager) NewDevice(instance *ec2.Instance, volumeID string) (*Dev // 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 ""