Add support for --detach flag in stack rm #4259

gmargaritis · 2023-05-05T12:42:26Z

Part of #373 along with #4258

Added --detach to stack rm. Setting --detach=false waits until all of the stack tasks have reached a terminal state.

neersighted · 2023-05-22T12:36:36Z

neersighted · 2023-05-22T13:35:24Z

cli/command/stack/swarm/remove.go

+var numberedStates = map[swarm.TaskState]int64{
+	swarm.TaskStateNew:       1,
+	swarm.TaskStateAllocated: 2,
+	swarm.TaskStatePending:   3,
+	swarm.TaskStateAssigned:  4,
+	swarm.TaskStateAccepted:  5,
+	swarm.TaskStatePreparing: 6,
+	swarm.TaskStateReady:     7,
+	swarm.TaskStateStarting:  8,
+	swarm.TaskStateRunning:   9,
+	swarm.TaskStateComplete:  10,
+	swarm.TaskStateShutdown:  11,
+	swarm.TaskStateFailed:    12,
+	swarm.TaskStateRejected:  13,
+}


We should rely on importing SwarmKit here instead of incorporating magic enums.

Also it's unclear if this logic even belongs on the client; this looks dangerously close to leaking concerns across layers.

Hey @neersighted, thanks a lot for getting back to me!

This map was inspired by the progress code for Service:

cli/cli/command/service/progress/progress.go

Lines 23 to 44 in 5be2139

var (

numberedStates = map[swarm.TaskState]int64{

swarm.TaskStateNew: 1,

swarm.TaskStateAllocated: 2,

swarm.TaskStatePending: 3,

swarm.TaskStateAssigned: 4,

swarm.TaskStateAccepted: 5,

swarm.TaskStatePreparing: 6,

swarm.TaskStateReady: 7,

swarm.TaskStateStarting: 8,

swarm.TaskStateRunning: 9,

// The following states are not actually shown in progress

// output, but are used internally for ordering.

swarm.TaskStateComplete: 10,

swarm.TaskStateShutdown: 11,

swarm.TaskStateFailed: 12,

swarm.TaskStateRejected: 13,

}

longestState int

)

cli/cli/command/service/progress/progress.go

Lines 66 to 68 in 5be2139

func terminalState(state swarm.TaskState) bool {

return numberedStates[state] > numberedStates[swarm.TaskStateRunning]

}

An alternative would be to create a set with all the terminal states and check against that instead.

By design, the server instantly acknowledges any changes (e.g. removing a stack) and then the action is asynchronous. Thus, I believe that this logic belongs to the client, as the client is the one that waits for the actions to complete.

The reason Swarm states are numbered like they are is so that intermediate states can be inserted at any time without disrupting the ordering of states committed to the object store already. If the code handling states isn't persistent, then there's no such concern. If we ever added an intermediate state, we can just renumber all of these harmlessly.

dperny · 2023-09-01T16:27:38Z

Looks fine to me.

gmargaritis · 2023-09-21T11:28:21Z

Hey @thaJeztah and @neersighted

Is there anything else I can do in order to move this forward?

gmargaritis · 2024-01-29T11:09:03Z

👋 @thaJeztah @neersighted

I have rebased this PR along with #4258, and it's ready for review!

Anything I can do to move this forward in order to close #373?

cpuguy83 · 2024-01-29T19:37:00Z

cli/command/stack/remove.go

@@ -27,5 +27,8 @@ func newRemoveCommand(dockerCli command.Cli) *cobra.Command {
 			return completeNames(dockerCli)(cmd, args, toComplete)
 		},
 	}
+
+	flags := cmd.Flags()
+	flags.BoolVarP(&opts.Detach, "detach", "d", true, "Exit immediately instead of waiting for the stack tasks to converge")


Suggested change

flags.BoolVarP(&opts.Detach, "detach", "d", true, "Exit immediately instead of waiting for the stack tasks to converge")

flags.BoolVarP(&opts.Detach, "detach", "d", true, "Do not wait for stack removal")

Updated it in b040ca8!

cpuguy83 · 2024-01-31T01:53:24Z

It looks like this is changing the default behavior of stack rm to wait for removal?

