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

Add reload/leave http endpoints #2516

Merged
merged 7 commits into from
Nov 30, 2016
Merged

Add reload/leave http endpoints #2516

merged 7 commits into from
Nov 30, 2016

Conversation

kyhavlov
Copy link
Contributor

@kyhavlov kyhavlov commented Nov 18, 2016

Add the reload/leave endpoints as part of #2502

@kyhavlov kyhavlov added this to the 0.7.2 milestone Nov 18, 2016
@kyhavlov kyhavlov changed the title Add reload http endpoint Add reload/leave http endpoints Nov 18, 2016
Copy link
Contributor

@slackpad slackpad left a comment

Choose a reason for hiding this comment

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

Looking good - noted some small stuff.

@@ -221,6 +235,19 @@ The return code is 200 on success.

### <a name="agent_force_leave"></a> /v1/agent/force-leave/\<node\>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be "v1/agent/leave" for this one (not "force").

@@ -221,6 +235,19 @@ The return code is 200 on success.

### <a name="agent_force_leave"></a> /v1/agent/force-leave/\<node\>

This endpoint is hit with a GET and is used to trigger a graceful leave and shutdown
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change this and the other leave docs to say PUT even though it takes any verb.

t.Fatalf("member status is %v, should be left", err)
})
}

func TestHTTPAgentForceLeave(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should have a reload test as well.

@@ -36,6 +36,11 @@ func (s *HTTPServer) AgentSelf(resp http.ResponseWriter, req *http.Request) (int
}, nil
}

func (s *HTTPServer) AgentReload(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
_, err := s.agent.command.handleReload(s.agent.config)
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar for these as the monitor - let's add the Raft check with a TODO so this needs the operator read privs in master. These are pretty serious otherwise.

@@ -36,6 +36,11 @@ func (s *HTTPServer) AgentSelf(resp http.ResponseWriter, req *http.Request) (int
}, nil
}

func (s *HTTPServer) AgentReload(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make these new ones require a PUT otherwise give a 405. Might as well start these off in a better spot :-)

@@ -472,6 +473,7 @@ func (c *Command) setupAgent(config *Config, logOutput io.Writer, logWriter *log
return err
}
c.agent = agent
agent.command = c
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels a little dirty to expose the whole command. How about type reloadFn func() err and pass that into Create()?

@@ -71,6 +71,9 @@ type Agent struct {
server *consul.Server
client *consul.Client

// The command used to launch this agent
command *Command
Copy link
Contributor

Choose a reason for hiding this comment

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

You can drop this now, right?

@@ -747,6 +749,9 @@ func (c *Command) Run(args []string) int {
c.logFilter = logFilter
c.logOutput = logOutput

// Setup the channel for triggering config reloads
c.configReloadCh = make(chan chan error, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need the 0 on here for unbuffered.

errCh := make(chan error, 0)
s.agent.reloadCh <- errCh

return nil, <-errCh
Copy link
Contributor

Choose a reason for hiding this comment

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

This should wait on the shutdown channel too, and return some kind of error.

@@ -196,6 +198,18 @@ It returns a JSON body like this:
}
```

### <a name="agent_reload"></a> /v1/agent/reload

Added in Consul 0.7.2, this endpoint is hit with a PUT and is used to instruct
Copy link
Contributor

@slackpad slackpad Nov 30, 2016

Choose a reason for hiding this comment

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

A community person went through and put all the HTTP verbs in backticks so we should keep that style here. Looks nicer.

agent as "left" instead of "failed". Nodes that leave will not attempt to re-join
the cluster on restarting with a snapshot.

For nodes in server mode, the node is removed from the Raft peer set in a graceful
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't true any more after Consul 0.7.0 so we should change this, and reference the leave_on_terminate config option. Did this get copied over from some place? We should fix the source as well.

@kyhavlov kyhavlov merged commit bd69c6d into master Nov 30, 2016
@kyhavlov kyhavlov deleted the f-reload-api branch November 30, 2016 18:29
@rhyas
Copy link

rhyas commented Dec 29, 2016

@kyhavlov @slackpad I realize this is merged, but this seemed the best place to comment about an issue before possibly opening a new bug/item.

The TestAgent_Reload in the tests for this feature break on a machine with 2 private IP's. It took me a while to figure out how to get the output from the MockUi, (this drove me NUTS btw) but once I did it led me to the understanding that while nexConfig is called to get a config, it's never actually used to create the instance of the server used in the test. It builds a DefaultConfig on a Run(), but in that default, the bind is 0.0.0.0. If that's the bind, the agent fails to start because of the multi-private-ip issue.

Not sure how/if this needs to be fixed. I've worked around it by simply adding a -bind to the args list for the Command. It seems odd I haven't hit this for any other tests, so I'm not sure what this test is doing that's different from the rest. I haven't dug into it deeply yet.

@slackpad
Copy link
Contributor

slackpad commented Feb 8, 2017

Hi @rhyas that sounds like it's worth tracking. Would you mind opening a new issue for that? Binding to the address in the config sounds like a reasonable fix for this.

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.

3 participants