Bug 221402 - [PATCH] net/haproxy: add support for hard stop mode
Summary: [PATCH] net/haproxy: add support for hard stop mode
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Dmitry Sivachenko
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2017-08-10 21:56 UTC by Frank Wall
Modified: 2017-08-16 08:16 UTC (History)
2 users (show)

See Also:
bugzilla: maintainer-feedback? (demon)


Attachments
patch for net/haproxy (947 bytes, patch)
2017-08-10 21:56 UTC, Frank Wall
no flags Details | Diff
patch for net/haproxy-devel (959 bytes, patch)
2017-08-10 21:56 UTC, Frank Wall
no flags Details | Diff
new patch for net/haproxy (1.03 KB, patch)
2017-08-14 21:19 UTC, Frank Wall
no flags Details | Diff
new patch for net/haproxy-devel (1.04 KB, patch)
2017-08-14 21:21 UTC, Frank Wall
no flags Details | Diff
The same patch with hardreload command added (1.40 KB, patch)
2017-08-15 11:10 UTC, Dmitry Sivachenko
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frank Wall 2017-08-10 21:56:23 UTC
Created attachment 185247 [details]
patch for net/haproxy

I think there's a bug in the RC script that makes it impossible to use HAProxy's "hard stop" feature where it *immediately* quits and closes all established connections.

The RC script always sends the USR1 signal to HAProxy, even if the force prefix is used (but in this case the TERM signal should be used). That's because the $rc_force variable is not available in the context. (See /etc/rc.d/bgfsck for another working example.)

I've attached patches for both net/haproxy and net/haproxy-devel.
Comment 1 Frank Wall 2017-08-10 21:56:53 UTC
Created attachment 185248 [details]
patch for net/haproxy-devel
Comment 2 Dmitry Sivachenko freebsd_committer 2017-08-11 08:34:23 UTC
Well, force prefix means to bypass haproxy_enabled check, not to send some other signal.
See rc(8).
Comment 3 Frank Wall 2017-08-11 21:20:04 UTC
Hi Dmitry, I get your point, the man page does not mention this use-case.
That being said, this doesn't mean it's wrong to use it to "force" a
certain behaviour.

For example, /etc/rc.d/bgfsck [1] is also using the "force" prefix
for something that is not described in the rc(8) man page.

Please note that the current HAProxy RC script already *tries* to do
exactly what my patch finally makes possible [2]. So I am not trying
something *new*, but instead fixing a bug in the RC script.

I think this is required, because HAProxy provides two methods to stop
it and expects the admin to decide which method should be used [3].
In it's current state the HAProxy RC script *always* uses the graceful
stop, which may lead to a *very* long downtime of HAProxy, because it
continues to process existing connections until they close. Really not
suitable for many production sites. On literally all production sites
I'd want to use HAProxy's "hard stop".

Do you see another way of getting this feature into the RC script?
Shall we add a "hardstop" command instead?

[1]
https://github.com/freebsd/freebsd/blob/master/etc/rc.d/bgfsck#L29-L31

[2]
https://github.com/freebsd/freebsd-ports/blob/master/net/haproxy/files/haproxy.in#L40-L42

[3]
https://www.haproxy.org/download/1.7/doc/management.txt
(see: 4. Stopping and restarting HAProxy)
Comment 4 Dmitry Sivachenko freebsd_committer 2017-08-12 11:07:30 UTC
For bgfsk that makes sence: in someone runs script with forcestart (usually means interactive), then set delay=0.

As for haproxy, (ab)using forcestop is a mistake.

Yes, I know about this feature of haproxy and I thing it should be implemented via another rc command, "hardstop" is a good proposal IMHO.

Do you want to submit a patch for this?
Comment 5 Frank Wall 2017-08-12 15:26:26 UTC
I'll provide a patch for an additional "hardstop" RC command.
Comment 6 Frank Wall 2017-08-14 21:19:48 UTC
Created attachment 185418 [details]
new patch for net/haproxy
Comment 7 Frank Wall 2017-08-14 21:21:41 UTC
Created attachment 185419 [details]
new patch for net/haproxy-devel
Comment 8 Frank Wall 2017-08-14 21:26:08 UTC
> Yes, I know about this feature of haproxy and I thing it should
> be implemented via another rc command, "hardstop" is a good proposal IMHO.

I've attached two new patches to add the new "hardstop" rc command.

Besides that it also replaces the nonfunctional check for $rc_force with a new default value for $sig_stop.
Comment 9 Dmitry Sivachenko freebsd_committer 2017-08-15 11:10:22 UTC
sig_stop="${haproxy_sig_stop:-USR1}"
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


What is haproxy_sig_stop? Do you propose another variable for rc.conf?

I am attaching a bit modified version of your patch which also adds "hardreload" command.  What do you think?
Comment 10 Dmitry Sivachenko freebsd_committer 2017-08-15 11:10:55 UTC
Created attachment 185436 [details]
The same patch with hardreload command added
Comment 11 Frank Wall 2017-08-15 20:34:28 UTC
> I am attaching a bit modified version of your patch which also 
> adds "hardreload" command.  What do you think?

Looks good to me.
Comment 12 commit-hook freebsd_committer 2017-08-16 08:07:15 UTC
A commit references this bug:

Author: demon
Date: Wed Aug 16 08:06:58 UTC 2017
New revision: 448026
URL: https://svnweb.freebsd.org/changeset/ports/448026

Log:
  Add hardstop/hardreload commands to rc script (to stop/reload haproxy without
  waiting for existing connections to close).

  PR:		221402
  Submitted by:	Frank Wall <fw@moov.de>

Changes:
  head/net/haproxy/files/haproxy.in
  head/net/haproxy-devel/files/haproxy.in
Comment 13 Dmitry Sivachenko freebsd_committer 2017-08-16 08:07:35 UTC
I committed the change.

Thanks for submission.
Comment 14 Franco Fichtner 2017-08-16 08:09:10 UTC
No port revision bump? That ends up with a situation where some people will have the newer script while others will not until the next version is released (or the revision bumped for another reason).

Just asking...
Comment 15 Franco Fichtner 2017-08-16 08:09:10 UTC
No port revision bump? That ends up with a situation where some people will have the newer script while others will not until the next version is released (or the revision bumped for another reason).

Just asking...
Comment 16 Dmitry Sivachenko freebsd_committer 2017-08-16 08:13:28 UTC
Well, yes, I am not a big fan of bumping portrevision for such minor changes: since nobody complained about missing hardstop/hardreload for a long time, I decided it is not a big deal for them to wait for another haproxy version.

I bumped it FWIW since you asked :)
Comment 17 Franco Fichtner 2017-08-16 08:16:53 UTC
Thank you. I was really just asking, because multiple committers have multiple opinions about these things.  It's nice to talk about to the reasoning to learn more. :)


Cheers,
Franco