fix(operations): replace the component removal recipe that undoes itself - #684
Aleksei Sviridkin (lexfrei) wants to merge 3 commits into
Conversation
✅ Deploy Preview for cozystack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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. Comment |
0125811 to
2e750d6
Compare
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
Reviewed at 2e750d69f against merge-base 257bbbbe3.
The fix itself is sound. I re-derived every factual claim in the PR body against the platform source at the matching tags rather than taking them on trust: Package ownership of the HelmRelease, Flux's default deletion behaviour, metallb's CRDs carrying no resource-policy, cert-manager's contrasting crds.keep, and the exact patch releases where the keep annotation appears. All of it holds. What blocks is that v1.0 still documents the recipe being replaced everywhere else, with no warning on the page.
Findings
- [MINOR]
content/en/docs/next/operations/configuration/components.md:83, Warning's named CRD list undercounts the blast radius under metallb.frrk8s.enabled: true - [MINOR]
content/en/docs/next/operations/configuration/components.md:89, No signal for when the destructive uninstall has actually finished - [MAJOR]
content/en/docs/v1.0/operations/configuration/components.md:70, the recipe this PR replaces is still live on v1.0, with no warning
Claim mismatches
[UNVERIFIABLE] "hugo --gc --minify builds clean". I could not reproduce it here: the build hung on Hugo Modules resolution, with the docsy module never appearing in GOMODCACHE despite proxy.golang.org being reachable, and was killed after two attempts. The sandbox Hugo is v0.153.2, below the 0.164.0 extended that CONTRIBUTING.md requires, which may be why. Secondary evidence supports the claim without settling it: the {{% alert %}} shortcode form used in the diff appears identically in 392 other places on the site, and open and close tags balance 1:1 in all seven touched files. A gap on my side, not a defect.
Caveats
v0/operations/configuration/components.mdalso still carrieskubectl delete hr, and that one is correctly out of scope.v0predates the Package mechanism, so the removal path there is different, and CONTRIBUTING.md explicitly demotes it to legacy-only fixes.v1.0has neither property.hack/check-i18n.shfails today on the four stale translations the PR body names (content/{de,hi,ru,zh-cn}/_index.md). None are touched here, so it is pre-existing and matches the body's own disclosure.
Recommended follow-ups
- File the tracking issue for the
v1.0gap if it does not get folded into this PR. - Consider replacing the hand-enumerated kind list in the Warning with a pointer to whatever CRDs the component's chart bundles, so a component like metallb with frr-k8s does not need the list maintained per component.
Findings not anchored to changed lines
These reference code outside this PR's diff (unchanged files, or lines outside a hunk), so GitHub cannot render them inline.
[MAJOR] content/en/docs/v1.0/operations/configuration/components.md:70 the recipe this PR replaces is still live on v1.0, with no warning
v1.0/operations/configuration/components.md:70 still tells the reader to run kubectl delete hr -n <namespace> <component>, and that page carries no warning at all. v1.0/install/providers/hetzner.md:347 still has the bare - metallb under disabledPackages that this PR fixes on the v1.1 equivalent, and the platform matches disabledPackages against the fully-qualified cozystack.metallb, so the bare name does nothing.
On v1.0 the rendered Package never carried helm.sh/resource-policy: keep, which is the same reason the PR gives for the v1.1/v1.2 caveat. So the v1.0 page documents the destructive path with zero disclosure:
$ grep -n 'kubectl delete hr' content/en/docs/v1.0/operations/configuration/components.md
70:kubectl delete hr -n <namespace> <component>
$ grep -c 'alert title="Warning"' content/en/docs/v1.0/operations/configuration/components.md
0
$ grep -n -A1 'disabledPackages' content/en/docs/v1.0/install/providers/hetzner.md | sed -n '3,4p'
346: disabledPackages:
347- - metallb
$ for t in v1.0.8 v1.1.4 v1.1.5; do printf '%-8s ' $t; \
git show $t:packages/core/platform/templates/_helpers.tpl | grep -c resource-policy; done
v1.0.8 0
v1.1.4 0
v1.1.5 2
v1.0 is not demoted. hugo.yaml carries exactly one hidden: true and it belongs to next, not to v1.0; CONTRIBUTING.md demotes only v0 ("Treat v0/ as legacy"), and its own routing table says a fix that also applies to an older supported version gets copied into each relevant vX.Y/.
The PR body scopes this out on purpose and explains why (v1.0 needs a different correction rather than a copy, since disabledPackages alone still prunes the Package there). That reasoning is sound, and I verified it. What is missing is the follow-up: no tracking issue, no companion change, and no notice on the page. Either extend this PR to v1.0 with the version-scoped variant of the same two paragraphs, or file the issue before merge.
| Then delete the Package object. `kubectl get packages` lists the names. | ||
|
|
||
| {{% alert title="Warning" color="warning" %}} | ||
| Deleting the Package uninstalls the component's Helm release, and that destroys more than the workloads. Anything the chart rendered as an ordinary template without `helm.sh/resource-policy: keep` goes with the release, CRDs and namespaces included, and Kubernetes deletes every custom resource of those CRD kinds along with them. Removing `cozystack.metallb` takes the MetalLB CRDs and with them every IPAddressPool, L2Advertisement, BGPPeer and the rest of those kinds cluster-wide; removing `cozystack.cozystack-basics` takes the `cozy-public` namespace and everything stored in it. Back up anything you still need first. |
There was a problem hiding this comment.
[MINOR] Warning's named CRD list undercounts the blast radius under metallb.frrk8s.enabled: true
The alert names IPAddressPool, L2Advertisement, BGPPeer as the destroyed kinds. packages/system/metallb/charts/metallb/charts/frr-k8s/charts/crds/templates/*.yaml renders four more CRDs (frrconfigurations, frrnodestates, bgpsessionstates, frrk8sconfigurations) under a different group, frrk8s.metallb.io, as a nested subchart of the same cozystack.metallb release, and metallb.frrk8s.enabled: true is a documented toggle. The preceding generic sentence already covers this correctly, so this is not wrong, just incomplete for BGP-mode operators reading only the named list. Same text is duplicated identically at v1.1:78, v1.2:83, v1.3:83, v1.4:83, v1.5:83, v1.6:83.
There was a problem hiding this comment.
Dropped the kind list. The sentence now points at the chart and says subcharts count, so it does not need updating per component.
One scope correction: frr-k8s only landed in packages/system/metallb at v1.5.0, so v1.1 through v1.4 never shipped those four CRDs. The new wording holds on all of them anyway.
| The namespace a component installs into is the exception: the operator applies that one itself, outside the component's release and with no ownerReference, so the uninstall never had it to remove. | ||
|
|
||
| ```bash | ||
| kubectl delete package.cozystack.io <package-name> |
There was a problem hiding this comment.
[MINOR] No signal for when the destructive uninstall has actually finished
internal/operator/package_reconciler.go registers no finalizer on Package (verified: the only finalizer usage in the tree belongs to an unrelated tapmaterializer reconciler), so kubectl delete package.cozystack.io returns as soon as the Package object itself is gone. The owned HelmRelease's deletion, and the helm uninstall inside it that destroys the CRDs/namespaces the Warning describes, proceeds asynchronously afterward via the ownerReference cascade. The recipe has no follow-up check for confirming the destructive step is actually done. Same text is duplicated identically at v1.1:84, v1.2:89, v1.3:89, v1.4:89, v1.5:89, v1.6:89.
There was a problem hiding this comment.
Added a wait after the delete:
kubectl wait --for=delete helmrelease/<component> --namespace <namespace> --timeout=10mwith a line saying why the delete returns early.
|
IvanHunters folded v1.0 into this PR instead of filing the issue. It needed different text, not a copy. At i18n-lint was red on four stale home-page translations, nothing to do with this PR. Fixed in |
Removing an installed component was documented as `kubectl delete hr -n <namespace> <component>`. The Package that emitted the release owns it through a controller ownerReference, and the operator watches HelmReleases with Owns(), so the delete fires the reconcile that recreates it. The command reports success, nothing logs an error, and the component is running again afterwards. That is not the harmless no-op it looks like. The operator sets no uninstall policy and no suspend on the HelmReleases it builds, so Flux's default applies and the release is uninstalled when the object goes away. Anything the uninstall destroys is gone before the reinstall brings the workloads back, and the restored release hides that it happened. The page now says so, for the benefit of anyone who already ran the old recipe. Deleting the cluster-scoped Package is what cascades into the uninstall deliberately. It needs the disabledPackages entry to land first, or the next platform upgrade renders the Package again and the removal undoes itself one level up. The reader confirms that by reading the disabledPackages list back off the platform HelmRelease, which is the only observation here that cannot go green early. A condition wait returns instantly against the Ready left standing by the previous reconcile; re-reading the Package after deleting it returns NotFound unconditionally; and comparing observedGeneration against generation on that HelmRelease is equal in exactly the window that matters, because the operator has not yet copied the edit across from the Package the reader changed. What the uninstall takes is stated above the command rather than after it, in a warning callout. Anything the chart rendered as an ordinary template and did not annotate resource-policy: keep goes with the release, CRDs and namespaces alike, and the apiserver cascades every custom resource of those CRD kinds. Removing cozystack.metallb takes the MetalLB CRDs and the objects of those kinds cluster-wide; removing cozystack.cozystack-basics takes the cozy-public namespace, which that chart renders as a plain template and which is not its install namespace. The one exception is the namespace a component installs into: the operator applies that itself, outside the release, so the uninstall never had it. The v1.1 and v1.2 pages qualify the step count in place, next to the instruction it governs, because the behaviour changed inside those lines rather than between them. The keep annotation on Packages arrived in v1.1.5 and v1.2.1 as backports, so v1.1.0 through v1.1.4 and v1.2.0 have no annotation and adding the name to disabledPackages removes the component by itself, putting the destruction on that step instead of the delete. The clauses name their range rather than saying "and earlier", which would be false across lines: v1.1.5 through v1.1.7 are lower-versioned than v1.2.0 and do carry the annotation. Applied to next and v1.1 through v1.6. Left out v1.0, whose line never received the annotation at all, so that page needs the single-step correction rather than this one. v0 predates Package objects: its platform chart renders HelmReleases directly, so deleting the HelmRelease there names the object the chart creates, though that page carries no destruction warning either. The v1.1 disabledPackages examples listed bare names, which match nothing against the fully-qualified names the platform chart feeds the helper. Both copies are fixed: the one in the components page, and the one in the Hetzner install guide that page links to as its worked example. Leaving the second would have had v1.1 teach the qualified form on one page and hand out the broken form on the page it cites, on the one provider where swapping MetalLB for RobotLB is mandatory. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>
44e27e6 to
4bdf52e
Compare
The named CRD kinds in the warning cover the top-level MetalLB chart only; a subchart such as frr-k8s brings its own group, so the list undercounts what an uninstall takes. Point at the chart instead of enumerating kinds, which also stops the list needing maintenance per component. Package carries no finalizer, so deleting it returns before the Helm uninstall that follows it has done anything. Add the wait that tells an operator the destructive part is over. Through v1.4 the cozystack-basics chart also renders the tenant-root namespace as a plain template with no keep annotation, so uninstalling it takes the whole root tenant, not just cozy-public. From v1.5 that namespace carries the no-delete label the platform's admission policy enforces, so the later pages keep the shorter example. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>
v1.0 documented `kubectl delete hr` as the way to remove an installed component, with no warning about what an uninstall takes with it. That command destroys the CRDs and every custom resource of those kinds, then the Package recreates the HelmRelease and the chart reinstalls, so the workloads come back and the data does not. v1.0 needs a different correction from later versions rather than a copy of it: the platform does not annotate rendered Packages with `helm.sh/resource-policy: keep`, so `disabledPackages` alone already removes an installed component there, in one step and with no confirmation. Also qualify the package names in the Hetzner and components examples. `disabledPackages` and `enabledPackages` are matched against the full `cozystack.<name>` form, so the bare names did nothing. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>
4bdf52e to
35a2827
Compare
|
IvanHunters I rebased onto main, so the SHAs in my earlier replies are gone. The i18n fix I referenced as Two changes since you looked. On v1.1 and v1.2 the note about v1.1.0 to v1.1.4 and v1.2.0 sat in the middle of the two-step instructions and read as if one of the steps applied to the old patches, so it's now a separate paragraph ahead of the steps. On v1.0 to v1.4 the warning now says that removing |
This branch has not been deployed
2 participants
kubectl delete hr -n <namespace> <component>is documented as the way to remove an installed component. It removes nothing, and it is not harmless. The Package that produced the release owns the HelmRelease through a controller ownerReference, and the operator watches them withOwns(&helmv2.HelmRelease{}), so the delete fires the reconcile that recreates it. ButbuildHelmReleaseSpecsets no uninstall policy and no suspend, so Flux's default applies and the release is uninstalled on the way out. The workloads come back with the reinstall. Anything the uninstall destroyed does not, and the restored release hides that it happened.Deleting the Package is what cascades into the uninstall deliberately. The
disabledPackagesentry has to land first, or the next platform upgrade renders the Package again and the removal undoes itself one level up. The page checks that by reading the list back from thecozystack-platformHelmRelease, since the operator copies the Platform Package's values into that HelmRelease'sspec.values. The obvious alternatives can't fail: a condition wait returns instantly against the Ready left standing by the previous reconcile, and re-reading the Package after deleting it returns NotFound either way. Nothing holds a finalizer on the Package, so the delete returns before the uninstall runs. Akubectl wait --for=deleteon the HelmRelease tells the reader when the destructive part is done.What the uninstall takes is stated above the command, since making the recipe work is what arms it.
packages/system/metallb/charts/metallb/charts/crds/templates/crds.yamlrenders its CRDs as ordinary subchart templates with noresource-policyannotation, so helm uninstall deletes them and the apiserver cascades every IPAddressPool, L2Advertisement and BGPPeer with them. From v1.5 the frr-k8s subchart adds its own CRDs under another group, so the warning points at the chart and its subcharts rather than listing kinds.cert-manager-crdssetscrds.keepand its CRDs survive the same uninstall. Through v1.4,cozystack-basicsrenders thetenant-rootNamespace the same way, so the warning on those pages says its removal takes the root tenant. From v1.5 that namespace carries theplatform.cozystack.io/no-deletelabel.v1.0 gets different text, not a copy. That line never got the
helm.sh/resource-policy: keepannotation on rendered Packages (none inpackages/core/platform/templates/_helpers.tplatv1.0.8), so adding a name todisabledPackagesalready removes an installed component there, in one step and with no confirmation. The page says that, carries the same warning, and drops thekubectl delete hrrecipe. v0 predates Package objects and is left alone.keepreached v1.1 and v1.2 mid-line, in v1.1.5 and v1.2.1. Those two pages state the single-step behaviour of v1.1.0 to v1.1.4 and v1.2.0 in its own paragraph ahead of the two-step recipe.The v1.0 and v1.1
disabledPackagesexamples, on the components page and in the Hetzner guide, listed baremetallb/hetzner-robotlb. Those match nothing against the fully-qualified names the chart feeds the helper, so they're qualified now.Not taken:
cozypkg delresolves dependents and deletes in reverse topological order, which rawkubectl deletedoes not, but whether that subcommand exists varies across the version trees.hugo --gc --minifybuilds clean andhack/check-i18n.shpasses.