diff --git a/pkg/reconciler/pipelinerun/affinity_assistant.go b/pkg/reconciler/pipelinerun/affinity_assistant.go index f0dccf3cdc5..a6f9e66d128 100644 --- a/pkg/reconciler/pipelinerun/affinity_assistant.go +++ b/pkg/reconciler/pipelinerun/affinity_assistant.go @@ -40,9 +40,9 @@ 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" ) @@ -53,14 +53,13 @@ const ( // every taskrun in the pipelinerun that use the same PVC based volume. // If the AffinityAssitantBehavior is AffinityAssistantPerPipelineRun or AffinityAssistantPerPipelineRunWithIsolation, // it creates one Affinity Assistant for the pipelinerun. -func (c *Reconciler) createOrUpdateAffinityAssistantsAndPVCs(ctx context.Context, pr *v1.PipelineRun, aaBehavior aa.AffinityAssitantBehavior) error { - var errs []error +func (c *Reconciler) createOrUpdateAffinityAssistantsAndPVCs(ctx context.Context, pr *v1.PipelineRun, aaBehavior aa.AffinityAssitantBehavior) (string, 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{} + claimTemplatesToWorkspace := map[*corev1.PersistentVolumeClaim]v1.WorkspaceBinding{} for _, w := range pr.Spec.Workspaces { if w.PersistentVolumeClaim == nil && w.VolumeClaimTemplate == nil { @@ -74,15 +73,7 @@ func (c *Reconciler) createOrUpdateAffinityAssistantsAndPVCs(ctx context.Context claimTemplate := w.VolumeClaimTemplate.DeepCopy() claimTemplate.Name = volumeclaim.GetPVCNameWithoutAffinityAssistant(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) + claimTemplatesToWorkspace[claimTemplate] = w } } @@ -90,17 +81,21 @@ func (c *Reconciler) createOrUpdateAffinityAssistantsAndPVCs(ctx context.Context case aa.AffinityAssistantPerWorkspace: for claim, workspaceName := range claimToWorkspace { 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, []corev1.PersistentVolumeClaimVolumeSource{*claim}, unschedulableNodes); err != nil { + return ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSet, errorutils.NewAggregate(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 claimTemplatesToWorkspace { + // To support PVC auto deletion at pipelinerun deletion time, the OwnerReference of the PVCs should be set to the owning pipelinerun, + // so we create PVCs from PipelineRuns' VolumeClaimTemplate and pass the PVCs to the Affinity Assistant StatefulSet for volume scheduling. + // If passed in as VolumeClaimTemplates directrly, the PVCs are owned by Affinity Assistant StatefulSet instead of the pipelinerun. + if err := c.pvcHandler.CreatePVCFromVolumeClaimTemplate(ctx, workspace, *kmeta.NewControllerRef(pr), pr.Namespace); err != nil { + return volumeclaim.ReasonCouldntCreateWorkspacePVC, err + } + aaName := GetAffinityAssistantName(workspace.Name, pr.Name) + if err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, nil, []corev1.PersistentVolumeClaimVolumeSource{{ClaimName: claimTemplate.Name}}, unschedulableNodes); err != nil { + return ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSet, errorutils.NewAggregate(err) + } } case aa.AffinityAssistantPerPipelineRun, aa.AffinityAssistantPerPipelineRunWithIsolation: if claims != nil || claimTemplates != nil { @@ -108,13 +103,19 @@ func (c *Reconciler) createOrUpdateAffinityAssistantsAndPVCs(ctx context.Context // 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...) + if err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, claimTemplates, claims, unschedulableNodes); err != nil { + return ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSet, errorutils.NewAggregate(err) + } } case aa.AffinityAssistantDisabled: + for _, workspace := range claimTemplatesToWorkspace { + if err := c.pvcHandler.CreatePVCFromVolumeClaimTemplate(ctx, workspace, *kmeta.NewControllerRef(pr), pr.Namespace); err != nil { + return volumeclaim.ReasonCouldntCreateWorkspacePVC, err + } + } } - return errorutils.NewAggregate(errs) + return "", nil } // createOrUpdateAffinityAssistant creates an Affinity Assistant Statefulset with the provided affinityAssistantName and pipelinerun information. diff --git a/pkg/reconciler/pipelinerun/affinity_assistant_test.go b/pkg/reconciler/pipelinerun/affinity_assistant_test.go index b18bdde3d2f..98704e2a993 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 @@ -169,9 +170,9 @@ func TestCreateAndDeleteOfAffinityAssistantPerPipelineRun(t *testing.T) { KubeClientSet: fakek8s.NewSimpleClientset(), } - err := c.createOrUpdateAffinityAssistantsAndPVCs(ctx, tc.pr, aa.AffinityAssistantPerPipelineRun) + reason, err := c.createOrUpdateAffinityAssistantsAndPVCs(ctx, tc.pr, aa.AffinityAssistantPerPipelineRun) if err != nil { - t.Errorf("unexpected error from createOrUpdateAffinityAssistantsPerPipelineRun: %v", err) + t.Errorf("unexpected error from createOrUpdateAffinityAssistantsPerPipelineRun: %v with reason: %v", err, reason) } // validate StatefulSets from Affinity Assistant @@ -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 @@ -273,9 +274,9 @@ func TestCreateAndDeleteOfAffinityAssistantPerWorkspaceOrDisabled(t *testing.T) pvcHandler: volumeclaim.NewPVCHandler(kubeClientSet, zap.NewExample().Sugar()), } - err := c.createOrUpdateAffinityAssistantsAndPVCs(ctx, tc.pr, tc.aaBehavior) + reason, 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 with reason %v", err, reason) } // validate StatefulSets from Affinity Assistant @@ -314,6 +315,78 @@ func TestCreateAndDeleteOfAffinityAssistantPerWorkspaceOrDisabled(t *testing.T) } } +func TestCreateOrUpdateAffinityAssistantsAndPVCs_Failure(t *testing.T) { + testCases := []struct { + name, failureType string + aaBehavior aa.AffinityAssitantBehavior + expectedErr error + expectedReason string + }{ + { + name: "affinity assistant creation failed - per workspace", + failureType: "statefulset", + aaBehavior: aa.AffinityAssistantPerWorkspace, + expectedErr: fmt.Errorf("failed to create StatefulSet affinity-assistant-4cf1a1c468: error creating statefulsets"), + expectedReason: ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSet, + }, + { + name: "affinity assistant creation failed - per pipelinerun", + failureType: "statefulset", + aaBehavior: aa.AffinityAssistantPerPipelineRun, + expectedErr: fmt.Errorf("failed to create StatefulSet affinity-assistant-426b306c50: error creating statefulsets"), + expectedReason: ReasonCouldntCreateOrUpdateAffinityAssistantStatefulSet, + }, + { + name: "pvc creation failed - per workspace", + failureType: "pvc", + aaBehavior: aa.AffinityAssistantPerWorkspace, + expectedErr: fmt.Errorf("failed to create PVC pvc-b9eea16dce: error creating persistentvolumeclaims"), + expectedReason: volumeclaim.ReasonCouldntCreateWorkspacePVC, + }, + { + name: "pvc creation failed - disabled", + failureType: "pvc", + aaBehavior: aa.AffinityAssistantDisabled, + expectedErr: fmt.Errorf("failed to create PVC pvc-b9eea16dce: error creating persistentvolumeclaims"), + expectedReason: volumeclaim.ReasonCouldntCreateWorkspacePVC, + }, + } + + 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") + }) + } + + reason, err := c.createOrUpdateAffinityAssistantsAndPVCs(ctx, testPRWithVolumeClaimTemplate, tc.aaBehavior) + + if err == nil { + t.Errorf("expect error from createOrUpdateAffinityAssistantsAndPVCs but got nil") + } + if d := cmp.Diff(tc.expectedErr.Error(), err.Error()); d != "" { + t.Errorf("expected err mismatching: %v", diff.PrintWantGot(d)) + } + if d := cmp.Diff(tc.expectedReason, reason); d != "" { + t.Errorf("expected failure reason 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) { @@ -417,9 +490,9 @@ func TestCreateOrUpdateAffinityAssistantWhenNodeIsCordoned(t *testing.T) { return true, &corev1.Pod{}, errors.New("error listing/deleting pod") }) } - err := c.createOrUpdateAffinityAssistantsAndPVCs(ctx, testPRWithPVC, aa.AffinityAssistantPerWorkspace) + reason, err := c.createOrUpdateAffinityAssistantsAndPVCs(ctx, testPRWithPVC, aa.AffinityAssistantPerWorkspace) if !tt.expectedError && err != nil { - t.Errorf("expected no error from createOrUpdateAffinityAssistantsPerWorkspace for the test \"%s\", but got: %v", tt.name, err) + t.Errorf("expected no error from createOrUpdateAffinityAssistantsPerWorkspace for the test \"%s\", but got: %v; reason: %v", tt.name, err, reason) } // the affinity assistant pod must have been deleted when it was running on a cordoned node if tt.validatePodDeletion { diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index f4ee460ffac..d33b51ecb10 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -613,18 +613,12 @@ 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 { + failReason, err := c.createOrUpdateAffinityAssistantsAndPVCs(ctx, pr, aaBehavior) + if 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 { - pr.Status.MarkFailed(volumeclaim.ReasonCouldntCreateWorkspacePVC, - "Failed to create PVC for PipelineRun %s/%s Workspaces correctly: %s", - pr.Namespace, pr.Name, err) - } - + pr.Status.MarkFailed(failReason, + "Failed to create StatefulSet for PipelineRun %s/%s correctly: %s", + pr.Namespace, pr.Name, err) return controller.NewPermanentError(err) } case affinityassistant.AffinityAssistantPerPipelineRun, affinityassistant.AffinityAssistantPerPipelineRunWithIsolation: diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 695e72d6d13..fee56682577 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -58,6 +58,7 @@ import ( k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + errorutils "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/kubernetes" corev1Listers "k8s.io/client-go/listers/core/v1" "k8s.io/utils/clock" @@ -484,12 +485,19 @@ 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) + var errs []error + for _, ws := range tr.Spec.Workspaces { + if err := c.pvcHandler.CreatePVCFromVolumeClaimTemplate(ctx, ws, *kmeta.NewControllerRef(tr), tr.Namespace); err != nil { + errs = append(errs, err) + } + } + if errs != nil { + aggregatedErrs := errorutils.NewAggregate(errs) + logger.Errorf("Failed to create PVC for TaskRun %s: %v", tr.Name, aggregatedErrs) 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) + fmt.Errorf("failed to create PVC for TaskRun %s workspaces correctly: %w", + fmt.Sprintf("%s/%s", tr.Namespace, tr.Name), aggregatedErrs)) + return controller.NewPermanentError(aggregatedErrs) } taskRunWorkspaces := applyVolumeClaimTemplates(tr.Spec.Workspaces, *kmeta.NewControllerRef(tr)) diff --git a/pkg/reconciler/volumeclaim/pvchandler.go b/pkg/reconciler/volumeclaim/pvchandler.go index 9e9283d3016..9b1e0cc7f90 100644 --- a/pkg/reconciler/volumeclaim/pvchandler.go +++ b/pkg/reconciler/volumeclaim/pvchandler.go @@ -26,7 +26,6 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - errorutils "k8s.io/apimachinery/pkg/util/errors" clientset "k8s.io/client-go/kubernetes" ) @@ -38,7 +37,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 } type defaultPVCHandler struct { @@ -51,50 +50,52 @@ 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)) - } - - if apierrors.IsAlreadyExists(err) { - c.logger.Infof("Tried to create PersistentVolumeClaim %s in namespace %s, but it already exists", - claim.Name, claim.Namespace) - } - - 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)) - } +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 } - return errorutils.NewAggregate(errs) -} -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 + _, 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) { + return fmt.Errorf("failed to create PVC %s: %w", claim.Name, err) + } + + if apierrors.IsAlreadyExists(err) { + c.logger.Infof("Tried to create PersistentVolumeClaim %s in namespace %s, but it already exists", + claim.Name, claim.Namespace) } - claim := workspaceBinding.VolumeClaimTemplate.DeepCopy() - claim.Name = GetPVCNameWithoutAffinityAssistant(workspaceBinding.VolumeClaimTemplate.Name, workspaceBinding, ownerReference) - claim.Namespace = namespace - claim.OwnerReferences = []metav1.OwnerReference{ownerReference} - claims[workspaceBinding.Name] = claim + if err == nil { + 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 claims + + return nil +} + +// 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 + } + + claim := workspaceBinding.VolumeClaimTemplate.DeepCopy() + claim.Name = GetPVCNameWithoutAffinityAssistant(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 diff --git a/pkg/reconciler/volumeclaim/pvchandler_test.go b/pkg/reconciler/volumeclaim/pvchandler_test.go index aaffa85d478..6cfa747dac5 100644 --- a/pkg/reconciler/volumeclaim/pvchandler_test.go +++ b/pkg/reconciler/volumeclaim/pvchandler_test.go @@ -83,9 +83,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" @@ -147,9 +149,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") @@ -186,7 +190,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) @@ -205,11 +213,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())) }