diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 64d2f06..0864f60 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -2002,6 +2002,35 @@ type unitReloader interface { ReloadUnits(ctx context.Context, run system.Runner) error } +// restReader is a service manager that can say whether a one-shot has nothing running now +// (novox/hq issue 1126). +type restReader interface { + ServiceAtRest(ctx context.Context, run system.Runner, unit string) (bool, error) +} + +// declaredState is a unit's state as a declaration of want reads it. **A one-shot at rest is +// stopped to a declaration that it be stopped** (novox/hq issue 1126): a one-shot that does not +// linger reads "running" once it ran and worked — that is what a declaration that it run asks — but +// it has no process between its runs, so it is also as stopped as it can be, and stopping it changes +// nothing. Read only as "running", a boot-time cleanup declared stopped and enabled failed every +// apply after the boot that ran it: "was asked to be stopped and is running". +func declaredState(ctx context.Context, sys system.System, run Runner, unit, want string) (string, error) { + state, err := sys.ServiceState(ctx, run, unit) + if err != nil || want != "stopped" || state != "running" { + return state, err + } + if r, ok := sys.(restReader); ok { + rest, err := r.ServiceAtRest(ctx, run, unit) + if err != nil { + return "", err + } + if rest { + return "stopped", nil + } + } + return state, nil +} + // serviceSettle is how long the host waits before looking at a unit a second time. A test sets it // to nothing; on a machine it is the window in which a daemon that refuses its configuration dies. var serviceSettle = 2 * time.Second @@ -2154,7 +2183,7 @@ func applyServiceIn(ctx context.Context, sys system.System, r *declaration.Servi // 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) + before, err := declaredState(ctx, sys, run, r.Unit, r.State) if err != nil { return out, err } @@ -2165,7 +2194,7 @@ func applyServiceIn(ctx context.Context, sys system.System, r *declaration.Servi // Read back. A service manager accepting a command says the transaction was accepted, // not that the unit is running — one that starts and immediately dies satisfies it. - after, err := sys.ServiceState(ctx, run, r.Unit) + after, err := declaredState(ctx, sys, run, r.Unit, r.State) if err == nil && r.State == "running" { after, err = stayedRunning(ctx, sys, run, r.Unit) } diff --git a/internal/apply/apply_test.go b/internal/apply/apply_test.go index ccefba0..fca922c 100644 --- a/internal/apply/apply_test.go +++ b/internal/apply/apply_test.go @@ -350,6 +350,44 @@ func TestAnUnknownServiceStateIsRefusedNotGuessed(t *testing.T) { } } +// A boot-time one-shot a package ships, declared stopped and enabled at boot, is satisfied while +// nothing of it runs: before its first boot run and after it (novox/hq issue 1126). The kernel +// module's linux-modules-cleanup.service failed on ace with "was asked to be stopped and is +// running" for a unit that had never run, and every apply after a reboot would have failed the +// same way for one that had. +func TestABootOneShotDeclaredStoppedIsSatisfiedAtRest(t *testing.T) { + for _, c := range []struct{ name, started, status string }{ + {"never ran", "0", "0"}, + {"ran at boot", "5123456", "0"}, + } { + t.Run(c.name, func(t *testing.T) { + var asked []string + run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) + switch args[0] { + case "show": + return "LoadState=loaded\nActiveState=inactive\nType=oneshot\nRemainAfterExit=no\n" + + "ExecMainStatus=" + c.status + "\nExecMainStartTimestampMonotonic=" + c.started + "\n", nil + case "is-enabled": + return "enabled\n", nil + } + asked = append(asked, strings.Join(args, " ")) + return "", nil + } + d := parse(t, `{"declaration":1,"resources":[ + {"id":"cleanup","type":"service","unit":"linux-modules-cleanup.service","state":"stopped","boot":"enabled"} + ]}`) + _, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, run, nil, nil) + if err != nil { + t.Fatalf("a one-shot at rest declared stopped failed: %v", err) + } + if len(asked) > 0 { + t.Errorf("the service manager was asked %q for a unit already as declared", asked) + } + }) + } +} + func TestADroppedServiceIsGivenBackTheStateItWasFoundIn(t *testing.T) { // novox/hq ADR 0118: undeclaring removes what the mesh made, gives back what it changed, and // leaves what was the machine's. A service resource never installs a unit — so undeclaring it diff --git a/internal/system/arch.go b/internal/system/arch.go index 34bf0aa..6126d2f 100644 --- a/internal/system/arch.go +++ b/internal/system/arch.go @@ -187,38 +187,11 @@ 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", - "--property=LoadState", "--property=ActiveState", "--property=Type", - "--property=RemainAfterExit", "--property=ExecMainStatus", "--", unit) - - var load, active, kind, remains, exited string - for _, line := range strings.Split(out, "\n") { - key, value, found := strings.Cut(strings.TrimSpace(line), "=") - if !found { - continue - } - switch key { - case "LoadState": - load = value - case "ActiveState": - active = value - case "Type": - kind = value - case "RemainAfterExit": - remains = value - case "ExecMainStatus": - exited = value - } + u, err := showUnit(ctx, run, unit) + if err != nil { + return "", err } - - switch load { - case "": - // With why, where it said why: an account's manager that could not be reached says so - // here, and nowhere else (novox/hq ADR 0177). - if asked != nil { - return "", fmt.Errorf("the service manager said nothing about %s: %w", unit, asked) - } - return "", fmt.Errorf("the service manager said nothing about %s", unit) + switch u.load { case "not-found": return "", fmt.Errorf( "%s does not exist on this machine. A declaration naming a unit that is not "+ @@ -227,7 +200,7 @@ func (arch) ServiceState(ctx context.Context, run Runner, unit string) (string, case "masked": return "", fmt.Errorf("%s is masked, so its state cannot be declared", unit) case "error", "bad-setting": - return "", fmt.Errorf("%s is installed but its unit file cannot be loaded (%s)", unit, load) + return "", fmt.Errorf("%s is installed but its unit file cannot be loaded (%s)", unit, u.load) } // **A one-shot that finished is not stopped.** A unit whose whole job is to apply something @@ -243,24 +216,95 @@ func (arch) ServiceState(ctx context.Context, run Runner, unit string) (string, // wrong with. // // So for that shape, what "running" means is "it ran, and it worked". - if kind == "oneshot" && remains != "yes" && active == "inactive" { - if exited == "0" || exited == "" { + // + // **And one that never ran is not one that worked** (novox/hq issue 1126). systemd says + // ExecMainStatus=0 for a unit that has not run since the manager loaded it exactly as for one + // that succeeded; only its start timestamp, 0, separates them. Read as "running", a package's + // boot-time one-shot declared stopped could never be satisfied — stopping an inactive unit + // changes nothing, and the read back said running again — and one declared running was never + // started. A timestamp the manager did not say keeps the reading it had before. + if u.oneShotAtRest() { + if u.started == "0" { + return "stopped", nil + } + if u.exited == "0" || u.exited == "" { return "running", nil } return "stopped", nil } - switch active { + switch u.active { case "active", "activating", "reloading": return "running", nil case "inactive", "failed", "deactivating": return "stopped", nil default: return "", fmt.Errorf( - "the service manager reports %s as %q, which is neither running nor stopped", unit, active) + "the service manager reports %s as %q, which is neither running nor stopped", unit, u.active) } } +// ServiceAtRest is whether unit is a one-shot that does not linger and has nothing running now +// (novox/hq issue 1126). Such a unit has no state in which a process is up between its runs, so a +// declaration that it be stopped is satisfied whatever its last run did: there is nothing to stop, +// and asking the manager to stop it changes nothing. A boot-time cleanup a package ships is declared +// this way — enabled at boot, never started by an apply. +func (arch) ServiceAtRest(ctx context.Context, run Runner, unit string) (bool, error) { + u, err := showUnit(ctx, run, unit) + if err != nil { + return false, err + } + return u.oneShotAtRest(), nil +} + +// shownUnit is what `systemctl show` says of a unit, as far as its state is read. +type shownUnit struct { + load, active, kind, remains, exited, started string +} + +// oneShotAtRest is a one-shot that does not linger and is not running now. +func (u shownUnit) oneShotAtRest() bool { + return u.kind == "oneshot" && u.remains != "yes" && u.active == "inactive" +} + +// showUnit reads a unit's state properties. A manager that said nothing is an error, with why +// where it said why: an account's manager that could not be reached says so here, and nowhere +// else (novox/hq ADR 0177). +func showUnit(ctx context.Context, run Runner, unit string) (shownUnit, error) { + out, asked := run(ctx, "systemctl", "show", + "--property=LoadState", "--property=ActiveState", "--property=Type", + "--property=RemainAfterExit", "--property=ExecMainStatus", + "--property=ExecMainStartTimestampMonotonic", "--", unit) + var u shownUnit + for _, line := range strings.Split(out, "\n") { + key, value, found := strings.Cut(strings.TrimSpace(line), "=") + if !found { + continue + } + switch key { + case "LoadState": + u.load = value + case "ActiveState": + u.active = value + case "Type": + u.kind = value + case "RemainAfterExit": + u.remains = value + case "ExecMainStatus": + u.exited = value + case "ExecMainStartTimestampMonotonic": + u.started = value + } + } + if u.load == "" { + if asked != nil { + return u, fmt.Errorf("the service manager said nothing about %s: %w", unit, asked) + } + return u, fmt.Errorf("the service manager said nothing about %s", unit) + } + return u, nil +} + func (arch) SetServiceState(ctx context.Context, run Runner, unit, state string) error { verb := "start" if state == "stopped" { diff --git a/internal/system/arch_test.go b/internal/system/arch_test.go index 22d6bf2..5e57236 100644 --- a/internal/system/arch_test.go +++ b/internal/system/arch_test.go @@ -5,47 +5,60 @@ import ( "testing" ) -// A one-shot unit that did its work and exited is satisfied, not stopped. +// How a one-shot unit's state is read: what it did, not whether a process is up. // -// **This made the firewall permanently unsatisfiable.** Its unit loads a rule set and exits, so it -// is inactive the instant it succeeds — the host started it, it worked, the host read back -// "stopped" and reported the machine as not doing what it was told. On every apply, for ever, with -// the rules correctly in place the whole time. -func TestAOneShotThatFinishedIsRunning(t *testing.T) { - run := func(_ context.Context, _ string, _ ...string) (string, error) { - return "LoadState=loaded\nActiveState=inactive\nType=oneshot\nRemainAfterExit=no\nExecMainStatus=0\n", nil - } - state, err := arch{}.ServiceState(context.Background(), run, "nftables.service") - if err != nil { - t.Fatalf("a one-shot that succeeded was an error: %v", err) - } - if state != "running" { - t.Fatalf("a one-shot that did its work reads as %q, so it can never be satisfied", state) - } -} - -// And one that failed is still stopped, or the host would report success for work that did not -// happen — which is the opposite mistake and the worse one. -func TestAOneShotThatFailedIsStopped(t *testing.T) { - run := func(_ context.Context, _ string, _ ...string) (string, error) { - return "LoadState=loaded\nActiveState=inactive\nType=oneshot\nRemainAfterExit=no\nExecMainStatus=1\n", nil - } - state, err := arch{}.ServiceState(context.Background(), run, "nftables.service") - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if state != "stopped" { - t.Fatalf("a one-shot that failed reads as %q, so a broken firewall reports success", state) - } -} - -// A unit that lingers deliberately is unaffected: it says so, and its active state is the answer. -func TestAOneShotThatRemainsIsJudgedByItsActiveState(t *testing.T) { - run := func(_ context.Context, _ string, _ ...string) (string, error) { - return "LoadState=loaded\nActiveState=active\nType=oneshot\nRemainAfterExit=yes\nExecMainStatus=0\n", nil - } - state, _ := arch{}.ServiceState(context.Background(), run, "thing.service") - if state != "running" { - t.Fatalf("a lingering one-shot reads as %q", state) +// **A one-shot that did its work and exited is satisfied, not stopped.** Its unit loads a rule set +// and exits, so it is inactive the instant it succeeds; reading that as stopped made the firewall +// permanently unsatisfiable, on every apply, with the rules correctly in place the whole time. +// +// **One that failed is still stopped**, or the host would report success for work that did not +// happen — the opposite mistake and the worse one. +// +// **One that never ran is stopped** (novox/hq issue 1126, the kernel's boot-time cleanup on ace): +// systemd says ExecMainStatus=0 for a unit that never ran exactly as for one that succeeded, and +// only its start timestamp separates them. Read as "running", a package's one-shot declared +// running was never started, and one declared stopped could never be. +// +// **At rest** is whether a one-shot that does not linger has nothing running now: a declaration +// that it be stopped is then satisfied, whatever it last did, because there is nothing to stop. +func TestAOneShotIsReadByWhatItDid(t *testing.T) { + for _, c := range []struct { + name, show, want string + atRest bool + }{ + {"finished", "ActiveState=inactive\nType=oneshot\nRemainAfterExit=no\nExecMainStatus=0\n" + + "ExecMainStartTimestampMonotonic=5123456\n", "running", true}, + {"finished, no timestamp read", "ActiveState=inactive\nType=oneshot\nRemainAfterExit=no\n" + + "ExecMainStatus=0\n", "running", true}, + {"failed", "ActiveState=inactive\nType=oneshot\nRemainAfterExit=no\nExecMainStatus=1\n" + + "ExecMainStartTimestampMonotonic=5123456\n", "stopped", true}, + {"never ran", "ActiveState=inactive\nType=oneshot\nRemainAfterExit=no\nExecMainStatus=0\n" + + "ExecMainStartTimestampMonotonic=0\n", "stopped", true}, + {"running now", "ActiveState=activating\nType=oneshot\nRemainAfterExit=no\nExecMainStatus=0\n" + + "ExecMainStartTimestampMonotonic=5123456\n", "running", false}, + {"lingers", "ActiveState=active\nType=oneshot\nRemainAfterExit=yes\nExecMainStatus=0\n", + "running", false}, + {"a daemon, stopped", "ActiveState=inactive\nType=simple\nRemainAfterExit=no\nExecMainStatus=0\n", + "stopped", false}, + } { + t.Run(c.name, func(t *testing.T) { + run := func(_ context.Context, _ string, _ ...string) (string, error) { + return "LoadState=loaded\n" + c.show, nil + } + state, err := arch{}.ServiceState(context.Background(), run, "thing.service") + if err != nil { + t.Fatalf("read as an error: %v", err) + } + if state != c.want { + t.Errorf("reads as %q, want %q", state, c.want) + } + rest, err := arch{}.ServiceAtRest(context.Background(), run, "thing.service") + if err != nil { + t.Fatalf("at rest read as an error: %v", err) + } + if rest != c.atRest { + t.Errorf("at rest is %v, want %v", rest, c.atRest) + } + }) } }