Repository navigation
fix(longhorn): default to a WaitForFirstConsumer StorageClass - #2421
Conversation
The default longhorn StorageClass uses volumeBindingMode: Immediate, so a PVC binds and its Longhorn volume is provisioned before the consuming pod is scheduled. That drags stateful pods onto whatever node the volume landed on — including autoscaler overflow nodes — which then can never scale down, and it breaks the cluster-autoscaler's scale-up simulation. Add a sibling longhorn-wffc StorageClass (WaitForFirstConsumer, identical parameters) and make it the cluster default; demote the chart-managed longhorn class to non-default but keep it, since existing PVCs and CNPG clusters name it directly. Existing volumes are untouched; migrating them onto longhorn-wffc is a deliberate per-workload follow-up (reclaimPolicy is Delete — no bulk PVC deletes). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR changes the default Longhorn StorageClass setup on Hetzner. It adds a new ChangesLonghorn StorageClass Defaulting
Estimated code review effort: 2 (Simple) | ~10 minutes Related issues: None provided. Related PRs: None provided. Suggested labels: kubernetes, storage, hetzner Suggested reviewers: devantler 🐰 A poem for this PRA burrow needs its default den, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
k8s/providers/hetzner/infrastructure/controllers/longhorn/storage-class-longhorn-wffc.yaml (1)
27-28: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAvoid manual sync: reuse the templated replica-count variable.
numberOfReplicas: "3"is hardcoded here, while the siblinglonghornclass in helm-release.yaml drives the same value via${longhorn_replica_count:=3}. The comment on Line 27 acknowledges this requires manual sync — that's an avoidable drift risk if the variable is ever changed without updating this file.♻️ Suggested fix using the shared variable
- # Mirrors the chart default (longhorn_replica_count, default 3); keep in sync. - numberOfReplicas: "3" + numberOfReplicas: "${longhorn_replica_count:=3}"If this directory isn't subject to the same variable-substitution pipeline as helm-release.yaml, disregard this suggestion.
🤖 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 `@k8s/providers/hetzner/infrastructure/controllers/longhorn/storage-class-longhorn-wffc.yaml` around lines 27 - 28, The Longhorn storage class is hardcoding the replica count instead of reusing the shared templated value, which risks drift from the sibling configuration. Update the `numberOfReplicas` setting in the Longhorn storage class to use the same `${longhorn_replica_count:=3}` substitution pattern used by the `longhorn` class in `helm-release.yaml`, keeping the comment and configuration in sync via the shared variable.
🤖 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.
Nitpick comments:
In
`@k8s/providers/hetzner/infrastructure/controllers/longhorn/storage-class-longhorn-wffc.yaml`:
- Around line 27-28: The Longhorn storage class is hardcoding the replica count
instead of reusing the shared templated value, which risks drift from the
sibling configuration. Update the `numberOfReplicas` setting in the Longhorn
storage class to use the same `${longhorn_replica_count:=3}` substitution
pattern used by the `longhorn` class in `helm-release.yaml`, keeping the comment
and configuration in sync via the shared variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b42402d8-aba8-4f7f-9292-8d2c6ae8359a
📒 Files selected for processing (1)
k8s/providers/hetzner/infrastructure/controllers/longhorn/storage-class-longhorn-wffc.yaml
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
k8s/providers/**/*.yaml
📄 CodeRabbit inference engine (AGENTS.md)
Use Kustomize
patches:in provider overlays instead of directly editing base resources; provider overlays are the correct place for environment-specific changes.
Files:
k8s/providers/hetzner/infrastructure/controllers/longhorn/storage-class-longhorn-wffc.yaml
k8s/**/*.{yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Use one Kubernetes resource per file, except for the explicitly whitelisted vendored operator bundles.
Files:
k8s/providers/hetzner/infrastructure/controllers/longhorn/storage-class-longhorn-wffc.yaml
k8s/**/{controllers,infrastructure}/**/*.yaml
📄 CodeRabbit inference engine (AGENTS.md)
Use kebab-case directory names and resource-led filenames; CR folders may use intent-describing
‹verb›-‹purpose›.yamlnames and must stay kebab-case.
Files:
k8s/providers/hetzner/infrastructure/controllers/longhorn/storage-class-longhorn-wffc.yaml
🧠 Learnings (1)
📚 Learning: 2026-07-01T21:13:36.950Z
Learnt from: devantler
Repo: devantler-tech/platform PR: 2359
File: k8s/bases/apps/actual-budget/helm-release.yaml:62-111
Timestamp: 2026-07-01T21:13:36.950Z
Learning: When reviewing Kustomize/Helm YAML in this repo, keep the base vs provider overlay split: `k8s/bases/apps/**` and `k8s/bases/infrastructure/**` should contain each app’s full, environment-agnostic configuration (including base-level postRenderer Kustomize patches such as deployment strategy, topology spread, probes, and env injection). `k8s/providers/{docker,hetzner}/**` should only add small provider-specific deltas (e.g., `interval`, `persistence.size`) via patch files (like `k8s/providers/<provider>/apps/<app>/patches/helm-release-patch.yaml`). If configuration is identical across providers (e.g., OIDC/OAuth env vars where `${domain}` is resolved per cluster via envsubst), it belongs in the base and must not be duplicated into provider overlays.
Applied to files:
k8s/providers/hetzner/infrastructure/controllers/longhorn/storage-class-longhorn-wffc.yaml
🔇 Additional comments (1)
k8s/providers/hetzner/infrastructure/controllers/longhorn/storage-class-longhorn-wffc.yaml (1)
1-26: LGTM!Also applies to: 29-37
Re: CodeRabbit nitpick — Keeping the literal
Leaving line 27-28 as-is. |
|
🎉 This PR is included in version 1.96.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Why
The default
longhornStorageClass usesvolumeBindingMode: Immediate, so a PVC binds and its volume is provisioned before the pod is scheduled. Stateful pods then get dragged onto whatever node the volume landed on — including autoscaler overflow nodes — where they block scale-down and defeat the restrict-storage-to-baseline-workers intent. It also breaks the cluster-autoscaler’s scale-up simulation.What
Adds a
longhorn-wffcStorageClass (WaitForFirstConsumer, identical Longhorn parameters) and makes it the cluster default; demotes the chart’slonghornclass to non-default but keeps it. New default-SC PVCs now bind lazily so the scheduler places the pod first and the volume follows.Safe rollout — no data loss (this is Phase 1 only)
longhornclass is kept (CNPG clusters and 14 live PVCs reference it by name); only its default annotation moves.longhorn-wffcone at a time via CNPG replica-recreation / Longhorn backup→restore, verifying data each time. Never bulk-delete PVCs —reclaimPolicy: Deletedestroys the volume.longhornSC still exists (non-default) andlonghorn-wffcis the sole default before anything creates new PVCs.Pairs with the autoscaler scale-down knobs (ksail#5756) and the cx33 removal (#2420) to complete node right-sizing.