Skip to content
This repository has been archived by the owner on Dec 4, 2024. It is now read-only.

use haproxy-systemd-wrapper, remove iptables #386

Closed
wants to merge 4 commits into from

Conversation

vixns
Copy link
Contributor

@vixns vixns commented Dec 18, 2016

I propose to switch from the current custom haproxy reload script to haproxy-systemd-wrapper.

My motivations for this change are

  • Use an officially supported haproxy tool.
  • Removing iptables allows to run MLB unprivileged.
  • Using custom iptables calls along with calico may have side effects.
  • The HAPROXY_RELOAD_SIGTERM_DELAY hack does not work ( haproxy never starts ), and even after fixing it, old process with outdated configuration can live for minutes.
  • Despite the great approach from Yelp, I prefer the risk of dropping a few connections than having unreliable reloads.

@mesosphere-ci
Copy link

Can one of the admins verify this patch?

@brndnmtthws
Copy link
Contributor

If HAPROXY_RELOAD_SIGTERM_DELAY doesn't work, we should either fix that or just remove it.

@vixns
Copy link
Contributor Author

vixns commented Dec 19, 2016

HAPROXY_RELOAD_SIGTERM_DELAY is not the point here.
I will open a separate issue for it if this PR, which removes it, is rejected.

The question is about switching from the whole bash script to haproxy-systemd-wrapper

@brndnmtthws
Copy link
Contributor

My only real concern with this PR is removing the iptables stuff. As much as I dislike the complexity, the "why do I occasionally see errors when reloading haproxy?" issue comes up more often than I'd like. AFAIK there are only 2 ways to work around this (excluding options such as putting another proxy in front), and this is the least complex of the 2 options.

@brndnmtthws
Copy link
Contributor

Could we just trap the SIGHUP and forward it to the wrapper instead? Then we can keep the iptables stuff.

@brndnmtthws
Copy link
Contributor

Actually I've realized the problem with doing that is that you wouldn't know when the reload has completed, without doing some kind of PID watching madness (which we already do inside marathon_lb.py).

@vixns
Copy link
Contributor Author

vixns commented Dec 21, 2016

haproxy use a file descriptor for this http://git.haproxy.org/?p=haproxy-1.7.git;a=commit;h=b957109727f7fed556c049d40bacf45f0df211db

Delaying TCP syn should not be required, SO_REUSEPORT allows several programs to bind the same port.
The underlying problem is that the linux implementation, it distribute requests along all connected programs, not just the last connected one like in bsd implementation.

The old process can still receive incoming connections while draining, that's why a SIGTTOU signal is sent to it, to stop accepting new ones.

In this case, we may have connections accepted by the kernel, but rejected by the old haproxy, I guess these are the 0.25% dropped connections.

Ways to workaround:

  • put another haproxy in front, tcp only, to queue + redispatch dropped connections.
  • use iptables to temporary drop the SYNs (MLB uses this)
  • use tc to queue SYNs internally ( cf yelp article )
  • use the new ( linux >= 4.5 ) SO_ATTACH_REUSEPORT_CBPF and a bpf policy to mimic BSD behavior by only using the last connected haproxy instance ( never seen this yet )

With the exception of putting another proxy in front, AFAIK all of these are requiring running privileged, which is not always possible and unsafe.

IMO, having tcp proxies which never reloads ( only use maps updated from the cli or DNS SRV, planned for haproxy 1.8 ) in front may be the best way to prevent all connection drops types and distribute traffic accross all "http" haproxy instances. This seems not possible to be part of MLB until it evolves to a mesos framework, but this is out of this PR's scope.

@brndnmtthws
Copy link
Contributor

Interesting. We do something similar to haproxy-systemd-wrapper, here: https://github.com/mesosphere/marathon-lb/blob/master/marathon_lb.py#L664-L666

However, it looks like maybe we should just emulate the bahaviour in haproxy-systemd-wrapper instead (by trying the pipe). To me, this is preferable because it's a smaller change.

@brndnmtthws
Copy link
Contributor

See #390 as an alternative to this.

@brndnmtthws
Copy link
Contributor

@vixns how do you feel about 1.5.0 (and #390)? Has it solved the root cause for you?

@vixns
Copy link
Contributor Author

vixns commented May 29, 2017

seems ok, thanks

@vixns vixns closed this May 29, 2017
@vixns vixns deleted the hap_wrapper branch May 29, 2017 06:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants