Fix race condition in PVC deletion

fixes [#7138][#7138]. In `coschedule:pipelineruns` mode, the `pvcs` created by `VolumeClaimTemplate` should be automatically deleted when the owning `pipelinerun` is completed.
To delete a `pvc` used by a pod (pipelinerun), the [kubernetes.io/pvc-protection][finalizer] finalizer must be removed.

Prior to this commit, the Tekton reconciler attempted to remove the `kubernetes.io/pvc-protection` finalizer first and then deletes the created pvcs. This results
in race condition since `PVCProtectionController` tries to add back the finalizer if the `pvc` is in `bounded` status . If the add back action happens before the PVC deletion call is completed,
the `PVC` will be left in `terminating` status.

This commit changes the reconciler to delete the `PVC` first and then removes the `finalizer` to fix the race condition.

This commit removes `TestPurgeFinalizerAndDeletePVCForWorkspace` UT, since the finalizer behavior cannot be mocked when a PVC is deleted.
This commit also adds integration test to cover this scenario.

/kind bug

[#7138]: #7138
[finalizer]: https://kubernetes.io/docs/concepts/storage/persistent-volumes/#storage-object-in-use-protection

pkg/reconciler/pipelinerun/affinity_assistant_test.go

@@ -781,7 +781,8 @@ func TestThatAffinityAssistantNameIsNoLongerThan53(t *testing.T) {
 		t.Errorf("affinity assistant name can not be longer than 53 chars")
 	}
 }
-func TestCleanupAffinityAssistants_Success(t *testing.T) {
+
+func TestCleanupAffinityAssistants_PerWorkspace_Success(t *testing.T) {
 	workspaces := []v1.WorkspaceBinding{
 		{
 			Name:                "volumeClaimTemplate workspace 1",
@@ -802,98 +803,63 @@ func TestCleanupAffinityAssistants_Success(t *testing.T) {
 		},
 	}
 
-	testCases := []struct {
-		name                   string
-		aaBehavior             aa.AffinityAssistantBehavior
-		cfgMap                 map[string]string
-		affinityAssistantNames []string
-		pvcNames               []string
-	}{{
-		name:       "Affinity Assistant Cleanup - per workspace",
-		aaBehavior: aa.AffinityAssistantPerWorkspace,
-		cfgMap: map[string]string{
-			"disable-affinity-assistant": "false",
-			"coschedule":                 "workspaces",
-		},
-		affinityAssistantNames: []string{"affinity-assistant-9d8b15fa2e", "affinity-assistant-39883fc3b2"},
-		pvcNames:               []string{"pvc-a12c589442", "pvc-5ce7cd98c5"},
-	}, {
-		name:       "Affinity Assistant Cleanup - per pipelinerun",
-		aaBehavior: aa.AffinityAssistantPerPipelineRun,
-		cfgMap: map[string]string{
-			"disable-affinity-assistant": "true",
-			"coschedule":                 "pipelineruns",
-		},
-		affinityAssistantNames: []string{"affinity-assistant-62843d388a"},
-		pvcNames:               []string{"pvc-a12c589442-affinity-assistant-62843d388a-0", "pvc-5ce7cd98c5-affinity-assistant-62843d388a-0"},
-	}}
-
-	for _, tc := range testCases {
-		// seed data to create StatefulSets and PVCs
-		var statefulsets []*appsv1.StatefulSet
-		for _, expectedAffinityAssistantName := range tc.affinityAssistantNames {
-			ss := &appsv1.StatefulSet{
-				TypeMeta: metav1.TypeMeta{
-					Kind:       "StatefulSet",
-					APIVersion: "apps/v1",
-				},
-				ObjectMeta: metav1.ObjectMeta{
-					Name:   expectedAffinityAssistantName,
-					Labels: getStatefulSetLabels(pr, expectedAffinityAssistantName),
-				},
-				Status: appsv1.StatefulSetStatus{
-					ReadyReplicas: 1,
-				},
-			}
-			statefulsets = append(statefulsets, ss)
-		}
-
-		var pvcs []*corev1.PersistentVolumeClaim
-		for _, expectedPVCName := range tc.pvcNames {
-			pvc := &corev1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: expectedPVCName,
-				},
-			}
-			pvcs = append(pvcs, pvc)
+	aaNames := []string{"affinity-assistant-9d8b15fa2e", "affinity-assistant-39883fc3b2"}
+	var statefulsets []*appsv1.StatefulSet
+	for _, expectedAffinityAssistantName := range aaNames {
+		ss := &appsv1.StatefulSet{
+			TypeMeta: metav1.TypeMeta{
+				Kind:       "StatefulSet",
+				APIVersion: "apps/v1",
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Name:   expectedAffinityAssistantName,
+				Labels: getStatefulSetLabels(pr, expectedAffinityAssistantName),
+			},
+			Status: appsv1.StatefulSetStatus{
+				ReadyReplicas: 1,
+			},
 		}
+		statefulsets = append(statefulsets, ss)
+	}
 
-		data := Data{
-			StatefulSets: statefulsets,
-			PVCs:         pvcs,
+	pvcNames := []string{"pvc-a12c589442", "pvc-5ce7cd98c5"}
+	var pvcs []*corev1.PersistentVolumeClaim
+	for _, expectedPVCName := range pvcNames {
+		pvc := &corev1.PersistentVolumeClaim{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: expectedPVCName,
+			},
 		}
+		pvcs = append(pvcs, pvc)
+	}
 
-		_, c, _ := seedTestData(data)
-		ctx := cfgtesting.SetFeatureFlags(context.Background(), t, tc.cfgMap)
+	data := Data{
+		StatefulSets: statefulsets,
+		PVCs:         pvcs,
+	}
+	_, c, _ := seedTestData(data)
+	ctx := context.Background()
 
-		// call clean up
-		err := c.cleanupAffinityAssistantsAndPVCs(ctx, pr)
-		if err != nil {
-			t.Fatalf("unexpected err when clean up Affinity Assistant: %v", err)
-		}
+	// call clean up
+	err := c.cleanupAffinityAssistantsAndPVCs(ctx, pr)
+	if err != nil {
+		t.Fatalf("unexpected err when clean up Affinity Assistant: %v", err)
+	}
 
-		// validate the cleanup result
-		for _, expectedAffinityAssistantName := range tc.affinityAssistantNames {
-			_, err = c.KubeClientSet.AppsV1().StatefulSets(pr.Namespace).Get(ctx, expectedAffinityAssistantName, metav1.GetOptions{})
-			if !apierrors.IsNotFound(err) {
-				t.Errorf("expected a NotFound response of StatefulSet, got: %v", err)
-			}
+	// validate the cleanup result
+	for _, expectedAffinityAssistantName := range aaNames {
+		_, err = c.KubeClientSet.AppsV1().StatefulSets(pr.Namespace).Get(ctx, expectedAffinityAssistantName, metav1.GetOptions{})
+		if !apierrors.IsNotFound(err) {
+			t.Errorf("expected a NotFound response of StatefulSet, got: %v", err)
 		}
+	}
 
-		// validate pvcs
-		for _, expectedPVCName := range tc.pvcNames {
-			if tc.aaBehavior == aa.AffinityAssistantPerWorkspace {
-				//  the PVCs are NOT expected to be deleted at Affinity Assistant cleanup time in AffinityAssistantPerWorkspace mode
-				_, err = c.KubeClientSet.CoreV1().PersistentVolumeClaims(pr.Namespace).Get(ctx, expectedPVCName, metav1.GetOptions{})
-				if err != nil {
-					t.Errorf("unexpected err when getting PersistentVolumeClaims, err: %v", err)
-				}
-			} else {
-				_, err = c.KubeClientSet.CoreV1().PersistentVolumeClaims(pr.Namespace).Get(ctx, expectedPVCName, metav1.GetOptions{})
-				if !apierrors.IsNotFound(err) {
-					t.Errorf("failed to clean up PersistentVolumeClaim")
-				}
-			}
+	// validate pvcs
+	for _, expectedPVCName := range pvcNames {
+		//  the PVCs are NOT expected to be deleted at Affinity Assistant cleanup time in AffinityAssistantPerWorkspace mode
+		_, err = c.KubeClientSet.CoreV1().PersistentVolumeClaims(pr.Namespace).Get(ctx, expectedPVCName, metav1.GetOptions{})
+		if err != nil {
+			t.Errorf("unexpected err when getting PersistentVolumeClaims, err: %v", err)
 		}
 	}
 }

pkg/reconciler/volumeclaim/pvchandler.go

@@ -85,7 +85,7 @@ func (c *defaultPVCHandler) CreatePVCFromVolumeClaimTemplate(ctx context.Context
 	return nil
 }
 
-// PurgeFinalizerAndDeletePVCForWorkspace purges the `kubernetes.io/pvc-protection` finalizer protection of the given pvc and then deletes it.
+// PurgeFinalizerAndDeletePVCForWorkspace deletes pvcs and then purges the `kubernetes.io/pvc-protection` finalizer protection.
 // Purging the `kubernetes.io/pvc-protection` finalizer allows the pvc to be deleted even when it is referenced by a taskrun pod.
 // See mode details in https://kubernetes.io/docs/concepts/storage/persistent-volumes/#storage-object-in-use-protection.
 func (c *defaultPVCHandler) PurgeFinalizerAndDeletePVCForWorkspace(ctx context.Context, pvcName, namespace string) error {
@@ -113,18 +113,18 @@ func (c *defaultPVCHandler) PurgeFinalizerAndDeletePVCForWorkspace(ctx context.C
 		return fmt.Errorf("failed to marshal jsonpatch: %w", err)
 	}
 
-	// patch the existing PVC to update the finalizers
-	_, err = c.clientset.CoreV1().PersistentVolumeClaims(namespace).Patch(ctx, pvcName, types.JSONPatchType, removeFinalizerBytes, metav1.PatchOptions{})
-	if err != nil {
-		return fmt.Errorf("failed to patch the PVC %s: %w", pvcName, err)
-	}
-
 	// delete PVC
 	err = c.clientset.CoreV1().PersistentVolumeClaims(namespace).Delete(ctx, pvcName, metav1.DeleteOptions{})
 	if err != nil {
 		return fmt.Errorf("failed to delete the PVC %s: %w", pvcName, err)
 	}
 
+	// remove finalizer
+	_, err = c.clientset.CoreV1().PersistentVolumeClaims(namespace).Patch(ctx, pvcName, types.JSONPatchType, removeFinalizerBytes, metav1.PatchOptions{})
+	if err != nil {
+		return fmt.Errorf("failed to patch the PVC %s: %w", pvcName, err)
+	}
+
 	return nil
 }
 

pkg/reconciler/volumeclaim/pvchandler_test.go

@@ -25,12 +25,12 @@ import (
 	"go.uber.org/zap"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	fakek8s "k8s.io/client-go/kubernetes/fake"
+	"k8s.io/client-go/kubernetes/typed/core/v1/fake"
 	client_go_testing "k8s.io/client-go/testing"
 )
 
@@ -263,15 +263,21 @@ func TestPurgeFinalizerAndDeletePVCForWorkspace(t *testing.T) {
 		t.Fatalf("unexpected error when creating PVC: %v", err)
 	}
 
+	// mocks `kubernetes.io/pvc-protection`` finalizer behavior by adding DeletionTimestamp when deleting pvcs with the finalizer
+	// see details in: https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers/#how-finalizers-work
+	pvc.DeletionTimestamp = &metav1.Time{
+		Time: metav1.Now().Time,
+	}
+	kubeClientSet.CoreV1().(*fake.FakeCoreV1).PrependReactor("delete", "persistentvolumeclaims",
+		func(action client_go_testing.Action) (handled bool, ret runtime.Object, err error) {
+			return true, pvc, nil
+		})
+
 	// call PurgeFinalizerAndDeletePVCForWorkspace to delete pvc
+	// note that the pvcs are not actually deleted in the unit test due to the mock limitation of fakek8s.NewSimpleClientset();
+	// full pvc lifecycle is tested in TestAffinityAssistant_PerPipelineRun integration test
 	pvcHandler := defaultPVCHandler{kubeClientSet, zap.NewExample().Sugar()}
 	if err := pvcHandler.PurgeFinalizerAndDeletePVCForWorkspace(ctx, pvcName, namespace); err != nil {
 		t.Fatalf("unexpected error when calling PurgeFinalizerAndDeletePVCForWorkspace: %v", err)
 	}
-
-	// validate pvc is deleted
-	_, err := kubeClientSet.CoreV1().PersistentVolumeClaims(namespace).Get(ctx, pvcName, metav1.GetOptions{})
-	if !apierrors.IsNotFound(err) {
-		t.Errorf("expected a NotFound response of PersistentVolumeClaims, got: %v", err)
-	}
 }

test/affinity_assistant_test.go

@@ -205,6 +205,12 @@ spec:
 		t.Errorf("Error waiting for PipelineRun to finish: %s", err)
 	}
 
+	// wait and check PVCs are deleted
+	t.Logf("Waiting for PVC in namespace %s to delete", namespace)
+	if err := WaitForPVCIsDeleted(ctx, c, timeout, prName, namespace, "PVCDeleted"); err != nil {
+		t.Errorf("Error waiting for PVC to be deleted: %s", err)
+	}
+
 	// validate PipelineRun pods sharing the same PVC are scheduled to the same node
 	podNames := []string{fmt.Sprintf("%v-foo-pod", prName), fmt.Sprintf("%v-bar-pod", prName), fmt.Sprintf("%v-double-ws-pod", prName), fmt.Sprintf("%v-no-ws-pod", prName)}
 	validatePodAffinity(t, ctx, podNames, namespace, c.KubeClient)

test/wait.go

@@ -144,6 +144,31 @@ func WaitForPodState(ctx context.Context, c *clients, name string, namespace str
 	})
 }
 
+// WaitForPVCIsDeleted polls the number of the PVC in the namespace from client every
+// interval until all the PVCs in the namespace are deleted. It returns an
+// error or timeout. desc will be used to name the metric that is emitted to
+// track how long it took to delete all the PVCs in the namespace.
+func WaitForPVCIsDeleted(ctx context.Context, c *clients, polltimeout time.Duration, name, namespace, desc string) error {
+	metricName := fmt.Sprintf("WaitForPVCIsDeleted/%s/%s", name, desc)
+	_, span := trace.StartSpan(context.Background(), metricName)
+	defer span.End()
+
+	ctx, cancel := context.WithTimeout(ctx, polltimeout)
+	defer cancel()
+
+	return pollImmediateWithContext(ctx, func() (bool, error) {
+		pvcList, err := c.KubeClient.CoreV1().PersistentVolumeClaims(namespace).List(ctx, metav1.ListOptions{})
+		if err != nil {
+			return true, err
+		}
+		if len(pvcList.Items) > 0 {
+			return false, nil
+		}
+
+		return true, nil
+	})
+}
+
 // WaitForPipelineRunState polls the status of the PipelineRun called name from client every
 // interval until inState returns `true` indicating it is done, returns an
 // error or timeout. desc will be used to name the metric that is emitted to