mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-08 21:14:56 +09:00
Aleksei Sviridkin
0ba567dd73
## What this PR does The tenant pre-delete hook deletes the tenant's HelmReleases in two waves with `kubectl delete --wait=true` and no `--timeout`. kubectl reads `--timeout=0` as "wait forever" and substitutes a week, so a wave hangs for as long as any child HelmRelease cannot finalize. helm then returns from the uninstall without deleting anything: the release stays in storage, flux keeps the finalizer, and the same teardown gets retried. The delete policy carries no `hook-failed`, so the waiting Job survives between attempts too. Each wave now gets 90s. Neither is guarded, on purpose. Nothing stands behind these deletes, so a wave that did not finish ends the run instead of reporting a cleanup that never happened. That also keeps the order the two waves exist for, since the tenant modules are what the applications run on. Whatever got deleted stays deleted, so the next attempt resumes from there. The Job gets `activeDeadlineSeconds: 270`. It bounds what the waits do not, since the DELETE calls carry kubectl's default `--request-timeout` of 0, and it sits under the 300s uninstall budget flux applies when the HelmRelease has no timeout of its own. That is the budget for tenants cozystack-api generates; `tenant-root` is the exception, a static release in `cozystack-basics` carrying `timeout: 15m0s`, and the ordering holds there too since 280 is under 900. The 30s gap is not cosmetic. The deadline runs from the Job's `.status.startTime` while helm's budget runs from before the Job is created, so at equal values helm gives up first, on a Job that is still running. That is the state this change is meant to end, and it would look fixed, because a number would be there. `terminationGracePeriodSeconds: 10` is part of the same arithmetic. Since 1.31 the Job controller holds back the terminal condition until every pod has finished terminating, so what helm waits for is the deadline plus the termination. At the 30s default that lands on 270+30, level with the budget, which is the gap the deadline was set to buy. That period gets spent in full, every time. The SIGTERM goes to PID 1, which is the shell: the script runs several commands, so the shell never execs into kubectl and stays PID 1 itself. A signal carrying its default disposition is discarded by the kernel when the target is PID 1, and a non-interactive shell installs no SIGTERM handler, so nothing acts on it. kubectl is a child and was not signalled at all. The pod ends on SIGKILL. Nothing in that script needs to finish gracefully, the DELETE calls are already sent and the process is only watching. The pod also gets resource requests, matching the sibling hook in `packages/apps/kubernetes`. Without them it is BestEffort and first to go under node pressure, which mattered less when a retry inside the Job absorbed an eviction. With a single attempt it does not. `backoffLimit: 0` and `restartPolicy: Never` go together, and neither implies the other. At the default limit of 6 the Job does not reach `Failed` inside the budget, so helm falls back on its own timeout with the same nameless "timed out waiting for the condition" as before. Under `OnFailure` the pod survives its own failure and the kubelet re-runs the container in place, and the Job controller finds out afterwards by summing `restartCount`. The script starts over, re-enters the applications wave that just expired, and gets cut down mid-run when the controller catches up. `Never` puts a failed pod in front of the controller directly. ## What this PR does not do No remedy behind the bounded waits. The `packages/apps/kubernetes` hook force-clears child finalizers when its own wait expires. That needs `patch` on HelmReleases in the Role, and it is only safe there because the whole tenant control plane is going away in the same run. Here a wave that expires ends the attempt. `packages/apps/kubernetes/templates/delete.yaml` is untouched. It does not make a doomed teardown converge. A tenant whose children cannot finalize still gets its uninstall retried. What changes is that each attempt ends in bounded time, instead of eating the whole uninstall budget and leaving a pod behind. 90s is a guess, and it is tight. The first wave deletes every application HelmRelease in the namespace, including kinds that are slow to tear down: a nested Tenant runs this same hook one level down before its release finalizes, and an app of kind Kubernetes tears a control plane down. Either one can outrun 90s. The bound is what is left once the Job deadline sits under helm's budget instead of level with it, split between the two waves. The 90s the deadline keeps back for scheduling and the image pull is not calibrated either, and I would rather say so than imply it was. `docker.io/clastix/kubectl:v1.32` comes from Docker Hub with no mirror in front of it, and this repo has measured Docker Hub pulls of a smaller image at about 1m42s in CI. So that remainder is roughly the observed worst case rather than comfortably above it. A cold pull that slow lands on the `activeDeadlineSeconds` path, which is the one that deletes the pod and leaves nothing to read. There is a band that pays for this, and I would rather name it than round it off. Neither shape above fits inside 300s today, so for them the bound costs nothing that works. But every generated child HelmRelease carries a wait strategy, so a wave inherits its slowest child, and a wave that settles in something like 95s does complete inside the budget today. After this change it expires instead, and that tenant trades one clean attempt for an extra flux retry. In wall clock that retry is about five minutes, since `helmrelease-interval` defaults to `5m`, so a tenant in that band takes noticeably longer to delete than it does now. I think that is the right side of the trade: an unbounded wave does not fail, it spends the whole budget and leaves helm reporting a timeout that names nothing. If the band turns out to be populated, the waits and the deadline move together, and the bats suite pins their ordering rather than their values. One caveat on the diagnostics, since the comments in the chart used to promise more than they deliver. The failing wave leaves its output in the failed pod, which `ttlSecondsAfterFinished: 300` then reaps while flux's uninstall backoff is still climbing. And on the `activeDeadlineSeconds` path there is no log at all, because the Job controller deletes the active pods when the deadline expires. That path is the backstop for a kubectl that never returns; the expected path is the wave's own `--timeout`, where the script exits on its own and the pod survives to be read. ## Tests `hack/tenant-pre-delete-hook.bats` renders the chart, pulls the hook script out of the Job and runs it against a stub `kubectl` on `PATH`. It covers a clean run reporting success, every delete reaching kubectl with a non-zero bound, the ordering of the four numbers, and a wave that gave up ending the run without going on to the next one. `--timeout` is implemented inside kubectl, so no stub can make that deadline actually elapse. The tests read the arguments that reached kubectl and inject the non-zero exit an expired delete produces. No EXIT traps. The ordering test reads the numbers out of the rendered Job instead of pinning them, so moving a value keeps it honest while a value that breaks the ordering still fails. An absent deadline or termination period is rejected explicitly, otherwise `null` satisfies the numeric comparison and a Job with no bound at all passes. The Job's own shape is pinned in the chart's helm-unittest suite, since the script cannot observe that about itself. Both suites are green locally, and I mutated each pinned value to confirm they fail for the right reason: dropping either `--timeout`, dropping `activeDeadlineSeconds`, dropping `terminationGracePeriodSeconds`, dropping the requests, `backoffLimit` back to 6, `restartPolicy` back to `OnFailure`, and a `|| echo` appended to the applications wave. Each one goes red in the test that claims that ground. Worth flagging so the check marks are not read as more than they are: `make bats-unit-tests` is red at the merge base, unrelated to this branch, and the loop it runs exits on the first failing file. `hack/cozyreport.bats` sorts ahead of this suite, so on CI the new bats file is not reached at all right now. It is not failing, it is not running. The helm-unittest cases are unaffected and do execute. ### Screenshots None. This changes a Job manifest and its tests, with no user-visible surface. ### Downstream repositories Walked the trigger map against the diff. The diff is one Job manifest inside the tenant chart, its helm-unittest suite, and a new `hack/*.bats` file. No package is added, renamed or removed; no `values.schema.json`, `values.yaml` default or version enum changes; no `ApplicationDefinition` semantics, namespace, variant or release asset changes; nothing under `hack/` is moved or renamed and no make target changes behaviour, since `bats-unit-tests` already globs `hack/*.bats`. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(tenant): bound the pre-delete cleanup hook's HelmRelease deletions and give its Job a wall-clock deadline, so a tenant whose children cannot finalize no longer leaves the hook waiting indefinitely and blocking the uninstall ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Reliability Improvements** * Tenant cleanup jobs now enforce execution deadlines and terminate gracefully. * Cleanup operations have bounded deletion timeouts and no automatic retries. * Failed cleanup steps now stop subsequent operations and correctly report failure. * Cleanup jobs now specify CPU and memory resource requirements. * **Tests** * Added comprehensive coverage for cleanup behavior, timeouts, failure handling, and job lifecycle settings. <!-- end of auto-generated comment: release notes by coderabbit.ai -->