From ed528b2c27bd6250d783aab792f5ed47d1ab6291 Mon Sep 17 00:00:00 2001 From: Connor Catlett Date: Thu, 2 Mar 2023 08:05:34 +0000 Subject: [PATCH 1/3] Add tests for manually mounted volumes (and other weird paths) Signed-off-by: Connor Catlett --- pkg/cloud/devicemanager/manager_test.go | 54 +++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/pkg/cloud/devicemanager/manager_test.go b/pkg/cloud/devicemanager/manager_test.go index 25e095f3ea..0f277be024 100644 --- a/pkg/cloud/devicemanager/manager_test.go +++ b/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 From 58fc07efd5ee5af1d84e17200df58c3437726c3f Mon Sep 17 00:00:00 2001 From: Connor Catlett Date: Thu, 2 Mar 2023 08:16:57 +0000 Subject: [PATCH 2/3] 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 | 16 +++++++-------- pkg/cloud/devicemanager/allocator_test.go | 11 ++++++---- pkg/cloud/devicemanager/manager.go | 25 ++++++----------------- 3 files changed, 20 insertions(+), 32 deletions(-) diff --git a/pkg/cloud/devicemanager/allocator.go b/pkg/cloud/devicemanager/allocator.go index e30cc90a99..56a3413372 100644 --- a/pkg/cloud/devicemanager/allocator.go +++ b/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 } 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 "" From 882acf6206eaca285b28cbadb3249726e23cb4a5 Mon Sep 17 00:00:00 2001 From: Connor Catlett Date: Fri, 10 Mar 2023 15:47:31 +0000 Subject: [PATCH 3/3] Prevent allocation of devices after /dev/xvddx EBS has undocumented validation on the device that prevents the use of device names after /dev/xvddx. Also, expand the beginning of the device pool to account for this smaller pool of device names. Signed-off-by: Connor Catlett --- Makefile | 2 +- pkg/cloud/cloud_test.go | 28 +++++++++++------------ pkg/cloud/devicemanager/allocator.go | 12 +++++++--- pkg/cloud/devicemanager/allocator_test.go | 12 ++++++---- pkg/cloud/devicemanager/manager_test.go | 8 +++---- 5 files changed, 36 insertions(+), 26 deletions(-) diff --git a/Makefile b/Makefile index 11d7a7a232..08dfe625fb 100644 --- a/Makefile +++ b/Makefile @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -VERSION?=v1.15.0 +VERSION?=v1.16.1 PKG=github.com/kubernetes-sigs/aws-ebs-csi-driver GIT_COMMIT?=$(shell git rev-parse HEAD) diff --git a/pkg/cloud/cloud_test.go b/pkg/cloud/cloud_test.go index 5b21b7ca22..975ca36c72 100644 --- a/pkg/cloud/cloud_test.go +++ b/pkg/cloud/cloud_test.go @@ -721,7 +721,7 @@ func TestAttachDisk(t *testing.T) { vol := &ec2.Volume{ VolumeId: aws.String(tc.volumeID), - Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdba"), InstanceId: aws.String("node-1234"), State: aws.String("attached")}}, + Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdaa"), InstanceId: aws.String("node-1234"), State: aws.String("attached")}}, } ctx := context.Background() @@ -1614,7 +1614,7 @@ func TestWaitForAttachmentState(t *testing.T) { volumeID: "vol-test-1234", expectedState: volumeAttachedState, expectedInstance: "1234", - expectedDevice: "/dev/xvdba", + expectedDevice: "/dev/xvdaa", alreadyAssigned: false, expectError: false, }, @@ -1623,7 +1623,7 @@ func TestWaitForAttachmentState(t *testing.T) { volumeID: "vol-test-1234", expectedState: volumeDetachedState, expectedInstance: "1234", - expectedDevice: "/dev/xvdba", + expectedDevice: "/dev/xvdaa", alreadyAssigned: false, expectError: false, }, @@ -1632,7 +1632,7 @@ func TestWaitForAttachmentState(t *testing.T) { volumeID: "vol-test-1234", expectedState: volumeDetachedState, expectedInstance: "1234", - expectedDevice: "/dev/xvdba", + expectedDevice: "/dev/xvdaa", alreadyAssigned: false, expectError: false, }, @@ -1641,7 +1641,7 @@ func TestWaitForAttachmentState(t *testing.T) { volumeID: "vol-test-1234", expectedState: volumeAttachedState, expectedInstance: "1234", - expectedDevice: "/dev/xvdba", + expectedDevice: "/dev/xvdaa", alreadyAssigned: false, expectError: true, }, @@ -1650,7 +1650,7 @@ func TestWaitForAttachmentState(t *testing.T) { volumeID: "vol-test-1234", expectedState: volumeAttachedState, expectedInstance: "1234", - expectedDevice: "/dev/xvdbb", + expectedDevice: "/dev/xvdab", alreadyAssigned: false, expectError: true, }, @@ -1659,7 +1659,7 @@ func TestWaitForAttachmentState(t *testing.T) { volumeID: "vol-test-1234", expectedState: volumeAttachedState, expectedInstance: "1235", - expectedDevice: "/dev/xvdba", + expectedDevice: "/dev/xvdaa", alreadyAssigned: false, expectError: true, }, @@ -1668,7 +1668,7 @@ func TestWaitForAttachmentState(t *testing.T) { volumeID: "vol-test-1234", expectedState: volumeAttachedState, expectedInstance: "1234", - expectedDevice: "/dev/xvdba", + expectedDevice: "/dev/xvdaa", alreadyAssigned: true, expectError: true, }, @@ -1677,7 +1677,7 @@ func TestWaitForAttachmentState(t *testing.T) { volumeID: "vol-test-1234", expectedState: volumeAttachedState, expectedInstance: "1234", - expectedDevice: "/dev/xvdba", + expectedDevice: "/dev/xvdaa", alreadyAssigned: false, expectError: false, }, @@ -1686,7 +1686,7 @@ func TestWaitForAttachmentState(t *testing.T) { volumeID: "vol-test-1234", expectedState: volumeAttachedState, expectedInstance: "1234", - expectedDevice: "/dev/xvdba", + expectedDevice: "/dev/xvdaa", alreadyAssigned: false, expectError: true, }, @@ -1702,22 +1702,22 @@ func TestWaitForAttachmentState(t *testing.T) { attachedVol := &ec2.Volume{ VolumeId: aws.String(tc.volumeID), - Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdba"), InstanceId: aws.String("1234"), State: aws.String("attached")}}, + Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdaa"), InstanceId: aws.String("1234"), State: aws.String("attached")}}, } attachingVol := &ec2.Volume{ VolumeId: aws.String(tc.volumeID), - Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdba"), InstanceId: aws.String("1234"), State: aws.String("attaching")}}, + Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdaa"), InstanceId: aws.String("1234"), State: aws.String("attaching")}}, } detachedVol := &ec2.Volume{ VolumeId: aws.String(tc.volumeID), - Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdba"), InstanceId: aws.String("1234"), State: aws.String("detached")}}, + Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdaa"), InstanceId: aws.String("1234"), State: aws.String("detached")}}, } multipleAttachmentsVol := &ec2.Volume{ VolumeId: aws.String(tc.volumeID), - Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdba"), InstanceId: aws.String("1235"), State: aws.String("attached")}, {Device: aws.String("/dev/xvdba"), InstanceId: aws.String("1234"), State: aws.String("attached")}}, + Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdaa"), InstanceId: aws.String("1235"), State: aws.String("attached")}, {Device: aws.String("/dev/xvdaa"), InstanceId: aws.String("1234"), State: aws.String("attached")}}, } ctx := context.Background() diff --git a/pkg/cloud/devicemanager/allocator.go b/pkg/cloud/devicemanager/allocator.go index 56a3413372..208fa53f34 100644 --- a/pkg/cloud/devicemanager/allocator.go +++ b/pkg/cloud/devicemanager/allocator.go @@ -46,12 +46,18 @@ 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, ..., ..., zz +// aa, ..., az, ba, ..., bz, ..., ..., dx // // and return the first one that is not used yet. +// We stop at dx because EBS performs undocumented validation on the device +// name that refuses to mount devices after /dev/xvddx func (d *nameAllocator) GetNext(existingNames ExistingNames, prefix string) (string, error) { - for c1 := 'b'; c1 <= 'z'; c1++ { - for c2 := 'a'; c2 <= 'z'; c2++ { + for c1 := 'a'; c1 <= 'd'; c1++ { + c2end := 'z' + if c1 == 'd' { + c2end = 'x' + } + for c2 := 'a'; c2 <= c2end; 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 9995efdf89..e6945f7c35 100644 --- a/pkg/cloud/devicemanager/allocator_test.go +++ b/pkg/cloud/devicemanager/allocator_test.go @@ -27,12 +27,18 @@ func TestNameAllocator(t *testing.T) { tests := []struct { expectedName string }{ + {"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"}, {"ba"}, {"bb"}, {"bc"}, {"bd"}, {"be"}, {"bf"}, {"bg"}, {"bh"}, {"bi"}, {"bj"}, {"bk"}, {"bl"}, {"bm"}, {"bn"}, {"bo"}, {"bp"}, {"bq"}, {"br"}, {"bs"}, {"bt"}, {"bu"}, {"bv"}, {"bw"}, {"bx"}, {"by"}, {"bz"}, {"ca"}, {"cb"}, {"cc"}, {"cd"}, {"ce"}, {"cf"}, {"cg"}, {"ch"}, {"ci"}, {"cj"}, {"ck"}, {"cl"}, {"cm"}, {"cn"}, {"co"}, {"cp"}, {"cq"}, {"cr"}, {"cs"}, {"ct"}, {"cu"}, {"cv"}, {"cw"}, {"cx"}, {"cy"}, {"cz"}, + {"da"}, {"db"}, {"dc"}, {"dd"}, {"de"}, {"df"}, {"dg"}, {"dh"}, {"di"}, {"dj"}, + {"dk"}, {"dl"}, {"dm"}, {"dn"}, {"do"}, {"dp"}, {"dq"}, {"dr"}, {"ds"}, {"dt"}, + {"du"}, {"dv"}, {"dw"}, {"dx"}, } for _, test := range tests { @@ -53,10 +59,8 @@ func TestNameAllocatorError(t *testing.T) { allocator := nameAllocator{} existingNames := map[string]string{} - // 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++ { + // 102 == number of allocations from aa ... dx (see allocator.go for why we stop at dx) + for i := 0; i < 102; i++ { name, _ := allocator.GetNext(existingNames, "/dev/xvd") existingNames[name] = "" } diff --git a/pkg/cloud/devicemanager/manager_test.go b/pkg/cloud/devicemanager/manager_test.go index 0f277be024..826ccf2c97 100644 --- a/pkg/cloud/devicemanager/manager_test.go +++ b/pkg/cloud/devicemanager/manager_test.go @@ -103,16 +103,16 @@ func TestNewDeviceWithExistingDevice(t *testing.T) { { name: "success: different volumes", existingID: "vol-1", - existingPath: "/dev/xvdba", + existingPath: "/dev/xvdaa", volumeID: "vol-2", - expectedPath: "/dev/xvdbb", + expectedPath: "/dev/xvdab", }, { name: "success: same volumes", existingID: "vol-1", - existingPath: "/dev/xvdba", + existingPath: "/dev/xvdcc", volumeID: "vol-1", - expectedPath: "/dev/xvdba", + expectedPath: "/dev/xvdcc", }, { name: "success: same volumes with /dev/sdX path",