From 21ffabbc145a0721cb79d757f2cbe0cfe9f441a3 Mon Sep 17 00:00:00 2001 From: Sergey Subbotin Date: Mon, 14 Sep 2026 12:22:06 +0200 Subject: [PATCH 1/2] cli/command/stack: wait for every task in waitOnTasks `docker stack rm --detach=false` is meant to return once all tasks of the stack have reached a terminal state, but waitOnTasks counted the polls in which *any* task was terminal instead of checking that *all* of them are: the counter was never reset between polls and the inner loop stopped at the first terminal task. With N tasks it returned after N polls in which at least one task had stopped, whatever the state of the others, and it polled the API in a tight loop without a pause in between. Check on every poll that no task is left in a non-terminal state, pause between polls, and stop when the context is done. Signed-off-by: Sergey Subbotin --- cli/command/stack/remove.go | 24 ++++++++++------- cli/command/stack/remove_test.go | 45 ++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/cli/command/stack/remove.go b/cli/command/stack/remove.go index 2f6d061b35b6..8ee307edbaa4 100644 --- a/cli/command/stack/remove.go +++ b/cli/command/stack/remove.go @@ -9,6 +9,7 @@ import ( "errors" "fmt" "slices" + "time" "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" @@ -171,24 +172,29 @@ func terminalState(state swarm.TaskState) bool { return numberedStates[state] > numberedStates[swarm.TaskStateRunning] } +// taskPollInterval is the pause between two polls of the task list while +// waiting for the tasks of a stack to stop. +const taskPollInterval = 200 * time.Millisecond + +// waitOnTasks blocks until every task of the stack has reached a terminal +// state or has been removed, or until ctx is done. func waitOnTasks(ctx context.Context, apiClient client.APIClient, namespace string) error { - terminalStatesReached := 0 for { res, err := getStackTasks(ctx, apiClient, namespace) if err != nil { return fmt.Errorf("failed to get tasks: %w", err) } - for _, task := range res.Items { - if terminalState(task.Status.State) { - terminalStatesReached++ - break - } + if !slices.ContainsFunc(res.Items, func(task swarm.Task) bool { + return !terminalState(task.Status.State) + }) { + return nil } - if terminalStatesReached == len(res.Items) { - break + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(taskPollInterval): } } - return nil } diff --git a/cli/command/stack/remove_test.go b/cli/command/stack/remove_test.go index 16c6675e4c0d..b546735638e2 100644 --- a/cli/command/stack/remove_test.go +++ b/cli/command/stack/remove_test.go @@ -1,12 +1,14 @@ package stack import ( + "context" "errors" "io" "strings" "testing" "github.com/docker/cli/internal/test" + "github.com/moby/moby/api/types/swarm" "github.com/moby/moby/client" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" @@ -141,3 +143,46 @@ func TestRemoveContinueAfterError(t *testing.T) { assert.Check(t, is.DeepEqual(allSecretIDs, apiClient.removedSecrets)) assert.Check(t, is.DeepEqual(allConfigIDs, apiClient.removedConfigs)) } + +// taskListStoppingAfter returns a TaskList stub reporting two tasks: one that +// has already stopped, and one that keeps running until the stub has been +// polled the given number of times. It counts the polls in *calls. +func taskListStoppingAfter(polls int, calls *int) func(client.TaskListOptions) (client.TaskListResult, error) { + return func(client.TaskListOptions) (client.TaskListResult, error) { + *calls++ + last := swarm.TaskStateRunning + if *calls >= polls { + last = swarm.TaskStateShutdown + } + return client.TaskListResult{Items: []swarm.Task{ + {Status: swarm.TaskStatus{State: swarm.TaskStateShutdown}}, + {Status: swarm.TaskStatus{State: last}}, + }}, nil + } +} + +func TestWaitOnTasksWaitsForAllTasks(t *testing.T) { + const pollsUntilStopped = 4 + var taskListCalls int + apiClient := &fakeClient{ + taskListFunc: taskListStoppingAfter(pollsUntilStopped, &taskListCalls), + } + + assert.NilError(t, waitOnTasks(context.Background(), apiClient, "foo")) + assert.Check(t, is.Equal(pollsUntilStopped, taskListCalls)) +} + +func TestWaitOnTasksReturnsWhenContextIsCancelled(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + apiClient := &fakeClient{ + taskListFunc: func(client.TaskListOptions) (client.TaskListResult, error) { + cancel() + return client.TaskListResult{Items: []swarm.Task{ + {Status: swarm.TaskStatus{State: swarm.TaskStateRunning}}, + }}, nil + }, + } + + assert.ErrorIs(t, waitOnTasks(ctx, apiClient, "foo"), context.Canceled) +} From d471f348b4776ae345f9bf04c3f00669e2b83888 Mon Sep 17 00:00:00 2001 From: Sergey Subbotin Date: Mon, 14 Sep 2026 12:23:40 +0200 Subject: [PATCH 2/2] cli/command/stack: wait for the tasks before removing the networks With --detach=false, `docker stack rm` waited for the tasks of the stack to stop only after everything, networks included, had been removed. The daemon accepts removing a network as soon as every task attached to it is marked for removal, that is, while the containers are still stopping, and a task that terminates after one of its networks is gone is never deallocated by the manager: the addresses it holds on its remaining networks, the ingress network in particular, leak until the manager is restarted (moby/moby#37338). That is one ingress address per task publishing a port, per `docker stack rm`; after a few dozen redeploys the pool runs dry and new services publishing ports stay in "New" with "could not find an available IP while allocating VIP". Wait for the tasks right after the services have been removed, before the secrets, configs and networks, so that `docker stack rm --detach=false` removes the networks only once their tasks have stopped and the manager has released their addresses. The default (detached) behaviour is unchanged. Signed-off-by: Sergey Subbotin --- cli/command/stack/remove.go | 23 +++++++++------ cli/command/stack/remove_test.go | 39 ++++++++++++++++++++++++++ docs/reference/commandline/stack_rm.md | 8 ++++++ 3 files changed, 62 insertions(+), 8 deletions(-) diff --git a/cli/command/stack/remove.go b/cli/command/stack/remove.go index 8ee307edbaa4..b18a3c5fc8b6 100644 --- a/cli/command/stack/remove.go +++ b/cli/command/stack/remove.go @@ -82,20 +82,27 @@ func runRemove(ctx context.Context, dockerCli command.Cli, opts removeOptions) e // TODO(thaJeztah): change this "hasError" boolean to return a (multi-)error for each of these functions instead. hasError := removeServices(ctx, dockerCli, services.Items) + + if !opts.detach && !hasError { + // Wait for the tasks of the services to stop before removing the + // networks. The daemon accepts removing a network as soon as every + // task attached to it is marked for removal, that is, while the + // containers are still stopping; a task that terminates after one + // of its networks is gone is never deallocated by the manager, and + // the addresses it holds on its remaining networks (the ingress + // network in particular) leak until the manager is restarted; see + // https://github.com/moby/moby/issues/37338. + if err := waitOnTasks(ctx, apiClient, namespace); err != nil { + errs = append(errs, fmt.Errorf("failed to wait on tasks of stack: %s: %w", namespace, err)) + } + } + hasError = removeSecrets(ctx, dockerCli, secrets.Items) || hasError hasError = removeConfigs(ctx, dockerCli, configs.Items) || hasError hasError = removeNetworks(ctx, dockerCli, networks.Items) || hasError if hasError { errs = append(errs, errors.New("failed to remove some resources from stack: "+namespace)) - continue - } - - if !opts.detach { - err = waitOnTasks(ctx, apiClient, namespace) - if err != nil { - errs = append(errs, fmt.Errorf("failed to wait on tasks of stack: %s: %w", namespace, err)) - } } } return errors.Join(errs...) diff --git a/cli/command/stack/remove_test.go b/cli/command/stack/remove_test.go index b546735638e2..fbac0b0e1c0d 100644 --- a/cli/command/stack/remove_test.go +++ b/cli/command/stack/remove_test.go @@ -186,3 +186,42 @@ func TestWaitOnTasksReturnsWhenContextIsCancelled(t *testing.T) { assert.ErrorIs(t, waitOnTasks(ctx, apiClient, "foo"), context.Canceled) } + +func TestRemoveStackWaitsForTasksBeforeRemovingNetworks(t *testing.T) { + const pollsUntilStopped = 4 + var taskListCalls, taskListCallsAtNetworkRemoval int + apiClient := &fakeClient{ + services: []string{objectName("foo", "service1")}, + networks: []string{objectName("foo", "network1")}, + taskListFunc: taskListStoppingAfter(pollsUntilStopped, &taskListCalls), + } + apiClient.networkRemoveFunc = func(networkID string) error { + taskListCallsAtNetworkRemoval = taskListCalls + apiClient.removedNetworks = append(apiClient.removedNetworks, networkID) + return nil + } + cmd := newRemoveCommand(test.NewFakeCli(apiClient)) + cmd.SetArgs([]string{"--detach=false", "foo"}) + cmd.SetOut(io.Discard) + + assert.NilError(t, cmd.Execute()) + assert.Check(t, is.DeepEqual(buildObjectIDs(apiClient.services), apiClient.removedServices)) + assert.Check(t, is.DeepEqual(buildObjectIDs(apiClient.networks), apiClient.removedNetworks)) + assert.Check(t, is.Equal(pollsUntilStopped, taskListCallsAtNetworkRemoval)) +} + +func TestRemoveStackDetachedDoesNotWaitOnTasks(t *testing.T) { + apiClient := &fakeClient{ + services: []string{objectName("foo", "service1")}, + networks: []string{objectName("foo", "network1")}, + taskListFunc: func(client.TaskListOptions) (client.TaskListResult, error) { + return client.TaskListResult{}, errors.New("tasks must not be listed when detached") + }, + } + cmd := newRemoveCommand(test.NewFakeCli(apiClient)) + cmd.SetArgs([]string{"foo"}) + cmd.SetOut(io.Discard) + + assert.NilError(t, cmd.Execute()) + assert.Check(t, is.DeepEqual(buildObjectIDs(apiClient.networks), apiClient.removedNetworks)) +} diff --git a/docs/reference/commandline/stack_rm.md b/docs/reference/commandline/stack_rm.md index aa82c72b2df0..2ba306ffb722 100644 --- a/docs/reference/commandline/stack_rm.md +++ b/docs/reference/commandline/stack_rm.md @@ -20,6 +20,14 @@ Remove one or more stacks Remove the stack from the swarm. +By default, the command returns as soon as the removal of the stack's services, +networks, secrets, and configs has been requested; the tasks of the services +are stopped in the background. With `--detach=false`, the command waits for the +tasks to stop before it removes the networks of the stack, and returns once the +stack is gone. Removing the networks after the tasks have stopped lets the +swarm manager release every address the tasks held, including their addresses +on the `ingress` network. + > [!NOTE] > This is a cluster management command, and must be executed on a swarm > manager node. To learn about managers and workers, refer to the