From 541b01d48a5adba903fb6043df0dcc23e82afad0 Mon Sep 17 00:00:00 2001 From: "Bryn M. Reeves" Date: Tue, 12 May 2026 19:22:22 +0100 Subject: [PATCH 1/3] Warn users that diff operations compare whole file system by default Resolves: RHEL-174474 Signed-off-by: Bryn M. Reeves --- ...-when-no-s-start-path-Options.from_p.patch | 42 ++++++++++++ ...ult-diff-start-path-behaviour-in-sna.patch | 48 ++++++++++++++ ...on-of-s-start-path-to-user_guide.rst.patch | 66 +++++++++++++++++++ snapm.spec | 5 +- 4 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 0002-fsdiff-warn-user-when-no-s-start-path-Options.from_p.patch create mode 100644 0003-doc-clarify-default-diff-start-path-behaviour-in-sna.patch create mode 100644 0004-doc-add-discussion-of-s-start-path-to-user_guide.rst.patch diff --git a/0002-fsdiff-warn-user-when-no-s-start-path-Options.from_p.patch b/0002-fsdiff-warn-user-when-no-s-start-path-Options.from_p.patch new file mode 100644 index 0000000..6b65ca8 --- /dev/null +++ b/0002-fsdiff-warn-user-when-no-s-start-path-Options.from_p.patch @@ -0,0 +1,42 @@ +From a8ded86017e1bdce8e2b5f3e69b1d798312d63f4 Mon Sep 17 00:00:00 2001 +From: "Bryn M. Reeves" +Date: Mon, 30 Mar 2026 13:44:57 +0100 +Subject: [PATCH 02/10] fsdiff: warn user when no + -s/--start-path/Options.from_path given + +Unless -q/--quiet/Options.quiet is specified issue a warning to the +termional when a diff operation does not have any explicit start path +defined (since these can almost always reasonably be assumed to be slow +and costly). + + # snapm snapset diff before-upgrade . + WARNING - Consider setting -s/--start-path to reduce diff runtime and memory usage + Gathering paths from before-upgrade /: Quit! + +Resolves: #968 + +Suggested-by: Filip Suba +Signed-off-by: Bryn M. Reeves +--- + snapm/fsdiff/fsdiffer.py | 5 +++++ + 1 file changed, 5 insertions(+) + +diff --git a/snapm/fsdiff/fsdiffer.py b/snapm/fsdiff/fsdiffer.py +index bd64f076..f9e2cf4f 100644 +--- a/snapm/fsdiff/fsdiffer.py ++++ b/snapm/fsdiff/fsdiffer.py +@@ -163,6 +163,11 @@ class FsDiffer: + if options.no_mem_check: + _log_warn("RSS memory pressure safety checks disabled") + ++ if not options.from_path and not options.quiet: ++ _log_warn( ++ "Consider setting -s/--start-path to reduce run time and memory usage" ++ ) ++ + #: Manager context for snapshot operations (used by future methods) + self.manager: "Manager" = manager + self.options: DiffOptions = options +-- +2.53.0 + diff --git a/0003-doc-clarify-default-diff-start-path-behaviour-in-sna.patch b/0003-doc-clarify-default-diff-start-path-behaviour-in-sna.patch new file mode 100644 index 0000000..7625b80 --- /dev/null +++ b/0003-doc-clarify-default-diff-start-path-behaviour-in-sna.patch @@ -0,0 +1,48 @@ +From 8d74490b35cb9592f554c4b5804fe0d4e5b6c77c Mon Sep 17 00:00:00 2001 +From: "Bryn M. Reeves" +Date: Tue, 31 Mar 2026 13:04:52 +0100 +Subject: [PATCH 03/10] doc: clarify default diff start path behaviour in + snapm.8 + +Related: #959 + +Signed-off-by: Bryn M. Reeves +--- + man/man8/snapm.8 | 14 ++++++++++++++ + 1 file changed, 14 insertions(+) + +diff --git a/man/man8/snapm.8 b/man/man8/snapm.8 +index 9baad38e..dcc20ee9 100644 +--- a/man/man8/snapm.8 ++++ b/man/man8/snapm.8 +@@ -1081,6 +1081,13 @@ Calculates the differences between the \fBfrom\fP and \fBto\fP targets. Targets + may be specified as a snapshot set name, or the special character \fB.\fP to + represent the currently running system (root file system). + .IP ++If \fB--start-path\fP is not specified the comparison proceeds from the root ++directory of the two volumes. This can be costly in terms of both time and ++memory: for better performance, specify one or more start paths in order to ++focus the difference generation on areas of interest. A resource use warning is ++emitted if no start path is given (this warning can be suppressed using the ++\fB--quiet\fP argument). ++.IP + The output format is controlled by \fB--output-format\fP. The \fBtree\fP + format provides a hierarchical view of changes, while \fBdiff\fP provides + standard unified diff output for modified content: see \fBOutput Formats\fP for +@@ -1155,6 +1162,13 @@ Compare snapshot sets and output results in a tabular report. + Performs the same comparison logic as \fBsnapm snapset diff\fP but formats + the output as a report using the standard reporting columns. + .IP ++If \fB--start-path\fP is not specified the comparison proceeds from the root ++directory of the two volumes. This can be costly in terms of both time and ++memory: for better performance, specify one or more start paths in order to ++focus the difference generation on areas of interest. A resource use warning is ++emitted if no start path is given (this warning can be suppressed using the ++\fB--quiet\fP argument). ++.IP + This command supports standard reporting arguments including \fB--options\fP, + \fB--sort\fP, \fB--noheadings\fP, and \fB--json\fP. + .TP +-- +2.53.0 + diff --git a/0004-doc-add-discussion-of-s-start-path-to-user_guide.rst.patch b/0004-doc-add-discussion-of-s-start-path-to-user_guide.rst.patch new file mode 100644 index 0000000..0b712a7 --- /dev/null +++ b/0004-doc-add-discussion-of-s-start-path-to-user_guide.rst.patch @@ -0,0 +1,66 @@ +From 88d4a0e5f4d7d619d58ecdc47a8a4f0ae74a21eb Mon Sep 17 00:00:00 2001 +From: "Bryn M. Reeves" +Date: Tue, 31 Mar 2026 16:23:30 +0100 +Subject: [PATCH 04/10] doc: add discussion of -s/--start-path to + user_guide.rst + +Resolves: #959 + +Signed-off-by: Bryn M. Reeves +--- + doc/user_guide.rst | 19 +++++++++++++++++++ + 1 file changed, 19 insertions(+) + +diff --git a/doc/user_guide.rst b/doc/user_guide.rst +index 6acfb943..71fee02b 100644 +--- a/doc/user_guide.rst ++++ b/doc/user_guide.rst +@@ -219,6 +219,11 @@ changes: + ownership (UID/GID), and extended attributes (xattrs). + * **Content Comparison**: Performs deep content comparisons for regular + files, including generating standard unified diffs for text files. ++* **Start Path Control**: By default difference engine operations proceed from ++ the root directory of snapshot sets (or the system root file system). This ++ can be overridden by specifying one or more start paths for the comparison. ++ This allows the user to focus the operation on areas of interest and lowers ++ run time and memory consumption. + + Output Formats and Use Cases + ---------------------------- +@@ -294,6 +299,14 @@ resource-intensive. To improve performance, ``snapm`` implements a + ``--cache-expires=EXPIRES_SECS`` options. These options are mutually + exclusive. + ++By default a ``diff`` or ``diffreport`` command will start at the root mount ++of the respective snapshot set (or the system root file system). This can be ++costly both in terms of time and memory consumption. The comparison can be ++narrowed to specific directories of interest by specifying one or more start ++paths using ``-s|--start-path PATH``. If no start path is given the tools ++will warn about the potential resource use. This warning can be suppressed if ++desired using ``--quiet``. ++ + Command Reference + ================= + +@@ -479,12 +492,18 @@ ignoring file modification times to focus only on content: + + Options are available to control the comparison: + ++* ``--start-path PATH`` / ``-s PATH``: Begin comparison at ``PATH`` ++ (may be specified zero or more times) + * ``--content-only`` / ``-c``: Only check for file content changes + * ``--ignore-timestamps`` / ``-t``: Ignore modification times + * ``--ignore-permissions`` / ``-p``: Ignore permission changes + * ``--ignore-ownership`` / ``-w``: Ignore ownership changes + * ``--include-pattern`` / ``--exclude-pattern``: Filter paths using glob patterns + ++The ``PATH`` value given to ``--start-path PATH`` / ``-s PATH`` is evaluated ++relative to the root directory of the snapshot set (or system root file system ++when using ``.``). ++ + Output Formats + ~~~~~~~~~~~~~~ + +-- +2.53.0 + diff --git a/snapm.spec b/snapm.spec index 8dd195f..293909b 100644 --- a/snapm.spec +++ b/snapm.spec @@ -8,7 +8,10 @@ Summary: %{summary} License: Apache-2.0 URL: https://github.com/snapshotmanager/%{name} Source0: %{url}/archive/v%{version}/%{name}-%{version}.tar.gz -Patch1: 0001-tests-skip-a-handful-of-tests-in-RH-CI-environments-.patch +Patch0: 0001-tests-skip-a-handful-of-tests-in-RH-CI-environments-.patch +Patch1: 0002-fsdiff-warn-user-when-no-s-start-path-Options.from_p.patch +Patch2: 0003-doc-clarify-default-diff-start-path-behaviour-in-sna.patch +Patch3: 0004-doc-add-discussion-of-s-start-path-to-user_guide.rst.patch BuildArch: noarch From cbdb26624acd5e036d42379f9f5fe2cb73d4ca70 Mon Sep 17 00:00:00 2001 From: "Bryn M. Reeves" Date: Tue, 12 May 2026 19:26:46 +0100 Subject: [PATCH 2/3] Fix plugin limit accounting for API users Resolves: RHEL-174493 Signed-off-by: Bryn M. Reeves --- ...-limit-accounting-in-create-delete-_.patch | 119 ++++++++ ...vm2-fix-pool-origin-limit-accounting.patch | 288 ++++++++++++++++++ snapm.spec | 2 + 3 files changed, 409 insertions(+) create mode 100644 0005-stratis-fix-pool-limit-accounting-in-create-delete-_.patch create mode 100644 0006-lvm2-fix-pool-origin-limit-accounting.patch diff --git a/0005-stratis-fix-pool-limit-accounting-in-create-delete-_.patch b/0005-stratis-fix-pool-limit-accounting-in-create-delete-_.patch new file mode 100644 index 0000000..92643e5 --- /dev/null +++ b/0005-stratis-fix-pool-limit-accounting-in-create-delete-_.patch @@ -0,0 +1,119 @@ +From 20fcc2e065cb01c4f47d5be4ab260571241c734c Mon Sep 17 00:00:00 2001 +From: "Bryn M. Reeves" +Date: Sun, 5 Apr 2026 16:27:35 +0100 +Subject: [PATCH 05/10] stratis: fix pool limit accounting in + {create,delete}_snapshot() + +Resolves: #971 + +Signed-off-by: Bryn M. Reeves +--- + snapm/manager/plugins/stratis.py | 29 ++++++++++++++++++++++++----- + tests/test_stratis.py | 22 ++++++++++++++++++++++ + 2 files changed, 46 insertions(+), 5 deletions(-) + +diff --git a/snapm/manager/plugins/stratis.py b/snapm/manager/plugins/stratis.py +index 4c6dc8c6..1321a897 100644 +--- a/snapm/manager/plugins/stratis.py ++++ b/snapm/manager/plugins/stratis.py +@@ -466,7 +466,7 @@ class Stratis(Plugin): + if self.priority == PLUGIN_NO_PRIORITY: + self.priority = STRATIS_STATIC_PRIORITY + +- # pylint: disable=too-many-locals ++ # pylint: disable=too-many-locals,too-many-branches + def discover_snapshots(self): + """ + Discover snapshots managed by this plugin class. +@@ -488,6 +488,12 @@ class Stratis(Plugin): + for path, info in pools().search(managed_objects) + ) + ++ # Initialise pool snapshot counters to 0 ++ for pool in path_to_name.values(): ++ pool_name = str(pool) ++ if pool_name not in self.pools: ++ self.pools[pool_name] = 0 ++ + filesystems_with_props = [ + MOFilesystem(info) + for objpath, info in filesystems(props=None) +@@ -532,11 +538,9 @@ class Stratis(Plugin): + ) + ) + ++ # Set or update pool snapshot counters (limit checking) + for snapshot in snapshots: +- if snapshot.pool_name not in self.pools: +- self.pools[snapshot.pool_name] = 1 +- else: +- self.pools[snapshot.pool_name] += 1 ++ self.pools[snapshot.pool_name] += 1 + + if self.limits.snapshots_per_pool > 0: + for pool, count in self.pools.items(): +@@ -713,6 +717,12 @@ class Stratis(Plugin): + f"Stratis daemon reported no change creating snapshot {snapshot_name}" + ) + ++ # Increment or set pool snapshot counter ++ if pool_name in self.pools: ++ self.pools[pool_name] += 1 ++ else: ++ self.pools[pool_name] = 1 ++ + return StratisSnapshot( + f"{pool_name}/{snapshot_name}", + snapset_name, +@@ -799,6 +809,15 @@ class Stratis(Plugin): + ) + ) + ++ # Decrement pool snapshot counter ++ if pool_name in self.pools: ++ self.pools[pool_name] -= 1 ++ else: ++ # This is harmless but indicates a bug (plugin methods called out-of-sequence). ++ self._log_warn( ++ "Deleted untracked snapshot set member: %s/%s", pool_name, fs_name ++ ) ++ + # pylint: disable=too-many-arguments + def rename_snapshot(self, old_name, origin, snapset_name, timestamp, mount_point): + """ +diff --git a/tests/test_stratis.py b/tests/test_stratis.py +index edbb1a0f..89e80f7c 100644 +--- a/tests/test_stratis.py ++++ b/tests/test_stratis.py +@@ -186,6 +186,28 @@ class StratisTests(unittest.TestCase): + snapshots = stratis_plugin.discover_snapshots() + self.assertEqual(len(snapshots), 2) + ++ def test_stratis_create_delete_pool_accounting(self): ++ # Verify that pool snapshot counters are properly updated around ++ # create/delete operations. ++ ++ # Plugin setup ++ stratis_plugin = stratis.Stratis(log, ConfigParser()) ++ snapshots = stratis_plugin.discover_snapshots() ++ pool1_count = stratis_plugin.pools["pool1"] ++ ++ # Create snapshot via Plugin.create_snapshot() ++ stratis_plugin.start_transaction() ++ stratis_plugin.check_create_snapshot("pool1/fs1", "test", 1721136677, "/opt", "1%SIZE") ++ stratis_plugin.create_snapshot("pool1/fs1", "test", 1721136677, "/opt", "1%SIZE") ++ stratis_plugin.end_transaction() ++ ++ # Verify counter incremented ++ self.assertEqual(stratis_plugin.pools["pool1"], pool1_count + 1) ++ ++ # Delete snapshot & verify decrement ++ stratis_plugin.delete_snapshot("pool1/fs1-snapset_test_1721136677_-opt") ++ self.assertEqual(stratis_plugin.pools["pool1"], pool1_count) ++ + def test_stratis_can_snapshot_no_stratisd(self): + systemctl_stop_args = _systemctl_args("stop") + run(systemctl_stop_args, check=True) +-- +2.53.0 + diff --git a/0006-lvm2-fix-pool-origin-limit-accounting.patch b/0006-lvm2-fix-pool-origin-limit-accounting.patch new file mode 100644 index 0000000..e4ddf6b --- /dev/null +++ b/0006-lvm2-fix-pool-origin-limit-accounting.patch @@ -0,0 +1,288 @@ +From b3bb94d36c1a83bff7b9fc6e7b25b1aa972f4f51 Mon Sep 17 00:00:00 2001 +From: "Bryn M. Reeves" +Date: Mon, 6 Apr 2026 00:02:26 +0100 +Subject: [PATCH 06/10] lvm2: fix pool/origin limit accounting + +Resolves: #974 + +Signed-off-by: Bryn M. Reeves +--- + snapm/manager/plugins/lvm2.py | 85 ++++++++++++++++++++++++++--------- + tests/test_lvm2.py | 74 +++++++++++++++++++++++++++++- + 2 files changed, 136 insertions(+), 23 deletions(-) + +diff --git a/snapm/manager/plugins/lvm2.py b/snapm/manager/plugins/lvm2.py +index 44487f5e..ebda23cb 100644 +--- a/snapm/manager/plugins/lvm2.py ++++ b/snapm/manager/plugins/lvm2.py +@@ -867,7 +867,7 @@ class _Lvm2(Plugin): + (vg_name, lv_name) = self.vg_lv_from_device_path(device) + return path_join(DEV_PREFIX, vg_name, lv_name) + +- def delete_snapshot(self, name): ++ def _delete_snapshot(self, name): + """ + Delete the snapshot named ``name`` + +@@ -1080,36 +1080,37 @@ class Lvm2Cow(_Lvm2): + lvs_dict = self.get_lvs_json_report(lvs_all=True) + for lv_dict in lvs_dict[LVS_REPORT][0][LVS_LV]: + if filter_cow_snapshot(lv_dict): ++ # Initialise pool counters to 0 ++ origin = lv_dict[LVS_LV_ORIGIN] ++ vg_name = lv_dict[LVS_VG_NAME] ++ self.origins[path_join(DEV_PREFIX, vg_name, origin)] = 0 + try: + lv_name = lv_dict[LVS_LV_NAME] + if lv_name.startswith(LVS_LV_HIDDEN_START): + lv_name = lv_name[1:-1] +- fields = parse_snapshot_name(lv_name, lv_dict[LVS_LV_ORIGIN]) ++ fields = parse_snapshot_name(lv_name, origin) + except ValueError: + continue + if fields is not None: + (snapset, timestamp, mount_point) = fields +- full_name = f"{lv_dict[LVS_VG_NAME]}/{lv_name}" ++ full_name = f"{vg_name}/{lv_name}" + self._log_debug("Found %s snapshot: %s", self.name, full_name) + snapshots.append( + Lvm2CowSnapshot( + full_name, + snapset, +- f"{lv_dict[LVS_LV_ORIGIN]}", ++ f"{origin}", + timestamp, + mount_point, + self, +- f"{lv_dict[LVS_VG_NAME]}", ++ f"{vg_name}", + f"{lv_name}", + lv_dict=lv_dict, + ) + ) + + for snapshot in snapshots: +- if snapshot.origin not in self.origins: +- self.origins[snapshot.origin] = 1 +- else: +- self.origins[snapshot.origin] += 1 ++ self.origins[snapshot.origin] += 1 + + if self.limits.snapshots_per_origin > 0: + for origin, count in self.origins.items(): +@@ -1289,10 +1290,11 @@ class Lvm2Cow(_Lvm2): + f"{LVCREATE_CMD} failed with: {_decode_stderr(err)}" + ) from err + +- if origin not in self.origins: +- self.origins[origin] = 1 ++ origin_path = path_join(DEV_PREFIX, origin) ++ if origin_path not in self.origins: ++ self.origins[origin_path] = 1 + else: +- self.origins[origin] += 1 ++ self.origins[origin_path] += 1 + + return Lvm2CowSnapshot( + f"{vg_name}/{snapshot_name}", +@@ -1305,6 +1307,24 @@ class Lvm2Cow(_Lvm2): + snapshot_name, + ) + ++ def delete_snapshot(self, name): ++ """ ++ Delete the CoW snapshot named ``name`` ++ ++ :param name: The name of the snapshot to be removed. ++ """ ++ lvs_dict = self.get_lvs_json_report(vg_lv=name) ++ lv_dict = lvs_dict[LVS_REPORT][0][LVS_LV][0] ++ origin = path_join(DEV_PREFIX, lv_dict[LVS_VG_NAME], lv_dict[LVS_LV_ORIGIN]) ++ ++ self._delete_snapshot(name) ++ ++ if origin in self.origins: ++ self.origins[origin] -= 1 ++ else: ++ # This is harmless but indicates a bug (plugin methods called out-of-sequence). ++ self._log_warn("Deleted untracked snapshot set member: %s", name) ++ + def check_resize_snapshot(self, name, origin, mount_point, size_policy): + vg_name, lv_name = vg_lv_from_origin(origin) + if vg_name not in self.size_map: +@@ -1389,6 +1409,11 @@ class Lvm2Thin(_Lvm2): + lvs_dict = self.get_lvs_json_report(lvs_all=True) + for lv_dict in lvs_dict[LVS_REPORT][0][LVS_LV]: + if filter_thin_snapshot(lv_dict): ++ # Initialise pool counters to 0 ++ vg_name = lv_dict[LVS_VG_NAME] ++ pool = lv_dict[LVS_POOL_LV] ++ pool_key = f"{vg_name}/{pool}" ++ self.pools[pool_key] = 0 + try: + lv_name = lv_dict[LVS_LV_NAME] + if lv_name.startswith(LVS_LV_HIDDEN_START): +@@ -1397,7 +1422,7 @@ class Lvm2Thin(_Lvm2): + except ValueError: + continue + if fields is not None: +- full_name = f"{lv_dict[LVS_VG_NAME]}/{lv_name}" ++ full_name = f"{vg_name}/{lv_name}" + self._log_debug("Found %s snapshot: %s", self.name, full_name) + (snapset, timestamp, mount_point) = fields + snapshots.append( +@@ -1408,17 +1433,15 @@ class Lvm2Thin(_Lvm2): + timestamp, + mount_point, + self, +- f"{lv_dict[LVS_VG_NAME]}", ++ f"{vg_name}", + f"{lv_name}", + lv_dict=lv_dict, + ) + ) + + for snapshot in snapshots: +- if snapshot.pool not in self.pools: +- self.pools[snapshot.pool] = 1 +- else: +- self.pools[snapshot.pool] += 1 ++ pool_key = f"{snapshot.vg_name}/{snapshot.pool}" ++ self.pools[pool_key] += 1 + + if self.limits.snapshots_per_pool > 0: + for pool, count in self.pools.items(): +@@ -1561,10 +1584,11 @@ class Lvm2Thin(_Lvm2): + f"{LVCREATE_CMD} failed with: {_decode_stderr(err)}" + ) from err + +- if pool_name not in self.pools: +- self.pools[pool_name] = 1 ++ pool_key = f"{vg_name}/{pool_name}" ++ if pool_key not in self.pools: ++ self.pools[pool_key] = 1 + else: +- self.pools[pool_name] += 1 ++ self.pools[pool_key] += 1 + + return Lvm2ThinSnapshot( + f"{vg_name}/{snapshot_name}", +@@ -1577,6 +1601,25 @@ class Lvm2Thin(_Lvm2): + snapshot_name, + ) + ++ def delete_snapshot(self, name): ++ """ ++ Delete the thin snapshot named ``name`` ++ ++ :param name: The name of the snapshot to be removed. ++ """ ++ lvs_dict = self.get_lvs_json_report(vg_lv=name) ++ lv_dict = lvs_dict[LVS_REPORT][0][LVS_LV][0] ++ vg_name = lv_dict[LVS_VG_NAME] ++ pool = lv_dict[LVS_POOL_LV] ++ self._delete_snapshot(name) ++ ++ pool_key = f"{vg_name}/{pool}" ++ if pool_key in self.pools: ++ self.pools[pool_key] -= 1 ++ else: ++ # This is harmless but indicates a bug (plugin methods called out-of-sequence). ++ self._log_warn("Deleted untracked snapshot set member: %s", name) ++ + def check_resize_snapshot(self, name, origin, mount_point, size_policy): + vg_name, lv_name = vg_lv_from_origin(origin) + pool_name = self.pool_name_from_vg_lv(origin) +diff --git a/tests/test_lvm2.py b/tests/test_lvm2.py +index 42e4f7c1..9d763473 100644 +--- a/tests/test_lvm2.py ++++ b/tests/test_lvm2.py +@@ -17,9 +17,12 @@ log = logging.getLogger() + import snapm.manager.plugins.lvm2 as lvm2 + from snapm import SnapmCalloutError + ++from tests import have_root ++from ._util import LvmLoopBacked + +-class Lvm2Tests(unittest.TestCase): +- """Test lvm2 plugin functions""" ++ ++class Lvm2TestsSimple(unittest.TestCase): ++ """Unit test lvm2 plugin functions with mock tool output""" + + _old_path = None + +@@ -197,3 +200,70 @@ class Lvm2Tests(unittest.TestCase): + snapshots = lvm2thin.discover_snapshots() + # FIXME: hardcoded value based on test data + self.assertEqual(len(snapshots), 5) ++ ++ ++@unittest.skipIf(not have_root(), "requires root privileges") ++class Lvm2Tests(unittest.TestCase): ++ """ ++ Test command interfaces with devices ++ """ ++ ++ volumes = ["root"] ++ thin_volumes = ["opt"] ++ ++ def setUp(self): ++ log.debug("Preparing %s", self._testMethodName) ++ def cleanup_lvm(): ++ log.debug("Cleaning up LVM (%s)", self._testMethodName) ++ if hasattr(self, "_lvm"): ++ self._lvm.destroy() ++ ++ self.addCleanup(cleanup_lvm) ++ ++ self._lvm = LvmLoopBacked(self.volumes, thin_volumes=self.thin_volumes) ++ ++ def test_lvm2cow_create_delete_origin_accounting(self): ++ # Verify that origin snapshot counters are properly updated around ++ # create/delete operations. ++ ++ # Plugin setup ++ lvm2cow_plugin = lvm2.Lvm2Cow(log, ConfigParser()) ++ snapshots = lvm2cow_plugin.discover_snapshots() ++ ++ # Create snapshot via Plugin.create_snapshot() ++ lvm2cow_plugin.start_transaction() ++ lvm2cow_plugin.check_create_snapshot("test_vg0/root", "test", 1721136677, "/", "1%SIZE") ++ lvm2cow_plugin.create_snapshot("test_vg0/root", "test", 1721136677, "/", "1%SIZE") ++ lvm2cow_plugin.end_transaction() ++ ++ # Verify counter set to one ++ origin_count = lvm2cow_plugin.origins["/dev/test_vg0/root"] ++ self.assertEqual(origin_count, 1) ++ ++ # Delete snapshot & verify decrement ++ lvm2cow_plugin.delete_snapshot("test_vg0/root-snapset_test_1721136677_-") ++ origin_count = lvm2cow_plugin.origins["/dev/test_vg0/root"] ++ self.assertEqual(origin_count, 0) ++ ++ def test_lvm2thin_create_delete_pool_accounting(self): ++ # Verify that pool snapshot counters are properly updated around ++ # create/delete operations. ++ ++ # Plugin setup ++ lvm2thin_plugin = lvm2.Lvm2Thin(log, ConfigParser()) ++ snapshots = lvm2thin_plugin.discover_snapshots() ++ ++ # Create snapshot via Plugin.create_snapshot() ++ lvm2thin_plugin.start_transaction() ++ lvm2thin_plugin.check_create_snapshot("test_vg0/opt", "test", 1721136677, "/opt", "1%SIZE") ++ lvm2thin_plugin.create_snapshot("test_vg0/opt", "test", 1721136677, "/opt", "1%SIZE") ++ lvm2thin_plugin.end_transaction() ++ ++ # Verify counter incremented ++ pool0_count = lvm2thin_plugin.pools["test_vg0/pool0"] ++ self.assertEqual(pool0_count, 1) ++ ++ # Delete snapshot & verify decrement ++ lvm2thin_plugin.delete_snapshot("test_vg0/opt-snapset_test_1721136677_-opt") ++ pool0_count = lvm2thin_plugin.pools["test_vg0/pool0"] ++ self.assertEqual(pool0_count, 0) +-- +2.53.0 + diff --git a/snapm.spec b/snapm.spec index 293909b..8aeb3b5 100644 --- a/snapm.spec +++ b/snapm.spec @@ -12,6 +12,8 @@ Patch0: 0001-tests-skip-a-handful-of-tests-in-RH-CI-environments-.patch Patch1: 0002-fsdiff-warn-user-when-no-s-start-path-Options.from_p.patch Patch2: 0003-doc-clarify-default-diff-start-path-behaviour-in-sna.patch Patch3: 0004-doc-add-discussion-of-s-start-path-to-user_guide.rst.patch +Patch4: 0005-stratis-fix-pool-limit-accounting-in-create-delete-_.patch +Patch5: 0006-lvm2-fix-pool-origin-limit-accounting.patch BuildArch: noarch From bdc9484c71d1db23d2b949078e6b719bd0fec773 Mon Sep 17 00:00:00 2001 From: "Bryn M. Reeves" Date: Tue, 12 May 2026 19:29:35 +0100 Subject: [PATCH 3/3] Adapt to stratisd D-Bus property changes Resolves: RHEL-174483 Signed-off-by: Bryn M. Reeves --- ...busClientMissingPropertyError-on-D-B.patch | 184 ++++++++++++++++++ ...isd-test-dependency-to-stratisd-v3.8.patch | 34 ++++ ...-filter_stratis_snapshot-to-allow-lo.patch | 96 +++++++++ ...busClientMissingPropertyError-on-.se.patch | 52 +++++ snapm.spec | 4 + 5 files changed, 370 insertions(+) create mode 100644 0007-stratis-handle-DbusClientMissingPropertyError-on-D-B.patch create mode 100644 0008-tests-bump-stratisd-test-dependency-to-stratisd-v3.8.patch create mode 100644 0009-stratis-refactor-filter_stratis_snapshot-to-allow-lo.patch create mode 100644 0010-stratis-handle-DbusClientMissingPropertyError-on-.se.patch diff --git a/0007-stratis-handle-DbusClientMissingPropertyError-on-D-B.patch b/0007-stratis-handle-DbusClientMissingPropertyError-on-D-B.patch new file mode 100644 index 0000000..70a0899 --- /dev/null +++ b/0007-stratis-handle-DbusClientMissingPropertyError-on-D-B.patch @@ -0,0 +1,184 @@ +From b24b0452cbce7e991b37b83a228bcae7c6f4266e Mon Sep 17 00:00:00 2001 +From: "Bryn M. Reeves" +Date: Sun, 12 Apr 2026 22:11:49 +0100 +Subject: [PATCH 07/10] stratis: handle DbusClientMissingPropertyError on D-Bus + properties + +Resolves: #983 + +Signed-off-by: Bryn M. Reeves +--- + snapm/manager/plugins/stratis.py | 90 +++++++++++++++++++++++++++----- + 1 file changed, 76 insertions(+), 14 deletions(-) + +diff --git a/snapm/manager/plugins/stratis.py b/snapm/manager/plugins/stratis.py +index 1321a897..f408af81 100644 +--- a/snapm/manager/plugins/stratis.py ++++ b/snapm/manager/plugins/stratis.py +@@ -17,6 +17,10 @@ from uuid import UUID + + from dbus.exceptions import DBusException + ++from dbus_client_gen import ( ++ DbusClientMissingPropertyError, ++) ++ + from dbus_python_client_gen import ( + DPClientInvocationError, + DPClientSetPropertyContext, +@@ -242,20 +246,47 @@ class StratisSnapshot(Snapshot): + @property + def status(self): + (_, filesystem) = self._get_dbus_cache() +- if filesystem.MergeScheduled() is True: +- return SnapStatus.REVERTING ++ try: ++ if filesystem.MergeScheduled() is True: ++ return SnapStatus.REVERTING ++ except DbusClientMissingPropertyError: # pragma: no cover ++ # pylint: disable=protected-access ++ self.provider._log_warn( ++ "Could not access filesystem.MergeScheduled() property" ++ ) + return SnapStatus.ACTIVE + + @property + def size(self): + (_, filesystem) = self._get_dbus_cache() +- return int(filesystem.Size()) ++ try: ++ return int(filesystem.Size()) ++ except DbusClientMissingPropertyError: # pragma: no cover ++ # pylint: disable=protected-access ++ self.provider._log_warn("Could not access filesystem.Size() property") ++ return 0 + + @property + def free(self): + (pool, _) = self._get_dbus_cache() +- size = int(pool.TotalPhysicalSize()) +- used = int(pool.TotalPhysicalUsed()[1]) if pool.TotalPhysicalUsed()[0] else 0 ++ try: ++ size = int(pool.TotalPhysicalSize()) ++ except DbusClientMissingPropertyError: # pragma: no cover ++ # pylint: disable=protected-access ++ self.provider._log_warn( ++ "Could not access pool.TotalPhysicalSize() property" ++ ) ++ return 0 ++ try: ++ used = ( ++ int(pool.TotalPhysicalUsed()[1]) if pool.TotalPhysicalUsed()[0] else 0 ++ ) ++ except DbusClientMissingPropertyError: # pragma: no cover ++ # pylint: disable=protected-access ++ self.provider._log_warn( ++ "Could not access pool.TotalPhysicalUsed() property" ++ ) ++ return 0 + return size - used + + # Pylint does not understand the decorator notation. +@@ -301,7 +332,10 @@ def filter_stratis_snapshot(filesystem): + snapshot or ``False`` otherwise. The ``filesystem`` argument must be a + DBus managed object corresponding to a Stratis filesystem. + """ +- return filesystem.Origin()[0] ++ try: ++ return filesystem.Origin()[0] ++ except DbusClientMissingPropertyError: ++ return False + + + def _snapshot_min_size(policy_size): +@@ -417,8 +451,13 @@ def _pool_free_space_bytes(managed_objects, pool_name): + ``pool_name``. + """ + (pool, _) = _get_pool_filesystem(managed_objects, pool_name, None) +- size = int(pool.TotalPhysicalSize()) +- used = int(pool.TotalPhysicalUsed()[1]) if pool.TotalPhysicalUsed()[0] else 0 ++ try: ++ size = int(pool.TotalPhysicalSize()) ++ used = int(pool.TotalPhysicalUsed()[1]) if pool.TotalPhysicalUsed()[0] else 0 ++ except DbusClientMissingPropertyError as err: # pragma: no cover ++ raise SnapmPluginError( ++ f"Could not access Stratis pool capacity properties for {pool_name}" ++ ) from err + return size - used + + +@@ -427,7 +466,12 @@ def _fs_size_bytes(managed_objects, pool_name, fs_name): + Return the size of the specified filesystem in bytes. + """ + (_, filesystem) = _get_pool_filesystem(managed_objects, pool_name, fs_name) +- return int(filesystem.Size()) ++ try: ++ return int(filesystem.Size()) ++ except DbusClientMissingPropertyError as err: # pragma: no cover ++ raise SnapmPluginError( ++ f"Could not access Stratis filesystem size for {pool_name}/{fs_name}" ++ ) from err + + + class Stratis(Plugin): +@@ -505,11 +549,19 @@ class Stratis(Plugin): + if not filter_stratis_snapshot(filesystem): + continue + +- pool_name = path_to_name[filesystem.Pool()] +- filesystem_name = str(filesystem.Name()) ++ try: ++ pool_object_path = filesystem.Pool() ++ origin_info = filesystem.Origin() ++ filesystem_name = str(filesystem.Name()) ++ except DbusClientMissingPropertyError as err: # pragma: no cover ++ self._log_warn( ++ "Skipping filesystem with missing D-Bus property: %s", err ++ ) ++ continue ++ pool_name = path_to_name[pool_object_path] + + origin = _origin_uuid_to_fs_name( +- managed_objects, filesystem.Pool(), str(filesystem.Origin()[1]) ++ managed_objects, pool_object_path, str(origin_info[1]) + ) + + try: +@@ -520,7 +572,6 @@ class Stratis(Plugin): + (snapset, timestamp, mount_point) = fields + full_name = f"{pool_name}/{filesystem_name}" + self._log_debug("Found %s snapshot: %s", self.name, full_name) +- pool_object_path = filesystem.Pool() + cache_pool = MOPool(managed_objects[pool_object_path]) + cache_filesystem = filesystem + snapshots.append( +@@ -989,7 +1040,13 @@ class Stratis(Plugin): + Filesystem.Properties.MergeScheduled.Set(get_object(fs_object_path), True) + except DPClientInvocationError as err: + if isinstance(err.context, DPClientSetPropertyContext): +- origin_uuid = filesystem.Origin()[1] ++ try: ++ origin_uuid = filesystem.Origin()[1] ++ except DbusClientMissingPropertyError: # pragma: no cover ++ self._log_warn("Could not access filesystem.Origin() property") ++ raise SnapmPluginError( ++ f"Unexpected D-Bus error setting property for {pool_name}/{fs_name}" ++ ) from err + if len( + _find_in_progress_merge( + managed_objects, pool_object_path, origin_uuid +@@ -1005,6 +1062,11 @@ class Stratis(Plugin): + raise SnapmPluginError( + f"Unexpected D-Bus error setting property for {pool_name}/{fs_name}" + ) from err ++ except DbusClientMissingPropertyError as err: # pragma: no cover ++ self._log_warn("Could not access filesystem.MergeScheduled property") ++ raise SnapmPluginError( ++ "Unexpected error accessing filesystem.MergeScheduled property" ++ ) from err + + def activate_snapshot(self, name): + """ +-- +2.53.0 + diff --git a/0008-tests-bump-stratisd-test-dependency-to-stratisd-v3.8.patch b/0008-tests-bump-stratisd-test-dependency-to-stratisd-v3.8.patch new file mode 100644 index 0000000..9665c27 --- /dev/null +++ b/0008-tests-bump-stratisd-test-dependency-to-stratisd-v3.8.patch @@ -0,0 +1,34 @@ +From 2eefbb115ac839d0a64ddc1e5c5f40cd7b556164 Mon Sep 17 00:00:00 2001 +From: "Bryn M. Reeves" +Date: Sun, 12 Apr 2026 22:25:36 +0100 +Subject: [PATCH 08/10] tests: bump stratisd test dependency to stratisd-v3.8.6 + +Resolves: #985 + +Signed-off-by: Bryn M. Reeves +--- + .github/workflows/snapm.yml | 4 ++-- + 1 file changed, 2 insertions(+), 2 deletions(-) + +diff --git a/.github/workflows/snapm.yml b/.github/workflows/snapm.yml +index d425a0b8..e3481701 100644 +--- a/.github/workflows/snapm.yml ++++ b/.github/workflows/snapm.yml +@@ -1,4 +1,4 @@ +---- ++-- + name: Snapshot Manager CI + permissions: + contents: read +@@ -113,7 +113,7 @@ jobs: + with: + toolchain: 1.85.0 # CURRENT DEVELOPMENT RUST TOOLCHAIN + - name: Check out stratisd +- run: git clone --depth 1 -b stratisd-v3.8.5 https://github.com/stratis-storage/stratisd ++ run: git clone --depth 1 -b stratisd-v3.8.6 https://github.com/stratis-storage/stratisd + working-directory: /var/tmp + - name: Build stratisd + run: make build-all +-- +2.53.0 + diff --git a/0009-stratis-refactor-filter_stratis_snapshot-to-allow-lo.patch b/0009-stratis-refactor-filter_stratis_snapshot-to-allow-lo.patch new file mode 100644 index 0000000..95e14bc --- /dev/null +++ b/0009-stratis-refactor-filter_stratis_snapshot-to-allow-lo.patch @@ -0,0 +1,96 @@ +From 92497a8874a1a5578564425240371609c940991b Mon Sep 17 00:00:00 2001 +From: "Bryn M. Reeves" +Date: Mon, 13 Apr 2026 16:22:16 +0100 +Subject: [PATCH 09/10] stratis: refactor filter_stratis_snapshot() to allow + logging + +Resolves: #987 + +Signed-off-by: Bryn M. Reeves +--- + snapm/manager/plugins/stratis.py | 31 ++++++++++++++++--------------- + tests/test_stratis.py | 8 ++++++-- + 2 files changed, 22 insertions(+), 17 deletions(-) + +diff --git a/snapm/manager/plugins/stratis.py b/snapm/manager/plugins/stratis.py +index f408af81..7000ed1b 100644 +--- a/snapm/manager/plugins/stratis.py ++++ b/snapm/manager/plugins/stratis.py +@@ -324,20 +324,6 @@ class StratisSnapshot(Snapshot): + return (self._pool, self._filesystem) + + +-def filter_stratis_snapshot(filesystem): +- """ +- Filter Stratis snapshots. +- +- Return ``True`` if the filesystem epresented by ``filesystem`` is a stratis +- snapshot or ``False`` otherwise. The ``filesystem`` argument must be a +- DBus managed object corresponding to a Stratis filesystem. +- """ +- try: +- return filesystem.Origin()[0] +- except DbusClientMissingPropertyError: +- return False +- +- + def _snapshot_min_size(policy_size): + """ + Return the minimum snapshot size given the space used by the snapshot +@@ -510,6 +496,21 @@ class Stratis(Plugin): + if self.priority == PLUGIN_NO_PRIORITY: + self.priority = STRATIS_STATIC_PRIORITY + ++ def _filter_stratis_snapshot(self, filesystem): ++ """ ++ Filter Stratis snapshots. ++ ++ Return ``True`` if the filesystem epresented by ``filesystem`` is a stratis ++ snapshot or ``False`` otherwise. The ``filesystem`` argument must be a ++ DBus managed object corresponding to a Stratis filesystem. ++ """ ++ try: ++ return filesystem.Origin()[0] ++ except DbusClientMissingPropertyError: # pragma: no cover ++ self._log_warn("Could not access filesystem.Origin() property") ++ return False ++ return False ++ + # pylint: disable=too-many-locals,too-many-branches + def discover_snapshots(self): + """ +@@ -546,7 +547,7 @@ class Stratis(Plugin): + ] + + for filesystem in filesystems_with_props: +- if not filter_stratis_snapshot(filesystem): ++ if not self._filter_stratis_snapshot(filesystem): + continue + + try: +diff --git a/tests/test_stratis.py b/tests/test_stratis.py +index 89e80f7c..16a6a636 100644 +--- a/tests/test_stratis.py ++++ b/tests/test_stratis.py +@@ -240,12 +240,16 @@ class StratisTests(unittest.TestCase): + proxy = get_object(TOP_OBJECT) + managed_objects = ObjectManager.Methods.GetManagedObjects(proxy, {}) + ++ stratis_plugin = stratis.Stratis(log, ConfigParser()) ++ + (pool, fs) = stratis._get_pool_filesystem(managed_objects, "pool1", "fs1-snapset_test_1721136677_-opt") +- self.assertEqual(True, stratis.filter_stratis_snapshot(fs)) ++ self.assertEqual(True, stratis_plugin._filter_stratis_snapshot(fs)) + + def test_filter_stratis_snapshot_nonsnapshot(self): + proxy = get_object(TOP_OBJECT) + managed_objects = ObjectManager.Methods.GetManagedObjects(proxy, {}) + ++ stratis_plugin = stratis.Stratis(log, ConfigParser()) ++ + (pool, fs) = stratis._get_pool_filesystem(managed_objects, "pool1", "fs1") +- self.assertEqual(False, stratis.filter_stratis_snapshot(fs)) ++ self.assertEqual(False, stratis_plugin._filter_stratis_snapshot(fs)) +-- +2.53.0 + diff --git a/0010-stratis-handle-DbusClientMissingPropertyError-on-.se.patch b/0010-stratis-handle-DbusClientMissingPropertyError-on-.se.patch new file mode 100644 index 0000000..a486b0b --- /dev/null +++ b/0010-stratis-handle-DbusClientMissingPropertyError-on-.se.patch @@ -0,0 +1,52 @@ +From 5347e0e45dce63ccad2b8ca3eddcafff0e9ba49b Mon Sep 17 00:00:00 2001 +From: "Bryn M. Reeves" +Date: Mon, 13 Apr 2026 16:36:08 +0100 +Subject: [PATCH 10/10] stratis: handle DbusClientMissingPropertyError on + .search() invocation + +Resolves: #984 + +Signed-off-by: Bryn M. Reeves +--- + snapm/manager/plugins/stratis.py | 18 ++++++++++++++---- + 1 file changed, 14 insertions(+), 4 deletions(-) + +diff --git a/snapm/manager/plugins/stratis.py b/snapm/manager/plugins/stratis.py +index 7000ed1b..a8be5500 100644 +--- a/snapm/manager/plugins/stratis.py ++++ b/snapm/manager/plugins/stratis.py +@@ -19,6 +19,7 @@ from dbus.exceptions import DBusException + + from dbus_client_gen import ( + DbusClientMissingPropertyError, ++ DbusClientMissingSearchPropertiesError, + ) + + from dbus_python_client_gen import ( +@@ -425,10 +426,19 @@ def _find_in_progress_merge(managed_objects, pool_object_path, origin_uuid): + "MergeScheduled": True, + } + +- return [ +- MOFilesystem(info) +- for objpath, info in filesystems(props=fs_props).search(managed_objects) +- ] ++ try: ++ return [ ++ MOFilesystem(info) ++ for objpath, info in filesystems(props=fs_props).search(managed_objects) ++ ] ++ # Outside a bug in stratisd this is exceedingly unlikely, but handle it ++ # by returning an empty list if we are unable to access the Origin or ++ # MergeScheduled properties for any reason. In the worst case we will end ++ # up returning a SnapmPluginError with a D-Bus error in the case that we ++ # fail to detect an in-progress merge and then on that basis attempt to ++ # start a revert operation on the same device. ++ except DbusClientMissingSearchPropertiesError: # pragma: no cover ++ return [] + + + def _pool_free_space_bytes(managed_objects, pool_name): +-- +2.53.0 + diff --git a/snapm.spec b/snapm.spec index 8aeb3b5..11849b8 100644 --- a/snapm.spec +++ b/snapm.spec @@ -14,6 +14,10 @@ Patch2: 0003-doc-clarify-default-diff-start-path-behaviour-in-sna.patch Patch3: 0004-doc-add-discussion-of-s-start-path-to-user_guide.rst.patch Patch4: 0005-stratis-fix-pool-limit-accounting-in-create-delete-_.patch Patch5: 0006-lvm2-fix-pool-origin-limit-accounting.patch +Patch6: 0007-stratis-handle-DbusClientMissingPropertyError-on-D-B.patch +Patch7: 0008-tests-bump-stratisd-test-dependency-to-stratisd-v3.8.patch +Patch8: 0009-stratis-refactor-filter_stratis_snapshot-to-allow-lo.patch +Patch9: 0010-stratis-handle-DbusClientMissingPropertyError-on-.se.patch BuildArch: noarch