Skip to content

Commit

Permalink
Merge pull request #1518 from ConnorJC3/manually-attached
Browse files Browse the repository at this point in the history 
Stop treating prefixes as magic in DeviceManager
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Mar 14, 2023
2 parents b9e51af + 882acf6 commit fd8b1e1
Show file tree
Hide file tree
Showing 5 changed files with 99 additions and 47 deletions.
28 changes: 14 additions & 14 deletions pkg/cloud/cloud_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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()
Expand Down
24 changes: 14 additions & 10 deletions pkg/cloud/devicemanager/allocator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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{}
Expand All @@ -48,13 +46,19 @@ 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
// aa, ..., az, ba, ..., bz, ..., ..., dx
//
// and return the first one that is not used yet.
func (d *nameAllocator) GetNext(existingNames ExistingNames) (string, error) {
for _, c1 := range []string{"b", "c"} {
for c2 := 'a'; c2 <= 'z'; c2++ {
name := fmt.Sprintf("%s%s", c1, string(c2))
// 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 := '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
}
Expand Down
15 changes: 11 additions & 4 deletions pkg/cloud/devicemanager/allocator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,23 @@ 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 {
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)
}
Expand All @@ -53,11 +59,12 @@ func TestNameAllocatorError(t *testing.T) {
allocator := nameAllocator{}
existingNames := map[string]string{}

for i := 0; i < 52; i++ {
name, _ := allocator.GetNext(existingNames)
// 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] = ""
}
name, err := allocator.GetNext(existingNames)
name, err := allocator.GetNext(existingNames, "/dev/xvd")
if err == nil {
t.Errorf("expected error, got device %q", name)
}
Expand Down
25 changes: 6 additions & 19 deletions pkg/cloud/devicemanager/manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand All @@ -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
}
Expand All @@ -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)
}

Expand All @@ -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 ""
Expand Down
54 changes: 54 additions & 0 deletions pkg/cloud/devicemanager/manager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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/xvdaa",
volumeID: "vol-2",
expectedPath: "/dev/xvdab",
},
{
name: "success: same volumes",
existingID: "vol-1",
existingPath: "/dev/xvdcc",
volumeID: "vol-1",
expectedPath: "/dev/xvdcc",
},
{
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
Expand Down

0 comments on commit fd8b1e1

Please sign in to comment.