Skip to content
This repository has been archived by the owner on Feb 15, 2019. It is now read-only.

Add sticky session support to containous/oxy's round robin #4

Merged
merged 4 commits into from
Jul 20, 2016

Conversation

owen
Copy link

@owen owen commented May 24, 2016

Bonjour @emilevauge,

This change is the same I've pushed through to vulcand/oxy(vulcand#41) - but that hasn't been accepted, so I'll throw it over here!

I've got another PR that depends on this one for Traefik to use it, that will be coming soon...

I've not implemented this PR for the rebalancing round robin since I'm not certain how that will function... any comments are appreciated :)

Owen Marshall and others added 3 commits March 31, 2016 22:29
Sticky sessions are set through an HTTP cookie.
If the cookie:
    * is not present, use the next server & set that as sticky
    * is present,
        * but is no longer valid, use the next server & set that as
          sticky
        * and valid, use that server without advancing .next.
layer 7, that is... layer 8 is something different (https://en.wikipedia.org/wiki/Layer_8)
@emilevauge
Copy link
Member

Hi @owen, thanks a lot!
Even if I don't promote the use of stickysessions, this is a great PR ;)
One question: what are the difficulties implementing this in rebalancing round robin ?

@owen
Copy link
Author

owen commented May 25, 2016

I don't like sticky sessions either. But we have ~100 legacy applications that I don't want to rewrite 😆

One question: what are the difficulties implementing this in rebalancing round robin ?

There shouldn't be a technical concern, I just haven't decided the best way to handle it -- I think what would be best is for rebalancing to occur for new connections, but for connections that have a cookie we keep the same backend.

I'll probably work on that today, actually!

@emilevauge
Copy link
Member

Great!

I think what would be best is for rebalancing to occur for new connections, but for connections that have a cookie we keep the same backend.

Seems good :)

@owen
Copy link
Author

owen commented May 25, 2016

OK - the logic remains unchanged for new connections; if, however, a cookie is set the connection will be stuck.

I'm going to ping the vulcand maintainers to see if they are interested at all as well

@emilevauge
Copy link
Member

Thanks a lot @owen, great job!
LGTM
If vulcand#41 has not been merged in a few days, I will merge this one as is.

// make shallow copy of request before chaning anything to avoid side effects
newReq := *req
newReq.URL = url
stuck := false
Copy link
Member

Choose a reason for hiding this comment

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

Hum, there is an issue here I think.
You don't call url, err := r.NextServer() if stuck is false.

Copy link
Author

Choose a reason for hiding this comment

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

Glad I saw your comment in my other PR, I didn't see this comment!

I've retraced the logic of that section. The flow is this:

  • Look to see if we have a sticky session
    ** if so, pull the server out of the cookie and store it.
    ** if not, fall through to the next session
  • Look to see if we use sticky sessions & do not have a server || if we do not use sticky sessions:
    ** call NextServer() to get the approp. URL
    ** if we have sticky sessions, stick that backend
  • Return the calculated backend URL

So we only call NextServer() if we have failed to retrieve a server URL from the cookie - that's in the if !stuck { stanza. We won't call it if stuck = true, which is only if we have retrieved the server from the cookie -- and, validated that it is still a valid server, which we handle in ss.GetBackend.

Copy link
Author

Choose a reason for hiding this comment

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

@emilevauge let me know if that clears it up! alternatively, if I'm missing something here - which I may be ;) - let me know and I can revise

@emilevauge emilevauge force-pushed the master branch 2 times, most recently from b3bd8a0 to b57d670 Compare June 22, 2016 12:56
@emilevauge emilevauge force-pushed the master branch 5 times, most recently from d12fc74 to ab7796d Compare July 18, 2016 15:48
@emilevauge
Copy link
Member

Hi @owen, sorry for the delay... We add to wait for traefik 1.0.0 stable before touching oxy ;)
Merging this one 👍

@emilevauge emilevauge merged commit a23b69f into containous:master Jul 20, 2016
@emilevauge
Copy link
Member

Hum, there is a minor issue in the test TestBadCookieVal, it passes alone, but fails while running the whole test suite. @owen any idea?

@owen
Copy link
Author

owen commented Jul 20, 2016

That's odd. I'll look at this today!

On Jul 20, 2016, at 08:03, Emile Vauge notifications@github.com wrote:

Hum, there is a minor issue in the test TestBadCookieVal, it passes alone, but fails while running the whole test suite. @owen any idea?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

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.

2 participants