fix(linstor-scheduler): re-vendor to upstream 0.3.1 with native admission webhook - #3201
Conversation
github-actions
Bot
added
area/storage
📝 WalkthroughWalkthroughThe linstor-scheduler chart is upgraded to Changeslinstor-scheduler admission upgrade
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
dosubot
Bot
added
area/virtualization
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict: request-changes
The fix is correct and needed, but the delivery mechanism will silently revert.
[MAJOR] appVersion bump is not durable — reverts on next make update
packages/system/linstor-scheduler/charts/linstor-scheduler/Chart.yaml:2
The appVersion is edited directly in the vendored chart, but there is no
corresponding entry in patches/*. The update target in the Makefile does
rm -rf charts && helm pull and re-applies only the two existing patches
(disable-ca-key-rotation, add-scheduler-component-label), neither of which
touches Chart.yaml. As you note yourself, no upstream chart ships appVersion
v0.3.5 yet. Result: the next make update silently reverts both containers to
v0.3.4 and the KubeVirt containerDisk regression returns with no error.
Requested changes
- Add
patches/bump-appversion-to-v0.3.5.patchcapturing theChart.yamlchange. - Apply it from the
updatetarget:
patch --no-backup-if-mismatch -p4 < patches/bump-appversion-to-v0.3.5.patch - Add a TODO above the line: drop the patch once piraeus-charts ships a chart
whose appVersion is >= v0.3.5.
Non-blocking follow-up: consider pinning the image via @sha256: (cozy-lib.image)
for air-gapped installs.
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
Approve — one-line, well-scoped fix.
Verified the mechanism: both the extender sidecar (deployment.yaml) and the admission webhook (admission-deployment.yaml) derive their image from extender.image.tag | default .Chart.AppVersion, extender.image.tag is empty, and this package sets admission.enabled: true, so the bump moves both containers to v0.3.5. Independently confirmed quay.io/piraeusdatastore/linstor-scheduler-extender:v0.3.5 exists (multi-arch manifest list, digest sha256:ccdc2fb4…, matching the PR body). No package version bump needed (Chart.yaml uses the 0.0.0 build-time placeholder) and there's no Helm-vendoring drift check in CI.
Non-blocking nit: the subchart's version stays 0.2.3 while appVersion is pinned 3 patch releases ahead of any published chart, and the package's make update target pulls the chart with no version pin. Until piraeus ships a chart whose appVersion >= v0.3.5, the next make update will silently regress this to v0.3.4 and look like a routine re-vendor. Worth a breadcrumb comment on the appVersion line so a future maintainer doesn't undo the fix by accident. Not a blocker.
b83ae69 to
16d6532
Compare
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM. The bump (appVersion v0.3.2 -> v0.3.5) is a correct, idempotent metadata change: the v0.3.5 image is pullable (multi-arch, digest matches), both the extender and admission deployments derive the tag from Chart.AppVersion, and the admission webhook is enabled at package level so the fix (#3198) actually applies. Upgrade rolls two deployments with no RBAC/schema/migration change.
On the earlier change-request: the concern about make update silently reverting appVersion is no longer reachable, since upstream piraeus now ships appVersion v0.3.5 as latest, so a re-vendor will not regress. The cleaner fix is still to run a real make update and commit the re-vendored chart rather than hand-editing Chart.yaml, so the vendored tree stays internally consistent. Non-blocking.
8fbe6a7
16d6532 to
8fbe6a7
Compare
github-actions
Bot
added
size/L
Aleksei Sviridkin (lexfrei)
changed the title
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 updates the linstor-scheduler component by aligning with the upstream 0.3.0 chart. By transitioning to the native admission webhook, the system resolves critical issues where unknown Pod fields were being stripped, which previously caused failures for KubeVirt containerDisk VMs and native sidecars. The changes include necessary migration logic for seamless upgrades, improved HA configurations, and robust test coverage to ensure future stability. 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
|
![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 upgrades the linstor-scheduler package to upstream version 0.3.0, adds a migration script (52 -> 53) to recreate the admission webhook deployment, and introduces comprehensive Helm unit tests. It also configures the scheduler to run with two replicas for high availability and enables automatic webhook reloading on certificate changes. Feedback is provided regarding a POSIX compatibility issue in the migration script, where the non-standard pipefail option is used with #!/bin/sh.
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.
| # creation. Safe when absent: fresh installs and clusters without the linstor | ||
| # stack simply skip it via --ignore-not-found. | ||
|
|
||
| set -euo pipefail |
There was a problem hiding this comment.
The script uses #!/bin/sh as its shebang, but specifies set -euo pipefail. The pipefail option is a non-standard shell option (a bashism) and is not supported by POSIX-compliant shells like dash (which is the default /bin/sh on Debian/Ubuntu systems). Running this script in such environments will cause it to fail with a set: Illegal option -o pipefail error.\n\nSince there are no pipelines used in this migration script, the pipefail option is redundant and can be safely removed to ensure POSIX compatibility.
| set -euo pipefail | |
| set -eu |
…sion webhook Re-vendor the linstor-scheduler chart from upstream 0.3.1 (appVersion v0.3.6), which carries the admission webhook binary that stopped stripping unknown Pod fields — the image VolumeSource and native-sidecar restartPolicy, which broke KubeVirt containerDisk VMs and native sidecars cluster-wide — and now reloads its TLS certificate on rotation instead of serving a stale one until restarted. The admission webhook ships in the upstream chart, so the previous downstream copy is dropped: `make update` pulls it directly and applies only cozystack-scheduler-deployment.patch (pod component label and a distro-suffix-stripped image tag), keeping the vendoring deterministic. Because the webhook now reloads its own certificate, the external cert-reload workaround is dropped. The upstream admission Deployment keeps the same name but changes its immutable pod selector, so an in-place upgrade of an existing install would fail. A migration deletes the old admission Deployment before the upgrade so the new one is recreated cleanly, and bumps the migration target version. Add a helm-unittest suite covering the admission resources, PDB selector isolation, least-privilege RBAC, the scheduler image tag, and backward compatibility. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
8fbe6a7 to
f4aef73
Compare
Aleksei Sviridkin (lexfrei)
changed the title
github-actions
Bot
added
the
area/uncategorized
Andrei Kvapil (kvaps)
left a comment
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
APPROVE — re-vendoring is byte-for-byte faithful, the field-stripping fix is real and delivered, and the upgrade path is safe. Verified against the actual upstream chart and the extender changelogs.
Vendoring fidelity — verified byte-for-byte
Reproduced make update exactly (helm pull piraeus-charts/linstor-scheduler --version 0.3.1 → rm -rf charts/linstor-scheduler/tests → patch -p4 < patches/cozystack-scheduler-deployment.patch) and diff -ru against the vendored copy in this PR: identical. So every admission template here (the createTLS enum, helm-mode genCA, dedicated ServiceAccount + PDB kept off the base scheduler's immutable selectors, the fail-guards, the readiness/liveness probes, the admission _helpers.tpl funcs) is genuine upstream 0.3.1 content from the merged piraeusdatastore/helm-charts#94 — not downstream code that would silently drift on the next re-vendor. appVersion: v0.3.6 matches upstream.
cozystack specifics preserved
The two old downstream patches were correctly consolidated, nothing dropped by accident:
add-scheduler-component-label.patch→ carried as hunk (a) ofcozystack-scheduler-deployment.patch(the wrapper'sextender-service.yamlselects scheduler pods byapp.kubernetes.io/component: scheduler).disable-ca-key-rotation.patch→ now the upstream default (admission.tls.certManager.rotationPolicy: Never), so correctly removed rather than re-applied.- New hunk (b) derives the kube-scheduler image tag from the distro-stripping
kubeVersionhelper; rendered output confirmsregistry.k8s.io/kube-scheduler:v1.34.0with no+k3s1/+rke2r1suffix. Pinned by a new test.
The webhook no longer strips unknown Pod fields — confirmed at the source
This is a binary-level fix, delivered by the v0.3.2 → v0.3.6 image bump the chart carries. Per the extender v0.3.5 changelog (#19): the webhook now returns a targeted RFC-6902 JSON Patch that only sets schedulerName, instead of deserializing the whole Pod into a struct built against an older k8s.io/api and re-serializing it. Because it no longer round-trips the object, all unknown fields survive — the native-sidecar initContainers[].restartPolicy and the image volume source alike. v0.3.6 then adds TLS cert hot-reload (#22). Rendered admission image is linstor-scheduler-extender:v0.3.6; failurePolicy: Ignore (fails open), timeoutSeconds: 5, admissionReviewVersions: ["v1"].
Cert hot-reload wired correctly
v0.3.6 serves its certificate via tls.Config.GetCertificate with content-hash reload across kubelet's atomic ..data swap, so the wrapper rightly leaves reloadOnCertChange: false and documents why. The vendored chart's own README/values text still says "does not hot-reload" — that's upstream text predating the v0.3.6 binary; leaving it unpatched avoids downstream drift and the wrapper values.yaml explains the discrepancy. No Reloader annotation is emitted (pinned by a test).
Upgrade safety
Migration 52 deletes the old linstor-scheduler-admission Deployment in cozy-linstor (namespace verified against sources/linstor-scheduler.yaml) before the component reconciles, because the upstream admission Deployment keeps the same name but changes its immutable pod selector — an in-place upgrade would fail field is immutable. --ignore-not-found makes it a no-op on fresh installs and clusters without the LINSTOR stack, and failurePolicy: Ignore covers the brief gap. Service / MutatingWebhookConfiguration / ClusterRole names are unchanged, so those update in place. The base scheduler 1 → 2 replicas also fixes a pre-existing drain deadlock (minAvailable: 1 PDB + 1 replica = undrainable node).
Render / tests
helm templaterenders the full stack cleanly (admission Deployment/Service/SA/PDB/MutatingWebhook + cert-manager Issuer/Certificate chain; base scheduler + extender Service).helm unittest: 22/22 passed across both suites.helm lint'smissing these dependencies: linstor-scheduleris the standard cozystack umbrella pattern (vendored subchart not declared underdependencies:) — pre-existing onmain, not introduced here.- CI is green; merge is BLOCKED only by
REVIEW_REQUIRED(this review), no red checks. E2E still pending at review time.
Clean, well-tested, faithfully-vendored consumption of an upstream fix. LGTM.
…pgrade (Phase 2, breaking) (#3315) ## What this PR does Phase 2 of the Kubernetes-app split ([cozystack/community#8](cozystack/community#8)) — the **breaking** half. Stacked on #3314 (review that first). Removes worker node pools from the `kubernetes` chart (they moved to `kubernetes-nodes` in #3314) and adopts existing pools on upgrade: - `kubernetes` chart is now **control-plane only**: drops the pool `range` block, the `talos-reconcile` Job + RBAC (deleted; moved to `kubernetes-nodes`), the preserve-old-KMT block, the `kubernetes.nodeGroups` helper, and the per-group dashboard RBAC. - **`spec.nodeGroups` is removed from the `Kubernetes` CR** (BREAKING). Worker pools are now separate `KubernetesNodes` resources. - **Migration 54** (bumps `targetVersion` to 55): pre-upgrade hook that, for every `kubernetes-<cluster>` release, creates one `kubernetes-nodes-<cluster>-<pool>` HelmRelease and re-annotates the existing worker objects onto it with `helm.sh/resource-policy=keep`, so the now control-plane-only parent upgrade does not prune running workers. The default `md0` (implicit when `nodeGroups` was empty) is materialised explicitly. Idempotent. Genuine infrastructure/read failures fail closed (`exit 1`, retried on the next upgrade); hostile stored shapes (a pool name over Helm's 53-char release-name cap, an RFC-1123-invalid or whitespace/slash map key, a non-object `nodeGroups`) are warned, pinned, and skipped rather than deadlocking the entire fleet's upgrade. - The `ingressNginx` addon no longer auto-provisions a node (the implicit `md0` default is gone). If enabled without a pool carrying `roles: [ingress-nginx]`, the controller DaemonSet simply schedules nothing (degraded, not failed) — the requirement is documented in the chart NOTES.txt rather than enforced at render time. Because the child chart renders the pool objects **byte-identically** to the old parent render (golden parity in #3314; the KubevirtMachineTemplate content-hash name is preserved), adoption does not churn live worker VMs. ### Migration number `54` is contiguous with main (latest is `53`) and `targetVersion` is `55`. The `52`/#3201 collision noted in earlier revisions has been resolved: this branch was rebased and merged past it, so the numbering is contiguous and `run-migrations.sh` has no gap to hard-fail on. ### Not yet done (blocking draft → ready) - [ ] **dev e2e upgrade validation** (`cozystack-pr-test`): upgrade a custom-`nodeGroups` cluster and a default-`md0` cluster; assert zero worker-VM churn (KMT hash, Machine uid/creationTimestamp stable) and that the adopted child HRs reconcile. - [ ] downstream trigger-map walk (website docs for the `nodeGroups` → `KubernetesNodes` change; terraform-provider for the removed API field). ### Downstream repositories Deliberately unticked (draft) — the breaking API change affects docs and the terraform provider; follow-ups to be filed before ready. - [ ] 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: ```release-note Worker node pools of a managed Kubernetes cluster are now managed as separate `KubernetesNodes` resources. `spec.nodeGroups` on the `Kubernetes` CR is removed; existing pools are adopted automatically on upgrade. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Worker pools are now separate `KubernetesNodes` resources with their own lifecycle. * Added a render-time compatibility guard to prevent worker pools from running ahead of the parent control-plane minor version. * Improved upgrade/uninstall behavior to better preserve already-adopted worker objects. * **Breaking Changes** * Removed per-pool `nodeGroups` and worker node health settings from the `Kubernetes` chart/CR; pool configuration now lives in `KubernetesNodes`. * Ingress requires at least one `KubernetesNodes` pool with `roles: [ingress-nginx]`. * `cluster` and `storageClass` are immutable after creation; pool `version` may lag but must not be ahead of the parent minor. * **Documentation** * Updated READMEs, notes, and schemas to reflect the `KubernetesNodes` split and the immutability/version rules. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…pgrade (Phase 2, breaking) (#3315) ## What this PR does Phase 2 of the Kubernetes-app split ([cozystack/community#8](cozystack/community#8)) — the **breaking** half. Stacked on #3314 (review that first). Removes worker node pools from the `kubernetes` chart (they moved to `kubernetes-nodes` in #3314) and adopts existing pools on upgrade: - `kubernetes` chart is now **control-plane only**: drops the pool `range` block, the `talos-reconcile` Job + RBAC (deleted; moved to `kubernetes-nodes`), the preserve-old-KMT block, the `kubernetes.nodeGroups` helper, and the per-group dashboard RBAC. - **`spec.nodeGroups` is removed from the `Kubernetes` CR** (BREAKING). Worker pools are now separate `KubernetesNodes` resources. - **Migration 54** (bumps `targetVersion` to 55): pre-upgrade hook that, for every `kubernetes-<cluster>` release, creates one `kubernetes-nodes-<cluster>-<pool>` HelmRelease and re-annotates the existing worker objects onto it with `helm.sh/resource-policy=keep`, so the now control-plane-only parent upgrade does not prune running workers. The default `md0` (implicit when `nodeGroups` was empty) is materialised explicitly. Idempotent. Genuine infrastructure/read failures fail closed (`exit 1`, retried on the next upgrade); hostile stored shapes (a pool name over Helm's 53-char release-name cap, an RFC-1123-invalid or whitespace/slash map key, a non-object `nodeGroups`) are warned, pinned, and skipped rather than deadlocking the entire fleet's upgrade. - The `ingressNginx` addon no longer auto-provisions a node (the implicit `md0` default is gone). If enabled without a pool carrying `roles: [ingress-nginx]`, the controller DaemonSet simply schedules nothing (degraded, not failed) — the requirement is documented in the chart NOTES.txt rather than enforced at render time. Because the child chart renders the pool objects **byte-identically** to the old parent render (golden parity in #3314; the KubevirtMachineTemplate content-hash name is preserved), adoption does not churn live worker VMs. ### Migration number `54` is contiguous with main (latest is `53`) and `targetVersion` is `55`. The `52`/#3201 collision noted in earlier revisions has been resolved: this branch was rebased and merged past it, so the numbering is contiguous and `run-migrations.sh` has no gap to hard-fail on. ### Not yet done (blocking draft → ready) - [ ] **dev e2e upgrade validation** (`cozystack-pr-test`): upgrade a custom-`nodeGroups` cluster and a default-`md0` cluster; assert zero worker-VM churn (KMT hash, Machine uid/creationTimestamp stable) and that the adopted child HRs reconcile. - [ ] downstream trigger-map walk (website docs for the `nodeGroups` → `KubernetesNodes` change; terraform-provider for the removed API field). ### Downstream repositories Deliberately unticked (draft) — the breaking API change affects docs and the terraform provider; follow-ups to be filed before ready. - [ ] 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: ```release-note Worker node pools of a managed Kubernetes cluster are now managed as separate `KubernetesNodes` resources. `spec.nodeGroups` on the `Kubernetes` CR is removed; existing pools are adopted automatically on upgrade. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Worker pools are now separate `KubernetesNodes` resources with their own lifecycle. * Added a render-time compatibility guard to prevent worker pools from running ahead of the parent control-plane minor version. * Improved upgrade/uninstall behavior to better preserve already-adopted worker objects. * **Breaking Changes** * Removed per-pool `nodeGroups` and worker node health settings from the `Kubernetes` chart/CR; pool configuration now lives in `KubernetesNodes`. * Ingress requires at least one `KubernetesNodes` pool with `roles: [ingress-nginx]`. * `cluster` and `storageClass` are immutable after creation; pool `version` may lag but must not be ahead of the parent minor. * **Documentation** * Updated READMEs, notes, and schemas to reflect the `KubernetesNodes` split and the immutability/version rules. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
4 participants
What this PR does
Re-vendors the
linstor-schedulerchart from upstream 0.3.1 (appVersion v0.3.6). Upstream now ships the admission webhook natively (contributed via piraeusdatastore/helm-charts#94), so the loose downstream admission templates — which round-tripped every Pod through an oldk8s.io/apiand silently stripped fields they did not know about, theimagevolume source and the native-sidecarrestartPolicyamong them, breaking KubeVirt containerDisk VMs and native sidecars cluster-wide — are dropped in favour of the upstream copy.make updatepulls 0.3.1 and applies onlycozystack-scheduler-deployment.patch(the scheduler pod component label and the distro-suffix-stripped kube-scheduler image tag, neither of which is upstream yet), so vendoring stays deterministic and reproducible.field is immutable. Migration 52 deletes the old admission Deployment before the upgrade so the new one is recreated cleanly (targetVersion52 → 53).failurePolicy: Ignore— silently stopped mutating until it was restarted. It now reloads on rotation, so no external cert-reload workaround is needed here.replicaCount: 2, now that the admission Pods have their own PodDisruptionBudget and no longer share the scheduler's.Closes #3198.
Screenshots
N/A — no UI changes.
Downstream repositories
I walked the trigger map against the diff, file by file. It touches
packages/system/linstor-scheduler/and, underpackages/core/platform/, only migration 52 and themigrations.targetVersionbump — an internal, release-managed counter, not a user-facingspec.components.platform.values.*key documented on the website. No trigger matches.Release note
Summary by CodeRabbit
schedulerNamemutation for LINSTOR CSI-backed PVC Pods.