diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 1ea2fda..586fa07 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -1167,14 +1167,13 @@ type inputs struct { // — an env-file a predecessor left, the superuser secret genesis writes before any declaration // names it — is read, so the digest is the same one the host records when it later writes the // same bytes there, and adopting a running store in place stays a reconcile rather than a -// recreate (bootstrap phase three). Empty when there is nothing readable there: the runtime -// refuses an absent env-file itself, with a better message than this could give. +// recreate (bootstrap phase three; genesis writes the value with no line ending for exactly this +// reason). Empty when there is nothing readable there: the runtime refuses an absent env-file +// itself, with a better message than this could give. func (in inputs) fileDigest(path string) string { if in.known != nil { - for _, f := range in.known.FilesUnder(path) { - if f.Target == path { - return f.Wrote - } + if f, recorded := in.known.At(string(declaration.TypeFile), path); recorded { + return f.Wrote } } info, err := os.Stat(path) @@ -1193,12 +1192,16 @@ func (in inputs) fileDigest(path string) string { // // - every env-file: the runtime reads it once, at create, and `docker restart` hands the // container the same environment it had. -// - a file bind-mounted into it, by its content — and, for a directory bind-mounted into it, -// every file THIS HOST wrote at or beneath the source: the secrets, bindings and configs it -// put under the module's state directories. What else is in a mounted directory is the -// service's own data, which changes while it runs and is nothing to recreate it for. +// - a file bind-mounted into it, DIRECTLY, by its content: a secret, a credential file. // -// A step is not here: a run-once or scheduled container reads its files when it runs, and +// **A directory bind-mounted into it is not looked inside**, not even for the files this host +// wrote there. What a service reads out of a mounted directory, and when, is the service's +// business: the route proxy re-reads its routes file live and would be recreated on every route +// change; a provisioner sidecar polls what it receives every few seconds and would be killed +// mid-reconcile on every grant. A module whose container does read such a file once, at start, +// says so with restart-on — that is what the field is for, and it stays the opt-in. +// +// A step is not here either: a run-once or scheduled container reads its files when it runs, and // runs fresh each time. Only a container that stays running holds what it read. func (in inputs) reads(r *declaration.Container) map[string]string { if r.RunOnce || r.Schedule != "" { @@ -1213,15 +1216,9 @@ func (in inputs) reads(r *declaration.Container) map[string]string { if !strings.HasPrefix(src, "/") { continue // a named volume: the runtime's, holding data } - if in.known != nil { - for _, f := range in.known.FilesUnder(src) { - out[f.Target] = f.Wrote - } - } - if _, recorded := out[src]; !recorded { - if digest := in.fileDigest(src); digest != "" { - out[src] = digest - } + // fileDigest is empty for a directory, and for anything else that is not a regular file. + if digest := in.fileDigest(src); digest != "" { + out[src] = digest } } if len(out) == 0 { @@ -1399,18 +1396,42 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner, // already has. reasons := restartedBy(r.RestartOn, changed) + // What this container was created reading, as last recorded. By its declared id first, and by + // its NAME when that id has no record: the bundle's `store` becomes the postgres module's + // `postgres.server`, the same container under a new id, and a file change on the day it is + // adopted is a real change with a real record — under the old id. + wasReading := previous.Reads + if previous.ID == "" && in.known != nil { + if byName, ok := in.known.At(string(declaration.TypeContainer), r.Name); ok { + wasReading = byName.Reads + } + } + // Which of the files it reads no longer hold what it was created reading. The spec label says // only that SOMETHING moved; the record of what was read says what — and that is the line a // person needs when a service went stale without a word (novox/hq 04-ISSUES/103). var changedFiles []string - for _, path := range sortedKeys(previous.Reads) { - if now, still := reads[path]; still && now != previous.Reads[path] { + for _, path := range sortedKeys(wasReading) { + if now, still := reads[path]; still && now != wasReading[path] { changedFiles = append(changedFiles, path) } } + // **A container labelled before the host folded in what it reads is accepted, not recreated.** + // + // Its label is the spec without the file lines. Recreating every such container on the first + // apply after the host upgraded would be a restart storm across the mesh in declaration order — + // the store first, under everything that uses it. So a label that matches the spec as it used + // to be computed is taken as current: what it reads is recorded now, and from the next apply + // on a changed file is caught by that record; the label itself is rewritten at the next + // genuine recreate. The trade-off, stated: a container that was ALREADY stale when the host + // upgraded — created against a file that has since changed — is not caught by this, and could + // not be by the alternative either, which recreates it without knowing whether it needed to. + legacy := len(reads) > 0 && before.Spec == containerSpecReading(r, in.declares, nil) && + (wasReading == nil || sameReads(wasReading, reads)) + switch { - case existed && before.Spec == want && before.Running && len(reasons) == 0: + case existed && (before.Spec == want || legacy) && before.Running && len(reasons) == 0: out.Action = "unchanged" return out, nil case existed: @@ -1478,10 +1499,6 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner, } case len(reasons) > 0: out.Detail = "recreated to pick up " + strings.Join(reasons, ", ") - case previous.Reads == nil && len(reads) > 0: - // A container made before this host kept what it read: whether the files it holds - // are the ones on disk cannot be known, so it is recreated once and from now on can. - out.Detail = "recreated: what it reads was not on record, so what it holds could not be checked" default: out.Detail = "replaced; a container's configuration is fixed when it is created" } @@ -1650,6 +1667,20 @@ func restartedBy(restartOn []string, changed map[string]bool) []string { return which } +// sameReads is whether two records of what a container reads name the same files holding the +// same content — a file added, dropped or changed makes them differ. +func sameReads(a, b map[string]string) bool { + if len(a) != len(b) { + return false + } + for path, digest := range a { + if b[path] != digest { + return false + } + } + return true +} + func sortedKeys(m map[string]string) []string { keys := make([]string, 0, len(m)) for k := range m { diff --git a/internal/apply/reads_test.go b/internal/apply/reads_test.go index a55507b..be2d8fe 100644 --- a/internal/apply/reads_test.go +++ b/internal/apply/reads_test.go @@ -132,24 +132,27 @@ func TestAContainerIsRecreatedWhenAMountedSecretChanged(t *testing.T) { } } -func TestWhatAServiceWritesInAMountedDirectoryIsNotPartOfWhatItIs(t *testing.T) { - // A bind-mounted directory is the service's data: it changes while the service runs, and - // recreating for it would restart a database for every row it wrote. What the HOST wrote - // under that directory — a config it rendered there — is another matter: the service read - // that once, at start. +func TestAMountedDirectoryIsNotLookedInside(t *testing.T) { + // A bind-mounted directory is not part of what a container is — not the data the service + // grows in it, and not the files the host itself writes there either. Whether a service reads + // a file under its directory once at start or watches it live is the service's business: the + // route proxy re-reads its routes live, a provisioner sidecar polls what it receives every few + // seconds, and recreating either for a file the host rewrote would kill them for nothing. A + // module whose container does read such a file once says so with restart-on, which stays the + // opt-in. dir := t.TempDir() state := filepath.Join(dir, "state") config := filepath.Join(state, "config.toml") - declare := func(level string) *declaration.Declaration { + declare := func(level, restartOn string) *declaration.Declaration { return parseTrusted(t, `{"declaration":1,"resources":[ {"id":"app.state","type":"directory","path":"`+state+`"}, {"id":"app.config","type":"file","path":"`+config+`","content":"level = \"`+level+`\"\n"}, {"id":"app.server","type":"container","name":"app","image":"`+pinned+`", - "volumes":["`+state+`:/var/lib/app"]} + "volumes":["`+state+`:/var/lib/app"]`+restartOn+`} ]}`) } m := &machine{containers: map[string]*fakeContainer{}} - _, known := applyCarried(t, declare("info"), store.State{}, m, nil) + _, known := applyCarried(t, declare("info", ""), store.State{}, m, nil) // The service grows its data in the directory it was given. if err := os.WriteFile(filepath.Join(state, "app.db"), []byte("rows"), 0o600); err != nil { @@ -162,20 +165,112 @@ func TestWhatAServiceWritesInAMountedDirectoryIsNotPartOfWhatItIs(t *testing.T) t.Fatal(err) } m.asked = nil - report, known := applyCarried(t, declare("info"), known, m, nil) + report, known := applyCarried(t, declare("info", ""), known, m, nil) if m.did("docker rm") || m.did("docker run") || report.Changed() { t.Errorf("a container was recreated for data its service wrote in a mounted directory: %v %+v", m.asked, report.Outcomes) } - // The host's own config under the same directory changes: that, the service read at start. + // The host rewrites its own file under the same directory: still not a reason. The container + // did not name it. m.asked = nil - report, _ = applyCarried(t, declare("debug"), known, m, nil) - if !m.removed("app") || !m.did("docker run") { - t.Fatalf("a config the host wrote under a mounted directory changed and the container was not recreated: %v", m.asked) + report, known = applyCarried(t, declare("debug", ""), known, m, nil) + if m.did("docker rm") || m.did("docker run") { + t.Errorf("a container was recreated for a file under a mounted directory it did not name: %v", m.asked) } - if o := outcomeOf(report, "app.server"); o.Detail != "recreated: "+config+" changed" { - t.Errorf("the recreate did not name the config: %+v", o) + if o := outcomeOf(report, "app.config"); o.Action != "updated" { + t.Fatalf("the config was not rewritten: %+v", o) + } + + // Naming it is what makes it a reason, as before this change. + m.asked = nil + report, _ = applyCarried(t, declare("trace", `,"restart-on":["app.config"]`), known, m, nil) + if !m.removed("app") || !m.did("docker run") { + t.Fatalf("a container naming a rewritten file under its mount was not recreated: %v", m.asked) + } + if o := outcomeOf(report, "app.server"); !strings.Contains(o.Detail, "app.config") { + t.Errorf("the recreate did not name why: %+v", o) + } +} + +func TestAContainerLabelledBeforeTheHostReadItsFilesIsAcceptedNotRecreated(t *testing.T) { + // The first apply after the host upgrades finds every container carrying a label computed + // without the file lines. Recreating them all would be a restart storm across the mesh in + // declaration order, the store first. A label that matches the spec as it used to be computed + // is accepted: what the container reads is recorded now, and from then on a change is caught. + dir := t.TempDir() + env := filepath.Join(dir, "forge.env") + declare := func(port string) *declaration.Declaration { + return parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"forge.env","type":"file","path":"`+env+`","content":"DATABASE_PORT=`+port+`\n","mode":"0600"}, + {"id":"forge.server","type":"container","name":"forge","image":"`+pinned+`","env-file":["`+env+`"]} + ]}`) + } + // The machine as the previous host left it: the file written and recorded, the container up + // under the label that host computed — the spec with nothing about the file's content. + if err := os.WriteFile(env, []byte("DATABASE_PORT=5432\n"), 0o600); err != nil { + t.Fatal(err) + } + d := declare("5432") + legacy := containerSpecReading(d.Resources[1].(*declaration.Container), nil, nil) + known := store.State{} + known.Record(store.Applied{ID: "forge.env", Type: "file", Origin: store.OriginCarried, Target: env, + Wrote: digestOf("DATABASE_PORT=5432\n")}) + known.Record(store.Applied{ID: "forge.server", Type: "container", Origin: store.OriginCarried, Target: "forge"}) + m := &machine{containers: map[string]*fakeContainer{"forge": {id: "made-by-host", running: true, spec: legacy}}} + + report, known := applyCarried(t, d, known, m, nil) + if m.did("docker rm") || m.did("docker run") || report.Changed() { + t.Fatalf("a container labelled by the previous host was recreated on upgrade: %v %+v", m.asked, report.Outcomes) + } + if got, _ := known.Find("forge.server"); got.Reads[env] != digestOf("DATABASE_PORT=5432\n") { + t.Fatalf("what the accepted container reads was not recorded: %+v", got) + } + // Accepted stays accepted: the next pass with nothing moved is quiet too. + m.asked = nil + report, known = applyCarried(t, d, known, m, nil) + if m.did("docker rm") || m.did("docker run") || report.Changed() { + t.Fatalf("an accepted container was recreated on the pass after: %v", m.asked) + } + + // And a change to the file is caught from the record, and the label is renewed. + m.asked = nil + report, _ = applyCarried(t, declare("5433"), known, m, nil) + if !m.removed("forge") || !m.did("docker run") { + t.Fatalf("an accepted container was not recreated when its env-file changed: %v", m.asked) + } + if o := outcomeOf(report, "forge.server"); o.Detail != "recreated: "+env+" changed" { + t.Errorf("the recreate did not name the file: %+v", o) + } + if m.containers["forge"].spec == legacy { + t.Error("the recreated container still carries the legacy label") + } +} + +func TestAContainerAdoptedUnderANewIdStillSaysWhichFileChanged(t *testing.T) { + // The bundle's `store` becomes the postgres module's `postgres.server`: the same container by + // name, under a new id with no record of its own. A file change on that day is a real change, + // and the record of what it read is under the old id — by name, it is found. + dir := t.TempDir() + env := filepath.Join(dir, "store.env") + m := &machine{containers: map[string]*fakeContainer{}} + raised := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"env","type":"file","path":"`+env+`","content":"PORT=5432\n","mode":"0600"}, + {"id":"store","type":"container","name":"mesh-store","image":"`+pinned+`","env-file":["`+env+`"]} + ]}`) + _, known := applyCarried(t, raised, store.State{}, m, nil) + + adopted := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"postgres.env","type":"file","path":"`+env+`","content":"PORT=5433\n","mode":"0600"}, + {"id":"postgres.server","type":"container","name":"mesh-store","image":"`+pinned+`","env-file":["`+env+`"]} + ]}`) + m.asked = nil + report, _, err := Apply(context.Background(), archHost(t), adopted, known, store.OriginDeclared, m.run, nil, nil) + if err != nil { + t.Fatal(err) + } + if o := outcomeOf(report, "postgres.server"); o.Action != "updated" || o.Detail != "recreated: "+env+" changed" { + t.Errorf("a container adopted under a new id did not say which file changed: %+v", o) } } diff --git a/internal/bootstrap/adopt_in_place_test.go b/internal/bootstrap/adopt_in_place_test.go new file mode 100644 index 0000000..9814cf7 --- /dev/null +++ b/internal/bootstrap/adopt_in_place_test.go @@ -0,0 +1,130 @@ +package bootstrap + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/novox/mesh-host/internal/apply" + "github.com/novox/mesh-host/internal/declaration" + "github.com/novox/mesh-host/internal/store" +) + +// Defends phase three's premise (phase3.go): the store genesis raised is adopted by the postgres +// module IN PLACE — same name, same image, same spec — so the applier reconciles it and never +// recreates the mesh's memory with the temporary control plane connected to it. +// +// The host folds a mounted file's content into the container's spec (novox/hq 04-ISSUES/103), so +// this now depends on a byte: the superuser file genesis writes and mounts must be the same bytes +// the module later declares. The module's value is what `secret accept` took — the operator's +// file with its line ending removed and nothing else (mesh-control, asSupplied). Reproduced before +// it was fixed: genesis wrote `value\n`, the module wrote `value`, and the store was recreated +// during install. + +// labelled is a runtime that keeps the spec label the host gives a container and hands it back. +type labelled struct { + spec map[string]string + created []string + removed []string +} + +func (l *labelled) run(_ context.Context, _ string, args ...string) (string, error) { + switch args[0] { + case "info": + return "27.0\n", nil + case "inspect": + spec, ok := l.spec[args[len(args)-1]] + if !ok { + return "", errors.New("no such container") + } + return "true\t" + spec + "\n", nil + case "rm": + l.removed = append(l.removed, args[len(args)-1]) + delete(l.spec, args[len(args)-1]) + case "run": + var name, spec string + for i, a := range args { + if a == "--name" { + name = args[i+1] + } + if a == "--label" && strings.HasPrefix(args[i+1], "mesh-host.spec=") { + spec = strings.TrimPrefix(args[i+1], "mesh-host.spec=") + } + } + l.spec[name] = spec + l.created = append(l.created, name) + return "made\n", nil + } + return "", nil +} + +func TestTheStoreGenesisRaisedIsAdoptedInPlaceNotRecreated(t *testing.T) { + dir := t.TempDir() + secret := filepath.Join(dir, "superuser.secret") + + // The bytes genesis really writes — the code path, not a fixture that agrees with it. + if _, made, err := keptOrMade(secret, false); err != nil || !made { + t.Fatalf("genesis did not make the superuser secret: made=%v err=%v", made, err) + } + onDisk, err := os.ReadFile(secret) + if err != nil { + t.Fatal(err) + } + + image := "docker.io/library/postgres@sha256:" + strings.Repeat("ab", 32) + mounts := `"volumes":["mesh-store-data:/var/lib/postgresql/data","` + secret + `:` + storeSuperuserMount + `:ro"]` + + // The foundation's store, as the produced bundle raises it (RewriteRoot): the file mounted, + // declared by nothing — genesis wrote it before there was a declaration to name it. + raise, err := declaration.ParseTrusted([]byte(`{"declaration":1,"resources":[ + {"id":"` + StoreID + `","type":"container","name":"mesh-store","image":"` + image + `", + "env":{"POSTGRES_PASSWORD_FILE":"` + storeSuperuserMount + `"},` + mounts + `} + ]}`)) + if err != nil { + t.Fatal(err) + } + runtime := &labelled{spec: map[string]string{}} + _, known, err := apply.Apply(context.Background(), arch(t), raise, store.State{}, store.OriginCarried, + runtime.run, nil, nil) + if err != nil { + t.Fatalf("raising the foundation's store: %v", err) + } + if len(runtime.created) != 1 { + t.Fatalf("the store was not raised once: %v", runtime.created) + } + + // The postgres module's declaration of the same store: the superuser file as `secret accept` + // took it in — its line ending removed and nothing else — then the same container. + accepted := strings.TrimRight(string(onDisk), "\r\n") + adopt, err := declaration.ParseTrusted([]byte(`{"declaration":1,"resources":[ + {"id":"postgres.superuser","type":"file","path":"` + secret + `","content":"` + accepted + `","mode":"0600"}, + {"id":"postgres.server","type":"container","name":"mesh-store","image":"` + image + `", + "env":{"POSTGRES_PASSWORD_FILE":"` + storeSuperuserMount + `"},` + mounts + `} + ]}`)) + if err != nil { + t.Fatal(err) + } + runtime.created, runtime.removed = nil, nil + report, _, err := apply.Apply(context.Background(), arch(t), adopt, known, store.OriginDeclared, + runtime.run, nil, nil) + if err != nil { + t.Fatalf("adopting the store: %v", err) + } + + if len(runtime.removed) > 0 || len(runtime.created) > 0 { + t.Fatalf("the module's declaration recreated the store genesis raised (removed %v, created %v): "+ + "the file genesis mounted and the file the module declares are not the same bytes", + runtime.removed, runtime.created) + } + for _, o := range report.Outcomes { + if o.ID == "postgres.server" && o.Action != "unchanged" { + t.Errorf("the store was not adopted in place: %+v", o) + } + if o.ID == "postgres.superuser" && o.Action != "unchanged" { + t.Errorf("the module rewrote the superuser file genesis wrote: %+v", o) + } + } +} diff --git a/internal/bootstrap/rootsecrets.go b/internal/bootstrap/rootsecrets.go index 06a81f2..4b5ec7c 100644 --- a/internal/bootstrap/rootsecrets.go +++ b/internal/bootstrap/rootsecrets.go @@ -102,8 +102,16 @@ func keptOrMade(path string, dryRun bool) (value string, made bool, err error) { } // Written whole and renamed into place, at 0600, owned by whoever runs the installer — root, // which is also who the host runs as when it later writes the sealed copy here. + // + // **The value alone, no line ending.** The module that adopts the store declares this same + // file, and what it declares is the value as `secret accept` took it — its line ending gone, + // by design. The host folds a mounted file's content into the container's spec (novox/hq + // 04-ISSUES/103), so a genesis that wrote `value\n` here would raise a store whose label + // digests one byte more than the module's file, and phase three would RECREATE the store it + // meant to adopt in place, with the temporary control plane connected to it. readCredentialFile + // tolerates either ending, so a file an earlier genesis wrote still reads. tmp := path + ".genesis" - if err := os.WriteFile(tmp, []byte(value+"\n"), 0o600); err != nil { + if err := os.WriteFile(tmp, []byte(value), 0o600); err != nil { return "", false, err } if err := os.Rename(tmp, path); err != nil { diff --git a/internal/declaration/declaration.go b/internal/declaration/declaration.go index 42aea06..d7e877d 100644 --- a/internal/declaration/declaration.go +++ b/internal/declaration/declaration.go @@ -785,13 +785,14 @@ type Container struct { // while every check passes because the file on disk is right. The host recreates the container // when one of these resources changed this pass, even if the spec matches. // - // What a running container reads at creation — its env-files, a file mounted into it, and the - // files the host wrote under a directory mounted into it — is part of its spec by content - // since novox/hq 04-ISSUES/103, and needs no naming here. RestartOn is for what the spec - // cannot see: a resource the container reflects without reading it directly, a step it - // consumes the result of. On a run-once step it means *run again*: a step that fetches a fact - // from a provider names the binding it reads, and is run again when the provider moved - // (novox/hq ADR 0099). + // What a running container reads at creation — its env-files, and a file mounted into it + // directly — is part of its spec by content since novox/hq 04-ISSUES/103, and needs no naming + // here. A directory mounted into it is NOT looked inside, not even for files the host wrote + // there: whether a service reads such a file once or watches it live is the service's, and + // RestartOn is how a module says "once, at start" — a config the host renders under the + // module's state directory, a step whose result it consumes. On a run-once step it means *run + // again*: a step that fetches a fact from a provider names the binding it reads, and is run + // again when the provider moved (novox/hq ADR 0099). RestartOn []string `json:"restart-on,omitempty"` // RunOnce marks a container the host runs to completion rather than leaves running: a step, diff --git a/internal/store/store.go b/internal/store/store.go index 09602e1..eb8264d 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -18,7 +18,6 @@ import ( "os" "path/filepath" "sort" - "strings" "time" ) @@ -209,24 +208,26 @@ func (s State) Recorded(kind, target string) bool { return false } -// FilesUnder returns every file this host has a record of writing at a path or beneath it, of any -// origin and under any id, in path order. +// At returns what this host has a record of putting at a target of this kind, under any id and of +// any origin — Recorded, with the record. // -// By path rather than by id because a container names what it reads by path: the id a file was -// declared under may change — the bundle's, then a module's, for the same file — and the container -// reading it does not care which. -func (s State) FilesUnder(path string) []Applied { - var out []Applied +// By target rather than by id because the id a thing was declared under may change while the thing +// does not: the bundle's `store` becomes a module's `postgres.server` for the same container, and +// the file that container reads is the same file under either id. Both ids may then hold a record +// for the one target — the bundle's is never removed by the mesh's declaration — and the most +// recently applied is the one that says what is there now. +func (s State) At(kind, target string) (Applied, bool) { + var latest Applied + found := false for _, r := range s.Resources { - if r.Type != "file" { + if r.Type != kind || r.Target != target { continue } - if r.Target == path || strings.HasPrefix(r.Target, strings.TrimSuffix(path, "/")+"/") { - out = append(out, r) + if !found || r.AppliedAt.After(latest.AppliedAt) { + latest, found = r, true } } - sort.Slice(out, func(i, j int) bool { return out[i].Target < out[j].Target }) - return out + return latest, found } // HeldAt returns what is held under a resource id.