lvm2: Fix "no device" case for _is_lvm_device() and avoid no provider test if /boot is not a mount point #589

bmr-cymru · 2025-11-05T15:02:53Z

Resolves: #587
Resolves: #588

Summary by CodeRabbit

Bug Fixes
- Improved LVM device detection so the system correctly recognises both absolute device paths and device‑mapper names, reducing false negatives when identifying LVM volumes.
- Updated device-name handling to ensure mapper names are resolved consistently when checking device types.
Tests
- Snapshot-related tests now run conditionally, skipping when the expected mount point is not present to avoid spurious failures.

Resolves: #588 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>

coderabbitai · 2025-11-05T15:03:01Z

Walkthrough

Changed _is_lvm_device() in the LVM2 plugin to accept either absolute device paths or device-mapper names, validate existence and major/device-mapper status accordingly; added _check_dm_major() helper, adjusted dm_name derivation and dmsetup args. One test now skips when /boot is not a mount point.

Changes

Cohort / File(s)	Summary
LVM2 Device Validation `snapm/manager/plugins/lvm2.py`	Renamed parameter from `devpath` to `device`. Added `_check_dm_major(dev)` helper. If `device` is an absolute path, validate existence then check device-major; if not absolute, treat as mapper name under `DEV_MAPPER_PREFIX`, validate existence, and check major. Derive `dm_name` by stripping `DEV_MAPPER_PREFIX` when present and update `dmsetup` arguments to use that `dm_name`.
Test Conditionals `tests/test_manager.py`	Added `os.path` import and decorated `test_create_snapshot_set_no_provider` with `@unittest.skipIf(not os.path.ismount("/boot"), "no suitable mount path")` to skip when `/boot` is not a mount point.

Sequence Diagram(s)

sequenceDiagram
    participant Caller as Caller
    participant LVM as _Lvm2._is_lvm_device
    participant FS as Filesystem
    participant DM as device-mapper

    Caller->>LVM: _is_lvm_device(device)
    alt device is absolute path
        LVM->>FS: stat(device) — exists?
        FS-->>LVM: exists / not exists
        alt exists
            LVM->>DM: _check_dm_major(dev) → read major
            DM-->>LVM: major == DM_MAJOR? (yes/no)
        end
    else device is mapper name
        LVM->>DM: build /dev/mapper/{device} and stat() — exists?
        DM-->>LVM: exists / not exists
        alt exists
            LVM->>DM: _check_dm_major(dev) → read major
            DM-->>LVM: major == DM_MAJOR? (yes/no)
        end
    end
    LVM-->>Caller: boolean (is_lvm_device)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Review focus:
- snapm/manager/plugins/lvm2.py: correctness of absolute vs mapper-name branching, _check_dm_major() implementation, dm_name derivation and dmsetup argument construction.
- tests/test_manager.py: skip condition correctness and import placement.
Pay attention to edge cases: non-existent paths, non-block or pseudo files (e.g. tmpfs), and correct device-major detection.

Possibly related PRs

lvm2: fix confusing error message when probing non-dm devices #478 — Modifies the same _is_lvm_device() logic in snapm/manager/plugins/lvm2.py (device-major checks and mapper-name handling).

🐰 I hopped where mappers hide and play,
Paths or names, I learned the way.
Tmpfs spared from dmsetup’s call,
A tidy burrow, no more fall.
Hooray — a neat little fix today!

