Skip to content

feat: client-side compression/dedup via VDO (issue #277) - #402

Open
boddumanohar wants to merge 9 commits into
mainfrom
issue-277-client-side-compression-impl
Open

boddumanohar wants to merge 9 commits into
mainfrom
issue-277-client-side-compression-impl

Conversation

@boddumanohar

@boddumanohar boddumanohar commented Aug 7, 2026

Copy link
Copy Markdown
Member

Summary

Client-side compression/deduplication (issue #277), built on atlas-lib/volstack's layered LVM stack.

Design

Node-side stackcsi-driver/internal/csi/node/vdo.go drives three volstack layers with the real volstack.Runner, behind a thin adapter standing in for the fabric layer (the already-connected NVMe-oF device):

graph TD
    A[NVMe-oF device<br/>already connected] --> B[lvmPhysicalVolume]
    B --> C[lvmVolumeGroup]
    C --> D["lvmLogicalVolume (VDO)"]
    D --> E[mkfs / mount]
Loading

Wired into NodeStageVolume/NodeUnstageVolume/restageVolume/NodeExpandVolume, only for volumes that ask for either parameter.

  • API: VolumeDefaults.EnableClientCompression/EnableClientDeduplication (v1alpha2), passed through to client_compression/client_deduplication StorageClass parameters.
  • Scheduling: vdoCapableSegment pins PersistentVolume.spec.nodeAffinity to a vdo-capable node from CreateVolume, mirroring dhchapAllowedNodeSegment.
  • Capability probe: csi-node's postStart hook probes both dm-vdo and kvdo (kernel 6.9+ ships the former in-tree; RHEL/Rocky 9-family kernels only ship the latter via kmod-kvdo) alongside its existing nvme-tcp/nvme-rdma modprobes, and self-labels the node only if either succeeds; RBAC widened to patch on nodes.
  • Formatting: mount.FormatOptions skips mkfs's default full-device discard on a VDO volume (-K for mkfs.xfs, -E nodiscard for mke2fs) — VDO's block map processes a discard proportionally to volume size, where a plain backend device does not.
  • Admission control: a new VDOSizeFloorValidator webhook rejects a PVC asking for either parameter below VDO's own ~5GiB floor at creation time, instead of that request only failing later, inside lvcreate, at NodeStageVolume.
  • Image: Dockerfile_base installs the vdo package (amd64 only).
  • Cleanup: atlas-lib/lvm/vdo (the old flat package) retired in favor of the volstack layers. Along the way, found it had never actually registered a VDO VolumeProvisioning handler — a real, separate bug where CreateLogicalVolume silently dropped compression/deduplication.
  • Resilience: total NVMe-oF path loss (the member device fully gone) hard-errored the whole volstack teardown before it ever reached the device-mapper force path. Added Manager.HasOrphanedDMNodes and taught the three LVM layers to tolerate it.
  • E2E: csi-driver/e2e/vdo.go reads vdostats inside the csi-node pod hosting the volume and asserts physical usage grows far less than logical usage — one spec compression-only, one deduplication-only — so a regression that silently drops the VDO layer fails here even though every other suite would still pass.

Testing

Verified live on a real cluster (k3s, Rocky Linux 9.5, real simplyblock storage backend).

Metric Result
NodeStageVolume, 100GB volume, first format 86.7s → 3.6s
NodeStageVolume, 20GB volume, first format 3.2s
Compression saving (500MB repeated-pattern payload) 49%
Dedup: physical growth from 9 duplicate 500MB copies 0 bytes (of ~4.5GB logical)
VDO size-floor webhook 1GiB rejected, 5GiB accepted
Automated e2e (SPDKCSI-VDO) 2/2 passing
CI Lint, Manifests, Helm Lint, Operator/CSI unit tests, Operator E2E, Docker builds — all green

E2E testing

Ran a full e2e test by skipping the multi cluster tests.

Will run 33 of 34 specs 
  Running in parallel across 6 processes

  Ran 33 of 34 Specs in 808.571 seconds
  SUCCESS! -- 33 Passed | 0 Failed | 0 Pending | 1 Skipped

  Ginkgo ran 1 suite in 13m39.529582073s
  Test Suite Passed

boddumanohar added a commit that referenced this pull request Aug 7, 2026
Tested against the real implementation (PR #402), not just raw LVM
commands: two real PVCs with clientCompression/clientDeduplication on the
same node, distinct checksummed data, node rebooted. Both VDO instances
reattached cleanly via fresh NodeStageVolume calls (kubelet's own
bookkeeping resets on reboot too) -- kvdo module usage count exactly 2, both
VDOOperatingMode normal, both checksums matched exactly.

Caveat found and documented: the two NodeStageVolume LVM command sequences
happened to complete sequentially rather than genuinely overlapping, so
LVM's internal command locking under truly concurrent vgchange/pvscan calls
remains unexercised -- narrowed the open item accordingly rather than
closing it outright.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
boddumanohar added a commit that referenced this pull request Aug 7, 2026
…ation

Tested both clone paths against PR #402's real ResolveClonedVDO: a direct
PVC-to-PVC clone and a snapshot restore, both scheduled onto the same node
as their still-live source (the specific co-location scenario this finding
warns about). Both correctly resolved via vgimportclone + lvrename, mounted
cleanly with data matching the source exactly, and coexisted with the
source and each other with independent VG identities and no
cross-contamination.

Also corrected the "Detection" section: the implementation ended up simpler
than originally planned -- detection is unconditional and purely
device-identity-based, not gated on VolumeContentSource, so no separate
content-source plumbing was needed.

Found and fixed one bug along the way: the collision-detection log message
was picking up pvs's stderr WARNING: lines merged into its output instead of
just the VG name -- harmless in this run, but fixed to parse cleanly.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
boddumanohar added a commit that referenced this pull request Aug 7, 2026
Deliberately reproduced against PR #402's real implementation: forcibly
disconnected a VDO volume's NVMe-oF subsystem at the host level while the
node stayed up, then deleted the pod. This exposed two real bugs (both now
fixed on that branch): DeactivateVDO had no fallback for an unreachable
device, and once added, the fallback's device-name matching didn't account
for device-mapper's dash-escaping and matched nothing. With both fixed,
cleanup is now fully automatic -- confirmed by reproducing the whole
sequence a second time.

Also documented an unplanned but valuable side observation: for ~19s after
disconnect, cached reads/writes silently appeared to succeed before the
real I/O failure surfaced, at which point VDO correctly fenced itself into
read-only mode and ext4 independently aborted its journal -- both layers
protected data correctly with no wiring needed from this design.

Narrowed the remaining open item: the node-NotReady-and-rejoin path is
still unverified (this test kept the node itself healthy throughout, only
the storage connection was severed).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@noctarius noctarius added the csi label Aug 7, 2026
@noctarius noctarius added this to the 26.4 milestone Aug 7, 2026
boddumanohar added a commit that referenced this pull request Aug 11, 2026
…lap as a known gap

#403 doesn't actually block #402 (it says so explicitly, and #402 already
works around it with a nodeSelector pin for its own verification), so
there's no need to carry the operator-side plumbing this fix grew to close
a real but narrow edge case: a single Kubernetes node listed in more than
one DHCHAP-gated pool's AllowedNodes.

Revert the dhchap_node_label StorageClass parameter and the operator change
that set it. hardPinTopologySegments goes back to matching the
"simplyblock.io/pool." prefix, with the multi-pool-overlap behavior now
documented as a deliberately accepted limitation (in both a code comment and
a test that documents current behavior rather than asserting correctness)
instead of solved. operator/ is now byte-for-byte identical to origin/main;
this PR is CSI-driver-only again.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
boddumanohar added a commit that referenced this pull request Aug 12, 2026
#403)

CreateVolume never populated AccessibleTopology for StorageClasses that
select their cluster directly via cluster_id (the common case), so
external-provisioner never set PV.spec.nodeAffinity. A bound PVC could then
be rescheduled onto any node, even when the StorageClass gates provisioning
to specific nodes via AllowedTopologies (DHCHAP's allowed-node label today,
and VDO's node-local state in the upcoming #402). Extract only the
segments that represent a genuine per-node constraint from the CSI
AccessibilityRequirements and echo them back, leaving plain
NVMe-oF-backed volumes (which behave identically from any node) unpinned.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
boddumanohar added a commit that referenced this pull request Aug 12, 2026
…lap as a known gap

#403 doesn't actually block #402 (it says so explicitly, and #402 already
works around it with a nodeSelector pin for its own verification), so
there's no need to carry the operator-side plumbing this fix grew to close
a real but narrow edge case: a single Kubernetes node listed in more than
one DHCHAP-gated pool's AllowedNodes.

Revert the dhchap_node_label StorageClass parameter and the operator change
that set it. hardPinTopologySegments goes back to matching the
"simplyblock.io/pool." prefix, with the multi-pool-overlap behavior now
documented as a deliberately accepted limitation (in both a code comment and
a test that documents current behavior rather than asserting correctness)
instead of solved. operator/ is now byte-for-byte identical to origin/main;
this PR is CSI-driver-only again.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
boddumanohar added a commit that referenced this pull request Aug 21, 2026
Tested against the real implementation (PR #402), not just raw LVM
commands: two real PVCs with clientCompression/clientDeduplication on the
same node, distinct checksummed data, node rebooted. Both VDO instances
reattached cleanly via fresh NodeStageVolume calls (kubelet's own
bookkeeping resets on reboot too) -- kvdo module usage count exactly 2, both
VDOOperatingMode normal, both checksums matched exactly.

Caveat found and documented: the two NodeStageVolume LVM command sequences
happened to complete sequentially rather than genuinely overlapping, so
LVM's internal command locking under truly concurrent vgchange/pvscan calls
remains unexercised -- narrowed the open item accordingly rather than
closing it outright.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
boddumanohar added a commit that referenced this pull request Aug 21, 2026
…ation

Tested both clone paths against PR #402's real ResolveClonedVDO: a direct
PVC-to-PVC clone and a snapshot restore, both scheduled onto the same node
as their still-live source (the specific co-location scenario this finding
warns about). Both correctly resolved via vgimportclone + lvrename, mounted
cleanly with data matching the source exactly, and coexisted with the
source and each other with independent VG identities and no
cross-contamination.

Also corrected the "Detection" section: the implementation ended up simpler
than originally planned -- detection is unconditional and purely
device-identity-based, not gated on VolumeContentSource, so no separate
content-source plumbing was needed.

Found and fixed one bug along the way: the collision-detection log message
was picking up pvs's stderr WARNING: lines merged into its output instead of
just the VG name -- harmless in this run, but fixed to parse cleanly.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
boddumanohar added a commit that referenced this pull request Aug 21, 2026
Deliberately reproduced against PR #402's real implementation: forcibly
disconnected a VDO volume's NVMe-oF subsystem at the host level while the
node stayed up, then deleted the pod. This exposed two real bugs (both now
fixed on that branch): DeactivateVDO had no fallback for an unreachable
device, and once added, the fallback's device-name matching didn't account
for device-mapper's dash-escaping and matched nothing. With both fixed,
cleanup is now fully automatic -- confirmed by reproducing the whole
sequence a second time.

Also documented an unplanned but valuable side observation: for ~19s after
disconnect, cached reads/writes silently appeared to succeed before the
real I/O failure surfaced, at which point VDO correctly fenced itself into
read-only mode and ext4 independently aborted its journal -- both layers
protected data correctly with no wiring needed from this design.

Narrowed the remaining open item: the node-NotReady-and-rejoin path is
still unverified (this test kept the node itself healthy throughout, only
the storage connection was severed).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@boddumanohar
boddumanohar force-pushed the issue-277-client-side-compression-impl branch from 88d5be8 to d201b02 Compare August 21, 2026 12:25
boddumanohar added a commit that referenced this pull request Aug 25, 2026
Tested against the real implementation (PR #402), not just raw LVM
commands: two real PVCs with clientCompression/clientDeduplication on the
same node, distinct checksummed data, node rebooted. Both VDO instances
reattached cleanly via fresh NodeStageVolume calls (kubelet's own
bookkeeping resets on reboot too) -- kvdo module usage count exactly 2, both
VDOOperatingMode normal, both checksums matched exactly.

Caveat found and documented: the two NodeStageVolume LVM command sequences
happened to complete sequentially rather than genuinely overlapping, so
LVM's internal command locking under truly concurrent vgchange/pvscan calls
remains unexercised -- narrowed the open item accordingly rather than
closing it outright.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
boddumanohar added a commit that referenced this pull request Aug 25, 2026
…ation

Tested both clone paths against PR #402's real ResolveClonedVDO: a direct
PVC-to-PVC clone and a snapshot restore, both scheduled onto the same node
as their still-live source (the specific co-location scenario this finding
warns about). Both correctly resolved via vgimportclone + lvrename, mounted
cleanly with data matching the source exactly, and coexisted with the
source and each other with independent VG identities and no
cross-contamination.

Also corrected the "Detection" section: the implementation ended up simpler
than originally planned -- detection is unconditional and purely
device-identity-based, not gated on VolumeContentSource, so no separate
content-source plumbing was needed.

Found and fixed one bug along the way: the collision-detection log message
was picking up pvs's stderr WARNING: lines merged into its output instead of
just the VG name -- harmless in this run, but fixed to parse cleanly.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
boddumanohar added a commit that referenced this pull request Aug 25, 2026
Deliberately reproduced against PR #402's real implementation: forcibly
disconnected a VDO volume's NVMe-oF subsystem at the host level while the
node stayed up, then deleted the pod. This exposed two real bugs (both now
fixed on that branch): DeactivateVDO had no fallback for an unreachable
device, and once added, the fallback's device-name matching didn't account
for device-mapper's dash-escaping and matched nothing. With both fixed,
cleanup is now fully automatic -- confirmed by reproducing the whole
sequence a second time.

Also documented an unplanned but valuable side observation: for ~19s after
disconnect, cached reads/writes silently appeared to succeed before the
real I/O failure surfaced, at which point VDO correctly fenced itself into
read-only mode and ext4 independently aborted its journal -- both layers
protected data correctly with no wiring needed from this design.

Narrowed the remaining open item: the node-NotReady-and-rejoin path is
still unverified (this test kept the node itself healthy throughout, only
the storage connection was severed).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@boddumanohar
boddumanohar force-pushed the issue-277-client-side-compression-impl branch from 6d47771 to da1673e Compare August 25, 2026 10:29
boddumanohar added a commit that referenced this pull request Aug 25, 2026
Extracts the general, non-VDO-specific LVM plumbing out of
csi-driver/pkg/util/vdo.go (issue #277, PR #402) into a shared
primitive: scoping every LVM command to an explicit device list via
--devices, and answering PV/VG/LV identity questions from a device's
own on-disk content rather than a name lookup.

The seam exists because a simplyblock NVMe-oF HA volume surfaces as
more than one local device node on the client, each presenting
byte-identical backend content. LVM's default behavior -- a
system-wide device scan, and existence checks keyed by name rather
than by device -- cannot tell those device nodes apart. VDO hit this
live as a genuine "duplicate PV" ambiguity, and separately as a
name-based `vgs <name>` check reporting a VG as present when it had
never actually been created on the device being asked about.

There is no second real consumer's code to adopt yet: PR #456
(pnfs-design) is design-only today, but its striped-volume design
explicitly plans the same pvcreate/vgcreate/lvcreate assembly pattern,
including the identical partial-state recovery problem VDO already
found and fixed (its own test plan's SI-03: "Re-running after a
failure between vgcreate and lvcreate completes without duplicating").
This extracts the primitive now so vdo.go can become the first real
consumer (adoption is a follow-up commit on PR #402), and future pNFS
implementation work builds on it from day one instead of rediscovering
the same bugs.

Package surface: Inspector.VolumeGroup (content-based PV -> VG
identity), Inspector.HasLogicalVolume (distinguishes a fully assembled
stack from an orphaned partial create), Inspector.Rescan
(device-scoped pvscan --cache), Inspector.Run (arbitrary LVM/dm
command, --devices-scoped), DeviceScope (the --devices argument,
generalized to a comma-joined multi-device list for a striped VG), and
EscapeDMName/RemoveOrphanedDMNodes (device-mapper's dash-escaping and
orphaned-node cleanup). All behind a Runner function type so callers
can test the identity logic without lvm2 or a kernel present.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq
boddumanohar added a commit that referenced this pull request Aug 26, 2026
Addresses PR review feedback:

- Inspector was a bad name once the type grew a Run method that
  executes arbitrary mutating commands, not just answers identity
  questions. Renamed to Manager, matching this repo's own
  nvmeof.Connector precedent (name the type after what it does).

- The package was too raw: every operation beyond the three identity
  checks (VolumeGroup, HasLogicalVolume, Rescan) required a caller to
  hand-build Run() argument lists -- exactly the duplication this
  package exists to prevent, and exactly what vdo.go had to do for
  pvcreate/vgcreate/lvcreate/vgchange/vgremove/lvextend/vgimportclone/
  lvrename. Added named, purpose-built methods for all of them
  (CreatePhysicalVolume, CreateVolumeGroup, CreateVDOLogicalVolume,
  Activate/DeactivateVolumeGroup, RemoveVolumeGroup,
  ImportClonedVolumeGroup, RenameLogicalVolume, ExtendPhysicalVolume,
  ExtendLogicalVolumeByFreeSpace, LogicalVolumeSize,
  ExtendLogicalVolumeToSize, SetVDOFeatures), following the shape
  openebs/lvm-localpv's pkg/lvm uses (named operations, not a raw
  command-line builder) minus the Kubernetes-CRD-shaped parts of that
  package, which don't belong in atlas-lib per this repo's own
  convention. Run stays as the escape hatch for anything not covered.

- Split the single lvm.go into one file per concern (lvm.go, identity.go,
  volume.go, vdo.go, clone.go, grow.go, dm.go), matching how nvmeof and
  nvme are already organized in this library, and matching the openebs
  package's own per-concern file layout.

vdo.go's adoption of this expanded surface is a follow-up commit on
PR #402.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq
boddumanohar added a commit that referenced this pull request Aug 26, 2026
Extracts the general, non-VDO-specific LVM plumbing out of
csi-driver/pkg/util/vdo.go (issue #277, PR #402) into a new shared
atlas-lib/lvm package, ahead of PR #456's pNFS design needing the same
class of LVM assembly (pvcreate/vgcreate/lvcreate --stripes) and the
same bugs VDO already found and fixed live: LVM's duplicate-PV
ambiguity between a volume's redundant HA device nodes, name-based
`vgs` lookups that lie about VG existence, and orphaned-VG detection
after a partial pvcreate+vgcreate-but-not-lvcreate.

lvm.Manager runs every LVM/dm-vdo command scoped to a fixed set of
devices, and answers device-content identity questions about them, all
behind a Runner function type (mirrors nvmeof.CommandRunner) so callers
can test the identity logic without lvm2 or a kernel present. One file
per concern, matching how nvmeof/nvme are already organized in this
library:

- identity.go: VolumeGroup (content-based PV -> VG identity lookup, not
  `vgs <name>`), ListLogicalVolumes, HasLogicalVolume (distinguishes a
  fully assembled stack from one orphaned by an interrupted create),
  Rescan (device-scoped `pvscan --cache`).
- volume.go: CreatePhysicalVolume, CreateVolumeGroup,
  ActivateVolumeGroup, DeactivateVolumeGroup, RemoveVolumeGroup.
- vdo.go: CreateVDOLogicalVolume, SetVDOFeatures.
- clone.go: ImportClonedVolumeGroup, RenameLogicalVolume (resolving a
  byte-level clone/snapshot restore's PV/VG UUID collision).
- grow.go: ExtendPhysicalVolume, ExtendLogicalVolumeByFreeSpace,
  LogicalVolumeSize, ExtendLogicalVolumeToSize.
- dm.go: EscapeDMName / RemoveOrphanedDMNodes (device-mapper's
  dash-escaping and orphaned-node cleanup, generalized from VDO's own
  fix for the same class of failure).
- lvm.go: Run, the escape hatch for anything not covered by a named
  method above, plus DeviceScope (the --devices argument, generalized
  to a comma-joined multi-device list -- VDO needs one device, a
  striped VG needs n).

vdo.go's adoption of this package is a follow-up commit on PR #402.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq
boddumanohar added a commit that referenced this pull request Aug 26, 2026
- Unexport DeviceScope -> deviceScope and EscapeDMName -> escapeDMName:
  neither has a caller outside this package (confirmed via grep across
  operator and csi-driver), so both were exposing an argument-building
  helper as public API rather than the named operation it exists to
  support.
- Add ExtendVolumeGroup (vgextend), the counterpart to CreateVolumeGroup
  for growing an existing VG's device membership -- flagged as a gap in
  grow.go, which otherwise only extends a PV or an LV, not the VG
  itself. Needed by a striped VG that grows by adding a member, not
  just by resizing one already in place.

ExtendLogicalVolumeToSize is used by csi-driver/pkg/util/vdo.go's
GrowVDO on PR #402 (growing the VDO logical volume to match its pool's
new size after ExtendLogicalVolumeByFreeSpace) -- not visible from this
PR's own diff since that adoption lives on the sibling branch.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq
noctarius pushed a commit that referenced this pull request Aug 28, 2026
Extracts the general, non-VDO-specific LVM plumbing out of
csi-driver/pkg/util/vdo.go (issue #277, PR #402) into a new shared
atlas-lib/lvm package, ahead of PR #456's pNFS design needing the same
class of LVM assembly (pvcreate/vgcreate/lvcreate --stripes) and the
same bugs VDO already found and fixed live: LVM's duplicate-PV
ambiguity between a volume's redundant HA device nodes, name-based
`vgs` lookups that lie about VG existence, and orphaned-VG detection
after a partial pvcreate+vgcreate-but-not-lvcreate.

lvm.Manager runs every LVM/dm-vdo command scoped to a fixed set of
devices, and answers device-content identity questions about them, all
behind a Runner function type (mirrors nvmeof.CommandRunner) so callers
can test the identity logic without lvm2 or a kernel present. One file
per concern, matching how nvmeof/nvme are already organized in this
library:

- identity.go: VolumeGroup (content-based PV -> VG identity lookup, not
  `vgs <name>`), ListLogicalVolumes, HasLogicalVolume (distinguishes a
  fully assembled stack from one orphaned by an interrupted create),
  Rescan (device-scoped `pvscan --cache`).
- volume.go: CreatePhysicalVolume, CreateVolumeGroup,
  ActivateVolumeGroup, DeactivateVolumeGroup, RemoveVolumeGroup.
- vdo.go: CreateVDOLogicalVolume, SetVDOFeatures.
- clone.go: ImportClonedVolumeGroup, RenameLogicalVolume (resolving a
  byte-level clone/snapshot restore's PV/VG UUID collision).
- grow.go: ExtendPhysicalVolume, ExtendLogicalVolumeByFreeSpace,
  LogicalVolumeSize, ExtendLogicalVolumeToSize.
- dm.go: EscapeDMName / RemoveOrphanedDMNodes (device-mapper's
  dash-escaping and orphaned-node cleanup, generalized from VDO's own
  fix for the same class of failure).
- lvm.go: Run, the escape hatch for anything not covered by a named
  method above, plus DeviceScope (the --devices argument, generalized
  to a comma-joined multi-device list -- VDO needs one device, a
  striped VG needs n).

vdo.go's adoption of this package is a follow-up commit on PR #402.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq
noctarius pushed a commit that referenced this pull request Aug 28, 2026
- Unexport DeviceScope -> deviceScope and EscapeDMName -> escapeDMName:
  neither has a caller outside this package (confirmed via grep across
  operator and csi-driver), so both were exposing an argument-building
  helper as public API rather than the named operation it exists to
  support.
- Add ExtendVolumeGroup (vgextend), the counterpart to CreateVolumeGroup
  for growing an existing VG's device membership -- flagged as a gap in
  grow.go, which otherwise only extends a PV or an LV, not the VG
  itself. Needed by a striped VG that grows by adding a member, not
  just by resizing one already in place.

ExtendLogicalVolumeToSize is used by csi-driver/pkg/util/vdo.go's
GrowVDO on PR #402 (growing the VDO logical volume to match its pool's
new size after ExtendLogicalVolumeByFreeSpace) -- not visible from this
PR's own diff since that adoption lives on the sibling branch.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq
boddumanohar added a commit that referenced this pull request Aug 28, 2026
Moves CreateOrAttachVDO, ResolveClonedVDO, DeactivateVDO, RemoveVDO,
and GrowVDO out of csi-driver/pkg/util/vdo.go into this package, as
CreateOrAttach, ResolveClone, Deactivate, Remove, and Grow. None of
these functions reference a Kubernetes or CSI type: they orchestrate
several lvm.Manager calls with judgment calls that are LVM/VDO-domain
(when to reactivate vs. recreate, when to fall back to
RemoveOrphanedDMNodes) rather than CSI-domain, so per this repo's own
atlas-lib placement rule -- a node-level primitive belongs here,
Kubernetes-shaped logic belongs in the consumer -- this is a better
fit than where it sat.

Also adds SetFeatures, a lvolID-keyed convenience wrapper over
UpdateVolume, and DevicePath, so every exported function in this file
is addressed by lvolID alone; the volume group/pool naming convention
(volumeGroupPrefix, poolName) stays internal to this package rather
than leaking to a caller.

ResolveClone is now a thin wrapper over ResolveClonedVolumeGroup
(already in lvm/clone.go) rather than its own copy of the
rescan/probe/import/rename sequence.

Two things changed in the move, both flagged in code review before
this landed:

- Logging: the original used k8s.io/klog directly. atlas-lib has no
  Kubernetes dependency anywhere, so this package exports Logger (a
  package-level *slog.Logger, nil-safe, matching the existing
  errs/deferrers.Logger precedent) instead.
- Timeouts: the original's per-call context.WithTimeout budgets
  (120s/300s) were csi-driver-owned constants. This package now owns
  them as its own defaults; a caller can still tighten via its own
  ctx deadline.

vdo.go's adoption of this package (deleting its own copies) is a
follow-up commit on PR #402.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq
boddumanohar added a commit that referenced this pull request Aug 28, 2026
Extracts the general, non-VDO-specific LVM plumbing out of
csi-driver/pkg/util/vdo.go (issue #277, PR #402) into a new shared
atlas-lib/lvm package, ahead of PR #456's pNFS design needing the same
class of LVM assembly (pvcreate/vgcreate/lvcreate --stripes) and the
same bugs VDO already found and fixed live: LVM's duplicate-PV
ambiguity between a volume's redundant HA device nodes, name-based
`vgs` lookups that lie about VG existence, and orphaned-VG detection
after a partial pvcreate+vgcreate-but-not-lvcreate.

lvm.Manager runs every LVM/dm-vdo command scoped to a fixed set of
devices, and answers device-content identity questions about them, all
behind a Runner function type (mirrors nvmeof.CommandRunner) so callers
can test the identity logic without lvm2 or a kernel present. One file
per concern, matching how nvmeof/nvme are already organized in this
library:

- identity.go: VolumeGroup (content-based PV -> VG identity lookup, not
  `vgs <name>`), ListLogicalVolumes, HasLogicalVolume (distinguishes a
  fully assembled stack from one orphaned by an interrupted create),
  Rescan (device-scoped `pvscan --cache`).
- volume.go: CreatePhysicalVolume, CreateVolumeGroup,
  ActivateVolumeGroup, DeactivateVolumeGroup, RemoveVolumeGroup.
- vdo.go: CreateVDOLogicalVolume, SetVDOFeatures.
- clone.go: ImportClonedVolumeGroup, RenameLogicalVolume (resolving a
  byte-level clone/snapshot restore's PV/VG UUID collision).
- grow.go: ExtendPhysicalVolume, ExtendLogicalVolumeByFreeSpace,
  LogicalVolumeSize, ExtendLogicalVolumeToSize.
- dm.go: EscapeDMName / RemoveOrphanedDMNodes (device-mapper's
  dash-escaping and orphaned-node cleanup, generalized from VDO's own
  fix for the same class of failure).
- lvm.go: Run, the escape hatch for anything not covered by a named
  method above, plus DeviceScope (the --devices argument, generalized
  to a comma-joined multi-device list -- VDO needs one device, a
  striped VG needs n).

vdo.go's adoption of this package is a follow-up commit on PR #402.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq
boddumanohar added a commit that referenced this pull request Aug 28, 2026
- Unexport DeviceScope -> deviceScope and EscapeDMName -> escapeDMName:
  neither has a caller outside this package (confirmed via grep across
  operator and csi-driver), so both were exposing an argument-building
  helper as public API rather than the named operation it exists to
  support.
- Add ExtendVolumeGroup (vgextend), the counterpart to CreateVolumeGroup
  for growing an existing VG's device membership -- flagged as a gap in
  grow.go, which otherwise only extends a PV or an LV, not the VG
  itself. Needed by a striped VG that grows by adding a member, not
  just by resizing one already in place.

ExtendLogicalVolumeToSize is used by csi-driver/pkg/util/vdo.go's
GrowVDO on PR #402 (growing the VDO logical volume to match its pool's
new size after ExtendLogicalVolumeByFreeSpace) -- not visible from this
PR's own diff since that adoption lives on the sibling branch.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq
boddumanohar added a commit that referenced this pull request Aug 28, 2026
Moves CreateOrAttachVDO, ResolveClonedVDO, DeactivateVDO, RemoveVDO,
and GrowVDO out of csi-driver/pkg/util/vdo.go into this package, as
CreateOrAttach, ResolveClone, Deactivate, Remove, and Grow. None of
these functions reference a Kubernetes or CSI type: they orchestrate
several lvm.Manager calls with judgment calls that are LVM/VDO-domain
(when to reactivate vs. recreate, when to fall back to
RemoveOrphanedDMNodes) rather than CSI-domain, so per this repo's own
atlas-lib placement rule -- a node-level primitive belongs here,
Kubernetes-shaped logic belongs in the consumer -- this is a better
fit than where it sat.

Also adds SetFeatures, a lvolID-keyed convenience wrapper over
UpdateVolume, and DevicePath, so every exported function in this file
is addressed by lvolID alone; the volume group/pool naming convention
(volumeGroupPrefix, poolName) stays internal to this package rather
than leaking to a caller.

ResolveClone is now a thin wrapper over ResolveClonedVolumeGroup
(already in lvm/clone.go) rather than its own copy of the
rescan/probe/import/rename sequence.

Two things changed in the move, both flagged in code review before
this landed:

- Logging: the original used k8s.io/klog directly. atlas-lib has no
  Kubernetes dependency anywhere, so this package exports Logger (a
  package-level *slog.Logger, nil-safe, matching the existing
  errs/deferrers.Logger precedent) instead.
- Timeouts: the original's per-call context.WithTimeout budgets
  (120s/300s) were csi-driver-owned constants. This package now owns
  them as its own defaults; a caller can still tighten via its own
  ctx deadline.

vdo.go's adoption of this package (deleting its own copies) is a
follow-up commit on PR #402.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq
boddumanohar added a commit that referenced this pull request Aug 28, 2026
Per review (the CHANGES_REQUESTED blocking this PR): every method that
named a device, a volume group, or a logical volume by bare string now
takes/returns one of three new value types instead:

- PhysicalVolume{DevicePath}
- VolumeGroup{Name}
- LogicalVolume{VolumeGroup, Name}

Passing a device path where a VG name belongs, or a VG where an LV is
expected, is now a compile error instead of an LVM failure discovered
at runtime. LogicalVolume carries its VolumeGroup rather than a bare
name so a caller pairing the wrong VG with an LV by hand is also a
type error, not a "volume group not found" surprise.

None of the three references a Manager: they are plain, comparable
values (== answers "same identity"), not handles, so the same value
works with any Manager instance -- keeping Manager itself stateless,
per the earlier stateless-contract discussion on this PR.

lvm/vdo's exported functions (CreateOrAttach, Deactivate, Remove, Grow,
ResolveClone, SetFeatures, DevicePath) keep their existing string-keyed
signatures unchanged -- VDO's own volume group/pool naming convention
already encapsulates the identity, so the typed values stay internal
to lvm/vdo/stack.go. csi-driver/pkg/util/vdo.go (PR #402) needs no
changes as a result: confirmed by rebuilding it against this commit's
atlas-lib/lvm with zero source edits.

Every call site across the package and its ~150 tests updated to
match; build/vet/test/lint/house-style gate all clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq
noctarius added a commit that referenced this pull request Aug 28, 2026
* feat(atlas-lib): add lvm package for device-scoped LVM commands

Extracts the general, non-VDO-specific LVM plumbing out of
csi-driver/pkg/util/vdo.go (issue #277, PR #402) into a new shared
atlas-lib/lvm package, ahead of PR #456's pNFS design needing the same
class of LVM assembly (pvcreate/vgcreate/lvcreate --stripes) and the
same bugs VDO already found and fixed live: LVM's duplicate-PV
ambiguity between a volume's redundant HA device nodes, name-based
`vgs` lookups that lie about VG existence, and orphaned-VG detection
after a partial pvcreate+vgcreate-but-not-lvcreate.

lvm.Manager runs every LVM/dm-vdo command scoped to a fixed set of
devices, and answers device-content identity questions about them, all
behind a Runner function type (mirrors nvmeof.CommandRunner) so callers
can test the identity logic without lvm2 or a kernel present. One file
per concern, matching how nvmeof/nvme are already organized in this
library:

- identity.go: VolumeGroup (content-based PV -> VG identity lookup, not
  `vgs <name>`), ListLogicalVolumes, HasLogicalVolume (distinguishes a
  fully assembled stack from one orphaned by an interrupted create),
  Rescan (device-scoped `pvscan --cache`).
- volume.go: CreatePhysicalVolume, CreateVolumeGroup,
  ActivateVolumeGroup, DeactivateVolumeGroup, RemoveVolumeGroup.
- vdo.go: CreateVDOLogicalVolume, SetVDOFeatures.
- clone.go: ImportClonedVolumeGroup, RenameLogicalVolume (resolving a
  byte-level clone/snapshot restore's PV/VG UUID collision).
- grow.go: ExtendPhysicalVolume, ExtendLogicalVolumeByFreeSpace,
  LogicalVolumeSize, ExtendLogicalVolumeToSize.
- dm.go: EscapeDMName / RemoveOrphanedDMNodes (device-mapper's
  dash-escaping and orphaned-node cleanup, generalized from VDO's own
  fix for the same class of failure).
- lvm.go: Run, the escape hatch for anything not covered by a named
  method above, plus DeviceScope (the --devices argument, generalized
  to a comma-joined multi-device list -- VDO needs one device, a
  striped VG needs n).

vdo.go's adoption of this package is a follow-up commit on PR #402.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq

* fix(atlas-lib/lvm): stop swallowing real command failures as not-found

VolumeGroup and HasLogicalVolume folded every pvs/lvs failure -- a
transient lock-contention or I/O error included -- into the same
"nothing found" result a genuinely blank device or empty VG produces.
The real adopting caller (csi-driver/pkg/util/vdo.go's
CreateOrAttachVDO) checks `err != nil` right after each call expecting
to catch exactly that class of failure, but the check was dead code:
a transient lvs error read as "orphaned VG, zero LVs" and would drive
CreateOrAttachVDO into vgremove -f against a real, valid volume.

VolumeGroup now only treats pvs's own "no PV signature" text as a
blank device (matching the text-based signal isAlreadyConnected reads
for nvme-cli elsewhere in atlas-lib) and propagates everything else.
HasLogicalVolume runs after VolumeGroup has already confirmed the VG
exists on this device, so an lvs failure there is never a legitimate
"empty VG" signal -- it now propagates unconditionally.

Also wraps DeactivateVolumeGroup's and RemoveVolumeGroup's errors with
operation/VG context, matching every sibling method in the file.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(atlas-lib/lvm): address review feedback on scope and coverage

- Unexport DeviceScope -> deviceScope and EscapeDMName -> escapeDMName:
  neither has a caller outside this package (confirmed via grep across
  operator and csi-driver), so both were exposing an argument-building
  helper as public API rather than the named operation it exists to
  support.
- Add ExtendVolumeGroup (vgextend), the counterpart to CreateVolumeGroup
  for growing an existing VG's device membership -- flagged as a gap in
  grow.go, which otherwise only extends a PV or an LV, not the VG
  itself. Needed by a striped VG that grows by adding a member, not
  just by resizing one already in place.

ExtendLogicalVolumeToSize is used by csi-driver/pkg/util/vdo.go's
GrowVDO on PR #402 (growing the VDO logical volume to match its pool's
new size after ExtendLogicalVolumeByFreeSpace) -- not visible from this
PR's own diff since that adoption lives on the sibling branch.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq

* refactor(atlas-lib/lvm): Extend -> Expand for the fixed-target grow ops

Per review: ExtendPhysicalVolume and ExtendLogicalVolumeByFreeSpace
always grow to a fixed target (the device's current full size, all
newly available free space) with no partial-amount use case, so
"Expand" fits better than "Extend." ExtendLogicalVolumeToSize and
ExtendVolumeGroup keep "Extend" -- both take an explicit target
(a byte size, a set of device paths) rather than growing to a fixed
endpoint.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq

* refactor(atlas-lib/lvm): unexport Runner -> runner

Per review: nothing outside this package injects a runner today, so
the type name itself doesn't need to be exported. NewManagerWithRunner
stays exported and still usable from outside the package regardless --
Go allows passing a matching func literal to a named func-type
parameter whether or not the type name is exported, so the seam for a
future consumer's tests is unchanged.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq

* Extracted vdo-specific code into its own sub-package

* Updated the parameter set

* quality gate run against atlas-lib/lvm

* fix(atlas-lib/lvm): fix CreateLogicalVolume dispatch and Volume construction

Three rough edges in the CreateLogicalVolume/LogicalVolumeDefinition
provisioning-handler design (introduced in 8908e47/e642b9af):

- CreateLogicalVolume hardcoded a lookup of volumeProvisioning["vdo"]
  rather than dispatching by asking each registered handler whether it
  Handles(def), so a second handler (striping, say) would never be
  reachable regardless of what it claimed to handle. Added Handles to
  the VolumeProvisioning interface (it existed only on the concrete
  vdo.volumeHandler, unreachable through the interface) and iterate
  registered handlers instead of hardcoding a key. Proven red first:
  a fake handler registered under a different name was silently
  ignored by the old hardcoded lookup.

- vdo.volumeHandler.Handles required both Compression and
  Deduplication (&&), while CreateVolumeArgs contributed flags for
  either one (||) -- harmless only because Handles was never actually
  called before this fix. Now that CreateLogicalVolume dispatches
  through it, the mismatch would have meant a compression-only or
  deduplication-only volume silently got no VDO flags at all. Fixed
  Handles to match CreateVolumeArgs's condition, test-first.

- vdo.Volume had every field unexported and no constructor, so nothing
  outside the vdo package (csi-driver, the actual intended caller of
  UpdateVolume) could construct one. Added vdo.NewVolume.

Also renamed CreateLogicalVolume's physicalVolume parameter to
poolName: it was never a physical volume, VDO's own call site already
passed its pool LV name ("vdopool") through it, and lvcreate's
<vg>/<pool> form only ever names a pool a handler creates alongside
the logical volume, never a device.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq

* feat(atlas-lib/lvm/vdo): move the VDO stack lifecycle from csi-driver

Moves CreateOrAttachVDO, ResolveClonedVDO, DeactivateVDO, RemoveVDO,
and GrowVDO out of csi-driver/pkg/util/vdo.go into this package, as
CreateOrAttach, ResolveClone, Deactivate, Remove, and Grow. None of
these functions reference a Kubernetes or CSI type: they orchestrate
several lvm.Manager calls with judgment calls that are LVM/VDO-domain
(when to reactivate vs. recreate, when to fall back to
RemoveOrphanedDMNodes) rather than CSI-domain, so per this repo's own
atlas-lib placement rule -- a node-level primitive belongs here,
Kubernetes-shaped logic belongs in the consumer -- this is a better
fit than where it sat.

Also adds SetFeatures, a lvolID-keyed convenience wrapper over
UpdateVolume, and DevicePath, so every exported function in this file
is addressed by lvolID alone; the volume group/pool naming convention
(volumeGroupPrefix, poolName) stays internal to this package rather
than leaking to a caller.

ResolveClone is now a thin wrapper over ResolveClonedVolumeGroup
(already in lvm/clone.go) rather than its own copy of the
rescan/probe/import/rename sequence.

Two things changed in the move, both flagged in code review before
this landed:

- Logging: the original used k8s.io/klog directly. atlas-lib has no
  Kubernetes dependency anywhere, so this package exports Logger (a
  package-level *slog.Logger, nil-safe, matching the existing
  errs/deferrers.Logger precedent) instead.
- Timeouts: the original's per-call context.WithTimeout budgets
  (120s/300s) were csi-driver-owned constants. This package now owns
  them as its own defaults; a caller can still tighten via its own
  ctx deadline.

vdo.go's adoption of this package (deleting its own copies) is a
follow-up commit on PR #402.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq

* feat(atlas-lib/lvm): typed PhysicalVolume, VolumeGroup, LogicalVolume

Per review (the CHANGES_REQUESTED blocking this PR): every method that
named a device, a volume group, or a logical volume by bare string now
takes/returns one of three new value types instead:

- PhysicalVolume{DevicePath}
- VolumeGroup{Name}
- LogicalVolume{VolumeGroup, Name}

Passing a device path where a VG name belongs, or a VG where an LV is
expected, is now a compile error instead of an LVM failure discovered
at runtime. LogicalVolume carries its VolumeGroup rather than a bare
name so a caller pairing the wrong VG with an LV by hand is also a
type error, not a "volume group not found" surprise.

None of the three references a Manager: they are plain, comparable
values (== answers "same identity"), not handles, so the same value
works with any Manager instance -- keeping Manager itself stateless,
per the earlier stateless-contract discussion on this PR.

lvm/vdo's exported functions (CreateOrAttach, Deactivate, Remove, Grow,
ResolveClone, SetFeatures, DevicePath) keep their existing string-keyed
signatures unchanged -- VDO's own volume group/pool naming convention
already encapsulates the identity, so the typed values stay internal
to lvm/vdo/stack.go. csi-driver/pkg/util/vdo.go (PR #402) needs no
changes as a result: confirmed by rebuilding it against this commit's
atlas-lib/lvm with zero source edits.

Every call site across the package and its ~150 tests updated to
match; build/vet/test/lint/house-style gate all clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Christoph Engelbert (noctarius) <me@noctarius.com>
boddumanohar added a commit that referenced this pull request Aug 28, 2026
…plementation

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.
@noctarius noctarius linked an issue Aug 31, 2026 that may be closed by this pull request
boddumanohar added a commit that referenced this pull request Aug 31, 2026
Tested against the real implementation (PR #402), not just raw LVM
commands: two real PVCs with clientCompression/clientDeduplication on the
same node, distinct checksummed data, node rebooted. Both VDO instances
reattached cleanly via fresh NodeStageVolume calls (kubelet's own
bookkeeping resets on reboot too) -- kvdo module usage count exactly 2, both
VDOOperatingMode normal, both checksums matched exactly.

Caveat found and documented: the two NodeStageVolume LVM command sequences
happened to complete sequentially rather than genuinely overlapping, so
LVM's internal command locking under truly concurrent vgchange/pvscan calls
remains unexercised -- narrowed the open item accordingly rather than
closing it outright.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
boddumanohar added a commit that referenced this pull request Aug 31, 2026
…ation

Tested both clone paths against PR #402's real ResolveClonedVDO: a direct
PVC-to-PVC clone and a snapshot restore, both scheduled onto the same node
as their still-live source (the specific co-location scenario this finding
warns about). Both correctly resolved via vgimportclone + lvrename, mounted
cleanly with data matching the source exactly, and coexisted with the
source and each other with independent VG identities and no
cross-contamination.

Also corrected the "Detection" section: the implementation ended up simpler
than originally planned -- detection is unconditional and purely
device-identity-based, not gated on VolumeContentSource, so no separate
content-source plumbing was needed.

Found and fixed one bug along the way: the collision-detection log message
was picking up pvs's stderr WARNING: lines merged into its output instead of
just the VG name -- harmless in this run, but fixed to parse cleanly.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
boddumanohar added a commit that referenced this pull request Aug 31, 2026
Deliberately reproduced against PR #402's real implementation: forcibly
disconnected a VDO volume's NVMe-oF subsystem at the host level while the
node stayed up, then deleted the pod. This exposed two real bugs (both now
fixed on that branch): DeactivateVDO had no fallback for an unreachable
device, and once added, the fallback's device-name matching didn't account
for device-mapper's dash-escaping and matched nothing. With both fixed,
cleanup is now fully automatic -- confirmed by reproducing the whole
sequence a second time.

Also documented an unplanned but valuable side observation: for ~19s after
disconnect, cached reads/writes silently appeared to succeed before the
real I/O failure surfaced, at which point VDO correctly fenced itself into
read-only mode and ext4 independently aborted its journal -- both layers
protected data correctly with no wiring needed from this design.

Narrowed the remaining open item: the node-NotReady-and-rejoin path is
still unverified (this test kept the node itself healthy throughout, only
the storage connection was severed).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
boddumanohar added a commit that referenced this pull request Sep 1, 2026
Tested against the real implementation (PR #402), not just raw LVM
commands: two real PVCs with clientCompression/clientDeduplication on the
same node, distinct checksummed data, node rebooted. Both VDO instances
reattached cleanly via fresh NodeStageVolume calls (kubelet's own
bookkeeping resets on reboot too) -- kvdo module usage count exactly 2, both
VDOOperatingMode normal, both checksums matched exactly.

Caveat found and documented: the two NodeStageVolume LVM command sequences
happened to complete sequentially rather than genuinely overlapping, so
LVM's internal command locking under truly concurrent vgchange/pvscan calls
remains unexercised -- narrowed the open item accordingly rather than
closing it outright.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
boddumanohar added a commit that referenced this pull request Sep 1, 2026
…ation

Tested both clone paths against PR #402's real ResolveClonedVDO: a direct
PVC-to-PVC clone and a snapshot restore, both scheduled onto the same node
as their still-live source (the specific co-location scenario this finding
warns about). Both correctly resolved via vgimportclone + lvrename, mounted
cleanly with data matching the source exactly, and coexisted with the
source and each other with independent VG identities and no
cross-contamination.

Also corrected the "Detection" section: the implementation ended up simpler
than originally planned -- detection is unconditional and purely
device-identity-based, not gated on VolumeContentSource, so no separate
content-source plumbing was needed.

Found and fixed one bug along the way: the collision-detection log message
was picking up pvs's stderr WARNING: lines merged into its output instead of
just the VG name -- harmless in this run, but fixed to parse cleanly.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
boddumanohar added a commit that referenced this pull request Sep 1, 2026
Deliberately reproduced against PR #402's real implementation: forcibly
disconnected a VDO volume's NVMe-oF subsystem at the host level while the
node stayed up, then deleted the pod. This exposed two real bugs (both now
fixed on that branch): DeactivateVDO had no fallback for an unreachable
device, and once added, the fallback's device-name matching didn't account
for device-mapper's dash-escaping and matched nothing. With both fixed,
cleanup is now fully automatic -- confirmed by reproducing the whole
sequence a second time.

Also documented an unplanned but valuable side observation: for ~19s after
disconnect, cached reads/writes silently appeared to succeed before the
real I/O failure surfaced, at which point VDO correctly fenced itself into
read-only mode and ext4 independently aborted its journal -- both layers
protected data correctly with no wiring needed from this design.

Narrowed the remaining open item: the node-NotReady-and-rejoin path is
still unverified (this test kept the node itself healthy throughout, only
the storage connection was severed).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
noctarius added a commit that referenced this pull request Sep 4, 2026
* Volume stack design iteration

* Use the American spelling "unparsable"

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) <noreply@anthropic.com>

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* 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.

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Manohar Reddy <manohar@simplyblock.io>
@noctarius
noctarius force-pushed the main branch 2 times, most recently from 60dceb7 to fbaabe4 Compare September 9, 2026 10:21
…#277)

Implements design-issue-277-client-side-compression.md against current main
rather than rebasing the abandoned PR #402 branch, which predates the
csi-driver restructuring (#497), the v1alpha2 CRD redesign, and the
SimplyblockDriver-managed DaemonSet (#513). Built on atlas-lib/volstack's
three LVM layers per the user's request, in place of PR #402's flat
atlas-lib/lvm/vdo package, which retires here with zero importers.

- atlas-lib/lvm: a built-in "vdo" VolumeProvisioning handler, registered by
  the package's own init (the registry existed with nothing real registered
  against it — CreateLogicalVolume silently dropped compression/deduplication
  before this). Manager.ForgetDevice/HasOrphanedDMNodes, new lvmdevices/
  dmsetup primitives.
- atlas-lib/volstack/layers: lvmPhysicalVolume, lvmVolumeGroup, and
  lvmLogicalVolume now tolerate total path loss (the member device gone
  entirely, not merely unreadable) without erroring the whole Down walk;
  lvmVolumeGroup.Release forgets stale system.devices entries (folds in the
  still-open PR #545).
- atlas-lib/kube: client_compression/client_deduplication StorageClass
  parameters, storage.simplyblock.io/vdo-capable label and its managed-by
  annotation, the shared vdo-capable marker path.
- operator: VolumeDefaults.EnableClientCompression/EnableClientDeduplication
  (v1alpha2), threaded through ClassParameters and the v1alpha1 conversion's
  hub-only stash; the csi-node DaemonSet's postStart hook probes dm-vdo
  alongside its existing nvme-tcp/nvme-rdma modprobes (no hostPID needed —
  the existing probes already prove that), with RBAC to self-label.
- csi-driver: vdoCapableSegment (twin of dhchapAllowedNodeSegment) pins PV
  nodeAffinity from CreateVolume, deliberately not via StorageClass
  AllowedTopologies — that mechanism was found broken for DHCHAP and removed
  in PR #484, for a reason that applies identically to a self-probed
  capability label. A rawDeviceLayer adapter lets the three LVM layers run
  through the real volstack.Runner without adopting fabric/filesystem
  (Phase 1, unrelated, unwired work), wired into NodeStageVolume/
  NodeUnstageVolume/restageVolume/NodeExpandVolume only for volumes that
  request either parameter.
- Dockerfile_base: the vdo package (vdoformat), x86_64 only.

Deviations from the merged design are called out inline in the doc's
2026-09-16 revision notes (§5, §7.1, §4.1, Q9).

Not implemented, matching the design's own already-open items: the
zero-capable-node pool event (§5.1, Q13), CSI metrics (§13, Q6), the
SetFeatures live-toggle path (Q1), the PVC-size floor admission webhook (Q2,
despite the doc's stale "Resolved" note — no such webhook exists on main),
and periodic capability re-checking (Q12).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@boddumanohar
boddumanohar force-pushed the issue-277-client-side-compression-impl branch from b865db3 to 91dca6d Compare September 16, 2026 09:08
boddumanohar and others added 6 commits September 16, 2026 11:27
- goconst: extract the repeated "true" literal into vdoCapableTrue in both
  csi-driver/internal/csi/controller and csi-driver/internal/csi/node.
- lll: shorten two over-length lines in stage.go and driver.go.
- go.mod: run go mod tidy in operator to move k8s.io/utils to a direct
  requirement, following k8s.io/utils/ptr's use in assignment_test.go.
- Trim several over-long comments to their point.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Verified live on a real Rocky Linux 9.5 host (kernel 5.14.0-687.39.1.el9_8,
vdo + kmod-kvdo installed, lvm segtypes lists vdo/vdo-pool): it has no
in-tree dm-vdo at all, but modprobe kvdo loads a working VDO target from
kmod-kvdo's weak-updates tree. Probing dm-vdo alone would have reported this
host, and likely most RHEL9-family nodes, as not VDO-capable despite being
fully able to run it.

The postStart hook now tries modprobe dm-vdo first, then kvdo; capable on
either succeeding, not capable (no install attempted) if neither does.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Verified live: mkfs.xfs and mke2fs both discard the whole device by
default, and on a VDO volume that is not free the way it is on the plain
backend device. Formatting the same 20G VDO volume:

  mkfs.xfs   with discard: 11.5s   without (-K):        0.13s  (~87x)
  mke2fs     with discard: 12.08s  without (-E nodiscard): 0.09s (~128x)

VDO's block map has to process a discard proportionally to the volume's
size; a freshly created volume has nothing on it worth discarding in the
first place. This was the dominant cost in NodeStageVolume for a VDO
volume (86.7s of a ~90s total on a real 100G volume end to end), not VDO's
data path itself (the lvcreate/vgcreate/pvcreate sequence took ~3s for the
same volume).

FormatOptions now skips discard for both filesystems whenever a VDO stack
is in play, alongside the existing stripe-alignment skip for XFS.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A PVC requesting client-side compression or deduplication below VDO's
own ~4.72GiB floor previously only failed inside lvcreate at
NodeStageVolume time, surfacing as an opaque stage error. The new
VDOSizeFloorValidator resolves the PVC's StorageClass and rejects the
request at admission time when it is under 5GiB, so the error is
immediate and readable instead of late.

failurePolicy=Ignore, unlike the pinned-volume validator's Fail: an
operator outage should fall back to today's behavior (a late lvcreate
failure) for VDO volumes only, not block every PVC creation cluster-wide.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds SPDKCSI-VDO: two live specs that read vdostats inside the csi-node
pod hosting the volume and assert physical usage grows far less than
logical usage — one with client_compression only (a highly compressible
payload), one with client_deduplication only (nine duplicate copies of
an incompressible blob) — so a regression that silently drops the VDO
layer fails here even though every other suite would still pass.

Also regenerates operator/dist/install.yaml via `make build-installer`,
missed in the previous commit and caught by CI's Operator: Manifests
drift check.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Confirmed live: vdostats 8.3.2.1 intermittently reports "Not a valid
running VDO device" for a target given as a full /dev/mapper/<name>
path, even while dmsetup reports that same mapping ACTIVE/LIVE, but
resolves the identical target reliably by its bare dmsetup name.

Also tightens the pool-device match to the "-vpool" suffix: a lvol's
VDO stack always shows two /dev/mapper entries (the pool device and
its backing data volume, "...-vdopool_vdata"), and a plain "vdopool"
substring match returned both, breaking the vdostats invocation.

Both SPDKCSI-VDO specs now pass against a live cluster.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread csi-driver/internal/csi/node/expand.go Outdated
// Grows the pool and the logical volume to the raw device's now larger
// physical size, ahead of the filesystem resize below, which still
// targets devicePath: the VDO device the filesystem actually sits on.
if err := ns.vdo.Grow(ctx, vdoLvolID(volumeID), rawDevicePath, compression, deduplication); err != nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this specifically called vdo? Shouldn't it be a general "layer" access, independent of what the stack is? Typically, the stack (after bring-up) knows its own configuration from the stack state file. Hence, it would know how the vdo layer is configured 🤔

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at the moment, the volume stack isn't used generally for nvmeOf and filesystem yet. this code path only runs when a volume asks for compression or dedup. And everything else with volume connect and mount still happens the old way without going through the stack at all.

To make it generic like you're describing, every volume (not just VDO ones) would need to go through the same stack mechanism. Which is a separate change on its own.

@@ -0,0 +1,228 @@
// Client-side compression and deduplication (issue #277): a VDO device

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that a pretty much full duplication of the vdo layer on the volume stack? I don't think this is a good idea. It's asking for drift and I don't see a reason why we would actually need it here, rather than asking the volstack package to read the current stack config.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Down and Grow were passed the parameters: compression/deduplication but never actually used them. So removed them in here. But having the stack just remember its own settings instead of being told them each time isn't something the stack can do yet.

volumeLocks: csicommon.NewVolumeLocks(),
kubeClient: kubeClient,
manager: manager,
vdo: newVDOStack(vdoStackRecordDir),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if caching the vdo stack is a good idea.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I din't understand this. What do you mean by caching here?

klog.Errorf("failed to lookup volume context, volumeID: %s err: %v", volumeID, err)
return nil, status.Error(codes.Internal, err.Error())
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every volume stack needs to be shut down, not just vdo stack. This needs to be a general wiring.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same point as the earlier. in general it should. But that's making every volume go through the stack, which is its own separate change.

}()

rawDevicePath := devicePath
if compression, deduplication, wantsVDO := vdoParams(vc); wantsVDO {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every volume stack needs to be brought up, not just vdo.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

// The same Up a fresh stage runs: Ensure only reactivates an existing
// stack (pvscan --cache, vgchange -ay), never lvcreate, because the data
// already exists.
devicePath, err = ns.vdo.Up(ctx, vdoLvolID(volumeID), rawDevicePath, compression, deduplication)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Every volstack needs to be brought up.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

// parameters carried in the volume context.
func FormatOptions(fsType string, volumeContext map[string]string) []string {
if fsType != "xfs" {
//

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in the volstack/layers/filesystem layer which handles the different format options already.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this volume's format still goes through the old internal/mount path. So didn't touch anything from that side as a part of this PR.

vdoStack.Down and Grow threaded compression/deduplication into
plan() purely to build the LVMLogicalVolume layer's Definition, but
neither Release (Down's path) nor Grow ever reads Definition — only
Ensure's create(), reached exclusively when the logical volume does
not exist yet (a fresh Up), does. Passing real values into Down/Grow
was dead weight, not merely unused: a caller-review comment on PR #402
flagged it as duplication that invites drift between what a volume
was actually created with and what a later call re-derives.

Up is unchanged: it still takes compression/deduplication, since
Ensure's create() genuinely needs them.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Three new SPDKCSI-VDO specs, reusing existing helpers (createPVC,
createPodForPVC, deploySnapshotOnly/waitForSnapshotReady, resizePVC/
waitForPVCStorageCapacity/waitForFilesystemSize) rather than new
templates:

- a volume restored from a snapshot of a VDO volume gets its own
  active VDO device and keeps the source's data
- a volume cloned from a VDO volume gets its own active VDO device
  and keeps the source's data
- a VDO volume can be expanded online: both the VDO pool's physical
  capacity and the filesystem grow, and data survives

All three confirmed manually first (source, snapshot-restore, and
clone volumes each got fully independent, correctly named VDO
devices; resize grew the VG, the VDO pool, and the filesystem, with
data intact throughout) before being written up as automated specs.

The snapshot spec needed an explicit sync after writing the source's
data marker: unlike SPDKCSI-SNAPSHOT's own spec, this one keeps the
source pod mounted through the snapshot, so the write needs its own
flush rather than getting one for free from unmounting.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Client-side Compression

2 participants