feat: add support for snapshot manager backing the role #97
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| lv = list_item["lv"] | ||
| except KeyError: | ||
| vg = None | ||
| lv = None |
Check notice
Code scanning / CodeQL
Unused local variable Note
trgill
force-pushed
the
snapm_support
branch
6 times, most recently
from
985ca4d to
87263a3
Compare
Reviewer's GuideThis PR refactors the snapshot role to support an alternative Snapshot Manager backend while maintaining compatibility with the existing LVM-based implementation. The monolithic snapshot.py library is slimmed down into a lightweight wrapper that imports and delegates to new module_utils under snapshot_lsr; commands are routed via use_snapshot_manager() based on snapm availability. Core logic is split into dedicated lvm, utils, snapmgr, validate, and consts modules, tests are updated to cover mount/umount operations and edge cases, and variable files now conditionally include snapm packages on supported distributions. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @trgill - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
-
Incorrect signature for
mgr_remove_cmd(link) -
In snapshot_cmd_execute(), there’s no default branch if
cmddoesn’t match any SnapshotCommand, which can lead to returning None—add a fallback or explicit error for unrecognized commands. -
The removal of the LC_ALL and LVM_COMMAND_PROFILE exports risks breaking JSON output parsing for
lvm fullreport—either restore these environment settings or clearly document that they must be set externally.
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 8 other issues
- 🟢 Review instructions: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def snapshot_cmd_execute(cmd, module, module_args, snapset_dict, vg_include): | ||
| cmd_result = None | ||
|
|
||
| if module_args["snapshot_lvm_verify_only"]: | ||
| rc, message = extend_verify_snapshot_set(module, snapset_dict) | ||
| if use_snapm: | ||
| if cmd == SnapshotCommand.SNAPSHOT: | ||
| cmd_result = mgr_snapshot_cmd(module, module_args, snapset_dict) | ||
| elif cmd == SnapshotCommand.CHECK: | ||
| cmd_result = mgr_check_cmd(module, module_args, snapset_dict) | ||
| elif cmd == SnapshotCommand.REMOVE: | ||
| cmd_result = mgr_remove_cmd(module_args, snapset_dict) |
There was a problem hiding this comment.
issue (bug_risk): Handle unexpected commands in snapshot_cmd_execute
Add a default else branch to log or return an explicit error to avoid returning None.
| elif cmd == SnapshotCommand.CHECK: | ||
| cmd_result = mgr_check_cmd(module, module_args, snapset_dict) | ||
| elif cmd == SnapshotCommand.REMOVE: | ||
| cmd_result = mgr_remove_cmd(module_args, snapset_dict) |
There was a problem hiding this comment.
issue (bug_risk): Incorrect signature for mgr_remove_cmd
Update the call to mgr_remove_cmd to pass module along with module_args and snapset_dict, matching the other manager functions.
| return ( | ||
| SnapshotStatus.ERROR_EXTEND_NOT_SNAPSHOT, | ||
| "LV with name: " + vg_name + "/" + snapshot_name + " is not a snapshot", | ||
| changed, | ||
| ) | ||
| else: | ||
| return ( | ||
| SnapshotStatus.ERROR_EXTEND_NOT_FOUND, | ||
| "snapshot not found with name: " + vg_name + "/" + snapshot_name, | ||
| changed, | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): Use f-string instead of string concatenation [×5] (use-fstring-for-concatenation)
|
|
||
| rc, _vg_exists, lv_exists = lvm_lv_exists(module, vg, snapshot_name) | ||
| if rc != SnapshotStatus.SNAPSHOT_OK: | ||
| return ( |
There was a problem hiding this comment.
issue (code-quality): Use f-string instead of string concatenation [×5] (use-fstring-for-concatenation)
| if suffix: | ||
| suffix_len = len(suffix) | ||
| else: | ||
| suffix_len = 0 | ||
|
|
||
| if len(lv_name) + suffix_len > MAX_LVM_NAME: | ||
| return ( | ||
| SnapshotStatus.ERROR_NAME_TOO_LONG, | ||
| "resulting snapshot name would exceed LVM maximum: " + lv_name + suffix, | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Replace if statement with if expression (
assign-if-exp) - Use f-string instead of string concatenation (
use-fstring-for-concatenation)
| ) | ||
|
|
||
| if not lv_exists: | ||
| return ( |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use f-string instead of string concatenation [×3] (
use-fstring-for-concatenation) - Remove redundant conditional (
remove-redundant-if)
| return ( | ||
| SnapshotStatus.ERROR_VG_NOTFOUND, | ||
| "source volume group does not exist: " + vg_name, | ||
| ) | ||
| if lv_name and not lv_found: | ||
| return ( | ||
| SnapshotStatus.ERROR_LV_NOTFOUND, | ||
| "source logical volume does not exist: " + vg_name + "/" + lv_name, | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): Use f-string instead of string concatenation [×4] (use-fstring-for-concatenation)
|
[citest] |
|
[citest] |
|
Need to remove the package definitions from the vars/Platform_X.yml files - vars/main.yml has the logic to determine which packages are available for which platform + version Need vars/CentOS_9.yml as a symlink to vars/RedHat_9.yml Need vars/CentOS_10.yml as a symlink to vars/RedHat_10.yml |
|
[citest] |
Snapshot Manager is available on Centos 10, RHEL 9 and RHEL 10. Update to be compatiable with legacy code that is already deployed. Note that Snapshot Manager has minimum sizes for any snapshot of an origin. If the minimum size is used, an extend may not expand the size if the additional space is still below the minimum size. Currently the minimum size for snapm is 512 MiB. Future versions will decrease to 64 MiB. Some adjustment to the extend tests were made in this change to deal with failures of verify expecting expand to return true when the requested size was still below the minimum. Signed-off-by: trgill <[email protected]> Co-authored-by: Richard Megginson <[email protected]>
|
[citest] |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #97 +/- ##
====================================
Coverage ? 0
====================================
Files ? 0
Lines ? 0
Branches ? 0
====================================
Hits ? 0
Misses ? 0
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Snapshot Manager is available on Centos 10, RHEL 9.6 and later
Update to be compatiable with legacy code that is already deployed.
Enhancement:
Use Snapshot Manager as a interface to LVM volumes rather than the LVM CLI.
#96
Reason:
Snapshot Manager will allow support for more volume types that are not just LVM. (Stratis, BTRFS)
Result:
Issue Tracker Tickets (Jira or BZ if any):
RHEL-64447
RHEL-61488
Summary by Sourcery
Add support for using the Snapshot Manager library as a backend for snapshot operations and refactor the existing LVM-based implementation into a modular ansible module_utils package. Introduce a feature flag to prefer snapm when available, keep the legacy LVM CLI path as fallback, and adjust role defaults and test suites accordingly.
New Features:
Enhancements:
Tests: