fail loudly when post-driver uncordon does not succeed - #210
Conversation
6f1ee1c to
44fed9f
Compare
|
@tariq1890 Do I need to mention this in an office hours or something? Happy to discuss the direction/motivation here. |
|
Thank you for your PR. We are in somewhat of a code freeze as we have a new planned release coming up. I will need to discuss with the rest of the team if we are to include this change in the next release. |
Thanks @tariq1890! This was just a weirdness that we noticed as our clusters come up for the first time (api server might not be fully ready). It's not too much of an issue during steady state or upgrades. Happy to discuss other ways to handle this as well if this doesn't work for you. |
A failed uncordon after driver work was logged with Warnf and swallowed, so the init container exited 0 while the node stayed Ready,SchedulingDisabled with no controller to reconcile it. Retry the uncordon with a bounded exponential backoff (5 attempts) and propagate the error on the main uninstall path so the container exits non-zero once the budget is exhausted. The cleanupOnFailure path stays best-effort. RetryBackoff is shared with UpdateNodeLabels so the retry policy has a single home. Fixes NVIDIA#190. Signed-off-by: Cody W. Eilar <cody.eilar@omniva.com>
44fed9f to
9800a7e
Compare
|
@tariq1890 any updates on getting this in? |
|
@AcidLeroy Apologies for the delayed responses. We are in a code-freeze right now as we are working on getting the next release of the gpu-operator out. Once that's done, we can get back to reviewing this PR. Should this PR be merged, it will be in the releases of gpu-operator following the next. |
|
/ok to test 9800a7e |
|
@AcidLeroy regarding
could you include this in the existing PR, I think that would be a good to have as well. Also could you update the PR description, it seems outdated |
I think we can include the retries for cordon as well
3 participants
Fixes #190.
k8s-driver-managercordons the node, performs its driver work, then calls uncordon and exits. When that final uncordon failed for a transient reason (API-server timeout, 5xx, connection refused), the error was logged withWarnfand swallowed: the init container exited 0 and the pod went Ready, but the node stayedReady,SchedulingDisabledindefinitely with no controller to reconcile it. The lost capacity was invisible until something downstream noticed.This change retries the uncordon with a bounded exponential backoff and propagates the error on the main uninstall path, so the init container exits non-zero once the retry budget is exhausted and the next run retries from a known state instead of leaving the node stuck cordoned. The retry budget is configurable via the
UNCORDON_RETRIESenv var (default 5). ThecleanupOnFailurepath stays best-effort, matching the existing convention there and the symmetricCordonNodeerror handling.The backoff is shared with
UpdateNodeLabelsthrough a smallRetryBackoffhelper so the retry policy lives in one place. Tests use a fake clientset to cover retry-until-success and exit-non-zero-when-exhausted.NOTE: the symmetric
CordonNodestill aborts on the first transient error (unchanged here) — only a failed uncordon leaves the node stuckReady,SchedulingDisabled. Happy to extend the same retry to cordon in a follow-up if maintainers prefer.