Never touch CloudSync tasks; restore the logger the async cleanup path dropped
create_snapshot is module-global in plugins/cloud/snapshot.py, and cloud_sync.py imports it as well as cloud_backup/sync.py. So the wrapper sat in the path of every rclone/Storj CloudSync task with snapshot=true, and ran a zfs.dataset.query before concluding it had nothing to do -- a new failure mode for jobs that worked before this patch existed. Worse: a CloudSync task that ever got staged would never be torn down. The teardown is wired into cloud_backup's restic_backup finally, and CRUD_BLOCK deliberately leaves CloudSync's guard intact, so the bind mounts would pin the snapshot forever. The staging path now bails out unless the snapshot is named cloud_backup-*, before any middleware call. Separately: the async wrapper's finally dropped logger=, which the sync one passes. run_in_thread forwards **kwargs, so a cleanup that failed to unmount a bind mount or delete a snapshot tree logged nothing at all -- on the only platform anyone runs.
This commit is contained in:
@@ -101,6 +101,24 @@ worse than no alert, because one day it carries a security fix.
|
||||
`get_dataset_recursive` is carried as our own copy — removing the dependency on both
|
||||
versions instead of asserting it.
|
||||
|
||||
- **The patch no longer reaches into CloudSync tasks it has no business touching.**
|
||||
`create_snapshot` is module-global in `plugins/cloud/snapshot.py` and is imported by
|
||||
**`cloud_sync.py` as well as `cloud_backup/sync.py`** — so the wrapper sat in the
|
||||
path of every rclone/Storj **CloudSync** task with `snapshot=true`, and issued a
|
||||
`zfs.dataset.query` before deciding it had nothing to do. That added a brand-new
|
||||
failure mode to jobs that worked fine before this patch was installed, and worse: a
|
||||
CloudSync task that ever *did* get staged would **never be torn down**, because the
|
||||
teardown is wired into `cloud_backup`'s `restic_backup` and `CRUD_BLOCK`
|
||||
deliberately leaves CloudSync's nesting guard intact — the bind mounts would pin the
|
||||
ZFS snapshot forever. The staging path now bails out immediately unless the snapshot
|
||||
is named `cloud_backup-*`, before any middleware call.
|
||||
|
||||
- **Teardown warnings are no longer silently swallowed on TrueNAS ≤ 25.10.** The async
|
||||
wrapper's `finally` dropped the `logger=` kwarg that the sync one passes, so a
|
||||
cleanup that failed to unmount a bind mount *or* to delete a snapshot tree logged
|
||||
**nothing at all** — on the only platform anyone actually runs. `run_in_thread`
|
||||
forwards `**kwargs` via `functools.partial`; it was a regression, not a limitation.
|
||||
|
||||
- **`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
|
||||
|
||||
@@ -526,6 +526,28 @@ if _tc_nested is not None:
|
||||
|
||||
def _tc_stage(middleware, path, name, snapshot, snap_path):
|
||||
# Synchronous, and always called from a worker thread (see above).
|
||||
#
|
||||
# ONLY cloud_backup. Bail out before touching anything otherwise.
|
||||
#
|
||||
# create_snapshot is module-global in plugins/cloud/snapshot.py and is
|
||||
# imported by cloud_sync.py as well as cloud_backup/sync.py -- so this
|
||||
# wrapper sits in the path of every rclone/Storj CloudSync task with
|
||||
# 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
|
||||
# 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
|
||||
# CRUD_BLOCK deliberately leaves CloudSync's nesting guard intact. The
|
||||
# bind mounts would pin the snapshot forever.
|
||||
#
|
||||
# cloud_backup names its snapshot "cloud_backup-<id>"; cloud_sync names it
|
||||
# "cloud_sync-<id>"; the stock default is "cloud_task-onetime". Anything that
|
||||
# is not ours gets stock behaviour, untouched, with no extra middleware call.
|
||||
if not name.startswith("cloud_backup"):
|
||||
return snapshot, snap_path
|
||||
|
||||
_logger = getattr(middleware, "logger", None)
|
||||
try:
|
||||
# Enumerate datasets AFTER the snapshot, never before. The snapshot is
|
||||
|
||||
@@ -407,3 +407,39 @@ class TestTheTwoNativeProbesCannotDrift:
|
||||
assert static_native == expect_native, (
|
||||
f"compat.py says native={static_native} for {src!r}"
|
||||
)
|
||||
|
||||
|
||||
class TestOnlyOurOwnTasksAreTouched:
|
||||
"""create_snapshot is module-global, and cloud_sync.py imports it too.
|
||||
|
||||
plugins/cloud/snapshot.py::create_snapshot is imported by BOTH
|
||||
cloud_backup/sync.py and cloud_sync.py, so our wrapper sits in the path of every
|
||||
rclone/Storj CloudSync task with snapshot=true -- tasks this patch has no business
|
||||
touching. Two consequences, the second much worse than the first:
|
||||
|
||||
* every middleware call we add is a NEW failure mode for a job that worked
|
||||
before we were installed;
|
||||
* a staged CloudSync task would NEVER be torn down. The teardown is wired into
|
||||
cloud_backup's restic_backup finally, and CRUD_BLOCK deliberately leaves
|
||||
CloudSync's nesting guard intact -- so the bind mounts would pin the ZFS
|
||||
snapshot forever.
|
||||
|
||||
cloud_backup names its snapshot "cloud_backup-<id>", cloud_sync "cloud_sync-<id>",
|
||||
and stock's default is "cloud_task-onetime".
|
||||
"""
|
||||
|
||||
@pytest.mark.parametrize("name", ["SNAPSHOT_ASYNC", "SNAPSHOT_SYNC"])
|
||||
def test_the_staging_path_is_gated_on_cloud_backup(self, name):
|
||||
block = extract_blocks()[name]
|
||||
assert 'if not name.startswith("cloud_backup"):' in block
|
||||
|
||||
@pytest.mark.parametrize("name", ["SNAPSHOT_ASYNC", "SNAPSHOT_SYNC"])
|
||||
def test_the_bail_out_precedes_every_middleware_call(self, name):
|
||||
# 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.
|
||||
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("):
|
||||
assert gate < block.index(call), f"{call} runs before the cloud_backup gate"
|
||||
|
||||
Reference in New Issue
Block a user