diff --git a/pkg/reconciler/pipelinerun/affinity_assistant.go b/pkg/reconciler/pipelinerun/affinity_assistant.go index 6a0a20033da..9e2c1efe8b5 100644 --- a/pkg/reconciler/pipelinerun/affinity_assistant.go +++ b/pkg/reconciler/pipelinerun/affinity_assistant.go @@ -181,22 +181,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) } diff --git a/pkg/reconciler/pipelinerun/affinity_assistant_test.go b/pkg/reconciler/pipelinerun/affinity_assistant_test.go index 2dadbea16a0..6dea45031a8 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" @@ -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,11 +630,16 @@ 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"}, @@ -642,57 +647,107 @@ func TestCleanupAffinityAssistants_Success(t *testing.T) { 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 + affinityAssistantNames []string + pvcNames []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, + 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"}, }} - 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.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) + } - // 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.pvcNames { + 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.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) + } + } - // 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.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") + } + } + } } } -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())) @@ -986,8 +1041,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 f2308c72065..f022e6efd87 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..5f6f54e380a 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,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. +// 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 { + 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..7b7b7d319d3 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 to delete pvc + 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) + } +}