STOR-2873: Implement metrics for mismatch symlinks

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

assets/templates/localmetrics/prometheus-rule.yaml

@@ -37,3 +37,36 @@ spec:
         annotations:
           summary: "LocalVolumeSet has had a deletion timestamp older than 72 hours"
           description: "LocalVolumeSet {{ $labels.lvSetName }} has been marked for deletion for more than 72 hours."
+    - name: lso_no_stable_volume_path
+      rules:
+      - alert: LSONoStableLocalVolumePath
+        expr: min_over_time(lso_device_link_without_stable_path{policy="None"}[5m]) == 1
+        for: 10m
+        labels:
+          severity: warning
+        annotations:
+          summary: |
+            Local Storage operator has observed that pv {{ $labels.persistent_volume }} does not
+            have a stable device-path in /dev/disk/by-id/ that it can use and hence it may be using
+            device paths which may change on reboot. Such device paths should be used carefully.
+          message: "LSO has detected pv {{ $labels.persistent_volume }} without any valid stable path"
+          description: |
+            Local Storage operator has observed that pv {{ $labels.persistent_volume }} does not
+            have a stable device-path in /dev/disk/by-id/ that it can use and hence it may be using
+            device paths which may change on reboot. Such device paths should be used carefully.
+    - name: lso_device_link_mismatch
+      rules:
+      - alert: LSODeviceLinkMismatch
+        expr: min_over_time(lso_device_link_mismatch{policy="None"}[5m]) == 1
+        for: 10m
+        labels:
+          severity: warning
+        annotations:
+          summary: |
+            Local Storage operator has observed that pv {{ $labels.persistent_volume }} does not
+            have matching symlink it should be using vs symlink LSO prefers it to use for stability between reboots and OS upgrade.
+          message: "LSO has detected pv {{ $labels.persistent_volume }} with symlink mismatch"
+          description: |
+            Local Storage operator has observed that pv {{ $labels.persistent_volume }} is using a symlink
+            that is not optimum for stability between reboots and OS upgrades. Consider updating
+            LocalVolumeDeviceLink policy to preferredSymlink to allow LSO to manage the symlink.

cmd/diskmaker-manager/manager.go

@@ -122,9 +122,13 @@ func startManager(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
+	// Create the LVDL custom collector. mgr.GetClient() is already backed by
+	// the controller-runtime informer cache and does not hit the API server.
+	deviceLinkCollector := localmetrics.NewDeviceLinkCollector(mgr.GetClient(), namespace)
+
 	// start local server to emit custom metrics
 	err = localmetrics.NewConfigBuilder().
-		WithCollectors(localmetrics.LVMetricsList).
+		WithCollectors(append(localmetrics.LVMetricsList, deviceLinkCollector)).
 		Build()
 	if err != nil {
 		return errors.Wrap(err, "failed to configure local metrics")

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

go.mod

@@ -64,7 +64,7 @@ require (
 	github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect
 	github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
 	github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
-	github.com/prometheus/client_model v0.6.2 // indirect
+	github.com/prometheus/client_model v0.6.2
 	github.com/prometheus/common v0.67.2 // indirect
 	github.com/prometheus/procfs v0.19.2 // indirect
 	github.com/spf13/pflag v1.0.10 // indirect

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/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