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