Skip to content

Commit

Permalink
Merge pull request #274 from leakingtapan/device-manager
Browse files Browse the repository at this point in the history 
Refactor device path allocator
  • Loading branch information
Cheng Pan authored Apr 11, 2019
2 parents dbafa92 + 1ace946 commit d94c30d
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 195 deletions.
80 changes: 12 additions & 68 deletions pkg/cloud/devicemanager/allocator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ package devicemanager

import (
"fmt"
"sort"
"sync"
)

// ExistingNames is a map of assigned device names. Presence of a key with a device
Expand All @@ -36,84 +34,30 @@ type ExistingNames map[string]string
// device name to the previously assigned one (from previous NameAllocator
// call), so all available device names are used eventually and it minimizes
// device name reuse.
// All these allocations are in-memory, nothing is written to / read from
// /dev directory.
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)

// Deprioritize the device name so as it can't be used immediately again
Deprioritize(chosen string)
}

type nameAllocator struct {
possibleNames map[string]int
counter int
mux sync.Mutex
}
type nameAllocator struct{}

var _ NameAllocator = &nameAllocator{}

type namePair struct {
name string
index int
}

type namePairList []namePair

func (p namePairList) Len() int { return len(p) }
func (p namePairList) Less(i, j int) bool { return p[i].index < p[j].index }
func (p namePairList) Swap(i, j int) { p[i], p[j] = p[j], p[i] }

// Allocates device names according to scheme ba..bz, ca..cz
// it moves along the ring and always picks next device until
// device list is exhausted.
func NewNameAllocator() NameAllocator {
possibleNames := make(map[string]int)
for _, firstChar := range []rune{'b', 'c'} {
for i := 'a'; i <= 'z'; i++ {
name := string([]rune{firstChar, i})
possibleNames[name] = 0
}
}
return &nameAllocator{
possibleNames: possibleNames,
counter: 0,
}
}

