diff --git a/internal/apply/apply.go b/internal/apply/apply.go index a87fcce..9905485 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -241,7 +241,7 @@ func applyOne(ctx context.Context, sys system.System, r declaration.Resource, ru case *declaration.Package: return applyPackage(ctx, sys, res, run) case *declaration.Container: - return applyContainer(ctx, res, run) + return applyContainer(ctx, res, run, changed) case *declaration.User: return applyUser(ctx, sys, res, run) case *declaration.Archive: @@ -521,13 +521,7 @@ func reflects(r *declaration.Service, changed map[string]bool) bool { // reflected is which of them changed, so the outcome can say why the service was restarted. A // restart with no reason given is indistinguishable from a service that keeps falling over. func reflected(r *declaration.Service, changed map[string]bool) []string { - var which []string - for _, id := range r.RestartOn { - if changed[id] { - which = append(which, id) - } - } - return which + return restartedBy(r.RestartOn, changed) } func applyService(ctx context.Context, sys system.System, r *declaration.Service, run Runner, @@ -886,7 +880,7 @@ func applyNetwork(ctx context.Context, r *declaration.Network, run Runner) (Outc return out, nil } -func applyContainer(ctx context.Context, r *declaration.Container, run Runner) (Outcome, error) { +func applyContainer(ctx context.Context, r *declaration.Container, run Runner, changed map[string]bool) (Outcome, error) { out := begin(r) want := containerSpec(r) @@ -898,8 +892,16 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner) ( before, err := containerState(ctx, r.Name, run) existed := err == nil + // A container reads a mounted file once, at start. When one of its restart-on resources changed + // this pass — a settings-merged config the runtime read, say — the file on disk is new and the + // running process still holds the old value, and the spec (image, env, volumes) has not moved, + // so the plain "spec matches, leave it" below would keep the stale process for ever + // (novox/hq 04-ISSUES/009). Recreating is how a container gets restart-on, which a service + // already has. + reasons := restartedBy(r.RestartOn, changed) + switch { - case existed && before.Spec == want && before.Running: + case existed && before.Spec == want && before.Running && len(reasons) == 0: out.Action = "unchanged" return out, nil case existed: @@ -959,11 +961,27 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner) ( out.Action = "created" if existed { out.Action = "updated" - out.Detail = "replaced; a container's configuration is fixed when it is created" + if len(reasons) > 0 { + out.Detail = "recreated to pick up " + strings.Join(reasons, ", ") + } else { + out.Detail = "replaced; a container's configuration is fixed when it is created" + } } return out, nil } +// restartedBy is which of the named resources changed this pass — the reason a container or service +// must be brought back rather than left as it is (novox/hq 04-ISSUES/009). +func restartedBy(restartOn []string, changed map[string]bool) []string { + var which []string + for _, id := range restartOn { + if changed[id] { + which = append(which, id) + } + } + return which +} + func sortedKeys(m map[string]string) []string { keys := make([]string, 0, len(m)) for k := range m { diff --git a/internal/apply/apply_test.go b/internal/apply/apply_test.go index 3800a22..0990fca 100644 --- a/internal/apply/apply_test.go +++ b/internal/apply/apply_test.go @@ -730,6 +730,67 @@ func TestAContainerThatMatchesIsLeftAlone(t *testing.T) { } } +func TestAContainerIsRecreatedWhenARestartOnResourceChanged(t *testing.T) { + // A container reads a mounted file once, at start. When the file changed this pass but the + // container's spec did not, the plain "spec matches, leave it" rule would keep the process + // holding the old value for ever, with every check passing (novox/hq 04-ISSUES/009). A + // container names the resources it must reflect in restart-on, the same as a service, and the + // host recreates it. Here the config file is fresh, so it is written this pass, and the + // already-running-and-matching container must still be replaced. + dir := t.TempDir() + conf := filepath.Join(dir, "config.json") + d := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"config","type":"file","path":"`+conf+`","content":"{\"token\":\"new\"}\n"}, + {"id":"app","type":"container","name":"app","image":"`+pinned+`","restart-on":["config"]} + ]}`) + spec := containerSpec(d.Resources[1].(*declaration.Container)) + + var removed, created bool + run := func(ctx context.Context, name string, args ...string) (string, error) { + switch args[0] { + case "info": + return "27.0\n", nil + case "inspect": + // The container already exists, running, with exactly the spec it is declared with — + // only the mounted file changed. + return "true\t" + spec, nil + case "rm": + removed = true + return "", nil + case "run": + created = true + return "deadbeef\n", nil + } + return "", nil + } + + report, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, run, nil, nil) + if err != nil { + t.Fatalf("apply failed: %v", err) + } + if !removed || !created { + t.Fatalf("a container was not recreated when its restart-on file changed (removed=%v created=%v)", removed, created) + } + app := report.Outcomes[len(report.Outcomes)-1] + if app.Action != "updated" || !strings.Contains(app.Detail, "config") { + t.Errorf("the recreation did not name why it happened: %+v", app) + } +} + +func TestRestartOnFiresOnlyForResourcesThatChanged(t *testing.T) { + // restart-on must not mean "always restart": it names resources, and only a resource that + // changed this pass is a reason. This is the guard shared by containers and services + // (novox/hq 04-ISSUES/009), so a container that reflects an unchanged file is left running. + changed := map[string]bool{"other": true} + if got := restartedBy([]string{"config"}, changed); len(got) != 0 { + t.Errorf("an unchanged resource was treated as a reason to restart: %v", got) + } + changed["config"] = true + if got := restartedBy([]string{"config", "missing"}, changed); len(got) != 1 || got[0] != "config" { + t.Errorf("restart-on did not name exactly the changed resource: %v", got) + } +} + // --- 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 diff --git a/internal/declaration/declaration.go b/internal/declaration/declaration.go index 98355f1..ed25882 100644 --- a/internal/declaration/declaration.go +++ b/internal/declaration/declaration.go @@ -462,6 +462,16 @@ type Container struct { // and guessing an address that works from inside a container, which is the same thing with a // worse failure mode. Network string `json:"network,omitempty"` + + // RestartOn names resources whose change means this container must be recreated — the same + // field a service has, for the same reason (novox/hq 04-ISSUES/009). A container reads a + // mounted file once at start; a changed file leaves the running process holding the old value, + // while every check passes because the file on disk is right. The container's spec — image, + // env, volumes — does not include a mounted file's *content*, so a settings change that + // re-renders that file is invisible to the ordinary spec diff. This closes that: the host + // recreates the container when one of these resources changed this pass, even if the spec + // matches. + RestartOn []string `json:"restart-on,omitempty"` } func (c *Container) Identity() string { return c.ID }