From 9d48b8baf93628b4e3df9ec74b5da43eada77e8f Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 01:35:27 +0200 Subject: [PATCH 1/3] Start again on removal what the mesh's own declaration stopped (hq ADR 0269) A unit found running and enabled that a declaration held stopped and disabled stayed stopped and disabled when the declaration went: ADR 0118 never starts a unit on the way out. Unassigning the zfs module would so have left the home server's pool with no scrub schedule at all, said nowhere. The host now records what it declared for a service (Declared, beside Found). On removal, a unit found running and declared stopped is started again, and one found enabled and declared disabled is enabled again, each read back; the report says restored, and the plan says it first. A record from before the field starts nothing. --- internal/apply/apply.go | 79 ++++++++++++++++++++++-- internal/apply/giveback_test.go | 103 ++++++++++++++++++++++++++++++++ internal/apply/plan.go | 12 ++++ internal/store/store.go | 6 ++ 4 files changed, 196 insertions(+), 4 deletions(-) create mode 100644 internal/apply/giveback_test.go diff --git a/internal/apply/apply.go b/internal/apply/apply.go index b63bc7b..5cbcdd1 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 @@ -732,6 +734,7 @@ func ApplyMindingWindows( Scope: outcome.scope, User: outcome.user, Found: outcome.found, + Declared: outcome.declared, Shell: outcome.shell, Linger: outcome.linger, Groups: outcome.groups, @@ -1512,6 +1515,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 @@ -2750,9 +2757,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 { @@ -2811,6 +2820,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 — @@ -2886,6 +2909,54 @@ 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: +// enabled first, then started, each read back, and the report says what was done (novox/hq ADR 0269). +func startUnitBack(ctx context.Context, sys system.System, a store.Applied, run Runner, start, enable bool) (string, string, error) { + state, err := sys.ServiceState(ctx, run, a.Target) + if err != nil { + if strings.Contains(err.Error(), "does not exist on this machine") { + return "forgotten", "the unit no longer exists", nil + } + return "", "", err + } + var did, already []string + if enable { + if boot, err := sys.ServiceBoot(ctx, run, a.Target); err == nil && boot == "enabled" { + already = append(already, "enabled at boot") + } else { + if err := sys.SetServiceBoot(ctx, run, a.Target, "enabled"); err != nil { + return "", "", fmt.Errorf("enabling %s again, which the mesh disabled and found enabled: %w", a.Target, err) + } + did = append(did, "enabled at boot") + } + } + if start { + if state == "running" { + already = append(already, "running") + } else { + if err := sys.SetServiceState(ctx, run, a.Target, "running"); err != nil { + return "", "", fmt.Errorf("starting %s again, which the mesh stopped and found running: %w", a.Target, err) + } + after, err := sys.ServiceState(ctx, run, a.Target) + if err != nil { + return "", "", err + } + if after != "running" { + return "", "", fmt.Errorf("%s was started again, as the host found it, and is %s", a.Target, after) + } + did = append(did, "started") + } + } + if len(did) == 0 { + return "forgotten", "already as the host found it (" + strings.Join(already, ", ") + ")", nil + } + detail := strings.Join(did, " and ") + " again, as the host found it before the mesh's declaration stopped it" + if len(already) > 0 { + detail += "; already " + strings.Join(already, ", ") + } + return "restored", detail, nil +} + // 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 @@ -2916,7 +2987,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/giveback_test.go b/internal/apply/giveback_test.go new file mode 100644 index 0000000..bbe99ce --- /dev/null +++ b/internal/apply/giveback_test.go @@ -0,0 +1,103 @@ +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, "started and enabled at boot again") { + t.Errorf("the plan says %q", said) + } +} diff --git a/internal/apply/plan.go b/internal/apply/plan.go index 68eb976..fba1bfa 100644 --- a/internal/apply/plan.go +++ b/internal/apply/plan.go @@ -125,6 +125,18 @@ 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") + } + step.Verb, step.Why = "restore", "no longer declared; the host found it running and enabled and "+ + "the mesh held it stopped, so it is "+strings.Join(back, " and ")+" again" case f.State == "stopped" || f.Boot == "disabled": var back []string if f.State == "stopped" { diff --git a/internal/store/store.go b/internal/store/store.go index a7ca2a6..e7a752f 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 From a018ebd1ca74f0b6b3984e946cbbecf721f27ef9 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 01:54:26 +0200 Subject: [PATCH 2/3] Pass -- before every unit, and give a unit back only when it is exactly as the mesh held it Third review of mesh-catalog #147. A unit's name can now come from a setting (a template's instance), so every systemctl call that names a unit passes `--` before it, and a name can never be read as an option. On removal, a unit is started or enabled again only when it is exactly the stopped and disabled the mesh held it to. A masked unit, or one somebody started or enabled since, is left as it is, and the report says why. A restore that fails (the start refused, or the unit not running after it) puts back what this attempt changed and is reported as waiting, so no apply is made fatal by it and the next one tries again. The tests' fake service managers read systemctl's arguments in the shape they were written for (oldShape); internal/system's own test holds the `--`. --- internal/apply/apply.go | 89 ++++++++++++++++---------- internal/apply/apply_test.go | 26 ++++++++ internal/apply/file_handover_test.go | 1 + internal/apply/former_archive_test.go | 1 + internal/apply/found_test.go | 1 + internal/apply/giveback_test.go | 67 +++++++++++++++++++ internal/apply/groups_test.go | 2 + internal/apply/handover_test.go | 3 + internal/apply/hold_test.go | 24 +++++++ internal/apply/inspect_scope_test.go | 1 + internal/apply/left_out_test.go | 4 ++ internal/apply/opening_test.go | 4 ++ internal/apply/placement_guard_test.go | 1 + internal/apply/placement_homes_test.go | 1 + internal/apply/plan_test.go | 2 + internal/apply/process_step_test.go | 1 + internal/apply/process_test.go | 2 + internal/apply/removal_order_test.go | 2 + internal/apply/restart_order_test.go | 1 + internal/apply/runonce_test.go | 8 +++ internal/apply/schedule_test.go | 8 +++ internal/apply/stateless_test.go | 1 + internal/apply/stayed_running_test.go | 3 + internal/apply/user_test.go | 2 + internal/apply/userscope_test.go | 3 + internal/apply/vocabulary_test.go | 4 ++ internal/system/arch.go | 14 ++-- internal/system/dashdash_test.go | 37 +++++++++++ 28 files changed, 271 insertions(+), 42 deletions(-) create mode 100644 internal/system/dashdash_test.go diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 5cbcdd1..803461a 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -2909,52 +2909,71 @@ 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: -// enabled first, then started, each read back, and the report says what was done (novox/hq ADR 0269). +// 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, err := sys.ServiceState(ctx, run, a.Target) - if err != nil { - if strings.Contains(err.Error(), "does not exist on this machine") { + 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) } - return "", "", err } - var did, already []string + 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 boot, err := sys.ServiceBoot(ctx, run, a.Target); err == nil && boot == "enabled" { - already = append(already, "enabled at boot") - } else { - if err := sys.SetServiceBoot(ctx, run, a.Target, "enabled"); err != nil { - return "", "", fmt.Errorf("enabling %s again, which the mesh disabled and found enabled: %w", a.Target, err) - } - did = append(did, "enabled at boot") + 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 { - if state == "running" { - already = append(already, "running") - } else { - if err := sys.SetServiceState(ctx, run, a.Target, "running"); err != nil { - return "", "", fmt.Errorf("starting %s again, which the mesh stopped and found running: %w", a.Target, err) - } - after, err := sys.ServiceState(ctx, run, a.Target) - if err != nil { - return "", "", err - } - if after != "running" { - return "", "", fmt.Errorf("%s was started again, as the host found it, and is %s", a.Target, after) - } - did = append(did, "started") + 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") } - if len(did) == 0 { - return "forgotten", "already as the host found it (" + strings.Join(already, ", ") + ")", nil + 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" } - detail := strings.Join(did, " and ") + " again, as the host found it before the mesh's declaration stopped it" - if len(already) > 0 { - detail += "; already " + strings.Join(already, ", ") - } - return "restored", detail, nil + return s } // foundAs is what a service's unit was before the mesh touched it, as far as this host can know: 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 index bbe99ce..2e64401 100644 --- a/internal/apply/giveback_test.go +++ b/internal/apply/giveback_test.go @@ -101,3 +101,70 @@ func TestThePlanSaysAStoppedUnitIsStartedAgain(t *testing.T) { 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_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/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, " ")) + } + } +} From dfb14151bd20e9c31ee06bba19681cf74a1bf988 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 02:03:51 +0200 Subject: [PATCH 3/3] Say in the plan that a unit is restored only if it is still as the mesh held it Confirmation pass of #66: the plan is a preview, and the removal reads the unit when it runs; a masked unit or one changed since is left as it is. --- internal/apply/giveback_test.go | 3 ++- internal/apply/plan.go | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/apply/giveback_test.go b/internal/apply/giveback_test.go index 2e64401..94c5a52 100644 --- a/internal/apply/giveback_test.go +++ b/internal/apply/giveback_test.go @@ -97,7 +97,8 @@ func TestThePlanSaysAStoppedUnitIsStartedAgain(t *testing.T) { said = s.Verb + ": " + s.Why } } - if !strings.HasPrefix(said, "restore: ") || !strings.Contains(said, "started and enabled at boot again") { + 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) } } diff --git a/internal/apply/plan.go b/internal/apply/plan.go index fba1bfa..e355737 100644 --- a/internal/apply/plan.go +++ b/internal/apply/plan.go @@ -135,8 +135,10 @@ func Plan(d *declaration.Declaration, known store.State, origin string) []Step { if f.Boot == "enabled" && orphan.Declared.Boot == "disabled" { back = append(back, "enabled at boot") } - step.Verb, step.Why = "restore", "no longer declared; the host found it running and enabled and "+ - "the mesh held it stopped, so it is "+strings.Join(back, " and ")+" again" + // 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" {