Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding force to node rm #25159

Merged
merged 1 commit into from Aug 2, 2016

Conversation

diogomonica
Copy link
Contributor

Added a --force flag to docker node rm that forces node removal independent of current state.

/cc @aaronlehmann

@aaronlehmann
Copy link
Contributor

Note to reviewers: this includes #25154 to allow vendoring of engine-api. Once #25154 is merged, it should be rebased.

Error response from daemon: rpc error: code = 9 desc = node swarm-node-03 is not down and can't be removed

If a node is compromised, starts misbehaving, or access to it is lost such that a
clean shutdown of the node is not possible, you can use the force option.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, you can't force removal of a manager, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Added text to say that managers have to be demoted before they can be removed.

@diogomonica
Copy link
Contributor Author

@sfsmithcha help on docs? 😞

@@ -30,6 +31,20 @@ Example output:
$ docker node rm swarm-node-02
Node swarm-node-02 removed from swarm

This command will only remove nodes that are in the down state. Attempting to remove
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typical convention is: "Removes nodes that are in the down state from the swarm."

@icecrime
Copy link
Contributor

icecrime commented Aug 1, 2016

Design LGTM

@diogomonica diogomonica force-pushed the adding-force-to-node-remove branch 4 times, most recently from bd80b92 to 66291fe Compare August 1, 2016 23:23
@diogomonica diogomonica changed the title [WIP] Adding force to node rm Adding force to node rm Aug 1, 2016
@diogomonica diogomonica force-pushed the adding-force-to-node-remove branch 2 times, most recently from 61ede8e to 440b788 Compare August 1, 2016 23:39
},
}
flags := cmd.Flags()
flags.BoolVar(&opts.force, "force", false, "Force remove an active node.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There shouldn't be a period at the end of this flag description.

Signed-off-by: Diogo Monica <diogo.monica@gmail.com>

```bash
$ docker node rm swarm-node-03
Error response from daemon: rpc error: code = 9 desc = node swarm-node-03 is not down and can't be removed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We noticed that the use of

```bash

here causes the text after the apostrophe in the error message to get highlighted.

ping @thaJeztah - should we drop the "bash" formatting? I'm sure there are other places where we have this problem, and just haven't noticed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's a known problem; we mark them as bash to make the highlighting more predictable, but basically many of our examples are combined "bash" / "shell" and "output". The output should just be plain text.

We tried putting it in a separate code-block but that's equally bad (they appear as two separate blocks in the layout).

Removing the bash hint is not solution; doing so will make the highlightjs "guess" what language it should pick; and in this case, it thinks it's Ruby, which looks even worse:

screen shot 2016-08-02 at 22 39 07

In this case; changing to formal English (cannot) perhaps? 😄

@aaronlehmann
Copy link
Contributor

LGTM pending an answer on the docs formatting question.

@vieux
Copy link
Contributor

vieux commented Aug 2, 2016

LGTM, this one is blocking #25096

@thaJeztah
Copy link
Member

LGTM, we should look at the highlighting in general, so no blocker

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants