Set a running container aside until its replacement runs, and put it back when the replacement does not come up (issue 1945)
This commit is contained in:
+42
-7
@@ -2964,6 +2964,7 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner,
|
||||
return out, nil
|
||||
}
|
||||
|
||||
aside := ""
|
||||
switch {
|
||||
case existed && (before.Spec == want || legacy) && before.Running && len(reasons) == 0:
|
||||
out.Action = "unchanged"
|
||||
@@ -2986,15 +2987,40 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner,
|
||||
if err := ensureImage(ctx, cri, r.Image, run); err != nil {
|
||||
return out, fmt.Errorf("not replacing container %s, which keeps running: %w", r.Name, err)
|
||||
}
|
||||
if _, err := run(ctx, cri, "rm", "-f", r.Name); err != nil {
|
||||
return out, fmt.Errorf("replacing container %s: %w", r.Name, err)
|
||||
// **The running container is set aside, never removed, until its replacement runs** (issue 1945): stopped and
|
||||
// renamed, so its name and ports are free, and started again under its own name when the replacement does not
|
||||
// come up. Removed first, the bus's own container left a node without a bus when its replacement was never
|
||||
// made (2026-10-25 at 11:23). A container set aside by an earlier apply that stopped midway goes first.
|
||||
aside = r.Name + asideSuffix
|
||||
_, _ = run(ctx, cri, "rm", "-f", aside)
|
||||
if _, err := run(ctx, cri, "rename", r.Name, aside); err != nil {
|
||||
return out, fmt.Errorf("replacing container %s: setting the running one aside: %w", r.Name, err)
|
||||
}
|
||||
if _, err := run(ctx, cri, "stop", aside); err != nil {
|
||||
_, _ = run(ctx, cri, "rename", aside, r.Name)
|
||||
return out, fmt.Errorf("replacing container %s: stopping the running one: %w", r.Name, err)
|
||||
}
|
||||
}
|
||||
|
||||
// putBack starts the container set aside again under its own name, after a replacement that did not come up.
|
||||
putBack := func(cause error) error {
|
||||
if aside == "" {
|
||||
return cause
|
||||
}
|
||||
_, _ = run(ctx, cri, "rm", "-f", r.Name)
|
||||
if _, err := run(ctx, cri, "rename", aside, r.Name); err != nil {
|
||||
return fmt.Errorf("%w; and the container it replaced could not be put back: %v", cause, err)
|
||||
}
|
||||
if _, err := run(ctx, cri, "start", r.Name); err != nil {
|
||||
return fmt.Errorf("%w; and the container it replaced could not be started again: %v", cause, err)
|
||||
}
|
||||
return fmt.Errorf("%w; the container it replaced was started again", cause)
|
||||
}
|
||||
|
||||
args := detachedRunArgs(r, want)
|
||||
|
||||
if _, err := run(ctx, cri, args...); err != nil {
|
||||
return out, fmt.Errorf("starting container %s: %w", r.Name, err)
|
||||
return out, putBack(fmt.Errorf("starting container %s: %w", r.Name, err))
|
||||
}
|
||||
|
||||
// Read back. `docker run --detach` returning an id says the container was created, not
|
||||
@@ -3002,15 +3028,21 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner,
|
||||
// command exactly as one that came up does.
|
||||
after, err := containerState(ctx, r.Name, run)
|
||||
if err != nil {
|
||||
return out, fmt.Errorf("started container %s and it is not there: %w", r.Name, err)
|
||||
return out, putBack(fmt.Errorf("started container %s and it is not there: %w", r.Name, err))
|
||||
}
|
||||
if !after.Running {
|
||||
return out, fmt.Errorf(
|
||||
return out, putBack(fmt.Errorf(
|
||||
"container %s was started and is not running. It exited; ask the runtime for its "+
|
||||
"logs", r.Name)
|
||||
"logs", r.Name))
|
||||
}
|
||||
if after.Spec != want {
|
||||
return out, fmt.Errorf("container %s is not the one that was declared after creating it", r.Name)
|
||||
return out, putBack(fmt.Errorf("container %s is not the one that was declared after creating it", r.Name))
|
||||
}
|
||||
// Only now, with the replacement running, is the container set aside removed.
|
||||
if aside != "" {
|
||||
if _, err := run(ctx, cri, "rm", "-f", aside); err != nil {
|
||||
out.Detail = fmt.Sprintf("the container it replaced, set aside as %s, could not be removed: %v", aside, err)
|
||||
}
|
||||
}
|
||||
// The found networks a per-machine setting keeps for it (novox/hq ADR 0163, rule 4), joined
|
||||
// once it runs: a runtime starts a container on one network, and the others are connected.
|
||||
@@ -3046,6 +3078,9 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner,
|
||||
return out, nil
|
||||
}
|
||||
|
||||
// asideSuffix names a running container set aside while its replacement is started (issue 1945).
|
||||
const asideSuffix = ".replaced"
|
||||
|
||||
// detachedRunArgs is the `docker run` that starts a service container and leaves it running.
|
||||
func detachedRunArgs(r *declaration.Container, want string) []string {
|
||||
args := []string{"run", "--detach", "--name", r.Name, "--restart", "unless-stopped"}
|
||||
|
||||
@@ -33,6 +33,7 @@ func newBusyMachine() *aBusyMachine {
|
||||
}
|
||||
|
||||
func (m *aBusyMachine) run(_ context.Context, _ string, args ...string) (string, error) {
|
||||
args = asideShape(args)
|
||||
m.mu.Lock()
|
||||
defer m.mu.Unlock()
|
||||
switch args[0] {
|
||||
|
||||
@@ -111,7 +111,10 @@ type fakeContainer struct {
|
||||
// options. The host passes `--` before every unit (third review of mesh-catalog #147), which
|
||||
// internal/system's own test holds; the fakes read the unit where they always did.
|
||||
func oldShape(name string, args []string) []string {
|
||||
if name != "systemctl" || len(args) < 2 || args[len(args)-2] != "--" {
|
||||
if name != "systemctl" {
|
||||
return asideShape(args)
|
||||
}
|
||||
if len(args) < 2 || args[len(args)-2] != "--" {
|
||||
return args
|
||||
}
|
||||
v := 0
|
||||
@@ -1128,3 +1131,16 @@ func TestAPathTheMeshOnlySetAccessOnIsStillFound(t *testing.T) {
|
||||
t.Errorf("it was re-moded to %o", info.Mode().Perm())
|
||||
}
|
||||
}
|
||||
|
||||
// asideShape reads the steps of a container set aside while its replacement starts (issue 1945) as the removal the
|
||||
// tests were written against: setting it aside is its removal, and what is done to the set-aside container afterwards
|
||||
// is no step of theirs.
|
||||
func asideShape(args []string) []string {
|
||||
if len(args) == 3 && args[0] == "rename" && args[2] == args[1]+asideSuffix {
|
||||
return []string{"rm", "-f", args[1]}
|
||||
}
|
||||
if len(args) > 0 && strings.HasSuffix(args[len(args)-1], asideSuffix) && (args[0] == "rm" || args[0] == "stop") {
|
||||
return []string{"aside"}
|
||||
}
|
||||
return args
|
||||
}
|
||||
|
||||
@@ -270,6 +270,7 @@ func newRegistryMachine(spec string) *aRegistryMachine {
|
||||
}
|
||||
|
||||
func (m *aRegistryMachine) run(_ context.Context, _ string, args ...string) (string, error) {
|
||||
args = asideShape(args)
|
||||
m.mu.Lock()
|
||||
switch args[0] {
|
||||
case "info", "image":
|
||||
|
||||
@@ -91,3 +91,74 @@ func TestAReplacementImageIsPresentBeforeTheRunningContainerIsRemoved(t *testing
|
||||
t.Errorf("order of runtime calls = %s, want the image pulled before rm", got)
|
||||
}
|
||||
}
|
||||
|
||||
// asideRun is a runtime whose container "store" runs from an older spec, recording the steps a replacement takes; run
|
||||
// fails when failRun, and the new container reads back as declared otherwise.
|
||||
func asideRun(t *testing.T, want string, failRun bool, order *[]string) Runner {
|
||||
created := false
|
||||
return func(ctx context.Context, name string, args ...string) (string, error) {
|
||||
switch args[0] {
|
||||
case "info":
|
||||
return "27.0\n", nil
|
||||
case "container":
|
||||
if created {
|
||||
return "true\t" + want, nil
|
||||
}
|
||||
return "true\tsome-older-spec", nil
|
||||
case "image":
|
||||
return "", nil
|
||||
case "rm", "rename", "stop", "start":
|
||||
*order = append(*order, strings.Join(args, " "))
|
||||
return "", nil
|
||||
case "run":
|
||||
*order = append(*order, "run")
|
||||
if failRun {
|
||||
return "", errors.New("port is already allocated")
|
||||
}
|
||||
created = true
|
||||
return "deadbeef\n", nil
|
||||
}
|
||||
return "", nil
|
||||
}
|
||||
}
|
||||
|
||||
// A container whose replacement does not come up is put back: the running one is set aside (renamed and stopped), never
|
||||
// removed before the new one runs, and started again under its name when the new one fails, the apply failing loudly.
|
||||
// Removed first, the bus's own container left novox without a bus when its replacement was never made (issue 1945,
|
||||
// live on 2026-10-25 at 11:23).
|
||||
func TestAContainerWhoseReplacementFailsIsPutBack(t *testing.T) {
|
||||
d := parseTrusted(t, `{"declaration":1,"resources":[
|
||||
{"id":"store","type":"container","name":"store","image":"`+pinned+`","env":{"PGDATA":"/data"}}
|
||||
]}`)
|
||||
want := containerSpec(d.Resources[0].(*declaration.Container), inputs{})
|
||||
var order []string
|
||||
_, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, asideRun(t, want, true, &order), nil, nil)
|
||||
if err == nil || !strings.Contains(err.Error(), "started again") {
|
||||
t.Fatalf("a failed replacement: %v", err)
|
||||
}
|
||||
got := strings.Join(order, "; ")
|
||||
if !strings.Contains(got, "rename store store.replaced; stop store.replaced; run") ||
|
||||
!strings.Contains(got, "rename store.replaced store; start store") {
|
||||
t.Fatalf("the steps were %q", got)
|
||||
}
|
||||
if strings.Contains(got, "rm -f store;") && strings.Index(got, "rm -f store;") < strings.Index(got, "run") {
|
||||
t.Fatalf("the running container was removed before its replacement ran: %q", got)
|
||||
}
|
||||
}
|
||||
|
||||
// A replacement that runs removes the container set aside, and only after it runs.
|
||||
func TestAReplacementThatRunsRemovesTheContainerSetAside(t *testing.T) {
|
||||
d := parseTrusted(t, `{"declaration":1,"resources":[
|
||||
{"id":"store","type":"container","name":"store","image":"`+pinned+`","env":{"PGDATA":"/data"}}
|
||||
]}`)
|
||||
want := containerSpec(d.Resources[0].(*declaration.Container), inputs{})
|
||||
var order []string
|
||||
if _, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, asideRun(t, want, false, &order), nil, nil); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
got := strings.Join(order, "; ")
|
||||
run, removed := strings.Index(got, "run"), strings.LastIndex(got, "rm -f store.replaced")
|
||||
if run < 0 || removed < run || strings.Contains(got, "start store") {
|
||||
t.Fatalf("the steps were %q: the set-aside container is removed after the replacement runs", got)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user