Skip to content

Commit

Permalink
[TEP-0135] Purge finalizer and delete PVC
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
QuanZhang-William authored and tekton-robot committed Jul 20, 2023
1 parent f5b1a3e commit 84b8621
Show file tree
Hide file tree
Showing 5 changed files with 221 additions and 62 deletions.
42 changes: 32 additions & 10 deletions pkg/reconciler/pipelinerun/affinity_assistant.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
157 changes: 107 additions & 50 deletions pkg/reconciler/pipelinerun/affinity_assistant_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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()
Expand Down Expand Up @@ -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
})
}
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
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{{
Expand All @@ -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")
}
Expand Down Expand Up @@ -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()))
Expand Down Expand Up @@ -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{})
Expand Down
4 changes: 2 additions & 2 deletions pkg/reconciler/pipelinerun/pipelinerun.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
47 changes: 47 additions & 0 deletions pkg/reconciler/volumeclaim/pvchandler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
Loading

0 comments on commit 84b8621

Please sign in to comment.