Skip to content

Commit

Permalink
🧹 Rename functions in blockdevices.go. Add more tests, sort suitable …
Browse files Browse the repository at this point in the history 
…partitions by size before choosing one. (#4110)

* 🧹 Rename functions in blockdevices.go. Add more tests, sort suitable partitions by size before choosing one.

Signed-off-by: Preslav <[email protected]>

* drop debug log.

Signed-off-by: Preslav <[email protected]>

---------

Signed-off-by: Preslav <[email protected]>
  • Loading branch information
@preslavgerchev
preslavgerchev authored May 28, 2024
1 parent 981a931 commit de49696
Show file tree
Hide file tree
Showing 4 changed files with 165 additions and 83 deletions.
6 changes: 3 additions & 3 deletions providers/os/connection/device/linux/device_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,10 @@ func (c *LinuxDeviceManager) identifyViaLun(lun int) (*snapshot.PartitionInfo, e
}
}

return c.volumeMounter.GetDeviceForMounting(target)
return c.volumeMounter.GetMountablePartition(target)
}

func (c *LinuxDeviceManager) identifyViaDeviceName(deviceName string) (*snapshot.PartitionInfo, error) {
// GetDeviceForMounting also supports passing in empty strings, in that case we do a best-effort guess
return c.volumeMounter.GetDeviceForMounting(deviceName)
// GetMountablePartition also supports passing in empty strings, in that case we do a best-effort guess
return c.volumeMounter.GetMountablePartition(deviceName)
}
58 changes: 42 additions & 16 deletions providers/os/connection/snapshot/blockdevices.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"errors"
"fmt"
"io"
"sort"
"strings"

