[TEP-0135] Purge finalizer and delete PVC

Part of [tektoncd#6740][tektoncd#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]

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

config/config-feature-flags.yaml

@@ -29,7 +29,7 @@ data:
   # See more in the workspace documentation about Affinity Assistant
   # https://github.com/tektoncd/pipeline/blob/main/docs/workspaces.md#affinity-assistant-and-specifying-workspace-order-in-a-pipeline
   # or https://github.com/tektoncd/pipeline/pull/2630 for more info.
-  disable-affinity-assistant: "false"
+  disable-affinity-assistant: "true"
   # Setting this flag will determine how PipelineRun Pods are scheduled with Affinity Assistant.
   # Acceptable values are "workspaces" (default), "pipelineruns", "isolate-pipelinerun", or "disabled".
   #
@@ -41,7 +41,7 @@ data:
   #
   # TODO: add links to documentation and migration strategy
   # NOTE: this feature is still under development and not yet functional.
-  coschedule: "workspaces"
+  coschedule: "pipelineruns"
   # Setting this flag to "true" will prevent Tekton scanning attached
   # service accounts and injecting any credentials it finds into your
   # Steps.

pkg/reconciler/pipelinerun/affinity_assistant.go

@@ -178,22 +178,43 @@ 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:
+		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"
@@ -178,7 +179,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 +296,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,7 +631,6 @@ 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",
@@ -646,53 +646,94 @@ func TestCleanupAffinityAssistants_Success(t *testing.T) {
 		},
 	}
 
-	// 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
+	}{{
+		name:       "Affinity Assistant Cleanup - per workspace",
+		aaBehavior: aa.AffinityAssistantPerWorkspace,
+		cfgMap: map[string]string{
+			"disable-affinity-assistant": "false",
+			"coschedule":                 "workspaces",
 		},
-		Status: appsv1.StatefulSetStatus{
-			ReadyReplicas: 1,
+	}, {
+		name:       "Affinity Assistant Cleanup - per pipelinerun",
+		aaBehavior: aa.AffinityAssistantPerPipelineRun,
+		cfgMap: map[string]string{
+			"disable-affinity-assistant": "true",
+			"coschedule":                 "pipelineruns",
 		},
 	}}
-	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 {
+		expectedAffinityAssistantName := ""
+		expectedPVCName := ""
+		if tc.aaBehavior == aa.AffinityAssistantPerPipelineRun {
+			expectedAffinityAssistantName = GetAffinityAssistantName("", pr.Name)
+			expectedPVCName = getPersistentVolumeClaimNameWithAffinityAssistant("", pr.Name, workspace, *kmeta.NewControllerRef(pr))
+		} else if tc.aaBehavior == aa.AffinityAssistantPerWorkspace {
+			expectedAffinityAssistantName = GetAffinityAssistantName(workspace.Name, pr.Name)
+			expectedPVCName = volumeclaim.GetPVCNameWithoutAffinityAssistant("", workspace, *kmeta.NewControllerRef(pr))
+		}
 
-	// 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)
-	}
+		// seed data to create StatefulSets and PVCs
+		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,
+			},
+		}}
+
+		pvc := []*corev1.PersistentVolumeClaim{{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: expectedPVCName,
+			}},
+		}
+		data := Data{
+			StatefulSets: ss,
+			PVCs:         pvc,
+		}
 
-	//  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)
+		_, 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
+		_, 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
+		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 +757,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 +788,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 +999,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,7 +230,7 @@ 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)
 		}

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,46 @@ func (c *defaultPVCHandler) CreatePVCsForWorkspaces(ctx context.Context, wb []v1
 	return errorutils.NewAggregate(errs)
 }
 
+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 {

pkg/reconciler/volumeclaim/pvchandler_test.go

@@ -25,6 +25,7 @@ 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"
@@ -233,3 +234,35 @@ func TestCreateExistPersistentVolumeClaims(t *testing.T) {
 		t.Fatalf("unexpected PVC name on created PVC; expected: %s got: %s", expectedPVCName, pvcList.Items[0].Name)
 	}
 }
+
+func TestPurgeFinalizerAndDeletePVCForWorkspace(t *testing.T) {
+	ctx := context.Background()
+	kubeClientSet := fakek8s.NewSimpleClientset()
+
+	// seed data
+	namespace := "my-ns"
+	pvcName := "my-pvc"
+	pvc := &corev1.PersistentVolumeClaim{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      pvcName,
+			Namespace: namespace,
+			Finalizers: []string{
+				"kubernetes.io/pvc-protection",
+			},
+		}}
+	if _, err := kubeClientSet.CoreV1().PersistentVolumeClaims(namespace).Create(ctx, pvc, metav1.CreateOptions{}); err != nil {
+		t.Fatalf("unexpected error when creating PVC: %v", err)
+	}
+
+	// call PurgeFinalizerAndDeletePVCForWorkspace
+	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)
+	}
+}