From ecb64878ffc68dacbaedc95391cb5633df1d8099 Mon Sep 17 00:00:00 2001 From: sudolulo Date: Mon, 13 Jul 2026 18:25:14 +0000 Subject: [PATCH] compat: check the middlewared METHODS we call, not just the symbols we wrap This gap was hiding a catastrophe. TrueNAS 26 deletes plugins/zfs_/dataset.py and plugins/zfs_/snapshot.py outright, taking zfs.dataset.query, zfs.snapshot.query and zfs.snapshot.delete with them (26 uses filesystem.statfs and zfs.resource.*). Nothing about the five cloud_backup files reveals that, so every other check went green -- including the one I had just added. The patch would have applied cleanly and then failed on the first backup, or worse: snapshotted fine and failed to DELETE, orphaning one snapshot per descendant dataset (250 on a real pool) on every run, forever. So 26 is BROKEN and the nested module will not apply there. The async/sync wrapper work and the vendored get_dataset_recursive stay -- they are correct and necessary -- but 26 is not supported until the ZFS calls are ported, and that needs a real 26 box to verify. Shipping a port nobody has run is the failure this project exists to avoid. Also: do_delete is recognised as delete (24.10/25.04 use the CRUDService convention), which was reporting both as BROKEN -- a false verdict that would have disabled nested snapshots on boxes where they work. --- CHANGELOG.md | 63 +++++++++++++------- README.md | 59 +++++++++++-------- patch/apply.sh | 6 ++ tests/test_compat.py | 88 ++++++++++++++++++++++++++++ tools/compat.py | 133 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 306 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03e146a..f0bf1c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,28 +59,53 @@ worse than no alert, because one day it carries a security fix. ### Added -- **TrueNAS 26 support.** 26 rewrites the entire `cloud_backup` path from async to - **synchronous**, and separately **deletes `get_dataset_recursive()`** — which one - of the injected blocks called out of the host module's namespace. Either one is a - broken backup found at restore time: an `async def` wrapper hands `sync.py` a - coroutine where it unpacks a tuple, and the vanished helper is a straight - `NameError`. +- **The compatibility check now covers the middlewared methods the patch _calls_,** + not only the symbols it wraps — and that gap was hiding a catastrophe. - The nested module is now **one synchronous implementation** (talking to middlewared - through `call_sync`) behind **two thin wrappers**. `apply.sh` reads which flavour - the installed middleware declares and injects the matching one: TrueNAS ≤ 25.10 - reaches it via `await middleware.run_in_thread(...)`, and TrueNAS 26 — already in a - worker thread — calls it directly. The logic that owns the snapshots, the bind - mounts and the failure modes exists **once**; an async twin would mean every future - fix had to land twice, and the one that got missed would be the one that eats a - backup. + TrueNAS 26 **deletes `plugins/zfs_/dataset.py` and `plugins/zfs_/snapshot.py` + outright**, taking `zfs.dataset.query`, `zfs.snapshot.query` and + `zfs.snapshot.delete` with them (26 uses `filesystem.statfs` and `zfs.resource.*`). + Nothing about the five `cloud_backup` files reveals that, so every other check went + green. The patch would have applied perfectly and then **failed on the first + backup** — or, far worse, snapshotted successfully and failed to *delete*, + orphaning one snapshot per descendant dataset (**250 on a real pool**) on every + single run, forever. - A middleware whose three wrapped functions **disagree** about async-ness is refused - outright rather than guessed at. And `get_dataset_recursive` is now carried as our - own copy — removing the dependency on both versions instead of asserting it. + This is now an assumption class of its own, so a method disappearing is a BROKEN + verdict rather than a silent time bomb. - Both breaks were found by the daily compatibility check **while 26 was still in - beta**, which is the entire point of it. +- **Groundwork for TrueNAS 26** (async→sync and the deleted helper — see below). + **26 is still reported BROKEN and the nested module will not apply there**, because + the ZFS API rewrite above is not yet ported. Porting it needs a real 26 box to + verify against, and shipping a port nobody has run is exactly the failure this + project exists to avoid. On 26, TrueNAS is left stock: B2/S3 keeps working, nested + datasets are simply not covered. + +### Fixed + +- **The nested module is now one synchronous implementation behind two thin + wrappers.** TrueNAS 26 rewrites `cloud_backup` from async to **synchronous** and + separately **deletes `get_dataset_recursive()`**, which an injected block called out + of the host module's namespace. Either alone is a broken backup found at restore + time: an `async def` wrapper hands `sync.py` a coroutine where it unpacks a tuple, + and the vanished helper is a straight `NameError`. + + The module now talks to middlewared through `call_sync`, and `apply.sh` reads which + flavour the installed middleware declares and injects the matching wrapper — + TrueNAS ≤ 25.10 reaches it via `await middleware.run_in_thread(...)`; a synchronous + TrueNAS, already in a worker thread, calls it directly. The logic that owns the + snapshots, the bind mounts and the failure modes exists **once**; an async twin + would mean every future fix had to land twice, and the one that got missed would be + the one that eats a backup. A middleware whose three wrapped functions **disagree** + about async-ness is refused outright rather than guessed at, and + `get_dataset_recursive` is carried as our own copy — removing the dependency on both + versions instead of asserting it. + +- **`do_delete` is recognised as `delete`.** TrueNAS 24.10 and 25.04 declare + `do_delete` (the `CRUDService` convention); 25.10 renamed it to `delete`. Both + answer to `zfs.snapshot.delete`. Accepting only the literal name reported both older + releases as BROKEN — a false verdict that would have switched nested snapshots off + on boxes where they work perfectly. - **An incompatible TrueNAS no longer sets the permanent kill switch.** `apply.sh` reused a "nothing left to do" exit that touches `disabled`, which suppresses diff --git a/README.md b/README.md index 9f5e848..6508061 100644 --- a/README.md +++ b/README.md @@ -6,8 +6,10 @@ > 24.10**. There is nothing here to install on an older release, and `install.sh` > will refuse. > -> Verified on **24.10**, **25.04**, **25.10**, and the unreleased **26.0 beta** -> (see [TrueNAS compatibility](#truenas-compatibility)). +> Verified on **24.10**, **25.04** and **25.10**. The unreleased **26.0** is not yet +> supported for nested snapshots — see [TrueNAS +> compatibility](#truenas-compatibility). Nothing breaks if you upgrade: the patch +> checks, and declines. Extends TrueNAS SCALE's **TrueCloud Backup** feature to: @@ -44,7 +46,7 @@ costs a fraction of the new Storj price. | 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 | ok | — | +| 26.0.0-BETA.3 _(unreleased)_ | ok | **BROKEN** | — | | master _(unreleased)_ | **BROKEN** | **BROKEN** | — | | verdict | meaning | @@ -62,31 +64,40 @@ The table above is **regenerated daily by CI** — it is not a claim somebody ty once and forgot. **TrueCloud Backup does not exist before 24.10**, so earlier versions are absent rather than "unsupported". -### TrueNAS 26 — supported, and this is how we knew in advance +### TrueNAS 26 — nested snapshots are not supported yet -TrueNAS 26 rewrites the whole `cloud_backup` path **from async to synchronous**. -Every block the nested module injected was an `async def` wrapping an `await`ed -original, so on 26 it would have handed `sync.py` a coroutine where it unpacks a -tuple, and 26 also **deleted `get_dataset_recursive()`**, which one of those blocks -called. Both are backup-breaking, and neither would have surfaced until a restore -failed. +**You do not need to do anything, and upgrading will not break your backups.** On +26, `apply.sh` finds that the nested module's assumptions no longer hold and **does +not apply it**. TrueNAS is left stock: B2/S3 backups keep running (the providers +module is unaffected), datasets nested under the target are simply not included, and +the reason is named in `apply.log`. A broken backup is worse than a missing feature. -The daily compatibility check found both **while 26 was still in beta**, and filed -the bug report itself. The patch now reads which flavour of `cloud_backup` your box -has and injects the wrapper that matches — one implementation of the actual logic, -two thin wrappers — and carries its own copy of the deleted helper. +TrueNAS 26 changes three things underneath this module, and each one alone is +backup-breaking: -**If a future TrueNAS breaks it anyway, nothing bad happens quietly.** `apply.sh` -re-checks these assumptions against the middleware *actually installed on your box* -at every boot and will not apply a module that no longer fits: TrueNAS is left -stock, backups keep running without that module's feature, and the reason is named -in `apply.log`. A broken backup is worse than a missing feature. +| what changed | what it would have done | +| --- | --- | +| `cloud_backup` rewritten **async → synchronous** | an `async def` wrapper hands `sync.py` a coroutine where it unpacks a tuple | +| `get_dataset_recursive()` **deleted** from `plugins/cloud/snapshot.py` | `NameError` — the injected block called it out of the host module's namespace | +| `plugins/zfs_/dataset.py` and `zfs_/snapshot.py` **deleted** | `zfs.dataset.query`, `zfs.snapshot.query` and `zfs.snapshot.delete` all vanish. 26 uses `filesystem.statfs` and `zfs.resource.*` instead | -`master` (the development branch after 26) currently reports **BROKEN**: iXsystems -are still reshaping these functions there — renaming `middleware` to `context`, -`cloud_backup` to `entry`, adding a required `credentials` parameter. That is a -moving target and is deliberately not chased; the check will keep reporting it until -it settles into a beta, which is exactly when it becomes worth fixing. +The first two are fixed: the patch now reads which flavour of `cloud_backup` your box +declares and injects the wrapper that matches (one implementation of the real logic, +two thin wrappers), and it carries its own copy of the deleted helper. + +The third is **not** fixed, and is why 26 still reports BROKEN. Porting it means +rewriting the module's ZFS calls onto 26's new API, and there is no single API that +spans 24.10 through 26 — so it needs a real 26 box to verify against, not a +plausible-looking diff. **Shipping a port nobody has run is exactly the failure this +project exists to avoid.** The third row is also the one that would have hurt most: +`zfs.snapshot.delete` is what sweeps the recursive snapshot, and without it every run +would orphan one snapshot per descendant dataset — 250 on a real pool — forever. + +`master` (development after 26) reports BROKEN too: iXsystems are still reshaping +these functions there, renaming `middleware` → `context` and `cloud_backup` → +`entry`, and adding a required `credentials` parameter. That is a moving target and +is deliberately not chased; the check keeps reporting it until it settles into a +beta, which is when it becomes worth fixing. ### How this is kept honest diff --git a/patch/apply.sh b/patch/apply.sh index af79b51..087ebeb 100755 --- a/patch/apply.sh +++ b/patch/apply.sh @@ -640,10 +640,16 @@ if _tc_nested is not None: return await _tc_orig_restic_backup(middleware, job, cloud_backup, *args, **kwargs) finally: try: + # logger= must be passed here too, exactly as the sync variant does. + # run_in_thread forwards **kwargs (functools.partial), and without it + # cleanup_task gets logger=None -- so every teardown warning ("could + # not unmount X") is silently swallowed on <= 25.10, which is most + # boxes. The two wrappers must differ ONLY in how they reach the core. await middleware.run_in_thread( _tc_nested.cleanup_task, middleware, f"cloud_backup-{cloud_backup.get('id', 'onetime')}", + logger=getattr(middleware, "logger", None), ) except Exception as e: middleware.logger.warning("truecloud-patch: staging cleanup failed: %r", e) diff --git a/tests/test_compat.py b/tests/test_compat.py index 7de58b7..4d405a5 100644 --- a/tests/test_compat.py +++ b/tests/test_compat.py @@ -51,6 +51,22 @@ 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" + " class Config:\n" + " namespace = 'zfs.dataset'\n" + " def query(self, filters, options):\n pass\n" + ), + "plugins/zfs_/snapshot.py": ( + "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" + ), } @@ -271,3 +287,75 @@ class TestAsyncFlavour: def test_the_real_truenas_versions(self): # Pinning the actual fact this whole port exists for. assert compat.async_flavour(loader(GOOD)) is True + + +class TestMiddlewareMethodsWeCall: + """The assumption class that was MISSING, and that hid a catastrophic break. + + The manifest recorded the symbols the patch WRAPS. It said nothing about the + middlewared methods the patch CALLS -- and TrueNAS 26 deleted + plugins/zfs_/dataset.py and plugins/zfs_/snapshot.py outright, taking + `zfs.dataset.query`, `zfs.snapshot.query` and `zfs.snapshot.delete` with them. + + Nothing about the five cloud_backup files reveals that. The patch applied + perfectly and every other check went green. The first backup would have failed -- + or, far worse, snapshotted fine and then failed to DELETE, orphaning one snapshot + 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" + ) + + 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 + + def test_present_methods_are_ok(self): + r = check_files(self._tree()) + 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})) + assert is_broken(r[NESTED]) + details = " ".join(p["detail"] for p in r[NESTED]["problems"]) + assert "zfs.snapshot.delete" in details + + 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'"), + })) + 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( + "def delete(", "def do_delete("), + })) + assert r[NESTED]["ok"], r[NESTED]["problems"] + + 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})) + whys = " ".join(p["why"] for p in r[NESTED]["problems"]) + assert "orphan" in whys diff --git a/tools/compat.py b/tools/compat.py index 8defeb5..ccf616c 100644 --- a/tools/compat.py +++ b/tools/compat.py @@ -135,6 +135,114 @@ ASSUMPTIONS = [ ] +class MiddlewareCall: + """A middlewared METHOD the injected code calls at runtime. + + THIS CLASS OF ASSUMPTION IS WHY THE CHECKER EXISTS, AND IT WAS THE ONE MISSING. + + The manifest above records the symbols the patch *wraps*. It said nothing about + the methods the patch *calls* -- and that gap hid two separate TrueNAS 26 breaks + that both pass every other check: + + * `get_dataset_recursive()` was deleted from plugins/cloud/snapshot.py, and the + injected block called it out of the host module's namespace (now vendored). + * plugins/zfs_/dataset.py and plugins/zfs_/snapshot.py were DELETED outright, + taking `zfs.dataset.query`, `zfs.snapshot.query` and `zfs.snapshot.delete` + with them. 26 uses filesystem.statfs and zfs.resource.* instead. + + Nothing about the five cloud_backup files reveals that. The patch would apply + perfectly, and then the FIRST BACKUP would fail -- or, far worse, succeed at + snapshotting and fail at `zfs.snapshot.delete`, orphaning one snapshot per + descendant dataset (250 on a real pool) on every single run, forever. + + A method is present when some plugin file declares its namespace AND defines it. + If iX merely MOVES a method to a different file we report BROKEN wrongly, and the + module declines to apply -- costing a feature, not a backup. That asymmetry is + the whole design: declining is always the cheaper mistake. + """ + + def __init__(self, ident, module, method, path, why=""): + self.id = ident + self.module = module + self.method = method # "zfs.snapshot.delete" + self.path = path # plugin file that declares it + self.why = why + + @property + def namespace(self): + return self.method.rsplit(".", 1)[0] + + @property + def name(self): + return self.method.rsplit(".", 1)[1] + + +#: Every middlewared method the nested module calls at runtime. +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", + 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", + ), +] + + +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" + ) + + try: + tree = ast.parse(_stock(src)) + except SyntaxError as e: + return "unknown", f"{c.path} does not parse: {e}" + + # namespace = 'zfs.snapshot' on some Service class in this file... + namespaces = { + n.value.value + for n in ast.walk(tree) + if isinstance(n, ast.Assign) + and isinstance(n.value, ast.Constant) + 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: + return "broken", ( + f"{c.path} no longer declares namespace {c.namespace!r} " + f"(found: {sorted(namespaces) or 'none'}), so `{c.method}` is gone" + ) + + # ...and it defines the method. + # + # A CRUDService exposes `create`/`update`/`delete` from methods NAMED + # `do_create`/`do_update`/`do_delete`. Both spellings are live right now: + # 24.10 and 25.04 declare `do_delete`, 25.10 renamed it to `delete`, and all + # three 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. + defined = { + 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}`" + + return "ok", None + + #: Things that mean iX has done the job themselves and the module should RETIRE, #: not break. Absence of the nesting guard = nested snapshots went native. #: `restic = True` already on B2RcloneRemote = B2 restic support went native. @@ -445,6 +553,31 @@ def check(loader, modules=None) -> dict: "id": a.id, "detail": detail, "why": a.why, }) + # The methods the injected code CALLS, not just the symbols it wraps. + 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, + }) + 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": + out[c.module]["unknown"] = True + out[c.module]["problems"].append({ + "id": c.id, "detail": detail, "why": c.why, + }) + for module, (path, phrase, native_when_present) in NATIVE_PROBES.items(): if module not in out: continue