fix(restore): stop containers before restoring, by default - #5553
Open
bettercalln1ck wants to merge 1 commit into
Open
bettercalln1ck wants to merge 1 commit into
bettercalln1ck wants to merge 1 commit into
Conversation
stop_first defaulted to "false", so `ods-restore.sh <id>` rsynced backup data
over data/* while the services owning those files were still running. Postgres,
Qdrant and n8n can hold the files open, write over what was just restored, or
carry stale state forward — and the operator gets no hint, because -f skips the
only prompt.
Two things say false was never the intent: the help text already called -s
"recommended", and the script never restarts anything, so its own closing
advice ("Start services: docker compose up -d") only makes sense if the stack
was stopped.
Default is now stop-first, with --no-stop-containers to opt out (warning
loudly). A stop that fails aborts the restore instead of proceeding
underneath live services, which also means stop_containers now reports
failure rather than only logging a warning.
ods-cli passes its arguments straight through, so it inherits the new default.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #4159.
ods-restore.sh:576defaultedstop_first="false", so the plainods-restore.sh <backup-id>path rsynced backup data overdata/*while the services that own those files were still running. Postgres, Qdrant and n8n can hold those files open, write over what was just restored, or carry stale in-memory state forward on top of it.-fskips the only confirmation prompt, so the scripted path — the one most likely to be automated — got no warning at all.Two things in the script say
falsewas never the intent:-sas "(recommended)".That instruction only makes sense if the stack was stopped — which, by default, it was not. The completion message was written for a path nobody took unless they knew to pass
-s.Changes:
--no-stop-containersrestores the old behaviour for anyone who wants it, and warns that running services may hold open or overwrite the restored files.stop_containerslogged"Some containers may not have stopped cleanly"and carried on into exactly the situation the stop was meant to prevent. It now returns non-zero, anddo_restorerefuses rather than restoring underneath live services — naming the two ways forward (stop by hand, or pass--no-stop-containersto accept the risk).ods-cliforwards its arguments verbatim to this script (ods-cli:4727), so it inherits the new default with no change.AI Assistance
AI-assisted: an AI coding assistant drafted the patch and the regression test and ran the validation recorded below. I reviewed the diff, chose the validation, and am accountable for the change.
Release Lane
release/2.6.xmainStable hotfix reason:
Changed Surface
Risk And Validation
git diff --checkrelease/2.6.xCommands/results:
Operational Change Check
Notes For Reviewers
ods-restore.sh -f <id>in a script will now have its containers stopped where previously they were left running. Given the script already tells the operator to start services afterwards, that seems like the behaviour it always meant to have — but if you would rather keep the default and only add a refusal when containers are detected running, say so and I will flip it round.dockerso it can assert on whethercompose downwas invoked, run the opt-out path, and force a failing stop viaODS_TEST_DOWN_EXIT=1. No Docker required.ods/Makefileand theintegration-smokejob intest-linux.yml. The workflow entry is the one that matters — no workflow invokesmake test, so a Makefile-only entry would not gate.tests/test-restore-safety-ux.shfails identically onmainand on this branch (it builds a fake$ODS_DIRwithoutlib/, soods-restore.shdies sourcinglib/rsync.sh), and it is referenced from neither the Makefile nor any workflow. I raised this in fix(restore): refuse to replace live config from an empty backup #2952 as well; it is still broken and still invisible.