From faa8cab42d9a01a032e3ab02a150b3596cd614b4 Mon Sep 17 00:00:00 2001 From: Billy Lynch Date: Thu, 25 Mar 2021 12:29:36 -0400 Subject: [PATCH] PVC: Use Owner UIDs instead of Owner names. We've noticed that using Owner names can lead to race conditions in deleting a volumeClaimTemplate PVC in where a PipelineRun is quickly deleted then recreated (see https://github.com/tektoncd/pipeline/issues/3855). Using the UID allows us to uniquely identify independent runs with the same name, avoiding this issue while keeping the same semantic. --- pkg/reconciler/pipelinerun/pipelinerun_test.go | 16 +++++++++++++--- pkg/reconciler/taskrun/taskrun_test.go | 15 +++++++++------ pkg/reconciler/volumeclaim/pvchandler.go | 7 ++++--- pkg/reconciler/volumeclaim/pvchandler_test.go | 7 ++++--- 4 files changed, 30 insertions(+), 15 deletions(-) diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index df40b77ad78..b155870cb27 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -41,6 +41,7 @@ import ( "github.com/tektoncd/pipeline/pkg/reconciler/pipelinerun/resources" taskrunresources "github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources" ttesting "github.com/tektoncd/pipeline/pkg/reconciler/testing" + "github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim" "github.com/tektoncd/pipeline/test" "github.com/tektoncd/pipeline/test/diff" "github.com/tektoncd/pipeline/test/names" @@ -2846,9 +2847,18 @@ func TestReconcileWithVolumeClaimTemplateWorkspace(t *testing.T) { t.Errorf("expected one PVC created. %d was created", len(pvcNames)) } - expectedPVCName := fmt.Sprintf("%s-%s", claimName, "cab465d09a") - if pvcNames[0] != expectedPVCName { - t.Errorf("expected the created PVC to be named %s. It was named %s", expectedPVCName, pvcNames[0]) + for _, pr := range prs { + for _, w := range pr.Spec.Workspaces { + expectedPVCName := volumeclaim.GetPersistentVolumeClaimName(&corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: w.VolumeClaimTemplate.Name, + }, + }, w, pr.GetOwnerReference()) + _, err := clients.Kube.CoreV1().PersistentVolumeClaims(pr.Namespace).Get(prt.TestAssets.Ctx, expectedPVCName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("expected PVC %s to exist but instead got error when getting it: %v", expectedPVCName, err) + } + } } taskRuns, err := clients.Pipeline.TektonV1beta1().TaskRuns("foo").List(prt.TestAssets.Ctx, metav1.ListOptions{}) diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index 2c812894031..b86ed0ba01f 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -2982,12 +2982,15 @@ func TestReconcileWorkspaceWithVolumeClaimTemplate(t *testing.T) { if w.PersistentVolumeClaim != nil { t.Fatalf("expected workspace from volumeClaimTemplate to be translated to PVC") } - } - - expectedPVCName := fmt.Sprintf("%s-%s", claimName, "a521418087") - _, err = clients.Kube.CoreV1().PersistentVolumeClaims(taskRun.Namespace).Get(testAssets.Ctx, expectedPVCName, metav1.GetOptions{}) - if err != nil { - t.Fatalf("expected PVC %s to exist but instead got error when getting it: %v", expectedPVCName, err) + expectedPVCName := volumeclaim.GetPersistentVolumeClaimName(&corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: w.VolumeClaimTemplate.Name, + }, + }, w, ttt.GetOwnerReference()) + _, err = clients.Kube.CoreV1().PersistentVolumeClaims(taskRun.Namespace).Get(testAssets.Ctx, expectedPVCName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("expected PVC %s to exist but instead got error when getting it: %v", expectedPVCName, err) + } } } diff --git a/pkg/reconciler/volumeclaim/pvchandler.go b/pkg/reconciler/volumeclaim/pvchandler.go index 1713a85bcdc..6eff3001aa3 100644 --- a/pkg/reconciler/volumeclaim/pvchandler.go +++ b/pkg/reconciler/volumeclaim/pvchandler.go @@ -91,13 +91,14 @@ func getPersistentVolumeClaims(workspaceBindings []v1beta1.WorkspaceBinding, own // GetPersistentVolumeClaimName gets the name of PersistentVolumeClaim for a Workspace and PipelineRun or TaskRun. claim // must be a PersistentVolumeClaim from a volumeClaimTemplate. The returned name must be consistent given the same -// workspaceBinding name and ownerReference name - because it is first used for creating a PVC and later, +// workspaceBinding name and ownerReference UID - because it is first used for creating a PVC and later, // possibly several TaskRuns to lookup the PVC to mount. +// We use ownerReference UID over ownerReference name to distinguish runs with the same name. func GetPersistentVolumeClaimName(claim *corev1.PersistentVolumeClaim, wb v1beta1.WorkspaceBinding, owner metav1.OwnerReference) string { if claim.Name == "" { - return fmt.Sprintf("%s-%s", "pvc", getPersistentVolumeClaimIdentity(wb.Name, owner.Name)) + return fmt.Sprintf("%s-%s", "pvc", getPersistentVolumeClaimIdentity(wb.Name, string(owner.UID))) } - return fmt.Sprintf("%s-%s", claim.Name, getPersistentVolumeClaimIdentity(wb.Name, owner.Name)) + return fmt.Sprintf("%s-%s", claim.Name, getPersistentVolumeClaimIdentity(wb.Name, string(owner.UID))) } func getPersistentVolumeClaimIdentity(workspaceName, ownerName string) string { diff --git a/pkg/reconciler/volumeclaim/pvchandler_test.go b/pkg/reconciler/volumeclaim/pvchandler_test.go index f0f74cbbd59..5f309cf30e5 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" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" fakek8s "k8s.io/client-go/kubernetes/fake" ) @@ -69,7 +70,7 @@ func TestCreatePersistentVolumeClaimsForWorkspaces(t *testing.T) { ctx, cancel := context.WithCancel(ctx) defer cancel() - ownerRef := metav1.OwnerReference{Name: ownerName} + ownerRef := metav1.OwnerReference{UID: types.UID(ownerName)} namespace := "ns" fakekubeclient := fakek8s.NewSimpleClientset() pvcHandler := defaultPVCHandler{fakekubeclient, zap.NewExample().Sugar()} @@ -110,7 +111,7 @@ func TestCreatePersistentVolumeClaimsForWorkspaces(t *testing.T) { t.Fatalf("unexpected number of ownerreferences on created PVC; expected: %d got %d", expectedNumberOfOwnerRefs, len(pvc.OwnerReferences)) } - if pvc.OwnerReferences[0].Name != ownerName { + if string(pvc.OwnerReferences[0].UID) != ownerName { t.Fatalf("unexptected name in ownerreference on created PVC; expected: %s got %s", ownerName, pvc.OwnerReferences[0].Name) } } @@ -134,7 +135,7 @@ func TestCreatePersistentVolumeClaimsForWorkspacesWithoutMetadata(t *testing.T) ctx, cancel := context.WithCancel(ctx) defer cancel() - ownerRef := metav1.OwnerReference{Name: ownerName} + ownerRef := metav1.OwnerReference{UID: types.UID(ownerName)} namespace := "ns" fakekubeclient := fakek8s.NewSimpleClientset() pvcHandler := defaultPVCHandler{fakekubeclient, zap.NewExample().Sugar()}