OCPVE-386: feat: added the optionalPaths device selector field

api/v1alpha1/lvmcluster_types.go

@@ -109,6 +109,12 @@ type DeviceSelector struct {
 	// We discourage using the device names as they can change over node restarts.
 	// +optional
 	Paths []string `json:"paths,omitempty"`
+
+	// A list of device paths which could be chosen for creating Volume Group.
+	// For example "/dev/disk/by-path/pci-0000:04:00.0-nvme-1"
+	// We discourage using the device names as they can change over node restarts.
+	// +optional
+	OptionalPaths []string `json:"optionalPaths,omitempty"`
 }
 
 // type DeviceClassConfig struct {

api/v1alpha1/lvmcluster_webhook.go

@@ -112,7 +112,7 @@ func (l *LVMCluster) ValidateUpdate(old runtime.Object) error {
 
 	for _, deviceClass := range l.Spec.Storage.DeviceClasses {
 		var newThinPoolConfig, oldThinPoolConfig *ThinPoolConfig
-		var newDevices, oldDevices []string
+		var newDevices, newOptionalDevices, oldDevices, oldOptionalDevices []string
 
 		newThinPoolConfig = deviceClass.ThinPoolConfig
 		oldThinPoolConfig, err = oldLVMCluster.getThinPoolsConfigOfDeviceClass(deviceClass.Name)
@@ -134,41 +134,61 @@ func (l *LVMCluster) ValidateUpdate(old runtime.Object) error {
 
 		if deviceClass.DeviceSelector != nil {
 			newDevices = deviceClass.DeviceSelector.Paths
+			newOptionalDevices = deviceClass.DeviceSelector.OptionalPaths
 		}
 
-		oldDevices, err = oldLVMCluster.getPathsOfDeviceClass(deviceClass.Name)
+		oldDevices, oldOptionalDevices, err = oldLVMCluster.getPathsOfDeviceClass(deviceClass.Name)
 
-		// if devices are removed now
-		if len(oldDevices) > len(newDevices) {
-			return fmt.Errorf("Invalid:devices can not be removed from the LVMCluster once added.")
+		// Is this a new device class?
+		if err == ErrDeviceClassNotFound {
+			continue
 		}
 
-		// if devices are added now
-		if len(oldDevices) == 0 && len(newDevices) > 0 && err != ErrDeviceClassNotFound {
-			return fmt.Errorf("Invalid:devices can not be added in the LVMCluster once created without devices.")
+		// Make sure a device path list was not added
+		if len(oldDevices) == 0 && len(newDevices) > 0 {
+			return fmt.Errorf("invalid: device paths can not be added after a device class has been initialized")
 		}
 
-		deviceMap := make(map[string]bool)
-
-		for _, device := range oldDevices {
-			deviceMap[device] = true
+		// Make sure an optionalPaths list was not added
+		if len(oldOptionalDevices) == 0 && len(newOptionalDevices) > 0 {
+			return fmt.Errorf("invalid: optional device paths can not be added after a device class has been initialized")
 		}
 
-		for _, device := range newDevices {
-			delete(deviceMap, device)
+		// Validate all the old paths still exist
+		err := validateDevicePathsStillExist(oldDevices, newDevices)
+		if err != nil {
+			return fmt.Errorf("invalid: required device paths were deleted from the LVMCluster: %v", err)
 		}
 
-		// if any old device is removed now
-		if len(deviceMap) != 0 {
-			return fmt.Errorf("Invalid:some of devices are deleted from the LVMCluster. "+
-				"Device can not be removed from the LVMCluster once added. "+
-				"oldDevices:%s, newDevices:%s", oldDevices, newDevices)
+		// Validate all the old optional paths still exist
+		err = validateDevicePathsStillExist(oldOptionalDevices, newOptionalDevices)
+		if err != nil {
+			return fmt.Errorf("invalid: optional device paths were deleted from the LVMCluster: %v", err)
 		}
 	}
 
 	return nil
 }
 
+func validateDevicePathsStillExist(old, new []string) error {
+	deviceMap := make(map[string]bool)
+
+	for _, device := range old {
+		deviceMap[device] = true
+	}
+
+	for _, device := range new {
+		delete(deviceMap, device)
+	}
+
+	// if any old device is removed now
+	if len(deviceMap) != 0 {
+		return fmt.Errorf("devices can not be removed from the LVMCluster once added oldDevices:%s, newDevices:%s", old, new)
+	}
+
+	return nil
+}
+
 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type
 func (l *LVMCluster) ValidateDelete() error {
 	lvmclusterlog.Info("validate delete", "name", l.Name)
@@ -203,8 +223,8 @@ func (l *LVMCluster) verifyPathsAreNotEmpty() error {
 	var deviceClassesWithoutPaths []string
 	for _, deviceClass := range l.Spec.Storage.DeviceClasses {
 		if deviceClass.DeviceSelector != nil {
-			if len(deviceClass.DeviceSelector.Paths) == 0 {
-				return fmt.Errorf("path list should not be empty when DeviceSelector is specified")
+			if len(deviceClass.DeviceSelector.Paths) == 0 && len(deviceClass.DeviceSelector.OptionalPaths) == 0 {
+				return fmt.Errorf("either paths or optionalPaths must be specified when DeviceSelector is specified")
 			}
 		} else {
 			deviceClassesWithoutPaths = append(deviceClassesWithoutPaths, deviceClass.Name)
@@ -223,8 +243,13 @@ func (l *LVMCluster) verifyAbsolutePath() error {
 		if deviceClass.DeviceSelector != nil {
 			for _, path := range deviceClass.DeviceSelector.Paths {
 				if !strings.HasPrefix(path, "/dev/") {
-					return fmt.Errorf("Given path %s is not an absolute path. "+
-						"Please provide the absolute path to the device", path)
+					return fmt.Errorf("path %s must be an absolute path to the device", path)
+				}
+			}
+
+			for _, path := range deviceClass.DeviceSelector.OptionalPaths {
+				if !strings.HasPrefix(path, "/dev/") {
+					return fmt.Errorf("optional path %s must be an absolute path to the device", path)
 				}
 			}
 		}
@@ -254,13 +279,34 @@ func (l *LVMCluster) verifyNoDeviceOverlap() error {
 	for _, deviceClass := range l.Spec.Storage.DeviceClasses {
 		if deviceClass.DeviceSelector != nil {
 			nodeSelector := deviceClass.NodeSelector.String()
+
+			// Required paths
 			for _, path := range deviceClass.DeviceSelector.Paths {
 				if val, ok := devices[nodeSelector][path]; ok {
 					var err error
 					if val != deviceClass.Name {
-						err = fmt.Errorf("Error: device path %s overlaps in two different deviceClasss %s and %s", path, val, deviceClass.Name)
+						err = fmt.Errorf("error: device path %s overlaps in two different deviceClasss %s and %s", path, val, deviceClass.Name)
 					} else {
-						err = fmt.Errorf("Error: device path %s is specified at multiple places in deviceClass %s", path, val)
+						err = fmt.Errorf("error: device path %s is specified at multiple places in deviceClass %s", path, val)
+					}
+					return err
+				}
+
+				if devices[nodeSelector] == nil {
+					devices[nodeSelector] = make(map[string]string)
+				}
+
+				devices[nodeSelector][path] = deviceClass.Name
+			}
+
+			// Optional paths
+			for _, path := range deviceClass.DeviceSelector.OptionalPaths {
+				if val, ok := devices[nodeSelector][path]; ok {
+					var err error
+					if val != deviceClass.Name {
+						err = fmt.Errorf("error: optional device path %s overlaps in two different deviceClasss %s and %s", path, val, deviceClass.Name)
+					} else {
+						err = fmt.Errorf("error: optional device path %s is specified at multiple places in deviceClass %s", path, val)
 					}
 					return err
 				}
@@ -277,18 +323,21 @@ func (l *LVMCluster) verifyNoDeviceOverlap() error {
 	return nil
 }
 
-func (l *LVMCluster) getPathsOfDeviceClass(deviceClassName string) ([]string, error) {
-
+func (l *LVMCluster) getPathsOfDeviceClass(deviceClassName string) (required []string, optional []string, err error) {
+	required, optional, err = []string{}, []string{}, nil
 	for _, deviceClass := range l.Spec.Storage.DeviceClasses {
 		if deviceClass.Name == deviceClassName {
 			if deviceClass.DeviceSelector != nil {
-				return deviceClass.DeviceSelector.Paths, nil
+				required = deviceClass.DeviceSelector.Paths
+				optional = deviceClass.DeviceSelector.OptionalPaths
 			}
-			return []string{}, nil
+
+			return
 		}
 	}
 
-	return []string{}, ErrDeviceClassNotFound
+	err = ErrDeviceClassNotFound
+	return
 }
 
 func (l *LVMCluster) getThinPoolsConfigOfDeviceClass(deviceClassName string) (*ThinPoolConfig, error) {

bundle/manifests/lvm.topolvm.io_lvmclusters.yaml

@@ -56,6 +56,14 @@ spec:
                           description: DeviceSelector is a set of rules that should
                             match for a device to be included in the LVMCluster
                           properties:
+                            optionalPaths:
+                              description: A list of device paths which could be chosen
+                                for creating Volume Group. For example "/dev/disk/by-path/pci-0000:04:00.0-nvme-1"
+                                We discourage using the device names as they can change
+                                over node restarts.
+                              items:
+                                type: string
+                              type: array
                             paths:
                               description: A list of device paths which would be chosen
                                 for creating Volume Group. For example "/dev/disk/by-path/pci-0000:04:00.0-nvme-1"

bundle/manifests/lvm.topolvm.io_lvmvolumegroups.yaml

@@ -42,6 +42,14 @@ spec:
                 description: DeviceSelector is a set of rules that should match for
                   a device to be included in this TopoLVMCluster
                 properties:
+                  optionalPaths:
+                    description: A list of device paths which could be chosen for
+                      creating Volume Group. For example "/dev/disk/by-path/pci-0000:04:00.0-nvme-1"
+                      We discourage using the device names as they can change over
+                      node restarts.
+                    items:
+                      type: string
+                    type: array
                   paths:
                     description: A list of device paths which would be chosen for
                       creating Volume Group. For example "/dev/disk/by-path/pci-0000:04:00.0-nvme-1"

config/crd/bases/lvm.topolvm.io_lvmclusters.yaml

@@ -54,6 +54,14 @@ spec:
                           description: DeviceSelector is a set of rules that should
                             match for a device to be included in the LVMCluster
                           properties:
+                            optionalPaths:
+                              description: A list of device paths which could be chosen
+                                for creating Volume Group. For example "/dev/disk/by-path/pci-0000:04:00.0-nvme-1"
+                                We discourage using the device names as they can change
+                                over node restarts.
+                              items:
+                                type: string
+                              type: array
                             paths:
                               description: A list of device paths which would be chosen
                                 for creating Volume Group. For example "/dev/disk/by-path/pci-0000:04:00.0-nvme-1"

config/crd/bases/lvm.topolvm.io_lvmvolumegroups.yaml

@@ -44,6 +44,14 @@ spec:
                 description: DeviceSelector is a set of rules that should match for
                   a device to be included in this TopoLVMCluster
                 properties:
+                  optionalPaths:
+                    description: A list of device paths which could be chosen for
+                      creating Volume Group. For example "/dev/disk/by-path/pci-0000:04:00.0-nvme-1"
+                      We discourage using the device names as they can change over
+                      node restarts.
+                    items:
+                      type: string
+                    type: array
                   paths:
                     description: A list of device paths which would be chosen for
                       creating Volume Group. For example "/dev/disk/by-path/pci-0000:04:00.0-nvme-1"

pkg/vgmanager/devices.go

@@ -126,26 +126,50 @@ DeviceLoop:
 // filterMatchingDevices filters devices based on DeviceSelector.Paths if specified.
 func (r *VGReconciler) filterMatchingDevices(blockDevices []internal.BlockDevice, vgs []VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) ([]internal.BlockDevice, error) {
 	var filteredBlockDevices []internal.BlockDevice
-	if volumeGroup.Spec.DeviceSelector != nil && len(volumeGroup.Spec.DeviceSelector.Paths) > 0 {
-		for _, path := range volumeGroup.Spec.DeviceSelector.Paths {
-			diskName, err := filepath.EvalSymlinks(path)
-			if err != nil {
-				err = fmt.Errorf("unable to find symlink for disk path %s: %v", path, err)
-				return []internal.BlockDevice{}, err
-			}
+	if volumeGroup.Spec.DeviceSelector != nil {
+
+		if err := checkDuplicateDeviceSelectorPaths(volumeGroup.Spec.DeviceSelector); err != nil {
+			return nil, fmt.Errorf("unable to validate device selector paths: %v", err)
+		}
+
+		// If Paths is specified, treat it as required paths
+		if len(volumeGroup.Spec.DeviceSelector.Paths) > 0 {
+			for _, path := range volumeGroup.Spec.DeviceSelector.Paths {
+				blockDevice, err := getValidDevice(path, blockDevices, vgs, volumeGroup)
+				if err != nil {
+					// An error for required devices is critical
+					return nil, fmt.Errorf("unable to validate device %s: %v", path, err)
+				}
+
+				// Check if we should skip this device
+				if blockDevice.DevicePath == "" {
+					continue
+				}
 
-			isAlreadyExist := isDeviceAlreadyPartOfVG(vgs, diskName, volumeGroup)
-			if isAlreadyExist {
-				continue
+				filteredBlockDevices = append(filteredBlockDevices, blockDevice)
 			}
+		}
+
+		// Check for any optional paths
+		if len(volumeGroup.Spec.DeviceSelector.OptionalPaths) > 0 {
+			for _, path := range volumeGroup.Spec.DeviceSelector.OptionalPaths {
+				blockDevice, err := getValidDevice(path, blockDevices, vgs, volumeGroup)
 
-			blockDevice, ok := hasExactDisk(blockDevices, diskName)
-			if !ok {
-				return []internal.BlockDevice{}, fmt.Errorf("can not find device name %s in the available block devices", path)
+				// Check if we should skip this device
+				if err != nil || blockDevice.DevicePath == "" {
+					continue
+				}
+
+				filteredBlockDevices = append(filteredBlockDevices, blockDevice)
 			}
 
-			blockDevice.DevicePath = path
-			filteredBlockDevices = append(filteredBlockDevices, blockDevice)
+			// At least 1 of the optional paths are required if:
+			//   - OptionalPaths was specified AND
+			//   - There were no required paths
+			// This guarantees at least 1 device could be found
+			if len(filteredBlockDevices) == 0 {
+				return nil, fmt.Errorf("at least 1 valid optional device is required if DeviceSelector.OptionalPaths is specified")
+			}
 		}
 
 		return filteredBlockDevices, nil
@@ -182,3 +206,66 @@ func hasExactDisk(blockDevices []internal.BlockDevice, deviceName string) (inter
 	}
 	return internal.BlockDevice{}, false
 }
+
+func checkDuplicateDeviceSelectorPaths(selector *lvmv1alpha1.DeviceSelector) error {
+	uniquePaths := make(map[string]bool)
+	duplicatePaths := make(map[string]bool)
+
+	// Check for duplicate required paths
+	for _, path := range selector.Paths {
+		if _, exists := uniquePaths[path]; exists {
+			duplicatePaths[path] = true
+			continue
+		}
+
+		uniquePaths[path] = true
+	}
+
+	// Check for duplicate optional paths
+	for _, path := range selector.OptionalPaths {
+		if _, exists := uniquePaths[path]; exists {
+			duplicatePaths[path] = true
+			continue
+		}
+
+		uniquePaths[path] = true
+	}
+
+	// Report any duplicate paths
+	if len(duplicatePaths) > 0 {
+		keys := make([]string, 0, len(duplicatePaths))
+		for k := range duplicatePaths {
+			keys = append(keys, k)
+		}
+
+		return fmt.Errorf("duplicate device paths found: %v", keys)
+	}
+
+	return nil
+}
+
+// getValidDevice will do various checks on a device path to make sure it is a valid device
+//
+//	An error will be returned if the device is invalid
+//	No error and an empty BlockDevice object will be returned if this device should be skipped (ex: duplicate device)
+func getValidDevice(devicePath string, blockDevices []internal.BlockDevice, vgs []VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) (internal.BlockDevice, error) {
+	// Make sure the symlink exists
+	diskName, err := filepath.EvalSymlinks(devicePath)
+	if err != nil {
+		return internal.BlockDevice{}, fmt.Errorf("unable to find symlink for required disk path %s: %v", devicePath, err)
+	}
+
+	// Make sure this isn't a duplicate in the VG
+	if isDeviceAlreadyPartOfVG(vgs, diskName, volumeGroup) {
+		return internal.BlockDevice{}, nil // No error, we just don't want a duplicate
+	}
+
+	// Make sure the block device exists
+	blockDevice, ok := hasExactDisk(blockDevices, diskName)
+	if !ok {
+		return internal.BlockDevice{}, fmt.Errorf("can not find device name %s in the available block devices", devicePath)
+	}
+
+	blockDevice.DevicePath = devicePath
+	return blockDevice, nil
+}