feat: client-side compression/dedup via VDO (issue #277) - #402
boddumanohar wants to merge 9 commits into
Conversation
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>
…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>
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>
…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>
#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>
…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>
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>
…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>
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>
88d5be8 to
d201b02
Compare
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>
…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>
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>
6d47771 to
da1673e
Compare
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
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
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
- 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
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
- 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
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
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
- 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
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
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
* 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>
…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.
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>
…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>
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>
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>
…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>
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>
* 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>
60dceb7 to
fbaabe4
Compare
…#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>
b865db3 to
91dca6d
Compare
- 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>
| // 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 { |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
Not sure if caching the vdo stack is a good idea.
There was a problem hiding this comment.
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()) | ||
| } | ||
|
|
There was a problem hiding this comment.
Every volume stack needs to be shut down, not just vdo stack. This needs to be a general wiring.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Every volume stack needs to be brought up, not just vdo.
| // 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) |
There was a problem hiding this comment.
Same here. Every volstack needs to be brought up.
| // parameters carried in the volume context. | ||
| func FormatOptions(fsType string, volumeContext map[string]string) []string { | ||
| if fsType != "xfs" { | ||
| // |
There was a problem hiding this comment.
This should be in the volstack/layers/filesystem layer which handles the different format options already.
There was a problem hiding this comment.
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>
Summary
Client-side compression/deduplication (issue #277), built on
atlas-lib/volstack's layered LVM stack.Design
Node-side stack —
csi-driver/internal/csi/node/vdo.godrives threevolstacklayers with the realvolstack.Runner, behind a thin adapter standing in for thefabriclayer (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]Wired into
NodeStageVolume/NodeUnstageVolume/restageVolume/NodeExpandVolume, only for volumes that ask for either parameter.VolumeDefaults.EnableClientCompression/EnableClientDeduplication(v1alpha2), passed through toclient_compression/client_deduplicationStorageClass parameters.vdoCapableSegmentpinsPersistentVolume.spec.nodeAffinityto avdo-capablenode fromCreateVolume, mirroringdhchapAllowedNodeSegment.postStarthook probes bothdm-vdoandkvdo(kernel 6.9+ ships the former in-tree; RHEL/Rocky 9-family kernels only ship the latter viakmod-kvdo) alongside its existingnvme-tcp/nvme-rdmamodprobes, and self-labels the node only if either succeeds; RBAC widened topatchonnodes.mount.FormatOptionsskips mkfs's default full-device discard on a VDO volume (-Kformkfs.xfs,-E nodiscardformke2fs) — VDO's block map processes a discard proportionally to volume size, where a plain backend device does not.VDOSizeFloorValidatorwebhook rejects a PVC asking for either parameter below VDO's own ~5GiB floor at creation time, instead of that request only failing later, insidelvcreate, atNodeStageVolume.Dockerfile_baseinstalls thevdopackage (amd64 only).atlas-lib/lvm/vdo(the old flat package) retired in favor of thevolstacklayers. Along the way, found it had never actually registered a VDOVolumeProvisioninghandler — a real, separate bug whereCreateLogicalVolumesilently dropped compression/deduplication.volstackteardown before it ever reached the device-mapper force path. AddedManager.HasOrphanedDMNodesand taught the three LVM layers to tolerate it.csi-driver/e2e/vdo.goreadsvdostatsinside 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).
NodeStageVolume, 100GB volume, first formatNodeStageVolume, 20GB volume, first formatSPDKCSI-VDO)E2E testing
Ran a full e2e test by skipping the multi cluster tests.