From 023618bf627e4b1e5a96098b8a25db36a16140f6 Mon Sep 17 00:00:00 2001 From: Nicolas MASSE Date: Wed, 2 Sep 2026 16:02:18 +0200 Subject: [PATCH] Let zfs-autobackup decide which domains are configured Before handing a domain over, snapshot-libvirt-domains guarded it behind 'zfs get -s local autobackup:libvirt-' and tested the result for non-emptiness. That answers neither of the questions that matter. -s local drops exactly the inherited lines, while zfs-autobackup looks the property up without -s and resolves inheritance (ZfsNode.selected_datasets rewrites "inherited from " to the parent's source, ZfsDataset.is_selected takes an inherited flag). Configuring the fleet once on the parent dataset - the normal way to drive zfs-autobackup, and the only way to cover datasets that already exist without walking them one by one - therefore made zvirt skip every domain, while the very same backup names handed to zfs-autobackup by hand snapshotted all of them. The guard also had no dataset operand, so it asked "does any dataset in the pools carry this property locally?"; and it tested presence where zfs-autobackup tests value, so a local =false passed it, and child/parent were misjudged in both directions. Drop the guard and let zfs-autobackup answer, since it is the one selecting. It reports "nothing selected" as exit 255 with a distinctive message, so keep its stderr aside for the length of one domain, long enough to tell that case (skip the domain, carry on with the fleet, exit 0) from a genuine failure (replay the stderr, abort with its exit code). Its own wording is swallowed in the skip case: it is printed as an error while nothing failed. Document the selection in the README - the four property values, the fact that inheritance counts, and the two properties involved (autobackup:libvirt- to snapshot, autobackup:libvirt to prune). This does not cover the case of a domain whose selection misses part of its storage: the property selects datasets, not domains, and only zvirt knows what a domain is made of. Tracked in #3. Closes #2 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014BSA2qQpCr7cY4qDEX7kuX --- README.md | 31 +++++++++++++++++++ packaging/zvirt.spec | 4 +++ src/bin/snapshot-libvirt-domains | 53 +++++++++++++++++++++++++++----- 3 files changed, 81 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index ff4e976..fe92b50 100644 --- a/README.md +++ b/README.md @@ -13,6 +13,37 @@ At the end, all components of a domain - Domain definition, TPM, NVRAM, VirtioFS - Support both crash-consistent and live snapshots. - Support batch mode (pause all domains, take snapshots, then resume all domains) +## Selecting the domains to snapshot + +`snapshot-libvirt-domains` iterates over the libvirt domains and hands each one to `zfs-autobackup` +under the backup name `libvirt-`, that is, the ZFS property `autobackup:libvirt-`. +Which datasets that property selects is `zfs-autobackup`'s decision, not zvirt's: it looks the +property up on every dataset, resolves inheritance, and tests its **value**: + +| Value | Selected | +| -------- | ------------------------------------------------ | +| `true` | yes, on the dataset and everything inheriting it | +| `false` | no, explicitly excluded | +| `child` | only the datasets that *inherit* it, not this one | +| `parent` | only this dataset, not the ones inheriting it | + +Inherited properties count, so the whole fleet can be configured on the parent dataset of all the +domains: + +```console +$ zfs set autobackup:libvirt-quay=true data/domains/quay # this domain and its children +$ zfs set autobackup:libvirt=true data/domains # the shared property, for pruning +``` + +A domain that carries the property nowhere is skipped with a message on stderr and the run carries +on with the next one, exiting 0 — configuring only some domains is a legitimate setup. + +> [!WARNING] +> The property selects **datasets**, not domains, and zvirt does not check that the selection +> actually covers a domain's storage. Set it on the domain's root dataset — set on a child only, the +> run reports success while the disks above it are never snapshotted. `zfs-autobackup --test +> --no-send --no-thinning libvirt-` prints the datasets it would select. + ## Snapshot retention `snapshot-libvirt-domains` runs `zfs-autobackup` with `--no-thinning`: it only creates snapshots and diff --git a/packaging/zvirt.spec b/packaging/zvirt.spec index 71b8513..d8a6dff 100644 --- a/packaging/zvirt.spec +++ b/packaging/zvirt.spec @@ -56,6 +56,10 @@ pip install --root %{buildroot} --prefix %{_prefix} --no-compile --no-deps --no- %{python3_sitelib}/zfs_autobackup-*.dist-info/ %changelog +* Wed Sep 02 2026 Nicolas Massé - 0.0.8-1 +- Let zfs-autobackup decide which domains are configured, so that an inherited + autobackup:libvirt- property is honoured + * Wed Sep 02 2026 Nicolas Massé - 0.0.7-1 - Make the snapshot name format queryable (--print-snapshot-format) - Ship /usr/share/zvirt/snapshot-format as the single source of truth diff --git a/src/bin/snapshot-libvirt-domains b/src/bin/snapshot-libvirt-domains index 10c2662..4f935fd 100755 --- a/src/bin/snapshot-libvirt-domains +++ b/src/bin/snapshot-libvirt-domains @@ -58,6 +58,18 @@ function run () { "$@" } +# Same as run(), but keeps the command's stderr in the given file instead of +# letting it through: the caller needs to read it before deciding what it means. +function run_capturing_stderr () { + local stderr_file="$1" + shift + + if [ "$verbose" -eq 1 ]; then + echo "$*" >&2 + fi + "$@" 2>"$stderr_file" +} + OPTIND=1 # Reset in case getopts has been used previously in the shell. while getopts "h?lv-:" opt; do @@ -116,12 +128,11 @@ if [ "$live" -eq 1 ]; then else virsh_args+=("--all") fi +# zfs-autobackup's stderr is kept aside for the length of one domain, see below. +stderr_file="$(mktemp)" +trap 'rm -f "$stderr_file"' EXIT + for domain in $(virsh list --name "${virsh_args[@]}"); do - if [ "$(zfs get -t filesystem,volume autobackup:libvirt-${domain} -o value -H -s local)" == "" ]; then - echo "Skipping domain ${domain} because it is not configured for autobackup" >&2 - continue - fi - declare -a zfs_autobackup_hooks_args=() if [ "$live" -eq 1 ]; then zfs_autobackup_hooks_args+=("-l" "-r" "/var/lib/libvirt/images/${domain}") @@ -130,8 +141,36 @@ for domain in $(virsh list --name "${virsh_args[@]}"); do zfs_autobackup_hooks_args+=("-v") fi - run zfs-autobackup "${zfs_autobackup_args[@]}" \ + # Whether a domain is configured for autobackup is zfs-autobackup's question to + # answer, not ours: it looks the autobackup:libvirt- property up on + # every dataset, resolves inheritance and tests the *value* (true, false, + # child, parent). A 'zfs get -s local' guard here could only test the presence + # of a *local* property, pool-wide: it skipped every domain configured by + # inheritance from a parent dataset - the normal way to configure a fleet - + # and, having no dataset operand, let through any domain whose property + # happened to be set on some unrelated dataset. + # + # zfs-autobackup reports "nothing selected" as exit 255 with a distinctive + # message, so keep its stderr aside long enough to tell that case (skip this + # domain, carry on with the others) from a genuine failure (abort). + rc=0 + run_capturing_stderr "$stderr_file" \ + zfs-autobackup "${zfs_autobackup_args[@]}" \ --pre-snapshot-cmd "$SCRIPT_DIR/libvirt-hook ${zfs_autobackup_hooks_args[*]} -k pre $domain" \ --post-snapshot-cmd "$SCRIPT_DIR/libvirt-hook ${zfs_autobackup_hooks_args[*]} -k post $domain" \ - "libvirt-${domain}" + "libvirt-${domain}" || rc=$? + + if [ "$rc" -ne 0 ] && grep -qF "No source filesystems selected" "$stderr_file"; then + # Not an error here: the domain simply carries no autobackup property. + # zfs-autobackup's own wording is swallowed on purpose, it reads as a + # failure while nothing failed. + echo "Skipping domain ${domain} because it is not configured for autobackup" >&2 + continue + fi + + cat "$stderr_file" >&2 + if [ "$rc" -ne 0 ]; then + echo "Error: zfs-autobackup failed on domain ${domain} with exit code ${rc}" >&2 + exit "$rc" + fi done