Skip to content

Commit

Permalink
PVC: Use Owner UIDs instead of Owner names.
Browse files Browse the repository at this point in the history
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 #3855).

Using the UID allows us to uniquely identify independent runs with the
same name, avoiding this issue while keeping the same semantic.
  • Loading branch information
wlynch committed Mar 25, 2021
1 parent bad4550 commit 52bed56
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 15 deletions.
17 changes: 14 additions & 3 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -2846,9 +2847,19 @@ 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 {
t.Logf("%+v", w)
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{})
Expand Down
15 changes: 9 additions & 6 deletions pkg/reconciler/taskrun/taskrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}

Expand Down
7 changes: 4 additions & 3 deletions pkg/reconciler/volumeclaim/pvchandler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
7 changes: 4 additions & 3 deletions pkg/reconciler/volumeclaim/pvchandler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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()}
Expand Down Expand Up @@ -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)
}
}
Expand All @@ -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()}
Expand Down

0 comments on commit 52bed56

Please sign in to comment.