feat(kubernetes-nodes): split worker node pool into its own app (Phase 2, additive)#3314
feat(kubernetes-nodes): split worker node pool into its own app (Phase 2, additive)#3314IvanHunters wants to merge 12 commits into
Conversation
Introduce the kubernetes-nodes application chart: one HelmRelease per worker node pool, attached to a parent Kubernetes cluster by name convention (kubernetes-<cluster>). Renders the KubevirtMachineTemplate, MachineDeployment, MachineHealthCheck, worker WorkloadMonitor and the talos-reconcile Job with its RBAC, extracted from the monolithic kubernetes chart. Pool objects are byte-identical to the parent's render for the same group, so a later adoption never churns live worker VMs. This is the additive half of the Phase 2 split (community#8); the parent kubernetes chart is left unchanged. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
Generated by cozyvalues-gen + controller-gen from values.yaml. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
Unit tests cover naming/selectors, clusterName wiring, autoscaler annotations, omitted replicas, bootstrap TCT reference, KMT hash name, worker WorkloadMonitor, and the empty-cluster / mis-prefixed-release render guards. render-parity.sh diffs the pool objects against the parent kubernetes chart and asserts byte-identity. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
Add the kubernetes-nodes-rd package (ApplicationDefinition), the cozystack.kubernetes-nodes-application PackageSource (kubevirt variant, kubernetes-nodes + kubernetes-nodes-rd components), and wire it into the iaas bundle. Mirrors the vm-instance/vm-disk registration precedent. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
github-actions
Bot
added
area/uncategorized
The preserve-old-KubevirtMachineTemplate block re-emitted any KMT owned by the CAPI Cluster and referenced by a MachineSet. A kubernetes-nodes pool's KMT lives in the same namespace under the same Cluster, so the parent would re-emit it with a foreign meta.helm.sh/release-name and its next upgrade would fail Helm ownership validation. Restrict the block to KMTs whose release-name annotation matches the parent release. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
…reign pool names Replace the collision-prone name-prefix match (pool 'gpu' matched sibling 'gpu-large-<hash>') with a release-name annotation scope, and add a render-time guard that fails cleanly when a pool's MachineDeployment already exists under a different release (e.g. naming a pool after a parent nodeGroup such as the default md0). Document the naming caveat in NOTES.txt. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
The cluster field referenced x-cozystack-options source 'kubernetes', which has no provider (cozystack-api returns NotFound), so the dashboard dropdown 404s. Drop the annotation so the field is a plain text input; a real provider listing Kubernetes CRs is a follow-up. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
…l to the parent Restore the inline machineconfig comments (nameservers, GPU node labels) inside the reconcile Job's data block so the applied TalosConfigTemplate matches the parent's, avoiding drift on adoption. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
Wire tests/render-parity.sh into the chart's test target so CI verifies the pool objects stay byte-identical to the parent render. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
📝 WalkthroughWalkthroughAdds a deployable ChangesKubernetes nodes worker pool
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Helm
participant MachineDeployment
participant ReconcileJob
participant KamajiControlPlane
participant TalosConfigTemplate
Helm->>MachineDeployment: render worker pool and infrastructure reference
ReconcileJob->>KamajiControlPlane: patch certificate SANs and read owner UID
ReconcileJob->>TalosConfigTemplate: apply Talos worker configuration
MachineDeployment->>TalosConfigTemplate: use bootstrap configuration reference
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.72.0)Trivy execution failed: 2026-07-20T12:00:39Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: kubernetes scan error: fs filter error: fs filter error: walk error range error: stat smartylint.json: no such file or directory: range error: stat smartylint.json: no such file or directory 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 |
Reference the adoption migration by role (Phase 2b) instead of a number that does not exist on this branch, and correct the NOTES operational-note count. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
…arity Extend the golden-parity matrix beyond the resources-sized pool to also diff a GPU pool and a kubelet-override pool against the parent render, so future drift in those branches (which would roll the content-hash-named KubevirtMachineTemplate) is caught in CI. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
Timofei Larkin (lllamnyp)
left a comment
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
Review: Phase 2a looks genuinely additive and independently mergeable
Reviewed the diff against origin/main with a focus on whether this can land on its own without changing behavior for any existing cluster. As far as I can tell it can. Leaving this as a comment rather than an approval since the stack's risk is concentrated in #3315 and it makes sense to judge the two together before merging either.
The one change to existing behavior checks out
Everything here is new files except packages/apps/kubernetes/templates/cluster.yaml:875, which narrows the old-KubevirtMachineTemplate preservation loop with an added meta.helm.sh/release-name == .Release.Name condition. That is a no-op for every pre-split cluster: Helm 3 stamps meta.helm.sh/release-name on every resource it applies, so each KubevirtMachineTemplate the loop matched before still matches. The scoping is needed for the split (sibling pool releases share the CAPI Cluster ownerReference) and costs nothing before it.
The one edge case worth naming: a KubevirtMachineTemplate that somehow lacks the annotation — hand-created, or restored by a tool that dropped it — would now silently fall out of the re-emission set and be pruned on the next parent upgrade, leaving a live MachineSet pointing at a missing infrastructure template. That should not occur in a Helm-managed cluster, so I would not gate on it, but it is the one way this line is not strictly a no-op.
The parity gate is real
packages/apps/kubernetes-nodes/tests/render-parity.sh passes locally: KubevirtMachineTemplate, MachineDeployment, MachineHealthCheck and the worker WorkloadMonitor come out byte-identical between the monolithic chart and the new pool chart across all three cases. Since the KubevirtMachineTemplate is content-hash-named, that is the property that makes adoption non-disruptive, and it is good to see it enforced mechanically rather than asserted.
Two limitations worth stating in the script header so the guarantee is not read as broader than it is. The script already notes that the instanceType-sized branch cannot render offline because lookup returns nil — that branch is the default (u1.medium) and the shape most existing md0 groups actually use, so the branch with the least coverage is the one with the most production instances behind it. And the comparison is scoped to the four pool objects; the talos-reconcile Job's rendered output is not compared, though its content-hash name means a divergence there is visible in a different way.
Behavior in the 2a-only state
A pool created while only this PR is deployed is handled correctly. kubernetes-nodes.assertNoForeignPool in templates/_helpers.tpl:96-107 catches a pool name colliding with a nodeGroup still owned by the parent chart and fails with an actionable message instead of a cryptic Helm ownership error. Both charts patching the same KamajiControlPlane.spec.network.certSANs is idempotent. So the intermediate state where 2a is merged and 2b is not is coherent rather than merely untested.
Minor
packages/apps/kubernetes-nodes/templates/nodegroup.yaml:86-88 — the ephemeralStorage guard cannot fire. $group is constructed at lines 34-44 from a fixed list of .Values keys, so hasKey .group "ephemeralStorage" is always false, and the migration-41 safety net it reproduces is not actually present in the new chart. Either check .Values directly or drop it; as written it reads like protection that is not there.
The kubernetes- and kubernetes-nodes- release prefixes overlap, so a Kubernetes CR named nodes-foo-md0 and a KubernetesNodes CR named foo-md0 map to the same HelmRelease name. Both read paths are safe — Get verifies the application labels and List filters by label selector, so neither kind can serve the other's HelmRelease, and a colliding create fails with AlreadyExists. So this is naming hygiene rather than a bug, but a validation rule rejecting Kubernetes names beginning with nodes- would close it cheaply.
…ty scope The ephemeralStorage guard could never fire (the pool group is built from a fixed set of chart values that never includes ephemeralStorage), so it read as protection that is not there — remove it. Note in the parity script header that only the four pool objects are compared, not the talos-reconcile Job. Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the additive phase of splitting worker node pools from the monolithic Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
dosubot
Bot
added
area/kubernetes
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the kubernetes-nodes application package, which splits the worker node pool management out of the main kubernetes control plane chart. It adds the necessary Helm templates, values schema, tests, and platform integration to provision and reconcile worker node pools independently. Feedback on the changes includes addressing a validation gap where the co-dependency of explicit CPU and memory resources is bypassed when instanceType is empty, as well as quoting template variables (deviceName and instancetype name) in YAML definitions to prevent potential parsing issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| {{- if and $group.instanceType (or (and $hasCpu (not $hasMem)) (and $hasMem (not $hasCpu))) }} | ||
| {{- fail (printf "nodeGroup %s: set both resources.cpu and resources.memory, or neither" $groupName) }} | ||
| {{- end }} |
There was a problem hiding this comment.
The validation check for ensuring both cpu and memory are set together (or neither) is unnecessarily restricted to when instanceType is non-empty. If instanceType is empty and only one of cpu or memory is set, the validation is bypassed, leading to an invalid VM spec that KubeVirt will reject at admission time. Removing the and $group.instanceType clause ensures this validation is always enforced.
{{- if or (and $hasCpu (not $hasMem)) (and $hasMem (not $hasCpu)) }}
{{- fail (printf "nodeGroup %s: set both resources.cpu and resources.memory, or neither" $groupName) }}
{{- end }}| - name: gpu{{ add $i 1 }} | ||
| deviceName: {{ $gpu.name }} |
There was a problem hiding this comment.
It is safer to quote the deviceName value to prevent any YAML parsing issues if the GPU name contains special characters.
- name: gpu{{ add $i 1 }}
deviceName: {{ $gpu.name | quote }}| kind: VirtualMachineClusterInstancetype | ||
| name: {{ .group.instanceType }} |
There was a problem hiding this comment.
Quoting the name field for the instancetype is recommended to ensure robust YAML rendering.
kind: VirtualMachineClusterInstancetype
name: {{ .group.instanceType | quote }}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/apps/kubernetes-nodes/tests/render-parity.sh`:
- Around line 73-95: Update diff_kinds so each helm template invocation checks
its exit status explicitly; when either render fails, print the failure context
and return nonzero immediately before extract or diff runs. Preserve the
existing comparison behavior for successful renders.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2a588133-d071-469f-98da-ad91bbb6ddce
⛔ Files ignored due to path filters (1)
packages/apps/kubernetes-nodes/logos/kubernetes.svgis excluded by!**/*.svg
📒 Files selected for processing (28)
api/apps/v1alpha1/kubernetesnodes/types.goapi/apps/v1alpha1/kubernetesnodes/zz_generated.deepcopy.gopackages/apps/kubernetes-nodes/.helmignorepackages/apps/kubernetes-nodes/Chart.yamlpackages/apps/kubernetes-nodes/Makefilepackages/apps/kubernetes-nodes/README.mdpackages/apps/kubernetes-nodes/charts/cozy-libpackages/apps/kubernetes-nodes/files/versions.yamlpackages/apps/kubernetes-nodes/images/kubectl.tagpackages/apps/kubernetes-nodes/templates/NOTES.txtpackages/apps/kubernetes-nodes/templates/_helpers.tplpackages/apps/kubernetes-nodes/templates/_resources.tplpackages/apps/kubernetes-nodes/templates/_versions.tplpackages/apps/kubernetes-nodes/templates/dashboard-resourcemap.yamlpackages/apps/kubernetes-nodes/templates/nodegroup.yamlpackages/apps/kubernetes-nodes/templates/talos-reconcile-job.yamlpackages/apps/kubernetes-nodes/tests/nodegroup_test.yamlpackages/apps/kubernetes-nodes/tests/render-parity.shpackages/apps/kubernetes-nodes/values.schema.jsonpackages/apps/kubernetes-nodes/values.yamlpackages/apps/kubernetes/templates/cluster.yamlpackages/core/platform/sources/kubernetes-nodes-application.yamlpackages/core/platform/templates/bundles/iaas.yamlpackages/system/kubernetes-nodes-rd/Chart.yamlpackages/system/kubernetes-nodes-rd/Makefilepackages/system/kubernetes-nodes-rd/cozyrds/kubernetes-nodes.yamlpackages/system/kubernetes-nodes-rd/templates/cozyrd.yamlpackages/system/kubernetes-nodes-rd/values.yaml
| diff_kinds() { # <case-name> | ||
| local case="$1" rc=0 | ||
| helm template "kubernetes-${CLUSTER}" "$PARENT" -n "$NS" -f "$WORK/parent.yaml" >"$WORK/parent-out.yaml" | ||
| helm template "kubernetes-nodes-${CLUSTER}-${POOL}" "$CHILD" -n "$NS" -f "$WORK/child.yaml" >"$WORK/child-out.yaml" | ||
| for spec in \ | ||
| 'KubevirtMachineTemplate|.kind == "KubevirtMachineTemplate"' \ | ||
| 'MachineDeployment|.kind == "MachineDeployment"' \ | ||
| 'MachineHealthCheck|.kind == "MachineHealthCheck"' \ | ||
| 'WorkloadMonitor(worker)|.kind == "WorkloadMonitor" and .spec.type == "worker"' \ | ||
| ; do | ||
| local label="${spec%%|*}" sel="${spec#*|}" | ||
| extract "$WORK/parent-out.yaml" "$sel" "$WORK/p.yaml" | ||
| extract "$WORK/child-out.yaml" "$sel" "$WORK/c.yaml" | ||
| if diff -u "$WORK/p.yaml" "$WORK/c.yaml" >"$WORK/d.txt"; then | ||
| echo "OK [${case}] ${label}" | ||
| else | ||
| echo "FAIL [${case}] ${label}" | ||
| cat "$WORK/d.txt" | ||
| rc=1 | ||
| fi | ||
| done | ||
| return "$rc" | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/apps/kubernetes-nodes/tests/render-parity.sh"
# Show the relevant function and call site with line numbers
sed -n '1,220p' "$FILE" | cat -n
# Locate the diff_kinds call and any error-handling around it
rg -n "diff_kinds|set -e|helm template|extract " "$FILE"Repository: cozystack/cozystack
Length of output: 1927
🌐 Web query:
Bash set -e function called in || list errexit suppressed manual
💡 Result:
In Bash, when a function or command is executed as part of an AND (&&) or OR (||) list, the errexit (set -e) option does not trigger an exit if that command returns a non-zero exit status [1][2]. This behavior is a deliberate feature of the shell's grammar: commands contained within these lists are "tested" by the shell to determine whether to proceed with the remainder of the list, so a failure is treated as part of the normal control flow rather than an error that should terminate the script [1][3][4]. Specifically, the errexit option is not triggered if the command is part of a command executed in a && or || list [2]. Note that this suppression applies to the command itself; if the command is a function, any internal command that fails will still be subject to errexit unless that internal command is also protected by one of the specific conditions [1][5]. The specific conditions where errexit is suppressed are [1][2]: - The command is part of the command list immediately following a while or until keyword. - The command is part of the test following an if or elif reserved word. - The command is part of a && or || list. - The command's return value is being inverted using!. If you require a function to trigger an exit even when used in a || list, you must explicitly handle the exit status within that function or at the point of the call, for example by using || { exit_command; } or by ensuring the function itself handles its failures internally [6].
Citations:
- 1: https://unix.stackexchange.com/questions/699108/why-does-a-command-with-a-non-zero-exit-status-send-an-err-signal-even-when-it-i
- 2: https://durak.org/sean/pubs/software/bash/bashref_15.html
- 3: https://man7.org/linux/man-pages/man1/bash.1.html
- 4: https://bash-hackers.gabe565.com/syntax/basicgrammar/
- 5: https://stackoverflow.com/questions/49724312/in-bash-which-traps-are-inherited-by-functions-and-builtins
- 6: https://lists.libreplanet.org/archive/html/bug-bash/2023-08/msg00024.html
Explicitly handle helm template failures here
diff_kinds is called from an || list, so set -e won’t abort on a failed render inside this function. Check each helm template exit status and return early before extract/diff, or the parity gate can report success after a render error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/apps/kubernetes-nodes/tests/render-parity.sh` around lines 73 - 95,
Update diff_kinds so each helm template invocation checks its exit status
explicitly; when either render fails, print the failure context and return
nonzero immediately before extract or diff runs. Preserve the existing
comparison behavior for successful renders.
2 participants
What this PR does
Phase 2 of the Kubernetes-app split (cozystack/community#8) — the additive half.
Introduces
kubernetes-nodes: a per-pool application that renders the worker node-pool objects (KubevirtMachineTemplate,MachineDeployment,MachineHealthCheck, workerWorkloadMonitor, and thetalos-reconcileJob + RBAC) extracted from the monolithickuberneteschart. AKubernetesNodesCR named<cluster>-<pool>attaches to a parentKubernetescluster in the same namespace by name convention (kubernetes-<cluster>), mirroring thevm-instance/vm-diskprecedent.This PR is additive: the parent
kuberneteschart gets exactly one scoping fix (its old-KMT preservation block is limited to KMTs owned by its own Helm release) so that attaching akubernetes-nodespool to a live cluster cannot break the parent's next upgrade. Existing clusters keep provisioning workers vianodeGroups. RemovingnodeGroupsfrom the parent and adopting existing pools intokubernetes-nodesreleases is a follow-up breaking PR (Phase 2b).Key properties:
tests/render-parity.sh), so the follow-up adoption never churns live worker VMs — the content-hashedKubevirtMachineTemplatename is preserved.MachineDeployments cross-release via the unchangedcluster.x-k8s.io/cluster-namelabel, namespace, and min/max annotations.cozystack.kubernetes-nodes-applicationPackageSource +kubernetes-nodes-rdApplicationDefinition, wired into the iaas bundle (mirrorsvm-instance/vm-disk).Downstream repositories
Left unticked deliberately — this is a draft. A new user-facing
KubernetesNodesAPI is added, so cozystack/website (docs) and cozystack/terraform-provider-cozystack likely need follow-ups; the trigger-map walk will be completed before this leaves draft.Summary by CodeRabbit