Merge pull request #380 from jakobmoellerdev/OCPVE-622-deviceMinAge

OCPVE-622: chore: remove deviceMinAge due to imperfect validation

Author: openshift-merge-robot

pkg/vgmanager/device_age.go

@@ -68,37 +68,28 @@ func (r *VGReconciler) addDevicesToVG(vgs []VolumeGroup, vgName string, devices
 }
 
 // getAvailableDevicesForVG determines the available devices that can be used to create a volume group.
-func (r *VGReconciler) getAvailableDevicesForVG(blockDevices []internal.BlockDevice, vgs []VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) ([]internal.BlockDevice, []internal.BlockDevice, error) {
+func (r *VGReconciler) getAvailableDevicesForVG(blockDevices []internal.BlockDevice, vgs []VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) ([]internal.BlockDevice, error) {
 	// filter devices based on DeviceSelector.Paths if specified
 	availableDevices, err := r.filterMatchingDevices(blockDevices, vgs, volumeGroup)
 	if err != nil {
 		r.Log.Error(err, "failed to filter matching devices", "VGName", volumeGroup.Name)
-		return nil, nil, err
+		return nil, err
 	}
 
-	// determine only available devices based on device age and filters in FilterMap
-	availableDevices, delayedDevices := r.filterAvailableDevices(availableDevices)
-
-	return availableDevices, delayedDevices, nil
+	return r.filterAvailableDevices(availableDevices), nil
 }
 
 // filterAvailableDevices returns:
 // availableDevices: the list of blockdevices considered available
-// delayedDevices: the list of blockdevices considered available, but first observed less than 'minDeviceAge' time ago
-func (r *VGReconciler) filterAvailableDevices(blockDevices []internal.BlockDevice) ([]internal.BlockDevice, []internal.BlockDevice) {
-	var availableDevices, delayedDevices []internal.BlockDevice
+func (r *VGReconciler) filterAvailableDevices(blockDevices []internal.BlockDevice) []internal.BlockDevice {
+	var availableDevices []internal.BlockDevice
 	// using a label so `continue DeviceLoop` can be used to skip devices
 DeviceLoop:
 	for _, blockDevice := range blockDevices {
-
-		// store device in deviceAgeMap
-		r.deviceAgeMap.storeDeviceAge(blockDevice.KName)
-
 		// check for partitions recursively
 		if blockDevice.HasChildren() {
-			childAvailableDevices, childDelayedDevices := r.filterAvailableDevices(blockDevice.Children)
+			childAvailableDevices := r.filterAvailableDevices(blockDevice.Children)
 			availableDevices = append(availableDevices, childAvailableDevices...)
-			delayedDevices = append(delayedDevices, childDelayedDevices...)
 		}
 
 		devLogger := r.Log.WithValues("Device.Name", blockDevice.Name)
@@ -113,15 +104,9 @@ DeviceLoop:
 				continue DeviceLoop
 			}
 		}
-		// check if the device is older than deviceMinAge
-		isOldEnough := r.deviceAgeMap.isOlderThan(blockDevice.KName)
-		if isOldEnough {
-			availableDevices = append(availableDevices, blockDevice)
-		} else {
-			delayedDevices = append(delayedDevices, blockDevice)
-		}
+		availableDevices = append(availableDevices, blockDevice)
 	}
-	return availableDevices, delayedDevices
+	return availableDevices
 }
 
 // filterMatchingDevices filters devices based on DeviceSelector.Paths if specified.

pkg/vgmanager/device_age_test.go

@@ -28,9 +28,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 		}
 	}
 
-	r := &VGReconciler{
-		deviceAgeMap: newAgeMap(&wallTime{}),
-	}
+	r := &VGReconciler{}
 
 	// remove noBindMounts filter as it reads `proc/1/mountinfo` file.
 	delete(FilterMap, "noBindMounts")
@@ -554,14 +552,13 @@ func TestAvailableDevicesForVG(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.description, func(t *testing.T) {
-			//use delayed devices as available devices in the test, as they are matching all the conditions but device age logic only considers them 30 seconds later
-			_, delayedDevices, err := r.getAvailableDevicesForVG(tc.existingBlockDevices, tc.existingVGs, &tc.volumeGroup)
+			availableDevices, err := r.getAvailableDevicesForVG(tc.existingBlockDevices, tc.existingVGs, &tc.volumeGroup)
 			if !tc.expectError {
 				assert.NoError(t, err)
 			} else {
 				assert.Error(t, err)
 			}
-			assert.Equal(t, tc.numOfAvailableDevices, len(delayedDevices), "expected numOfAvailableDevices is not equal to actual number")
+			assert.Equal(t, tc.numOfAvailableDevices, len(availableDevices), "expected numOfAvailableDevices is not equal to actual number")
 		})
 	}
 }

