diff --git a/cli/command/stack/remove.go b/cli/command/stack/remove.go index 2f6d061b35b6..b18a3c5fc8b6 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" @@ -81,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...) @@ -171,24 +179,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..fbac0b0e1c0d 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,85 @@ 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) +} + +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