Add support of acceptance of datacenter as a param for force-leave from WAN pool #6609

dedifferentiator · 2019-10-09T16:14:48Z

I hope I did everything the proper way, waiting for review.
Also need some help with writing tests for -datacenter arg, need to know how to create fake node in another datacenter in forceleave_test.go

Resolves #6582

Add support for datacenter as a param for force-leave from WAN pool

hashicorp-cla · 2019-10-09T16:14:51Z

All committers have signed the CLA.

…st.go

dedifferentiator · 2019-10-09T17:20:47Z

api/agent_test.go

 	t.Parallel()
 	c, s := makeClient(t)
 	defer s.Stop()

 	agent := c.Agent()

 	// Eject somebody
-	err := agent.ForceLeavePrune("foo")
+	q := &QueryOptions{Datacenter: "", Prune: true}


Also maybe it's worth to add test for forceLeaveWithOpts with datacenter specified

Yep, that would be useful.

freddygv

Nice work @dedifferentiator! Sorry about the delay in reviewing this. I made comments in line with some recommendations.

Aside from those comments, we should also add some tests for removing nodes in a different datacenter. These should be added in the command and agent packages, and they would be modeled after: TestForceLeaveCommand and TestAgent_ForceLeave_ACLDeny.

Here is a setup that should be verified in both:

Set up two agents, with each one in a different datacenter (passing a datacenter config into the hcl param of NewTestAgent)
Join them via a WAN join
Shut down one of them
Run force-leave without prune, and verify that the status transitions to serf.StatusLeft
Run force-leave with prune, and verify that the node is removed from the WAN member list.

If these tests pass we can be more confident that the datacenter flag is working as intended.

Lastly, since the HTTP handler and CLI command are being updated, the website docs should also be updated to reflect the datacenter param.

These are the docs files to update:
website/source/api/agent.html.md
website/source/docs/commands/force-leave.html.markdown.erb

freddygv · 2019-10-22T07:59:39Z

agent/agent_endpoint.go

-	_, prune := req.URL.Query()["prune"]
+	//Check the value of the prune and datacenter query
+	datacenter := req.URL.Query().Get("datacenter")
+	prune, err := strconv.ParseBool(req.URL.Query().Get("prune"))


Getting the value of prune should be left as it was before.

We intend for users to use the flag like this:
/agent/force-leave/:node?prune

If a user only passes prune and not something like prune=true, then ParseBool will return an error and prune would be set to false.

freddygv · 2019-10-22T08:14:51Z

api/agent.go

