From 5dd439df5042742c61df8f283daf27adc697ef1c Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 24 Sep 2026 19:50:16 +0200 Subject: [PATCH] =?UTF-8?q?inspect=20by=20kind,=20not=20the=20ambiguous=20?= =?UTF-8?q?bare=20form=20=E2=80=94=20a=20same-named=20network=20stops=20a?= =?UTF-8?q?=20container=20from=20ever=20being=20found?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docker inspect resolves across every object kind, not just containers. A module regularly names a network the same as the container that joins it (keycloak does this today, ordinarily) — so when the container does not exist yet but the same-named network already does, the bare form answers with the network's JSON instead of reporting the container absent, and the template these callers use (.State.Running) fails to execute against it entirely. Live on novox tonight: minio's LB container, named the same as its network ("minio"), could never be created — every apply crashed on "the container runtime could not say whether minio is here", stuck since first push, because the check itself never got a clean answer. Fixed at every call site asking a container's state by name (containerState, inspectFound, NamesFree, raiseGiteaServer, containerRunning) by scoping to `docker container inspect`, matching the type-scoped form this codebase already uses correctly for networks, volumes and images elsewhere. Also scoped the one image inspect that was still bare (publish.go), for the same reason. mesh-host runs as a host-level service (nox-mesh-host.service), not a Docker module — merging this does not redeploy it. The live novox failure persists until the service itself is rebuilt and updated. --- internal/apply/apply.go | 6 ++- internal/apply/apply_test.go | 16 +++--- internal/apply/hold.go | 11 +++- internal/apply/hold_test.go | 9 ++-- internal/apply/inspect_scope_test.go | 66 +++++++++++++++++++++++ internal/apply/runonce_test.go | 4 +- internal/apply/schedule_test.go | 2 +- internal/apply/vocabulary_test.go | 2 +- internal/bootstrap/adopt_in_place_test.go | 2 +- internal/bootstrap/phase_packages.go | 5 +- internal/bootstrap/ports.go | 4 +- internal/bootstrap/ports_test.go | 2 +- internal/bootstrap/publish.go | 2 +- internal/bootstrap/publish_test.go | 8 +-- internal/bootstrap/registry_test.go | 2 +- internal/bootstrap/verify.go | 5 +- internal/bootstrap/verify_test.go | 14 ++--- 17 files changed, 123 insertions(+), 37 deletions(-) create mode 100644 internal/apply/inspect_scope_test.go diff --git a/internal/apply/apply.go b/internal/apply/apply.go index c063283..823ddfc 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -1347,11 +1347,15 @@ func containerSpecReading(r *declaration.Container, declares, reads map[string]s // containerState reports whether a container is running and which spec made it. // The error means the container does not exist. +// +// `container inspect`, not the bare form: a name is not unique across object kinds (a network and +// its container are routinely named alike), and the bare form can resolve to the wrong kind +// instead of reporting absence — see inspectFound in hold.go for the failure this produces. func containerState(ctx context.Context, name string, run Runner) (state struct { Running bool Spec string }, err error) { - out, err := run(ctx, "docker", "inspect", "--format", + out, err := run(ctx, "docker", "container", "inspect", "--format", "{{.State.Running}}\t{{index .Config.Labels \""+specLabel+"\"}}", name) if err != nil { return state, fmt.Errorf("no container named %s", name) diff --git a/internal/apply/apply_test.go b/internal/apply/apply_test.go index fea616b..31c04e5 100644 --- a/internal/apply/apply_test.go +++ b/internal/apply/apply_test.go @@ -635,7 +635,7 @@ func TestAContainerThatExitsImmediatelyFailsTheApply(t *testing.T) { switch { case args[0] == "info": return "27.0\n", nil - case args[0] == "inspect": + case args[0] == "container": return "false\t" + "", nil // exists, not running case args[0] == "run": return "deadbeef\n", nil @@ -673,7 +673,7 @@ func TestAContainerWhoseDeclarationChangedIsReplaced(t *testing.T) { switch args[0] { case "info": return "27.0\n", nil - case "inspect": + case "container": if created { return "true\t" + want, nil } @@ -711,7 +711,7 @@ func TestAContainerThatMatchesIsLeftAlone(t *testing.T) { switch args[0] { case "info": return "27.0\n", nil - case "inspect": + case "container": return "true\t" + spec, nil } touched = true @@ -756,7 +756,7 @@ func TestAContainerIsRecreatedWhenARestartOnResourceChanged(t *testing.T) { switch args[0] { case "info": return "27.0\n", nil - case "inspect": + case "container": // Already there and running, created against the file as it was. After the host // recreates it, the runtime holds the one it just made — as a real one would. if created { @@ -1000,7 +1000,7 @@ func TestAContainerUsesTheRuntimeTheMachineHas(t *testing.T) { switch args[0] { case "info": return "6.1.0\n", nil - case "inspect": + case "container": return "false\t\n", errors.New("no such container") case "run": return "deadbeef\n", nil @@ -1326,7 +1326,7 @@ func TestAContainerIsGivenTheMeshsNames(t *testing.T) { switch args[0] { case "info": return "29.0.0\n", nil - case "inspect": + case "container": return "false\t\n", errors.New("no such container") case "run": ran = args @@ -1361,7 +1361,7 @@ func TestAContainerGivenNoNamesIsRunAsBefore(t *testing.T) { switch args[0] { case "info": return "29.0.0\n", nil - case "inspect": + case "container": return "false\t\n", errors.New("no such container") case "run": ran = args @@ -1546,7 +1546,7 @@ func TestAContainerStaleFromAnEarlierApplyIsReplaced(t *testing.T) { switch args[0] { case "info": return "27.0\n", nil - case "inspect": + case "container": if created { return "true\t" + fresh, nil } diff --git a/internal/apply/hold.go b/internal/apply/hold.go index 3776c41..b65fad4 100644 --- a/internal/apply/hold.go +++ b/internal/apply/hold.go @@ -424,12 +424,21 @@ type foundContainer struct { // inspectFound reads a container by name the way a hold needs it: its id, whether it runs, and // whether a host made it. +// +// **`container inspect`, not the bare form.** A name is not unique across object kinds — a +// module regularly names a network the same as the container that joins it (`keycloak` names +// both, and it is ordinary). The bare form resolves across every kind and returns whichever it +// finds, so a container that does not exist yet but a same-named network does answers with the +// network's JSON — no `.State` field at all — and the template below fails to execute rather +// than failing to find anything. That reads as "the runtime could not say", which this function's +// caller correctly refuses to build on (novox/hq ADR 0100) — but there was something to say, a +// question of kind, not of ambiguity that should have stopped anything. func inspectFound(ctx context.Context, name string, run Runner) (foundContainer, bool, error) { cri, err := containerRuntime(ctx, run) if err != nil { return foundContainer{}, false, fmt.Errorf("%w, so nothing can be said about %q", err, name) } - out, err := run(ctx, cri, "inspect", "--format", + out, err := run(ctx, cri, "container", "inspect", "--format", "{{.Id}}\t{{.State.Running}}\t{{index .Config.Labels \""+specLabel+"\"}}", name) if err != nil { if absent(err) { diff --git a/internal/apply/hold_test.go b/internal/apply/hold_test.go index be2277c..850f254 100644 --- a/internal/apply/hold_test.go +++ b/internal/apply/hold_test.go @@ -120,7 +120,10 @@ func (m *machine) run(_ context.Context, name string, args ...string) (string, e return "", errors.New("no such volume") case "info": return "27.0\n", nil - case "inspect": + case "container": + if args[1] != "inspect" { + return "", errors.New("unexpected docker container command") + } c, ok := m.containers[args[len(args)-1]] if !ok { return "", errors.New("no such container") @@ -129,7 +132,7 @@ func (m *machine) run(_ context.Context, name string, args ...string) (string, e if c.running { running = "true" } - if strings.HasPrefix(args[2], "{{.Id}}") { + if strings.HasPrefix(args[3], "{{.Id}}") { return c.id + "\t" + running + "\t" + c.spec + "\n", nil } return running + "\t" + c.spec + "\n", nil @@ -916,7 +919,7 @@ func TestAVolumeTheRuntimeCannotBeAskedAboutStopsTheContainer(t *testing.T) { 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": + case name == "docker" && args[0] == "container": return "", errors.New("Error: No such object: hello-web") case name == "docker": return "", errors.New("docker run must not happen") diff --git a/internal/apply/inspect_scope_test.go b/internal/apply/inspect_scope_test.go new file mode 100644 index 0000000..aba3141 --- /dev/null +++ b/internal/apply/inspect_scope_test.go @@ -0,0 +1,66 @@ +package apply + +import ( + "context" + "errors" + "testing" +) + +// A name is not unique across object kinds: a module regularly names a network the same as the +// container that joins it (keycloak does this today, ordinarily). `docker inspect `, unlike +// `docker container inspect `, resolves across every kind — so when the container does not +// exist yet but a same-named network does, the bare form answers with the network's JSON instead +// of reporting the container absent. containerState and inspectFound must ask by kind, or a +// same-named network makes them unable to tell "not here yet" from "the runtime is broken" +// (novox/hq ADR 0100's refusal, tripped by nothing wrong). +// +// dockerLikeByKind is Docker's real behaviour, not the bug: `container inspect` only ever +// answers from the container namespace. A fake that also answered the bare, unscoped form would +// not catch a regression back to it — this one refuses to, on purpose. +func dockerLikeByKind(containers map[string]bool) func(context.Context, string, ...string) (string, error) { + return func(_ context.Context, name string, args ...string) (string, error) { + if name != "docker" { + return "", errors.New("unexpected program: " + name) + } + if len(args) > 0 && args[0] == "info" { + // containerRuntime's probe, answered so inspectFound gets past it to the check under + // test. + return "27.0\n", nil + } + if len(args) < 2 || args[0] != "container" || args[1] != "inspect" { + return "", errors.New("unexpected command: only `docker container inspect` is modelled here") + } + target := args[len(args)-1] + if !containers[target] { + return "", errors.New("Error: No such container: " + target) + } + return "false\t\n", nil + } +} + +func TestContainerStateAsksTheContainerNamespaceNotTheBareForm(t *testing.T) { + // "minio" exists only as a network in this scenario — never in `containers` — matching the + // live failure this guards: a module's network and its container share a name, and the + // container does not exist yet. + run := dockerLikeByKind(map[string]bool{"keycloak": true}) + + if _, err := containerState(context.Background(), "minio", run); err == nil { + t.Fatal("a container that does not exist should report absent, not be mistaken for found") + } + if state, err := containerState(context.Background(), "keycloak", run); err != nil { + t.Fatalf("a container that does exist should be found: %v", err) + } else if state.Running { + t.Errorf("the fake said not running; containerState disagreed: %+v", state) + } +} + +func TestInspectFoundAsksTheContainerNamespaceNotTheBareForm(t *testing.T) { + run := dockerLikeByKind(map[string]bool{"keycloak": true}) + + if _, exists, err := inspectFound(context.Background(), "minio", run); err != nil || exists { + t.Fatalf("a container that does not exist should be reported absent cleanly, not refused: exists=%v err=%v", exists, err) + } + if _, exists, err := inspectFound(context.Background(), "keycloak", run); err != nil || !exists { + t.Fatalf("a container that does exist should be found: exists=%v err=%v", exists, err) + } +} diff --git a/internal/apply/runonce_test.go b/internal/apply/runonce_test.go index 16b3919..442e71a 100644 --- a/internal/apply/runonce_test.go +++ b/internal/apply/runonce_test.go @@ -113,7 +113,7 @@ func TestAFailedRunOnceStepGatesWhatFollows(t *testing.T) { switch args[0] { case "info": return "27.0\n", nil - case "inspect": + case "container": return "false\t\n", errors.New("no such container") case "run": startedNames = append(startedNames, nameOf(args)) @@ -287,7 +287,7 @@ func TestAContainerNamingARunOnceStepIsRecreatedWhenItRan(t *testing.T) { switch args[0] { case "info": return "27.0\n", nil - case "inspect": + case "container": // The server is up, made from exactly this spec — nothing but the step's run says // it must be replaced. return "true\t" + spec, nil diff --git a/internal/apply/schedule_test.go b/internal/apply/schedule_test.go index 715b0db..6c3d4b7 100644 --- a/internal/apply/schedule_test.go +++ b/internal/apply/schedule_test.go @@ -208,7 +208,7 @@ func TestAScheduledStepDoesNotGateWhatFollows(t *testing.T) { switch args[0] { case "info": return "27.0\n", nil - case "inspect": + case "container": // The container name is the last argument to `docker inspect --format ... `. target := args[len(args)-1] if spec, up := specs[target]; up { diff --git a/internal/apply/vocabulary_test.go b/internal/apply/vocabulary_test.go index 1670f49..9c46dcc 100644 --- a/internal/apply/vocabulary_test.go +++ b/internal/apply/vocabulary_test.go @@ -376,7 +376,7 @@ func TestAContainerIsGivenItsEnvironmentFiles(t *testing.T) { var ran []string run := func(_ context.Context, name string, args ...string) (string, error) { ran = append(ran, name+" "+strings.Join(args, " ")) - if len(args) > 0 && args[0] == "inspect" { + if len(args) > 0 && args[0] == "container" { return "", fmt.Errorf("no such container") } return "", nil diff --git a/internal/bootstrap/adopt_in_place_test.go b/internal/bootstrap/adopt_in_place_test.go index 9814cf7..27a80d1 100644 --- a/internal/bootstrap/adopt_in_place_test.go +++ b/internal/bootstrap/adopt_in_place_test.go @@ -35,7 +35,7 @@ func (l *labelled) run(_ context.Context, _ string, args ...string) (string, err switch args[0] { case "info": return "27.0\n", nil - case "inspect": + case "container": spec, ok := l.spec[args[len(args)-1]] if !ok { return "", errors.New("no such container") diff --git a/internal/bootstrap/phase_packages.go b/internal/bootstrap/phase_packages.go index c2b00d7..a446b31 100644 --- a/internal/bootstrap/phase_packages.go +++ b/internal/bootstrap/phase_packages.go @@ -230,8 +230,9 @@ func raiseGiteaServer(ctx context.Context, run Runner, timeout time.Duration, db defer cancel() // Already there: a re-run does not raise a second one. `docker start` is a no-op on a running - // container and revives a stopped one. - if out, _ := run(asking, "docker", "inspect", "--format", "{{.Id}}", giteaBootstrap); strings.TrimSpace(out) != "" { + // container and revives a stopped one. `container inspect`, not the bare form: a name is not + // unique across object kinds, and `.Id` resolves on a network or volume too. + if out, _ := run(asking, "docker", "container", "inspect", "--format", "{{.Id}}", giteaBootstrap); strings.TrimSpace(out) != "" { _, _ = run(asking, "docker", "start", giteaBootstrap) return nil } diff --git a/internal/bootstrap/ports.go b/internal/bootstrap/ports.go index cb09372..110b791 100644 --- a/internal/bootstrap/ports.go +++ b/internal/bootstrap/ports.go @@ -431,7 +431,9 @@ func NamesFree(ctx context.Context, run Runner, names []string, known store.Stat sorted := append([]string{}, names...) sort.Strings(sorted) for _, name := range sorted { - out, err := run(ctx, "docker", "inspect", "--format", + // `container inspect`: a name is not unique across object kinds, and the bare form can + // resolve to a same-named network or volume instead of reporting the container absent. + out, err := run(ctx, "docker", "container", "inspect", "--format", "{{index .Config.Labels \"mesh-host.spec\"}}", name) if err != nil { continue // no such container diff --git a/internal/bootstrap/ports_test.go b/internal/bootstrap/ports_test.go index ac0ec92..b83e370 100644 --- a/internal/bootstrap/ports_test.go +++ b/internal/bootstrap/ports_test.go @@ -145,7 +145,7 @@ func (m machineRunner) run(_ context.Context, name string, args ...string) (stri return m.ss, nil case name == "docker" && args[0] == "ps": return m.ps, nil - case name == "docker" && args[0] == "inspect": + case name == "docker" && args[0] == "container": n := args[len(args)-1] if m.labelled[n] { return "abc\n", nil diff --git a/internal/bootstrap/publish.go b/internal/bootstrap/publish.go index 0b01ca7..561ce92 100644 --- a/internal/bootstrap/publish.go +++ b/internal/bootstrap/publish.go @@ -150,7 +150,7 @@ func digestOf(ctx context.Context, o Options, d Deps, remote string) (string, er // The registry has it. What digest, according to the runtime that pushed it. reading, cancel := context.WithTimeout(ctx, o.Timeout) - out, err := d.Run(reading, "docker", "inspect", "--format", "{{json .RepoDigests}}", + out, err := d.Run(reading, "docker", "image", "inspect", "--format", "{{json .RepoDigests}}", remote+":"+genesisTag) cancel() if err != nil { diff --git a/internal/bootstrap/publish_test.go b/internal/bootstrap/publish_test.go index c1b526d..9d0de66 100644 --- a/internal/bootstrap/publish_test.go +++ b/internal/bootstrap/publish_test.go @@ -50,7 +50,7 @@ func TestAnImageNoRegistryHasEverHeldIsPushed(t *testing.T) { case "push": pushed = true return "", nil - case "inspect": + case "image": return `["127.0.0.1:5000/mesh-controller@sha256:` + strings.Repeat("a", 64) + `"]`, nil } return "", fmt.Errorf("unexpected: %v", args) @@ -80,7 +80,7 @@ func TestAnImageTheRegistryAlreadyServesIsNotPushedAgain(t *testing.T) { return http.StatusOK, `{"name":"mesh-controller","tags":["genesis"]}`, nil }, func(_ string, args []string) (string, error) { - if args[0] == "inspect" { + if args[0] == "image" { return `["127.0.0.1:5000/mesh-controller@sha256:` + strings.Repeat("b", 64) + `"]`, nil } return "", fmt.Errorf("unexpected: %v", args) @@ -111,7 +111,7 @@ func TestTheDigestComesFromThisMeshsOwnRegistry(t *testing.T) { return http.StatusOK, `{"tags":["genesis"]}`, nil }, func(_ string, args []string) (string, error) { - if args[0] == "inspect" { + if args[0] == "image" { return `["` + elsewhere + `","` + ours + `"]`, nil } return "", fmt.Errorf("unexpected: %v", args) @@ -166,7 +166,7 @@ func TestATagIsNotAPin(t *testing.T) { return http.StatusOK, `{"tags":["genesis"]}`, nil }, func(_ string, args []string) (string, error) { - if args[0] == "inspect" { + if args[0] == "image" { return `["127.0.0.1:5000/mesh-controller:genesis"]`, nil } return "", nil diff --git a/internal/bootstrap/registry_test.go b/internal/bootstrap/registry_test.go index 4d86d71..d05bb83 100644 --- a/internal/bootstrap/registry_test.go +++ b/internal/bootstrap/registry_test.go @@ -62,7 +62,7 @@ func aMeshThatAgrees(answers map[string]string) func(string, []string) (string, return "", fmt.Errorf("unexpected program %q", name) case args[0] == "cp": return "", nil - case args[0] == "inspect": + case args[0] == "container": return "true running\n", nil case args[0] == "exec": return "", nil diff --git a/internal/bootstrap/verify.go b/internal/bootstrap/verify.go index 6285257..5f5ff66 100644 --- a/internal/bootstrap/verify.go +++ b/internal/bootstrap/verify.go @@ -129,8 +129,9 @@ func containerRunning(ctx context.Context, run Runner, probe time.Duration, name defer cancel() // Both facts in one answer, so a container that is not running is reported with what it IS - // rather than with the absence of what it should be. - out, err := run(asking, "docker", "inspect", "--format", "{{.State.Running}} {{.State.Status}}", name) + // rather than with the absence of what it should be. `container inspect`, not the bare form: + // a same-named network or volume would otherwise answer in the container's place. + out, err := run(asking, "docker", "container", "inspect", "--format", "{{.State.Running}} {{.State.Status}}", name) if err != nil { return containerState{}, fmt.Errorf( "the container %q is not there at all, and the apply reported it applied: %w", name, err) diff --git a/internal/bootstrap/verify_test.go b/internal/bootstrap/verify_test.go index c8decc6..14617b7 100644 --- a/internal/bootstrap/verify_test.go +++ b/internal/bootstrap/verify_test.go @@ -30,7 +30,7 @@ func TestAContainerThatIsUpIsNotAControlPlaneThatReplies(t *testing.T) { runtime := &asked{answer: func(_ string, args []string) (string, error) { switch args[0] { - case "inspect": + case "container": return "true running\n", nil case "exec": // Up, and saying nothing. The program inside is not answering. @@ -59,7 +59,7 @@ func TestAControlPlaneThatSaysNothingHasNotAnswered(t *testing.T) { defer func() { answerEvery = previous }() runtime := &asked{answer: func(_ string, args []string) (string, error) { - if args[0] == "inspect" { + if args[0] == "container" { return "true running\n", nil } return " \n", nil @@ -74,7 +74,7 @@ func TestAControlPlaneThatSaysNothingHasNotAnswered(t *testing.T) { // The foundation answering is the whole point, and what it said is reported rather than asserted. func TestAFoundationThatIsUpAndAnsweringIsAccepted(t *testing.T) { runtime := &asked{answer: func(_ string, args []string) (string, error) { - if args[0] == "inspect" { + if args[0] == "container" { return "true running\n", nil } return "1 node, 0 waiting\n", nil @@ -112,7 +112,7 @@ func TestAControlPlaneThatIsStillStartingIsWaitedFor(t *testing.T) { attempts := 0 runtime := &asked{answer: func(_ string, args []string) (string, error) { - if args[0] == "inspect" { + if args[0] == "container" { return "true running\n", nil } attempts++ @@ -132,10 +132,10 @@ func TestAControlPlaneThatIsStillStartingIsWaitedFor(t *testing.T) { // than being told only that something is not what it should be. func TestAContainerThatExitedIsNamedWithItsState(t *testing.T) { runtime := &asked{answer: func(_ string, args []string) (string, error) { - if args[0] == "inspect" && args[len(args)-1] == "mesh-broker" { + if args[0] == "container" && args[len(args)-1] == "mesh-broker" { return "false exited\n", nil } - if args[0] == "inspect" { + if args[0] == "container" { return "true running\n", nil } return "", fmt.Errorf("unexpected command: %v", args) @@ -155,7 +155,7 @@ func TestAContainerThatExitedIsNamedWithItsState(t *testing.T) { // `FROM scratch` and has no shell for a command line to be interpreted by. func TestTheControlPlaneIsAskedByRunningItsOwnBinary(t *testing.T) { runtime := &asked{answer: func(_ string, args []string) (string, error) { - if args[0] == "inspect" { + if args[0] == "container" { return "true running\n", nil } return "1 node\n", nil -- 2.54.0