QubitProducts / bamboo

HAProxy auto configuration and auto service discovery for Mesos Marathon
Apache License 2.0
794 stars 214 forks source link

Bamboo add tasks too early to haProxy #143

Open dplate opened 9 years ago

dplate commented 9 years ago

We're using Marathon/Bamboo/HAProxy. When we deploy a new version of an app or just scale it up, we're seeing 503 (service unavailable) responses for a short time from our requests to HAProxy.

It looks like the reason is: Marathon reports the new task as soon as possible to Bamboo (before the deployment has finished and the first successful health check). Bamboo directly changes the configuration of HAProxy. After the restart of HAProxy, all instances of a cluster are active. It doesn't check them via health check first. The reasons for this are mentioned here: http://comments.gmane.org/gmane.comp.web.haproxy/14789

To solve this, I would like to use the health check status, which is exposed by Marathon: https://mesosphere.github.io/marathon/docs/rest-api.html#get-/v2/tasks I think the unchecked/unhealthy tasks could be filtered out here: https://github.com/QubitProducts/bamboo/blob/master/services/marathon/marathon.go#L179

Any thoughts or concerns regarding this idea?

j1n6 commented 9 years ago

That's a good solution. I'm trying to minimise the haproxy health check logic as Marathon has provided this with multiple methods.

I think a good way would be still generating the tasks, but add a single Boolean attribute to indicate if this is healthy or not. Filtering can be done at template level by preferences.

On 6 Jul 2015, at 07:52, Dirk Plate notifications@github.com wrote:

We're using Marathon/Bamboo/HAProxy. When we deploy a new version of an app or just scale it up, we're seeing 503 (service unavailable) responses for a short time from our requests to HAProxy.

It looks like the reason is: Marathon reports the new task as soon as possible to Bamboo (before the deployment has finished and the first successful health check). Bamboo directly changes the configuration of HAProxy. After the restart of HAProxy, all instances of a cluster are active. It doesn't check them via health check first. The reasons for this are mentioned here: http://comments.gmane.org/gmane.comp.web.haproxy/14789

To solve this, I would like to use the health check status, which is exposed by Marathon: https://mesosphere.github.io/marathon/docs/rest-api.html#get-/v2/tasks I think the unchecked/unhealthy tasks could be filtered out here: https://github.com/QubitProducts/bamboo/blob/master/services/marathon/marathon.go#L179

Any thoughts or concerns regarding this idea?

— Reply to this email directly or view it on GitHub.

timoreimann commented 9 years ago

@activars, I'd be curious to know what kind of use cases you have in mind that warrant being able to filter on the template level. I personally cannot think of a scenario where you would want to have a non-healthy task configured in HAProxy, but it may be totally possible that I am missing something.

Thanks.

malterb commented 9 years ago

Marathon used to be somewhat inconsistent with health checks. It also shuts down old tasks before the new one is healthy. Just two things to consider imho

Timo Reimann notifications@github.com schrieb am Di., 7. Juli 2015 um 13:35:

@activars https://github.com/activars, I'd be curious to know what kind of use cases you have in mind that warrant being able to filter on the template level. I personally cannot think of a scenario where you would want to have a non-healthy task configured in HAProxy, but I might be missing something.

Thanks.

— Reply to this email directly or view it on GitHub https://github.com/QubitProducts/bamboo/issues/143#issuecomment-119328497 .

timoreimann commented 9 years ago

@elmalto According to the documentation, Marathon should only leave the system in a state where no healthy instances exist if the minimumHealthCapacity parameter is set to zero. AFAICS, any larger value should guarantee a minimum number of healthy instances being available at all times.

If that's not the case for whatever reason or Marathon behaves inconsistently otherwise, my preferred approach would be to file bug reports on the Marathon end in order to get those issues sorted out. That way, Bamboo can hopefully be kept "workaround-free" in the long run.

j1n6 commented 9 years ago

Before we make the decision, I need to test those edge cases behaviour.

drewrobb commented 9 years ago

I've taken a stab at implementing this here: https://github.com/drewrobb/bamboo/tree/dr/health.

For my usage I'm adding {{ if $task.Healthy }} filtering in the haproxy template. May turn this into a proper PR soon.

lclarkmichalek commented 9 years ago

So, I decided on a different approach. Our HAProxy health checks are pretty effective at working out quickly if a task is OK or not, so by keeping the state of the existing servers' health across reloads, we keep the good tasks health, new tasks come in as NOLB, and get to progress to health independently of the Marathon health state (which is generally much more permissive than the HAProxy state, due to the fact that unhealthy tasks find themselves culled). The script below uses the HAProxy admin socket to query and persist the state:

!/bin/bash
set -e
set -u
set -x

HAPROXY_SOCKET=${HAPROXY_SOCKET:-'/var/run/haproxy/admin.sock'}
COMMAND_LOC="/tmp/haproxy_reload.$(tr -dc 'a-zA-Z0-9' </dev/urandom | fold -w 10 | head -1)"

dump_table() {
    echo "show stat" | socat stdio $HAPROXY_SOCKET
}

# Take a stats file, and generate a sequence of haproxy admin commands to
# replicate the state of existing servers after restart
dump_server_state() {
    egrep -v '(FRONTEND|BACKEND)' | # Filter out any non server entries
        (while read line; do
             local backend="$(cut -d, -f1 <<<$line)"
             local server="$(cut -d, -f2 <<<$line)"
             local state="$(cut -d, -f18 <<<$line | tr '[:upper:]' '[:lower:]')"
             echo "set server ${backend}/${server} health ${state}"
         done) |
        tr '\n' ';' # Join with the HAProxy command separator
}

reload_haproxy() {
    iptables -I INPUT -p tcp --dport 80 --syn -j DROP
    sleep 0.2
    if [ ! -f /var/run/haproxy.pid ]; then
        haproxy -f /etc/haproxy/haproxy.cfg -p /var/run/haproxy.pid -D -sf;
    else
        haproxy -f /etc/haproxy/haproxy.cfg -p /var/run/haproxy.pid -D -sf $(cat /var/run/haproxy.pid);
    fi
    sleep 0.5
    iptables -D INPUT -p tcp --dport 80 --syn -j DROP
}

dump_table | dump_server_state >$COMMAND_LOC
reload_haproxy
socat stdio $HAPROXY_SOCKET <$COMMAND_LOC

I haven't tested this beyond running it manually, and pretty much every edge case has been ignored :+1: However, it should be somewhat robust, due to HAProxy not terminating a stream of commands on the existence of a missing server (which will happen when a task is removed). Another consideration is whether to wait until the health check state has been sorted out before removing the iptables SYN shield; this is just something I whipped together as a POC.

pdpi commented 9 years ago

I love the assertion that it should be (somewhat) robust despite ignoring all corner cases. :+1:

dkesler commented 8 years ago

We fixed this by adding an 'Alive' flag (as well as a couple other useful fields) and submitted a PR for it while back (https://github.com/QubitProducts/bamboo/pull/126).

SEJeff commented 8 years ago

@bluepeppers

COMMAND_LOC="/tmp/haproxy_reload.$(tr -dc 'a-zA-Z0-9' </dev/urandom | fold -w 10 | head -1)"

Can also be:

COMMAND_LOC=$(mktemp -u /tmp/haproxy_reload.XXXXXXXXXX)

Clever solution however :+1: