test: fix flaky finalizer removal in TriggerReconcilerOnAllEventIT - #3543
Conversation
Sorry, something went wrong.
The event count is increased at the beginning of the reconciliation, thus waiting for it released the test into the middle of a reconciliation that was still about to remove the finalizer. The subsequent update then raced with that removal and failed with a conflict. Wait for the finalizer removal to actually land, and retry the (optimistically locked) update with a fresh read on conflict.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 4860cdbb-e80f-445b-8d4e-57b864f3ee17 You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file. Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Fixes a flaky integration test around finalizer removal in TriggerReconcilerOnAllEventIT by synchronizing on the actual finalizer state (not just event count) and making the update step resilient to optimistic-lock conflicts.
Changes:
- Waits until the reconciler’s finalizer has been removed (resource has only ADDITIONAL_FINALIZER) before proceeding.
- Retries the finalizer-removal update step to handle resourceVersion conflicts during concurrent reconciliation.
Sorry, something went wrong.
| await() | ||
| .ignoreException(KubernetesClientException.class) | ||
| .untilAsserted( | ||
| () -> { | ||
| var res = getResource(); | ||
| res.removeFinalizer(ADDITIONAL_FINALIZER); | ||
| extension.update(res); | ||
| }); |
csviri
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
The event count is increased at the beginning of the reconciliation, thus
waiting for it released the test into the middle of a reconciliation that
was still about to remove the finalizer. The subsequent update then raced
with that removal and failed with a conflict.
Wait for the finalizer removal to actually land, and retry the (optimistically
locked) update with a fresh read on conflict.