New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid spurious calls to update/delete validation #104182
Conversation
@liggitt: GitHub didn't allow me to request PR reviews from the following users: rngy. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes/test-infra repository. |
/sig api-machinery |
Unit test failure looks related, will look |
34cd03c
to
561ce03
Compare
fun... the failing unit test was trying to exercise storage hook behavior on retry, but wasn't actually setting up the conditions to trigger a retry correctly... instead of providing a cachedObject with a mismatched resourceVersion, it was getting retried only because of the spurious retry-on-any-failure behavior this PR is fixing. Updated the unit test that wanted to test behavior on retry to set up the conditions to trigger retry correctly. |
/retest |
Thanks for addressing it! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, wojtek-t 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:
Approvers can indicate their approval by writing |
/retest Review the full test history for this PR. Silence the bot with an |
/retest |
…182-upstream-release-1.20 Automated cherry pick of #104182: Avoid spurious calls to update/delete validation
…182-upstream-release-1.19 Automated cherry pick of #104182: Avoid spurious calls to update/delete validation
…182-upstream-release-1.22 Automated cherry pick of #104182: Avoid spurious calls to update/delete validation
…182-upstream-release-1.21 Automated cherry pick of #104182: Avoid spurious calls to update/delete validation
/triage accepted |
What type of PR is this?
/kind bug
What this PR does / why we need it:
The cached etcd storage uses in-memory cached versions of objects as a hint for the existing object when handling update/delete API calls.
If the update/delete fails for any reason, it does a live lookup of the existing object and retries. While correct, this means that when the failure was not due to the in-memory object being stale (for example, the object fails validation or is rejected by admission), the retry is done unnecessarily.
Because the retry involves admission, this means an admission webhook that rejects an update or delete request can get immediately reinvoked a second time with the same data.
If the live lookup reveals the cached object was actually up to date, skip the retry.
This retry has existed since 1.12, and the spurious retry only affects requests that were destined to fail anyway, but this will give a nice performance bump to update/delete requests rejected by admission webhooks.
/cc @wojtek-t @rngy @lavalamp