stratis: re-use initial DBus query for StratisSnapshot cache #615
Conversation
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
bmr-cymru
force-pushed
the
bmr-stratis-cache-harder
branch
from
2aa5c99 to
4cca663
Compare
|
I think we had a run collision on the Jenkins nodes; everything was failing on "Schedule named 'hourly' already exists". No stale state on manually logging into the nodes. Re-pushing to re-test. |
bmr-cymru
force-pushed
the
bmr-stratis-cache-harder
branch
from
4cca663 to
49be4d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
snapm/manager/plugins/stratis.py (1)
510-512: Consider reusing the loop's filesystem object to avoid redundant lookup.Whilst
_get_pool_filesystem()performs an in-memory search rather than a DBus call (which achieves the PR's primary goal), you could further optimize by reusing thefilesystemobject from line 491 and looking up the pool directly frommanaged_objects:pool_object_path = filesystem.Pool() cache_pool = MOPool(managed_objects[pool_object_path]) cache_filesystem = filesystemThis avoids the search overhead of calling
_get_pool_filesystem()for each snapshot.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snapm/manager/plugins/stratis.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-14T02:14:30.316Z
Learning: PR #478 in snapshotmanager/snapm fixes a regression in the LVM2 plugin where `can_snapshot()` method throws `SnapmCalloutError` when probing non-device-mapper devices. The fix adds device major number validation to prevent calling dmsetup on non-dm devices, allowing proper "no provider found" error messages instead of confusing dmsetup errors.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (38)
- GitHub Check: rpm-build:fedora-42-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-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: python-basic-tests
🔇 Additional comments (3)
snapm/manager/plugins/stratis.py (3)
195-196: LGTM! Signature change supports cache seeding.The optional parameters enable discovery to seed the DBus cache, reducing redundant GMO calls whilst maintaining backward compatibility.
201-209: LGTM! Conditional initialisation correctly handles cache seeding.The logic appropriately initialises the cache from provided parameters or falls back to eager GMO via
_get_dbus_cache(), preserving correct behaviour for snapshots created outside of discovery.
523-524: LGTM! Cache parameters correctly passed to constructor.The cached pool and filesystem objects are properly supplied to enable the performance optimisation.
Currently each instantiated StratisSnapshot issues GetManagedObjects() to initalise its cache. This leads to very poor performance when many Stratis snapshots exist. Fix this by passing the initial GMO() result from Stratis.discover_snapshots() through to the StratisSnapshot contstructor and only initiate per-Snapshot GMO() calls once the cache has been invalidated. Resolves: #614 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
bmr-cymru
force-pushed
the
bmr-stratis-cache-harder
branch
from
49be4d4 to
18cb4c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
snapm/manager/plugins/stratis.py (1)
201-209: Consider explicit None checking for clarity.The truthiness check on line 201 works correctly for DBus objects, but using explicit
is not Nonecomparisons would be clearer and more defensive against edge cases.Apply this diff to make the intent explicit:
- if cache_pool and cache_filesystem: + if cache_pool is not None and cache_filesystem is not None: self._pool = cache_pool self._filesystem = cache_filesystem self._cache_ts = time()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snapm/manager/plugins/stratis.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-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.
🧬 Code graph analysis (1)
snapm/manager/plugins/stratis.py (1)
snapm/manager/plugins/lvm2.py (1)
origin(336-337)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (38)
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-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: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: python-basic-tests
🔇 Additional comments (3)
snapm/manager/plugins/stratis.py (3)
195-196: LGTM! Constructor signature updated to accept cache parameters.The addition of
cache_poolandcache_filesystemoptional parameters aligns with the PR objective to reuse shared GMO data during discovery, eliminating per-snapshot DBus calls.
510-512: LGTM! Cache construction is correct.The cache is properly constructed from the shared
managed_objectsretrieved at the start ofdiscover_snapshots. Thepool_object_pathis guaranteed to exist inmanaged_objectssince it was used to buildpath_to_name, and the types match what_get_dbus_cacheexpects.
523-524: Excellent! Optimization achieved as intended.Passing the cached DBus objects here successfully eliminates the per-snapshot GMO call, reducing O(n) calls to a single O(1) call during discovery. The cache parameters are correctly omitted in
create_snapshot(line 709) andrename_snapshot(line 849), ensuring fresh data is fetched for newly created snapshots.
Currently each instantiated StratisSnapshot issues GetManagedObjects() to initalise its cache. This leads to very poor performance when many Stratis snapshots exist. Fix this by passing the initial GMO() result from Stratis.discover_snapshots() through to the StratisSnapshot contstructor and only initiate per-Snapshot GMO() calls once the cache has been invalidated.
Resolves: #614
Summary by CodeRabbit
✏️ Tip: You can customise this high-level summary in your review settings.