Pre-merge checks and finishing touches

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the two main changes: fixing _is_lvm_device() for non-device filesystems and skipping a test when /boot is not mounted.
Linked Issues check	✅ Passed	The pull request addresses all coding requirements from both linked issues: _is_lvm_device() now handles non-device filesystems correctly [#587], and the test is skipped when /boot is not a mount point [#588].
Out of Scope Changes check	✅ Passed	All changes are directly scoped to resolving issues #587 and #588; no unrelated modifications are present.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch bmr-no-provider-err-lvm2

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

packit-as-a-service · 2025-11-05T15:06:08Z

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

sudo yum install -y dnf-plugins-core on RHEL 8
sudo dnf install -y dnf-plugins-core on Fedora
dnf copr enable packit/snapshotmanager-snapm-589
And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

tests/test_manager.py (1)
11-11: Consider using os.ismount() instead of importing os.path separately.

Since os is already imported at line 10, you can use os.path.ismount() without the separate import, or use os.ismount() if available. However, the current approach is also valid and explicit.
-import os.path
Then at line 510, use:
@unittest.skipIf(not os.path.ismount("/boot"), "no suitable mount path")
This works because os.path is automatically available when os is imported.
snapm/manager/plugins/lvm2.py (1)
539-542: Consider adding block device validation for consistency.

When the device is not an absolute path, the code only checks if /dev/mapper/{device} exists but skips the block device and major number validation performed for absolute paths (lines 534-538). Whilst this is likely safe—since entries under /dev/mapper should always be device-mapper devices—adding these checks would make the validation more consistent and robust.

Apply this diff to add consistent validation:
 else:
     check_path = path_join(DEV_MAPPER_PREFIX, device)
     if not path_exists(check_path):
         return False
+    st = stat(check_path, follow_symlinks=True)
+    if not S_ISBLK(st.st_mode):
+        return False
+    if dev_major(st.st_rdev) != _get_dm_major():
+        return False

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a719f28 and ca7de0e.

📒 Files selected for processing (2)

snapm/manager/plugins/lvm2.py (1 hunks)
tests/test_manager.py (2 hunks)

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-14T02:14:30.316Z
Learning: PR #478 in snapshotmanager/snapm fixes a regression in the LVM2 plugin where `can_snapshot()` method throws `SnapmCalloutError` when probing non-device-mapper devices. The fix adds device major number validation to prevent calling dmsetup on non-dm devices, allowing proper "no provider found" error messages instead of confusing dmsetup errors.

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-25T17:19:19.563Z
Learning: The man page changes mentioned in the AI summary for PR #383 were rebasing artifacts and should be ignored when reviewing this branch - there are no actual man page changes on the bmr-vm-tests branch.

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-26T19:07:42.290Z
Learning: In the snapm virt_tests module, verify_lvm_setup() is intended as a lightweight sanity check for essential volumes, not comprehensive validation for every storage layout. The responsibility for testing new layout kickstart fragments lies with the developer submitting them. Adding extensive validation for each layout would be redundant and create clutter.

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 383
File: virt_tests/testvm.py:284-296
Timestamp: 2025-08-26T03:42:26.409Z
Learning: In the snapm virt_tests module, the current virt-install --boot configuration using "uefi=on/off" syntax works correctly across all target platforms that bmr-cymru tests on, so compatibility changes to avoid "uefi=off" are not needed.

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:36:57.822Z
Learning: The removeprefix() method in Python correctly removes the entire prefix string. For example, "/dev/mapper/rhel-root".removeprefix("/dev/mapper/") returns "rhel-root", not "/rhel-root". The existing LVM2 plugin logic correctly handles device-mapper name extraction.

📚 Learning: 2025-09-14T02:14:30.316Z

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-14T02:14:30.316Z
Learning: PR #478 in snapshotmanager/snapm fixes a regression in the LVM2 plugin where `can_snapshot()` method throws `SnapmCalloutError` when probing non-device-mapper devices. The fix adds device major number validation to prevent calling dmsetup on non-dm devices, allowing proper "no provider found" error messages instead of confusing dmsetup errors.

Applied to files:

snapm/manager/plugins/lvm2.py

📚 Learning: 2025-09-14T17:36:57.822Z

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:36:57.822Z
Learning: The removeprefix() method in Python correctly removes the entire prefix string. For example, "/dev/mapper/rhel-root".removeprefix("/dev/mapper/") returns "rhel-root", not "/rhel-root". The existing LVM2 plugin logic correctly handles device-mapper name extraction.

Applied to files:

snapm/manager/plugins/lvm2.py

📚 Learning: 2025-10-20T21:12:52.344Z

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:372-377
Timestamp: 2025-10-20T21:12:52.344Z
Learning: In snapm/manager/_mounts.py mount operations: do not create missing mount point directories on snapshot filesystems; instead, check if the directory exists with isdir(), log a warning if missing, and skip that mount to avoid modifying the snapshot filesystem unnecessarily.

Applied to files:

tests/test_manager.py

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)

