Skip to content

Commit

Permalink
[TEP-0135] Refactor CreatePVCsForWorkspaces
Browse files Browse the repository at this point in the history 
Part of [tektoncd#6740][tektoncd#6740] and closes [tektoncd#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

[tektoncd#6740]: tektoncd#6740
[tektoncd#6915]: tektoncd#6915
  • Loading branch information
@QuanZhang-William
QuanZhang-William committed Jul 11, 2023
1 parent b769b56 commit b84bdd8
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 74 deletions.
34 changes: 16 additions & 18 deletions pkg/reconciler/pipelinerun/affinity_assistant.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}
}

Expand All @@ -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 {
Expand All @@ -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)
Expand Down
15 changes: 8 additions & 7 deletions pkg/reconciler/taskrun/taskrun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -484,14 +484,15 @@ 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))
// This is used by createPod below. Changes to the Spec are not updated.
tr.Spec.Workspaces = taskRunWorkspaces
Expand Down
77 changes: 39 additions & 38 deletions pkg/reconciler/volumeclaim/pvchandler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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 {
Expand All @@ -51,50 +50,52 @@ func NewPVCHandler(clientset clientset.Interface, logger *zap.SugaredLogger) Pvc
return &defaultPVCHandler{clientset, logger}
}

// CreatePVCsForWorkspaces checks if a PVC named <claim-name>-<workspace-name>-<owner-name> exists;
// CreatePVCFromVolumeClaimTemplate checks if a PVC named <claim-name>-<workspace-name>-<owner-name> 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 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
}

// 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
Expand Down
31 changes: 20 additions & 11 deletions pkg/reconciler/volumeclaim/pvchandler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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)
Expand All @@ -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()))
}
Expand Down

0 comments on commit b84bdd8

Please sign in to comment.