feat(atlas-lib): add lvm package for device-scoped LVM commands by boddumanohar · Pull Request #457 · simplyblock/simplyblock-operator

boddumanohar · 2026-08-25T13:30:18Z

Summary

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.

What's in the package

lvm.Manager runs every LVM/dm-vdo command scoped to a fixed set of devices, and answers device-content identity questions about them. Named, purpose-built methods, one file per concern:

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

All behind a Runner function type (mirrors nvmeof.CommandRunner), so callers can test the identity logic without lvm2 or a kernel present.

Run stays as the escape hatch for anything the named surface doesn't cover yet.

Test plan

go build ./..., go vet ./..., go test ./... — all green across every atlas-lib module
golangci-lint run ./lvm/... — 0 issues
House-style gate clean (one justified exception: the package doc comment's required // Package lvm ... form, which the terminology checker can't distinguish from prose)
Adoption in vdo.go (PR feat: client-side compression/dedup via VDO (issue #277) #402) — follow-up commit on that branch, now pushed

vdo.go's device-scoping and content-based identity logic (devicesArgs/vgExists/vgHasLV/pvVGName/runLVMCommand/removeOrphanedDMNodes) was never VDO-specific -- it's a general fix for LVM's default behavior breaking against a simplyblock HA volume's two redundant local device nodes. Extracted to github.com/simplyblock/atlas/lvm (PR #457) so a future pNFS striped-volume implementation (PR #456's design already plans the same pvcreate/vgcreate/lvcreate assembly pattern) doesn't rediscover the same bugs. This adopts the extracted package here and deletes the private copy; behavior is unchanged, confirmed by the existing pkg/spdk and pkg/util tests passing unmodified. The atlas-lib/lvm source lands here too, ahead of PR #457 merging -- both branches carry the identical commit content until #457 merges and this branch rebases past it, at which point the duplication collapses on its own. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq

Follow-up to PR #457's rewrite of atlas-lib/lvm (Inspector -> Manager, plus named methods for every LVM operation VDO needs). Replaces every lvmInspector.Run(...) argument-building call site in vdo.go with the matching named method: CreatePhysicalVolume, CreateVolumeGroup, CreateVDOLogicalVolume, ActivateVolumeGroup, DeactivateVolumeGroup, RemoveVolumeGroup, ImportClonedVolumeGroup, RenameLogicalVolume, ListLogicalVolumes, ExtendPhysicalVolume, ExtendLogicalVolumeByFreeSpace, LogicalVolumeSize, ExtendLogicalVolumeToSize, SetVDOFeatures. Drops vdo.go's now-redundant local yn() helper (folded into the named CreateVDOLogicalVolume/SetVDOFeatures methods) and the raw lvs-based size-parsing/LV-listing logic now handled by LogicalVolumeSize/ ListLogicalVolumes. Behavior is unchanged -- same commands, same scoping, same fallback paths (DeactivateVDO's "not found" string match, RemoveVDO/DeactivateVDO's dmsetup fallback). Copies atlas-lib/lvm/*.go from the extract-lvm-atlas-lib branch (PR #457) into this worktree's copy of atlas-lib, since the two branches aren't rebased onto each other yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq

noctarius

Much much better than the previous, very low-level implementation :)

In general, I wonder if it'd make sense to provide lvm.VolumeGroup, lvm.PhysicalVolume, lvm.LogicalVolume structs which encapsulate the internal knowledge of the items and ensures type-safety of the methods (like passing an array of lvm.PhysicalVolumes to lvm.Manager::CreateLogicalVolume).

Syncs vdo.go with atlas-lib/lvm's ExtendPhysicalVolume -> ExpandPhysicalVolume / ExtendLogicalVolumeByFreeSpace -> ExpandLogicalVolume rename (PR #457, commit 5ebffe9). No behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq

Follows PR #457 commit 960cb06. No effect on this branch's own code -- vdo.go never referenced lvm.Runner or lvm.NewManagerWithRunner directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq

noctarius

I think the general "string-slice isn't great" comment is not addressed.

Syncs atlas-lib/lvm with extract-lvm-atlas-lib (PR #457) through 74c745d: CreateLogicalVolume dispatch/Handles fixes, the physicalVolume->poolName rename, Extend->Expand renaming, Runner unexport, and -- the actual point of this commit -- CreateOrAttachVDO, ResolveClonedVDO, DeactivateVDO, RemoveVDO, and GrowVDO moving from this file into atlas-lib/lvm/vdo as CreateOrAttach, ResolveClone, Deactivate, Remove, and Grow. vdo.go is now thin wiring: one shared lvm.Manager, and one function per NodeStageVolume/NodeUnstageVolume/NodeExpandVolume concern, each delegating straight into the vdo package. The klog.Warningf calls and the vgName/poolLVName naming convention moved with the functions that used them; nothing here duplicates them anymore. 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

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>

- 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

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

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

…ruction 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

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

…olume Follows PR #457 commit f562d50. No changes needed to csi-driver/pkg/util/vdo.go -- vdo.CreateOrAttach/Deactivate/Remove/ Grow/ResolveClone/SetFeatures keep their existing string-keyed signatures; the new typed values stay internal to atlas-lib/lvm/vdo/stack.go. Confirmed by rebuilding with zero source edits on this branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViL41RrgYVvQaTVShyDxsq

boddumanohar · 2026-08-28T11:37:57Z

Added typed PhysicalVolume/VolumeGroup/LogicalVolume (commit f562d50) — every method now takes/returns these instead of bare strings, so a device path can't be passed where a VG name belongs. LogicalVolume carries its parent VolumeGroup, so a wrong VG/LV pairing is also a type error now. Nothing references Manager, so it stays stateless.

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

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

boddumanohar force-pushed the extract-lvm-atlas-lib branch from be44cb3 to 58373ab Compare August 25, 2026 13:54

boddumanohar added this to the 26.4 milestone Aug 25, 2026

boddumanohar added the atlas-lib label Aug 25, 2026

boddumanohar requested review from geoffrey1330 and noctarius August 25, 2026 13:55

geoffrey1330 previously approved these changes Aug 25, 2026

View reviewed changes

boddumanohar self-assigned this Aug 25, 2026

boddumanohar dismissed geoffrey1330’s stale review via f6bddbf August 26, 2026 06:33

boddumanohar force-pushed the extract-lvm-atlas-lib branch from f6bddbf to 1193143 Compare August 26, 2026 06:51

noctarius requested changes Aug 26, 2026

View reviewed changes

noctarius reviewed Aug 26, 2026

View reviewed changes

Comment thread atlas-lib/lvm/dm.go Outdated

Comment thread atlas-lib/lvm/grow.go Outdated

Comment thread atlas-lib/lvm/vdo.go Outdated

Comment thread atlas-lib/lvm/grow.go Outdated

noctarius force-pushed the extract-lvm-atlas-lib branch from 960cb06 to a793985 Compare August 28, 2026 07:17

boddumanohar and others added 10 commits August 28, 2026 11:38

Extracted vdo-specific code into its own sub-package

ccbb080

Updated the parameter set

df44d5b

quality gate run against atlas-lib/lvm

672b35c

boddumanohar force-pushed the extract-lvm-atlas-lib branch from 74c745d to b84b4c4 Compare August 28, 2026 09:39

boddumanohar requested review from geoffrey1330 and noctarius August 28, 2026 11:16

noctarius approved these changes Aug 28, 2026

View reviewed changes

noctarius merged commit d7305e2 into main Aug 28, 2026
12 checks passed

noctarius deleted the extract-lvm-atlas-lib branch August 28, 2026 13:18

Navigation Menu

feat(atlas-lib): add lvm package for device-scoped LVM commands - #457

feat(atlas-lib): add lvm package for device-scoped LVM commands#457
noctarius merged 11 commits into
mainfrom
extract-lvm-atlas-lib

boddumanohar commented Aug 25, 2026 •

edited

Loading

Uh oh!

noctarius left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

noctarius left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

boddumanohar commented Aug 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

boddumanohar commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What's in the package

Test plan

Uh oh!

noctarius left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

noctarius left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

boddumanohar commented Aug 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

boddumanohar commented Aug 25, 2026 •

edited

Loading