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

Rework haproxy config for stickiness and balance strategy #179

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

vuntz
Copy link
Member

@vuntz vuntz commented Feb 8, 2017

  • We enable stickiness based on SSL sessions
  • We add the ability to do stickiness based on cookies
  • We allow to define the balance strategy on a per-resource basis
  • We stop overriding the default balance strategy (and therefore use roundrobin by default)

We have haproxy 1.5.x now, so we can use persistence for SSL sessions.
This matters as if we want to allow people to not use "source" as
balance algorithm, then we need to make sure that sessions keep going to
the same backend to avoid breakages.

This reverts commit 32323b0.
This is done in the LWRP and in the template; let's just do it in the
LWRP to simplify the template.
@vuntz vuntz added the wip label Feb 8, 2017
@@ -25,6 +25,8 @@
attribute :address, kind_of: String, default: "0.0.0.0"
attribute :port, kind_of: Integer, default: 0
attribute :mode, kind_of: String, default: "http", equal_to: ["http", "tcp", "health"]
attribute :balance, kind_of: String, default: "", equal_to: ["", "roundrobin", "static-rr", "leastconn", "first", "source"]

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [124/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

Copy link
Contributor

@AbelNavarro AbelNavarro left a comment

Choose a reason for hiding this comment

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

Speaking out of memory, I had trouble having complex configuration in a listen section in haproxy config file. A better approach would be a frontend + backend separate sections, but that would require major changes.

This is useful to achieve persistence for web apps which have a session,
which is important in order to allow using a different algorithm than
"source" for balancing without breaking sessions.
This allows achieving persistence for a normal session, but also for the
login form where there's usually a CSRF token (and which is not
associated to a real session in the web app).
This enables customization of the balance strategy for each service.
The default in the haproxy cookbook is roundrobin, and this should
actually work fine. In cases where this may be troublesome (like web
apps), we can now configure stickiness to avoid issues.

With roundrobin, we spread the load accross the various backends, which
results in much improved performance.
@vuntz
Copy link
Member Author

vuntz commented Feb 8, 2017

Speaking out of memory, I had trouble having complex configuration in a listen section in haproxy config file.

This works fine here. Just need to validate the SSL case, but everything else seems good.

@vuntz
Copy link
Member Author

vuntz commented Feb 8, 2017

So overall, this looks good, except for horizon with ssl: there, the browser is creating multiple connections, so the ssl session id affinity doesn't work as expected. Easy workaround is to use the source balance strategy in that case. But I'll play a bit more with it.

Copy link
Contributor

@nicolasbock nicolasbock left a comment

Choose a reason for hiding this comment

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

Maybe I should add that it looks good to me once the gating tests pass.

Copy link
Contributor

@dirkmueller dirkmueller left a comment

Choose a reason for hiding this comment

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

LGTM, can't really comment on the details without a lot of research myself

@sjamgade
Copy link
Contributor

One change has been cherry-picked in #193, do consider reviewing that one.

@matelakat
Copy link
Contributor

re-running a build on a separate PR to see if ssl is still broken.

@matelakat
Copy link
Contributor

Given that the tests passed in #208 - including the SSL tests, I am merging this PR.

@matelakat
Copy link
Contributor

Ah, I cannot merge it in the lack of passing CI.

@matelakat
Copy link
Contributor

@nicolasbock @dirkmueller could you please transfer your +1 to #208 so we can get this change merged please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

9 participants