[TEP-0135] Purge finalizer and delete PVC

Part of [#6740][#6740], developed based on Priti's [prototype][prototype] and partially completes the PVC deletion behavior [discussion][discussion].

Prior to this commit, the `PVCs` created from `PipelineRun's` `VolumeClaimTemplate` are not auto deleted when the owning `PipelineRun` is completed.
This commit updates the `cleanupAffinityAssistantsAndPVCs` function to remove the `kubernetes.io/pvc-protection` finalizer protection (so that the pvc is allowed to be deleted while the pod consuming it is not deleted).
The function then explicitly delete such `PVC` when cleaning up the `Affinity Assistants` at pr completion time.

This change is NOT applied to `coschedule: workspaces` mode as there is backward compatability concern. See more details in this [discussion][discussion]

[#6740]: #6740
[prototype]: #6635
[discussion]: #6741 (comment)

Author: QuanZhang-William

pkg/reconciler/pipelinerun/affinity_assistant.go

@@ -178,22 +178,44 @@ func (c *Reconciler) createOrUpdateAffinityAssistant(ctx context.Context, affini
 	return errs
 }
 
-// TODO(#6740)(WIP) implement cleanupAffinityAssistants for AffinityAssistantPerPipelineRun and AffinityAssistantPerPipelineRunWithIsolation affinity assistant modes
-func (c *Reconciler) cleanupAffinityAssistants(ctx context.Context, pr *v1.PipelineRun) error {
-	// omit cleanup if the feature is disabled
-	if c.isAffinityAssistantDisabled(ctx) {
-		return nil
+// cleanupAffinityAssistantsAndPVCs deletes Affinity Assistant StatefulSets and PVCs created from VolumeClaimTemplates
+func (c *Reconciler) cleanupAffinityAssistantsAndPVCs(ctx context.Context, pr *v1.PipelineRun) error {
+	aaBehavior, err := aa.GetAffinityAssistantBehavior(ctx)
+	if err != nil {
+		return err
 	}
 
 	var errs []error
-	for _, w := range pr.Spec.Workspaces {
-		if w.PersistentVolumeClaim != nil || w.VolumeClaimTemplate != nil {
-			affinityAssistantStsName := GetAffinityAssistantName(w.Name, pr.Name)
-			if err := c.KubeClientSet.AppsV1().StatefulSets(pr.Namespace).Delete(ctx, affinityAssistantStsName, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
-				errs = append(errs, fmt.Errorf("failed to delete StatefulSet %s: %w", affinityAssistantStsName, err))
+	switch aaBehavior {
+	case aa.AffinityAssistantPerWorkspace:
+		// TODO (#5776): support optional PVC deletion behavior for per-workspace mode
+		for _, w := range pr.Spec.Workspaces {
+			if w.PersistentVolumeClaim != nil || w.VolumeClaimTemplate != nil {
+				affinityAssistantName := GetAffinityAssistantName(w.Name, pr.Name)
+				if err := c.KubeClientSet.AppsV1().StatefulSets(pr.Namespace).Delete(ctx, affinityAssistantName, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
+					errs = append(errs, fmt.Errorf("failed to delete StatefulSet %s: %w", affinityAssistantName, err))
+				}
+			}
+		}
+	case aa.AffinityAssistantPerPipelineRun, aa.AffinityAssistantPerPipelineRunWithIsolation:
+		affinityAssistantName := GetAffinityAssistantName("", pr.Name)
+		if err := c.KubeClientSet.AppsV1().StatefulSets(pr.Namespace).Delete(ctx, affinityAssistantName, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
+			errs = append(errs, fmt.Errorf("failed to delete StatefulSet %s: %w", affinityAssistantName, err))
+		}
+
+		// cleanup PVCs created by Affinity Assistants
+		for _, w := range pr.Spec.Workspaces {
+			if w.VolumeClaimTemplate != nil {
+				pvcName := getPersistentVolumeClaimNameWithAffinityAssistant("", pr.Name, w, *kmeta.NewControllerRef(pr))
+				if err := c.pvcHandler.PurgeFinalizerAndDeletePVCForWorkspace(ctx, pvcName, pr.Namespace); err != nil {
+					errs = append(errs, err)
+				}
 			}
 		}
+	case aa.AffinityAssistantDisabled:
+		return nil
 	}
+
 	return errorutils.NewAggregate(errs)
 }
 

pkg/reconciler/pipelinerun/affinity_assistant_test.go

@@ -24,6 +24,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/tektoncd/pipeline/pkg/apis/config"
+	cfgtesting "github.com/tektoncd/pipeline/pkg/apis/config/testing"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/pod"
 	v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
@@ -42,7 +43,6 @@ import (
 	fakek8s "k8s.io/client-go/kubernetes/fake"
 	"k8s.io/client-go/kubernetes/typed/core/v1/fake"
 	testing2 "k8s.io/client-go/testing"
-	"knative.dev/pkg/kmeta"
 	logtesting "knative.dev/pkg/logging/testing"
 	"knative.dev/pkg/system"
 	_ "knative.dev/pkg/system/testing" // Setup system.Namespace()
@@ -178,7 +178,7 @@ func TestCreateAndDeleteOfAffinityAssistantPerPipelineRun(t *testing.T) {
 			expectAAName := GetAffinityAssistantName("", tc.pr.Name)
 			validateStatefulSetSpec(t, ctx, c, expectAAName, tc.expectStatefulSetSpec)
 
-			// TODO(#6740)(WIP): test cleanupAffinityAssistants for coscheduling-pipelinerun mode when fully implemented
+			// TODO(#6740)(WIP): test cleanupAffinityAssistantsAndPVCs for coscheduling-pipelinerun mode when fully implemented
 		})
 	}
 }
@@ -295,9 +295,9 @@ func TestCreateAndDeleteOfAffinityAssistantPerWorkspaceOrDisabled(t *testing.T)
 			}
 
 			// clean up Affinity Assistant
-			c.cleanupAffinityAssistants(ctx, tc.pr)
+			c.cleanupAffinityAssistantsAndPVCs(ctx, tc.pr)
 			if err != nil {
-				t.Errorf("unexpected error from cleanupAffinityAssistants: %v", err)
+				t.Errorf("unexpected error from cleanupAffinityAssistantsAndPVCs: %v", err)
 			}
 			for _, w := range tc.pr.Spec.Workspaces {
 				if w.PersistentVolumeClaim == nil && w.VolumeClaimTemplate == nil {
@@ -630,69 +630,124 @@ func TestThatAffinityAssistantNameIsNoLongerThan53(t *testing.T) {
 		t.Errorf("affinity assistant name can not be longer than 53 chars")
 	}
 }
-
 func TestCleanupAffinityAssistants_Success(t *testing.T) {
-	workspace := v1.WorkspaceBinding{
-		Name:                "volumeClaimTemplate workspace",
-		VolumeClaimTemplate: &corev1.PersistentVolumeClaim{},
+	workspaces := []v1.WorkspaceBinding{
+		{
+			Name:                "volumeClaimTemplate workspace 1",
+			VolumeClaimTemplate: &corev1.PersistentVolumeClaim{},
+		},
+		{
+			Name:                "volumeClaimTemplate workspace 2",
+			VolumeClaimTemplate: &corev1.PersistentVolumeClaim{},
+		},
 	}
 	pr := &v1.PipelineRun{
 		TypeMeta: metav1.TypeMeta{Kind: "PipelineRun"},
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "test-pipelinerun-volumeclaimtemplate",
 		},
 		Spec: v1.PipelineRunSpec{
-			Workspaces: []v1.WorkspaceBinding{workspace},
+			Workspaces: workspaces,
 		},
 	}
 
-	// seed data to create StatefulSets and PVCs
-	expectedAffinityAssistantName := GetAffinityAssistantName(workspace.Name, pr.Name)
-	aa := []*appsv1.StatefulSet{{
-		TypeMeta: metav1.TypeMeta{
-			Kind:       "StatefulSet",
-			APIVersion: "apps/v1",
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Name:   expectedAffinityAssistantName,
-			Labels: getStatefulSetLabels(pr, expectedAffinityAssistantName),
+	testCases := []struct {
+		name                           string
+		aaBehavior                     aa.AffinityAssistantBehavior
+		cfgMap                         map[string]string
+		expectedAffinityAssistantNames []string
+		expectedPVCNames               []string
+	}{{
+		name:       "Affinity Assistant Cleanup - per workspace",
+		aaBehavior: aa.AffinityAssistantPerWorkspace,
+		cfgMap: map[string]string{
+			"disable-affinity-assistant": "false",
+			"coschedule":                 "workspaces",
 		},
-		Status: appsv1.StatefulSetStatus{
-			ReadyReplicas: 1,
+		expectedAffinityAssistantNames: []string{"affinity-assistant-9d8b15fa2e", "affinity-assistant-39883fc3b2"},
+		expectedPVCNames:               []string{"pvc-a12c589442", "pvc-5ce7cd98c5"},
+	}, {
+		name:       "Affinity Assistant Cleanup - per pipelinerun",
+		aaBehavior: aa.AffinityAssistantPerPipelineRun,
+		cfgMap: map[string]string{
+			"disable-affinity-assistant": "true",
+			"coschedule":                 "pipelineruns",
 		},
+		expectedAffinityAssistantNames: []string{"affinity-assistant-62843d388a"},
+		expectedPVCNames:               []string{"pvc-a12c589442-affinity-assistant-62843d388a-0", "pvc-5ce7cd98c5-affinity-assistant-62843d388a-0"},
 	}}
-	expectedPVCName := getPersistentVolumeClaimNameWithAffinityAssistant(workspace.Name, pr.Name, workspace, *kmeta.NewControllerRef(pr))
-	pvc := []*corev1.PersistentVolumeClaim{{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: expectedPVCName,
-		}},
-	}
-	data := Data{
-		StatefulSets: aa,
-		PVCs:         pvc,
-	}
-	ctx, c, _ := seedTestData(data)
 
-	// call clean up
-	err := c.cleanupAffinityAssistants(ctx, pr)
-	if err != nil {
-		t.Fatalf("unexpected err when clean up Affinity Assistant: %v", err)
-	}
+	for _, tc := range testCases {
+		// seed data to create StatefulSets and PVCs
+		var statefulsets []*appsv1.StatefulSet
+		for _, expectedAffinityAssistantName := range tc.expectedAffinityAssistantNames {
+			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)
+		}
 
-	// validate the cleanup result
-	_, 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)
-	}
+		var pvcs []*corev1.PersistentVolumeClaim
+		for _, expectedPVCName := range tc.expectedPVCNames {
+			pvc := &corev1.PersistentVolumeClaim{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: expectedPVCName,
+				},
+			}
+			pvcs = append(pvcs, pvc)
+		}
+
+		data := Data{
+			StatefulSets: statefulsets,
+			PVCs:         pvcs,
+		}
+
+		_, c, _ := seedTestData(data)
+		ctx := cfgtesting.SetFeatureFlags(context.Background(), t, tc.cfgMap)
+
+		// 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.expectedAffinityAssistantNames {
+			_, 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)
+			}
+		}
 
-	//  the PVCs are NOT expected to be deleted at Affinity Assistant cleanup time
-	_, 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)
+		// validate pvcs
+		for _, expectedPVCName := range tc.expectedPVCNames {
+			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")
+				}
+			}
+		}
 	}
 }
 
