[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)

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 {