Fix VolumeAttachment garbage collection for migrated PVs

openshift · Jun 2, 2021 · e58f614 · e58f614
1 parent 27ffcba
commit e58f614
Show file tree

Hide file tree

Showing 3 changed files with 66 additions and 44 deletions.
diff --git a/pkg/controller/volume/attachdetach/BUILD b/pkg/controller/volume/attachdetach/BUILD
@@ -63,7 +63,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
@@ -710,33 +710,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. Error: %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
@@ -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"
 )
 
@@ -348,17 +347,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) {
@@ -395,15 +395,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) {
@@ -421,7 +432,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()
@@ -565,8 +576,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/<volName> should not be marked when CSI Migration is on