GitHub Check: rpm-build:centos-stream-10-x86_64
GitHub Check: rpm-build:fedora-41-x86_64
GitHub Check: rpm-build:centos-stream-9-x86_64
GitHub Check: rpm-build:fedora-rawhide-x86_64
GitHub Check: rpm-build:epel-9-x86_64
GitHub Check: rpm-build:fedora-43-x86_64
GitHub Check: rpm-build:fedora-42-x86_64
GitHub Check: rpm-build:centos-stream-10-x86_64
GitHub Check: rpm-build:fedora-rawhide-x86_64
GitHub Check: rpm-build:fedora-42-x86_64
GitHub Check: rpm-build:fedora-43-x86_64
GitHub Check: rpm-build:centos-stream-9-x86_64
GitHub Check: rpm-build:epel-9-x86_64
GitHub Check: rpm-build:fedora-41-x86_64
GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
GitHub Check: virt_tests (bios, lvm-thin, fedora41)
GitHub Check: virt_tests (bios, lvm, centos-stream9)
GitHub Check: virt_tests (uefi, lvm, centos-stream10)
GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
GitHub Check: virt_tests (bios, lvm, fedora42)
GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
GitHub Check: virt_tests (uefi, lvm, centos-stream9)
GitHub Check: virt_tests (uefi, lvm, fedora41)
GitHub Check: virt_tests (uefi, lvm, fedora42)
GitHub Check: virt_tests (bios, lvm, centos-stream10)
GitHub Check: virt_tests (bios, lvm-thin, fedora42)
GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
GitHub Check: virt_tests (bios, lvm, fedora41)
GitHub Check: python-basic-tests

🔇 Additional comments (4)

tests/test_manager.py (1)

510-513: LGTM! Test skip condition properly addresses issue #588.

The skip condition correctly ensures this test only runs when /boot is a mount point, avoiding false failures on systems where /boot is not separately mounted.

Based on PR objectives.

snapm/manager/plugins/lvm2.py (3)

523-529: LGTM! Parameter rename improves clarity.

The parameter name change from devpath to device better reflects that the method now handles both absolute device paths and device-mapper names, supporting the fix for issue #587.

Based on PR objectives.

530-538: LGTM! Absolute path validation properly handles non-device filesystems.

The validation correctly returns False for non-device filesystems (e.g., tmpfs) at line 536, addressing issue #587. The checks for existence, block device status, and device-mapper major number are appropriate.

Based on PR objectives and retrieved learnings.

544-547: LGTM! Device name extraction logic is correct.

The dm_name resolution properly handles both /dev/mapper/ prefixed paths and bare device-mapper names, ensuring the correct argument is passed to the dmsetup command.

Based on retrieved learnings.

Before: root@f42-snapm-vm1:~/src/git/snapm# snapm snapset create badset /tmp ERROR - Command failed: Error calling dmsetup: Device does not exist. Command failed. After: root@f42-snapm-vm1:~/src/git/snapm# snapm snapset create badset /tmp ERROR - Command failed: Could not find snapshot provider for /tmp Resolves: #587 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

snapm/manager/plugins/lvm2.py (1)
550-553: Consider simplifying the dm_name derivation.

Since removeprefix() only removes the prefix when it matches (returning the original string otherwise), you could simplify this to:
dm_name = device.removeprefix(DEV_MAPPER_PREFIX)
This is functionally equivalent, but the current explicit form is also perfectly acceptable and may be clearer about intent.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ca7de0e and 4b37e10.

📒 Files selected for processing (1)

