fix: offer the pristine rollback only when the mark actually exists
Both reviewers landed on the same line independently, and they are right. cmd_new's hook-failure path offered `box restore $name pristine` unconditionally, but three mints reach that line with no pristine mark, all by this stack's own design: a `dir` pool (skipped, no CoW), BOX_SNAPSHOT_PRISTINE=0, and a create incus refused (warned, mint continued). On a `dir` host that is EVERY hook failure — so the operator standing at the one moment the message exists for copy-pastes a restore that errors. It could not simply be read off the return status. The never-fatal contract means snapshot_mark returns 0 on all four paths on purpose — taken, skipped, knob-disabled, refused — so `if snapshot_mark ...` cannot distinguish a mark that exists from one that does not, and making it distinguish would put the mint's success at the mercy of a checkpoint, which is what that contract exists to prevent. So the fact is recorded rather than inferred: `marks` accumulates the labels snapshot_mark actually created, and `mark_taken <label>` answers the only question a rollback offer may ask. Per-label, so one mark never answers for another. This is the same refusal the message one line earlier already makes when it declines to invent a `bootstrapped` it never watched — a promise the disk cannot deliver is the failure mode #130 was built to refuse. It just was not applied to the restore offer sitting under it. Pinned per path rather than once, because the three no-mark paths fail differently and a single case would let the other two regress silently: five drives over snapshot_mark + mark_taken (created / dir-skip / knob / refused / no bleed between labels) and a static check that the call site is gated. Verified by mutation — dropping the recording, and un-gating the offer, each fail the suite rather than passing quietly.
This commit is contained in:
parent
050c90966e
commit
4c595eb945
2 changed files with 76 additions and 2 deletions
24
bin/box
24
bin/box
|
|
@ -9,6 +9,12 @@ root="$(cd "$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")/.." && pwd)"
|
||||||
remote=""; mode="auto"; name=""; from=""; template=""; force=0; json=0; want_help=0
|
remote=""; mode="auto"; name=""; from=""; template=""; force=0; json=0; want_help=0
|
||||||
cpu=""; memory=""; disk=""; instance_only=0
|
cpu=""; memory=""; disk=""; instance_only=0
|
||||||
inst="" # the resolved Incus instance, set by the 'box' precondition
|
inst="" # the resolved Incus instance, set by the 'box' precondition
|
||||||
|
# Labels snapshot_mark actually CREATED this run, space-separated. Every path
|
||||||
|
# through snapshot_mark returns 0 on purpose — the never-fatal contract — so
|
||||||
|
# the exit status cannot tell a mark that was taken from one that was skipped
|
||||||
|
# (dir pool, knob=0) or refused (incus said no). Anything that offers the
|
||||||
|
# operator a mark must ask this, not assume the call happened.
|
||||||
|
marks=""
|
||||||
|
|
||||||
die() { echo "box: $*" >&2; exit 1; } # 1 = it went wrong
|
die() { echo "box: $*" >&2; exit 1; } # 1 = it went wrong
|
||||||
usage_error() { echo "box: $*" >&2; echo "try 'box help'." >&2; exit 2; } # 2 = you asked wrong
|
usage_error() { echo "box: $*" >&2; echo "try 'box help'." >&2; exit 2; } # 2 = you asked wrong
|
||||||
|
|
@ -1216,9 +1222,15 @@ snapshot_mark() {
|
||||||
echo "box: the box is fine; it just has no $label mark. The mint continues." >&2
|
echo "box: the box is fine; it just has no $label mark. The mint continues." >&2
|
||||||
return 0
|
return 0
|
||||||
fi
|
fi
|
||||||
|
marks="$marks $label"
|
||||||
echo "box: roll back to it any time with: box restore $name $label"
|
echo "box: roll back to it any time with: box restore $name $label"
|
||||||
}
|
}
|
||||||
|
|
||||||
|
# Did THIS run actually create <label>? The only honest source for a message
|
||||||
|
# that offers a rollback: see 'marks' above for why the return status cannot
|
||||||
|
# answer it.
|
||||||
|
mark_taken() { case " $marks " in *" $1 "*) return 0 ;; *) return 1 ;; esac; }
|
||||||
|
|
||||||
# Take the 'pristine' snapshot, or say loudly why not. Default ON: the value
|
# Take the 'pristine' snapshot, or say loudly why not. Default ON: the value
|
||||||
# of this mark only exists if it is already there on the bad day, and nobody
|
# of this mark only exists if it is already there on the bad day, and nobody
|
||||||
# takes it by hand at the one moment it is true. The escape hatch is the
|
# takes it by hand at the one moment it is true. The escape hatch is the
|
||||||
|
|
@ -1557,7 +1569,17 @@ cmd_new() {
|
||||||
echo " and a by-hand re-run happens in a shell it does not watch. Take it yourself once" >&2
|
echo " and a by-hand re-run happens in a shell it does not watch. Take it yourself once" >&2
|
||||||
echo " the role converges, at the moment it is true:" >&2
|
echo " the role converges, at the moment it is true:" >&2
|
||||||
echo " box snapshot $name bootstrapped" >&2
|
echo " box snapshot $name bootstrapped" >&2
|
||||||
echo "box: 'box restore $name pristine' is still there if you would rather start the role over." >&2
|
# Only offer the rollback that EXISTS. Three mints reach this line
|
||||||
|
# with no pristine mark, all by this stack's own design: a 'dir' pool
|
||||||
|
# (skipped), BOX_SNAPSHOT_PRISTINE=0, and a refused create (warned,
|
||||||
|
# mint continued). On a 'dir' host that is EVERY hook failure, and an
|
||||||
|
# operator standing at the one moment this message exists for would
|
||||||
|
# copy-paste a restore that errors. Offering a mark the disk does not
|
||||||
|
# have is the same lie this PR refuses one message earlier, when it
|
||||||
|
# declines to invent a 'bootstrapped' it never watched.
|
||||||
|
if mark_taken pristine; then
|
||||||
|
echo "box: 'box restore $name pristine' is still there if you would rather start the role over." >&2
|
||||||
|
fi
|
||||||
die "the tenant role did not converge — the box is incomplete, so refusing to call it ready"
|
die "the tenant role did not converge — the box is incomplete, so refusing to call it ready"
|
||||||
fi
|
fi
|
||||||
# The hook RAN and box WATCHED it succeed — so right here the box is
|
# The hook RAN and box WATCHED it succeed — so right here the box is
|
||||||
|
|
|
||||||
54
test/cli.sh
54
test/cli.sh
|
|
@ -698,7 +698,7 @@ check "bootstrapped: exactly one auto-mark policy — 'incus snapshot create' tw
|
||||||
|
|
||||||
# The policy half, DRIVEN. Same stub shape as pris() above, one label over.
|
# The policy half, DRIVEN. Same stub shape as pris() above, one label over.
|
||||||
BOOTFN="$(mktemp)"
|
BOOTFN="$(mktemp)"
|
||||||
awk '/^storage_driver\(\) \{/,/^\}/;/^snapshot_mark\(\) \{/,/^\}/;/^snapshot_bootstrapped\(\) \{/,/^\}/' \
|
awk '/^storage_driver\(\) \{/,/^\}/;/^snapshot_mark\(\) \{/,/^\}/;/^mark_taken\(\)/;/^snapshot_bootstrapped\(\) \{/,/^\}/' \
|
||||||
"$ROOT/bin/box" > "$BOOTFN"
|
"$ROOT/bin/box" > "$BOOTFN"
|
||||||
check "bootstrapped: the functions extracted from bin/box (guards the awk)" 0 "snapshot_bootstrapped" cat "$BOOTFN"
|
check "bootstrapped: the functions extracted from bin/box (guards the awk)" 0 "snapshot_bootstrapped" cat "$BOOTFN"
|
||||||
check "bootstrapped: the extracted functions are valid bash" 0 "" bash -n "$BOOTFN"
|
check "bootstrapped: the extracted functions are valid bash" 0 "" bash -n "$BOOTFN"
|
||||||
|
|
@ -755,6 +755,58 @@ check "bootstrapped: the opt-out is a per-label knob — PRISTINE=0 does not sil
|
||||||
boot_took_mark btrfs inst-x BOX_SNAPSHOT_PRISTINE=0
|
boot_took_mark btrfs inst-x BOX_SNAPSHOT_PRISTINE=0
|
||||||
check "bootstrapped: a failed snapshot warns and returns 0 (never fails a good mint)" 0 "WARNING" \
|
check "bootstrapped: a failed snapshot warns and returns 0 (never fails a good mint)" 0 "WARNING" \
|
||||||
boot btrfs fail-x
|
boot btrfs fail-x
|
||||||
|
|
||||||
|
# --- only offer a rollback that EXISTS -------------------------------------
|
||||||
|
# The never-fatal contract means snapshot_mark returns 0 whether it took the
|
||||||
|
# mark, skipped it, or was refused — so the exit status cannot answer "is
|
||||||
|
# there something to restore?" and any message offering one must ask 'marks'.
|
||||||
|
# Driven per path rather than asserted once: the three no-mark paths fail
|
||||||
|
# differently and a single case would let the other two regress silently.
|
||||||
|
took() { # took <driver> <label> [VAR=VAL...] — did THIS run create the mark?
|
||||||
|
local driver="$1" label="$2"; shift 2
|
||||||
|
# shellcheck disable=SC2016 # the body is the stub's source, expanded by the
|
||||||
|
# inner bash from the environment 'env' sets up — never by this shell.
|
||||||
|
env "$@" DRIVER="$driver" LABEL="$label" BOOTFN="$BOOTFN" bash -c '
|
||||||
|
incus() {
|
||||||
|
case "$*" in
|
||||||
|
"profile device get box-net root pool") printf "boxpool\n" ;;
|
||||||
|
"storage show boxpool")
|
||||||
|
[ "$DRIVER" = none ] && return 1
|
||||||
|
printf "name: boxpool\ndriver: %s\n" "$DRIVER" ;;
|
||||||
|
"storage list --format csv")
|
||||||
|
[ "$DRIVER" = none ] && return 1
|
||||||
|
printf "boxpool,%s,,0,CREATED\n" "$DRIVER" ;;
|
||||||
|
"snapshot create fail-x "*) return 1 ;;
|
||||||
|
"snapshot create "*) printf "STUB: snapshot created\n" ;;
|
||||||
|
*) return 1 ;;
|
||||||
|
esac
|
||||||
|
}
|
||||||
|
marks=""
|
||||||
|
. "$BOOTFN"
|
||||||
|
snapshot_mark "$INSTANCE_X" boxname "$LABEL" "${ENABLED:-1}" "some state" >/dev/null 2>&1
|
||||||
|
mark_taken "$LABEL" && echo TAKEN || echo ABSENT
|
||||||
|
' 2>&1
|
||||||
|
}
|
||||||
|
check "rollback: a mark that WAS created is remembered" 0 "TAKEN" \
|
||||||
|
took btrfs pristine INSTANCE_X=inst-x
|
||||||
|
check "rollback: a 'dir' skip is NOT remembered (no CoW, no mark)" 0 "ABSENT" \
|
||||||
|
took dir pristine INSTANCE_X=inst-x
|
||||||
|
check "rollback: the opt-out knob is NOT remembered" 0 "ABSENT" \
|
||||||
|
took btrfs pristine INSTANCE_X=inst-x ENABLED=0
|
||||||
|
check "rollback: a REFUSED create is not remembered (incus said no)" 0 "ABSENT" \
|
||||||
|
took btrfs pristine INSTANCE_X=fail-x
|
||||||
|
# One label's mark must not answer for another's.
|
||||||
|
check "rollback: marks do not bleed between labels" 0 "ABSENT" \
|
||||||
|
bash -c 'marks=" bootstrapped "; . "'"$BOOTFN"'"; mark_taken pristine && echo TAKEN || echo ABSENT'
|
||||||
|
# The call site itself, pinned statically: the hook-failure message offers the
|
||||||
|
# restore only under the guard. On a 'dir' host EVERY hook failure reaches this
|
||||||
|
# line with no pristine mark, so an unconditional offer is a copy-pasteable
|
||||||
|
# command that errors at the one moment the operator is standing there.
|
||||||
|
# shellcheck disable=SC2016 # the $-strings are literals in the target file
|
||||||
|
check "rollback: the hook-failure restore offer is GATED on the mark existing" 0 "" bash -c '
|
||||||
|
awk "/^cmd_new\(\) \{/,/^\}/" "'"$ROOT"'/bin/box" \
|
||||||
|
| grep -B12 "box restore \$name pristine. is still there" \
|
||||||
|
| grep -q "if mark_taken pristine; then"'
|
||||||
rm -f "$BOOTFN"
|
rm -f "$BOOTFN"
|
||||||
|
|
||||||
# The label is one-directional and the docs must say so: its PRESENCE means the
|
# The label is one-directional and the docs must say so: its PRESENCE means the
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue