Use BusPath to resolve dm device paths

pkg/controller/blockdevice/controller.go

@@ -137,9 +137,6 @@ func (c *Controller) OnBlockDeviceChange(_ string, device *diskv1.BlockDevice) (
 	if err != nil {
 		return nil, err
 	}
-	if devPath == "" {
-		return nil, fmt.Errorf("failed to resolve persistent dev path for block device %s", device.Name)
-	}
 
 	logrus.Debugf("Checking to format device %s", device.Name)
 	if formatted, requeue, err := provisionerInst.Format(devPath); !formatted {

pkg/provisioner/common.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 	"sync"
 
 	ghwutil "github.com/jaypipes/ghw/pkg/util"
@@ -200,49 +201,98 @@ func convertMountStr(mountOP NeedMountUpdateOP) string {
 	}
 }
 
+// ResolvePersistentDevPath tries to determine the currently active short
+// device path (e.g. "/dev/sda") for a given block device.  When the scanner
+// first finds a new block device, device.Spec.DevPath is set to the short
+// device path at that time, and device.Status.DeviceStatus.Details is filled
+// in with data that uniquely identifies the device (e.g.: WWN).  It's possible
+// that on subsequent reboots, the short path will change, for example if
+// devices are added or removed, so we have this function to try to figure
+// out the _current_ short device path based on the unique identifying
+// information in device.Status.DeviceStatus.Details.
 func ResolvePersistentDevPath(device *diskv1.BlockDevice) (string, error) {
 	switch device.Status.DeviceStatus.Details.DeviceType {
 	case diskv1.DeviceTypeDisk:
-		// Disk naming priority.
-		// #1 WWN (REF: https://en.wikipedia.org/wiki/World_Wide_Name#Formats)
-		// #2 filesystem UUID (UUID) (REF: https://wiki.archlinux.org/title/Persistent_block_device_naming#by-uuid)
-		// #3 partition table UUID (PTUUID) (DEPRECATED)
-		// #4 PtUUID as UUID to query disk info (DEPRECATED)
-		//    (NDM might reuse PtUUID as UUID to format a disk)
-		if wwn := device.Status.DeviceStatus.Details.WWN; valueExists(wwn) {
-			if device.Status.DeviceStatus.Details.StorageController == string(diskv1.StorageControllerNVMe) {
-				return filepath.EvalSymlinks("/dev/disk/by-id/nvme-" + wwn)
+		// The following closure is the original implementation to resolve disk
+		// device paths, but there is a problem: it multipathd has taken over a
+		// device, the calls to filepath.EvalSymlinks() with "/dev/disk/by-id/"
+		// or "/dev/disk/by-uuid" paths will actually return a "/dev/dm-*"
+		// device, which we don't want.  We want the real underlying device
+		// (e.g. "/dev/sda").  If we take a "/dev/dm-*" path and later update
+		// the blockdevice CR with it, we lose all the interesting DeviceStatus
+		// information, like the WWN.
+		path, err := func() (string, error) {
+			// Disk naming priority.
+			// #1 WWN (REF: https://en.wikipedia.org/wiki/World_Wide_Name#Formats)
+			// #2 filesystem UUID (UUID) (REF: https://wiki.archlinux.org/title/Persistent_block_device_naming#by-uuid)
+			// #3 partition table UUID (PTUUID) (DEPRECATED)
+			// #4 PtUUID as UUID to query disk info (DEPRECATED)
+			//    (NDM might reuse PtUUID as UUID to format a disk)
+			if wwn := device.Status.DeviceStatus.Details.WWN; valueExists(wwn) {
+				if device.Status.DeviceStatus.Details.StorageController == string(diskv1.StorageControllerNVMe) {
+					return filepath.EvalSymlinks("/dev/disk/by-id/nvme-" + wwn)
+				}
+				return filepath.EvalSymlinks("/dev/disk/by-id/wwn-" + wwn)
 			}
-			return filepath.EvalSymlinks("/dev/disk/by-id/wwn-" + wwn)
+			if fsUUID := device.Status.DeviceStatus.Details.UUID; valueExists(fsUUID) {
+				path, err := filepath.EvalSymlinks("/dev/disk/by-uuid/" + fsUUID)
+				if err == nil {
+					return path, nil
+				}
+				if !errors.Is(err, os.ErrNotExist) {
+					return "", err
+				}
+			}
+
+			if ptUUID := device.Status.DeviceStatus.Details.PtUUID; valueExists(ptUUID) {
+				path, err := block.GetDevPathByPTUUID(ptUUID)
+				if err != nil {
+					return "", err
+				}
+				if path != "" {
+					return path, nil
+				}
+				return filepath.EvalSymlinks("/dev/disk/by-uuid/" + ptUUID)
+			}
+
+			// If we haven't resolved a path by now, it means the device has no WWN,
+			// no FSUUID and no PTUUID, but there is still one more thing we can try...
+			return "", nil
+		}()
+
+		if err != nil {
+			return "", err
+		}
+
+		// ...at this point, if there's no error, we've either got the device we're
+		// interested in (e.g. "/dev/sda", "/dev/nvme0n1", etc.), _or_ we've got a
+		// "/dev/dm-*" device, _or_ we've got no path...
+		if path != "" && !strings.HasPrefix(path, "/dev/dm-") {
+			logrus.Debugf("Resolved device path %s for %s", path, device.Name)
+			return path, nil
 		}
-		if fsUUID := device.Status.DeviceStatus.Details.UUID; valueExists(fsUUID) {
-			path, err := filepath.EvalSymlinks("/dev/disk/by-uuid/" + fsUUID)
+		// ...in the latter two cases, we can try to resolve via "/dev/disk/by-path/...",
+		// which works for devices that don't have a WWN, and also in the dm case will
+		// return the path to the underlying device that we're actually interested in.
+		if busPath := device.Status.DeviceStatus.Details.BusPath; valueExists(busPath) {
+			path, err = filepath.EvalSymlinks("/dev/disk/by-path/" + busPath)
 			if err == nil {
+				logrus.Debugf("Resolved BusPath %s to %s for %s", busPath, path, device.Name)
 				return path, nil
 			}
 			if !errors.Is(err, os.ErrNotExist) {
 				return "", err
 			}
 		}
 
-		if ptUUID := device.Status.DeviceStatus.Details.PtUUID; valueExists(ptUUID) {
-			path, err := block.GetDevPathByPTUUID(ptUUID)
-			if err != nil {
-				return "", err
-			}
-			if path != "" {
-				return path, nil
-			}
-			return filepath.EvalSymlinks("/dev/disk/by-uuid/" + ptUUID)
-		}
-		return "", fmt.Errorf("WWN/UUID/PTUUID was not found on device %s", device.Name)
+		return "", fmt.Errorf("WWN/UUID/PTUUID/BusPath was not found on device %s", device.Name)
 	case diskv1.DeviceTypePart:
 		partUUID := device.Status.DeviceStatus.Details.PartUUID
 		if partUUID == "" {
 			return "", fmt.Errorf("PARTUUID was not found on device %s", device.Name)
 		}
 		return filepath.EvalSymlinks("/dev/disk/by-partuuid/" + partUUID)
 	default:
-		return "", nil
+		return "", fmt.Errorf("failed to resolve persistent dev path for block device %s", device.Name)
 	}
 }

pkg/utils/utils.go

@@ -76,8 +76,9 @@ func MakeExt4DiskFormatting(devPath, uuid string) error {
 		args = append(args, "-U", uuid)
 	}
 	cmd := exec.Command("mkfs.ext4", args...)
-	if _, err := cmd.CombinedOutput(); err != nil {
-		return fmt.Errorf("failed to format %s. err: %v", devPath, err)
+	if output, err := cmd.CombinedOutput(); err != nil {
+		return fmt.Errorf("failed to format %s. %v: %s", devPath, err,
+			strings.ReplaceAll(strings.TrimSpace(string(output)), "\n", " "))
 	}
 	return nil
 }