-func (a *Agent) ForceLeavePrune(node string) error {
+//ForceLeaveWithOpts is used to have an a failed agent removed
+//from the list of members with specified additional arguments
+func (a *Agent) ForceLeaveWithOpts(node string, q *QueryOptions) error {


After some internal discussion we decided that we shouldn't piggy-backoff of QueryOptions here. QueryOptions should mostly be reserved for params that apply to multiple functions, rather than just for one (prune only applies to ForceLeave).

Rather than passing in QueryOptions, the signature could instead be:

func (a *Agent) ForceLeaveWithOpts(node string, datacenter string, prune bool) error

Then the params could be set like this:

r.params.Set("prune", "") r.params.Set("dc", datacenter)

freddygv · 2019-10-22T08:20:38Z

api/api.go

@@ -155,6 +155,10 @@ type QueryOptions struct {
 	// services. This currently affects prepared query execution.
 	Connect bool

+	// Prune is used in force leave command to remove agent completely


This would be removed, as per the comment above

freddygv · 2019-10-22T08:20:45Z

api/api.go

@@ -676,6 +680,9 @@ func (r *request) setQueryOptions(q *QueryOptions) {
 	if q.Connect {
 		r.params.Set("connect", "true")
 	}
+	if q.Prune {


Also to be removed

freddygv · 2019-10-22T08:27:28Z

command/forceleave/forceleave.go

@@ -85,5 +89,6 @@ Usage: consul force-leave [options] name
  Consul will attempt to reconnect to those failed nodes for some period of
  time before eventually reaping them.

-  -prune    Remove agent completely from list of members
+  -prune       Remove agent completely from list of members
+  -datacenter  Specify datacenter name


Suggested change

-datacenter Specify datacenter name

-datacenter Name of the datacenter to remove the node from

freddygv · 2019-10-22T08:28:00Z

command/forceleave/forceleave.go

 }

 func (c *cmd) init() {
 	c.flags = flag.NewFlagSet("", flag.ContinueOnError)
 	c.flags.BoolVar(&c.prune, "prune", false,
 		"Remove agent completely from list of members")
+	c.flags.StringVar(&c.datacenter, "datacenter", "", "Specify datacenter name")


Suggested change

c.flags.StringVar(&c.datacenter, "datacenter", "", "Specify datacenter name")

c.flags.StringVar(&c.datacenter, "datacenter", "", "Name of the datacenter to remove the node from")

freddygv · 2019-10-22T08:28:55Z

command/forceleave/forceleave.go

@@ -52,8 +55,9 @@ func (c *cmd) Run(args []string) int {
 		return 1
 	}

-	if c.prune {
-		err = client.Agent().ForceLeavePrune(nodes[0])
+	if c.prune || c.datacenter != "" {


This should be adjusted after removing prune from QueryOptions

if c.prune || c.datacenter != "" { err = client.Agent().ForceLeaveWithOpts(nodes[0], c.datacenter, c.prune) } else { err = client.Agent().ForceLeave(nodes[0]) }

Which way should it be adjusted? ForceLeaveWithOpts can be with unspecified prune and specified datacenter and vice versa, in other case it's just ForceLeave

That block you just posted is what I had in mind.

freddygv · 2019-10-22T08:37:23Z

api/agent_test.go

 	t.Parallel()
 	c, s := makeClient(t)
 	defer s.Stop()

 	agent := c.Agent()

 	// Eject somebody
-	err := agent.ForceLeavePrune("foo")
+	q := &QueryOptions{Datacenter: "", Prune: true}


Yep, that would be useful.

dedifferentiator · 2019-11-05T14:55:54Z

@freddygv
Okay, so what do we have, I spend a couple of days trying to fix error in TestForceLeaveCommand_datacenter, but with no success, after running forceleave command with specified dc I got:

    log.go:172: TestForceLeaveCommand_datacenter - 2019/11/05 16:41:52.160920 [DEBUG] http: Request PUT /v1/agent/force-leave/Node-f1fb52c3-1d0f-9604-3c00-692921472212?dc=dc2 (42.959µs) from=127.0.0.1:41948
    log.go:172: TestForceLeaveCommand_datacenter - 2019/11/05 16:41:52.762668 [DEBUG] tlsutil: OutgoingRPCWrapper with version 1
    log.go:172: TestForceLeaveCommand_datacenter - 2019/11/05 16:41:58.330392 [DEBUG] memberlist: Failed ping: Node-f1fb52c3-1d0f-9604-3c00-692921472212.dc2 (timeout reached)
    retry.go:121: forceleave_test.go:93: got status "leaving" want "left"
        forceleave_test.go:93: got status "alive" want "left"

For now I decided not to push changes in master since they obviously are not correct, so here they are. Maybe I should push them here? Not sure about that.

But at the same time when forceleave -datacenter=dc doesn't work, TestForceLeaveCommand_prune_datacenter for forceleave -prune -datacenter=dcpasses with no error occured. Maybe I don't see something what doesn't allow forceleave work properly, so help is required

freddygv · 2020-01-13T20:00:15Z

Hi @dedifferentiator,

After looking through your two PRs, the changes you made look good, and we really appreciate that you took the time to contribute.

Unfortunately, we have decided to not implement this feature. There are a few reasons for this:

There was a separate PR merged to mitigate a significant portion of the need for this feature, which was to avoid listing decommissioned datacenters in catalog endpoints.
There is additional complexity regarding functionality in Consul Enterprise that wasn't accounted for. This is because cross-DC force-leave would interact with Network Areas which also function across DCs.
Also an API release was tagged in November, which means that ForceLeavePrune is now in the customer-facing API. Making changes there would require adding a third function or making a breaking change.
The ACL permissions required for cross-DC requests like this one need more thinking, since ACL rules are typically restricted to a single datacenter.

dedifferentiator · 2020-01-14T13:21:43Z

@freddygv Understand, thank you for allowing trying myself in open-source, you have been helping me a lot during this PR, despite it is unmerged, I liked working on it

Add support for datacenter as a param for force-leave from WAN pool

69e8825

Add support for datacenter as a param for force-leave from WAN pool

dedifferentiator added 2 commits October 9, 2019 19:49

fix delegate implementation error in leader_test.go and acl_test.go

515ac2d

probably fixed wrong function name for ForceLeaveWithOpts in agent_te…

e9cb2be

…st.go

dedifferentiator commented Oct 9, 2019

View reviewed changes

remove unnecessary addition of datacenter suffix in function

258fac7

freddygv suggested changes Oct 22, 2019

View reviewed changes

dedifferentiator requested a review from freddygv November 20, 2019 14:07

hanshasselberg assigned hanshasselberg and freddygv and unassigned hanshasselberg Jan 13, 2020

freddygv closed this Jan 13, 2020

freddygv mentioned this pull request Jan 14, 2020

Accept datacenter as a param for force-leave from WAN pool #6582

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support of acceptance of datacenter as a param for force-leave from WAN pool #6609

Add support of acceptance of datacenter as a param for force-leave from WAN pool #6609

dedifferentiator commented Oct 9, 2019

hashicorp-cla commented Oct 9, 2019 •

edited

dedifferentiator Oct 9, 2019

freddygv Oct 22, 2019

freddygv left a comment

freddygv Oct 22, 2019

freddygv Oct 22, 2019

freddygv Oct 22, 2019

freddygv Oct 22, 2019

freddygv Oct 22, 2019

freddygv Oct 22, 2019

freddygv Oct 22, 2019

dedifferentiator Oct 31, 2019

freddygv Oct 31, 2019

freddygv Oct 22, 2019

dedifferentiator commented Nov 5, 2019 •

edited

freddygv commented Jan 13, 2020

dedifferentiator commented Jan 14, 2020

	-datacenter Specify datacenter name
	-datacenter Name of the datacenter to remove the node from

	c.flags.StringVar(&c.datacenter, "datacenter", "", "Specify datacenter name")
	c.flags.StringVar(&c.datacenter, "datacenter", "", "Name of the datacenter to remove the node from")

Add support of acceptance of datacenter as a param for force-leave from WAN pool #6609

Add support of acceptance of datacenter as a param for force-leave from WAN pool #6609

Conversation

dedifferentiator commented Oct 9, 2019

hashicorp-cla commented Oct 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freddygv left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dedifferentiator commented Nov 5, 2019 • edited

freddygv commented Jan 13, 2020

dedifferentiator commented Jan 14, 2020

hashicorp-cla commented Oct 9, 2019 •

edited

dedifferentiator commented Nov 5, 2019 •

edited