cpuguy83 · 2024-01-31T01:55:11Z

Oh nevermind I see the default for the flag is true.
This makes usage a little weird because you have to --detach=false.

🤔

gmargaritis · 2024-01-31T10:09:56Z

@cpuguy83 Thank you for your feedback!

I see where you're coming from regarding the --detach flag behavior. We set --detach to true by default, in order to keep the user experience of the command the same. This approach was actually based on the implementation of the --detach flag in service create and service update (moby/moby#31144). In this case, the default was --detach=true and in a later release it was decided to change the default behavior of the commands to --detach=false.

If the plan is to switch the default behavior of stack rm to non-detachable in a future release, we can add a message indicating so, just like we did in moby/moby#31144 (comment) and in #4258 (comment).

I hope this clears things up and takes us closer to also merging #4258 and closing #373 😄

/cc @thaJeztah

codecov-commenter · 2024-02-08T13:35:36Z

Codecov Report

Merging #4259 (238d659) into master (4a43b8e) will decrease coverage by 0.06%.
The diff coverage is 8.33%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4259      +/-   ##
==========================================
- Coverage   61.18%   61.13%   -0.06%     
==========================================
  Files         287      287              
  Lines       20112    20136      +24     
==========================================
+ Hits        12306    12310       +4     
- Misses       6912     6933      +21     
+ Partials      894      893       -1

thaJeztah · 2024-02-08T15:06:19Z

docs/reference/commandline/stack_rm.md

+
+| Name             | Type | Default | Description                   |
+|:-----------------|:-----|:--------|:------------------------------|
+| `-d`, `--detach` |      |         | Do not wait for stack removal |


Interesting; for some reason it doesn't show the default here 🤔

--help shows the correct output, but the markdown generator doesn't 🤔 Any ideas @crazy-max ?

./build/docker stack rm --help Usage: docker stack rm [OPTIONS] STACK [STACK...] Remove one or more stacks Aliases: docker stack rm, docker stack remove, docker stack down Options: -d, --detach Do not wait for stack removal (default true)

I think it makes sense to not show default as this is a bool flag anyway? (like --pull or --push)

Oh my bad this is true by default so opt-out flag right?

@crazy-max Yes, it's true by default and the user can use --detach=false if they want to wait for stack removal to complete

It's the same for service create/update where we already have implemented this flag

cli/docs/reference/commandline/service_create.md

Lines 6 to 16 in 79fa65e

### Options

| Name | Type | Default | Description |

|:----------------------------------------------------|:------------------|:-------------|:----------------------------------------------------------------------------------------------------|

| `--cap-add` | `list` | | Add Linux capabilities |

| `--cap-drop` | `list` | | Drop Linux capabilities |

