mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-09 05:25:14 +09:00
Aleksei Sviridkin
b113cc77d5
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does Deleting a Kafka application left its brokers running. The migration hook creates the KafkaNodePools with `kubectl apply`, so they are not in the release manifest and `helm uninstall` does not remove them, and each pool owns the StrimziPodSet that runs its brokers. The pre-delete hook now deletes the release's pools, selected by `strimzi.io/cluster=<release>`, which is the label the migration hook stamps. Its Role gains `list` and `delete` on `kafkanodepools`, which is what a label-selected delete issues. Fixes #4488. The pools are deleted after the KafkaTopics, also when the topic wait timed out. The topic operator clears topic finalizers through the brokers, so the brokers have to outlive it. Deleting the pools before Helm deletes the Kafka CR does not bring brokers back. For the few seconds in between, Strimzi 0.45.1-rc1 (the version the operator chart pins) fails the reconcile with `KafkaNodePools are enabled, but no KafkaNodePools found` before it builds any StrimziPodSet. A regular pool is the ownerReference of its StrimziPodSet (`NodePoolUtils.createKafkaPools` at that tag), so the pods go away with the pool by garbage collection. If the Kafka CR is already gone when the hook runs, the pools are still found by label. An absent CRD exits 0, and so does a release with no pools, because a label-selected delete that matches nothing succeeds. Any other failure exits 1. The call uses `--wait=false` and no request timeout, so the #4276 invariants and the 270s deadline arithmetic in `delete.yaml` stay as they were. The cleanup lives in the pre-delete hook because only there a failure gets retried. In Helm v4.1.1, which helm-controller v1.5.0 vendors, a pre-delete hook error returns from `Uninstall.Run` before anything is deleted, so the next reconcile runs the hook again. A post-delete error is only collected: the release is still purged and Helm never runs its hooks again, so a failure there would leave the pools for good. An ownerReference from each pool to the Kafka CR would also work. I did not take it because it changes what the migration hook applies on every install and upgrade, including the ZooKeeper migration path that adopts a namespace-wide `kafka` pool. PVCs are out of scope. The pools set `deleteClaim: false`, so Strimzi leaves the broker and controller volumes in place when a pool is deleted, before and after this change. Not covered by this change: - Pools leaked by releases deleted before this fix stay where they are. Find them with `kubectl get kafkanodepools.kafka.strimzi.io -A -L strimzi.io/cluster`; a pool whose cluster has no Kafka CR in its namespace (`kubectl get kafkas.kafka.strimzi.io -A`) is leaked, and `kubectl -n <namespace> delete kafkanodepools.kafka.strimzi.io -l strimzi.io/cluster=<cluster>` removes it and its brokers. - `examples/backups/kafka-metadata/cleanup.sh` returns without waiting for the Kafka uninstalls, unlike `examples/backups/kafka/cleanup.sh`, which is why the metadata suite's new checks carry a 12m budget. The new step already holds the suite until the pools and pods are gone, so the next suite no longer starts into a namespace that is still uninstalling. Adding the same `wait_deleted` calls to the script would let those checks drop to the default timeout; left as a follow-up because the example is outside this change. - The "Deletion and PVC retention" section of `packages/apps/kafka/README.md` does not yet say that the pre-delete hook now removes the node pools while `deleteClaim: false` keeps the PVCs. That one sentence is left as a follow-up. - Helm runs the hooks stored in the release's latest revision, so a release uninstalled before it has been upgraded onto this chart still runs the old hook and still leaks its pools. Tests: - `packages/apps/kafka/tests/delete_test.yaml`: a new case pins, in one pattern, that the pool delete follows the topic delete, is scoped to this release, and does not wait, and pins the exact Role rule. - `hack/kafka-pre-delete-hook.bats` runs the rendered hook against a fake kubectl. Its fake now answers the pool call separately, and four new cases pin the exit codes and ordering: pools are deleted after the topics, still deleted after a topic-wait timeout, an absent pool CRD exits 0, and a failed pool delete exits non-zero. - Each new pin was checked by a mutation that turned it red: widening the selector, moving the pool delete before the topics, waiting on it, exiting on an absent CRD, not exiting on a failure, skipping the pools on the timeout branch, and removing or widening the Role rule. - E2E: the `kafka`, `kafka-2-backup-roundtrip` and `kafka-3-metadata-roundtrip` tests each gain a step, `verify-uninstall-removes-node-pools`, that checks, after the apps are deleted, that no KafkaNodePool and no pod labelled `strimzi.io/cluster=<cluster>` remains. The pod check covers every Strimzi pod of the cluster, not only the brokers, on purpose. The `kafka` test now deletes its app itself so it can look afterwards. E2E success criterion, registered before the run: in this PR's E2E run, every `verify-uninstall-removes-node-pools` step (one in each of the tests `kafka`, `kafka-2-backup-roundtrip` and `kafka-3-metadata-roundtrip`) passes on the first attempt. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> No UI change. ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [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: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: The diff touches one chart template and its unit tests, one hack unit test, and two e2e suites. No values, schema, README, package list, hack tooling, CRD or node requirement changes, so no entry of the trigger map applies. ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(kafka): deleting a Kafka application now also deletes its KafkaNodePools, so its brokers stop instead of running on after the application is gone. Volumes are kept, as before. Node pools left behind by applications deleted before this fix are not removed automatically: delete them with `kubectl -n <namespace> delete kafkanodepools.kafka.strimzi.io -l strimzi.io/cluster=<cluster>`. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Kafka app uninstall now removes associated Kafka node pools after attempting to remove Kafka topics. If topic cleanup times out, uninstall continues; node pool cleanup failures are reported. * End-to-end checks now verify that Kafka node pools and pods are removed after uninstall and backup cleanup. If resources remain, diagnostic details are provided. * **Tests** * Added coverage for node pool cleanup outcomes, including missing resource definitions and deletion failures. <!-- end of auto-generated comment: release notes by coderabbit.ai -->