Repository navigation
fix(kubernetes): make the default md0 node group removable - #2753
mattia-eleuteri wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughChart values now default ChangesNode Groups Default Provisioning
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.69.3)Trivy execution failed: 2026-06-10T12:49:13Z 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 🔧 golangci-lint (2.12.2)Command failed 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 |
github-actions
Bot
added
area/kubernetes
|
MetalLB reconcile/VIP-flap issue tracked in #2754 — relevant for migration 34's maintenance-window note. |
|
Validated on a live lab cluster with `cozyhr`:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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 addresses a limitation in the Kubernetes chart where the default 'md0' node group could not be removed due to Helm's deep-merging behavior. By shifting the default 'md0' definition into a template helper that only triggers when no node groups are configured, users gain full control over their node group topology. A migration script has been included to ensure existing clusters maintain their current state by explicitly pinning 'md0' before the change takes effect. 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. 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
|
![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 refactors how the default md0 worker node group is handled in the Kubernetes application package. Instead of baking the default md0 group into values.yaml (which prevents users from fully overriding node groups due to Helm merging behavior), the default is now dynamically applied via a template helper only when nodeGroups is empty. A migration script (migrations/34) is added to explicitly pin the md0 group on existing clusters to preserve their topology. The review feedback highlights two issues in the migration script: a shell compatibility issue where pipefail is used with a #!/bin/sh shebang, and a fragile check using grep -q . to detect existing node groups which could be bypassed by null or <nil> outputs.
| @@ -0,0 +1,47 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
The script uses set -euo pipefail on line 20, but the shebang is #!/bin/sh. The pipefail option is a bash extension and is not supported by standard POSIX shells (like dash, which is the default /bin/sh on Debian/Ubuntu). This will cause the migration script to fail immediately with an error like set: Illegal option -o pipefail at runtime.
Please change the shebang to #!/bin/bash to ensure the script runs correctly with pipefail enabled.
| #!/bin/sh | |
| #!/bin/bash |
| if kubectl -n "$ns" get kuberneteses.apps.cozystack.io "$name" \ | ||
| -o jsonpath='{.spec.nodeGroups.md0}' 2>/dev/null | grep -q .; then |
There was a problem hiding this comment.
Using grep -q . to check if md0 is defined can be fragile. Depending on the kubectl version and the exact state of the resource, a missing or null field might be output as null or <nil>, which grep -q . will incorrectly match as a valid value (since they are non-empty strings). This would cause the script to skip pinning md0 on clusters that actually need it.
Additionally, we can avoid spawning a subshell and grep process by capturing the output directly in a shell variable and performing a robust check.
| if kubectl -n "$ns" get kuberneteses.apps.cozystack.io "$name" \ | |
| -o jsonpath='{.spec.nodeGroups.md0}' 2>/dev/null | grep -q .; then | |
| res=$(kubectl -n "$ns" get kuberneteses.apps.cozystack.io "$name" -o jsonpath='{.spec.nodeGroups.md0}' 2>/dev/null) | |
| if [ -n "$res" ] && [ "$res" != "null" ] && [ "$res" != "<nil>" ]; then |
Important: existing-cluster prune behaviour (and why migration 34 is required)On existing clusters the Migration 34 prevents the prune by pinning
Ordering: migration 34 runs via the platform chart's |
![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/core/platform/images/migrations/migrations/34`:
- Around line 27-29: The kubectl listing command (kubectl get
kuberneteses.apps.cozystack.io -A ...) can fail under set -euo pipefail when the
CRD is absent, causing the migration to abort; change the logic to capture the
kubectl output safely (e.g. output=$(kubectl get kuberneteses.apps.cozystack.io
-A -o jsonpath=... 2>/dev/null || true)), then check if output is empty and
treat that as a successful no-op (exit/return success) otherwise pipe the output
into the existing while read -r key loop to continue processing; reference the
existing kubectl get ... command and the while read -r key loop when making the
change.
🪄 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: eb08a3d3-c7d1-4985-b9c9-0f9c6a324efe
📒 Files selected for processing (8)
packages/apps/kubernetes/README.mdpackages/apps/kubernetes/templates/_helpers.tplpackages/apps/kubernetes/templates/cluster.yamlpackages/apps/kubernetes/templates/dashboard-resourcemap.yamlpackages/apps/kubernetes/values.schema.jsonpackages/apps/kubernetes/values.yamlpackages/core/platform/images/migrations/migrations/34packages/system/kubernetes-rd/cozyrds/kubernetes.yaml
| kubectl get kuberneteses.apps.cozystack.io -A \ | ||
| -o jsonpath='{range .items[*]}{.metadata.namespace}/{.metadata.name}{"\n"}{end}' \ | ||
| | while read -r key; do |
There was a problem hiding this comment.
Handle missing CRD/no-instance list as a successful no-op.
With set -euo pipefail, kubectl get kuberneteses.apps.cozystack.io -A fails hard when the CRD is absent, so the migration exits instead of completing successfully.
💡 Suggested fix
-kubectl get kuberneteses.apps.cozystack.io -A \
- -o jsonpath='{range .items[*]}{.metadata.namespace}/{.metadata.name}{"\n"}{end}' \
- | while read -r key; do
+kubectl get kuberneteses.apps.cozystack.io -A \
+ -o jsonpath='{range .items[*]}{.metadata.namespace}/{.metadata.name}{"\n"}{end}' 2>/dev/null \
+ | while read -r key; do
[ -z "$key" ] && continue
ns="${key%%/*}"
name="${key##*/}"
@@
- done
+ done || trueBased on learnings: "if a kubectl list returns no results (CRD not present or no instances), the migration should complete successfully without requiring an explicit CRD existence check."
🤖 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/core/platform/images/migrations/migrations/34` around lines 27 - 29,
The kubectl listing command (kubectl get kuberneteses.apps.cozystack.io -A ...)
can fail under set -euo pipefail when the CRD is absent, causing the migration
to abort; change the logic to capture the kubectl output safely (e.g.
output=$(kubectl get kuberneteses.apps.cozystack.io -A -o jsonpath=...
2>/dev/null || true)), then check if output is empty and treat that as a
successful no-op (exit/return success) otherwise pipe the output into the
existing while read -r key loop to continue processing; reference the existing
kubectl get ... command and the while read -r key loop when making the change.
Deterministic manifest-level proof of the prune (and mitigation)Verified against a live cluster's Helm release for an app whose CR defines only
Note: a full live upgrade (watching the node) still needs a cluster that can provision healthy tenant clusters; this is best confirmed on staging in a maintenance window. An alternative design worth discussing: have the chart keep |
7ad9da0 to
feccf3a
Compare
|
Rebased onto latest `main` and addressed review feedback:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Find migration 43 file
fd -t f '43' packages/core/platform/images/migrations/migrations/
echo "---"
cat packages/core/platform/images/migrations/migrations/43 2>/dev/null || echo "not found at that path"
echo "---"
# Also check if there's a different path structure
find packages/core/platform/images/migrations -type f | sortLength of output: 6870
The migration-before-chart ordering via 🐇 ✓ [resolve] |
feccf3a to
1e21392
Compare
|
The previous E2E run failed on the 3 tenant-Kubernetes tests (`kubernetes-latest`, `kubernetes-previous`, `ouroboros`). Root cause was the same pattern as the unit-test fixtures: ```
The E2E test CRs (`hack/e2e-apps/run-kubernetes.sh`, `hack/e2e-apps/ouroboros.bats`) defined `md0` without `resources` (and ouroboros also without `gpus`). On main, the chart's values.yaml default `md0` supplied those required fields via Helm values merge; removing the default in this PR exposed that the test CRs relied on it. Updated both fixtures to declare those fields explicitly. Re-running CI. |
1e21392 to
5462dea
Compare
|
Re-rebased onto latest main (PRs #2728 / #2759 / #2726 / #2743 landed). |
There was a problem hiding this comment.
Code Review
This pull request refactors how the default md0 worker node group is handled in the Kubernetes application chart. Instead of defining the default md0 group in values.yaml (which interferes with Helm values merging when users specify custom node groups), the default is now dynamically applied in the Helm templates via a helper when nodeGroups is left empty. To ensure backward compatibility and prevent existing tenant clusters from losing their md0 node group, a new platform migration script (43) has been added to explicitly pin the md0 configuration on existing clusters, and the platform's target migration version has been bumped to 44. There are no review comments, and I have no feedback to provide.
|
Actionable comments posted: 0 |
|
This has gone stale against 1. Migration number collision.
The runner loops 2. Generated cozyrds conflict — after rebasing, Minor: the commit title says "add migration 43" — worth updating to 44 once renumbered. |
… empty
The chart shipped a default nodeGroups.md0 in values.yaml. Helm deep-merges
values, so the default md0 was re-added on top of any user-supplied
nodeGroups; combined with Kubernetes stripping null values, md0 could never
be removed by configuration. Move the default into the template
("kubernetes.nodeGroups" helper) so it is applied only when nodeGroups is
empty. User-supplied node groups now fully replace the default, letting users
name and omit groups (including md0) freely.
Test fixtures that relied on the chart-default md0 to supply diskSize via
values merge now set it explicitly.
Signed-off-by: mattia-eleuteri <[email protected]>
After the kubernetes chart stops force-merging a default md0 node group, clusters that relied on the implicit md0 (defined other nodeGroups without md0) would have their live md0 MachineDeployment pruned by Helm on the next reconcile. Migration 44 makes md0 explicit on those clusters to preserve the current node topology; it is idempotent, treats a missing CRD as a no-op, and skips clusters that already define md0. Signed-off-by: mattia-eleuteri <[email protected]>
5462dea to
d73d1d9
Compare
|
myasnikovdaniil done as instructed: rebased onto latest |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/core/platform/images/migrations/migrations/44`:
- Around line 97-99: The script currently exits with code 0 when FAILURES > 0
which signals success; change the exit behavior so that when the FAILURES
variable is greater than zero the script exits with a non-zero status (e.g.,
exit 1) instead of exit 0 to ensure run-migrations.sh treats the migration as
failed; update the branch that prints "WARN: $FAILURES patch(es) failed; leaving
CURRENT_VERSION at 44 for retry on next upgrade." to call exit 1 while keeping
the warning message and leaving CURRENT_VERSION untouched.
- Around line 102-104: The migration script hard-codes the namespace
"cozy-system" when creating the cozystack-version ConfigMap, which breaks
non-default installs; change the kubectl command to use the release namespace
used elsewhere (the same value passed as NAMESPACE / .Release.Namespace) so the
ConfigMap is created/updated in the release namespace (cozystack-version) rather
than always in cozy-system; update the command invocation that references
cozystack-version to substitute the release namespace variable
(.Release.Namespace or the NAMESPACE env var) instead of the literal
"cozy-system".
- Around line 55-56: The current line that runs obj=$(kubectl get
kuberneteses.apps.cozystack.io ... 2>/dev/null || true) swallows all kubectl
errors and treats them as SKIPPED; change it to capture kubectl's exit status
and stderr and only increment SKIPPED when the error is an actual NotFound
(e.g., "NotFound" / 404 from the API). Specifically, replace the unconditional
2>/dev/null || true pattern around the kubectl get (the obj assignment) with
logic that records the command's exit code, inspects stderr/output for a
NotFound indication, and: if rc==0 proceed using obj, if error indicates
NotFound increment SKIPPED and continue, otherwise surface/log the error and
exit non‑zero so transient API errors don't get treated as successful skips.
🪄 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: be0a9fdf-949a-449a-b2fa-44d5feccb962
📒 Files selected for processing (12)
api/apps/v1alpha1/kubernetes/types.gohack/e2e-apps/run-kubernetes.shpackages/apps/kubernetes/README.mdpackages/apps/kubernetes/templates/_helpers.tplpackages/apps/kubernetes/templates/cluster.yamlpackages/apps/kubernetes/templates/dashboard-resourcemap.yamlpackages/apps/kubernetes/tests/values-ci.yamlpackages/apps/kubernetes/tests/values/common.yamlpackages/apps/kubernetes/values.schema.jsonpackages/apps/kubernetes/values.yamlpackages/core/platform/images/migrations/migrations/44packages/core/platform/values.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/core/platform/values.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/apps/kubernetes/templates/cluster.yaml
- hack/e2e-apps/run-kubernetes.sh
- api/apps/v1alpha1/kubernetes/types.go
- packages/apps/kubernetes/README.md
| obj=$(kubectl get kuberneteses.apps.cozystack.io --namespace "$ns" "$name" --output json 2>/dev/null || true) | ||
| if [ -z "$obj" ]; then SKIPPED=$((SKIPPED + 1)); return 0; fi |
There was a problem hiding this comment.
Don't collapse kubectl get failures into a skip.
If this per-object read hits a transient API error, 2>/dev/null || true turns it into an empty string, increments SKIPPED, and lets the migration keep going. The script can then still stamp version 45 even though that cluster never got md0, which reintroduces the prune risk this migration is meant to prevent.
Suggested fix
- obj=$(kubectl get kuberneteses.apps.cozystack.io --namespace "$ns" "$name" --output json 2>/dev/null || true)
- if [ -z "$obj" ]; then SKIPPED=$((SKIPPED + 1)); return 0; fi
+ if ! obj=$(kubectl get kuberneteses.apps.cozystack.io --namespace "$ns" "$name" --output json 2>&1); then
+ case "$obj" in
+ *"NotFound"*)
+ SKIPPED=$((SKIPPED + 1))
+ return 0
+ ;;
+ esac
+ echo "WARN: failed to read $ns/$name: $obj" >&2
+ FAILURES=$((FAILURES + 1))
+ return 0
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| obj=$(kubectl get kuberneteses.apps.cozystack.io --namespace "$ns" "$name" --output json 2>/dev/null || true) | |
| if [ -z "$obj" ]; then SKIPPED=$((SKIPPED + 1)); return 0; fi | |
| if ! obj=$(kubectl get kuberneteses.apps.cozystack.io --namespace "$ns" "$name" --output json 2>&1); then | |
| case "$obj" in | |
| *"NotFound"*) | |
| SKIPPED=$((SKIPPED + 1)) | |
| return 0 | |
| ;; | |
| esac | |
| echo "WARN: failed to read $ns/$name: $obj" >&2 | |
| FAILURES=$((FAILURES + 1)) | |
| return 0 | |
| fi |
🤖 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/core/platform/images/migrations/migrations/44` around lines 55 - 56,
The current line that runs obj=$(kubectl get kuberneteses.apps.cozystack.io ...
2>/dev/null || true) swallows all kubectl errors and treats them as SKIPPED;
change it to capture kubectl's exit status and stderr and only increment SKIPPED
when the error is an actual NotFound (e.g., "NotFound" / 404 from the API).
Specifically, replace the unconditional 2>/dev/null || true pattern around the
kubectl get (the obj assignment) with logic that records the command's exit
code, inspects stderr/output for a NotFound indication, and: if rc==0 proceed
using obj, if error indicates NotFound increment SKIPPED and continue, otherwise
surface/log the error and exit non‑zero so transient API errors don't get
treated as successful skips.
| if [ "$FAILURES" -gt 0 ]; then | ||
| echo "WARN: $FAILURES patch(es) failed; leaving CURRENT_VERSION at 44 for retry on next upgrade." | ||
| exit 0 |
There was a problem hiding this comment.
Return non-zero when any patch fails.
run-migrations.sh treats exit code 0 as “migration completed” and immediately continues to later migration numbers in the same job. Once a future release sets targetVersion > 45, this script will allow migration 45+ to run even though 44 explicitly did not finish.
Suggested fix
if [ "$FAILURES" -gt 0 ]; then
echo "WARN: $FAILURES patch(es) failed; leaving CURRENT_VERSION at 44 for retry on next upgrade."
- exit 0
+ exit 1
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ "$FAILURES" -gt 0 ]; then | |
| echo "WARN: $FAILURES patch(es) failed; leaving CURRENT_VERSION at 44 for retry on next upgrade." | |
| exit 0 | |
| if [ "$FAILURES" -gt 0 ]; then | |
| echo "WARN: $FAILURES patch(es) failed; leaving CURRENT_VERSION at 44 for retry on next upgrade." | |
| exit 1 | |
| fi |
🤖 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/core/platform/images/migrations/migrations/44` around lines 97 - 99,
The script currently exits with code 0 when FAILURES > 0 which signals success;
change the exit behavior so that when the FAILURES variable is greater than zero
the script exits with a non-zero status (e.g., exit 1) instead of exit 0 to
ensure run-migrations.sh treats the migration as failed; update the branch that
prints "WARN: $FAILURES patch(es) failed; leaving CURRENT_VERSION at 44 for
retry on next upgrade." to call exit 1 while keeping the warning message and
leaving CURRENT_VERSION untouched.
| kubectl create configmap --namespace cozy-system cozystack-version \ | ||
| --from-literal version=45 --dry-run=client --output yaml \ | ||
| | kubectl apply --filename - |
There was a problem hiding this comment.
Use the release namespace for cozystack-version.
The hook and the template both read/write this ConfigMap in .Release.Namespace and pass that value in NAMESPACE. Hard-coding cozy-system here updates the wrong object for non-default installs, so the real release namespace stays at version 44 and reruns migrations forever.
Suggested fix
-kubectl create configmap --namespace cozy-system cozystack-version \
+kubectl create configmap --namespace "${NAMESPACE:?NAMESPACE is required}" cozystack-version \
--from-literal version=45 --dry-run=client --output yaml \
| kubectl apply --filename -📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kubectl create configmap --namespace cozy-system cozystack-version \ | |
| --from-literal version=45 --dry-run=client --output yaml \ | |
| | kubectl apply --filename - | |
| kubectl create configmap --namespace "${NAMESPACE:?NAMESPACE is required}" cozystack-version \ | |
| --from-literal version=45 --dry-run=client --output yaml \ | |
| | kubectl apply --filename - |
🤖 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/core/platform/images/migrations/migrations/44` around lines 102 -
104, The migration script hard-codes the namespace "cozy-system" when creating
the cozystack-version ConfigMap, which breaks non-default installs; change the
kubectl command to use the release namespace used elsewhere (the same value
passed as NAMESPACE / .Release.Namespace) so the ConfigMap is created/updated in
the release namespace (cozystack-version) rather than always in cozy-system;
update the command invocation that references cozystack-version to substitute
the release namespace variable (.Release.Namespace or the NAMESPACE env var)
instead of the literal "cozy-system".
Same-repo mirror of #2753 by @mattia-eleuteri, opened so the `Build` CI job can run. Fork PRs skip the OCIR registry login (`if: !github.event.pull_request.head.repo.fork` in `pull-requests.yaml`) and so can't push the per-PR images the build needs — meaning a fork PR can never pass `Build`, regardless of the change. The commits here are unchanged and authored by @mattia-eleuteri; full description and discussion in #2753. This includes migration **44** (renumbered from 43 after the seaweedfs-db migration landed on `main`) and bumps `targetVersion` to **45**. Supersedes #2753. ```release-note fix(kubernetes): the default `md0` node group is now applied only when no node groups are configured, so it can be removed or replaced. Migration 44 pins `md0` explicitly on existing clusters to preserve their topology. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Empty `nodeGroups` settings now automatically provision a default `md0` worker group. * Existing environments are updated to keep this default group available during upgrades. * **Bug Fixes** * User-defined node groups now fully replace the default instead of being merged with it. * Improved handling of node-group resources and permissions in Kubernetes app rendering. * **Documentation** * Updated Kubernetes app docs and schema details to reflect the new default behavior. * **Tests** * Added coverage for default and custom node-group configurations, plus migration behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
Closing: this is effectively already in main. The default md0 node group is now applied only when |
3 participants
What this PR does
Today the
kuberneteschart ships a defaultnodeGroups.md0invalues.yaml. Because Helm deep-merges values, thatmd0is re-added on top of any user-suppliednodeGroups, and since Kubernetes stripsnullvalues it can never be removed by configuration.md0into akubernetes.nodeGroupstemplate helper, applied only whennodeGroupsis empty. User-supplied node groups now fully replace the default — users can name and omit groups (includingmd0) freely. Used by bothcluster.yamlanddashboard-resourcemap.yaml. Regenerated schema/README/-rdCRD.md0explicit on existing clusters that relied on the implicit default, to preserve their current node topology on the next reconcile. Idempotent; treats a missing CRD as a no-op; skips clusters that already definemd0. Stamps version 45;migrations.targetVersionbumped to 45.Notes for reviewers
md0will no longer getmd0. On existing clusters the implicitmd0MachineDeployment is Helm-tracked, so without the migration it would be pruned (→ CAPI node deletion). Migration 44 pins it explicitly first (platformpre-upgradehook ordering), so existing clusters are unaffected; operators can then removemd0where unwanted.kubernetesapp currently causes MetalLB L2 VIP re-announcement (brief edge outage, see Reconciling a tenant kubernetes app triggers MetalLB L2 VIP re-announcement and brief edge outage #2754) — until that is fixed, the migration should run during a maintenance window.Release note
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Refactor
Tests
Chores