OCPBUGS-78953: Fix bug with PV not getting provisioned after deletion

pkg/common/symlink_utils.go

@@ -47,6 +47,24 @@ func GetSymLinkSourceAndTarget(dev internal.BlockDevice, symlinkDir, existingSym
 
 }
 
+func GetSymlinkedForCurrentSC(symlinkDir string, currentDevice internal.BlockDevice) (string, error) {
+	paths, err := filepath.Glob(filepath.Join(symlinkDir, "*"))
+	if err != nil {
+		return "", err
+	}
+
+	for _, path := range paths {
+		isMatch, err := internal.PathEvalsToDiskLabel(path, currentDevice.KName)
+		if err != nil {
+			return "", err
+		}
+		if isMatch {
+			return filepath.Base(path), nil
+		}
+	}
+	return "", nil
+}
+
 func hasSymlinkFinalizer(pv *corev1.PersistentVolume) bool {
 	return controllerutil.ContainsFinalizer(pv, LSOSymlinkDeleterFinalizer)
 }

pkg/diskmaker/controllers/lv/reconcile.go

@@ -542,6 +542,7 @@ func (r *LocalVolumeReconciler) findMatchingDisks(diskConfig *DiskConfig, blockD
 	for storageClass, disks := range diskConfig.Disks {
 		devicePaths := disks.DevicePaths
 		forceWipe := disks.ForceWipeDevicesAndDestroyAllData
+		symLinkDirPath := path.Join(r.symlinkLocation, storageClass)
 		for _, devicePath := range devicePaths {
 			// handle user provided device_ids first
 			if strings.HasPrefix(devicePath, diskByIDPrefix) {
@@ -575,7 +576,15 @@ func (r *LocalVolumeReconciler) findMatchingDisks(diskConfig *DiskConfig, blockD
 				baseDeviceName := filepath.Base(diskDevPath)
 				blockDevice, matched := hasExactDisk(blockDevices, baseDeviceName)
 				if matched {
-					matchedDeviceID, err := blockDevice.GetPathByID("" /*existing symlinkpath */)
+					existingSymlink, err := common.GetSymlinkedForCurrentSC(symLinkDirPath, blockDevice)
+					if err != nil {
+						klog.ErrorS(err, "error reading existing symlinks for device",
+							"blockDevice", blockDevice.Name)
+						continue
+					}
+
+					// prefer existing symlink
+					matchedDeviceID, err := blockDevice.GetPathByID(existingSymlink)
 					// This means no /dev/disk/by-id entry was created for requested device.
 					if err != nil {
 						klog.ErrorS(err, "unable to find disk ID for local pool",

pkg/diskmaker/controllers/lvset/reconcile.go

@@ -173,7 +173,7 @@ func (r *LocalVolumeSetReconciler) Reconcile(ctx context.Context, request ctrl.R
 	var totalProvisionedPVs int
 	var noMatch []string
 	for _, blockDevice := range validDevices {
-		existingSymlink, err := getSymlinkedForCurrentSC(symLinkDir, blockDevice)
+		existingSymlink, err := common.GetSymlinkedForCurrentSC(symLinkDir, blockDevice)
 		if err != nil {
 			klog.ErrorS(err, "error reading existing symlinks for device",
 				"blockDevice", blockDevice.Name)
@@ -388,24 +388,6 @@ PathLoop:
 	return count, currentDeviceSymlinked, noMatch, nil
 }
 
-func getSymlinkedForCurrentSC(symlinkDir string, currentDevice internal.BlockDevice) (string, error) {
-	paths, err := filepath.Glob(filepath.Join(symlinkDir, "/*"))
-	if err != nil {
-		return "", err
-	}
-
-	for _, path := range paths {
-		isMatch, err := internal.PathEvalsToDiskLabel(path, currentDevice.KName)
-		if err != nil {
-			return "", err
-		}
-		if isMatch {
-			return filepath.Base(path), nil
-		}
-	}
-	return "", nil
-}
-
 func (r *LocalVolumeSetReconciler) provisionPV(
 	obj *localv1alpha1.LocalVolumeSet,
 	dev internal.BlockDevice,

test/e2e/cleanup_hostdirs.go

@@ -56,7 +56,7 @@ set +x
 `
 	nodeName, _ := node.Labels[corev1.LabelHostname]
 	return newNodeJob(
-		node,
+		nodeName,
 		namespace,
 		fmt.Sprintf("cleanup-%s", nodeName),
 		"cleans up the hostdir artificats on the node following functional tests",

test/e2e/hostudev.go

@@ -0,0 +1,100 @@
+package e2e
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/onsi/gomega"
+	framework "github.com/openshift/local-storage-operator/test/framework"
+	batchv1 "k8s.io/api/batch/v1"
+	corev1 "k8s.io/api/core/v1"
+)
+
+// addNewUdevSymlink adds a new udev symlink on the node. The link will point to the same device as the currentLink.
+func addNewUdevSymlink(t *testing.T, ctx *framework.TestCtx, nodeHostname string, currentLink, newLink string) {
+	t.Logf("adding new udev symlink %s -> %s on node %s", newLink, currentLink, nodeHostname)
+	matcher := gomega.NewWithT(t)
+
+	namespace, err := ctx.GetOperatorNamespace()
+	matcher.Expect(err).NotTo(gomega.HaveOccurred(), "could not determine namespace")
+
+	symlinkJob, err := newAddUdevSymlinkJob(nodeHostname, namespace, currentLink, newLink)
+	matcher.Expect(err).NotTo(gomega.HaveOccurred(), "could not create symlink job")
+	createOrReplaceJob(t, ctx, symlinkJob, fmt.Sprintf("creating symlink job on node: %q", nodeHostname))
+
+	waitForJobCompletion(t, symlinkJob, fmt.Sprintf("waiting for check-symlink job to complete: %q", symlinkJob.GetName()))
+	symlinkJob.TypeMeta.Kind = "Job"
+	eventuallyDelete(t, false, symlinkJob)
+	t.Logf("added udev symlink %s -> %s on node %s", newLink, currentLink, nodeHostname)
+}
+
+// removeUdevSymlink removes a udev symlink on the node. It literally calls `rm -f $linkPattern` (in the root directory),
+// so it can be used to remove multiple symlinks at once.
+func removeUdevSymlink(t *testing.T, ctx *framework.TestCtx, nodeHostname string, linkPattern string) {
+	t.Logf("removing udev symlinks matching %s on node %s", linkPattern, nodeHostname)
+	matcher := gomega.NewWithT(t)
+
+	namespace, err := ctx.GetOperatorNamespace()
+	matcher.Expect(err).NotTo(gomega.HaveOccurred(), "could not determine namespace")
+
+	symlinkJob, err := newRemoveUdevSymlinksJob(nodeHostname, namespace, linkPattern)
+	matcher.Expect(err).NotTo(gomega.HaveOccurred(), "could not create symlink job")
+	createOrReplaceJob(t, ctx, symlinkJob, fmt.Sprintf("creating symlink job on node: %q", nodeHostname))
+
+	waitForJobCompletion(t, symlinkJob, fmt.Sprintf("waiting for check-symlink job to complete: %q", symlinkJob.GetName()))
+	symlinkJob.TypeMeta.Kind = "Job"
+	eventuallyDelete(t, false, symlinkJob)
+	t.Logf("removed udev symlinks matching %s on node %s", linkPattern, nodeHostname)
+}
+
+func newAddUdevSymlinkJob(nodeHostname, namespace, currentUdevLink, newLink string) (*batchv1.Job, error) {
+	script := `
+set -eu
+set -x
+
+echo "adding new udev symlink $CURRENT_LINK -> $NEW_LINK"
+if [[ ! -L "$CURRENT_LINK" ]]; then
+	echo "not a symlink: $CURRENT_LINK"
+	exit 1
+fi
+
+DEVICE=$(readlink -f "$CURRENT_LINK")
+ln -sfv "$DEVICE" "$NEW_LINK"
+`
+	return newNodeJob(
+		nodeHostname,
+		namespace,
+		fmt.Sprintf("add-udev-symlink-job-%s", nodeHostname),
+		"adds a new udev symlink",
+		[]string{"/bin/bash", "-c", script},
+		&NodeJobOptions{
+			ContainerRestartPolicy: corev1.RestartPolicyNever,
+			Env: []corev1.EnvVar{
+				{Name: "CURRENT_LINK", Value: currentUdevLink},
+				{Name: "NEW_LINK", Value: newLink},
+			},
+		})
+
+}
+
+func newRemoveUdevSymlinksJob(nodeHostname, namespace, pattern string) (*batchv1.Job, error) {
+	script := `
+set -eu
+set -x
+
+ls -la "$PATTERN" || echo "No matching files found"
+rm -fv "$PATTERN"
+`
+	return newNodeJob(
+		nodeHostname,
+		namespace,
+		fmt.Sprintf("remove-udev-symlink-job-%s", nodeHostname),
+		"removes udev symlinks matching the pattern",
+		[]string{"/bin/bash", "-c", script},
+		&NodeJobOptions{
+			ContainerRestartPolicy: corev1.RestartPolicyNever,
+			Env: []corev1.EnvVar{
+				{Name: "PATTERN", Value: pattern},
+			},
+		})
+}

test/e2e/jobs.go

@@ -28,12 +28,7 @@ type NodeJobOptions struct {
 
 // newNodeJob returns a Job that runs the given command on the specified node.
 // The job uses the diskmaker image, mounts local-disks and /dev, and is pinned to the node via affinity.
-func newNodeJob(node corev1.Node, namespace, jobName, description string, command []string, opts *NodeJobOptions) (*batchv1.Job, error) {
-	nodeName, found := node.Labels[corev1.LabelHostname]
-	if !found {
-		return nil, fmt.Errorf("could not get %q label for node: %q", corev1.LabelHostname, node.GetName())
-	}
-
+func newNodeJob(nodeHostname, namespace, jobName, description string, command []string, opts *NodeJobOptions) (*batchv1.Job, error) {
 	hostContainerPropagation := corev1.MountPropagationHostToContainer
 	directoryHostPath := corev1.HostPathDirectory
 	volumes := []corev1.Volume{
@@ -95,7 +90,7 @@ func newNodeJob(node corev1.Node, namespace, jobName, description string, comman
 							{
 								Key:      corev1.LabelHostname,
 								Operator: corev1.NodeSelectorOpIn,
-								Values:   []string{nodeName},
+								Values:   []string{nodeHostname},
 							},
 						},
 					},
@@ -124,7 +119,7 @@ func newNodeJob(node corev1.Node, namespace, jobName, description string, comman
 			Name:      jobName,
 			Namespace: namespace,
 			Labels: map[string]string{
-				corev1.LabelHostname: nodeName,
+				corev1.LabelHostname: nodeHostname,
 				"app":                jobName,
 			},
 			Annotations: map[string]string{

test/e2e/localvolume_test.go

@@ -159,6 +159,20 @@ func LocalVolumeTest(ctx *framework.Context, cleanupFuncs *[]cleanupFn) func(*te
 		t.Logf("looking for %q annotation on pvs", provCommon.AnnProvisionedBy)
 		verifyProvisionerAnnotation(t, pvs, nodeList.Items)
 
+		t.Logf("Checking for recreation of PVs on symlink change")
+		// get first PV and change its symlink to verify if PV comes back when preferredSymlink changes
+		selectedPV := pvs[0]
+		nodeHostName := findNodeHostnameForPV(t, &selectedPV)
+		t.Logf("Using hostname %s", nodeHostName)
+		var currentSymlink string
+		if selectedDisk.id != "" {
+			currentSymlink = filepath.Join("/dev/disk/by-id", selectedDisk.id)
+		} else {
+			currentSymlink = filepath.Join("/dev", "name")
+		}
+		newPreferredTarget := "/dev/disk/by-id/scsi-1-local-storage-e2e-test"
+		addNewUdevSymlink(t, ctx, nodeHostName, currentSymlink, newPreferredTarget)
+
 		// verify deletion
 		for _, pv := range pvs {
 			eventuallyDelete(t, false, &pv)
@@ -573,3 +587,22 @@ func deleteResource(obj client.Object, namespace, name string, client framework.
 	})
 	return waitErr
 }
+
+// findNodeHostnameForLVDL returns the node hostname where the LVDL's PV is scheduled,
+// by inspecting the PV's spec.nodeAffinity (Required, LabelHostname).
+func findNodeHostnameForPV(t *testing.T, pv *v1.PersistentVolume) string {
+	t.Helper()
+	pvName := pv.Name
+	if pv.Spec.NodeAffinity == nil || pv.Spec.NodeAffinity.Required == nil {
+		t.Fatalf("PV %s has no NodeAffinity.Required", pvName)
+	}
+	for _, term := range pv.Spec.NodeAffinity.Required.NodeSelectorTerms {
+		for _, expr := range term.MatchExpressions {
+			if expr.Key == corev1.LabelHostname && len(expr.Values) > 0 {
+				return expr.Values[0]
+			}
+		}
+	}
+	t.Fatalf("PV %s NodeAffinity has no %q expression", pvName, corev1.LabelHostname)
+	return ""
+}

test/e2e/symlink_check.go

@@ -56,7 +56,7 @@ set +x
 	nodeName, _ := node.Labels[corev1.LabelHostname]
 	backoffLimit := int32(1)
 	return newNodeJob(
-		node,
+		nodeName,
 		namespace,
 		fmt.Sprintf("symlink-check-%s", nodeName),
 		"checks for leftover symlinks on the node following functional tests",