Refactor the code to directly use BlockDevice

Author: gnufied

pkg/common/provisioner_utils.go

@@ -50,21 +50,14 @@ type CreateLocalPVArgs struct {
 	StorageClass          storagev1.StorageClass
 	MountPointMap         sets.String
 	Client                client.Client
-	SymLinkPath           string
-	// DevicePath is the full path to the device (e.g. /dev/sda).
-	// filepath.Base is applied internally where only the kernel name is needed.
-	DevicePath       string
+	SymLinkPath      string
 	IDExists         bool
 	ExtraLabelsForPV map[string]string
 
-	// preferredsymlink stores the preferred symlink according to
-	// LSO heuristics
-	PreferredSymLink string
-	// CurrentSymlink points to source to which
-	// SymLinkPath points to.
+	// CurrentSymlink points to source to which SymLinkPath points to.
 	CurrentSymlink string
-	// Kernel name of the device
-	KName string
+	// BlockDevice is the block device backing this PV.
+	BlockDevice internal.BlockDevice
 }
 
 // CreateLocalPV is used to create a local PV against a symlink
@@ -76,7 +69,7 @@ func CreateLocalPV(ctx context.Context, args CreateLocalPVArgs) error {
 	mountPointMap := args.MountPointMap
 	client := args.Client
 	symLinkPath := args.SymLinkPath
-	deviceName := filepath.Base(args.DevicePath)
+	deviceName := args.BlockDevice.KName
 	idExists := args.IDExists
 	extraLabelsForPV := args.ExtraLabelsForPV
 	nodeLabels := runtimeConfig.Node.GetLabels()
@@ -126,7 +119,7 @@ func CreateLocalPV(ctx context.Context, args CreateLocalPVArgs) error {
 		return fmt.Errorf("name: %q, namespace: %q, or  kind: %q is empty for obj: %+v", name, namespace, kind, obj)
 	}
 
-	deviceHandler := internal.NewDeviceLinkHandler(args.CurrentSymlink, args.PreferredSymLink, client)
+	deviceHandler := internal.NewDeviceLinkHandler(args.CurrentSymlink, client)
 
 	_, err = deviceHandler.Create(ctx, pvName, runtimeConfig.Namespace, obj)
 	if err != nil {
@@ -263,7 +256,7 @@ func CreateLocalPV(ctx context.Context, args CreateLocalPVArgs) error {
 		return fmt.Errorf("error creating pv %s: %w", pvName, err)
 	}
 
-	_, err = deviceHandler.ApplyStatus(ctx, pvName, runtimeConfig.Namespace, args.KName, args.DevicePath, obj)
+	_, err = deviceHandler.ApplyStatus(ctx, pvName, runtimeConfig.Namespace, args.BlockDevice, obj)
 	if err != nil {
 		return fmt.Errorf("error updating localvolumedevicelink object: %w", err)
 	}

pkg/diskmaker/controllers/lv/create_pv_test.go

@@ -255,7 +255,7 @@ func TestCreatePV(t *testing.T) {
 			MountPointMap:         tc.mountPoints,
 			Client:                r.Client,
 			SymLinkPath:           tc.symlinkpath,
-			DevicePath:            tc.deviceName,
+			BlockDevice:           internal.BlockDevice{KName: filepath.Base(tc.deviceName)},
 			IDExists:              true,
 			ExtraLabelsForPV:      map[string]string{},
 		})
@@ -304,7 +304,7 @@ func TestCreatePV(t *testing.T) {
 			MountPointMap:         tc.mountPoints,
 			Client:                r.Client,
 			SymLinkPath:           tc.symlinkpath,
-			DevicePath:            tc.deviceName,
+			BlockDevice:           internal.BlockDevice{KName: filepath.Base(tc.deviceName)},
 			IDExists:              true,
 			ExtraLabelsForPV:      map[string]string{},
 		})
