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

Prune Unhealthy Agents #6571

Merged
merged 13 commits into from
Oct 4, 2019
Merged

Prune Unhealthy Agents #6571

merged 13 commits into from
Oct 4, 2019

Conversation

schristoff
Copy link
Contributor

Note: #580 in Serf needs to be merged, Serf to be tagged, Consul re-vendored and the go.mod updated.

This allows for users to remove unhealthy agents by adding the -prune flag in the CLI. This will completely remove the agent from the list of members.

If you are pruning a server from WAN you should specify your command like such consul force-leave -prune nodename.datacenter , as the formatting that we have added doesn't always work.

Fixes #2981

@schristoff schristoff requested review from a team October 2, 2019 18:13
agent/agent_endpoint.go Outdated Show resolved Hide resolved
agent/consul/server.go Outdated Show resolved Hide resolved
api/api.go Outdated Show resolved Hide resolved
api/agent.go Outdated Show resolved Hide resolved
api/agent.go Outdated Show resolved Hide resolved
api/api.go Outdated Show resolved Hide resolved
if err != nil {
c.UI.Error(fmt.Sprintf("Error force leaving: %s", err))
return 1
switch c.prune {
Copy link
Member

Choose a reason for hiding this comment

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

It seems more natural to use if c.prune { ... } else { ... } instead of a boolean switch statement here.

Also you can move the common error handling outside of the conditional block, like:

if c.prune {
   err = client.Agent().ForceLeavePrune(...)
} else {
  err = client.Agent().ForceLeave(...)
}
if err != nil {
   ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you say natural, do you mean easier to read? Or what do you mean by natural?

Not against your suggestion, but I'm wondering which is more idiomatic but also user friendly? I've been going back and forth on which is better, why, and when to use a switch versus an if/else. Feedback would be amazing ❤️
(Perhaps it's more like the rest of Consul code to be an if/else 🤷‍♀ ?)

vendor/modules.txt Outdated Show resolved Hide resolved
@rboyer
Copy link
Member

rboyer commented Oct 2, 2019

Since you are changing the HTTP API for /v1/agent/force-leave you should also update the API docs in the website as well.

schristoff and others added 3 commits October 2, 2019 20:48
Co-Authored-By: R.B. Boyer <rb@hashicorp.com>
Co-Authored-By: R.B. Boyer <rb@hashicorp.com>
@schristoff schristoff changed the title [WIP] Prune Unhealthy Agents Prune Unhealthy Agents Oct 4, 2019
@schristoff
Copy link
Contributor Author

Necessary Serf stuff has been added and re-vendored. 🤞 Updated those API docs, as well.

@schristoff schristoff requested a review from rboyer October 4, 2019 04:16
agent/consul/server.go Outdated Show resolved Hide resolved
Copy link
Member

@rboyer rboyer left a comment

Choose a reason for hiding this comment

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

A few little things and possibly two control flow bugs.

agent/agent_endpoint_test.go Outdated Show resolved Hide resolved
agent/agent_endpoint_test.go Outdated Show resolved Hide resolved
agent/agent_endpoint_test.go Outdated Show resolved Hide resolved
agent/agent_endpoint_test.go Show resolved Hide resolved
command/forceleave/forceleave_test.go Outdated Show resolved Hide resolved
command/forceleave/forceleave_test.go Outdated Show resolved Hide resolved
command/forceleave/forceleave_test.go Show resolved Hide resolved
schristoff and others added 6 commits October 4, 2019 14:32
Co-Authored-By: R.B. Boyer <rb@hashicorp.com>
Co-Authored-By: R.B. Boyer <rb@hashicorp.com>
Co-Authored-By: R.B. Boyer <rb@hashicorp.com>
Co-Authored-By: R.B. Boyer <rb@hashicorp.com>
Co-Authored-By: R.B. Boyer <rb@hashicorp.com>
Copy link
Member

@rboyer rboyer left a comment

Choose a reason for hiding this comment

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

LGTM

@schristoff schristoff merged commit 5e26971 into master Oct 4, 2019
schristoff added a commit that referenced this pull request Oct 4, 2019
* Add -prune flag to ForceLeave
@schristoff schristoff deleted the 3361-forceleave-prune branch June 8, 2020 13:35
blake added a commit that referenced this pull request Dec 18, 2021
Document prune parameter added in #6571 and wan parameter added in
#11722.
blake added a commit that referenced this pull request Dec 20, 2021
Document prune parameter added in #6571 and wan parameter added in
#11722.

Co-authored-by: Freddy <freddygv@users.noreply.github.com>
hc-github-team-consul-core pushed a commit that referenced this pull request Dec 20, 2021
Document prune parameter added in #6571 and wan parameter added in
#11722.

Co-authored-by: Freddy <freddygv@users.noreply.github.com>
hc-github-team-consul-core pushed a commit that referenced this pull request Dec 20, 2021
Document prune parameter added in #6571 and wan parameter added in
#11722.

Co-authored-by: Freddy <freddygv@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: CLI command to force reaping/removal of specific nodes
2 participants