snapm/manager/plugins/lvm2.py (1 hunks)

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-14T02:14:30.316Z
Learning: PR #478 in snapshotmanager/snapm fixes a regression in the LVM2 plugin where `can_snapshot()` method throws `SnapmCalloutError` when probing non-device-mapper devices. The fix adds device major number validation to prevent calling dmsetup on non-dm devices, allowing proper "no provider found" error messages instead of confusing dmsetup errors.

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-25T17:19:19.563Z
Learning: The man page changes mentioned in the AI summary for PR #383 were rebasing artifacts and should be ignored when reviewing this branch - there are no actual man page changes on the bmr-vm-tests branch.

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-26T19:07:42.290Z
Learning: In the snapm virt_tests module, verify_lvm_setup() is intended as a lightweight sanity check for essential volumes, not comprehensive validation for every storage layout. The responsibility for testing new layout kickstart fragments lies with the developer submitting them. Adding extensive validation for each layout would be redundant and create clutter.

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 383
File: virt_tests/testvm.py:284-296
Timestamp: 2025-08-26T03:42:26.409Z
Learning: In the snapm virt_tests module, the current virt-install --boot configuration using "uefi=on/off" syntax works correctly across all target platforms that bmr-cymru tests on, so compatibility changes to avoid "uefi=off" are not needed.

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:36:57.822Z
Learning: The removeprefix() method in Python correctly removes the entire prefix string. For example, "/dev/mapper/rhel-root".removeprefix("/dev/mapper/") returns "rhel-root", not "/rhel-root". The existing LVM2 plugin logic correctly handles device-mapper name extraction.

📚 Learning: 2025-09-14T02:14:30.316Z

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-14T02:14:30.316Z
Learning: PR #478 in snapshotmanager/snapm fixes a regression in the LVM2 plugin where `can_snapshot()` method throws `SnapmCalloutError` when probing non-device-mapper devices. The fix adds device major number validation to prevent calling dmsetup on non-dm devices, allowing proper "no provider found" error messages instead of confusing dmsetup errors.

Applied to files:

snapm/manager/plugins/lvm2.py

📚 Learning: 2025-09-14T17:36:57.822Z

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:36:57.822Z
Learning: The removeprefix() method in Python correctly removes the entire prefix string. For example, "/dev/mapper/rhel-root".removeprefix("/dev/mapper/") returns "rhel-root", not "/rhel-root". The existing LVM2 plugin logic correctly handles device-mapper name extraction.

Applied to files:

snapm/manager/plugins/lvm2.py

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)

GitHub Check: rpm-build:centos-stream-10-x86_64
GitHub Check: rpm-build:epel-9-x86_64
GitHub Check: rpm-build:fedora-rawhide-x86_64
GitHub Check: rpm-build:fedora-43-x86_64
GitHub Check: rpm-build:centos-stream-9-x86_64
GitHub Check: rpm-build:fedora-41-x86_64
GitHub Check: rpm-build:fedora-42-x86_64
GitHub Check: virt_tests (bios, lvm, centos-stream9)
GitHub Check: virt_tests (uefi, lvm, fedora41)
GitHub Check: virt_tests (uefi, lvm, centos-stream9)
GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
GitHub Check: virt_tests (uefi, lvm, centos-stream10)
GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
GitHub Check: virt_tests (bios, lvm, centos-stream10)
GitHub Check: virt_tests (uefi, lvm, fedora42)
GitHub Check: virt_tests (bios, lvm-thin, fedora42)
GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
GitHub Check: virt_tests (bios, lvm, fedora42)
GitHub Check: virt_tests (bios, lvm, fedora41)
GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
GitHub Check: virt_tests (bios, lvm-thin, fedora41)
GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
GitHub Check: python-basic-tests

🔇 Additional comments (1)

snapm/manager/plugins/lvm2.py (1)

523-570: LGTM! Correctly handles non-device filesystems.

The updated implementation properly addresses issue #587 by:

Returning False early for non-block devices (e.g., tmpfs) via the S_ISBLK check in _check_dm_major

Supporting both absolute device paths and device-mapper names

Validating path existence before attempting device-major checks

The inner helper _check_dm_major centralises the block device and device-mapper major number validation, making the logic clearer and avoiding duplicate stat() calls.

Based on learnings.

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4b37e10 and 479f6e1.

📒 Files selected for processing (1)

