[TEP-0135] Refactor CreatePVCsForWorkspaces #6921

QuanZhang-William · 2023-07-12T00:04:27Z

Changes

Part of #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 cleanup

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

tekton-robot · 2023-07-12T00:04:29Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tekton-robot · 2023-07-12T00:10:56Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	97.2%	98.2%	1.0
pkg/reconciler/taskrun/taskrun.go	84.8%	84.5%	-0.3
pkg/reconciler/volumeclaim/pvchandler.go	90.0%	89.7%	-0.3

tekton-robot · 2023-07-12T00:12:36Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	97.2%	98.2%	1.0
pkg/reconciler/taskrun/taskrun.go	84.8%	84.5%	-0.3
pkg/reconciler/volumeclaim/pvchandler.go	90.0%	89.7%	-0.3

tekton-robot · 2023-07-12T18:57:10Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	97.2%	99.1%	1.9
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	92.1%	0.3
pkg/reconciler/taskrun/taskrun.go	84.8%	84.5%	-0.3
pkg/reconciler/volumeclaim/pvchandler.go	90.0%	89.7%	-0.3

tekton-robot · 2023-07-12T19:02:13Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	97.2%	99.1%	1.9
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	92.1%	0.3
pkg/reconciler/taskrun/taskrun.go	84.8%	84.5%	-0.3
pkg/reconciler/volumeclaim/pvchandler.go	90.0%	89.7%	-0.3

tekton-robot · 2023-07-12T19:02:44Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	97.2%	99.1%	1.9
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	92.1%	0.3
pkg/reconciler/taskrun/taskrun.go	84.8%	84.5%	-0.3
pkg/reconciler/volumeclaim/pvchandler.go	90.0%	89.7%	-0.3

pkg/reconciler/pipelinerun/pipelinerun.go

pkg/reconciler/pipelinerun/affinity_assistant.go

lbernick · 2023-07-12T19:35:05Z

pkg/reconciler/volumeclaim/pvchandler.go

+		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)
 	}


can this conditional logic be simplified?

I refactored this part a bit. It is not really simplified but I found it is easier to read for me

lbernick · 2023-07-12T19:37:11Z

pkg/reconciler/volumeclaim/pvchandler.go

+	}
+
+	claim := workspaceBinding.VolumeClaimTemplate.DeepCopy()
+	claim.Name = GetPVCNameWithoutAffinityAssistant(workspaceBinding.VolumeClaimTemplate.Name, workspaceBinding, ownerReference)


maybe a better name for this function could be "GeneratePVCNameFromWorkspaceBinding"?

SGTM, function name changed

Part of [tektoncd#6740][tektoncd#6740] and closes [tektoncd#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 [tektoncd#6740]: tektoncd#6740 [tektoncd#6915]: tektoncd#6915

tekton-robot · 2023-07-24T20:56:09Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	94.4%	96.0%	1.6
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	91.7%	-0.1
pkg/reconciler/taskrun/taskrun.go	84.8%	84.8%	0.0
pkg/reconciler/volumeclaim/pvchandler.go	83.3%	82.6%	-0.7

tekton-robot · 2023-07-24T20:56:33Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	94.4%	96.0%	1.6
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	91.7%	-0.1
pkg/reconciler/taskrun/taskrun.go	84.8%	84.8%	0.0
pkg/reconciler/volumeclaim/pvchandler.go	83.3%	82.6%	-0.7

QuanZhang-William · 2023-07-24T21:03:06Z

pkg/reconciler/pipelinerun/affinity_assistant_test.go

@@ -314,6 +315,76 @@ func TestCreateAndDeleteOfAffinityAssistantPerWorkspaceOrDisabled(t *testing.T)
 	}
 }

