From da008460ac7ac0cce75cdaa87e6a4b8c77490783 Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 22 Sep 2026 19:46:50 +0200 Subject: [PATCH] Fail a resource when the container runtime cannot answer, instead of reading silence as nothing there (hq ADR 0100) --- internal/apply/hold.go | 74 ++++++++++++++++++++++++++-------- internal/apply/hold_test.go | 79 +++++++++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 16 deletions(-) diff --git a/internal/apply/hold.go b/internal/apply/hold.go index a71bf4d..8c2497e 100644 --- a/internal/apply/hold.go +++ b/internal/apply/hold.go @@ -51,11 +51,21 @@ func KeepIn(dir string) Keep { // record (novox/hq ADR 0103). Looked at first, because the apply itself makes such things — a // file's parent directory, a unit file a module writes, a package that brings its unit — and what // the mesh made in this apply was not found. -type foundBefore map[string]bool +type foundBefore struct { + is map[string]bool + // trouble is what could not be asked about, by the same key, so the resource that would need + // the answer fails rather than proceeding as if the machine had nothing there. + trouble map[string]string +} + +func (f foundBefore) has(key string) bool { return f.is[key] } + +// why is the reason a key could not be settled, and empty when there was none. +func (f foundBefore) why(key string) string { return f.trouble[key] } func lookBefore(ctx context.Context, sys system.System, d *declaration.Declaration, known store.State, run Runner) foundBefore { - seen := foundBefore{} + seen := foundBefore{is: map[string]bool{}, trouble: map[string]string{}} if d.Adoption == nil { return seen } @@ -70,25 +80,25 @@ func lookBefore(ctx context.Context, sys system.System, d *declaration.Declarati switch res := r.(type) { case *declaration.Directory: if present(res.Path) && !recordedPath(known, res.Path) { - seen["path:"+res.Path] = true + seen.is["path:"+res.Path] = true } case *declaration.Archive: // Unpacking over it, and re-owning it recursively, would change the predecessor's // files. if present(res.Path) && !recordedPath(known, res.Path) { - seen["path:"+res.Path] = true + seen.is["path:"+res.Path] = true } case *declaration.Process: // Its unit would be written over and restarted. if !known.Recorded(string(declaration.TypeProcess), res.Name) && present(filepath.Join(unitDir, res.Name+".service")) { - seen["unit-file:"+res.Name] = true + seen.is["unit-file:"+res.Name] = true } case *declaration.User: // Its shell and groups would be changed. if !known.Recorded(string(declaration.TypeUser), res.Name) { if _, exists, err := system.LookUpUser(ctx, run, res.Name); err == nil && exists { - seen["user:"+res.Name] = true + seen.is["user:"+res.Name] = true } } case *declaration.Service: @@ -105,7 +115,7 @@ func lookBefore(ctx context.Context, sys system.System, d *declaration.Declarati } boot, _ := sys.ServiceBoot(ctx, run, res.Unit) if state == "running" || boot == "enabled" { - seen["unit:"+res.Unit] = true + seen.is["unit:"+res.Unit] = true } case *declaration.Container: if known.Recorded(string(declaration.TypeContainer), res.Name) { @@ -117,7 +127,7 @@ func lookBefore(ctx context.Context, sys system.System, d *declaration.Declarati case src == "": case strings.HasPrefix(src, "/"): if !systemPath(src) && present(src) && !recordedPath(known, src) { - seen["path:"+src] = true + seen.is["path:"+src] = true } default: if !asked { @@ -128,7 +138,10 @@ func lookBefore(ctx context.Context, sys system.System, d *declaration.Declarati continue } if _, err := run(ctx, cri, "volume", "inspect", src); err == nil { - seen["volume:"+src] = true + seen.is["volume:"+src] = true + } else if !absent(err) { + seen.trouble["volume:"+src] = fmt.Sprintf( + "the container runtime could not say whether the volume %s is here: %v", src, err) } } } @@ -287,21 +300,28 @@ func holdOnAdopted(ctx context.Context, sys system.System, r declaration.Resourc if strings.HasPrefix(src, "/") { key = "path:" + src } - if src != "" && before[key] { + if src == "" { + continue + } + if trouble := before.why(key); trouble != "" { + return false, false, begin(r), fmt.Errorf( + "%s, so it is not safe to create a container that would mount it", trouble) + } + if before.has(key) { isFound, why = true, "would mount "+src+", found on the machine" break } } case *declaration.Directory: - isFound = before["path:"+res.Path] + isFound = before.has("path:" + res.Path) case *declaration.Archive: - isFound = before["path:"+res.Path] + isFound = before.has("path:" + res.Path) case *declaration.Process: - isFound = before["unit-file:"+res.Name] + isFound = before.has("unit-file:" + res.Name) case *declaration.User: - isFound = before["user:"+res.Name] + isFound = before.has("user:" + res.Name) case *declaration.Service: - isFound = before["unit:"+res.Unit] + isFound = before.has("unit:" + res.Unit) } } if !isFound { @@ -373,7 +393,16 @@ func inspectFound(ctx context.Context, name string, run Runner) (foundContainer, out, err := run(ctx, cri, "inspect", "--format", "{{.Id}}\t{{.State.Running}}\t{{index .Config.Labels \""+specLabel+"\"}}", name) if err != nil { - return foundContainer{}, false, nil + if absent(err) { + return foundContainer{}, false, nil + } + // **A runtime that could not answer is not a machine with nothing there.** Read as + // absence, a daemon that is down or a permission denied would let the mesh create its own + // container over a predecessor's — the one thing an adopted node must never do + // (novox/hq ADR 0100). + return foundContainer{}, false, fmt.Errorf( + "the container runtime could not say whether %s is here, so it is not safe to make one: %w", + name, err) } parts := strings.Split(strings.TrimSpace(out), "\t") for len(parts) < 3 { @@ -386,6 +415,19 @@ func inspectFound(ctx context.Context, name string, run Runner) (foundContainer, return foundContainer{id: strings.TrimSpace(parts[0]), running: parts[1] == "true", spec: spec}, true, nil } +// absent is whether a runtime said the thing is not there, rather than failing to answer. Its own +// words: docker and podman both say "No such object", "No such container" or "No such volume". +func absent(err error) bool { + said := strings.ToLower(err.Error()) + for _, missing := range []string{"no such object", "no such container", "no such volume", + "no such image"} { + if strings.Contains(said, missing) { + return true + } + } + return false +} + // hold keeps a found file or container as it is, and reports it — the first time by recording // what was found, every time after by comparing against that. Nothing is reverted, restarted or // created: a held target that disappears stays held and gone until its module is taken. diff --git a/internal/apply/hold_test.go b/internal/apply/hold_test.go index b2efcb6..cdf8232 100644 --- a/internal/apply/hold_test.go +++ b/internal/apply/hold_test.go @@ -825,3 +825,82 @@ func TestMountingTheMachinesOwnPlumbingIsNotFoundData(t *testing.T) { t.Errorf("held: %+v", state.Held) } } + +// Defends novox/hq ADR 0100: a runtime that cannot answer is not a machine with nothing there. + +// unreachable is a machine whose container runtime answers everything with a daemon that is down. +type unreachable struct{ asked []string } + +func (u *unreachable) run(_ context.Context, name string, args ...string) (string, error) { + u.asked = append(u.asked, name+" "+strings.Join(args, " ")) + if name == "docker" && args[0] == "info" { + return "27.0\n", nil + } + if name == "docker" { + return "", errors.New("docker exited 1: Cannot connect to the Docker daemon at unix:///var/run/docker.sock") + } + return "", nil +} + +func TestARuntimeThatCannotAnswerNeverLetsTheMeshCreateOverAFoundContainer(t *testing.T) { + dir := t.TempDir() + u := &unreachable{} + d := adopted(t, untaken("hello-web.server"), + `{"id":"hello-web.server","type":"container","name":"hello-web","image":"`+pinned+`"}`) + _, state, err := ApplyKeeping(context.Background(), archHost(t), d, store.State{}, store.OriginDeclared, + u.run, nil, nil, KeepIn(dir)) + if err == nil || !strings.Contains(err.Error(), "not safe to make one") { + t.Fatalf("a runtime that could not answer was read as nothing there: %v", err) + } + for _, a := range u.asked { + if strings.HasPrefix(a, "docker run") || strings.HasPrefix(a, "docker rm") { + t.Errorf("the mesh acted on a container it could not ask about: %s", a) + } + } + if len(state.Held) != 0 { + t.Errorf("something was held on an answer the machine never gave: %+v", state.Held) + } +} + +func TestAHeldContainerStaysHeldWhenTheRuntimeCannotAnswer(t *testing.T) { + dir, page, m := predecessor(t) + d := adopted(t, untaken("hello-web.page", "hello-web.server"), webResources(page)) + _, state := applyAdopted(t, d, store.State{}, m, dir) + if _, held := state.HeldAt("hello-web.server"); !held { + t.Fatal("the found container was not held to begin with") + } + u := &unreachable{} + _, state, err := ApplyKeeping(context.Background(), archHost(t), d, state, store.OriginDeclared, + u.run, nil, nil, KeepIn(dir)) + if err == nil { + t.Fatal("a runtime that could not answer reported success") + } + if h, held := state.HeldAt("hello-web.server"); !held || h.Changed != "" { + t.Errorf("a hold was let go or called changed on an answer the machine never gave: %+v", h) + } +} + +func TestAVolumeTheRuntimeCannotBeAskedAboutStopsTheContainer(t *testing.T) { + dir := t.TempDir() + run := func(_ context.Context, name string, args ...string) (string, error) { + switch { + case name == "docker" && args[0] == "info": + return "27.0\n", nil + case name == "docker" && args[0] == "volume": + return "", errors.New("docker exited 1: Cannot connect to the Docker daemon") + case name == "docker" && args[0] == "inspect": + return "", errors.New("Error: No such object: hello-web") + case name == "docker": + return "", errors.New("docker run must not happen") + } + return "", nil + } + d := adopted(t, untaken("hello-web.server"), + `{"id":"hello-web.server","type":"container","name":"hello-web","image":"`+pinned+`", + "volumes":["predecessor-data:/data"]}`) + _, _, err := ApplyKeeping(context.Background(), archHost(t), d, store.State{}, store.OriginDeclared, + run, nil, nil, KeepIn(dir)) + if err == nil || !strings.Contains(err.Error(), "could not say whether the volume") { + t.Fatalf("a volume the runtime could not be asked about did not stop the container: %v", err) + } +}