From e50a7fbc3e68d02dd075c5a8d37258c05721904b Mon Sep 17 00:00:00 2001 From: Quan Zhang Date: Tue, 11 Jul 2023 12:05:53 -0400 Subject: [PATCH] [TEP-0135] Refactor CreatePVCsForWorkspaces Part of [#6740][#6740] and closes [#6915]. Prior to this commit, the `createOrUpdateAffinityAssistantsAndPVCs` function attempts to create all Affinity Assistant StatefulSets and returns aggregated errors. This could result in time and resource waste when executing a pipelinerun that will fail. This commit updates it to "fail fast" strategy where the function is returned as soon as the first error is encountered. This commit also refactors the original `CreatePVCsForWorkspacesWithoutAffinityAssistant` (renamed to `CreatePVCFromVolumeClaimTemplate`) function and its usages to improve readability since the PVC creation logic is now dependent on `AffinityAssistantBehavior`. /kind feature [#6740]: #6740 [#6915]: https://github.com/tektoncd/pipeline/issues/6915 --- .../pipelinerun/affinity_assistant.go | 93 ++++++++++--------- .../pipelinerun/affinity_assistant_test.go | 93 ++++++++++++++++--- pkg/reconciler/pipelinerun/pipelinerun.go | 22 +++-- .../pipelinerun/pipelinerun_test.go | 2 +- pkg/reconciler/taskrun/taskrun.go | 16 ++-- pkg/reconciler/taskrun/taskrun_test.go | 2 +- pkg/reconciler/volumeclaim/pvchandler.go | 69 +++++++------- pkg/reconciler/volumeclaim/pvchandler_test.go | 31 ++++--- 8 files changed, 209 insertions(+), 119 deletions(-) diff --git a/pkg/reconciler/pipelinerun/affinity_assistant.go b/pkg/reconciler/pipelinerun/affinity_assistant.go index 9e2c1efe8b5..b7a16b319bd 100644 --- a/pkg/reconciler/pipelinerun/affinity_assistant.go +++ b/pkg/reconciler/pipelinerun/affinity_assistant.go @@ -19,6 +19,7 @@ package pipelinerun import ( "context" "crypto/sha256" + "errors" "fmt" "github.com/tektoncd/pipeline/pkg/apis/config" @@ -40,13 +41,18 @@ import ( ) const ( - // ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSetPerWorkspace indicates that a PipelineRun uses workspaces with PersistentVolumeClaim + // ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSet indicates that a PipelineRun uses workspaces with PersistentVolumeClaim // as a volume source and expect an Assistant StatefulSet in AffinityAssistantPerWorkspace behavior, but couldn't create a StatefulSet. - ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSetPerWorkspace = "ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSetPerWorkspace" + ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSet = "ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSet" featureFlagDisableAffinityAssistantKey = "disable-affinity-assistant" ) +var ( + ErrPvcCreationFailed = errors.New("PVC creation error") + ErrAffinityAssistantCreationFailed = errors.New("Affinity Assistant creation error") +) + // createOrUpdateAffinityAssistantsAndPVCs creates Affinity Assistant StatefulSets and PVCs based on AffinityAssistantBehavior. // This is done to achieve Node Affinity for taskruns in a pipelinerun, and make it possible for the taskruns to execute parallel while sharing volume. // If the AffinityAssistantBehavior is AffinityAssistantPerWorkspace, it creates an Affinity Assistant for @@ -54,72 +60,73 @@ const ( // If the AffinityAssistantBehavior is AffinityAssistantPerPipelineRun or AffinityAssistantPerPipelineRunWithIsolation, // it creates one Affinity Assistant for the pipelinerun. func (c *Reconciler) createOrUpdateAffinityAssistantsAndPVCs(ctx context.Context, pr *v1.PipelineRun, aaBehavior aa.AffinityAssistantBehavior) error { - var errs []error var unschedulableNodes sets.Set[string] = nil var claimTemplates []corev1.PersistentVolumeClaim - var claims []corev1.PersistentVolumeClaimVolumeSource - claimToWorkspace := map[*corev1.PersistentVolumeClaimVolumeSource]string{} - claimTemplatesToWorkspace := map[*corev1.PersistentVolumeClaim]string{} + var claimNames []string + claimNameToWorkspaceName := map[string]string{} + claimTemplateToWorkspace := map[*corev1.PersistentVolumeClaim]v1.WorkspaceBinding{} for _, w := range pr.Spec.Workspaces { if w.PersistentVolumeClaim == nil && w.VolumeClaimTemplate == nil { continue } if w.PersistentVolumeClaim != nil { - claim := w.PersistentVolumeClaim.DeepCopy() - claims = append(claims, *claim) - claimToWorkspace[claim] = w.Name + claim := w.PersistentVolumeClaim + claimNames = append(claimNames, claim.ClaimName) + claimNameToWorkspaceName[claim.ClaimName] = w.Name } else if w.VolumeClaimTemplate != nil { claimTemplate := w.VolumeClaimTemplate.DeepCopy() - claimTemplate.Name = volumeclaim.GetPVCNameWithoutAffinityAssistant(w.VolumeClaimTemplate.Name, w, *kmeta.NewControllerRef(pr)) + claimTemplate.Name = volumeclaim.GeneratePVCNameFromWorkspaceBinding(w.VolumeClaimTemplate.Name, w, *kmeta.NewControllerRef(pr)) claimTemplates = append(claimTemplates, *claimTemplate) - claimTemplatesToWorkspace[claimTemplate] = w.Name - } - } - - // create PVCs from PipelineRun's VolumeClaimTemplate when aaBehavior is AffinityAssistantPerWorkspace or AffinityAssistantDisabled before creating - // affinity assistant so that the OwnerReference of the PVCs are the pipelineruns, which is used to achieve PVC auto deletion at PipelineRun deletion time - if (aaBehavior == aa.AffinityAssistantPerWorkspace || aaBehavior == aa.AffinityAssistantDisabled) && pr.HasVolumeClaimTemplate() { - if err := c.pvcHandler.CreatePVCsForWorkspaces(ctx, pr.Spec.Workspaces, *kmeta.NewControllerRef(pr), pr.Namespace); err != nil { - return fmt.Errorf("failed to create PVC for PipelineRun %s: %w", pr.Name, err) + claimTemplateToWorkspace[claimTemplate] = w } } switch aaBehavior { case aa.AffinityAssistantPerWorkspace: - for claim, workspaceName := range claimToWorkspace { + for claimName, workspaceName := range claimNameToWorkspaceName { aaName := GetAffinityAssistantName(workspaceName, pr.Name) - err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, nil, []corev1.PersistentVolumeClaimVolumeSource{*claim}, unschedulableNodes) - errs = append(errs, err...) + if err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, nil, []string{claimName}, unschedulableNodes); err != nil { + return fmt.Errorf("%w: %v", ErrAffinityAssistantCreationFailed, err) + } } - for claimTemplate, workspaceName := range claimTemplatesToWorkspace { - aaName := GetAffinityAssistantName(workspaceName, pr.Name) - // To support PVC auto deletion at pipelinerun deletion time, the OwnerReference of the PVCs should be set to the owning pipelinerun. - // In AffinityAssistantPerWorkspace mode, the reconciler has created PVCs (owned by pipelinerun) from pipelinerun's VolumeClaimTemplate at this point, - // so the VolumeClaimTemplates are pass in as PVCs when creating affinity assistant StatefulSet for volume scheduling. - // If passed in as VolumeClaimTemplates, the PVCs are owned by Affinity Assistant StatefulSet instead of the pipelinerun. - err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, nil, []corev1.PersistentVolumeClaimVolumeSource{{ClaimName: claimTemplate.Name}}, unschedulableNodes) - errs = append(errs, err...) + for claimTemplate, workspace := range claimTemplateToWorkspace { + // To support PVC auto deletion at pipelinerun deletion time, the OwnerReference of the PVCs should be set to the owning pipelinerun instead of the StatefulSets, + // so we create PVCs from PipelineRuns' VolumeClaimTemplate and pass the PVCs to the Affinity Assistant StatefulSet for volume scheduling. + if err := c.pvcHandler.CreatePVCFromVolumeClaimTemplate(ctx, workspace, *kmeta.NewControllerRef(pr), pr.Namespace); err != nil { + return fmt.Errorf("%w: %v", ErrPvcCreationFailed, err) //nolint:errorlint + } + aaName := GetAffinityAssistantName(workspace.Name, pr.Name) + if err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, nil, []string{claimTemplate.Name}, unschedulableNodes); err != nil { + return fmt.Errorf("%w: %v", ErrAffinityAssistantCreationFailed, err) + } } case aa.AffinityAssistantPerPipelineRun, aa.AffinityAssistantPerPipelineRunWithIsolation: - if claims != nil || claimTemplates != nil { + if claimNames != nil || claimTemplates != nil { aaName := GetAffinityAssistantName("", pr.Name) - // In AffinityAssistantPerPipelineRun or AffinityAssistantPerPipelineRunWithIsolation modes, the PVCs are created via StatefulSet for volume scheduling. - // PVCs from pipelinerun's VolumeClaimTemplate are enforced to be deleted at pipelinerun completion time, - // so we don't need to worry the OwnerReference of the PVCs - err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, claimTemplates, claims, unschedulableNodes) - errs = append(errs, err...) + // The PVCs are created via StatefulSet's VolumeClaimTemplate for volume scheduling + // in AffinityAssistantPerPipelineRun or AffinityAssistantPerPipelineRunWithIsolation modes. + // This is because PVCs from pipelinerun's VolumeClaimTemplate are enforced to be deleted at pipelinerun completion time in these modes, + // and there is no requirement of the PVC OwnerReference. + if err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, claimTemplates, claimNames, unschedulableNodes); err != nil { + return fmt.Errorf("%w: %v", ErrAffinityAssistantCreationFailed, err) + } } case aa.AffinityAssistantDisabled: + for _, workspace := range claimTemplateToWorkspace { + if err := c.pvcHandler.CreatePVCFromVolumeClaimTemplate(ctx, workspace, *kmeta.NewControllerRef(pr), pr.Namespace); err != nil { + return fmt.Errorf("%w: %v", ErrPvcCreationFailed, err) //nolint:errorlint + } + } } - return errorutils.NewAggregate(errs) + return nil } // createOrUpdateAffinityAssistant creates an Affinity Assistant Statefulset with the provided affinityAssistantName and pipelinerun information. // The VolumeClaimTemplates and Volumes of StatefulSet reference the resolved claimTemplates and claims respectively. // It maintains a set of unschedulableNodes to detect and recreate Affinity Assistant in case of the node is cordoned to avoid pipelinerun deadlock. -func (c *Reconciler) createOrUpdateAffinityAssistant(ctx context.Context, affinityAssistantName string, pr *v1.PipelineRun, claimTemplates []corev1.PersistentVolumeClaim, claims []corev1.PersistentVolumeClaimVolumeSource, unschedulableNodes sets.Set[string]) []error { +func (c *Reconciler) createOrUpdateAffinityAssistant(ctx context.Context, affinityAssistantName string, pr *v1.PipelineRun, claimTemplates []corev1.PersistentVolumeClaim, claimNames []string, unschedulableNodes sets.Set[string]) []error { logger := logging.FromContext(ctx) cfg := config.FromContextOrDefaults(ctx) @@ -132,7 +139,7 @@ func (c *Reconciler) createOrUpdateAffinityAssistant(ctx context.Context, affini if err != nil { return []error{err} } - affinityAssistantStatefulSet := affinityAssistantStatefulSet(aaBehavior, affinityAssistantName, pr, claimTemplates, claims, c.Images.NopImage, cfg.Defaults.DefaultAAPodTemplate) + affinityAssistantStatefulSet := affinityAssistantStatefulSet(aaBehavior, affinityAssistantName, pr, claimTemplates, claimNames, c.Images.NopImage, cfg.Defaults.DefaultAAPodTemplate) _, err = c.KubeClientSet.AppsV1().StatefulSets(pr.Namespace).Create(ctx, affinityAssistantStatefulSet, metav1.CreateOptions{}) if err != nil { errs = append(errs, fmt.Errorf("failed to create StatefulSet %s: %w", affinityAssistantName, err)) @@ -227,7 +234,7 @@ func (c *Reconciler) cleanupAffinityAssistantsAndPVCs(ctx context.Context, pr *v // The PVCs created by StatefulSet VolumeClaimTemplates follow the format `--0` // TODO(#6740)(WIP): use this function when adding end-to-end support for AffinityAssistantPerPipelineRun mode func getPersistentVolumeClaimNameWithAffinityAssistant(pipelineWorkspaceName, prName string, wb v1.WorkspaceBinding, owner metav1.OwnerReference) string { - pvcName := volumeclaim.GetPVCNameWithoutAffinityAssistant(wb.VolumeClaimTemplate.Name, wb, owner) + pvcName := volumeclaim.GeneratePVCNameFromWorkspaceBinding(wb.VolumeClaimTemplate.Name, wb, owner) affinityAssistantName := GetAffinityAssistantName(pipelineWorkspaceName, prName) return fmt.Sprintf("%s-%s-0", pvcName, affinityAssistantName) } @@ -258,7 +265,7 @@ func getStatefulSetLabels(pr *v1.PipelineRun, affinityAssistantName string) map[ // 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(aaBehavior aa.AffinityAssistantBehavior, name string, pr *v1.PipelineRun, claimTemplates []corev1.PersistentVolumeClaim, claims []corev1.PersistentVolumeClaimVolumeSource, affinityAssistantImage string, defaultAATpl *pod.AffinityAssistantTemplate) *appsv1.StatefulSet { +func affinityAssistantStatefulSet(aaBehavior aa.AffinityAssistantBehavior, name string, pr *v1.PipelineRun, claimTemplates []corev1.PersistentVolumeClaim, claimNames []string, affinityAssistantImage string, defaultAATpl *pod.AffinityAssistantTemplate) *appsv1.StatefulSet { // We want a singleton pod replicas := int32(1) @@ -295,7 +302,7 @@ func affinityAssistantStatefulSet(aaBehavior aa.AffinityAssistantBehavior, name }} var volumes []corev1.Volume - for i, claim := range claims { + for i, claimName := range claimNames { volumes = append(volumes, corev1.Volume{ Name: fmt.Sprintf("workspace-%d", i), VolumeSource: corev1.VolumeSource{ @@ -307,7 +314,7 @@ func affinityAssistantStatefulSet(aaBehavior aa.AffinityAssistantBehavior, name // 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(), + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ClaimName: claimName}, }, }) } diff --git a/pkg/reconciler/pipelinerun/affinity_assistant_test.go b/pkg/reconciler/pipelinerun/affinity_assistant_test.go index 6dea45031a8..020357850bb 100644 --- a/pkg/reconciler/pipelinerun/affinity_assistant_test.go +++ b/pkg/reconciler/pipelinerun/affinity_assistant_test.go @@ -19,6 +19,7 @@ package pipelinerun import ( "context" "errors" + "fmt" "testing" "github.com/google/go-cmp/cmp" @@ -108,9 +109,9 @@ var testPRWithEmptyDir = &v1.PipelineRun{ }, } -// TestCreateAndDeleteOfAffinityAssistantPerPipelineRun tests to create and delete an Affinity Assistant +// TestCreateOrUpdateAffinityAssistantsAndPVCsPerPipelineRun tests to create and delete Affinity Assistants and PVCs // per pipelinerun for a given PipelineRun -func TestCreateAndDeleteOfAffinityAssistantPerPipelineRun(t *testing.T) { +func TestCreateOrUpdateAffinityAssistantsAndPVCsPerPipelineRun(t *testing.T) { tests := []struct { name string pr *v1.PipelineRun @@ -183,9 +184,9 @@ func TestCreateAndDeleteOfAffinityAssistantPerPipelineRun(t *testing.T) { } } -// TestCreateAndDeleteOfAffinityAssistantPerWorkspaceOrDisabled tests to create and delete an Affinity Assistant -// per workspace for a given PipelineRun -func TestCreateAndDeleteOfAffinityAssistantPerWorkspaceOrDisabled(t *testing.T) { +// TestCreateOrUpdateAffinityAssistantsAndPVCsPerWorkspaceOrDisabled tests to create and delete Affinity Assistants and PVCs +// per workspace or disabled for a given PipelineRun +func TestCreateOrUpdateAffinityAssistantsAndPVCsPerWorkspaceOrDisabled(t *testing.T) { tests := []struct { name, expectedPVCName string pr *v1.PipelineRun @@ -275,7 +276,7 @@ func TestCreateAndDeleteOfAffinityAssistantPerWorkspaceOrDisabled(t *testing.T) err := c.createOrUpdateAffinityAssistantsAndPVCs(ctx, tc.pr, tc.aaBehavior) if err != nil { - t.Fatalf("unexpected error from createOrUpdateAffinityAssistantsPerWorkspace: %v", err) + t.Fatalf("unexpected error from createOrUpdateAffinityAssistantsAndPVCs: %v", err) } // validate StatefulSets from Affinity Assistant @@ -314,6 +315,76 @@ func TestCreateAndDeleteOfAffinityAssistantPerWorkspaceOrDisabled(t *testing.T) } } +func TestCreateOrUpdateAffinityAssistantsAndPVCs_Failure(t *testing.T) { + testCases := []struct { + name, failureType string + aaBehavior aa.AffinityAssistantBehavior + expectedErr error + }{{ + name: "affinity assistant creation failed - per workspace", + failureType: "statefulset", + aaBehavior: aa.AffinityAssistantPerWorkspace, + expectedErr: fmt.Errorf("%w: [failed to create StatefulSet affinity-assistant-4cf1a1c468: error creating statefulsets]", ErrAffinityAssistantCreationFailed), + }, { + name: "affinity assistant creation failed - per pipelinerun", + failureType: "statefulset", + aaBehavior: aa.AffinityAssistantPerPipelineRun, + expectedErr: fmt.Errorf("%w: [failed to create StatefulSet affinity-assistant-426b306c50: error creating statefulsets]", ErrAffinityAssistantCreationFailed), + }, { + name: "pvc creation failed - per workspace", + failureType: "pvc", + aaBehavior: aa.AffinityAssistantPerWorkspace, + expectedErr: fmt.Errorf("%w: failed to create PVC pvc-b9eea16dce: error creating persistentvolumeclaims", ErrPvcCreationFailed), + }, { + name: "pvc creation failed - disabled", + failureType: "pvc", + aaBehavior: aa.AffinityAssistantDisabled, + expectedErr: fmt.Errorf("%w: failed to create PVC pvc-b9eea16dce: error creating persistentvolumeclaims", ErrPvcCreationFailed), + }} + + for _, tc := range testCases { + ctx := context.Background() + kubeClientSet := fakek8s.NewSimpleClientset() + c := Reconciler{ + KubeClientSet: kubeClientSet, + pvcHandler: volumeclaim.NewPVCHandler(kubeClientSet, zap.NewExample().Sugar()), + } + + switch tc.failureType { + case "pvc": + c.KubeClientSet.CoreV1().(*fake.FakeCoreV1).PrependReactor("create", "persistentvolumeclaims", + func(action testing2.Action) (handled bool, ret runtime.Object, err error) { + return true, &corev1.PersistentVolumeClaim{}, errors.New("error creating persistentvolumeclaims") + }) + case "statefulset": + c.KubeClientSet.CoreV1().(*fake.FakeCoreV1).PrependReactor("create", "statefulsets", + func(action testing2.Action) (handled bool, ret runtime.Object, err error) { + return true, &appsv1.StatefulSet{}, errors.New("error creating statefulsets") + }) + } + + err := c.createOrUpdateAffinityAssistantsAndPVCs(ctx, testPRWithVolumeClaimTemplate, tc.aaBehavior) + + if err == nil { + t.Errorf("expect error from createOrUpdateAffinityAssistantsAndPVCs but got nil") + } + + switch tc.failureType { + case "pvc": + if !errors.Is(err, ErrPvcCreationFailed) { + t.Errorf("expected err type mismatching, expecting %v but got: %v", ErrPvcCreationFailed, err) + } + case "statefulset": + if !errors.Is(err, ErrAffinityAssistantCreationFailed) { + t.Errorf("expected err type mismatching, expecting %v but got: %v", ErrAffinityAssistantCreationFailed, err) + } + } + if d := cmp.Diff(tc.expectedErr.Error(), err.Error()); d != "" { + t.Errorf("expected err mismatching: %v", diff.PrintWantGot(d)) + } + } +} + // TestCreateAffinityAssistantWhenNodeIsCordoned tests an existing Affinity Assistant can identify the node failure and // can migrate the affinity assistant pod to a healthy node so that the existing pipelineRun runs to competition func TestCreateOrUpdateAffinityAssistantWhenNodeIsCordoned(t *testing.T) { @@ -461,7 +532,7 @@ func TestPipelineRunPodTemplatesArePropagatedToAffinityAssistant(t *testing.T) { }, } - stsWithTolerationsAndNodeSelector := affinityAssistantStatefulSet(aa.AffinityAssistantPerWorkspace, "test-assistant", prWithCustomPodTemplate, []corev1.PersistentVolumeClaim{}, []corev1.PersistentVolumeClaimVolumeSource{}, "nginx", nil) + stsWithTolerationsAndNodeSelector := affinityAssistantStatefulSet(aa.AffinityAssistantPerWorkspace, "test-assistant", prWithCustomPodTemplate, []corev1.PersistentVolumeClaim{}, []string{}, "nginx", nil) if len(stsWithTolerationsAndNodeSelector.Spec.Template.Spec.Tolerations) != 1 { t.Errorf("expected Tolerations in the StatefulSet") @@ -499,7 +570,7 @@ func TestDefaultPodTemplatesArePropagatedToAffinityAssistant(t *testing.T) { }}, } - stsWithTolerationsAndNodeSelector := affinityAssistantStatefulSet(aa.AffinityAssistantPerWorkspace, "test-assistant", prWithCustomPodTemplate, []corev1.PersistentVolumeClaim{}, []corev1.PersistentVolumeClaimVolumeSource{}, "nginx", defaultTpl) + stsWithTolerationsAndNodeSelector := affinityAssistantStatefulSet(aa.AffinityAssistantPerWorkspace, "test-assistant", prWithCustomPodTemplate, []corev1.PersistentVolumeClaim{}, []string{}, "nginx", defaultTpl) if len(stsWithTolerationsAndNodeSelector.Spec.Template.Spec.Tolerations) != 1 { t.Errorf("expected Tolerations in the StatefulSet") @@ -546,7 +617,7 @@ func TestMergedPodTemplatesArePropagatedToAffinityAssistant(t *testing.T) { }}, } - stsWithTolerationsAndNodeSelector := affinityAssistantStatefulSet(aa.AffinityAssistantPerWorkspace, "test-assistant", prWithCustomPodTemplate, []corev1.PersistentVolumeClaim{}, []corev1.PersistentVolumeClaimVolumeSource{}, "nginx", defaultTpl) + stsWithTolerationsAndNodeSelector := affinityAssistantStatefulSet(aa.AffinityAssistantPerWorkspace, "test-assistant", prWithCustomPodTemplate, []corev1.PersistentVolumeClaim{}, []string{}, "nginx", defaultTpl) if len(stsWithTolerationsAndNodeSelector.Spec.Template.Spec.Tolerations) != 1 { t.Errorf("expected Tolerations from spec in the StatefulSet") @@ -584,7 +655,7 @@ func TestOnlySelectPodTemplateFieldsArePropagatedToAffinityAssistant(t *testing. }, } - stsWithTolerationsAndNodeSelector := affinityAssistantStatefulSet(aa.AffinityAssistantPerWorkspace, "test-assistant", prWithCustomPodTemplate, []corev1.PersistentVolumeClaim{}, []corev1.PersistentVolumeClaimVolumeSource{}, "nginx", nil) + stsWithTolerationsAndNodeSelector := affinityAssistantStatefulSet(aa.AffinityAssistantPerWorkspace, "test-assistant", prWithCustomPodTemplate, []corev1.PersistentVolumeClaim{}, []string{}, "nginx", nil) if len(stsWithTolerationsAndNodeSelector.Spec.Template.Spec.Tolerations) != 1 { t.Errorf("expected Tolerations from spec in the StatefulSet") @@ -604,7 +675,7 @@ func TestThatTheAffinityAssistantIsWithoutNodeSelectorAndTolerations(t *testing. Spec: v1.PipelineRunSpec{}, } - stsWithoutTolerationsAndNodeSelector := affinityAssistantStatefulSet(aa.AffinityAssistantPerWorkspace, "test-assistant", prWithoutCustomPodTemplate, []corev1.PersistentVolumeClaim{}, []corev1.PersistentVolumeClaimVolumeSource{}, "nginx", nil) + stsWithoutTolerationsAndNodeSelector := affinityAssistantStatefulSet(aa.AffinityAssistantPerWorkspace, "test-assistant", prWithoutCustomPodTemplate, []corev1.PersistentVolumeClaim{}, []string{}, "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 f022e6efd87..b2435daabe9 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -620,18 +620,20 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel switch aaBehavior { case affinityassistant.AffinityAssistantPerWorkspace, affinityassistant.AffinityAssistantDisabled: - if err = c.createOrUpdateAffinityAssistantsAndPVCs(ctx, pr, aaBehavior); err != nil { - logger.Errorf("Failed to create PVC or affinity assistant StatefulSet for PipelineRun %s: %v", pr.Name, err) - if aaBehavior == affinityassistant.AffinityAssistantPerWorkspace { - pr.Status.MarkFailed(ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSetPerWorkspace, - "Failed to create StatefulSet for PipelineRun %s/%s correctly: %s", - pr.Namespace, pr.Name, err) - } else { + if err := c.createOrUpdateAffinityAssistantsAndPVCs(ctx, pr, aaBehavior); err != nil { + switch { + case errors.Is(err, ErrPvcCreationFailed): + 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", + "Failed to create PVC for PipelineRun %s/%s correctly: %s", pr.Namespace, pr.Name, err) + case errors.Is(err, ErrAffinityAssistantCreationFailed): + 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) + default: } - return controller.NewPermanentError(err) } case affinityassistant.AffinityAssistantPerPipelineRun, affinityassistant.AffinityAssistantPerPipelineRunWithIsolation: @@ -1125,7 +1127,7 @@ func (c *Reconciler) taskWorkspaceByWorkspaceVolumeSource(ctx context.Context, p // TODO(#6740)(WIP): get binding for AffinityAssistantPerPipelineRun and AffinityAssistantPerPipelineRunWithIsolation mode if aaBehavior == affinityassistant.AffinityAssistantDisabled || aaBehavior == affinityassistant.AffinityAssistantPerWorkspace { - binding.PersistentVolumeClaim.ClaimName = volumeclaim.GetPVCNameWithoutAffinityAssistant(wb.VolumeClaimTemplate.Name, wb, owner) + binding.PersistentVolumeClaim.ClaimName = volumeclaim.GeneratePVCNameFromWorkspaceBinding(wb.VolumeClaimTemplate.Name, wb, owner) } return binding diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 26576e3152f..556ddfa81ab 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -4229,7 +4229,7 @@ spec: for _, pr := range prs { for _, w := range pr.Spec.Workspaces { - expectedPVCName := volumeclaim.GetPVCNameWithoutAffinityAssistant(w.VolumeClaimTemplate.Name, w, *kmeta.NewControllerRef(pr)) + expectedPVCName := volumeclaim.GeneratePVCNameFromWorkspaceBinding(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) diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 695e72d6d13..040ca08fb3f 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -484,12 +484,14 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1.TaskRun, rtr *resourc // 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.CreatePVCsForWorkspaces(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", - fmt.Sprintf("%s/%s", tr.Namespace, tr.Name), err)) - return controller.NewPermanentError(err) + for _, ws := range tr.Spec.Workspaces { + if err := c.pvcHandler.CreatePVCFromVolumeClaimTemplate(ctx, ws, *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", + fmt.Sprintf("%s/%s", tr.Namespace, tr.Name), err)) + return controller.NewPermanentError(err) + } } taskRunWorkspaces := applyVolumeClaimTemplates(tr.Spec.Workspaces, *kmeta.NewControllerRef(tr)) @@ -871,7 +873,7 @@ func applyVolumeClaimTemplates(workspaceBindings []v1.WorkspaceBinding, owner me Name: wb.Name, SubPath: wb.SubPath, PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ - ClaimName: volumeclaim.GetPVCNameWithoutAffinityAssistant(wb.VolumeClaimTemplate.Name, wb, owner), + ClaimName: volumeclaim.GeneratePVCNameFromWorkspaceBinding(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 de4a14c5836..f92b997acba 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -3398,7 +3398,7 @@ spec: if w.PersistentVolumeClaim != nil { t.Fatalf("expected workspace from volumeClaimTemplate to be translated to PVC") } - expectedPVCName := volumeclaim.GetPVCNameWithoutAffinityAssistant(w.VolumeClaimTemplate.Name, w, *kmeta.NewControllerRef(ttt)) + expectedPVCName := volumeclaim.GeneratePVCNameFromWorkspaceBinding(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 5f6f54e380a..12a8aed4b43 100644 --- a/pkg/reconciler/volumeclaim/pvchandler.go +++ b/pkg/reconciler/volumeclaim/pvchandler.go @@ -29,7 +29,6 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - errorutils "k8s.io/apimachinery/pkg/util/errors" clientset "k8s.io/client-go/kubernetes" ) @@ -41,7 +40,7 @@ const ( // PvcHandler is used to create PVCs for workspaces type PvcHandler interface { - CreatePVCsForWorkspaces(ctx context.Context, wb []v1.WorkspaceBinding, ownerReference metav1.OwnerReference, namespace string) error + CreatePVCFromVolumeClaimTemplate(ctx context.Context, wb v1.WorkspaceBinding, ownerReference metav1.OwnerReference, namespace string) error PurgeFinalizerAndDeletePVCForWorkspace(ctx context.Context, pvcName, namespace string) error } @@ -55,34 +54,35 @@ func NewPVCHandler(clientset clientset.Interface, logger *zap.SugaredLogger) Pvc return &defaultPVCHandler{clientset, logger} } -// CreatePVCsForWorkspaces checks if a PVC named -- exists; +// CreatePVCFromVolumeClaimTemplate 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) CreatePVCsForWorkspaces(ctx context.Context, wb []v1.WorkspaceBinding, ownerReference metav1.OwnerReference, namespace string) error { - var errs []error - 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): - _, err := c.clientset.CoreV1().PersistentVolumeClaims(claim.Namespace).Create(ctx, claim, metav1.CreateOptions{}) - if err != nil && !apierrors.IsAlreadyExists(err) { - errs = append(errs, fmt.Errorf("failed to create PVC %s: %w", claim.Name, err)) - } +func (c *defaultPVCHandler) CreatePVCFromVolumeClaimTemplate(ctx context.Context, wb v1.WorkspaceBinding, ownerReference metav1.OwnerReference, namespace string) error { + claim := c.getPVCFromVolumeClaimTemplate(wb, ownerReference, namespace) + if claim == nil { + return nil + } + _, err := c.clientset.CoreV1().PersistentVolumeClaims(claim.Namespace).Get(ctx, claim.Name, metav1.GetOptions{}) + switch { + case apierrors.IsNotFound(err): + _, err := c.clientset.CoreV1().PersistentVolumeClaims(claim.Namespace).Create(ctx, claim, metav1.CreateOptions{}) + if err != nil { if apierrors.IsAlreadyExists(err) { c.logger.Infof("Tried to create PersistentVolumeClaim %s in namespace %s, but it already exists", claim.Name, claim.Namespace) + } else { + return fmt.Errorf("failed to create PVC %s: %w", claim.Name, err) } - - if err == nil { - c.logger.Infof("Created PersistentVolumeClaim %s in namespace %s", claim.Name, claim.Namespace) - } - case err != nil: - errs = append(errs, fmt.Errorf("failed to retrieve PVC %s: %w", claim.Name, err)) + } else { + c.logger.Infof("Created PersistentVolumeClaim %s in namespace %s", claim.Name, claim.Namespace) } + case err != nil: + return fmt.Errorf("failed to retrieve PVC %s: %w", claim.Name, err) } - return errorutils.NewAggregate(errs) + + return nil } // PurgeFinalizerAndDeletePVCForWorkspace purges the `kubernetes.io/pvc-protection` finalizer protection of the given pvc and then deletes it. @@ -128,30 +128,29 @@ func (c *defaultPVCHandler) PurgeFinalizerAndDeletePVCForWorkspace(ctx context.C return nil } -func getPVCsWithoutAffinityAssistant(workspaceBindings []v1.WorkspaceBinding, ownerReference metav1.OwnerReference, namespace string) map[string]*corev1.PersistentVolumeClaim { - claims := make(map[string]*corev1.PersistentVolumeClaim) - for _, workspaceBinding := range workspaceBindings { - if workspaceBinding.VolumeClaimTemplate == nil { - continue - } - - claim := workspaceBinding.VolumeClaimTemplate.DeepCopy() - claim.Name = GetPVCNameWithoutAffinityAssistant(workspaceBinding.VolumeClaimTemplate.Name, workspaceBinding, ownerReference) - claim.Namespace = namespace - claim.OwnerReferences = []metav1.OwnerReference{ownerReference} - claims[workspaceBinding.Name] = claim +// getPVCFromVolumeClaimTemplate returns a PersistentVolumeClaim based on given workspaceBinding (using VolumeClaimTemplate), ownerReference and namespace +func (c *defaultPVCHandler) getPVCFromVolumeClaimTemplate(workspaceBinding v1.WorkspaceBinding, ownerReference metav1.OwnerReference, namespace string) *corev1.PersistentVolumeClaim { + if workspaceBinding.VolumeClaimTemplate == nil { + c.logger.Infof("workspace binding %v does not contain VolumeClaimTemplate, skipping creating PVC", workspaceBinding.Name) + return nil } - return claims + + claim := workspaceBinding.VolumeClaimTemplate.DeepCopy() + claim.Name = GeneratePVCNameFromWorkspaceBinding(workspaceBinding.VolumeClaimTemplate.Name, workspaceBinding, ownerReference) + claim.Namespace = namespace + claim.OwnerReferences = []metav1.OwnerReference{ownerReference} + + return claim } -// GetPVCNameWithoutAffinityAssistant gets the name of PersistentVolumeClaim for a Workspace and PipelineRun or TaskRun. claim +// GeneratePVCNameFromWorkspaceBinding 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. // 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 v1.WorkspaceBinding, owner metav1.OwnerReference) string { +func GeneratePVCNameFromWorkspaceBinding(claimName string, wb v1.WorkspaceBinding, owner metav1.OwnerReference) string { if claimName == "" { return fmt.Sprintf("%s-%s", "pvc", getPersistentVolumeClaimIdentity(wb.Name, string(owner.UID))) } diff --git a/pkg/reconciler/volumeclaim/pvchandler_test.go b/pkg/reconciler/volumeclaim/pvchandler_test.go index 7b7b7d319d3..9cd1c8350fe 100644 --- a/pkg/reconciler/volumeclaim/pvchandler_test.go +++ b/pkg/reconciler/volumeclaim/pvchandler_test.go @@ -84,9 +84,11 @@ func TestCreatePersistentVolumeClaimsForWorkspaces(t *testing.T) { // when - err := pvcHandler.CreatePVCsForWorkspaces(ctx, workspaces, ownerRef, namespace) - if err != nil { - t.Fatalf("unexpexted error: %v", err) + for _, ws := range workspaces { + err := pvcHandler.CreatePVCFromVolumeClaimTemplate(ctx, ws, ownerRef, namespace) + if err != nil { + t.Fatalf("unexpexted error: %v", err) + } } expectedPVCName := claimName1 + "-ad02547921" @@ -148,9 +150,11 @@ func TestCreatePersistentVolumeClaimsForWorkspacesWithoutMetadata(t *testing.T) // when - err := pvcHandler.CreatePVCsForWorkspaces(ctx, workspaces, ownerRef, namespace) - if err != nil { - t.Fatalf("unexpexted error: %v", err) + for _, ws := range workspaces { + err := pvcHandler.CreatePVCFromVolumeClaimTemplate(ctx, ws, ownerRef, namespace) + if err != nil { + t.Fatalf("unexpexted error: %v", err) + } } expectedPVCName := fmt.Sprintf("%s-%s", "pvc", "3fc56c2bb2") @@ -187,7 +191,11 @@ func TestCreateExistPersistentVolumeClaims(t *testing.T) { fakekubeclient := fakek8s.NewSimpleClientset() pvcHandler := defaultPVCHandler{fakekubeclient, zap.NewExample().Sugar()} - for _, claim := range getPVCsWithoutAffinityAssistant(workspaces, ownerRef, namespace) { + for _, ws := range workspaces { + claim := pvcHandler.getPVCFromVolumeClaimTemplate(ws, ownerRef, namespace) + if claim == nil { + t.Fatalf("expect PVC but got nil from workspace: %v", ws.Name) + } _, err := fakekubeclient.CoreV1().PersistentVolumeClaims(namespace).Create(ctx, claim, metav1.CreateOptions{}) if err != nil { t.Fatalf("unexpected error: %v", err) @@ -206,11 +214,12 @@ func TestCreateExistPersistentVolumeClaims(t *testing.T) { } fakekubeclient.Fake.PrependReactor(actionGet, "*", fn) - err := pvcHandler.CreatePVCsForWorkspaces(ctx, workspaces, ownerRef, namespace) - if err != nil { - t.Fatalf("unexpected error: %v", err) + for _, ws := range workspaces { + err := pvcHandler.CreatePVCFromVolumeClaimTemplate(ctx, ws, ownerRef, namespace) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } } - if len(fakekubeclient.Fake.Actions()) != 3 { t.Fatalf("unexpected numer of actions; expected: %d got: %d", 3, len(fakekubeclient.Fake.Actions())) }