Repository: kubernetes-csi/external-attacher
Pull request #294 (2021-03-12 05:43:12)

Process PV finalizer for migrated PV when CSIMigration turned off
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
3. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**What type of PR is this?**
/kind bug


**What this PR does / why we need it**:
This PR removes PV external-attacher finalizer when CSIMigration is turned off. Without this change, the in-tree PV controller is not able to delete the PV object when there is no CSI driver in the cluster.

**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Partially Fixes # https://github.com/kubernetes-csi/external-provisioner/issues/576

**Special notes for your reviewer**:
This does not fix the issue if the corresponding CSI driver is uninstalled when CSI Migration is turned off. This fix is based on the assumption that CSI driver will still be around for at least a while to remove the finalizer. 

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note
Fix a bug where external-attacher finalizer not get lifted for migrated PV when CSIMigration is turned off.
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-csi/external-attacher/pull/294#" title="Author self-approved">Jiawei0227</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **saad-ali** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @saad-ali` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-csi%2Fexternal-attacher).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-csi/external-attacher/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["saad-ali"]} -->
Jiawei0227: /cc @msau42
/assign @jsafrane 
Jiawei0227: /cc @msau42 

Patch:
diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
index b511dce7..fca583f7 100644
--- a/pkg/controller/controller.go
+++ b/pkg/controller/controller.go
@@ -36,9 +36,14 @@ import (
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/client-go/util/workqueue"
+	csitrans "k8s.io/csi-translation-lib"
 	"k8s.io/klog/v2"
 )
 
+const (
+	annMigratedTo = "pv.kubernetes.io/migrated-to"
+)
+
 // CSIAttachController is a controller that attaches / detaches CSI volumes using provided Handler interface
 type CSIAttachController struct {
 	client        kubernetes.Interface
@@ -55,6 +60,7 @@ type CSIAttachController struct {
 
 	shouldReconcileVolumeAttachment bool
 	reconcileSync                   time.Duration
+	translator                      AttacherCSITranslator
 }
 
 // Handler is responsible for handling VolumeAttachment events from informer.
@@ -91,6 +97,7 @@ func NewCSIAttachController(client kubernetes.Interface, attacherName string, ha
 		pvQueue:                         workqueue.NewNamedRateLimitingQueue(paRateLimiter, "csi-attacher-pv"),
 		shouldReconcileVolumeAttachment: shouldReconcileVolumeAttachment,
 		reconcileSync:                   reconcileSync,
+		translator:                      csitrans.New(),
 	}
 
 	volumeAttachmentInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
@@ -217,7 +224,24 @@ func (ctrl *CSIAttachController) syncVA() {
 }
 
 func (ctrl *CSIAttachController) processFinalizers(pv *v1.PersistentVolume) bool {
-	return pv.DeletionTimestamp != nil && sets.NewString(pv.Finalizers...).Has(GetFinalizerName(ctrl.attacherName))
+	if sets.NewString(pv.Finalizers...).Has(GetFinalizerName(ctrl.attacherName)) {
+		if pv.DeletionTimestamp != nil {
+			return true
+		}
+
+		// if PV is provisioned by in-tree plugin and does not have migrated-to label
+		// this normally means this is a rollback scenario, we need to remove the finalizer as well
+		if ctrl.translator.IsPVMigratable(pv) {
+			if ann := pv.Annotations; ann != nil {
+				if migratedToDriver := ann[annMigratedTo]; migratedToDriver == ctrl.attacherName {
+					// migrated-to annonation detected, keep the finalizer
+					return false
+				}
+			}
+			return true
+		}
+	}
+	return false
 }
 
 // syncPV deals with one key off the queue.  It returns false when it's time to quit.
diff --git a/pkg/controller/csi_handler.go b/pkg/controller/csi_handler.go
index 2aa4d66e..6f808b0a 100644
--- a/pkg/controller/csi_handler.go
+++ b/pkg/controller/csi_handler.go
@@ -625,10 +625,29 @@ func (h *csiHandler) SyncNewOrUpdatedPersistentVolume(pv *v1.PersistentVolume) {
 	klog.V(4).Infof("CSIHandler: processing PV %q", pv.Name)
 	// Sync and remove finalizer on given PV
 	if pv.DeletionTimestamp == nil {
-		// Don't process anything that has no deletion timestamp.
-		klog.V(4).Infof("CSIHandler: processing PV %q: no deletion timestamp, ignoring", pv.Name)
-		h.pvQueue.Forget(pv.Name)
-		return
+		ignore := true
+
+		// if the PV is migrated this means CSIMigration is disabled so we need to remove the finalizer
+		// to give back the control of the PV to Kube-Controller-Manager
+		if h.translator.IsPVMigratable(pv) {
+			ignore = false
+			if ann := pv.Annotations; ann != nil {
+				if migratedToDriver := ann[annMigratedTo]; migratedToDriver == h.attacherName {
+					ignore = true
+				} else {
+					klog.V(4).Infof("CSIHandler: PV %q is an in-tree PV but does not have migrated-to annotation "+
+						"or the annotation does not match. Expect %v, Get %v. Remove the finalizer for this PV ",
+						pv.Name, h.attacherName, migratedToDriver)
+				}
+			}
+		}
+
+		if ignore {
+			// Don't process anything that has no deletion timestamp.
+			klog.V(4).Infof("CSIHandler: processing PV %q: no deletion timestamp, ignoring", pv.Name)
+			h.pvQueue.Forget(pv.Name)
+			return
+		}
 	}
 
 	// Check if the PV has finalizer


Test patch:
diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go
index 778665e0..10a8c592 100644
--- a/pkg/controller/controller_test.go
+++ b/pkg/controller/controller_test.go
@@ -19,8 +19,10 @@ package controller
 import (
 	"testing"
 
+	v1 "k8s.io/api/core/v1"
 	storage "k8s.io/api/storage/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	csitrans "k8s.io/csi-translation-lib"
 )
 
 func TestShouldEnqueueVAChange(t *testing.T) {
@@ -129,3 +131,96 @@ func TestShouldEnqueueVAChange(t *testing.T) {
 		})
 	}
 }