-func TestCleanupAffinityAssistants_Failure(t *testing.T) {
+func TestCleanupAffinityAssistantsAndPVCs_Failure(t *testing.T) {
 	pr := &v1.PipelineRun{
 		Spec: v1.PipelineRunSpec{
 			Workspaces: []v1.WorkspaceBinding{{
@@ -716,7 +771,7 @@ func TestCleanupAffinityAssistants_Failure(t *testing.T) {
 		errors.New("failed to delete StatefulSet affinity-assistant-e3b0c44298: error deleting statefulsets"),
 	})
 
-	errs := c.cleanupAffinityAssistants(ctx, pr)
+	errs := c.cleanupAffinityAssistantsAndPVCs(ctx, pr)
 	if errs == nil {
 		t.Fatalf("expecting Affinity Assistant cleanup error but got nil")
 	}
@@ -747,7 +802,7 @@ func TestThatCleanupIsAvoidedIfAssistantIsDisabled(t *testing.T) {
 	store := config.NewStore(logtesting.TestLogger(t))
 	store.OnConfigChanged(configMap)
 
-	_ = c.cleanupAffinityAssistants(store.ToContext(context.Background()), testPRWithPVC)
+	_ = c.cleanupAffinityAssistantsAndPVCs(store.ToContext(context.Background()), testPRWithPVC)
 
 	if len(fakeClientSet.Actions()) != 0 {
 		t.Errorf("Expected 0 k8s client requests, did %d request", len(fakeClientSet.Actions()))
@@ -958,8 +1013,10 @@ func seedTestData(d Data) (context.Context, Reconciler, func()) {
 	ctx := context.Background()
 	ctx, cancel := context.WithCancel(ctx)
 
+	kubeClientSet := fakek8s.NewSimpleClientset()
 	c := Reconciler{
-		KubeClientSet: fakek8s.NewSimpleClientset(),
+		KubeClientSet: kubeClientSet,
+		pvcHandler:    volumeclaim.NewPVCHandler(kubeClientSet, zap.NewExample().Sugar()),
 	}
 	for _, s := range d.StatefulSets {
 		c.KubeClientSet.AppsV1().StatefulSets(s.Namespace).Create(ctx, s, metav1.CreateOptions{})

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -230,9 +230,9 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pr *v1.PipelineRun) pkgr
 
 	if pr.IsDone() {
 		pr.SetDefaults(ctx)
-		err := c.cleanupAffinityAssistants(ctx, pr)
+		err := c.cleanupAffinityAssistantsAndPVCs(ctx, pr)
 		if err != nil {
-			logger.Errorf("Failed to delete StatefulSet for PipelineRun %s: %v", pr.Name, err)
+			logger.Errorf("Failed to delete StatefulSet or PVC for PipelineRun %s: %v", pr.Name, err)
 		}
 		return c.finishReconcileUpdateEmitEvents(ctx, pr, before, err)
 	}

pkg/reconciler/volumeclaim/pvchandler.go

@@ -19,13 +19,16 @@ package volumeclaim
 import (
 	"context"
 	"crypto/sha256"
+	"encoding/json"
 	"fmt"
 
 	v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
 	"go.uber.org/zap"
+	"gomodules.xyz/jsonpatch/v2"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	errorutils "k8s.io/apimachinery/pkg/util/errors"
 	clientset "k8s.io/client-go/kubernetes"
 )
@@ -39,6 +42,7 @@ const (
 // PvcHandler is used to create PVCs for workspaces
 type PvcHandler interface {
 	CreatePVCsForWorkspaces(ctx context.Context, wb []v1.WorkspaceBinding, ownerReference metav1.OwnerReference, namespace string) error
+	PurgeFinalizerAndDeletePVCForWorkspace(ctx context.Context, pvcName, namespace string) error
 }
 
 type defaultPVCHandler struct {
@@ -81,6 +85,49 @@ func (c *defaultPVCHandler) CreatePVCsForWorkspaces(ctx context.Context, wb []v1
 	return errorutils.NewAggregate(errs)
 }
 
+// PurgeFinalizerAndDeletePVCForWorkspace purges the `kubernetes.io/pvc-protection` finalizer protection of the given pvc and then deletes it.
+// Pruging 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 {
+	p, err := c.clientset.CoreV1().PersistentVolumeClaims(namespace).Get(ctx, pvcName, metav1.GetOptions{})
+	if err != nil {
+		return fmt.Errorf("failed to get the PVC %s: %w", pvcName, err)
+	}
+
+	// get the list of existing finalizers and drop `pvc-protection` if exists
+	var finalizers []string
+	for _, f := range p.ObjectMeta.Finalizers {
+		if f == "kubernetes.io/pvc-protection" {
+			continue
+		}
+		finalizers = append(finalizers, f)
+	}
+
+	// prepare data to remove pvc-protection from the list of finalizers
+	removeFinalizerBytes, err := json.Marshal([]jsonpatch.JsonPatchOperation{{
+		Path:      "/metadata/finalizers",
+		Operation: "replace",
+		Value:     finalizers,
+	}})
+	if err != nil {
+		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)
+	}
+
+	return nil
+}
+
 func getPVCsWithoutAffinityAssistant(workspaceBindings []v1.WorkspaceBinding, ownerReference metav1.OwnerReference, namespace string) map[string]*corev1.PersistentVolumeClaim {
 	claims := make(map[string]*corev1.PersistentVolumeClaim)
 	for _, workspaceBinding := range workspaceBindings {