snapm: btrfs plugin pre-requisite features and fixes #843
Conversation
bmr-cymru
added
enhancement
WalkthroughAdds plugin-priority support across code, docs and tests: Snapshot and Plugin become ABCs; plugins read/configure numeric priorities (with static defaults for lvm2/stratis); Manager resolves the highest-priority provider per source; CLI/reporting and debug expose plugin diagnostics; sample configs/docs updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Manager
participant Config as "plugins.d/*.conf"
participant PluginA as "Plugin A"
participant PluginB as "Plugin B"
Note over Manager,Config: Load per-plugin configs and gather candidate providers
User->>Manager: Request operation for source X
Manager->>PluginA: can_snapshot(source X)?
PluginA-->>Manager: True
Manager->>PluginB: can_snapshot(source X)?
PluginB-->>Manager: True
Manager->>Config: Read PluginPriority for A and B
Manager->>Manager: Compare priorities (A vs B)
alt B has higher priority
Manager->>PluginB: perform operation on source X
PluginB-->>Manager: Success
else A higher or only A
Manager->>PluginA: perform operation on source X
PluginA-->>Manager: Success
end
Manager->>User: Return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
snapm/manager/_manager.py (1)
931-981: Sources without a provider are silently dropped instead of raisingSnapmNoProviderErrorThe new multi-provider logic and
get_highest_priority()helper are a good fit for the priority model, but there’s a subtle regression in how missing providers are handled:
provider_mapis adefaultdict(list), and you only touchprovider_map[source]insideif plugin.can_snapshot(source): provider_map[source].append(plugin).- If no plugin claims a given
source, that key is never created.- The verification loop iterates
for source in provider_map:, so sources with no providers are completely skipped and never triggerSnapmNoProviderError.- The returned mapping omits those sources entirely, so later code (e.g.
create_snapshot_set) just never processes them, resulting in silent partial snapshot sets.The contract in the docstring (“verify that a provider exists for each source present”) suggests we should fail if any requested source has no provider.
You can fix this by validating against the full
sourceslist, and then selecting the highest-priority provider:Proposed fix for provider selection and verification
- def get_highest_priority(providers): + def get_highest_priority(providers): if not providers: - raise ValueError("Empty list") + raise ValueError("Empty provider list") max_priority = max(prov.priority for prov in providers) candidates = [prov for prov in providers if prov.priority == max_priority] if len(candidates) > 1: names = ",".join(prov.name for prov in candidates) raise SnapmNoProviderError( f"Multiple providers with priority {max_priority}: {names}" ) return candidates[0] # Initialise provider mapping. provider_map = defaultdict(list) @@ - # Verify each mount point has a provider plugin - for source in provider_map: - if provider_map[source] == []: - raise SnapmNoProviderError( - f"Could not find snapshot provider for {source}" - ) - - # Select highest priority provider for each source - return { - src: get_highest_priority(providers) - for src, providers in provider_map.items() - } + # Verify each requested source has at least one provider, then + # select the highest-priority provider per source. + resolved = {} + for source in sources: + providers = provider_map.get(source, []) + if not providers: + raise SnapmNoProviderError( + f"Could not find snapshot provider for {source}" + ) + resolved[source] = get_highest_priority(providers) + + return resolvedThis restores the expected behaviour: every requested source must have exactly one effective provider after priority resolution, or the operation fails clearly.
🧹 Nitpick comments (3)
snapm/manager/plugins/_plugin.py (2)
82-108: Improve robustness when logging invalid PluginLimits valuesThe new try/except around
getint()with a warning that includes the offending value andcfg.plugin_conf_fileis a nice usability improvement. However, this assumescfgalways has aplugin_conf_fileattribute; ifPluginLimitsis ever constructed with a plainConfigParser(e.g. in tests or helper code), this will raiseAttributeErrorinstead of warning cleanly.You could make this more robust by falling back when the attribute is missing:
Suggested defensive tweak
- _log_warn( - "Ignoring invalid plugin %s value in %s: %s", - _PLUGIN_CFG_SNAPS_PER_ORIGIN, - cfg.plugin_conf_file, - str(value), - ) + conf_name = getattr(cfg, "plugin_conf_file", "<unknown>") + _log_warn( + "Ignoring invalid plugin %s value in %s: %s", + _PLUGIN_CFG_SNAPS_PER_ORIGIN, + conf_name, + str(value), + )(and similarly for
_PLUGIN_CFG_SNAPS_PER_POOL).
48-56: Priority configuration and validation look sound, with a small caveatThe priority configuration path is well-structured: you read
PluginPriority, attemptgetint(), fall back to the raw value onValueError, then accept only positive integers, treating0, negatives, and non-ints as invalid and logging before resetting toPLUGIN_NO_PRIORITY. ExportingPLUGIN_NO_PRIORITYand documenting it as a sentinel for subclasses is also clear.One small robustness point: like
PluginLimits, the invalid-priority warning usesplugin_cfg.plugin_conf_file, which relies on the caller having set that attribute (Manager does, but ad-hoc users ofPluginmay not). If you expectPluginto be constructed in tests or tooling with a bareConfigParser, the samegetattr(..., "<unknown>")pattern as suggested forPluginLimitswould avoid surprisingAttributeErrors.Otherwise, the semantics mesh cleanly with the Manager’s new highest-priority selection logic.
Also applies to: 120-154, 542-542
snapm/manager/_manager.py (1)
899-917: Plugin config path/metadata handling is good; consider always definingplugin_conf_fileUsing
plugin_conf_file = f"{plugin_name}.conf"andplugin_conf_path = join(_PLUGINS_D_PATH, plugin_conf_file)makes the logging clearer, and attachingcfg.plugin_conf_filewhen a config file exists enables nicer diagnostics inPluginLimitsand the priority parsing.For consistency and robustness, you might want to set
cfg.plugin_conf_fileunconditionally (e.g. before theexists()check, or defaulting it if the file is absent) so downstream code can always rely on the attribute being present, even when a plugin has no on-disk config:Example tweak
- cfg = ConfigParser() - - if exists(plugin_conf_path): - _log_debug("Loading plugin configuration from '%s'", plugin_conf_path) - cfg.read([plugin_conf_path]) - cfg.plugin_conf_file = plugin_conf_file # for debugging + cfg = ConfigParser() + # Always record the logical config file name for logging. + cfg.plugin_conf_file = plugin_conf_file + + if exists(plugin_conf_path): + _log_debug("Loading plugin configuration from '%s'", plugin_conf_path) + cfg.read([plugin_conf_path])This would dovetail nicely with the new warnings in
PluginLimitsandPlugin.__init__().
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
etc/snapm/plugins.d/lvm2-cow.confetc/snapm/plugins.d/lvm2-thin.confetc/snapm/plugins.d/stratis.confman/man5/snapm-plugins.d.5man/man8/snapm.8snapm/_snapm.pysnapm/command.pysnapm/manager/_manager.pysnapm/manager/plugins/_plugin.pysnapm/manager/plugins/lvm2.pysnapm/manager/plugins/stratis.pytests/test_command.py
🧰 Additional context used
🧠 Learnings (14)
📓 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: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:31:58.457Z
Learning: In snapm, plugins are designed to fail loading when their required dependencies are missing (e.g., LVM2 plugin fails to load if device-mapper support is absent, Stratis plugin fails if D-Bus interfaces are missing). This is intentional behavior to ensure plugins only operate when all dependencies are available.
📚 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:
etc/snapm/plugins.d/lvm2-thin.confetc/snapm/plugins.d/lvm2-cow.confsnapm/manager/_manager.pysnapm/manager/plugins/lvm2.py
📚 Learning: 2025-09-14T17:31:58.457Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:31:58.457Z
Learning: In snapm, plugins are designed to fail loading when their required dependencies are missing (e.g., LVM2 plugin fails to load if device-mapper support is absent, Stratis plugin fails if D-Bus interfaces are missing). This is intentional behavior to ensure plugins only operate when all dependencies are available.
Applied to files:
etc/snapm/plugins.d/lvm2-thin.confetc/snapm/plugins.d/stratis.confman/man8/snapm.8etc/snapm/plugins.d/lvm2-cow.confman/man5/snapm-plugins.d.5snapm/manager/plugins/lvm2.py
📚 Learning: 2025-12-22T10:31:24.641Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 773
File: snapm.spec:148-149
Timestamp: 2025-12-22T10:31:24.641Z
Learning: In the snapm project's snapm.spec file, do not flag typos, formatting issues, or other problems in the %changelog section, as these entries are auto-generated from git commit messages and represent the historical record. Modifying them would require rewriting git history, which the project avoids.
Applied to files:
etc/snapm/plugins.d/stratis.confman/man8/snapm.8snapm/_snapm.pyetc/snapm/plugins.d/lvm2-cow.conf
📚 Learning: 2025-09-10T23:42:35.992Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 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:
etc/snapm/plugins.d/stratis.confman/man8/snapm.8etc/snapm/plugins.d/lvm2-cow.confman/man5/snapm-plugins.d.5
📚 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:
etc/snapm/plugins.d/stratis.confman/man8/snapm.8man/man5/snapm-plugins.d.5
📚 Learning: 2025-08-22T18:27:31.307Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 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-08-26T13:22:28.829Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-26T13:22:28.829Z
Learning: In the snapm project's man pages, single `.` lines are intentional spacers used to separate and improve readability between macro definitions and section headers like `.SH`/`.SS`. These should not be flagged as formatting errors or suggested for removal.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-09-10T23:42:35.992Z
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.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-11-12T21:27:50.414Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 596
File: snapm/_snapm.py:36-44
Timestamp: 2025-11-12T21:27:50.414Z
Learning: In the snapm codebase, the numeric values of SNAPM_DEBUG_* constants (e.g., SNAPM_DEBUG_MANAGER, SNAPM_DEBUG_MOUNTS, SNAPM_DEBUG_SCHEDULE) are not guaranteed to remain stable across versions. Consumers should always use the symbolic constant names rather than persisting numeric masks. SNAPM_DEBUG_ALL is designed to change over time as new subsystems are added, and repurposing or reassigning bit positions for debug flags is acceptable and intentional.
Applied to files:
man/man8/snapm.8snapm/_snapm.pysnapm/command.pytests/test_command.pysnapm/manager/plugins/_plugin.py
📚 Learning: 2025-08-23T22:16:04.019Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 400
File: man/man8/snapm.8:11-12
Timestamp: 2025-08-23T22:16:04.019Z
Learning: In man pages for the snapm project, prefer backslash (`\`) continuations over multiple macro invocations when breaking long command lists across lines, as this maintains a single macro invocation while being readable in 80x25 terminals.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-08-22T19:34:08.396Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-22T19:34:08.396Z
Learning: The snapshotmanager organization (including snapm and boom projects) uses "SHORTMONTH DAY YEAR" format for man page headers. This format has been used by boom since 2017. Organizational consistency within snapshotmanager is prioritized over cross-project consistency when it comes to formatting standards.
Applied to files:
man/man8/snapm.8
📚 Learning: 2025-10-10T19:18:28.912Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 524
File: snapm/command.py:90-92
Timestamp: 2025-10-10T19:18:28.912Z
Learning: In the snapm codebase, logging wrapper functions like `_log_debug_command`, `_log_debug_manager`, and `_log_debug_report` are module-internal helpers that intentionally do not merge caller-supplied `extra` kwargs. These wrappers hard-code the subsystem tag in `extra={"subsystem": SNAPM_SUBSYSTEM_*}`. It is a bug for callers to pass their own `extra` kwarg to these wrappers. This design keeps the code simple and avoids unnecessary defensive checks.
Applied to files:
snapm/_snapm.pysnapm/command.pytests/test_command.pysnapm/manager/plugins/_plugin.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:
snapm/manager/plugins/_plugin.py
🧬 Code graph analysis (2)
snapm/command.py (1)
snapm/report.py (2)
FieldType(232-286)report_num(397-410)
snapm/manager/_manager.py (1)
snapm/manager/plugins/_plugin.py (1)
_log_debug(173-177)
🪛 Ruff (0.14.10)
snapm/manager/_manager.py
933-933: Avoid specifying long messages outside the exception class
(TRY003)
940-942: Avoid specifying long messages outside the exception class
(TRY003)
973-975: 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). (35)
- 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:centos-stream-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: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:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-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:centos-stream-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-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: python-basic-tests
🔇 Additional comments (19)
etc/snapm/plugins.d/lvm2-thin.conf (1)
3-8: Documentation addition looks good.The commented
[Priority]section withPluginPriority = 15correctly documents the lvm2-thin static priority and provides a clear example for administrators who wish to override it.etc/snapm/plugins.d/stratis.conf (1)
3-8: Documentation addition looks good.The commented
[Priority]section withPluginPriority = 20correctly documents the stratis static priority and provides a clear example for administrators.etc/snapm/plugins.d/lvm2-cow.conf (1)
3-8: Documentation addition looks good.The commented
[Priority]section withPluginPriority = 10correctly documents the lvm2-cow static priority and provides a clear example for administrators.tests/test_command.py (1)
209-209: Test update correctly includes the new plugin debug token.The addition of
pluginto the debug token list aligns with the newSNAPM_DEBUG_PLUGINflag. Based on learnings,SNAPM_DEBUG_ALLis designed to evolve as new subsystems are added.man/man5/snapm-plugins.d.5 (1)
38-44: Documentation for the Priority section is clear and accurate.The new section correctly documents the
PluginPrioritykey and provides a practical use case for overriding default priorities between lvm2-cow and lvm2-thin.man/man8/snapm.8 (2)
646-646: Debug classes list correctly updated.The addition of
fsdiffandpluginto the debug classes documentation aligns with the new debug subsystems available in the codebase.
903-913: Plugin priority documentation is well-written.The new paragraphs clearly explain the priority mechanism (higher value wins) and correctly reference
snapm-plugins.d(5)for configuration override details.snapm/manager/plugins/lvm2.py (4)
42-42: Import correctly added for plugin priority support.The import of
PLUGIN_NO_PRIORITYenables the static priority defaulting logic.
180-184: Static priority constants correctly defined.The priority values (lvm2-cow=10, lvm2-thin=15) align with the PR objectives from issue #837, establishing clear precedence where lvm2-thin is preferred over lvm2-cow by default.
1067-1071: Lvm2Cow initialisation correctly applies static priority.The
originsdictionary initialisation and the conditional priority assignment ensure the plugin has a sensible default priority when none is configured.
1384-1385: Lvm2Thin correctly applies static priority when unconfigured.The pattern is consistent with Lvm2Cow, ensuring a coherent priority mechanism across LVM2 plugins.
snapm/manager/plugins/stratis.py (3)
44-44: Import correctly added for plugin priority support.The import of
PLUGIN_NO_PRIORITYenables the static priority defaulting logic.
83-84: Static priority constant correctly defined.The priority value (stratis=20) aligns with the PR objectives from issue #837.
466-467: Stratis correctly applies static priority after version checks.The placement after
check_stratisd_version()is appropriate — the priority is only set if the plugin successfully initialises. The pattern is consistent with LVM2 plugins.snapm/manager/plugins/_plugin.py (1)
57-67: Plugin debug wrapper follows established logging patternThe
_log_debug_plugin()helper cleanly mirrors the existing subsystem debug wrappers, fixing the subsystem tag viaextra={"subsystem": SNAPM_SUBSYSTEM_PLUGIN}and not attempting to merge caller-suppliedextra, which matches the intended design for these helpers.
Based on learnings, this is consistent with the project’s logging conventions.snapm/manager/_manager.py (1)
77-80: Manager debug wrapper correctly tags subsystem
_log_debug_manager()mirrors the other subsystem wrappers by hard-codingextra={"subsystem": SNAPM_SUBSYSTEM_MANAGER}and delegating to the module logger, which fits the existing logging and filtering design.
Based on learnings, this is aligned with how debug wrappers are expected to behave.snapm/_snapm.py (1)
37-53: Plugin debug subsystem wiring is consistent and self-containedAdding
SNAPM_DEBUG_PLUGIN, including it inSNAPM_DEBUG_ALL, mapping it via_DEBUG_MASK_TO_SUBSYSTEM, and exporting both the flag andSNAPM_SUBSYSTEM_PLUGINgives plugins first-class debug control without disturbing existing subsystems. This fits neatly with the existing debug-mask design that expects callers to use symbolic constants, not hard-coded integers.
Based on learnings, this respects the intended debug-mask semantics.Also applies to: 56-72, 2354-2355, 2370-2371
snapm/command.py (2)
450-489: Plugin priority reporting integrates cleanly with the report systemThe new
FieldTypeforPR_PLUGIN(name="priority",dtype=REP_NUM,report_fn=lambda f, d: f.report_num(d.priority)) is a good fit: priorities are treated as numeric for display and sorting, and including"priority"in_DEFAULT_PLUGIN_FIELDSmakes the new mechanism visible by default insnapm plugin list. This lines up with the basePlugin.priorityattribute introduced in the plugins module.
68-69: Command-line debug options correctly expose plugin debuggingImporting
SNAPM_DEBUG_PLUGINand adding a"plugin": SNAPM_DEBUG_PLUGINentry toset_debug()’smask_mapmeans users can now enable plugin-specific debug output via-d plugin(and it is also covered by-d all). This is consistent with how other subsystems are wired and relies on the symbolic constants rather than any assumed bit positions.
Based on learnings, this aligns with the intended debug-mask usage.Also applies to: 2406-2415
|
@coderabbitai: re:
And:
The |
|
Tip For best results, initiate chat on the files or code changes. @bmr-cymru: Thank you for the clarification! I understand now—since 🧠 Learnings used |
bmr-cymru
force-pushed
the
bmr-btrfs-plugin
branch
from
284ac8d to
5e93934
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
snapm/manager/_manager.py (1)
936-996: LGTM: Plugin priority resolution logic correctly implemented.The refactored logic successfully implements per-source highest-priority provider selection:
- Collects all capable providers per source using
defaultdict(list)- Resolves to a single provider per source by selecting the highest numerical priority
- Properly handles tie-breaking by raising
SnapmNoProviderErrorwhen multiple providers share the maximum priority- Validates that every source has at least one provider
The implementation aligns with PR objectives #836, #837, and #839 for plugin priority support.
Optional: Consider moving exception messages to exception classes
Static analysis flags the exception messages as potentially better suited to exception class definitions (Ruff TRY003 at lines 948, 955-957). This is a style preference rather than a functional issue.
snapm/manager/plugins/_plugin.py (1)
126-153: Verify that treating priority=0 in configuration as invalid is intentional.The validation logic at lines 141-145 treats
priority == PLUGIN_NO_PRIORITY(which equals 0) as invalid. This means if a user explicitly setsPluginPriority=0in the configuration, it will be rejected and reset toPLUGIN_NO_PRIORITY.According to issue #837, priorities are tiebreakers with higher numerical values preferred. If 0 is intended to be a valid explicit priority (lowest), the validation should be adjusted. If 0 is reserved as a sentinel for "use static default", this is correct but should perhaps be documented in the configuration examples.
Additionally, the validation conditions could be simplified. The current logic:
priority == PLUGIN_NO_PRIORITY or (isinstance(priority, int) and priority < 0) or not isinstance(priority, int)Could be more concise (if 0 should remain invalid):
not isinstance(priority, int) or priority <= 0Or if 0 should be allowed:
not isinstance(priority, int) or priority < 0
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snapm/manager/_manager.pysnapm/manager/plugins/_plugin.pysnapm/manager/plugins/lvm2.py
🧰 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: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:31:58.457Z
Learning: In snapm, plugins are designed to fail loading when their required dependencies are missing (e.g., LVM2 plugin fails to load if device-mapper support is absent, Stratis plugin fails if D-Bus interfaces are missing). This is intentional behavior to ensure plugins only operate when all dependencies are available.
📚 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/_plugin.pysnapm/manager/_manager.pysnapm/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:
snapm/manager/plugins/_plugin.pysnapm/manager/_manager.py
📚 Learning: 2025-10-10T19:18:28.912Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 524
File: snapm/command.py:90-92
Timestamp: 2025-10-10T19:18:28.912Z
Learning: In the snapm codebase, logging wrapper functions like `_log_debug_command`, `_log_debug_manager`, and `_log_debug_report` are module-internal helpers that intentionally do not merge caller-supplied `extra` kwargs. These wrappers hard-code the subsystem tag in `extra={"subsystem": SNAPM_SUBSYSTEM_*}`. It is a bug for callers to pass their own `extra` kwarg to these wrappers. This design keeps the code simple and avoids unnecessary defensive checks.
Applied to files:
snapm/manager/plugins/_plugin.py
🪛 Ruff (0.14.10)
snapm/manager/_manager.py
948-948: Avoid specifying long messages outside the exception class
(TRY003)
955-957: Avoid specifying long messages outside the exception class
(TRY003)
991-993: 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). (25)
- 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:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-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: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm, 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, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: python-basic-tests
🔇 Additional comments (13)
snapm/manager/_manager.py (4)
14-14: LGTM: Import additions support the multi-provider collection.The
defaultdictandTYPE_CHECKINGimports are appropriately used for collecting multiple providers per source and providing forward-declared type hints without circular imports.Also applies to: 21-21
70-72: LGTM: Proper use of TYPE_CHECKING for forward references.The TYPE_CHECKING block correctly imports Plugin for type annotations whilst avoiding circular import issues.
913-923: LGTM: Plugin config loading enhanced with file context.The refactored logic correctly builds the plugin config path and stores the filename on the ConfigParser instance for improved warning logging, aligning with the PR's objective to include config file names in plugin configuration warnings.
1258-1258: LGTM: Correctly uses resolved providers throughout snapshot creation.The changes properly iterate over the resolved provider map and use the selected highest-priority provider for all plugin operations (
origin_from_mount_point,check_create_snapshot,create_snapshot).Also applies to: 1269-1269, 1272-1272, 1296-1296, 1303-1303
snapm/manager/plugins/_plugin.py (5)
13-13: LGTM!The logging imports are appropriate for the new plugin debug subsystem.
Also applies to: 15-15
48-55: LGTM!The priority configuration constants are well-documented and follow established naming conventions.
57-68: LGTM!The logging subsystem setup follows the established pattern in the snapm codebase where subsystem-specific wrappers hard-code the subsystem tag in the
extraparameter. Based on learnings, callers should not pass their ownextrakwarg to these wrappers.
84-108: LGTM!Excellent improvement to configuration validation. The try-except blocks properly handle invalid integer values and provide clear warnings including the config file name. This aligns with issue #842's objective to enhance plugin configuration warning logging.
542-542: LGTM!Correctly exposes
PLUGIN_NO_PRIORITYin the public API, consistent with its usage across plugin implementations.snapm/manager/plugins/lvm2.py (4)
42-42: LGTM!The import of
PLUGIN_NO_PRIORITYis necessary for the priority defaulting logic in both LVM2 plugin classes.
180-184: LGTM!The static priority values correctly implement issue #837's specification: lvm2-cow=10 and lvm2-thin=15. This ensures that when both plugins can handle a device, the thin-provider is preferred due to its higher priority.
1067-1071: LGTM!The implementation correctly:
- Initialises the
originstracking dictionary for limit checking- Applies the static default priority when not explicitly configured
This aligns with issues #836 and #837, allowing configuration override whilst providing a sensible default priority.
1381-1385: LGTM!Consistent implementation with
Lvm2Cow, properly initialising thepoolstracking dictionary and applying the static default priority when not explicitly configured.
|
@coderabbitai: re:
The validation is SUPPOSED to reject 0. That's an internal value that means "Plugins should define their own static priority". This is completely correct. The conciseness suggestion I'll take, and add in a future commit. |
|
Tip For best results, initiate chat on the files or code changes. @bmr-cymru: Thank you for the clarification! I understand now— Appreciate your note about the conciseness suggestion—looking forward to seeing that refinement in a future commit! 👍 🧠 Learnings used |
bmr-cymru
force-pushed
the
bmr-btrfs-plugin
branch
from
5e93934 to
f910330
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/test_snapm.py (1)
297-315: Clarify ConcreteSnapshot.autoactivate return value
autoactivatecurrently does a barereturn, so evaluates toNone. That fulfils the ABC for this test, but is slightly misleading given the documented boolean contract and its use elsewhere (e.g.SnapshotSet.autoactivateusesall(s.autoactivate …)). Returning an explicitFalsewould make the intent clearer and avoid surprises if this helper is reused.tests/test_plugin.py (1)
43-81: ConcretePlugin correctly exercises the Plugin ABC; tidy up unused argumentThe local
ConcretePlugingives you a minimal concrete subclass to testPlugin.info()and the defaultPLUGIN_NO_PRIORITY, andtest_instantiate_base_raisesnow correctly asserts that the abstract base cannot be instantiated directly.Ruff’s ARG002 about the unused
sourceparameter incan_snapshotis valid here. If you want to silence it without changing semantics, you could, for example, rename the parameter to_sourceor assign to_ = sourceinside the method.snapm/manager/plugins/_plugin.py (1)
50-57: Plugin limits, priority parsing, and ABC conversion are well‑structured
PluginLimitsnow defensively parsesMaxSnapshotsPerOrigin/MaxSnapshotsPerPooland emits clear warnings including the config filename when values are invalid, which should make misconfigured plugins much easier to diagnose.PLUGIN_NO_PRIORITYas a sentinel, combined with thePriority/PluginPriorityparsing and validation inPlugin.__init__, cleanly enforces the rule that only positive integers are accepted and that a configured value of 0 is treated as invalid and reset to the sentinel for subclasses to handle.- Turning
Plugininto anABCwith all lifecycle methods marked@abstractmethodmatches how the concrete plugins already behave and lets tests assert that the base cannot be instantiated directly._log_debug_pluginmirrors the existing subsystem‑specific debug wrappers and correctly tags messages withSNAPM_SUBSYSTEM_PLUGIN; as with the other wrappers, callers should avoid passing their ownextrakwargs.One minor assumption to keep in mind: both
PluginLimitsand the priority‑parsing path rely onplugin_cfg.plugin_conf_filebeing set for warning messages. Manager does this in_load_plugin_config, and the tests set it explicitly, but any new code constructing aConfigParserfor plugins will need to follow that pattern.Also applies to: 58-69, 76-82, 83-110, 121-151, 194-349, 527-547
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
snapm/_snapm.pysnapm/manager/_manager.pysnapm/manager/plugins/_plugin.pysnapm/manager/plugins/lvm2.pytests/test_plugin.pytests/test_snapm.py
🧰 Additional context used
🧠 Learnings (5)
📓 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.
📚 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.pysnapm/_snapm.pysnapm/manager/_manager.pysnapm/manager/plugins/_plugin.py
📚 Learning: 2025-11-12T21:27:50.414Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 596
File: snapm/_snapm.py:36-44
Timestamp: 2025-11-12T21:27:50.414Z
Learning: In the snapm codebase, the numeric values of SNAPM_DEBUG_* constants (e.g., SNAPM_DEBUG_MANAGER, SNAPM_DEBUG_MOUNTS, SNAPM_DEBUG_SCHEDULE) are not guaranteed to remain stable across versions. Consumers should always use the symbolic constant names rather than persisting numeric masks. SNAPM_DEBUG_ALL is designed to change over time as new subsystems are added, and repurposing or reassigning bit positions for debug flags is acceptable and intentional.
Applied to files:
snapm/_snapm.py
📚 Learning: 2025-10-10T19:18:28.912Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 524
File: snapm/command.py:90-92
Timestamp: 2025-10-10T19:18:28.912Z
Learning: In the snapm codebase, logging wrapper functions like `_log_debug_command`, `_log_debug_manager`, and `_log_debug_report` are module-internal helpers that intentionally do not merge caller-supplied `extra` kwargs. These wrappers hard-code the subsystem tag in `extra={"subsystem": SNAPM_SUBSYSTEM_*}`. It is a bug for callers to pass their own `extra` kwarg to these wrappers. This design keeps the code simple and avoids unnecessary defensive checks.
Applied to files:
snapm/_snapm.pysnapm/manager/plugins/_plugin.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:
snapm/manager/_manager.pysnapm/manager/plugins/_plugin.py
🧬 Code graph analysis (5)
snapm/_snapm.py (3)
tests/test_snapm.py (5)
origin(300-300)devpath(304-304)status(306-306)size(308-308)free(310-310)snapm/manager/plugins/lvm2.py (7)
origin(343-344)devpath(360-363)status(366-375)size(378-380)free(383-384)free(417-420)free(437-439)snapm/manager/plugins/stratis.py (5)
origin(225-226)devpath(239-240)status(243-247)size(250-252)free(255-259)
tests/test_plugin.py (1)
snapm/manager/plugins/_plugin.py (16)
Plugin(112-348)discover_snapshots(195-201)can_snapshot(204-212)check_create_snapshot(215-227)create_snapshot(230-242)rename_snapshot(245-255)check_resize_snapshot(258-268)resize_snapshot(271-281)check_revert_snapshot(284-295)revert_snapshot(298-307)delete_snapshot(310-315)activate_snapshot(318-323)deactivate_snapshot(326-331)set_autoactivate(334-340)origin_from_mount_point(343-348)info(176-180)
snapm/manager/_manager.py (3)
snapm/manager/plugins/_plugin.py (3)
origin_from_mount_point(343-348)check_create_snapshot(215-227)create_snapshot(230-242)snapm/manager/plugins/lvm2.py (7)
origin_from_mount_point(857-868)check_create_snapshot(826-839)check_create_snapshot(1238-1257)check_create_snapshot(1512-1532)create_snapshot(842-855)create_snapshot(1259-1306)create_snapshot(1534-1578)snapm/manager/plugins/stratis.py (3)
origin_from_mount_point(727-735)check_create_snapshot(622-654)create_snapshot(657-725)
snapm/manager/plugins/_plugin.py (3)
snapm/_snapm.py (3)
Snapshot(1531-1892)name(1126-1130)name(1621-1625)tests/test_plugin.py (5)
discover_snapshots(45-45)delete_snapshot(61-62)activate_snapshot(63-64)deactivate_snapshot(65-66)set_autoactivate(67-68)snapm/manager/plugins/lvm2.py (7)
discover_snapshots(806-812)discover_snapshots(1073-1124)discover_snapshots(1387-1433)delete_snapshot(870-882)activate_snapshot(973-980)deactivate_snapshot(982-989)set_autoactivate(991-1009)
tests/test_snapm.py (3)
snapm/_snapm.py (14)
origin(1659-1662)origin_options(1666-1669)devpath(1716-1719)status(1211-1224)status(1723-1727)SnapStatus(925-948)size(902-922)size(1731-1737)free(1741-1747)autoactivate(1227-1233)autoactivate(1236-1248)autoactivate(1751-1755)autoactivate(1758-1765)invalidate_cache(1875-1878)snapm/manager/plugins/lvm2.py (10)
origin(343-344)origin_options(347-354)devpath(360-363)status(366-375)size(378-380)free(383-384)free(417-420)free(437-439)autoactivate(389-394)invalidate_cache(396-398)snapm/manager/plugins/stratis.py (8)
origin(225-226)origin_options(229-236)devpath(239-240)status(243-247)size(250-252)free(255-259)autoactivate(264-266)invalidate_cache(268-271)
🪛 Ruff (0.14.10)
tests/test_plugin.py
46-46: Unused method argument: source
(ARG002)
snapm/manager/_manager.py
948-948: Avoid specifying long messages outside the exception class
(TRY003)
955-957: Avoid specifying long messages outside the exception class
(TRY003)
991-993: 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). (37)
- GitHub Check: rpm-build:fedora-43-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:fedora-rawhide-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: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:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-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:centos-stream-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-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- 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 (bios, lvm, 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-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: python-basic-tests
🔇 Additional comments (4)
snapm/manager/plugins/lvm2.py (1)
42-42: Priority defaults and relaxedcan_snapshot()semantics look consistentThe LVM2 plugins’ use of
PLUGIN_NO_PRIORITYas a sentinel plus static defaults (LVM2_COW_STATIC_PRIORITY=10,LVM2_THIN_STATIC_PRIORITY=15) integrates cleanly with the new basePluginpriority parsing: invalid/zero config values get normalised back to the sentinel, and subclasses then apply their static defaults.The relaxed
Lvm2Cow.can_snapshot()(dropping the lv_attr‑type filter and only rejecting merging origins) is in line with the goal of supporting more LV types while relying on priorities to prefer thin/Stratis/Btrfs where present._is_lvm_device()still enforces the device‑mapper major check, so this doesn’t re‑introduce the earlier dmsetup regression. Based on learnings, this behaviour looks correct.Also applies to: 180-185, 1067-1072, 1126-1152, 1381-1386
snapm/_snapm.py (2)
38-55: Plugin debug flag and subsystem wiring are coherent
SNAPM_DEBUG_PLUGINandSNAPM_SUBSYSTEM_PLUGINare correctly added to the mask constants,_DEBUG_MASK_TO_SUBSYSTEM, and__all__.set_debug_mask/get_debug_maskcontinue to derive mask values from the mapping, so plugin debug output will participate cleanly in the existing subsystem‑filter mechanism.Also applies to: 56-73, 2309-2372
1531-1756: Snapshot ABC conversion is sound but may affect out‑of‑tree pluginsTurning
Snapshotinto anABCand markingorigin,origin_options,devpath,status,size,free,autoactivate, andinvalidate_cacheas abstract matches how the in‑tree plugins already behave and makes the contract explicit. The existing LVM2 and Stratis snapshot classes implement all of these, so internal behaviour remains unchanged.Do verify whether any external plugins subclass
Snapshotwithout implementing these members; those would now fail to instantiate with aTypeErrorand may need to be updated accordingly.Also applies to: 1874-1886
snapm/manager/_manager.py (1)
913-923: Priority-based provider resolution is clear; check any callers relying on_requested_providerThe manager now:
- Loads per‑plugin config from
<name>.confunderplugins.d, annotating theConfigParserwithplugin_conf_filefor downstream logging.- Collects all candidate providers per source and then, via
get_highest_priority, enforces a single highest‑priority provider for each source, raisingSnapmNoProviderErrorif multiple plugins share the maximum priority. That makes misconfigured ties explicit rather than silently picking an arbitrary provider.- Wires the resolved provider map through
create_snapshot_set, starting a transaction per distinct provider and consistently using the chosen provider for origin probing, pre‑check, and creation.One thing to verify:
_find_and_verify_pluginsstill accepts_requested_providerbut no longer uses it.create_snapshot_setpassesNone, so there’s no change there, but if any other internal callers were previously relying on constraining the provider set by name, that hint is now effectively ignored and they will get full priority‑based selection instead.Also applies to: 937-997, 1248-1253, 1258-1274, 1290-1297, 1318-1319
|
@coderabbitai: re:
There isn't going to be any new code constructing a |
|
Tip For best results, initiate chat on the files or code changes. @bmr-cymru: Thank you for the clarification! I understand now—the centralization of ConfigParser construction for plugins in exactly one place (Manager) is an intentional architectural decision that keeps the codebase maintainable and prevents technical debt. Duplicating that pattern elsewhere would indeed work against that goal. I appreciate you explaining the design rationale! 👍 (_/) ✏️ Learnings added
🧠 Learnings used |
bmr-cymru
force-pushed
the
bmr-btrfs-plugin
branch
from
f910330 to
1225c2a
Compare
bmr-cymru
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
doc/user_guide.rstsnapm/manager/plugins/_plugin.pytests/__init__.pytests/test_manager.pytests/test_plugin.py
🧰 Additional context used
🧠 Learnings (11)
📓 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: 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: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:31:58.457Z
Learning: In snapm, plugins are designed to fail loading when their required dependencies are missing (e.g., LVM2 plugin fails to load if device-mapper support is absent, Stratis plugin fails if D-Bus interfaces are missing). This is intentional behavior to ensure plugins only operate when all dependencies are available.
📚 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:
doc/user_guide.rsttests/test_manager.pysnapm/manager/plugins/_plugin.py
📚 Learning: 2025-09-14T17:31:58.457Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:31:58.457Z
Learning: In snapm, plugins are designed to fail loading when their required dependencies are missing (e.g., LVM2 plugin fails to load if device-mapper support is absent, Stratis plugin fails if D-Bus interfaces are missing). This is intentional behavior to ensure plugins only operate when all dependencies are available.
Applied to files:
doc/user_guide.rsttests/test_manager.py
📚 Learning: 2025-08-27T21:01:32.324Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-27T21:01:32.324Z
Learning: In the snapm project, the design philosophy is to fail fast when expected files (like systemd units or tmpfiles.d configuration) don't exist during installation steps. This is intentional behavior - if these files are missing, it indicates a fundamental issue with the build/test setup that should abort the process immediately rather than continue with an incomplete installation.
Applied to files:
doc/user_guide.rst
📚 Learning: 2025-09-24T02:49:21.752Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 490
File: snapm/manager/plugins/lvm2.py:618-629
Timestamp: 2025-09-24T02:49:21.752Z
Learning: When API changes affect return types (like changing from int to tuple), all supporting infrastructure needs updating including mock scripts in tests, not just the call sites that consume the API.
Applied to files:
tests/__init__.pytests/test_plugin.py
📚 Learning: 2025-12-30T14:19:44.887Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-30T14:19:44.887Z
Learning: In snapshotmanager/snapm, ConfigParser construction for plugins is intentionally centralized in exactly one place within Manager._find_and_verify_plugins(). This architectural decision ensures maintainability, and duplicating this pattern elsewhere would create technical debt.
Applied to files:
tests/__init__.pytests/test_manager.pysnapm/manager/plugins/_plugin.py
📚 Learning: 2025-08-26T19:07:42.290Z
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.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-08-30T13:46:15.300Z
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.
Applied to files:
tests/test_manager.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.pysnapm/manager/plugins/_plugin.py
📚 Learning: 2025-12-13T00:14:20.405Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-13T00:14:20.405Z
Learning: In the snapm codebase, snapshot set names are unique and two mounts cannot exist for the same `Selection(name=...)`. Therefore, `manager.mounts.find_mounts(Selection(name=...))` cannot return multiple mounts.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-10-10T19:18:28.912Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 524
File: snapm/command.py:90-92
Timestamp: 2025-10-10T19:18:28.912Z
Learning: In the snapm codebase, logging wrapper functions like `_log_debug_command`, `_log_debug_manager`, and `_log_debug_report` are module-internal helpers that intentionally do not merge caller-supplied `extra` kwargs. These wrappers hard-code the subsystem tag in `extra={"subsystem": SNAPM_SUBSYSTEM_*}`. It is a bug for callers to pass their own `extra` kwarg to these wrappers. This design keeps the code simple and avoids unnecessary defensive checks.
Applied to files:
snapm/manager/plugins/_plugin.py
🧬 Code graph analysis (3)
tests/__init__.py (1)
snapm/manager/plugins/_plugin.py (15)
Plugin(112-348)discover_snapshots(195-201)can_snapshot(204-212)check_create_snapshot(215-227)create_snapshot(230-242)rename_snapshot(245-255)check_resize_snapshot(258-268)resize_snapshot(271-281)check_revert_snapshot(284-295)revert_snapshot(298-307)delete_snapshot(310-315)activate_snapshot(318-323)deactivate_snapshot(326-331)set_autoactivate(334-340)origin_from_mount_point(343-348)
tests/test_plugin.py (2)
tests/__init__.py (1)
MockPlugin(61-88)snapm/manager/plugins/_plugin.py (3)
info(176-180)Plugin(112-348)PluginLimits(71-109)
snapm/manager/plugins/_plugin.py (4)
snapm/_snapm.py (3)
Snapshot(1531-1892)name(1126-1130)name(1621-1625)tests/__init__.py (5)
discover_snapshots(63-63)delete_snapshot(79-80)activate_snapshot(81-82)deactivate_snapshot(83-84)set_autoactivate(85-86)snapm/manager/plugins/lvm2.py (7)
discover_snapshots(806-812)discover_snapshots(1073-1124)discover_snapshots(1387-1433)delete_snapshot(870-882)activate_snapshot(973-980)deactivate_snapshot(982-989)set_autoactivate(991-1009)snapm/manager/_manager.py (1)
set_autoactivate(1621-1639)
🪛 Ruff (0.14.10)
tests/__init__.py
64-64: Unused method argument: source
(ARG002)
tests/test_manager.py
159-159: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
160-160: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
161-161: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
410-410: Local variable cm is assigned to but never used
Remove assignment to unused variable cm
(F841)
993-993: Unused method argument: source
(ARG002)
1004-1004: Unused method argument: source
(ARG002)
1044-1044: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
1045-1045: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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). (30)
- 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:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-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-rawhide-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:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: python-basic-tests
🔇 Additional comments (18)
tests/__init__.py (1)
61-89: LGTM! MockPlugin correctly fulfils the ABC contract.The MockPlugin implementation provides a minimal concrete subclass for testing purposes. All abstract methods are properly implemented with sensible defaults (returning empty lists, False, or raising NotImplementedError).
Minor inconsistency: Line 73 uses
nameinstead of_namefor the unused parameter, unlike other methods. This is a trivial nit and does not affect functionality.tests/test_plugin.py (5)
44-50: LGTM! Test correctly verifies plugin info and default priority.The test properly sets up a MockPlugin with a ConfigParser and verifies both the
info()method output and the default priority value (PLUGIN_NO_PRIORITY).
52-54: LGTM! Correctly verifies Plugin cannot be instantiated directly.This test ensures the ABC enforcement is working as expected—attempting to instantiate the abstract
Pluginclass raisesTypeError.
187-199: LGTM! Correctly tests static priority override in subclass.The test demonstrates that a plugin subclass can set its own priority after calling
super().__init__(), which is the intended pattern for plugins to provide static default priorities.
201-214: LGTM! Correctly tests configuration-driven priority override.The test verifies that the
[Priority]section withPluginPriorityin the ConfigParser correctly sets the plugin's priority, demonstrating the config-driven override mechanism.
216-227: LGTM! Correctly tests PluginLimits error handling.The test verifies that
PluginLimitsgracefully handles invalid integer values by falling back to 0, which matches the implementation's error handling behaviour.doc/user_guide.rst (2)
810-868: LGTM! Comprehensive documentation for plugin priority feature.The documentation clearly explains the priority system, default values, and how administrators can override priorities via configuration files. The examples are practical and show expected command output.
1580-1608: LGTM! Helpful troubleshooting section for plugin selection issues.The "Wrong Plugin Selected for Snapshots" troubleshooting guide provides clear, actionable steps for diagnosing and resolving plugin selection problems.
tests/test_manager.py (4)
159-161: LGTM! Test volume sets appropriately reduced.The reduced volume sets align with PR objective #552 to reduce test-suite runtime whilst maintaining adequate coverage.
403-415: LGTM! Test correctly overrides plugin limits for exhaustion testing.The changes appropriately override
snapshots_per_originto allow more snapshot creation iterations before the test validates theSnapmNoSpaceErroris raised on space exhaustion.
931-974: LGTM! Correctly tests priority-based plugin selection.The test effectively validates that when multiple plugins can handle the same source, the plugin with the highest numerical priority is selected by
_find_and_verify_plugins.
1044-1045: LGTM! Test volume sets appropriately reduced for thin-only tests.The reduced volume sets for
ManagerTestsThinalign with the PR objective to reduce test-suite runtime.snapm/manager/plugins/_plugin.py (6)
49-57: LGTM! Priority configuration constants and sentinel value correctly defined.The constants for priority configuration parsing and the
PLUGIN_NO_PRIORITYsentinel value are appropriately defined and documented.
58-68: LGTM! Logging setup follows established codebase patterns.The module-level logger and
_log_debug_pluginwrapper correctly implement the subsystem-specific debug logging pattern used elsewhere in the codebase.
83-109: LGTM! Robust error handling for PluginLimits configuration parsing.The try/except blocks properly handle invalid integer values, log warnings with the configuration filename for debugging, and fall back to the default value of 0.
112-150: LGTM! Plugin ABC conversion and priority handling correctly implemented.The
Pluginclass is properly converted to an ABC with priority parsing that:
- Reads from the
[Priority]section in plugin config- Validates the value is a positive integer (rejecting 0 and negatives per design)
- Logs warnings with config filename for invalid values
- Falls back to
PLUGIN_NO_PRIORITY(0) for subclasses to set static defaultsThis aligns with the design discussion in PR comments where
PluginPriority=0is intentionally invalid because 0 is the internal sentinel for "use plugin's static default".
194-348: LGTM! All plugin lifecycle methods correctly marked as abstract.All 14 plugin lifecycle methods are properly decorated with
@abstractmethod, ensuring concrete plugin implementations must provide implementations. The empty method bodies with docstrings are appropriate for abstract methods.
527-548: LGTM! Public exports updated to include new symbols.
PLUGIN_NO_PRIORITYandPluginLimitsare correctly added to__all__, making them part of the public API.
bmr-cymru
force-pushed
the
bmr-btrfs-plugin
branch
from
9dc1022 to
e4dcd23
Compare
|
Everything is ready to go, except there is a 7% coverage regression for Investigating... Most likely seems commit 41aa0a9, specifically: diff --git a/tests/test_boot.py b/tests/test_boot.py
index 49185237..cd8cf249 100644
--- a/tests/test_boot.py
+++ b/tests/test_boot.py
@@ -83,7 +83,7 @@ class BootTestsBase(unittest.TestCase):
Base class for boot tests.
"""
volumes = ["root", "home", "var"]
- thin_volumes = ["opt", "srv"]
+ thin_volumes = []
boot_volumes = [
("root", "/"),
("home", "/home"),Every other change in that commit leaves at least one thin volume in play... |
|
Long story short: It's a configuration regression caused by some testing I was doing earlier: The modified priority on the lvm2-cow plugin causes it to take precedence over lvm2-thin, even in the test suite, causing the 7% coverage drop. The unbearable irony is that the EPIC FACEPALM |
bmr-cymru
force-pushed
the
bmr-btrfs-plugin
branch
from
e4dcd23 to
d11a43d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tests/test_boot.py (1)
85-87: Thin volume list reduction looks fine; consider addressing RUF012 laterSetting
thin_volumes = []to slim the test setup aligns with the CI/runtime reduction goals and keepsLvmLoopBackedinitialisation straightforward. If you want to keep Ruff happy, you could later switchvolumes/thin_volumesto tuples or annotate them asClassVar[...], but that’s purely cosmetic here.man/man5/snapm-plugins.d.5 (1)
38-44: Priority subsection matches implementation; only tiny wording nitsThe new
Prioritysubsection correctly documentsPluginPriorityas a positive integer override and the “higher wins” behaviour. If you touch this again, you might want to tweak “for, e.g,” to “for example,” for slightly smoother prose, but functionally the docs look accurate.doc/user_guide.rst (1)
1870-1883: Performance notes correctly characterise thin vs CoW trade‑offsThe new performance notes (“LVM2 thin provisioning generally offers better performance than copy‑on‑write”, separate backstores, monitoring space) are reasonable operational guidance and consistent with how the back‑ends behave. Nothing to change here unless you later want to add explicit caveats for very fragmented thin pools.
snapm/manager/plugins/_plugin.py (1)
112-349: Plugin ABC surface is clear and enforces provider contractsTurning
Plugininto anABCwith explicit@abstractmethodhooks for discovery, capability checks, snapshot lifecycle, resize, revert, activation, and origin resolution is a nice tightening of the provider contract. Existing concrete plugins that already implemented this surface should continue to work, and the new MockPlugin intests/__init__.pycleanly fulfils the ABC for unit tests.One small thing to keep in mind is that tests that pass a bare
Mock()asplugin_cfgwill end up withlimits.snapshots_per_*potentially set toMockinstances. You’re not relying on those in tests today, so it’s harmless, but if future tests start asserting on limits it may be worth switching test plugin_cfgs toConfigParserthere as well.tests/test_scheduler.py (1)
56-59: Reduced volume set keeps coverage while helping test runtimeLimiting
thin_volumesto["opt"]andstratis_volumesto["fs1"]still exercises mixed‑backend scheduling but cuts down the combinatorial surface, which fits the stated CI/runtime goals.If you’re planning to fix RUF012 at some point, you could convert these to tuples or
ClassVar[List[str]], but that’s purely stylistic in this context.tests/test_manager.py (2)
9-10: New testing imports are appropriate; one minor unused importBringing in
Mock,patch, andMockPluginis appropriate for the new priority tests. The directPluginimport fromsnapm.manager.pluginsappears unused in this file; leaving it is harmless, but you could drop it in a later clean‑up to avoid confusion.Also applies to: 19-25
404-416: No‑space test tweak is reasonable but a bit heavyOverriding
p.limits.snapshots_per_origin = 100for all plugins intest_create_snapshot_set_no_space_raisesis a pragmatic way to bypass the logical per‑origin cap and drive the underlying “real no space” path. Bumping the loop to 50 iterations should make hitting the exhaustion condition more deterministic, especially after slimming the volume set.If this test ever becomes a runtime hotspot, consider lowering the iteration cap and/or constraining it to the LVM2‑CoW provider only, but functionally the change is fine.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
doc/user_guide.rstetc/snapm/plugins.d/lvm2-cow.confetc/snapm/plugins.d/lvm2-thin.confetc/snapm/plugins.d/stratis.confman/man5/snapm-plugins.d.5man/man8/snapm.8snapm/_snapm.pysnapm/command.pysnapm/manager/_manager.pysnapm/manager/plugins/_plugin.pysnapm/manager/plugins/lvm2.pysnapm/manager/plugins/stratis.pytests/__init__.pytests/test_boot.pytests/test_command.pytests/test_manager.pytests/test_plugin.pytests/test_scheduler.pytests/test_snapm.py
🚧 Files skipped from review as they are similar to previous changes (7)
- etc/snapm/plugins.d/lvm2-cow.conf
- etc/snapm/plugins.d/stratis.conf
- snapm/manager/plugins/lvm2.py
- man/man8/snapm.8
- snapm/manager/plugins/stratis.py
- tests/test_snapm.py
- etc/snapm/plugins.d/lvm2-thin.conf
🧰 Additional context used
🧠 Learnings (16)
📓 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: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:31:58.457Z
Learning: In snapm, plugins are designed to fail loading when their required dependencies are missing (e.g., LVM2 plugin fails to load if device-mapper support is absent, Stratis plugin fails if D-Bus interfaces are missing). This is intentional behavior to ensure plugins only operate when all dependencies are available.
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-12-30T14:19:44.887Z
Learning: In snapshotmanager/snapm, ConfigParser construction for plugins is intentionally centralized in exactly one place within Manager._find_and_verify_plugins(). This architectural decision ensures maintainability, and duplicating this pattern elsewhere would create technical debt.
📚 Learning: 2025-09-24T02:49:21.752Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 490
File: snapm/manager/plugins/lvm2.py:618-629
Timestamp: 2025-09-24T02:49:21.752Z
Learning: When API changes affect return types (like changing from int to tuple), all supporting infrastructure needs updating including mock scripts in tests, not just the call sites that consume the API.
Applied to files:
tests/test_plugin.pytests/__init__.py
📚 Learning: 2025-12-30T14:19:44.887Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-30T14:19:44.887Z
Learning: In snapshotmanager/snapm, ConfigParser construction for plugins is intentionally centralized in exactly one place within Manager._find_and_verify_plugins(). This architectural decision ensures maintainability, and duplicating this pattern elsewhere would create technical debt.
Applied to files:
tests/__init__.pytests/test_manager.pysnapm/manager/plugins/_plugin.pysnapm/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:
tests/test_manager.pysnapm/_snapm.pysnapm/manager/plugins/_plugin.pydoc/user_guide.rstsnapm/manager/_manager.py
📚 Learning: 2025-08-26T19:07:42.290Z
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.
Applied to files:
tests/test_manager.pytests/test_scheduler.pytests/test_boot.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:
tests/test_manager.pydoc/user_guide.rst
📚 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:
tests/test_manager.pydoc/user_guide.rst
📚 Learning: 2025-08-30T13:46:15.300Z
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.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-09-14T17:31:58.457Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:31:58.457Z
Learning: In snapm, plugins are designed to fail loading when their required dependencies are missing (e.g., LVM2 plugin fails to load if device-mapper support is absent, Stratis plugin fails if D-Bus interfaces are missing). This is intentional behavior to ensure plugins only operate when all dependencies are available.
Applied to files:
tests/test_manager.pydoc/user_guide.rst
📚 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.pysnapm/manager/plugins/_plugin.pysnapm/manager/_manager.py
📚 Learning: 2025-12-13T00:14:20.405Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-13T00:14:20.405Z
Learning: In the snapm codebase, snapshot set names are unique and two mounts cannot exist for the same `Selection(name=...)`. Therefore, `manager.mounts.find_mounts(Selection(name=...))` cannot return multiple mounts.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-11-12T21:27:50.414Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 596
File: snapm/_snapm.py:36-44
Timestamp: 2025-11-12T21:27:50.414Z
Learning: In the snapm codebase, the numeric values of SNAPM_DEBUG_* constants (e.g., SNAPM_DEBUG_MANAGER, SNAPM_DEBUG_MOUNTS, SNAPM_DEBUG_SCHEDULE) are not guaranteed to remain stable across versions. Consumers should always use the symbolic constant names rather than persisting numeric masks. SNAPM_DEBUG_ALL is designed to change over time as new subsystems are added, and repurposing or reassigning bit positions for debug flags is acceptable and intentional.
Applied to files:
snapm/_snapm.pysnapm/command.pytests/test_command.py
📚 Learning: 2025-10-10T19:18:28.912Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 524
File: snapm/command.py:90-92
Timestamp: 2025-10-10T19:18:28.912Z
Learning: In the snapm codebase, logging wrapper functions like `_log_debug_command`, `_log_debug_manager`, and `_log_debug_report` are module-internal helpers that intentionally do not merge caller-supplied `extra` kwargs. These wrappers hard-code the subsystem tag in `extra={"subsystem": SNAPM_SUBSYSTEM_*}`. It is a bug for callers to pass their own `extra` kwarg to these wrappers. This design keeps the code simple and avoids unnecessary defensive checks.
Applied to files:
snapm/_snapm.pysnapm/manager/plugins/_plugin.pytests/test_command.py
📚 Learning: 2025-08-26T17:35:41.366Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-26T17:35:41.366Z
Learning: In the snapm virt_tests SnapmTestVM class, the run_command() method defaults to check=True, so explicit check=True parameters should not be added throughout the code. The check=False parameter is used intentionally for specific scenarios like expected-fail file existence checks.
Applied to files:
tests/test_command.py
📚 Learning: 2025-08-26T17:36:29.365Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-26T17:36:29.365Z
Learning: In the snapm virt_tests SnapmTestVM class, the get_command_output() method already strips whitespace from returned output, so additional .strip() calls on the returned values are unnecessary.
Applied to files:
tests/test_command.py
📚 Learning: 2025-08-27T21:01:32.324Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-27T21:01:32.324Z
Learning: In the snapm project, the design philosophy is to fail fast when expected files (like systemd units or tmpfiles.d configuration) don't exist during installation steps. This is intentional behavior - if these files are missing, it indicates a fundamental issue with the build/test setup that should abort the process immediately rather than continue with an incomplete installation.
Applied to files:
doc/user_guide.rst
🧬 Code graph analysis (4)
tests/test_plugin.py (3)
tests/__init__.py (1)
MockPlugin(61-88)snapm/manager/plugins/_plugin.py (3)
info(176-180)Plugin(112-348)PluginLimits(71-109)tests/test_manager.py (2)
HighPrioPlugin(951-959)LowPrioPlugin(941-949)
tests/__init__.py (1)
snapm/manager/plugins/_plugin.py (15)
Plugin(112-348)discover_snapshots(195-201)can_snapshot(204-212)check_create_snapshot(215-227)create_snapshot(230-242)rename_snapshot(245-255)check_resize_snapshot(258-268)resize_snapshot(271-281)check_revert_snapshot(284-295)revert_snapshot(298-307)delete_snapshot(310-315)activate_snapshot(318-323)deactivate_snapshot(326-331)set_autoactivate(334-340)origin_from_mount_point(343-348)
tests/test_manager.py (4)
snapm/manager/plugins/_plugin.py (4)
Plugin(112-348)can_snapshot(204-212)_log_debug(170-174)_log_error(152-156)tests/__init__.py (2)
MockPlugin(61-88)can_snapshot(64-64)tests/test_plugin.py (2)
LowPrioPlugin(209-211)HighPrioPlugin(193-196)snapm/manager/_manager.py (1)
_find_and_verify_plugins(925-996)
snapm/command.py (1)
snapm/report.py (2)
FieldType(232-286)report_num(397-410)
🪛 Ruff (0.14.10)
tests/__init__.py
64-64: Unused method argument: source
(ARG002)
tests/test_manager.py
159-159: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
160-160: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
161-161: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
410-410: Local variable cm is assigned to but never used
Remove assignment to unused variable cm
(F841)
993-993: Unused method argument: source
(ARG002)
1004-1004: Unused method argument: source
(ARG002)
1044-1044: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
1045-1045: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/test_scheduler.py
57-57: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
58-58: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/test_command.py
269-269: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
270-270: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
271-271: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
snapm/manager/_manager.py
948-948: Avoid specifying long messages outside the exception class
(TRY003)
955-957: Avoid specifying long messages outside the exception class
(TRY003)
991-993: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_boot.py
86-86: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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). (37)
- 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-42-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-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: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:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-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-42-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-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, 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, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, 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 (27)
doc/user_guide.rst (2)
1063-1160: Plugin priority documentation is consistent with the CLI and core logicThe new “Plugin Priority” and related sections line up well with the code:
- Default priorities (lvm2‑cow=10, lvm2‑thin=15, stratis=20) match the static priorities used by the plugins.
- The
plugin listexample (Priority column and header names) matches_DEFAULT_PLUGIN_FIELDS = "name,version,type,priority"and thepriorityFieldTypeinsnapm.command.- The note that priority values must be positive and that
0/negative values fall back to the plugin’s default priority matches thePlugininitialiser plus per‑plugin static defaults.No functional mismatches spotted; the section gives users enough detail to reason about and override provider selection.
1838-1865: Troubleshooting section for “Wrong Plugin Selected” is well aligned with priority handlingThe “Wrong Plugin Selected for Snapshots” flow sensibly guides users through:
- Inspecting current priorities via
snapm plugin list.- Checking plugin config files under
/etc/snapm/plugins.d/.- Using verbose output to see which plugins can handle a source.
- Overriding priority with
PluginPriority.That matches how
_find_and_verify_plugins()probescan_snapshot()and then picks the highest‑priority provider, so the suggested steps should map directly to real debugging behaviour.snapm/command.py (1)
68-69: Priority field and plugin debug flag are wired correctly
- The new plugin
FieldTypeforpriorityusesREP_NUMandreport_num(d.priority), which matches thePlugin.priorityinteger field.- Updating
_DEFAULT_PLUGIN_FIELDSto includeprioritykeeps thesnapm plugin listoutput and the user guide in sync.- Adding
"plugin": SNAPM_DEBUG_PLUGINtomask_maplets-d pluginbehave consistently with other debug categories.All of this is cohesive with the rest of the reporting and debug infrastructure.
Also applies to: 450-489, 2399-2415
snapm/manager/plugins/_plugin.py (3)
71-110: PluginLimits validation behaves safely on bad configurationThe extra
try/except ValueErroraroundgetint()forMaxSnapshotsPerOriginandMaxSnapshotsPerPoolmeans malformed values are now logged (with filename context viacfg.plugin_conf_file) and cleanly ignored, defaulting to0rather than exploding during plugin initialisation. That’s a good balance between strictness and robustness.
111-151: Priority handling matches the documented sentinel semanticsThe new
PLUGIN_NO_PRIORITY = 0sentinel andPlugin.__init__logic:
- Accepts only strictly positive integer
PluginPriorityvalues from the[Priority]section.- Treats
0, negatives, and non‑integers as invalid, logs a warning including the config file name, and resets toPLUGIN_NO_PRIORITY.This is exactly what you described in the PR comments: explicit
PluginPriority=0is rejected and plugins can treat0as “no configured priority, apply static default”. The implementation looks sound and should interoperate cleanly with the static defaults in the concrete plugins. Based on learnings, this also keeps config‑filename logging consistent with Manager’s centralised ConfigParser setup.
527-548: Exporting PLUGIN_NO_PRIORITY and PluginLimits from the module is appropriateAdding
PLUGIN_NO_PRIORITYandPluginLimitsto__all__matches their new role as public API used by concrete plugins and tests. No issues here.tests/test_plugin.py (3)
9-18: Base Plugin and MockPlugin usage in tests correctly reflect the ABC
- Importing and using
MockPluginfromteststo satisfy thePluginABC keeps these tests focused on core behaviour without depending on real backends.test_plugin_infoandtest_instantiate_base_raisesaccurately capture the expected defaultname/versionand ABC instantiation semantics.All good here.
Also applies to: 44-55
179-215: Priority tests give good coverage of default, static, and config‑driven casesThe trio of tests:
test_default_priority(no config →0),test_static_priority(subclass overridesself.priority), andtest_config_priority_override(INI‑drivenPluginPriority),exercise the key branches in the new priority initialisation logic. They mirror how real plugins will behave (static defaults overridden by config). This gives solid confidence in the sentinel and override behaviour.
216-227: PluginLimits error‑path test matches the new warning/ignore behaviour
test_limits_error_checkingverifies that an invalidMaxSnapshotsPerOriginstring leavessnapshots_per_originat0, which is exactly what the newPluginLimitserror handling implements. That’s a useful regression test for misconfigured plugin limit values.tests/test_manager.py (3)
153-162: Adjusted volume/thin/Stratis sets sensibly reduce test surfaceIn both
ManagerTestsandManagerTestsThin, narrowing:
volumesto["root", "var"],thin_volumesto["opt"](and to["root", "var"]in the thin‑only suite),stratis_volumesto["fs1"],keeps you covering all three provider types while trimming the matrix, which should help with CI duration without losing meaningful behaviour coverage.
Also applies to: 1043-1046
931-975: Priority‑based provider selection test is well‑targeted
test_find_plugins_respects_priority:
- Correctly patches
existsandismountinsnapm.manager._manager, matching how_find_and_verify_plugins()imports them.- Injects two
MockPluginsubclasses with priorities 10 and 20 that both support the same source.- Asserts that the higher‑priority plugin is selected.
This directly exercises the new “highest numerical priority wins” logic and will catch regressions in the selection helper.
976-1034: LVM2 CoW override test accurately models dynamic priority changes
test_lvm2_cow_priority_overrideneatly simulates:
- Default behaviour where an
lvm2-thin‑like plugin (priority 15) wins overlvm2-cow(priority 10) for the same source.- A user raising
lvm2-cow’s priority (here to 100) so that it becomes the preferred provider.The use of
@patchonexists/ismount, plus direct manipulation ofcow_plugin.priority, mirrors what happens when an admin edits/etc/snapm/plugins.d/lvm2-cow.conf. This gives very clear coverage of the override semantics you’re documenting elsewhere.tests/test_command.py (2)
209-211: LGTM: Debug list correctly updated to include the new plugin subsystem.The addition of "plugin" to the debug string ensures the test validates that
SNAPM_DEBUG_ALLnow encompasses all seven subsystems: manager, command, report, schedule, mounts, fsdiff, and plugin.
269-271: Volume reduction aligns with PR objective to slim test setup.Reducing volumes from multiple items to single items addresses issue #552 to lower CI and local-run time. The static analysis hints about
typing.ClassVarare optional for test code and can be safely deferred.snapm/manager/_manager.py (5)
69-72: LGTM: TYPE_CHECKING guard for forward reference.Using
TYPE_CHECKINGwith a forward-declaredPlugintype is the standard pattern to avoid circular imports whilst still providing type hints.
913-922: LGTM: Plugin config loading now includes filename for debugging.The refactored
_load_plugin_configcorrectly computes the per-plugin config path, storesplugin_conf_fileon theConfigParserfor warning logging (addressing #842), and only reads the file if it exists.
937-959: LGTM: Priority-based provider selection with proper tie-breaking.The
get_highest_priorityfunction correctly:
- Raises
ValueErroron an empty providers list.- Selects the provider with the highest numerical priority.
- Raises
SnapmNoProviderErrorwhen multiple providers share the maximum priority, preventing ambiguous selection.This implements the priority mechanism described in #839.
985-996: LGTM: Per-source provider resolution with explicit no-provider error.The refactored flow correctly verifies each source has at least one provider before selecting the highest-priority one, ensuring clear error messages when no provider is available.
1258-1269: LGTM: Iteration now uses resolved (source, provider) pairs.The loop correctly iterates over
provider_map.items()where each value is now the single resolved provider per source, ensuring all downstream calls use the priority-selected provider.tests/__init__.py (2)
13-14: LGTM: Import added for plugin ABC access.The import of
snapm.manager.pluginsis required to access thePluginabstract base class for theMockPluginimplementation.
61-89: LGTM: MockPlugin satisfies the Plugin ABC contract.The
MockPluginclass correctly implements all abstract methods from thePluginABC:
discover_snapshots()returns an empty list.can_snapshot()returnsFalse(the unusedsourceparameter is required by the ABC signature).- All other methods raise
NotImplementedError, which is appropriate for a minimal test mock.This enables priority and configuration tests to instantiate a concrete plugin without requiring real storage backends.
snapm/_snapm.py (6)
45-54: LGTM: New plugin debug subsystem added.The
SNAPM_DEBUG_PLUGINconstant (64) follows the established power-of-two pattern, andSNAPM_DEBUG_ALLcorrectly includes the new flag. This aligns with the PR objective to add a plugin debug subsystem (#841).
63-73: LGTM: Plugin subsystem name and mapping added.The
SNAPM_SUBSYSTEM_PLUGINconstant and its mapping in_DEBUG_MASK_TO_SUBSYSTEMcorrectly integrate the new plugin debug subsystem with the existing logging infrastructure.
1531-1535: LGTM: Snapshot converted to abstract base class.Making
Snapshotinherit fromABCenforces that plugin implementations must provide concrete subclasses, addressing PR objective #834.
1657-1666: LGTM: Abstract property decorators correctly ordered.The
@propertydecorator is correctly placed after@abstractmethod, which is the required ordering in Python for abstract properties.
1874-1878: LGTM: invalidate_cache marked as abstract.Marking
invalidate_cacheas abstract ensures concrete snapshot implementations must provide cache invalidation logic appropriate to their storage backend.
2355-2356: LGTM: New debug constants exported.Both
SNAPM_DEBUG_PLUGINandSNAPM_SUBSYSTEM_PLUGINare correctly added to__all__for public API exposure.
|
@coderabbitai: re:
These aren't new. They were committed in September: Where are you getting that idea from? |
We should have added this a long time ago... Resolves: #841 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
While we're there also add proper error checking and warnings to PluginLimits. Resolves: #842 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Now that plugins expose a priority value have the Manager class use it when selecting plugins via _fimd_and_verify_plugins(): just choose the candidate plugin with the greatest numerical priority value. Resolves: #839 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
The Lvm2Cow plugin is overly conservative about what it can handle
incan_snapshot(): the underlying dm-snapshot target and the LVM2 tools
allow snapshots of linear, thin, mirror, raid volumes etc. This was done
originally to prevent lvm2-cow greedily attempting to snapshot thin
devices (where thin snapshots are generally preferred).
With plugin priorities in place this can now be relaxed, allowing the
plugin to service a wider range of device types:
root@f42-snapm-vm1:~/src/git/snapm# snapm snapshot list
SnapsetName Name Origin Source Status Size Free Autoactivate Provider
prio-test fedora/root-snapset_prio-test_1767033618_- /dev/fedora/root / Active 512.0MiB 511.8MiB yes lvm2-cow
prio-test fedora/var-snapset_prio-test_1767033618_-var /dev/fedora/var /var Active 512.0MiB 511.6MiB yes lvm2-cow
prio-test fedora/home-snapset_prio-test_1767033618_-home /dev/fedora/home /home Active 1.0GiB 1.9GiB yes lvm2-thin # <<< thin snap for thin vol by default, as expected.
root@f42-snapm-vm1:~/src/git/snapm# snapm snapset delete prio-test
root@f42-snapm-vm1:~/src/git/snapm# vim /etc/snapm/plugins.d/lvm2-cow.conf # Twiddle lvm2-cow.conf PluginPriority to be > lvm2-thin static priority
root@f42-snapm-vm1:~/src/git/snapm# snapm snapset create -br --size-policy=2%SIZE prio-test /home / /var
SnapsetName: prio-test
Sources: /home, /, /var
NrSnapshots: 3
Time: 2025-12-29 18:40:58
UUID: b86f280f-6874-512d-8b5a-2798380c3dfb
Status: Active
Autoactivate: yes
OriginMounted: yes
Mounted: no
Bootable: yes
BootEntries:
SnapshotEntry: 38da7dc
RevertEntry: 458d870
root@f42-snapm-vm1:~/src/git/snapm# snapm snapshot list
SnapsetName Name Origin Source Status Size Free Autoactivate Provider
prio-test fedora/home-snapset_prio-test_1767033658_-home /dev/fedora/home /home Active 512.0MiB 512.0MiB yes lvm2-cow
prio-test fedora/root-snapset_prio-test_1767033658_- /dev/fedora/root / Active 512.0MiB 511.8MiB yes lvm2-cow
prio-test fedora/var-snapset_prio-test_1767033658_-var /dev/fedora/var /var Active 512.0MiB 511.5MiB yes lvm2-cow # <<< thick snapshot for thin volume, because that's what the user asked for.
Resolves: #838
Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
bmr-cymru
force-pushed
the
bmr-btrfs-plugin
branch
from
d11a43d to
ad76353
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
snapm/_snapm.py (1)
1531-1879: Snapshot ABC contract is clear; ensure all concrete snapshots implement itTurning
Snapshotinto anABCwith abstractorigin,origin_options,devpath,status,size,free,autoactivateandinvalidate_cache()tightens the contract and keeps the base logic (e.g.source,origin_mounted,snapshot_mounted, autoactivate setter, resize/revert helpers) provider‑agnostic. Just ensure every in‑tree snapshot implementation (and any out‑of‑tree plugins) now implements these members as callables/properties with the expected semantics, particularlyinvalidate_cache()as a method that can be invoked from the base class.snapm/manager/plugins/_plugin.py (1)
112-151: Plugin ABC and priority parsing match the intended semanticsMaking
PluginanABCand marking all lifecycle methods as@abstractmethodclearly enforces the provider contract, and storingself.priorityfrom the config only when it’s a positive integer (otherwise resetting toPLUGIN_NO_PRIORITYand warning) lines up with the “0 = internal sentinel, not a valid explicit value” design. The tests usingMockPlugin/subclasses cover the default, static override, and config‑override paths; you might optionally add a test for explicitPluginPriority = 0and negative values to lock that validation behaviour in.Also applies to: 194-345, 527-548
tests/test_plugin.py (1)
9-10: Plugin tests now exercise ABC, priority semantics, and limits parsing wellSwitching to
MockPluginfortest_plugin_info, addingtest_instantiate_base_raises, and the new tests for default/static/config priority and invalidPluginLimitsvalues give good coverage of the new Plugin contract and config parsing. You might optionally add a small test forPluginPriority = 0and a malformedMaxSnapshotsPerPoolto pin down those edge cases too, but the critical paths are well covered.Also applies to: 17-18, 45-55, 179-227
tests/test_manager.py (2)
403-410: Minor clean‑ups possible in no‑space testThe logic of forcing a
SnapmNoSpaceErrorby relaxing per‑origin limits and iterating more snapshot creates is sound, but there are a couple of small tidy‑ups you might consider:
cmfromwith self.assertRaises(... ) as cm:is never used; you can drop theas cmbinding.self._lvm.dump_lvs()inside the inner loop will generate a lot of log output when the test passes; if you only need it for debugging failures, moving it behind a flag or calling it once on failure would keep normal runs quieter.These are non‑functional nits and can be deferred.
Example diff to drop unused context variable
- with self.assertRaises(snapm.SnapmNoSpaceError) as cm: + with self.assertRaises(snapm.SnapmNoSpaceError): for i in range(0, 50): self.manager.create_snapshot_set( f"testset{i}", self.mount_points() ) self._lvm.dump_lvs()
930-1034: Priority selection tests are solid; consider adding tie‑priority coverageThe new tests:
test_find_plugins_respects_priority– verifies that, for a single source handled by two providers, the highest‑priority plugin is selected.test_lvm2_cow_priority_override– exercises a realistic thin‑vs‑CoW scenario and shows that raisinglvm2-cow’spriorityat runtime causes it to win overlvm2-thin.These both track the manager’s
_find_and_verify_pluginsbehaviour well.Two optional improvements you might consider:
- Add a small test that sets up two providers with the same highest
priorityfor a source and asserts thatSnapmNoProviderErroris raised, so the failure mode of the tie‑break logic is explicitly covered.- When instantiating the test plugins, passing an actual
ConfigParser()(or a thin wrapper) rather than a bareMock()forplugin_cfgwould decouple the tests from the internals ofPlugin.__init__if its interaction withConfigParserbecomes stricter in future.Both are nice‑to‑haves rather than blockers.
Sketch of an extra tie‑priority test
@patch("snapm.manager._manager.exists", return_value=True) @patch("snapm.manager._manager.ismount", return_value=True) def test_find_plugins_priority_tie_raises(self, _ismount_mock, _exists_mock): source_path = "/dev/test/vol" class P1(MockPlugin): name = "p1" def __init__(self, logger, plugin_cfg): super().__init__(logger, plugin_cfg) self.priority = 10 def can_snapshot(self, _source): return True class P2(MockPlugin): name = "p2" def __init__(self, logger, plugin_cfg): super().__init__(logger, plugin_cfg) self.priority = 10 def can_snapshot(self, _source): return True self.manager.plugins = [P1(Mock(), ConfigParser()), P2(Mock(), ConfigParser())] sources = [source_path] size_policies = {source_path: None} with self.assertRaises(snapm.SnapmNoProviderError): self.manager._find_and_verify_plugins(sources, size_policies)tests/__init__.py (1)
13-24: CentralisedMockPluginhelper cleanly fulfils the Plugin ABCPulling in
snapm.manager.pluginsand definingMockPluginas a minimal concrete subclass gives the tests a single, consistent implementation of the Plugin ABC. The methods that should never be called in these tests correctly raiseNotImplementedError, whilediscover_snapshots/can_snapshotprovide harmless defaults that specific tests can override.If you want to silence Ruff’s unused‑argument warning for the base
can_snapshot, you could rename its parameter to_source, but that’s purely cosmetic.Also applies to: 61-88
snapm/manager/_manager.py (1)
925-997: Remove the unused_requested_providerparameter from_find_and_verify_pluginsThe
_requested_providerparameter is defined in the method signature but is never referenced in the method body. All callers (test and production) either omit it entirely or explicitly passNone. The method's docstring does not document this parameter, creating a signature–docstring mismatch. Remove the parameter and update the docstring accordingly to reflect the current behaviour: the method collects all candidate plugins per source and selects the highest-priority provider, with ties raisingSnapmNoProviderError.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
doc/user_guide.rstetc/snapm/plugins.d/lvm2-cow.confetc/snapm/plugins.d/lvm2-thin.confetc/snapm/plugins.d/stratis.confman/man5/snapm-plugins.d.5man/man8/snapm.8snapm/_snapm.pysnapm/command.pysnapm/manager/_manager.pysnapm/manager/plugins/_plugin.pysnapm/manager/plugins/lvm2.pysnapm/manager/plugins/stratis.pytests/__init__.pytests/test_boot.pytests/test_command.pytests/test_manager.pytests/test_plugin.pytests/test_scheduler.pytests/test_snapm.py
✅ Files skipped from review due to trivial changes (1)
- etc/snapm/plugins.d/stratis.conf
🚧 Files skipped from review as they are similar to previous changes (6)
- man/man8/snapm.8
- etc/snapm/plugins.d/lvm2-cow.conf
- etc/snapm/plugins.d/lvm2-thin.conf
- snapm/manager/plugins/lvm2.py
- snapm/manager/plugins/stratis.py
- man/man5/snapm-plugins.d.5
🧰 Additional context used
🧠 Learnings (16)
📓 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: 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: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:31:58.457Z
Learning: In snapm, plugins are designed to fail loading when their required dependencies are missing (e.g., LVM2 plugin fails to load if device-mapper support is absent, Stratis plugin fails if D-Bus interfaces are missing). This is intentional behavior to ensure plugins only operate when all dependencies are available.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-30T14:19:44.887Z
Learning: In snapshotmanager/snapm, ConfigParser construction for plugins is intentionally centralized in exactly one place within Manager._find_and_verify_plugins(). This architectural decision ensures maintainability, and duplicating this pattern elsewhere would create technical debt.
📚 Learning: 2025-11-12T21:27:50.414Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 596
File: snapm/_snapm.py:36-44
Timestamp: 2025-11-12T21:27:50.414Z
Learning: In the snapm codebase, the numeric values of SNAPM_DEBUG_* constants (e.g., SNAPM_DEBUG_MANAGER, SNAPM_DEBUG_MOUNTS, SNAPM_DEBUG_SCHEDULE) are not guaranteed to remain stable across versions. Consumers should always use the symbolic constant names rather than persisting numeric masks. SNAPM_DEBUG_ALL is designed to change over time as new subsystems are added, and repurposing or reassigning bit positions for debug flags is acceptable and intentional.
Applied to files:
snapm/command.pysnapm/_snapm.pytests/test_command.py
📚 Learning: 2025-09-24T02:49:21.752Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 490
File: snapm/manager/plugins/lvm2.py:618-629
Timestamp: 2025-09-24T02:49:21.752Z
Learning: When API changes affect return types (like changing from int to tuple), all supporting infrastructure needs updating including mock scripts in tests, not just the call sites that consume the API.
Applied to files:
tests/test_plugin.pytests/__init__.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:
doc/user_guide.rstsnapm/manager/plugins/_plugin.pytests/test_manager.pysnapm/manager/_manager.py
📚 Learning: 2025-09-14T17:31:58.457Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:31:58.457Z
Learning: In snapm, plugins are designed to fail loading when their required dependencies are missing (e.g., LVM2 plugin fails to load if device-mapper support is absent, Stratis plugin fails if D-Bus interfaces are missing). This is intentional behavior to ensure plugins only operate when all dependencies are available.
Applied to files:
doc/user_guide.rsttests/test_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:
doc/user_guide.rsttests/test_manager.pytests/test_command.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:
doc/user_guide.rsttests/test_manager.py
📚 Learning: 2025-08-27T21:01:32.324Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-27T21:01:32.324Z
Learning: In the snapm project, the design philosophy is to fail fast when expected files (like systemd units or tmpfiles.d configuration) don't exist during installation steps. This is intentional behavior - if these files are missing, it indicates a fundamental issue with the build/test setup that should abort the process immediately rather than continue with an incomplete installation.
Applied to files:
doc/user_guide.rst
📚 Learning: 2025-12-30T14:19:44.887Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-30T14:19:44.887Z
Learning: In snapshotmanager/snapm, ConfigParser construction for plugins is intentionally centralized in exactly one place within Manager._find_and_verify_plugins(). This architectural decision ensures maintainability, and duplicating this pattern elsewhere would create technical debt.
Applied to files:
tests/__init__.pysnapm/manager/plugins/_plugin.pysnapm/manager/_manager.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:
snapm/manager/plugins/_plugin.pytests/test_manager.pysnapm/manager/_manager.py
📚 Learning: 2025-10-10T19:18:28.912Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 524
File: snapm/command.py:90-92
Timestamp: 2025-10-10T19:18:28.912Z
Learning: In the snapm codebase, logging wrapper functions like `_log_debug_command`, `_log_debug_manager`, and `_log_debug_report` are module-internal helpers that intentionally do not merge caller-supplied `extra` kwargs. These wrappers hard-code the subsystem tag in `extra={"subsystem": SNAPM_SUBSYSTEM_*}`. It is a bug for callers to pass their own `extra` kwarg to these wrappers. This design keeps the code simple and avoids unnecessary defensive checks.
Applied to files:
snapm/manager/plugins/_plugin.pysnapm/_snapm.pytests/test_command.py
📚 Learning: 2025-08-30T13:46:15.300Z
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.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-08-26T19:07:42.290Z
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.
Applied to files:
tests/test_manager.pytests/test_boot.pytests/test_command.pytests/test_scheduler.py
📚 Learning: 2025-12-13T00:14:20.405Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-13T00:14:20.405Z
Learning: In the snapm codebase, snapshot set names are unique and two mounts cannot exist for the same `Selection(name=...)`. Therefore, `manager.mounts.find_mounts(Selection(name=...))` cannot return multiple mounts.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-08-26T17:35:41.366Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-26T17:35:41.366Z
Learning: In the snapm virt_tests SnapmTestVM class, the run_command() method defaults to check=True, so explicit check=True parameters should not be added throughout the code. The check=False parameter is used intentionally for specific scenarios like expected-fail file existence checks.
Applied to files:
tests/test_command.py
📚 Learning: 2025-08-26T17:36:29.365Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-26T17:36:29.365Z
Learning: In the snapm virt_tests SnapmTestVM class, the get_command_output() method already strips whitespace from returned output, so additional .strip() calls on the returned values are unnecessary.
Applied to files:
tests/test_command.py
🧬 Code graph analysis (5)
snapm/command.py (1)
snapm/report.py (2)
FieldType(232-286)report_num(397-410)
tests/test_snapm.py (3)
snapm/_snapm.py (14)
origin(1659-1662)origin_options(1666-1669)devpath(1716-1719)status(1211-1224)status(1723-1727)SnapStatus(925-948)size(902-922)size(1731-1737)free(1741-1747)autoactivate(1227-1233)autoactivate(1236-1248)autoactivate(1751-1755)autoactivate(1758-1765)invalidate_cache(1875-1878)snapm/manager/plugins/lvm2.py (10)
origin(343-344)origin_options(347-354)devpath(360-363)status(366-375)size(378-380)free(383-384)free(417-420)free(437-439)autoactivate(389-394)invalidate_cache(396-398)snapm/manager/plugins/stratis.py (8)
origin(225-226)origin_options(229-236)devpath(239-240)status(243-247)size(250-252)free(255-259)autoactivate(264-266)invalidate_cache(268-271)
tests/__init__.py (1)
snapm/manager/plugins/_plugin.py (15)
Plugin(112-348)discover_snapshots(195-201)can_snapshot(204-212)check_create_snapshot(215-227)create_snapshot(230-242)rename_snapshot(245-255)check_resize_snapshot(258-268)resize_snapshot(271-281)check_revert_snapshot(284-295)revert_snapshot(298-307)delete_snapshot(310-315)activate_snapshot(318-323)deactivate_snapshot(326-331)set_autoactivate(334-340)origin_from_mount_point(343-348)
tests/test_manager.py (4)
tests/__init__.py (2)
MockPlugin(61-88)can_snapshot(64-64)tests/test_plugin.py (2)
LowPrioPlugin(209-211)HighPrioPlugin(193-196)snapm/manager/plugins/lvm2.py (3)
can_snapshot(814-823)can_snapshot(1126-1152)can_snapshot(1435-1462)snapm/manager/_manager.py (1)
_find_and_verify_plugins(925-996)
snapm/manager/_manager.py (3)
snapm/manager/plugins/_plugin.py (5)
Plugin(112-348)_log_debug(170-174)origin_from_mount_point(343-348)check_create_snapshot(215-227)create_snapshot(230-242)snapm/manager/plugins/lvm2.py (7)
origin_from_mount_point(857-868)check_create_snapshot(826-839)check_create_snapshot(1238-1257)check_create_snapshot(1512-1532)create_snapshot(842-855)create_snapshot(1259-1306)create_snapshot(1534-1578)snapm/manager/plugins/stratis.py (3)
origin_from_mount_point(727-735)check_create_snapshot(622-654)create_snapshot(657-725)
🪛 Ruff (0.14.10)
tests/__init__.py
64-64: Unused method argument: source
(ARG002)
tests/test_manager.py
158-158: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
159-159: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
160-160: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
409-409: Local variable cm is assigned to but never used
Remove assignment to unused variable cm
(F841)
992-992: Unused method argument: source
(ARG002)
1003-1003: Unused method argument: source
(ARG002)
1043-1043: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
1044-1044: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
snapm/manager/_manager.py
948-948: Avoid specifying long messages outside the exception class
(TRY003)
955-957: Avoid specifying long messages outside the exception class
(TRY003)
991-993: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_boot.py
86-86: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/test_command.py
269-269: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
270-270: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
271-271: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/test_scheduler.py
57-57: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
58-58: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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). (37)
- 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:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-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: rpm-build:fedora-42-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:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-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:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: python-basic-tests
🔇 Additional comments (11)
snapm/_snapm.py (1)
38-73: Plugin debug subsystem wiring looks consistentAdding
SNAPM_DEBUG_PLUGINto the mask,SNAPM_SUBSYSTEM_PLUGINto the subsystem names, updating_DEBUG_MASK_TO_SUBSYSTEM, and exporting both in__all__is coherent with the existing debug design;get_debug_mask()/set_debug_mask()will naturally include the new plugin subsystem without further changes.Also applies to: 2349-2372
snapm/manager/plugins/_plugin.py (1)
71-110: PluginLimits error handling for invalid integers is soundCatching
ValueErrorfromgetint()and falling back toget()purely for logging, while leaving the limit at its default of0, gives robust behaviour for malformed configs. Logging the bad value together withcfg.plugin_conf_fileshould make diagnosing misconfigurations straightforward, providedplugin_conf_fileis always set by the manager as per the architecture notes.tests/test_scheduler.py (1)
56-59: Slimmed volume sets align with faster device‑backed testsReducing
thin_volumesto["opt"]andstratis_volumesto["fs1"]should cut setup time while still providing enough distinct mount points for the scheduler tests that index intomount_points()[0]and[1]. No behavioural issues are apparent.tests/test_boot.py (1)
80-88: Removing thin volumes from boot tests is safe and reduces overheadSetting
BootTestsBase.thin_volumes = []stops provisioning unused thin‑provisioned volumes while the tests only exerciseboot_volumesmapped from"root","home"and"var". This looks like a straightforward runtime win with no change in test coverage.snapm/command.py (1)
68-69: CLI wiring for plugin priority and plugin debug flag looks correctImporting
SNAPM_DEBUG_PLUGIN, exposing a numericpriorityFieldTypein_plugin_fields, extending_DEFAULT_PLUGIN_FIELDSto"name,version,type,priority", and teachingset_debug()about the"plugin"token integrate the new plugin‑priority and debug subsystem cleanly into the CLI and report layer.Also applies to: 450-490, 2406-2415
doc/user_guide.rst (2)
1063-1160: Plugin priority behaviour and field docs look consistent with implementationThe description of default priorities, higher‑wins semantics, and validation of non‑positive values matches the new manager/plugin logic and the intended “0 = sentinel, fall back to static default” behaviour. The
plugin listexamples andpriorityfield description are clear and should be very helpful when tuning installations.
1838-1865: Troubleshooting flow for “wrong plugin selected” is clear and actionableThe new troubleshooting steps (inspect priorities, configs, verbose
snapset create, then override via/etc/snapm/plugins.d) line up with the priority system and Manager’s provider selection, and give users a practical way to debug and correct mis-selection.tests/test_manager.py (1)
158-160: Slimmed device matrices still cover mixed-provider pathsReducing
volumes,thin_volumes, andstratis_volumesinManagerTestsandManagerTestsThinkeeps coverage of CoW, thin, and Stratis paths while materially cutting setup cost, which should help with test runtime without weakening behaviour coverage.Also applies to: 1042-1045
tests/test_command.py (1)
205-212: Debug flag expectations and fixture slimming align with the new plugin subsystemIncluding
"plugin"intest_set_debug_list’s comma‑separated mask while still assertingSNAPM_DEBUG_ALLkeeps this test aligned with the new plugin debug category and the documented “use symbolic names, not raw masks” guidance. The reducedvolumes/thin_volumes/stratis_volumesset inCommandTestsmirrors the manager tests and should help shorten CLI‑layer test runs without reducing meaningful coverage.Also applies to: 269-271
snapm/manager/_manager.py (2)
904-923: Plugin config loading and logging look robustUsing
<plugin_name>.confunder_PLUGINS_D_PATH, attachingplugin_conf_fileto theConfigParserfor downstream logging, and guarding theread()call with anexists()check makes plugin configuration loading explicit and keeps error messages nicely contextualised per plugin. No issues spotted here.
357-410: Shared runtime-dir helper_check_snapm_dirhardens lock/mount directory handlingThe new
_check_snapm_dircentralises creation/verification of runtime directories, rejects symlinks, and normalises permissions if they drift from the expected mode. Reusing it from_check_lock_dirand_check_mounts_diris a solid refactor and aligns with the project’s “fail fast on setup problems” philosophy.
Resolves: #834 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Cut the number of volumes used in device-backed tests to improve the test suite runtime: on f42-snapm-vm1 this reduces the runtime from ~33m to 27m. Resolves: #552 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Resolves: #845 Assisted-by: Google Gemini https://gemini.google.com Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Resolves: #846 Assisted-by: Anthropic Claude https://claude.ai Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
It's vestigial from the block device work: let's clean it up before Btrfs goes in. Resolves: #850 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
bmr-cymru
force-pushed
the
bmr-btrfs-plugin
branch
from
ad76353 to
a81ef3a
Compare
bmr-cymru
force-pushed
the
bmr-btrfs-plugin
branch
from
a81ef3a to
724ff7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
snapm/manager/_manager.py (1)
1248-1319: Fix_find_and_verify_pluginscall-site to match new signature
_find_and_verify_pluginsnow only accepts(self, sources, size_policies), butcreate_snapshot_set()still passes a third positional argument (None), which will raise aTypeErrorat runtime when creating a snapshot set.Proposed fix: drop the vestigial argument
- # Initialise provider mapping. - provider_map = self._find_and_verify_plugins(sources, size_policies, None) + # Initialise provider mapping. + provider_map = self._find_and_verify_plugins(sources, size_policies)Once this is adjusted, the subsequent use of
provider_map(iteratingitems()and using the resolved provider per source) is consistent with the new priority-resolution behaviour.
🧹 Nitpick comments (7)
scripts/setpaths.sh (1)
35-37: Add directory existence check for consistency.The boom-boot PATH modification (lines 29-33) checks for directory existence before updating, but the PYTHONPATH modification does not. Whilst adding a non-existent directory to PYTHONPATH is harmless, wrapping this block in the same existence check would maintain consistency.
🔎 Suggested refactor
-if ! [[ "$PYTHONPATH" =~ "$PWD/../boom-boot:" ]]; then - export PYTHONPATH="$PWD/../boom-boot:$PYTHONPATH" -fi +if ! [[ "$PYTHONPATH" == *"$PWD/../boom-boot:"* ]]; then + if [ -d "$PWD/../boom-boot" ]; then + export PYTHONPATH="$PWD/../boom-boot:$PYTHONPATH" + fi +fiNote: This also includes the glob pattern fix from the previous comment.
tests/test_boot.py (1)
85-91: Slimming boot test thin volumes looks fine; ClassVar annotation is optionalSetting
thin_volumes = []here to avoid creating thin-provisioned volumes in the boot tests is reasonable given thin coverage elsewhere. If you run Ruff with RUF012 enabled across tests, you may want to annotatevolumes,thin_volumes, andboot_volumesastyping.ClassVarto silence the “mutable class attribute” warning, but it’s not functionally significant.tests/test_scheduler.py (1)
56-59: Reduced fixture volumes keep coverage while speeding scheduler testsLimiting
thin_volumesto["opt"]andstratis_volumesto["fs1"]still exercises both thin and Stratis scheduling paths while cutting device setup cost, which matches the CI runtime objectives. If Ruff’s RUF012 is enabled for tests, you could annotate these lists asClassVarto avoid “mutable class attribute” warnings, but it’s not functionally required.tests/test_manager.py (3)
152-161: Slimmed Manager test fixtures maintain coverage while reducing setup costReducing
volumes,thin_volumes, andstratis_volumesinManagerTestsandManagerTestsThinkeeps LVM2 COW, LVM2 thin, and Stratis paths exercised while cutting the number of loop-backed devices, which should help test runtime. If you run Ruff with RUF012 enabled on tests, you may want to annotate these lists asClassVarto silence “mutable class attribute” warnings, but that’s optional.Also applies to: 1068-1071
403-413: Consider making the no-space test less iteration-heavy or confirm it remains stableOverriding
snapshots_per_originto 100 avoids PluginLimits interfering, and looping up to 50 snapshot-set creations should reliably triggerSnapmNoSpaceError. That said, this many real snapshot creations can be relatively expensive and might behave differently on environments with unusually generous backing storage; if CI ever becomes slow or flaky here, consider driving the error more deterministically (e.g. via smaller test VG size or a dedicated low-limit config) instead of a high iteration count.
929-1059: Priority-selection tests are solid; clean up minor static-analysis nitsThe new tests for
_find_and_verify_pluginscorrectly exercise:
- higher-priority provider winning when multiple plugins can snapshot the same source,
- dynamic override of a lower-priority plugin to outrank a higher-priority one, and
- tie detection raising
SnapmNoProviderError.To keep Ruff happy without changing behaviour, you could:
- rename the unused
sourceargument inLvm2Thin.can_snapshot/Lvm2Cow.can_snapshotand the tie-test classes to_source(or adddel sourcein the body), and- drop the unused
mock_loggerassignment intest_find_plugins_priority_tie_raises.These are purely cosmetic and won’t affect the validity of the tests.
tests/test_command.py (1)
269-271: Command test fixture volume slimming is fine; ClassVar for lists is optionalUsing only
["root"],["opt"], and["fs1"]inCommandTestskeeps the CLI exercising all provider types while reducing loop-backed setup work, consistent with the broader test-slimming in this PR. As with other fixtures, you can optionally annotate these list attributes asClassVarif you want to silence Ruff’s RUF012 warning about mutable class attributes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
doc/user_guide.rstscripts/setpaths.shsnapm/_snapm.pysnapm/manager/_manager.pysnapm/manager/plugins/_plugin.pytests/__init__.pytests/test_boot.pytests/test_command.pytests/test_manager.pytests/test_plugin.pytests/test_scheduler.pytests/test_snapm.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/init.py
- tests/test_snapm.py
🧰 Additional context used
🧠 Learnings (16)
📓 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: 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: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:31:58.457Z
Learning: In snapm, plugins are designed to fail loading when their required dependencies are missing (e.g., LVM2 plugin fails to load if device-mapper support is absent, Stratis plugin fails if D-Bus interfaces are missing). This is intentional behavior to ensure plugins only operate when all dependencies are available.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-30T14:19:44.887Z
Learning: In snapshotmanager/snapm, ConfigParser construction for plugins is intentionally centralized in exactly one place within Manager._find_and_verify_plugins(). This architectural decision ensures maintainability, and duplicating this pattern elsewhere would create technical debt.
📚 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:
tests/test_manager.pydoc/user_guide.rstsnapm/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:
tests/test_manager.pydoc/user_guide.rsttests/test_command.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:
tests/test_manager.pydoc/user_guide.rst
📚 Learning: 2025-08-30T13:46:15.300Z
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.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-09-14T17:31:58.457Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:31:58.457Z
Learning: In snapm, plugins are designed to fail loading when their required dependencies are missing (e.g., LVM2 plugin fails to load if device-mapper support is absent, Stratis plugin fails if D-Bus interfaces are missing). This is intentional behavior to ensure plugins only operate when all dependencies are available.
Applied to files:
tests/test_manager.pydoc/user_guide.rst
📚 Learning: 2025-08-26T19:07:42.290Z
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.
Applied to files:
tests/test_manager.pytests/test_boot.pytests/test_scheduler.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.pysnapm/manager/plugins/_plugin.pysnapm/manager/_manager.py
📚 Learning: 2025-12-13T00:14:20.405Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-13T00:14:20.405Z
Learning: In the snapm codebase, snapshot set names are unique and two mounts cannot exist for the same `Selection(name=...)`. Therefore, `manager.mounts.find_mounts(Selection(name=...))` cannot return multiple mounts.
Applied to files:
tests/test_manager.py
📚 Learning: 2025-08-27T21:01:32.324Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-27T21:01:32.324Z
Learning: In the snapm project, the design philosophy is to fail fast when expected files (like systemd units or tmpfiles.d configuration) don't exist during installation steps. This is intentional behavior - if these files are missing, it indicates a fundamental issue with the build/test setup that should abort the process immediately rather than continue with an incomplete installation.
Applied to files:
doc/user_guide.rst
📚 Learning: 2025-11-12T21:27:50.414Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 596
File: snapm/_snapm.py:36-44
Timestamp: 2025-11-12T21:27:50.414Z
Learning: In the snapm codebase, the numeric values of SNAPM_DEBUG_* constants (e.g., SNAPM_DEBUG_MANAGER, SNAPM_DEBUG_MOUNTS, SNAPM_DEBUG_SCHEDULE) are not guaranteed to remain stable across versions. Consumers should always use the symbolic constant names rather than persisting numeric masks. SNAPM_DEBUG_ALL is designed to change over time as new subsystems are added, and repurposing or reassigning bit positions for debug flags is acceptable and intentional.
Applied to files:
snapm/_snapm.pytests/test_command.py
📚 Learning: 2025-10-10T19:18:28.912Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 524
File: snapm/command.py:90-92
Timestamp: 2025-10-10T19:18:28.912Z
Learning: In the snapm codebase, logging wrapper functions like `_log_debug_command`, `_log_debug_manager`, and `_log_debug_report` are module-internal helpers that intentionally do not merge caller-supplied `extra` kwargs. These wrappers hard-code the subsystem tag in `extra={"subsystem": SNAPM_SUBSYSTEM_*}`. It is a bug for callers to pass their own `extra` kwarg to these wrappers. This design keeps the code simple and avoids unnecessary defensive checks.
Applied to files:
snapm/_snapm.pytests/test_command.pysnapm/manager/plugins/_plugin.py
📚 Learning: 2025-08-26T17:35:41.366Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-26T17:35:41.366Z
Learning: In the snapm virt_tests SnapmTestVM class, the run_command() method defaults to check=True, so explicit check=True parameters should not be added throughout the code. The check=False parameter is used intentionally for specific scenarios like expected-fail file existence checks.
Applied to files:
tests/test_command.py
📚 Learning: 2025-08-26T17:36:29.365Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-26T17:36:29.365Z
Learning: In the snapm virt_tests SnapmTestVM class, the get_command_output() method already strips whitespace from returned output, so additional .strip() calls on the returned values are unnecessary.
Applied to files:
tests/test_command.py
📚 Learning: 2025-12-30T14:19:44.887Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-30T14:19:44.887Z
Learning: In snapshotmanager/snapm, ConfigParser construction for plugins is intentionally centralized in exactly one place within Manager._find_and_verify_plugins(). This architectural decision ensures maintainability, and duplicating this pattern elsewhere would create technical debt.
Applied to files:
snapm/manager/plugins/_plugin.pysnapm/manager/_manager.py
📚 Learning: 2025-09-24T02:49:21.752Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 490
File: snapm/manager/plugins/lvm2.py:618-629
Timestamp: 2025-09-24T02:49:21.752Z
Learning: When API changes affect return types (like changing from int to tuple), all supporting infrastructure needs updating including mock scripts in tests, not just the call sites that consume the API.
Applied to files:
tests/test_plugin.py
🧬 Code graph analysis (3)
tests/test_manager.py (2)
tests/test_plugin.py (2)
LowPrioPlugin(209-211)HighPrioPlugin(193-196)snapm/manager/_manager.py (1)
_find_and_verify_plugins(925-996)
snapm/manager/plugins/_plugin.py (2)
snapm/_snapm.py (1)
Snapshot(1531-1892)snapm/manager/plugins/lvm2.py (6)
discover_snapshots(806-812)discover_snapshots(1073-1124)discover_snapshots(1387-1433)activate_snapshot(973-980)deactivate_snapshot(982-989)set_autoactivate(991-1009)
snapm/manager/_manager.py (1)
snapm/manager/plugins/_plugin.py (1)
Plugin(112-348)
🪛 Ruff (0.14.10)
tests/test_manager.py
158-158: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
159-159: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
160-160: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
991-991: Unused method argument: source
(ARG002)
1002-1002: Unused method argument: source
(ARG002)
1053-1053: Local variable mock_logger is assigned to but never used
Remove assignment to unused variable mock_logger
(F841)
1069-1069: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
1070-1070: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/test_command.py
269-269: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
270-270: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
271-271: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
snapm/manager/_manager.py
948-948: Avoid specifying long messages outside the exception class
(TRY003)
955-957: Avoid specifying long messages outside the exception class
(TRY003)
991-993: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_boot.py
86-86: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/test_scheduler.py
57-57: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
58-58: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🪛 Shellcheck (0.11.0)
scripts/setpaths.sh
[warning] 17-17: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
[warning] 21-21: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
[warning] 29-29: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
[warning] 35-35: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
⏰ 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). (32)
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-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: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-43-x86_64
- GitHub Check: rpm-build:epel-9-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-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-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-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, fedora42)
🔇 Additional comments (17)
snapm/manager/_manager.py (2)
904-923: Plugin config loader matches centralised ConfigParser designDeriving
plugin_conf_fileas<plugin_name>.conf, loading it from_PLUGINS_D_PATHwhen present, and always attachingcfg.plugin_conf_filefor logging keeps plugin configuration handling consistent with the intended single construction point inManager._find_and_verify_plugins(). This should give clearer warning messages without changing behaviour for plugins that lack config.
925-997: Priority-based provider selection logic is consistent and well-scopedCollecting all capable plugins per source, then resolving a single provider via
get_highest_priority(with a hard failure on priority ties) aligns with the documented priority semantics and avoids ambiguous provider choice. UsingSnapmNoProviderErrorfor the tie case is slightly overloaded but acceptable, and the implementation guards against empty provider lists before calling the helper.tests/test_plugin.py (1)
9-18: New plugin priority and limits tests cover key behaviours wellUsing
MockPluginplusConfigParserto assert default (sentinel) priority, subclass-set priorities, config-file overrides, andPluginLimitshandling of invalid integers gives good coverage of the new priority and limits semantics. Enforcing that the abstractPluginbase class cannot be instantiated directly is also a useful guard on the public API.Also applies to: 44-55, 179-227
doc/user_guide.rst (1)
1063-1160: Plugin priority documentation aligns with the implemented selection modelThe new sections clearly describe default priorities for existing plugins, how to override them via
/etc/snapm/plugins.d/*.conf, and how the manager prefers the highest-priority eligible provider per source, while leaving existing snapshots untouched. The added report-field and troubleshooting guidance (“Wrong Plugin Selected for Snapshots”, including checkingplugin listand configs) matches the manager’s behaviour and should be very helpful operationally.Also applies to: 1838-1866, 1870-1883
tests/test_command.py (1)
205-212: Debug mask test correctly tracks the new plugin debug subsystemExtending
args.debugto"manager,command,report,schedule,mounts,fsdiff,plugin"and still assertingget_debug_mask() == SNAPM_DEBUG_ALLensures the new plugin debug flag is wired into the “all” aggregate as expected. This keeps the CLI behaviour and test aligned with the evolving set of debug subsystems.snapm/_snapm.py (5)
12-12: LGTM! Plugin debug subsystem additions are well-structured.The new
SNAPM_DEBUG_PLUGINconstant correctly follows the power-of-2 sequence (64), and the corresponding subsystem nameSNAPM_SUBSYSTEM_PLUGINfollows the establishedsnapm.*naming convention. All mappings are consistently updated.Also applies to: 45-45, 53-53, 63-63, 72-72
1531-1535: LGTM! Snapshot converted to abstract base class correctly.The conversion to
ABCproperly enforces that snapshot plugins must implement concrete subclasses. The docstring accurately reflects the new role as an abstract base class.
1657-1755: LGTM! Abstract property declarations follow correct patterns.The
@abstractmethoddecorators are correctly placed after@property(required ordering). The selection of which properties to make abstract is appropriate—only those requiring plugin-specific implementations (origin,origin_options,devpath,status,size,free,autoactivategetter) are marked abstract, whilst properties with base-class implementations remain concrete.
1874-1878: LGTM! Abstract method declaration is correct.The
invalidate_cachemethod is appropriately marked as abstract since cache invalidation logic varies by plugin implementation.
2355-2355: LGTM! New constants properly exported.Both
SNAPM_DEBUG_PLUGINandSNAPM_SUBSYSTEM_PLUGINare correctly added to__all__, maintaining consistency with the existing export pattern.Also applies to: 2371-2371
snapm/manager/plugins/_plugin.py (7)
13-14: LGTM! Logging infrastructure follows established patterns.The
_log_debug_pluginhelper correctly hard-codes the subsystem tag inextra, following the established pattern for module-internal logging wrappers. Based on learnings, this design keeps the code simple and avoids unnecessary defensive checks.Also applies to: 16-16, 58-68
49-56: LGTM! Priority constants are well-defined.The
PLUGIN_NO_PRIORITY = 0sentinel is correctly documented and aligns with the PR discussion confirming that0is an internal sentinel meaning "plugins should use their static default priority."
71-109: LGTM! PluginLimits validation handles errors gracefully.The validation correctly catches
ValueErrorfromgetint()and logs warnings including the configuration filename (cfg.plugin_conf_file). Per PR discussion, Manager always setsplugin_conf_fileon the ConfigParser before plugin initialisation.
112-150: LGTM! Plugin ABC conversion and priority validation are correctly implemented.The priority validation at line 142 correctly rejects both non-integer values and values ≤ 0. Per the PR discussion, rejecting
priority == 0is intentional since0is reserved as an internal sentinel indicating plugins should use their static default priority.
194-348: LGTM! Abstract method declarations enforce plugin contract.All plugin lifecycle methods are correctly marked with
@abstractmethod, ensuring concrete plugin implementations must provide these methods. The comprehensive docstrings clearly document the expected parameters and exceptions for each method.
539-540: LGTM! New exports are correctly added.Both
PLUGIN_NO_PRIORITYandPluginLimitsare appropriately exported for use by plugin implementations and external code.
66-68: Remove unused_log_debug_pluginhelper function.This function is defined at lines 66–68 but is not called anywhere in the codebase. Dead code should be removed.
⛔ Skipped due to learnings
Learnt from: bmr-cymru Repo: snapshotmanager/snapm PR: 524 File: snapm/command.py:90-92 Timestamp: 2025-10-10T19:18:28.912Z Learning: In the snapm codebase, logging wrapper functions like `_log_debug_command`, `_log_debug_manager`, and `_log_debug_report` are module-internal helpers that intentionally do not merge caller-supplied `extra` kwargs. These wrappers hard-code the subsystem tag in `extra={"subsystem": SNAPM_SUBSYSTEM_*}`. It is a bug for callers to pass their own `extra` kwarg to these wrappers. This design keeps the code simple and avoids unnecessary defensive checks.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/setpaths.sh (2)
17-17: Use glob patterns instead of quoted regex for substring checks.The
=~operator treats quoted right-hand sides as literal strings rather than regex patterns. For substring containment checks, glob pattern matching with==is the proper approach.🔎 Proposed fix for all four guard checks
-if ! [[ "$PATH" =~ "$PWD/bin:" ]]; then +if ! [[ "$PATH" == *"$PWD/bin:"* ]]; then export PATH="$PWD/bin:$PATH" fi -if ! [[ "$PYTHONPATH" =~ "$PWD:" ]]; then +if ! [[ "$PYTHONPATH" == *"$PWD:"* ]]; then if [ "$PYTHONPATH" != "" ]; then export PYTHONPATH="$PWD:$PYTHONPATH" else export PYTHONPATH="$PWD" fi fi -if ! [[ "$PATH" =~ "$PWD/../boom-boot/bin:" ]]; then +if ! [[ "$PATH" == *"$PWD/../boom-boot/bin:"* ]]; then if [ -d "$PWD/../boom-boot" ]; then export PATH="$PWD/../boom-boot/bin:$PATH" fi fi -if ! [[ "$PYTHONPATH" =~ "$PWD/../boom-boot:" ]]; then +if ! [[ "$PYTHONPATH" == *"$PWD/../boom-boot:"* ]]; then export PYTHONPATH="$PWD/../boom-boot:$PYTHONPATH" fiAlso applies to: 21-21, 29-29, 35-35
35-37: Add directory existence and empty PYTHONPATH checks for consistency.The boom-boot PYTHONPATH handling lacks two safeguards present elsewhere in the script:
- Lines 29-33 verify boom-boot exists before modifying PATH, but lines 35-37 unconditionally modify PYTHONPATH
- Lines 22-26 handle empty PYTHONPATH specially, but lines 35-37 always append (potentially creating a leading colon)
Consider aligning the boom-boot PYTHONPATH logic with the established patterns.
🔎 Proposed fix for consistent handling
-if ! [[ "$PYTHONPATH" =~ "$PWD/../boom-boot:" ]]; then - export PYTHONPATH="$PWD/../boom-boot:$PYTHONPATH" -fi +if ! [[ "$PYTHONPATH" == *"$PWD/../boom-boot:"* ]]; then + if [ -d "$PWD/../boom-boot" ]; then + if [ "$PYTHONPATH" != "" ]; then + export PYTHONPATH="$PWD/../boom-boot:$PYTHONPATH" + else + export PYTHONPATH="$PWD/../boom-boot" + fi + fi +fi
🧹 Nitpick comments (1)
snapm/manager/_manager.py (1)
939-961: LGTM: Priority resolution logic is sound.The
get_highest_priorityhelper correctly selects the single provider with maximum priority and appropriately raises an error when multiple providers share the highest priority, preventing ambiguous selection.Note: Static analysis flags the error messages as potentially long (TRY003), but they provide clear context for debugging multi-provider conflicts, so this is acceptable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/setpaths.shsnapm/manager/_manager.py
🧰 Additional context used
🧠 Learnings (6)
📓 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: 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: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:31:58.457Z
Learning: In snapm, plugins are designed to fail loading when their required dependencies are missing (e.g., LVM2 plugin fails to load if device-mapper support is absent, Stratis plugin fails if D-Bus interfaces are missing). This is intentional behavior to ensure plugins only operate when all dependencies are available.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-30T14:19:44.887Z
Learning: In snapshotmanager/snapm, ConfigParser construction for plugins is intentionally centralized in exactly one place within Manager._find_and_verify_plugins(). This architectural decision ensures maintainability, and duplicating this pattern elsewhere would create technical debt.
📚 Learning: 2025-12-30T14:19:44.887Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-30T14:19:44.887Z
Learning: In snapshotmanager/snapm, ConfigParser construction for plugins is intentionally centralized in exactly one place within Manager._find_and_verify_plugins(). This architectural decision ensures maintainability, and duplicating this pattern elsewhere would create technical debt.
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-09-24T18:12:29.925Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-24T18:12:29.925Z
Learning: Plugin.start_transaction() initializes self.size_map = {} for all providers and Plugin.end_transaction() resets it to None, so logging that references provider.size_map is safe and cannot encounter a missing attribute in active transactions.
Applied to files:
snapm/manager/_manager.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:
snapm/manager/_manager.py
📚 Learning: 2025-08-21T20:16:21.492Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 383
File: virt_tests/strategy.py:211-211
Timestamp: 2025-08-21T20:16:21.492Z
Learning: For boot title validation in shell scripts, when a string should not contain single quotes, the proper approach is to check for single quotes and fail rather than attempting complex escaping that would violate shell quoting rules.
Applied to files:
scripts/setpaths.sh
🧬 Code graph analysis (1)
snapm/manager/_manager.py (1)
snapm/manager/plugins/_plugin.py (4)
Plugin(112-348)origin_from_mount_point(343-348)check_create_snapshot(215-227)create_snapshot(230-242)
🪛 Ruff (0.14.10)
snapm/manager/_manager.py
950-950: Avoid specifying long messages outside the exception class
(TRY003)
957-959: Avoid specifying long messages outside the exception class
(TRY003)
993-995: Avoid specifying long messages outside the exception class
(TRY003)
🪛 Shellcheck (0.11.0)
scripts/setpaths.sh
[warning] 17-17: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
[warning] 21-21: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
[warning] 29-29: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
[warning] 35-35: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
⏰ 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). (37)
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-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:centos-stream-10-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-43-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: rpm-build:fedora-42-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:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-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, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: python-basic-tests
🔇 Additional comments (5)
snapm/manager/_manager.py (5)
14-14: LGTM: Import changes support priority resolution.The addition of
defaultdictfor multi-provider collection andTYPE_CHECKINGfor forward declarations are appropriate for the new priority-based provider selection mechanism.Also applies to: 21-21, 70-72
913-923: LGTM: Config path handling with context for logging.The per-plugin configuration path construction and assignment of
plugin_conf_fileto theConfigParserinstance provides useful context for warning logs. Based on learnings, this centralised pattern is intentional and maintained in exactly one location.
964-998: LGTM: Multi-provider collection with priority resolution.The refactored logic correctly collects all capable providers per source using
defaultdict(list), then resolves each source to its single highest-priority provider. The returned mapping maintains the same interface (source → provider), ensuring backward compatibility with calling code.
1251-1251: LGTM: Updated iteration over resolved provider map.The iteration pattern correctly unpacks
(source, provider)pairs from the resolved provider mapping returned by_find_and_verify_plugins.Also applies to: 1260-1260, 1298-1298
1271-1271: LGTM: Provider method calls use resolved provider.The calls to
origin_from_mount_point,check_create_snapshot, andcreate_snapshotcorrectly use the resolvedproviderinstance from the iteration, replacing previousprovider_map[source]lookups.Also applies to: 1274-1274, 1305-1305
|
@coderabbitai: re:
That's because it's impossible for |
|
Tip For best results, initiate chat on the files or code changes. @bmr-cymru: You're absolutely right! I see the control flow now—if 🧠 Learnings used |
|
@coderabbitai: re:
You seem confused. The From the
Which is what we want. This is how it is done in the Fedora |
|
Tip For best results, initiate chat on the files or code changes. @bmr-cymru: You're absolutely right, and I apologize for the confusion in my original comment! The I was conflating two related behaviors: when you quote the pattern (or quote a variable expansion containing the pattern), it forces literal matching rather than regex pattern matching. So for substring containment checks like checking if Best practice, as documented in various bash references, is to put patterns into variables when using =~, and if you're doing literal substring matching (rather than regex pattern matching), quoting the variable expansion gives you that literal behavior. You're absolutely right that if it's good enough for Fedora's 🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. @bmr-cymru: You're absolutely right, and I apologize for the confusion! The The pattern used in the script: [[ "$PATH" =~ (^|:)"${repo_bin}"(:|$) ]]is the proper way to perform regex matching in bash to check if a path component exists in 🧠 Learnings used |
We've been using a little script for ages, setpaths.sh, that sets up PATH and PYTHONPATH for running snapm/boom from a git checkout: add it to the scripts/ directory in the repository. Resolves: #847 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
bmr-cymru
force-pushed
the
bmr-btrfs-plugin
branch
from
724ff7f to
a67143c
Compare
|
Tip For best results, initiate chat on the files or code changes. @bmr-cymru: You're absolutely correct, and I apologize for the confusion in my review comment. The 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/setpaths.sh (1)
17-39: LGTM with note on prior discussion.The script logic is correct and handles PATH/PYTHONPATH modifications appropriately, including empty PYTHONPATH and directory existence checks. The use of
=~with quoted strings has been discussed in prior comments and you've clarified the intended behaviour. Whilst glob patterns ([[ "$PATH" == *"$PWD/bin:"* ]]) would be more idiomatic for substring checks, the current implementation works correctly for literal substring matching in modern Bash.
🧹 Nitpick comments (1)
snapm/manager/_manager.py (1)
939-998: Excellent implementation of priority-based provider resolution.The refactored logic correctly:
- Collects all candidate providers per source using
defaultdict(list)- Selects the highest-priority provider via the inner
get_highest_priorityfunction- Detects and raises on priority ties to prevent ambiguity
- Verifies each source has at least one provider
The implementation properly addresses the PR's objective of using numerical priority as a tiebreaker when multiple providers can snapshot a source.
Optional: Consider extracting error messages to reduce TRY003 warnings
Static analysis flags the long error messages at lines 950, 957-959. Whilst the current implementation is acceptable, you could optionally extract these as module-level constants or define shorter messages:
+_ERR_MULTIPLE_PROVIDERS = "Multiple providers with priority {}: {}" + def get_highest_priority(providers: List["Plugin"]) -> "Plugin": """ Given a list of candidate providers, select the one with the highest numeric priority. """ if not providers: raise ValueError("Empty providers list") max_priority = max(prov.priority for prov in providers) candidates = [prov for prov in providers if prov.priority == max_priority] if len(candidates) > 1: names = ",".join(prov.name for prov in candidates) - raise SnapmNoProviderError( - f"Multiple providers with priority {max_priority}: {names}" - ) + raise SnapmNoProviderError(_ERR_MULTIPLE_PROVIDERS.format(max_priority, names)) return candidates[0]
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/setpaths.shsnapm/manager/_manager.py
🧰 Additional context used
🧠 Learnings (6)
📓 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: 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: 478
File: snapm/manager/plugins/lvm2.py:512-518
Timestamp: 2025-09-14T17:31:58.457Z
Learning: In snapm, plugins are designed to fail loading when their required dependencies are missing (e.g., LVM2 plugin fails to load if device-mapper support is absent, Stratis plugin fails if D-Bus interfaces are missing). This is intentional behavior to ensure plugins only operate when all dependencies are available.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-30T14:19:44.887Z
Learning: In snapshotmanager/snapm, ConfigParser construction for plugins is intentionally centralized in exactly one place within Manager._find_and_verify_plugins(). This architectural decision ensures maintainability, and duplicating this pattern elsewhere would create technical debt.
📚 Learning: 2025-12-30T14:19:44.887Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-12-30T14:19:44.887Z
Learning: In snapshotmanager/snapm, ConfigParser construction for plugins is intentionally centralized in exactly one place within Manager._find_and_verify_plugins(). This architectural decision ensures maintainability, and duplicating this pattern elsewhere would create technical debt.
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-09-24T18:12:29.925Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-24T18:12:29.925Z
Learning: Plugin.start_transaction() initializes self.size_map = {} for all providers and Plugin.end_transaction() resets it to None, so logging that references provider.size_map is safe and cannot encounter a missing attribute in active transactions.
Applied to files:
snapm/manager/_manager.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:
snapm/manager/_manager.py
📚 Learning: 2025-08-21T20:16:21.492Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 383
File: virt_tests/strategy.py:211-211
Timestamp: 2025-08-21T20:16:21.492Z
Learning: For boot title validation in shell scripts, when a string should not contain single quotes, the proper approach is to check for single quotes and fail rather than attempting complex escaping that would violate shell quoting rules.
Applied to files:
scripts/setpaths.sh
🪛 Ruff (0.14.10)
snapm/manager/_manager.py
950-950: Avoid specifying long messages outside the exception class
(TRY003)
957-959: Avoid specifying long messages outside the exception class
(TRY003)
993-995: Avoid specifying long messages outside the exception class
(TRY003)
🪛 Shellcheck (0.11.0)
scripts/setpaths.sh
[warning] 17-17: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
[warning] 21-21: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
[warning] 29-29: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
[warning] 35-35: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
⏰ 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:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-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-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-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: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-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-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- 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 (uefi, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: python-basic-tests
🔇 Additional comments (3)
snapm/manager/_manager.py (3)
14-14: LGTM! Proper typing infrastructure added.The addition of
defaultdictfor multi-provider collection and theTYPE_CHECKINGguard with forward declaration forPluginfollows Python best practices to avoid circular imports whilst maintaining type safety.Also applies to: 21-21, 70-72
913-922: LGTM! Correct handling of optional plugin configuration.The method correctly computes the per-plugin configuration path, sets
plugin_conf_fileon the ConfigParser for debugging context, and only loads the configuration if it exists. This aligns with the PR's objective to enhance plugin configuration handling.
1251-1251: LGTM! Cleaner iteration pattern with resolved providers.The updates to
create_snapshot_setcorrectly use the new provider resolution structure. Iterating overprovider_map.items()directly provides(source, provider)tuples, eliminating repeated dictionary lookups and improving code clarity whilst maintaining correct duplicate detection logic.Also applies to: 1260-1276, 1298-1308
This pull request lays the groundwork for adding the Btrfs plugin
(and eventually, the plugin itself).Lvm2Cow.can_snapshot()Edit: Let's keep this PR for just the pre-req work and have a new clean slate for the plugin itself. It makes the branch name a bit misleading, but we can live with that.
Resolves: #552
Resolves: #834
Resolves: #835
Resolves: #836
Resolves: #837
Resolves: #838
Resolves: #839
Resolves: #841
Resolves: #842
Resolves: #845
Resolves: #846
Resolves: #847
Resolves: #850
Summary by CodeRabbit
New Features
Documentation
Behaviour
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.