snapm/manager/plugins/lvm2.py (1 hunks)

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-14T02:14:30.316Z
Learning: PR #478 in snapshotmanager/snapm fixes a regression in the LVM2 plugin where `can_snapshot()` method throws `SnapmCalloutError` when probing non-device-mapper devices. The fix adds device major number validation to prevent calling dmsetup on non-dm devices, allowing proper "no provider found" error messages instead of confusing dmsetup errors.

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-26T19:07:42.290Z
Learning: In the snapm virt_tests module, verify_lvm_setup() is intended as a lightweight sanity check for essential volumes, not comprehensive validation for every storage layout. The responsibility for testing new layout kickstart fragments lies with the developer submitting them. Adding extensive validation for each layout would be redundant and create clutter.

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-25T17:19:19.563Z
Learning: The man page changes mentioned in the AI summary for PR #383 were rebasing artifacts and should be ignored when reviewing this branch - there are no actual man page changes on the bmr-vm-tests branch.

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:36:57.822Z
Learning: The removeprefix() method in Python correctly removes the entire prefix string. For example, "/dev/mapper/rhel-root".removeprefix("/dev/mapper/") returns "rhel-root", not "/rhel-root". The existing LVM2 plugin logic correctly handles device-mapper name extraction.

📚 Learning: 2025-09-14T02:14:30.316Z

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-14T02:14:30.316Z
Learning: PR #478 in snapshotmanager/snapm fixes a regression in the LVM2 plugin where `can_snapshot()` method throws `SnapmCalloutError` when probing non-device-mapper devices. The fix adds device major number validation to prevent calling dmsetup on non-dm devices, allowing proper "no provider found" error messages instead of confusing dmsetup errors.

Applied to files:

snapm/manager/plugins/lvm2.py

📚 Learning: 2025-09-14T17:36:57.822Z

Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:36:57.822Z
Learning: The removeprefix() method in Python correctly removes the entire prefix string. For example, "/dev/mapper/rhel-root".removeprefix("/dev/mapper/") returns "rhel-root", not "/rhel-root". The existing LVM2 plugin logic correctly handles device-mapper name extraction.

Applied to files:

snapm/manager/plugins/lvm2.py

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (44)

GitHub Check: rpm-build:fedora-43-x86_64
GitHub Check: rpm-build:fedora-rawhide-x86_64
GitHub Check: rpm-build:centos-stream-10-x86_64
GitHub Check: rpm-build:centos-stream-9-x86_64
GitHub Check: rpm-build:fedora-42-x86_64
GitHub Check: rpm-build:epel-9-x86_64
GitHub Check: rpm-build:fedora-42-x86_64
GitHub Check: rpm-build:fedora-41-x86_64
GitHub Check: rpm-build:fedora-rawhide-x86_64
GitHub Check: rpm-build:centos-stream-10-x86_64
GitHub Check: rpm-build:fedora-43-x86_64
GitHub Check: rpm-build:centos-stream-9-x86_64
GitHub Check: rpm-build:epel-9-x86_64
GitHub Check: rpm-build:fedora-41-x86_64
GitHub Check: rpm-build:fedora-42-x86_64
GitHub Check: rpm-build:centos-stream-10-x86_64
GitHub Check: rpm-build:fedora-rawhide-x86_64
GitHub Check: rpm-build:epel-9-x86_64
GitHub Check: rpm-build:fedora-43-x86_64
GitHub Check: rpm-build:centos-stream-9-x86_64
GitHub Check: rpm-build:fedora-42-x86_64
GitHub Check: rpm-build:centos-stream-10-x86_64
GitHub Check: rpm-build:centos-stream-9-x86_64
GitHub Check: rpm-build:epel-9-x86_64
GitHub Check: rpm-build:fedora-43-x86_64
GitHub Check: rpm-build:fedora-rawhide-x86_64
GitHub Check: rpm-build:fedora-41-x86_64
GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
GitHub Check: virt_tests (uefi, lvm, fedora42)
GitHub Check: virt_tests (bios, lvm-thin, fedora42)
GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
GitHub Check: virt_tests (uefi, lvm, centos-stream10)
GitHub Check: virt_tests (bios, lvm-thin, fedora41)
GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
GitHub Check: virt_tests (uefi, lvm, fedora41)
GitHub Check: virt_tests (uefi, lvm, centos-stream9)
GitHub Check: virt_tests (bios, lvm, fedora42)
GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
GitHub Check: virt_tests (bios, lvm, centos-stream10)
GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
GitHub Check: virt_tests (bios, lvm, centos-stream9)
GitHub Check: virt_tests (bios, lvm, fedora41)
GitHub Check: python-basic-tests

