Enable redispatch even when drain-support is enabled

[gavinbunney]

This fixes an issue where drain-support would lock a cookie affinity to a backend pod, even after it was terminated. Enabling redispatch as per haproxy doc recommendations fixes this to redispatch the session once the target pod is finally shutdown.

Without this change, haproxy returns a 503 status until a new session is started by the user.

Reference from haproxy: https://cbonte.github.io/haproxy-dconv/1.7/configuration.html#4-option%20persist

[gavinbunney]

Is it possible for this to be bundled in a release (say v0.7.2)? We currently have the entire template copied to inject the change in there so would be good to not have to worry 💃

Thanks!

[jcmoraisjr]

Hi, it looks good to me, but since I don't use this functionallity myself I cannot say about any undesired side effect.

@brianloss can you have a look? Why option redispatch isn't used with drainsupport/option persist?

[brianloss]

I think this was a little specific to my use case. With the drain mode, only clients with a cookie will come back to the draining server. In my use case, that is because there is state that only exists on that server. If the server is not reachable, then having the client silently go to a different server is more confusing, so we wanted the request to simply fail.
I would say rather than have this change hard-coded, it should be an option in the configmap or via annotations.

[gavinbunney]

Interesting @brianloss - we have a similar situation where users have state on that server only. The drain process we use is to add a system notification for logged in users that the server is being shutdown in X minutes, so if they are still logged in after that time, they are booted out to the login screen (which in reality is another server)

For your scenario, what does the client do when receiving a failed request? Start a new session or?

Happy for this to be optional, but would it make sense for it to be the default? Something like drain-support-redispatch with true|false to switch between modes?

[brianloss]

@gavinbunney we have a stateful query API where it acts like a database cursor. So in our case, the client would just have to run the query again from scratch if the server went down. There are some other monitoring cases where having redispatch send a request to a different server than indicated by the cookie is not desirable.

It seems to me like making it optional with a default of enabled is a good solution. In thinking about it more, we could also work around by using config snippets to put "no options redispatch" in our backends. We're fine with doing that as well.

[gavinbunney]

I've added a new commit with the drain-support-redispatch flag to toggle between the two redispatching/not when drain-support is enabled

[brianloss]

These changes will not break anything for our use case.

[jcmoraisjr]

@gavinbunney thanks! Note that since this is a new functionality I cannot merge it to v0.7 branch. Otoh the next v0.8 snapshot is almost there, it has almost the exact same code of v0.7 - thanks to --v07-controller which defaults to true on snapshots - and you can use v0.7's config-frontend configmap option with option redispatch which should give the same behaviour.

@brianloss as you can see this will break backward compatibility because the default value is true, but I'm fine with this change if this is not a problem to you, and @gavinbunney and you can confirm that "move to another server" sounds to be the behaviour most people will expect as the default one.

[brianloss]

@jcmoraisjr it's fine by me. I can work around the default, and that I think the default of redispatch on will be better for most people.

The documentation for option redispatch indicates it cannot be specified in the frontend section (or in the global section), so the only way to override with snippets is to use the ingress.kubernetes.io/config-backend annotation. That is exactly the workaround that I have in place now for whenever we pull this in. It might be nice to have a config-defaults configmap option to add a snippet to the defaults section. Maybe I'll throw up a request for that.

[jcmoraisjr]

Sorry guys about this long delay. Merging now and issuing a new snapshot tag as soon as I finish, test, push and merge some new v0.8 stuff.

[gavinbunney]

Great thanks @jcmoraisjr !