@@ -412,8 +412,7 @@ func TestCreateLocalPV_DeviceLinkArgOrder(t *testing.T) {
 		MountPointMap:         sets.NewString(),
 		Client:                r.Client,
 		SymLinkPath:           symLinkPath,
-		DevicePath:            devPath,
-		KName:                 kName,
+		BlockDevice:           internal.BlockDevice{KName: kName, PathByID: fakeByIDLink},
 		IDExists:              true,
 		ExtraLabelsForPV:      map[string]string{},
 	})

pkg/diskmaker/controllers/lv/reconcile.go

@@ -463,12 +463,10 @@ func (r *LocalVolumeReconciler) Reconcile(ctx context.Context, request ctrl.Requ
 					MountPointMap:         mountPointMap,
 					Client:                r.Client,
 					SymLinkPath:           target,
-					DevicePath:            deviceNameLocation.DiskNamePath,
 					IDExists:              idExists,
 					ExtraLabelsForPV:      lvOwnerLabels,
-					PreferredSymLink:      deviceNameLocation.DiskID,
 					CurrentSymlink:        source,
-					KName:                 deviceNameLocation.BlockDevice.KName,
+					BlockDevice:           deviceNameLocation.BlockDevice,
 				}
 
 				err = common.CreateLocalPV(ctx, createLocalPVArgs)
@@ -526,22 +524,11 @@ func (r *LocalVolumeReconciler) processRejectedDevicesForDeviceLinks(ctx context
 
 			blockDevice := diskLocation.BlockDevice
 
-			preferredSymLink, err := blockDevice.GetPathByID("")
-			if err != nil {
-				klog.ErrorS(err, "", "failed to get preferred device link", "disk", blockDevice.Name)
-				continue
-			}
-
-			devicePath, err := blockDevice.GetDevPath()
-			if err != nil {
-				klog.ErrorS(err, "failed to get /dev path for block device", "disk", blockDevice.Name)
-				continue
-			}
-
 			pvName := common.GeneratePVName(existingSymlink, r.runtimeConfig.Node.Name, storageClassName)
-			deviceHandler := internal.NewDeviceLinkHandler(source, preferredSymLink, r.Client)
-			_, err = deviceHandler.ApplyStatus(ctx, pvName, r.runtimeConfig.Namespace, blockDevice.KName, devicePath, r.localVolume)
+			deviceHandler := internal.NewDeviceLinkHandler(source, r.Client)
+			_, err = deviceHandler.ApplyStatus(ctx, pvName, r.runtimeConfig.Namespace, blockDevice, r.localVolume)
 			if err != nil {
+				r.eventSync.Report(r.localVolume, newDiskEvent(diskmaker.ErrorCreatingLVDL, "failed to create localvolumedevicelink", blockDevice.KName, corev1.EventTypeWarning))
 				klog.ErrorS(err, "error updating LocalVolumeDeviceLink", "device", blockDevice.Name)
 			}
 		}

pkg/diskmaker/controllers/lvset/create_pv_test.go

@@ -234,7 +234,7 @@ func TestCreatePV(t *testing.T) {
 			MountPointMap:         tc.mountPoints,
 			Client:                r.Client,
 			SymLinkPath:           tc.symlinkpath,
-			DevicePath:            tc.deviceName,
+			BlockDevice:           internal.BlockDevice{KName: filepath.Base(tc.deviceName)},
 			IDExists:              true,
 			ExtraLabelsForPV:      map[string]string{},
 		})
@@ -283,7 +283,7 @@ func TestCreatePV(t *testing.T) {
 			MountPointMap:         tc.mountPoints,
 			Client:                r.Client,
 			SymLinkPath:           tc.symlinkpath,
-			DevicePath:            tc.deviceName,
+			BlockDevice:           internal.BlockDevice{KName: filepath.Base(tc.deviceName)},
 			IDExists:              true,
 			ExtraLabelsForPV:      map[string]string{},
 		})
