🧹 Split device discovery from partition discovery in blockdevices.go

providers/os/connection/device/linux/device_manager.go

@@ -104,27 +104,43 @@ func (c *LinuxDeviceManager) identifyViaLun(lun int) (*snapshot.PartitionInfo, e
 	if len(filteredScsiDevices) == 0 {
 		return nil, errors.New("no matching scsi devices found")
 	}
-
-	var target string
-	// if we have exactly one device present at the LUN we can directly point the volume mounter towards it
+	blockDevices, err := c.volumeMounter.CmdRunner.GetBlockDevices()
+	if err != nil {
+		return nil, err
+	}
+	var device snapshot.BlockDevice
+	var deviceErr error
+	// if we have exactly one device present at the LUN we can directly search for it
 	if len(filteredScsiDevices) == 1 {
-		target = filteredScsiDevices[0].VolumePath
+		devicePath := filteredScsiDevices[0].VolumePath
+		device, deviceErr = blockDevices.FindDevice(devicePath)
 	} else {
-		// we have multiple devices at the same LUN. we find the first non-mounted block devices in that list
-		blockDevices, err := c.volumeMounter.CmdRunner.GetBlockDevices()
-		if err != nil {
-			return nil, err
-		}
-		target, err = findMatchingDeviceByBlock(filteredScsiDevices, blockDevices)
-		if err != nil {
-			return nil, err
-		}
+		device, deviceErr = findMatchingDeviceByBlock(filteredScsiDevices, blockDevices)
 	}
-
-	return c.volumeMounter.GetMountablePartition(target)
+	if deviceErr != nil {
+		return nil, deviceErr
+	}
+	return device.GetMountablePartition()
 }
 
 func (c *LinuxDeviceManager) identifyViaDeviceName(deviceName string) (*snapshot.PartitionInfo, error) {
-	// GetMountablePartition also supports passing in empty strings, in that case we do a best-effort guess
-	return c.volumeMounter.GetMountablePartition(deviceName)
+	blockDevices, err := c.volumeMounter.CmdRunner.GetBlockDevices()
+	if err != nil {
+		return nil, err
+	}
+
+	// if we don't have a device name we can just return the first non-boot, non-mounted partition.
+	// this is a best-guess approach
+	if deviceName == "" {
+		// TODO: we should rename/simplify this method
+		return blockDevices.GetUnnamedBlockEntry()
+	}
+
+	// if we have a specific device we're looking for we can just ask only for that
+	device, err := blockDevices.FindDevice(deviceName)
+	if err != nil {
+		return nil, err
+	}
+
+	return device.GetMountablePartition()
 }

providers/os/connection/device/linux/lun.go

@@ -57,7 +57,7 @@ func filterScsiDevices(scsiDevices scsiDevices, lun int) []scsiDeviceInfo {
 // there can be multiple devices mounted at the same LUN.
 // the LUN so we need to find all the blocks, mounted at that LUN. then we find the first one
 // that has no mounted partitions and use that as the target device. this is a best-effort approach
-func findMatchingDeviceByBlock(scsiDevices scsiDevices, blockDevices *snapshot.BlockDevices) (string, error) {
+func findMatchingDeviceByBlock(scsiDevices scsiDevices, blockDevices *snapshot.BlockDevices) (snapshot.BlockDevice, error) {
 	matchingBlocks := []snapshot.BlockDevice{}
 	for _, device := range scsiDevices {
 		for _, block := range blockDevices.BlockDevices {
@@ -69,25 +69,22 @@ func findMatchingDeviceByBlock(scsiDevices scsiDevices, blockDevices *snapshot.B
 	}
 
 	if len(matchingBlocks) == 0 {
-		return "", errors.New("no matching blocks found")
+		return snapshot.BlockDevice{}, errors.New("no matching blocks found")
 	}
 
-	var target string
 	for _, b := range matchingBlocks {
 		log.Debug().Str("name", b.Name).Msg("device connection> checking block")
-		mounted := false
 		for _, ch := range b.Children {
 			if len(ch.MountPoint) > 0 && ch.MountPoint != "" {
 				log.Debug().Str("name", ch.Name).Msg("device connection> has mounted partitons, skipping")
-				mounted = true
-			}
-			if !mounted {
-				target = "/dev/" + b.Name
+			} else {
+				// we found a block that has no mounted partitions
+				return b, nil
 			}
 		}
 	}
 
-	return target, nil
+	return snapshot.BlockDevice{}, errors.New("no matching block found")
 }
 
 // parses the output from running 'lsscsi --brief' and gets the device info, the output looks like this:

providers/os/connection/device/linux/lun_test.go

@@ -79,7 +79,7 @@ func TestFindDeviceByBlock(t *testing.T) {
 		}
 		target, err := findMatchingDeviceByBlock(devices, blockDevices)
 		assert.NoError(t, err)
-		expected := "/dev/sdb"
+		expected := blockDevices.BlockDevices[1]
 		assert.Equal(t, expected, target)
 	})
 
@@ -132,8 +132,7 @@ func TestFindDeviceByBlock(t *testing.T) {
 				},
 			},
 		}
