[Hrishikesh Kashyap (Hrishikeshkashyap)]

Durability placement remains enforceable after binding and during node staging, rather than relying only on Pod admission.

Changes

• Persist the effective failover mode and initial owner during CreateVolume.
• Retain AccessibleTopology for durability volumes while preserving availability-mode recovery.
• Resolve PVs by indexed CSI driver and volume handle, including when the PV name differs from the logical volume name.
• Resolve durability ownership from retained PV topology when owner annotations are unavailable, and reject wrong-owner or missing-owner staging.
• Reuse existing durability LVs without allocating replacements; preserve availability-mode create-if-missing recovery.
• Use PV storage capacity when private capacity metadata is missing, including for static availability PVs.
• Permit staging of existing LVs during a pending resize, while preserving controller provisioning size validation and upper-limit checks.
• Keep orphan cleanup consistent with staging when selected-node is empty, falling back to the initial owner.
• Persist the actual expanded LV capacity on the PV when available, and preserve expansion for volumes without a PV.

Tests

Regression coverage includes CSI identity lookup, topology and ownership fallback, static PV capacity, pending resize, orphan cleanup, expansion annotation persistence, and PV-less expansion.

[Copilot]

🟡 Changes recommended

Both ownership fallbacks ignore the driver's actual CSI topology key, breaking resolution from retained PV topology.

2 open findings

• Use the driver's topology key for ownerless LV staging · New
• Use the driver's topology key for durability affinity injection · New

What changed in this PR

Enforces durability ownership beyond Pod admission by retaining PV topology and requiring existing storage during CSI staging.

Changes:

• Persists failover policy and initial ownership during provisioning.
• Adds staging ownership checks and an existing-only volume recovery path.
• Extends tests and documents durability behavior.

File	Description
internal/​webhook/​hyperconverged/​handler.go	Adds retained-topology placement fallback.
internal/​webhook/​hyperconverged/​handler_unit_test.go	Tests topology fallback.
internal/​csi/​node/​node.go	Enforces ownership and existing-only staging.
internal/​csi/​node/​node_test.go	Tests durability staging and failures.
internal/​csi/​core/​lvm/​node.go	Exposes existing-only volume preparation.
internal/​csi/​core/​lvm/​lvm.go	Prevents allocation when existing storage is required.
internal/​csi/​core/​lvm/​lvm_test.go	Tests reuse and missing-volume behavior.
internal/​csi/​core/​interfaces.go	Adds shared policies and existing-only interface.
internal/​csi/​core/​fake.go	Tracks recovery-path calls for tests.
internal/​csi/​controller/​controller.go	Persists policy and retains durability topology.
internal/​csi/​controller/​controller_test.go	Tests durability topology retention.
docs/​user-guide.md	Explains durability guarantees.
docs/​design/​webhooks.md	Documents scheduling and staging enforcement.
docs/​design/​pv-recovery.md	Distinguishes recovery policies.
docs/​architecture.md	Summarizes durability enforcement.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Topology fallback can authorize excluded nodes, and metadata lookup can reject valid pre-provisioned durability volumes.

2 open findings

• Evaluate all topology expressions with Kubernetes AND/OR semantics · New
• Resolve durability metadata by CSI volume handle, not LV name · New

2 resolved since last review

• Use the driver's topology key for durability affinity injection
• Use the driver's topology key for ownerless LV staging

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Unresolved identity resolution, static-volume capacity, and topology-label issues can select unsafe recovery behavior or block valid workloads.

3 open findings

• Disambiguate all matching PVs before selecting policy · New
• Fall back to PV storage capacity for existing static volumes · New
• Preserve driver topology keys when constructing Pod affinity · New

2 resolved since last review

• Evaluate all topology expressions with Kubernetes AND/OR semantics
• Resolve durability metadata by CSI volume handle, not LV name

🧠 Review effort: Balanced

[Hrishikesh Kashyap (Hrishikeshkashyap)]

Replaced the full cached-PV scan during staging with a controller-runtime field index keyed by CSI driver and volume handle. The index is registered during driver manager setup, and staging retrieves only matching PVs while preserving duplicate-identity rejection. Added tests for index registration, identity filtering, updates, and existing ambiguity checks.

[Copilot]

🔵 Needs a closer look

Data-preservation behavior needs human validation, with unresolved staging and integration-test regressions.

2 open findings

• Fix hyperconverged volume test Node fixture and cache synchronization · New
• Allow staging existing volumes while offline expansion is pending · New

3 resolved since last review

• Disambiguate all matching PVs before selecting policy
• Preserve driver topology keys when constructing Pod affinity
• Fall back to PV storage capacity for existing static volumes

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Cleanup can delete a valid durability volume, and availability-mode static imports can regress during staging.

