diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 22359ef..134b767 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -64,6 +64,8 @@ type Outcome struct { scope, user string // found is, for a service, its unit as the host first found it (novox/hq ADR 0118). found *store.FoundUnit + // declared is, for a service with a state, what this apply held its unit to (novox/hq ADR 0269). + declared *store.FoundUnit // shell is, for a user, the login shell it was found with and the one the mesh set (novox/hq // ADR 0176 §2, issue 228). shell *store.LoginShell @@ -736,6 +738,7 @@ func ApplyMindingWindows( Scope: outcome.scope, User: outcome.user, Found: outcome.found, + Declared: outcome.declared, Shell: outcome.shell, Linger: outcome.linger, Groups: outcome.groups, @@ -1533,6 +1536,10 @@ func applyServiceIn(ctx context.Context, sys system.System, r *declaration.Servi } } + // What the mesh holds the unit to, recorded with what was found, so its removal can tell the mesh's + // own stopping from anybody else's (novox/hq ADR 0269). + out.declared = &store.FoundUnit{Unit: r.Unit, State: r.State, Boot: r.Boot} + before, err := sys.ServiceState(ctx, run, r.Unit) if err != nil { return out, err @@ -2771,9 +2778,11 @@ func declaredDigest(r declaration.Resource) string { // disabled at boot, as a process's unit is, before its file goes: orphans are removed newest // first, and a unit's file is declared before the service that starts it. // -// **Never started on the way out.** A unit the mesh stopped is not started again when its -// declaration goes: starting something is a decision, and the operator makes it. The report says -// what was actually done — a unit already as it was found is forgotten, not "restored". +// **Never started on the way out — but for what the mesh's own declaration stopped.** A unit the mesh +// stopped is not started again when its declaration goes: starting something is a decision, and the +// operator makes it. The exception is a unit found running or enabled that the declaration itself held +// stopped or disabled: that is the mesh's doing, and it is undone (novox/hq ADR 0269, startUnitBack). The +// report says what was actually done — a unit already as it was found is forgotten, not "restored". func removeService(ctx context.Context, sys system.System, a store.Applied, run Runner, made bool) (string, string, error) { if a.Stateless { @@ -2832,6 +2841,20 @@ func giveUnitBack(ctx context.Context, sys system.System, a store.Applied, run R stop := made || a.Found.State == "stopped" disable := made || a.Found.Boot == "disabled" + // **What the mesh's own declaration stopped is started again** (novox/hq ADR 0269, narrowing ADR + // 0118's "never started on the way out"): a unit found running that the mesh held stopped, and one + // found enabled that it held disabled. Only the mesh's declaration did that, and leaving it so is the + // mesh's doing outliving the mesh — a pool whose distribution scrub timer a module stopped would have + // no schedule at all once the module went. A unit stopped for any other reason is not known to have + // been stopped by the mesh, and is left as before. + if !made && a.Declared != nil && (a.Declared.Unit == "" || a.Declared.Unit == a.Target) { + start := a.Found.State == "running" && a.Declared.State == "stopped" + enable := a.Found.Boot == "enabled" && a.Declared.Boot == "disabled" + if start || enable { + return startUnitBack(ctx, sys, a, run, start, enable) + } + } + if !stop && !disable { // Found running, and not disabled by anyone but the mesh: nothing to give back. What the // mesh did since is said, so a unit left stopped is not reported as the machine's doing — @@ -2907,6 +2930,73 @@ func giveUnitBack(ctx context.Context, sys system.System, a store.Applied, run R return "restored", detail, nil } +// startUnitBack gives back a unit the mesh's declaration stopped or disabled, found running or enabled +// (novox/hq ADR 0269). +// +// **Only a unit exactly as the mesh held it.** A masked unit is somebody's word that it must not run, and +// one started or enabled since the mesh held it is somebody's doing: both are left as they are, and the +// report says why. **A restore that fails waits**: the start refused, or the unit not running after it, +// puts back what this attempt changed — so the next apply finds the unit as the mesh held it and tries +// again — and is reported as waiting, never as an error that would make an apply fatal. +func startUnitBack(ctx context.Context, sys system.System, a store.Applied, run Runner, start, enable bool) (string, string, error) { + state, serr := sys.ServiceState(ctx, run, a.Target) + boot, berr := sys.ServiceBoot(ctx, run, a.Target) + for _, err := range []error{serr, berr} { + switch { + case err == nil: + case strings.Contains(err.Error(), "does not exist on this machine"): + return "forgotten", "the unit no longer exists", nil + case strings.Contains(err.Error(), "masked"): + return "forgotten", a.Target + " is masked, which is somebody's word that it must not run; left as it is", nil + default: + return "", "", fmt.Errorf("%w: %s could not be read to give it back (%v); tried again by the next apply", + errRemovalWaits, a.Target, err) + } + } + if state != a.Declared.State || (a.Declared.Boot != "" && boot != a.Declared.Boot) { + return "forgotten", fmt.Sprintf("it is %s and %s at boot, not the %s and %s the mesh held it to: somebody "+ + "changed it since, so it is left as it is", state, boot, a.Declared.State, orUnset(a.Declared.Boot)), nil + } + var did []string + if enable { + if err := sys.SetServiceBoot(ctx, run, a.Target, "enabled"); err != nil { + return "", "", fmt.Errorf("%w: enabling %s again failed (%v); tried again by the next apply", errRemovalWaits, a.Target, err) + } + did = append(did, "enabled at boot") + } + if start { + why := "" + if err := sys.SetServiceState(ctx, run, a.Target, "running"); err != nil { + why = "its start failed: " + err.Error() + } else if after, err := sys.ServiceState(ctx, run, a.Target); err != nil { + why = "it could not be read after its start: " + err.Error() + } else if after != "running" { + why = "it is " + after + " after its start, not running" + } + if why != "" { + undo := "" + if enable { + if err := sys.SetServiceBoot(ctx, run, a.Target, "disabled"); err != nil { + undo = "; it stays enabled at boot, its disabling again failed: " + err.Error() + } else { + undo = "; disabled at boot again, as the mesh held it, until then" + } + } + return "", "", fmt.Errorf("%w: giving %s back running, as the host found it: %s%s; tried again by the next apply", + errRemovalWaits, a.Target, why, undo) + } + did = append(did, "started") + } + return "restored", strings.Join(did, " and ") + " again, as the host found it before the mesh's declaration stopped it", nil +} + +func orUnset(s string) string { + if s == "" { + return "as it was at boot" + } + return s +} + // foundAs is what a service's unit was before the mesh touched it, as far as this host can know: // what an earlier apply recorded, or — the first time the mesh is about to act on the unit — what // is there now (novox/hq ADR 0118). Nil when it cannot be known: a record written before the host @@ -2937,7 +3027,7 @@ func foundAs(ctx context.Context, sys system.System, r *declaration.Service, run // Given back as if undeclared, never as the mesh's own: whether the mesh wrote the old // unit's file is known to the removal of orphans, and that file's own record goes with it. action, detail, err := removeService(ctx, sys, - store.Applied{Type: string(declaration.TypeService), Target: unit, Found: f, + store.Applied{Type: string(declaration.TypeService), Target: unit, Found: f, Declared: previous.Declared, Scope: previous.Scope, User: previous.User}, run, false) switch { case errors.Is(err, errRemovalWaits): diff --git a/internal/apply/apply_test.go b/internal/apply/apply_test.go index b5a11d4..35f27eb 100644 --- a/internal/apply/apply_test.go +++ b/internal/apply/apply_test.go @@ -313,6 +313,7 @@ func TestAServiceIsReadBackNotAssumed(t *testing.T) { // running. A unit that starts and immediately dies satisfies the command. started := false run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if args[0] == "show" { if started { return "LoadState=loaded\nActiveState=failed\n", nil // started, then died @@ -337,6 +338,7 @@ func TestAServiceIsReadBackNotAssumed(t *testing.T) { func TestAnUnknownServiceStateIsRefusedNotGuessed(t *testing.T) { run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) return "LoadState=loaded\nActiveState=reticent\n", nil } d := parse(t, `{"declaration":1,"resources":[ @@ -371,6 +373,7 @@ func TestADroppedServiceIsGivenBackTheStateItWasFoundIn(t *testing.T) { t.Run(c.name, func(t *testing.T) { var commands []string run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) commands = append(commands, strings.Join(args, " ")) if args[0] == "show" { return "LoadState=loaded\nActiveState=active\n", nil @@ -413,6 +416,7 @@ func TestAServiceRecordsTheStateItWasFoundInOnceAndCarriesIt(t *testing.T) { active := "inactive" enabled := "disabled" run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch args[0] { case "show": return "LoadState=loaded\nActiveState=" + active + "\n", nil @@ -464,6 +468,7 @@ func TestAUnitThatDoesNotExistIsNotStopped(t *testing.T) { // Absence read as satisfaction — 04-ISSUES/007 wearing a different hat, and the mirror of // the degraded-init bug the capability detector had. absent := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) return "LoadState=not-found\nActiveState=inactive\n", nil } d := parse(t, `{"declaration":1,"resources":[ @@ -486,6 +491,7 @@ func TestAMaskedUnitIsRefused(t *testing.T) { // Masked means someone deliberately made it unstartable. Applying over that would undo a // decision the host did not make and cannot see the reason for. masked := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) return "LoadState=masked\nActiveState=inactive\n", nil } d := parse(t, `{"declaration":1,"resources":[ @@ -506,6 +512,7 @@ func TestForgettingAUnitThatIsGoneDoesNotStrandTheNode(t *testing.T) { // already true. var stopped bool run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if args[0] == "show" { return "LoadState=not-found\nActiveState=inactive\n", nil } @@ -557,6 +564,7 @@ func TestABrokenPackageDatabaseIsNotReadAsNotInstalled(t *testing.T) { // answer would silently reinstall on a machine whose package manager is broken, or report // "installed nothing" as success. The apply must fail instead. run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) return "", errors.New("pacman: error: could not lock database") } d := parseTrusted(t, `{"declaration":1,"resources":[ @@ -575,6 +583,7 @@ func TestABrokenPackageDatabaseIsNotReadAsNotInstalled(t *testing.T) { func TestAnInstalledPackageIsNotReinstalled(t *testing.T) { var installed bool run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if args[0] == "-S" { installed = true } @@ -602,6 +611,7 @@ func TestAPackageIsNeverUninstalled(t *testing.T) { // machine may have had it before the mesh ever saw it. Undeclaring is not "remove it". var uninstalled bool run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if len(args) > 0 && (args[0] == "-R" || args[0] == "-Rs") { uninstalled = true } @@ -635,6 +645,7 @@ func TestAnActionThatIsAlreadyTrueDoesNotRun(t *testing.T) { // database is, so "is it already there" is a question only the declaration can ask. var ran bool run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name == "create-db" { ran = true } @@ -661,6 +672,7 @@ func TestAnActionThatSucceedsAndDoesNothingFails(t *testing.T) { // this repository's most expensive failure shape. Here the command "succeeds" every time // and verify never passes. run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name == "has-db" { return "", errors.New("no such database") } @@ -687,6 +699,7 @@ func TestAnActionRunsInsideTheContainerItNames(t *testing.T) { // there is any mesh to ask. var sawExec bool run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name == "docker" && args[0] == "exec" && args[1] == "store" { sawExec = true return "", nil @@ -710,6 +723,7 @@ func TestAContainerThatExitsImmediatelyFailsTheApply(t *testing.T) { // still running. A container whose entrypoint dies satisfies the command exactly as one // that came up does — which is the read-back rule, in the place it matters most. run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch { case args[0] == "info": return "27.0\n", nil @@ -748,6 +762,7 @@ func TestAContainerWhoseDeclarationChangedIsReplaced(t *testing.T) { var removed, created bool run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch args[0] { case "info": return "27.0\n", nil @@ -786,6 +801,7 @@ func TestAContainerThatMatchesIsLeftAlone(t *testing.T) { var touched bool run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch args[0] { case "info": return "27.0\n", nil @@ -831,6 +847,7 @@ func TestAContainerIsRecreatedWhenARestartOnResourceChanged(t *testing.T) { var removed, created bool run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch args[0] { case "info": return "27.0\n", nil @@ -886,6 +903,7 @@ func TestRestartOnFiresOnlyForResourcesThatChanged(t *testing.T) { func systemctlStub(t *testing.T, load, active, enabled string, verbs *[]string) Runner { t.Helper() return func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch args[0] { case "show": return "LoadState=" + load + "\nActiveState=" + active + "\n", nil @@ -1043,6 +1061,7 @@ func TestTheRuntimeProbeIsPerRuntime(t *testing.T) { t.Run(tc.name, func(t *testing.T) { var probedWith string run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name != tc.present { return "", errors.New("not installed") } @@ -1071,6 +1090,7 @@ func TestAContainerUsesTheRuntimeTheMachineHas(t *testing.T) { // machine already has (novox/hq research 012), so hardcoding one contradicts it. var calledWith []string run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name == "docker" { return "", errors.New("not installed") } @@ -1107,6 +1127,7 @@ func TestNoRuntimeIsSaidPlainly(t *testing.T) { // Naming what was tried, because "docker: command not found" on a machine that deliberately // runs podman sends the reader looking for the wrong thing. run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) return "", errors.New("not installed") } d := parseTrusted(t, `{"declaration":1,"resources":[ @@ -1250,6 +1271,7 @@ func TestAServiceIsNotRestartedByAChangeItDoesNotName(t *testing.T) { // before and after one. func recordingServices(commands *[]string) Runner { return func(_ context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) line := name + " " + strings.Join(args, " ") *commands = append(*commands, line) switch { @@ -1398,6 +1420,7 @@ func TestAFailedActionStopsWhatFollows(t *testing.T) { func TestAContainerIsGivenTheMeshsNames(t *testing.T) { var ran []string run := func(_ context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name != "docker" { return "", errors.New("not installed") } @@ -1433,6 +1456,7 @@ func TestAContainerIsGivenTheMeshsNames(t *testing.T) { func TestAContainerGivenNoNamesIsRunAsBefore(t *testing.T) { var ran []string run := func(_ context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name != "docker" { return "", errors.New("not installed") } @@ -1477,6 +1501,7 @@ func TestResourcesAreAppliedInTheOrderTheyWereDeclared(t *testing.T) { var order []string done := map[string]bool{} run := func(_ context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) order = append(order, name+" "+strings.Join(args, " ")) // `verify` is the idempotency check, so it has to fail before the step and pass after — // a stub that always succeeds means every action is already done and nothing ever runs, @@ -1627,6 +1652,7 @@ func TestAContainerStaleFromAnEarlierApplyIsReplaced(t *testing.T) { var removed, created bool run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch args[0] { case "info": return "27.0\n", nil diff --git a/internal/apply/file_handover_test.go b/internal/apply/file_handover_test.go index 37c9f51..58a392c 100644 --- a/internal/apply/file_handover_test.go +++ b/internal/apply/file_handover_test.go @@ -43,6 +43,7 @@ func watching(t *testing.T, path string) Runner { var commands []string services := recordingServices(&commands) return func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if _, err := os.Stat(path); err != nil { t.Errorf("the file was not there while %s %s ran: %v", name, strings.Join(args, " "), err) } diff --git a/internal/apply/former_archive_test.go b/internal/apply/former_archive_test.go index 16d9765..8643634 100644 --- a/internal/apply/former_archive_test.go +++ b/internal/apply/former_archive_test.go @@ -15,6 +15,7 @@ import ( // starts (ADR 0141). func TestTheHostsOwnFormerArchiveIsLeftInPlaceNotFatal(t *testing.T) { run := func(_ context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name == "docker" && args[0] == "info" { return "29.0.0\n", nil } diff --git a/internal/apply/found_test.go b/internal/apply/found_test.go index 047436c..f46fe57 100644 --- a/internal/apply/found_test.go +++ b/internal/apply/found_test.go @@ -59,6 +59,7 @@ func TestAUnitWhoseFileTheMeshWroteIsStoppedWhateverItsRecordSays(t *testing.T) m := unitsMachine(map[string]*fakeUnit{"mesh-guard.service": {active: "active", enabled: "enabled"}}) fileThereAtStop := false run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name == "systemctl" && args[0] == "stop" { _, err := os.Stat(unitFile) fileThereAtStop = err == nil diff --git a/internal/apply/giveback_test.go b/internal/apply/giveback_test.go new file mode 100644 index 0000000..94c5a52 --- /dev/null +++ b/internal/apply/giveback_test.go @@ -0,0 +1,171 @@ +package apply + +import ( + "strings" + "testing" + + "github.com/novox/mesh-host/internal/store" +) + +// A unit the mesh found running and enabled, and held stopped and disabled by its own declaration, is +// given back running and enabled when that declaration goes (the zfs module holds the distribution's +// scrub timer stopped while its own timer scrubs the pool; unassigning it must not leave the pool with no +// schedule at all). The report says so. +func TestAUnitTheMeshStoppedAndDisabledIsGivenBackRunningAndEnabled(t *testing.T) { + m := unitsMachine(map[string]*fakeUnit{ + "zfs-scrub-weekly@storage.timer": {active: "active", enabled: "enabled"}, + }) + _, state, err := applyOn(t, `{"declaration":1,"resources":[ + {"id":"zfs.distribution-weekly","type":"service","unit":"zfs-scrub-weekly@storage.timer","state":"stopped","boot":"disabled"}]}`, + store.State{}, m) + if err != nil { + t.Fatal(err) + } + if u := m.units["zfs-scrub-weekly@storage.timer"]; u.active != "inactive" || u.enabled != "disabled" { + t.Fatalf("the fixture did not stop and disable it: %s and %s", u.active, u.enabled) + } + report, _, err := applyOn(t, nothingButA(t), copyOf(t, state), m) + if err != nil { + t.Fatal(err) + } + if u := m.units["zfs-scrub-weekly@storage.timer"]; u.active != "active" || u.enabled != "enabled" { + t.Errorf("given back as %s and %s; it was found running and enabled", u.active, u.enabled) + } + said := "" + for _, r := range report.Outcomes { + if r.ID == "zfs.distribution-weekly" { + said = r.Action + ": " + r.Detail + } + } + if !strings.HasPrefix(said, "restored: ") || !strings.Contains(said, "started") || !strings.Contains(said, "enabled at boot") { + t.Errorf("the report says %q", said) + } +} + +// Only what the mesh's own declaration did is undone: a unit the mesh held stopped that was found stopped +// stays stopped, and one found running that the mesh never stopped is left running as before. +func TestOnlyWhatTheMeshsDeclarationStoppedIsStartedAgain(t *testing.T) { + m := unitsMachine(map[string]*fakeUnit{ + "found-stopped.timer": {active: "inactive", enabled: "disabled"}, + "left-running.timer": {active: "active", enabled: "enabled"}, + }) + _, state, err := applyOn(t, `{"declaration":1,"resources":[ + {"id":"a","type":"service","unit":"found-stopped.timer","state":"stopped","boot":"disabled"}, + {"id":"b","type":"service","unit":"left-running.timer","state":"running","boot":"enabled"}]}`, + store.State{}, m) + if err != nil { + t.Fatal(err) + } + if _, _, err := applyOn(t, nothingButA(t), state, m); err != nil { + t.Fatal(err) + } + if u := m.units["found-stopped.timer"]; u.active != "inactive" || u.enabled != "disabled" { + t.Errorf("found stopped and disabled, given back as %s and %s", u.active, u.enabled) + } + if u := m.units["left-running.timer"]; u.active != "active" || u.enabled != "enabled" { + t.Errorf("found running, given back as %s and %s", u.active, u.enabled) + } +} + +// A record from before the host kept what it declared says nothing about who stopped the unit: it is +// left as it is, never started. +func TestARecordWithoutWhatWasDeclaredStartsNothing(t *testing.T) { + m := unitsMachine(map[string]*fakeUnit{"old.timer": {active: "inactive", enabled: "disabled"}}) + state := store.State{} + state.Record(store.Applied{ID: "old", Type: "service", Target: "old.timer", Origin: store.OriginDeclared, + Found: &store.FoundUnit{Unit: "old.timer", State: "running", Boot: "enabled"}}) + if _, _, err := applyOn(t, nothingButA(t), state, m); err != nil { + t.Fatal(err) + } + if u := m.units["old.timer"]; u.active != "inactive" || u.enabled != "disabled" { + t.Errorf("an old record started %s and %s", u.active, u.enabled) + } +} + +// The plan says it before the apply does it. +func TestThePlanSaysAStoppedUnitIsStartedAgain(t *testing.T) { + m := unitsMachine(map[string]*fakeUnit{"zfs-scrub-weekly@storage.timer": {active: "active", enabled: "enabled"}}) + _, state, err := applyOn(t, `{"declaration":1,"resources":[ + {"id":"w","type":"service","unit":"zfs-scrub-weekly@storage.timer","state":"stopped","boot":"disabled"}]}`, + store.State{}, m) + if err != nil { + t.Fatal(err) + } + said := "" + for _, s := range Plan(parse(t, nothingButA(t)), state, store.OriginDeclared) { + if s.ID == "w" { + said = s.Verb + ": " + s.Why + } + } + if !strings.HasPrefix(said, "restore: ") || !strings.Contains(said, "restore if still as the mesh held it") || + strings.Contains(said, "so it is started") { + t.Errorf("the plan says %q", said) + } +} + +// Third review: on removal a unit is started again only when it is exactly as the mesh held it, stopped and +// disabled. A masked unit, or one somebody started or enabled since, is left alone, and the report says why. +func TestAMaskedOrChangedUnitIsLeftAloneOnRemoval(t *testing.T) { + for _, c := range []struct { + name, active, enabled, says string + }{ + {"masked", "inactive", "masked", "masked"}, + {"enabled since", "inactive", "enabled", "not the stopped and disabled"}, + {"started since", "active", "disabled", "not the stopped and disabled"}, + } { + m := unitsMachine(map[string]*fakeUnit{"w.timer": {active: "active", enabled: "enabled"}}) + _, state, err := applyOn(t, `{"declaration":1,"resources":[ + {"id":"w","type":"service","unit":"w.timer","state":"stopped","boot":"disabled"}]}`, store.State{}, m) + if err != nil { + t.Fatal(err) + } + m.units["w.timer"].active, m.units["w.timer"].enabled = c.active, c.enabled + report, _, err := applyOn(t, nothingButA(t), state, m) + if err != nil { + t.Fatalf("%s: %v", c.name, err) + } + if u := m.units["w.timer"]; u.active != c.active || u.enabled != c.enabled { + t.Errorf("%s: changed to %s and %s", c.name, u.active, u.enabled) + } + said := "" + for _, o := range report.Outcomes { + if o.ID == "w" { + said = o.Action + ": " + o.Detail + } + } + if !strings.HasPrefix(said, "forgotten: ") || !strings.Contains(said, c.says) { + t.Errorf("%s: the report says %q", c.name, said) + } + } +} + +// A restore that fails — the start refused, or the unit not running after it — waits: the record stays, +// the report says so, and no apply is made fatal by it. +func TestAFailedRestoreWaitsAndIsNeverFatal(t *testing.T) { + m := unitsMachine(map[string]*fakeUnit{"w.timer": {active: "active", enabled: "enabled"}}) + _, state, err := applyOn(t, `{"declaration":1,"resources":[ + {"id":"w","type":"service","unit":"w.timer","state":"stopped","boot":"disabled"}]}`, store.State{}, m) + if err != nil { + t.Fatal(err) + } + m.units["w.timer"].wontStart = true + for i := 0; i < 2; i++ { + report, next, err := applyOn(t, nothingButA(t), state, m) + if err != nil { + t.Fatalf("apply %d failed: %v", i+1, err) + } + said := "" + for _, o := range report.Outcomes { + if o.ID == "w" { + said = o.Action + ": " + o.Detail + } + } + if !strings.Contains(said, "wait") || !strings.Contains(said, "not running") { + t.Errorf("apply %d: the report says %q", i+1, said) + } + if _, kept := next.Find("w"); !kept { + t.Errorf("apply %d: the record went, so nothing would try again", i+1) + } + state = next + } +} diff --git a/internal/apply/groups_test.go b/internal/apply/groups_test.go index 03346d2..78630ec 100644 --- a/internal/apply/groups_test.go +++ b/internal/apply/groups_test.go @@ -35,6 +35,8 @@ func newGroupDB(in []string, exists ...string) *groupDB { } func (g *groupDB) run(_ context.Context, name string, args ...string) (string, error) { + + args = oldShape(name, args) g.asked = append(g.asked, name+" "+strings.Join(args, " ")) switch { case name == "getent" && args[0] == "passwd": diff --git a/internal/apply/handover_test.go b/internal/apply/handover_test.go index 041507e..15e538f 100644 --- a/internal/apply/handover_test.go +++ b/internal/apply/handover_test.go @@ -30,6 +30,8 @@ type aMachine struct { } func (m *aMachine) run(ctx context.Context, name string, args ...string) (string, error) { + + args = oldShape(name, args) line := name + " " + strings.Join(args, " ") m.commands = append(m.commands, line) switch { @@ -245,6 +247,7 @@ func TestAStepThatFailsGatesItsModule(t *testing.T) { body, digest := anArchive(t, map[string]string{"mesh-controller": "#!/bin/sh\n"}) src := serving(t, body) run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name == "systemctl" && len(args) > 1 && args[0] == "start" { return "", errors.New("Job for mesh-controller-prepare.service failed") } diff --git a/internal/apply/hold_test.go b/internal/apply/hold_test.go index 3d7d821..868ff1e 100644 --- a/internal/apply/hold_test.go +++ b/internal/apply/hold_test.go @@ -107,7 +107,28 @@ type fakeContainer struct { networks, mounts, ports []string } +// oldShape is a systemctl call as these fakes were written to read it: the verb, the unit, then the +// options. The host passes `--` before every unit (third review of mesh-catalog #147), which +// internal/system's own test holds; the fakes read the unit where they always did. +func oldShape(name string, args []string) []string { + if name != "systemctl" || len(args) < 2 || args[len(args)-2] != "--" { + return args + } + v := 0 + for v < len(args) && strings.HasPrefix(args[v], "-") { + v++ + } + if v >= len(args)-2 { + return args + } + out := append([]string{}, args[:v+1]...) + out = append(out, args[len(args)-1]) + return append(out, args[v+1:len(args)-2]...) +} + func (m *machine) run(_ context.Context, name string, args ...string) (string, error) { + + args = oldShape(name, args) m.asked = append(m.asked, name+" "+strings.Join(args, " ")) if name == "systemctl" { return m.systemctl(args) @@ -907,6 +928,8 @@ func TestMountingTheMachinesOwnPlumbingIsNotFoundData(t *testing.T) { type unreachable struct{ asked []string } func (u *unreachable) run(_ context.Context, name string, args ...string) (string, error) { + + args = oldShape(name, args) u.asked = append(u.asked, name+" "+strings.Join(args, " ")) if name == "docker" && args[0] == "info" { return "27.0\n", nil @@ -958,6 +981,7 @@ func TestAHeldContainerStaysHeldWhenTheRuntimeCannotAnswer(t *testing.T) { func TestAVolumeTheRuntimeCannotBeAskedAboutStopsTheContainer(t *testing.T) { dir := t.TempDir() run := func(_ context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch { case name == "docker" && args[0] == "info": return "27.0\n", nil diff --git a/internal/apply/inspect_scope_test.go b/internal/apply/inspect_scope_test.go index aba3141..3fdb051 100644 --- a/internal/apply/inspect_scope_test.go +++ b/internal/apply/inspect_scope_test.go @@ -19,6 +19,7 @@ import ( // not catch a regression back to it — this one refuses to, on purpose. func dockerLikeByKind(containers map[string]bool) func(context.Context, string, ...string) (string, error) { return func(_ context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name != "docker" { return "", errors.New("unexpected program: " + name) } diff --git a/internal/apply/left_out_test.go b/internal/apply/left_out_test.go index 897655d..ac9175d 100644 --- a/internal/apply/left_out_test.go +++ b/internal/apply/left_out_test.go @@ -16,6 +16,7 @@ func TestAContainerJoinsTheNetworksItKeeps(t *testing.T) { var ran []string connectFails := false run := func(_ context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name != "docker" { return "", errors.New("not installed") } @@ -81,6 +82,7 @@ func TestALeftOutModuleIsNeitherRemovedNorForgotten(t *testing.T) { var removed []string gone := map[string]bool{} run := func(_ context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name != "docker" { return "", nil } @@ -139,6 +141,7 @@ func TestALeftOutModuleIsNeitherRemovedNorForgotten(t *testing.T) { func TestACapabilityReachesTheRuntimeAndTheSpec(t *testing.T) { var ran []string run := func(_ context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name != "docker" { return "", errors.New("not installed") } @@ -180,6 +183,7 @@ func TestAPackageDeclaredAbsentIsRemovedWhenPresentAndLeftWhenNot(t *testing.T) installed := true var ran []string run := func(_ context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) ran = append(ran, name+" "+strings.Join(args, " ")) if name != "pacman" { return "", nil diff --git a/internal/apply/opening_test.go b/internal/apply/opening_test.go index cc97156..91c701f 100644 --- a/internal/apply/opening_test.go +++ b/internal/apply/opening_test.go @@ -42,6 +42,8 @@ func (u *ufwMachine) iptables(args []string) (string, error) { } func (u *ufwMachine) run(_ context.Context, name string, args ...string) (string, error) { + + args = oldShape(name, args) u.asked = append(u.asked, name+" "+strings.Join(args, " ")) switch name { case "nft": @@ -374,6 +376,7 @@ func TestReturningToAdoptedLoadsTheGuardBeforeRemovingTheFilter(t *testing.T) { } guardUpAtStop := false run := func(_ context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name != "systemctl" { return "", &exec.Error{Name: name, Err: exec.ErrNotFound} } @@ -471,6 +474,7 @@ func TestAGuardThatFailsLeavesTheDerivedFilterInForce(t *testing.T) { `","content":"table inet mesh_guard {}\n"}` stopped := false run := func(_ context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name != "systemctl" { return "", &exec.Error{Name: name, Err: exec.ErrNotFound} } diff --git a/internal/apply/placement_guard_test.go b/internal/apply/placement_guard_test.go index 68d53bb..156563c 100644 --- a/internal/apply/placement_guard_test.go +++ b/internal/apply/placement_guard_test.go @@ -58,6 +58,7 @@ func TestNoAccessIsAtTheMachinesOwn(t *testing.T) { func TestNoContainerMountsTheMachinesOwn(t *testing.T) { ran := false run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch { case args[0] == "info": return "27.0\n", nil diff --git a/internal/apply/placement_homes_test.go b/internal/apply/placement_homes_test.go index 6d924f9..6d4f508 100644 --- a/internal/apply/placement_homes_test.go +++ b/internal/apply/placement_homes_test.go @@ -46,6 +46,7 @@ func TestBelowAHomeOnlyThatAccountsDirectoryIsPlaced(t *testing.T) { // A container mounting a refused directory is refused with it, and not run. ran := false run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch args[0] { case "info": return "27.0\n", nil diff --git a/internal/apply/plan.go b/internal/apply/plan.go index 68eb976..e355737 100644 --- a/internal/apply/plan.go +++ b/internal/apply/plan.go @@ -125,6 +125,20 @@ func Plan(d *declaration.Declaration, known store.State, origin string) []Step { case f == nil: step.Verb, step.Why = "forget", "no longer declared; recorded before the host kept what it "+ "found, so it is left as it is" + case orphan.Declared != nil && ((f.State == "running" && orphan.Declared.State == "stopped") || + (f.Boot == "enabled" && orphan.Declared.Boot == "disabled")): + // startUnitBack's words (novox/hq ADR 0269): what the mesh's declaration stopped goes back. + var back []string + if f.State == "running" && orphan.Declared.State == "stopped" { + back = append(back, "started") + } + if f.Boot == "enabled" && orphan.Declared.Boot == "disabled" { + back = append(back, "enabled at boot") + } + // A preview: whether it is still as the mesh held it is read when the removal runs, and a + // masked unit or one changed since is left as it is. + step.Verb, step.Why = "restore", "no longer declared; restore if still as the mesh held it: "+ + strings.Join(back, " and ")+" again, as the host found it, unless it is masked or was changed since" case f.State == "stopped" || f.Boot == "disabled": var back []string if f.State == "stopped" { diff --git a/internal/apply/plan_test.go b/internal/apply/plan_test.go index dcbeecc..f08619a 100644 --- a/internal/apply/plan_test.go +++ b/internal/apply/plan_test.go @@ -103,6 +103,8 @@ type both struct { } func (b *both) run(ctx context.Context, name string, args ...string) (string, error) { + + args = oldShape(name, args) switch name { case "nft", "ufw", "iptables", "ip6tables", "firewall-cmd": return b.u.run(ctx, name, args...) diff --git a/internal/apply/process_step_test.go b/internal/apply/process_step_test.go index bfda159..a42f1ce 100644 --- a/internal/apply/process_step_test.go +++ b/internal/apply/process_step_test.go @@ -29,6 +29,7 @@ func TestARunOnceProcessRunsAsItsOneshotUnit(t *testing.T) { var commands []string fail := false run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) commands = append(commands, name+" "+strings.Join(args, " ")) if fail && name == "systemctl" && len(args) > 0 && args[0] == "start" { return "", errors.New("exit status 1") diff --git a/internal/apply/process_test.go b/internal/apply/process_test.go index 21308d5..71530e5 100644 --- a/internal/apply/process_test.go +++ b/internal/apply/process_test.go @@ -187,6 +187,7 @@ func TestAnUndeclaredProcessIsRemovedWithItsUnitAndBundle(t *testing.T) { } var commands []string run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) commands = append(commands, strings.Join(args, " ")) return "", nil } @@ -269,6 +270,7 @@ func TestAProcessAppliedAgainIsUnchangedAndNotRestarted(t *testing.T) { body, digest := anArchive(t, map[string]string{"main.js": "console.log(1)\n"}) var commands []string run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) commands = append(commands, name+" "+strings.Join(args, " ")) return "", nil } diff --git a/internal/apply/removal_order_test.go b/internal/apply/removal_order_test.go index 711603c..dd23aec 100644 --- a/internal/apply/removal_order_test.go +++ b/internal/apply/removal_order_test.go @@ -35,6 +35,7 @@ func TestAnUnassignedModulesProcessIsStoppedAfterTheConfigurationItReads(t *test var configAtStop []bool run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name == "systemctl" && strings.Join(args, " ") == "disable --now nfs-server-exports.service" { _, err := os.Stat(config) configAtStop = append(configAtStop, err == nil) @@ -96,6 +97,7 @@ func TestAProcessRecordedAfterItsConfigurationIsStoppedFirst(t *testing.T) { var configAtStop []bool run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name == "systemctl" && strings.Join(args, " ") == "disable --now m-exports.service" { _, err := os.Stat(config) configAtStop = append(configAtStop, err == nil) diff --git a/internal/apply/restart_order_test.go b/internal/apply/restart_order_test.go index 82a83ed..1cecd10 100644 --- a/internal/apply/restart_order_test.go +++ b/internal/apply/restart_order_test.go @@ -34,6 +34,7 @@ func TestAServiceIsRestartedOnlyOnceEveryFileItNamesIsWritten(t *testing.T) { var commands []string services := recordingServices(&commands) run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if strings.Contains(strings.Join(args, " "), "start resolver.service") { if _, err := os.Stat(zones); err != nil { commands = append(commands, name+" "+strings.Join(args, " ")) diff --git a/internal/apply/runonce_test.go b/internal/apply/runonce_test.go index e19231f..3cfaed1 100644 --- a/internal/apply/runonce_test.go +++ b/internal/apply/runonce_test.go @@ -32,6 +32,7 @@ func TestARunOnceStepIsRunToCompletionNotLeftRunning(t *testing.T) { // container exits) nor --restart (a step that is restarted is not a step). var runArgs []string run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch args[0] { case "info": return "27.0\n", nil @@ -78,6 +79,7 @@ func TestARunOnceStepThatExitsNonZeroFailsTheApply(t *testing.T) { // Run in the foreground, a non-zero exit is an error the runtime hands back. That must fail // the apply, not be swallowed — a seed that did not happen leaves the machine unready. run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch args[0] { case "info": return "27.0\n", nil @@ -110,6 +112,7 @@ func TestAFailedRunOnceStepGatesWhatFollows(t *testing.T) { // mirror of a failed action stopping what follows. var startedNames []string run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch args[0] { case "info": return "27.0\n", nil @@ -159,6 +162,7 @@ func TestARunOnceStepAlreadyCompletedIsNotReRun(t *testing.T) { var ran bool run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch args[0] { case "info": return "27.0\n", nil @@ -194,6 +198,7 @@ func TestARunOnceStepReRunsWhenItsDeclarationChanged(t *testing.T) { var ran bool run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch args[0] { case "info": return "27.0\n", nil @@ -236,6 +241,7 @@ func TestARunOnceStepRunsAgainWhenWhatItReadsChanged(t *testing.T) { var ran bool run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch args[0] { case "info": return "27.0\n", nil @@ -284,6 +290,7 @@ func TestAContainerNamingARunOnceStepIsRecreatedWhenItRan(t *testing.T) { var removed, created bool run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch args[0] { case "info": return "27.0\n", nil @@ -326,6 +333,7 @@ func TestAFailedStepGatesItsModuleAndNotTheMachine(t *testing.T) { // unreachable must not stop every module declared after it. var startedNames []string run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch args[0] { case "info": return "27.0\n", nil diff --git a/internal/apply/schedule_test.go b/internal/apply/schedule_test.go index 6c3d4b7..01b6b2e 100644 --- a/internal/apply/schedule_test.go +++ b/internal/apply/schedule_test.go @@ -44,6 +44,8 @@ type recordingRun struct { } func (r *recordingRun) run(ctx context.Context, name string, args ...string) (string, error) { + + args = oldShape(name, args) r.mu.Lock() defer r.mu.Unlock() r.all = append(r.all, append([]string{name}, args...)) @@ -91,6 +93,7 @@ func TestInstallingAScheduleDoesNotRunItAndReportsCurrent(t *testing.T) { var fired bool // a `docker run` — the container was started var pulled bool // an image reported present was pulled anyway run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch args[0] { case "info": return "27.0\n", nil // a container runtime answers the apply's probe @@ -154,6 +157,7 @@ func TestInstallingAScheduleEnsuresItsImageIsPresentWithoutStartingIt(t *testing var pulledImage string var fired bool run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch args[0] { case "info": return "27.0\n", nil @@ -205,6 +209,7 @@ func TestAScheduledStepDoesNotGateWhatFollows(t *testing.T) { var startedWeb bool specs := map[string]string{} // name -> spec label, so read-back sees a running container run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) switch args[0] { case "info": return "27.0\n", nil @@ -295,6 +300,8 @@ func TestAFailedRunIsRecordedAndDoesNotFailAnything(t *testing.T) { } run := func(ctx context.Context, name string, args ...string) (string, error) { + + args = oldShape(name, args) switch args[0] { case "info": return "27.0\n", nil @@ -343,6 +350,7 @@ func TestASlowRunSkipsTheNextDueRunRatherThanStacking(t *testing.T) { var enters int var mu sync.Mutex run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if args[0] == "info" { return "27.0\n", nil } diff --git a/internal/apply/stateless_test.go b/internal/apply/stateless_test.go index 7ebdaa1..896af58 100644 --- a/internal/apply/stateless_test.go +++ b/internal/apply/stateless_test.go @@ -17,6 +17,7 @@ import ( // unitIn is a service manager whose one unit is active or not, recording what it is asked. func unitIn(active bool, commands *[]string) Runner { return func(_ context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) line := name + " " + strings.Join(args, " ") *commands = append(*commands, line) switch { diff --git a/internal/apply/stayed_running_test.go b/internal/apply/stayed_running_test.go index d08fb24..553a637 100644 --- a/internal/apply/stayed_running_test.go +++ b/internal/apply/stayed_running_test.go @@ -20,6 +20,7 @@ func TestAServiceThatDiesJustAfterItsRestartIsNotReportedRestarted(t *testing.T) // refuses what it was just given. started, looks := false, 0 run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if args[0] == "show" { state := "active" if started { @@ -56,6 +57,7 @@ func TestAServiceThatDiesJustAfterItsRestartIsNotReportedRestarted(t *testing.T) func TestAUnitStillStartingIsNotAFailure(t *testing.T) { serviceSettle = 0 run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if args[0] == "show" { return "LoadState=loaded\nActiveState=activating\n", nil } @@ -74,6 +76,7 @@ func TestAServiceAskedToStopIsNotWaitedOn(t *testing.T) { serviceSettle = 0 shows := 0 run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if args[0] == "show" { shows++ if shows == 1 { diff --git a/internal/apply/user_test.go b/internal/apply/user_test.go index e6e81a9..d3e4cf8 100644 --- a/internal/apply/user_test.go +++ b/internal/apply/user_test.go @@ -22,6 +22,8 @@ type logins struct { } func (l *logins) run(_ context.Context, name string, args ...string) (string, error) { + + args = oldShape(name, args) l.asked = append(l.asked, name+" "+strings.Join(args, " ")) who := args[len(args)-1] switch name { diff --git a/internal/apply/userscope_test.go b/internal/apply/userscope_test.go index 5c9a8a1..ad5a56d 100644 --- a/internal/apply/userscope_test.go +++ b/internal/apply/userscope_test.go @@ -59,6 +59,8 @@ func (a *account) did(prefix string) bool { } func (a *account) run(_ context.Context, name string, args ...string) (string, error) { + + args = oldShape(name, args) line := name + " " + strings.Join(args, " ") a.asked = append(a.asked, line) switch name { @@ -256,6 +258,7 @@ func TestAManagerThatStopsDuringTheApplyIsWaitedFor(t *testing.T) { a.busDown = true calls := 0 run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name == "systemctl" && len(args) == 2 && args[0] == "is-active" { calls++ if calls > 1 { diff --git a/internal/apply/vocabulary_test.go b/internal/apply/vocabulary_test.go index cc14fd9..5fc8257 100644 --- a/internal/apply/vocabulary_test.go +++ b/internal/apply/vocabulary_test.go @@ -259,6 +259,7 @@ func TestANetworkIsCreatedAndThenRemovedWhenNoLongerDeclared(t *testing.T) { var calls []string there := map[string]bool{} run := func(_ context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) calls = append(calls, name+" "+strings.Join(args, " ")) if name != "docker" || len(args) < 2 || args[0] != "network" { return "", nil // the runtime probe @@ -302,6 +303,7 @@ func TestANetworkIsCreatedAndThenRemovedWhenNoLongerDeclared(t *testing.T) { func TestANetworkAlreadyThereIsNotRebuilt(t *testing.T) { var created int run := func(_ context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) if name == "docker" && len(args) > 1 && args[0] == "network" && args[1] == "create" { created++ } @@ -375,6 +377,7 @@ func TestASealedValueIsPutIntoTheFileThatNamesIt(t *testing.T) { func TestAContainerIsGivenItsEnvironmentFiles(t *testing.T) { var ran []string run := func(_ context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) ran = append(ran, name+" "+strings.Join(args, " ")) if len(args) > 0 && args[0] == "container" { return "", fmt.Errorf("no such container") @@ -410,6 +413,7 @@ func TestAContainerIsGivenItsEnvironmentFiles(t *testing.T) { func TestAContainerIsGivenItsResolverAndItsAddress(t *testing.T) { var ran []string run := func(_ context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) ran = append(ran, name+" "+strings.Join(args, " ")) if len(args) > 0 && args[0] == "container" { return "", fmt.Errorf("no such container") diff --git a/internal/store/store.go b/internal/store/store.go index 92d48d5..b2f3db3 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -99,6 +99,12 @@ type Applied struct { // when its declaration goes; one the mesh started is stopped again. Absent on a record written // before the host kept it, and then the unit is left exactly as it is. Found *FoundUnit `json:"found,omitempty"` + // Declared is, for a service with a state, what the mesh last held its unit to: running or stopped, + // and enabled or disabled where the declaration said. With Found, it tells a unit the mesh's own + // declaration stopped and disabled from one somebody stopped since, so removal gives back running + // and enabled only the first (novox/hq ADR 0269). Absent on a record from before the host kept it, + // and then nothing is started on the way out. + Declared *FoundUnit `json:"declared,omitempty"` // Into is set for a file written into rather than over (novox/hq ADR 0102): the format, what // each of the mesh's keys held before it set them, which of them were absent, and whether the diff --git a/internal/system/arch.go b/internal/system/arch.go index 9aeab7e..6ab34ea 100644 --- a/internal/system/arch.go +++ b/internal/system/arch.go @@ -187,9 +187,9 @@ func describeAge(when, now time.Time) string { // so LoadState is what is read — and it is the thing an interface spanning systemd and OpenRC // would have had to drop. func (arch) ServiceState(ctx context.Context, run Runner, unit string) (string, error) { - out, asked := run(ctx, "systemctl", "show", unit, + out, asked := run(ctx, "systemctl", "show", "--property=LoadState", "--property=ActiveState", "--property=Type", - "--property=RemainAfterExit", "--property=ExecMainStatus") + "--property=RemainAfterExit", "--property=ExecMainStatus", "--", unit) var load, active, kind, remains, exited string for _, line := range strings.Split(out, "\n") { @@ -266,7 +266,7 @@ func (arch) SetServiceState(ctx context.Context, run Runner, unit, state string) if state == "stopped" { verb = "stop" } - _, err := run(ctx, "systemctl", verb, unit) + _, err := run(ctx, "systemctl", verb, "--", unit) return err } @@ -276,7 +276,7 @@ func (arch) SetServiceState(ctx context.Context, run Runner, unit, state string) // install section and CANNOT be enabled. Reading it as "disabled" would have the host try, fail, // and blame the wrong thing — the same shape as reading a missing unit as "stopped". func (arch) ServiceBoot(ctx context.Context, run Runner, unit string) (string, error) { - out, _ := run(ctx, "systemctl", "is-enabled", unit) + out, _ := run(ctx, "systemctl", "is-enabled", "--", unit) switch state := strings.TrimSpace(out); state { case "enabled", "enabled-runtime", "alias": return "enabled", nil @@ -302,7 +302,7 @@ func (arch) SetServiceBoot(ctx context.Context, run Runner, unit, boot string) e if boot == "disabled" { verb = "disable" } - _, err := run(ctx, "systemctl", verb, unit) + _, err := run(ctx, "systemctl", verb, "--", unit) return err } @@ -354,7 +354,7 @@ func (arch) RemoveUserFromGroup(ctx context.Context, run Runner, name, group str // is how the host tells a unit an administrator installed, under /etc or /run, from one a package // ships under /usr (novox/hq ADR 0103). Empty, with no error, for a unit that loads from nowhere. func (arch) ServiceUnitFile(ctx context.Context, run Runner, unit string) (string, error) { - out, err := run(ctx, "systemctl", "show", unit, "--property=FragmentPath") + out, err := run(ctx, "systemctl", "show", "--property=FragmentPath", "--", unit) if err != nil { return "", fmt.Errorf("the service manager did not say where %s comes from: %w", unit, err) } @@ -376,7 +376,7 @@ func (arch) ReloadUnits(ctx context.Context, run Runner) error { // ReloadService tells a running unit to read its configuration again, without stopping it. func (arch) ReloadService(ctx context.Context, run Runner, unit string) error { - _, err := run(ctx, "systemctl", "reload", unit) + _, err := run(ctx, "systemctl", "reload", "--", unit) return err } diff --git a/internal/system/dashdash_test.go b/internal/system/dashdash_test.go new file mode 100644 index 0000000..2f67e39 --- /dev/null +++ b/internal/system/dashdash_test.go @@ -0,0 +1,37 @@ +package system + +import ( + "context" + "strings" + "testing" +) + +// Third review of mesh-catalog #147: a unit's name reaches systemctl as an argument, and since a setting +// may now name a template's instance, a name beginning with '-' must never be read as an option. Every +// call that names a unit passes `--` before it. +func TestEverySystemctlCallPassesDashDashBeforeTheUnit(t *testing.T) { + var asked [][]string + run := func(_ context.Context, name string, args ...string) (string, error) { + if name == "systemctl" { + asked = append(asked, args) + } + return "LoadState=loaded\nActiveState=active\nType=simple\nFragmentPath=/x\n", nil + } + ctx := context.Background() + a := arch{} + unit := "-x@y.timer" + a.ServiceState(ctx, run, unit) + a.SetServiceState(ctx, run, unit, "running") + a.SetServiceState(ctx, run, unit, "stopped") + a.ServiceBoot(ctx, run, unit) + a.SetServiceBoot(ctx, run, unit, "enabled") + a.SetServiceBoot(ctx, run, unit, "disabled") + if len(asked) != 6 { + t.Fatalf("asked %d times: %v", len(asked), asked) + } + for _, args := range asked { + if len(args) < 2 || args[len(args)-1] != unit || args[len(args)-2] != "--" { + t.Errorf("systemctl %s", strings.Join(args, " ")) + } + } +}