| [`--config`](#config) | `config` | | Specify configurations to expose to the service |

| [`--constraint`](#constraint) | `list` | | Placement constraints |

| `--container-label` | `list` | | Container labels |

| `--credential-spec` | `credential-spec` | | Credential spec for managed service account (Windows only) |

| `-d`, `--detach` | | | Exit immediately instead of waiting for the service to converge |

Crazy thought, how about "--wait"?

So the intent here is to (like docker service create) flip the default at some point, so make stack deploy act the same as docker compose up.

I don't think the markdown here is a real blocker for the PR (we can fix in "post"), but I noticed it, and was curious what went wrong here (looks like a bug in the markdown generator potentially?)

I agree, we can investigate further in a separate issue, as it seems like a generalized bug in the markdown generator.

Is there anything else left to be done in this PR?

gmargaritis · 2024-02-14T11:20:39Z

@thaJeztah @crazy-max 👋

Is there anything left to be done in this one?

neersighted

Overall LGTM/agreed that we can worry about the markdown later, given this is no more broken then other commands.

gmargaritis · 2024-02-20T16:29:22Z

@neersighted We have fixed the markdown issue in docker/cli-docs-tool#48 ✌️

Added --detach/-d to stack rm. Setting --detach=false waits until all of the stack tasks have reached a terminal state. Co-authored-by: Sebastiaan van Stijn <[email protected]> Signed-off-by: George Margaritis <[email protected]>

this was addressed

thaJeztah · 2024-03-04T09:04:35Z

Did a quick rebase and re-generated the markdown as we updated the cli-docs-tool with a fix for the boolean flags;

diff --git a/docs/reference/commandline/stack_rm.md b/docs/reference/commandline/stack_rm.md
index 8dd872b7c..5cfbfaab1 100644
--- a/docs/reference/commandline/stack_rm.md
+++ b/docs/reference/commandline/stack_rm.md
@@ -9,9 +9,9 @@ Remove one or more stacks

 ### Options

-| Name             | Type | Default | Description                   |
-|:-----------------|:-----|:--------|:------------------------------|
-| `-d`, `--detach` |      |         | Do not wait for stack removal |
+| Name             | Type   | Default | Description                   |
+|:-----------------|:-------|:--------|:------------------------------|
+| `-d`, `--detach` | `bool` | `true`  | Do not wait for stack removal |

I also opened a tracking ticket to look at follow-up improvements discussed above; #4907

See docker/cli#4259

gmargaritis requested a review from silvin-lubecki as a code owner May 5, 2023 12:42

gmargaritis mentioned this pull request May 5, 2023

Add support for --detach flag in stack deploy #4258

Merged

neersighted reviewed May 22, 2023

View reviewed changes

gmargaritis requested a review from neersighted May 29, 2023 09:13

pschichtel mentioned this pull request Jun 12, 2023

Timeout in case of one-shot container sudo-bmitch/docker-stack-wait#19

Open

gmargaritis force-pushed the 373-support-detach-flag-in-stack-rm branch from 074f182 to 08204d8 Compare September 14, 2023 12:29

gmargaritis force-pushed the 373-support-detach-flag-in-stack-rm branch 2 times, most recently from bda291b to 153b2f3 Compare January 24, 2024 13:53

cpuguy83 approved these changes Jan 29, 2024

View reviewed changes

cpuguy83 previously requested changes Jan 29, 2024

View reviewed changes

gmargaritis force-pushed the 373-support-detach-flag-in-stack-rm branch from 153b2f3 to b040ca8 Compare January 29, 2024 20:43

gmargaritis requested a review from thaJeztah as a code owner January 29, 2024 20:43

gmargaritis requested a review from cpuguy83 January 29, 2024 20:44

thaJeztah added status/2-code-review area/swarm area/stack labels Feb 8, 2024

thaJeztah reviewed Feb 8, 2024

View reviewed changes

gmargaritis force-pushed the 373-support-detach-flag-in-stack-rm branch from b040ca8 to 23e079d Compare February 9, 2024 10:47

This was referenced Feb 10, 2024

display flag type and default value in a dedicated columns docker/cli-docs-tool#19

Merged

Improve documentation for boolean flags with true as default value docker/cli-docs-tool#47

Closed

neersighted approved these changes Feb 20, 2024

View reviewed changes

neersighted added this to the 26.0.0 milestone Feb 20, 2024

thaJeztah mentioned this pull request Mar 4, 2024

Follow-up improvements for docker stack --detach options #4907

Open

Add support for --detach/-d flag in stack rm

238d659

Added --detach/-d to stack rm. Setting --detach=false waits until all of the stack tasks have reached a terminal state. Co-authored-by: Sebastiaan van Stijn <[email protected]> Signed-off-by: George Margaritis <[email protected]>

thaJeztah force-pushed the 373-support-detach-flag-in-stack-rm branch from 23e079d to 238d659 Compare March 4, 2024 08:57

thaJeztah merged commit 310daf2 into docker:master Mar 4, 2024
88 checks passed

thaJeztah added the impact/changelog label Mar 4, 2024

thaJeztah mentioned this pull request Mar 4, 2024

Support --detach flag in docker stack deploy / rm #373

Closed

matt9ucci added a commit to matt9ucci/DockerCompletion that referenced this pull request Apr 8, 2024

CLI 26.0.0: new stack rm --detach

c5a46cd

See docker/cli#4259

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for --detach flag in stack rm #4259

Add support for --detach flag in stack rm #4259

gmargaritis commented May 5, 2023 •

edited

Loading

neersighted commented May 22, 2023

neersighted May 22, 2023

gmargaritis May 26, 2023 •

edited

Loading

dperny Sep 1, 2023

dperny commented Sep 1, 2023

gmargaritis commented Sep 21, 2023

gmargaritis commented Jan 29, 2024

cpuguy83 Jan 29, 2024

gmargaritis Jan 29, 2024

cpuguy83 commented Jan 31, 2024

cpuguy83 commented Jan 31, 2024

gmargaritis commented Jan 31, 2024 •

edited

Loading

codecov-commenter commented Feb 8, 2024 •

edited

Loading

thaJeztah Feb 8, 2024

crazy-max Feb 8, 2024

crazy-max Feb 8, 2024

gmargaritis Feb 8, 2024

gmargaritis Feb 8, 2024 •

edited

Loading

cpuguy83 Feb 8, 2024

thaJeztah Feb 8, 2024

gmargaritis Feb 9, 2024

gmargaritis commented Feb 14, 2024

neersighted left a comment

gmargaritis commented Feb 20, 2024

thaJeztah commented Mar 4, 2024

	var (
	numberedStates = map[swarm.TaskState]int64{
	swarm.TaskStateNew: 1,
	swarm.TaskStateAllocated: 2,
	swarm.TaskStatePending: 3,
	swarm.TaskStateAssigned: 4,
	swarm.TaskStateAccepted: 5,
	swarm.TaskStatePreparing: 6,
	swarm.TaskStateReady: 7,
	swarm.TaskStateStarting: 8,
	swarm.TaskStateRunning: 9,

	// The following states are not actually shown in progress
	// output, but are used internally for ordering.
	swarm.TaskStateComplete: 10,
	swarm.TaskStateShutdown: 11,
	swarm.TaskStateFailed: 12,
	swarm.TaskStateRejected: 13,
	}

	longestState int
	)

	func terminalState(state swarm.TaskState) bool {
	return numberedStates[state] > numberedStates[swarm.TaskStateRunning]
	}

	flags.BoolVarP(&opts.Detach, "detach", "d", true, "Exit immediately instead of waiting for the stack tasks to converge")
	flags.BoolVarP(&opts.Detach, "detach", "d", true, "Do not wait for stack removal")

	### Options

	\| Name \| Type \| Default \| Description \|
	\|:----------------------------------------------------\|:------------------\|:-------------\|:----------------------------------------------------------------------------------------------------\|
	\| `--cap-add` \| `list` \| \| Add Linux capabilities \|
	\| `--cap-drop` \| `list` \| \| Drop Linux capabilities \|
	\| [`--config`](#config) \| `config` \| \| Specify configurations to expose to the service \|
	\| [`--constraint`](#constraint) \| `list` \| \| Placement constraints \|
	\| `--container-label` \| `list` \| \| Container labels \|
	\| `--credential-spec` \| `credential-spec` \| \| Credential spec for managed service account (Windows only) \|
	\| `-d`, `--detach` \| \| \| Exit immediately instead of waiting for the service to converge \|

Add support for --detach flag in stack rm #4259

Add support for --detach flag in stack rm #4259

Conversation

gmargaritis commented May 5, 2023 • edited Loading

neersighted commented May 22, 2023

Choose a reason for hiding this comment

gmargaritis May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dperny commented Sep 1, 2023

gmargaritis commented Sep 21, 2023

gmargaritis commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpuguy83 commented Jan 31, 2024

cpuguy83 commented Jan 31, 2024

gmargaritis commented Jan 31, 2024 • edited Loading

codecov-commenter commented Feb 8, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmargaritis Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmargaritis commented Feb 14, 2024

neersighted left a comment

Choose a reason for hiding this comment

gmargaritis commented Feb 20, 2024

thaJeztah commented Mar 4, 2024

gmargaritis commented May 5, 2023 •

edited

Loading

gmargaritis May 26, 2023 •

edited

Loading

gmargaritis commented Jan 31, 2024 •

edited

Loading

codecov-commenter commented Feb 8, 2024 •

edited

Loading

gmargaritis Feb 8, 2024 •

edited

Loading