From c9001abdb42374f6d353e603fee3df639d411852 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 5 Oct 2026 22:21:21 +0200 Subject: [PATCH 1/2] Leave a unit alone when another declared service still holds it (hq issue 190) Removing one record of a unit gave back what that record found, even while another declared service holds the unit: when the private network's docker.service record goes and the docker module's stays, a record that found the runtime stopped would stop it, and every container with it, only for the docker module to start it again in the same apply. The record is forgotten instead, and the plan says so. A test pins the registry member moving from the network's record to the docker module's in one apply without leaving the list. --- internal/apply/apply.go | 32 +++++++ internal/apply/into_handover_test.go | 129 +++++++++++++++++++++++++++ internal/apply/plan.go | 5 ++ 3 files changed, 166 insertions(+) create mode 100644 internal/apply/into_handover_test.go diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 0521d3c..4bb1871 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -241,9 +241,30 @@ func ApplyMindingWindows( // this apply — a removal included — could change it or its records (novox/hq ADR 0103). before := lookBefore(ctx, sys, d, known, run) + // **A unit still declared with a state is not given back when another record of it goes** + // (novox/hq issue 190, ADR 0222). Two resources may name one unit — the private network declared + // the container runtime's service to reload it, beside the runtime's own module — and what the + // one going found is not the machine's to restore while the other still holds the unit: giving + // it back would stop or disable a unit the declaration says is running, every container on it + // with it, only for the declared one to start it again in the same apply. The record is + // forgotten; the unit's found state is the remaining record's to give back. + heldUnits := unitsHeld(d.Resources) + removeOrphan := func(orphan store.Applied) error { var action, detail string var err error + if declaration.Type(orphan.Type) == declaration.TypeService { + if by, held := heldUnits[unitKey(orphan.Scope, orphan.User, orphan.Target)]; held { + known.Forget(orphan.ID) + detail := "no longer declared; " + by + " still holds the unit, so it was left as it is" + report.Outcomes = append(report.Outcomes, Outcome{ + ID: orphan.ID, Type: orphan.Type, Target: orphan.Target, + Action: "forgotten", Detail: detail, + }) + log(fmt.Sprintf(" forgotten %s (%s): %s", orphan.ID, orphan.Target, detail)) + return nil + } + } if declaration.Type(orphan.Type) == declaration.TypeOpening { action, detail, err = removeOpening(ctx, orphan, run, known.Firewall) } else { @@ -2834,6 +2855,17 @@ func userUnitFiles(account, unit string) []string { // unitKey names a unit by the manager it is in and its name (novox/hq ADR 0177): the machine's // manager, or one account's. A system unit is the same key whether its record says "system" or // nothing, as every record before the scope existed does. +// unitsHeld is every unit a declared service gives a state, by unitKey, naming the service. +func unitsHeld(resources []declaration.Resource) map[string]string { + held := map[string]string{} + for _, r := range resources { + if s, ok := r.(*declaration.Service); ok && s.State != "" { + held[unitKey(s.Scope, s.User, s.Unit)] = s.ID + } + } + return held +} + func unitKey(scope, user, unit string) string { if scope == declaration.ScopeUser { return "user:" + user + "/" + unit diff --git a/internal/apply/into_handover_test.go b/internal/apply/into_handover_test.go new file mode 100644 index 0000000..640be24 --- /dev/null +++ b/internal/apply/into_handover_test.go @@ -0,0 +1,129 @@ +package apply + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/novox/mesh-host/internal/declaration" + "github.com/novox/mesh-host/internal/store" +) + +// novox/hq issue 190, ADR 0222: the private network stops writing the mesh's registry into the +// container runtime's file, and the runtime's own module writes the same member. Both are moved in +// one apply, and the machine never loses the member: the network's record goes first (its member +// leaves the list), the runtime's module is applied after (the member is added back, now recorded +// as the runtime's), and the daemon is reloaded once, never stopped, disabled or restarted — even +// though the record going says the unit was found stopped and disabled. + +const theRegistry = "anchor.internal:5100" + +func runtimeDecl(t *testing.T, path string, network bool) *declaration.Declaration { + t.Helper() + var resources []string + if network { + resources = append(resources, + fmt.Sprintf(`{"id":"networking.registry-trust","type":"file","path":%q,"into":"json","content":%q}`, + path, `{"insecure-registries":["`+theRegistry+`"]}`), + `{"id":"networking.registry-trust-reload","type":"service","unit":"docker.service","state":"running", + "reload-on":["networking.registry-trust"]}`) + } + resources = append(resources, + fmt.Sprintf(`{"id":"docker.daemon","type":"file","path":%q,"into":"json","content":%q}`, + path, `{"live-restore":true,"insecure-registries":["`+theRegistry+`"]}`), + `{"id":"docker.runtime","type":"service","unit":"docker.service","state":"running","boot":"enabled", + "reload-on":["docker.daemon"]}`) + return parse(t, `{"declaration":1,"resources":[`+strings.Join(resources, ",")+`]}`) +} + +func TestTheRegistryMovesToTheRuntimesModuleWithoutLeavingTheList(t *testing.T) { + path := filepath.Join(t.TempDir(), "daemon.json") + if err := os.WriteFile(path, []byte(`{"insecure-registries":["192.0.2.7:5000"]}`), 0o644); err != nil { + t.Fatal(err) + } + var commands []string + run := recordingServices(&commands) + + // Both declare it: the network's record added the member, so the runtime's finds it there. + _, state, err := Apply(context.Background(), archHost(t), runtimeDecl(t, path, true), store.State{}, + store.OriginDeclared, run, nil, nil) + if err != nil { + t.Fatal(err) + } + // Say the network's record found the runtime stopped and disabled: giving that back would stop + // every container on the machine. + for i := range state.Resources { + if state.Resources[i].ID == "networking.registry-trust-reload" { + state.Resources[i].Found = &store.FoundUnit{Unit: "docker.service", State: "stopped", Boot: "disabled"} + } + } + + commands = nil + report, state, err := Apply(context.Background(), archHost(t), runtimeDecl(t, path, false), state, + store.OriginDeclared, run, nil, nil) + if err != nil { + t.Fatal(err) + } + if got := fmt.Sprint(readObject(t, path)["insecure-registries"]); got != "[192.0.2.7:5000 "+theRegistry+"]" { + t.Fatalf("the list after the move: %s", got) + } + rec, _ := state.Find("docker.daemon") + if added := rec.Into.Added["insecure-registries"]; len(added) != 1 || canonical(added[0]) != `"`+theRegistry+`"` { + t.Errorf("the member is not recorded as the runtime's module's: %s", added) + } + reloads := 0 + for _, c := range commands { + if strings.Contains(c, "docker.service") && (strings.Contains(c, " stop ") || strings.Contains(c, " restart ") || + strings.Contains(c, " disable ")) { + t.Errorf("the runtime was given back as the network's record found it: %q", c) + } + if strings.Contains(c, "reload docker.service") { + reloads++ + } + } + if reloads != 1 { + t.Errorf("the runtime was reloaded %d times; commands were %v", reloads, commands) + } + for _, o := range report.Outcomes { + if o.ID == "networking.registry-trust-reload" && o.Action != "forgotten" { + t.Errorf("the network's record of the unit was %q: %s", o.Action, o.Detail) + } + } + + // Steady from here on, and undeclaring the runtime's module takes only what it added. + report, state, err = Apply(context.Background(), archHost(t), runtimeDecl(t, path, false), state, + store.OriginDeclared, run, nil, nil) + if err != nil { + t.Fatal(err) + } + for _, o := range report.Outcomes { + if o.ID == "docker.daemon" && o.Action != "unchanged" { + t.Errorf("a second apply was %q", o.Action) + } + } + if _, _, err := Apply(context.Background(), archHost(t), somethingElse(t), state, store.OriginDeclared, run, nil, nil); err != nil { + t.Fatal(err) + } + if got := fmt.Sprint(readObject(t, path)["insecure-registries"]); got != "[192.0.2.7:5000]" { + t.Errorf("undeclaring the runtime's module left %s", got) + } +} + +// And the preview says the same before it happens. +func TestThePlanForgetsARecordOfAUnitStillHeld(t *testing.T) { + path := filepath.Join(t.TempDir(), "daemon.json") + var commands []string + _, state, err := Apply(context.Background(), archHost(t), runtimeDecl(t, path, true), store.State{}, + store.OriginDeclared, recordingServices(&commands), nil, nil) + if err != nil { + t.Fatal(err) + } + for _, step := range Plan(runtimeDecl(t, path, false), state, store.OriginDeclared) { + if step.ID == "networking.registry-trust-reload" && step.Verb != "forget" { + t.Errorf("the plan would %s the network's record of a unit docker.runtime holds: %s", step.Verb, step.Why) + } + } +} diff --git a/internal/apply/plan.go b/internal/apply/plan.go index 2f1db0b..3a225be 100644 --- a/internal/apply/plan.go +++ b/internal/apply/plan.go @@ -84,10 +84,15 @@ func Plan(d *declaration.Declaration, known store.State, origin string) []Step { var protecting, orphans []Step made := meshMadeUnits(known) + heldUnits := unitsHeld(d.Resources) for _, orphan := range known.Orphans(declared, origin) { step := Step{Verb: "remove", Type: orphan.Type, ID: orphan.ID, Target: orphan.Target, Why: "recorded here and no longer declared"} + held := heldUnits[unitKey(orphan.Scope, orphan.User, orphan.Target)] switch { + case orphan.Type == string(declaration.TypeService) && held != "": + // In removeOrphan's words (novox/hq issue 190). + step.Verb, step.Why = "forget", "no longer declared; "+held+" still holds the unit, so it is left as it is" case orphan.Stateless: step.Verb, step.Why = "forget", "no longer declared; its unit's state was never the mesh's and is left as it is" case orphan.Type == string(declaration.TypeArchive) && store.IsFormer(orphan.ID): From f0f58732024cbc4b0151d8542e7e8d16e5666d6a Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 5 Oct 2026 22:27:32 +0200 Subject: [PATCH 2/2] Keep unitKey's comment on unitKey --- internal/apply/apply.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 4bb1871..292e4eb 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -2852,9 +2852,6 @@ func userUnitFiles(account, unit string) []string { return paths } -// unitKey names a unit by the manager it is in and its name (novox/hq ADR 0177): the machine's -// manager, or one account's. A system unit is the same key whether its record says "system" or -// nothing, as every record before the scope existed does. // unitsHeld is every unit a declared service gives a state, by unitKey, naming the service. func unitsHeld(resources []declaration.Resource) map[string]string { held := map[string]string{} @@ -2866,6 +2863,9 @@ func unitsHeld(resources []declaration.Resource) map[string]string { return held } +// unitKey names a unit by the manager it is in and its name (novox/hq ADR 0177): the machine's +// manager, or one account's. A system unit is the same key whether its record says "system" or +// nothing, as every record before the scope existed does. func unitKey(scope, user, unit string) string { if scope == declaration.ScopeUser { return "user:" + user + "/" + unit