"github.com/rs/zerolog/log"
Expand All @@ -19,12 +20,12 @@ type BlockDevices struct {

type BlockDevice struct {
Name string `json:"name,omitempty"`
FsType string `json:"FsType,omitempty"`
FsType string `json:"fstype,omitempty"`
Label string `json:"label,omitempty"`
Uuid string `json:"uuid,omitempty"`
MountPoint string `json:"mountpoint,omitempty"`
Children []BlockDevice `json:"children,omitempty"`
FsUse string `json:"fsuse%,omitempty"`
Size int `json:"size,omitempty"`
}

type PartitionInfo struct {
Expand All @@ -33,7 +34,7 @@ type PartitionInfo struct {
}

func (cmdRunner *LocalCommandRunner) GetBlockDevices() (*BlockDevices, error) {
cmd, err := cmdRunner.RunCommand("lsblk -f --json")
cmd, err := cmdRunner.RunCommand("lsblk -bo NAME,SIZE,FSTYPE,MOUNTPOINT,LABEL,UUID --json")
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -71,36 +72,61 @@ func (blockEntries BlockDevices) GetRootBlockEntry() (*PartitionInfo, error) {
return nil, errors.New("target volume not found on instance")
}

func (blockEntries BlockDevices) GetBlockEntryByName(name string) (*PartitionInfo, error) {
log.Debug().Str("name", name).Msg("get matching block entry")
// Searches all the partitions in the target device and finds one that can be mounted. It must be unmounted, non-boot partition
// If multiple partitions meet this criteria, the largest one is returned.
func (blockEntries BlockDevices) GetMountablePartitionByDevice(device string) (*PartitionInfo, error) {
log.Debug().Str("device", device).Msg("get partitions for device")
var block BlockDevice
partitions := []BlockDevice{}
var secondName string
if strings.HasPrefix(name, "/dev/sd") {
if strings.HasPrefix(device, "/dev/sd") {
// sdh and xvdh are interchangeable
end := strings.TrimPrefix(name, "/dev/sd")
end := strings.TrimPrefix(device, "/dev/sd")
secondName = "/dev/xvd" + end
}
for i := range blockEntries.BlockDevices {
d := blockEntries.BlockDevices[i]
log.Debug().Str("name", d.Name).Interface("children", d.Children).Interface("mountpoint", d.MountPoint).Msg("found block device")
fullDeviceName := "/dev/" + d.Name
if name != fullDeviceName { // check if the device name matches
if device != fullDeviceName { // check if the device name matches
if secondName == "" {
continue
}
if secondName != fullDeviceName { // check if the device name matches the second name option (sdh and xvdh are interchangeable)
continue
}
}
log.Debug().Str("location", d.Name).Msg("found match")
for i := range d.Children {
entry := d.Children[i]
if entry.IsNotBootOrRootVolumeAndUnmounted() {
devFsName := "/dev/" + entry.Name
return &PartitionInfo{Name: devFsName, FsType: entry.FsType}, nil
}
log.Debug().Str("name", d.Name).Msg("found matching device")
block = d
break
}
if len(block.Name) == 0 {
return nil, fmt.Errorf("no block device found with name %s", device)
}

for _, partition := range block.Children {
log.Debug().Str("name", partition.Name).Int("size", partition.Size).Msg("checking partition")
if partition.IsNotBootOrRootVolumeAndUnmounted() {
partitions = append(partitions, partition)
}
}
return nil, errors.New("target volume not found on instance")

if len(partitions) == 0 {
return nil, fmt.Errorf("no suitable partitions found on device %s", block.Name)
}

// sort the candidates by size, so we can pick the largest one
sortPartitionsBySize(partitions)

// return the largest partition. we can extend this to be a parameter in the future
devFsName := "/dev/" + partitions[0].Name
return &PartitionInfo{Name: devFsName, FsType: partitions[0].FsType}, nil
}

func sortPartitionsBySize(partitions []BlockDevice) {
sort.Slice(partitions, func(i, j int) bool {
return partitions[i].Size > partitions[j].Size
})
}

func (blockEntries BlockDevices) GetUnnamedBlockEntry() (*PartitionInfo, error) {
Expand Down
156 changes: 110 additions & 46 deletions providers/os/connection/snapshot/blockdevices_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,54 +12,116 @@ import (
"github.com/stretchr/testify/require"
)

var RootDevice = BlockDevice{Name: "sda", Children: []BlockDevice{{Uuid: "1234", FsType: "xfs", Label: "ROOT", Name: "sda1", MountPoint: "/"}}}

func TestGetMatchingBlockEntryByName(t *testing.T) {
blockEntries := BlockDevices{BlockDevices: []BlockDevice{RootDevice}}
blockEntries.BlockDevices = append(blockEntries.BlockDevices, []BlockDevice{
{Name: "nvme0n1", Children: []BlockDevice{{Uuid: "12345", FsType: "xfs", Label: "ROOT", Name: "nvmd1n1"}, {Uuid: "12345", FsType: "", Label: "EFI"}}},
{Name: "sdx", Children: []BlockDevice{{Uuid: "12346", FsType: "xfs", Label: "ROOT", Name: "sdh1"}, {Uuid: "12345", FsType: "", Label: "EFI"}}},
}...)

realPartitionInfo, err := blockEntries.GetBlockEntryByName("/dev/sdx")
require.Nil(t, err)
require.Equal(t, PartitionInfo{FsType: "xfs", Name: "/dev/sdh1"}, *realPartitionInfo)

blockEntries = BlockDevices{BlockDevices: []BlockDevice{RootDevice}}
blockEntries.BlockDevices = append(blockEntries.BlockDevices, []BlockDevice{
{Name: "nvme0n1", Children: []BlockDevice{{Uuid: "12345", FsType: "xfs", Label: "ROOT", Name: "nvmd1n1"}, {Uuid: "12345", FsType: "", Label: "EFI"}}},
{Name: "xvdx", Children: []BlockDevice{{Uuid: "12346", FsType: "xfs", Label: "ROOT", Name: "xvdh1"}, {Uuid: "12345", FsType: "", Label: "EFI"}}},
}...)

realPartitionInfo, err = blockEntries.GetBlockEntryByName("/dev/sdx")
require.Nil(t, err)
require.Equal(t, PartitionInfo{FsType: "xfs", Name: "/dev/xvdh1"}, *realPartitionInfo)

blockEntries = BlockDevices{BlockDevices: []BlockDevice{RootDevice}}
blockEntries.BlockDevices = append(blockEntries.BlockDevices, []BlockDevice{
{Name: "nvme0n1", Children: []BlockDevice{{Uuid: "12345", FsType: "xfs", Label: "ROOT", Name: "nvmd1n1"}, {Uuid: "12345", FsType: "", Label: "EFI"}}},
{Name: "xvdh", Children: []BlockDevice{{Uuid: "12346", FsType: "xfs", Label: "ROOT", Name: "xvdh1"}, {Uuid: "12345", FsType: "", Label: "EFI"}}},
}...)

realPartitionInfo, err = blockEntries.GetBlockEntryByName("/dev/xvdh")
require.Nil(t, err)
require.Equal(t, PartitionInfo{FsType: "xfs", Name: "/dev/xvdh1"}, *realPartitionInfo)

blockEntries = BlockDevices{BlockDevices: []BlockDevice{RootDevice}}
blockEntries.BlockDevices = append(blockEntries.BlockDevices, []BlockDevice{
{Name: "nvme0n1", Children: []BlockDevice{{Uuid: "12345", FsType: "xfs", Label: "ROOT", Name: "nvmd1n1"}, {Uuid: "12345", FsType: "", Label: "EFI"}}},
}...)

_, err = blockEntries.GetBlockEntryByName("/dev/sdh")
require.Error(t, err)

blockEntries = BlockDevices{BlockDevices: []BlockDevice{RootDevice}}
_, err = blockEntries.GetBlockEntryByName("/dev/sdh")
require.Error(t, err)
func TestGetMountablePartitionByDevice(t *testing.T) {
t.Run("match by exact name", func(t *testing.T) {
blockEntries := BlockDevices{
BlockDevices: []BlockDevice{
{Name: "sda", Children: []BlockDevice{{Uuid: "1234", FsType: "xfs", Label: "ROOT", Name: "sda1", MountPoint: "/"}}},
{Name: "nvme0n1", Children: []BlockDevice{{Uuid: "12345", FsType: "xfs", Label: "ROOT", Name: "nvmd1n1"}, {Uuid: "12345", FsType: "", Label: "EFI"}}},
{Name: "sdx", Children: []BlockDevice{{Uuid: "12346", FsType: "xfs", Label: "ROOT", Name: "sdh1"}, {Uuid: "12345", FsType: "", Label: "EFI"}}},
},
}

partition, err := blockEntries.GetMountablePartitionByDevice("/dev/sdx")
require.Nil(t, err)
require.Equal(t, &PartitionInfo{FsType: "xfs", Name: "/dev/sdh1"}, partition)
})
t.Run("match by interchangeable name", func(t *testing.T) {
blockEntries := BlockDevices{
BlockDevices: []BlockDevice{
{Name: "sda", Children: []BlockDevice{{Uuid: "1234", FsType: "xfs", Label: "ROOT", Name: "sda1", MountPoint: "/"}}},
{Name: "nvme0n1", Children: []BlockDevice{{Uuid: "12345", FsType: "xfs", Label: "ROOT", Name: "nvmd1n1"}, {Uuid: "12345", FsType: "", Label: "EFI"}}},
{Name: "xvdx", Children: []BlockDevice{{Uuid: "12346", FsType: "xfs", Label: "ROOT", Name: "xvdh1"}, {Uuid: "12345", FsType: "", Label: "EFI"}}},
},
}

partition, err := blockEntries.GetMountablePartitionByDevice("/dev/sdx")
require.Nil(t, err)
require.Equal(t, &PartitionInfo{FsType: "xfs", Name: "/dev/xvdh1"}, partition)
})

t.Run("no match by device name", func(t *testing.T) {
blockEntries := BlockDevices{
BlockDevices: []BlockDevice{
{
Name: "sda", Children: []BlockDevice{{Uuid: "1234", FsType: "xfs", Label: "ROOT", Name: "sda1", MountPoint: "/"}},
},
},
}
_, err := blockEntries.GetMountablePartitionByDevice("/dev/sdh")
require.Error(t, err)
require.Contains(t, err.Error(), "no block device found with name")
})

t.Run("no suitable partition", func(t *testing.T) {
blockEntries := BlockDevices{
BlockDevices: []BlockDevice{
{
Name: "sda", Children: []BlockDevice{{Uuid: "1234", FsType: "xfs", Label: "ROOT", Name: "sda1", MountPoint: "/"}},
},
},
}
_, err := blockEntries.GetMountablePartitionByDevice("/dev/sda")
require.Error(t, err)
require.Contains(t, err.Error(), "no suitable partitions found")
})

t.Run("return biggest partition", func(t *testing.T) {
blockEntries := BlockDevices{
BlockDevices: []BlockDevice{
{
Name: "sde",
Children: []BlockDevice{
{Uuid: "12346", FsType: "xfs", Size: 110, Label: "ROOT", Name: "sde1"},
{Uuid: "12345", FsType: "xfs", Size: 120, Label: "ROOT", Name: "sde2"},
},
},
},
}
partition, err := blockEntries.GetMountablePartitionByDevice("/dev/sde")
require.Nil(t, err)
require.Equal(t, &PartitionInfo{FsType: "xfs", Name: "/dev/sde2"}, partition)
})

t.Run("ignore boot partition (EFI label)", func(t *testing.T) {
blockEntries := BlockDevices{
BlockDevices: []BlockDevice{
{
Name: "sde",
Children: []BlockDevice{
{Uuid: "12346", FsType: "xfs", Size: 110, Label: "ROOT", Name: "sde1"},
{Uuid: "12345", FsType: "xfs", Size: 120, Label: "EFI", Name: "sde2"},
},
},
},
}
partition, err := blockEntries.GetMountablePartitionByDevice("/dev/sde")
require.Nil(t, err)
require.Equal(t, &PartitionInfo{FsType: "xfs", Name: "/dev/sde1"}, partition)
})

t.Run("ignore boot partition (boot label)", func(t *testing.T) {
blockEntries := BlockDevices{
BlockDevices: []BlockDevice{
{
Name: "sde",
Children: []BlockDevice{
{Uuid: "12346", FsType: "xfs", Size: 110, Label: "ROOT", Name: "sde1"},
{Uuid: "12345", FsType: "xfs", Size: 120, Label: "boot", Name: "sde2"},
},
},
},
}
partition, err := blockEntries.GetMountablePartitionByDevice("/dev/sde")
require.Nil(t, err)
require.Equal(t, &PartitionInfo{FsType: "xfs", Name: "/dev/sde1"}, partition)
})
}

func TestGetNonRootBlockEntry(t *testing.T) {
blockEntries := BlockDevices{BlockDevices: []BlockDevice{RootDevice}}
blockEntries := BlockDevices{BlockDevices: []BlockDevice{
{Name: "sda", Children: []BlockDevice{{Uuid: "1234", FsType: "xfs", Label: "ROOT", Name: "sda1", MountPoint: "/"}}},
}}
blockEntries.BlockDevices = append(blockEntries.BlockDevices, []BlockDevice{
{Name: "nvme0n1", Children: []BlockDevice{{Uuid: "12345", FsType: "xfs", Label: "ROOT", Name: "nvmd1n1"}, {Uuid: "12345", FsType: "", Label: "EFI"}}},
}...)
Expand All @@ -69,7 +131,9 @@ func TestGetNonRootBlockEntry(t *testing.T) {
}

func TestGetRootBlockEntry(t *testing.T) {
blockEntries := BlockDevices{BlockDevices: []BlockDevice{RootDevice}}
blockEntries := BlockDevices{BlockDevices: []BlockDevice{
{Name: "sda", Children: []BlockDevice{{Uuid: "1234", FsType: "xfs", Label: "ROOT", Name: "sda1", MountPoint: "/"}}},
}}
realPartitionInfo, err := blockEntries.GetRootBlockEntry()
require.Nil(t, err)
require.Equal(t, PartitionInfo{FsType: "xfs", Name: "/dev/sda1"}, *realPartitionInfo)
Expand Down
28 changes: 10 additions & 18 deletions providers/os/connection/snapshot/volumemounter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func (m *VolumeMounter) getFsInfo() (*PartitionInfo, error) {
return blockEntries.GetUnnamedBlockEntry()
}

fsInfo, err = blockEntries.GetBlockEntryByName(m.VolumeAttachmentLoc)
fsInfo, err = blockEntries.GetMountablePartitionByDevice(m.VolumeAttachmentLoc)
if err == nil && fsInfo != nil {
return fsInfo, nil
} else {
Expand All @@ -121,33 +121,25 @@ func (m *VolumeMounter) getFsInfo() (*PartitionInfo, error) {
}

// GetDeviceForMounting iterates through all the partitions of the target and returns the first one that matches the filters
// If target is empty, it will return the first unnamed block device (best-effort guessing)
// If device is not specified, it will return the first non-mounted, non-boot partition (best-effort guessing)
// E.g. if target is "sda", it will return the first partition of the block device "sda" that satisfies the filters
func (m *VolumeMounter) GetDeviceForMounting(target string) (*PartitionInfo, error) {
if target == "" {
log.Debug().Msg("no target provided, searching for unnamed block device")
func (m *VolumeMounter) GetMountablePartition(device string) (*PartitionInfo, error) {
if device == "" {
log.Debug().Msg("no device provided, searching for unnamed block device")
} else {
log.Debug().Str("target", target).Msg("search for target partition")
log.Debug().Str("device", device).Msg("search for target partition")
}

cmd, err := m.CmdRunner.RunCommand("sudo lsblk -f --json")
if err != nil {
return nil, err
}
data, err := io.ReadAll(cmd.Stdout)
blockDevices, err := m.CmdRunner.GetBlockDevices()
if err != nil {
return nil, err
}
blockEntries := BlockDevices{}
if err := json.Unmarshal(data, &blockEntries); err != nil {
return nil, err
}
if target == "" {
if device == "" {
// TODO: i dont know what the difference between GetUnnamedBlockEntry and GetUnmountedBlockEntry is
// we need to simplify those
return blockEntries.GetUnnamedBlockEntry()
return blockDevices.GetUnnamedBlockEntry()
}
return blockEntries.GetBlockEntryByName(target)
return blockDevices.GetMountablePartitionByDevice(device)
}

func (m *VolumeMounter) mountVolume(fsInfo *PartitionInfo) error {
Expand Down

0 comments on commit de49696

Please sign in to comment.