Boskos: Retry on conflict

[alvaroaleman]

This PR adds retrying on conflicts to the boskos handlers, in order to resolve the occasional the object has been modified; please apply your changes to the latest version and try again errors we are seeing and that will presumably vastly increase once we remove the global lock.

Fixes https://github.com/openshift/release/issues/7183

/assign @ixdy @bbguimaraes

[k8s-ci-robot]

@alvaroaleman: GitHub didn't allow me to assign the following users: bbguimaraes.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

This PR adds retrying on conflicts to the boskos handlers, in order to resolve the occasional the object has been modified; please apply your changes to the latest version and try again errors we are seeing and that will presumably vastly increase once we remove the global lock.

Fixes https://github.com/openshift/release/issues/7183

/assign @ixdy @bbguimaraes

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/test-infra repository.

[alvaroaleman]

/cc @bbguimaraes

[k8s-ci-robot]

@alvaroaleman: GitHub didn't allow me to request PR reviews from the following users: bbguimaraes.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @bbguimaraes

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/test-infra repository.

[ixdy]

this somewhat feels weird to me, like the abstraction is leaking - why have the handlers do retries rather than putting that into the ranch implementation? (this is arguably easier, probably - the ranch functions are very long.)

[ixdy]

it looks like this is really a copy of retry.OnError except that you curried the retriable parameter.

Couldn't we instead do this?

func retryOnConflict(backoff wait.Backoff, fn func() error) error {
	return retry.OnError(backoff, isConflict, fn)
}

Or am I missing something?

[alvaroaleman]

yeah, retry.RetryOnError is not available in the client-go version we currently use. I am working on another PR that will also bump the kube dependencies, once that is in I could change this to use retry.OnError: #16524

[ixdy]

are you working on a follow-up PR now that #16524 is in?

[ixdy]

to answer my own question: #16698

[alvaroaleman]

this somewhat feels weird to me, like the abstraction is leaking - why have the handlers do retries rather than putting that into the ranch implementation? (this is arguably easier, probably - the ranch functions are very long.)

That is a very good point, updated the PR accordingly. Good you caught this, because we do the locking within ranch, so the retying on handler level would have meant that we might have to acquire the lock multiple times for a single request 😬

I also added retrying to SyncResources, which required me to adjust the error propagation a bit so we can still properly recognize conflicts.

[stevekuznetsov]

Do we want a tracking issue on this

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, stevekuznetsov

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:

• boskos/OWNERS [stevekuznetsov]

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