feat: client-side compression/dedup via VDO (issue #277) - #402
Draft
boddumanohar wants to merge 12 commits into
Draft
feat: client-side compression/dedup via VDO (issue #277)#402boddumanohar wants to merge 12 commits into
boddumanohar wants to merge 12 commits into
Conversation
Implements the design in design-issue-277-client-side-compression.md (PR #398): new clientCompression/clientDeduplication Pool StorageClass params, VDO-capable node topology gating, a new csi-driver/pkg/util/vdo.go managing per-volume VDO stacks over LVM, and nodeserver.go wiring to create/reattach/grow/remove VDO devices across stage/unstage/restage/expand. Not yet covered (tracked as follow-ups): unit tests, deliberate exercise of clone/snapshot VDO resolution, multi-instance+reboot, XFS-on-VDO, and crash-consistency of async write policy. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
goconst nudged a look at nodeserver.go's raw == "true" checks against client_compression/client_deduplication: mergeStorageClassParameters actually emits "True" (capitalized), so those checks would never have matched. Replaced with kube.BoolParam (the same parser already used for encryption/replicate), factored into a shared vdoParams helper. Also regenerated dist/install.yaml (missed by the earlier make manifests run) and fixed a goconst/unparam/lll nit in vdo.go and simplyblockpool_controller.go. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Found live on the test cluster: CSINode's topology key set is captured once
at CSI plugin registration (seconds after pod start), while
advertiseVDOCapability patches the underlying node label asynchronously in
the background (can take minutes, gated on the postStart hook's dnf
install). Conditionally omitting the vdo-capable key when false meant it was
essentially never present at registration time, permanently breaking the
topology gate for that node until its csi-node pod restarted -- confirmed by
a real ProvisioningFailed error ("topology ... is not in requisite") when
provisioning against a Pool with clientCompression/clientDeduplication
enabled. Now always present, matching the existing
topologyKeyStorageNodeUUIDPrefix pattern's own documented rationale: only
the key's presence needs to be stable, not its value, since
external-provisioner reads the live Node label value fresh on every
provision.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pkg/util/vdo.go execs pvcreate/vgcreate/lvcreate/vgchange/lvextend/dmsetup etc. from inside the csi-node container, but base_image only ships nvme-cli/e2fsprogs/xfsprogs -- confirmed live on the test cluster (MountVolume.MountDevice failing with "pvcreate: executable file not found in $PATH"). Added to this branch-tagged Dockerfile rather than base_image/Dockerfile_base, since that image tag is shared across every branch and rebuilding it would affect unrelated in-flight work; worth promoting there once this feature lands. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…iner) Confirmed live: lvcreate --type vdo failed with "device not cleared, Aborting. Failed to wipe start of new LV" -- device-mapper's default behavior waits on udev to create/settle the resulting device node, but this container has no udev daemon running. DM_DISABLE_UDEV=1 is the standard fix for LVM tooling run inside a container. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
lvm2 alone wasn't enough: lvcreate --type vdo shells out to vdoformat
internally to format the new VDO pool, and that binary ships in the separate
vdo package, not lvm2. Confirmed live ("/usr/bin/vdoformat: execvp failed:
No such file or directory").
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The multi-arch build was failing entirely on the arm64 leg: "No match for argument: vdo" -- Oracle Linux 9's configured repos don't carry a vdo build for aarch64. Client-side VDO is x86_64-only for now anyway (matches every host used in this design's validation), so gate the install on TARGETARCH rather than block the whole multi-arch image. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Critical bug caught live: deleting and recreating a pod on the same node (same PVC) came back with an empty filesystem -- all test data gone. NodeUnstageVolume was calling RemoveVDO (vgchange -an + vgremove -f) on every routine unstage, but NodeUnstageVolume fires any time no pod on this node currently needs the volume mounted, not only when the volume is actually being deleted. vgremove destroys the VG's LVM metadata, which is what makes VDO's compressed/deduplicated physical layout decodable back into the original file bytes -- so this was silently destroying user data on an entirely ordinary pod restart, directly contradicting the design's own "must be re-included every time the node-side CSI driver re-provisions the volume" requirement. Added DeactivateVDO (vgchange -an only, non-destructive, reversible via CreateOrAttachVDO's existing vgchange -ay reactivate path) and wired NodeUnstageVolume to use it instead. RemoveVDO is kept for genuine destroy/cleanup scenarios, just no longer called from the routine unstage path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Confirmed live: lvextend -l100%FREE failed with "New size given (1024 extents) not larger than existing size (1535 extents)" during a real PVC expand (6Gi -> 10Gi). Unlike lvcreate, lvextend's bare "100%FREE" is an absolute target (100% of currently-free space alone), not "grow by" -- after the backend resize, free space (1024 extents) was smaller than the pool's current size (1535 extents), so the absolute interpretation rejected it as not larger. The "+" prefix makes it additive (current size + free space), which is the actual intent. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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>
Confirmed live during a real clone test: ResolveClonedVDO's own log message
came out polluted with LVM's duplicate-PV warnings ("WARNING: Not using
device /dev/nvme2n1 for PV ...") ahead of the actual VG name, because
runLVMCommand merges stdout+stderr and pvVGName trusted the whole trimmed
blob. Didn't cause a functional problem in that run (the dirty string still
correctly differed from the target VG name), but is a latent correctness
risk and produced a confusing log line. Now takes the first non-empty,
non-WARNING line instead.
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>
Critical gap confirmed live: forcibly disconnected a VDO volume's NVMe-oF connection at the host level (storage-side disconnect while the node stays up), then deleted the pod. NodeUnstageVolume's DeactivateVDO call failed with "Volume group ... not found" on every one of 18 retries -- the exact same failure mode vgremove hits in RemoveVDO when the backing device is gone, since vgchange -an also needs to read/write VG metadata that no longer exists. Kubelet eventually force-removed the pod anyway, leaving the orphaned dm-vdo stack permanently stuck with nothing left to clean it up -- my earlier fix (switching NodeUnstageVolume from the destructive RemoveVDO to the safe DeactivateVDO) accidentally dropped the dmsetup fallback robustness RemoveVDO already had for this exact case. DeactivateVDO now falls back to the same direct dmsetup removal when vgchange -an fails with a "not found"-style error (mirroring vgExists' existing pattern-matching), gated on that specific failure signature so a genuinely busy/in-use VG (device still reachable) is never forced. Kernel logs from the same test also confirmed VDO's own fencing behavior worked correctly and safely: once the disconnected device's I/O actually started failing (~19s after disconnect, buffered writes had masked it until then), VDO fenced itself into read-only mode and ext4 aborted its journal, rather than corrupting data. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Confirmed live: the dmsetup fallback added for DeactivateVDO correctly
triggered ("vgchange -an failed ... falling back"), but matched zero dm
device names and left the orphaned stack in place. device-mapper flattens
"<vg>-<lv>" into a single dm name by doubling every literal "-" within the
VG/LV name components (e.g. vg "vdo-<uuid-with-dashes>" becomes
"vdo--<uuid-with-double-dashes>" in dmsetup ls output) -- the prefix match
was comparing against the unescaped VG name, which never matches. Escapes
the VG name the same way device-mapper does before matching.
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the design in
operator/docs/designs/design-issue-277-client-side-compression.md(PR #398) and its test plan inoperator/docs/tests/test-plan-issue-277-client-side-compression.md, closing issue #277.Pool.spec.storageClassParameters.clientCompression/clientDeduplicationfields, independently switchable, mapped through toclient_compression/client_deduplicationStorageClass parameters.upsertStorageClassnow composes a VDO-capable topology requirement (simplyblock.io/vdo-capable=true) with the existing DHCHAP topology gate when either client-side parameter is set.csi-driver/pkg/util/vdo.go:CreateOrAttachVDO,ResolveClonedVDO,DeactivateVDO,RemoveVDO(with admsetup-based fallback for the orphaned-stack case documented in the design's spike log),GrowVDO,SetVDOFeatures. Each volume gets its own PV/VG/vdo-pool/LV stack via LVM (lvcreate --type vdo).nodeserver.gowiring:NodeStageVolumecreates/attaches VDO betweeninitiator.Connectand mount;NodeUnstageVolumedeactivates it before disconnecting the raw device;restageVolumereattaches (never recreates) on reconnect;NodeExpandVolumegrows the VDO stack before the filesystem resize;xfsStripeOptionsis skipped once VDO is in play;buildAccessibleTopologysurfaces the new node label.kmod-kvdo+vdoviansenter(addedhostPID: true), writing a marker file;newNodeServerreads it in the background and patches the node'ssimplyblock.io/vdo-capablelabel via a new RBACpatch/updategrant onnodes.csi-driver/deploy/image/Dockerfilenow installslvm2+vdo(amd64 only) and disables udev sync for LVM commands, both required to actually run LVM/VDO tooling from inside this container.Verified end-to-end on the live test cluster
Created a real
Poolwith bothclientCompressionandclientDeduplicationenabled, provisioned a PVC/Pod through the resulting StorageClass, and confirmed:client_compression/client_deduplication = "True"and anallowedTopologiesrequiringvdo-capable=true.kmod-kvdobuild (the other 3 nodes correctly advertisevdo-capable=false)./dev/mapper/vdo-<lvol-uuid>-...), withVDOCompression/VDODeduplicationbothenabled(lvs).vdostatssaving percent.NodeStageVolumecalls (kvdomodule usage count exactly 2, bothVDOOperatingMode: normal, both checksums matched). Caveat: the two stage sequences happened to run sequentially rather than genuinely overlapping, so LVM's internal locking under truly concurrentvgchange/pvscancalls remains unexercised.ResolveClonedVDOcorrectly ranvgimportclone+lvrename, both mounted cleanly with data matching the source exactly, and all three volumes (source, clone, restore) coexisted with independent VG identities and no cross-contamination.nvme disconnect) while the node and pod both stayed up, simulating "storage side disconnects while the node stays up." Kernel logs showed buffered reads/writes silently appearing to succeed for ~19s before the real failure surfaced, at which point VDO correctly fenced itself into read-only mode (and ext4 independently aborted its journal) rather than corrupting anything — Kubernetes reported the pod as healthy the whole time regardless. Deleting the pod afterward is now confirmed to clean up fully automatically, with no orphaned state left behind and no manual intervention needed.fsType=xfs(every prior spike usedext4) —mkfs.xfsran with no stripe-alignment flags (confirming thexfsStripeOptionsskip fires correctly), mounted cleanly with the existingnouuidflag intact, compression/dedup stayed enabled, and reattach-on-recreate worked identically toext4. No bugs found.asyncwrite policy: forcedvdo_write_policy=asyncexplicitly, wrote one file withfsync()and one without, then genuinely crashed the node viasysrq(immediate reboot, zero filesystem sync — confirmed via a new boot timestamp, not a graceful reboot that would have proven nothing). Thefsync()'d file survived with an exact checksum match; the non-fsync()'d file was lost entirely — the correct POSIX outcome. Resolves the design doc's open safety question:asynccorrectly honors flush/FUA durability end-to-end through NVMe-oF to the simplyblock backend.This process found and fixed nine real, hands-on-only-discoverable bugs beyond the original design (each is its own commit on this branch):
buildAccessibleTopologyonly added thevdo-capabletopology key when the label was alreadytrue— since CSINode's topology key set is captured once at plugin registration (seconds after pod start) while the label gets patched asynchronously afterward, the key was essentially never present at registration time, permanently breaking the topology gate until pod restart.spdkcsicontainer image had nolvm2installed at all (pvcreate: not found).lvm2alone wasn't enough —lvcreate --type vdoshells out tovdoformat, which ships in the separatevdopackage.lvcreate --type vdofailed with "device not cleared" — no udev daemon runs inside this container, so device-mapper's default udev-sync handshake never completes. Fixed withDM_DISABLE_UDEV=1.NodeUnstageVolumewas calling the destructiveRemoveVDO(vgremove) on every routine unstage, not only when the volume was actually being deleted — an ordinary pod delete+recreate on the same node silently destroyed all VDO-backed data. Fixed by adding a non-destructiveDeactivateVDO(vgchange -anonly) for this path.GrowVDO'slvextend -l100%FREEused the absolute (not additive) percentage form, which computed a target smaller than the pool's current size on every real device resize. Fixed with-l+100%FREE.pvVGNametrustedpvs's combined stdout+stderr output wholesale, so a duplicate-PVWARNING:line from a real clone test polluted both the identity comparison and the resulting log message. Harmless in that run (the dirty string still differed from the target either way), but fixed to parse just the actual field.DeactivateVDO(the non-destructive replacement from bug updated the api create param to include cr objects #5) had no fallback at all for an unreachable backing device —vgchange -anfailed identically to howvgremovefailed in the original spike, on every one of 18 retries, until kubelet gave up and force-removed the pod anyway, leaving the orphaned dm-vdo stack permanently stuck. Fixed by adding the samedmsetup removefallbackRemoveVDOalready had.vdo-<uuid>→vdo--<uuid-with-double-dashes>indmsetup lsoutput), so it matched nothing and the stack stayed orphaned even with the fallback wired in. Fixed to escape the VG name the same way device-mapper does before matching.Deliberate scope decisions (see design doc for full rationale)
ResolveClonedVDO's clone-collision detection is driven purely by the device's actual on-disk VG identity, not by threadingVolumeContentSourcethrough the volume context as the design doc originally proposed — simpler and correct either way, so no new plumbing was added.GrowVDO's signature grows to the pool's new physical capacity (-l100%FREE/+100%FREE, then matching logical size) rather than taking an explicitnewSizeparameter, matching the100%FREEconvention already used at creation time.csi-driver/pkg/util/vdo.goper the design doc, notatlas-lib(raised as an idea mid-discussion, never confirmed).lvm2/vdoare installed in this branch-taggedDockerfile, not the sharedbase_image/Dockerfile_base— rebuilding that shared, cross-branch tag for an in-progress feature felt like the wrong blast radius; worth promoting there once this lands.vdohas no aarch64 build in the configured repos, so it's installed amd64-only; client-side VDO is x86_64-only for now, matching every host used in this design's validation.New finding, not yet fixed (separate, pre-existing gap)
The CSI driver's
CreateVolumedoes not populatePersistentVolume.spec.nodeAffinity, so once a PVC is bound, a pod using it can be rescheduled to any node — not just the one the topology gate originally selected. This was harmless before (a raw NVMe-oF connection works identically from any node) but is now materially important, since VDO state is node-local. Reproduced live: deleting and recreating the pod (not the PVC) let it land on a non-vdo-capablenode, where it correctly failed to mount. Worked around for this verification by pinning the test pod'snodeSelector; not fixed here since it's a pre-existing driver gap orthogonal to this feature (same category as #401). Filing a follow-up issue.Explicitly out of scope for this PR
SetVDOFeatures(live compression/dedup toggle) is implemented but not wired into any update path — v1 non-goal per the design doc.upsertStorageClasscreate-only bug (bug: upsertStorageClass is create-only — Pool StorageClassParameters edits on an existing Pool silently no-op #401) — orthogonal, tracked separately.NodeUnstageVolumeon the original node specifically after that node goes NotReady and later rejoins (as opposed to the connection just being severed while the node stays healthy, which is what was tested) remains unverified — a different code path.vgchange/pvscancalls actually racing at the LVM level remain unexercised — the multi-instance reboot test's two stage sequences happened to run sequentially rather than overlapping.All items originally flagged as open in the test plan (XFS-on-VDO, crash-consistency of
async, clone/snapshot resolution, multi-instance-across-a-reboot, stale-state-after-unclean-disconnect) are now verified — see above.ensureDeviceConnected(block-volume reconnect) is untouched — every VDO example in the design is filesystem-mode; block-mode + VDO was never designed or tested.NodeExpandVolume/GrowVDOisn't fully idempotent against a redundant re-invocation after the volume is already at its target size — logs a scary-looking but harmless error on kubelet's post-success reconciliation retry. Worth a follow-up polish pass.Why draft
Unit tests from the test plan are not written yet. Draft status reflects that, not any doubt about the functional behavior above — that part is now hands-on verified against a real cluster, real NVMe-oF-backed lvols, and real compression/dedup savings.
Test plan
go build ./...clean for bothoperatorandcsi-drivermodulesgo test ./...—pkg/spdk,pkg/utilpass;internal/controller's envtest-based suite requires a localkubebuilderbinary not present in this environment (pre-existing, unrelated);e2epackage requires a live e2e cluster fixture (pre-existing, unrelated)vdo.goand the new wiring (test plan Sections 1-7)🤖 Generated with Claude Code