OCPBUGS-56736: Improve error messages for project Delete errors #520

vrutkovs · 2025-05-28T13:53:06Z

No description provided.

openshift-ci-robot · 2025-05-28T13:53:14Z

@vrutkovs: This pull request references Jira Issue OCPBUGS-56736, which is invalid:

expected the bug to target the "4.20.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

vrutkovs · 2025-05-28T13:54:20Z

/payload-job periodic-ci-openshift-release-master-ci-4.20-e2e-aws-ovn-techpreview-serial

openshift-ci · 2025-05-28T13:54:40Z

@vrutkovs: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

periodic-ci-openshift-release-master-ci-4.20-e2e-aws-ovn-techpreview-serial

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/41a86440-3bcb-11f0-9228-f720f943c789-0

openshift-ci · 2025-05-28T13:56:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vrutkovs
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

pkg/project/OWNERS

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

vrutkovs · 2025-05-29T06:39:42Z

/payload-job periodic-ci-openshift-release-master-ci-4.20-e2e-aws-ovn-techpreview-serial

openshift-ci · 2025-05-29T06:39:45Z

@vrutkovs: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

periodic-ci-openshift-release-master-ci-4.20-e2e-aws-ovn-techpreview-serial

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/b4464210-3c57-11f0-8efa-bdf704c2be55-0

vrutkovs · 2025-05-29T12:11:10Z

/payload-aggregate periodic-ci-openshift-release-master-ci-4.20-e2e-aws-ovn-techpreview-serial 10

openshift-ci · 2025-05-29T12:11:39Z

@vrutkovs: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

periodic-ci-openshift-release-master-ci-4.20-e2e-aws-ovn-techpreview-serial

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/023d2050-3c86-11f0-9770-8c3264ee69bc-0

vrutkovs · 2025-05-29T15:49:14Z

/payload-aggregate periodic-ci-openshift-release-master-ci-4.20-e2e-aws-ovn-techpreview-serial 10

openshift-ci · 2025-05-29T15:49:18Z

@vrutkovs: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

periodic-ci-openshift-release-master-ci-4.20-e2e-aws-ovn-techpreview-serial

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/795a9cd0-3ca4-11f0-8236-ddb4864c0c81-0

…g an object" This reverts commit 28133f9.

vrutkovs · 2025-05-30T06:28:21Z

/payload-aggregate periodic-ci-openshift-release-master-ci-4.20-e2e-aws-ovn-techpreview-serial 10

openshift-ci · 2025-05-30T06:28:24Z

@vrutkovs: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

periodic-ci-openshift-release-master-ci-4.20-e2e-aws-ovn-techpreview-serial

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/48de3cd0-3d1f-11f0-9dae-2794b1500ac8-0

vrutkovs · 2025-05-30T13:44:14Z

/jira refresh

openshift-ci-robot · 2025-05-30T13:44:20Z

@vrutkovs: This pull request references Jira Issue OCPBUGS-56736, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.20.0) matches configured target version for branch (4.20.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @wangke19

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

benluddy · 2025-05-30T14:22:35Z

pkg/project/apiserver/registry/project/proxy/proxy.go

+				return false, fmt.Errorf("validation func failed: %w", err)
+			}
+		}
+		err := s.client.Delete(ctx, name, opts)


This shouldn't retry unless the error is a conflict. Also, I see that the Delete call is inside the retried func even when the validation func is nil -- if there is any scenario today or in the future that could cause the delete to be retried without preconditions, that would be bad. I'd feel better having two separate delete paths to prevent it, with a comment.

benluddy · 2025-05-30T14:28:25Z

