From eba52e6e3146ab0be6ca09fb79eaedc7a2477b9e Mon Sep 17 00:00:00 2001 From: Quan Zhang Date: Thu, 25 May 2023 22:44:38 +0800 Subject: [PATCH] TEP-0135: Refactor Affinity Assistant PVC creation Part of [#6740][#6740]. [TEP-0135][tep-0135] introduces a feature that allows a cluster operator to ensure that all of a PipelineRun's pods are scheduled to the same node. Before this commit, the PipelineRun reconciler creates PVC for each `VolumeClaimTemplate` backed workspace, and mount the PVCs to the AA to avoid PV availability zone conflict. This implementation works for `AffinityAssistantPerWorkspace` but introduces availability zone conflict issue in the `AffinityAssistantPerPipelineRun` mode since we cannot enforce all the PVC are created in the same availability zone. Instead of directly creating a PVC for each PipelineRun workspace backed by a VolumeClaimTemplate, this commit sets one VolumeClaimTemplate per PVC workspace in the affinity assistant StatefulSet spec, which enforces all VolumeClaimTemplates in StatefulSets are all provisioned on the same node/availability zone. This commit just refactors the current implementation in favor of the `AffinityAssistantPerPipelineRun` feature. There is no functionality change. The `AffinityAssistantPerPipelineRun` feature will be added in the follow up PRs. [#6740]: https://github.com/tektoncd/pipeline/issues/6740 [tep-0135]: https://github.com/tektoncd/community/blob/main/teps/0135-coscheduling-pipelinerun-pods.md --- .../pipelinerun/affinity_assistant.go | 176 +++++++++++------- .../pipelinerun/affinity_assistant_test.go | 175 ++++++++++++++--- pkg/reconciler/pipelinerun/pipelinerun.go | 59 +++--- .../pipelinerun/pipelinerun_test.go | 105 ++++++++++- pkg/reconciler/taskrun/taskrun.go | 4 +- pkg/reconciler/taskrun/taskrun_test.go | 6 +- pkg/reconciler/volumeclaim/pvchandler.go | 24 ++- pkg/reconciler/volumeclaim/pvchandler_test.go | 8 +- 8 files changed, 402 insertions(+), 155 deletions(-) diff --git a/pkg/reconciler/pipelinerun/affinity_assistant.go b/pkg/reconciler/pipelinerun/affinity_assistant.go index 7a27bdda4b2..9e2a51f49b5 100644 --- a/pkg/reconciler/pipelinerun/affinity_assistant.go +++ b/pkg/reconciler/pipelinerun/affinity_assistant.go @@ -56,73 +56,75 @@ func (c *Reconciler) createOrUpdateAffinityAssistants(ctx context.Context, wb [] var errs []error var unschedulableNodes sets.Set[string] = nil for _, w := range wb { - if w.PersistentVolumeClaim != nil || w.VolumeClaimTemplate != nil { - affinityAssistantName := getAffinityAssistantName(w.Name, pr.Name) - a, err := c.KubeClientSet.AppsV1().StatefulSets(namespace).Get(ctx, affinityAssistantName, metav1.GetOptions{}) - claimName := getClaimName(w, *kmeta.NewControllerRef(pr)) - switch { - // check whether the affinity assistant (StatefulSet) exists or not, create one if it does not exist - case apierrors.IsNotFound(err): - affinityAssistantStatefulSet := affinityAssistantStatefulSet(affinityAssistantName, pr, claimName, c.Images.NopImage, cfg.Defaults.DefaultAAPodTemplate) - _, err := c.KubeClientSet.AppsV1().StatefulSets(namespace).Create(ctx, affinityAssistantStatefulSet, metav1.CreateOptions{}) + if w.PersistentVolumeClaim == nil && w.VolumeClaimTemplate == nil { + continue + } + + var claimTemplates []corev1.PersistentVolumeClaim + var claims []corev1.PersistentVolumeClaimVolumeSource + if w.PersistentVolumeClaim != nil { + claims = append(claims, *w.PersistentVolumeClaim.DeepCopy()) + } else if w.VolumeClaimTemplate != nil { + claimTemplate := w.VolumeClaimTemplate.DeepCopy() + // PVCs Will be created by Affinity Assistant StatefulSet and will follow the naming format `--0` + claimTemplate.Name = volumeclaim.GetPVCNameWithoutAffinityAssistant(w.VolumeClaimTemplate.Name, w, *kmeta.NewControllerRef(pr)) + claimTemplates = append(claimTemplates, *claimTemplate) + } + + affinityAssistantName := getAffinityAssistantName(w.Name, pr.Name) + a, err := c.KubeClientSet.AppsV1().StatefulSets(namespace).Get(ctx, affinityAssistantName, metav1.GetOptions{}) + switch { + // check whether the affinity assistant (StatefulSet) exists or not, create one if it does not exist + case apierrors.IsNotFound(err): + affinityAssistantStatefulSet := affinityAssistantStatefulSet(affinityAssistantName, pr, claimTemplates, claims, c.Images.NopImage, cfg.Defaults.DefaultAAPodTemplate) + _, err := c.KubeClientSet.AppsV1().StatefulSets(namespace).Create(ctx, affinityAssistantStatefulSet, metav1.CreateOptions{}) + if err != nil { + errs = append(errs, fmt.Errorf("failed to create StatefulSet %s: %w", affinityAssistantName, err)) + } + if err == nil { + logger.Infof("Created StatefulSet %s in namespace %s", affinityAssistantName, namespace) + } + // check whether the affinity assistant (StatefulSet) exists and the affinity assistant pod is created + // this check requires the StatefulSet to have the readyReplicas set to 1 to allow for any delay between the StatefulSet creation + // and the necessary pod creation, the delay can be caused by any dependency on PVCs and PVs creation + // this case addresses issues specified in https://github.com/tektoncd/pipeline/issues/6586 + case err == nil && a != nil && a.Status.ReadyReplicas == 1: + if unschedulableNodes == nil { + ns, err := c.KubeClientSet.CoreV1().Nodes().List(ctx, metav1.ListOptions{ + FieldSelector: "spec.unschedulable=true", + }) if err != nil { - errs = append(errs, fmt.Errorf("failed to create StatefulSet %s: %w", affinityAssistantName, err)) + errs = append(errs, fmt.Errorf("could not get the list of nodes, err: %w", err)) } - if err == nil { - logger.Infof("Created StatefulSet %s in namespace %s", affinityAssistantName, namespace) + unschedulableNodes = sets.Set[string]{} + // maintain the list of nodes which are unschedulable + for _, n := range ns.Items { + unschedulableNodes.Insert(n.Name) } - // check whether the affinity assistant (StatefulSet) exists and the affinity assistant pod is created - // this check requires the StatefulSet to have the readyReplicas set to 1 to allow for any delay between the StatefulSet creation - // and the necessary pod creation, the delay can be caused by any dependency on PVCs and PVs creation - // this case addresses issues specified in https://github.com/tektoncd/pipeline/issues/6586 - case err == nil && a != nil && a.Status.ReadyReplicas == 1: - if unschedulableNodes == nil { - ns, err := c.KubeClientSet.CoreV1().Nodes().List(ctx, metav1.ListOptions{ - FieldSelector: "spec.unschedulable=true", - }) - if err != nil { - errs = append(errs, fmt.Errorf("could not get the list of nodes, err: %w", err)) - } - unschedulableNodes = sets.Set[string]{} - // maintain the list of nodes which are unschedulable - for _, n := range ns.Items { - unschedulableNodes.Insert(n.Name) - } + } + if unschedulableNodes.Len() > 0 { + // get the pod created for a given StatefulSet, pod is assigned ordinal of 0 with the replicas set to 1 + p, err := c.KubeClientSet.CoreV1().Pods(pr.Namespace).Get(ctx, a.Name+"-0", metav1.GetOptions{}) + // ignore instead of failing if the affinity assistant pod was not found + if err != nil && !apierrors.IsNotFound(err) { + errs = append(errs, fmt.Errorf("could not get the affinity assistant pod for StatefulSet %s: %w", a.Name, err)) } - if unschedulableNodes.Len() > 0 { - // get the pod created for a given StatefulSet, pod is assigned ordinal of 0 with the replicas set to 1 - p, err := c.KubeClientSet.CoreV1().Pods(pr.Namespace).Get(ctx, a.Name+"-0", metav1.GetOptions{}) - // ignore instead of failing if the affinity assistant pod was not found - if err != nil && !apierrors.IsNotFound(err) { - errs = append(errs, fmt.Errorf("could not get the affinity assistant pod for StatefulSet %s: %w", a.Name, err)) - } - // check the node which hosts the affinity assistant pod if it is unschedulable or cordoned - if p != nil && unschedulableNodes.Has(p.Spec.NodeName) { - // if the node is unschedulable, delete the affinity assistant pod such that a StatefulSet can recreate the same pod on a different node - err = c.KubeClientSet.CoreV1().Pods(p.Namespace).Delete(ctx, p.Name, metav1.DeleteOptions{}) - if err != nil { - errs = append(errs, fmt.Errorf("error deleting affinity assistant pod %s in ns %s: %w", p.Name, p.Namespace, err)) - } + // check the node which hosts the affinity assistant pod if it is unschedulable or cordoned + if p != nil && unschedulableNodes.Has(p.Spec.NodeName) { + // if the node is unschedulable, delete the affinity assistant pod such that a StatefulSet can recreate the same pod on a different node + err = c.KubeClientSet.CoreV1().Pods(p.Namespace).Delete(ctx, p.Name, metav1.DeleteOptions{}) + if err != nil { + errs = append(errs, fmt.Errorf("error deleting affinity assistant pod %s in ns %s: %w", p.Name, p.Namespace, err)) } } - case err != nil: - errs = append(errs, fmt.Errorf("failed to retrieve StatefulSet %s: %w", affinityAssistantName, err)) } + case err != nil: + errs = append(errs, fmt.Errorf("failed to retrieve StatefulSet %s: %w", affinityAssistantName, err)) } } return errorutils.NewAggregate(errs) } -func getClaimName(w v1beta1.WorkspaceBinding, ownerReference metav1.OwnerReference) string { - if w.PersistentVolumeClaim != nil { - return w.PersistentVolumeClaim.ClaimName - } else if w.VolumeClaimTemplate != nil { - return volumeclaim.GetPersistentVolumeClaimName(w.VolumeClaimTemplate, w, ownerReference) - } - - return "" -} - func (c *Reconciler) cleanupAffinityAssistants(ctx context.Context, pr *v1beta1.PipelineRun) error { // omit cleanup if the feature is disabled if c.isAffinityAssistantDisabled(ctx) { @@ -136,11 +138,28 @@ func (c *Reconciler) cleanupAffinityAssistants(ctx context.Context, pr *v1beta1. 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)) } + + // cleanup PVCs created by Affinity Assistants + if w.VolumeClaimTemplate != nil { + pvcName := getPersistentVolumeClaimNameWithAffinityAssistant(w.Name, pr.Name, w, *kmeta.NewControllerRef(pr)) + if err := c.KubeClientSet.CoreV1().PersistentVolumeClaims(pr.Namespace).Delete(ctx, pvcName, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { + errs = append(errs, fmt.Errorf("failed to delete PersistentVolumeClaim %s: %w", pvcName, err)) + } + } } } return errorutils.NewAggregate(errs) } +// getPersistentVolumeClaimNameWithAffinityAssistant returns the PersistentVolumeClaim name that is +// created by the Affinity Assistant StatefulSet VolumeClaimTemplate when Affinity Assistant is enabled. +// The PVCs created by StatefulSet VolumeClaimTemplates follow the format `--0` +func getPersistentVolumeClaimNameWithAffinityAssistant(pipelineWorkspaceName, prName string, wb v1beta1.WorkspaceBinding, owner metav1.OwnerReference) string { + pvcName := volumeclaim.GetPVCNameWithoutAffinityAssistant(wb.VolumeClaimTemplate.Name, wb, owner) + affinityAssistantName := getAffinityAssistantName(pipelineWorkspaceName, prName) + return fmt.Sprintf("%s-%s-0", pvcName, affinityAssistantName) +} + func getAffinityAssistantName(pipelineWorkspaceName string, pipelineRunName string) string { hashBytes := sha256.Sum256([]byte(pipelineWorkspaceName + pipelineRunName)) hashString := fmt.Sprintf("%x", hashBytes) @@ -162,7 +181,10 @@ func getStatefulSetLabels(pr *v1beta1.PipelineRun, affinityAssistantName string) return labels } -func affinityAssistantStatefulSet(name string, pr *v1beta1.PipelineRun, claimName string, affinityAssistantImage string, defaultAATpl *pod.AffinityAssistantTemplate) *appsv1.StatefulSet { +// affinityAssistantStatefulSet returns an Affinity Assistant as a StatefulSet with the given AffinityAssistantTemplate applied to the StatefulSet PodTemplateSpec. +// The VolumeClaimTemplates and Volume of StatefulSet reference the PipelineRun WorkspaceBinding VolumeClaimTempalte and the PVCs respectively. +// The PVs created by the StatefulSet are scheduled to the same availability zone which avoids PV scheduling conflict. +func affinityAssistantStatefulSet(name string, pr *v1beta1.PipelineRun, claimTemplates []corev1.PersistentVolumeClaim, claims []corev1.PersistentVolumeClaimVolumeSource, affinityAssistantImage string, defaultAATpl *pod.AffinityAssistantTemplate) *appsv1.StatefulSet { // We want a singleton pod replicas := int32(1) @@ -172,6 +194,11 @@ func affinityAssistantStatefulSet(name string, pr *v1beta1.PipelineRun, claimNam tpl = pod.MergeAAPodTemplateWithDefault(pr.Spec.PodTemplate.ToAffinityAssistantTemplate(), defaultAATpl) } + var mounts []corev1.VolumeMount + for _, claimTemplate := range claimTemplates { + mounts = append(mounts, corev1.VolumeMount{Name: claimTemplate.Name, MountPath: claimTemplate.Name}) + } + containers := []corev1.Container{{ Name: "affinity-assistant", Image: affinityAssistantImage, @@ -190,8 +217,27 @@ func affinityAssistantStatefulSet(name string, pr *v1beta1.PipelineRun, claimNam "memory": resource.MustParse("100Mi"), }, }, + VolumeMounts: mounts, }} + var volumes []corev1.Volume + for i, claim := range claims { + volumes = append(volumes, corev1.Volume{ + Name: fmt.Sprintf("workspace-%d", i), + VolumeSource: corev1.VolumeSource{ + // A Pod mounting a PersistentVolumeClaim that has a StorageClass with + // volumeBindingMode: Immediate + // the PV is allocated on a Node first, and then the pod need to be + // scheduled to that node. + // To support those PVCs, the Affinity Assistant must also mount the + // same PersistentVolumeClaim - to be sure that the Affinity Assistant + // pod is scheduled to the same Availability Zone as the PV, when using + // a regional cluster. This is called VolumeScheduling. + PersistentVolumeClaim: claim.DeepCopy(), + }, + }) + } + return &appsv1.StatefulSet{ TypeMeta: metav1.TypeMeta{ Kind: "StatefulSet", @@ -207,6 +253,8 @@ func affinityAssistantStatefulSet(name string, pr *v1beta1.PipelineRun, claimNam Selector: &metav1.LabelSelector{ MatchLabels: getStatefulSetLabels(pr, name), }, + // by setting VolumeClaimTemplates from StatefulSet, all the PVs are scheduled to the same Availability Zone as the StatefulSet + VolumeClaimTemplates: claimTemplates, Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Labels: getStatefulSetLabels(pr, name), @@ -219,21 +267,7 @@ func affinityAssistantStatefulSet(name string, pr *v1beta1.PipelineRun, claimNam ImagePullSecrets: tpl.ImagePullSecrets, Affinity: getAssistantAffinityMergedWithPodTemplateAffinity(pr), - Volumes: []corev1.Volume{{ - Name: "workspace", - VolumeSource: corev1.VolumeSource{ - PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ - // A Pod mounting a PersistentVolumeClaim that has a StorageClass with - // volumeBindingMode: Immediate - // the PV is allocated on a Node first, and then the pod need to be - // scheduled to that node. - // To support those PVCs, the Affinity Assistant must also mount the - // same PersistentVolumeClaim - to be sure that the Affinity Assistant - // pod is scheduled to the same Availability Zone as the PV, when using - // a regional cluster. This is called VolumeScheduling. - ClaimName: claimName, - }}, - }}, + Volumes: volumes, }, }, }, diff --git a/pkg/reconciler/pipelinerun/affinity_assistant_test.go b/pkg/reconciler/pipelinerun/affinity_assistant_test.go index 2e347a66bf7..bf7eee6dd6f 100644 --- a/pkg/reconciler/pipelinerun/affinity_assistant_test.go +++ b/pkg/reconciler/pipelinerun/affinity_assistant_test.go @@ -22,6 +22,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline" "github.com/tektoncd/pipeline/pkg/apis/pipeline/pod" @@ -29,6 +30,7 @@ import ( "github.com/tektoncd/pipeline/pkg/workspace" "github.com/tektoncd/pipeline/test/diff" "github.com/tektoncd/pipeline/test/parse" + appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -60,36 +62,151 @@ var testPipelineRun = &v1beta1.PipelineRun{ } // TestCreateAndDeleteOfAffinityAssistant tests to create and delete an Affinity Assistant -// for a given PipelineRun with a PVC workspace +// for a given PipelineRun func TestCreateAndDeleteOfAffinityAssistant(t *testing.T) { - ctx := context.Background() - ctx, cancel := context.WithCancel(ctx) - defer cancel() + tests := []struct { + name string + pr *v1beta1.PipelineRun + expectStatefulSetSpec []*appsv1.StatefulSetSpec + }{{ + name: "PersistentVolumeClaim Workspace type", + pr: &v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{Name: "pipelinerun-with-pvc"}, + Spec: v1beta1.PipelineRunSpec{ + Workspaces: []v1beta1.WorkspaceBinding{{ + Name: "PersistentVolumeClaim Workspace", + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "myclaim", + }, + }}, + }, + }, + expectStatefulSetSpec: []*appsv1.StatefulSetSpec{{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{ + Name: "workspace-0", + VolumeSource: corev1.VolumeSource{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ClaimName: "myclaim"}, + }, + }}, + }, + }, + }}, + }, { + name: "VolumeClaimTemplate Workspace type", + pr: &v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{Name: "pipelinerun-with-volumeClaimTemplate"}, + Spec: v1beta1.PipelineRunSpec{ + Workspaces: []v1beta1.WorkspaceBinding{{ + Name: "VolumeClaimTemplate Workspace", + VolumeClaimTemplate: &corev1.PersistentVolumeClaim{}, + }}, + }, + }, + expectStatefulSetSpec: []*appsv1.StatefulSetSpec{{ + VolumeClaimTemplates: []corev1.PersistentVolumeClaim{{ + ObjectMeta: metav1.ObjectMeta{Name: "pvc-f0680e1c9c"}, + }}, + }}, + }, { + name: "VolumeClaimTemplate and PersistentVolumeClaim Workspaces", + pr: &v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{Name: "pipelinerun-with-volumeClaimTemplate-and-pvc"}, + Spec: v1beta1.PipelineRunSpec{ + Workspaces: []v1beta1.WorkspaceBinding{{ + Name: "VolumeClaimTemplate Workspace", + VolumeClaimTemplate: &corev1.PersistentVolumeClaim{}, + }, { + Name: "PersistentVolumeClaim Workspace", + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "myclaim", + }}, + }, + }, + }, + expectStatefulSetSpec: []*appsv1.StatefulSetSpec{{ + VolumeClaimTemplates: []corev1.PersistentVolumeClaim{{ + ObjectMeta: metav1.ObjectMeta{Name: "pvc-f0680e1c9c"}, + }}}, { + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{ + Name: "workspace-0", + VolumeSource: corev1.VolumeSource{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ClaimName: "myclaim"}, + }, + }}, + }, + }, + }}, + }, { + name: "other Workspace type", + pr: &v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{Name: "pipelinerun-with-emptyDir"}, + Spec: v1beta1.PipelineRunSpec{ + Workspaces: []v1beta1.WorkspaceBinding{{ + Name: "EmptyDir Workspace", + EmptyDir: &corev1.EmptyDirVolumeSource{}, + }}, + }, + }, + expectStatefulSetSpec: nil, + }} - c := Reconciler{ - KubeClientSet: fakek8s.NewSimpleClientset(), - Images: pipeline.Images{}, - } + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + ctx, cancel := context.WithCancel(ctx) + defer cancel() - err := c.createOrUpdateAffinityAssistants(ctx, testPipelineRun.Spec.Workspaces, testPipelineRun, testPipelineRun.Namespace) - if err != nil { - t.Errorf("unexpected error from createOrUpdateAffinityAssistants: %v", err) - } + c := Reconciler{ + KubeClientSet: fakek8s.NewSimpleClientset(), + } - expectedAffinityAssistantName := getAffinityAssistantName(workspaceName, testPipelineRun.Name) - _, err = c.KubeClientSet.AppsV1().StatefulSets(testPipelineRun.Namespace).Get(ctx, expectedAffinityAssistantName, metav1.GetOptions{}) - if err != nil { - t.Errorf("unexpected error when retrieving StatefulSet: %v", err) - } + err := c.createOrUpdateAffinityAssistants(ctx, tc.pr.Spec.Workspaces, tc.pr, tc.pr.Namespace) + if err != nil { + t.Errorf("unexpected error from createOrUpdateAffinityAssistants: %v", err) + } - err = c.cleanupAffinityAssistants(ctx, testPipelineRun) - if err != nil { - t.Errorf("unexpected error from cleanupAffinityAssistants: %v", err) - } + // validate StatefulSets from Affinity Assistant + for i, w := range tc.pr.Spec.Workspaces { + expectAAName := getAffinityAssistantName(w.Name, tc.pr.Name) + aa, err := c.KubeClientSet.AppsV1().StatefulSets(testPipelineRun.Namespace).Get(ctx, expectAAName, metav1.GetOptions{}) + if tc.expectStatefulSetSpec != nil { + if err != nil { + t.Fatalf("unexpected error when retrieving StatefulSet: %v", err) + } + + podSpecFilter := cmpopts.IgnoreFields(corev1.PodSpec{}, "Containers", "Affinity") + statefulSetSpecFilter := cmpopts.IgnoreFields(appsv1.StatefulSetSpec{}, "Replicas", "Selector") + podTemplateSpecFilter := cmpopts.IgnoreFields(corev1.PodTemplateSpec{}, "ObjectMeta") + if d := cmp.Diff(tc.expectStatefulSetSpec[i], &aa.Spec, statefulSetSpecFilter, podSpecFilter, podTemplateSpecFilter); d != "" { + t.Errorf("StatefulSetSpec diff: %s", diff.PrintWantGot(d)) + } + } else if !apierrors.IsNotFound(err) { + t.Errorf("unexpected error when retrieving StatefulSet which expects nil: %v", err) + } + } - _, err = c.KubeClientSet.AppsV1().StatefulSets(testPipelineRun.Namespace).Get(ctx, expectedAffinityAssistantName, metav1.GetOptions{}) - if !apierrors.IsNotFound(err) { - t.Errorf("expected a NotFound response, got: %v", err) + // clean up Affinity Assistant + c.cleanupAffinityAssistants(ctx, tc.pr) + if err != nil { + t.Errorf("unexpected error from cleanupAffinityAssistants: %v", err) + } + for _, w := range tc.pr.Spec.Workspaces { + if w.PersistentVolumeClaim == nil && w.VolumeClaimTemplate == nil { + continue + } + + expectAAName := getAffinityAssistantName(w.Name, tc.pr.Name) + _, err = c.KubeClientSet.AppsV1().StatefulSets(tc.pr.Namespace).Get(ctx, expectAAName, metav1.GetOptions{}) + if !apierrors.IsNotFound(err) { + t.Errorf("expected a NotFound response, got: %v", err) + } + } + }) } } @@ -239,7 +356,7 @@ func TestPipelineRunPodTemplatesArePropagatedToAffinityAssistant(t *testing.T) { }, } - stsWithTolerationsAndNodeSelector := affinityAssistantStatefulSet("test-assistant", prWithCustomPodTemplate, "mypvc", "nginx", nil) + stsWithTolerationsAndNodeSelector := affinityAssistantStatefulSet("test-assistant", prWithCustomPodTemplate, []corev1.PersistentVolumeClaim{}, []corev1.PersistentVolumeClaimVolumeSource{}, "nginx", nil) if len(stsWithTolerationsAndNodeSelector.Spec.Template.Spec.Tolerations) != 1 { t.Errorf("expected Tolerations in the StatefulSet") @@ -277,7 +394,7 @@ func TestDefaultPodTemplatesArePropagatedToAffinityAssistant(t *testing.T) { }}, } - stsWithTolerationsAndNodeSelector := affinityAssistantStatefulSet("test-assistant", prWithCustomPodTemplate, "mypvc", "nginx", defaultTpl) + stsWithTolerationsAndNodeSelector := affinityAssistantStatefulSet("test-assistant", prWithCustomPodTemplate, []corev1.PersistentVolumeClaim{}, []corev1.PersistentVolumeClaimVolumeSource{}, "nginx", defaultTpl) if len(stsWithTolerationsAndNodeSelector.Spec.Template.Spec.Tolerations) != 1 { t.Errorf("expected Tolerations in the StatefulSet") @@ -323,7 +440,7 @@ func TestMergedPodTemplatesArePropagatedToAffinityAssistant(t *testing.T) { }}, } - stsWithTolerationsAndNodeSelector := affinityAssistantStatefulSet("test-assistant", prWithCustomPodTemplate, "mypvc", "nginx", defaultTpl) + stsWithTolerationsAndNodeSelector := affinityAssistantStatefulSet("test-assistant", prWithCustomPodTemplate, []corev1.PersistentVolumeClaim{}, []corev1.PersistentVolumeClaimVolumeSource{}, "nginx", defaultTpl) if len(stsWithTolerationsAndNodeSelector.Spec.Template.Spec.Tolerations) != 1 { t.Errorf("expected Tolerations from spec in the StatefulSet") @@ -360,7 +477,7 @@ func TestOnlySelectPodTemplateFieldsArePropagatedToAffinityAssistant(t *testing. }, } - stsWithTolerationsAndNodeSelector := affinityAssistantStatefulSet("test-assistant", prWithCustomPodTemplate, "mypvc", "nginx", nil) + stsWithTolerationsAndNodeSelector := affinityAssistantStatefulSet("test-assistant", prWithCustomPodTemplate, []corev1.PersistentVolumeClaim{}, []corev1.PersistentVolumeClaimVolumeSource{}, "nginx", nil) if len(stsWithTolerationsAndNodeSelector.Spec.Template.Spec.Tolerations) != 1 { t.Errorf("expected Tolerations from spec in the StatefulSet") @@ -380,7 +497,7 @@ func TestThatTheAffinityAssistantIsWithoutNodeSelectorAndTolerations(t *testing. Spec: v1beta1.PipelineRunSpec{}, } - stsWithoutTolerationsAndNodeSelector := affinityAssistantStatefulSet("test-assistant", prWithoutCustomPodTemplate, "mypvc", "nginx", nil) + stsWithoutTolerationsAndNodeSelector := affinityAssistantStatefulSet("test-assistant", prWithoutCustomPodTemplate, []corev1.PersistentVolumeClaim{}, []corev1.PersistentVolumeClaimVolumeSource{}, "nginx", nil) if len(stsWithoutTolerationsAndNodeSelector.Spec.Template.Spec.Tolerations) != 0 { t.Errorf("unexpected Tolerations in the StatefulSet") diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 92d9d139049..8f46247f469 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -596,9 +596,20 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get return controller.NewPermanentError(err) } - if pr.HasVolumeClaimTemplate() { + // Make an attempt to create Affinity Assistant if it does not exist + // if the Affinity Assistant already exists, handle the possibility of assigned node becoming unschedulable by deleting the pod + if !c.isAffinityAssistantDisabled(ctx) { + // create Affinity Assistant (StatefulSet) so that taskRun pods that share workspace PVC achieve Node Affinity + if err = c.createOrUpdateAffinityAssistants(ctx, pr.Spec.Workspaces, pr, pr.Namespace); err != nil { + logger.Errorf("Failed to create affinity assistant StatefulSet for PipelineRun %s: %v", pr.Name, err) + pr.Status.MarkFailed(ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSet, + "Failed to create StatefulSet for PipelineRun %s/%s correctly: %s", + pr.Namespace, pr.Name, err) + return controller.NewPermanentError(err) + } + } else if pr.HasVolumeClaimTemplate() { // create workspace PVC from template - if err = c.pvcHandler.CreatePersistentVolumeClaimsForWorkspaces(ctx, pr.Spec.Workspaces, *kmeta.NewControllerRef(pr), pr.Namespace); err != nil { + if err = c.pvcHandler.CreatePVCsForWorkspacesWithoutAffinityAssistant(ctx, pr.Spec.Workspaces, *kmeta.NewControllerRef(pr), pr.Namespace); err != nil { logger.Errorf("Failed to create PVC for PipelineRun %s: %v", pr.Name, err) pr.Status.MarkFailed(volumeclaim.ReasonCouldntCreateWorkspacePVC, "Failed to create PVC for PipelineRun %s/%s Workspaces correctly: %s", @@ -608,19 +619,6 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get } } - // Make an attempt to create Affinity Assistant if it does not exist - // if the Affinity Assistant already exists, handle the possibility of assigned node becoming unschedulable by deleting the pod - if !c.isAffinityAssistantDisabled(ctx) { - // create Affinity Assistant (StatefulSet) so that taskRun pods that share workspace PVC achieve Node Affinity - if err = c.createOrUpdateAffinityAssistants(ctx, pr.Spec.Workspaces, pr, pr.Namespace); err != nil { - logger.Errorf("Failed to create affinity assistant StatefulSet for PipelineRun %s: %v", pr.Name, err) - pr.Status.MarkFailed(ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSet, - "Failed to create StatefulSet or update affinity assistant replicas for PipelineRun %s/%s correctly: %s", - pr.Namespace, pr.Name, err) - return controller.NewPermanentError(err) - } - } - if pr.Status.FinallyStartTime == nil { if pr.HaveTasksTimedOut(ctx, c.Clock) { tasksToTimeOut := sets.NewString() @@ -868,7 +866,7 @@ func (c *Reconciler) createTaskRun(ctx context.Context, taskRunName string, para var pipelinePVCWorkspaceName string var err error - tr.Spec.Workspaces, pipelinePVCWorkspaceName, err = getTaskrunWorkspaces(ctx, pr, rpt) + tr.Spec.Workspaces, pipelinePVCWorkspaceName, err = c.getTaskrunWorkspaces(ctx, pr, rpt) if err != nil { return nil, err } @@ -917,7 +915,7 @@ func (c *Reconciler) createRunObject(ctx context.Context, runName string, params var pipelinePVCWorkspaceName string var err error var workspaces []v1beta1.WorkspaceBinding - workspaces, pipelinePVCWorkspaceName, err = getTaskrunWorkspaces(ctx, pr, rpt) + workspaces, pipelinePVCWorkspaceName, err = c.getTaskrunWorkspaces(ctx, pr, rpt) if err != nil { return nil, err } @@ -991,7 +989,7 @@ func propagateWorkspaces(rpt *resources.ResolvedPipelineTask) (*resources.Resolv return rpt, nil } -func getTaskrunWorkspaces(ctx context.Context, pr *v1beta1.PipelineRun, rpt *resources.ResolvedPipelineTask) ([]v1beta1.WorkspaceBinding, string, error) { +func (c *Reconciler) getTaskrunWorkspaces(ctx context.Context, pr *v1beta1.PipelineRun, rpt *resources.ResolvedPipelineTask) ([]v1beta1.WorkspaceBinding, string, error) { var err error var workspaces []v1beta1.WorkspaceBinding var pipelinePVCWorkspaceName string @@ -1021,7 +1019,9 @@ func getTaskrunWorkspaces(ctx context.Context, pr *v1beta1.PipelineRun, rpt *res if b.PersistentVolumeClaim != nil || b.VolumeClaimTemplate != nil { pipelinePVCWorkspaceName = pipelineWorkspace } - workspaces = append(workspaces, taskWorkspaceByWorkspaceVolumeSource(b, taskWorkspaceName, pipelineTaskSubPath, *kmeta.NewControllerRef(pr))) + + workspace := c.taskWorkspaceByWorkspaceVolumeSource(ctx, pipelinePVCWorkspaceName, pr.Name, b, taskWorkspaceName, pipelineTaskSubPath, *kmeta.NewControllerRef(pr)) + workspaces = append(workspaces, workspace) } else { workspaceIsOptional := false if rpt.ResolvedTask != nil && rpt.ResolvedTask.TaskSpec != nil { @@ -1052,9 +1052,10 @@ func getTaskrunWorkspaces(ctx context.Context, pr *v1beta1.PipelineRun, rpt *res return workspaces, pipelinePVCWorkspaceName, nil } -// taskWorkspaceByWorkspaceVolumeSource is returning the WorkspaceBinding with the TaskRun specified name. -// If the volume source is a volumeClaimTemplate, the template is applied and passed to TaskRun as a persistentVolumeClaim -func taskWorkspaceByWorkspaceVolumeSource(wb v1beta1.WorkspaceBinding, taskWorkspaceName string, pipelineTaskSubPath string, owner metav1.OwnerReference) v1beta1.WorkspaceBinding { +// taskWorkspaceByWorkspaceVolumeSource returns the WorkspaceBinding to be bound to each TaskRun in the Pipeline Task. +// If the PipelineRun WorkspaceBinding is a volumeClaimTemplate, the returned WorkspaceBinding references a PersistentVolumeClaim created for the PipelineRun WorkspaceBinding based on the PipelineRun as OwnerReference. +// Otherwise, the returned WorkspaceBinding references the same volume as the PipelineRun WorkspaceBinding, with the file path joined with pipelineTaskSubPath as the binding subpath. +func (c *Reconciler) taskWorkspaceByWorkspaceVolumeSource(ctx context.Context, pipelineWorkspaceName string, prName string, wb v1beta1.WorkspaceBinding, taskWorkspaceName string, pipelineTaskSubPath string, owner metav1.OwnerReference) v1beta1.WorkspaceBinding { if wb.VolumeClaimTemplate == nil { binding := *wb.DeepCopy() binding.Name = taskWorkspaceName @@ -1062,14 +1063,18 @@ func taskWorkspaceByWorkspaceVolumeSource(wb v1beta1.WorkspaceBinding, taskWorks return binding } - // apply template binding := v1beta1.WorkspaceBinding{ - SubPath: combinedSubPath(wb.SubPath, pipelineTaskSubPath), - PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ - ClaimName: volumeclaim.GetPersistentVolumeClaimName(wb.VolumeClaimTemplate, wb, owner), - }, + SubPath: combinedSubPath(wb.SubPath, pipelineTaskSubPath), + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{}, } binding.Name = taskWorkspaceName + + if !c.isAffinityAssistantDisabled(ctx) { + binding.PersistentVolumeClaim.ClaimName = getPersistentVolumeClaimNameWithAffinityAssistant(pipelineWorkspaceName, prName, wb, owner) + } else { + binding.PersistentVolumeClaim.ClaimName = volumeclaim.GetPVCNameWithoutAffinityAssistant(wb.VolumeClaimTemplate.Name, wb, owner) + } + return binding } diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 1ca8eb3106d..ac8bc7861f2 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -57,6 +57,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + fakek8s "k8s.io/client-go/kubernetes/fake" ktesting "k8s.io/client-go/testing" "k8s.io/client-go/tools/record" clock "k8s.io/utils/clock/testing" @@ -108,6 +109,7 @@ const ( apiFieldsFeatureFlag = "enable-api-fields" ociBundlesFeatureFlag = "enable-tekton-oci-bundles" maxMatrixCombinationsCountFlag = "default-max-matrix-combinations-count" + disableAffinityAssistantFlag = "disable-affinity-assistant" ) type PipelineRunTest struct { @@ -1218,6 +1220,12 @@ func withMaxMatrixCombinationsCount(cm *corev1.ConfigMap, count int) *corev1.Con return newCM } +func withoutAffinityAssistant(cm *corev1.ConfigMap) *corev1.ConfigMap { + newCM := cm.DeepCopy() + newCM.Data[disableAffinityAssistantFlag] = "true" + return newCM +} + // TestReconcileOnCancelledPipelineRun runs "Reconcile" on a PipelineRun that // has been cancelled. It verifies that reconcile is successful, the pipeline // status updated and events generated. @@ -3979,11 +3987,13 @@ spec: name: myclaim `)} ts := []*v1beta1.Task{simpleHelloWorldTask} + cms := []*corev1.ConfigMap{withoutAffinityAssistant(newFeatureFlagsConfigMap())} d := test.Data{ PipelineRuns: prs, Pipelines: ps, Tasks: ts, + ConfigMaps: cms, } prt := newPipelineRunTest(t, d) defer prt.Cancel() @@ -4007,11 +4017,7 @@ spec: for _, pr := range prs { for _, w := range pr.Spec.Workspaces { - expectedPVCName := volumeclaim.GetPersistentVolumeClaimName(&corev1.PersistentVolumeClaim{ - ObjectMeta: metav1.ObjectMeta{ - Name: w.VolumeClaimTemplate.Name, - }, - }, w, *kmeta.NewControllerRef(pr)) + expectedPVCName := volumeclaim.GetPVCNameWithoutAffinityAssistant(w.VolumeClaimTemplate.Name, w, *kmeta.NewControllerRef(pr)) _, 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) @@ -7385,8 +7391,11 @@ spec: ctx := config.EnableAlphaAPIFields(context.Background()) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + c := Reconciler{ + KubeClientSet: fakek8s.NewSimpleClientset(), + } rprt := &resources.ResolvedPipelineTask{PipelineTask: &tt.pr.Spec.PipelineSpec.Tasks[0]} - _, _, err := getTaskrunWorkspaces(ctx, tt.pr, rprt) + _, _, err := c.getTaskrunWorkspaces(ctx, tt.pr, rprt) if err == nil { t.Errorf("Pipeline.getTaskrunWorkspaces() did not return error for invalid workspace") } else if d := cmp.Diff(tt.expectedError, err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { @@ -7492,7 +7501,10 @@ spec: } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, _, err := getTaskrunWorkspaces(context.Background(), tt.pr, tt.rprt) + c := Reconciler{ + KubeClientSet: fakek8s.NewSimpleClientset(), + } + _, _, err := c.getTaskrunWorkspaces(context.Background(), tt.pr, tt.rprt) if err != nil { t.Errorf("Pipeline.getTaskrunWorkspaces() returned error for valid pipeline: %v", err) @@ -7501,6 +7513,85 @@ spec: } } +func Test_taskWorkspaceByWorkspaceVolumeSource(t *testing.T) { + testPr := &v1beta1.PipelineRun{} + tests := []struct { + name, taskWorkspaceName, pipelineWorkspaceName, prName string + wb v1beta1.WorkspaceBinding + expectedBinding v1beta1.WorkspaceBinding + disableAffinityAssistant bool + }{ + { + name: "PVC Workspace with Affinity Assistant", + prName: "test-pipeline-run", + taskWorkspaceName: "task-workspace", + pipelineWorkspaceName: "pipeline-workspace", + wb: v1beta1.WorkspaceBinding{ + Name: "foo", + VolumeClaimTemplate: &corev1.PersistentVolumeClaim{}, + }, + expectedBinding: v1beta1.WorkspaceBinding{ + Name: "task-workspace", + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc-2c26b46b68-affinity-assistant-e011a5ef79-0", + }, + }, + }, + { + name: "PVC Workspace without Affinity Assistant", + prName: "test-pipeline-run", + taskWorkspaceName: "task-workspace", + wb: v1beta1.WorkspaceBinding{ + Name: "foo", + VolumeClaimTemplate: &corev1.PersistentVolumeClaim{}, + }, + expectedBinding: v1beta1.WorkspaceBinding{ + Name: "task-workspace", + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc-2c26b46b68", + }, + }, + disableAffinityAssistant: true, + }, + { + name: "non-PVC Workspace", + taskWorkspaceName: "task-workspace", + wb: v1beta1.WorkspaceBinding{ + Name: "foo", + EmptyDir: &corev1.EmptyDirVolumeSource{}, + }, + expectedBinding: v1beta1.WorkspaceBinding{ + Name: "task-workspace", + EmptyDir: &corev1.EmptyDirVolumeSource{}, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + c := Reconciler{} + ctx := context.Background() + if tc.disableAffinityAssistant { + featureFlags, err := config.NewFeatureFlagsFromMap(map[string]string{ + "disable-affinity-assistant": "true", + }) + if err != nil { + t.Fatalf("error creating feature flag disable-affinity-assistant from map: %v", err) + } + cfg := &config.Config{ + FeatureFlags: featureFlags, + } + ctx = config.ToContext(context.Background(), cfg) + } + + binding := c.taskWorkspaceByWorkspaceVolumeSource(ctx, tc.pipelineWorkspaceName, tc.prName, tc.wb, tc.taskWorkspaceName, "", *kmeta.NewControllerRef(testPr)) + if d := cmp.Diff(tc.expectedBinding, binding); d != "" { + t.Errorf("WorkspaceBinding diff: %s", diff.PrintWantGot(d)) + } + }) + } +} + func TestReconcile_PropagatePipelineTaskRunSpecMetadata(t *testing.T) { names.TestingSeed() diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 42f6a71db02..1ea5cbb6dcc 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -464,7 +464,7 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1beta1.TaskRun, rtr *re // Please note that this block is required to run before `applyParamsContextsResultsAndWorkspaces` is called the first time, // and that `applyParamsContextsResultsAndWorkspaces` _must_ be called on every reconcile. if pod == nil && tr.HasVolumeClaimTemplate() { - if err := c.pvcHandler.CreatePersistentVolumeClaimsForWorkspaces(ctx, tr.Spec.Workspaces, *kmeta.NewControllerRef(tr), tr.Namespace); err != nil { + if err := c.pvcHandler.CreatePVCsForWorkspacesWithoutAffinityAssistant(ctx, tr.Spec.Workspaces, *kmeta.NewControllerRef(tr), tr.Namespace); err != nil { logger.Errorf("Failed to create PVC for TaskRun %s: %v", tr.Name, err) tr.Status.MarkResourceFailed(volumeclaim.ReasonCouldntCreateWorkspacePVC, fmt.Errorf("Failed to create PVC for TaskRun %s workspaces correctly: %w", @@ -851,7 +851,7 @@ func applyVolumeClaimTemplates(workspaceBindings []v1beta1.WorkspaceBinding, own Name: wb.Name, SubPath: wb.SubPath, PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ - ClaimName: volumeclaim.GetPersistentVolumeClaimName(wb.VolumeClaimTemplate, wb, owner), + ClaimName: volumeclaim.GetPVCNameWithoutAffinityAssistant(wb.VolumeClaimTemplate.Name, wb, owner), }, } taskRunWorkspaceBindings = append(taskRunWorkspaceBindings, b) diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index 1139e790f30..23bc587a9dc 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -3385,11 +3385,7 @@ spec: if w.PersistentVolumeClaim != nil { t.Fatalf("expected workspace from volumeClaimTemplate to be translated to PVC") } - expectedPVCName := volumeclaim.GetPersistentVolumeClaimName(&corev1.PersistentVolumeClaim{ - ObjectMeta: metav1.ObjectMeta{ - Name: w.VolumeClaimTemplate.Name, - }, - }, w, *kmeta.NewControllerRef(ttt)) + expectedPVCName := volumeclaim.GetPVCNameWithoutAffinityAssistant(w.VolumeClaimTemplate.Name, w, *kmeta.NewControllerRef(ttt)) _, 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 384bcb516d1..37f1a7a513c 100644 --- a/pkg/reconciler/volumeclaim/pvchandler.go +++ b/pkg/reconciler/volumeclaim/pvchandler.go @@ -38,7 +38,7 @@ const ( // PvcHandler is used to create PVCs for workspaces type PvcHandler interface { - CreatePersistentVolumeClaimsForWorkspaces(ctx context.Context, wb []v1beta1.WorkspaceBinding, ownerReference metav1.OwnerReference, namespace string) error + CreatePVCsForWorkspacesWithoutAffinityAssistant(ctx context.Context, wb []v1beta1.WorkspaceBinding, ownerReference metav1.OwnerReference, namespace string) error } type defaultPVCHandler struct { @@ -51,13 +51,15 @@ func NewPVCHandler(clientset clientset.Interface, logger *zap.SugaredLogger) Pvc return &defaultPVCHandler{clientset, logger} } -// CreatePersistentVolumeClaimsForWorkspaces checks if a PVC named -- exists; +// CreatePVCsForWorkspacesWithoutAffinityAssistant checks if a PVC named -- exists; // where claim-name is provided by the user in the volumeClaimTemplate, and owner-name is the name of the // resource with the volumeClaimTemplate declared, a PipelineRun or TaskRun. If the PVC did not exist, a new PVC // with that name is created with the provided OwnerReference. -func (c *defaultPVCHandler) CreatePersistentVolumeClaimsForWorkspaces(ctx context.Context, wb []v1beta1.WorkspaceBinding, ownerReference metav1.OwnerReference, namespace string) error { +// This function is only called when Affinity Assistant is disabled. +// When Affinity Assistant is enabled, the PersistentVolumeClaims will be created by the Affinity Assistant StatefulSet VolumeClaimTemplate instead. +func (c *defaultPVCHandler) CreatePVCsForWorkspacesWithoutAffinityAssistant(ctx context.Context, wb []v1beta1.WorkspaceBinding, ownerReference metav1.OwnerReference, namespace string) error { var errs []error - for _, claim := range getPersistentVolumeClaims(wb, ownerReference, namespace) { + for _, claim := range getPVCsWithoutAffinityAssistant(wb, ownerReference, namespace) { _, err := c.clientset.CoreV1().PersistentVolumeClaims(claim.Namespace).Get(ctx, claim.Name, metav1.GetOptions{}) switch { case apierrors.IsNotFound(err): @@ -81,7 +83,7 @@ func (c *defaultPVCHandler) CreatePersistentVolumeClaimsForWorkspaces(ctx contex return errorutils.NewAggregate(errs) } -func getPersistentVolumeClaims(workspaceBindings []v1beta1.WorkspaceBinding, ownerReference metav1.OwnerReference, namespace string) map[string]*corev1.PersistentVolumeClaim { +func getPVCsWithoutAffinityAssistant(workspaceBindings []v1beta1.WorkspaceBinding, ownerReference metav1.OwnerReference, namespace string) map[string]*corev1.PersistentVolumeClaim { claims := make(map[string]*corev1.PersistentVolumeClaim) for _, workspaceBinding := range workspaceBindings { if workspaceBinding.VolumeClaimTemplate == nil { @@ -89,7 +91,7 @@ func getPersistentVolumeClaims(workspaceBindings []v1beta1.WorkspaceBinding, own } claim := workspaceBinding.VolumeClaimTemplate.DeepCopy() - claim.Name = GetPersistentVolumeClaimName(workspaceBinding.VolumeClaimTemplate, workspaceBinding, ownerReference) + claim.Name = GetPVCNameWithoutAffinityAssistant(workspaceBinding.VolumeClaimTemplate.Name, workspaceBinding, ownerReference) claim.Namespace = namespace claim.OwnerReferences = []metav1.OwnerReference{ownerReference} claims[workspaceBinding.Name] = claim @@ -97,16 +99,18 @@ func getPersistentVolumeClaims(workspaceBindings []v1beta1.WorkspaceBinding, own return claims } -// GetPersistentVolumeClaimName gets the name of PersistentVolumeClaim for a Workspace and PipelineRun or TaskRun. claim +// GetPVCNameWithoutAffinityAssistant 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 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 == "" { +// If the given volumeClaimTemplate name is empty, the prefix "pvc" will be applied to the PersistentVolumeClaim name. +// See function `getPersistentVolumeClaimNameWithAffinityAssistant` when the PersistentVolumeClaim is created by Affinity Assistant StatefulSet. +func GetPVCNameWithoutAffinityAssistant(claimName string, wb v1beta1.WorkspaceBinding, owner metav1.OwnerReference) string { + if claimName == "" { return fmt.Sprintf("%s-%s", "pvc", getPersistentVolumeClaimIdentity(wb.Name, string(owner.UID))) } - return fmt.Sprintf("%s-%s", claim.Name, getPersistentVolumeClaimIdentity(wb.Name, string(owner.UID))) + return fmt.Sprintf("%s-%s", claimName, 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 7c4f09cd0a2..a0a4192f80e 100644 --- a/pkg/reconciler/volumeclaim/pvchandler_test.go +++ b/pkg/reconciler/volumeclaim/pvchandler_test.go @@ -83,7 +83,7 @@ func TestCreatePersistentVolumeClaimsForWorkspaces(t *testing.T) { // when - err := pvcHandler.CreatePersistentVolumeClaimsForWorkspaces(ctx, workspaces, ownerRef, namespace) + err := pvcHandler.CreatePVCsForWorkspacesWithoutAffinityAssistant(ctx, workspaces, ownerRef, namespace) if err != nil { t.Fatalf("unexpexted error: %v", err) } @@ -147,7 +147,7 @@ func TestCreatePersistentVolumeClaimsForWorkspacesWithoutMetadata(t *testing.T) // when - err := pvcHandler.CreatePersistentVolumeClaimsForWorkspaces(ctx, workspaces, ownerRef, namespace) + err := pvcHandler.CreatePVCsForWorkspacesWithoutAffinityAssistant(ctx, workspaces, ownerRef, namespace) if err != nil { t.Fatalf("unexpexted error: %v", err) } @@ -186,7 +186,7 @@ func TestCreateExistPersistentVolumeClaims(t *testing.T) { fakekubeclient := fakek8s.NewSimpleClientset() pvcHandler := defaultPVCHandler{fakekubeclient, zap.NewExample().Sugar()} - for _, claim := range getPersistentVolumeClaims(workspaces, ownerRef, namespace) { + for _, claim := range getPVCsWithoutAffinityAssistant(workspaces, ownerRef, namespace) { _, err := fakekubeclient.CoreV1().PersistentVolumeClaims(namespace).Create(ctx, claim, metav1.CreateOptions{}) if err != nil { t.Fatalf("unexpected error: %v", err) @@ -205,7 +205,7 @@ func TestCreateExistPersistentVolumeClaims(t *testing.T) { } fakekubeclient.Fake.PrependReactor(actionGet, "*", fn) - err := pvcHandler.CreatePersistentVolumeClaimsForWorkspaces(ctx, workspaces, ownerRef, namespace) + err := pvcHandler.CreatePVCsForWorkspacesWithoutAffinityAssistant(ctx, workspaces, ownerRef, namespace) if err != nil { t.Fatalf("unexpected error: %v", err) }