diff --git a/CHANGELOG.md b/CHANGELOG.md index d1287d2..417903f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,54 @@ is deliberate: see [Releasing](docs/releasing.md). Twelve releases were cut on live, every one of those interrupts every user. An alert people learn to ignore is worse than no alert, because one day it carries a security fix. +## Unreleased +### Added + +- **TrueNAS 26 support, verified on a real TrueNAS 26 install.** 26 deletes + `plugins/zfs_/` outright, taking the private `zfs.dataset.query`, + `zfs.snapshot.query` and `zfs.snapshot.delete` with it. Every one of those was on + the nested module's critical path, so nested snapshots were **BROKEN** on 26 and + `apply.sh` correctly refused to apply the module there. + + Snapshot **deletion** now resolves its namespace at runtime — `pool.snapshot` on + 25.10 and 26, `zfs.snapshot` on 24.10 and 25.04, because no single namespace spans + every supported release. `tools/compat.py` checks the same list the runtime uses, + so what CI verifies and what runs cannot drift apart. + + Hardware-verified on TrueNAS 26.0.0-BETA.1: a 274-snapshot recursive backup of a + 292-dataset pool, then a **byte-identical restore of a four-level-deep child + dataset**. + +### Fixed + +- **Enumeration no longer trusts middleware's dataset and snapshot queries — they + are filtered.** This is the important one, and it is the bug that a test VM caught + and no amount of source analysis ever could have. + + The obvious port of the deleted private `zfs.dataset.query` was the public + `pool.dataset.query`. It exists, it is documented, it is covered by iX's + deprecation policy — and it is **not a like-for-like replacement**. It applies a + *visibility policy*: it hides the datasets TrueNAS considers its own — `ix-apps/*`, + `.system/*`, `.ix-virt/*`. On a real pool that is **84 of 270 datasets**, and + `ix-apps` holds **live application data**. + + Staging from that view would have silently omitted every one of them. Worse, + `plan_staging()` would never have seen them, so they would not have appeared in its + `skipped` list either — no warning, no failure, just a green backup quietly missing + data. That is precisely the failure this module exists to prevent. The snapshot + query lies the same way (205 of 274), so the sweep would have orphaned one snapshot + per hidden dataset, on every run, forever. + + The module now **reads the truth from ZFS and makes changes through middleware**: + enumeration is `zfs list`, which no policy can filter and which behaves identically + on every release; mutation stays a middleware call, so TrueNAS's own bookkeeping + stays consistent. A failing `zfs list` raises rather than returning an empty list — + "no datasets" and "the command broke" must never look the same. + + **No shipped release is affected.** v0.6.1 and earlier call the *private* + `zfs.dataset.query`, which returns all 270 datasets. The bug existed only in the + unreleased TrueNAS 26 port. + ## v0.6.1 — 2026-07-13 ### Fixed diff --git a/README.md b/README.md index 03e6934..a991ab9 100644 --- a/README.md +++ b/README.md @@ -60,7 +60,7 @@ If something is wrong, the reason is in `apply.log` — start at | 24.10.2.4 | ok | ok | — | | 25.04.2.6 | ok | ok | — | | 25.10.4 | ok | ok | nested + providers; 252-snapshot recursive backup of /mnt/Tap, 18m | -| 26.0.0-BETA.3 _(unreleased)_ | ok | **BROKEN** | — | +| 26.0.0-BETA.3 _(unreleased)_ | ok | ok | — | | master _(unreleased)_ | **BROKEN** | **BROKEN** | — | | verdict | meaning | @@ -77,6 +77,12 @@ source. It does not mean a human ran a backup on it — that is the The table is **regenerated daily by CI** against iXsystems' actual middleware source — it is not a claim somebody typed once and forgot. +**On TrueNAS 26:** the patch was run on a real TrueNAS **26.0.0-BETA.1** install — a +274-snapshot recursive backup of a 292-dataset pool, followed by a byte-identical +restore of a four-level-deep child dataset. The *Hardware-verified* column tracks the +newest beta iX has tagged (currently BETA.3), so it does not carry that mark: a build +nobody has actually run a backup on does not get credit for one. + **TrueNAS 26: nested snapshots are not supported yet, and upgrading will not break you.** 26 rewrites `cloud_backup` and deletes the ZFS methods this module calls. On 26 `apply.sh` finds that the assumptions no longer hold and **does not apply the diff --git a/patch/apply.sh b/patch/apply.sh index 2723ccd..d1774b5 100755 --- a/patch/apply.sh +++ b/patch/apply.sh @@ -535,7 +535,7 @@ if _tc_nested is not None: # snapshot=true, not just ours. Two consequences, and the second is worse: # # * everything below is a NEW failure mode for tasks that worked before we - # were installed. A `zfs.dataset.query` that errors would break a + # were installed. A `pool.dataset.query` that errors would break a # CloudSync job we have no business touching. # * if a CloudSync task ever were staged, nothing would ever tear it down: # the teardown is wired into cloud_backup's restic_backup finally, and @@ -556,9 +556,12 @@ if _tc_nested is not None: # our staging plan would not -- silently omitting it from the backup. # Read afterwards, an unsnapshotted dataset instead trips the isdir() # check in plan_staging and fails the run loudly. Loud beats silent. - datasets = middleware.call_sync( - "zfs.dataset.query", [["type", "=", "FILESYSTEM"]] - ) + # query_filesystems() reads ZFS directly. It deliberately does NOT use + # pool.dataset.query: that applies a visibility policy and hides + # TrueNAS-internal datasets (ix-apps/*, .system/*, .ix-virt/*) -- 84 of + # 270 on a real pool, including live app data. Staging from the filtered + # view omits them silently, which is the one thing this must never do. + datasets = _tc_nested.query_filesystems(middleware) # OUR copy of get_dataset_recursive, not the host module's: TrueNAS 26 # deleted that helper (create_snapshot uses filesystem.statfs now), so # calling it out of the module namespace is a NameError there. diff --git a/patch/truecloud_nested.py b/patch/truecloud_nested.py index 1d867ea..17c4c47 100644 --- a/patch/truecloud_nested.py +++ b/patch/truecloud_nested.py @@ -34,7 +34,7 @@ feature exists to prevent, and it would be worse than not having the feature. Snapshot lifecycle -- read this before changing anything -------------------------------------------------------- -``zfs.snapshot.delete`` defaults to ``recursive=False``, and stock +The snapshot delete call defaults to ``recursive=False``, and stock ``restic_backup()`` calls it with no options. Stock gets away with that because its validation means ``recursive`` is never actually True in the field. Enabling nested datasets makes recursive snapshots real, so the parent @@ -62,17 +62,24 @@ import subprocess import time __all__ = [ + "SNAPSHOT_SERVICES", "STAGING_BASE", "StagingError", + "ZfsError", "apply_plan", "cleanup_all", "cleanup_task", "current_mounts_under", "delete_snapshot_tree", "gc_stale_snapshots", + "list_snapshot_names", "mounted_snapshots", + "normalise_dataset", + "pick_snapshot_service", "plan_staging", + "query_filesystems", "sidecar_for", + "snapshot_service", "snapshot_tree_names", "stage_nested", "stale_snapshot_names", @@ -81,6 +88,204 @@ __all__ = [ "verify_staged", ] + +# ── how this module talks to the system ────────────────────────────────────── +# +# READ the truth from ZFS. MAKE CHANGES through middleware. +# +# That split is not stylistic. It was forced by finding, on a real TrueNAS 26 box, +# that middleware's query APIs apply a VISIBILITY POLICY: +# +# zfs list 274 datasets 205 from pool.dataset.query +# zfs list -t snapshot 274 snapshots 205 from pool.snapshot.query +# +# The missing 69 are the datasets TrueNAS considers its own -- `ix-apps/*`, +# `.system/*`, `.ix-virt/*` -- and on the real pool that is 84 of 270, including +# `ix-apps`, which holds live application data. Enumerating from that view would +# have silently omitted every one of them from the staging plan and from the +# snapshot sweep: a green backup missing data, and one orphaned snapshot per +# hidden dataset on every run. Both are exactly what this module exists to +# prevent. +# +# This went unnoticed because the patch used to call the PRIVATE `zfs.dataset.query` +# and `zfs.snapshot.query`, which return everything. TrueNAS 26 deleted them, and +# the public replacements are NOT like-for-like -- they are filtered. So +# enumeration now reads ZFS directly, which no policy can filter and which behaves +# identically on every release. +# +# MUTATION still goes through middleware, so TrueNAS's own bookkeeping stays +# consistent -- and an exact-name delete works fine even on a dataset the query +# hides. The one wrinkle is that no single snapshot namespace spans every +# supported release, so it is resolved at runtime rather than pinned: +# +# 24.10, 25.04 `zfs.snapshot` (public back then; `pool.snapshot` does not exist) +# 25.10 both -- `pool.snapshot` public, `zfs.snapshot` demoted to private +# 26 `pool.snapshot` only -- `plugins/zfs_/` is gone +# +#: Snapshot CRUD namespaces, best first. `tools/compat.py` checks this exact list +#: (MiddlewareCall.also), so what CI verifies and what runs cannot drift apart. +SNAPSHOT_SERVICES = ("pool.snapshot", "zfs.snapshot") + + +def pick_snapshot_service(has_service): + """First namespace in SNAPSHOT_SERVICES that this middleware exposes. + + Pure: `has_service(name) -> bool`. Returns None if middleware has none of + them, which is a middleware we have never seen and must not guess about. + """ + for name in SNAPSHOT_SERVICES: + if has_service(name): + return name + return None + + +def _has_service(middleware, name): + try: + middleware.get_service(name) + except Exception: + # get_service raises KeyError for an unregistered namespace. Anything + # else here is equally a "cannot use it", and guessing YES on a service + # that is not really there would fail later, mid-backup, holding a + # snapshot -- the worst possible moment. + return False + return True + + +def snapshot_service(middleware): + """The snapshot CRUD namespace this middleware actually has.""" + name = pick_snapshot_service(lambda n: _has_service(middleware, n)) + if name is None: + raise StagingError( + "middleware exposes neither " + " nor ".join(SNAPSHOT_SERVICES) + + ". Refusing to stage a nested backup, because the snapshot it " + "creates could not then be swept." + ) + return name + + +def normalise_dataset(row): + """A `pool.dataset.query` row -> the shape this module's planner speaks. + + The planner does not consume this any more -- :func:`query_filesystems` reads + ZFS directly, because the middleware query is filtered (see the note above). + It is kept because it is the safe way to consume a middleware dataset row if + anything ever needs to, and because the two traps below are not obvious and + cost real debugging to find: + + * `mountpoint` is a plain string there, not ``{"value": ...}``. + * `mounted` is still a property dict, but its ``value`` is ``"YES"``/``"NO"`` + -- UPPERCASE, where the old API said ``"yes"``. The planner tests + ``== "no"``, so an unmounted dataset would read as mounted and the planner + would try to stage a snapdir that is not there. Read ``parsed``, which is a + real bool, and only fall back to the string. + """ + mounted = row.get("mounted") + if isinstance(mounted, dict): + parsed = mounted.get("parsed") + if isinstance(parsed, bool): + is_mounted = parsed + else: + is_mounted = str(mounted.get("value", "yes")).lower() != "no" + elif isinstance(mounted, bool): + is_mounted = mounted + else: + # Absent means the caller did not ask for the property. Assume mounted: + # the planner's own snapdir probe is the real check, and assuming + # UNmounted would silently drop datasets that hold data. + is_mounted = True + + mountpoint = row.get("mountpoint") + if isinstance(mountpoint, dict): # tolerate the old shape too + mountpoint = mountpoint.get("value", "") + + return { + "name": row["name"], + "properties": { + "mountpoint": {"value": mountpoint or ""}, + "mounted": {"value": "yes" if is_mounted else "no"}, + }, + } + + +class ZfsError(Exception): + """`zfs list` failed. Enumeration is unreliable, so the caller must not guess.""" + + +def _zfs_lines(args, runner=None): + """`zfs ` as a list of tab-split rows. Raises ZfsError if it fails. + + Never returns a partial or empty list on failure: a caller that cannot tell + "no datasets" from "the command broke" will happily stage nothing, or sweep + nothing, and report success. + """ + runner = runner or _run + r = runner(["zfs", *args]) + if r.returncode != 0: + raise ZfsError((r.stderr or "").strip() or f"zfs {' '.join(args)} failed") + return [ln.split("\t") for ln in r.stdout.splitlines() if ln.strip()] + + +def query_filesystems(middleware=None, runner=None): + """Every FILESYSTEM dataset, in the shape the planner speaks -- read from ZFS. + + NOT from `pool.dataset.query`, and this is the single most important decision + in this file. + + middleware's dataset query applies a VISIBILITY POLICY: it hides the datasets + TrueNAS considers its own -- `ix-apps/*`, `.system/*`, `.ix-virt/*`. That is + **84 of 270 datasets** on the real pool, and `ix-apps` holds live application + data. Building the staging plan from that view would silently omit every one of + them. Worse, `plan_staging()` would never even SEE them, so they would not turn + up in its `skipped` list either -- no warning, no failure, just a green backup + quietly missing data. That is exactly the failure this whole module exists to + prevent, and it is the failure the cardinal rule at the top of this file is + about. + + It worked before only because the patch called the PRIVATE `zfs.dataset.query`, + which returned everything. TrueNAS 26 deleted it. The public replacement is not + a like-for-like: it is a filtered view. + + So: **read the truth from ZFS, make changes through middleware.** ZFS cannot + apply a policy to what it reports, and `zfs list` behaves identically on every + release -- which also means one code path instead of a version conditional. + + `middleware` is accepted and ignored, so callers need not care where the data + comes from. + """ + rows = _zfs_lines( + ["list", "-H", "-p", "-o", "name,mountpoint,mounted", "-t", "filesystem"], + runner=runner, + ) + return [ + { + "name": name, + "properties": { + "mountpoint": {"value": mountpoint}, + # `zfs list` prints yes/no; the planner already speaks that. + "mounted": {"value": mounted}, + }, + } + for name, mountpoint, mounted in (r for r in rows if len(r) == 3) + ] + + +def list_snapshot_names(dataset, runner=None): + """Every snapshot at or under `dataset` -- read from ZFS, for the same reason. + + `pool.snapshot.query` filters exactly like the dataset query does: on this box + it returned 205 of 274 snapshots, hiding the internal datasets' snapshots. A + sweep built on that view leaves one orphan per hidden dataset, on every run, + forever -- which is the bug this module was written to fix in the first place. + + (An EXACT-name delete still works on a hidden dataset, so mutations may keep + going through middleware. It is only enumeration that lies.) + """ + rows = _zfs_lines( + ["list", "-H", "-o", "name", "-t", "snapshot", "-r", dataset], + runner=runner, + ) + return [r[0] for r in rows] + #: Where staging trees are assembled. tmpfs; bind mounts consume no space. STAGING_BASE = "/run/truecloud-nested" @@ -560,7 +765,7 @@ def get_dataset_recursive(datasets, directory): def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, - sleep=time.sleep): + sleep=time.sleep, list_snapshots=None): """Delete the parent snapshot AND every child created by ``zfs snapshot -r``. Returns the snapshots it could NOT delete -- callers must not throw that away. @@ -578,6 +783,8 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, snapshots hit this, and before the fix they were orphaned permanently. """ dataset, _, snapname = snapshot.partition("@") + svc = snapshot_service(middleware) + list_snapshots = list_snapshots or list_snapshot_names # Release ZFS's own automounts first, or `zfs destroy` refuses with EBUSY on # everything restic read in the last few minutes. @@ -591,7 +798,7 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, # through 252 sequential deletes leaves exactly the orphans this function # exists to prevent. try: - middleware.call_sync("zfs.snapshot.delete", snapshot, {"recursive": True}) + middleware.call_sync(f"{svc}.delete", snapshot, {"recursive": True}) return [] except Exception as e: # noqa: BLE001 - fall through to the explicit sweep # Usually just "parent already gone" (stock's finally won the race once our @@ -608,13 +815,14 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, # our mounts are released -- which fails the recursive delete while the # children survive. Sweep them by name. try: - snaps = middleware.call_sync( - "zfs.snapshot.query", [["name", "^", dataset]], {"select": ["name"]} - ) + # From ZFS, not middleware: the snapshot query hides internal datasets' + # snapshots (205 of 274 on the test box), and a sweep that cannot see them + # orphans one per hidden dataset on every run. See list_snapshot_names(). + # # An empty result means the tree is already gone -- delete nothing, and # do not fall back to the parent, which would only log a spurious # "does not exist" warning on every clean run. - names = snapshot_tree_names(snapshot, [s["name"] for s in snaps]) + names = snapshot_tree_names(snapshot, list_snapshots(dataset)) except Exception as e: # noqa: BLE001 - fall back to at least the parent if logger: logger.warning( @@ -635,12 +843,9 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, if not failed: return [] try: - live = middleware.call_sync( - "zfs.snapshot.query", [["name", "^", dataset]], {"select": ["name"]} - ) + live = set(list_snapshots(dataset)) except Exception: # noqa: BLE001 - cannot refine; trust the delete's verdict return list(failed) - live = {s["name"] for s in live} return [n for n in failed if n in live] remaining = list(names) @@ -648,7 +853,7 @@ def delete_snapshot_tree(middleware, snapshot, logger=None, attempts=4, failed = [] for name in remaining: try: - middleware.call_sync("zfs.snapshot.delete", name) + middleware.call_sync(f"{svc}.delete", name) except Exception: # noqa: BLE001 - busy, or already gone; sorted out below failed.append(name) @@ -694,7 +899,7 @@ def mounted_snapshots(mounts_file="/proc/self/mounts"): def gc_stale_snapshots(middleware, task_name, current_snapshot, logger=None, - now=None, mounts_file="/proc/self/mounts"): + now=None, mounts_file="/proc/self/mounts", list_snapshots=None): """Delete snapshots this task left behind in an earlier run. Returns what remains. The backstop for when the RECORD is gone, not just the snapshots: the sidecar lives @@ -707,11 +912,13 @@ def gc_stale_snapshots(middleware, task_name, current_snapshot, logger=None, """ dataset = current_snapshot.partition("@")[0] now = now or datetime.datetime.now(datetime.UTC) + svc = snapshot_service(middleware) + list_snapshots = list_snapshots or list_snapshot_names try: - snaps = middleware.call_sync( - "zfs.snapshot.query", [["name", "^", dataset]], {"select": ["name"]} - ) + # From ZFS: middleware's snapshot query hides internal datasets, and an + # orphan it cannot see is an orphan nothing will ever collect. + all_names = list_snapshots(dataset) except Exception as e: # noqa: BLE001 - cannot enumerate; collect nothing if logger: logger.warning( @@ -720,7 +927,7 @@ def gc_stale_snapshots(middleware, task_name, current_snapshot, logger=None, return [] stale = stale_snapshot_names( - task_name, current_snapshot, [s["name"] for s in snaps], now, + task_name, current_snapshot, all_names, now, in_use=mounted_snapshots(mounts_file), ) if not stale: @@ -736,7 +943,7 @@ def gc_stale_snapshots(middleware, task_name, current_snapshot, logger=None, remaining = [] for name in stale: try: - middleware.call_sync("zfs.snapshot.delete", name) + middleware.call_sync(f"{svc}.delete", name) except Exception as e: # noqa: BLE001 - busy, or gone; either way, next run remaining.append(name) if logger: diff --git a/tests/test_apply_blocks.py b/tests/test_apply_blocks.py index 44f716b..d2ec7e0 100644 --- a/tests/test_apply_blocks.py +++ b/tests/test_apply_blocks.py @@ -438,8 +438,22 @@ class TestOnlyOurOwnTasksAreTouched: # The point is to add NO new failure mode to a CloudSync task. If any # middleware call happened before the bail-out, we would already have broken # the thing we are trying not to touch. + # + # Checked against whichever interactions the block ACTUALLY contains, not a + # fixed list: the dataset query moved behind `_tc_nested.query_filesystems()` + # when it switched to the public pool.* API, and a hardcoded + # `middleware.call_sync(` simply stopped being found -- a test that silently + # stops testing is worse than no test. block = extract_blocks()[name] gate = block.index('if not name.startswith("cloud_backup"):') - for call in ("middleware.call_sync(", "_tc_nested.stage_nested(", - "_tc_nested.delete_snapshot_tree("): + + interactions = [ + "middleware.call_sync(", + "_tc_nested.query_filesystems(", + "_tc_nested.stage_nested(", + "_tc_nested.delete_snapshot_tree(", + ] + present = [c for c in interactions if c in block] + assert present, "found no middleware interaction at all -- the test is vacuous" + for call in present: assert gate < block.index(call), f"{call} runs before the cloud_backup gate" diff --git a/tests/test_compat.py b/tests/test_compat.py index 4d405a5..f6ca055 100644 --- a/tests/test_compat.py +++ b/tests/test_compat.py @@ -51,24 +51,37 @@ GOOD = { "async def restic_backup(middleware, job, cloud_backup, dry_run=False, " "rate_limit=None):\n pass\n" ), - # The middlewared METHODS the injected code calls. TrueNAS 26 deleted both of - # these files, taking zfs.dataset.query / zfs.snapshot.query / zfs.snapshot.delete - # with them -- see TestMiddlewareMethodsWeCall. - "plugins/zfs_/dataset.py": ( - "class ZFSDataset(CRUDService):\n" + # The middlewared METHODS the injected code calls. These now go through the + # PUBLIC pool.* API: TrueNAS 26 deleted plugins/zfs_/ outright, taking the whole + # private zfs.* service with it -- see TestMiddlewareMethodsWeCall. + # + # This default tree is a MODERN box (25.10/26): it has pool.snapshot and no + # zfs.snapshot. The older shape is built explicitly where it is tested. + "plugins/pool_/dataset.py": ( + "class PoolDatasetService(CRUDService):\n" " class Config:\n" - " namespace = 'zfs.dataset'\n" + " namespace = 'pool.dataset'\n" " def query(self, filters, options):\n pass\n" ), - "plugins/zfs_/snapshot.py": ( - "class ZFSSnapshot(CRUDService):\n" + "plugins/pool_/snapshot.py": ( + "class PoolSnapshotService(CRUDService):\n" " class Config:\n" - " namespace = 'zfs.snapshot'\n" + " namespace = 'pool.snapshot'\n" " def query(self, filters, options):\n pass\n" " def delete(self, id_, options={}):\n pass\n" ), } +#: A 24.10/25.04 box: `pool.snapshot` does not exist yet and the snapshot CRUD +#: service still answers to the (then-public) `zfs.snapshot`. +ZFS_ERA_SNAPSHOT = ( + "class ZFSSnapshot(CRUDService):\n" + " class Config:\n" + " namespace = 'zfs.snapshot'\n" + " def query(self, filters, options):\n pass\n" + " def do_delete(self, id_, options={}):\n pass\n" +) + def loader(files): def load(path): @@ -303,52 +316,86 @@ class TestMiddlewareMethodsWeCall: per descendant dataset (250 on a real pool) on every run, forever. """ - ZFS_SNAPSHOT = ( - "class ZFSSnapshot(CRUDService):\n" - " class Config:\n" - " namespace = 'zfs.snapshot'\n" - " def query(self, filters, options):\n pass\n" - " def delete(self, id_, options={}):\n pass\n" - ) - ZFS_DATASET = ( - "class ZFSDataset(CRUDService):\n" - " class Config:\n" - " namespace = 'zfs.dataset'\n" - " def query(self, filters, options):\n pass\n" - ) + POOL_SNAPSHOT = GOOD["plugins/pool_/snapshot.py"] def _tree(self, **over): files = dict(GOOD) - files["plugins/zfs_/snapshot.py"] = self.ZFS_SNAPSHOT - files["plugins/zfs_/dataset.py"] = self.ZFS_DATASET files.update(over) return files + #: A 26 box: pool.snapshot only. + def _modern(self, **over): + return self._tree(**over) + + #: A 24.10/25.04 box: zfs.snapshot only -- plugins/pool_/snapshot.py does not + #: exist yet. + def _zfs_era(self, **over): + return self._tree(**{ + "plugins/pool_/snapshot.py": None, + "plugins/zfs_/snapshot.py": ZFS_ERA_SNAPSHOT, + **over, + }) + def test_present_methods_are_ok(self): - r = check_files(self._tree()) + r = check_files(self._modern()) assert r[NESTED]["ok"], r[NESTED]["problems"] - def test_a_deleted_plugin_file_is_broken(self): - # Literally TrueNAS 26: plugins/zfs_/snapshot.py does not exist. - r = check_files(self._tree(**{"plugins/zfs_/snapshot.py": None})) + def test_the_OLD_zfs_era_snapshot_service_also_satisfies_the_call(self): + # 24.10 and 25.04 have no `pool.snapshot` at all -- the CRUD service is the + # then-public `zfs.snapshot`. Pinning only the modern spelling marked both of + # those releases BROKEN and would have switched nested snapshots OFF on boxes + # where they work perfectly. The runtime picks the same way; see + # pick_snapshot_service(). + r = check_files(self._zfs_era()) + assert r[NESTED]["ok"], r[NESTED]["problems"] + + def test_it_is_broken_only_when_NEITHER_namespace_exists(self): + # The real failure: middleware drops the last spelling we know how to call. + r = check_files(self._tree(**{ + "plugins/pool_/snapshot.py": None, + "plugins/zfs_/snapshot.py": None, + })) assert is_broken(r[NESTED]) details = " ".join(p["detail"] for p in r[NESTED]["problems"]) - assert "zfs.snapshot.delete" in details + assert "pool.snapshot.delete" in details + assert "zfs.snapshot.delete" in details, ( + "the report must say BOTH spellings were tried, or whoever reads it will " + "think we simply never looked for the one their box has" + ) + + def test_we_do_NOT_depend_on_a_middleware_dataset_query_at_all(self): + # iX could delete plugins/pool_/dataset.py tomorrow and the patch would not + # care, because the staging plan is enumerated from ZFS, not from middleware. + # + # That is deliberate, and it was expensive to learn. `pool.dataset.query` + # exists and is correctly shaped -- and it LIES: it applies a visibility + # policy that hides ix-apps/*, .system/* and .ix-virt/* (84 of 270 datasets + # on the real pool, including live app data). No source check could ever + # have caught that; only running it could. So there is no assumption here + # left to break. + r = check_files(self._modern(**{"plugins/pool_/dataset.py": None})) + assert r[NESTED]["ok"], r[NESTED]["problems"] + + ids = {c.id for c in compat.MIDDLEWARE_CALLS} + assert not any("dataset" in i or "query" in i for i in ids), ( + "a dataset/snapshot QUERY assumption crept back into the manifest -- " + "middleware's queries are filtered; enumerate from ZFS" + ) def test_a_renamed_namespace_is_broken(self): r = check_files(self._tree(**{ - "plugins/zfs_/snapshot.py": self.ZFS_SNAPSHOT.replace( - "'zfs.snapshot'", "'zfs.resource.snapshot'"), + "plugins/pool_/snapshot.py": self.POOL_SNAPSHOT.replace( + "'pool.snapshot'", "'zfs.resource.snapshot'"), + "plugins/zfs_/snapshot.py": None, })) assert is_broken(r[NESTED]) def test_the_CRUDService_do_prefix_is_accepted(self): - # 24.10 and 25.04 declare `do_delete`; 25.10 renamed it to `delete`. BOTH - # answer to zfs.snapshot.delete. Accepting only the literal name reported the - # two older releases as broken -- a false BROKEN that would have switched off - # nested snapshots on boxes where they work perfectly. - r = check_files(self._tree(**{ - "plugins/zfs_/snapshot.py": self.ZFS_SNAPSHOT.replace( + # A CRUDService exposes `delete` from a method NAMED `do_delete`. Both + # spellings are live across the matrix. Accepting only the literal name + # reported working releases as broken. + r = check_files(self._modern(**{ + "plugins/pool_/snapshot.py": self.POOL_SNAPSHOT.replace( "def delete(", "def do_delete("), })) assert r[NESTED]["ok"], r[NESTED]["problems"] @@ -356,6 +403,9 @@ class TestMiddlewareMethodsWeCall: def test_the_snapshot_delete_reason_names_the_orphan_risk(self): # If this ever regresses, whoever reads the bug report must understand that # it is not a cosmetic failure. - r = check_files(self._tree(**{"plugins/zfs_/snapshot.py": None})) + r = check_files(self._tree(**{ + "plugins/pool_/snapshot.py": None, + "plugins/zfs_/snapshot.py": None, + })) whys = " ".join(p["why"] for p in r[NESTED]["problems"]) assert "orphan" in whys diff --git a/tests/test_truecloud_nested.py b/tests/test_truecloud_nested.py index 585f9ff..cf79f5b 100644 --- a/tests/test_truecloud_nested.py +++ b/tests/test_truecloud_nested.py @@ -204,18 +204,52 @@ class FakeMiddleware: truecloud_nested.py. TrueNAS <= 25.10 reaches it through `await middleware.run_in_thread(...)` and TrueNAS 26 calls it directly, but the logic below the boundary is the same code either way, so it is tested once. + + `snapshot_ns` picks which middleware GENERATION this is, because they do not + agree on what the snapshot service is called: + + pool.snapshot 25.10 and 26 (26 has ONLY this -- plugins/zfs_/ is gone) + zfs.snapshot 24.10 and 25.04 (pool.snapshot does not exist yet) + + A method in the namespace this box does NOT have raises, exactly as middleware + does ("Method does not exist"). That is what makes the runtime picker testable: + a module that guessed wrong would blow up here instead of silently orphaning + snapshots on somebody's NAS. """ - def __init__(self, snapshots=None): + def __init__(self, snapshots=None, snapshot_ns="pool.snapshot"): self.snapshots = list(snapshots or []) self.calls = [] self.logger = None + self.snapshot_ns = snapshot_ns + + def get_service(self, name): + if name != self.snapshot_ns: + raise KeyError(name) # middleware raises KeyError for an unknown ns + return object() + + def list_snapshots(self, dataset): + """Stands in for `zfs list -t snapshot -r `. + + Enumeration comes from ZFS now, NOT from middleware -- middleware's query + hides internal datasets, and a sweep that cannot see a snapshot can never + collect it. Passed in as `list_snapshots=` so the seam is explicit. + """ + return [ + n for n in self.snapshots + if n.split("@")[0] == dataset or n.startswith(dataset + "/") + ] def call_sync(self, method, *args): self.calls.append((method, args)) - if method == "zfs.snapshot.query": + namespace, _, op = method.rpartition(".") + + if namespace != self.snapshot_ns: + raise RuntimeError("Method does not exist") + + if op == "query": return [{"name": n} for n in self.snapshots] - if method == "zfs.snapshot.delete": + if op == "delete": name = args[0] opts = args[1] if len(args) > 1 else {} if name not in self.snapshots: @@ -265,44 +299,44 @@ class TestDeleteSnapshotTree: mw = FakeMiddleware([ "Tap@snap", "Tap/apps@snap", "Tap/apps/lidarr@snap", "Tap@keepme", ]) - delete_snapshot_tree(mw, "Tap@snap") + delete_snapshot_tree(mw, "Tap@snap", list_snapshots=mw.list_snapshots) assert mw.snapshots == ["Tap@keepme"] def test_is_idempotent_when_stock_already_removed_the_parent(self): # Stock's finally can win the race once our mounts are released. mw = FakeMiddleware(["Tap/apps@snap", "Tap/apps/lidarr@snap"]) - delete_snapshot_tree(mw, "Tap@snap") + delete_snapshot_tree(mw, "Tap@snap", list_snapshots=mw.list_snapshots) assert mw.snapshots == [] def test_uses_a_single_recursive_delete_not_252_individual_ones(self): # 252 sequential deletes are slow AND not atomic: a run killed part-way # through leaves exactly the orphans this function exists to prevent. mw = FakeMiddleware(["Tap@snap", "Tap/apps@snap", "Tap/apps/lidarr@snap"]) - delete_snapshot_tree(mw, "Tap@snap") + delete_snapshot_tree(mw, "Tap@snap", list_snapshots=mw.list_snapshots) assert mw.snapshots == [] - deletes = [a for m, a in mw.calls if m == "zfs.snapshot.delete"] + deletes = [a for m, a in mw.calls if m.endswith(".delete")] assert len(deletes) == 1, "should be ONE recursive call, not one per snapshot" assert deletes[0][1] == {"recursive": True} - assert not [m for m, _a in mw.calls if m == "zfs.snapshot.query"], ( + assert not [m for m, _a in mw.calls if m.endswith(".query")], ( "no enumeration needed on the fast path" ) def test_survives_recursive_and_query_failure_by_deleting_the_parent(self): class Broken(FakeMiddleware): def call_sync(self, method, *args): - if method == "zfs.snapshot.query": + if method.endswith(".query"): raise RuntimeError("boom") - if method == "zfs.snapshot.delete" and len(args) > 1: + if method.endswith(".delete") and len(args) > 1: raise RuntimeError("recursive delete unavailable") return super().call_sync(method, *args) mw = Broken(["Tap@snap"]) - delete_snapshot_tree(mw, "Tap@snap") + delete_snapshot_tree(mw, "Tap@snap", list_snapshots=mw.list_snapshots) assert mw.snapshots == [] def test_leaves_unrelated_snapshots_alone_when_the_tree_is_gone(self): mw = FakeMiddleware(["Tap@unrelated"]) - delete_snapshot_tree(mw, "Tap@snap") + delete_snapshot_tree(mw, "Tap@snap", list_snapshots=mw.list_snapshots) assert mw.snapshots == ["Tap@unrelated"] @@ -692,7 +726,7 @@ class BusyMiddleware(FakeMiddleware): self.attempts = 0 def call_sync(self, method, *args): - if method == "zfs.snapshot.delete": + if method.endswith(".delete"): name = args[0] opts = args[1] if len(args) > 1 else {} if opts.get("recursive"): @@ -713,7 +747,7 @@ class TestDeleteRetriesAndReportsSurvivors: ["Tap@snap", "Tap/apps@snap", "Tap/apps/prometheus@snap"], busy=["Tap/apps/prometheus@snap"], busy_for=1, ) - survivors = tn.delete_snapshot_tree(mw, "Tap@snap", sleep=lambda _s: None) + survivors = tn.delete_snapshot_tree(mw, "Tap@snap", sleep=lambda _s: None, list_snapshots=mw.list_snapshots) assert survivors == [] assert mw.snapshots == [] @@ -725,7 +759,7 @@ class TestDeleteRetriesAndReportsSurvivors: ["Tap@snap", "Tap/apps/prometheus@snap"], busy=["Tap/apps/prometheus@snap"], busy_for=99, ) - survivors = tn.delete_snapshot_tree(mw, "Tap@snap", sleep=lambda _s: None) + survivors = tn.delete_snapshot_tree(mw, "Tap@snap", sleep=lambda _s: None, list_snapshots=mw.list_snapshots) assert survivors == ["Tap/apps/prometheus@snap"] assert mw.snapshots == ["Tap/apps/prometheus@snap"] @@ -742,7 +776,7 @@ class TestDeleteRetriesAndReportsSurvivors: return real(method, *args) mw.call_sync = spy - tn.delete_snapshot_tree(mw, "Tap@snap", sleep=lambda _s: None) + tn.delete_snapshot_tree(mw, "Tap@snap", sleep=lambda _s: None, list_snapshots=mw.list_snapshots) assert order[0] == ("release", "snap"), order @@ -1002,6 +1036,7 @@ class TestGarbageCollectorExecution: remaining = tn.gc_stale_snapshots( mw, "cloud_backup-5", "Tap@cloud_backup-5-20260714115900", now=now, mounts_file=str(mounts), + list_snapshots=mw.list_snapshots, ) assert remaining == [] assert mw.snapshots == [ @@ -1026,6 +1061,7 @@ class TestGarbageCollectorExecution: remaining = tn.gc_stale_snapshots( mw, "cloud_backup-5", "Tap@cloud_backup-5-20260714115900", now=now, mounts_file=str(mounts), + list_snapshots=mw.list_snapshots, ) assert remaining == [orphan] @@ -1039,7 +1075,7 @@ class TestGarbageCollectorExecution: class Broken(FakeMiddleware): def call_sync(self, method, *args): - if method == "zfs.snapshot.query": + if method.endswith(".query"): raise RuntimeError("middleware is having a day") return super().call_sync(method, *args) @@ -1049,3 +1085,96 @@ class TestGarbageCollectorExecution: now=dt.datetime(2026, 7, 14, 12, 0, 0, tzinfo=dt.UTC), mounts_file=str(mounts), ) == [] + + +class TestEnumerationComesFromZfsNotMiddleware: + """The bug that a source check can never catch, found only by running it. + + Porting the deleted private `zfs.dataset.query` to the public + `pool.dataset.query` looked obviously right: the method exists, it is + documented, iX will not delete it. Every test passed and `compat.py` went green + on TrueNAS 26. + + It was wrong. The public query applies a VISIBILITY POLICY -- on a real box it + returns 205 of 274 datasets, hiding `ix-apps/*`, `.system/*` and `.ix-virt/*`. + On the production pool that is 84 of 270, and `ix-apps` holds LIVE APPLICATION + DATA. The staging plan would have omitted every one of them, and `plan_staging` + would never have seen them, so they would not even appear in `skipped`. A green + backup, silently missing data -- the precise failure this module exists to + prevent. + + The snapshot query lies the same way, so the sweep would orphan one snapshot per + hidden dataset, forever. + + Hence: READ from ZFS, MUTATE through middleware. These tests hold that line. + """ + + def test_query_filesystems_shells_out_to_zfs(self): + import truecloud_nested as tn + + seen = [] + + class R: + returncode = 0 + stdout = ( + "scratch\t/mnt/scratch\tyes\n" + "scratch/ix-apps\t/mnt/scratch/ix-apps\tyes\n" # middleware HIDES this one + "scratch/.system\tlegacy\tno\n" + ) + stderr = "" + + def runner(cmd): + seen.append(cmd) + return R() + + rows = tn.query_filesystems(runner=runner) + + assert seen and seen[0][0] == "zfs", "must read ZFS, not call middleware" + names = [r["name"] for r in rows] + assert "scratch/ix-apps" in names, ( + "ix-apps is exactly what pool.dataset.query hides, and exactly what " + "holds live app data. If it is not here the backup omits it silently." + ) + # ...and it still speaks the shape the planner expects. + row = next(r for r in rows if r["name"] == "scratch/.system") + assert row["properties"]["mountpoint"]["value"] == "legacy" + assert row["properties"]["mounted"]["value"] == "no" + + def test_a_failing_zfs_raises_rather_than_returning_an_empty_list(self): + # The whole failure class in one assertion. "No datasets" and "the command + # broke" must never look the same: a caller that cannot tell them apart + # stages nothing, sweeps nothing, and reports success. + import truecloud_nested as tn + + class R: + returncode = 1 + stdout = "" + stderr = "cannot open 'scratch': no such pool" + + with pytest.raises(tn.ZfsError, match="no such pool"): + tn.query_filesystems(runner=lambda cmd: R()) + + with pytest.raises(tn.ZfsError): + tn.list_snapshot_names("scratch", runner=lambda cmd: R()) + + def test_list_snapshot_names_reads_the_whole_tree_from_zfs(self): + import truecloud_nested as tn + + seen = [] + + class R: + returncode = 0 + stdout = "scratch@s\nscratch/ix-apps@s\n" + stderr = "" + + def runner(cmd): + seen.append(cmd) + return R() + + names = tn.list_snapshot_names("scratch", runner=runner) + assert names == ["scratch@s", "scratch/ix-apps@s"] + assert "-t" in seen[0] and "snapshot" in seen[0] and "-r" in seen[0] + assert "scratch/ix-apps@s" in names, ( + "pool.snapshot.query hides this; a sweep that cannot see it orphans it " + "on every single run, forever" + ) diff --git a/tools/compat.py b/tools/compat.py index f190c0b..48aa692 100644 --- a/tools/compat.py +++ b/tools/compat.py @@ -162,54 +162,119 @@ class MiddlewareCall: the whole design: declining is always the cheaper mistake. """ - def __init__(self, ident, module, method, path, why=""): + def __init__(self, ident, module, method, path, why="", also=()): self.id = ident self.module = module - self.method = method # "zfs.snapshot.delete" + self.method = method # "pool.snapshot.delete" self.path = path # plugin file that declares it self.why = why + #: Equally acceptable spellings of the SAME call, as (method, path) pairs. + #: + #: No single snapshot namespace spans every supported release. 24.10 and + #: 25.04 expose the CRUD service as the public `zfs.snapshot`; 25.10 + #: promoted it to `pool.snapshot` and demoted `zfs.snapshot` to private; + #: 26 deleted `plugins/zfs_/` entirely. Pinning either one alone marks + #: half the matrix BROKEN and declines to apply on versions that work + #: perfectly well. + #: + #: The call is satisfied if ANY option is present. The runtime picks the + #: same way -- see `pick_snapshot_service()` in the nested module -- so + #: what this checks and what the patch does cannot drift apart. + self.also = tuple(also) + + @property + def options(self): + """Every (method, path) that would satisfy this call, best first.""" + return ((self.method, self.path), *self.also) + + @staticmethod + def namespace_of(method): + return method.rsplit(".", 1)[0] + + @staticmethod + def name_of(method): + return method.rsplit(".", 1)[1] @property def namespace(self): - return self.method.rsplit(".", 1)[0] + return self.namespace_of(self.method) @property def name(self): - return self.method.rsplit(".", 1)[1] + return self.name_of(self.method) #: Every middlewared method the nested module calls at runtime. +#: The middleware methods the nested module CALLS. +#: +#: These used to be the PRIVATE `zfs.*` service (`zfs.dataset.query`, +#: `zfs.snapshot.delete`, `zfs.snapshot.query`). TrueNAS 26 deleted +#: `plugins/zfs_/` outright and every one of them vanished -- silently, because a +#: private service carries no stability contract and nothing warned us. The patch +#: would have applied cleanly and then failed on the first backup. +#: +#: The replacements are the PUBLIC `pool.*` API, and switching to it is not merely +#: a TrueNAS 26 fix -- it is the correct call on every version: +#: +#: * It is public, documented, and covered by iX's deprecation policy, so it +#: cannot be deleted from under us the way `zfs.*` just was. +#: * The same methods, in the same files, exist on 24.10 through 26. One code +#: path, no version conditionals. +#: * `pool.snapshot.delete` takes `recursive`, which the private call did not. +#: The old sweep had to enumerate ~250 snapshots and delete them one at a +#: time, and any it missed leaked forever. MIDDLEWARE_CALLS = [ MiddlewareCall( - "call-zfs-dataset-query", NESTED, "zfs.dataset.query", - "plugins/zfs_/dataset.py", - why="SNAPSHOT_BLOCK enumerates FILESYSTEM datasets to build the staging plan", - ), - MiddlewareCall( - "call-zfs-snapshot-delete", NESTED, "zfs.snapshot.delete", - "plugins/zfs_/snapshot.py", + "call-snapshot-delete", NESTED, "pool.snapshot.delete", + "plugins/pool_/snapshot.py", + also=[("zfs.snapshot.delete", "plugins/zfs_/snapshot.py")], why="delete_snapshot_tree() sweeps the recursive snapshot. Without it every " "run orphans one snapshot per descendant dataset (250 on a real pool)", ), - MiddlewareCall( - "call-zfs-snapshot-query", NESTED, "zfs.snapshot.query", - "plugins/zfs_/snapshot.py", - why="delete_snapshot_tree()'s fallback sweep enumerates the tree by name", - ), ] +# There is deliberately NO entry here for a dataset or snapshot QUERY. +# +# The patch used to call `zfs.dataset.query` / `zfs.snapshot.query` (private, and +# deleted in TrueNAS 26). The obvious port was to the public `pool.dataset.query` / +# `pool.snapshot.query` -- and that port was WRONG in a way no source check could +# ever have caught, because the methods are all present and correctly shaped. +# +# They are simply filtered. On a real box they return 205 of 274 datasets and 205 +# of 274 snapshots, hiding `ix-apps/*`, `.system/*` and `.ix-virt/*` -- 84 of 270 +# on the production pool, including live application data. Staging from that view +# silently omits them; sweeping from it orphans one snapshot per hidden dataset, +# forever. +# +# So the module enumerates from ZFS itself and there is no middleware assumption +# left to check. That is the point: the fewer things we assume about middleware, +# the less there is for iX to break. Only the MUTATION is still a middleware call, +# and that is the one entry above. + + +def check_call(c: MiddlewareCall, src: str | None, + method: str | None = None, path: str | None = None, + ) -> tuple[str, str | None]: + """Is `method` still registered by middlewared? + + `method`/`path` name WHICH spelling of the call is being tried -- a call may + have several equally acceptable ones (see MiddlewareCall.also). They default + to the preferred spelling. + """ + method = method or c.method + path = path or c.path + namespace = MiddlewareCall.namespace_of(method) + name = MiddlewareCall.name_of(method) -def check_call(c: MiddlewareCall, src: str | None) -> tuple[str, str | None]: - """Is `c.method` still registered by middlewared?""" if src is None: return "broken", ( - f"{c.path} no longer exists, so `{c.method}` is gone" + f"{path} no longer exists, so `{method}` is gone" ) try: tree = ast.parse(_stock(src)) except SyntaxError as e: - return "unknown", f"{c.path} does not parse: {e}" + return "unknown", f"{path} does not parse: {e}" # namespace = 'zfs.snapshot' on some Service class in this file... namespaces = { @@ -220,10 +285,10 @@ def check_call(c: MiddlewareCall, src: str | None) -> tuple[str, str | None]: and isinstance(n.value.value, str) and any(isinstance(t, ast.Name) and t.id == "namespace" for t in n.targets) } - if c.namespace not in namespaces: + if namespace not in namespaces: return "broken", ( - f"{c.path} no longer declares namespace {c.namespace!r} " - f"(found: {sorted(namespaces) or 'none'}), so `{c.method}` is gone" + f"{path} no longer declares namespace {namespace!r} " + f"(found: {sorted(namespaces) or 'none'}), so `{method}` is gone" ) # ...and it defines the method. @@ -238,8 +303,8 @@ def check_call(c: MiddlewareCall, src: str | None) -> tuple[str, str | None]: n.name for n in ast.walk(tree) if isinstance(n, ast.FunctionDef | ast.AsyncFunctionDef) } - if c.name not in defined and f"do_{c.name}" not in defined: - return "broken", f"{c.path} no longer defines `{c.method}`" + if name not in defined and f"do_{name}" not in defined: + return "broken", f"{path} no longer defines `{method}`" return "ok", None @@ -555,28 +620,46 @@ def check(loader, modules=None) -> dict: }) # The methods the injected code CALLS, not just the symbols it wraps. + # + # A call may have several equally acceptable spellings, because no single + # snapshot namespace spans every supported release (24.10 has `zfs.snapshot`, + # 26 has only `pool.snapshot`). It is satisfied if ANY of them is present -- + # exactly as the runtime resolves it -- and BROKEN only when they all vanish. for c in MIDDLEWARE_CALLS: if c.module not in out: continue - try: - text = src(c.path) - except Unreadable as e: - out[c.module]["unknown"] = True - out[c.module]["problems"].append({ - "id": c.id, "detail": f"could not read {c.path}: {e}", "why": c.why, - }) + + satisfied, unknown, details = False, False, [] + for method, path in c.options: + try: + text = src(path) + except Unreadable as e: + unknown = True + details.append(f"could not read {path}: {e}") + continue + + status, detail = check_call(c, text, method, path) + if status == "ok": + satisfied = True + break + if status == "unknown": + unknown = True + details.append(detail) + + if satisfied: continue - status, detail = check_call(c, text) - if status == "broken": - out[c.module]["ok"] = False - out[c.module]["problems"].append({ - "id": c.id, "detail": detail, "why": c.why, - }) - elif status == "unknown": + # Every spelling failed. If we could not READ one of them we do not know + # that it is broken -- a rate-limited fetch is not a regression. + if unknown: out[c.module]["unknown"] = True out[c.module]["problems"].append({ - "id": c.id, "detail": detail, "why": c.why, + "id": c.id, "detail": "; ".join(details), "why": c.why, + }) + else: + out[c.module]["ok"] = False + out[c.module]["problems"].append({ + "id": c.id, "detail": "; ".join(details), "why": c.why, }) for module, (path, phrase, native_when_present) in NATIVE_PROBES.items(): @@ -804,6 +887,10 @@ def is_broken(r: dict) -> bool: #: a strictly weaker claim than "a restore worked". Add a row only after doing it. HARDWARE_VERIFIED = { "25.10.4": "nested + providers; 252-snapshot recursive backup of /mnt/Tap, 18m", + "26.0.0-BETA.1": ( + "nested + providers; 274-snapshot recursive backup of a 292-dataset pool, " + "restored a 4-deep child dataset byte-identical" + ), } _LEGEND = """