pkg/project/apiserver/registry/project/proxy/proxy.go

 // Delete deletes a Project specified by its name
 func (s *REST) Delete(ctx context.Context, name string, objectFunc rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) {
 	var opts metav1.DeleteOptions
 	if options != nil {
 		opts = *options
 	}
-	return &metav1.Status{Status: metav1.StatusSuccess}, false, s.client.Delete(ctx, name, opts)
+	attempt := 0
+	err := wait.ExponentialBackoffWithContext(ctx, wait.Backoff{Steps: maxRetriesOnConflict}, func(ctx context.Context) (bool, error) {


Is this is appropriate retry function? How do you want to configure the backoff? Edit: It would be nice to see a comment with some reasoning about how we arrive at specific numbers for tuning this. This is happening on the critical path for project deletes and will have implications on client-perceived latency.

I've capped this to up to 1 second with additional comment to make sure it doesn't get lost

benluddy · 2025-05-30T14:42:03Z

pkg/project/apiserver/registry/project/proxy/proxy.go

+		err := s.client.Delete(ctx, name, opts)
+		return err == nil, err
+	})
+	if err != nil {


ExponentialBackoffWithContext [..] immediately returns an error if the condition returns an error

I don't understand how this helped. Isn't the doc for this function telling me that it would return immediately if there were a delete conflict?

benluddy · 2025-05-30T14:42:40Z

pkg/project/apiserver/registry/project/proxy/proxy.go

 // Delete deletes a Project specified by its name
 func (s *REST) Delete(ctx context.Context, name string, objectFunc rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) {
 	var opts metav1.DeleteOptions
 	if options != nil {
 		opts = *options
 	}
-	return &metav1.Status{Status: metav1.StatusSuccess}, false, s.client.Delete(ctx, name, opts)
+	attempt := 0


Did you mean to remove or use this? I see it being incremented but not used anywhere else.

Yes, it was part of extended debugging

vrutkovs · 2025-05-30T15:45:50Z

/payload-aggregate periodic-ci-openshift-release-master-ci-4.20-e2e-aws-ovn-techpreview-serial 10

openshift-ci · 2025-05-30T15:45:59Z

@vrutkovs: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

periodic-ci-openshift-release-master-ci-4.20-e2e-aws-ovn-techpreview-serial

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/2a4a5710-3d6d-11f0-898d-a24e04f40707-0

vrutkovs · 2025-06-02T08:14:40Z

/payload-aggregate periodic-ci-openshift-release-master-ci-4.20-e2e-aws-ovn-techpreview-serial 10

openshift-ci · 2025-06-02T08:14:42Z

@vrutkovs: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

periodic-ci-openshift-release-master-ci-4.20-e2e-aws-ovn-techpreview-serial

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/a2233090-3f89-11f0-954b-910aecaa9a62-0

vrutkovs · 2025-06-02T10:41:24Z

/payload-aggregate periodic-ci-openshift-release-master-ci-4.20-e2e-aws-ovn-techpreview-serial 10

openshift-ci · 2025-06-02T10:41:28Z

@vrutkovs: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

periodic-ci-openshift-release-master-ci-4.20-e2e-aws-ovn-techpreview-serial

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/21ccadd0-3f9e-11f0-81cd-6839ae5e2905-0

benluddy · 2025-06-03T12:54:24Z

pkg/project/apiserver/registry/project/proxy/proxy.go

+			opts.Preconditions.UID = &projectObj.UID
+			opts.Preconditions.ResourceVersion = &projectObj.ResourceVersion


What if the client already specified either precondition in its request options? We need to check for that and return a conflict response directly instead of ignoring them. Please add a test case for this too.

benluddy · 2025-06-03T13:00:26Z

pkg/project/apiserver/registry/project/proxy/proxy.go

 // Delete deletes a Project specified by its name
 func (s *REST) Delete(ctx context.Context, name string, objectFunc rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) {
 	var opts metav1.DeleteOptions
 	if options != nil {
 		opts = *options
 	}
-	return &metav1.Status{Status: metav1.StatusSuccess}, false, s.client.Delete(ctx, name, opts)
+	var lastErr error
+	err := wait.ExponentialBackoffWithContext(ctx, wait.Backoff{Steps: maxRetriesOnConflict, Duration: maxDuration}, func(ctx context.Context) (bool, error) {


If I'm understanding the Godoc for Duration correctly, this means that we will sleep for one second between retries. That seems high to me. I bet it is a lot longer than a typical total latency of both namespace requests combined.

We can configure the other fields for exponential backoff so that initial retry is fairly fast.

Oh, right, somehow I though "Duration" is max duration we're allowed to spend. I think wait.Backoff{Steps: maxRetriesOnConflict, Factor: 1/maxRetriesOnConflict, Cap: maxDuration, Duration: maxDuration/maxRetriesOnConflict} would make it "up to 1 second" and ensure it has several retries

benluddy · 2025-06-03T13:01:29Z

pkg/project/apiserver/registry/project/proxy/proxy.go

+		case err == nil:
+			return true, nil
+		case kerrors.IsConflict(err):
+			lastErr = err


Can you add tests showing that retry happens on conflict, no retry happens on non-conflict, and one where retries are exhausted please?

benluddy · 2025-06-03T13:05:08Z

pkg/project/apiserver/registry/project/proxy/proxy.go

+	if err != nil && wait.ErrorInterrupted(err) != nil {
+		return &metav1.Status{Status: metav1.StatusFailure}, false, lastErr
+	}


The response should indicate timeout if our wait loop times out. If you use https://github.com/kubernetes/kubernetes/blob/62f72addf26d2fd25e060554bcd8cf5bdc10e50c/staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go#L364 as the returned error and nil for the returned runtime.Object, does the client see what we want?

benluddy · 2025-06-03T13:08:29Z

pkg/project/apiserver/registry/project/proxy/proxy.go

+			return false, err
+		}
+	})
+	if err != nil && wait.ErrorInterrupted(err) != nil {


Won't wait.ErrorInterrupted(err) != nil always be true? I think you meant wait.Interrupted(err).

Good catch, updated

benluddy · 2025-06-03T13:14:03Z

pkg/project/apiserver/registry/project/proxy/proxy.go

+		}
+	})
+	if err != nil && wait.ErrorInterrupted(err) != nil {
+		return &metav1.Status{Status: metav1.StatusFailure}, false, lastErr


I don't think this will plumb non-conflict errors to the client. For example, if the namespace is not found then we should return project not found -- is there a test for that?

Added a test for that

vrutkovs · 2025-06-11T07:05:28Z

/test e2e-aws-ovn-serial

openshift-ci · 2025-06-11T11:07:03Z

@vrutkovs: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/okd-scos-e2e-aws-ovn	`15d0b7d`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/e2e-aws-ovn-serial	`15d0b7d`	link	true	`/test e2e-aws-ovn-serial`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

benluddy · 2025-06-20T14:35:49Z

pkg/project/apiserver/registry/project/proxy/proxy.go

+	if err != nil && wait.Interrupted(err) {
+		return &metav1.Status{Status: metav1.StatusFailure}, false, lastErr
+	}
+	return &metav1.Status{Status: metav1.StatusSuccess}, false, nil


What happens when err != nil && !wait.Interrupted(err)?

benluddy · 2025-06-20T14:53:35Z

pkg/project/apiserver/registry/project/proxy/proxy.go

+			if opts.Preconditions.UID == nil {
+				opts.Preconditions.UID = &projectObj.UID
+			}
+			if opts.Preconditions.ResourceVersion == nil {
+				opts.Preconditions.ResourceVersion = &projectObj.ResourceVersion
+			}


We don't want to retry conflicts that are caused by client-provided preconditions (they are probably doomed unless the request changes).

If we might have propagated one precondition from the request, and added a second precondition here, it becomes hard to robustly determine which precondition caused a conflict. One way to solve this might be to inspect the fresh namespace returned from Get and enforce any client-provided preconditions immediately. After that, we know that both preconditions passed to the namespace Delete came from this code and that a retry might succeed with a newer UID/RV.

openshift-ci bot requested review from deads2k and derekwaynecarr May 28, 2025 13:56

vrutkovs force-pushed the validate-delete-error-message branch from b1385d3 to 6983f0d Compare May 29, 2025 06:39

vrutkovs force-pushed the validate-delete-error-message branch from 6983f0d to 7d73533 Compare May 29, 2025 15:48

openshift-ci-robot mentioned this pull request May 29, 2025

OCPBUGS-56736: Revert "OCPBUGS-38120: project: run validation function when deleting an object" #519

Merged

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2025

Reapply "OCPBUGS-38120: project: run validation function when deletin…

b0852ea

…g an object" This reverts commit 28133f9.

vrutkovs force-pushed the validate-delete-error-message branch from 7d73533 to 5f18128 Compare May 30, 2025 06:26

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2025

vrutkovs force-pushed the validate-delete-error-message branch from 5f18128 to a5ca6cb Compare May 30, 2025 06:27

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels May 30, 2025

openshift-ci bot requested a review from wangke19 May 30, 2025 13:44

vrutkovs force-pushed the validate-delete-error-message branch from a5ca6cb to 3fdbc85 Compare May 30, 2025 13:46

benluddy reviewed May 30, 2025

View reviewed changes

vrutkovs force-pushed the validate-delete-error-message branch from 3fdbc85 to d429563 Compare May 30, 2025 15:45

vrutkovs force-pushed the validate-delete-error-message branch from d429563 to cc4321e Compare June 2, 2025 08:14

benluddy reviewed Jun 3, 2025

View reviewed changes

Retry project Delete attempts with backoff

15d0b7d

vrutkovs force-pushed the validate-delete-error-message branch from cc4321e to 15d0b7d Compare June 10, 2025 09:03

benluddy reviewed Jun 20, 2025

View reviewed changes

		opts.Preconditions.UID = &projectObj.UID
		opts.Preconditions.ResourceVersion = &projectObj.ResourceVersion

OCPBUGS-56736: Improve error messages for project Delete errors #520

Are you sure you want to change the base?

OCPBUGS-56736: Improve error messages for project Delete errors #520

Uh oh!

Conversation

vrutkovs commented May 28, 2025

Uh oh!

openshift-ci-robot commented May 28, 2025

Uh oh!

vrutkovs commented May 28, 2025

Uh oh!

openshift-ci bot commented May 28, 2025

Uh oh!

openshift-ci bot commented May 28, 2025

Uh oh!

vrutkovs commented May 29, 2025

Uh oh!

openshift-ci bot commented May 29, 2025

Uh oh!

vrutkovs commented May 29, 2025

Uh oh!

openshift-ci bot commented May 29, 2025

Uh oh!

vrutkovs commented May 29, 2025

Uh oh!

openshift-ci bot commented May 29, 2025

Uh oh!

vrutkovs commented May 30, 2025

Uh oh!

openshift-ci bot commented May 30, 2025

Uh oh!

vrutkovs commented May 30, 2025

Uh oh!

openshift-ci-robot commented May 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benluddy May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrutkovs commented May 30, 2025

Uh oh!

openshift-ci bot commented May 30, 2025

Uh oh!

vrutkovs commented Jun 2, 2025

Uh oh!

openshift-ci bot commented Jun 2, 2025

Uh oh!

vrutkovs commented Jun 2, 2025

Uh oh!

openshift-ci bot commented Jun 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

benluddy May 30, 2025 •

edited

Loading