From 101f0c0d61e6f019dca8135b18d16279eff50c06 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 5 Oct 2026 21:55:47 +0200 Subject: [PATCH] Restart a service only after every file it names is written (hq issue 260) A service was restarted where it was declared, so one declared ahead of a file in its restart-on was restarted before that file existed (the resolver's zones file, a fact appended after its module's resources), and a later change to such a file was never acted on. Each service is now applied right after the last resource it names under restart-on or reload-on; nothing else moves. --- internal/apply/apply.go | 80 +++++++++++++++++--- internal/apply/restart_order_test.go | 109 +++++++++++++++++++++++++++ 2 files changed, 180 insertions(+), 9 deletions(-) create mode 100644 internal/apply/restart_order_test.go diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 77a4dfd..0521d3c 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -339,21 +339,20 @@ func ApplyMindingWindows( } orphans = append(orphans, orphan) } - ordered := d.Resources + ordered := afterWhatTheyRead(d.Resources) guardFirst := 0 if d.Adoption != nil { - ordered = nil + // Each part ordered on its own, so a guard service never leaves the part applied first. + var guard, rest []declaration.Resource for _, r := range d.Resources { if strings.HasPrefix(r.Identity(), guardPrefix) { - ordered = append(ordered, r) - } - } - guardFirst = len(ordered) - for _, r := range d.Resources { - if !strings.HasPrefix(r.Identity(), guardPrefix) { - ordered = append(ordered, r) + guard = append(guard, r) + } else { + rest = append(rest, r) } } + ordered = append(afterWhatTheyRead(guard), afterWhatTheyRead(rest)...) + guardFirst = len(guard) } orphansRemoved := false removeOrphans := func() error { @@ -775,6 +774,69 @@ func ApplyMindingWindows( return report, known, nil } +// afterWhatTheyRead is the resources in declared order, except that **a service comes after every +// resource it names under restart-on or reload-on** (novox/hq issue 260). +// +// A service is started, restarted or reloaded where it is reached, and what it reads must be on the +// machine by then. Declared ahead of one of its files, it was restarted for the file before it while +// the other did not exist yet — the resolver, restarted on its configuration before the zones file +// that configuration names was written, failed its first start on every machine. And a change to a +// file applied after its service was never acted on at all: what moved is only known within one +// apply, and the service had already been passed. +// +// Only a service moves, and only as far as the last of what it names; everything else keeps its +// declared place. A name not in the list is ignored, as it is when the restart is decided. Only +// what is declared after a service is waited for, so no ring can form and nothing is left out. +func afterWhatTheyRead(resources []declaration.Resource) []declaration.Resource { + at := map[string]int{} + for i, r := range resources { + at[r.Identity()] = i + } + // What each service still waits for: the resources it names that come after it. + waits := map[int]map[string]bool{} + for i, r := range resources { + svc, ok := r.(*declaration.Service) + if !ok { + continue + } + for _, id := range append(append([]string{}, svc.RestartOn...), svc.ReloadOn...) { + if j, declared := at[id]; declared && j > i { + if waits[i] == nil { + waits[i] = map[string]bool{} + } + waits[i][id] = true + } + } + } + if len(waits) == 0 { + return resources + } + ordered := make([]declaration.Resource, 0, len(resources)) + placed := map[int]bool{} + var place func(i int) + place = func(i int) { + placed[i] = true + ordered = append(ordered, resources[i]) + id := resources[i].Identity() + // Whatever was waiting only for this comes now, in declared order. + for j := range resources { + if w := waits[j]; w != nil && w[id] { + delete(w, id) + if len(w) == 0 && !placed[j] { + delete(waits, j) + place(j) + } + } + } + } + for i := range resources { + if !placed[i] && len(waits[i]) == 0 { + place(i) + } + } + return ordered +} + // guardPrefix is the ids of the mesh's guard on an adopted node: its package, table, unit and // service (novox/hq ADR 0100). const guardPrefix = declaration.AdoptionPrefix + "guard" diff --git a/internal/apply/restart_order_test.go b/internal/apply/restart_order_test.go new file mode 100644 index 0000000..82a83ed --- /dev/null +++ b/internal/apply/restart_order_test.go @@ -0,0 +1,109 @@ +package apply + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/novox/mesh-host/internal/store" +) + +// The resolver's configuration named a second file, the mesh's zones, declared after the service — +// as a module's facts are, after its resources. The host restarted the service for the +// configuration, the daemon could not read the zones file the same apply had not written yet, and +// the apply failed on every machine (novox/hq issue 260). A service is restarted only once every +// resource it names under restart-on has been applied. +func TestAServiceIsRestartedOnlyOnceEveryFileItNamesIsWritten(t *testing.T) { + dir := t.TempDir() + conf := filepath.Join(dir, "resolver.conf") + zones := filepath.Join(dir, "zones.conf") + declare := func(zonesContent string) string { + return fmt.Sprintf(`{"declaration":1,"resources":[ + {"id":"resolver.config","type":"file","path":%q,"content":"conf-file=%s\n","mode":"0644"}, + {"id":"resolver.service","type":"service","unit":"resolver.service","state":"running", + "restart-on":["resolver.config","resolver.fact-zones"]}, + {"id":"resolver.fact-zones","type":"file","path":%q,"content":%q,"mode":"0644"} + ]}`, conf, zones, zones, zonesContent) + } + + // A daemon that cannot start without both of its files, as dnsmasq cannot. + var commands []string + services := recordingServices(&commands) + run := func(ctx context.Context, name string, args ...string) (string, error) { + if strings.Contains(strings.Join(args, " "), "start resolver.service") { + if _, err := os.Stat(zones); err != nil { + commands = append(commands, name+" "+strings.Join(args, " ")) + return "", errors.New("cannot read " + zones + ": no such file or directory") + } + } + return services(ctx, name, args...) + } + + report, state, err := Apply(context.Background(), archHost(t), parse(t, declare("server=/a/1\n")), + store.State{}, store.OriginCarried, run, nil, nil) + if err != nil { + t.Fatalf("the service was restarted before every file it reads was written: %v", err) + } + var detail string + for _, o := range report.Outcomes { + if o.ID == "resolver.service" { + detail = o.Detail + } + } + if !strings.Contains(detail, "resolver.config") || !strings.Contains(detail, "resolver.fact-zones") { + t.Errorf("one restart for both files was expected, the outcome says %q", detail) + } + restarts := 0 + for _, c := range commands { + if strings.Contains(c, "stop resolver.service") { + restarts++ + } + } + if restarts != 1 { + t.Errorf("the service was restarted %d times; once, after both files: %v", restarts, commands) + } + + // And a change to the file declared after the service alone is acted on in the apply that + // makes it — not passed by because the service was reached first. + commands = nil + if _, _, err := Apply(context.Background(), archHost(t), parse(t, declare("server=/b/2\n")), + state, store.OriginCarried, run, nil, nil); err != nil { + t.Fatal(err) + } + restarted := false + for _, c := range commands { + if strings.Contains(c, "stop resolver.service") { + restarted = true + } + } + if !restarted { + t.Errorf("the zones file changed and the service was not restarted: %v", commands) + } +} + +// Only a service moves, and only to just after the last resource it names — under restart-on or +// reload-on, in its own module or another's. Everything else keeps its declared place, and a name +// not in the declaration is no reason to move. +func TestAServiceIsOrderedAfterWhatItNamesAndNothingElseMoves(t *testing.T) { + d := parse(t, `{"declaration":1,"resources":[ + {"id":"a.dir","type":"directory","path":"/tmp/a"}, + {"id":"a.runtime","type":"service","unit":"docker.service","state":"running","reload-on":["b.daemon"]}, + {"id":"a.svc","type":"service","unit":"a.service","state":"running","restart-on":["a.conf","gone"]}, + {"id":"a.conf","type":"file","path":"/tmp/a/conf","content":"x\n"}, + {"id":"b.daemon","type":"file","path":"/tmp/b/daemon.json","content":"{}\n"}, + {"id":"b.after","type":"file","path":"/tmp/b/after","content":"y\n"}, + {"id":"b.svc","type":"service","unit":"b.service","state":"running","restart-on":["a.conf"]} + ]}`) + var got []string + for _, r := range afterWhatTheyRead(d.Resources) { + got = append(got, r.Identity()) + } + want := "a.dir a.conf a.svc b.daemon a.runtime b.after b.svc" + if strings.Join(got, " ") != want { + t.Errorf("ordered\n %s\nwant\n %s", strings.Join(got, " "), want) + } +}