Add e2e test that checks PreferredLinkTarget is updated

Update LocalVolume + LocalVolumeSets with a check for PreferredLinkTarget
update.

The check picks one PV / LVDL, on the corresponding node adds a new symlink
to the corresponding device that's more preferred than the current one, and
expects that the LVDL gets updated. Then it removes the symlink + checks
the old PreferredLinkTarget is restored.

The test expects the test disks are NVME. It creates a symlink `scsi-1`,
which is more preferable by LSO.

I also added some common functionality to add / remove symlinks easily
and updated newNodeJob to get hostname instead of the whole Node object -
it saves one API call to get a Node from hostname label from PV
nodeAffinity.

Author: jsafrane

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/gomega_util.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	"github.com/onsi/gomega"
+	localv1 "github.com/openshift/local-storage-operator/api/v1"
 	framework "github.com/openshift/local-storage-operator/test/framework"
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -278,3 +279,16 @@ func consumePV(t *testing.T, ctx *framework.Context, pv corev1.PersistentVolume)
 	matchingPod.TypeMeta.Kind = "Pod"
 	return pvc, job, &matchingPod
 }
+
+func waitForLVDLContent(t *testing.T, f *framework.Framework, namespace, lvdlName string, message string, check func(lvdl *localv1.LocalVolumeDeviceLink) error) {
+	matcher := gomega.NewWithT(t)
+	matcher.Eventually(func() error {
+		lvdl := &localv1.LocalVolumeDeviceLink{}
+		if err := f.Client.Get(goctx.TODO(), types.NamespacedName{Name: lvdlName, Namespace: namespace}, lvdl); err != nil {
+			t.Logf("error getting LVDL %q: %v", lvdlName, err)
+			return err
+		}
+		return check(lvdl)
+	}, 30*time.Minute, time.Second*2).ShouldNot(gomega.HaveOccurred(), message)
+	// TODO: lower the timeout above, using 30 minutes to have time for debugging
+}

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 := newRemoveUdevSymlinkJobs(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 newRemoveUdevSymlinkJobs(nodeHostname, namespace, pattern string) (*batchv1.Job, error) {
+	script := `
+set -eu
+set -x
+
+ls -la "$PATTERN"
+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

@@ -163,6 +163,7 @@ func LocalVolumeTest(ctx *framework.Context, cleanupFuncs *[]cleanupFn) func(*te
 		for _, pv := range pvs {
 			pvNames = append(pvNames, pv.Name)
 		}
+
 		t.Log("verifying LocalVolumeDeviceLink objects were created for PVs")
 		lvdls := eventuallyFindLVDLsForPVs(t, f, namespace, pvNames)
 		lvdlNames := make([]string, 0, len(lvdls))
@@ -173,6 +174,33 @@ func LocalVolumeTest(ctx *framework.Context, cleanupFuncs *[]cleanupFn) func(*te
 			matcher.Expect(lvdl.Status.ValidLinkTargets).ToNot(gomega.BeEmpty(), "expected ValidLinkTargets for LVDL %q", lvdl.Name)
 		}
 
+		// Use just the first LVDL for the test. Figure its node by inspecting the corresponding PV spec.nodeAffinity.
+		lvdl := lvdls[0]
+		nodeHostname := findNodeHostnameForLVDL(t, f, lvdl)
+		t.Logf("using node %q for the test of LVDL %s", nodeHostname, lvdl.Name)
+		// On AWS, the preferred link is usually "/dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_*"
+		// scsi-1 has a higher priority in dikutil.go and should become the new preferred target
+		newPreferredTarget := "/dev/disk/by-id/scsi-1-local-storage-e2e-test"
+		oldPreferredTarget := lvdl.Status.PreferredLinkTarget
+		addNewUdevSymlink(t, ctx, nodeHostname, lvdl.Status.PreferredLinkTarget, newPreferredTarget)
+		waitForLVDLContent(t, f, namespace, lvdl.Name, "waiting for LVDL to get new preferredLinkTarget", func(lvdl *localv1.LocalVolumeDeviceLink) error {
+			if lvdl.Status.PreferredLinkTarget != newPreferredTarget {
+				return fmt.Errorf("expected PreferredLinkTarget for LVDL %q to be updated, got %q", lvdl.Name, lvdl.Status.PreferredLinkTarget)
+			}
+			t.Logf("PreferredLinkTarget for LVDL %q is updated, got %q", lvdl.Name, lvdl.Status.PreferredLinkTarget)
+			return nil
+		})
+
+		removeUdevSymlink(t, ctx, nodeHostname, "/dev/disk/by-id/scsi-1-local-storage-e2e-test")
+
+		waitForLVDLContent(t, f, namespace, lvdl.Name, "waiting for LVDL to restore old preferredLinkTarget", func(lvdl *localv1.LocalVolumeDeviceLink) error {
+			if lvdl.Status.PreferredLinkTarget != oldPreferredTarget {
+				return fmt.Errorf("expected PreferredLinkTarget for LVDL %q to be restored, got %q", lvdl.Name, lvdl.Status.PreferredLinkTarget)
+			}
+			t.Logf("PreferredLinkTarget for LVDL %q is restored, got %q", lvdl.Name, lvdl.Status.PreferredLinkTarget)
+			return nil
+		})
+
 		// verify deletion
 		for _, pv := range pvs {
 			eventuallyDelete(t, false, &pv)
@@ -258,6 +286,33 @@ func LocalVolumeTest(ctx *framework.Context, cleanupFuncs *[]cleanupFn) func(*te
 
 }
 
+// findNodeHostnameForLVDL returns the node hostname where the LVDL's PV is scheduled,
+// by inspecting the PV's spec.nodeAffinity (Required, LabelHostname).
+func findNodeHostnameForLVDL(t *testing.T, f *framework.Framework, lvdl localv1.LocalVolumeDeviceLink) string {
+	t.Helper()
+	pvName := lvdl.Spec.PersistentVolumeName
+	if pvName == "" {
+		t.Fatalf("LVDL %q has no PersistentVolumeName", lvdl.Name)
+	}
+	pv := &corev1.PersistentVolume{}
+	err := f.Client.Get(goctx.TODO(), types.NamespacedName{Name: pvName}, pv)
+	if err != nil {
+		t.Fatalf("expected to find PV %s for LVDL %q: %v", pvName, lvdl.Name, err)
+	}
+	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 ""
+}
+
 func eventuallyFindLVDLsForPVs(t *testing.T, f *framework.Framework, namespace string, pvNames []string) []localv1.LocalVolumeDeviceLink {
 	matcher := gomega.NewWithT(t)
 	pvNameSet := sets.New(pvNames...)

test/e2e/localvolumeset_test.go

@@ -371,6 +371,33 @@ func LocalVolumeSetTest(ctx *framework.TestCtx, cleanupFuncs *[]cleanupFn) func(
 			matcher.Expect(lvdl.Status.ValidLinkTargets).ToNot(gomega.BeEmpty(), "expected ValidLinkTargets for LVDL %q", lvdl.Name)
 		}
 
+		// Use just the first LVDL for the test. Figure its node by inspecting the corresponding PV spec.nodeAffinity.
+		lvdl := fsLVDLs[0]
+		nodeHostname := findNodeHostnameForLVDL(t, f, lvdl)
+		t.Logf("using node %q for the test of LVDL %s", nodeHostname, lvdl.Name)
+		// On AWS, the preferred link is usually "/dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_*"
+		// scsi-1 has a higher priority in dikutil.go and should become the new preferred target
+		newPreferredTarget := "/dev/disk/by-id/scsi-1-local-storage-e2e-test"
+		oldPreferredTarget := lvdl.Status.PreferredLinkTarget
+		addNewUdevSymlink(t, ctx, nodeHostname, lvdl.Status.PreferredLinkTarget, newPreferredTarget)
+		waitForLVDLContent(t, f, namespace, lvdl.Name, "waiting for LVDL to get new preferredLinkTarget", func(lvdl *localv1.LocalVolumeDeviceLink) error {
+			if lvdl.Status.PreferredLinkTarget != newPreferredTarget {
+				return fmt.Errorf("expected PreferredLinkTarget for LVDL %q to be updated, got %q", lvdl.Name, lvdl.Status.PreferredLinkTarget)
+			}
+			t.Logf("PreferredLinkTarget for LVDL %q is updated, got %q", lvdl.Name, lvdl.Status.PreferredLinkTarget)
+			return nil
+		})
+
+		removeUdevSymlink(t, ctx, nodeHostname, "/dev/disk/by-id/scsi-1-local-storage-e2e-test")
+
+		waitForLVDLContent(t, f, namespace, lvdl.Name, "waiting for LVDL to restore old preferredLinkTarget", func(lvdl *localv1.LocalVolumeDeviceLink) error {
+			if lvdl.Status.PreferredLinkTarget != oldPreferredTarget {
+				return fmt.Errorf("expected PreferredLinkTarget for LVDL %q to be restored, got %q", lvdl.Name, lvdl.Status.PreferredLinkTarget)
+			}
+			t.Logf("PreferredLinkTarget for LVDL %q is restored, got %q", lvdl.Name, lvdl.Status.PreferredLinkTarget)
+			return nil
+		})
+
 		// verify deletion
 		t.Log("verify that filesystem PVs come back after deletion")
 		for _, pv := range fsPVs {

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",