Retry on conflict during CRD dry-run updates in XRD validation webhook #5868
Conversation
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
| if kerrors.IsNotFound(err) { | ||
| return v.client.Create(ctx, crd, client.DryRunAll) | ||
| } |
There was a problem hiding this comment.
This can be outside of the loop.
There was a problem hiding this comment.
Yes, correct. Keeping it in the retry function is more concise though, as in the retry function we still need to try to get the CRD to retry an update.
| got := crd.DeepCopy() | ||
| err := v.client.Get(ctx, client.ObjectKey{Name: crd.Name}, got) | ||
| if err == nil { | ||
| got.Spec = crd.Spec |
There was a problem hiding this comment.
preexisting: doesn't this mean that Update below can mutate crd? The json unmarshaller under the hood might reuse referenced data structures.
There was a problem hiding this comment.
The semantics of this function are fishy. crd might get some changes on update, but not all.
There was a problem hiding this comment.
out of scope of the PR, just saying.
There was a problem hiding this comment.
I believe the deep copy is preserving the original object. We pass a pointer to the deep copied object in the Update call.
There was a problem hiding this comment.
The object got is fresh, but crd.Spec is not. The DeepCopy does not help for that.
There was a problem hiding this comment.
IIRC that's the point, I was getting some error due to something else being updated, so I only switched in the part I care validating, the spec. Happy to drop it if that's not needed.
|
Successfully created backport PR for |
Description of your changes
The XRD validation webhook's
ValidateUpdatedoes not currently retry on HTTP 409 errors. We've observed this in e2e tests, causing sporadic failures. This PR proposes to introduce a retry mechanism for the CRD dry-run updates on conflicts.I have:
earthly +reviewableto ensure this PR is ready for review.- [ ] Added or updated unit tests.- [ ] Added or updated e2e tests.- [ ] Linked a PR or a docs tracking issue to document this change.- [ ] Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.