2 open issues

• Orphan scanner deletes staged PV with empty selected-node annotation · New
• Availability PV staging rejects existing LV when names differ · New

2 resolved since last review

• Fix hyperconverged volume test Node fixture and cache synchronization
• Allow staging existing volumes while offline expansion is pending

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

Unresolved ownership tracking and topology matching defects can expose stale data or prevent valid durability staging.

2 open issues

• Record selected node for ownerless static PV recovery · New
• Honor MatchFields-only node ownership for durability PVs · New

4 resolved since last review

• Preserve PV-less expansion for existing CSI volumes
• Fix non-addressable Quantity.Sign call
• Update missing PV capacity error expectation
• Allow staging during pending expansion for static PVs

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Storage ownership and recovery changes need human validation backed by verified regression tests and CSI conformance results.

No open issues 🎉

2 resolved since last review

• Record selected node for ownerless static PV recovery
• Honor MatchFields-only node ownership for durability PVs

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Data-preservation changes span provisioning, staging, and cleanup and require human review backed by executable regression results.

No open issues 🎉

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Admission and cache-scaling issues remain unresolved, and mixed-version rollout and rollback require human validation.

2 open issues

• Scope driver Node cache to avoid cluster-wide informer overhead · New
• Keep hostname resolution optional for availability volumes · New

_{Review effort: Balanced}

[jmclong (jmclong)]

Please add durability-mode E2E coverage for the new staging guarantees, using the existing E2E fixtures/infrastructure.

The shared E2E StorageClass does not set localdisk.csi.acstor.io/failover-mode: durability; the hyperconvergence test asserts preferred affinity, and the failover/expansion test skips hard-pinned PVs. This PR adds fake-backed durability cases but no changes under test/, so the green E2E jobs do not exercise the new policy end to end.

The important scenarios are:

1. With an explicitly durability-mode StorageClass, write data and record a checksum, then recreate/restage the workload on its owner and verify the original data survives.
2. Make the owner unavailable for scheduling and verify a replacement stays Pending rather than recovering on another node with an empty volume.
3. In the disposable E2E storage fixture, make the backing LV absent and force staging on the correct owner; verify staging fails and the driver does not allocate an empty replacement LV.

These should exercise the actual provisioner -> persisted PV metadata/topology -> scheduler -> node plugin -> LVM path. The fake-backed tests are useful for branch/call-count assertions, but cannot establish those real scheduling and storage postconditions.

[Hrishikesh Kashyap (Hrishikeshkashyap)]

Please add durability-mode E2E coverage for the new staging guarantees, using the existing E2E fixtures/infrastructure.

The shared E2E StorageClass does not set localdisk.csi.acstor.io/failover-mode: durability; the hyperconvergence test asserts preferred affinity, and the failover/expansion test skips hard-pinned PVs. This PR adds fake-backed durability cases but no changes under test/, so the green E2E jobs do not exercise the new policy end to end.

The important scenarios are:

1. With an explicitly durability-mode StorageClass, write data and record a checksum, then recreate/restage the workload on its owner and verify the original data survives.
2. Make the owner unavailable for scheduling and verify a replacement stays Pending rather than recovering on another node with an empty volume.
3. In the disposable E2E storage fixture, make the backing LV absent and force staging on the correct owner; verify staging fails and the driver does not allocate an empty replacement LV.

These should exercise the actual provisioner -> persisted PV metadata/topology -> scheduler -> node plugin -> LVM path. The fake-backed tests are useful for branch/call-count assertions, but cannot establish those real scheduling and storage postconditions.

Added explicit durability tests for checksum-preserving restaging, Pending replacement when the owner is cordoned, and missing-LV rejection without recreation. Cross-compilation passes; cluster execution is still pending. Destructive coverage requires explicitly disposable Kind.

[Copilot]

🔵 Needs a closer look

Durability recovery safety remains unverified because v0.4.1 can recreate missing durability LVs, while mixed-version rollout/rollback testing, disposable-cluster staging/resize validation, and exact runtime LVM2 provenance are unavailable.

1 open issue

• Scope driver Node cache to avoid cluster-wide informer overhead

1 resolved since last review

• Keep hostname resolution optional for availability volumes

_{Review effort: Balanced}

[jmclong (jmclong)]

Follow-up idea: in the durability ownership fallback, consider fetching the Node as PartialObjectMetadata rather than a full Node, since this check only needs its name and labels. Also consider coalescing concurrent Node reads (for example, with singleflight keyed by node name) so simultaneous staging requests on the DaemonSet pod do not issue duplicate GETs. These can be handled in a follow-up PR, separately from this PR’s correctness changes.