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.

Sign up for GitHub

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

Jump to bottom

Make ForceLeave and RemoveFailedNode fail when node doesn't exist #504

Closed
wants to merge 7 commits into from
Closed

Make ForceLeave and RemoveFailedNode fail when node doesn't exist #504

wants to merge 7 commits into from

Conversation

hanshasselberg
Copy link
Member

@hanshasselberg hanshasselberg commented Feb 20, 2018 •
edited
Loading

This PR fixes #500 and hashicorp/consul#3757. I added a test and mentioned the return code in the docs since it changes the CLI return codes.

I notices a couple of things during implementing and testing the changes:

  1. when there are no members, serf doesn't do anything. But after this change I would expect that it says that there are no nodes and that force leaving is not possible.
  2. since consul uses RemoveFailedNode, consul's CLI is changing as well. Before it returned 0, now it returns 1 if you try to remove a node that doesn't exist.
    Before:
MacBook-Pro% consul force-leave abc; echo $?;
0

After:

MacBook-Pro% consul force-leave abc; echo $?;
Error force leaving: Unexpected response code: 500 (node not found: abc)
1
  1. I noticed that a node cannot force-leave itself

@hanshasselberg
Copy link
Member Author

hanshasselberg commented Feb 20, 2018 •
edited
Loading

In this PR I changed force-leave to complain about non-existing nodes. That doesn't fix hashicorp/consul#3757 because it is using RemoveFailedNode. This is the reason I closed this PR, but maybe it is useful nevertheless?!

@hanshasselberg
Copy link
Member Author

hanshasselberg commented Feb 20, 2018 •
edited
Loading

I added the check to RemoveFailedNode as well. Now it should fix hashicorp/consul#3757.

@hanshasselberg hanshasselberg changed the title Make forceleave fail when node is not found Make ForceLeave and RemoveFailedNode fail when node doesn't exist Feb 20, 2018
@hanshasselberg
Copy link
Member Author

The build failure should be unrelated. I ran it successfully on my machine.

kyhavlov
kyhavlov reviewed Feb 22, 2018
View reviewed changes
Copy link
Contributor

@kyhavlov kyhavlov left a comment

Choose a reason for hiding this comment

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

Hey @i0rek this looks good, just one small comment and then I think we can merge.

cmd/serf/command/force_leave_test.go
}

code := c.Run(args)
if code != 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

could you check the output of ui.ErrorWriter.String() and make sure it has the 'node not found' text?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@grounded042
Copy link

@kyhavlov just curious why this is being reviewed when I've had a PR (#501) in for this for a while now that was never reviewed?

@hanshasselberg
Copy link
Member Author

ping @kyhavlov
@grounded042 maybe it is partially my fault too. I only discovered your work after I finished my PR.

banks
banks approved these changes Sep 10, 2018
View reviewed changes
Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks @i0rek and @grounded042 for fixes here. Apologies it's been so long to get approved.

@angrycub angrycub mentioned this pull request May 5, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kyhavlov kyhavlov kyhavlov left review comments

@banks banks banks approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

force-leave is not validating input
4 participants
@hanshasselberg @grounded042 @banks @kyhavlov