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 ae302e6fd84..72c8326de32 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -4230,7 +4230,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 14dc358ddf0..bbcc5c7edc8 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -3399,7 +3399,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())) }