OCPBUGS-98984: Implement opt in for reclaim disk

pkg/common/current_block_device_info.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"os"
 	"path/filepath"
 	"slices"
 
@@ -59,11 +60,11 @@ type CurrentBlockDeviceInfo struct {
 //
 //	example - /dev/disk/by-id/wwn-0x123432
 //
-// This function MUST be called only when there is no valid symlinks for `newSymlinkSourcePath`
-// in `/mnt/local-storage/<sc>` and yet LSO MUST have created a PV for device pointed by `newSymlinkSourcePath`
-// in previously. So this function is our last resort to find a valid symlink path for PV.
-// Only return valid new SymlinkPath if currentLinkTarget doesn't resolve and user has asked
-// for symlinks to be recreated.
+// This function MUST be called only when there is no valid symlink for `newSymlinkSourcePath`
+// in `/mnt/local-storage/<sc>` and yet LSO previously created a PV for the device pointed by
+// `newSymlinkSourcePath`. It is our last resort to find the PV symlink path that should be
+// recreated. Returns the recovered path only when the LVDL policy is PreferredLinkTarget and
+// the PV symlink is missing or no longer points at status.currentLinkTarget.
 func (c CurrentBlockDeviceInfo) RecoverPVSymlinkPath(ctx context.Context, symlinkDir, newSymlinkSourcePath string, client client.Client) (string, error) {
 	lvdls := c.lvdls
 	if len(lvdls) > 1 {
@@ -92,10 +93,19 @@ func (c CurrentBlockDeviceInfo) RecoverPVSymlinkPath(ctx context.Context, symlin
 		return "", PolicyNotPreferredError{SymlinkSource: newSymlinkSourcePath, SymlinkDir: symlinkDir, LVDL: lvdl, PVSymlinkPath: symlinkPath}
 	}
 
-	// check if currentLinkTarget resolves to a valid device, if yes then no need to do anything
-	resolvedCurrent, err := internal.FilePathEvalSymLinks(currentLinkTarget)
-	if err == nil {
-		return "", fmt.Errorf("currentSymlink %s still resolves to %s for %s", currentLinkTarget, resolvedCurrent, newSymlinkSourcePath)
+	// If the PV symlink still points at currentLinkTarget and that target is
+	// already the source we are recovering for, nothing is broken.
+	// When newSymlinkSourcePath differs (e.g. stale by-id / sibling fallback),
+	// keep recovering so PreferredLinkTarget can relink to the new preferred path.
+	// Only treat a missing symlink (or a successfully read alternate target) as
+	// recoverable; permission/I/O/non-symlink failures must surface.
+	linkTarget, err := internal.Readlink(symlinkPath)
+	if err != nil {
+		if !os.IsNotExist(err) {
+			return "", fmt.Errorf("error reading PV symlink %s: %w", symlinkPath, err)
+		}
+	} else if linkTarget == currentLinkTarget && currentLinkTarget == newSymlinkSourcePath {
+		return "", fmt.Errorf("PV symlink %s still points to currentLinkTarget %s for %s", symlinkPath, currentLinkTarget, newSymlinkSourcePath)
 	}
 
 	if slices.Contains(validLinkTargets, newSymlinkSourcePath) {

pkg/common/device_link_handler.go

@@ -263,8 +263,9 @@ func (dl *DeviceLinkHandler) findOrCreateLVDL(ctx context.Context, pvName, names
 	return existing, nil
 }
 
-// RecreateSymlinkIfNeeded checks the LVDL policy and atomically recreates
-// the symlink if policy is PreferredLinkTarget and currentLinkTarget != preferredLinkTarget.
+// RecreateSymlinkIfNeeded atomically recreates the symlink at symLinkPath to point at
+// the preferred by-id path. Callers should invoke this when HasMismatchingSymlink is true
+// (preferred != current, or the PV symlink is missing under PreferredLinkTarget policy).
 // symLinkPath is the full path under /mnt/local-storage/<storageClass>/<deviceName>.
 // Returns nil if no action is needed or action succeeded, error if action failed.
 // On error, it sets a failure OperatorCondition on the LVDL object.
@@ -289,6 +290,12 @@ func (dl *DeviceLinkHandler) RecreateSymlinkIfNeeded(ctx context.Context, lvdl *
 	}
 
 	symLinkDir := filepath.Dir(symLinkPath)
+	// Ensure the parent directory exists (e.g. after an admin removed /mnt/local-storage/<sc>).
+	if err := os.MkdirAll(symLinkDir, 0755); err != nil {
+		msg := fmt.Sprintf("failed to create symlink dir %s: %v", symLinkDir, err)
+		condition := getCondition("MkdirFailed", msg, operatorv1.ConditionTrue)
+		return dl.updateStatus(ctx, lvdl, condition, blockDevice, preferredTarget, currentTarget, symLinkPath)
+	}
 	entries, err := internal.FilePathGlob(symLinkDir + "/*")
 	if err != nil {
 		msg := fmt.Sprintf("failed to list symlink dir %s: %v", symLinkDir, err)
@@ -441,36 +448,51 @@ func (dl *DeviceLinkHandler) setLVDLCondition(lvdl *v1.LocalVolumeDeviceLink, co
 	return lvdl
 }
 
-func HasMismatchingSymlink(lvdl *v1.LocalVolumeDeviceLink, blockDevice internal.BlockDevice) bool {
+// HasMismatchingSymlink reports whether the PV symlink under /mnt/local-storage needs
+// recreation under PreferredLinkTarget policy. That is true when the symlink is missing
+// on disk, or when currentLinkTarget differs from the preferred by-id path.
+// Unexpected Lstat failures (permission, I/O, etc.) are returned so callers can
+// requeue instead of treating the link as present.
+func HasMismatchingSymlink(lvdl *v1.LocalVolumeDeviceLink, blockDevice internal.BlockDevice, symlinkPath string) (bool, error) {
 	lvdlName := "<nil>"
 	if lvdl != nil {
 		lvdlName = lvdl.Name
 	}
 	klog.V(4).Infof("checking for mismatching symlinks, lvdl %s", lvdlName)
 	if lvdl == nil {
-		return false
+		return false, nil
 	}
 	if lvdl.Spec.Policy != v1.DeviceLinkPolicyPreferredLinkTarget {
-		return false
+		return false, nil
+	}
+
+	if symlinkPath != "" {
+		if _, err := os.Lstat(symlinkPath); err != nil {
+			if os.IsNotExist(err) {
+				klog.InfoS("PV symlink is missing; recreation needed", "symlinkPath", symlinkPath, "lvdl", lvdlName)
+				return true, nil
+			}
+			return false, fmt.Errorf("error checking PV symlink %s: %w", symlinkPath, err)
+		}
 	}
 
 	preferredTarget, err := blockDevice.GetPathByID()
 	if err != nil {
 		klog.ErrorS(err, "error getting pathbyid for device", "device", blockDevice.Name)
-		return false
+		return false, nil
 	}
 
 	currentTarget := lvdl.Status.CurrentLinkTarget
 	klog.Infof("checking for mismatching symlinks current: %s, preferred: %s", currentTarget, preferredTarget)
 
 	if preferredTarget == "" {
-		return false
+		return false, nil
 	}
 
 	if currentTarget == preferredTarget {
-		return false
+		return false, nil
 	}
-	return true
+	return true, nil
 }
 
 func getCondition(reason, msg string, status operatorv1.ConditionStatus) operatorv1.OperatorCondition {

pkg/common/device_link_handler_test.go

@@ -511,10 +511,15 @@ func assertSingleConditionReason(t *testing.T, lvdl *v1.LocalVolumeDeviceLink, r
 
 // TestHasMismatchingSymlink verifies the HasMismatchingSymlink helper for various policies and states.
 func TestHasMismatchingSymlink(t *testing.T) {
+	existingSymlink := filepath.Join(t.TempDir(), "existing-symlink")
+	assert.NoError(t, os.Symlink("/dev/null", existingSymlink))
+	missingSymlink := filepath.Join(t.TempDir(), "missing-symlink")
+
 	testCases := []struct {
 		name        string
 		lvdl        *v1.LocalVolumeDeviceLink
 		blockDevice internal.BlockDevice
+		symlinkPath string
 		expected    bool
 	}{
 		{
@@ -527,12 +532,14 @@ func TestHasMismatchingSymlink(t *testing.T) {
 			name:        "policy None",
 			lvdl:        newLVDLWithPolicy("pv", "ns", v1.DeviceLinkPolicyNone, "/current", "/preferred"),
 			blockDevice: internal.BlockDevice{PathByID: "/preferred"},
+			symlinkPath: missingSymlink,
 			expected:    false,
 		},
 		{
 			name:        "policy CurrentLinkTarget",
 			lvdl:        newLVDLWithPolicy("pv", "ns", v1.DeviceLinkPolicyCurrentLinkTarget, "/current", "/preferred"),
 			blockDevice: internal.BlockDevice{PathByID: "/preferred"},
+			symlinkPath: missingSymlink,
 			expected:    false,
 		},
 		{
@@ -545,14 +552,30 @@ func TestHasMismatchingSymlink(t *testing.T) {
 			name:        "policy PreferredLinkTarget with matching targets",
 			lvdl:        newLVDLWithPolicy("pv", "ns", v1.DeviceLinkPolicyPreferredLinkTarget, "/same", "/same"),
 			blockDevice: internal.BlockDevice{PathByID: "/same"},
+			symlinkPath: existingSymlink,
 			expected:    false,
 		},
 		{
 			name:        "policy PreferredLinkTarget with mismatching targets",
 			lvdl:        newLVDLWithPolicy("pv", "ns", v1.DeviceLinkPolicyPreferredLinkTarget, "/current", "/dev/disk/by-id/preferred"),
 			blockDevice: internal.BlockDevice{KName: "sda", PathByID: "/dev/disk/by-id/preferred"},
+			symlinkPath: existingSymlink,
 			expected:    true,
 		},
+		{
+			name:        "policy PreferredLinkTarget with missing PV symlink",
+			lvdl:        newLVDLWithPolicy("pv", "ns", v1.DeviceLinkPolicyPreferredLinkTarget, "/same", "/same"),
+			blockDevice: internal.BlockDevice{PathByID: "/same"},
+			symlinkPath: missingSymlink,
+			expected:    true,
+		},
+		{
+			name:        "policy PreferredLinkTarget with matching targets and empty symlink path",
+			lvdl:        newLVDLWithPolicy("pv", "ns", v1.DeviceLinkPolicyPreferredLinkTarget, "/same", "/same"),
+			blockDevice: internal.BlockDevice{PathByID: "/same"},
+			symlinkPath: "",
+			expected:    false,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -561,11 +584,18 @@ func TestHasMismatchingSymlink(t *testing.T) {
 			internal.FilePathEvalSymLinks = func(path string) (string, error) {
 				return "/dev/sda", nil
 			}
-			assert.Equal(t, tc.expected, HasMismatchingSymlink(tc.lvdl, tc.blockDevice))
+			assert.Equal(t, tc.expected, mustHasMismatchingSymlink(t, tc.lvdl, tc.blockDevice, tc.symlinkPath))
 		})
 	}
 }
 
+func mustHasMismatchingSymlink(t *testing.T, lvdl *v1.LocalVolumeDeviceLink, blockDevice internal.BlockDevice, symlinkPath string) bool {
+	t.Helper()
+	got, err := HasMismatchingSymlink(lvdl, blockDevice, symlinkPath)
+	assert.NoError(t, err)
+	return got
+}
+
 func TestRecreateSymlinkIfNeeded(t *testing.T) {
 	type recreateTestCase struct {
 		name              string

pkg/common/provisioner_utils.go

@@ -123,10 +123,22 @@ func SyncPVAndLVDL(ctx context.Context, args SyncPVAndLVDLArgs) error {
 		return fmt.Errorf("error finding localvolumedevicelink object %s: %w", pvName, err)
 	}
 
+	// Do not recreate symlinks or update LVDL/PV state for released volumes.
+	existingPV := &corev1.PersistentVolume{ObjectMeta: metav1.ObjectMeta{Name: pvName}}
+	err = client.Get(ctx, types.NamespacedName{Name: pvName}, existingPV)
+	if err == nil && existingPV.Status.Phase == corev1.VolumeReleased {
+		klog.InfoS("PV is still being cleaned, not going to recreate it", "pvName", pvName, "disk", deviceName)
+		// Caller should try again soon
+		return ErrTryAgain
+	}
+
 	// Symlink recreation must happen before PV creation because it fixes the
 	// symlink that the PV will reference. RecreateSymlinkIfNeeded already
 	// updates the LVDL status, so we skip ApplyStatus later.
-	requiresSymlinkRecreation := HasMismatchingSymlink(lvdl, args.BlockDevice)
+	requiresSymlinkRecreation, err := HasMismatchingSymlink(lvdl, args.BlockDevice, symLinkPath)
+	if err != nil {
+		return fmt.Errorf("error checking symlink mismatch: %w", err)
+	}
 	if requiresSymlinkRecreation {
 		if _, err := deviceHandler.RecreateSymlinkIfNeeded(ctx, lvdl, symLinkPath, args.BlockDevice); err != nil {
 			return fmt.Errorf("error recreating symlink: %w", err)
@@ -154,16 +166,6 @@ func SyncPVAndLVDL(ctx context.Context, args SyncPVAndLVDLArgs) error {
 		return fmt.Errorf("could not read the device's volume mode from the node: %w", err)
 	}
 
-	// Do not attempt to create or update existing PV's that have been released
-	existingPV := &corev1.PersistentVolume{ObjectMeta: metav1.ObjectMeta{Name: pvName}}
-	err = client.Get(ctx, types.NamespacedName{Name: pvName}, existingPV)
-
-	if err == nil && existingPV.Status.Phase == corev1.VolumeReleased {
-		klog.InfoS("PV is still being cleaned, not going to recreate it", "pvName", pvName, "disk", deviceName)
-		// Caller should try again soon
-		return ErrTryAgain
-	}
-
 	var capacityBytes int64
 	switch actualVolumeMode {
 	case corev1.PersistentVolumeBlock: