-
Notifications
You must be signed in to change notification settings - Fork 300
Add an endpoint to reload HAProxy config #312
Conversation
* Add --api-listen parameter
* Catch errors early * Apparently the http:// is important...
* Simplify logic around skipping validation * In preparation for testing existing config...
* <endpoint>/reload?existing
Can one of the admins verify this patch? |
test this please |
Tests seem to be failing due to my changes in #308 finally taking effect. Jenkins isn't installing |
I'm generally onboard with this, but could we do it in such a way that we don't add a new endpoint to the python script? It would be great if we could do it thru HAProxy. How about we have a lua script endpoint which sends a SIGHUP to the python script? |
@brndnmtthws I did have that thought too but initially thought it would be too complicated. I had a go at implementing signal handling now and it seems better than I had expected :) So now there are two signals:
I've left the HTTP endpoints as just accepting any HTTP method. These endpoints are a little different to the others in that they are "mutating" in a way, so perhaps they should be limited to I made the API endpoints quite explicit -- As usual, tested locally but can't promise more than that 😜 The git log is a bit of a mess now.. let me know if I should do a rebase. |
test this please |
end | ||
|
||
core.register_service("signalmlbhup", "http", function(applet) | ||
local _, success, code = run("pkill -HUP -f '^python.*marathon_lb.py'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One concern here is what happens if they're running multiple MLBs within the same PID namespace.
end) | ||
|
||
core.register_service("signalmlbusr1", "http", function(applet) | ||
local _, success, code = run("pkill -USR1 -f '^python.*marathon_lb.py'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
What if we had MLB pass its own PID as an env var to HAProxy? |
Actually I suppose it may be safe to ignore, since we already assume MLB is running within a namespaced container elsewhere in the code. I suppose we could just make that a requirement, which simplifies things. |
applet:send(response) | ||
end | ||
|
||
core.register_service("signalmlbhup", "http", function(applet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should limit these endpoints to POST
requests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how best to do this with HAProxy.. can do something like this (I think? HAProxy manual is long and complicated):
acl signalmlbhup path /_mlb_signal/hup
http-request deny 405 if !METH_POST signalmlbhup
http-request use-service lua.signalmlbhup if signalmlbhup
acl signalmlbusr1 path /_mlb_signal/usr1
http-request deny 405 if !METH_POST signalmlbusr1
http-request use-service lua.signalmlbusr1 if signalmlbusr1
Like I said in this comment, HAProxy is not ideal for doing this, and the other endpoints don't limit their valid methods.
@brndnmtthws yeah I don't feel that |
Would you mind adding a note to the README under the 'operation best practices' section stating that MLB should be run inside a container with namespace isolation? |
@brndnmtthws I added a README note.. let me know what you think about limiting the APIs to |
test this please |
EDIT:
This PR adds the ability to trigger a reload manually by sending a
SIGHUP
orSIGUSR1
to the marathon-lb script. It also adds HAProxy Lua extensions so that those signals can be sent to marathon-lb using two HTTP endpoints.Old comment:
This PR adds the ability to trigger a reload manually via a very (very) basic HTTP API.
Making a
POST /reload
to marathon-lb will trigger a full reload, as though a relevant event was received from Marathon.You can also do
POST /reload?existing=true
to trigger a reload of the existing config, without fetching the app data from Marathon.I think this API would generally be useful from a debugging standpoint -- if an event from Marathon is missed or something it might be useful to be able to poke marathon-lb to get a reload without restarting the whole container/service.
Our actual use case for this uses the
?existing=true
endpoint. In some cases the HAProxy config text hasn't changed but we still want to reload HAProxy because something else has changed -- most notably an SSL certificate could have been renewed or a certificate file added to a directory that HAProxy loads. We're trying to build an automated system to issue certificates that would then ping marathon-lb to load the new certs.I've tried to add this with as little disruption as possible. So I'm starting with the precedent set by the event server and just using the facilities in the
wsgiref
module. Also, there aren't really tests for the existing event server and the infrastructure needed to test all this is.. well.. there would be quite a lot of it. So there aren't really tests for my addition. I've tested it locally and it works as I'd expect 😕.I've also only added this functionality when in SSE mode, as the event server mode is deprecated and I didn't want to have 2 different listening ports for marathon-lb.
There are a couple of areas where things are a bit rough, mostly due to the existing structure of marathon-lb (the event processor has a thread and is notified to do reloads). The main issues I have are:
processor.stop()
if something goes wrong, in theirfinally
clauses. I'm not sure how best to deal with this.Anyway, this is a first prototype so feedback is appreciated :)