fix: Replace deprecated lvdisplay with pvs for LVM detection in detect_mounts - #198
fix: Replace deprecated lvdisplay with pvs for LVM detection in detect_mounts#198sayalibhavsar wants to merge 3 commits into
Conversation
…t_mounts The old code walked /dev/mapper/* and parsed lvdisplay --maps output to find physical volumes. Replace with a pvs-based approach that directly queries all LVM PVs and also derives base disk names (handling both SCSI/virtio and NVMe/mmcblk partition suffixes). Guard the block with command -v pvs so systems without lvm2 are unaffected.
|
This relates to RPOPC-610 |
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesLVM physical-volume discovery
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@detect_mounts`:
- Around line 22-23: Update the pvs discovery logic in detect_mounts to capture
and validate the command’s exit status, not just whether pvs is installed. Treat
query failures such as locks, configuration errors, permissions, or missing
metadata as a failed-closed condition, while still skipping discovery when pvs
is genuinely unavailable; do not allow an empty result from a failed query to
report success.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
dvalinrh
left a comment
dvalinrh
left a comment
There was a problem hiding this comment.
Address what coderabbit found, and done. Simply catch the error, report it and exit out. To make sure the calling program does not simply continue on, put all the disks into the in use file.
Capture pvs exit status instead of silently discarding errors with 2>/dev/null. On failure (LVM lock, permissions, missing metadata), report the error to stderr and mark all requested disks as in-use so the caller cannot proceed with a destructive operation.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
detect_mounts (1)
29-37: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winResolve mapped PVs to their backing disks.
pvscan report PV names that are device-mapper devices, symlinks such as/dev/disk/by-id/..., or namespaced devices with partition suffixes. This only strips a partition suffix and writes/dev/<base>, sodetect_mounts /dev/sdXcan report success while that disk backs an LVM PV through a mapping. Resolve each PV through its full block-device parent chain vialsblkor sysfs before writingmount_tmp, or explicitly restrict and validate the supported PV path forms.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@detect_mounts` around lines 29 - 37, Update the PV-processing loop around pv_output to resolve each reported PV through its complete block-device parent chain, including device-mapper devices, symlinks, and namespaced partitioned devices, before writing entries to mount_tmp. Use lsblk or sysfs for the resolution and retain the resolved backing disk alongside the PV entry; do not rely solely on stripping partition suffixes.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@detect_mounts`:
- Around line 29-37: Update the PV-processing loop around pv_output to resolve
each reported PV through its complete block-device parent chain, including
device-mapper devices, symlinks, and namespaced partitioned devices, before
writing entries to mount_tmp. Use lsblk or sysfs for the resolution and retain
the resolved backing disk alongside the PV entry; do not rely solely on
stripping partition suffixes.
2 participants
Description
Replace the old /dev/mapper + lvdisplay --maps LVM detection in detect_mounts with a pvs-based approach that directly queries all physical volumes, derives base disk names (handling both SCSI/virtio and NVMe/mmcblk partition suffixes), and guards with command -v pvs so systems without lvm2 are unaffected.
Before/After Comparison
Before: Walked /dev/mapper/* and parsed lvdisplay --maps output, producing malformed paths (missing /dev prefix) and incorrect base devices for NVMe partitions (nvme0n1p3 → nvme0n1pcinstead of nvme0n1).
After: pvs --noheadings -o pv_name lists every PV directly, and sed strips partition suffixes correctly for both SCSI (sda1 → sda) and NVMe/mmcblk (nvme0n1p2 → nvme0n1) devices.
Clerical Stuff
This closes #57
Relates to JIRA: RPOPC-610