From 5b7b2e2f6c1a08a44fbc95b41478dbbb26387e64 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Tue, 7 Dec 2021 11:50:15 -0500 Subject: [PATCH] When volume is not marked in-use, do not backoff --- .../volumemanager/reconciler/reconciler.go | 5 ++ .../reconciler/reconciler_test.go | 80 +++++++++++++++++++ .../operationexecutor/operation_generator.go | 9 ++- 3 files changed, 90 insertions(+), 4 deletions(-) diff --git a/pkg/kubelet/volumemanager/reconciler/reconciler.go b/pkg/kubelet/volumemanager/reconciler/reconciler.go index 32ce780267c..c61b4853f73 100644 --- a/pkg/kubelet/volumemanager/reconciler/reconciler.go +++ b/pkg/kubelet/volumemanager/reconciler/reconciler.go @@ -202,6 +202,11 @@ func (rc *reconciler) mountAttachVolumes() { volumeToMount.DevicePath = devicePath if cache.IsVolumeNotAttachedError(err) { if rc.controllerAttachDetachEnabled || !volumeToMount.PluginIsAttachable { + //// lets not spin a goroutine and unnecessarily trigger exponential backoff if this happens + if volumeToMount.PluginIsAttachable && !volumeToMount.ReportedInUse { + klog.V(5).InfoS(volumeToMount.GenerateMsgDetailed("operationExecutor.VerifyControllerAttachedVolume failed", " volume not marked in-use"), "pod", klog.KObj(volumeToMount.Pod)) + continue + } // Volume is not attached (or doesn't implement attacher), kubelet attach is disabled, wait // for controller to finish attaching volume. klog.V(5).InfoS(volumeToMount.GenerateMsgDetailed("Starting operationExecutor.VerifyControllerAttachedVolume", ""), "pod", klog.KObj(volumeToMount.Pod)) diff --git a/pkg/kubelet/volumemanager/reconciler/reconciler_test.go b/pkg/kubelet/volumemanager/reconciler/reconciler_test.go index 2c70e0d49a9..858df94808e 100644 --- a/pkg/kubelet/volumemanager/reconciler/reconciler_test.go +++ b/pkg/kubelet/volumemanager/reconciler/reconciler_test.go @@ -261,6 +261,86 @@ func Test_Run_Positive_VolumeMountControllerAttachEnabled(t *testing.T) { assert.NoError(t, volumetesting.VerifyZeroDetachCallCount(fakePlugin)) } +// Populates desiredStateOfWorld cache with one volume/pod. +// Enables controllerAttachDetachEnabled. +// volume is not repored-in-use +// Calls Run() +// Verifies that there is not wait-for-mount call +// Verifies that there is no exponential-backoff triggered +func Test_Run_Negative_VolumeMountControllerAttachEnabled(t *testing.T) { + // Arrange + volumePluginMgr, fakePlugin := volumetesting.GetTestKubeletVolumePluginMgr(t) + dsw := cache.NewDesiredStateOfWorld(volumePluginMgr) + asw := cache.NewActualStateOfWorld(nodeName, volumePluginMgr) + kubeClient := createTestClient() + fakeRecorder := &record.FakeRecorder{} + fakeHandler := volumetesting.NewBlockVolumePathHandler() + oex := operationexecutor.NewOperationExecutor(operationexecutor.NewOperationGenerator( + kubeClient, + volumePluginMgr, + fakeRecorder, + false, /* checkNodeCapabilitiesBeforeMount */ + fakeHandler)) + reconciler := NewReconciler( + kubeClient, + true, /* controllerAttachDetachEnabled */ + reconcilerLoopSleepDuration, + waitForAttachTimeout, + nodeName, + dsw, + asw, + hasAddedPods, + oex, + mount.NewFakeMounter(nil), + hostutil.NewFakeHostUtil(nil), + volumePluginMgr, + kubeletPodsDir) + pod := &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod1", + UID: "pod1uid", + }, + Spec: v1.PodSpec{ + Volumes: []v1.Volume{ + { + Name: "volume-name", + VolumeSource: v1.VolumeSource{ + GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{ + PDName: "fake-device1", + }, + }, + }, + }, + }, + } + + volumeSpec := &volume.Spec{Volume: &pod.Spec.Volumes[0]} + podName := util.GetUniquePodName(pod) + generatedVolumeName, err := dsw.AddPodToVolume( + podName, pod, volumeSpec, volumeSpec.Name(), "" /* volumeGidValue */) + + // Assert + if err != nil { + t.Fatalf("AddPodToVolume failed. Expected: Actual: <%v>", err) + } + + // Act + runReconciler(reconciler) + time.Sleep(reconcilerSyncWaitDuration) + + ok := oex.IsOperationSafeToRetry(generatedVolumeName, podName, nodeName, operationexecutor.VerifyControllerAttachedVolumeOpName) + if !ok { + t.Errorf("operation on volume %s is not safe to retry", generatedVolumeName) + } + + // Assert + assert.NoError(t, volumetesting.VerifyZeroAttachCalls(fakePlugin)) + assert.NoError(t, volumetesting.VerifyWaitForAttachCallCount( + 0 /* expectedWaitForAttachCallCount */, fakePlugin)) + assert.NoError(t, volumetesting.VerifyMountDeviceCallCount( + 0 /* expectedMountDeviceCallCount */, fakePlugin)) +} + // Populates desiredStateOfWorld cache with one volume/pod. // Calls Run() // Verifies there is one attach/mount/etc call and no detach calls. diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index d7cb5121a6c..b35aa655c59 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -48,9 +48,10 @@ import ( ) const ( - unknownVolumePlugin string = "UnknownVolumePlugin" - unknownAttachableVolumePlugin string = "UnknownAttachableVolumePlugin" - DetachOperationName string = "volume_detach" + unknownVolumePlugin string = "UnknownVolumePlugin" + unknownAttachableVolumePlugin string = "UnknownAttachableVolumePlugin" + DetachOperationName string = "volume_detach" + VerifyControllerAttachedVolumeOpName string = "verify_controller_attached_volume" ) // InTreeToCSITranslator contains methods required to check migratable status @@ -1579,7 +1580,7 @@ func (og *operationGenerator) GenerateVerifyControllerAttachedVolumeFunc( } return volumetypes.GeneratedOperations{ - OperationName: "verify_controller_attached_volume", + OperationName: VerifyControllerAttachedVolumeOpName, OperationFunc: verifyControllerAttachedVolumeFunc, CompleteFunc: util.OperationCompleteHook(util.GetFullQualifiedPluginNameForVolume(volumePlugin.GetPluginName(), volumeToMount.VolumeSpec), "verify_controller_attached_volume"), EventRecorderFunc: nil, // nil because we do not want to generate event on error