+func TestCreateOrUpdateAffinityAssistantsAndPVCs_Failure(t *testing.T) {


This test is copied from #6927

QuanZhang-William · 2023-07-24T21:03:35Z

pkg/reconciler/pipelinerun/pipelinerun.go

-						"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 {


This part is copied from #6927

QuanZhang-William · 2023-07-24T21:10:33Z

/hold until v0.50 LTS is released

QuanZhang-William · 2023-07-24T21:11:38Z

/cc @Yongxuanzhang @JeromeJu , could you please also help review this PR, which is probably the last implementation PR for this feature 😁 ?

There are some overlap between this PR and #6927, both PRs are open to review but I think it is probably better to review and merge this one first (sorry for the overlap but I have addressed it carefully and it shouldn't cause too much conflict).

pkg/reconciler/pipelinerun/affinity_assistant.go

pkg/reconciler/volumeclaim/pvchandler.go

tekton-robot · 2023-07-24T22:27:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernick

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [lbernick]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Yongxuanzhang · 2023-07-25T03:23:22Z

/assign

QuanZhang-William · 2023-07-25T14:27:56Z

/hold cancel

v0.50 LTS is released

Yongxuanzhang

/lgtm

QuanZhang-William · 2023-07-26T14:01:22Z

/test pull-tekton-pipeline-alpha-integration-tests

tekton-robot requested review from lbernick and wlynch July 12, 2023 00:04

QuanZhang-William marked this pull request as ready for review July 12, 2023 00:04

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 12, 2023

tekton-robot requested review from afrittoli and dibyom July 12, 2023 00:04

QuanZhang-William marked this pull request as draft July 12, 2023 00:43

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 12, 2023

lbernick self-assigned this Jul 12, 2023

QuanZhang-William force-pushed the tep-0135-refactor-pvc-creation branch 2 times, most recently from 70fd19d to 1b38911 Compare July 12, 2023 18:56

QuanZhang-William marked this pull request as ready for review July 12, 2023 18:56

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 12, 2023

tekton-robot requested review from imjasonh and vdemeester July 12, 2023 18:56

lbernick reviewed Jul 12, 2023

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2023

wlynch removed their request for review July 19, 2023 13:25

QuanZhang-William force-pushed the tep-0135-refactor-pvc-creation branch from 1b38911 to e50a7fb Compare July 24, 2023 20:49

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 24, 2023

QuanZhang-William commented Jul 24, 2023

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 24, 2023

lbernick approved these changes Jul 24, 2023

View reviewed changes

pkg/reconciler/pipelinerun/affinity_assistant.go Show resolved Hide resolved

pkg/reconciler/volumeclaim/pvchandler.go Show resolved Hide resolved

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 24, 2023

tekton-robot assigned Yongxuanzhang Jul 25, 2023

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 25, 2023

QuanZhang-William force-pushed the tep-0135-refactor-pvc-creation branch from e50a7fb to f130104 Compare July 25, 2023 14:32

Yongxuanzhang reviewed Jul 26, 2023

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2023

tekton-robot merged commit 1eef4db into tektoncd:main Jul 26, 2023

QuanZhang-William mentioned this pull request Jul 25, 2023

TEP-0135: Coscheduling PipelineRun pods Implementation #6740

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEP-0135] Refactor CreatePVCsForWorkspaces #6921

[TEP-0135] Refactor CreatePVCsForWorkspaces #6921

QuanZhang-William commented Jul 12, 2023 •

edited

Loading

tekton-robot commented Jul 12, 2023

tekton-robot commented Jul 12, 2023

tekton-robot commented Jul 12, 2023

tekton-robot commented Jul 12, 2023

tekton-robot commented Jul 12, 2023

tekton-robot commented Jul 12, 2023

lbernick Jul 12, 2023

QuanZhang-William Jul 24, 2023

lbernick Jul 12, 2023

QuanZhang-William Jul 24, 2023

tekton-robot commented Jul 24, 2023

tekton-robot commented Jul 24, 2023

QuanZhang-William Jul 24, 2023

QuanZhang-William Jul 24, 2023

QuanZhang-William commented Jul 24, 2023

QuanZhang-William commented Jul 24, 2023 •

edited

Loading

tekton-robot commented Jul 24, 2023

Yongxuanzhang commented Jul 25, 2023

QuanZhang-William commented Jul 25, 2023

Yongxuanzhang left a comment

QuanZhang-William commented Jul 26, 2023

[TEP-0135] Refactor CreatePVCsForWorkspaces #6921

[TEP-0135] Refactor CreatePVCsForWorkspaces #6921

Conversation

QuanZhang-William commented Jul 12, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jul 12, 2023

tekton-robot commented Jul 12, 2023

tekton-robot commented Jul 12, 2023

tekton-robot commented Jul 12, 2023

tekton-robot commented Jul 12, 2023

tekton-robot commented Jul 12, 2023

lbernick Jul 12, 2023

Choose a reason for hiding this comment

QuanZhang-William Jul 24, 2023

Choose a reason for hiding this comment

lbernick Jul 12, 2023

Choose a reason for hiding this comment

QuanZhang-William Jul 24, 2023

Choose a reason for hiding this comment

tekton-robot commented Jul 24, 2023

tekton-robot commented Jul 24, 2023

QuanZhang-William Jul 24, 2023

Choose a reason for hiding this comment

QuanZhang-William Jul 24, 2023

Choose a reason for hiding this comment

QuanZhang-William commented Jul 24, 2023

QuanZhang-William commented Jul 24, 2023 • edited Loading

tekton-robot commented Jul 24, 2023

Yongxuanzhang commented Jul 25, 2023

QuanZhang-William commented Jul 25, 2023

Yongxuanzhang left a comment

Choose a reason for hiding this comment

QuanZhang-William commented Jul 26, 2023

QuanZhang-William commented Jul 12, 2023 •

edited

Loading

QuanZhang-William commented Jul 24, 2023 •

edited

Loading