diff --git a/pkg/controller/blockdevice/controller.go b/pkg/controller/blockdevice/controller.go index 8ef4c184..f06e3c79 100644 --- a/pkg/controller/blockdevice/controller.go +++ b/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 { diff --git a/pkg/provisioner/common.go b/pkg/provisioner/common.go index faa3a1ce..145e8b48 100644 --- a/pkg/provisioner/common.go +++ b/pkg/provisioner/common.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "sync" ghwutil "github.com/jaypipes/ghw/pkg/util" @@ -200,24 +201,83 @@ 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) { @@ -225,17 +285,7 @@ func ResolvePersistentDevPath(device *diskv1.BlockDevice) (string, error) { } } - 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 == "" { @@ -243,6 +293,6 @@ func ResolvePersistentDevPath(device *diskv1.BlockDevice) (string, error) { } 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) } } diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index b77d5432..f11036c3 100644 --- a/pkg/utils/utils.go +++ b/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 }