diff --git a/internal/apply/apply.go b/internal/apply/apply.go index c0d0d7e..6d487fe 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -316,40 +316,101 @@ func writeAtomically(path string, content []byte, mode os.FileMode) error { func applyService(ctx context.Context, r *declaration.Service, run Runner) (Outcome, error) { out := begin(r) + var changes []string + + // Boot first. A unit asked to be running and enabled should survive this apply failing + // half way in the more useful direction: enabled-and-stopped comes back at the next boot, + // where running-and-disabled does not. + if r.Boot != "" { + bootBefore, err := serviceBoot(ctx, r.Unit, run) + if err != nil { + return out, err + } + if bootBefore != r.Boot { + verb := "enable" + if r.Boot == "disabled" { + verb = "disable" + } + if _, err := run(ctx, "systemctl", verb, r.Unit); err != nil { + return out, fmt.Errorf("%s %s: %w", verb, r.Unit, err) + } + bootAfter, err := serviceBoot(ctx, r.Unit, run) + if err != nil { + return out, err + } + if bootAfter != r.Boot { + return out, fmt.Errorf( + "%s was asked to be %s at boot and is %s", r.Unit, r.Boot, bootAfter) + } + changes = append(changes, "boot "+bootBefore+" to "+bootAfter) + } + } before, err := serviceState(ctx, r.Unit, run) if err != nil { return out, err } - if before == r.State { + if before != r.State { + verb := "start" + if r.State == "stopped" { + verb = "stop" + } + if _, err := run(ctx, "systemctl", verb, r.Unit); err != nil { + return out, fmt.Errorf("%s %s: %w", verb, r.Unit, err) + } + + // Read back. `systemctl start` returning zero says the transaction was accepted, not + // that the unit is running — a unit that starts and immediately dies satisfies the + // command. + after, err := serviceState(ctx, r.Unit, run) + if err != nil { + return out, err + } + if after != r.State { + return out, fmt.Errorf("%s was asked to be %s and is %s", r.Unit, r.State, after) + } + changes = append(changes, before+" to "+after) + } + + if len(changes) == 0 { out.Action = "unchanged" out.Detail = before return out, nil } - - verb := "start" - if r.State == "stopped" { - verb = "stop" - } - if _, err := run(ctx, "systemctl", verb, r.Unit); err != nil { - return out, fmt.Errorf("%s %s: %w", verb, r.Unit, err) - } - - // Read back. `systemctl start` returning zero says the transaction was accepted, not that - // the unit is running — a unit that starts and immediately dies satisfies the command. - after, err := serviceState(ctx, r.Unit, run) - if err != nil { - return out, err - } - if after != r.State { - return out, fmt.Errorf("%s was asked to be %s and is %s", r.Unit, r.State, after) - } - out.Action = "updated" - out.Detail = before + " to " + after + out.Detail = strings.Join(changes, ", ") return out, nil } +// serviceBoot reads whether a unit starts at boot. +// +// The same trap as serviceState, in a new place. `systemctl is-enabled` exits non-zero for +// nearly everything that is not "enabled", so the exit code says nothing useful — and it has +// more than two answers. `static` in particular is neither enabled nor disabled: the unit has +// no install section and CANNOT be enabled, so reporting it as "disabled" would let the host +// try, fail, and blame the wrong thing. +func serviceBoot(ctx context.Context, unit string, run Runner) (string, error) { + out, _ := run(ctx, "systemctl", "is-enabled", unit) + switch state := strings.TrimSpace(out); state { + case "enabled", "enabled-runtime", "alias": + return "enabled", nil + case "disabled": + return "disabled", nil + case "": + return "", fmt.Errorf("the service manager said nothing about whether %s starts at boot", unit) + case "static": + return "", fmt.Errorf( + "%s is static — it has no install section, so it cannot be enabled or disabled. "+ + "Something else pulls it in, and that is what a declaration should name", unit) + case "masked", "masked-runtime": + return "", fmt.Errorf("%s is masked, so its boot state cannot be declared", unit) + default: + return "", fmt.Errorf( + "the service manager reports %s as %q at boot, which is neither enabled nor disabled", + unit, state) + } +} + // serviceState reads what the service manager says about a unit. // // Two traps here, and both were hit before this read what it now reads. @@ -614,10 +675,9 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner) ( out := begin(r) want := containerSpec(r) - if _, err := run(ctx, "docker", "version", "--format", "{{.Server.Version}}"); err != nil { - return out, fmt.Errorf( - "the container runtime does not answer on this machine, so nothing can be said "+ - "about %q: %w", r.Name, err) + cri, err := containerRuntime(ctx, run) + if err != nil { + return out, fmt.Errorf("%w, so nothing can be said about %q", err, r.Name) } before, err := containerState(ctx, r.Name, run) @@ -628,7 +688,7 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner) ( out.Action = "unchanged" return out, nil case existed: - if _, err := run(ctx, "docker", "rm", "-f", r.Name); err != nil { + if _, err := run(ctx, cri, "rm", "-f", r.Name); err != nil { return out, fmt.Errorf("replacing container %s: %w", r.Name, err) } } @@ -647,7 +707,7 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner) ( args = append(args, r.Image) args = append(args, r.Args...) - if _, err := run(ctx, "docker", args...); err != nil { + if _, err := run(ctx, cri, args...); err != nil { return out, fmt.Errorf("starting container %s: %w", r.Name, err) } @@ -726,3 +786,42 @@ func runAction(ctx context.Context, r *declaration.Action, argv []string, run Ru } return run(ctx, argv[0], argv[1:]...) } + +// Container runtimes the host knows how to ask. +// +// Two, because two exist on machines the mesh runs on. The list is short on purpose: each entry +// is a claim that its probe and its CLI have been checked, not that a binary of that name might +// work (novox/hq ADR 0060). +// +// The probe differs and the rest does not, which is what makes this a lookup rather than an +// interface. `docker info --format {{.ServerVersion}}` fails on podman — the field does not +// exist in its report — while `run`, `inspect --format` and `rm -f` are identical, including +// docker's own Go template syntax for reading state and labels. +var containerRuntimes = []struct { + command string + // probe asks the runtime for its version in the form THAT runtime understands. It must + // prove the runtime is FUNCTIONING, never that a binary is on disk + // (novox/hq 04-ISSUES/007). + probe []string +}{ + {command: "docker", probe: []string{"info", "--format", "{{.ServerVersion}}"}}, + {command: "podman", probe: []string{"info", "--format", "{{.Version.Version}}"}}, +} + +// containerRuntime returns the runtime this machine actually has, or says there is none. +// +// Detected rather than declared, because a machine already carrying one keeps it: adoption +// takes over what is there rather than replacing it (novox/hq research 012). Which runtime a +// machine has is reported upward in the profile; what to install on a machine with none is the +// control plane's decision, not this one's. +func containerRuntime(ctx context.Context, run Runner) (string, error) { + var tried []string + for _, rt := range containerRuntimes { + if _, err := run(ctx, rt.command, rt.probe...); err == nil { + return rt.command, nil + } + tried = append(tried, rt.command) + } + return "", fmt.Errorf( + "no container runtime answers on this machine (tried %s)", strings.Join(tried, ", ")) +} diff --git a/internal/apply/apply_test.go b/internal/apply/apply_test.go index 3b40a66..028e591 100644 --- a/internal/apply/apply_test.go +++ b/internal/apply/apply_test.go @@ -589,7 +589,7 @@ func TestAContainerThatExitsImmediatelyFailsTheApply(t *testing.T) { // 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) { switch { - case args[0] == "version": + case args[0] == "info": return "27.0\n", nil case args[0] == "inspect": return "false\t" + "", nil // exists, not running @@ -627,7 +627,7 @@ func TestAContainerWhoseDeclarationChangedIsReplaced(t *testing.T) { var removed, created bool run := func(ctx context.Context, name string, args ...string) (string, error) { switch args[0] { - case "version": + case "info": return "27.0\n", nil case "inspect": if created { @@ -665,7 +665,7 @@ func TestAContainerThatMatchesIsLeftAlone(t *testing.T) { var touched bool run := func(ctx context.Context, name string, args ...string) (string, error) { switch args[0] { - case "version": + case "info": return "27.0\n", nil case "inspect": return "true\t" + spec, nil @@ -685,3 +685,249 @@ func TestAContainerThatMatchesIsLeftAlone(t *testing.T) { t.Errorf("a matching container reported a change: %+v", report.Outcomes) } } + +// --- boot state (novox/hq: a unit started but not enabled stops being true at the next reboot) --- + +// systemctlStub answers `show` and `is-enabled` the way systemd does, and records the verbs it +// was asked to perform. Real command shapes, because the trap being tested is what systemd +// actually says rather than what a fake would. +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) { + switch args[0] { + case "show": + return "LoadState=" + load + "\nActiveState=" + active + "\n", nil + case "is-enabled": + // Non-zero for everything but "enabled" — the exit code says nothing useful, which + // is the whole reason this reads the output. + if enabled == "enabled" { + return enabled + "\n", nil + } + return enabled + "\n", errors.New("exit status 1") + case "enable": + *verbs = append(*verbs, "enable") + enabled = "enabled" + return "", nil + case "disable": + *verbs = append(*verbs, "disable") + enabled = "disabled" + return "", nil + case "start": + *verbs = append(*verbs, "start") + active = "active" + return "", nil + case "stop": + *verbs = append(*verbs, "stop") + active = "inactive" + return "", nil + } + return "", nil + } +} + +func TestAServiceIsEnabledAtBootWhenAsked(t *testing.T) { + // The gap this closes: the host could start a unit and never make it survive a reboot, so + // the declaration reported success and stopped being true at the next power cut. + var verbs []string + d := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"rt","type":"service","unit":"docker.service","state":"running","boot":"enabled"} + ]}`) + + report, _, err := Apply(context.Background(), d, store.State{}, + systemctlStub(t, "loaded", "inactive", "disabled", &verbs), nil) + if err != nil { + t.Fatalf("apply failed: %v", err) + } + if len(verbs) != 2 || verbs[0] != "enable" || verbs[1] != "start" { + t.Errorf("expected enable then start, got %v", verbs) + } + if report.Outcomes[0].Action != "updated" { + t.Errorf("enabling and starting was not reported as an update: %+v", report.Outcomes[0]) + } +} + +func TestBootIsEnabledBeforeTheUnitIsStarted(t *testing.T) { + // Order matters when an apply fails part way. Enabled-and-stopped comes back at the next + // boot; running-and-disabled does not. So the more durable half is made true first. + var verbs []string + d := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"rt","type":"service","unit":"docker.service","state":"running","boot":"enabled"} + ]}`) + if _, _, err := Apply(context.Background(), d, store.State{}, + systemctlStub(t, "loaded", "inactive", "disabled", &verbs), nil); err != nil { + t.Fatal(err) + } + if len(verbs) < 2 || verbs[0] != "enable" { + t.Errorf("boot state was not made true first: %v", verbs) + } +} + +func TestAlreadyEnabledAndRunningIsUnchanged(t *testing.T) { + var verbs []string + d := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"rt","type":"service","unit":"docker.service","state":"running","boot":"enabled"} + ]}`) + + report, _, err := Apply(context.Background(), d, store.State{}, + systemctlStub(t, "loaded", "active", "enabled", &verbs), nil) + if err != nil { + t.Fatalf("apply failed: %v", err) + } + if len(verbs) != 0 { + t.Errorf("a unit already in the declared state was touched: %v", verbs) + } + if report.Changed() { + t.Errorf("an unchanged service reported a change: %+v", report.Outcomes) + } +} + +func TestOmittingBootLeavesItAlone(t *testing.T) { + // Absent means the host asserts nothing. A machine whose operator enabled something must + // not have it silently disabled because a declaration did not mention it. + var verbs []string + d := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"rt","type":"service","unit":"docker.service","state":"running"} + ]}`) + if _, _, err := Apply(context.Background(), d, store.State{}, + systemctlStub(t, "loaded", "inactive", "enabled", &verbs), nil); err != nil { + t.Fatal(err) + } + for _, v := range verbs { + if v == "enable" || v == "disable" { + t.Errorf("boot state was changed by a declaration that did not mention it: %v", verbs) + } + } +} + +func TestAStaticUnitCannotBeEnabled(t *testing.T) { + // `static` is neither enabled nor disabled: the unit has no 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". + var verbs []string + d := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"rt","type":"service","unit":"dbus.socket","state":"running","boot":"enabled"} + ]}`) + + _, _, err := Apply(context.Background(), d, store.State{}, + systemctlStub(t, "loaded", "active", "static", &verbs), nil) + if err == nil { + t.Fatal("a static unit was accepted as enable-able") + } + if !strings.Contains(err.Error(), "no install section") { + t.Errorf("failed for the wrong reason: %v", err) + } +} + +func TestAnUnknownBootStateIsRefusedNotGuessed(t *testing.T) { + var verbs []string + d := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"rt","type":"service","unit":"x.service","state":"running","boot":"enabled"} + ]}`) + _, _, err := Apply(context.Background(), d, store.State{}, + systemctlStub(t, "loaded", "active", "indirect", &verbs), nil) + if err == nil { + t.Fatal("an unrecognised boot state was guessed at instead of refused") + } +} + +// --- more than one container runtime (novox/hq ADR 0060) --- + +func TestTheRuntimeProbeIsPerRuntime(t *testing.T) { + // Verified against a real podman 6.1.0 before this was written: + // + // docker info --format '{{.ServerVersion}}' -> 29.7.2 + // podman info --format '{{.ServerVersion}}' -> Error: can't evaluate field + // ServerVersion in type system.infoReport + // podman info --format '{{.Version.Version}}' -> 6.1.0 + // + // So a single probe cannot find both, and a host that used docker's would report a machine + // with podman as having no container runtime at all. + for _, tc := range []struct { + name, present, wantProbe string + }{ + {"docker", "docker", "{{.ServerVersion}}"}, + {"podman", "podman", "{{.Version.Version}}"}, + } { + t.Run(tc.name, func(t *testing.T) { + var probedWith string + run := func(ctx context.Context, name string, args ...string) (string, error) { + if name != tc.present { + return "", errors.New("not installed") + } + if args[0] == "info" { + probedWith = args[2] + } + return "ok\n", nil + } + got, err := containerRuntime(context.Background(), run) + if err != nil { + t.Fatalf("%s was present and was not found: %v", tc.present, err) + } + if got != tc.present { + t.Errorf("found %q, expected %q", got, tc.present) + } + if probedWith != tc.wantProbe { + t.Errorf("probed %s with %q; that template does not work on it", + tc.present, probedWith) + } + }) + } +} + +func TestAContainerUsesTheRuntimeTheMachineHas(t *testing.T) { + // The applier must not call `docker` on a machine that has podman. Adoption keeps what the + // 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) { + if name == "docker" { + return "", errors.New("not installed") + } + calledWith = append(calledWith, name) + switch args[0] { + case "info": + return "6.1.0\n", nil + case "inspect": + return "false\t\n", errors.New("no such container") + case "run": + return "deadbeef\n", nil + } + return "", nil + } + d := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"store","type":"container","name":"store","image":"`+pinned+`"} + ]}`) + + // It will fail at read-back — the stub never reports it running — and what matters is + // WHICH binary it used getting there. + _, _, _ = Apply(context.Background(), d, store.State{}, run, nil) + + for _, c := range calledWith { + if c != "podman" { + t.Errorf("called %q on a machine that only has podman", c) + } + } + if len(calledWith) == 0 { + t.Error("nothing was called; the runtime was not found") + } +} + +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) { + return "", errors.New("not installed") + } + d := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"store","type":"container","name":"store","image":"`+pinned+`"} + ]}`) + + _, _, err := Apply(context.Background(), d, store.State{}, run, nil) + if err == nil { + t.Fatal("a machine with no container runtime applied a container") + } + for _, want := range []string{"docker", "podman", "no container runtime"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the failure does not mention %q: %v", want, err) + } + } +} diff --git a/internal/declaration/declaration.go b/internal/declaration/declaration.go index f500777..0fa9af2 100644 --- a/internal/declaration/declaration.go +++ b/internal/declaration/declaration.go @@ -98,11 +98,21 @@ func (f *File) validate(where string, _ bool) []string { } // Service is a unit the host puts into a state. It does not install the unit. +// +// Two states, and they are orthogonal rather than one scale. A unit can be enabled and stopped +// (it will come back at boot), or disabled and running (started by hand, gone after a reboot). +// Folding them into one field would make the second expressible only by accident. type Service struct { ID string `json:"id"` Type Type `json:"type"` Unit string `json:"unit"` State string `json:"state"` + // Boot is "enabled" or "disabled" — whether the unit starts at boot. Optional: absent means + // the host asserts nothing about it and leaves whatever is there. + // + // Without this the host could start a unit and not make it survive a reboot, which is a + // declaration that reports success and stops being true at the next power cut. + Boot string `json:"boot,omitempty"` } func (s *Service) Identity() string { return s.ID } @@ -118,6 +128,11 @@ func (s *Service) validate(where string, _ bool) []string { problems = append(problems, fmt.Sprintf( "%s: state %q; a service is \"running\" or \"stopped\"", where, s.State)) } + if s.Boot != "" && s.Boot != "enabled" && s.Boot != "disabled" { + problems = append(problems, fmt.Sprintf( + "%s: boot %q; a service is \"enabled\" or \"disabled\" at boot, or omits it to "+ + "leave the machine's own setting alone", where, s.Boot)) + } return problems }