Do not fail PipelineRun if pvc creation error is because of exceeded quotas #8903

vdemeester · 2025-07-22T10:47:42Z

Changes

In case of the PVC creation (from volumeclaimtemplate) is due to a
quota error (quota exceeded), do not fail with a permanent error, and
instead mark the PipelineRun as pending. Once there is some quota
available back, it will be able to start.

Signed-off-by: Vincent Demeester vdemeest@redhat.com

Closes #7672

/kind feature

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
pre-commit Passed
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

PipelineRun do not fail anymore if the pvc creation is due to an exceeded quota ; it will be requeued instead (until quota is available or it times out)

tekton-robot · 2025-07-22T10:47:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from vdemeester after the PR has been reviewed.

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

Needs approval from an approver in each of these files:

OWNERS

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

vdemeester · 2025-07-22T10:48:14Z

/wip
I need to update/create some e2e tests and makes sure it times out.

tekton-robot · 2025-07-22T11:02:54Z

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/pipelinerun.go	91.6%	91.2%	-0.4
pkg/reconciler/volumeclaim/pvchandler.go	83.7%	77.4%	-6.3

tekton-robot · 2025-07-22T11:04:34Z

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/pipelinerun.go	91.6%	91.2%	-0.4
pkg/reconciler/volumeclaim/pvchandler.go	83.7%	77.4%	-6.3

vdemeester · 2025-07-23T09:13:30Z

/hold

…quotas In case of the PVC creation (from volumeclaimtemplate) is due to a quota error (quota exceeded), do not fail with a permanent error, and instead mark the PipelineRun as pending. Once there is some quota available back, it will be able to start. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

tekton-robot · 2025-07-23T09:35:22Z

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/pipelinerun.go	91.6%	91.2%	-0.4
pkg/reconciler/volumeclaim/pvchandler.go	83.7%	77.4%	-6.3

vdemeester · 2025-07-23T10:54:24Z

/retest

vdemeester · 2025-07-23T11:57:40Z

Timeout is taken into account 👼🏼

twoGiants · 2025-07-24T15:44:33Z

/assign

twoGiants · 2025-07-24T15:44:56Z

/cc @twoGiants

twoGiants

Good job, it makes sense to set the status to pending and let it retry 😸 👍.

Now it will re-queue right away. Do you want to make it configurable at some point like suggested in the issue?

My comments are below. I would add unit tests, simplify the conditional logic and remove the re-declaration of the errors in affinity_assistant.go.

twoGiants · 2025-07-24T16:12:47Z

pkg/reconciler/volumeclaim/pvchandler.go

@@ -73,14 +80,20 @@ func (c *defaultPVCHandler) CreatePVCFromVolumeClaimTemplate(ctx context.Context
 			if apierrors.IsAlreadyExists(err) {


Optional: We could use the chance to untangle the conditions a bit. If you pick this suggestion the comment right below can be skipped :).

Suggested change

if apierrors.IsAlreadyExists(err) {

_, getErr := c.clientset.CoreV1().PersistentVolumeClaims(claim.Namespace).Get(ctx, claim.Name, metav1.GetOptions{})

switch {

case apierrors.IsNotFound(getErr):

_, createErr := c.clientset.CoreV1().PersistentVolumeClaims(claim.Namespace).Create(ctx, claim, metav1.CreateOptions{})

if createErr == nil {

c.logger.Infof("Created PersistentVolumeClaim %s in namespace %s", claim.Name, claim.Namespace)

return nil

}

if apierrors.IsAlreadyExists(createErr) {

c.logger.Infof("Tried to create PersistentVolumeClaim %s in namespace %s, but it already exists",

claim.Name, claim.Namespace)

return nil

}

// This is a retry-able error

if apierrors.IsForbidden(createErr) && strings.Contains(createErr.Error(), "exceeded quota") {

return fmt.Errorf("%w: %v", ErrPvcCreationFailedRetryable, createErr.Error())

}

return fmt.Errorf("%w for %s: %w", ErrPvcCreationFailed, claim.Name, createErr)

case getErr != nil:

return fmt.Errorf("%w: failed to retrieve PVC %s: %w", ErrPvcCreationFailed, claim.Name, getErr)

}

}

twoGiants · 2025-07-24T16:17:31Z

pkg/reconciler/volumeclaim/pvchandler.go

+					// This is a retry-able error
+					return fmt.Errorf("%w: %v", ErrPvcCreationFailedRetryable, err.Error())
+				}
+				return fmt.Errorf("%w for %s: %w", ErrPvcCreationFailed, claim.Name, err)


This return and the one in the else right after can be simplified.

Suggested change

return fmt.Errorf("%w for %s: %w", ErrPvcCreationFailed, claim.Name, err)

} else if apierrors.IsForbidden(err) && strings.Contains(err.Error(), "exceeded quota") {

// This is a retry-able error

return fmt.Errorf("%w: %v", ErrPvcCreationFailedRetryable, err.Error())

}

return fmt.Errorf("%w for %s: %w", ErrPvcCreationFailed, claim.Name, err)

twoGiants · 2025-07-24T16:20:13Z

pkg/reconciler/volumeclaim/pvchandler.go