@@ -361,8 +361,7 @@ func TestCreatePV_SetsLVDLOwnerRefToLocalVolumeSet(t *testing.T) {
 		MountPointMap:         sets.NewString(),
 		Client:                r.Client,
 		SymLinkPath:           symLinkPath,
-		DevicePath:            "/dev/device-ownerref",
-		KName:                 "device-ownerref",
+		BlockDevice:           internal.BlockDevice{KName: "device-ownerref"},
 		IDExists:              true,
 		ExtraLabelsForPV:      map[string]string{},
 	})

pkg/diskmaker/controllers/lvset/reconcile.go

@@ -283,22 +283,11 @@ func (r *LocalVolumeSetReconciler) processRejectedDevicesForDeviceLinks(ctx cont
 				"blockDevice", blockDevice.Name)
 			continue
 		}
-		preferredSymLink, err := blockDevice.GetPathByID("")
-		if err != nil {
-			klog.ErrorS(err, "", "failed to get preferred device link", "disk", blockDevice.Name)
-			continue
-		}
-
-		devicePath, err := blockDevice.GetDevPath()
-		if err != nil {
-			klog.ErrorS(err, "failed to get /dev path for block device", "disk", blockDevice.Name)
-			continue
-		}
-
 		pvName := common.GeneratePVName(existingSymlink, r.runtimeConfig.Node.Name, storageClassName)
-		deviceHandler := internal.NewDeviceLinkHandler(symlinkSourcePath, preferredSymLink, r.Client)
-		_, err = deviceHandler.ApplyStatus(ctx, pvName, r.runtimeConfig.Namespace, blockDevice.KName, devicePath, lvset)
+		deviceHandler := internal.NewDeviceLinkHandler(symlinkSourcePath, r.Client)
+		_, err = deviceHandler.ApplyStatus(ctx, pvName, r.runtimeConfig.Namespace, blockDevice, lvset)
 		if err != nil {
+			r.eventReporter.Report(lvset, newDiskEvent(diskmaker.ErrorCreatingLVDL, "failed to create localvolumedevicelink", blockDevice.KName, corev1.EventTypeWarning))
 			klog.ErrorS(err, "error updating LocalVolumeDeviceLink", "device", blockDevice.Name)
 		}
 	}
@@ -468,28 +457,17 @@ func (r *LocalVolumeSetReconciler) provisionPV(
 		}
 	}
 
-	// find what would be preferred symlink for the device
-	// TODO: This may not be a fatal error
-	preferredSymLink, err := dev.GetPathByID("")
-	if err != nil {
-		klog.ErrorS(err, "", "failed to get preferred device link", "disk", devLabelPath)
-		return err
-	}
-
 	createLocalPVArgs := common.CreateLocalPVArgs{
 		LocalVolumeLikeObject: obj,
 		RuntimeConfig:         r.runtimeConfig,
 		StorageClass:          storageClass,
 		MountPointMap:         mountPointMap,
 		Client:                r.Client,
 		SymLinkPath:           symlinkPath,
-		DevicePath:            devLabelPath,
 		IDExists:              idExists,
 		ExtraLabelsForPV:      map[string]string{},
-
-		CurrentSymlink:   symlinkSourcePath,
-		KName:            dev.KName,
-		PreferredSymLink: preferredSymLink,
+		CurrentSymlink:        symlinkSourcePath,
+		BlockDevice:           dev,
 	}
 
 	defer unlockFunc()

pkg/diskmaker/event_reporter.go

