From c5a2c8007ed492b3ff0ed1a0dc8856f1094e5367 Mon Sep 17 00:00:00 2001 From: Quan Zhang Date: Wed, 20 Sep 2023 17:00:05 -0400 Subject: [PATCH] Fix race condition in PVC deletion fixes [#7138][#7138]. In `coschedule:pipelineruns` mode, the `pvcs` created by `VolumeClaimTemplate` should be automatically deleted when the owning `pipelinerun` is completed. To delete a `pvc` used by a pod (pipelinerun), the [kubernetes.io/pvc-protection][finalizer] finalizer must be removed. Prior to this commit, the Tekton reconciler attempted to remove the `kubernetes.io/pvc-protection` finalizer first and then deletes the created pvcs. This results in race condition since `PVCProtectionController` tries to add back the finalizer if the `pvc` is in `bounded` status . If the add back action happens before the PVC deletion call is completed, the `PVC` will be left in `terminating` status. This commit changes the reconciler to delete the `PVC` first and then removes the `finalizer` to fix the race condition. This commit removes `TestPurgeFinalizerAndDeletePVCForWorkspace` UT, since the finalizer behavior cannot be mocked when a PVC is deleted. This commit also adds integration test to cover this scenario. /kind bug [#7138]: https://github.com/tektoncd/pipeline/issues/7138 [finalizer]: https://kubernetes.io/docs/concepts/storage/persistent-volumes/#storage-object-in-use-protection --- .../pipelinerun/affinity_assistant_test.go | 25 ++++++++++++++++--- pkg/reconciler/volumeclaim/pvchandler.go | 14 +++++------ pkg/reconciler/volumeclaim/pvchandler_test.go | 25 +++++++++++++++---- test/affinity_assistant_test.go | 6 +++++ test/wait.go | 25 +++++++++++++++++++ 5 files changed, 80 insertions(+), 15 deletions(-) diff --git a/pkg/reconciler/pipelinerun/affinity_assistant_test.go b/pkg/reconciler/pipelinerun/affinity_assistant_test.go index e86d12a5eee..3024dd740b0 100644 --- a/pkg/reconciler/pipelinerun/affinity_assistant_test.go +++ b/pkg/reconciler/pipelinerun/affinity_assistant_test.go @@ -781,6 +781,7 @@ func TestThatAffinityAssistantNameIsNoLongerThan53(t *testing.T) { t.Errorf("affinity assistant name can not be longer than 53 chars") } } + func TestCleanupAffinityAssistants_Success(t *testing.T) { workspaces := []v1.WorkspaceBinding{ { @@ -866,6 +867,20 @@ func TestCleanupAffinityAssistants_Success(t *testing.T) { _, c, _ := seedTestData(data) ctx := cfgtesting.SetFeatureFlags(context.Background(), t, tc.cfgMap) + // mocks `kubernetes.io/pvc-protection` finalizer behavior by adding DeletionTimestamp when deleting pvcs with the finalizer + // see details in: https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers/#how-finalizers-work + if tc.aaBehavior == aa.AffinityAssistantPerPipelineRun { + for _, pvc := range pvcs { + pvc.DeletionTimestamp = &metav1.Time{ + Time: metav1.Now().Time, + } + c.KubeClientSet.CoreV1().(*fake.FakeCoreV1).PrependReactor("delete", "persistentvolumeclaims", + func(action testing2.Action) (handled bool, ret runtime.Object, err error) { + return true, pvc, nil + }) + } + } + // call clean up err := c.cleanupAffinityAssistantsAndPVCs(ctx, pr) if err != nil { @@ -889,9 +904,13 @@ func TestCleanupAffinityAssistants_Success(t *testing.T) { 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") + pvc, err := c.KubeClientSet.CoreV1().PersistentVolumeClaims(pr.Namespace).Get(ctx, expectedPVCName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("unexpect err when getting PersistentVolumeClaims, err: %v", err) + } + // validate the finalizer is removed, which mocks the pvc is deleted properly + if len(pvc.Finalizers) > 0 { + t.Errorf("pvc %s kubernetes.io/pvc-protection finalizer is not removed properly", pvc.Name) } } } diff --git a/pkg/reconciler/volumeclaim/pvchandler.go b/pkg/reconciler/volumeclaim/pvchandler.go index 12a8aed4b43..b8c8de27fba 100644 --- a/pkg/reconciler/volumeclaim/pvchandler.go +++ b/pkg/reconciler/volumeclaim/pvchandler.go @@ -85,7 +85,7 @@ func (c *defaultPVCHandler) CreatePVCFromVolumeClaimTemplate(ctx context.Context return nil } -// PurgeFinalizerAndDeletePVCForWorkspace purges the `kubernetes.io/pvc-protection` finalizer protection of the given pvc and then deletes it. +// PurgeFinalizerAndDeletePVCForWorkspace deletes pvcs and then purges the `kubernetes.io/pvc-protection` finalizer protection. // 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 { @@ -113,18 +113,18 @@ func (c *defaultPVCHandler) PurgeFinalizerAndDeletePVCForWorkspace(ctx context.C 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) } + // remove finalizer + _, 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) + } + return nil } diff --git a/pkg/reconciler/volumeclaim/pvchandler_test.go b/pkg/reconciler/volumeclaim/pvchandler_test.go index 9cd1c8350fe..9b05c23dc00 100644 --- a/pkg/reconciler/volumeclaim/pvchandler_test.go +++ b/pkg/reconciler/volumeclaim/pvchandler_test.go @@ -25,12 +25,12 @@ 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" "k8s.io/apimachinery/pkg/types" fakek8s "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/kubernetes/typed/core/v1/fake" client_go_testing "k8s.io/client-go/testing" ) @@ -263,15 +263,30 @@ func TestPurgeFinalizerAndDeletePVCForWorkspace(t *testing.T) { t.Fatalf("unexpected error when creating PVC: %v", err) } + // mocks `kubernetes.io/pvc-protection` finalizer behavior by adding DeletionTimestamp when deleting pvcs with the finalizer + // see details in: https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers/#how-finalizers-work + pvc.DeletionTimestamp = &metav1.Time{ + Time: metav1.Now().Time, + } + kubeClientSet.CoreV1().(*fake.FakeCoreV1).PrependReactor("delete", "persistentvolumeclaims", + func(action client_go_testing.Action) (handled bool, ret runtime.Object, err error) { + return true, pvc, nil + }) + // call PurgeFinalizerAndDeletePVCForWorkspace to delete pvc + // note that the pvcs are not actually deleted in the unit test due to the mock limitation of fakek8s.NewSimpleClientset(); + // full pvc lifecycle is tested in TestAffinityAssistant_PerPipelineRun integration test 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) + // validate the `kubernetes.io/pvc-protection` finalizer is removed + pvc, err := kubeClientSet.CoreV1().PersistentVolumeClaims(namespace).Get(ctx, pvcName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(pvc.Finalizers) > 0 { + t.Errorf("pvc %s kubernetes.io/pvc-protection finalizer is not removed properly", pvcName) } } diff --git a/test/affinity_assistant_test.go b/test/affinity_assistant_test.go index 4c23676031b..e74942b299f 100644 --- a/test/affinity_assistant_test.go +++ b/test/affinity_assistant_test.go @@ -205,6 +205,12 @@ spec: t.Errorf("Error waiting for PipelineRun to finish: %s", err) } + // wait and check PVCs are deleted + t.Logf("Waiting for PVC in namespace %s to delete", namespace) + if err := WaitForPVCIsDeleted(ctx, c, timeout, prName, namespace, "PVCDeleted"); err != nil { + t.Errorf("Error waiting for PVC to be deleted: %s", err) + } + // validate PipelineRun pods sharing the same PVC are scheduled to the same node podNames := []string{fmt.Sprintf("%v-foo-pod", prName), fmt.Sprintf("%v-bar-pod", prName), fmt.Sprintf("%v-double-ws-pod", prName), fmt.Sprintf("%v-no-ws-pod", prName)} validatePodAffinity(t, ctx, podNames, namespace, c.KubeClient) diff --git a/test/wait.go b/test/wait.go index 70ad1b0466f..aa5a19d272d 100644 --- a/test/wait.go +++ b/test/wait.go @@ -144,6 +144,31 @@ func WaitForPodState(ctx context.Context, c *clients, name string, namespace str }) } +// WaitForPVCIsDeleted polls the number of the PVC in the namespace from client every +// interval until all the PVCs in the namespace are deleted. It returns an +// error or timeout. desc will be used to name the metric that is emitted to +// track how long it took to delete all the PVCs in the namespace. +func WaitForPVCIsDeleted(ctx context.Context, c *clients, polltimeout time.Duration, name, namespace, desc string) error { + metricName := fmt.Sprintf("WaitForPVCIsDeleted/%s/%s", name, desc) + _, span := trace.StartSpan(context.Background(), metricName) + defer span.End() + + ctx, cancel := context.WithTimeout(ctx, polltimeout) + defer cancel() + + return pollImmediateWithContext(ctx, func() (bool, error) { + pvcList, err := c.KubeClient.CoreV1().PersistentVolumeClaims(namespace).List(ctx, metav1.ListOptions{}) + if err != nil { + return true, err + } + if len(pvcList.Items) > 0 { + return false, nil + } + + return true, nil + }) +} + // WaitForPipelineRunState polls the status of the PipelineRun called name from client every // interval until inState returns `true` indicating it is done, returns an // error or timeout. desc will be used to name the metric that is emitted to