Unit tests for the new conditional logic in CreatePVCFromVolumeClaimTemplate should be added.

twoGiants · 2025-07-24T16:24:20Z

pkg/reconciler/pipelinerun/affinity_assistant.go

@@ -50,7 +50,8 @@ const (
 )

 var (
-	ErrPvcCreationFailed               = errors.New("PVC creation error")
+	ErrPvcCreationFailed               = volumeclaim.ErrPvcCreationFailed


I wouldn't re-declare them here if they are already declared in volumeclaim and public. You don't use them in this file anyway.

twoGiants · 2025-07-24T16:25:20Z

pkg/reconciler/pipelinerun/affinity_assistant_test.go

@@ -577,54 +577,56 @@ func TestCreateOrUpdateAffinityAssistantsAndPVCs_Failure(t *testing.T) {
 		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),
+		expectedErr: fmt.Errorf("%w for pvc-b9eea16dce: error creating persistentvolumeclaims", ErrPvcCreationFailed),


You could use volumeclaim.ErrPvcCreationFailed instead, then you don't need to re-declare them in affinity_assistant.go.

twoGiants · 2025-07-24T16:26:14Z

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -755,6 +755,10 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel
 				pr.Status.MarkFailed(volumeclaim.ReasonCouldntCreateWorkspacePVC,
 					"Failed to create PVC for PipelineRun %s/%s correctly: %s",
 					pr.Namespace, pr.Name, err)
+			case errors.Is(err, ErrPvcCreationFailedRetryable):


Here I would also use it from volumeclaim.ErrPvcCreationFailed.

github-project-automation bot added this to Tekton Community Roadmap Jul 22, 2025

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 22, 2025

github-project-automation bot moved this to Todo in Tekton Community Roadmap Jul 22, 2025

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 22, 2025

tekton-robot requested review from dibyom and pritidesai July 22, 2025 10:47

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 22, 2025

vdemeester added this to the v1.3.0 (LTS) milestone Jul 22, 2025

vdemeester force-pushed the 7672-retry-pvc-quota branch from 4a5edbf to 52464a8 Compare July 22, 2025 10:53

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

vdemeester force-pushed the 7672-retry-pvc-quota branch from 52464a8 to 6b0c7ae Compare July 23, 2025 09:22

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 23, 2025

tekton-robot assigned twoGiants Jul 24, 2025

tekton-robot requested a review from twoGiants July 24, 2025 15:44

twoGiants suggested changes Jul 24, 2025

View reviewed changes

		@@ -73,14 +80,20 @@ func (c *defaultPVCHandler) CreatePVCFromVolumeClaimTemplate(ctx context.Context
		if apierrors.IsAlreadyExists(err) {

-			if apierrors.IsAlreadyExists(err) {
+	_, getErr := c.clientset.CoreV1().PersistentVolumeClaims(claim.Namespace).Get(ctx, claim.Name, metav1.GetOptions{})
+	switch {
+	case apierrors.IsNotFound(getErr):
+		_, createErr := c.clientset.CoreV1().PersistentVolumeClaims(claim.Namespace).Create(ctx, claim, metav1.CreateOptions{})
+		if createErr == nil {
+			c.logger.Infof("Created PersistentVolumeClaim %s in namespace %s", claim.Name, claim.Namespace)
+			return nil
+		}
+		if apierrors.IsAlreadyExists(createErr) {
+			c.logger.Infof("Tried to create PersistentVolumeClaim %s in namespace %s, but it already exists",
+				claim.Name, claim.Namespace)
+			return nil
+		}
+		// This is a retry-able error
+		if apierrors.IsForbidden(createErr) && strings.Contains(createErr.Error(), "exceeded quota") {
+			return fmt.Errorf("%w: %v", ErrPvcCreationFailedRetryable, createErr.Error())
+		}
+		return fmt.Errorf("%w for %s: %w", ErrPvcCreationFailed, claim.Name, createErr)
+	case getErr != nil:
+		return fmt.Errorf("%w: failed to retrieve PVC %s: %w", ErrPvcCreationFailed, claim.Name, getErr)
+	}
+}

Do not fail PipelineRun if pvc creation error is because of exceeded quotas #8903

Are you sure you want to change the base?

Do not fail PipelineRun if pvc creation error is because of exceeded quotas #8903

Conversation

vdemeester commented Jul 22, 2025

Changes

Submitter Checklist

Release Notes

Uh oh!

tekton-robot commented Jul 22, 2025

Uh oh!

vdemeester commented Jul 22, 2025

Uh oh!

tekton-robot commented Jul 22, 2025

Uh oh!

tekton-robot commented Jul 22, 2025

Uh oh!

vdemeester commented Jul 23, 2025

Uh oh!

tekton-robot commented Jul 23, 2025

Uh oh!

vdemeester commented Jul 23, 2025

Uh oh!

vdemeester commented Jul 23, 2025

Uh oh!

twoGiants commented Jul 24, 2025

Uh oh!

twoGiants commented Jul 24, 2025

Uh oh!

twoGiants left a comment

Choose a reason for hiding this comment

Uh oh!

twoGiants Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

twoGiants Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

twoGiants Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

twoGiants Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

twoGiants Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

twoGiants Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!