From 84c18fcc56682c83a7373018e8a18f2ec9548966 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]. This commit 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 cleanup [#6740]: #6740 [#6915]: https://github.com/tektoncd/pipeline/issues/6915 --- .../pipelinerun/affinity_assistant.go | 34 ++++---- pkg/reconciler/taskrun/taskrun.go | 12 ++- pkg/reconciler/volumeclaim/pvchandler.go | 77 ++++++++++--------- pkg/reconciler/volumeclaim/pvchandler_test.go | 31 +++++--- 4 files changed, 84 insertions(+), 70 deletions(-) diff --git a/pkg/reconciler/pipelinerun/affinity_assistant.go b/pkg/reconciler/pipelinerun/affinity_assistant.go index f0dccf3cdc5..a0a1b91064c 100644 --- a/pkg/reconciler/pipelinerun/affinity_assistant.go +++ b/pkg/reconciler/pipelinerun/affinity_assistant.go @@ -60,7 +60,7 @@ func (c *Reconciler) createOrUpdateAffinityAssistantsAndPVCs(ctx context.Context 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 +74,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 } } @@ -93,14 +85,16 @@ func (c *Reconciler) createOrUpdateAffinityAssistantsAndPVCs(ctx context.Context err := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, nil, []corev1.PersistentVolumeClaimVolumeSource{*claim}, unschedulableNodes) errs = append(errs, 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. + pvcErr := c.pvcHandler.CreatePVCFromVolumeClaimTemplate(ctx, workspace, *kmeta.NewControllerRef(pr), pr.Namespace) + errs = append(errs, pvcErr) + + aaName := GetAffinityAssistantName(workspace.Name, pr.Name) + aaErr := c.createOrUpdateAffinityAssistant(ctx, aaName, pr, nil, []corev1.PersistentVolumeClaimVolumeSource{{ClaimName: claimTemplate.Name}}, unschedulableNodes) + errs = append(errs, aaErr...) } case aa.AffinityAssistantPerPipelineRun, aa.AffinityAssistantPerPipelineRunWithIsolation: if claims != nil || claimTemplates != nil { @@ -112,6 +106,10 @@ func (c *Reconciler) createOrUpdateAffinityAssistantsAndPVCs(ctx context.Context errs = append(errs, err...) } case aa.AffinityAssistantDisabled: + for _, workspace := range claimTemplatesToWorkspace { + pvcErr := c.pvcHandler.CreatePVCFromVolumeClaimTemplate(ctx, workspace, *kmeta.NewControllerRef(pr), pr.Namespace) + errs = append(errs, pvcErr) + } } return errorutils.NewAggregate(errs) diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 695e72d6d13..3fc32ad5dcb 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -484,10 +484,16 @@ 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 { + 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.Errorf("failed to create PVC for TaskRun %s workspaces correctly: %w", fmt.Sprintf("%s/%s", tr.Namespace, tr.Name), err)) return controller.NewPermanentError(err) } diff --git a/pkg/reconciler/volumeclaim/pvchandler.go b/pkg/reconciler/volumeclaim/pvchandler.go index 9e9283d3016..a7216e2d921 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.getPVCWithoutAffinityAssistant(wb, ownerReference, namespace) + if claim == nil { + return fmt.Errorf("attempted to create PVC but VolumeClaimTemplate is nil in WorkspaceBinding: %v", wb.Name) } - 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 +} + +// getPVCWithoutAffinityAssistant returns a PersistentVolumeClaim based on given workspaceBinding (using VolumeClaimTemplate), ownerReference and namespace +func (c *defaultPVCHandler) getPVCWithoutAffinityAssistant(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..ba14e35b703 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.getPVCWithoutAffinityAssistant(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())) }