Add new drained state

[xp0]

SUMMARY

New 'drained' state allows to set haproxy servers to 'DRAIN' mode

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

haproxy module (haproxy.py)

ANSIBLE VERSION

ansible 2.3.0.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = Default w/o overrides
  python version = 2.7.12+ (default, Sep 17 2016, 12:08:02) [GCC 6.2.0 20160914]

ADDITIONAL INFORMATION

The haproxy module was missing the ability to set servers in DRAIN mode, only UP (enabled) and MAINT (disabled) were supported.

[ansibot]

cc @ravibhure
click here for bot help

[ravibhure]

@xp0
This is good, but I will say use DRAIN from options, as we are maintaining two choices in action state (enabled, disabled).
In disabled action, use mode DRAIN to perform action (default will be MAINT).

options:
  drained:
    description:
      - When disabling a server, setting the mode to "drain" only removes the server 
        from load balancing but still allows it to be checked and to accept new persistent 
        connections. Changes are propagated to tracking servers if any.
    required: false
    default: false

[xp0]

@ravibhure

Currently there is a load balancing module for apache that actually has multiple states including 'drained' mode in addition to enabled/disabled (http://docs.ansible.com/ansible/apache2_mod_proxy_module.html).

Of course an extra option would also work however I'm thinking that adding the 'drained' mode to the state param will allow us to maintain some degree of consistency across modules.

[ravibhure]

Thanks @xp0
I agree in some, but its true maintaining minimum state enable/disable will provide consistency across modules to usages with the other options.
I would like to have this feature in haproxy module.
refer #21420

[ravibhure]

👍
shipit

[agx]

"drained" is imho not correct, it should be "drain" since the node is not drained yet. It's in the process of being drained. "drained" would be correct if the node is in drain mode and no sessions are alive on that node.

[ravibhure]

@xp0 / @agx
I am okay to have another state available, since this is community module and I will say, the context here to make module better is to have minimize and meaningful states,

When we say enabled will enable the backend server, since disabled might have multiple choices, can be MAINT or DRAIN, both are equally mean to disabled ?

Since this is nice to have feature and I love to admit in the current.

[xp0]

@ravibhure
From my understanding "DRAIN" mode is not equal to disabled, it is an individual state between enabled and disabled and it's used as a intermediate step to prepare a server before going to maintenance.

It's not really enabled since it doesn't allow new connections, but its not really disabled either since the node is still up and will still handle traffic for the existing persistent connections.

For the applications that use sessions this is important since changing the state of a running node straight to MAINT will cause the sessions to break (traffic is immediately re-routed to the other running nodes).

I understand that you want to minimize the number of states, I'm just afraid of causing more confusion by over-simplifying it. Either way I'm happy we can discuss about it.

@agx
I see your point regarding the naming, if there are no other comments on changing the name of the state from "drained" to "drain" I'll do a quick update.

[ravibhure]

Perfect, I am certainly agree on few, but this is fine for now.

Thanks @xp0

[ravibhure]

👍

[krislindgren]

So one thing is that only newer versions of haproxy ( >1.5 )support a state of "drain". So this won't work for those of us using rhel, that aren't compiling our own version of haproxy. Ideally you want to set the drain state of an lb for a period of time, then after that time switch a node to maintenance to disable the host. Basically give connection x amount of time to die off naturally then after that time kill it off. Which is why under my linked pr, drain is basically an intermediate state to the final desired state of maint. The approach that was taken allows older versions of haproxy to let active connections die off, then when a node has no active connections place it in maintenance mode, or place in maintenance mode after x amount of time.

I would like if we could get the haproxy version and be able to handle the request for draining as good as possible for the version we are running against. Only commenting because we both had commits that will clash on "drain".

[bcoca]

Also, add note to docs that states 'drained only works on haproxy versions x and above' aside from handling it in code

[xp0]

@krislindgren
Thanks for pointing this out, I guess your suggestion to allow draining mode based on version would be ideal, maybe we can get it done.

For now I plan to check how to get the haproxy version and verify if it supports the DRAIN state (>=1.5).

[ansibot]

The test ansible-test sanity --test pep8 failed with the following errors:

lib/ansible/modules/net_tools/haproxy.py:67:52: W291 trailing whitespace
lib/ansible/modules/net_tools/haproxy.py:68:83: W291 trailing whitespace
lib/ansible/modules/net_tools/haproxy.py:69:55: W291 trailing whitespace
lib/ansible/modules/net_tools/haproxy.py:279:1: W293 blank line contains whitespace
lib/ansible/modules/net_tools/haproxy.py:374:34: E231 missing whitespace after ','
lib/ansible/modules/net_tools/haproxy.py:374:57: W291 trailing whitespace

click here for bot help

[xp0]

I added the check for haproxy version, I suppose code could use some scrutiny

[resmo]

@ravibhure ready for merge?

[ravibhure]

@resmo good to go 👍