feat(kubernetes): propagate remote-accessible LINSTOR StorageClasses to tenant clusters - #2872

Code Review

This pull request implements StorageClass propagation from the infrastructure cluster to the tenant cluster, ensuring remote-accessible LINSTOR classes are auto-propagated under the same name while filtering out node-local classes and retaining the legacy 'kubevirt' alias. The changes include updates to Helm templates, schemas, documentation, and new unit and E2E tests. The feedback suggests quoting templated string values in the Helm templates to prevent potential YAML parsing issues.

Actionable comments posted: 1

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/templates/helmreleases/csi.yaml`:
- Line 37: The condition is using the wrong parameter key; update the Helm
template conditional that references $params to check "allowRemoteVolumeAccess"
(without the "linstor.csi.linbit.com/" prefix) instead of
"linstor.csi.linbit.com/allowRemoteVolumeAccess". Specifically, modify the
expression that currently reads (index $params
"linstor.csi.linbit.com/allowRemoteVolumeAccess" | default "true") to use (index
$params "allowRemoteVolumeAccess" | default "true") so the overall if-statement
with $sc.provisioner and the comparison to "false" reflects LINSTOR CSI’s actual
StorageClass parameter name.

Actionable comments posted: 1

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 `@hack/e2e-apps/run-kubernetes.sh`:
- Line 209: Remove the EXIT trap that invokes the _tenant_snapshot_on_fail
function (at line 209 and also at lines 643-644) as this violates the
repository's e2e shell scripting conventions which prohibit EXIT and RETURN
traps. Instead of relying on the trap for teardown/snapshot logic, explicitly
invoke the _tenant_snapshot_on_fail function at the appropriate failure handling
points in the script's control flow where snapshot capture is actually needed,
making the failure-path invocation explicit rather than trap-based.

Actionable comments posted: 1

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 `@hack/e2e-apps/run-kubernetes.sh`:
- Line 209: Remove the EXIT trap that invokes the _tenant_snapshot_on_fail
function (at line 209 and also at lines 643-644) as this violates the
repository's e2e shell scripting conventions which prohibit EXIT and RETURN
traps. Instead of relying on the trap for teardown/snapshot logic, explicitly
invoke the _tenant_snapshot_on_fail function at the appropriate failure handling
points in the script's control flow where snapshot capture is actually needed,
making the failure-path invocation explicit rather than trap-based.

NOT LGTM (REQUEST_CHANGES)

Business context

Tenant Kubernetes clusters previously got a single hardcoded kubevirt StorageClass. This auto-discovers the management cluster's remote-accessible LINSTOR classes via Helm lookup and propagates each into the tenant under the same name, keeping the legacy kubevirt class as a non-default alias.

I verified the design on the points that matter most. Tenant isolation is not widened: the kubevirt-csi-driver defaults to AllowAll enforcement, so a tenant could already reference any infra StorageClass; this change only auto-creates the convenience classes. Evidence: packages/apps/kubernetes/images/kubevirt-csi-driver/main.go:190-192 (empty enforcement string -> AllowAll: true). The filter parameter key linstor.csi.linbit.com/allowRemoteVolumeAccess is correct for this project: it matches the parameter form the LINSTOR classes actually use here, so a bare-key concern does not apply. Evidence: hack/e2e-post-install-prep.sh:82,95. Both helm-unittest suites (132 + 3) and the new e2e assertions render and pass locally.

One blocker remains.

Blockers

B1 - Fallback default can select the kubevirt alias pointing at a non-existent infra class

File: packages/apps/kubernetes/templates/helmreleases/csi.yaml:47-54

Evidence: The kubevirt alias (infraStorageClass: $defaultName, line 48) is inserted into $out before the default is chosen. When neither the configured storageClass nor replicated is among the propagated classes, the default is picked by keys $out | sortAlpha | first (line 54), and that candidate set still contains kubevirt. If every propagated class name sorts after kubevirt (for example nvme, ssd, standard), kubevirt becomes the tenant default with infraStorageClassName: <defaultName> (for example replicated), which by assumption does not exist on the management cluster.

Impact: The tenant's default StorageClass then provisions against a non-existent infra class - default PVCs stay Pending with no error surfaced, while the real working classes are left non-default. This is the feature's own target configuration (operators running multiple custom tiers without a replicated class). The standard path (a remote replicated class present) is unaffected and is covered by e2e, so this is not a regression - but the new fallback has the information to avoid the broken default and does not.

Fix: Choose the default among the propagated classes before inserting the kubevirt alias, or exclude kubevirt from the sortAlpha candidate set.

Non-blocking

1. Lookup result is invisible to the Flux release digest. Evidence: packages/apps/kubernetes/templates/helmreleases/csi.yaml:36. The propagated set is computed from live cluster state via lookup, but helm-controller re-renders on a digest of chart plus values that does not include lookup output. StorageClasses added to or removed from the management cluster after a tenant exists will not propagate or be pruned on a normal reconcile - only when the kubernetes HelmRelease inputs otherwise change. This limits the auto-discover behavior to create/update time. Consider routing the discovered set through valuesFrom (a controller-maintained ConfigMap) so changes enter the digest, or documenting the limitation.

2. allowRemoteVolumeAccess absent is treated as remote. Evidence: packages/apps/kubernetes/templates/helmreleases/csi.yaml:38 (| default "true"). A LINSTOR class whose layer list has no network-replication layer (for example layerList: storage without DRBD) is effectively node-local even with the parameter unset, so it would be propagated as if remote and its volumes could not follow a live-migrated VM. The project's own node-local class sets allowRemoteVolumeAccess: "false" explicitly, so in-convention setups are safe; operator-authored classes that omit the flag are the gap. Consider also gating on the layer list, or documenting the requirement.

3. Filter logic has no unit coverage. Evidence: packages/apps/kubernetes/tests/csi_test.yaml (header comment). helm-unittest has no live cluster, so the provisioner and allowRemoteVolumeAccess filter is exercised only by e2e; the unit tests cover the fallback map only. This is acknowledged in the test, and is acceptable, but the core selection has no fast-feedback coverage.

4. Quote templated scalars. Evidence: packages/system/kubevirt-csi-node/templates/deploy.yaml:274,282. name: {{ $name }} and infraStorageClassName: {{ $m.infraStorageClass }} render unquoted. StorageClass names are DNS-1123 so the risk is low, but | quote is the safer default.

5. Upgrade caveat lives only in the PR description. Evidence: PR body "Upgrade note". The instruction that a manually-created tenant StorageClass named replicated must be deleted before upgrade (to avoid a Helm ownership conflict) is operator-facing and is not in any persistent doc; I found no sibling website docs PR. Worth a durable note.

NOT LGTM (REQUEST_CHANGES) — re-review on the current head. The B1 fix is correct, but the behavior it fixes has no automated regression coverage, and that path is the feature's own target configuration.

What's resolved since the previous review

• B1 fixed (verified by inspection). Default selection now runs before the kubevirt alias is inserted into $out, so keys $out | sortAlpha | first chooses among the propagated classes only — the kubevirt alias can no longer be auto-selected as the tenant default. csi.yaml rule order confirms it.
• Non-blocking #4 addressed: name and infraStorageClassName are now | quoted.
• Non-blocking #5 addressed: the upgrade caveat now lives in packages/apps/kubernetes/README.md (durable doc), not just the PR body.
• Non-blocking #1 and #2 are now documented in that same README note (render-time lookup semantics; absent allowRemoteVolumeAccess treated as remote).

Blocker

B1-test: the fallback-default fix ships with no test pinning it

The fix is correct, but nothing guards it, and the branch it lives in — propagated classes present, none named the configured storageClass, none named replicated — is the exact multi-tier config this feature targets, with zero automated coverage today:

• helm-unittest can't reach it. With no live cluster, lookup returns empty, so $out is empty and every unit test falls into the replicated fallback. The two existing unit tests only exercise that fallback map — not the propagated-classes branch where B1 lived.
• e2e doesn't cover it either. The e2e provisions a setup where a remote replicated class is present, so the no-replicated multi-tier branch is never exercised.

So the regression that was just fixed can silently return. Since the logic is lookup-gated and not reachable by helm-unittest, the regression belongs in e2e: add a case that provisions >=2 remote LINSTOR classes with none named replicated (ideally names sorting after kubevirt, e.g. nvme/ssd), then asserts:

1. exactly one tenant default StorageClass,
2. the default is one of the propagated classes — never the kubevirt alias,
3. the kubevirt alias exists with default: false.

That locks the fix and documents the intended behavior for the multi-tier configuration. Everything else is ready.

LGTM — the test-completeness blocker is resolved.

The B1 fallback-default fix is intact at the current head: in csi.yaml, $chosen is picked before the kubevirt alias is inserted into $out, so keys $out | sortAlpha | first operates on a candidate set that never contains kubevirt — the alias can no longer become the tenant default.

And there is now a real regression test for the exact path I flagged. The new e2e case (hack/e2e-apps/kubernetes-latest.bats + verify_storageclass_fallback_default in run-kubernetes.sh) provisions two remote LINSTOR classes whose names sort after kubevirt, removes replicated for a single server-side dry-run render (so neither the configured class nor replicated is in the propagated set, forcing the sortAlpha | first branch), and asserts exactly one default, that it is nvme/ssd and never the kubevirt alias, and that the alias is present but non-default. It correctly exercises the lookup-gated branch that helm-unittest cannot reach — helm install --dry-run=server runs Helm lookup against the live cluster — and restores the management-cluster StorageClasses inline before any assertion can exit, per the e2e teardown rules. That is exactly the multi-tier, no-replicated coverage the fix needed.

Code-level checks (Build, Analyze (go), CodeQL, generated-code, pre-commit, DCO) are green; E2E is the separate merge gate and will exercise this new case. Approving.