diff --git a/pkg/vgmanager/device_age.go b/pkg/vgmanager/device_age.go deleted file mode 100644 index e49c21c51..000000000 --- a/pkg/vgmanager/device_age.go +++ /dev/null @@ -1,76 +0,0 @@ -/* -Copyright © 2023 Red Hat, Inc. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package vgmanager - -import ( - "sync" - "time" -) - -var ( - // deviceMinAge is the minimum age for a device to be considered safe to claim - // otherwise, it could be a device that some other entity has attached and we have not claimed. - deviceMinAge = time.Second * 30 -) - -// timeInterface exists so as it can be patched for testing purpose -type timeInterface interface { - getCurrentTime() time.Time -} - -type wallTime struct{} - -func (t *wallTime) getCurrentTime() time.Time { - return time.Now() -} - -type ageMap struct { - ageMap map[string]time.Time - mux sync.RWMutex - clock timeInterface -} - -func newAgeMap(clock timeInterface) *ageMap { - return &ageMap{ - clock: clock, - ageMap: map[string]time.Time{}, - } -} - -// checks if older than, -// records current time if this is the first observation of key -func (a *ageMap) isOlderThan(key string) bool { - a.mux.RLock() - defer a.mux.RUnlock() - - firstObserved, found := a.ageMap[key] - if !found { - return false - } - return a.clock.getCurrentTime().Sub(firstObserved) > deviceMinAge -} - -func (a *ageMap) storeDeviceAge(key string) { - a.mux.Lock() - defer a.mux.Unlock() - - _, found := a.ageMap[key] - // set firstObserved if it doesn't exist - if !found { - a.ageMap[key] = a.clock.getCurrentTime() - } -} diff --git a/pkg/vgmanager/device_age_test.go b/pkg/vgmanager/device_age_test.go deleted file mode 100644 index a66d507e5..000000000 --- a/pkg/vgmanager/device_age_test.go +++ /dev/null @@ -1,60 +0,0 @@ -package vgmanager - -import ( - "testing" - "time" - - "github.com/stretchr/testify/assert" -) - -type fakeClock struct { - ftime time.Time -} - -func (f *fakeClock) getCurrentTime() time.Time { - return f.ftime -} - -func Test_isOlderThan(t *testing.T) { - start := time.Now() - a := &ageMap{ - clock: &fakeClock{ftime: start}, - ageMap: map[string]time.Time{ - "/dev/sdb": time.Now(), - "/dev/sdc": start.Add(-(time.Second + deviceMinAge)), - "/dev/sdd": start.Add(-(deviceMinAge / 2)), - }, - } - - testcases := []struct { - label string - device string - expected bool - }{ - {label: "device is old enough", device: "/dev/sdc", expected: true}, - {label: "device not old enough", device: "/dev/sdb", expected: false}, - {label: "device not old enough", device: "/dev/sdd", expected: false}, - {label: "device not found", device: "/dev/sde", expected: false}, - } - - for _, tc := range testcases { - - result := a.isOlderThan(tc.device) - assert.Equal(t, tc.expected, result) - } -} - -func Test_storeAgeMap(t *testing.T) { - myFakeClock := &fakeClock{ftime: time.Now()} - a := &ageMap{ - clock: myFakeClock, - ageMap: map[string]time.Time{ - "/dev/sdb": time.Now(), - }, - } - _, found := a.ageMap["/dev/nvme0"] - assert.False(t, found) - a.storeDeviceAge("/dev/nvme0") - _, found = a.ageMap["/dev/nvme0"] - assert.True(t, found) -} diff --git a/pkg/vgmanager/devices.go b/pkg/vgmanager/devices.go index 3a1bdfde8..b8a10413b 100644 --- a/pkg/vgmanager/devices.go +++ b/pkg/vgmanager/devices.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. diff --git a/pkg/vgmanager/devices_test.go b/pkg/vgmanager/devices_test.go index 9f1f8e5e3..461f72ce7 100644 --- a/pkg/vgmanager/devices_test.go +++ b/pkg/vgmanager/devices_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") }) } } diff --git a/pkg/vgmanager/vgmanager_controller.go b/pkg/vgmanager/vgmanager_controller.go index adfbc1dd2..315da52ee 100644 --- a/pkg/vgmanager/vgmanager_controller.go +++ b/pkg/vgmanager/vgmanager_controller.go @@ -46,7 +46,7 @@ import ( const ( ControllerName = "vg-manager" DefaultChunkSize = "128" - reconcileInterval = 1 * time.Minute + reconcileInterval = 15 * time.Second ) var ( @@ -55,7 +55,6 @@ 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) @@ -63,13 +62,11 @@ func (r *VGReconciler) SetupWithManager(mgr ctrl.Manager) error { 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 {