+
+func TestProcessFinalizers(t *testing.T) {
+	type testcase struct {
+		name           string
+		pv             *v1.PersistentVolume
+		expectedResult bool
+	}
+
+	c := &CSIAttachController{}
+	c.translator = csitrans.New()
+	c.attacherName = "pd.csi.storage.gke.io"
+	time := metav1.Now()
+
+	testcases := []testcase{
+		{
+			name:           "nothing interesting in the PV",
+			pv:             &v1.PersistentVolume{},
+			expectedResult: false,
+		},
+		{
+			name: "no deletion timestamp, has finalizer",
+			pv: &v1.PersistentVolume{
+				ObjectMeta: metav1.ObjectMeta{
+					Finalizers: []string{"external-attacher/pd-csi-storage-gke-io"},
+				},
+			},
+			expectedResult: false,
+		},
+		{
+			name: "Has deletion timestamp, has finalizer",
+			pv: &v1.PersistentVolume{
+				ObjectMeta: metav1.ObjectMeta{
+					DeletionTimestamp: &time,
+					Finalizers:        []string{"external-attacher/pd-csi-storage-gke-io"},
+				},
+			},
+			expectedResult: true,
+		},
+		{
+			name: "no deletion timestamp, has finalizer, migrated PV, no migrated-to annotation",
+			pv: &v1.PersistentVolume{
+				ObjectMeta: metav1.ObjectMeta{
+					Finalizers: []string{"external-attacher/pd-csi-storage-gke-io"},
+				},
+				Spec: v1.PersistentVolumeSpec{
+					PersistentVolumeSource: v1.PersistentVolumeSource{
+						GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{},
+					},
+				},
+			},
+			expectedResult: true,
+		},
+		{
+			name: "no deletion timestamp, has finalizer, migrated PV, no migrated-to annotation with random anno",
+			pv: &v1.PersistentVolume{
+				ObjectMeta: metav1.ObjectMeta{
+					Finalizers:  []string{"external-attacher/pd-csi-storage-gke-io"},
+					Annotations: map[string]string{"random": "random"},
+				},
+				Spec: v1.PersistentVolumeSpec{
+					PersistentVolumeSource: v1.PersistentVolumeSource{
+						GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{},
+					},
+				},
+			},
+			expectedResult: true,
+		},
+		{
+			name: "no deletion timestamp, has finalizer, migrated PV, has migrated-to annotation",
+			pv: &v1.PersistentVolume{
+				ObjectMeta: metav1.ObjectMeta{
+					Finalizers: []string{"external-attacher/pd-csi-storage-gke-io"},
+					Annotations: map[string]string{
+						"pv.kubernetes.io/migrated-to": "pd.csi.storage.gke.io",
+					},
+				},
+				Spec: v1.PersistentVolumeSpec{
+					PersistentVolumeSource: v1.PersistentVolumeSource{
+						GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{},
+					},
+				},
+			},
+			expectedResult: false,
+		},
+	}
+
+	for _, tc := range testcases {
+		result := c.processFinalizers(tc.pv)
+		if result != tc.expectedResult {
+			t.Errorf("Error executing test %v: expected result %v, got %v", tc.name, tc.expectedResult, result)
+		}
+	}
+}