-
Notifications
You must be signed in to change notification settings - Fork 587
fix(snapshot): refuse standalone restore while vcluster.service is active #4083
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,25 @@ func NewServiceManager() (ServiceManager, error) { | |
| return newServiceManager(defaultSystemctlRunner) | ||
| } | ||
|
|
||
| // IsServiceActive reports whether the standalone vCluster systemd unit is | ||
| // currently active on this host. It reports false wherever systemd cannot | ||
| // answer affirmatively (non-Linux, no systemctl binary, systemd not running, | ||
| // unit stopped or not installed), so callers can use it as a guard that only | ||
| // engages on a standalone host with a running control plane. | ||
| func IsServiceActive() bool { | ||
| if runtime.GOOS != "linux" { | ||
| return false | ||
| } | ||
| return isServiceActive(defaultSystemctlRunner) | ||
| } | ||
|
|
||
| func isServiceActive(run systemctlRunner) bool { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider (adjacent, out of diff) — This new out, err := exec.Command("systemctl", "is-active", constants.VClusterStandaloneSystemdServiceName).Output()
if err == nil && strings.TrimSpace(string(out)) == "active" { status = StatusRunning }
|
||
| // "systemctl is-active" exits zero only when the unit is active; a missing | ||
| // systemctl binary, an unreachable systemd, a stopped unit, and an unknown | ||
| // unit all exit non-zero and therefore report not-active. | ||
| return run("is-active", "--quiet", constants.VClusterStandaloneSystemdServiceName) == nil | ||
| } | ||
|
|
||
| func newServiceManager(run systemctlRunner) (ServiceManager, error) { | ||
| if runtime.GOOS != "linux" { | ||
| return nil, fmt.Errorf("systemd manager is only supported on Linux (current OS: %s)", runtime.GOOS) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blocking — [coverage gap] This guard (lines 41–48) is the fix's core in-code protection, but its wiring is untested in this repo.
TestIsServiceActiveexercises only the unexportedisServiceActive(run)helper; no test reaches thisRunEhandler. 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'sRestart=alwaysrespawns 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:
then a table/fake test that stubs it
trueand assertsRunEreturns the refusal error (naming the service + remediation), plus afalsecase 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.