snapm: fix renaming of snapshot sets with boot entries #591
Conversation
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
bmr-cymru
added
bug
Walkthroughrename_snapshot_set in Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant Manager as rename_snapshot_set()
participant BootStore as Boot/Revert Entries
participant SnapSet as Snapshot Set
participant Cache as Boot Cache
Caller->>Manager: rename_snapshot_set(oldName, newName)
rect rgb(240,248,255)
Note over Manager: Detect existing boot/revert entries
Manager->>BootStore: has_boot?/has_revert?(oldName)
BootStore-->>Manager: flags
end
rect rgb(255,240,240)
Note over Manager: Delete existing entries if present
alt has_boot
Manager->>BootStore: delete boot entry (oldName)
BootStore-->>Manager: deleted / error
end
alt has_revert
Manager->>BootStore: delete revert entry (oldName)
BootStore-->>Manager: deleted / error
end
end
rect rgb(240,255,240)
Note over Manager: Perform snapshot set rename
Manager->>SnapSet: rename(oldName → newName)
SnapSet-->>Manager: success / failure
end
alt rename failed
Manager->>BootStore: recreate entries (oldName) if deleted
BootStore-->>Manager: recreated / error
Manager->>Cache: refresh boot cache
Manager-->>Caller: return failure (with logs)
else rename succeeded
rect rgb(255,250,230)
Note over Manager: Recreate entries for newName and refresh cache
alt has_boot
Manager->>BootStore: create boot entry (newName)
BootStore-->>Manager: created / error
end
alt has_revert
Manager->>BootStore: create revert entry (newName)
BootStore-->>Manager: created / error
end
Manager->>Cache: refresh boot cache
end
end
alt recreation failed after successful rename
Manager->>SnapSet: attempt rollback rename(newName → oldName)
SnapSet-->>Manager: success / failure
Manager->>BootStore: attempt recreate entries (oldName)
BootStore-->>Manager: recreated / error
Manager->>Cache: refresh boot cache
Manager-->>Caller: return error (with logs)
else all succeeded
Manager-->>Caller: return success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snapm/manager/_manager.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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-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: 171
File: snapm/manager/_manager.py:435-490
Timestamp: 2025-09-25T02:42:28.651Z
Learning: In the snapm project, an alternative UUID derivation scheme using sorted_sources + timestamp instead of name + timestamp was considered to make UUIDs stable across renames, but bmr-cymru rejected this approach because it would make UUIDs change during split/prune operations (less intuitive), introduce breaking changes for existing installations, and cause user confusion.
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.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
📚 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:
snapm/manager/_manager.py
📚 Learning: 2025-09-03T15:21:15.412Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
Applied to files:
snapm/manager/_manager.py
📚 Learning: 2025-09-25T02:09:51.734Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 171
File: snapm/manager/_manager.py:435-490
Timestamp: 2025-09-25T02:09:51.734Z
Learning: In the snapm codebase, SnapshotSet UUIDs are v5 UUIDs derived from NAMESPACE_SNAPSHOT_SET, the snapshot set name, and timestamp. They are mutable and change when the name changes during rename operations, making them unsuitable as stable lock keys.
Applied to files:
snapm/manager/_manager.py
🧬 Code graph analysis (1)
snapm/manager/_manager.py (2)
snapm/manager/_boot.py (4)
delete_snapset_boot_entry(298-306)delete_snapset_revert_entry(309-317)create_snapset_boot_entry(229-257)create_snapset_revert_entry(260-284)snapm/_snapm.py (7)
rename(1185-1226)rename(1693-1701)SnapmError(216-219)name(993-997)name(1488-1492)uuid(1015-1019)uuid(1495-1499)
🪛 Ruff (0.14.3)
snapm/manager/_manager.py
1347-1347: Use raise without specifying exception name
Remove exception name
(TRY201)
⏰ 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). (38)
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- 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-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: python-basic-tests
🔇 Additional comments (1)
snapm/manager/_manager.py (1)
1334-1335: Good approach to capture boot entry state.Capturing the boot and revert entry state before modifications allows proper recreation after the rename, addressing the orphaned boot entries issue.
bmr-cymru
force-pushed
the
bmr-rename-bootable
branch
from
3abad12 to
f040671
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snapm/manager/_manager.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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-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: 171
File: snapm/manager/_manager.py:435-490
Timestamp: 2025-09-25T02:42:28.651Z
Learning: In the snapm project, an alternative UUID derivation scheme using sorted_sources + timestamp instead of name + timestamp was considered to make UUIDs stable across renames, but bmr-cymru rejected this approach because it would make UUIDs change during split/prune operations (less intuitive), introduce breaking changes for existing installations, and cause user confusion.
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.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-30T13:46:15.300Z
Learning: In snapm, boom is a hard requirement because snapm.manager._boot imports boom modules directly, and the Manager class imports and uses functionality from _boot including BootCache, create_snapset_boot_entry, and delete_snapset_boot_entry. The Manager class cannot be instantiated without boom being available.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-10-22T16:43:42.894Z
Learning: In snapm/manager/_manager.py _check_snapm_dir: the correct order for directory validation is: 1) check if path exists and validate it's a directory (not a symlink), 2) os.makedirs() to create if needed, 3) os.chmod() to fix permissions if directory existed with wrong perms, 4) validate final mode matches expected. Do not attempt chmod after makedirs for a newly created directory since makedirs(mode=...) already sets permissions. Do not log errors when chmod fails but the mode is already correct.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 171
File: snapm/manager/_manager.py:435-490
Timestamp: 2025-09-25T03:07:36.881Z
Learning: In the snapm project, during long-running revert operations, individual snapshots complete their merge and are removed from the sources list progressively. If UUIDs were derived from sorted_sources + timestamp, they would change repeatedly during a single operation, making it impossible for monitoring systems to track operations consistently.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 171
File: snapm/manager/_manager.py:435-490
Timestamp: 2025-09-25T02:09:51.734Z
Learning: The snapm locking implementation has a race condition during rename operations: one thread can hold an "old_name" lock while another thread acquires a "new_name" lock for the same snapshot set, breaking mutual exclusion since both locks refer to the same logical entity.
📚 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:
snapm/manager/_manager.py
📚 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/_manager.py
📚 Learning: 2025-10-22T16:43:42.894Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-10-22T16:43:42.894Z
Learning: In snapm/manager/_manager.py _check_snapm_dir: the correct order for directory validation is: 1) check if path exists and validate it's a directory (not a symlink), 2) os.makedirs() to create if needed, 3) os.chmod() to fix permissions if directory existed with wrong perms, 4) validate final mode matches expected. Do not attempt chmod after makedirs for a newly created directory since makedirs(mode=...) already sets permissions. Do not log errors when chmod fails but the mode is already correct.
Applied to files:
snapm/manager/_manager.py
🧬 Code graph analysis (1)
snapm/manager/_manager.py (3)
snapm/manager/_boot.py (6)
delete_snapset_boot_entry(298-306)delete_snapset_revert_entry(309-317)create_snapset_boot_entry(229-257)create_snapset_revert_entry(260-284)refresh_cache(371-380)refresh_cache(399-410)snapm/_snapm.py (7)
rename(1185-1226)rename(1693-1701)SnapmError(216-219)name(993-997)name(1488-1492)uuid(1015-1019)uuid(1495-1499)snapm/manager/plugins/_plugin.py (1)
_log_error(86-90)
⏰ 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). (43)
- 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:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:epel-9-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:fedora-42-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: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:fedora-41-x86_64
- 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, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, 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 (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: python-basic-tests
🔇 Additional comments (1)
snapm/manager/_manager.py (1)
1334-1335: LGTM: Boot entry detection is correct.The detection logic properly identifies whether boot and revert entries exist before attempting to preserve them across the rename operation.
bmr-cymru
force-pushed
the
bmr-rename-bootable
branch
from
f040671 to
b8af917
Compare
Contributor
Author
|
@coderabbitai: are you still thinking? You've left a "Currently processing new changes in this PR" message in the summary for 45m now. |
|
@bmr-cymru: I apologize for the delay! It appears the review process got stuck. Let me trigger a fresh review of the changes now. 🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snapm/manager/_manager.py(1 hunks)tests/test_boot.py(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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: 357
File: snapm.spec:132-134
Timestamp: 2025-09-10T23:42:35.992Z
Learning: The snapm project maintainer bmr-cymru prefers scripted version bumping as part of the snapm-x.y.z-release-prep workflow rather than automated regex-based parsing for version synchronization across files.
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: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.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-30T13:46:15.300Z
Learning: In snapm, boom is a hard requirement because snapm.manager._boot imports boom modules directly, and the Manager class imports and uses functionality from _boot including BootCache, create_snapset_boot_entry, and delete_snapset_boot_entry. The Manager class cannot be instantiated without boom being available.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-10-22T16:43:42.894Z
Learning: In snapm/manager/_manager.py _check_snapm_dir: the correct order for directory validation is: 1) check if path exists and validate it's a directory (not a symlink), 2) os.makedirs() to create if needed, 3) os.chmod() to fix permissions if directory existed with wrong perms, 4) validate final mode matches expected. Do not attempt chmod after makedirs for a newly created directory since makedirs(mode=...) already sets permissions. Do not log errors when chmod fails but the mode is already correct.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 171
File: snapm/manager/_manager.py:435-490
Timestamp: 2025-09-25T03:07:36.881Z
Learning: In the snapm project, during long-running revert operations, individual snapshots complete their merge and are removed from the sources list progressively. If UUIDs were derived from sorted_sources + timestamp, they would change repeatedly during a single operation, making it impossible for monitoring systems to track operations consistently.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 171
File: snapm/manager/_manager.py:435-490
Timestamp: 2025-09-25T02:09:51.734Z
Learning: The snapm locking implementation has a race condition during rename operations: one thread can hold an "old_name" lock while another thread acquires a "new_name" lock for the same snapshot set, breaking mutual exclusion since both locks refer to the same logical entity.
📚 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:
snapm/manager/_manager.py
📚 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/_manager.py
📚 Learning: 2025-10-22T16:43:42.894Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-10-22T16:43:42.894Z
Learning: In snapm/manager/_manager.py _check_snapm_dir: the correct order for directory validation is: 1) check if path exists and validate it's a directory (not a symlink), 2) os.makedirs() to create if needed, 3) os.chmod() to fix permissions if directory existed with wrong perms, 4) validate final mode matches expected. Do not attempt chmod after makedirs for a newly created directory since makedirs(mode=...) already sets permissions. Do not log errors when chmod fails but the mode is already correct.
Applied to files:
snapm/manager/_manager.py
📚 Learning: 2025-10-22T20:06:54.993Z
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.
Applied to files:
snapm/manager/_manager.py
📚 Learning: 2025-08-25T17:19:19.563Z
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.
Applied to files:
snapm/manager/_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). (28)
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-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-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
🔇 Additional comments (4)
snapm/manager/_manager.py (4)
1334-1335: LGTM!Correctly captures the presence of boot and revert entries before proceeding with the rename operation.
1337-1355: LGTM!The error handling for rename failure is comprehensive and correct:
- Deletes boot/revert entries before attempting rename
- On rename failure, restores the deleted entries
- Refreshes boot cache after restoration
- Restores lookup tables
- Uses bare
raiseto preserve exception tracebackAll previous review concerns have been properly addressed.
1357-1364: LGTM!Boot entries are correctly recreated after successful rename, and the boot cache is refreshed to maintain consistency.
1365-1381: LGTM!The rollback logic is comprehensive and handles all failure scenarios:
- Catches all relevant exception types (SnapmError, OSError, ValueError) that can be raised by boot entry creation functions
- Attempts to rollback the rename and restore boot entries
- Includes boot cache refresh in the rollback path
- Inner exception handler catches rollback failures and logs them appropriately
- Restores lookup tables correctly regardless of rollback outcome
- Uses bare
raiseto preserve exception tracebackAll previous review concerns have been properly addressed.
Resolves: #590 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
bmr-cymru
force-pushed
the
bmr-rename-bootable
branch
from
b8af917 to
2d5b1b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_boot.py (1)
311-333: Excellent test coverage for the rename fix!The test correctly verifies that boot and revert entries are preserved across a snapshot set rename, and that the
boot_idvalues change (confirming the entries were recreated rather than orphaned). The logic is sound and the cleanup is correct.Optional enhancements for even more thorough verification:
If you'd like to make the test more comprehensive, consider:
- Verify old entry deletion: Check that boot entry files containing "bootset0" no longer exist in the boot directory after rename.
- Verify entry content: Assert that the new boot entries contain the updated snapshot set name "bootset1" in their content.
- Clarity comment: Add a brief comment after line 323 noting that
ssetcontinues to reference the same object (now renamed to "bootset1"), which might help future readers.These are optional refinements that would make the test more explicit about the complete cleanup behavior, but the current test already validates the essential fix for issue #590.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snapm/manager/_manager.py(1 hunks)tests/test_boot.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- snapm/manager/_manager.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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: 357
File: snapm.spec:132-134
Timestamp: 2025-09-10T23:42:35.992Z
Learning: The snapm project maintainer bmr-cymru prefers scripted version bumping as part of the snapm-x.y.z-release-prep workflow rather than automated regex-based parsing for version synchronization across files.
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.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-10-22T16:43:42.894Z
Learning: In snapm/manager/_manager.py _check_snapm_dir: the correct order for directory validation is: 1) check if path exists and validate it's a directory (not a symlink), 2) os.makedirs() to create if needed, 3) os.chmod() to fix permissions if directory existed with wrong perms, 4) validate final mode matches expected. Do not attempt chmod after makedirs for a newly created directory since makedirs(mode=...) already sets permissions. Do not log errors when chmod fails but the mode is already correct.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-03T15:21:15.412Z
Learning: In the snapm codebase, the `_with_snapset_lock` decorator introduced in PR #171 is only applied to Manager class methods, not Scheduler class methods. The Scheduler class remains unchanged in this locking implementation.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-30T13:46:15.300Z
Learning: In snapm, boom is a hard requirement because snapm.manager._boot imports boom modules directly, and the Manager class imports and uses functionality from _boot including BootCache, create_snapset_boot_entry, and delete_snapset_boot_entry. The Manager class cannot be instantiated without boom being available.
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: 171
File: snapm/manager/_manager.py:435-490
Timestamp: 2025-09-25T03:07:36.881Z
Learning: In the snapm project, during long-running revert operations, individual snapshots complete their merge and are removed from the sources list progressively. If UUIDs were derived from sorted_sources + timestamp, they would change repeatedly during a single operation, making it impossible for monitoring systems to track operations consistently.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 171
File: snapm/manager/_manager.py:435-490
Timestamp: 2025-09-25T02:09:51.734Z
Learning: The snapm locking implementation has a race condition during rename operations: one thread can hold an "old_name" lock while another thread acquires a "new_name" lock for the same snapshot set, breaking mutual exclusion since both locks refer to the same logical entity.
🧬 Code graph analysis (1)
tests/test_boot.py (2)
snapm/manager/_manager.py (5)
find_snapshot_sets(1075-1096)create_snapshot_set_boot_entry(1719-1743)create_snapshot_set_revert_entry(1747-1761)rename_snapshot_set(1312-1386)delete_snapshot_sets(1409-1424)snapm/_snapm.py (3)
Selection(370-633)name(993-997)name(1488-1492)
⏰ 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). (45)
- GitHub Check: rpm-build:fedora-rawhide-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:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: python-basic-tests
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.
Resolves: #590
Summary by CodeRabbit
Bug Fixes
Tests