@@ -22,6 +22,8 @@ const (
 	FoundMatchingDisk   = "FoundMatchingDisk"
 	DeviceSymlinkExists = "DeviceSymlinkExists"
 
+	ErrorCreatingLVDL = "ErrorCreatingLocalVolumeDeviceLink"
+
 	// LocalVolumeDiscovery events
 	ErrorCreatingDiscoveryResultObject = "ErrorCreatingDiscoveryResultObject"
 	ErrorUpdatingDiscoveryResultObject = "ErrorUpdatingDiscoveryResultObject"

pkg/internal/device_link_handler.go

@@ -2,6 +2,7 @@ package internal
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os/exec"
 	"reflect"
@@ -22,21 +23,14 @@ import (
 )
 
 type DeviceLinkHandler struct {
-	currentSymlink   string
-	preferredSymlink string
-
-	// devicePoints to current device such as /dev/sda or something similar
-	// so basically this is direct path in /dev filesystem to which
-	// currentSymlink or preferredSymlink resolves to
-	devicePath string
-	client     client.Client
+	currentSymlink string
+	client         client.Client
 }
 
-func NewDeviceLinkHandler(currentSymlink, preferredSymlink string, client client.Client) *DeviceLinkHandler {
+func NewDeviceLinkHandler(currentSymlink string, client client.Client) *DeviceLinkHandler {
 	return &DeviceLinkHandler{
-		currentSymlink:   currentSymlink,
-		preferredSymlink: preferredSymlink,
-		client:           client,
+		currentSymlink: currentSymlink,
+		client:         client,
 	}
 }
 
@@ -145,8 +139,24 @@ func isNilOwnerObject(ownerObj runtime.Object) bool {
 	return value.Kind() == reflect.Ptr && value.IsNil()
 }
 
-func (dl *DeviceLinkHandler) ApplyStatus(ctx context.Context, pvName, namespace, kName, devicePath string, ownerObj runtime.Object) (*v1.LocalVolumeDeviceLink, error) {
-	klog.V(2).Infof("updating lvdl with currentSymlink: %s, preferredSymlink: %s, devicePath: %s, kname: %s", dl.currentSymlink, dl.preferredSymlink, devicePath, kName)
+func (dl *DeviceLinkHandler) ApplyStatus(ctx context.Context, pvName, namespace string, blockDevice BlockDevice, ownerObj runtime.Object) (*v1.LocalVolumeDeviceLink, error) {
+	preferredSymlink, err := blockDevice.GetUncachedPathID()
+	if err != nil {
+		// IDPathNotFoundError means no by-id symlink exists for this device;
+		// treat it as "no preferred symlink" rather than a hard error.
+		var idNotFound IDPathNotFoundError
+		if !errors.As(err, &idNotFound) {
+			return nil, fmt.Errorf("failed to get preferred device link for %s: %w", blockDevice.Name, err)
+		}
+		preferredSymlink = ""
+	}
+
+	devicePath, err := blockDevice.GetDevPath()
+	if err != nil {
+		return nil, fmt.Errorf("failed to get /dev path for %s: %w", blockDevice.Name, err)
+	}
+
+	klog.V(2).Infof("updating lvdl with currentSymlink: %s, preferredSymlink: %s, devicePath: %s, kname: %s", dl.currentSymlink, preferredSymlink, devicePath, blockDevice.KName)
 
 	// Update is best-effort and independent from Create: if either the PV or
 	// the LVDL does not exist yet, return without doing anything.
@@ -169,7 +179,7 @@ func (dl *DeviceLinkHandler) ApplyStatus(ctx context.Context, pvName, namespace,
 		return nil, nil
 	}
 
-	validLinks, err := dl.getValidByIDSymlinks(kName)
+	validLinks, err := dl.getValidByIDSymlinks(blockDevice.KName)
 	if err != nil {
 		return nil, err
 	}
@@ -183,7 +193,7 @@ func (dl *DeviceLinkHandler) ApplyStatus(ctx context.Context, pvName, namespace,
 	updatedCopy := existing.DeepCopy()
 
 	updatedCopy.Status.CurrentLinkTarget = dl.currentSymlink
-	updatedCopy.Status.PreferredLinkTarget = dl.preferredSymlink
+	updatedCopy.Status.PreferredLinkTarget = preferredSymlink
 	updatedCopy.Status.ValidLinkTargets = validLinks
 	updatedCopy.Status.FilesystemUUID = filesystemUUID