fix(snapshot): refuse standalone restore while vcluster.service is active#4083
fix(snapshot): refuse standalone restore while vcluster.service is active#4083rlmcpherson wants to merge 1 commit into
Conversation
…tive
The server binary's `restore` subcommand rewrites the backing store and must
never run while the standalone control plane is up: it renames the live
kine/etcd data out from under the running server and races the unit's
Restart=always respawn loop. The vcluster CLI ("vcluster restore
--standalone") stops and restarts the service around this command, but a
direct invocation had no protection.
Add standalone.IsServiceActive (systemctl is-active probe that reports
not-active wherever systemd cannot answer affirmatively, so the in-pod
restore path is unaffected) and refuse the restore with a clear error
pointing at the CLI when the unit is active. Document the contract in both
commands' help text.
Verified by a vcluster-pro e2e spec that snapshots a running standalone,
invokes the binary restore directly, and asserts the refusal plus an
untouched datastore (ENGCP-1022).
E2E Ginkgo Tests
|
| // invocations that skipped that envelope. On hosts without systemd | ||
| // (e.g. the in-pod restore path) this reports not-active and the | ||
| // restore proceeds. | ||
| if standaloneutil.IsServiceActive() { |
There was a problem hiding this comment.
blocking — [coverage gap] This guard (lines 41–48) is the fix's core in-code protection, but its wiring is untested in this repo. TestIsServiceActive exercises only the unexported isServiceActive(run) helper; no test reaches this RunE handler. So a negated condition (if !standaloneutil.IsServiceActive()), a bad import alias, or a refactor that drops the branch would ship on a green build and silently re-open the corruption this PR closes (a restore racing the live kine/etcd while the unit's Restart=always respawns mid-restore).
The red-first e2e in vcluster-pro and the helper unit test both help, but neither gates this repo's CI — a regression here is only caught after a downstream dependency bump.
Consider making the probe injectable and adding a small unit test on the wiring:
// package-level seam
var isServiceActive = standaloneutil.IsServiceActivethen a table/fake test that stubs it true and asserts RunE returns the refusal error (naming the service + remediation), plus a false case that proceeds past the guard. This matches the repo's standing expectation that new snapshot/restore branch logic carries a table/fake-client unit test.
| return isServiceActive(defaultSystemctlRunner) | ||
| } | ||
|
|
||
| func isServiceActive(run systemctlRunner) bool { |
There was a problem hiding this comment.
consider (adjacent, out of diff) — This new isServiceActive is a clean, reusable probe, but a near-duplicate inline check already exists at pkg/cli/find/find.go:837:
out, err := exec.Command("systemctl", "is-active", constants.VClusterStandaloneSystemdServiceName).Output()
if err == nil && strings.TrimSpace(string(out)) == "active" { status = StatusRunning }find.go already imports standaloneutil, so that call site could adopt standaloneutil.IsServiceActive() and leave a single implementation of the is-active probe. Follow-up only — it's outside this PR's diff, so no change required here.
2 participants
Summary
restoresubcommand now refuses to run while the standalonevcluster.serviceunit is active, with an error pointing at the supported entry point (vcluster restore --standalone, which stops and restarts the service around the restore).standalone.IsServiceActive()probe (systemctl is-active --quiet): reports not-active wherever systemd cannot answer affirmatively, so the in-pod restore path and non-systemd hosts are unaffected (fail-open by design; the guard only ever refuses on an affirmative "active").restoreis an internal interface that must run with the control plane down; the CLI owns the service lifecycle on standalone hosts.Why
A direct binary restore against a live standalone had no protection and demonstrably corrupts: exercised on an active control plane, the restore renamed the live
state.db(and-wal/-shm) out from under the running server, started a second kine on the same socket, and was SIGKILLed mid-restore while racing the unit'sRestart=alwaysrespawn loop. The CLI envelope (restoreStandaloneVCluster) is the intended design (ENGCP-975, resolved as "intended: CLI-managed"); this guard closes the direct-invocation gap it leaves.Test plan
TestIsServiceActivecovers the probe (active on zero exit, not-active on any failure, exact systemctl args);go test ./pkg/util/standalone/green.restoredirectly, and asserts the refusal message, an unchanged MainPID (no restart, no flapping), and an untouched datastore. Failed against the unguarded binary with the corruption behavior above; passes with this change. A second spec drivesvcluster restore --standaloneend-to-end (failure and success paths) and confirms the CLI envelope is unaffected by the guard. Both land in vcluster-pro together with a dependency bump once this merges.Part of ENGCP-1022 (the e2e specs and the vcluster bump in vcluster-pro complete it).
🤖 Generated with Claude Code
Note
Medium Risk
Touches the snapshot restore path and standalone control-plane lifecycle; incorrect guard behavior could block legitimate restores, though the fail-open design limits impact outside an affirmatively active systemd unit.
Overview
Adds a safety gate on the internal server
restorecommand so it refuses to run when the standalonevcluster.serviceunit is active, with an error that points users tovcluster restore --standaloneorsystemctl stop.Introduces
standalone.IsServiceActive(), which usessystemctl is-active --quieton Linux and treats any non-success as not active (so in-pod / non-systemd restores are unchanged). Help text on both the serverrestoreandvclusterctl restoredocuments that standalone restores must go through the CLI lifecycle envelope.Unit tests cover the
is-activeprobe arguments and success/failure behavior.Reviewed by Cursor Bugbot for commit 0cc2998. Bugbot is set up for automated code reviews on this repo. Configure here.