🔇 Additional comments (2)

snapm/manager/plugins/lvm2.py (2)

523-537: The helper function approach is sound.

The addition of the _check_dm_major() helper centralises the device-major validation logic. The rename from devpath to device better reflects the broader input types now accepted (absolute paths or mapper names).

Note: stat() at line 532 could raise OSError for permission issues or other filesystem errors beyond non-existence. Whilst this may be acceptable (as it signals a genuine error condition), consider whether explicit error handling would improve robustness.

539-549: Correctly handles non-device filesystems.

The dual-path logic properly addresses issue #587:

Absolute paths are validated directly

Relative names (assumed to be mapper names) are validated under /dev/mapper/

Non-device filesystems (e.g., tmpfs) return False early without invoking dmsetup

This prevents the "Device does not exist" errors mentioned in the PR objectives.

snapm/manager/plugins/lvm2.py

tests: skip test_create_snapshot_set_no_provider if !ismount("/boot")

a25c448

Resolves: #588 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>

bmr-cymru self-assigned this Nov 5, 2025

bmr-cymru added bug Something isn't working Testing Test related labels Nov 5, 2025

bmr-cymru added this to Bug Fixes Nov 5, 2025

bmr-cymru added UI/UX User interface and experience LVM2 labels Nov 5, 2025

coderabbitai bot reviewed Nov 5, 2025

View reviewed changes

coderabbitai bot approved these changes Nov 5, 2025

View reviewed changes

bmr-cymru added Plugin Integration Regression labels Nov 5, 2025

bmr-cymru moved this to In Progress in Bug Fixes Nov 5, 2025

bmr-cymru force-pushed the bmr-no-provider-err-lvm2 branch from ca7de0e to 4b37e10 Compare November 5, 2025 15:40

coderabbitai bot reviewed Nov 5, 2025

View reviewed changes

bmr-cymru force-pushed the bmr-no-provider-err-lvm2 branch from 4b37e10 to 479f6e1 Compare November 5, 2025 16:41

coderabbitai bot requested changes Nov 5, 2025

View reviewed changes

snapm/manager/plugins/lvm2.py Show resolved Hide resolved

coderabbitai bot approved these changes Nov 5, 2025

View reviewed changes

bmr-cymru merged commit 479f6e1 into main Nov 5, 2025
35 of 36 checks passed

github-project-automation bot moved this from In Progress to Done in Bug Fixes Nov 5, 2025

bmr-cymru deleted the bmr-no-provider-err-lvm2 branch November 10, 2025 20:00

coderabbitai bot mentioned this pull request Dec 29, 2025

lvm2: relax restrictions on Lvm2Cow.can_snapshot() #838

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

lvm2: Fix "no device" case for _is_lvm_device() and avoid no provider test if /boot is not a mount point #589

lvm2: Fix "no device" case for _is_lvm_device() and avoid no provider test if /boot is not a mount point #589

Uh oh!

bmr-cymru commented Nov 5, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Nov 5, 2025 •

edited

Loading

Uh oh!

packit-as-a-service bot commented Nov 5, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lvm2: Fix "no device" case for _is_lvm_device() and avoid no provider test if /boot is not a mount point #589

lvm2: Fix "no device" case for _is_lvm_device() and avoid no provider test if /boot is not a mount point #589

Uh oh!

Conversation

bmr-cymru commented Nov 5, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Pre-merge checks and finishing touches

Uh oh!

packit-as-a-service bot commented Nov 5, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bmr-cymru commented Nov 5, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 5, 2025 •

edited

Loading