From a3f3695137fa42e43e36b459e79ad4fb0fe956c7 Mon Sep 17 00:00:00 2001 From: Tim Ebert Date: Thu, 20 May 2021 08:38:52 +0200 Subject: [PATCH] Fix VolumeAttachment garbage collection for migrated PVs --- pkg/controller/volume/attachdetach/BUILD | 1 - .../attachdetach/attach_detach_controller.go | 50 ++++++++++------ .../attach_detach_controller_test.go | 59 +++++++++++-------- 3 files changed, 66 insertions(+), 44 deletions(-) diff --git a/pkg/controller/volume/attachdetach/BUILD b/pkg/controller/volume/attachdetach/BUILD index 5b686d8d317ae..6276c755efdcb 100644 --- a/pkg/controller/volume/attachdetach/BUILD +++ b/pkg/controller/volume/attachdetach/BUILD @@ -64,7 +64,6 @@ go_test( "//pkg/features:go_default_library", "//pkg/volume:go_default_library", "//pkg/volume/csi:go_default_library", - "//pkg/volume/gcepd:go_default_library", "//pkg/volume/util:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", diff --git a/pkg/controller/volume/attachdetach/attach_detach_controller.go b/pkg/controller/volume/attachdetach/attach_detach_controller.go index 018a3d1111810..31e6a6b7c082a 100644 --- a/pkg/controller/volume/attachdetach/attach_detach_controller.go +++ b/pkg/controller/volume/attachdetach/attach_detach_controller.go @@ -716,33 +716,45 @@ func (adc *attachDetachController) processVolumeAttachments() error { klog.Errorf("Unable to lookup pv object for: %q, err: %v", *pvName, err) continue } + + var plugin volume.AttachableVolumePlugin volumeSpec := volume.NewSpecFromPersistentVolume(pv, false) - plugin, err := adc.volumePluginMgr.FindAttachablePluginBySpec(volumeSpec) - if err != nil || plugin == nil { - // Currently VA objects are created for CSI volumes only. nil plugin is unexpected, generate a warning - klog.Warningf( - "Skipping processing the volume %q on nodeName: %q, no attacher interface found. err=%v", - *pvName, - nodeName, - err) - continue + + // Consult csiMigratedPluginManager first before querying the plugins registered during runtime in volumePluginMgr. + // In-tree plugins that provisioned PVs will not be registered anymore after migration to CSI, once the respective + // feature gate is enabled. + if inTreePluginName, err := adc.csiMigratedPluginManager.GetInTreePluginNameFromSpec(pv, nil); err == nil { + if adc.csiMigratedPluginManager.IsMigrationEnabledForPlugin(inTreePluginName) { + // PV is migrated and should be handled by the CSI plugin instead of the in-tree one + plugin, _ = adc.volumePluginMgr.FindAttachablePluginByName(csi.CSIPluginName) + // podNamespace is not needed here for Azurefile as the volumeName generated will be the same with or without podNamespace + volumeSpec, err = csimigration.TranslateInTreeSpecToCSI(volumeSpec, "" /* podNamespace */, adc.intreeToCSITranslator) + if err != nil { + klog.Errorf( + "Failed to translate intree volumeSpec to CSI volumeSpec for volume:%q, va.Name:%q, nodeName:%q: %s. Error: %v", + *pvName, + va.Name, + nodeName, + inTreePluginName, + err) + continue + } + } } - pluginName := plugin.GetPluginName() - if adc.csiMigratedPluginManager.IsMigrationEnabledForPlugin(pluginName) { - plugin, _ = adc.volumePluginMgr.FindAttachablePluginByName(csi.CSIPluginName) - // podNamespace is not needed here for Azurefile as the volumeName generated will be the same with or without podNamespace - volumeSpec, err = csimigration.TranslateInTreeSpecToCSI(volumeSpec, "" /* podNamespace */, adc.intreeToCSITranslator) - if err != nil { - klog.Errorf( - "Failed to translate intree volumeSpec to CSI volumeSpec for volume:%q, va.Name:%q, nodeName:%q: %v", + + if plugin == nil { + plugin, err = adc.volumePluginMgr.FindAttachablePluginBySpec(volumeSpec) + if err != nil || plugin == nil { + // Currently VA objects are created for CSI volumes only. nil plugin is unexpected, generate a warning + klog.Warningf( + "Skipping processing the volume %q on nodeName: %q, no attacher interface found. err=%v", *pvName, - va.Name, nodeName, - pluginName, err) continue } } + volumeName, err := volumeutil.GetUniqueVolumeNameFromSpec(plugin, volumeSpec) if err != nil { klog.Errorf( diff --git a/pkg/controller/volume/attachdetach/attach_detach_controller_test.go b/pkg/controller/volume/attachdetach/attach_detach_controller_test.go index 6c88e6e5e6727..a9ceaa646b4f1 100644 --- a/pkg/controller/volume/attachdetach/attach_detach_controller_test.go +++ b/pkg/controller/volume/attachdetach/attach_detach_controller_test.go @@ -36,7 +36,6 @@ import ( "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/volume" "k8s.io/kubernetes/pkg/volume/csi" - "k8s.io/kubernetes/pkg/volume/gcepd" "k8s.io/kubernetes/pkg/volume/util" ) @@ -351,17 +350,18 @@ func attachDetachRecoveryTestCase(t *testing.T, extraPods1 []*v1.Pod, extraPods2 } type vaTest struct { - testName string - volName string - podName string - podNodeName string - pvName string - vaName string - vaNodeName string - vaAttachStatus bool - csiMigration bool - expected_attaches map[string][]string - expected_detaches map[string][]string + testName string + volName string + podName string + podNodeName string + pvName string + vaName string + vaNodeName string + vaAttachStatus bool + csiMigration bool + expected_attaches map[string][]string + expected_detaches map[string][]string + expectedASWAttachState cache.AttachState } func Test_ADC_VolumeAttachmentRecovery(t *testing.T) { @@ -398,15 +398,26 @@ func Test_ADC_VolumeAttachmentRecovery(t *testing.T) { expected_attaches: map[string][]string{}, expected_detaches: map[string][]string{"mynode-1": {"vol1"}}, }, - { - testName: "CSI Migration", - volName: "vol1", - podNodeName: "mynode-1", - pvName: "pv1", - vaName: "va1", - vaNodeName: "mynode-1", - vaAttachStatus: false, - csiMigration: true, + { // pod is scheduled, volume is migrated, attach status:false, verify volume is marked as attached + testName: "Scheduled Pod with migrated PV", + volName: "vol1", + podNodeName: "mynode-1", + pvName: "pv1", + vaName: "va1", + vaNodeName: "mynode-1", + vaAttachStatus: false, + csiMigration: true, + expectedASWAttachState: cache.AttachStateAttached, + }, + { // pod is deleted, volume is migrated, attach status:false, verify volume is marked as uncertain + testName: "Deleted Pod with migrated PV", + volName: "vol1", + pvName: "pv1", + vaName: "va1", + vaNodeName: "mynode-1", + vaAttachStatus: false, + csiMigration: true, + expectedASWAttachState: cache.AttachStateUncertain, }, } { t.Run(tc.testName, func(t *testing.T) { @@ -424,7 +435,7 @@ func volumeAttachmentRecoveryTestCase(t *testing.T, tc vaTest) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIMigrationGCE, tc.csiMigration)() defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIMigrationGCEComplete, tc.csiMigration)() - plugins = gcepd.ProbeVolumePlugins() + // if InTreePluginGCEUnregister is enabled, only the CSI plugin is registered but not the in-tree one plugins = append(plugins, csi.ProbeVolumePlugins()...) } else { plugins = controllervolumetesting.CreateTestPlugin() @@ -569,8 +580,8 @@ func verifyExpectedVolumeState(t *testing.T, adc *attachDetachController, tc vaT // Since csi migration is turned on, the attach state for the PV should be in CSI format. attachedState := adc.actualStateOfWorld.GetAttachState( v1.UniqueVolumeName(csiPDUniqueNamePrefix+tc.volName), types.NodeName(tc.vaNodeName)) - if attachedState != cache.AttachStateAttached { - t.Fatalf("Expected attachedState %v, but it is %v", cache.AttachStateAttached, attachedState) + if attachedState != tc.expectedASWAttachState { + t.Fatalf("Expected attachedState %v, but it is %v", tc.expectedASWAttachState, attachedState) } // kubernetes.io/gce-pd/ should not be marked when CSI Migration is on