From fb7f08c44efabe2f779af572ffe436ac4d9b2cb2 Mon Sep 17 00:00:00 2001 From: "Christoph Engelbert (noctarius)" Date: Wed, 26 Aug 2026 08:31:22 +0200 Subject: [PATCH 1/4] Volume stack design iteration --- .../docs/designs/design-node-volume-stack.md | 1323 +++++++++++++++++ .../docs/tests/test-plan-node-volume-stack.md | 441 ++++++ 2 files changed, 1764 insertions(+) create mode 100644 operator/docs/designs/design-node-volume-stack.md create mode 100644 operator/docs/tests/test-plan-node-volume-stack.md diff --git a/operator/docs/designs/design-node-volume-stack.md b/operator/docs/designs/design-node-volume-stack.md new file mode 100644 index 000000000..3a9cafa2c --- /dev/null +++ b/operator/docs/designs/design-node-volume-stack.md @@ -0,0 +1,1323 @@ +# Design Document: Node-Side Volume Stack + +**Status:** Draft +**Author:** Christoph Engelbert (noctarius) +**Date:** 2026-08-25 +**Related Issues:** + +- [#277](https://github.com/simplyblock/simplyblock-operator/issues/277) — client-side compression and deduplication via VDO, whose node-side wiring this design absorbs +- [PR #402](https://github.com/simplyblock/simplyblock-operator/pull/402) — the VDO implementation this design generalizes + +**Test Plan:** [`tests/test-plan-node-volume-stack.md`](../tests/test-plan-node-volume-stack.md) + +--- + +## Phasing Overview + +| Phase | Status | Scope | Behavior change | +|-------------------------|---------|---------------------------------------------------------------------------------------------------------------------------|-------------------------------------------------------------------------| +| **Phase 1** (§4–§8) | Planned | The `blockdev` split, the layer contract, the runner, the stack record, and the `fabric` and `filesystem` layers | None. RWO parity with today's node service | +| **Phase 2** (§5.3–§5.4) | Planned | The `lvmPV` and `lvmVolume` layers, the VDO call sites migrated onto the stack, the LVM primitives moved into `atlas-lib` | None. VDO parity with PR #402 | +| **Phase 3** (§9) | Planned | `Healer` and `Grower`, so heal, restage, and expand walk the stack | Heal and expand become correct for every layer, not only the bottom one | +| **Phase 4** (§10) | Planned | Node requirements derived from the plan on the controller side | Topology gating stops being hand-written per feature | + +Phase 1 is shippable on its own because it changes no observable behavior: the +existing RWO plan is `fabric` → `filesystem`, and the runner performs exactly the +calls `NodeStageVolume` performs today. Phase 2 is shippable because it moves code +that is already validated on a live cluster. Phase 3 is the first phase that fixes +something, and Phase 4 is the only phase that touches the CSI controller service. + +Phase 4 is planned rather than committed. It is in this document because the +pattern it replaces is already duplicated, and a design that leaves it out invites +the third copy. + +--- + +## Phase 0 — External Prerequisites + +| # | Prerequisite | Kind | Blocks | Status | +|------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------|------------|---------|------------------------------------------------------------| +| P0-1 | `lvm2` and `vdo` packages present in the CSI node image. `vdo` has no AArch64 build in the configured repositories, so LVM-backed plans are x86_64 only | Ecosystem | Phase 2 | On the PR #402 branch, not on `main` | +| P0-2 | `kmod-kvdo` built for the running host kernel, per node | Node OS | Phase 2 | Available on some hosts only, which is why §10 gates on it | +| P0-3 | `hostPID: true` on the CSI node DaemonSet, so the module load can `nsenter` the host namespace | Kubernetes | Phase 2 | On the PR #402 branch | +| P0-4 | No udev daemon runs inside the node container, so device-mapper's udev-sync handshake never completes and `DM_DISABLE_UDEV=1` is required for every LVM invocation | Ecosystem | Phase 2 | Known, handled on the PR #402 branch | + +Without P0-1 or P0-2 a node cannot run an LVM-backed plan at all. The consequence +is not a degraded volume but an unschedulable one: §10 keeps such volumes off such +nodes, and until Phase 4 lands the pod stays `Pending` with the failure visible on +the node plugin's log rather than on the PVC. P0-3 and P0-4 are environment facts +rather than decisions, and a node image that lacks either produces a layer whose +`Ensure` fails on its first command. + +--- + +## Table of Contents + +1. [Background](#1-background) +2. [Goals and Non-Goals](#2-goals-and-non-goals) +3. [Architecture Overview](#3-architecture-overview) +4. [The Layer Contract](#4-the-layer-contract) +5. [Layer Catalog](#5-layer-catalog) +6. [The Stack Record](#6-the-stack-record) +7. [Bring-Up and Bring-Down](#7-bring-up-and-bring-down) +8. [Co-Tenant Namespaces](#8-co-tenant-namespaces) +9. [Heal and Grow](#9-heal-and-grow) +10. [Node Requirements and Capability Gating (Phase 4)](#10-node-requirements-and-capability-gating-phase-4) +11. [Package Layout](#11-package-layout) +12. [Concurrency and Locking](#12-concurrency-and-locking) +13. [Failure Modes and Fallback](#13-failure-modes-and-fallback) +14. [Observability](#14-observability) +15. [Testing Strategy](#15-testing-strategy) +16. [Migration Strategy](#16-migration-strategy) +17. [Open Questions](#17-open-questions) +- [Appendix A: `blockdev.Device`](#appendix-a-blockdevdevice) +- [Appendix B: `LockScope`](#appendix-b-lockscope) + +--- + +## Overview + +A persistent volume on a node is a stack of objects, each one built on the one +below it. The simplest stack is two objects deep: an NVMe-oF namespace, and a +filesystem mounted on it. Client-side deduplication adds an LVM physical volume +and a VDO logical volume between them. A striped pNFS export adds several +namespaces at the bottom, a striped logical volume above them, and an NFS export +at the top, and the client that mounts that export builds a different stack out of +the same pieces. + +The node service does not model this. It has one extension point, the +`util.SpdkCsiInitiator` interface, which answers a single question ("connect this +and return a device path") and has exactly one implementation. Everything above +that seam is written inline, so each new capability is added as a conditional in +every RPC that touches the data path. Client-side VDO cost five such call sites +for one optional object, and the pNFS work adds four more objects on two different +stack shapes. + +This design replaces the inline conditionals with a `Layer`: one reversible +transform, with four verbs, that can be composed into a **plan**. A plan is an +ordered list of layers, derived from the volume's parameters at stage time and +recorded on the host, and a **runner** brings it up bottom to top and takes it +down top to bottom. Raw block mode is not a branch in this model, it is a shorter +plan. A striped export and a VDO volume are the same LVM layer with a different +logical-volume type. + +The four verbs are the load-bearing part, and they come from a defect that PR #402 +found on a live cluster. `Release` gives up the host's hold on an object and keeps +its data, `Destroy` removes the object. Conflating them made an ordinary pod +delete and recreate run `vgremove` and silently destroy the volume's data. Once +the two are separate, `NodeUnstageVolume` calls only `Release`, a failed bring-up +unwinds with only `Release`, and `Destroy` has a different caller entirely. + +--- + +## 1. Background + +`NodeStageVolume` in `csi-driver/pkg/spdk/nodeserver.go` performs a fixed +sequence: parse the volume handle, refresh the connection from the control plane, +build a `util.SpdkCsiInitiator` from the volume context, call `Connect` for a +device path, then call `stageVolume`, which is a single +`FormatAndMountSensitiveWithFormatOptions` followed by an `xfs` and `ext4` +if-chain. `NodeUnstageVolume` reads the volume context back from the staging +parent directory, unmounts, and calls `Disconnect`. + +`util.SpdkCsiInitiator` is the only seam, and it is a polymorphism seam rather +than a composition seam: `Connect(ctx) (string, error)` and `Disconnect(ctx) +error`, with `initiatorNVMf` as the sole implementation. It can answer "which +kind of thing is at the bottom of the stack" and nothing about what sits above. + +Four capabilities are arriving at once, and none of them fits that seam: + +- **Client-side compression and deduplication** (issue #277, PR #402) inserts an + LVM physical volume and a VDO logical volume between the namespace and the + filesystem. +- **Single-volume pNFS** ([`design-pnfs-rwx.md`](design-pnfs-rwx.md) §8, §10) + needs two different stacks over one namespace: the MDS host formats, mounts, and + exports it, and the client publishes an `eui64` alias and mounts the export over + NFS. +- **Striped pNFS** ([`design-pnfs-striped.md`](design-pnfs-striped.md) §2.1, §2.2) + puts *n* namespaces at the bottom and a striped logical volume above them, again + in two shapes, with the client activating the same volume group read-only. +- **Raw, ext4, and XFS** already differ in whether the filesystem object exists at + all and in what options create it. + +PR #402 is the measurement of what the missing seam costs. Its VDO mechanics are +sound and validated end to end on a live cluster, including nine defects only +findable on real hardware. Its wiring repeats + +```go +mountDevicePath := devicePath +if compression, deduplication, wantsVDO := vdoParams(vc); wantsVDO { + lvolID := volumeID + if spdkVol, perr := parseVolumeID(volumeID); perr == nil { lvolID = spdkVol.lvolID } + // ... +} +``` + +in `NodeStageVolume`, `NodeUnstageVolume`, `NodeExpandVolume`, and +`restageVolume`, plus a fifth negative gate inside `stageVolume` that suppresses +the `xfs` stripe hints. That is one optional object costing five call sites, +before pNFS adds four more objects across two stack shapes. + +Three of the defects PR #402 fixed are contract questions rather than +implementation slips, and §4 answers each of them once instead of per layer: + +- `NodeUnstageVolume` called the destructive `RemoveVDO` (`vgremove`) on every + unstage, including a routine pod delete and recreate on the same node, which + silently destroyed the data. The fix introduced a non-destructive + `DeactivateVDO`. +- `vgchange -an` fails on every retry when the backing NVMe-oF device is already + gone, leaving a permanently orphaned device-mapper stack. The fix added a + `dmsetup remove` force path, which then matched nothing until it accounted for + device-mapper's dash escaping. +- A byte-level clone carries its source's LVM metadata, so two volumes on one host + claim the same volume group. The fix, `ResolveClonedVDO`, runs `vgimportclone` + and `lvrename` before touching the device. + +--- + +## 2. Goals and Non-Goals + +### Goals + +- A single interface that composes the node-side objects of a persistent volume + into an ordered plan, so a new object is one implementation rather than a + conditional in every data-path RPC. +- Bring-up and bring-down that are convergent, not transactional: every verb is + safe to re-enter, because `NodeStageVolume` is retried, a heal re-runs a live + stack, and a teardown may resume after a crash. +- Idempotence at the layer and not only convergence at the runner: `Ensure`, + `Release`, and `Destroy` leave the host in the state one application leaves it + in when applied twice, and a verb whose object is already in its target state + succeeds rather than reporting an error. A `vgremove` against a volume group + that is already gone is a success, because the alternative wedges a delete path + on an object nobody can remove. This is the property `unwind` rests on rather + than a tension with it (§7.3): a failed bring-up releases best-effort and may + itself be interrupted, so correctness comes from the next attempt converging. +- A structural separation between releasing a host's hold on an object and + destroying the object, so the defect that made an ordinary pod restart destroy + data cannot be written again. +- Bring-down that works when the layer below is already gone, which is the normal + case after total path loss. +- Bring-down that never disconnects a subsystem another volume is using (§8). +- Heal, restage, and expand that decompose over the same plan, so they stop being + correct only for the bottom layer. +- A record on the host of what was built, written before the first side effect, so + a partially built stack is discoverable and removable after a crash. +- A stack that outlives the process that built it: after a csi-node pod restart + mid-stage, the plan and how far it got are recoverable from the record and + `Observe` alone. No layer keeps bring-up state only in process memory, and the + record's directory outlives the pod (§6). +- No behavior change for volumes that stage today, and no behavior change for the + VDO volumes PR #402 validated. +- Node capability and node pinning derived from the plan rather than hand-written + per feature (Phase 4). + +### Non-Goals + +- **The pNFS layers themselves.** `alias`, `nfsExport`, and `nfsMount`, the + read-only client activation, `fsid` allocation, and MDS selection belong to + [`design-pnfs-rwx.md`](design-pnfs-rwx.md) and + [`design-pnfs-striped.md`](design-pnfs-striped.md). This design defines the + contract they implement and names them in §5 only to show that the contract + fits. +- **A user-authored plan.** A plan is derived from a small set of named volume + kinds and their options. A StorageClass parameter carrying a list of steps + would be an API forever and an unbounded test surface. +- **Parallel layer execution.** §7 brings layers up one at a time. On this data + path the objects that look independent usually are not, and the ordering is the + correctness property. +- **Changing what VDO does.** Phase 2 moves PR #402's mechanics behind the + contract. The commands it runs, the names it derives, and the force paths it + falls back to are preserved, because they are what the live validation covered. +- **Replacing the volume-context stash.** `util.StashVolumeContext` keeps its + current role. §6 adds a record of the plan beside it and does not merge the two. +- **Block-mode plus VDO.** PR #402 excludes it explicitly and this design does not + add it. The plan for it is representable (`fabric` → `lvmPV` → + `lvmVolume(vdo)`, with no `filesystem`), which makes it a scoping decision + rather than an untested combination, but it is still out of scope. + +--- + +## 3. Architecture Overview + +``` +┌────────────────────────────────────────────────────────────────────────┐ +│ csi-node (one per host) │ +│ │ +│ NodeStageVolume ────┐ │ +│ NodeUnstageVolume │ │ +│ NodeExpandVolume ├──▶ plan(VolumeContext, VolumeCapability, Role) │ +│ NodePublishVolume │ │ │ +│ restage / heal ─────┘ ▼ │ +│ ┌─────────────┐ │ +│ │ Runner │ Up / Down / Heal / Grow │ +│ └─────────────┘ │ +│ │ │ │ +│ reads and writes │ │ Observe / Ensure │ +│ ▼ │ Release / Destroy │ +│ /var/lib/simplyblock/ │ │ +│ stacks/.json ▼ │ +│ ┌─────────────┐ │ +│ │ Layers │ │ +│ └─────────────┘ │ +└────────────────────────────────────────────┬───────────────────────────┘ + │ + ┌──────────────────────────────┼──────────────────────────┐ + ▼ ▼ ▼ + NVMe-oF fabric LVM / device-mapper mkfs and mount + (atlas-lib nvmeof, nvme) (lvm2, kvdo, dmsetup) (k8s mount-utils) +``` + +**The CSI RPCs no longer know the shape of the stack.** They build a plan, hand it +to the runner, and act on the artifact the top layer produces. `stageVolume`'s +`xfs` and `ext4` if-chain becomes the `filesystem` layer, and `vdoParams(vc)` +disappears from every RPC because the plan already carries the answer. + +**The plan is built once per RPC and recorded once per volume.** Building it is a +pure function of the volume context and the volume capability, which is what makes +it unit-testable without a host. Recording it is what makes teardown possible when +the process that built it is gone (§6). + +The plans the contract has to express, bottom to top. The last four differ from +each other only by the node's role for the volume, which is why the role is an +input to plan construction and not a property of the volume: + +| Volume kind | Plan | +|-----------------------------------------------|----------------------------------------------------------------------------| +| Plain, raw block | `fabric` | +| Plain, ext4 or XFS | `fabric` → `filesystem` | +| Client-side dedup or compression, ext4 or XFS | `fabric` → `lvmPV` → `lvmVolume(vdo)` → `filesystem` | +| pNFS single, MDS host | `fabric` → `filesystem` → `nfsExport` | +| pNFS single, client node | `fabric` → `alias` → `nfsMount` | +| pNFS striped, MDS host | `members(n)` → `lvmPV` → `lvmVolume(striped)` → `filesystem` → `nfsExport` | +| pNFS striped, client node | `members(n)` → `alias` → `lvmVolume(activate, read-only)` → `nfsMount` | + +Two results are worth reading off that table. Raw block mode is the plain plan +with its top layer absent rather than a conditional inside a stage function. And +the VDO volume and the striped export use the same `lvmVolume` layer with a +different logical-volume type, which is why §5.4 treats striping as a parameter +and not as a second implementation. + +--- + +## 4. The Layer Contract + +### 4.1 `Layer` + +```go +// Layer is one reversible transform in a volume's node-side stack: it takes what +// the layer below exposes and exposes something for the layer above. Every method +// is safe to re-enter, because NodeStageVolume is retried, a heal re-runs a live +// stack, and a teardown may resume after a crash. +type Layer interface { + // Name identifies the layer in logs and in the stack record. It is stable + // across releases: a teardown after an upgrade replays a record an earlier + // version wrote. + Name() string + + // Observe reports what of this layer is present on the host without changing + // anything. Ensure, Release, and Destroy all dispatch on what it found rather + // than re-deriving the same facts. + Observe(ctx context.Context, below Artifact) (State, error) + + // Ensure converges the layer and returns what the layer above consumes. + Ensure(ctx context.Context, below Artifact) (Artifact, error) + + // Release drops this host's hold on the layer and keeps its data. It is the + // only verb NodeUnstageVolume calls, and it has to succeed when the layer + // below is already gone, which is the normal case after total path loss. + Release(ctx context.Context, below Artifact) error + + // Destroy removes the layer's durable object. Only a deletion path calls it, + // never an unstage. + Destroy(ctx context.Context, below Artifact) error +} +``` + +**`Release` and `Destroy` are separate because conflating them destroys data.** +`NodeUnstageVolume` fires whenever no pod on this node needs the volume mounted, +which includes an ordinary pod delete and recreate against the same PVC on the +same node. A teardown path that removes durable objects there removes them on a +pod restart. The existing `defer initiator.Disconnect()` in `NodeStageVolume` is +already a `Release` and is safe for that reason. The same reflex applied to a +volume group is the defect PR #402 fixed. + +Not every layer implements all four distinctly. `lvmPV` has nothing to release, +because a physical-volume signature is not something a host holds. `fabric` has +nothing to destroy, because the namespace belongs to the control plane. A verb +with nothing to do returns without error rather than returning "unsupported": the +runner calls all four uniformly, and a layer that has to be special-cased by the +runner is not a layer. + +### 4.2 `State` + +```go +// State is what Observe found. The distinctions matter because Ensure's response +// to each is different, and two of them are the difference between reactivating a +// volume and reformatting it. +type State int + +const ( + // StateAbsent means nothing of this layer exists. Ensure creates it, which is + // the only circumstance under which a layer may format anything. + StateAbsent State = iota + + // StatePartial means an interrupted Ensure left an incomplete object. An LVM + // volume group whose logical volume was never created reports zero logical + // volumes and activates successfully while producing no usable device, so + // "the group exists" is not the same question as "the layer is ready". + StatePartial + + // StateForeign means the object exists but carries another volume's identity. + // A byte-level clone copies its source's LVM metadata, so the clone's device + // claims the source's volume group until vgimportclone renames it. + StateForeign + + // StateInactive means the object is complete but not currently mapped on this + // host. It is what Release leaves behind and what a node reboot leaves behind, + // and Ensure reactivates rather than recreating. + StateInactive + + // StateReady means present, complete, and usable. + StateReady +) +``` + +**The `StateAbsent` and `StateInactive` distinction is the one that loses data +when it is wrong.** Every layer that can create a durable object must be able to +tell "this volume has never been set up here" from "this volume is set up and +merely not activated," because the first answer permits a `mkfs` or an `lvcreate` +and the second forbids it. PR #402's `CreateOrAttachVDO` encodes exactly this rule +in prose ("if the volume group already exists it is reactivated, never +recreated"), and lifting it into the type is what makes it checkable. + +### 4.3 `Artifact` and `Geometry` + +```go +// Artifact is what one layer hands to the layer above it. It carries what a +// higher layer can act on and nothing about how the layer below produced it. +type Artifact struct { + // Devices are the block devices this layer exposes, in a defined order. A + // fan-in layer exposes several; every other layer exposes one. + Devices []blockdev.Device + + // Path is the filesystem path this layer mounted, empty until a layer mounts + // one. + Path string + + // Geometry is the stripe layout of Devices, for a layer above that aligns to + // it. + Geometry Geometry +} + +// Geometry is a stripe layout: the per-stripe chunk size and the number of +// stripes data is spread across. The zero value means unknown, which is the +// correct answer for a device whose blocks are virtualized. +type Geometry struct { + ChunkBytes int64 + Stripes int +} +``` + +`Devices` carries a type rather than a path. `blockdev.Device`, its fields, and +why a path string is insufficient are [Appendix A](#appendix-a-blockdevdevice). + +**`Geometry` exists because a filesystem's format options depend on what is +underneath it.** PR #402 discovered this as a special case: once VDO is in play the +`xfs` stripe hints must be suppressed, because VDO virtualizes and relocates +blocks and the filesystem is no longer laid out over the erasure-coded backend +device those hints were computed for. Applying them there is misleading rather +than merely useless. Expressed as a conditional, that is a fifth call site. +Expressed as a value, the VDO layer reports `Geometry{}` and the `filesystem` +layer passes no `-d su=,sw=` because there is nothing to align to. + +The same field improves the striped case rather than merely unifying it. A striped +`lvmVolume` layer knows its own stripe count and chunk size, so the `filesystem` +layer above it receives real geometry instead of the `xfs_su` and `xfs_sw` +StorageClass parameters and their `16k`/`1` fallbacks. + +### 4.4 Optional interfaces + +A layer implements these when it has something to contribute. The runner type +asserts for each and skips the layers that do not. + +```go +// Healer is implemented by a layer whose object can go bad under a live stack and +// be repaired in place. Heal never recreates: the data already exists. +type Healer interface { + // Healthy is a read. It reports whether this layer is currently serving. + Healthy(ctx context.Context, own Artifact) (bool, error) + + // Heal repairs the layer against the layer below, which may itself have just + // been healed. + Heal(ctx context.Context, below, own Artifact) error +} + +// Grower is implemented by a layer that has to be enlarged when the volume behind +// it grows. Grow is convergent: a layer already at its target size succeeds +// without doing anything, because kubelet reissues NodeExpandVolume after it has +// already succeeded. +type Grower interface { + Grow(ctx context.Context, below Artifact) (Artifact, error) +} + +// NodeRequirements is implemented by a layer that constrains where the volume may +// be staged: it needs something from the node, or its durable state stays there. +// The volume carrying it can then be staged only on a node that can run the +// layer, and only on one node at a time (§10). +// +// Unlike Healer and Grower this is a declaration rather than an action, which is +// why it is a noun: the runner interrogates it instead of calling it. +type NodeRequirements interface { + // NodeCapability is the label a node must carry, or the zero value when any + // node will do. + NodeCapability() Capability + + // PinsToNode reports whether this layer's durable state lives on the host. + PinsToNode() bool +} +``` + +Optional rather than mandatory is deliberate. Three of the seven layers in §5 have +nothing to heal and four have nothing to grow, and a mandatory interface would +fill them with methods that return nil. The runner's assertion is also what keeps +`NodeExpandVolume` honest: a plan whose layers implement no `Grower` at all is a +plan that needs no node-side expansion, which is the correct answer for a pNFS +client. + +`Grower` being convergent answers the last loose end PR #402 left open, where a +redundant `NodeExpandVolume` after a successful one logs an alarming but harmless +error on kubelet's reconciliation retry. + +--- + +## 5. Layer Catalog + +| Layer | Ensure | Release | Destroy | Optional | +|---------------------|--------------------------------------------------------------------------------------|---------------------------------------------------------------------|---------------------------|------------------------------| +| `fabric` (§5.1) | Connect every endpoint in the control plane's priority order, wait for the namespace | Detach, disconnecting only when the subsystem cannot be shared (§8) | — | `Healer` | +| `members` (§5.2) | *n* × `fabric` in the recorded order | Reverse order | — | `Healer` | +| `lvmPV` (§5.3) | `pvcreate`, or re-identify when `StateForeign` | — | `pvremove` | — | +| `lvmVolume` (§5.4) | `vgcreate` and `lvcreate` of the configured type, or activate when `StateInactive` | `vgchange -an`, with a `dmsetup` force path | `lvremove` and `vgremove` | `Grower`, `NodeRequirements` | +| `filesystem` (§5.5) | `mkfs` when unformatted, then mount | Unmount | — | `Healer`, `Grower` | +| `alias` | Publish the `eui64` symlink | Remove the symlink | — | — | +| `nfsExport` | Write the export drop-in, `exportfs -ra` | `exportfs -u` | Remove the drop-in | `Grower` | +| `nfsMount` | `mount -t nfs -o v4.1` | Unmount | — | `Healer` | + +`alias`, `nfsExport`, and `nfsMount` are listed for completeness and are out of +scope here (§2). They are specified by +[`design-pnfs-rwx.md`](design-pnfs-rwx.md) §8 and §10 and +[`design-pnfs-striped.md`](design-pnfs-striped.md) §2. + +### 5.1 `fabric` + +Wraps the existing NVMe-oF connect. `Ensure` asks the control plane where the +volume lives, builds one target per endpoint, connects them in the control plane's +priority order, and waits for the namespace device, which is the flow +`atlas-lib`'s `nvmeof.ConnectPaths` and `nvmeof.WaitForDevice` implement and which +`csi-driver/pkg/util/initiator.go` implements today with `nvme-cli`. Which of the +two implements it is a separate migration and not this design's business. The +layer's contract is the same either way. + +`Observe` maps the device state onto §4.2: no namespace device is `StateAbsent`, +a device present but not accessible is `StatePartial`, and a device that +`nvme.Device.Accessible` reports as serving is `StateReady`. `StateForeign` and +`StateInactive` do not arise, because a namespace carries no host-local identity +and cannot be present-but-deactivated. + +`Ensure` reports the backend's stripe geometry in the `Artifact` when it is known, +which is what lets the `filesystem` layer above align to it in the plain plan. + +`Release` is a detach rather than a disconnect and is specified in §8. `Destroy` +does nothing: the namespace belongs to the control plane and is removed by +`DeleteVolume`. + +### 5.2 `members` + +A composite layer holding *n* `fabric` layers, for a plan whose bottom is several +namespaces rather than one. `Ensure` runs them in the recorded order and returns +one `Artifact` whose `Devices` are their `blockdev.Device` values in that same +order. `Release` +reverses. + +**Member order is contract, not convenience.** A stripe over the same members in a +different order is a different device, and +[`design-pnfs-striped.md`](design-pnfs-striped.md) §2.3 requires that the order be +recorded and replayed rather than re-derived from a set. `members` is where that +requirement is satisfied, and it is why the plan is recorded (§6) instead of +rebuilt from the current StorageClass. + +`members` is also the reason this design needs no dependency graph. Fan-in is the +only non-linear shape any of the plans in §3 has, and a composite layer expresses +it without making the ordering of anything else implicit. + +### 5.3 `lvmPV` (Phase 2) + +`Ensure` on `StateAbsent` runs `pvcreate` against the device below. `Observe` +reads the device's on-disk LVM signature to answer which volume group it currently +belongs to, and reports `StateForeign` when that is a volume group belonging to +another volume, which is what a byte-level clone produces. `Ensure` on +`StateForeign` re-identifies the device with `vgimportclone` and `lvrename` before +anything above it activates. + +`Release` does nothing, because a physical-volume signature is not a hold. That +asymmetry is why PVs are their own layer rather than part of §5.4: the two objects +have different lifetimes, and `vgremove` does not remove the physical volumes it +released. + +**The clone collision is not VDO-specific.** Any layer whose object is identified +by on-disk content has it, and +[`design-pnfs-striped.md`](design-pnfs-striped.md) §2.3 specifies deterministic +volume-group names without addressing what happens when a clone and its source are +staged on one host. `StateForeign` is that specification, in one place, for both. + +### 5.4 `lvmVolume` (Phase 2) + +One volume group holding one logical volume, whose type is a parameter: linear, +`vdo`, or striped. `Ensure` on `StateAbsent` runs `vgcreate` over the physical +volumes below and then `lvcreate` of the configured type. `Ensure` on +`StateInactive` runs `vgchange -ay` and creates nothing. `Ensure` on +`StatePartial`, the volume group whose logical volume was never created, completes +the `lvcreate`. + +`Release` runs `vgchange -an`, and falls back to removing the device-mapper nodes +directly when the backing device is gone and every LVM retry fails. That force +path has to escape the volume-group name the way device-mapper does, doubling +dashes, or it matches nothing. + +`Destroy` runs `lvremove` and `vgremove`. Its callers are volume deletion and, for +a pNFS export, `DeleteExport`. It is never reached from `NodeUnstageVolume`. + +`Grow` extends the logical volume to the new physical capacity of the group and +then matches the logical size, and succeeds without acting when the volume is +already at its target. + +Naming is derived from the logical volume's UUID and nothing host-specific, so a +plan replayed on another host arrives at the same names. The lvol UUID is already +globally unique, stable, and inside LVM's length and character limits, which makes +it a better identifier than a hash of namespace and PVC name and is the convention +PR #402 established. [`design-pnfs-striped.md`](design-pnfs-striped.md) §2.3 +should adopt it rather than deriving a separate one, because a failover that +recomputes a different name cannot find the export it is recovering. + +The striped and the VDO plans differ only in the logical-volume type and its +options. The `Artifact` a striped volume reports carries its real `Geometry`, and +a VDO volume reports the zero value (§4.3). + +### 5.5 `filesystem` + +`Ensure` formats the device below when `blkid` shows it is unformatted, then mounts +it at the staging path. Formatting and mounting stay one layer because +`mount-utils`' `SafeFormatAndMount` couples them deliberately and splitting them +loses its protection against formatting a device that another process is about to +mount. + +Format options come from the volume's parameters and from the `Artifact` below. +`xfs` receives the feature options unconditionally, because on-disk feature +compatibility across kernel versions has nothing to do with the backend's layout, +and receives stripe alignment only when the layer below reports a non-zero +`Geometry`. `ext4` receives the reserved-blocks adjustment when the parameter is +set, and nothing when it is unset, preserving today's behavior where an unset +parameter means `mkfs.ext4`'s own default rather than `tune2fs -m 0`. + +`Healthy` detects the dead mount that total path loss leaves behind, which is the +`stagingMountDead` check the node service performs today: an `ENOTCONN`, `ESTALE`, +or `EIO`-class error from the mount point, plus the additional probe `ext4` needs +because it does not shut down when its backing device is removed and therefore +looks healthy from cache. `Heal` remounts without reformatting. + +`Grow` resizes the filesystem, and is absent from the plan entirely for a raw +block volume, which is how `NodeExpandVolume`'s current block-device special case +disappears. + +--- + +## 6. The Stack Record + +Every volume with a plan has one file on the host: + +``` +/var/lib/simplyblock/stacks/.json +``` + +It holds the plan, which is the ordered list of layer names and the parameters +each was constructed with, plus a per-layer marker recording that `Ensure` was +attempted. The file is written **before the first `Ensure` runs** and removed +after the last `Release` succeeds. + +**Writing it first is what makes a partially built stack removable.** A crash +between a fabric connect and the recording of that connect leaves paths attached +that nothing will ever release, which is a failure mode this repository has +already paid for on a different code path. Ordering the write ahead of the side +effect is the same discipline the operator's reconcilers apply to control-plane +calls. + +**The directory has to outlive the container.** `/var/lib/simplyblock` is a host +path mounted into the csi-node pod rather than container-local storage, because a +plugin restart is an ordinary event and the record is the only thing that tells +the restarted process what the previous one built. A layer that caches its +bring-up progress in process memory defeats the same property, which is why +`Observe` is the only way any verb learns what is present. + +**The record holds parameters, not device paths.** A device path is not stable +across a reconnect, which is why LVM identifies its physical volumes by on-disk +metadata rather than by path. Layer parameters are derived from volume identity +and are stable, so a teardown re-derives the artifacts through `Observe` rather +than trusting a path an earlier process wrote down. + +Given that, the per-layer markers are a diagnostic and an optimization rather than +a correctness mechanism: `Release` on a layer whose `Observe` reports +`StateAbsent` is already a no-op, so a teardown that ignored the markers entirely +would reach the same end state. They earn their place by making "what was +attempted, in what order" answerable after the fact, and by letting a teardown +skip the layers that were never reached. + +**The plan is recorded rather than re-derived because the StorageClass is not a +record of the past.** A class can be edited or deleted after a volume is +provisioned, and teardown owes the truth about what was built. The same reasoning +covers member order (§5.2), which cannot be recovered from a set. + +**An absent record means the legacy plan.** A volume staged by a version of the +node service that predates this design has no file, and unstaging it uses +`fabric` → `filesystem`, which is exactly what that version built. No migration +step runs on the node and no volume needs to be restaged (§16). + +This is host-local state, which has a consequence and an alternative. The +consequence is that the record is unreachable from the operator, so an orphaned +stack is found by a host sweep rather than by a cluster-wide query. The +alternative is to keep the record on the operator and reach it over csi-link, +which makes it visible cluster-wide at the cost of an RPC on the unstage path and +a dependency on the operator being reachable during teardown. §17 Q3 carries the +decision. Phase 1 uses the host-local file, because unstage has to work when the +operator does not. + +### 6.1 File Format + +```go +// Record is the on-disk form of a stack, one file per volume under +// /var/lib/simplyblock/stacks/. +type Record struct { + // Version is the schema version of this file and not the release that wrote + // it. A reader that does not recognize it refuses the record rather than + // guessing (§13), because a teardown driven by a misread plan is worse than a + // teardown that stops and says why. + Version int `json:"version"` + + // VolumeHandle is an lvol.VolumeHandle, "clusterID:poolID:volumeID". It + // repeats the filename so that a record found on its own identifies itself. + VolumeHandle string `json:"volumeHandle"` + + // Plan is the ordered layer list, bottom first. The order is most of why the + // file exists: Up walks it forward and Down walks it back. + Plan []Entry `json:"plan"` +} + +// Entry is one layer as the plan named it. +type Entry struct { + // Layer is the value Layer.Name() returns, stable across releases (§4.1). + Layer string `json:"layer"` + + // Params is what the layer was constructed with, opaque to the runner: the + // layer that declared them is the only thing that parses them. A new layer + // therefore ships without this format changing. + Params json.RawMessage `json:"params,omitempty"` + + // Members is the ordered sub-plan of a fan-in layer (§5.2) and is empty for + // every other layer. It is a field of its own rather than part of Params + // because the runner walks it, and member order is a runner concern. + Members []Entry `json:"members,omitempty"` + + // Attempted records that Ensure was called on this layer. It is a diagnostic + // and an optimization, never a correctness mechanism. + Attempted bool `json:"attempted"` +} +``` + +A striped stack whose `filesystem` layer was never reached: + +```json +{ + "version": 1, + "volumeHandle": "11111111-1111-1111-1111-111111111111:22222222-2222-2222-2222-222222222222:33333333-3333-3333-3333-333333333333", + "plan": [ + { + "layer": "members", + "attempted": true, + "members": [ + {"layer": "fabric", "params": {"nqn": "nqn.2023-05.io.simplyblock:lvol:aaaa"}, "attempted": true}, + {"layer": "fabric", "params": {"nqn": "nqn.2023-05.io.simplyblock:lvol:bbbb"}, "attempted": true} + ] + }, + {"layer": "lvmPV", "attempted": true}, + {"layer": "lvmVolume", "params": {"type": "vdo", "stripes": 2, "chunkBytes": 65536}, "attempted": true}, + {"layer": "filesystem", "params": {"fsType": "xfs"}, "attempted": false} + ] +} +``` + +**The write is atomic, and that is what makes the ordering real.** Each write goes +to a temporary file in the same directory, is `fsync`ed, is renamed over the +target, and the directory is `fsync`ed after the rename. A torn file would be +worse than no file at all, because an absent record means the legacy plan and a +half-written one would be read as a plan nobody built. Every `Attempted` flip +rewrites the whole record the same way, which is one small local write per layer +and is affordable precisely because the record holds no device state. + +**Params name secrets rather than carrying them.** A DHCHAP key is a credential +and this file outlives the pod that wrote it, so `fabric` records where to read its +secret and re-reads it on the teardown path, exactly as `NodeStageVolume` did. A +record that embedded the value would put a credential in cleartext on every node +that ever staged the volume. The file is mode `0600` and its directory `0700` +regardless. + +**What the format deliberately omits** is device paths and the reason above, +geometry and sizes, which `Observe` re-derives and which a `Grow` would invalidate +anyway, and anything naming the node, because the file is already on the node and a +record recording its own origin invites treating a copy from elsewhere as +authoritative. + +--- + +## 7. Bring-Up and Bring-Down + +### 7.1 `Up` + +``` +record.write(plan) // before any side effect +below := Artifact{} +for i, layer := range plan { + record.mark(layer) // before this layer's side effect + state, err := layer.Observe(ctx, below) + if err != nil { unwind(plan[:i], below); return err } + above, err := layer.Ensure(ctx, below) // dispatches on state + if err != nil { unwind(plan[:i], below); return err } + below = above +} +return below // what the RPC acts on +``` + +### 7.2 `Down` + +``` +for i := len(plan) - 1; i >= 0; i-- { + layer := plan[i] + if !record.marked(layer) { continue } + state, err := layer.Observe(ctx, below(i)) + if err != nil { return err } + if state != StateAbsent { + if err := layer.Release(ctx, below(i)); err != nil { return err } + } + record.unmark(layer) +} +record.remove() +``` + +`below(i)` re-derives layer *i*'s input by observing the layers beneath it, rather +than reading a device path out of the record (§6). + +### 7.3 A failed bring-up releases and never destroys + +`unwind` walks the layers already brought up, top-down, and calls `Release` on +each. It never calls `Destroy`. + +**This is the rule the four verbs exist for.** A `mkfs` that fails must not +trigger a `vgremove`, because the volume group underneath it may hold data that a +misfiring format check failed to see. `Release` is safe in the same situation +because it gives up a hold and takes nothing away. The existing +`defer initiator.Disconnect()` is this rule already, applied to the one layer the +node service has today, and generalizing it correctly means generalizing it as +`Release`. + +A stack left partly up by a failed `Up` is not an error state that needs +resolving. `NodeStageVolume` is retried, every verb is convergent, and the next +attempt observes what is there and continues. The record survives the failure, so +even a process that never retries leaves a removable stack behind. + +### 7.4 `Down` tolerates a dead foundation + +Bring-down proceeds top-down through layers whose foundation may already be gone, +which is the normal case rather than an edge case: total path loss removes the +namespace while the device-mapper stack above it is still mapped, and the pod is +deleted afterward. Each layer owns its own force path for that situation, and +§5.4's `dmsetup` fallback is the worked example. A layer that has no force path and +whose command depends on the layer below is a layer that will strand a stack. + +**`Release` returning without error does not mean the object is gone.** §8 has a +`fabric` layer that legitimately leaves its device present, so `Down` asserts +nothing about the state a released layer is in, and removes the record either way. + +### 7.5 Which RPC calls what + +| RPC or path | Runner call | Notes | +|--------------------------------|-------------------------|--------------------------------------------------------------------------------------------------------------------------| +| `NodeStageVolume` | `Up` | Acts on the top artifact's `Path`, or `Devices[0].Path` for raw block | +| `NodeUnstageVolume` | `Down` | `Release` only. `Destroy` is never reached from here | +| `NodePublishVolume` | `Heal`, then bind-mount | kubelet skips `NodeStage` when the volume is still referenced on the node, so publish is where a heal has to happen (§9) | +| `NodeExpandVolume` | `Grow` | Bottom to top, skipping layers that implement no `Grower` | +| `restageVolume` | `Heal` | Never `Up`, because the data exists and nothing may be formatted | +| `DeleteVolume`, `DeleteExport` | `Down`, then `Destroy` | The only callers of `Destroy` | + +--- + +## 8. Co-Tenant Namespaces + +A simplyblock subsystem can hold several namespaces. The +`max_namespace_per_subsys` StorageClass parameter is what provisions one that way, +and it means two different volumes can arrive at one host behind a single NQN. + +**Disconnecting a subsystem tears down every namespace on it.** So the `fabric` +layer's `Release` is a detach, not a disconnect. `atlas-lib`'s +`nvmeof.DetachDevice` answers the question and reports `SharedSubsystem`, and when +it is set the layer releases nothing at the fabric level and leaves the paths up +for the co-tenants. + +**The gate is whether the subsystem *can* be shared, not whether it currently +is.** `nvme.Device.IsMultiNamespace` is that question. Enumerating the neighbors +describes only the moment they were counted: a namespace can join between the +check and the disconnect, and a correct "none right now" answer is still +destructive when it does. A subsystem provisioned to be shared is therefore never +disconnected on one volume's behalf, even while it happens to hold only that +volume. + +The node service does not use that gate today. `selectDisconnectTarget` in +`csi-driver/pkg/util/initiator.go` counts the namespace devices the by-id glob +currently matches and disconnects when the count reaches one, which is the +weaker, enumerate-the-neighbors answer. Moving the decision behind `fabric`'s +`Release` is what replaces it, and the existing behavior for a subsystem that +genuinely still holds co-tenants has to be preserved while the gate is +strengthened. + +Three consequences for the rest of this design: + +- **`Down` cannot assert that a released layer is absent.** A `fabric` layer over + a shared subsystem returns from `Release` with its device still present and + still serving another volume. §7.4 states this as a rule and it is where the + rule comes from. The stack record is removed regardless, because this volume's + stack is down even though the fabric it stood on is not. +- **Ordering matters more, not less.** A device-mapper stack holding the namespace + open is what makes even a legitimate disconnect fail. Every layer above + `fabric` must be released first, which is what top-down teardown already + guarantees, and which is why the force paths in §7.4 exist for the cases where + it was not. +- **`Destroy` must stay per-volume.** Removing one volume's LVM objects must not + touch a co-tenant's. Deriving every LVM name from the logical volume's UUID + (§5.4) is what makes that true by construction rather than by care. + +Reaping a subsystem whose controllers are all dead is a deliberate +`connector.Disconnect` and never a default, which matches `atlas-lib`'s existing +contract: `DetachDevice` returns the error rather than guessing when the question +needs a live controller to answer. + +--- + +## 9. Heal and Grow + +Heal and expand are where the current design's cost is highest, because both are +implemented for the bottom layer only. `healVolumeBeforePublish` reconnects the +namespace, `ensureDeviceConnected` checks for a device, and `restageVolume` +remounts, and none of them knows that an LVM or VDO object might sit between the +two. PR #402 patched `restageVolume` with a fourth copy of its conditional to +close exactly that gap. + +**`Heal` walks the plan bottom to top.** For each layer that implements `Healer`, +the runner asks `Healthy` and calls `Heal` when the answer is no, passing the +artifact of the layer below, which may itself have just been healed. A layer that +implements no `Healer` is skipped and its artifact is re-derived through `Observe`, +so a healed foundation propagates upward. + +Bottom to top is the only workable order. A remount over a namespace that has not +been reconnected fails, and a namespace reconnect underneath a filesystem that is +still holding a dead mount does not clear the dead mount. + +**`Grow` walks the plan bottom to top as well**, and for the same reason: a +logical volume cannot be extended past a physical volume that has not been +resized, and a filesystem cannot be grown past its logical volume. Every `Grow` is +convergent, so kubelet's reconciliation retry after a successful expansion is a +sequence of no-ops rather than a sequence of alarming errors. + +The three layers with something to heal are `fabric` (path reconnection and ANA +reconciliation, which the existing `MonitorConnection` and guardian machinery +already perform), `filesystem` (dead-mount detection and remount), and `nfsMount` +(`ESTALE` detection). The three with something to grow are `lvmVolume`, +`filesystem`, and `nfsExport`. Every other layer implements neither, which is the +argument for the interfaces being optional (§4.4). + +--- + +## 10. Node Requirements and Capability Gating (Phase 4) + +A layer whose durable state lives on the host makes two demands on scheduling. The +volume can be staged only on a node that can run the layer, and once it is staged +it can be staged nowhere else, because the state does not follow the pod. + +Both demands are already implemented twice on the CSI controller side. +`dhchapAllowedNodeSegment` merges a DHCHAP topology segment into +`CreateVolume`'s `AccessibleTopology` so `external-provisioner` pins +`PersistentVolume.spec.nodeAffinity`, and PR #402 adds `vdoCapableSegment` +mirroring it exactly. The StoragePool controller composes the matching +`TopologySelectorTerm`s in `createStorageClassIfNotExists`. pNFS brings a third +demand, with the opposite polarity: an MDS host must be eligible, and a pNFS +client must *not* be pinned, because RWX is the point. + +`NodeRequirements` (§4.4) makes the plan the single source of truth for both. The +controller service builds the plan for a `CreateVolume` request from the same pure +function the node service uses, asks each layer for its capability and its pinning +answer, and merges the results into `AccessibleTopology`. The StoragePool +controller derives its topology terms the same way. + +Two demands then need one implementation rather than one per feature, and a +capability that is missing at admission is reported once rather than discovered as +a mount failure on the wrong node. + +Capability advertisement itself follows PR #402: the node DaemonSet's `postStart` +hook installs and loads the kernel module through `nsenter`, writes a marker file, +and the node plugin reads the marker in the background and patches the node's +capability label. Generalizing it means a layer names its marker and its label +rather than each feature adding a pair. + +**The registration race is real and belongs to the generalization.** A CSINode's +topology key set is captured once at plugin registration, seconds after the pod +starts, while the label is patched asynchronously afterward. A +`buildAccessibleTopology` that reports a capability key only when the label is +already `true` therefore reports it essentially never, which permanently breaks +the topology gate until the pod restarts. PR #402 found this as its first defect. +The key must be present at registration regardless of the label's current value, +and the label carries the answer. + +--- + +## 11. Package Layout + +Every primitive in §5 is a node-level primitive: fabric connect, `pvcreate`, +`lvcreate`, VDO, `mkfs`, `mount`, `exportfs`, and the `eui64` alias. None of them +is Kubernetes-shaped, and the striped pNFS design needs two different compositions +of the same set, one on the MDS host and one on every client. They belong in +`atlas-lib`. + +| Package | Holds | +|------------------------------|-----------------------------------------------------------------------------------------------------| +| `atlas-lib/blockdev/` | `Device`: what a Linux block device is, independent of what produced it (Appendix A) | +| `atlas-lib/volstack/` | `Layer`, `State`, `Artifact`, `Geometry`, the optional interfaces, the runner, and the stack record | +| `atlas-lib/volstack/layers/` | The layer implementations | +| `atlas-lib/lvm/` | The LVM and device-mapper primitives PR #402 wrote as `csi-driver/pkg/util/vdo.go` | +| `csi-driver/pkg/spdk` | The plan: a pure function from `VolumeContext`, `VolumeCapability`, and `Role` to a layer list | + +Plan construction stays in the CSI driver because it is the one Kubernetes-shaped +part, and it is deliberately thin. The package name `volstack` is provisional +(§17 Q1). + +**The role is an input, and resolving it is not part of the pure function.** A +pNFS volume has two plans over one namespace (§1), and §3's last four rows differ +by nothing else, so a plan function of the volume context and the capability alone +cannot select between them. Answering "is this node the MDS for this volume" means +asking something, which is what would cost the pure function its testability, so +it is resolved first and passed in: an impure step that reads whatever the pNFS +designs make authoritative, then `plan(vc, cap, role)` deriving the layer list +from three values and no host. Which values decide the role, and whether the +answer can change over a volume's life, belong to +[`design-pnfs-rwx.md`](design-pnfs-rwx.md) and +[`design-pnfs-striped.md`](design-pnfs-striped.md), exactly as the layers +themselves do (§2). That the plan takes it as an input belongs here. + +**Phase 4 needs every role rather than the plan.** §10 has the controller service +building the plan for a `CreateVolume` request, and at that point no node has a +role yet. For a pNFS volume it therefore derives the requirements of both roles +and merges them, which is precisely where §10's note about the opposite polarity +lands: the MDS role contributes a capability and a pin, and the client role +contributes a capability and the absence of one. + +**PR #402's `vdo.go` moves rather than being rewritten.** `runLVMCommand`, +`devicesArgs`, `pvVGName`, `vgExists`, `vgHasLV`, and the orphaned-node removal +are exactly the primitives §5.4 needs, and the striped pNFS layer needs the same +ones, which is the second consumer that makes the move mandatory rather than +tidy. The move is mechanical and is Phase 2 work, sequenced after PR #402 merges +rather than imposed on it: its value is in behavior validated on live hardware, +and rebasing that validation onto a package boundary buys nothing. + +--- + +## 12. Concurrency and Locking + +`util.VolumeLocks` serializes the node RPCs per volume ID today, and the runner +inherits that: one volume's stack is brought up, brought down, healed, or grown by +one goroutine at a time. + +Per-volume locking is not sufficient for every layer. `lvmPV` and `lvmVolume` +invoke LVM commands that take LVM's own host-wide locks and scan every visible +device, and two volumes staging at the same moment on one host contend there +regardless of their volume IDs. The pNFS layers are worse: `/etc/exports` is one +file per host, and a striped export's volume group is active on the MDS host and +on every client at once. + +**This is an open question and not a solved one (§17 Q2).** What is known is that +it is unexercised rather than proven safe: PR #402's multi-instance validation +happened to run its two stage sequences sequentially rather than overlapping, so +genuinely concurrent `vgchange` and `pvscan` calls have not raced on a real host. +That is a risk in shipped code once PR #402 merges, not only a risk in this +design. + +A scope declared per layer is the candidate mechanism, specified in +[Appendix B](#appendix-b-lockscope). It is not part of the contract in §4, because +the granularity it carries depends on a measurement nobody has taken. Until §17 Q2 +is answered the runner locks per volume, as the node RPCs do today. + +--- + +## 13. Failure Modes and Fallback + +| Failure | Detection | Behavior | +|---------------------------------------------------|--------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| A layer's `Ensure` fails | Error returned | Layers below are `Release`d top-down and never `Destroy`ed (§7.3). The RPC fails, kubelet retries, and the next `Up` converges from where this one stopped | +| Backing device gone during `Release` | Every LVM retry fails identically | The layer's own force path runs, removing the device-mapper nodes directly with the volume-group name escaped as device-mapper escapes it (§5.4) | +| An interrupted `Ensure` left an incomplete object | `Observe` reports `StatePartial` | `Ensure` completes the object instead of treating it as ready. A volume group with no logical volume activates successfully and produces no usable device, so the state has to be distinguished | +| A clone carries its source's LVM metadata | `Observe` reports `StateForeign` | `Ensure` re-identifies the device before anything above it activates (§5.3) | +| Subsystem shared with another volume | `nvmeof.DetachDevice` reports `SharedSubsystem` | `fabric`'s `Release` leaves the paths up. The stack record is still removed (§8) | +| csi-node pod restarted mid-bring-up | A record exists, layers `Observe` short of ready | The restarted process re-reads the record and the next `Up` converges from there. Resuming needs no in-memory state (§6) | +| Node rebooted with a stack recorded | Every layer `Observe`s `StateInactive` | `Ensure` reactivates. Nothing is created and nothing is formatted (§4.2) | +| Stack record absent at unstage | No file for this volume handle | The legacy plan is assumed: `fabric` → `filesystem` (§6) | +| Stack record's version is not recognized | `Version` is a schema this release does not read | The unstage fails and reports the version, on the same reasoning as an unknown layer name: a plan read under the wrong schema releases the wrong objects (§6.1) | +| Stack record present but unparseable | The file does not decode | The unstage fails and reports it rather than falling back to the legacy plan. That fallback answers an absent record, and a corrupt file is not evidence that a legacy stack was built (§6.1) | +| Stack record present, plan unrecognized | A layer name the running version does not know | The unstage fails and reports the unknown layer, rather than silently skipping an object nobody will release. Layer names are stable across releases for this reason (§4.1) | +| Node lacks a layer's capability | The capability label is absent | Phase 4 keeps the volume off the node. Until then `Ensure` fails on its first command and the pod stays `Pending` (§10) | +| Total path loss under a live stack | `filesystem`'s `Healthy` reports the dead mount | `NodePublishVolume` heals bottom-to-top before bind-mounting, so the pod does not inherit a dead mount (§9) | + +--- + +## 14. Observability + +The CSI node plugin emits no Kubernetes events and exposes no Prometheus metrics +today. Its entire observability surface is `klog`, which is why a stack that +stranded itself on a host is currently found by reading logs. Both tables below +are therefore new infrastructure in this design rather than additions to an +existing registry, and both are Phase 1 work: a layered bring-up that cannot be +observed per layer is harder to debug than the inline version it replaces, not +easier. + +### Kubernetes Events + +Events need a target object. The node plugin holds a `kubernetes.Interface` and a +shared `sbkube.Manager` that resolves the PV and the PVC for a volume handle +already, so the PVC is the target: it is the object a user owns and looks at, and +it outlives the pod. + +| Event | Type | Reason | +|------------------------------------------------------------------|---------|--------------------------------| +| A layer's `Ensure` failed and the stack was released back | Warning | `VolumeStackEnsureFailed` | +| A clone's device was re-identified before activation | Normal | `VolumeStackIdentityResolved` | +| An interrupted object was completed rather than treated as ready | Normal | `VolumeStackPartialCompleted` | +| A shared subsystem was left connected for its co-tenants | Normal | `VolumeStackSubsystemShared` | +| A `Release` fell back to its force path | Warning | `VolumeStackForceReleased` | +| The node lacks a capability the plan requires | Warning | `VolumeStackCapabilityMissing` | +| A layer was healed under a live stack | Normal | `VolumeStackLayerHealed` | + +### Prometheus Metrics + +| Metric | Labels | Description | +|-----------------------------------------------------|------------------|-----------------------------------------------------------------------------------------------------| +| `simplyblock_csi_node_stack_layer_duration_seconds` | `layer`, `verb` | Histogram of `Observe`, `Ensure`, `Release`, `Destroy`, `Heal`, and `Grow` durations per layer kind | +| `simplyblock_csi_node_stack_layer_errors_total` | `layer`, `verb` | Failed layer operations by kind | +| `simplyblock_csi_node_stack_force_release_total` | `layer` | Releases that fell back to a force path, which is the signal that stacks are being stranded | +| `simplyblock_csi_node_stack_observed_state_total` | `layer`, `state` | `Observe` outcomes, so `StateForeign` and `StatePartial` are countable rather than anecdotal | +| `simplyblock_csi_node_stacks` | `plan` | Stack records present on this host, by plan shape | +| `simplyblock_csi_node_stack_records_orphaned` | — | Records whose volume no longer has a pod or a staging path on this node | + +`simplyblock_csi_node_stacks` and `simplyblock_csi_node_stack_records_orphaned` +are what make the host-local record (§6) usable operationally: an orphan count +that is not zero is the alert, and the record names what to remove. + +--- + +## 15. Testing Strategy + +Full scenario matrix, coverage status, and hand-off test concepts: +[`tests/test-plan-node-volume-stack.md`](../tests/test-plan-node-volume-stack.md) + +- **Unit:** plan construction is a pure function and every plan in §3 must be + derived from its volume context, capability, and role without a host. The four + pNFS rows are the cases that matter, because they differ by role alone and a + plan function that ignored it would return the MDS plan on a client. The runner's + ordering, its unwind rule, and its refusal to `Destroy` on a failed `Up` are + provable against fake layers that record their calls, which is where the + highest-value coverage sits: a fake layer set makes "a failed `Ensure` at index + 2 releases index 1 and index 0, top-down, and destroys nothing" a table test. + `State` classification per layer is testable against a faked host surface. +- **Integration:** the record's write-ahead ordering and its survival across a + simulated crash, against a temporary directory rather than a cluster. The + format (§6.1) adds a round-trip over every plan in §3, a refusal on an + unrecognized version, and a refusal on a truncated file, which is the case a + crash mid-rename must not be able to produce. There is + no `envtest` component to this design in Phases 1 through 3, because nothing + reconciles. Phase 4 adds the controller-side plan derivation and with it the + first integration surface. +- **E2E:** every plan in §3 that is in scope, staged and unstaged on a live + cluster with data written and checksummed across the cycle. The claims that + only a live cluster can settle are the ones PR #402 had to settle by hand: a + pod delete and recreate reattaches rather than reformats, a node reboot + reattaches every stack on the host, a clone and its source coexist on one node, + and an unclean disconnect leaves no orphaned device-mapper stack. The e2e suite + is Ginkgo (`csi-driver/e2e`), and these are new `SPDKCSI-` blocks. +- **Load and long-running:** genuinely concurrent staging of several + LVM-backed volumes on one host, which is the specific gap §12 names and which + PR #402's validation did not reach. It decides §17 Q2: overlapping `pvscan` and + `vgchange` either survive, and `lvmPV` and `lvmVolume` keep separate keys, or + they do not, and both layers return the one key that serializes all LVM work. + +Risk concentrates in §4.2 and §7.3. A `State` misclassification formats a volume +that had data, and an unwind that calls `Destroy` removes one. Those scenarios +must not be the ones cut when the schedule slips. + +--- + +## 16. Migration Strategy + +**Phase 1 changes no observable behavior.** The plan for every volume that stages +today is `fabric` → `filesystem`, and the runner performs the same connect, +format, and mount calls in the same order. The migration is that +`NodeStageVolume` stops performing them directly. + +**Volumes staged before Phase 1 need no action.** They have no stack record, and +§6 defines an absent record as the legacy plan, which is the plan those volumes +were built with. Nothing is restaged, no node is drained, and a rolling upgrade of +the node DaemonSet is sufficient. + +**Phase 2 moves PR #402's code rather than rewriting it.** The commands, the +derived names, the force paths, and the clone resolution move behind the contract +unchanged, because their value is validation on live hardware that a rewrite would +discard (§11). + +**No VDO volume needs a legacy plan.** PR #402 and Phase 2 land in the same +release, so no released version ever stages a VDO volume without recording its +plan. The legacy plan of §6 is therefore always `fabric` → `filesystem`, and +nothing infers a plan from `client_compression` or `client_deduplication`. A +cluster tracking `main` between the two merges is the only way to reach a VDO +stack with no record, and such a volume is restaged rather than inferred. + +**Phase 3 removes the special cases it replaces.** `healVolumeBeforePublish`, +`ensureDeviceConnected`, `restageVolume`, and `NodeExpandVolume`'s block-device +branch become runner calls, and the VDO conditionals PR #402 added to +`restageVolume` and `NodeExpandVolume` are deleted rather than ported. + +**Phase 4 is additive on the controller side.** `dhchapAllowedNodeSegment` and +`vdoCapableSegment` are replaced by plan-derived segments that produce the same +topology keys and the same values, so existing `PersistentVolume.spec.nodeAffinity` +stays valid and no PV is rewritten. + +--- + +## 17. Open Questions + +| # | Question | Owner | +|-----|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------------| +| 1 | **Package name.** `atlas-lib/volstack/` is provisional. The existing package names in `atlas-lib` are short concrete nouns (`nvme`, `nvmeof`, `lvol`, `nqn`), and this one is neither short nor a noun anybody uses out loud | — | +| 2 | **Whether the LVM layers share one lock key or hold two.** [Appendix B](#appendix-b-lockscope) proposes the mechanism. What decides the granularity is empirical: concurrent `pvscan` and `vgchange` on one host is unexercised rather than proven safe, and the answer is a load-test result (§15). Until it is taken, the runner locks per volume | — | +| 3 | **Where the stack record lives.** Host-local (§6) works when the operator is unreachable and needs no RPC on the unstage path. Operator-side over csi-link is visible cluster-wide and finds orphans without a host sweep. Phase 1 takes the host-local file, and whether the operator-side record is an addition or a replacement is open | — | +| 4 | **Whether `Destroy` is reachable from a node RPC at all.** For an LVM stack the metadata dies with the logical volume the control plane deletes, so `Destroy` would only ever remove node-local remnants. For a pNFS export it does not, and `DeleteExport` genuinely destroys. If the answer is "only a deletion path," the node RPCs get a narrower contract than §4.1 gives them | — | +| 5 | **`nfsExport` and `nfsMount` as layers.** §5 asserts they fit the contract. That is a claim this design cannot verify, because it does not build them. The pNFS designs are where it is settled, and a verb they cannot express is a finding against §4.1 | pNFS designs | +| 6 | **Whether Phase 4 ships.** It is planned, not committed. The cost of leaving it out is a third hand-written copy of the topology pattern when pNFS lands | — | + +--- + +## Appendix A: `blockdev.Device` + +The type `Artifact.Devices` holds (§4.3). It lives in `atlas-lib/blockdev` rather +than in `volstack`, because what it describes is a property of the host and not of +this contract. + +```go +// Device is one Linux block device as the kernel presents it, independent of +// what produced it: an NVMe namespace, a device-mapper node, or a disk handed to +// a storage cluster at deployment. +type Device struct { + Path string // the canonical /dev path + Name string // the kernel name: "nvme0n1", "dm-3" + Major, Minor uint32 + LogicalBlockSize uint32 + PhysicalBlockSize uint32 + SizeBytes uint64 + ReadOnly bool +} +``` + +**A path is not an identity, and it is not sufficient either.** `/dev/dm-3` and +`/dev/mapper/vg--name-lv` are one object under two strings whose escaping rules +§5.4 already has to reason about, and §6 says outright that a path does not +survive a reconnect. Carrying the major and minor numbers beside the path puts the +stable identifier where a layer above can compare on it. + +**The block sizes are the `Geometry` argument again.** A `mkfs` aligns to the +logical block size as well as to the stripe layout, and `Geometry` does not carry +it: a virtualized device reports `Geometry{}` and still has a block size. Derived +at the call site, that is a fifth inspection of a path, which is the shape §4.3 +exists to remove. Reported as a field, the `filesystem` layer reads it. + +**`Device` is the intersection and not a union.** No NVMe field, no LVM field, and +no discriminator saying which produced it. A layer needing NVMe specifics resolves +`nvme.Device` from the path, and a `Device` that grew an `isNVMe` field would be +the conditional this contract replaces. + +**It is a split rather than a new type.** `atlas-lib/nvme`'s `Namespace` already +carries the name, the device path, the major and minor numbers, the logical block +size, the capacity, and the read-only flag, resolved from sysfs and exercised +there. Those are facts about a block device rather than facts about NVMe. +`blockdev.Device` is that half named on its own, and `Namespace` keeps the +NVMe-specific remainder. The split is Phase 1 work because §4.3 depends on the +type, and it is independent of PR #402: it touches `nvme` and nothing PR #402 +wrote. + +**A second consumer puts it in `atlas-lib`.** Handing logical block devices to a +storage cluster at deployment needs the same value on the operator side, where +nothing represents a device today. A `Device` +defined inside `volstack` would be written a second time within one release. + +**Resolution is deliberately absent.** The type is a value in Phase 1, following +the immutable-snapshot convention `atlas-lib/nvme` already holds to: a snapshot is +re-resolved rather than refreshed in place. A resolver reading these fields from +sysfs can be added beside it when a consumer needs one, and adding it changes +nothing a layer holds. + +--- + +## Appendix B: `LockScope` + +The candidate mechanism for §12's lock scopes. It is an appendix and not part of +§4 because §17 Q2 is open: the granularity the mechanism would carry is a +measurement, and the contract does not depend on the answer. + +```go +// LockScope is implemented by a layer whose commands are not safe to run at the +// same time as the same commands for another volume. It names the lock the runner +// holds around every verb it dispatches to the layer. +type LockScope interface { + // LockKey is the lock this layer needs held. A layer constructed for one + // volume returns that volume's handle, which is per-volume scope. A constant + // is host-wide scope, shared by every layer that returns it. + LockKey() string +} +``` + +A layer that does not implement it is serialized per volume. `fabric` returns its +volume handle. `lvmPV` and `lvmVolume` return a key naming the LVM work, so two +volumes staging at once serialize through `pvscan` and `vgchange` while their +fabric connects still run concurrently. `nfsExport` returns a key naming +`/etc/exports`, which is one file per host. + +**The scope is a key and not a named scope.** Whether `lvmPV` and `lvmVolume` +return one key or two follows from whether overlapping `pvscan` and `vgchange` are +safe. As a key that answer changes a returned value; as a choice between "per +volume" and "host-wide" it would change the contract. + +The keys are acquired against a registry in `atlas-lib/locks`, generalized from +`csi-driver/pkg/util`'s `VolumeLocks`: the same map of mutexes, keyed by a string +instead of by a volume ID, with `locks.WithLock` scoping each acquisition to one +call so that a verb returning early or panicking cannot leave a key held. +`VolumeLocks` becomes a caller of it rather than a second implementation. + +A verb runs entirely inside its own acquisition, so work owed to the lock happens +in the verb and cleanup before release is a `defer` there. `sync.Mutex` is not +reentrant, which makes a key held by a composite layer unavailable to its members +(§5.2): a member needing the LVM key cannot be run by a parent already holding it. + +Adopting the mechanism adds one metric to §14, +`simplyblock_csi_node_stack_lock_wait_seconds`, labeled by `layer` and by scope +kind. It is never labeled by the key, because a volume handle is unbounded label +cardinality. diff --git a/operator/docs/tests/test-plan-node-volume-stack.md b/operator/docs/tests/test-plan-node-volume-stack.md new file mode 100644 index 000000000..baf35f16d --- /dev/null +++ b/operator/docs/tests/test-plan-node-volume-stack.md @@ -0,0 +1,441 @@ +# Test Plan: Node-Side Volume Stack + +Related design: [`designs/design-node-volume-stack.md`](../designs/design-node-volume-stack.md) +Harness: [`csi-driver/pkg/util`](../../../csi-driver/pkg/util), [`csi-driver/pkg/spdk`](../../../csi-driver/pkg/spdk), [`csi-driver/e2e`](../../../csi-driver/e2e) + +Scope: the operator, the CSI driver, and the Kubernetes surface of this +repository. Control-plane (`sbcli`) and SPDK behavior is a dependency, faked at +the boundary. LVM, device-mapper, VDO, and the NVMe-oF kernel driver are host +dependencies, faked through a command runner and a sysfs fixture for the unit +class and exercised for real in the end-to-end class. + +Scenario IDs are permanent: `U-` unit (no cluster: pure functions, fake host +surface, mock HTTP), `I-` integration (the runner against a real temporary +directory and a faked host, no Kubernetes), `E-` end-to-end (live cluster, real +data path), `M-` manual (needs failure injection not yet automated). Types are +`Positive`, `Negative`, `Boundary`, `Regression`. The `Test` column names the +implementing function, or `—` when the scenario is not yet covered. Every `—` +is accounted for in §9 What Is Not Yet Covered. + +Section references written as `§n` mean this plan. References to the design are +written `design §n`. + +--- + +## Axes Selected + +| Axis | Applies | Reading for this feature | +|-----------------------------|---------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| A. Storage cluster topology | Yes, reframed | Node count is visible to the node service only as the number of NVMe-oF endpoints the control plane returns, so the axis is exercised as path count: one path (single-node cluster), several paths in priority order, and one of several unreachable | +| B. Namespace scope | Yes | LVM object names are derived from the logical volume's UUID, so the same PVC name in two namespaces must produce distinct volume groups (design §5.4) | +| C. Cluster count | Yes | Two `StorageCluster`s in one Kubernetes cluster put two volumes on one host whose stack records and LVM names must not collide | +| D. Failure domains | No | The node service performs no placement. Node eligibility is design §10 and is covered by the Phase 4 block in §5 | +| E. Object scale | Yes | Plan length (one layer through five), and stack count per host (zero, one, many) | +| F. Lifecycle and timing | Yes, primary | A crash at each point in `Up`, a node reboot, an unstage after total path loss, and a redundant RPC after success are the axis this design exists to get right | +| G. Trigger and actor | Yes | Six entry points drive the runner differently (design §7.5), and which verb each may call is the contract | +| H. Component version skew | Yes | A volume staged before Phase 1, a VDO volume staged by PR #402, and a record naming a layer the running version does not know | + +--- + +## 1. Unit Tests + +No host and no cluster. Layers are faked as recorders that log the verb, the +index, and the artifact they received, so ordering and unwind rules are table +tests. LVM and NVMe behavior is faked through a command runner and a sysfs +fixture of the kind `csi-driver/pkg/util/initiator_device_test.go` already +builds. Numbering runs continuously across the groups below. + +### Plan Construction (design §3) + +File: `csi-driver/pkg/spdk/plan_test.go` (new) + +| # | Scenario | Type | Test | +|------|----------------------------------------------------------------------------------------------------------------------------------------------|----------|------| +| U-01 | `volumeMode: Block` with no filesystem parameters yields `fabric` alone | Positive | — | +| U-02 | `fsType=ext4` yields `fabric` → `filesystem` | Positive | — | +| U-03 | `fsType=xfs` yields `fabric` → `filesystem`, with the XFS feature options set | Positive | — | +| U-04 | `client_compression` set alone yields `fabric` → `lvmPV` → `lvmVolume(vdo)` → `filesystem` | Positive | — | +| U-05 | `client_deduplication` set alone yields the same plan: a dedup-only volume still needs a working `kvdo` module | Positive | — | +| U-06 | Both client-side parameters set yields one `lvmVolume(vdo)` layer, not two | Boundary | — | +| U-07 | Neither client-side parameter set yields no LVM layer at all, asserted by plan length | Negative | — | +| U-08 | An unset `fsType` defaults to the plain filesystem plan rather than raw block | Boundary | — | +| U-09 | An unrecognized `fsType` is rejected at plan construction, not at `mkfs` | Negative | — | +| U-10 | A malformed volume handle is rejected before any layer is constructed | Negative | — | +| U-11 | `nsId` absent or below one is rejected, preserving today's initiator-factory validation | Negative | — | +| U-12 | Plan construction is a pure function: the same context and capability yield an equal plan twice, with no host access | Positive | — | +| U-13 | The plan derived from a volume context is identical to the plan recorded for that volume, so a re-derivation never disagrees with the record | Positive | — | + +### Runner Ordering and Unwind (design §7) + +File: `atlas-lib/volstack/runner_test.go` (new) + +| # | Scenario | Type | Test | +|------|----------------------------------------------------------------------------------------------------------------------------------------------------------|----------|------| +| U-14 | `Up` calls `Ensure` bottom to top, each layer receiving the artifact the layer below returned | Positive | — | +| U-15 | `Up` calls `Observe` before `Ensure` on every layer | Positive | — | +| U-16 | `Ensure` failing at index 2 releases index 1 and index 0, in that order | Negative | — | +| U-17 | `Ensure` failing at index 2 calls `Destroy` on nothing at all, asserted by a zero call count on every fake layer | Negative | — | +| U-18 | `Observe` failing at index 2 unwinds identically to `Ensure` failing there | Negative | — | +| U-19 | `Ensure` failing at index 0 releases nothing and returns the error | Boundary | — | +| U-20 | A `Release` that fails during an unwind does not stop the unwind of the layers below it | Negative | — | +| U-21 | `Down` calls `Release` top to bottom | Positive | — | +| U-22 | `Down` calls `Destroy` on nothing, asserted by a zero call count | Negative | — | +| U-23 | `Down` skips `Release` on a layer whose `Observe` reports `StateAbsent` | Negative | — | +| U-24 | `Down` still removes the stack record when a layer's `Release` left its object present, which is the shared-subsystem case | Positive | — | +| U-25 | A plan of one layer brings up and down correctly | Boundary | — | +| U-26 | An empty plan is rejected at construction rather than running as a no-op `Up` | Boundary | — | +| U-27 | `Up` is idempotent: a second `Up` over a fully ready stack performs no `Ensure` that changes anything, asserted by the fakes' recorded state transitions | Positive | — | +| U-28 | `Down` is idempotent: a second `Down` after a complete one is a no-op and does not error on the missing record | Positive | — | +| U-29 | The delete path calls `Down` and then `Destroy`, top to bottom, in that order | Positive | — | + +### State Classification (design §4.2) + +File: `atlas-lib/volstack/layers/state_test.go` (new) + +| # | Scenario | Type | Test | +|------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------|------------|------| +| U-30 | A device carrying no LVM signature classifies as `StateAbsent` | Positive | — | +| U-31 | A volume group with its logical volume present classifies as `StateReady` | Positive | — | +| U-32 | A volume group present but reporting zero logical volumes classifies as `StatePartial`, not `StateReady` | Boundary | — | +| U-33 | A volume group present, complete, and not activated classifies as `StateInactive`, not `StateAbsent` | Boundary | — | +| U-34 | A device whose on-disk volume group belongs to another volume classifies as `StateForeign` | Negative | — | +| U-35 | `Ensure` on `StateInactive` activates and issues no `vgcreate`, `lvcreate`, or `mkfs`, asserted by the command runner's recorded calls | Negative | — | +| U-36 | `Ensure` on `StateForeign` re-identifies before activating, asserted by the order of the recorded calls | Positive | — | +| U-37 | `Ensure` on `StatePartial` completes the object and does not recreate the volume group | Positive | — | +| U-38 | An LVM probe whose output carries a `WARNING:` line ahead of the field value still classifies correctly, which a byte-level clone produces (pins PR #402 defect 7) | Regression | — | +| U-39 | A probe that fails outright classifies as `StateAbsent` rather than propagating an error, matching the "nothing to resolve" reading | Boundary | — | +| U-40 | An unformatted device classifies as `StateAbsent` for the filesystem layer, and a formatted one as `StateInactive` when unmounted | Positive | — | + +### Artifact and Geometry Propagation (design §4.3) + +File: `atlas-lib/volstack/artifact_test.go` (new) + +| # | Scenario | Type | Test | +|------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------|------------|------| +| U-41 | A `fabric` layer over a backend with known striping reports that geometry upward | Positive | — | +| U-42 | An `lvmVolume(vdo)` layer reports the zero `Geometry`, because VDO virtualizes blocks | Positive | — | +| U-43 | The `filesystem` layer passes `mkfs.xfs` no stripe alignment when the layer below reports the zero `Geometry` (replaces PR #402's `xfsStripeOptions` conditional) | Regression | — | +| U-44 | The `filesystem` layer passes `mkfs.xfs` the XFS feature options regardless of geometry, because feature-bit compatibility is unrelated to layout | Positive | — | +| U-45 | An `lvmVolume(striped)` layer reports its own stripe count and chunk size, and the filesystem above aligns to those rather than to the StorageClass parameters | Positive | — | +| U-46 | A `Geometry` with a stripe count but no chunk size is treated as unknown rather than half-applied | Boundary | — | +| U-47 | A fan-in layer reports its member devices in the recorded order, and a differently ordered member list produces a different artifact | Positive | — | + +### The Stack Record (design §6) + +File: `atlas-lib/volstack/record_test.go` (new) + +| # | Scenario | Type | Test | +|------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|----------|------| +| U-48 | The record is written before the first `Ensure` runs, asserted by the fakes observing the file already present | Positive | — | +| U-49 | The record holds layer parameters and no device path, so a reconnect that renames the device leaves it valid | Positive | — | +| U-50 | A per-layer marker is written before that layer's `Ensure`, not after | Positive | — | +| U-51 | The record is removed only after the last `Release` succeeds | Positive | — | +| U-52 | A `Release` that fails leaves the record in place, so the stack stays discoverable | Negative | — | +| U-53 | An absent record resolves to the legacy plan `fabric` → `filesystem` | Negative | — | +| U-54 | A record naming an unknown layer fails the unstage with the layer named, rather than skipping the layer | Negative | — | +| U-55 | A truncated or malformed record fails with an error and does not resolve to the legacy plan, because a partial record is not an absent one | Boundary | — | +| U-56 | Two volumes from two `StorageCluster`s produce distinct record filenames, because the volume handle carries the cluster ID | Positive | — | +| U-57 | The same PVC name in two namespaces produces distinct record filenames and distinct LVM names | Positive | — | +| U-58 | A record filename is filesystem-safe for every volume handle the driver accepts | Boundary | — | + +### LVM Naming and Primitives (design §5.3, §5.4) + +File: `atlas-lib/lvm/lvm_test.go` (moved from `csi-driver/pkg/util/vdo.go`'s tests) + +| # | Scenario | Type | Test | +|------|-----------------------------------------------------------------------------------------------------------------------------------------------------------|------------|------| +| U-60 | The volume group name is derived from the logical volume's UUID and nothing host-specific, so it is reproducible on another host | Positive | — | +| U-61 | The derived name is within LVM's length and character limits for every accepted UUID form | Boundary | — | +| U-62 | Two volumes on one host derive distinct volume group names | Positive | — | +| U-63 | The device-mapper force path escapes the volume group name by doubling dashes before matching (pins PR #402 defect 9) | Regression | — | +| U-64 | The force path matches nothing and returns cleanly when no device-mapper node for the group exists | Negative | — | +| U-65 | Every LVM invocation carries `DM_DISABLE_UDEV=1`, because no udev daemon runs in the node container (pins PR #402 defect 4) | Regression | — | +| U-66 | Every LVM invocation is scoped to the device under management rather than scanning every visible device | Positive | — | +| U-67 | `Grow` extends by the additive percentage form, so the computed target is never smaller than the current size (pins PR #402 defect 6) | Regression | — | +| U-68 | `Grow` on a volume already at its target size succeeds without issuing an extend, which is kubelet's post-success retry (pins PR #402's open polish item) | Regression | — | +| U-69 | An LVM command that exceeds its timeout returns an error rather than blocking the caller | Negative | — | + +### Co-Tenant Detach (design §8) + +File: `csi-driver/pkg/util/initiator_device_test.go`, extended + +| # | Scenario | Type | Test | +|------|-------------------------------------------------------------------------------------------------------------------------------------------|----------|---------------------------------------------------------| +| U-70 | A subsystem that can hold several namespaces is not disconnected when one volume releases, and the paths stay up | Positive | — | +| U-71 | A subsystem that cannot hold several namespaces is disconnected on release | Positive | `TestDisconnectGlobOnLastNamespace` | +| U-72 | A subsystem currently holding co-tenants is not disconnected | Positive | `TestDisconnectGlobOnRealNode` | +| U-73 | A subsystem that can be shared but currently holds one namespace is still not disconnected, which the current count-based gate gets wrong | Boundary | — | +| U-74 | The capability question failing to resolve leaves the fabric untouched and returns the error, rather than assuming either answer | Negative | — | +| U-75 | Releasing a volume on a shared subsystem removes that volume's stack record and leaves the co-tenant's record alone | Negative | — | +| U-76 | `Destroy` on one volume's LVM objects issues no command naming a co-tenant's volume group | Negative | — | +| U-77 | A namespace device belonging to a neighboring namespace is not selected as this volume's device | Negative | `TestMatchNamespaceDeviceRejectsNeighbouringNamespaces` | + +### Optional Interface Dispatch (design §4.4, §9) + +File: `atlas-lib/volstack/optional_test.go` (new) + +| # | Scenario | Type | Test | +|------|------------------------------------------------------------------------------------------------------------------------------------|----------|------| +| U-78 | `Heal` visits only the layers implementing `Healer`, bottom to top | Positive | — | +| U-79 | `Heal` skips a layer whose `Healthy` returns true, asserted by a zero `Heal` call count on it | Negative | — | +| U-80 | A plan whose layers implement no `Healer` heals as a no-op and returns no error | Boundary | — | +| U-81 | `Heal` on a layer receives the artifact of the layer below as re-derived after that layer was healed, not the artifact from before | Positive | — | +| U-82 | `Grow` visits only the layers implementing `Grower`, bottom to top | Positive | — | +| U-83 | A plan whose layers implement no `Grower`, which is the pNFS client shape, grows as a no-op | Boundary | — | +| U-84 | `Grow` stops and reports when a lower layer's grow fails, and does not attempt the layers above it | Negative | — | +| U-85 | `Heal` never calls `Ensure`, asserted by a zero call count, because the data already exists | Negative | — | + +--- + +## 2. Integration Tests + +The runner against a real temporary directory for the stack record and a faked +host surface for the layers. No Kubernetes and no `envtest`: nothing in Phases 1 +through 3 reconciles. The value of this class is crash simulation, which the unit +class cannot express because it needs a record that survives the process. + +### Crash and Resume (design §6, §7) + +File: `atlas-lib/volstack/resume_test.go` (new) + +| # | Scenario | Type | Test | +|------|-----------------------------------------------------------------------------------------------------------------------------------------------------|----------|------| +| I-01 | `Up` interrupted after the record is written and before the first `Ensure`: a fresh `Down` finds the record and releases nothing, leaving no orphan | Positive | — | +| I-02 | `Up` interrupted after `fabric`'s `Ensure` and before its marker would have been cleared: a fresh `Down` releases the fabric | Positive | — | +| I-03 | `Up` interrupted at each layer index in turn: a fresh `Down` releases exactly the layers that were reached | Positive | — | +| I-04 | `Up` interrupted mid-`Ensure` on a layer that had created its object: the next `Up` observes `StatePartial` and completes it | Positive | — | +| I-05 | `Down` interrupted after two layers were released: a second `Down` releases the rest and removes the record | Positive | — | +| I-06 | The whole host restarts with a record present and every layer inactive: `Up` reactivates and issues no create or format command | Positive | — | +| I-07 | A record present for a volume that has no staging path and no pod on this host is reported as orphaned rather than released automatically | Negative | — | +| I-08 | The record directory is unwritable: `Up` fails before its first side effect rather than proceeding unrecorded | Negative | — | +| I-09 | Two records for the same volume handle cannot exist, and a second `Up` reuses the first | Boundary | — | +| I-10 | 100 records on one host are enumerated and classified without exceeding the enumeration's bound, and the time is recorded | Boundary | — | + +### Verb Contract Under a Dead Foundation (design §7.4) + +File: `atlas-lib/volstack/deadfoundation_test.go` (new) + +| # | Scenario | Type | Test | +|------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------|------------|------| +| I-11 | Every layer's `Release` succeeds when the layer below reports `StateAbsent` | Positive | — | +| I-12 | `lvmVolume`'s `Release` falls back to the device-mapper force path when the LVM command fails on every retry, and the fallback is recorded (pins PR #402 defect 8) | Regression | — | +| I-13 | `filesystem`'s `Release` unmounts a dead mount rather than erroring on it | Positive | — | +| I-14 | A layer with no force path whose command depends on a dead foundation reports the failure and leaves the record in place | Negative | — | +| I-15 | `Down` over a stack whose every layer is already gone completes and removes the record | Boundary | — | + +--- + +## 3. E2E Tests + +A live simplyblock cluster and a real data path. Every row that touches data +asserts correctness by checksum across the operation, not merely that I/O +continued. The suite is Ginkgo, and new blocks are named in the existing +`SPDKCSI-` style. + +### No Behavior Change (design §16) + +The Phase 1 claim is that nothing observable changes, so the existing blocks are +the assertion and must pass unmodified. + +| # | Scenario | Type | Test | +|------|----------------------------------------------------------------------------|----------|-------------------------------------| +| E-01 | An ext4 and an XFS volume stage, publish, and unstage through the runner | Positive | `SPDKCSI-FILESYSTEM` | +| E-02 | A raw block volume stages through a one-layer plan | Positive | `SPDKCSI-RAWBLOCK` | +| E-03 | Data survives a pod delete and an immediate re-mount, verified by checksum | Positive | `SPDKCSI-VOLUME-PERSIST` | +| E-04 | A volume reconnects after path loss and the mount recovers | Positive | `SPDKCSI-RECONNECT` | +| E-05 | A volume recovers after total path loss | Positive | `SPDKCSI-RECONNECT-FULLLOSS` | +| E-06 | The guardian repairs a broken volume under a running pod | Positive | `SPDKCSI-RECONNECT-GUARDIAN` | +| E-07 | An unmanaged subsystem on the host is left alone | Negative | `SPDKCSI-RECONNECT-UNMANAGED` | +| E-08 | A clone and a snapshot restore stage alongside their live source | Positive | `SPDKCSI-CLONE`, `SPDKCSI-SNAPSHOT` | +| E-09 | Volumes from two `StorageCluster`s stage on one host | Positive | `SPDKCSI-MULTICLUSTER` | +| E-10 | An invalid request is rejected as it is today | Negative | `SPDKCSI-NEGATIVE` | + +### Stacked Plans (design §3, §5) + +| # | Scenario | Type | Test | +|------|------------------------------------------------------------------------------------------------------------------------------------------------------|------------|------| +| E-11 | A VDO-backed ext4 volume stages, and `lvs` reports compression and deduplication enabled | Positive | — | +| E-12 | A VDO-backed XFS volume stages with no stripe alignment passed to `mkfs.xfs` | Positive | — | +| E-13 | A pod delete and recreate on the same node reattaches the same VDO device, and the checksum matches (pins PR #402 defect 5, the destructive unstage) | Regression | — | +| E-14 | A node reboot reattaches every stack on the host, with a checksum per volume and the `kvdo` module usage count matching the stack count | Positive | — | +| E-15 | A clone and its source coexist on one node with independent volume group identities, both checksums matching their sources | Positive | — | +| E-16 | An unclean NVMe-oF disconnect under a live pod, followed by a pod delete, leaves no orphaned device-mapper stack and needs no manual intervention | Regression | — | +| E-17 | A PVC expansion grows the LVM stack and then the filesystem online, with data intact throughout | Positive | — | +| E-18 | A second expansion request after a successful one is a no-op and logs no error | Boundary | — | +| E-19 | A volume whose plan needs a capability the node lacks fails to stage there with the reason visible on the PVC | Negative | — | +| E-20 | Two volumes on a shared subsystem: unstaging one leaves the other serving I/O with no error | Positive | — | +| E-21 | Deleting the PVC of one volume on a shared subsystem leaves the co-tenant's volume group intact | Negative | — | +| E-22 | The same PVC name in two namespaces, both staged on one node, produce two distinct volume groups and two distinct datasets | Positive | — | +| E-23 | A volume staged before the upgrade unstages cleanly after it, with no record present | Positive | — | +| E-24 | A VDO volume staged by the pre-Phase-2 code unstages cleanly after Phase 2, without destroying data | Regression | — | +| E-25 | A single-path volume, from a single-node storage cluster, stages and unstages | Boundary | — | +| E-26 | A multipath volume with one endpoint unreachable stages on the reachable paths and reports the unavailable one | Negative | — | +| E-27 | Sustained fio across a stage, heal, and expand cycle: no I/O errors and verify-mode data integrity | Positive | — | + +--- + +## 4. Unit Tests — Phase 4 (Planned) + +Node requirements derived from the plan (design §10). No `Type` and no `Test` +column: both are decided when the phase is scoped, and design §17 Q6 records that +the phase is planned rather than committed. This is the first block with an +`envtest` surface, because the StoragePool controller composes the topology terms. + +| # | Scenario | +|---------|----------------------------------------------------------------------------------------------------------------------------------------------------| +| U-P4-01 | A plan containing a host-local layer yields that layer's capability in the volume's accessible topology | +| U-P4-02 | A plan containing no host-local layer yields no capability segment, which is the pNFS client shape | +| U-P4-03 | Two host-local layers in one plan yield both capabilities, composed into one topology term so they are required together | +| U-P4-04 | A plan-derived segment produces the same key and value the hand-written DHCHAP segment produces, so existing volume node affinity stays valid | +| U-P4-05 | A capability key is present in the node's reported topology at plugin registration regardless of the label's current value (pins PR #402 defect 1) | +| U-P4-06 | A layer reporting `PinsToNode` false contributes no node affinity even when it reports a capability | +| U-P4-07 | The StoragePool controller derives the same topology terms from the plan that the CSI controller derives | + +--- + +## 5. Manual Scenarios and Test Concepts + +### M-01 — Concurrent staging of two LVM-backed volumes on one host + +**Design reference:** design §12 + +**What to verify:** two `Ensure` calls whose LVM commands genuinely overlap do +not corrupt either volume group and do not leave either stack partial. + +**Current behavior:** unknown, and unexercised rather than proven safe. PR #402's +multi-instance validation ran its two stage sequences sequentially rather than +overlapping, so LVM's internal locking under truly concurrent `vgchange` and +`pvscan` has never been exercised on a real host. Per-volume locking does not +serialize them, because the contention is on LVM's host-wide locks and its device +scan. + +**Open question:** design §17 Q2. The lock scope per layer is undecided, so this +scenario is as much a measurement as a test. + +**Test concept:** +1. Provision two PVCs whose StorageClass sets `client_deduplication`, both + scheduled to one node. +2. Start both pods in one `kubectl apply`, so both `NodeStageVolume` calls arrive + within the same second. Confirm from the node plugin's log that the two LVM + command sequences actually interleave, rather than asserting they did. +3. Write and checksum distinct data in each pod. +4. Assert both volume groups exist with the expected logical volume, both + checksums match, and no LVM command reported a lock or a metadata error. +5. Repeat with ten volumes to widen the overlap window. + +### M-02 — A host crash between the record write and the first `Ensure` + +**Design reference:** design §6 + +**What to verify:** the record written ahead of the first side effect is what +makes an orphan discoverable, and a crash in that window leaves nothing attached +that no record names. + +**Test concept:** +1. Build a node plugin whose runner panics after writing the record and before + the first `Ensure`, gated behind an environment variable. +2. Stage a volume with a four-layer plan, triggering the panic. +3. Assert the record exists, no NVMe-oF path is attached, and no device-mapper + node exists. +4. Restart the plugin, unstage, and assert the record is gone. +5. Repeat with the panic moved to each later point in `Up`, asserting that + everything below the panic point is released by the unstage. + +### M-03 — A namespace joins a shared subsystem between the check and the release + +**Design reference:** design §8 + +**What to verify:** the capability gate, not the neighbor count, is what prevents +a destructive disconnect, so a namespace arriving in that window changes nothing. + +**Current behavior:** `selectDisconnectTarget` counts the namespace devices the +by-id glob currently matches, so a subsystem that has just become the last one is +disconnected. A namespace joining immediately afterward loses its paths. + +**Test concept:** +1. Provision two volumes on one subsystem with `max_namespace_per_subsys` above + one, both staged on one node. +2. Unstage the first, and while its `Release` runs, stage a third volume onto the + same subsystem. +3. Assert the second volume never loses a path, and that the third stages + successfully. +4. Repeat with the second volume unstaged first, so the window opens on the last + remaining namespace. + +### M-04 — A record naming a layer the running plugin does not know + +**Design reference:** design §13, design §17 Q1 + +**What to verify:** a downgrade fails loudly rather than skipping an object +nobody will release. + +**Test concept:** +1. Stage a volume with a plan containing a layer, then hand-edit its record to + name a layer the plugin does not implement. +2. Unstage, and assert the RPC fails with the unknown layer named, and that the + record is left in place. +3. Assert the objects the plugin does know about are not released either, because + a teardown that releases half a stack is worse than one that refuses. + +--- + +## 6. Axis Coverage + +| Axis | Values covered | IDs | Not covered | +|-------------------------|-----------------------------------------------------------------------------------------------------------------------|-------------------------------------|-------------------------------------------------------------------| +| A. Path count | One path, several in priority order, one of several unreachable | E-25, E-04, E-26 | More paths than the cluster has nodes | +| B. Namespace scope | Single, two namespaces with the same PVC name | U-01 … U-13, U-58, E-22 | Namespace deleted mid-stage | +| C. Cluster count | One `StorageCluster`, two in one Kubernetes cluster | E-01, U-57, E-09 | Two Kubernetes clusters, which this feature cannot see | +| D. Failure domains | — | — | Excluded: the node service performs no placement (§Axes Selected) | +| E. Object scale | Plan of one layer, of four, of five; zero, one, and 100 stacks per host | U-25, U-04, U-26, I-10, E-14 | More than 100 stacks per host | +| F. Lifecycle and timing | Crash at each `Up` index, crash mid-`Release`, node reboot, redundant post-success RPC, unstage after total path loss | I-01 … I-06, U-68, E-14, E-16, M-02 | Kubelet restart between `Up` and the record's removal | +| G. Trigger and actor | Stage, unstage, publish-heal, expand, restage, delete | U-14, U-21, U-78, U-82, U-85, U-29 | `NodeGetVolumeStats` over a stacked plan | +| H. Version skew | Pre-Phase-1 volume, pre-Phase-2 VDO volume, unknown layer in a record | E-23, U-53, E-24, U-54, U-55, M-04 | A record written by a version two phases ahead | + +--- + +## 7. Coverage Summary + +| Class | Scenarios | Covered | Not covered | +|----------------|-----------|---------|---------------------------------------| +| Unit | 85 | 3 | U-01 … U-70, U-73 … U-76, U-78 … U-85 | +| Integration | 15 | 0 | I-01 … I-15 | +| E2E | 27 | 10 | E-11 … E-27 | +| Unit — Phase 4 | 7 | 0 | U-P4-01 … U-P4-07 | +| Manual | 4 | 0 | M-01 … M-04 | + +The three covered unit scenarios are existing tests whose behavior this design +preserves rather than introduces: `TestDisconnectGlobOnLastNamespace` (U-71), +`TestDisconnectGlobOnRealNode` (U-72), and +`TestMatchNamespaceDeviceRejectsNeighbouringNamespaces` (U-77). The ten covered +end-to-end scenarios are the existing suite, which is the assertion that Phase 1 +changes nothing observable (design §16). + +--- + +## 8. What Is Not Yet Covered + +Nothing in this design is implemented, so the gap list is stated as ranges rather +than as 130 identical rows. The reason column says what each range waits on +rather than repeating "not implemented." + +| # | Gap | Reason | +|-------------------|-------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------| +| U-01 … U-13 | Plan construction | Phase 1. The plan type does not exist, and `plan_test.go` is created with it | +| U-14 … U-29 | Runner ordering and unwind | Phase 1. The highest-value block: U-17 and U-22 are what make the release-never-destroy rule checkable | +| U-30 … U-40 | State classification | Phase 1 for the filesystem states, Phase 2 for the LVM states | +| U-41 … U-47 | Artifact and geometry | Phase 1 for the plain plan, Phase 2 for U-42, U-43, and U-45 | +| U-48 … U-59 | The stack record | Phase 1 | +| U-60 … U-69 | LVM naming and primitives | Phase 2. Six of these pin defects PR #402 fixed and must be ported with the code, not rewritten from the design | +| U-70, U-73 … U-76 | The strengthened co-tenant gate | Phase 1. U-73 fails against the current count-based gate, which is the point of the row | +| U-78 … U-85 | Optional interface dispatch | Phase 3 | +| I-01 … I-15 | Crash, resume, and dead-foundation contracts | Phase 1 for I-01 through I-11 and I-13 through I-15, Phase 2 for I-12 | +| E-11 … E-27 | Stacked plans on a live cluster | Phase 2 for E-11 through E-18 and E-24, Phase 1 for E-20 through E-23 and E-25 through E-27, Phase 4 for E-19 | +| U-P4-01 … U-P4-07 | Plan-derived node requirements | Phase 4, which design §17 Q6 records as planned rather than committed | +| M-01 … M-04 | Concurrency and crash injection | Needs failure injection the suites do not have. M-01 is a risk in shipped code once PR #402 merges, not only in this design | +| — | Two Kubernetes clusters | The node service has no cross-cluster surface. Excluded, not deferred | +| — | Failure domains and placement topology | The node service performs no placement (§Axes Selected) | +| — | More than 100 stacks per host | No bound is claimed above that, so no row asserts one. A bound belongs in the design before a test asserts it | +| — | Kubelet restart between `Up` and the record's removal | The record is designed to survive it (design §6), but the injection point is inside kubelet rather than inside the plugin | +| — | `NodeGetVolumeStats` over a stacked plan | `statfs` on the mount point is indifferent to what is underneath it, so no layer participates. Revisit if a layer ever reports its own capacity | +| — | Block mode combined with an LVM layer | Out of scope (design §2), and representable rather than untested | From 58d6f6beb6008089ae5f3d723fb1e273c25ee957 Mon Sep 17 00:00:00 2001 From: "Christoph Engelbert (noctarius)" Date: Wed, 26 Aug 2026 11:21:42 +0200 Subject: [PATCH 2/4] Use the American spelling "unparsable" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The §13 row read "unparseable", which is the British pattern of keeping the silent e before -able. American English drops it, which is what the house style wordlist already encodes for every comparable pair: useable to usable, moveable to movable, sizeable to sizable. "parse" is not a soft-c or soft-g stem, so it takes the same path. Co-Authored-By: Claude Opus 5 (1M context) --- operator/docs/designs/design-node-volume-stack.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator/docs/designs/design-node-volume-stack.md b/operator/docs/designs/design-node-volume-stack.md index 3a9cafa2c..18dd47ebf 100644 --- a/operator/docs/designs/design-node-volume-stack.md +++ b/operator/docs/designs/design-node-volume-stack.md @@ -1076,7 +1076,7 @@ is answered the runner locks per volume, as the node RPCs do today. | Node rebooted with a stack recorded | Every layer `Observe`s `StateInactive` | `Ensure` reactivates. Nothing is created and nothing is formatted (§4.2) | | Stack record absent at unstage | No file for this volume handle | The legacy plan is assumed: `fabric` → `filesystem` (§6) | | Stack record's version is not recognized | `Version` is a schema this release does not read | The unstage fails and reports the version, on the same reasoning as an unknown layer name: a plan read under the wrong schema releases the wrong objects (§6.1) | -| Stack record present but unparseable | The file does not decode | The unstage fails and reports it rather than falling back to the legacy plan. That fallback answers an absent record, and a corrupt file is not evidence that a legacy stack was built (§6.1) | +| Stack record present but unparsable | The file does not decode | The unstage fails and reports it rather than falling back to the legacy plan. That fallback answers an absent record, and a corrupt file is not evidence that a legacy stack was built (§6.1) | | Stack record present, plan unrecognized | A layer name the running version does not know | The unstage fails and reports the unknown layer, rather than silently skipping an object nobody will release. Layer names are stable across releases for this reason (§4.1) | | Node lacks a layer's capability | The capability label is absent | Phase 4 keeps the volume off the node. Until then `Ensure` fails on its first command and the pod stays `Pending` (§10) | | Total path loss under a live stack | `filesystem`'s `Healthy` reports the dead mount | `NodePublishVolume` heals bottom-to-top before bind-mounting, so the pod does not inherit a dead mount (§9) | From f6c89c475ceb729395e24e2e0a6285a30ea0e77c Mon Sep 17 00:00:00 2001 From: noctarius aka Christoph Engelbert Date: Wed, 26 Aug 2026 11:26:23 +0200 Subject: [PATCH 3/4] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- operator/docs/tests/test-plan-node-volume-stack.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/operator/docs/tests/test-plan-node-volume-stack.md b/operator/docs/tests/test-plan-node-volume-stack.md index baf35f16d..e132caf22 100644 --- a/operator/docs/tests/test-plan-node-volume-stack.md +++ b/operator/docs/tests/test-plan-node-volume-stack.md @@ -13,9 +13,10 @@ Scenario IDs are permanent: `U-` unit (no cluster: pure functions, fake host surface, mock HTTP), `I-` integration (the runner against a real temporary directory and a faked host, no Kubernetes), `E-` end-to-end (live cluster, real data path), `M-` manual (needs failure injection not yet automated). Types are +Types are `Positive`, `Negative`, `Boundary`, `Regression`. The `Test` column names the implementing function, or `—` when the scenario is not yet covered. Every `—` -is accounted for in §9 What Is Not Yet Covered. +is accounted for in §8 What Is Not Yet Covered. Section references written as `§n` mean this plan. References to the design are written `design §n`. From 30419cc26619e9f53629063eb8f02771a9458dee Mon Sep 17 00:00:00 2001 From: Manohar Reddy Date: Fri, 28 Aug 2026 16:44:33 +0200 Subject: [PATCH 4/4] Rewrite the issue #277 design doc and test plan around the shipped implementation The document under this filename described a generalized, pluggable node-side-stack framework that was never built. Replace it with a design doc and test plan describing the actual mechanism PR #402 implements: StoragePool-level VDO compression/deduplication built on atlas-lib/lvm and atlas-lib/lvm/vdo (PR #457), node capability gating, and CSI wiring. Rename both files from design-node-volume-stack.md / test-plan-node-volume-stack.md to design-client-side-vdo-compression.md / test-plan-client-side-vdo-compression.md to match. --- .../design-client-side-vdo-compression.md | 507 +++++++ .../docs/designs/design-node-volume-stack.md | 1323 ----------------- .../test-plan-client-side-vdo-compression.md | 232 +++ .../docs/tests/test-plan-node-volume-stack.md | 442 ------ 4 files changed, 739 insertions(+), 1765 deletions(-) create mode 100644 operator/docs/designs/design-client-side-vdo-compression.md delete mode 100644 operator/docs/designs/design-node-volume-stack.md create mode 100644 operator/docs/tests/test-plan-client-side-vdo-compression.md delete mode 100644 operator/docs/tests/test-plan-node-volume-stack.md diff --git a/operator/docs/designs/design-client-side-vdo-compression.md b/operator/docs/designs/design-client-side-vdo-compression.md new file mode 100644 index 000000000..0c65bc4a7 --- /dev/null +++ b/operator/docs/designs/design-client-side-vdo-compression.md @@ -0,0 +1,507 @@ +# Design Document: Client-Side Compression and Deduplication via VDO + +**Status:** Implemented (PR [#402](https://github.com/simplyblock/simplyblock-operator/pull/402), pending review) +**Author:** Manohar Reddy +**Date:** 2026-08-28 +**Issue:** [#277](https://github.com/simplyblock/simplyblock-operator/issues/277) +**Depends on:** [`atlas-lib/lvm`](https://github.com/simplyblock/simplyblock-operator/pull/457) (merged) +**Test Plan:** [`tests/test-plan-client-side-vdo-compression.md`](../tests/test-plan-client-side-vdo-compression.md) + +--- + +## Overview + +**What this is.** A `StoragePool` opt-in that gives every volume in it client-side +compression and deduplication, independently switchable. The mechanism is VDO +(`dm-vdo`): the CSI node plugin assembles an LVM stack on top of the volume's raw +NVMe-oF device, one physical volume, one volume group, one VDO pool, and one VDO +logical volume, and mounts the VDO logical volume in place of the raw device. VDO +does the actual compression and deduplication, in the kernel, on the client node, +before a write ever leaves the host. + +**Why client-side.** The storage backend already offers server-side compression +per pool. Client-side compression trades that off differently: it spends CPU and +RAM on the node running the pod instead of on the storage node, and it can +deduplicate across writes from the same client in ways the backend cannot see. +Neither replaces the other. A pool picks one, the other, both, or neither. + +**The two layers this is built on.** [`atlas-lib/lvm`](https://github.com/simplyblock/simplyblock-operator/pull/457) +is a general-purpose, typed wrapper around Linux LVM commands: `PhysicalVolume`, +`VolumeGroup`, and `LogicalVolume` as distinct value types, and a `Manager` whose +methods assemble, activate, deactivate, grow, and inspect an LVM stack, scoping +each command to a device only where LVM's own identity resolution needs it (§3). +[`atlas-lib/lvm/vdo`](#3-atlas-liblvmvdo-the-stack-lifecycle) is the VDO-specific +layer on top: it owns the one-volume-group-per-lvol naming convention, the +create-or-reactivate idempotence a CSI `NodeStageVolume` retry needs, and the +fallback path for a backing device that has gone unreachable without a clean +unstage. Neither layer knows anything about Kubernetes or CSI. `csi-driver/pkg/util/vdo.go` +(§5) is eighty-five lines of wiring between them and the node plugin's RPC +handlers. + +**A node either has VDO or it does not, and that has to be known before a pod +lands there.** `kmod-kvdo` is not available for every kernel this product +supports, so a pool with either client-side flag set carries a topology +requirement, and a node advertises whether it actually has a working `kvdo` +module before the scheduler is allowed to place a pod that needs one (§4). + +**What is out of scope.** Live-toggling compression or deduplication on a volume +that already exists (the mechanism is built, `SetFeatures`, but nothing calls it, +§8). Server-side and client-side compression composing or conflicting on the same +pool (§8). Non-x86_64 hosts (`vdo` has no `aarch64` build in the repositories +this product installs from, §6). + +--- + +## Table of Contents + +1. [Goals and Non-Goals](#1-goals-and-non-goals) +2. [API Design: StoragePool Parameters](#2-api-design-storagepool-parameters) +3. [`atlas-lib/lvm/vdo`: the Stack Lifecycle](#3-atlas-liblvmvdo-the-stack-lifecycle) +4. [Node Capability Gating](#4-node-capability-gating) +5. [CSI Driver Wiring](#5-csi-driver-wiring) +6. [Deployment](#6-deployment) +7. [Failure Modes Found and Fixed](#7-failure-modes-found-and-fixed) +8. [Open Questions](#8-open-questions) +9. [Testing Strategy](#9-testing-strategy) + +--- + +## 1. Goals and Non-Goals + +### Goals + +- Compression and deduplication, each independently switchable per `StoragePool`, + applied on the client before a write reaches the network. +- No change to a volume's identity: the CSI volume handle, the raw NVMe-oF + connection, and everything the control plane knows about the lvol stay exactly + as they are for a volume with neither flag set. VDO is a layer the node adds + locally, not a different kind of volume. +- Idempotent, crash-safe node operations: a `NodeStageVolume` retried after a + partial failure reactivates what already exists rather than recreating it, and + a pod delete-and-recreate on the same node never destroys the volume's data. +- Correct behavior for the entire volume lifecycle a plain volume already + supports: create, stage, expand, clone, snapshot restore, and reconnect after + the storage side disconnects while the node stays up. + +### Non-Goals + +- **A generalized, pluggable node-side stack abstraction** (layers for the + fabric, LVM, and filesystem stages, composed and persisted as a stack record) + was proposed as a superset of this mechanism. It is not built. This document + describes the concrete, VDO-specific path the CSI driver actually runs. A + future feature that needs the same kind of node-side assembly (a striped + volume group across several members, for instance) is free to generalize + `atlas-lib/lvm` further, but nothing here commits to a particular shape for + that ahead of a second real consumer. +- **Live toggling.** Changing `clientCompression`/`clientDeduplication` on a + `StoragePool` whose `StorageClass` already exists has no effect (§2). Doing so + for an individual already-provisioned volume is not wired into any code path + (§8). +- **Server-side and client-side compression interacting.** Both parameters can + be set on the same pool today. Whether that combination is meaningful, wasteful, + or should be rejected is unresolved (§8). +- **Non-x86_64 nodes and non-RHEL-family distributions.** VDO's node-capability + installer shells out to `dnf` (§4). A node whose package manager is not `dnf`, + or whose architecture has no `vdo` build, never becomes VDO-capable (§6). + +--- + +## 2. API Design: StoragePool Parameters + +Two new fields on `StorageClassParameters` (`operator/api/v1alpha1/storagepool_types.go`), +next to the existing server-side `Compression`: + +```go +// Compression enables compression for logical volumes. +// +kubebuilder:default="False" +Compression string `json:"compression,omitempty"` + +// ClientCompression enables client-side (VDO) compression for logical volumes in this +// pool. Distinct from Compression (server-side). Independent of ClientDeduplication -- +// either, both, or neither may be set. Changing this on a Pool whose StorageClass +// already exists has no effect (see issue #401) -- it only takes effect for pools +// whose StorageClass does not exist yet. +// +kubebuilder:default=false +ClientCompression *bool `json:"clientCompression,omitempty"` + +// ClientDeduplication enables client-side (VDO) deduplication for logical volumes in +// this pool. Carries a significant, measured, fixed RAM cost per volume independent of +// ClientCompression -- intended to be opt-in on specific pools where duplicate data is +// actually expected, not enabled by default. Same StorageClass-immutability caveat as +// ClientCompression applies. +// +kubebuilder:default=false +ClientDeduplication *bool `json:"clientDeduplication,omitempty"` +``` + +`StorageClassParameters` as a whole is `+k8s:immutable`, because the Kubernetes +`StorageClass` fields it produces, `Parameters` and `AllowedTopologies`, are +themselves immutable once the object exists. `createStorageClassIfNotExists` +(`operator/internal/controller/simplyblockstoragepool_controller.go`) is create-only +for exactly this reason: there is no drift to reconcile, because the API server +would reject the update. A `StoragePool` whose flags change after its +`StorageClass` already exists keeps the old behavior until a new pool, and a new +`StorageClass`, is created. + +`mergeStorageClassParameters` writes both fields into the `StorageClass`'s +`Parameters` map under the CSI driver's own key spelling: + +```go +dst["client_compression"] = boolStr(p.ClientCompression) +dst["client_deduplication"] = boolStr(p.ClientDeduplication) +``` + +`boolStr` renders `"True"`/`"False"` (capitalized), matching every other boolean +`StorageClassParameters` field. The CSI driver reads these back with +`kube.BoolParam`, which accepts that capitalization alongside the lowercase form. + +### Topology gating + +`createStorageClassIfNotExists` adds a topology requirement whenever *either* +flag is true, not `client_compression` alone: a dedup-only volume still needs a +working `kvdo` module on the node just as much as a compression-only one does. + +```go +if params["client_compression"] == scParamTrue || params["client_deduplication"] == scParamTrue { + topologyExprs = append(topologyExprs, corev1.TopologySelectorLabelRequirement{ + Key: "simplyblock.io/vdo-capable", + Values: []string{"true"}, + }) +} +``` + +This requirement is merged into the *same* `TopologySelectorTerm` as the existing +DHCHAP node-allow-list requirement, when both apply, rather than becoming a +second term. Kubernetes ANDs the expressions within one term and ORs separate +terms, and a pool that is both DHCHAP-restricted and VDO-only needs both +conditions to hold at once, not either one. + +--- + +## 3. `atlas-lib/lvm/vdo`: the Stack Lifecycle + +Every operation below is a function in `atlas-lib/lvm/vdo`, taking a `*lvm.Manager` +and an `lvolID` and returning a device path (create/grow) or an error. None of +them reference a Kubernetes or CSI type: they are node-level LVM/VDO +orchestration, moved out of the CSI driver into `atlas-lib` because nothing about +them is CSI-shaped (§5 explains what the CSI driver keeps instead). Each one is +built from the typed primitives `atlas-lib/lvm` provides: + +```go +type PhysicalVolume struct{ DevicePath string } +type VolumeGroup struct{ Name string } +type LogicalVolume struct { + VolumeGroup VolumeGroup + Name string +} +``` + +A device path, a volume group, and a logical volume are three distinct types +rather than three strings, so a volume group name accidentally passed where a +device path belongs is a compile error rather than an `lvcreate` failure +discovered against a real device. Every simplyblock volume's VDO stack lives in +its own volume group, named `vdo-`, containing exactly one VDO pool +(`vdopool`) and one VDO logical volume (``). + +### `CreateOrAttach`: idempotent create-or-reactivate + +```go +func CreateOrAttach( + ctx context.Context, manager *lvm.Manager, devicePath, lvolID string, compression, deduplication bool, +) (string, error) +``` + +Rescans the device, probes its on-disk volume-group identity, and branches three +ways: + +1. **The volume group already exists and has the logical volume:** reactivate it + (`vgchange -ay`) and return. Nothing is recreated. This is the path a retried + `NodeStageVolume`, or a pod restaged after a routine reconnect, takes every + time after the first. +2. **The volume group exists but the logical volume does not:** an earlier + `pvcreate`/`vgcreate` completed and `lvcreate` did not, the signature of a + create interrupted partway through. The empty volume group is removed and + creation falls through to (3), because reactivating it forever would never + produce a mountable device. +3. **Nothing exists on the device yet:** `pvcreate`, `vgcreate`, then + `CreateLogicalVolume` with a `LogicalVolumeDefinition{Compression, Deduplication}`, + which dispatches through `atlas-lib/lvm`'s `VolumeProvisioning` registry to + contribute the `--type vdo --compression --deduplication ` flags + this package registers at `init()`. + +### `ResolveClone`: a byte-level clone's foreign identity + +```go +func ResolveClone(ctx context.Context, manager *lvm.Manager, devicePath, lvolID string) error +``` + +A CSI clone or snapshot restore is a byte-level copy at the storage layer, so the +new device's on-disk LVM signature is still the *source* volume's: the same +volume group name, the same PV and VG UUIDs. `CreateOrAttach`'s own "does +`vdo-` already exist" check cannot see this, because the volume group on +disk answers to the source's name, not this volume's. `ResolveClone` wraps +`atlas-lib/lvm`'s `ResolveClonedVolumeGroup` (rescan, probe, `vgimportclone` to +regenerate fresh UUIDs and rename the volume group, then rename the logical +volume inside), passing this stack's own `vdopool` name as the one logical volume +to leave alone. Driven from the device's actual on-disk identity rather than a +flag threaded from the CSI `VolumeContentSource`, so it is safe and cheap to call +on any freshly attached device, whether or not it turns out to be a clone. Must +run before `CreateOrAttach`. + +### `Deactivate` and `Remove`: two different answers to the same failure + +```go +func Deactivate(ctx context.Context, manager *lvm.Manager, lvolID string) error +func Remove(ctx context.Context, manager *lvm.Manager, lvolID string) error +``` + +`Deactivate` is the non-destructive counterpart to a plain NVMe-oF disconnect, +called from `NodeUnstageVolume` (§5): it deactivates the volume group +(`vgchange -an`) without destroying anything, because `NodeUnstageVolume` fires +whenever nothing on the node currently needs the volume mounted, including an +ordinary pod delete-and-recreate, not only when the volume is actually being +deleted. `Remove` is `Deactivate`'s destructive counterpart, called only when the +volume itself is being removed: it deactivates and then `vgremove -f`s the +volume group. + +Both fall back to `RemoveOrphanedDMNodes` when the backing device has gone +unreachable without a clean unstage (crash, forced reschedule), because +`vgchange`/`vgremove` need to read and write volume-group metadata that lives on +the now-gone device and cannot do either. The two functions use **different +rules** for when that fallback fires, because they are trying to do opposite +things: `Deactivate` falls back only on the specific "volume group not found" +failure text, since anything else is a real problem worth surfacing rather than +papering over. `Remove` falls back on any failure at all, since the volume is +already being destroyed and there is no worse outcome to protect against. + +### `Grow` + +```go +func Grow(ctx context.Context, manager *lvm.Manager, devicePath, lvolID string) (string, error) +``` + +Grows the physical volume to the device's new full size, then the VDO pool to +consume the newly available space (`ExpandLogicalVolume`'s `-l+100%FREE`, which +is additive, not the absolute-size form `-l100%FREE` would be), then reads the +pool's new size back and grows the VDO logical volume to match it exactly +(`ExtendLogicalVolumeToSize`, an explicit byte count). The VDO logical volume +cannot be grown by the same free-space percentage the pool was, because only the +pool is a real, physical-extent-consuming volume group member. The VDO logical +volume on top is a virtual device sized in bytes, and after the pool's own +`100%FREE` grow there are no volume-group extents left for a second +percentage-based call to reference. + +### `SetFeatures` and node-side logging + +```go +func SetFeatures(ctx context.Context, manager *lvm.Manager, lvolID string, compression, deduplication bool) error +``` + +Toggles compression and deduplication on an already-active VDO volume without +recreating it (`lvchange`). Nothing calls it (§8). + +This package has no Kubernetes dependency anywhere, so warnings (a best-effort +rescan that failed, an unreachable-device fallback firing) go through a +package-level `Logger *slog.Logger`, nil-safe and defaulting to `slog.Default()`, +rather than `klog`. + +--- + +## 4. Node Capability Gating + +`kmod-kvdo` is not available for every kernel this product runs on. A node +therefore has to prove it actually has a working `kvdo` module before the +scheduler is allowed to place a pod whose volume needs one, and that has to +happen without blocking every other node from becoming Ready while it does. + +**Install, at DaemonSet start.** The CSI node DaemonSet runs with `hostPID: true`, +and its `csi-registrar` container's `postStart` hook `nsenter`s into the host PID +namespace to install `kmod-kvdo` and `vdo` via `dnf` if they are not already +present, then `modprobe kvdo`, and writes the result (`true`/`false`) to a marker +file on a host-path volume (`/var/lib/simplyblock/vdo-capable` on the host, +mounted at `/var/run/simplyblock/vdo-capable` in the container): + +```sh +mkdir -p /var/run/simplyblock/vdo-capable; +if nsenter -t 1 -m -u -n -i -- sh -c 'rpm -q kmod-kvdo vdo >/dev/null 2>&1 || dnf install -y kmod-kvdo vdo' && modprobe kvdo; then +echo true > /var/run/simplyblock/vdo-capable/marker; +else +echo false > /var/run/simplyblock/vdo-capable/marker; +fi +``` + +**Advertise, from the running node plugin.** `newNodeServer` spawns +`advertiseVDOCapability` as a background goroutine, which polls the marker file +every five seconds for up to five minutes, then merge-patches the node's own +`simplyblock.io/vdo-capable` label to match. This runs independently of, and does +not block, `NodeStageVolume`/`NodeUnstageVolume`/CSI registration, which is what +lets the `dnf install` (seconds to minutes, and a hard failure on a kernel with no +`kvdo` build) happen without holding up every other volume operation on the node. + +**An operator can override the label by hand.** Every automatic update writes a +`simplyblock.io/vdo-capable-managed-by: auto-detect` annotation alongside the +label. If the label is present without that annotation, `advertiseVDOCapability` +treats it as a deliberate human override and never touches it again. + +**The topology key is always present, only its value changes.** +`buildAccessibleTopology` includes the `simplyblock.io/vdo-capable` key +unconditionally, never omitting it when the value is currently `false`. A CSI +node's topology key *set* is captured once, at plugin registration, moments +after the pod starts. The label itself is patched asynchronously afterward and +can take minutes. Omitting the key while the value is false would mean the key +is essentially never present at the moment registration actually happens, which +would defeat the topology gate entirely rather than merely delaying it. + +**The scheduling pin, not only the storage class gate.** `AllowedTopologies` on +the `StorageClass` (§2) constrains where `WaitForFirstConsumer` binds a PVC, but +it says nothing about where a pod is rescheduled afterward, and a raw NVMe-oF +volume works identically from any node while VDO state does not. `vdoCapableSegment` +in `CreateVolume` merges a `simplyblock.io/vdo-capable=true` segment into the +provisioned `PersistentVolume`'s `AccessibleTopology` whenever either client-side +flag is set, alongside the existing DHCHAP segment, which is what pins +`PersistentVolume.spec.nodeAffinity` so a later pod reschedule cannot land the +volume on a non-VDO-capable node. This mirrors an existing fix for the same gap +in the DHCHAP case (issue #403). + +--- + +## 5. CSI Driver Wiring + +`csi-driver/pkg/util/vdo.go` is thin wiring: one shared `lvm.Manager`, and one +function per RPC concern, each delegating straight into `atlas-lib/lvm/vdo`. +Every call site shares one gate: + +```go +func vdoParams(vc map[string]string) (compression, deduplication, wantsVDO bool) { + compression, _ = kube.BoolParam(vc, paramClientCompression, false) + deduplication, _ = kube.BoolParam(vc, paramClientDeduplication, false) + return compression, deduplication, compression || deduplication +} +``` + +**`NodeStageVolume`** (`pkg/spdk/nodeserver.go`): after `initiator.Connect`, if +`wantsVDO`, calls `ResolveClonedVDO` unconditionally, then `CreateOrAttachVDO`. +The *returned* device path, not the raw NVMe-oF path, is what gets formatted and +mounted. + +**`NodeUnstageVolume`**: calls `DeactivateVDO` before the raw device disconnects. +Order matters here for the same reason `Deactivate` exists at all (§3): this path +runs on every routine unstage, not only on deletion. + +**`NodeExpandVolume`**: raw-block volumes skip filesystem resize entirely, since +the resize tools this driver uses cannot operate on an unmounted raw block +device. For filesystem-mode volumes with `wantsVDO`, `GrowVDO` runs first and the +resize targets the grown VDO device path it returns. + +**`restageVolume`**: the same `CreateOrAttachVDO` call as `NodeStageVolume`, +explicitly idempotent, reactivating rather than reformatting. + +**XFS stripe hints are skipped for VDO.** `stageVolume` only appends +`xfsStripeOptions` to `mkfs.xfs` when `wantsVDO` is false. Those hints are +computed for the raw, erasure-coded backend device. Once VDO is in the stack, the +filesystem sits on a device VDO virtualizes and relocates blocks on, so the +hints no longer describe anything real. + +**`SetVDOFeatures` has no caller.** The mechanism exists (§3, §8) but nothing in +the CSI driver invokes it. + +--- + +## 6. Deployment + +**Image.** `csi-driver/deploy/image/Dockerfile` installs `lvm2` unconditionally +(`pvcreate`/`vgcreate`/`lvcreate`/`vgchange`/`lvextend`/`dmsetup`, all of which +`atlas-lib/lvm` shells out to) and `vdo` only on `amd64`, since `vdo` has no +`aarch64` build in the repositories this image installs from. Client-side VDO is +therefore x86_64-only. The `arm64` leg of the image build still succeeds, with +`lvm2` present but `vdo` and `vdoformat` absent, so `lvcreate --type vdo` would +fail on an `arm64` node regardless of what the topology gate says. `vdo`'s +absence there is a hard architecture limit, not a gap the capability gate closes. + +**Chart.** `helm-charts/charts/simplyblock-operator/templates/node.yaml` adds +`hostPID: true` and the marker-file host-path volume (§4) to the CSI node +DaemonSet. `templates/node-rbac.yaml` grants the node ServiceAccount `patch` and +`update` on `nodes`, on top of the pre-existing `get`/`list`/`watch`, so +`advertiseVDOCapability` can write the label. + +--- + +## 7. Failure Modes Found and Fixed + +Every one of these was found live, against a real cluster, and each is fixed in +`csi-driver/pkg/util/vdo.go`'s history (now folded into `atlas-lib/lvm`) before +the code that carried it shipped: + +- **Duplicate-PV ambiguity.** A simplyblock NVMe-oF HA volume presents two + redundant local device nodes with byte-identical content. An LVM command run + without `--devices` scans every visible device and cannot tell the two apart, + reporting a "duplicate PV" error. Every `atlas-lib/lvm` command that names a + device scopes itself to exactly that device (`atlas-lib/lvm`'s own package doc + comment covers this in full). +- **Name-based existence checks lie.** `vgs ` answers "does a volume group + by this name exist anywhere LVM can see," not "does it exist on this specific + device." On a host whose LVM devices file restricts default visibility, this + reported a volume group as present when it had never been created on the + device actually being asked about, leaving no logical volume behind it. + Replaced with a content-based probe (`pvs` on the specific device). +- **An interrupted create leaves an orphaned volume group.** `pvcreate` and + `vgcreate` completing while `lvcreate` does not leaves a volume group with zero + logical volumes. `vgchange -ay` against it "succeeds" while producing nothing + mountable. `CreateOrAttach` detects the zero-LV case and removes the orphan + before falling through to a fresh create, rather than reactivating it forever. +- **The destructive removal ran on a routine unstage.** Calling the equivalent + of `Remove` from `NodeUnstageVolume` destroyed a volume's VDO metadata on an + ordinary pod delete-and-recreate, well before the volume was ever meant to be + removed. `NodeUnstageVolume` calls the non-destructive `Deactivate` instead. +- **The unreachable-device fallback had no escaping match for device-mapper's own + naming.** `dmsetup` doubles every literal `-` in a compound name. Matching + against the unescaped volume-group name found nothing in `dmsetup ls` output, + leaving an orphaned stack that the fallback could not actually clean up. +- **`pvs`'s combined output pollutes an identity comparison.** A `WARNING:` + line ahead of the actual field value (duplicate-PV warnings on a byte-level + clone, in particular) corrupted both the identity check and any log message + built from the raw output. Both the identity probe and the size probe now read + only the first non-`WARNING:` line. +- **No udev daemon runs inside this container.** `lvcreate --type vdo` shelled + out to `vdoformat`, which needs device-mapper's udev-sync handshake to + complete. Without a udev daemon present it never does, failing with "device + not cleared." `DM_DISABLE_UDEV=1` on every command's environment fixes it. +- **`lvextend -l100%FREE` is absolute, not additive.** Growing the VDO pool with + the unprefixed form computed a target smaller than the pool's already-current + size, since "100% of what is currently free" is smaller than the pool's + existing size. The additive form, `-l+100%FREE`, is what "grow to consume all + newly available space" actually means. + +--- + +## 8. Open Questions + +| # | Question | Owner | +|-----|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-------------| +| Q1 | Should setting both `clientCompression`/`clientDeduplication` and server-side `compression` on the same `StoragePool` be rejected at admission, or left as a valid, if likely wasteful, combination? Nothing today prevents both from being set. | Product | +| Q2 | `SetFeatures` (§3) exists and is tested but has no caller. Toggling compression/deduplication on an already-provisioned volume needs a `VolumeAttributesClass`-driven update path, or an explicit decision that this stays create-time-only. | Product | +| Q3 | The node-capability installer (§4) assumes `dnf` and RHEL-family package naming (`kmod-kvdo`, `vdo`). A Debian/Ubuntu host never becomes VDO-capable today. Whether that is an acceptable permanent restriction or a gap to close is undecided. | Product | +| Q4 | No minimum VDO pool size is enforced in code. A VDO pool below dm-vdo's own practical minimum will fail at `lvcreate` time with whatever error the tool itself produces, not a validated, CSI-level error earlier in provisioning. | Engineering | + +--- + +## 9. Testing Strategy + +`atlas-lib/lvm` and `atlas-lib/lvm/vdo` carry unit coverage for every named +operation in §3, against a fake command runner, with no `lvm2` binary or kernel +module required (`atlas-lib/lvm/vdo/stack_test.go`, `volume_test.go`, and the +general-purpose primitive tests under `atlas-lib/lvm/*_test.go`). `vdoCapableSegment` +(§4) has direct coverage in `csi-driver/pkg/spdk/controllerserver_test.go`. +`csi-driver/pkg/util/vdo.go` itself, the thin wiring in §5, has no direct unit +tests of its own: every branch it adds over the functions it calls is one `if` +around a delegation, and the fake-runner tests already cover the delegated +behavior. + +What unit tests cannot cover, and what §7's findings were all found by instead: +whether `dm-vdo` is actually present and behaves as `atlas-lib/lvm/vdo` assumes, +whether a real duplicate-PV HA volume actually produces the ambiguity §7 +describes, and whether the whole stack survives a genuine node crash or storage-side +disconnect. Live-cluster verification against a real `StoragePool` with both +flags enabled, covering create, reattach-after-recreate, expand, clone/snapshot +resolution, and reconnect after an unclean disconnect, is the test plan's `M-` +series and is the harness every finding in §7 came from. See +[`tests/test-plan-client-side-vdo-compression.md`](../tests/test-plan-client-side-vdo-compression.md). diff --git a/operator/docs/designs/design-node-volume-stack.md b/operator/docs/designs/design-node-volume-stack.md deleted file mode 100644 index 18dd47ebf..000000000 --- a/operator/docs/designs/design-node-volume-stack.md +++ /dev/null @@ -1,1323 +0,0 @@ -# Design Document: Node-Side Volume Stack - -**Status:** Draft -**Author:** Christoph Engelbert (noctarius) -**Date:** 2026-08-25 -**Related Issues:** - -- [#277](https://github.com/simplyblock/simplyblock-operator/issues/277) — client-side compression and deduplication via VDO, whose node-side wiring this design absorbs -- [PR #402](https://github.com/simplyblock/simplyblock-operator/pull/402) — the VDO implementation this design generalizes - -**Test Plan:** [`tests/test-plan-node-volume-stack.md`](../tests/test-plan-node-volume-stack.md) - ---- - -## Phasing Overview - -| Phase | Status | Scope | Behavior change | -|-------------------------|---------|---------------------------------------------------------------------------------------------------------------------------|-------------------------------------------------------------------------| -| **Phase 1** (§4–§8) | Planned | The `blockdev` split, the layer contract, the runner, the stack record, and the `fabric` and `filesystem` layers | None. RWO parity with today's node service | -| **Phase 2** (§5.3–§5.4) | Planned | The `lvmPV` and `lvmVolume` layers, the VDO call sites migrated onto the stack, the LVM primitives moved into `atlas-lib` | None. VDO parity with PR #402 | -| **Phase 3** (§9) | Planned | `Healer` and `Grower`, so heal, restage, and expand walk the stack | Heal and expand become correct for every layer, not only the bottom one | -| **Phase 4** (§10) | Planned | Node requirements derived from the plan on the controller side | Topology gating stops being hand-written per feature | - -Phase 1 is shippable on its own because it changes no observable behavior: the -existing RWO plan is `fabric` → `filesystem`, and the runner performs exactly the -calls `NodeStageVolume` performs today. Phase 2 is shippable because it moves code -that is already validated on a live cluster. Phase 3 is the first phase that fixes -something, and Phase 4 is the only phase that touches the CSI controller service. - -Phase 4 is planned rather than committed. It is in this document because the -pattern it replaces is already duplicated, and a design that leaves it out invites -the third copy. - ---- - -## Phase 0 — External Prerequisites - -| # | Prerequisite | Kind | Blocks | Status | -|------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------|------------|---------|------------------------------------------------------------| -| P0-1 | `lvm2` and `vdo` packages present in the CSI node image. `vdo` has no AArch64 build in the configured repositories, so LVM-backed plans are x86_64 only | Ecosystem | Phase 2 | On the PR #402 branch, not on `main` | -| P0-2 | `kmod-kvdo` built for the running host kernel, per node | Node OS | Phase 2 | Available on some hosts only, which is why §10 gates on it | -| P0-3 | `hostPID: true` on the CSI node DaemonSet, so the module load can `nsenter` the host namespace | Kubernetes | Phase 2 | On the PR #402 branch | -| P0-4 | No udev daemon runs inside the node container, so device-mapper's udev-sync handshake never completes and `DM_DISABLE_UDEV=1` is required for every LVM invocation | Ecosystem | Phase 2 | Known, handled on the PR #402 branch | - -Without P0-1 or P0-2 a node cannot run an LVM-backed plan at all. The consequence -is not a degraded volume but an unschedulable one: §10 keeps such volumes off such -nodes, and until Phase 4 lands the pod stays `Pending` with the failure visible on -the node plugin's log rather than on the PVC. P0-3 and P0-4 are environment facts -rather than decisions, and a node image that lacks either produces a layer whose -`Ensure` fails on its first command. - ---- - -## Table of Contents - -1. [Background](#1-background) -2. [Goals and Non-Goals](#2-goals-and-non-goals) -3. [Architecture Overview](#3-architecture-overview) -4. [The Layer Contract](#4-the-layer-contract) -5. [Layer Catalog](#5-layer-catalog) -6. [The Stack Record](#6-the-stack-record) -7. [Bring-Up and Bring-Down](#7-bring-up-and-bring-down) -8. [Co-Tenant Namespaces](#8-co-tenant-namespaces) -9. [Heal and Grow](#9-heal-and-grow) -10. [Node Requirements and Capability Gating (Phase 4)](#10-node-requirements-and-capability-gating-phase-4) -11. [Package Layout](#11-package-layout) -12. [Concurrency and Locking](#12-concurrency-and-locking) -13. [Failure Modes and Fallback](#13-failure-modes-and-fallback) -14. [Observability](#14-observability) -15. [Testing Strategy](#15-testing-strategy) -16. [Migration Strategy](#16-migration-strategy) -17. [Open Questions](#17-open-questions) -- [Appendix A: `blockdev.Device`](#appendix-a-blockdevdevice) -- [Appendix B: `LockScope`](#appendix-b-lockscope) - ---- - -## Overview - -A persistent volume on a node is a stack of objects, each one built on the one -below it. The simplest stack is two objects deep: an NVMe-oF namespace, and a -filesystem mounted on it. Client-side deduplication adds an LVM physical volume -and a VDO logical volume between them. A striped pNFS export adds several -namespaces at the bottom, a striped logical volume above them, and an NFS export -at the top, and the client that mounts that export builds a different stack out of -the same pieces. - -The node service does not model this. It has one extension point, the -`util.SpdkCsiInitiator` interface, which answers a single question ("connect this -and return a device path") and has exactly one implementation. Everything above -that seam is written inline, so each new capability is added as a conditional in -every RPC that touches the data path. Client-side VDO cost five such call sites -for one optional object, and the pNFS work adds four more objects on two different -stack shapes. - -This design replaces the inline conditionals with a `Layer`: one reversible -transform, with four verbs, that can be composed into a **plan**. A plan is an -ordered list of layers, derived from the volume's parameters at stage time and -recorded on the host, and a **runner** brings it up bottom to top and takes it -down top to bottom. Raw block mode is not a branch in this model, it is a shorter -plan. A striped export and a VDO volume are the same LVM layer with a different -logical-volume type. - -The four verbs are the load-bearing part, and they come from a defect that PR #402 -found on a live cluster. `Release` gives up the host's hold on an object and keeps -its data, `Destroy` removes the object. Conflating them made an ordinary pod -delete and recreate run `vgremove` and silently destroy the volume's data. Once -the two are separate, `NodeUnstageVolume` calls only `Release`, a failed bring-up -unwinds with only `Release`, and `Destroy` has a different caller entirely. - ---- - -## 1. Background - -`NodeStageVolume` in `csi-driver/pkg/spdk/nodeserver.go` performs a fixed -sequence: parse the volume handle, refresh the connection from the control plane, -build a `util.SpdkCsiInitiator` from the volume context, call `Connect` for a -device path, then call `stageVolume`, which is a single -`FormatAndMountSensitiveWithFormatOptions` followed by an `xfs` and `ext4` -if-chain. `NodeUnstageVolume` reads the volume context back from the staging -parent directory, unmounts, and calls `Disconnect`. - -`util.SpdkCsiInitiator` is the only seam, and it is a polymorphism seam rather -than a composition seam: `Connect(ctx) (string, error)` and `Disconnect(ctx) -error`, with `initiatorNVMf` as the sole implementation. It can answer "which -kind of thing is at the bottom of the stack" and nothing about what sits above. - -Four capabilities are arriving at once, and none of them fits that seam: - -- **Client-side compression and deduplication** (issue #277, PR #402) inserts an - LVM physical volume and a VDO logical volume between the namespace and the - filesystem. -- **Single-volume pNFS** ([`design-pnfs-rwx.md`](design-pnfs-rwx.md) §8, §10) - needs two different stacks over one namespace: the MDS host formats, mounts, and - exports it, and the client publishes an `eui64` alias and mounts the export over - NFS. -- **Striped pNFS** ([`design-pnfs-striped.md`](design-pnfs-striped.md) §2.1, §2.2) - puts *n* namespaces at the bottom and a striped logical volume above them, again - in two shapes, with the client activating the same volume group read-only. -- **Raw, ext4, and XFS** already differ in whether the filesystem object exists at - all and in what options create it. - -PR #402 is the measurement of what the missing seam costs. Its VDO mechanics are -sound and validated end to end on a live cluster, including nine defects only -findable on real hardware. Its wiring repeats - -```go -mountDevicePath := devicePath -if compression, deduplication, wantsVDO := vdoParams(vc); wantsVDO { - lvolID := volumeID - if spdkVol, perr := parseVolumeID(volumeID); perr == nil { lvolID = spdkVol.lvolID } - // ... -} -``` - -in `NodeStageVolume`, `NodeUnstageVolume`, `NodeExpandVolume`, and -`restageVolume`, plus a fifth negative gate inside `stageVolume` that suppresses -the `xfs` stripe hints. That is one optional object costing five call sites, -before pNFS adds four more objects across two stack shapes. - -Three of the defects PR #402 fixed are contract questions rather than -implementation slips, and §4 answers each of them once instead of per layer: - -- `NodeUnstageVolume` called the destructive `RemoveVDO` (`vgremove`) on every - unstage, including a routine pod delete and recreate on the same node, which - silently destroyed the data. The fix introduced a non-destructive - `DeactivateVDO`. -- `vgchange -an` fails on every retry when the backing NVMe-oF device is already - gone, leaving a permanently orphaned device-mapper stack. The fix added a - `dmsetup remove` force path, which then matched nothing until it accounted for - device-mapper's dash escaping. -- A byte-level clone carries its source's LVM metadata, so two volumes on one host - claim the same volume group. The fix, `ResolveClonedVDO`, runs `vgimportclone` - and `lvrename` before touching the device. - ---- - -## 2. Goals and Non-Goals - -### Goals - -- A single interface that composes the node-side objects of a persistent volume - into an ordered plan, so a new object is one implementation rather than a - conditional in every data-path RPC. -- Bring-up and bring-down that are convergent, not transactional: every verb is - safe to re-enter, because `NodeStageVolume` is retried, a heal re-runs a live - stack, and a teardown may resume after a crash. -- Idempotence at the layer and not only convergence at the runner: `Ensure`, - `Release`, and `Destroy` leave the host in the state one application leaves it - in when applied twice, and a verb whose object is already in its target state - succeeds rather than reporting an error. A `vgremove` against a volume group - that is already gone is a success, because the alternative wedges a delete path - on an object nobody can remove. This is the property `unwind` rests on rather - than a tension with it (§7.3): a failed bring-up releases best-effort and may - itself be interrupted, so correctness comes from the next attempt converging. -- A structural separation between releasing a host's hold on an object and - destroying the object, so the defect that made an ordinary pod restart destroy - data cannot be written again. -- Bring-down that works when the layer below is already gone, which is the normal - case after total path loss. -- Bring-down that never disconnects a subsystem another volume is using (§8). -- Heal, restage, and expand that decompose over the same plan, so they stop being - correct only for the bottom layer. -- A record on the host of what was built, written before the first side effect, so - a partially built stack is discoverable and removable after a crash. -- A stack that outlives the process that built it: after a csi-node pod restart - mid-stage, the plan and how far it got are recoverable from the record and - `Observe` alone. No layer keeps bring-up state only in process memory, and the - record's directory outlives the pod (§6). -- No behavior change for volumes that stage today, and no behavior change for the - VDO volumes PR #402 validated. -- Node capability and node pinning derived from the plan rather than hand-written - per feature (Phase 4). - -### Non-Goals - -- **The pNFS layers themselves.** `alias`, `nfsExport`, and `nfsMount`, the - read-only client activation, `fsid` allocation, and MDS selection belong to - [`design-pnfs-rwx.md`](design-pnfs-rwx.md) and - [`design-pnfs-striped.md`](design-pnfs-striped.md). This design defines the - contract they implement and names them in §5 only to show that the contract - fits. -- **A user-authored plan.** A plan is derived from a small set of named volume - kinds and their options. A StorageClass parameter carrying a list of steps - would be an API forever and an unbounded test surface. -- **Parallel layer execution.** §7 brings layers up one at a time. On this data - path the objects that look independent usually are not, and the ordering is the - correctness property. -- **Changing what VDO does.** Phase 2 moves PR #402's mechanics behind the - contract. The commands it runs, the names it derives, and the force paths it - falls back to are preserved, because they are what the live validation covered. -- **Replacing the volume-context stash.** `util.StashVolumeContext` keeps its - current role. §6 adds a record of the plan beside it and does not merge the two. -- **Block-mode plus VDO.** PR #402 excludes it explicitly and this design does not - add it. The plan for it is representable (`fabric` → `lvmPV` → - `lvmVolume(vdo)`, with no `filesystem`), which makes it a scoping decision - rather than an untested combination, but it is still out of scope. - ---- - -## 3. Architecture Overview - -``` -┌────────────────────────────────────────────────────────────────────────┐ -│ csi-node (one per host) │ -│ │ -│ NodeStageVolume ────┐ │ -│ NodeUnstageVolume │ │ -│ NodeExpandVolume ├──▶ plan(VolumeContext, VolumeCapability, Role) │ -│ NodePublishVolume │ │ │ -│ restage / heal ─────┘ ▼ │ -│ ┌─────────────┐ │ -│ │ Runner │ Up / Down / Heal / Grow │ -│ └─────────────┘ │ -│ │ │ │ -│ reads and writes │ │ Observe / Ensure │ -│ ▼ │ Release / Destroy │ -│ /var/lib/simplyblock/ │ │ -│ stacks/.json ▼ │ -│ ┌─────────────┐ │ -│ │ Layers │ │ -│ └─────────────┘ │ -└────────────────────────────────────────────┬───────────────────────────┘ - │ - ┌──────────────────────────────┼──────────────────────────┐ - ▼ ▼ ▼ - NVMe-oF fabric LVM / device-mapper mkfs and mount - (atlas-lib nvmeof, nvme) (lvm2, kvdo, dmsetup) (k8s mount-utils) -``` - -**The CSI RPCs no longer know the shape of the stack.** They build a plan, hand it -to the runner, and act on the artifact the top layer produces. `stageVolume`'s -`xfs` and `ext4` if-chain becomes the `filesystem` layer, and `vdoParams(vc)` -disappears from every RPC because the plan already carries the answer. - -**The plan is built once per RPC and recorded once per volume.** Building it is a -pure function of the volume context and the volume capability, which is what makes -it unit-testable without a host. Recording it is what makes teardown possible when -the process that built it is gone (§6). - -The plans the contract has to express, bottom to top. The last four differ from -each other only by the node's role for the volume, which is why the role is an -input to plan construction and not a property of the volume: - -| Volume kind | Plan | -|-----------------------------------------------|----------------------------------------------------------------------------| -| Plain, raw block | `fabric` | -| Plain, ext4 or XFS | `fabric` → `filesystem` | -| Client-side dedup or compression, ext4 or XFS | `fabric` → `lvmPV` → `lvmVolume(vdo)` → `filesystem` | -| pNFS single, MDS host | `fabric` → `filesystem` → `nfsExport` | -| pNFS single, client node | `fabric` → `alias` → `nfsMount` | -| pNFS striped, MDS host | `members(n)` → `lvmPV` → `lvmVolume(striped)` → `filesystem` → `nfsExport` | -| pNFS striped, client node | `members(n)` → `alias` → `lvmVolume(activate, read-only)` → `nfsMount` | - -Two results are worth reading off that table. Raw block mode is the plain plan -with its top layer absent rather than a conditional inside a stage function. And -the VDO volume and the striped export use the same `lvmVolume` layer with a -different logical-volume type, which is why §5.4 treats striping as a parameter -and not as a second implementation. - ---- - -## 4. The Layer Contract - -### 4.1 `Layer` - -```go -// Layer is one reversible transform in a volume's node-side stack: it takes what -// the layer below exposes and exposes something for the layer above. Every method -// is safe to re-enter, because NodeStageVolume is retried, a heal re-runs a live -// stack, and a teardown may resume after a crash. -type Layer interface { - // Name identifies the layer in logs and in the stack record. It is stable - // across releases: a teardown after an upgrade replays a record an earlier - // version wrote. - Name() string - - // Observe reports what of this layer is present on the host without changing - // anything. Ensure, Release, and Destroy all dispatch on what it found rather - // than re-deriving the same facts. - Observe(ctx context.Context, below Artifact) (State, error) - - // Ensure converges the layer and returns what the layer above consumes. - Ensure(ctx context.Context, below Artifact) (Artifact, error) - - // Release drops this host's hold on the layer and keeps its data. It is the - // only verb NodeUnstageVolume calls, and it has to succeed when the layer - // below is already gone, which is the normal case after total path loss. - Release(ctx context.Context, below Artifact) error - - // Destroy removes the layer's durable object. Only a deletion path calls it, - // never an unstage. - Destroy(ctx context.Context, below Artifact) error -} -``` - -**`Release` and `Destroy` are separate because conflating them destroys data.** -`NodeUnstageVolume` fires whenever no pod on this node needs the volume mounted, -which includes an ordinary pod delete and recreate against the same PVC on the -same node. A teardown path that removes durable objects there removes them on a -pod restart. The existing `defer initiator.Disconnect()` in `NodeStageVolume` is -already a `Release` and is safe for that reason. The same reflex applied to a -volume group is the defect PR #402 fixed. - -Not every layer implements all four distinctly. `lvmPV` has nothing to release, -because a physical-volume signature is not something a host holds. `fabric` has -nothing to destroy, because the namespace belongs to the control plane. A verb -with nothing to do returns without error rather than returning "unsupported": the -runner calls all four uniformly, and a layer that has to be special-cased by the -runner is not a layer. - -### 4.2 `State` - -```go -// State is what Observe found. The distinctions matter because Ensure's response -// to each is different, and two of them are the difference between reactivating a -// volume and reformatting it. -type State int - -const ( - // StateAbsent means nothing of this layer exists. Ensure creates it, which is - // the only circumstance under which a layer may format anything. - StateAbsent State = iota - - // StatePartial means an interrupted Ensure left an incomplete object. An LVM - // volume group whose logical volume was never created reports zero logical - // volumes and activates successfully while producing no usable device, so - // "the group exists" is not the same question as "the layer is ready". - StatePartial - - // StateForeign means the object exists but carries another volume's identity. - // A byte-level clone copies its source's LVM metadata, so the clone's device - // claims the source's volume group until vgimportclone renames it. - StateForeign - - // StateInactive means the object is complete but not currently mapped on this - // host. It is what Release leaves behind and what a node reboot leaves behind, - // and Ensure reactivates rather than recreating. - StateInactive - - // StateReady means present, complete, and usable. - StateReady -) -``` - -**The `StateAbsent` and `StateInactive` distinction is the one that loses data -when it is wrong.** Every layer that can create a durable object must be able to -tell "this volume has never been set up here" from "this volume is set up and -merely not activated," because the first answer permits a `mkfs` or an `lvcreate` -and the second forbids it. PR #402's `CreateOrAttachVDO` encodes exactly this rule -in prose ("if the volume group already exists it is reactivated, never -recreated"), and lifting it into the type is what makes it checkable. - -### 4.3 `Artifact` and `Geometry` - -```go -// Artifact is what one layer hands to the layer above it. It carries what a -// higher layer can act on and nothing about how the layer below produced it. -type Artifact struct { - // Devices are the block devices this layer exposes, in a defined order. A - // fan-in layer exposes several; every other layer exposes one. - Devices []blockdev.Device - - // Path is the filesystem path this layer mounted, empty until a layer mounts - // one. - Path string - - // Geometry is the stripe layout of Devices, for a layer above that aligns to - // it. - Geometry Geometry -} - -// Geometry is a stripe layout: the per-stripe chunk size and the number of -// stripes data is spread across. The zero value means unknown, which is the -// correct answer for a device whose blocks are virtualized. -type Geometry struct { - ChunkBytes int64 - Stripes int -} -``` - -`Devices` carries a type rather than a path. `blockdev.Device`, its fields, and -why a path string is insufficient are [Appendix A](#appendix-a-blockdevdevice). - -**`Geometry` exists because a filesystem's format options depend on what is -underneath it.** PR #402 discovered this as a special case: once VDO is in play the -`xfs` stripe hints must be suppressed, because VDO virtualizes and relocates -blocks and the filesystem is no longer laid out over the erasure-coded backend -device those hints were computed for. Applying them there is misleading rather -than merely useless. Expressed as a conditional, that is a fifth call site. -Expressed as a value, the VDO layer reports `Geometry{}` and the `filesystem` -layer passes no `-d su=,sw=` because there is nothing to align to. - -The same field improves the striped case rather than merely unifying it. A striped -`lvmVolume` layer knows its own stripe count and chunk size, so the `filesystem` -layer above it receives real geometry instead of the `xfs_su` and `xfs_sw` -StorageClass parameters and their `16k`/`1` fallbacks. - -### 4.4 Optional interfaces - -A layer implements these when it has something to contribute. The runner type -asserts for each and skips the layers that do not. - -```go -// Healer is implemented by a layer whose object can go bad under a live stack and -// be repaired in place. Heal never recreates: the data already exists. -type Healer interface { - // Healthy is a read. It reports whether this layer is currently serving. - Healthy(ctx context.Context, own Artifact) (bool, error) - - // Heal repairs the layer against the layer below, which may itself have just - // been healed. - Heal(ctx context.Context, below, own Artifact) error -} - -// Grower is implemented by a layer that has to be enlarged when the volume behind -// it grows. Grow is convergent: a layer already at its target size succeeds -// without doing anything, because kubelet reissues NodeExpandVolume after it has -// already succeeded. -type Grower interface { - Grow(ctx context.Context, below Artifact) (Artifact, error) -} - -// NodeRequirements is implemented by a layer that constrains where the volume may -// be staged: it needs something from the node, or its durable state stays there. -// The volume carrying it can then be staged only on a node that can run the -// layer, and only on one node at a time (§10). -// -// Unlike Healer and Grower this is a declaration rather than an action, which is -// why it is a noun: the runner interrogates it instead of calling it. -type NodeRequirements interface { - // NodeCapability is the label a node must carry, or the zero value when any - // node will do. - NodeCapability() Capability - - // PinsToNode reports whether this layer's durable state lives on the host. - PinsToNode() bool -} -``` - -Optional rather than mandatory is deliberate. Three of the seven layers in §5 have -nothing to heal and four have nothing to grow, and a mandatory interface would -fill them with methods that return nil. The runner's assertion is also what keeps -`NodeExpandVolume` honest: a plan whose layers implement no `Grower` at all is a -plan that needs no node-side expansion, which is the correct answer for a pNFS -client. - -`Grower` being convergent answers the last loose end PR #402 left open, where a -redundant `NodeExpandVolume` after a successful one logs an alarming but harmless -error on kubelet's reconciliation retry. - ---- - -## 5. Layer Catalog - -| Layer | Ensure | Release | Destroy | Optional | -|---------------------|--------------------------------------------------------------------------------------|---------------------------------------------------------------------|---------------------------|------------------------------| -| `fabric` (§5.1) | Connect every endpoint in the control plane's priority order, wait for the namespace | Detach, disconnecting only when the subsystem cannot be shared (§8) | — | `Healer` | -| `members` (§5.2) | *n* × `fabric` in the recorded order | Reverse order | — | `Healer` | -| `lvmPV` (§5.3) | `pvcreate`, or re-identify when `StateForeign` | — | `pvremove` | — | -| `lvmVolume` (§5.4) | `vgcreate` and `lvcreate` of the configured type, or activate when `StateInactive` | `vgchange -an`, with a `dmsetup` force path | `lvremove` and `vgremove` | `Grower`, `NodeRequirements` | -| `filesystem` (§5.5) | `mkfs` when unformatted, then mount | Unmount | — | `Healer`, `Grower` | -| `alias` | Publish the `eui64` symlink | Remove the symlink | — | — | -| `nfsExport` | Write the export drop-in, `exportfs -ra` | `exportfs -u` | Remove the drop-in | `Grower` | -| `nfsMount` | `mount -t nfs -o v4.1` | Unmount | — | `Healer` | - -`alias`, `nfsExport`, and `nfsMount` are listed for completeness and are out of -scope here (§2). They are specified by -[`design-pnfs-rwx.md`](design-pnfs-rwx.md) §8 and §10 and -[`design-pnfs-striped.md`](design-pnfs-striped.md) §2. - -### 5.1 `fabric` - -Wraps the existing NVMe-oF connect. `Ensure` asks the control plane where the -volume lives, builds one target per endpoint, connects them in the control plane's -priority order, and waits for the namespace device, which is the flow -`atlas-lib`'s `nvmeof.ConnectPaths` and `nvmeof.WaitForDevice` implement and which -`csi-driver/pkg/util/initiator.go` implements today with `nvme-cli`. Which of the -two implements it is a separate migration and not this design's business. The -layer's contract is the same either way. - -`Observe` maps the device state onto §4.2: no namespace device is `StateAbsent`, -a device present but not accessible is `StatePartial`, and a device that -`nvme.Device.Accessible` reports as serving is `StateReady`. `StateForeign` and -`StateInactive` do not arise, because a namespace carries no host-local identity -and cannot be present-but-deactivated. - -`Ensure` reports the backend's stripe geometry in the `Artifact` when it is known, -which is what lets the `filesystem` layer above align to it in the plain plan. - -`Release` is a detach rather than a disconnect and is specified in §8. `Destroy` -does nothing: the namespace belongs to the control plane and is removed by -`DeleteVolume`. - -### 5.2 `members` - -A composite layer holding *n* `fabric` layers, for a plan whose bottom is several -namespaces rather than one. `Ensure` runs them in the recorded order and returns -one `Artifact` whose `Devices` are their `blockdev.Device` values in that same -order. `Release` -reverses. - -**Member order is contract, not convenience.** A stripe over the same members in a -different order is a different device, and -[`design-pnfs-striped.md`](design-pnfs-striped.md) §2.3 requires that the order be -recorded and replayed rather than re-derived from a set. `members` is where that -requirement is satisfied, and it is why the plan is recorded (§6) instead of -rebuilt from the current StorageClass. - -`members` is also the reason this design needs no dependency graph. Fan-in is the -only non-linear shape any of the plans in §3 has, and a composite layer expresses -it without making the ordering of anything else implicit. - -### 5.3 `lvmPV` (Phase 2) - -`Ensure` on `StateAbsent` runs `pvcreate` against the device below. `Observe` -reads the device's on-disk LVM signature to answer which volume group it currently -belongs to, and reports `StateForeign` when that is a volume group belonging to -another volume, which is what a byte-level clone produces. `Ensure` on -`StateForeign` re-identifies the device with `vgimportclone` and `lvrename` before -anything above it activates. - -`Release` does nothing, because a physical-volume signature is not a hold. That -asymmetry is why PVs are their own layer rather than part of §5.4: the two objects -have different lifetimes, and `vgremove` does not remove the physical volumes it -released. - -**The clone collision is not VDO-specific.** Any layer whose object is identified -by on-disk content has it, and -[`design-pnfs-striped.md`](design-pnfs-striped.md) §2.3 specifies deterministic -volume-group names without addressing what happens when a clone and its source are -staged on one host. `StateForeign` is that specification, in one place, for both. - -### 5.4 `lvmVolume` (Phase 2) - -One volume group holding one logical volume, whose type is a parameter: linear, -`vdo`, or striped. `Ensure` on `StateAbsent` runs `vgcreate` over the physical -volumes below and then `lvcreate` of the configured type. `Ensure` on -`StateInactive` runs `vgchange -ay` and creates nothing. `Ensure` on -`StatePartial`, the volume group whose logical volume was never created, completes -the `lvcreate`. - -`Release` runs `vgchange -an`, and falls back to removing the device-mapper nodes -directly when the backing device is gone and every LVM retry fails. That force -path has to escape the volume-group name the way device-mapper does, doubling -dashes, or it matches nothing. - -`Destroy` runs `lvremove` and `vgremove`. Its callers are volume deletion and, for -a pNFS export, `DeleteExport`. It is never reached from `NodeUnstageVolume`. - -`Grow` extends the logical volume to the new physical capacity of the group and -then matches the logical size, and succeeds without acting when the volume is -already at its target. - -Naming is derived from the logical volume's UUID and nothing host-specific, so a -plan replayed on another host arrives at the same names. The lvol UUID is already -globally unique, stable, and inside LVM's length and character limits, which makes -it a better identifier than a hash of namespace and PVC name and is the convention -PR #402 established. [`design-pnfs-striped.md`](design-pnfs-striped.md) §2.3 -should adopt it rather than deriving a separate one, because a failover that -recomputes a different name cannot find the export it is recovering. - -The striped and the VDO plans differ only in the logical-volume type and its -options. The `Artifact` a striped volume reports carries its real `Geometry`, and -a VDO volume reports the zero value (§4.3). - -### 5.5 `filesystem` - -`Ensure` formats the device below when `blkid` shows it is unformatted, then mounts -it at the staging path. Formatting and mounting stay one layer because -`mount-utils`' `SafeFormatAndMount` couples them deliberately and splitting them -loses its protection against formatting a device that another process is about to -mount. - -Format options come from the volume's parameters and from the `Artifact` below. -`xfs` receives the feature options unconditionally, because on-disk feature -compatibility across kernel versions has nothing to do with the backend's layout, -and receives stripe alignment only when the layer below reports a non-zero -`Geometry`. `ext4` receives the reserved-blocks adjustment when the parameter is -set, and nothing when it is unset, preserving today's behavior where an unset -parameter means `mkfs.ext4`'s own default rather than `tune2fs -m 0`. - -`Healthy` detects the dead mount that total path loss leaves behind, which is the -`stagingMountDead` check the node service performs today: an `ENOTCONN`, `ESTALE`, -or `EIO`-class error from the mount point, plus the additional probe `ext4` needs -because it does not shut down when its backing device is removed and therefore -looks healthy from cache. `Heal` remounts without reformatting. - -`Grow` resizes the filesystem, and is absent from the plan entirely for a raw -block volume, which is how `NodeExpandVolume`'s current block-device special case -disappears. - ---- - -## 6. The Stack Record - -Every volume with a plan has one file on the host: - -``` -/var/lib/simplyblock/stacks/.json -``` - -It holds the plan, which is the ordered list of layer names and the parameters -each was constructed with, plus a per-layer marker recording that `Ensure` was -attempted. The file is written **before the first `Ensure` runs** and removed -after the last `Release` succeeds. - -**Writing it first is what makes a partially built stack removable.** A crash -between a fabric connect and the recording of that connect leaves paths attached -that nothing will ever release, which is a failure mode this repository has -already paid for on a different code path. Ordering the write ahead of the side -effect is the same discipline the operator's reconcilers apply to control-plane -calls. - -**The directory has to outlive the container.** `/var/lib/simplyblock` is a host -path mounted into the csi-node pod rather than container-local storage, because a -plugin restart is an ordinary event and the record is the only thing that tells -the restarted process what the previous one built. A layer that caches its -bring-up progress in process memory defeats the same property, which is why -`Observe` is the only way any verb learns what is present. - -**The record holds parameters, not device paths.** A device path is not stable -across a reconnect, which is why LVM identifies its physical volumes by on-disk -metadata rather than by path. Layer parameters are derived from volume identity -and are stable, so a teardown re-derives the artifacts through `Observe` rather -than trusting a path an earlier process wrote down. - -Given that, the per-layer markers are a diagnostic and an optimization rather than -a correctness mechanism: `Release` on a layer whose `Observe` reports -`StateAbsent` is already a no-op, so a teardown that ignored the markers entirely -would reach the same end state. They earn their place by making "what was -attempted, in what order" answerable after the fact, and by letting a teardown -skip the layers that were never reached. - -**The plan is recorded rather than re-derived because the StorageClass is not a -record of the past.** A class can be edited or deleted after a volume is -provisioned, and teardown owes the truth about what was built. The same reasoning -covers member order (§5.2), which cannot be recovered from a set. - -**An absent record means the legacy plan.** A volume staged by a version of the -node service that predates this design has no file, and unstaging it uses -`fabric` → `filesystem`, which is exactly what that version built. No migration -step runs on the node and no volume needs to be restaged (§16). - -This is host-local state, which has a consequence and an alternative. The -consequence is that the record is unreachable from the operator, so an orphaned -stack is found by a host sweep rather than by a cluster-wide query. The -alternative is to keep the record on the operator and reach it over csi-link, -which makes it visible cluster-wide at the cost of an RPC on the unstage path and -a dependency on the operator being reachable during teardown. §17 Q3 carries the -decision. Phase 1 uses the host-local file, because unstage has to work when the -operator does not. - -### 6.1 File Format - -```go -// Record is the on-disk form of a stack, one file per volume under -// /var/lib/simplyblock/stacks/. -type Record struct { - // Version is the schema version of this file and not the release that wrote - // it. A reader that does not recognize it refuses the record rather than - // guessing (§13), because a teardown driven by a misread plan is worse than a - // teardown that stops and says why. - Version int `json:"version"` - - // VolumeHandle is an lvol.VolumeHandle, "clusterID:poolID:volumeID". It - // repeats the filename so that a record found on its own identifies itself. - VolumeHandle string `json:"volumeHandle"` - - // Plan is the ordered layer list, bottom first. The order is most of why the - // file exists: Up walks it forward and Down walks it back. - Plan []Entry `json:"plan"` -} - -// Entry is one layer as the plan named it. -type Entry struct { - // Layer is the value Layer.Name() returns, stable across releases (§4.1). - Layer string `json:"layer"` - - // Params is what the layer was constructed with, opaque to the runner: the - // layer that declared them is the only thing that parses them. A new layer - // therefore ships without this format changing. - Params json.RawMessage `json:"params,omitempty"` - - // Members is the ordered sub-plan of a fan-in layer (§5.2) and is empty for - // every other layer. It is a field of its own rather than part of Params - // because the runner walks it, and member order is a runner concern. - Members []Entry `json:"members,omitempty"` - - // Attempted records that Ensure was called on this layer. It is a diagnostic - // and an optimization, never a correctness mechanism. - Attempted bool `json:"attempted"` -} -``` - -A striped stack whose `filesystem` layer was never reached: - -```json -{ - "version": 1, - "volumeHandle": "11111111-1111-1111-1111-111111111111:22222222-2222-2222-2222-222222222222:33333333-3333-3333-3333-333333333333", - "plan": [ - { - "layer": "members", - "attempted": true, - "members": [ - {"layer": "fabric", "params": {"nqn": "nqn.2023-05.io.simplyblock:lvol:aaaa"}, "attempted": true}, - {"layer": "fabric", "params": {"nqn": "nqn.2023-05.io.simplyblock:lvol:bbbb"}, "attempted": true} - ] - }, - {"layer": "lvmPV", "attempted": true}, - {"layer": "lvmVolume", "params": {"type": "vdo", "stripes": 2, "chunkBytes": 65536}, "attempted": true}, - {"layer": "filesystem", "params": {"fsType": "xfs"}, "attempted": false} - ] -} -``` - -**The write is atomic, and that is what makes the ordering real.** Each write goes -to a temporary file in the same directory, is `fsync`ed, is renamed over the -target, and the directory is `fsync`ed after the rename. A torn file would be -worse than no file at all, because an absent record means the legacy plan and a -half-written one would be read as a plan nobody built. Every `Attempted` flip -rewrites the whole record the same way, which is one small local write per layer -and is affordable precisely because the record holds no device state. - -**Params name secrets rather than carrying them.** A DHCHAP key is a credential -and this file outlives the pod that wrote it, so `fabric` records where to read its -secret and re-reads it on the teardown path, exactly as `NodeStageVolume` did. A -record that embedded the value would put a credential in cleartext on every node -that ever staged the volume. The file is mode `0600` and its directory `0700` -regardless. - -**What the format deliberately omits** is device paths and the reason above, -geometry and sizes, which `Observe` re-derives and which a `Grow` would invalidate -anyway, and anything naming the node, because the file is already on the node and a -record recording its own origin invites treating a copy from elsewhere as -authoritative. - ---- - -## 7. Bring-Up and Bring-Down - -### 7.1 `Up` - -``` -record.write(plan) // before any side effect -below := Artifact{} -for i, layer := range plan { - record.mark(layer) // before this layer's side effect - state, err := layer.Observe(ctx, below) - if err != nil { unwind(plan[:i], below); return err } - above, err := layer.Ensure(ctx, below) // dispatches on state - if err != nil { unwind(plan[:i], below); return err } - below = above -} -return below // what the RPC acts on -``` - -### 7.2 `Down` - -``` -for i := len(plan) - 1; i >= 0; i-- { - layer := plan[i] - if !record.marked(layer) { continue } - state, err := layer.Observe(ctx, below(i)) - if err != nil { return err } - if state != StateAbsent { - if err := layer.Release(ctx, below(i)); err != nil { return err } - } - record.unmark(layer) -} -record.remove() -``` - -`below(i)` re-derives layer *i*'s input by observing the layers beneath it, rather -than reading a device path out of the record (§6). - -### 7.3 A failed bring-up releases and never destroys - -`unwind` walks the layers already brought up, top-down, and calls `Release` on -each. It never calls `Destroy`. - -**This is the rule the four verbs exist for.** A `mkfs` that fails must not -trigger a `vgremove`, because the volume group underneath it may hold data that a -misfiring format check failed to see. `Release` is safe in the same situation -because it gives up a hold and takes nothing away. The existing -`defer initiator.Disconnect()` is this rule already, applied to the one layer the -node service has today, and generalizing it correctly means generalizing it as -`Release`. - -A stack left partly up by a failed `Up` is not an error state that needs -resolving. `NodeStageVolume` is retried, every verb is convergent, and the next -attempt observes what is there and continues. The record survives the failure, so -even a process that never retries leaves a removable stack behind. - -### 7.4 `Down` tolerates a dead foundation - -Bring-down proceeds top-down through layers whose foundation may already be gone, -which is the normal case rather than an edge case: total path loss removes the -namespace while the device-mapper stack above it is still mapped, and the pod is -deleted afterward. Each layer owns its own force path for that situation, and -§5.4's `dmsetup` fallback is the worked example. A layer that has no force path and -whose command depends on the layer below is a layer that will strand a stack. - -**`Release` returning without error does not mean the object is gone.** §8 has a -`fabric` layer that legitimately leaves its device present, so `Down` asserts -nothing about the state a released layer is in, and removes the record either way. - -### 7.5 Which RPC calls what - -| RPC or path | Runner call | Notes | -|--------------------------------|-------------------------|--------------------------------------------------------------------------------------------------------------------------| -| `NodeStageVolume` | `Up` | Acts on the top artifact's `Path`, or `Devices[0].Path` for raw block | -| `NodeUnstageVolume` | `Down` | `Release` only. `Destroy` is never reached from here | -| `NodePublishVolume` | `Heal`, then bind-mount | kubelet skips `NodeStage` when the volume is still referenced on the node, so publish is where a heal has to happen (§9) | -| `NodeExpandVolume` | `Grow` | Bottom to top, skipping layers that implement no `Grower` | -| `restageVolume` | `Heal` | Never `Up`, because the data exists and nothing may be formatted | -| `DeleteVolume`, `DeleteExport` | `Down`, then `Destroy` | The only callers of `Destroy` | - ---- - -## 8. Co-Tenant Namespaces - -A simplyblock subsystem can hold several namespaces. The -`max_namespace_per_subsys` StorageClass parameter is what provisions one that way, -and it means two different volumes can arrive at one host behind a single NQN. - -**Disconnecting a subsystem tears down every namespace on it.** So the `fabric` -layer's `Release` is a detach, not a disconnect. `atlas-lib`'s -`nvmeof.DetachDevice` answers the question and reports `SharedSubsystem`, and when -it is set the layer releases nothing at the fabric level and leaves the paths up -for the co-tenants. - -**The gate is whether the subsystem *can* be shared, not whether it currently -is.** `nvme.Device.IsMultiNamespace` is that question. Enumerating the neighbors -describes only the moment they were counted: a namespace can join between the -check and the disconnect, and a correct "none right now" answer is still -destructive when it does. A subsystem provisioned to be shared is therefore never -disconnected on one volume's behalf, even while it happens to hold only that -volume. - -The node service does not use that gate today. `selectDisconnectTarget` in -`csi-driver/pkg/util/initiator.go` counts the namespace devices the by-id glob -currently matches and disconnects when the count reaches one, which is the -weaker, enumerate-the-neighbors answer. Moving the decision behind `fabric`'s -`Release` is what replaces it, and the existing behavior for a subsystem that -genuinely still holds co-tenants has to be preserved while the gate is -strengthened. - -Three consequences for the rest of this design: - -- **`Down` cannot assert that a released layer is absent.** A `fabric` layer over - a shared subsystem returns from `Release` with its device still present and - still serving another volume. §7.4 states this as a rule and it is where the - rule comes from. The stack record is removed regardless, because this volume's - stack is down even though the fabric it stood on is not. -- **Ordering matters more, not less.** A device-mapper stack holding the namespace - open is what makes even a legitimate disconnect fail. Every layer above - `fabric` must be released first, which is what top-down teardown already - guarantees, and which is why the force paths in §7.4 exist for the cases where - it was not. -- **`Destroy` must stay per-volume.** Removing one volume's LVM objects must not - touch a co-tenant's. Deriving every LVM name from the logical volume's UUID - (§5.4) is what makes that true by construction rather than by care. - -Reaping a subsystem whose controllers are all dead is a deliberate -`connector.Disconnect` and never a default, which matches `atlas-lib`'s existing -contract: `DetachDevice` returns the error rather than guessing when the question -needs a live controller to answer. - ---- - -## 9. Heal and Grow - -Heal and expand are where the current design's cost is highest, because both are -implemented for the bottom layer only. `healVolumeBeforePublish` reconnects the -namespace, `ensureDeviceConnected` checks for a device, and `restageVolume` -remounts, and none of them knows that an LVM or VDO object might sit between the -two. PR #402 patched `restageVolume` with a fourth copy of its conditional to -close exactly that gap. - -**`Heal` walks the plan bottom to top.** For each layer that implements `Healer`, -the runner asks `Healthy` and calls `Heal` when the answer is no, passing the -artifact of the layer below, which may itself have just been healed. A layer that -implements no `Healer` is skipped and its artifact is re-derived through `Observe`, -so a healed foundation propagates upward. - -Bottom to top is the only workable order. A remount over a namespace that has not -been reconnected fails, and a namespace reconnect underneath a filesystem that is -still holding a dead mount does not clear the dead mount. - -**`Grow` walks the plan bottom to top as well**, and for the same reason: a -logical volume cannot be extended past a physical volume that has not been -resized, and a filesystem cannot be grown past its logical volume. Every `Grow` is -convergent, so kubelet's reconciliation retry after a successful expansion is a -sequence of no-ops rather than a sequence of alarming errors. - -The three layers with something to heal are `fabric` (path reconnection and ANA -reconciliation, which the existing `MonitorConnection` and guardian machinery -already perform), `filesystem` (dead-mount detection and remount), and `nfsMount` -(`ESTALE` detection). The three with something to grow are `lvmVolume`, -`filesystem`, and `nfsExport`. Every other layer implements neither, which is the -argument for the interfaces being optional (§4.4). - ---- - -## 10. Node Requirements and Capability Gating (Phase 4) - -A layer whose durable state lives on the host makes two demands on scheduling. The -volume can be staged only on a node that can run the layer, and once it is staged -it can be staged nowhere else, because the state does not follow the pod. - -Both demands are already implemented twice on the CSI controller side. -`dhchapAllowedNodeSegment` merges a DHCHAP topology segment into -`CreateVolume`'s `AccessibleTopology` so `external-provisioner` pins -`PersistentVolume.spec.nodeAffinity`, and PR #402 adds `vdoCapableSegment` -mirroring it exactly. The StoragePool controller composes the matching -`TopologySelectorTerm`s in `createStorageClassIfNotExists`. pNFS brings a third -demand, with the opposite polarity: an MDS host must be eligible, and a pNFS -client must *not* be pinned, because RWX is the point. - -`NodeRequirements` (§4.4) makes the plan the single source of truth for both. The -controller service builds the plan for a `CreateVolume` request from the same pure -function the node service uses, asks each layer for its capability and its pinning -answer, and merges the results into `AccessibleTopology`. The StoragePool -controller derives its topology terms the same way. - -Two demands then need one implementation rather than one per feature, and a -capability that is missing at admission is reported once rather than discovered as -a mount failure on the wrong node. - -Capability advertisement itself follows PR #402: the node DaemonSet's `postStart` -hook installs and loads the kernel module through `nsenter`, writes a marker file, -and the node plugin reads the marker in the background and patches the node's -capability label. Generalizing it means a layer names its marker and its label -rather than each feature adding a pair. - -**The registration race is real and belongs to the generalization.** A CSINode's -topology key set is captured once at plugin registration, seconds after the pod -starts, while the label is patched asynchronously afterward. A -`buildAccessibleTopology` that reports a capability key only when the label is -already `true` therefore reports it essentially never, which permanently breaks -the topology gate until the pod restarts. PR #402 found this as its first defect. -The key must be present at registration regardless of the label's current value, -and the label carries the answer. - ---- - -## 11. Package Layout - -Every primitive in §5 is a node-level primitive: fabric connect, `pvcreate`, -`lvcreate`, VDO, `mkfs`, `mount`, `exportfs`, and the `eui64` alias. None of them -is Kubernetes-shaped, and the striped pNFS design needs two different compositions -of the same set, one on the MDS host and one on every client. They belong in -`atlas-lib`. - -| Package | Holds | -|------------------------------|-----------------------------------------------------------------------------------------------------| -| `atlas-lib/blockdev/` | `Device`: what a Linux block device is, independent of what produced it (Appendix A) | -| `atlas-lib/volstack/` | `Layer`, `State`, `Artifact`, `Geometry`, the optional interfaces, the runner, and the stack record | -| `atlas-lib/volstack/layers/` | The layer implementations | -| `atlas-lib/lvm/` | The LVM and device-mapper primitives PR #402 wrote as `csi-driver/pkg/util/vdo.go` | -| `csi-driver/pkg/spdk` | The plan: a pure function from `VolumeContext`, `VolumeCapability`, and `Role` to a layer list | - -Plan construction stays in the CSI driver because it is the one Kubernetes-shaped -part, and it is deliberately thin. The package name `volstack` is provisional -(§17 Q1). - -**The role is an input, and resolving it is not part of the pure function.** A -pNFS volume has two plans over one namespace (§1), and §3's last four rows differ -by nothing else, so a plan function of the volume context and the capability alone -cannot select between them. Answering "is this node the MDS for this volume" means -asking something, which is what would cost the pure function its testability, so -it is resolved first and passed in: an impure step that reads whatever the pNFS -designs make authoritative, then `plan(vc, cap, role)` deriving the layer list -from three values and no host. Which values decide the role, and whether the -answer can change over a volume's life, belong to -[`design-pnfs-rwx.md`](design-pnfs-rwx.md) and -[`design-pnfs-striped.md`](design-pnfs-striped.md), exactly as the layers -themselves do (§2). That the plan takes it as an input belongs here. - -**Phase 4 needs every role rather than the plan.** §10 has the controller service -building the plan for a `CreateVolume` request, and at that point no node has a -role yet. For a pNFS volume it therefore derives the requirements of both roles -and merges them, which is precisely where §10's note about the opposite polarity -lands: the MDS role contributes a capability and a pin, and the client role -contributes a capability and the absence of one. - -**PR #402's `vdo.go` moves rather than being rewritten.** `runLVMCommand`, -`devicesArgs`, `pvVGName`, `vgExists`, `vgHasLV`, and the orphaned-node removal -are exactly the primitives §5.4 needs, and the striped pNFS layer needs the same -ones, which is the second consumer that makes the move mandatory rather than -tidy. The move is mechanical and is Phase 2 work, sequenced after PR #402 merges -rather than imposed on it: its value is in behavior validated on live hardware, -and rebasing that validation onto a package boundary buys nothing. - ---- - -## 12. Concurrency and Locking - -`util.VolumeLocks` serializes the node RPCs per volume ID today, and the runner -inherits that: one volume's stack is brought up, brought down, healed, or grown by -one goroutine at a time. - -Per-volume locking is not sufficient for every layer. `lvmPV` and `lvmVolume` -invoke LVM commands that take LVM's own host-wide locks and scan every visible -device, and two volumes staging at the same moment on one host contend there -regardless of their volume IDs. The pNFS layers are worse: `/etc/exports` is one -file per host, and a striped export's volume group is active on the MDS host and -on every client at once. - -**This is an open question and not a solved one (§17 Q2).** What is known is that -it is unexercised rather than proven safe: PR #402's multi-instance validation -happened to run its two stage sequences sequentially rather than overlapping, so -genuinely concurrent `vgchange` and `pvscan` calls have not raced on a real host. -That is a risk in shipped code once PR #402 merges, not only a risk in this -design. - -A scope declared per layer is the candidate mechanism, specified in -[Appendix B](#appendix-b-lockscope). It is not part of the contract in §4, because -the granularity it carries depends on a measurement nobody has taken. Until §17 Q2 -is answered the runner locks per volume, as the node RPCs do today. - ---- - -## 13. Failure Modes and Fallback - -| Failure | Detection | Behavior | -|---------------------------------------------------|--------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| A layer's `Ensure` fails | Error returned | Layers below are `Release`d top-down and never `Destroy`ed (§7.3). The RPC fails, kubelet retries, and the next `Up` converges from where this one stopped | -| Backing device gone during `Release` | Every LVM retry fails identically | The layer's own force path runs, removing the device-mapper nodes directly with the volume-group name escaped as device-mapper escapes it (§5.4) | -| An interrupted `Ensure` left an incomplete object | `Observe` reports `StatePartial` | `Ensure` completes the object instead of treating it as ready. A volume group with no logical volume activates successfully and produces no usable device, so the state has to be distinguished | -| A clone carries its source's LVM metadata | `Observe` reports `StateForeign` | `Ensure` re-identifies the device before anything above it activates (§5.3) | -| Subsystem shared with another volume | `nvmeof.DetachDevice` reports `SharedSubsystem` | `fabric`'s `Release` leaves the paths up. The stack record is still removed (§8) | -| csi-node pod restarted mid-bring-up | A record exists, layers `Observe` short of ready | The restarted process re-reads the record and the next `Up` converges from there. Resuming needs no in-memory state (§6) | -| Node rebooted with a stack recorded | Every layer `Observe`s `StateInactive` | `Ensure` reactivates. Nothing is created and nothing is formatted (§4.2) | -| Stack record absent at unstage | No file for this volume handle | The legacy plan is assumed: `fabric` → `filesystem` (§6) | -| Stack record's version is not recognized | `Version` is a schema this release does not read | The unstage fails and reports the version, on the same reasoning as an unknown layer name: a plan read under the wrong schema releases the wrong objects (§6.1) | -| Stack record present but unparsable | The file does not decode | The unstage fails and reports it rather than falling back to the legacy plan. That fallback answers an absent record, and a corrupt file is not evidence that a legacy stack was built (§6.1) | -| Stack record present, plan unrecognized | A layer name the running version does not know | The unstage fails and reports the unknown layer, rather than silently skipping an object nobody will release. Layer names are stable across releases for this reason (§4.1) | -| Node lacks a layer's capability | The capability label is absent | Phase 4 keeps the volume off the node. Until then `Ensure` fails on its first command and the pod stays `Pending` (§10) | -| Total path loss under a live stack | `filesystem`'s `Healthy` reports the dead mount | `NodePublishVolume` heals bottom-to-top before bind-mounting, so the pod does not inherit a dead mount (§9) | - ---- - -## 14. Observability - -The CSI node plugin emits no Kubernetes events and exposes no Prometheus metrics -today. Its entire observability surface is `klog`, which is why a stack that -stranded itself on a host is currently found by reading logs. Both tables below -are therefore new infrastructure in this design rather than additions to an -existing registry, and both are Phase 1 work: a layered bring-up that cannot be -observed per layer is harder to debug than the inline version it replaces, not -easier. - -### Kubernetes Events - -Events need a target object. The node plugin holds a `kubernetes.Interface` and a -shared `sbkube.Manager` that resolves the PV and the PVC for a volume handle -already, so the PVC is the target: it is the object a user owns and looks at, and -it outlives the pod. - -| Event | Type | Reason | -|------------------------------------------------------------------|---------|--------------------------------| -| A layer's `Ensure` failed and the stack was released back | Warning | `VolumeStackEnsureFailed` | -| A clone's device was re-identified before activation | Normal | `VolumeStackIdentityResolved` | -| An interrupted object was completed rather than treated as ready | Normal | `VolumeStackPartialCompleted` | -| A shared subsystem was left connected for its co-tenants | Normal | `VolumeStackSubsystemShared` | -| A `Release` fell back to its force path | Warning | `VolumeStackForceReleased` | -| The node lacks a capability the plan requires | Warning | `VolumeStackCapabilityMissing` | -| A layer was healed under a live stack | Normal | `VolumeStackLayerHealed` | - -### Prometheus Metrics - -| Metric | Labels | Description | -|-----------------------------------------------------|------------------|-----------------------------------------------------------------------------------------------------| -| `simplyblock_csi_node_stack_layer_duration_seconds` | `layer`, `verb` | Histogram of `Observe`, `Ensure`, `Release`, `Destroy`, `Heal`, and `Grow` durations per layer kind | -| `simplyblock_csi_node_stack_layer_errors_total` | `layer`, `verb` | Failed layer operations by kind | -| `simplyblock_csi_node_stack_force_release_total` | `layer` | Releases that fell back to a force path, which is the signal that stacks are being stranded | -| `simplyblock_csi_node_stack_observed_state_total` | `layer`, `state` | `Observe` outcomes, so `StateForeign` and `StatePartial` are countable rather than anecdotal | -| `simplyblock_csi_node_stacks` | `plan` | Stack records present on this host, by plan shape | -| `simplyblock_csi_node_stack_records_orphaned` | — | Records whose volume no longer has a pod or a staging path on this node | - -`simplyblock_csi_node_stacks` and `simplyblock_csi_node_stack_records_orphaned` -are what make the host-local record (§6) usable operationally: an orphan count -that is not zero is the alert, and the record names what to remove. - ---- - -## 15. Testing Strategy - -Full scenario matrix, coverage status, and hand-off test concepts: -[`tests/test-plan-node-volume-stack.md`](../tests/test-plan-node-volume-stack.md) - -- **Unit:** plan construction is a pure function and every plan in §3 must be - derived from its volume context, capability, and role without a host. The four - pNFS rows are the cases that matter, because they differ by role alone and a - plan function that ignored it would return the MDS plan on a client. The runner's - ordering, its unwind rule, and its refusal to `Destroy` on a failed `Up` are - provable against fake layers that record their calls, which is where the - highest-value coverage sits: a fake layer set makes "a failed `Ensure` at index - 2 releases index 1 and index 0, top-down, and destroys nothing" a table test. - `State` classification per layer is testable against a faked host surface. -- **Integration:** the record's write-ahead ordering and its survival across a - simulated crash, against a temporary directory rather than a cluster. The - format (§6.1) adds a round-trip over every plan in §3, a refusal on an - unrecognized version, and a refusal on a truncated file, which is the case a - crash mid-rename must not be able to produce. There is - no `envtest` component to this design in Phases 1 through 3, because nothing - reconciles. Phase 4 adds the controller-side plan derivation and with it the - first integration surface. -- **E2E:** every plan in §3 that is in scope, staged and unstaged on a live - cluster with data written and checksummed across the cycle. The claims that - only a live cluster can settle are the ones PR #402 had to settle by hand: a - pod delete and recreate reattaches rather than reformats, a node reboot - reattaches every stack on the host, a clone and its source coexist on one node, - and an unclean disconnect leaves no orphaned device-mapper stack. The e2e suite - is Ginkgo (`csi-driver/e2e`), and these are new `SPDKCSI-` blocks. -- **Load and long-running:** genuinely concurrent staging of several - LVM-backed volumes on one host, which is the specific gap §12 names and which - PR #402's validation did not reach. It decides §17 Q2: overlapping `pvscan` and - `vgchange` either survive, and `lvmPV` and `lvmVolume` keep separate keys, or - they do not, and both layers return the one key that serializes all LVM work. - -Risk concentrates in §4.2 and §7.3. A `State` misclassification formats a volume -that had data, and an unwind that calls `Destroy` removes one. Those scenarios -must not be the ones cut when the schedule slips. - ---- - -## 16. Migration Strategy - -**Phase 1 changes no observable behavior.** The plan for every volume that stages -today is `fabric` → `filesystem`, and the runner performs the same connect, -format, and mount calls in the same order. The migration is that -`NodeStageVolume` stops performing them directly. - -**Volumes staged before Phase 1 need no action.** They have no stack record, and -§6 defines an absent record as the legacy plan, which is the plan those volumes -were built with. Nothing is restaged, no node is drained, and a rolling upgrade of -the node DaemonSet is sufficient. - -**Phase 2 moves PR #402's code rather than rewriting it.** The commands, the -derived names, the force paths, and the clone resolution move behind the contract -unchanged, because their value is validation on live hardware that a rewrite would -discard (§11). - -**No VDO volume needs a legacy plan.** PR #402 and Phase 2 land in the same -release, so no released version ever stages a VDO volume without recording its -plan. The legacy plan of §6 is therefore always `fabric` → `filesystem`, and -nothing infers a plan from `client_compression` or `client_deduplication`. A -cluster tracking `main` between the two merges is the only way to reach a VDO -stack with no record, and such a volume is restaged rather than inferred. - -**Phase 3 removes the special cases it replaces.** `healVolumeBeforePublish`, -`ensureDeviceConnected`, `restageVolume`, and `NodeExpandVolume`'s block-device -branch become runner calls, and the VDO conditionals PR #402 added to -`restageVolume` and `NodeExpandVolume` are deleted rather than ported. - -**Phase 4 is additive on the controller side.** `dhchapAllowedNodeSegment` and -`vdoCapableSegment` are replaced by plan-derived segments that produce the same -topology keys and the same values, so existing `PersistentVolume.spec.nodeAffinity` -stays valid and no PV is rewritten. - ---- - -## 17. Open Questions - -| # | Question | Owner | -|-----|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------------| -| 1 | **Package name.** `atlas-lib/volstack/` is provisional. The existing package names in `atlas-lib` are short concrete nouns (`nvme`, `nvmeof`, `lvol`, `nqn`), and this one is neither short nor a noun anybody uses out loud | — | -| 2 | **Whether the LVM layers share one lock key or hold two.** [Appendix B](#appendix-b-lockscope) proposes the mechanism. What decides the granularity is empirical: concurrent `pvscan` and `vgchange` on one host is unexercised rather than proven safe, and the answer is a load-test result (§15). Until it is taken, the runner locks per volume | — | -| 3 | **Where the stack record lives.** Host-local (§6) works when the operator is unreachable and needs no RPC on the unstage path. Operator-side over csi-link is visible cluster-wide and finds orphans without a host sweep. Phase 1 takes the host-local file, and whether the operator-side record is an addition or a replacement is open | — | -| 4 | **Whether `Destroy` is reachable from a node RPC at all.** For an LVM stack the metadata dies with the logical volume the control plane deletes, so `Destroy` would only ever remove node-local remnants. For a pNFS export it does not, and `DeleteExport` genuinely destroys. If the answer is "only a deletion path," the node RPCs get a narrower contract than §4.1 gives them | — | -| 5 | **`nfsExport` and `nfsMount` as layers.** §5 asserts they fit the contract. That is a claim this design cannot verify, because it does not build them. The pNFS designs are where it is settled, and a verb they cannot express is a finding against §4.1 | pNFS designs | -| 6 | **Whether Phase 4 ships.** It is planned, not committed. The cost of leaving it out is a third hand-written copy of the topology pattern when pNFS lands | — | - ---- - -## Appendix A: `blockdev.Device` - -The type `Artifact.Devices` holds (§4.3). It lives in `atlas-lib/blockdev` rather -than in `volstack`, because what it describes is a property of the host and not of -this contract. - -```go -// Device is one Linux block device as the kernel presents it, independent of -// what produced it: an NVMe namespace, a device-mapper node, or a disk handed to -// a storage cluster at deployment. -type Device struct { - Path string // the canonical /dev path - Name string // the kernel name: "nvme0n1", "dm-3" - Major, Minor uint32 - LogicalBlockSize uint32 - PhysicalBlockSize uint32 - SizeBytes uint64 - ReadOnly bool -} -``` - -**A path is not an identity, and it is not sufficient either.** `/dev/dm-3` and -`/dev/mapper/vg--name-lv` are one object under two strings whose escaping rules -§5.4 already has to reason about, and §6 says outright that a path does not -survive a reconnect. Carrying the major and minor numbers beside the path puts the -stable identifier where a layer above can compare on it. - -**The block sizes are the `Geometry` argument again.** A `mkfs` aligns to the -logical block size as well as to the stripe layout, and `Geometry` does not carry -it: a virtualized device reports `Geometry{}` and still has a block size. Derived -at the call site, that is a fifth inspection of a path, which is the shape §4.3 -exists to remove. Reported as a field, the `filesystem` layer reads it. - -**`Device` is the intersection and not a union.** No NVMe field, no LVM field, and -no discriminator saying which produced it. A layer needing NVMe specifics resolves -`nvme.Device` from the path, and a `Device` that grew an `isNVMe` field would be -the conditional this contract replaces. - -**It is a split rather than a new type.** `atlas-lib/nvme`'s `Namespace` already -carries the name, the device path, the major and minor numbers, the logical block -size, the capacity, and the read-only flag, resolved from sysfs and exercised -there. Those are facts about a block device rather than facts about NVMe. -`blockdev.Device` is that half named on its own, and `Namespace` keeps the -NVMe-specific remainder. The split is Phase 1 work because §4.3 depends on the -type, and it is independent of PR #402: it touches `nvme` and nothing PR #402 -wrote. - -**A second consumer puts it in `atlas-lib`.** Handing logical block devices to a -storage cluster at deployment needs the same value on the operator side, where -nothing represents a device today. A `Device` -defined inside `volstack` would be written a second time within one release. - -**Resolution is deliberately absent.** The type is a value in Phase 1, following -the immutable-snapshot convention `atlas-lib/nvme` already holds to: a snapshot is -re-resolved rather than refreshed in place. A resolver reading these fields from -sysfs can be added beside it when a consumer needs one, and adding it changes -nothing a layer holds. - ---- - -## Appendix B: `LockScope` - -The candidate mechanism for §12's lock scopes. It is an appendix and not part of -§4 because §17 Q2 is open: the granularity the mechanism would carry is a -measurement, and the contract does not depend on the answer. - -```go -// LockScope is implemented by a layer whose commands are not safe to run at the -// same time as the same commands for another volume. It names the lock the runner -// holds around every verb it dispatches to the layer. -type LockScope interface { - // LockKey is the lock this layer needs held. A layer constructed for one - // volume returns that volume's handle, which is per-volume scope. A constant - // is host-wide scope, shared by every layer that returns it. - LockKey() string -} -``` - -A layer that does not implement it is serialized per volume. `fabric` returns its -volume handle. `lvmPV` and `lvmVolume` return a key naming the LVM work, so two -volumes staging at once serialize through `pvscan` and `vgchange` while their -fabric connects still run concurrently. `nfsExport` returns a key naming -`/etc/exports`, which is one file per host. - -**The scope is a key and not a named scope.** Whether `lvmPV` and `lvmVolume` -return one key or two follows from whether overlapping `pvscan` and `vgchange` are -safe. As a key that answer changes a returned value; as a choice between "per -volume" and "host-wide" it would change the contract. - -The keys are acquired against a registry in `atlas-lib/locks`, generalized from -`csi-driver/pkg/util`'s `VolumeLocks`: the same map of mutexes, keyed by a string -instead of by a volume ID, with `locks.WithLock` scoping each acquisition to one -call so that a verb returning early or panicking cannot leave a key held. -`VolumeLocks` becomes a caller of it rather than a second implementation. - -A verb runs entirely inside its own acquisition, so work owed to the lock happens -in the verb and cleanup before release is a `defer` there. `sync.Mutex` is not -reentrant, which makes a key held by a composite layer unavailable to its members -(§5.2): a member needing the LVM key cannot be run by a parent already holding it. - -Adopting the mechanism adds one metric to §14, -`simplyblock_csi_node_stack_lock_wait_seconds`, labeled by `layer` and by scope -kind. It is never labeled by the key, because a volume handle is unbounded label -cardinality. diff --git a/operator/docs/tests/test-plan-client-side-vdo-compression.md b/operator/docs/tests/test-plan-client-side-vdo-compression.md new file mode 100644 index 000000000..90f1b7bd5 --- /dev/null +++ b/operator/docs/tests/test-plan-client-side-vdo-compression.md @@ -0,0 +1,232 @@ +# Test Plan: Client-Side Compression and Deduplication via VDO + +**Related design:** [`../designs/design-client-side-vdo-compression.md`](../designs/design-client-side-vdo-compression.md) + +**Legend:** `U-` unit (Go, fake command runner, no `lvm2`/kernel), `M-` manual +(live cluster). Type: `Positive` / `Negative` / `Boundary` / `Regression`. + +--- + +## 1. `atlas-lib/lvm` Typed Primitives + +The general-purpose LVM layer `atlas-lib/lvm/vdo` is built on (design §3). Not +VDO-specific. Covers `PhysicalVolume`/`VolumeGroup`/`LogicalVolume` construction, +device scoping, and volume-group/logical-volume identity. + +### Volume Group and Logical Volume Assembly (`atlas-lib/lvm/volume_test.go`) + +| # | Scenario | Type | Test | +|------|-----------------------------------------------------------------------------------------------------------------------|----------|----------------------------------------------------------------------| +| U-1 | Create a physical volume, scoped to its device | Positive | `TestManager_CreatePhysicalVolume` | +| U-2 | Physical volume creation propagates a runner error | Negative | `TestManager_CreatePhysicalVolume_WrapsRunnerError` | +| U-3 | Create a volume group on a single device | Positive | `TestManager_CreateVolumeGroup` (single device) | +| U-4 | Create a volume group across several devices (striped-VG shape) | Positive | `TestManager_CreateVolumeGroup` (multiple devices) | +| U-5 | Activate a volume group | Positive | `TestManager_ActivateVolumeGroup` | +| U-6 | Deactivate a volume group | Positive | `TestManager_DeactivateVolumeGroup` | +| U-7 | Deactivation propagates a runner error | Negative | `TestManager_DeactivateVolumeGroup_WrapsRunnerError` | +| U-8 | Remove a volume group | Positive | `TestManager_RemoveVolumeGroup` | +| U-9 | Removal propagates a runner error | Negative | `TestManager_RemoveVolumeGroup_WrapsRunnerError` | +| U-10 | `CreateLogicalVolume` dispatches to a registered `VolumeProvisioning` handler by `Handles(def)`, not a hardcoded name | Positive | `TestManager_CreateLogicalVolume_DispatchesByHandles` | +| U-11 | No registered handler matches the definition: no extra flags contributed | Negative | `TestManager_CreateLogicalVolume_NoHandlerMatchesContributesNothing` | + +### Growing a Stack (`atlas-lib/lvm/grow_test.go`) + +| # | Scenario | Type | Test | +|------|----------------------------------------------------------------------------|----------|----------------------------------------------------------| +| U-12 | Expand a physical volume to its device's current full size | Positive | `TestManager_ExpandPhysicalVolume` | +| U-13 | Extend a volume group with an additional device | Positive | `TestManager_ExtendVolumeGroup` | +| U-14 | Volume group extension propagates a runner error | Negative | `TestManager_ExtendVolumeGroup_WrapsRunnerError` | +| U-15 | Expand a logical volume by the additive `-l+100%FREE` form (design §3, §7) | Positive | `TestManager_ExpandLogicalVolume` | +| U-16 | Read a logical volume's current size in bytes | Positive | `TestManager_LogicalVolumeSize` | +| U-17 | Unparsable `lvs` size output is an error, not a silent zero | Negative | `TestManager_LogicalVolumeSize_UnparsableOutput` | +| U-18 | Extend a logical volume to an absolute byte size | Positive | `TestManager_ExtendLogicalVolumeToSize` | +| U-19 | Absolute-size extension propagates a runner error | Negative | `TestManager_ExtendLogicalVolumeToSize_WrapsRunnerError` | + +### Clone and Snapshot-Restore Identity (`atlas-lib/lvm/clone_test.go`) + +| # | Scenario | Type | Test | +|------|----------------------------------------------------------------------------------------|----------|-----------------------------------------------------------------| +| U-20 | Import a cloned volume group's identity (`vgimportclone`) | Positive | `TestManager_ImportClonedVolumeGroup` | +| U-21 | Import propagates a runner error | Negative | `TestManager_ImportClonedVolumeGroup_WrapsRunnerError` | +| U-22 | Rename a logical volume after import | Positive | `TestManager_RenameLogicalVolume` | +| U-23 | Full resolution sequence: refresh, probe, import, rename, in order | Positive | `TestManager_ResolveClonedVolumeGroup_ResolvesAForeignIdentity` | +| U-24 | A device already carrying this volume's own identity, or a blank device, is left alone | Negative | `TestManager_ResolveClonedVolumeGroup_NoOps` | +| U-25 | The structural (pool) logical volume is never renamed | Boundary | `TestManager_ResolveClonedVolumeGroup_PreservesStructuralLVs` | +| U-26 | A failed cache refresh is non-fatal, and the content probe recovers | Negative | `TestManager_ResolveClonedVolumeGroup_SurvivesAFailedRescan` | +| U-27 | A genuine probe failure is returned, not swallowed | Negative | `TestManager_ResolveClonedVolumeGroup_WrapsAProbeFailure` | + +### Identity Probes and Rescan (`atlas-lib/lvm/identity_test.go`) + +| # | Scenario | Type | Test | +|------|-------------------------------------------------------------------------|----------|--------------------------------------------------------| +| U-28 | Resolve a device's volume group by content, not by name | Positive | `TestManager_VolumeGroup` | +| U-29 | A real probe failure is an error, not folded into "blank" | Negative | `TestManager_VolumeGroup_PropagatesRealProbeError` | +| U-30 | List every logical volume in a volume group | Positive | `TestManager_ListLogicalVolumes` | +| U-31 | Listing propagates a runner error | Negative | `TestManager_ListLogicalVolumes_PropagatesRunnerError` | +| U-32 | Detect whether a specific logical volume exists (orphaned-VG detection) | Positive | `TestManager_HasLogicalVolume` | +| U-33 | Detection propagates a runner error | Negative | `TestManager_HasLogicalVolume_PropagatesRunnerError` | +| U-34 | Rescan is scoped to exactly the given devices | Positive | `TestManager_Rescan` | +| U-35 | Rescan propagates a runner error | Negative | `TestManager_Rescan_PropagatesRunnerError` | + +### Orphaned Device-Mapper Node Cleanup (`atlas-lib/lvm/dm_test.go`) + +| # | Scenario | Type | Test | +|------|-----------------------------------------------------------------------|----------|--------------------------------------------------------------------------------------------------| +| U-36 | Device-mapper's dash-escaping is applied before matching (design §7) | Positive | `TestEscapeDMName` | +| U-37 | No matching orphaned nodes: a no-op | Negative | `TestManager_RemoveOrphanedDMNodes` (no matching nodes) | +| U-38 | Matching nodes removed, unrelated nodes left alone | Positive | `TestManager_RemoveOrphanedDMNodes` (matches escaped names and removes them) | +| U-39 | A dependent node stuck on one pass clears once its dependency is gone | Boundary | `TestManager_RemoveOrphanedDMNodes` (a node stuck on pass one clears once its dependent is gone) | +| U-40 | `dmsetup ls` itself failing is a real error | Negative | `TestManager_RemoveOrphanedDMNodes` (dmsetup ls itself fails) | + +--- + +## 2. `atlas-lib/lvm/vdo`: the VDO Provisioning Handler + +### VolumeProvisioning Registration (design §3) (`atlas-lib/lvm/vdo/volume_test.go`) + +| # | Scenario | Type | Test | +|------|---------------------------------------------------------------------------------------------------------|----------|---------------------------------------------------------------------| +| U-41 | `CreateVolumeArgs` contributes `--type vdo` flags for compression, deduplication, or both | Positive | `TestVolumeHandler_CreateVolumeArgs` | +| U-42 | Neither flag set: no flags contributed | Negative | `TestVolumeHandler_CreateVolumeArgs` (neither, contributes nothing) | +| U-43 | `Handles` agrees with `CreateVolumeArgs` on which definitions this handler owns (either flag, not both) | Boundary | `TestVolumeHandler_Handles` | +| U-44 | Importing this package registers the handler, and `CreateLogicalVolume` actually reaches it | Positive | `TestRegisteredHandlerReachesCreateLogicalVolume` | +| U-45 | Toggle compression/deduplication on an existing, active pool | Positive | `TestUpdateVolume` | +| U-46 | Toggling propagates a runner error | Negative | `TestUpdateVolume_WrapsRunnerError` | + +## 3. `atlas-lib/lvm/vdo`: the Stack Lifecycle (design §3) + +### Create, Attach, and Reactivate (`atlas-lib/lvm/vdo/stack_test.go`) + +| # | Scenario | Type | Test | +|------|-----------------------------------------------------------------------------|----------|---------------------------------------------------------------| +| U-47 | `DevicePath` derives the mount path from `lvolID` alone | Positive | `TestDevicePath` | +| U-48 | Fresh device: full pvcreate/vgcreate/lvcreate sequence, VDO flags present | Positive | `TestCreateOrAttach_FreshDevice` | +| U-49 | Existing, complete volume group: reactivated, never recreated | Positive | `TestCreateOrAttach_ExistingVolumeGroupReactivates` | +| U-50 | Orphaned volume group (interrupted create, zero LVs): removed and recreated | Boundary | `TestCreateOrAttach_OrphanedVolumeGroupIsRemovedAndRecreated` | + +### Deactivate and Remove: the Unreachable-Device Fallback (design §3, §7) + +| # | Scenario | Type | Test | +|------|-----------------------------------------------------------------------------|----------|--------------------------------------------------------| +| U-51 | Deactivate succeeds normally | Positive | `TestDeactivate_Success` | +| U-52 | Deactivate falls back to `dmsetup` cleanup only on "volume group not found" | Boundary | `TestDeactivate_UnreachableDeviceFallsBackToDMCleanup` | +| U-53 | Deactivate does not swallow an unrelated error into the fallback path | Negative | `TestDeactivate_OtherErrorIsNotSwallowed` | +| U-54 | Remove succeeds normally | Positive | `TestRemove_Success` | +| U-55 | Remove falls back to `dmsetup` cleanup on any failure | Boundary | `TestRemove_UnreachableDeviceFallsBackToDMCleanup` | + +### Grow, Clone Resolution, and Feature Toggling + +| # | Scenario | Type | Test | +|------|----------------------------------------------------------------------------------------|----------|--------------------------------------------------| +| U-56 | Full grow sequence: expand PV, expand pool, read pool size, extend VDO LV to that size | Positive | `TestGrow` | +| U-57 | A foreign volume-group identity is re-stamped and the source's LV renamed | Positive | `TestResolveClone_ForeignVolumeGroupIsReStamped` | +| U-58 | A device already carrying this volume's own identity is left alone | Negative | `TestResolveClone_OwnIdentityIsANoOp` | +| U-59 | `SetFeatures` builds the right `lvchange` command against the pool | Positive | `TestSetFeatures` | + +## 4. CSI Driver: Topology and `PersistentVolume` Pinning + +`### vdoCapableSegment (design §4)` (`csi-driver/pkg/spdk/controllerserver_test.go`) + +| # | Scenario | Type | Test | +|------|----------------------------------------------------------------------------|----------|-------------------------| +| U-60 | Either client-side flag set contributes the `vdo-capable` topology segment | Positive | `TestVDOCapableSegment` | +| U-61 | Neither flag set: no segment contributed | Negative | `TestVDOCapableSegment` | + +--- + +## 5. Manual Scenarios (Live Cluster) + +`csi-driver/pkg/util/vdo.go` itself has no direct unit tests (design §9): its +only logic is a one-line delegation per RPC concern into already-tested +`atlas-lib/lvm/vdo` functions. What unit tests structurally cannot reach is +whether `dm-vdo` is actually present and behaves as assumed, and whether a real +HA volume's duplicate local device nodes actually produce the ambiguity design +§7 describes. Every finding in design §7 came from the scenarios below. + +### M-01: Create a pool with both flags, provision a volume + +**Design reference:** §2, §4 + +**What to verify:** a `StoragePool` with `clientCompression: true` and +`clientDeduplication: true` produces a `StorageClass` carrying +`client_compression`/`client_deduplication = "True"` and an `allowedTopologies` +requiring `vdo-capable=true`. A PVC against it schedules only onto a node that +has advertised `vdo-capable=true`. The pod's mount is the VDO logical device +(`/dev/vdo-/`), not the raw NVMe-oF path. `vdostats` shows +`VDOCompression`/`VDODeduplication` both enabled. + +**Test concept:** +1. Create the pool, PVC, and pod. +2. Confirm the `StorageClass` and `PersistentVolume.spec.nodeAffinity` (design §4). +3. Confirm the mount device and `vdostats` on the node. +4. Write compressible and duplicate data, then confirm `vdostats` savings are non-zero. + +### M-02: Reattach on pod recreate (no data loss) + +**Design reference:** §3 (`CreateOrAttach`, `Deactivate`) + +**What to verify:** deleting and recreating the pod on the same node reattaches +the existing VDO device rather than recreating it, and data survives with a +matching checksum. This is the live-cluster proof for the bug design §7 +describes: an earlier version called the destructive `Remove` from +`NodeUnstageVolume` and destroyed data on exactly this sequence. + +**Test concept:** +1. Write and checksum data through the pod. +2. Delete the pod, recreate it against the same PVC. +3. Confirm no `pvcreate`/`vgcreate`/`lvcreate` ran (log inspection), and the + checksum still matches. + +### M-03: Expand + +**Design reference:** §3 (`Grow`) + +**What to verify:** growing the PVC grows the VDO pool and the VDO logical +volume online, filesystem included, with data intact throughout. + +### M-04: Clone and snapshot restore co-located with a still-live source + +**Design reference:** §3 (`ResolveClone`) + +**What to verify:** a direct PVC clone and a snapshot restore, each scheduled +onto the same node as their still-live source, each resolve to their own +volume-group identity (`vgimportclone` + rename) and mount with data matching +the source exactly, with no cross-contamination between source, clone, and +restore. + +### M-05: Storage-side disconnect while the node stays up + +**Design reference:** §3 (`Deactivate`'s unreachable-device fallback), §7 + +**What to verify:** severing a VDO volume's NVMe-oF connection at the storage +side while the node and pod stay up eventually surfaces as a real I/O failure +(VDO fences itself read-only rather than corrupting data), and deleting the pod +afterward cleans up fully automatically, with no orphaned `dm-vdo` stack left +behind. This is the live-cluster proof for design §7's device-mapper +dash-escaping finding. + +### M-06: XFS on top of VDO + +**Design reference:** §5 (`xfsStripeOptions` skip) + +**What to verify:** `mkfs.xfs` against a VDO volume runs with no stripe-alignment +flags, and the volume behaves identically to `ext4` on VDO for the rest of this +plan's scenarios. + +--- + +## Coverage Summary + +61 unit scenarios across `atlas-lib/lvm`, `atlas-lib/lvm/vdo`, and the CSI +driver's topology segment, plus 6 manual live-cluster scenarios covering what +unit tests cannot reach (design §9). + +## What Is Not Yet Covered + +| Gap | Reason | +|-------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------| +| `csi-driver/pkg/util/vdo.go`'s own delegation lines | No direct unit test file exists for this wiring. Every branch is a one-line call into already-tested `atlas-lib/lvm/vdo` functions (design §5, §9). | +| `advertiseVDOCapability`'s marker-file poll and label patch (design §4) | No unit or manual scenario in this plan exercises the DaemonSet `postStart` install path or the polling goroutine directly. | +| Server-side and client-side compression enabled together on one pool | Open question (design §8, Q1). No defined expected behavior to test against yet. | +| A non-RHEL-family node's capability install failing gracefully | Open question (design §8, Q3). Current behavior is whatever `dnf`'s absence produces, not a validated path. | +| Minimum VDO pool size | No enforcement exists (design §8, Q4), so there is nothing to assert beyond dm-vdo's own error text. | diff --git a/operator/docs/tests/test-plan-node-volume-stack.md b/operator/docs/tests/test-plan-node-volume-stack.md deleted file mode 100644 index e132caf22..000000000 --- a/operator/docs/tests/test-plan-node-volume-stack.md +++ /dev/null @@ -1,442 +0,0 @@ -# Test Plan: Node-Side Volume Stack - -Related design: [`designs/design-node-volume-stack.md`](../designs/design-node-volume-stack.md) -Harness: [`csi-driver/pkg/util`](../../../csi-driver/pkg/util), [`csi-driver/pkg/spdk`](../../../csi-driver/pkg/spdk), [`csi-driver/e2e`](../../../csi-driver/e2e) - -Scope: the operator, the CSI driver, and the Kubernetes surface of this -repository. Control-plane (`sbcli`) and SPDK behavior is a dependency, faked at -the boundary. LVM, device-mapper, VDO, and the NVMe-oF kernel driver are host -dependencies, faked through a command runner and a sysfs fixture for the unit -class and exercised for real in the end-to-end class. - -Scenario IDs are permanent: `U-` unit (no cluster: pure functions, fake host -surface, mock HTTP), `I-` integration (the runner against a real temporary -directory and a faked host, no Kubernetes), `E-` end-to-end (live cluster, real -data path), `M-` manual (needs failure injection not yet automated). Types are -Types are -`Positive`, `Negative`, `Boundary`, `Regression`. The `Test` column names the -implementing function, or `—` when the scenario is not yet covered. Every `—` -is accounted for in §8 What Is Not Yet Covered. - -Section references written as `§n` mean this plan. References to the design are -written `design §n`. - ---- - -## Axes Selected - -| Axis | Applies | Reading for this feature | -|-----------------------------|---------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| A. Storage cluster topology | Yes, reframed | Node count is visible to the node service only as the number of NVMe-oF endpoints the control plane returns, so the axis is exercised as path count: one path (single-node cluster), several paths in priority order, and one of several unreachable | -| B. Namespace scope | Yes | LVM object names are derived from the logical volume's UUID, so the same PVC name in two namespaces must produce distinct volume groups (design §5.4) | -| C. Cluster count | Yes | Two `StorageCluster`s in one Kubernetes cluster put two volumes on one host whose stack records and LVM names must not collide | -| D. Failure domains | No | The node service performs no placement. Node eligibility is design §10 and is covered by the Phase 4 block in §5 | -| E. Object scale | Yes | Plan length (one layer through five), and stack count per host (zero, one, many) | -| F. Lifecycle and timing | Yes, primary | A crash at each point in `Up`, a node reboot, an unstage after total path loss, and a redundant RPC after success are the axis this design exists to get right | -| G. Trigger and actor | Yes | Six entry points drive the runner differently (design §7.5), and which verb each may call is the contract | -| H. Component version skew | Yes | A volume staged before Phase 1, a VDO volume staged by PR #402, and a record naming a layer the running version does not know | - ---- - -## 1. Unit Tests - -No host and no cluster. Layers are faked as recorders that log the verb, the -index, and the artifact they received, so ordering and unwind rules are table -tests. LVM and NVMe behavior is faked through a command runner and a sysfs -fixture of the kind `csi-driver/pkg/util/initiator_device_test.go` already -builds. Numbering runs continuously across the groups below. - -### Plan Construction (design §3) - -File: `csi-driver/pkg/spdk/plan_test.go` (new) - -| # | Scenario | Type | Test | -|------|----------------------------------------------------------------------------------------------------------------------------------------------|----------|------| -| U-01 | `volumeMode: Block` with no filesystem parameters yields `fabric` alone | Positive | — | -| U-02 | `fsType=ext4` yields `fabric` → `filesystem` | Positive | — | -| U-03 | `fsType=xfs` yields `fabric` → `filesystem`, with the XFS feature options set | Positive | — | -| U-04 | `client_compression` set alone yields `fabric` → `lvmPV` → `lvmVolume(vdo)` → `filesystem` | Positive | — | -| U-05 | `client_deduplication` set alone yields the same plan: a dedup-only volume still needs a working `kvdo` module | Positive | — | -| U-06 | Both client-side parameters set yields one `lvmVolume(vdo)` layer, not two | Boundary | — | -| U-07 | Neither client-side parameter set yields no LVM layer at all, asserted by plan length | Negative | — | -| U-08 | An unset `fsType` defaults to the plain filesystem plan rather than raw block | Boundary | — | -| U-09 | An unrecognized `fsType` is rejected at plan construction, not at `mkfs` | Negative | — | -| U-10 | A malformed volume handle is rejected before any layer is constructed | Negative | — | -| U-11 | `nsId` absent or below one is rejected, preserving today's initiator-factory validation | Negative | — | -| U-12 | Plan construction is a pure function: the same context and capability yield an equal plan twice, with no host access | Positive | — | -| U-13 | The plan derived from a volume context is identical to the plan recorded for that volume, so a re-derivation never disagrees with the record | Positive | — | - -### Runner Ordering and Unwind (design §7) - -File: `atlas-lib/volstack/runner_test.go` (new) - -| # | Scenario | Type | Test | -|------|----------------------------------------------------------------------------------------------------------------------------------------------------------|----------|------| -| U-14 | `Up` calls `Ensure` bottom to top, each layer receiving the artifact the layer below returned | Positive | — | -| U-15 | `Up` calls `Observe` before `Ensure` on every layer | Positive | — | -| U-16 | `Ensure` failing at index 2 releases index 1 and index 0, in that order | Negative | — | -| U-17 | `Ensure` failing at index 2 calls `Destroy` on nothing at all, asserted by a zero call count on every fake layer | Negative | — | -| U-18 | `Observe` failing at index 2 unwinds identically to `Ensure` failing there | Negative | — | -| U-19 | `Ensure` failing at index 0 releases nothing and returns the error | Boundary | — | -| U-20 | A `Release` that fails during an unwind does not stop the unwind of the layers below it | Negative | — | -| U-21 | `Down` calls `Release` top to bottom | Positive | — | -| U-22 | `Down` calls `Destroy` on nothing, asserted by a zero call count | Negative | — | -| U-23 | `Down` skips `Release` on a layer whose `Observe` reports `StateAbsent` | Negative | — | -| U-24 | `Down` still removes the stack record when a layer's `Release` left its object present, which is the shared-subsystem case | Positive | — | -| U-25 | A plan of one layer brings up and down correctly | Boundary | — | -| U-26 | An empty plan is rejected at construction rather than running as a no-op `Up` | Boundary | — | -| U-27 | `Up` is idempotent: a second `Up` over a fully ready stack performs no `Ensure` that changes anything, asserted by the fakes' recorded state transitions | Positive | — | -| U-28 | `Down` is idempotent: a second `Down` after a complete one is a no-op and does not error on the missing record | Positive | — | -| U-29 | The delete path calls `Down` and then `Destroy`, top to bottom, in that order | Positive | — | - -### State Classification (design §4.2) - -File: `atlas-lib/volstack/layers/state_test.go` (new) - -| # | Scenario | Type | Test | -|------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------|------------|------| -| U-30 | A device carrying no LVM signature classifies as `StateAbsent` | Positive | — | -| U-31 | A volume group with its logical volume present classifies as `StateReady` | Positive | — | -| U-32 | A volume group present but reporting zero logical volumes classifies as `StatePartial`, not `StateReady` | Boundary | — | -| U-33 | A volume group present, complete, and not activated classifies as `StateInactive`, not `StateAbsent` | Boundary | — | -| U-34 | A device whose on-disk volume group belongs to another volume classifies as `StateForeign` | Negative | — | -| U-35 | `Ensure` on `StateInactive` activates and issues no `vgcreate`, `lvcreate`, or `mkfs`, asserted by the command runner's recorded calls | Negative | — | -| U-36 | `Ensure` on `StateForeign` re-identifies before activating, asserted by the order of the recorded calls | Positive | — | -| U-37 | `Ensure` on `StatePartial` completes the object and does not recreate the volume group | Positive | — | -| U-38 | An LVM probe whose output carries a `WARNING:` line ahead of the field value still classifies correctly, which a byte-level clone produces (pins PR #402 defect 7) | Regression | — | -| U-39 | A probe that fails outright classifies as `StateAbsent` rather than propagating an error, matching the "nothing to resolve" reading | Boundary | — | -| U-40 | An unformatted device classifies as `StateAbsent` for the filesystem layer, and a formatted one as `StateInactive` when unmounted | Positive | — | - -### Artifact and Geometry Propagation (design §4.3) - -File: `atlas-lib/volstack/artifact_test.go` (new) - -| # | Scenario | Type | Test | -|------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------|------------|------| -| U-41 | A `fabric` layer over a backend with known striping reports that geometry upward | Positive | — | -| U-42 | An `lvmVolume(vdo)` layer reports the zero `Geometry`, because VDO virtualizes blocks | Positive | — | -| U-43 | The `filesystem` layer passes `mkfs.xfs` no stripe alignment when the layer below reports the zero `Geometry` (replaces PR #402's `xfsStripeOptions` conditional) | Regression | — | -| U-44 | The `filesystem` layer passes `mkfs.xfs` the XFS feature options regardless of geometry, because feature-bit compatibility is unrelated to layout | Positive | — | -| U-45 | An `lvmVolume(striped)` layer reports its own stripe count and chunk size, and the filesystem above aligns to those rather than to the StorageClass parameters | Positive | — | -| U-46 | A `Geometry` with a stripe count but no chunk size is treated as unknown rather than half-applied | Boundary | — | -| U-47 | A fan-in layer reports its member devices in the recorded order, and a differently ordered member list produces a different artifact | Positive | — | - -### The Stack Record (design §6) - -File: `atlas-lib/volstack/record_test.go` (new) - -| # | Scenario | Type | Test | -|------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|----------|------| -| U-48 | The record is written before the first `Ensure` runs, asserted by the fakes observing the file already present | Positive | — | -| U-49 | The record holds layer parameters and no device path, so a reconnect that renames the device leaves it valid | Positive | — | -| U-50 | A per-layer marker is written before that layer's `Ensure`, not after | Positive | — | -| U-51 | The record is removed only after the last `Release` succeeds | Positive | — | -| U-52 | A `Release` that fails leaves the record in place, so the stack stays discoverable | Negative | — | -| U-53 | An absent record resolves to the legacy plan `fabric` → `filesystem` | Negative | — | -| U-54 | A record naming an unknown layer fails the unstage with the layer named, rather than skipping the layer | Negative | — | -| U-55 | A truncated or malformed record fails with an error and does not resolve to the legacy plan, because a partial record is not an absent one | Boundary | — | -| U-56 | Two volumes from two `StorageCluster`s produce distinct record filenames, because the volume handle carries the cluster ID | Positive | — | -| U-57 | The same PVC name in two namespaces produces distinct record filenames and distinct LVM names | Positive | — | -| U-58 | A record filename is filesystem-safe for every volume handle the driver accepts | Boundary | — | - -### LVM Naming and Primitives (design §5.3, §5.4) - -File: `atlas-lib/lvm/lvm_test.go` (moved from `csi-driver/pkg/util/vdo.go`'s tests) - -| # | Scenario | Type | Test | -|------|-----------------------------------------------------------------------------------------------------------------------------------------------------------|------------|------| -| U-60 | The volume group name is derived from the logical volume's UUID and nothing host-specific, so it is reproducible on another host | Positive | — | -| U-61 | The derived name is within LVM's length and character limits for every accepted UUID form | Boundary | — | -| U-62 | Two volumes on one host derive distinct volume group names | Positive | — | -| U-63 | The device-mapper force path escapes the volume group name by doubling dashes before matching (pins PR #402 defect 9) | Regression | — | -| U-64 | The force path matches nothing and returns cleanly when no device-mapper node for the group exists | Negative | — | -| U-65 | Every LVM invocation carries `DM_DISABLE_UDEV=1`, because no udev daemon runs in the node container (pins PR #402 defect 4) | Regression | — | -| U-66 | Every LVM invocation is scoped to the device under management rather than scanning every visible device | Positive | — | -| U-67 | `Grow` extends by the additive percentage form, so the computed target is never smaller than the current size (pins PR #402 defect 6) | Regression | — | -| U-68 | `Grow` on a volume already at its target size succeeds without issuing an extend, which is kubelet's post-success retry (pins PR #402's open polish item) | Regression | — | -| U-69 | An LVM command that exceeds its timeout returns an error rather than blocking the caller | Negative | — | - -### Co-Tenant Detach (design §8) - -File: `csi-driver/pkg/util/initiator_device_test.go`, extended - -| # | Scenario | Type | Test | -|------|-------------------------------------------------------------------------------------------------------------------------------------------|----------|---------------------------------------------------------| -| U-70 | A subsystem that can hold several namespaces is not disconnected when one volume releases, and the paths stay up | Positive | — | -| U-71 | A subsystem that cannot hold several namespaces is disconnected on release | Positive | `TestDisconnectGlobOnLastNamespace` | -| U-72 | A subsystem currently holding co-tenants is not disconnected | Positive | `TestDisconnectGlobOnRealNode` | -| U-73 | A subsystem that can be shared but currently holds one namespace is still not disconnected, which the current count-based gate gets wrong | Boundary | — | -| U-74 | The capability question failing to resolve leaves the fabric untouched and returns the error, rather than assuming either answer | Negative | — | -| U-75 | Releasing a volume on a shared subsystem removes that volume's stack record and leaves the co-tenant's record alone | Negative | — | -| U-76 | `Destroy` on one volume's LVM objects issues no command naming a co-tenant's volume group | Negative | — | -| U-77 | A namespace device belonging to a neighboring namespace is not selected as this volume's device | Negative | `TestMatchNamespaceDeviceRejectsNeighbouringNamespaces` | - -### Optional Interface Dispatch (design §4.4, §9) - -File: `atlas-lib/volstack/optional_test.go` (new) - -| # | Scenario | Type | Test | -|------|------------------------------------------------------------------------------------------------------------------------------------|----------|------| -| U-78 | `Heal` visits only the layers implementing `Healer`, bottom to top | Positive | — | -| U-79 | `Heal` skips a layer whose `Healthy` returns true, asserted by a zero `Heal` call count on it | Negative | — | -| U-80 | A plan whose layers implement no `Healer` heals as a no-op and returns no error | Boundary | — | -| U-81 | `Heal` on a layer receives the artifact of the layer below as re-derived after that layer was healed, not the artifact from before | Positive | — | -| U-82 | `Grow` visits only the layers implementing `Grower`, bottom to top | Positive | — | -| U-83 | A plan whose layers implement no `Grower`, which is the pNFS client shape, grows as a no-op | Boundary | — | -| U-84 | `Grow` stops and reports when a lower layer's grow fails, and does not attempt the layers above it | Negative | — | -| U-85 | `Heal` never calls `Ensure`, asserted by a zero call count, because the data already exists | Negative | — | - ---- - -## 2. Integration Tests - -The runner against a real temporary directory for the stack record and a faked -host surface for the layers. No Kubernetes and no `envtest`: nothing in Phases 1 -through 3 reconciles. The value of this class is crash simulation, which the unit -class cannot express because it needs a record that survives the process. - -### Crash and Resume (design §6, §7) - -File: `atlas-lib/volstack/resume_test.go` (new) - -| # | Scenario | Type | Test | -|------|-----------------------------------------------------------------------------------------------------------------------------------------------------|----------|------| -| I-01 | `Up` interrupted after the record is written and before the first `Ensure`: a fresh `Down` finds the record and releases nothing, leaving no orphan | Positive | — | -| I-02 | `Up` interrupted after `fabric`'s `Ensure` and before its marker would have been cleared: a fresh `Down` releases the fabric | Positive | — | -| I-03 | `Up` interrupted at each layer index in turn: a fresh `Down` releases exactly the layers that were reached | Positive | — | -| I-04 | `Up` interrupted mid-`Ensure` on a layer that had created its object: the next `Up` observes `StatePartial` and completes it | Positive | — | -| I-05 | `Down` interrupted after two layers were released: a second `Down` releases the rest and removes the record | Positive | — | -| I-06 | The whole host restarts with a record present and every layer inactive: `Up` reactivates and issues no create or format command | Positive | — | -| I-07 | A record present for a volume that has no staging path and no pod on this host is reported as orphaned rather than released automatically | Negative | — | -| I-08 | The record directory is unwritable: `Up` fails before its first side effect rather than proceeding unrecorded | Negative | — | -| I-09 | Two records for the same volume handle cannot exist, and a second `Up` reuses the first | Boundary | — | -| I-10 | 100 records on one host are enumerated and classified without exceeding the enumeration's bound, and the time is recorded | Boundary | — | - -### Verb Contract Under a Dead Foundation (design §7.4) - -File: `atlas-lib/volstack/deadfoundation_test.go` (new) - -| # | Scenario | Type | Test | -|------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------|------------|------| -| I-11 | Every layer's `Release` succeeds when the layer below reports `StateAbsent` | Positive | — | -| I-12 | `lvmVolume`'s `Release` falls back to the device-mapper force path when the LVM command fails on every retry, and the fallback is recorded (pins PR #402 defect 8) | Regression | — | -| I-13 | `filesystem`'s `Release` unmounts a dead mount rather than erroring on it | Positive | — | -| I-14 | A layer with no force path whose command depends on a dead foundation reports the failure and leaves the record in place | Negative | — | -| I-15 | `Down` over a stack whose every layer is already gone completes and removes the record | Boundary | — | - ---- - -## 3. E2E Tests - -A live simplyblock cluster and a real data path. Every row that touches data -asserts correctness by checksum across the operation, not merely that I/O -continued. The suite is Ginkgo, and new blocks are named in the existing -`SPDKCSI-` style. - -### No Behavior Change (design §16) - -The Phase 1 claim is that nothing observable changes, so the existing blocks are -the assertion and must pass unmodified. - -| # | Scenario | Type | Test | -|------|----------------------------------------------------------------------------|----------|-------------------------------------| -| E-01 | An ext4 and an XFS volume stage, publish, and unstage through the runner | Positive | `SPDKCSI-FILESYSTEM` | -| E-02 | A raw block volume stages through a one-layer plan | Positive | `SPDKCSI-RAWBLOCK` | -| E-03 | Data survives a pod delete and an immediate re-mount, verified by checksum | Positive | `SPDKCSI-VOLUME-PERSIST` | -| E-04 | A volume reconnects after path loss and the mount recovers | Positive | `SPDKCSI-RECONNECT` | -| E-05 | A volume recovers after total path loss | Positive | `SPDKCSI-RECONNECT-FULLLOSS` | -| E-06 | The guardian repairs a broken volume under a running pod | Positive | `SPDKCSI-RECONNECT-GUARDIAN` | -| E-07 | An unmanaged subsystem on the host is left alone | Negative | `SPDKCSI-RECONNECT-UNMANAGED` | -| E-08 | A clone and a snapshot restore stage alongside their live source | Positive | `SPDKCSI-CLONE`, `SPDKCSI-SNAPSHOT` | -| E-09 | Volumes from two `StorageCluster`s stage on one host | Positive | `SPDKCSI-MULTICLUSTER` | -| E-10 | An invalid request is rejected as it is today | Negative | `SPDKCSI-NEGATIVE` | - -### Stacked Plans (design §3, §5) - -| # | Scenario | Type | Test | -|------|------------------------------------------------------------------------------------------------------------------------------------------------------|------------|------| -| E-11 | A VDO-backed ext4 volume stages, and `lvs` reports compression and deduplication enabled | Positive | — | -| E-12 | A VDO-backed XFS volume stages with no stripe alignment passed to `mkfs.xfs` | Positive | — | -| E-13 | A pod delete and recreate on the same node reattaches the same VDO device, and the checksum matches (pins PR #402 defect 5, the destructive unstage) | Regression | — | -| E-14 | A node reboot reattaches every stack on the host, with a checksum per volume and the `kvdo` module usage count matching the stack count | Positive | — | -| E-15 | A clone and its source coexist on one node with independent volume group identities, both checksums matching their sources | Positive | — | -| E-16 | An unclean NVMe-oF disconnect under a live pod, followed by a pod delete, leaves no orphaned device-mapper stack and needs no manual intervention | Regression | — | -| E-17 | A PVC expansion grows the LVM stack and then the filesystem online, with data intact throughout | Positive | — | -| E-18 | A second expansion request after a successful one is a no-op and logs no error | Boundary | — | -| E-19 | A volume whose plan needs a capability the node lacks fails to stage there with the reason visible on the PVC | Negative | — | -| E-20 | Two volumes on a shared subsystem: unstaging one leaves the other serving I/O with no error | Positive | — | -| E-21 | Deleting the PVC of one volume on a shared subsystem leaves the co-tenant's volume group intact | Negative | — | -| E-22 | The same PVC name in two namespaces, both staged on one node, produce two distinct volume groups and two distinct datasets | Positive | — | -| E-23 | A volume staged before the upgrade unstages cleanly after it, with no record present | Positive | — | -| E-24 | A VDO volume staged by the pre-Phase-2 code unstages cleanly after Phase 2, without destroying data | Regression | — | -| E-25 | A single-path volume, from a single-node storage cluster, stages and unstages | Boundary | — | -| E-26 | A multipath volume with one endpoint unreachable stages on the reachable paths and reports the unavailable one | Negative | — | -| E-27 | Sustained fio across a stage, heal, and expand cycle: no I/O errors and verify-mode data integrity | Positive | — | - ---- - -## 4. Unit Tests — Phase 4 (Planned) - -Node requirements derived from the plan (design §10). No `Type` and no `Test` -column: both are decided when the phase is scoped, and design §17 Q6 records that -the phase is planned rather than committed. This is the first block with an -`envtest` surface, because the StoragePool controller composes the topology terms. - -| # | Scenario | -|---------|----------------------------------------------------------------------------------------------------------------------------------------------------| -| U-P4-01 | A plan containing a host-local layer yields that layer's capability in the volume's accessible topology | -| U-P4-02 | A plan containing no host-local layer yields no capability segment, which is the pNFS client shape | -| U-P4-03 | Two host-local layers in one plan yield both capabilities, composed into one topology term so they are required together | -| U-P4-04 | A plan-derived segment produces the same key and value the hand-written DHCHAP segment produces, so existing volume node affinity stays valid | -| U-P4-05 | A capability key is present in the node's reported topology at plugin registration regardless of the label's current value (pins PR #402 defect 1) | -| U-P4-06 | A layer reporting `PinsToNode` false contributes no node affinity even when it reports a capability | -| U-P4-07 | The StoragePool controller derives the same topology terms from the plan that the CSI controller derives | - ---- - -## 5. Manual Scenarios and Test Concepts - -### M-01 — Concurrent staging of two LVM-backed volumes on one host - -**Design reference:** design §12 - -**What to verify:** two `Ensure` calls whose LVM commands genuinely overlap do -not corrupt either volume group and do not leave either stack partial. - -**Current behavior:** unknown, and unexercised rather than proven safe. PR #402's -multi-instance validation ran its two stage sequences sequentially rather than -overlapping, so LVM's internal locking under truly concurrent `vgchange` and -`pvscan` has never been exercised on a real host. Per-volume locking does not -serialize them, because the contention is on LVM's host-wide locks and its device -scan. - -**Open question:** design §17 Q2. The lock scope per layer is undecided, so this -scenario is as much a measurement as a test. - -**Test concept:** -1. Provision two PVCs whose StorageClass sets `client_deduplication`, both - scheduled to one node. -2. Start both pods in one `kubectl apply`, so both `NodeStageVolume` calls arrive - within the same second. Confirm from the node plugin's log that the two LVM - command sequences actually interleave, rather than asserting they did. -3. Write and checksum distinct data in each pod. -4. Assert both volume groups exist with the expected logical volume, both - checksums match, and no LVM command reported a lock or a metadata error. -5. Repeat with ten volumes to widen the overlap window. - -### M-02 — A host crash between the record write and the first `Ensure` - -**Design reference:** design §6 - -**What to verify:** the record written ahead of the first side effect is what -makes an orphan discoverable, and a crash in that window leaves nothing attached -that no record names. - -**Test concept:** -1. Build a node plugin whose runner panics after writing the record and before - the first `Ensure`, gated behind an environment variable. -2. Stage a volume with a four-layer plan, triggering the panic. -3. Assert the record exists, no NVMe-oF path is attached, and no device-mapper - node exists. -4. Restart the plugin, unstage, and assert the record is gone. -5. Repeat with the panic moved to each later point in `Up`, asserting that - everything below the panic point is released by the unstage. - -### M-03 — A namespace joins a shared subsystem between the check and the release - -**Design reference:** design §8 - -**What to verify:** the capability gate, not the neighbor count, is what prevents -a destructive disconnect, so a namespace arriving in that window changes nothing. - -**Current behavior:** `selectDisconnectTarget` counts the namespace devices the -by-id glob currently matches, so a subsystem that has just become the last one is -disconnected. A namespace joining immediately afterward loses its paths. - -**Test concept:** -1. Provision two volumes on one subsystem with `max_namespace_per_subsys` above - one, both staged on one node. -2. Unstage the first, and while its `Release` runs, stage a third volume onto the - same subsystem. -3. Assert the second volume never loses a path, and that the third stages - successfully. -4. Repeat with the second volume unstaged first, so the window opens on the last - remaining namespace. - -### M-04 — A record naming a layer the running plugin does not know - -**Design reference:** design §13, design §17 Q1 - -**What to verify:** a downgrade fails loudly rather than skipping an object -nobody will release. - -**Test concept:** -1. Stage a volume with a plan containing a layer, then hand-edit its record to - name a layer the plugin does not implement. -2. Unstage, and assert the RPC fails with the unknown layer named, and that the - record is left in place. -3. Assert the objects the plugin does know about are not released either, because - a teardown that releases half a stack is worse than one that refuses. - ---- - -## 6. Axis Coverage - -| Axis | Values covered | IDs | Not covered | -|-------------------------|-----------------------------------------------------------------------------------------------------------------------|-------------------------------------|-------------------------------------------------------------------| -| A. Path count | One path, several in priority order, one of several unreachable | E-25, E-04, E-26 | More paths than the cluster has nodes | -| B. Namespace scope | Single, two namespaces with the same PVC name | U-01 … U-13, U-58, E-22 | Namespace deleted mid-stage | -| C. Cluster count | One `StorageCluster`, two in one Kubernetes cluster | E-01, U-57, E-09 | Two Kubernetes clusters, which this feature cannot see | -| D. Failure domains | — | — | Excluded: the node service performs no placement (§Axes Selected) | -| E. Object scale | Plan of one layer, of four, of five; zero, one, and 100 stacks per host | U-25, U-04, U-26, I-10, E-14 | More than 100 stacks per host | -| F. Lifecycle and timing | Crash at each `Up` index, crash mid-`Release`, node reboot, redundant post-success RPC, unstage after total path loss | I-01 … I-06, U-68, E-14, E-16, M-02 | Kubelet restart between `Up` and the record's removal | -| G. Trigger and actor | Stage, unstage, publish-heal, expand, restage, delete | U-14, U-21, U-78, U-82, U-85, U-29 | `NodeGetVolumeStats` over a stacked plan | -| H. Version skew | Pre-Phase-1 volume, pre-Phase-2 VDO volume, unknown layer in a record | E-23, U-53, E-24, U-54, U-55, M-04 | A record written by a version two phases ahead | - ---- - -## 7. Coverage Summary - -| Class | Scenarios | Covered | Not covered | -|----------------|-----------|---------|---------------------------------------| -| Unit | 85 | 3 | U-01 … U-70, U-73 … U-76, U-78 … U-85 | -| Integration | 15 | 0 | I-01 … I-15 | -| E2E | 27 | 10 | E-11 … E-27 | -| Unit — Phase 4 | 7 | 0 | U-P4-01 … U-P4-07 | -| Manual | 4 | 0 | M-01 … M-04 | - -The three covered unit scenarios are existing tests whose behavior this design -preserves rather than introduces: `TestDisconnectGlobOnLastNamespace` (U-71), -`TestDisconnectGlobOnRealNode` (U-72), and -`TestMatchNamespaceDeviceRejectsNeighbouringNamespaces` (U-77). The ten covered -end-to-end scenarios are the existing suite, which is the assertion that Phase 1 -changes nothing observable (design §16). - ---- - -## 8. What Is Not Yet Covered - -Nothing in this design is implemented, so the gap list is stated as ranges rather -than as 130 identical rows. The reason column says what each range waits on -rather than repeating "not implemented." - -| # | Gap | Reason | -|-------------------|-------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------| -| U-01 … U-13 | Plan construction | Phase 1. The plan type does not exist, and `plan_test.go` is created with it | -| U-14 … U-29 | Runner ordering and unwind | Phase 1. The highest-value block: U-17 and U-22 are what make the release-never-destroy rule checkable | -| U-30 … U-40 | State classification | Phase 1 for the filesystem states, Phase 2 for the LVM states | -| U-41 … U-47 | Artifact and geometry | Phase 1 for the plain plan, Phase 2 for U-42, U-43, and U-45 | -| U-48 … U-59 | The stack record | Phase 1 | -| U-60 … U-69 | LVM naming and primitives | Phase 2. Six of these pin defects PR #402 fixed and must be ported with the code, not rewritten from the design | -| U-70, U-73 … U-76 | The strengthened co-tenant gate | Phase 1. U-73 fails against the current count-based gate, which is the point of the row | -| U-78 … U-85 | Optional interface dispatch | Phase 3 | -| I-01 … I-15 | Crash, resume, and dead-foundation contracts | Phase 1 for I-01 through I-11 and I-13 through I-15, Phase 2 for I-12 | -| E-11 … E-27 | Stacked plans on a live cluster | Phase 2 for E-11 through E-18 and E-24, Phase 1 for E-20 through E-23 and E-25 through E-27, Phase 4 for E-19 | -| U-P4-01 … U-P4-07 | Plan-derived node requirements | Phase 4, which design §17 Q6 records as planned rather than committed | -| M-01 … M-04 | Concurrency and crash injection | Needs failure injection the suites do not have. M-01 is a risk in shipped code once PR #402 merges, not only in this design | -| — | Two Kubernetes clusters | The node service has no cross-cluster surface. Excluded, not deferred | -| — | Failure domains and placement topology | The node service performs no placement (§Axes Selected) | -| — | More than 100 stacks per host | No bound is claimed above that, so no row asserts one. A bound belongs in the design before a test asserts it | -| — | Kubelet restart between `Up` and the record's removal | The record is designed to survive it (design §6), but the injection point is inside kubelet rather than inside the plugin | -| — | `NodeGetVolumeStats` over a stacked plan | `statfs` on the mount point is indifferent to what is underneath it, so no layer participates. Revisit if a layer ever reports its own capacity | -| — | Block mode combined with an LVM layer | Out of scope (design §2), and representable rather than untested |