wip: Add e2e test that checks PreferredLinkTarget is updated

HACKING.md

@@ -6,6 +6,11 @@
 
 2. Create images as documented below
 
+## Overriding diskmaker image or operator image directly after deployment
+
+1. Delete the subscription object so as OLM is no longer managing the operator.
+2. Edit the CSV object to override diskmaker image.
+
 ## Running the operator locally
 
 1. Install LSO via OLM/OperatorHub in GUI

api/v1/localvolumedevicelink_types.go

@@ -79,13 +79,12 @@ type LocalVolumeDeviceLinkStatus struct {
 	// preferredLinkTarget is the /dev/disk/by-id symlink for the device that the local storage
 	// operator evaluated as the most stable and the least error prone. The local storage operator
 	// recommends using this symlink, after a careful review by the cluster admin.
-	// +required
-	// +kubebuilder:validation:MinLength=1
+	// +optional
 	// +kubebuilder:validation:MaxLength=4096
 	PreferredLinkTarget string `json:"preferredLinkTarget,omitempty"`
 	// validLinkTargets is the list of /dev/disk/by-id symlinks for the device that the local
 	// storage operator considers as valid. The list may contain at most 256 entries.
-	// +required
+	// +optional
 	// +listType=set
 	// +kubebuilder:validation:MaxItems=256
 	ValidLinkTargets []string `json:"validLinkTargets,omitempty"`
@@ -113,6 +112,10 @@ type LocalVolumeDeviceLinkList struct {
 	Items []LocalVolumeDeviceLink `json:"items"`
 }
 
+func init() {
+	SchemeBuilder.Register(&LocalVolumeDeviceLink{}, &LocalVolumeDeviceLinkList{})
+}
+
 // DeviceLinkPolicy defines how symlinks for given volumes should be treated
 // by the LSO. Valid values are - None, CurrentLinkTarget and PreferredLinkTarget
 // +kubebuilder:validation:Enum=None;CurrentLinkTarget;PreferredLinkTarget

config/manifests/stable/local-storage-operator.clusterserviceversion.yaml

@@ -225,6 +225,8 @@ spec:
                 - localvolumediscoveries/status
                 - localvolumediscoveryresults
                 - localvolumediscoveryresults/status
+                - localvolumedevicelinks
+                - localvolumedevicelinks/status
               verbs:
                 - get
                 - list
@@ -313,6 +315,8 @@ spec:
                 - localvolumediscoveries/status
                 - localvolumediscoveryresults
                 - localvolumediscoveryresults/status
+                - localvolumedevicelinks
+                - localvolumedevicelinks/status
               verbs:
                 - get
                 - list

config/manifests/stable/local.storage.openshift.io_localvolumedevicelinks.yaml

@@ -131,7 +131,6 @@ spec:
                   operator evaluated as the most stable and the least error prone. The local storage operator
                   recommends using this symlink, after a careful review by the cluster admin.
                 maxLength: 4096
-                minLength: 1
                 type: string
               validLinkTargets:
                 description: |-
@@ -144,8 +143,6 @@ spec:
                 x-kubernetes-list-type: set
             required:
             - currentLinkTarget
-            - preferredLinkTarget
-            - validLinkTargets
             type: object
         required:
         - spec

pkg/common/provisioner_utils.go

@@ -8,6 +8,7 @@ import (
 	"path/filepath"
 
 	localv1 "github.com/openshift/local-storage-operator/api/v1"
+	"github.com/openshift/local-storage-operator/pkg/internal"
 	corev1 "k8s.io/api/core/v1"
 	storagev1 "k8s.io/api/storage/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -42,19 +43,35 @@ func GenerateMountMap(runtimeConfig *provCommon.RuntimeConfig) (sets.String, err
 	return mountPointMap, nil
 }
 
+// CreateLocalPVArgs holds the arguments for CreateLocalPV.
+type CreateLocalPVArgs struct {
+	LocalVolumeLikeObject runtime.Object
+	RuntimeConfig         *provCommon.RuntimeConfig
+	StorageClass          storagev1.StorageClass
+	MountPointMap         sets.String
+	Client                client.Client
+	SymLinkPath      string
+	IDExists         bool
+	ExtraLabelsForPV map[string]string
+
+	// CurrentSymlink points to source to which SymLinkPath points to.
+	CurrentSymlink string
+	// BlockDevice is the block device backing this PV.
+	BlockDevice internal.BlockDevice
+}
+
 // CreateLocalPV is used to create a local PV against a symlink
 // after passing the same validations against that symlink that local-static-provisioner uses
-func CreateLocalPV(
-	obj runtime.Object,
-	runtimeConfig *provCommon.RuntimeConfig,
-	storageClass storagev1.StorageClass,
-	mountPointMap sets.String,
-	client client.Client,
-	symLinkPath string,
-	deviceName string,
-	idExists bool,
-	extraLabelsForPV map[string]string,
-) error {
+func CreateLocalPV(ctx context.Context, args CreateLocalPVArgs) error {
+	obj := args.LocalVolumeLikeObject
+	runtimeConfig := args.RuntimeConfig
+	storageClass := args.StorageClass
+	mountPointMap := args.MountPointMap
+	client := args.Client
+	symLinkPath := args.SymLinkPath
+	deviceName := args.BlockDevice.KName
+	idExists := args.IDExists
+	extraLabelsForPV := args.ExtraLabelsForPV
 	nodeLabels := runtimeConfig.Node.GetLabels()
 	hostname, found := nodeLabels[corev1.LabelHostname]
 	if !found {
@@ -91,9 +108,28 @@ func CreateLocalPV(
 		return fmt.Errorf("could not read the device's volume mode from the node: %w", err)
 	}
 
+	accessor, err := meta.Accessor(obj)
+	if err != nil {
+		return fmt.Errorf("could not get object metadata accessor from obj: %+v", obj)
+	}
+	name := accessor.GetName()
+	namespace := accessor.GetNamespace()
+	kind := obj.GetObjectKind().GroupVersionKind().Kind
+	if len(name) == 0 || len(namespace) == 0 || len(kind) == 0 {
+		return fmt.Errorf("name: %q, namespace: %q, or  kind: %q is empty for obj: %+v", name, namespace, kind, obj)
+	}
+
+	deviceHandler := internal.NewDeviceLinkHandler(args.CurrentSymlink, client)
+
+	_, err = deviceHandler.Create(ctx, pvName, runtimeConfig.Namespace, obj)
+	if err != nil {
+		return fmt.Errorf("error creating localvolumedevicelink object: %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(context.TODO(), types.NamespacedName{Name: pvName}, existingPV)
+	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
@@ -128,19 +164,6 @@ func CreateLocalPV(
 		return fmt.Errorf("path %q has unexpected volume type %q", symLinkPath, actualVolumeMode)
 	}
 
-	accessor, err := meta.Accessor(obj)
-	if err != nil {
-
-		return fmt.Errorf("could not get object metadata accessor from obj: %+v", obj)
-	}
-
-	name := accessor.GetName()
-	namespace := accessor.GetNamespace()
-	kind := obj.GetObjectKind().GroupVersionKind().Kind
-	if len(name) == 0 || len(namespace) == 0 || len(kind) == 0 {
-		return fmt.Errorf("name: %q, namespace: %q, or  kind: %q is empty for obj: %+v", name, namespace, kind, obj)
-	}
-
 	labels := map[string]string{
 		corev1.LabelHostname:  hostname,
 		PVOwnerKindLabel:      kind,
@@ -229,8 +252,16 @@ func CreateLocalPV(
 	if opRes != controllerutil.OperationResultNone {
 		klog.InfoS("PV changed", "pvName", pvName, "status", opRes)
 	}
+	if err != nil {
+		return fmt.Errorf("error creating pv %s: %w", pvName, err)
+	}
+
+	_, err = deviceHandler.ApplyStatus(ctx, pvName, runtimeConfig.Namespace, args.BlockDevice, obj)
+	if err != nil {
+		return fmt.Errorf("error updating localvolumedevicelink object: %w", err)
+	}
 
-	return err
+	return nil
 }
 
 // GeneratePVName is used to generate a PV name based on the filename, node, and storageclass

pkg/common/symlink_utils.go

@@ -47,6 +47,26 @@ func GetSymLinkSourceAndTarget(dev internal.BlockDevice, symlinkDir, existingSym
 
 }
 
+// GetSymlinkedForCurrentSC returns the basename of the symlink in symlinkDir
+// that currently points to currentDevice. Empty string means no match found.
+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/controllers/localvolume/localvolume_controller.go

@@ -142,15 +142,13 @@ func (r *LocalVolumeReconciler) syncLocalVolumeProvider(ctx context.Context, ins
 		return r.addFailureCondition(instance, o, err)
 	}
 
-	if diskMakerDS != nil {
-		children = append(children, operatorv1.GenerationStatus{
-			Group:          appsv1.GroupName,
-			Resource:       "DaemonSet",
-			Namespace:      diskMakerDS.Namespace,
-			Name:           diskMakerDS.Name,
-			LastGeneration: diskMakerDS.Generation,
-		})
-	}
+	children = append(children, operatorv1.GenerationStatus{
+		Group:          appsv1.GroupName,
+		Resource:       "DaemonSet",
+		Namespace:      diskMakerDS.Namespace,
+		Name:           diskMakerDS.Name,
+		LastGeneration: diskMakerDS.Generation,
+	})
 
 	o.Status.Generations = children
 	o.Status.State = operatorv1.Managed