From 56ebb8845fe588953b6b649c0fb7aa1696baa156 Mon Sep 17 00:00:00 2001 From: Quan Zhang Date: Tue, 18 Jul 2023 15:05:16 -0400 Subject: [PATCH] [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]: https://github.com/tektoncd/pipeline/pull/6635 [discussion]: https://github.com/tektoncd/pipeline/pull/6741#issuecomment-1610123340 --- .../pipelinerun/affinity_assistant.go | 41 ++++-- .../pipelinerun/affinity_assistant_test.go | 133 ++++++++++++------ pkg/reconciler/pipelinerun/pipelinerun.go | 4 +- pkg/reconciler/volumeclaim/pvchandler.go | 44 ++++++ pkg/reconciler/volumeclaim/pvchandler_test.go | 33 +++++ 5 files changed, 198 insertions(+), 57 deletions(-) diff --git a/pkg/reconciler/pipelinerun/affinity_assistant.go b/pkg/reconciler/pipelinerun/affinity_assistant.go index f1a38aa857e..dfbfe0a75f7 100644 --- a/pkg/reconciler/pipelinerun/affinity_assistant.go +++ b/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) } diff --git a/pkg/reconciler/pipelinerun/affinity_assistant_test.go b/pkg/reconciler/pipelinerun/affinity_assistant_test.go index 4f5e874decd..ebb91a20fce 100644 --- a/pkg/reconciler/pipelinerun/affinity_assistant_test.go +++ b/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{}) diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 2552c7a8173..eb2a9d6ee0c 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/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) } diff --git a/pkg/reconciler/volumeclaim/pvchandler.go b/pkg/reconciler/volumeclaim/pvchandler.go index 9e9283d3016..c34705cb8dc 100644 --- a/pkg/reconciler/volumeclaim/pvchandler.go +++ b/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 { diff --git a/pkg/reconciler/volumeclaim/pvchandler_test.go b/pkg/reconciler/volumeclaim/pvchandler_test.go index aaffa85d478..98290ec4323 100644 --- a/pkg/reconciler/volumeclaim/pvchandler_test.go +++ b/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) + } +}