pkg/vgmanager/devices.go

@@ -46,7 +46,7 @@ import (
 const (
 	ControllerName    = "vg-manager"
 	DefaultChunkSize  = "128"
-	reconcileInterval = 1 * time.Minute
+	reconcileInterval = 15 * time.Second
 )
 
 var (
@@ -55,21 +55,18 @@ var (
 
 // SetupWithManager sets up the controller with the Manager.
 func (r *VGReconciler) SetupWithManager(mgr ctrl.Manager) error {
-	r.deviceAgeMap = newAgeMap(&wallTime{})
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&lvmv1alpha1.LVMVolumeGroup{}).
 		Complete(r)
 }
 
 type VGReconciler struct {
 	client.Client
-	Scheme *runtime.Scheme
-	Log    logr.Logger
-	// map from KNAME of device to time when the device was first observed since the process started
-	deviceAgeMap *ageMap
-	executor     internal.Executor
-	NodeName     string
-	Namespace    string
+	Scheme    *runtime.Scheme
+	Log       logr.Logger
+	executor  internal.Executor
+	NodeName  string
+	Namespace string
 }
 
 func (r *VGReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
@@ -143,35 +140,19 @@ func (r *VGReconciler) reconcile(ctx context.Context, volumeGroup *lvmv1alpha1.L
 	}
 
 	//Get the available block devices that can be used for this volume group
-	availableDevices, delayedDevices, err := r.getAvailableDevicesForVG(blockDevices, vgs, volumeGroup)
+	availableDevices, err := r.getAvailableDevicesForVG(blockDevices, vgs, volumeGroup)
 	if err != nil {
 		r.Log.Error(err, "failed to get block devices for volumegroup, will retry", "name", volumeGroup.Name)
-		// Set a failure status only if there is an error and there is no delayed devices. If there are delayed devices, there is a chance that this will pass in the next reconciliation.
-		if len(delayedDevices) == 0 {
-			if statuserr := r.setVolumeGroupFailedStatus(ctx, volumeGroup.Name, fmt.Sprintf("failed to get block devices for volumegroup %s: %v", volumeGroup.Name, err.Error())); statuserr != nil {
-				r.Log.Error(statuserr, "failed to update status", "name", volumeGroup.Name)
-			}
-		}
-
 		// Failed to get devices for this volume group. Reconcile again.
 		return reconcileAgain, err
 	}
 
-	r.Log.Info("listing available and delayed devices", "availableDevices", availableDevices, "delayedDevices", delayedDevices)
+	r.Log.Info("listing available and delayed devices", "availableDevices", availableDevices)
 
 	// If there are no available devices, that could mean either
 	// - There is no available devices to attach to the volume group
 	// - All the available devices are already attached
 	if len(availableDevices) == 0 {
-		if len(delayedDevices) > 0 {
-			r.Log.Info("there are delayed devices, will retry them in the next reconciliation", "VGName", volumeGroup.Name, "delayedDevices", delayedDevices)
-			if statuserr := r.setVolumeGroupProgressingStatus(ctx, volumeGroup.Name); statuserr != nil {
-				r.Log.Error(statuserr, "failed to update status", "VGName", volumeGroup.Name)
-				return reconcileAgain, statuserr
-			}
-			return ctrl.Result{Requeue: true, RequeueAfter: 30 * time.Second}, nil //30 seconds to make sure delayed devices become available
-		}
-
 		devicesExist := false
 		for _, vg := range vgs {
 			if volumeGroup.Name == vg.Name {
@@ -262,12 +243,7 @@ func (r *VGReconciler) reconcile(ctx context.Context, volumeGroup *lvmv1alpha1.L
 		return reconcileAgain, nil
 	}
 
-	// requeue faster if some devices are too recently observed to consume
-	requeueAfter := time.Minute * 1
-	if len(delayedDevices) > 0 {
-		requeueAfter = time.Second * 30
-	}
-	return ctrl.Result{RequeueAfter: requeueAfter}, nil
+	return reconcileAgain, nil
 }
 
 func (r *VGReconciler) processDelete(ctx context.Context, volumeGroup *lvmv1alpha1.LVMVolumeGroup) error {