Merge pull request #397 from openshift-cherrypick-robot/cherry-pick-3…

…87-to-release-4.14 [release-4.14] OCPBUGS-18066: fix: don't fail if devices already in vg
openshift · Aug 24, 2023 · af195ed · af195ed
2 parents 00ae496 + 05e262e
commit af195ed
Show file tree

Hide file tree

Showing 2 changed files with 20 additions and 3 deletions.
diff --git a/pkg/vgmanager/devices.go b/pkg/vgmanager/devices.go
@@ -127,6 +127,7 @@ 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
+	devicesAlreadyInVG := false
 
 	if volumeGroup.Spec.DeviceSelector != nil {
 
@@ -146,6 +147,7 @@ func (r *VGReconciler) filterMatchingDevices(blockDevices []internal.BlockDevice
 				// Check if we should skip this device
 				if blockDevice.DevicePath == "" {
 					r.Log.Info(fmt.Sprintf("skipping required device that is already part of volume group %s: %s", volumeGroup.Name, path))
+					devicesAlreadyInVG = true
 					continue
 				}
 
@@ -160,13 +162,14 @@ func (r *VGReconciler) filterMatchingDevices(blockDevices []internal.BlockDevice
 
 				// Check if we should skip this device
 				if err != nil {
-					r.Log.Info(fmt.Sprintf("skipping optional device path due to error: %v", err))
+					r.Log.Info(fmt.Sprintf("skipping optional device path: %v", err))
 					continue
 				}
 
 				// Check if we should skip this device
 				if blockDevice.DevicePath == "" {
 					r.Log.Info(fmt.Sprintf("skipping optional device path that is already part of volume group %s: %s", volumeGroup.Name, path))
+					devicesAlreadyInVG = true
 					continue
 				}
 
@@ -176,8 +179,10 @@ func (r *VGReconciler) filterMatchingDevices(blockDevices []internal.BlockDevice
 			// At least 1 of the optional paths are required if:
 			//   - OptionalPaths was specified AND
 			//   - There were no required paths
+			//   - Devices were not already part of the volume group (meaning this was run after vg creation)
 			// This guarantees at least 1 device could be found between optionalPaths and paths
-			if len(filteredBlockDevices) == 0 {
+			//if len(filteredBlockDevices) == 0 && !devicesAlreadyInVG {
+			if len(filteredBlockDevices) == 0 && !devicesAlreadyInVG {
 				return nil, errors.New("at least 1 valid device is required if DeviceSelector paths or optionalPaths are specified")
 			}
 		}

diff --git a/pkg/vgmanager/devices_test.go b/pkg/vgmanager/devices_test.go
@@ -293,7 +293,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 			numOfAvailableDevices: 0,
 		},
 		{
-			description: "vg has device path that is already a part of the existing vg",
+			description: "vg has device paths that are already a part of the existing vg",
 			volumeGroup: v1alpha1.LVMVolumeGroup{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "vg1",
@@ -303,6 +303,9 @@ func TestAvailableDevicesForVG(t *testing.T) {
 						Paths: []string{
 							devicePaths["nvme1n1p1"],
 						},
+						OptionalPaths: []string{
+							devicePaths["nvme1n1p2"],
+						},
 					},
 				},
 			},
@@ -311,6 +314,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					Name: "vg1",
 					PVs: []string{
 						calculateDevicePath(t, "nvme1n1p1"),
+						calculateDevicePath(t, "nvme1n1p2"),
 					},
 				},
 			},
@@ -323,6 +327,14 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					ReadOnly: false,
 					State:    "live",
 				},
+				{
+					Name:     "nvme1n1p2",
+					KName:    calculateDevicePath(t, "nvme1n1p2"),
+					Type:     "disk",
+					Size:     "279.4G",
+					ReadOnly: false,
+					State:    "live",
+				},
 			},
 			numOfAvailableDevices: 0,
 			expectError:           false,