From 097cfd096b6b6a7b9d55357632bb0e811a967732 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 | 136 +++++++----------- pkg/reconciler/volumeclaim/pvchandler.go | 14 +- pkg/reconciler/volumeclaim/pvchandler_test.go | 20 ++- test/affinity_assistant_test.go | 6 + test/wait.go | 25 ++++ 5 files changed, 102 insertions(+), 99 deletions(-) diff --git a/pkg/reconciler/pipelinerun/affinity_assistant_test.go b/pkg/reconciler/pipelinerun/affinity_assistant_test.go index e86d12a5eee..2511c710058 100644 --- a/pkg/reconciler/pipelinerun/affinity_assistant_test.go +++ b/pkg/reconciler/pipelinerun/affinity_assistant_test.go @@ -781,7 +781,8 @@ func TestThatAffinityAssistantNameIsNoLongerThan53(t *testing.T) { t.Errorf("affinity assistant name can not be longer than 53 chars") } } -func TestCleanupAffinityAssistants_Success(t *testing.T) { + +func TestCleanupAffinityAssistants_PerWorkspace_Success(t *testing.T) { workspaces := []v1.WorkspaceBinding{ { Name: "volumeClaimTemplate workspace 1", @@ -802,98 +803,63 @@ func TestCleanupAffinityAssistants_Success(t *testing.T) { }, } - 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", - }, - 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"}, - }} - - 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) - } - - var pvcs []*corev1.PersistentVolumeClaim - for _, expectedPVCName := range tc.pvcNames { - pvc := &corev1.PersistentVolumeClaim{ - ObjectMeta: metav1.ObjectMeta{ - Name: expectedPVCName, - }, - } - pvcs = append(pvcs, pvc) + aaNames := []string{"affinity-assistant-9d8b15fa2e", "affinity-assistant-39883fc3b2"} + var statefulsets []*appsv1.StatefulSet + for _, expectedAffinityAssistantName := range aaNames { + 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) + } - data := Data{ - StatefulSets: statefulsets, - PVCs: pvcs, + pvcNames := []string{"pvc-a12c589442", "pvc-5ce7cd98c5"} + var pvcs []*corev1.PersistentVolumeClaim + for _, expectedPVCName := range pvcNames { + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: expectedPVCName, + }, } + pvcs = append(pvcs, pvc) + } - _, c, _ := seedTestData(data) - ctx := cfgtesting.SetFeatureFlags(context.Background(), t, tc.cfgMap) + data := Data{ + StatefulSets: statefulsets, + PVCs: pvcs, + } + _, c, _ := seedTestData(data) + ctx := context.Background() - // call clean up - err := c.cleanupAffinityAssistantsAndPVCs(ctx, pr) - if err != nil { - t.Fatalf("unexpected err when clean up Affinity Assistant: %v", err) - } + // 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) - } + // validate the cleanup result + for _, expectedAffinityAssistantName := range aaNames { + _, 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 - 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") - } - } + // validate pvcs + for _, expectedPVCName := range pvcNames { + // 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) } } } 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..fd24ef9efb7 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,21 @@ 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) - } } 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