// GetNext gets next available device from the pool, this function assumes that caller
// holds the necessary lock on nameAllocator
// GetNext gets next available device given existing names that are being used
// This function iterate through the device names in deterministic order of:
// a, b, ... , z, aa, ab, ... , az
// and return the first one that is not used yet.
func (d *nameAllocator) GetNext(existingNames ExistingNames) (string, error) {
d.mux.Lock()
defer d.mux.Unlock()

for _, namePair := range d.sortByCount() {
if _, found := existingNames[namePair.name]; !found {
return namePair.name, nil
for _, c1 := range []string{"", "a"} {
for c2 := 'a'; c2 <= 'z'; c2++ {
name := fmt.Sprintf("%s%s", c1, string(c2))
if _, found := existingNames[name]; !found {
return name, nil
}
}
}
return "", fmt.Errorf("there are no names available")
}

// Deprioritize the name so as it can't be used immediately again
func (d *nameAllocator) Deprioritize(chosen string) {
d.mux.Lock()
defer d.mux.Unlock()

if _, ok := d.possibleNames[chosen]; ok {
d.counter++
d.possibleNames[chosen] = d.counter
}
}

func (d *nameAllocator) sortByCount() namePairList {
npl := make(namePairList, 0)
for name, index := range d.possibleNames {
npl = append(npl, namePair{name, index})
}
sort.Sort(npl)
return npl
return "", fmt.Errorf("there are no names available")
}
67 changes: 23 additions & 44 deletions pkg/cloud/devicemanager/allocator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,65 +21,44 @@ import (
)

func TestNameAllocator(t *testing.T) {
existingNames := map[string]string{}
allocator := nameAllocator{}

tests := []struct {
name string
existingNames ExistingNames
deviceMap map[string]int
expectedOutput string
expectedName string
}{
{
"empty device list with wrap",
ExistingNames{},
generateUnsortedNameList(),
"bd", // next to 'cz' is the first one, 'ba'
},
{"a"}, {"b"}, {"c"}, {"d"}, {"e"}, {"f"}, {"g"}, {"h"}, {"i"}, {"j"},
{"k"}, {"l"}, {"m"}, {"n"}, {"o"}, {"p"}, {"q"}, {"r"}, {"s"}, {"t"},
{"u"}, {"v"}, {"w"}, {"x"}, {"y"}, {"z"},
{"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"},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
allocator := NewNameAllocator().(*nameAllocator)
for k, v := range test.deviceMap {
allocator.possibleNames[k] = v
}

got, err := allocator.GetNext(test.existingNames)
t.Run(test.expectedName, func(t *testing.T) {
actual, err := allocator.GetNext(existingNames)
if err != nil {
t.Errorf("text %q: unexpected error: %v", test.name, err)
t.Errorf("test %q: unexpected error: %v", test.expectedName, err)
}
if got != test.expectedOutput {
t.Errorf("text %q: expected %q, got %q", test.name, test.expectedOutput, got)
if actual != test.expectedName {
t.Errorf("test %q: expected %q, got %q", test.expectedName, test.expectedName, actual)
}
existingNames[actual] = ""
})
}
}

func generateUnsortedNameList() map[string]int {
possibleNames := make(map[string]int)
for _, firstChar := range []rune{'b', 'c'} {
for i := 'a'; i <= 'z'; i++ {
dev := string([]rune{firstChar, i})
possibleNames[dev] = 3
}
}
possibleNames["bd"] = 0
return possibleNames
}

func TestNameAllocatorError(t *testing.T) {
allocator := NewNameAllocator().(*nameAllocator)
existingNames := ExistingNames{}
allocator := nameAllocator{}
existingNames := map[string]string{}

// make all devices used
var first, second byte
for first = 'b'; first <= 'c'; first++ {
for second = 'a'; second <= 'z'; second++ {
device := [2]byte{first, second}
existingNames[string(device[:])] = "used"
}
for i := 0; i < 52; i++ {
name, _ := allocator.GetNext(existingNames)
existingNames[name] = ""
}

device, err := allocator.GetNext(existingNames)
name, err := allocator.GetNext(existingNames)
if err == nil {
t.Errorf("expected error, got device %q", device)
t.Errorf("expected error, got device %q", name)
}
}
46 changes: 19 additions & 27 deletions pkg/cloud/devicemanager/manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ type DeviceManager interface {
}

type deviceManager struct {
// nameAllocators holds the state of a device allocator for each node.
nameAllocators map[string]NameAllocator
// nameAllocator assigns new device name
nameAllocator NameAllocator

// We keep an active list of devices we have assigned but not yet
// attached, to avoid a race condition where we assign a device mapping
Expand Down Expand Up @@ -101,59 +101,48 @@ func (i inFlightAttaching) GetVolume(nodeID, name string) string {

func NewDeviceManager() DeviceManager {
return &deviceManager{
nameAllocators: make(map[string]NameAllocator),
inFlight: make(inFlightAttaching),
nameAllocator: &nameAllocator{},
inFlight: make(inFlightAttaching),
}
}

func (d *deviceManager) NewDevice(instance *ec2.Instance, volumeID string) (*Device, error) {
nodeID, err := getInstanceID(instance)
if err != nil {
return nil, err
}

d.mux.Lock()
defer d.mux.Unlock()

if instance == nil {
return nil, fmt.Errorf("instance is nil")
}

// Get device names being attached and already attached to this instance
inUse := d.getDeviceNamesInUse(instance, nodeID)
inUse := d.getDeviceNamesInUse(instance)

// Check if this volume is already assigned a device on this machine
if path := d.getPath(inUse, volumeID); path != "" {
return d.newBlockDevice(instance, volumeID, path, true), nil
}

// Find the next unused device name
nameAllocator := d.nameAllocators[nodeID]
if nameAllocator == nil {
nameAllocator = NewNameAllocator()
d.nameAllocators[nodeID] = nameAllocator
nodeID, err := getInstanceID(instance)
if err != nil {
return nil, err
}

name, err := nameAllocator.GetNext(inUse)
name, err := d.nameAllocator.GetNext(inUse)
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)

// Deprioritize this name so it's not picked again right away.
nameAllocator.Deprioritize(name)

return d.newBlockDevice(instance, volumeID, devPreffix+name, false), nil
}

func (d *deviceManager) GetDevice(instance *ec2.Instance, volumeID string) (*Device, error) {
nodeID, err := getInstanceID(instance)
if err != nil {
return nil, err
}

d.mux.Lock()
defer d.mux.Unlock()

inUse := d.getDeviceNamesInUse(instance, nodeID)
inUse := d.getDeviceNamesInUse(instance)

if path := d.getPath(inUse, volumeID); path != "" {
return d.newBlockDevice(instance, volumeID, path, true), nil
Expand Down Expand Up @@ -188,7 +177,7 @@ func (d *deviceManager) release(device *Device) error {

var name string
if len(device.Path) > 2 {
name = device.Path[len(device.Path)-2:]
name = strings.TrimPrefix(device.Path, devPreffix)
}

existingVolumeID := d.inFlight.GetVolume(nodeID, name)
Expand All @@ -211,7 +200,10 @@ func (d *deviceManager) release(device *Device) error {
return nil
}

func (d *deviceManager) getDeviceNamesInUse(instance *ec2.Instance, nodeID string) map[string]string {
// getDeviceNamesInUse returns the device to volume ID mapping
// the mapping includes both already attached and being attached volumes
func (d *deviceManager) getDeviceNamesInUse(instance *ec2.Instance) map[string]string {
nodeID := aws.StringValue(instance.InstanceId)
inUse := map[string]string{}
for _, blockDevice := range instance.BlockDeviceMappings {
name := aws.StringValue(blockDevice.DeviceName)
Expand Down
59 changes: 3 additions & 56 deletions pkg/cloud/devicemanager/manager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,7 @@ func TestNewDevice(t *testing.T) {
dm := NewDeviceManager()

for _, tc := range testCases {
tc := tc // capture tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()

// Should fail if instance is nil
dev1, err := dm.NewDevice(nil, tc.volumeID)
if err == nil {
Expand All @@ -83,12 +80,12 @@ func TestNewDevice(t *testing.T) {
t.Fatalf("Expected equal paths, got %v and %v", dev1.Path, dev2.Path)
}

// Should create new Device with a different path after releasing
// Should create new Device with the same path after releasing
dev2.Release(false)
dev3, err := dm.NewDevice(fakeInstance, tc.volumeID)
assertDevice(t, dev3, false, err)
if dev3.Path == dev1.Path {
t.Fatalf("Expected equal paths, got %v and %v", dev1.Path, dev2.Path)
if dev3.Path != dev1.Path {
t.Fatalf("Expected equal paths, got %v and %v", dev1.Path, dev3.Path)
}
dev3.Release(false)
})
Expand Down Expand Up @@ -172,56 +169,6 @@ func TestReleaseDevice(t *testing.T) {
}
}

func TestExaustDevices(t *testing.T) {
testCases := []struct {
name string
instanceID string
existingDevicePath string
existingVolumeID string
volumeID string
}{
{
name: "success: normal",
instanceID: "instance-1",
existingDevicePath: "",
existingVolumeID: "",
volumeID: "vol-2",
},
}

dm := NewDeviceManager()
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
fakeInstance := newFakeInstance(tc.instanceID, tc.existingVolumeID, tc.existingDevicePath)

// Create one device and save it for later
dev, err := dm.NewDevice(fakeInstance, tc.volumeID)
assertDevice(t, dev, false /*IsAlreadyAssigned*/, err)
dev.Release(true)

// The maximum number of the ring is 52, so create enough devices
// to circle back to the first device gotten, i.e., dev
for i := 0; i < 51; i++ {
d, err := dm.NewDevice(fakeInstance, tc.volumeID)
assertDevice(t, d, false, err)
// Make sure none of them have the same path as the first device created
if d.Path == dev.Path {
t.Fatalf("Expected different device paths, got equals %q", d.Path)
}
d.Release(true)
}

dev2, err := dm.NewDevice(fakeInstance, tc.volumeID)
assertDevice(t, dev2, false /*IsAlreadyAssigned*/, err)

//Should be equal to the first device created
if dev2.Path != dev.Path {
t.Fatalf("Expected %q, got %q", dev2.Path, dev.Path)
}
})
}
}

func newFakeInstance(instanceID, volumeID, devicePath string) *ec2.Instance {
return &ec2.Instance{
InstanceId: aws.String(instanceID),
Expand Down

0 comments on commit d94c30d

Please sign in to comment.