-		target, err := findMatchingDeviceByBlock(devices, blockDevices)
-		assert.NoError(t, err)
-		assert.Empty(t, target)
+		_, err := findMatchingDeviceByBlock(devices, blockDevices)
+		assert.Error(t, err)
 	})
 }

providers/os/connection/snapshot/blockdevices.go

@@ -74,6 +74,7 @@ func (blockEntries BlockDevices) GetRootBlockEntry() (*PartitionInfo, error) {
 
 // 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.
+// Deprecated: Use GetMountablePartition instead
 func (blockEntries BlockDevices) GetMountablePartitionByDevice(device string) (*PartitionInfo, error) {
 	log.Debug().Str("device", device).Msg("get partitions for device")
 	var block BlockDevice
@@ -123,6 +124,55 @@ func (blockEntries BlockDevices) GetMountablePartitionByDevice(device string) (*
 	return &PartitionInfo{Name: devFsName, FsType: partitions[0].FsType}, nil
 }
 
+// Searches for a device by name
+func (blockEntries BlockDevices) FindDevice(name string) (BlockDevice, error) {
+	log.Debug().Str("device", name).Msg("searching for device")
+	var secondName string
+	if strings.HasPrefix(name, "/dev/sd") {
+		// sdh and xvdh are interchangeable
+		end := strings.TrimPrefix(name, "/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 secondName == "" || secondName != fullDeviceName {
+				continue
+			}
+		}
+		log.Debug().Str("name", d.Name).Msg("found matching device")
+		return d, nil
+	}
+
+	return BlockDevice{}, fmt.Errorf("no block device found with name %s", name)
+}
+
+// Searches all the partitions in the 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 (device BlockDevice) GetMountablePartition() (*PartitionInfo, error) {
+	log.Debug().Str("device", device.Name).Msg("get partitions for device")
+	partitions := []BlockDevice{}
+	for _, partition := range device.Children {
+		log.Debug().Str("name", partition.Name).Int("size", partition.Size).Msg("checking partition")
+		if partition.IsNoBootVolumeAndUnmounted() {
+			partitions = append(partitions, partition)
+		}
+	}
+
+	if len(partitions) == 0 {
+		return nil, fmt.Errorf("no suitable partitions found on device %s", device.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
@@ -177,10 +227,18 @@ func (entry BlockDevice) IsNoBootVolume() bool {
 	return entry.Uuid != "" && entry.FsType != "" && entry.FsType != "vfat" && entry.Label != "EFI" && entry.Label != "boot"
 }
 
+func (entry BlockDevice) IsNoBootVolumeAndUnmounted() bool {
+	return entry.IsNoBootVolume() && !entry.IsMounted()
+}
+
 func (entry BlockDevice) IsRootVolume() bool {
 	return strings.Contains(entry.Name, "sda") || strings.Contains(entry.Name, "xvda") || strings.Contains(entry.Name, "nvme0n1")
 }
 
 func (entry BlockDevice) IsNotBootOrRootVolumeAndUnmounted() bool {
-	return entry.IsNoBootVolume() && entry.MountPoint == "" && !entry.IsRootVolume()
+	return entry.IsNoBootVolumeAndUnmounted() && !entry.IsRootVolume()
+}
+
+func (entry BlockDevice) IsMounted() bool {
+	return entry.MountPoint != ""
 }

providers/os/connection/snapshot/blockdevices_test.go

@@ -118,6 +118,147 @@ func TestGetMountablePartitionByDevice(t *testing.T) {
 	})
 }
 
+func TestFindDevice(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"}}},
+			},
+		}
+
+		res, err := blockEntries.FindDevice("/dev/sdx")
+		require.Nil(t, err)
+		require.Equal(t, res, blockEntries.BlockDevices[2])
+	})
+
+	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: "xvdc", Children: []BlockDevice{{Uuid: "12346", FsType: "xfs", Label: "ROOT", Name: "sdh1"}, {Uuid: "12345", FsType: "", Label: "EFI"}}},
+			},
+		}
+
+		res, err := blockEntries.FindDevice("/dev/sdc")
+		require.Nil(t, err)
+		require.Equal(t, res, blockEntries.BlockDevices[2])
+	})
+
+	t.Run("no match", 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: "xvdc", Children: []BlockDevice{{Uuid: "12346", FsType: "xfs", Label: "ROOT", Name: "sdh1"}, {Uuid: "12345", FsType: "", Label: "EFI"}}},
+			},
+		}
+
+		_, err := blockEntries.FindDevice("/dev/sdd")
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "no block device found with name")
+	})
+}
+
+func TestGetMountablePartition(t *testing.T) {
+	t.Run("no suitable partition (mounted)", func(t *testing.T) {
+		block := BlockDevice{
+			Name: "sda",
+			Children: []BlockDevice{
+				{Uuid: "1234", FsType: "xfs", Label: "ROOT", Name: "sda1", MountPoint: "/"},
+			},
+		}
+		_, err := block.GetMountablePartition()
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "no suitable partitions found")
+	})
+
+	t.Run("no suitable partition (no fs type)", func(t *testing.T) {
+		block := BlockDevice{
+			Name: "sda",
+			Children: []BlockDevice{
+				{Uuid: "1234", FsType: "", Label: "ROOT", Name: "sda1", MountPoint: ""},
+			},
+		}
+		_, err := block.GetMountablePartition()
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "no suitable partitions found")
+	})
+
+	t.Run("no suitable partition (EFI label)", func(t *testing.T) {
+		block := BlockDevice{
+			Name: "sda",
+			Children: []BlockDevice{
+				{Uuid: "1234", FsType: "xfs", Label: "EFI", Name: "sda1", MountPoint: ""},
+			},
+		}
+		_, err := block.GetMountablePartition()
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "no suitable partitions found")
+	})
+
+	t.Run("no suitable partition (vfat fs type)", func(t *testing.T) {
+		block := BlockDevice{
+			Name: "sda",
+			Children: []BlockDevice{
+				{Uuid: "1234", FsType: "vfat", Label: "", Name: "sda1", MountPoint: ""},
+			},
+		}
+		_, err := block.GetMountablePartition()
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "no suitable partitions found")
+	})
+
+	t.Run("no suitable partition (boot label)", func(t *testing.T) {
+		block := BlockDevice{
+			Name: "sda",
+			Children: []BlockDevice{
+				{Uuid: "1234", FsType: "xfs", Label: "boot", Name: "sda1", MountPoint: ""},
+			},
+		}
+		_, err := block.GetMountablePartition()
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "no suitable partitions found")
+	})
+
+	t.Run("no suitable partition (empty)", func(t *testing.T) {
+		block := BlockDevice{
+			Name:     "sda",
+			Children: []BlockDevice{},
+		}
+		_, err := block.GetMountablePartition()
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "no suitable partitions found")
+	})
+
+	t.Run("suitable single partition", func(t *testing.T) {
+		block := BlockDevice{
+			Name: "sde",
+			Children: []BlockDevice{
+				{Uuid: "12346", FsType: "xfs", Size: 110, Label: "ROOT", Name: "sde1"},
+			},
+		}
+		partition, err := block.GetMountablePartition()
+		require.Nil(t, err)
+		require.Equal(t, &PartitionInfo{FsType: "xfs", Name: "/dev/sde1"}, partition)
+	})
+
+	t.Run("largest suitable partition", func(t *testing.T) {
+		block := BlockDevice{
+			Name: "sda",
+			Children: []BlockDevice{
+				{Uuid: "12346", FsType: "xfs", Size: 110, Label: "ROOT", Name: "sda1"},
+				{Uuid: "12346", FsType: "xfs", Size: 120, Label: "ROOT", Name: "sda2"},
+			},
+		}
+		partition, err := block.GetMountablePartition()
+		require.Nil(t, err)
+		require.Equal(t, &PartitionInfo{FsType: "xfs", Name: "/dev/sda2"}, partition)
+	})
+}
+
 func TestGetNonRootBlockEntry(t *testing.T) {
 	blockEntries := BlockDevices{BlockDevices: []BlockDevice{
 		{Name: "sda", Children: []BlockDevice{{Uuid: "1234", FsType: "xfs", Label: "ROOT", Name: "sda1", MountPoint: "/"}}},

providers/os/connection/snapshot/volumemounter.go

@@ -139,7 +139,12 @@ func (m *VolumeMounter) GetMountablePartition(device string) (*PartitionInfo, er
 		// we need to simplify those
 		return blockDevices.GetUnnamedBlockEntry()
 	}
-	return blockDevices.GetMountablePartitionByDevice(device)
+
+	d, err := blockDevices.FindDevice(device)
+	if err != nil {
+		return nil, err
+	}
+	return d.GetMountablePartition()
 }
 
 func (m *VolumeMounter) mountVolume(fsInfo *PartitionInfo) error {