Conversation
bmr-cymru
added
Bug
WalkthroughUpdated LVM2 snapshot sizing and diagnostics: extent-size retrieval and rounding added, CoW snapshot shrinking explicitly rejected, debug logging added during create/resize flows, plugin APIs updated to return extent size, tests and manpage updated, and CI workflow adjusted. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Snapm as Snapm Core
participant Manager
participant LVM2Plugin as LVM2 Plugin
participant LVM2 as LVM2 Tools
User->>Snapm: request create/resize snapshot
Snapm->>Manager: orchestrate snapshot operation
Manager->>LVM2Plugin: _check_free_space(origin, mount_point, policy, snapshot_name?)
LVM2Plugin->>LVM2: vgs --reportfields vg_name,vg_free,vg_extent_size
LVM2-->>LVM2Plugin: (vg_free, vg_extent_size)
LVM2Plugin->>LVM2Plugin: _round_up_extents(policy.size, vg_extent_size)
LVM2Plugin->>LVM2Plugin: determine current_size if snapshot exists
alt rounded_target < current_size
LVM2Plugin-->>Manager: error (shrink not supported)
else rounded_target == current_size
LVM2Plugin-->>Manager: needed_space = 0 (no-op)
else
LVM2Plugin-->>Manager: needed_space = rounded_target - current_size (or rounded_target for new)
end
Manager->>Manager: debug-log provider/transaction size_map
alt needed_space > 0
Manager->>LVM2: lvresize -L+<size_change>
LVM2-->>Manager: OK
Manager-->>User: Done
else needed_space == 0
Manager-->>User: No-op / already sized
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
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: 2
🧹 Nitpick comments (5)
man/man8/snapm.8 (1)
847-850: Fix provider name and align wording with the implementation (no shrink support).The plugin is “lvm2-cow”, and the text should reflect that shrinking is not supported by LVM2 for CoW snapshots (which the code now enforces).
-The \fBlvm2cow\fP plugin does not support shrinking snapshots as this is -currently not supported by the LVM2 tools. +The \fBlvm2-cow\fP plugin does not support shrinking snapshots, as this is +not supported by the LVM2 tools.snapm/manager/_manager.py (1)
968-974: Pre‑journal debug of provider size maps: verify presence.Same note as elsewhere: ensure
size_mapexists on all providers inprovider_mapbefore logging, or guard withgetattrto avoid surprises if a provider doesn’t participate in the current transaction.snapm/manager/plugins/lvm2.py (3)
1136-1150: Update _check_free_space docstring to match signature.Docstring still refers to
vg_name; please align parameter docs withorigin, mount_point, size_policy, snapshot_name=None.
1156-1160: Make snapshot LV name parsing more robust.Guard against unexpected
snapshot_nameformats.- current_size = ( - self.lv_dev_size(vg_name, snapshot_name.split("/", maxsplit=1)[1]) if snapshot_name else 0 - ) + current_size = 0 + if snapshot_name: + _, _, snap_lv = snapshot_name.partition("/") + if not snap_lv: + raise SnapmPluginError(f"Malformed snapshot name: {snapshot_name}") + current_size = self.lv_dev_size(vg_name, snap_lv)
1188-1191: Error message is clear; consider using size_fmt for both operands.You already use
size_fmt—good. Nothing to change here beyond the broader note on TRY003 from linters (optional).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
man/man8/snapm.8(2 hunks)snapm/_snapm.py(1 hunks)snapm/manager/_manager.py(1 hunks)snapm/manager/plugins/lvm2.py(9 hunks)tests/test_lvm2.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#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.
📚 Learning: 2025-08-22T18:27:31.307Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-22T18:27:31.307Z
Learning: The snapm project only supports GNU Groff markup for man pages and does not support other troff toolchains. All man page formatting should follow GNU Groff standards and practices.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-09-14T02:14:30.316Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#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
🧬 Code graph analysis (4)
tests/test_lvm2.py (1)
snapm/manager/plugins/lvm2.py (2)
size(371-373)_round_up_extents(244-258)
snapm/manager/_manager.py (2)
snapm/manager/plugins/_plugin.py (1)
_log_debug(104-108)snapm/_snapm.py (2)
name(894-898)name(1393-1397)
snapm/_snapm.py (1)
snapm/manager/plugins/_plugin.py (1)
_log_debug(104-108)
snapm/manager/plugins/lvm2.py (3)
snapm/_snapm.py (9)
SnapmSizePolicyError(212-215)size_fmt(565-579)origin(1430-1434)mount_point(1458-1462)SizePolicy(639-718)size(698-718)size(1502-1509)SnapmNoSpaceError(200-203)SnapmCalloutError(194-197)snapm/manager/plugins/stratis.py (3)
origin(214-215)_snapshot_min_size(296-304)size(239-241)snapm/manager/plugins/_plugin.py (2)
mount_point_space_used(439-449)_log_debug(104-108)
🪛 Ruff (0.13.1)
snapm/manager/plugins/lvm2.py
253-253: Avoid specifying long messages outside the exception class
(TRY003)
256-256: Avoid specifying long messages outside the exception class
(TRY003)
1182-1184: Avoid specifying long messages outside the exception class
(TRY003)
1188-1191: Avoid specifying long messages outside the exception class
(TRY003)
1295-1295: Unused method argument: mount_point
(ARG002)
1295-1295: Unused method argument: size_policy
(ARG002)
1309-1311: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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:centos-stream-10-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:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-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:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-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:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-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:fedora-43-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-stream9)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: python-basic-tests
🔇 Additional comments (7)
tests/test_lvm2.py (2)
40-72: Good coverage for rounding behavior.Edge cases (zero, unit extent, varied sizes) and happy paths are well covered.
73-84: Error‑path tests look good.Validation of zero/negative inputs aligns with the helper’s contract.
snapm/manager/plugins/lvm2.py (4)
120-121: Including vg_extent_size in VGS fields is appropriate; confirm availability across supported LVM versions.You already gate json_std vs json; please verify
vg_extent_sizeexists in both formats for your minimum versions.
244-259: _round_up_extents is correct and safe.Input validation and integer math are solid. Tests exercise the core cases.
1240-1247: Create path logs snapshot size using size_map: LGTM.Uses the computed (rounded) size; consistent with new checks.
1295-1314: No‑op resize handling is correct.Skips lvresize when size_change is zero and logs a debug message.
snapm/_snapm.py (1)
1198-1204: No AttributeError — size_map is defined on Plugin; optionally coerce None to {} when logging.snapm/manager/plugins/_plugin.py sets self.size_map = None in init, start_transaction sets {} and end_transaction resets None, so providers always have the attribute. If you want logs to show an empty map instead of None, use: size_map = provider.size_map if provider.size_map is not None else {} and log size_map.
Likely an incorrect or invalid review comment.
Resolves: #488 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
|
@coderabbitai: re:
Eh, see |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 22739 @bmr-cymru You're absolutely right, and thank you for the clarification! After examining the Plugin base class, I can see that:
My previous concern about I appreciate you pointing me to the transaction lifecycle methods - it's clear that the architecture ensures proper initialization across all providers participating in any transaction. 🧠 Learnings used |
bmr-cymru
force-pushed
the
bmr-resize-fixes
branch
from
4ca0a9f to
f731dfb
Compare
bmr-cymru
force-pushed
the
bmr-resize-fixes
branch
from
f731dfb to
da66e68
Compare
|
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. |
2 similar comments
|
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. |
|
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. |
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 (7)
.github/workflows/snapm.yml(2 hunks)man/man8/snapm.8(1 hunks)snapm/_snapm.py(1 hunks)snapm/manager/_manager.py(1 hunks)snapm/manager/plugins/lvm2.py(9 hunks)tests/bin/vgs(4 hunks)tests/test_lvm2.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_lvm2.py
- snapm/_snapm.py
- snapm/manager/_manager.py
- man/man8/snapm.8
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#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.
📚 Learning: 2025-09-10T23:42:35.992Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#357
File: snapm.spec:132-134
Timestamp: 2025-09-10T23:42:35.992Z
Learning: In the snapm project, changelog entries in snapm.spec are auto-generated from git log using `git log --oneline v0.4.0..v0.5.0 | sed -e 's/^[a-f0-9]* /- /' -e 's/%//'` and should not be manually edited to avoid rewriting git history.
Applied to files:
.github/workflows/snapm.yml
📚 Learning: 2025-09-14T02:14:30.316Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#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-08-26T19:07:42.290Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#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.
Applied to files:
snapm/manager/plugins/lvm2.py
🧬 Code graph analysis (1)
snapm/manager/plugins/lvm2.py (2)
snapm/_snapm.py (10)
SnapmSizePolicyError(212-215)size_fmt(565-579)origin(1426-1430)mount_point(1454-1458)SnapmPluginError(256-259)SizePolicy(639-718)size(698-718)size(1498-1505)SnapmNoSpaceError(200-203)SnapmCalloutError(194-197)snapm/manager/plugins/_plugin.py (1)
mount_point_space_used(439-449)
🪛 Ruff (0.13.1)
snapm/manager/plugins/lvm2.py
253-253: Avoid specifying long messages outside the exception class
(TRY003)
256-256: Avoid specifying long messages outside the exception class
(TRY003)
1162-1162: Avoid specifying long messages outside the exception class
(TRY003)
1188-1190: Avoid specifying long messages outside the exception class
(TRY003)
1194-1197: Avoid specifying long messages outside the exception class
(TRY003)
1301-1301: Unused method argument: mount_point
(ARG002)
1301-1301: Unused method argument: size_policy
(ARG002)
1316-1318: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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: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: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-41-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:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-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: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:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: python-basic-tests
🔇 Additional comments (13)
.github/workflows/snapm.yml (1)
117-117: LGTM! Consistent version alignment.The update to stratis-cli v3.8.5 properly aligns with the stratisd version change, ensuring compatibility between the daemon and CLI components.
tests/bin/vgs (2)
6-8: LGTM! Field options updated to match main code.The
VGS_FIELD_OPTIONShas been correctly updated to includevg_extent_size, aligning with the changes in the main LVM2 plugin code that now requires extent size information for size calculations.
14-22: LGTM! Mock data updated with extent size fields.All three mock volume group responses (
fedora,vg_hex,appdata) have been correctly updated to include thevg_extent_sizefield with the standard 4MB (4194304B) extent size. This aligns with the API change in the main code that now returns(vg_free, vg_extent_size)tuples.Also applies to: 24-32, 34-42
snapm/manager/plugins/lvm2.py (10)
22-22: LGTM! Imports updated for enhanced functionality.The addition of
SnapmSizePolicyErrorandsize_fmtimports properly supports the new extent-rounding logic and enhanced error messaging functionality implemented in this PR.Also applies to: 32-32
120-120: LGTM! VGS field options extended for extent size.The update to include
vg_extent_sizein the VGS field options is essential for the new extent-rounding functionality and aligns with the mock test data changes.
244-259: LGTM! Well-designed extent rounding utility.The
_round_up_extentsfunction is correctly implemented with:
- Proper input validation for both parameters
- Clear mathematical logic using ceiling division
- Appropriate docstring documentation
This addresses the PR objective to implement extent-boundary rounding functionality.
618-630: API breaking change handled correctly.The change from returning
intto(vg_free: int, vg_extent_size: int)tuple is well-implemented and necessary for extent-rounding calculations. The mock data and tests have been appropriately updated to handle the new return signature.
1136-1199: Excellent implementation of enhanced space checking logic.This comprehensive implementation properly addresses multiple PR objectives:
- Extent-boundary rounding: Uses
_round_up_extentswith retrieved VG extent size- Shrinking prohibition: Prevents CoW snapshot shrinking operations
- Free-space accounting: Correctly calculates needed space as
rounded_size - current_size- Debug logging: Comprehensive logging of size policy calculations
- Resize operation support: Handles resize logic with
snapshot_nameparameterThe function correctly:
- Retrieves VG extent size using the new tuple API
- Applies size policy with proper parameters
- Rounds sizes to extent boundaries
- Detects and rejects shrinking attempts
- Calculates accurate space requirements
- Provides detailed debug logging
1230-1234: LGTM! Proper API integration for create snapshot checks.The call to
_check_free_spacecorrectly passes the required parameters without the optionalsnapshot_nameparameter, which is appropriate for create operations (not resize operations).
1244-1265: LGTM! Improved create snapshot implementation.The changes properly:
- Use the calculated
snapshot_sizefrom the size map- Enhance logging with human-readable size formatting
- Apply the computed size directly to the
lvcreatecommandThis addresses the PR objective to use computed sizes from size policy calculations.
1294-1299: LGTM! Resize check passes snapshot name correctly.The call to
_check_free_spaceproperly includes thesnapshot_nameparameter, which enables the resize logic path in the space checking function to determine current size and prevent shrinking operations.
1301-1321: Excellent resize implementation with no-op detection.This implementation properly addresses PR objectives:
- No-op resize avoidance: Skips
lvresizewhensize_changeis zero- Relative size usage: Uses
+{size_change}bfor relative resize operations- Enhanced logging: Provides clear debug messages for both resize and no-op cases
The logic correctly uses the calculated size difference from the size map and only performs the actual resize operation when needed.
1301-1301: False positive: Method arguments are used correctly.The static analysis warning about unused
mount_pointandsize_policyparameters appears to be a false positive. These parameters are indirectly used via theself.size_map[vg_name][lv_name]lookup, where the size value was computed during thecheck_resize_snapshotcall that used these parameters.
bmr-cymru
force-pushed
the
bmr-resize-fixes
branch
from
7f437e3 to
6d98163
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/snapm.yml (3)
104-104: Drop “components: cargo” (cargo ships with the toolchain)Specifying cargo as a component is unnecessary; it’s included by default and can occasionally cause resolution quirks.
Apply this diff:
- components: cargo
107-107: Use shallow clone to speed up CI and reduce network flakinessShallow cloning the tagged ref is sufficient and faster.
Apply this diff:
- run: git clone https://github.com/stratis-storage/stratisd -b stratisd-v3.8.5 + run: git clone --depth 1 -b stratisd-v3.8.5 https://github.com/stratis-storage/stratisd
118-118: Shallow clone stratis-cli as wellConsistent with the above; speeds CI.
Apply this diff:
- run: git clone https://github.com/stratis-storage/stratis-cli -b v3.8.5 + run: git clone --depth 1 -b v3.8.5 https://github.com/stratis-storage/stratis-cli
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/snapm.yml(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#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.
⏰ 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:fedora-41-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:fedora-rawhide-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: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:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-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:centos-stream-9-x86_64
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- 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, fedora42)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: python-basic-tests
🔇 Additional comments (1)
.github/workflows/snapm.yml (1)
105-105: Pin Rust to stratisd’s declared toolchain or verify 1.85.0 compatibility — upstream tag missingI attempted to fetch stratisd's rust-toolchain for tag "stratisd-v3.8.5" but that tag and rust-toolchain files were not found (repo tags go up to v3.5.7). Update .github/workflows/snapm.yml (toolchain: 1.85.0) to match the actual stratisd rust-toolchain.toml from the correct tag or main, or keep 1.85.0 but add a short compatibility justification/verification step.
Likely an incorrect or invalid review comment.
bmr-cymru
force-pushed
the
bmr-resize-fixes
branch
from
6d98163 to
6cfcf47
Compare
Build errors have appeared in CI:
error: unnecessary parentheses around closure body
--> src/dbus_api/filesystem/prop_conv.rs:30:36
|
30 | option_to_tuple(origin.map(|u| (uuid_to_string!(u))), String::new())
| ^ ^
|
= note: `-D unused-parens` implied by `-D unused`
= help: to override `-D unused` add `#[allow(unused_parens)]`
help: remove these parentheses
|
30 - option_to_tuple(origin.map(|u| (uuid_to_string!(u))), String::new())
30 + option_to_tuple(origin.map(|u| uuid_to_string!(u)), String::new())
|
error: could not compile `stratisd` (lib) due to 1 previous error
make: *** [Makefile:121: build] Error 101
Error: Process completed with exit code 2.
This appears to be a result of dtolnay/rust-toolchain@master moving up
to Rust 1.90.0: testing with the latest release and current development
toolchain (1.85.0) the build passes.
Resolves: #491
Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Resolves: #492 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
bmr-cymru
force-pushed
the
bmr-resize-fixes
branch
from
6cfcf47 to
52eaf0c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/snapm.yml (3)
57-57: Pin boom-boot checkout to a tag or commit for reproducible CI.Cloning the default branch can introduce non‑determinism across runs. Prefer a release tag or commit SHA.
104-104: Pin the rust-toolchain action to a commit SHA (and consider centralizing the version).Using @master is mutable; pin to a specific commit for supply‑chain safety. Optionally define the Rust version once (env/matrix) to simplify future bumps.
106-106: LGTM on stratisd v3.8.5 shallow clone; optionally cache Cargo to speed builds.No issues with the version or shallow clone. For faster CI, add an actions/cache step for Cargo registry and target dirs keyed by toolchain + Cargo.lock.
Example step to insert before “Build stratisd”:
- name: Cache cargo uses: actions/cache@v4 with: path: | ~/.cargo/registry ~/.cargo/git /var/tmp/stratisd/target key: cargo-${{ runner.os }}-rust-1.85.0-${{ hashFiles('/var/tmp/stratisd/Cargo.lock') }} restore-keys: | cargo-${{ runner.os }}-rust-1.85.0-
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/snapm.yml(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#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.
📚 Learning: 2025-08-25T19:20:21.720Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-25T19:20:21.720Z
Learning: In the snapm virt_tests workflow, optimizing Git checkouts by using virtiofs mapping is complex because it requires checkouts for both `snapm` and `boom` repositories. Git checkouts aren't the dominant time cost, so the current approach of cloning within VMs is more practical than adding workflow complexity and local user setup requirements.
Applied to files:
.github/workflows/snapm.yml
📚 Learning: 2025-09-10T23:42:35.992Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#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.
Applied to files:
.github/workflows/snapm.yml
📚 Learning: 2025-08-30T13:46:15.300Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#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.
Applied to files:
.github/workflows/snapm.yml
📚 Learning: 2025-08-30T14:13:38.717Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#454
File: README.md:36-38
Timestamp: 2025-08-30T14:13:38.717Z
Learning: The boom-boot package structure: `boom-boot` is the leaf package containing the binaries in all distributions, while `python3-boom` contains only the Python modules. For installation instructions, `boom-boot` is the correct package name to recommend.
Applied to files:
.github/workflows/snapm.yml
⏰ 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). (29)
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-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-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-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:centos-stream-9-x86_64
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- 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 (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: python-basic-tests
🔇 Additional comments (1)
.github/workflows/snapm.yml (1)
117-117: LGTM: stratis-cli v3.8.2 is appropriate.Matches current availability and prior discussion; shallow clone is fine.
|
@coderabbitai: re:
I've updated #482 to make it clear that there's no need to store the
See #487 - this explicitly requests
See #484 and commit 56a4900. The extent rounding is always applied to the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@bmr-cymru Thank you for the detailed clarifications! I can confirm the implementation details you've referenced:
The implementation is well-structured and addresses all the resize operation requirements comprehensively. Thank you for the references to the specific issues and commits - they helped clarify the design decisions behind this approach. 🏁 Script executed: Length of output: 3166 @bmr-cymru Thank you for the detailed clarifications! I can confirm the implementation details you've referenced:
The approach of retrieving 🏁 Script executed: Length of output: 8971 @bmr-cymru Perfect! Now I can see all three implementation details confirmed in the code:
The implementation elegantly addresses all the resize operation requirements. The approach of retrieving 🧠 Learnings used |
|
@coderabbitai: please regenerate the summary and Pre-merge checks to account for the points in my last comment that address the issues flagged:
|
|
@bmr-cymru Thank you for the clarifications! Based on your explanations, I can confirm that the implementation correctly addresses all the resize operation requirements:
These implementation details demonstrate full compliance with the linked issues' requirements. I'll regenerate the summary to reflect these clarifications and mark the linked-issues compliance as passed. 🧠 Learnings used✅ Actions performedSummary regeneration triggered. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
1 participant
Fixes for the
lvm2cowplugin resize ops, resize logging enhancements toManagerandSnapshotSetand doc change to note the limitations of "resize" for CoW volumes. Additionally include fixes for Rust toolchain updates that break thestratisdbuild and a minor optimization togit cloneoperations.Resolves: #150
Resolves: #151
Resolves: #152
Resolves: #482
Resolves: #483
Resolves: #484
Resolves: #486
Resolves: #487
Resolves: #488
Resolves: #489
Resolves: #491
Resolves: #492
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation
Tests / Fixtures / CI