A service can be declared to reflect a file

Because a running service does not re-read its configuration. Replace the file,
find the service running, do nothing -- and the machine keeps behaving as it
did while every check passes, because the file is right and the service is up.

That is not hypothetical. It is how a third node joining a mesh left the first
two carrying a private network that no longer existed, with every part of it
reporting success.

Declared state rather than a command: the declaration says the running service
must reflect these files, and the host works out that it does not. A command to
restart would be an action, and the link may not carry one -- the host refused
precisely that when I tried it, correctly, which is how this shape was arrived
at rather than the other.

Scoped to one apply. A change from an earlier one has already been reflected,
and restarting for it every time would make a steady machine bounce its
services for ever.

Also: the node generates its overlay key at enrolment and reports the public
half, and the store waits three minutes rather than one for the database --
sixty seconds is not enough for a cold machine running initdb, and it failed
that way three times, which is the worst kind of flake because a second run
always fixed it.

internal/apply/apply_test.go

@@ -3,6 +3,7 @@ package apply
 import (
 	"context"
 	"errors"
+	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
@@ -943,3 +944,118 @@ func archHost(t *testing.T) system.System {
 	}
 	return s
 }
+
+func TestAServiceIsRestartedWhenWhatItReflectsChanges(t *testing.T) {
+	// A running service does not re-read its configuration. Replace the file, find the service
+	// already running, do nothing — and the machine keeps behaving as it did while every check
+	// passes, because the file is right and the service is up.
+	//
+	// That is how a third node joining a mesh left the first two carrying a network that no
+	// longer existed. Found in the lab; this is the shape of the fix.
+	dir := t.TempDir()
+	path := filepath.Join(dir, "thing.conf")
+
+	d := parse(t, fmt.Sprintf(`{"declaration":1,"resources":[
+	  {"id":"conf","type":"file","path":%q,"content":"first\n","mode":"0644"},
+	  {"id":"svc","type":"service","unit":"thing.service","state":"running","restart-on":["conf"]}
+	]}`, path))
+
+	var commands []string
+	run := recordingServices(&commands)
+
+	if _, state, err := Apply(context.Background(), archHost(t), d, store.State{},
+		store.OriginCarried, run, nil); err != nil {
+		t.Fatal(err)
+	} else {
+		// Second apply with the same content: nothing moved, so nothing restarts. A machine that
+		// restarted its services on every reconcile would never be steady.
+		commands = nil
+		if _, _, err := Apply(context.Background(), archHost(t), d, state,
+			store.OriginCarried, run, nil); err != nil {
+			t.Fatal(err)
+		}
+		for _, c := range commands {
+			if strings.Contains(c, "stop") {
+				t.Errorf("an unchanged declaration restarted the service: %s", c)
+			}
+		}
+
+		// Now the file changes. The service is already running and must still be restarted.
+		changedDecl := parse(t, fmt.Sprintf(`{"declaration":1,"resources":[
+		  {"id":"conf","type":"file","path":%q,"content":"second\n","mode":"0644"},
+		  {"id":"svc","type":"service","unit":"thing.service","state":"running","restart-on":["conf"]}
+		]}`, path))
+		commands = nil
+		if _, _, err := Apply(context.Background(), archHost(t), changedDecl, state,
+			store.OriginCarried, run, nil); err != nil {
+			t.Fatal(err)
+		}
+		var stopped, started bool
+		for _, c := range commands {
+			if strings.Contains(c, "stop thing.service") {
+				stopped = true
+			}
+			if strings.Contains(c, "start thing.service") {
+				started = true
+			}
+		}
+		if !stopped || !started {
+			t.Errorf("the file changed and the service was not restarted; commands were %v", commands)
+		}
+	}
+}
+
+func TestAServiceIsNotRestartedByAChangeItDoesNotName(t *testing.T) {
+	// The list is what it reflects, not everything in the declaration. A service restarted by any
+	// change anywhere would make every apply a fleet-wide bounce.
+	dir := t.TempDir()
+	conf := filepath.Join(dir, "thing.conf")
+	other := filepath.Join(dir, "unrelated")
+
+	first := parse(t, fmt.Sprintf(`{"declaration":1,"resources":[
+	  {"id":"conf","type":"file","path":%q,"content":"same\n","mode":"0644"},
+	  {"id":"other","type":"file","path":%q,"content":"one\n","mode":"0644"},
+	  {"id":"svc","type":"service","unit":"thing.service","state":"running","restart-on":["conf"]}
+	]}`, conf, other))
+
+	var commands []string
+	run := recordingServices(&commands)
+	_, state, err := Apply(context.Background(), archHost(t), first, store.State{},
+		store.OriginCarried, run, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	second := parse(t, fmt.Sprintf(`{"declaration":1,"resources":[
+	  {"id":"conf","type":"file","path":%q,"content":"same\n","mode":"0644"},
+	  {"id":"other","type":"file","path":%q,"content":"two\n","mode":"0644"},
+	  {"id":"svc","type":"service","unit":"thing.service","state":"running","restart-on":["conf"]}
+	]}`, conf, other))
+	commands = nil
+	if _, _, err := Apply(context.Background(), archHost(t), second, state,
+		store.OriginCarried, run, nil); err != nil {
+		t.Fatal(err)
+	}
+	for _, c := range commands {
+		if strings.Contains(c, "stop") {
+			t.Errorf("a change to a file the service does not name restarted it: %s", c)
+		}
+	}
+}
+
+// recordingServices answers the way a machine with a running unit would, and remembers what it
+// was asked to do — which is what a restart has to be proved by, since "running" looks the same
+// before and after one.
+func recordingServices(commands *[]string) Runner {
+	return func(_ context.Context, name string, args ...string) (string, error) {
+		line := name + " " + strings.Join(args, " ")
+		*commands = append(*commands, line)
+		switch {
+		case strings.Contains(line, "is-enabled"):
+			return "enabled", nil
+		case strings.Contains(line, "show") && strings.Contains(line, "ActiveState"):
+			return "LoadState=loaded\nActiveState=active\nSubState=running", nil
+		}
+		return "", nil
+	}
+}