Welcome! Log In Create A New Profile

Advanced

[PATCH][RFC] MEDIUM: global: add a 'grace' option to cap the soft-stop time

Posted by Cyril Bonté 
Several use cases may accept to abruptly close the connections when the
instance is stopping instead of waiting for timeouts to happen.
This option allows to specify a grace period which defines the maximum
time to spend to perform a soft-stop (occuring when SIGUSR1 is
received).

With this global option defined in the configuration, once all connections are
closed or the grace time is reached, the instance will quit.
---
doc/configuration.txt | 12 ++++++++++++
include/types/global.h | 2 ++
src/cfgparse.c | 17 +++++++++++++++++
src/haproxy.c | 8 ++++++++
src/proxy.c | 2 ++
5 files changed, 41 insertions(+)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index 228e8132..b1109906 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -535,6 +535,7 @@ The following keywords are supported in the "global" section :
- deviceatlas-properties-cookie
- external-check
- gid
+ - grace
- group
- log
- log-tag
@@ -703,6 +704,17 @@ gid <number>
will only be able to drop these groups if started with superuser privileges.
See also "group" and "uid".

+grace <time>
+ Defines the grace time allowed to perform a clean soft-stop.
+
+ Arguments :
+ <time> is the maximum time (by default in milliseconds) for which the
+ instance will remain alive when a soft-stop is received via the
+ SIGUSR1 signal.
+
+ This may be used to ensure that the instance will quit even if connections
+ remain opened (for example with long timeouts for a proxy in tcp mode).
+
group <group name>
Similar to "gid" but uses the GID of group name <group name> from /etc/group.
See also "gid" and "user".
diff --git a/include/types/global.h b/include/types/global.h
index e14a2add..1d273bf4 100644
--- a/include/types/global.h
+++ b/include/types/global.h
@@ -80,6 +80,7 @@ struct global {
int gid;
int external_check;
int nbproc;
+ unsigned int grace; /* grace period to process soft stop */
int maxconn, hardmaxconn;
int maxsslconn;
int ssl_session_max_cost; /* how many bytes an SSL session may cost */
@@ -170,6 +171,7 @@ extern const int zero;
extern const int one;
extern const struct linger nolinger;
extern int stopping; /* non zero means stopping in progress */
+extern int grace;
extern char hostname[MAX_HOSTNAME_LEN];
extern char localpeer[MAX_HOSTNAME_LEN];
extern struct list global_listener_queue; /* list of the temporarily limited listeners */
diff --git a/src/cfgparse.c b/src/cfgparse.c
index 2eb25edb..8b6855f7 100644
--- a/src/cfgparse.c
+++ b/src/cfgparse.c
@@ -1011,6 +1011,23 @@ int cfg_parse_global(const char *file, int linenum, char **args, int kwm)
}
}
/* end of user/group name handling*/
+ else if (strcmp(args[0], "grace") == 0) {
+ const char *res;
+
+ if (!*args[1]) {
+ Alert("parsing [%s:%d] : '%s' expects <time> as argument.\n",
+ file, linenum, args[0]);
+ err_code |= ERR_ALERT | ERR_FATAL;
+ goto out;
+ }
+ res = parse_time_err(args[1], &global.grace, TIME_UNIT_MS);
+ if (res) {
+ Alert("parsing [%s:%d]: unexpected character '%c' in argument to <%s>.\n",
+ file, linenum, *res, args[0]);
+ err_code |= ERR_ALERT | ERR_FATAL;
+ goto out;
+ }
+ }
else if (!strcmp(args[0], "nbproc")) {
if (alertif_too_many_args(1, file, linenum, args, &err_code))
goto out;
diff --git a/src/haproxy.c b/src/haproxy.c
index 559b4811..400fb7e7 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -118,6 +118,7 @@ int relative_pid = 1; /* process id starting at 1 */
/* global options */
struct global global = {
.nbproc = 1,
+ .grace = TICK_ETERNITY,
.req_count = 0,
.logsrvs = LIST_HEAD_INIT(global.logsrvs),
.maxzlibmem = 0,
@@ -158,6 +159,7 @@ struct global global = {

int stopping; /* non zero means stopping in progress */
int jobs = 0; /* number of active jobs (conns, listeners, active tasks, ...) */
+int grace = TICK_ETERNITY;

/* Here we store informations about the pids of the processes we may pause
* or kill. We will send them a signal every 10 ms until we can bind to all
@@ -1585,6 +1587,12 @@ static void run_poll_loop()

/* Check if we can expire some tasks */
next = wake_expired_tasks();
+ if (stopping && (grace != TICK_ETERNITY)) {
+ if (tick_is_expired(grace, now_ms))
+ break;
+ if (next == TICK_ETERNITY || tick_is_expired(next, grace))
+ next = grace;
+ }

/* stop when there's nothing left to do */
if (jobs == 0)
diff --git a/src/proxy.c b/src/proxy.c
index c76d55d5..024b36fa 100644
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -925,6 +925,8 @@ void soft_stop(void)
struct peers *prs;

stopping = 1;
+ if (global.grace != TICK_ETERNITY)
+ grace = tick_add(now_ms, global.grace);
p = proxy;
tv_update_date(0,1); /* else, the old time before select will be used */
while (p) {
--
2.11.0
> On Mar 15, 2017, at Mar 15, 4:44 PM, Cyril Bonté <[email protected]> wrote:
>
> Several use cases may accept to abruptly close the connections when the
> instance is stopping instead of waiting for timeouts to happen.
> This option allows to specify a grace period which defines the maximum
> time to spend to perform a soft-stop (occuring when SIGUSR1 is
> received).
>
> With this global option defined in the configuration, once all connections are
> closed or the grace time is reached, the instance will quit.


Most of the other settings for time-limits include the word “timeout”. Maybe “timeout grace”, “timeout shutdown”, “timeout exit” or something is more consistent with other configuration options?

-Bryan
Hi Bryan,

Le 16/03/2017 à 00:52, Bryan Talbot a écrit :
>
>> On Mar 15, 2017, at Mar 15, 4:44 PM, Cyril Bonté <[email protected]> wrote:
>>
>> Several use cases may accept to abruptly close the connections when the
>> instance is stopping instead of waiting for timeouts to happen.
>> This option allows to specify a grace period which defines the maximum
>> time to spend to perform a soft-stop (occuring when SIGUSR1 is
>> received).
>>
>> With this global option defined in the configuration, once all connections are
>> closed or the grace time is reached, the instance will quit.
>
>
> Most of the other settings for time-limits include the word “timeout”. Maybe “timeout grace”, “timeout shutdown”, “timeout exit” or something is more consistent with other configuration options?

Thanks for raising that point. The choice was intended and may be
subject to discussion.

timeout keywords are (most of them, except maybe "timeout mail") defined
in defaults/frontend/backend/listen sections, whereas this one is in the
global one. I wanted to clearly differentiate that timeout to prevent
some misconfigurations.

But I'm definitely not closed to add a "timeout" prefix. Also, I was not
very decided about the "grace" name but I didn't spend a lot of time to
write the patch (hence the [RFC] tag ;-)). You suggested "timeout
shutdown", this one may be a better choice, indeed.


--
Cyril Bonté
Hi guys,

On Thu, Mar 16, 2017 at 01:03:24AM +0100, Cyril Bonté wrote:
> Hi Bryan,
>
> Le 16/03/2017 à 00:52, Bryan Talbot a écrit :
> >
> > > On Mar 15, 2017, at Mar 15, 4:44 PM, Cyril Bonté <[email protected]> wrote:
> > >
> > > Several use cases may accept to abruptly close the connections when the
> > > instance is stopping instead of waiting for timeouts to happen.
> > > This option allows to specify a grace period which defines the maximum
> > > time to spend to perform a soft-stop (occuring when SIGUSR1 is
> > > received).
> > >
> > > With this global option defined in the configuration, once all connections are
> > > closed or the grace time is reached, the instance will quit.
> >
> >
> > Most of the other settings for time-limits include the word "timeout". Maybe "timeout grace", "timeout shutdown", "timeout exit" or something is more consistent with other configuration options?
>
> Thanks for raising that point. The choice was intended and may be subject to
> discussion.
>
> timeout keywords are (most of them, except maybe "timeout mail") defined in
> defaults/frontend/backend/listen sections, whereas this one is in the global
> one. I wanted to clearly differentiate that timeout to prevent some
> misconfigurations.
>
> But I'm definitely not closed to add a "timeout" prefix. Also, I was not
> very decided about the "grace" name but I didn't spend a lot of time to
> write the patch (hence the [RFC] tag ;-)). You suggested "timeout shutdown",
> this one may be a better choice, indeed.

In my opinion it's irrelevant to the section, but to the role. It's not
a timeout but a grace time. We do already have "grace" in frontends for
a similar purpose. In fact in my opinion a timeout is very different in
that it's a maximum time waiting for an event to appear. Here we're not
waiting for an event, we offer a grace time after the event (the reload
signal). So in my opinion your choice of "grace" is perfectly suited here.

Willy
Hi Cyril,

A few comments below. First, I'd prefer not to add anything in
run_poll_loop() since we have to pass there a lot of times and it's on
the critical path. Instead I think we could create a task upon receipt
of SIGUSR1 that will either kill all connections or simply exit after
expiration (and emit a log). This can still be discussed, if the resulting
code is more fragile or more complex, but that's a general principle I
mention here.

On Thu, Mar 16, 2017 at 12:44:40AM +0100, Cyril Bonté wrote:
> @@ -1585,6 +1587,12 @@ static void run_poll_loop()
>
> /* Check if we can expire some tasks */
> next = wake_expired_tasks();
> + if (stopping && (grace != TICK_ETERNITY)) {

You can use tick_isset(grace) instead of comparing with TICK_ETERNITY.

> + if (tick_is_expired(grace, now_ms))
> + break;
> + if (next == TICK_ETERNITY || tick_is_expired(next, grace))
> + next = grace;
> + }

I could be wrong but I fear that it can cause some longer poll() pauses
because here you're postponing "next" if it was supposed to trigger
earlier. You should have used tick_first(next, grace), this would give
something like this :

if (unlikely(stopping)) {
if (tick_is_expired(grace, now_ms))
break;
next = tick_first(next, grace);
}

> /* stop when there's nothing left to do */
> if (jobs == 0)
> diff --git a/src/proxy.c b/src/proxy.c
> index c76d55d5..024b36fa 100644
> --- a/src/proxy.c
> +++ b/src/proxy.c
> @@ -925,6 +925,8 @@ void soft_stop(void)
> struct peers *prs;
>
> stopping = 1;
> + if (global.grace != TICK_ETERNITY)
> + grace = tick_add(now_ms, global.grace);

You can use the following which does these two operations :

grace = tick_add_ifset(now_ms, global.grace);

Cheers,
Willy
Hi Willy,

Le 20/03/2017 à 07:15, Willy Tarreau a écrit :
> Hi Cyril,
>
> A few comments below. First, I'd prefer not to add anything in
> run_poll_loop() since we have to pass there a lot of times and it's on
> the critical path. Instead I think we could create a task upon receipt
> of SIGUSR1 that will either kill all connections or simply exit after
> expiration (and emit a log). This can still be discussed, if the resulting
> code is more fragile or more complex, but that's a general principle I
> mention here.

I agree, while I modified the code, I didn't like having to add some new
tests in run_pool_loop(). Using a new task does not seem to make the
code too complex. It may even be clearer.
I already have something usable but I want to spend some more imes to
test some cases.

> On Thu, Mar 16, 2017 at 12:44:40AM +0100, Cyril Bonté wrote:
>> @@ -1585,6 +1587,12 @@ static void run_poll_loop()
>>
>> /* Check if we can expire some tasks */
>> next = wake_expired_tasks();
>> + if (stopping && (grace != TICK_ETERNITY)) {
>
> You can use tick_isset(grace) instead of comparing with TICK_ETERNITY.
>
>> + if (tick_is_expired(grace, now_ms))
>> + break;
>> + if (next == TICK_ETERNITY || tick_is_expired(next, grace))
>> + next = grace;
>> + }
>
> I could be wrong but I fear that it can cause some longer poll() pauses
> because here you're postponing "next" if it was supposed to trigger
> earlier.

I'm not sure, because "next" is modified only if it's greater than
"grace". The goal was exactly the contrary to wake up earlier if the
grace time triggers before "next".
But using a task, this is now obsolete and less error-prone :-)

> You should have used tick_first(next, grace), this would give
> something like this :
>
> if (unlikely(stopping)) {
> if (tick_is_expired(grace, now_ms))
> break;
> next = tick_first(next, grace);
> }

Oh indeed, I didn't notice the tick_first function :) It also becomes
obsolete using a task ;-)

>> /* stop when there's nothing left to do */
>> if (jobs == 0)
>> diff --git a/src/proxy.c b/src/proxy.c
>> index c76d55d5..024b36fa 100644
>> --- a/src/proxy.c
>> +++ b/src/proxy.c
>> @@ -925,6 +925,8 @@ void soft_stop(void)
>> struct peers *prs;
>>
>> stopping = 1;
>> + if (global.grace != TICK_ETERNITY)
>> + grace = tick_add(now_ms, global.grace);
>
> You can use the following which does these two operations :
>
> grace = tick_add_ifset(now_ms, global.grace);

Right, and obsolete too :-)


--
Cyril Bonté
Hi Cyril,

On Mon, Mar 20, 2017 at 11:19:37PM +0100, Cyril Bonté wrote:
> > A few comments below. First, I'd prefer not to add anything in
> > run_poll_loop() since we have to pass there a lot of times and it's on
> > the critical path. Instead I think we could create a task upon receipt
> > of SIGUSR1 that will either kill all connections or simply exit after
> > expiration (and emit a log). This can still be discussed, if the resulting
> > code is more fragile or more complex, but that's a general principle I
> > mention here.
>
> I agree, while I modified the code, I didn't like having to add some new
> tests in run_pool_loop(). Using a new task does not seem to make the code
> too complex. It may even be clearer.
> I already have something usable but I want to spend some more imes to test
> some cases.

OK, do not hesitate to ask if you're unsure about certain parts.

> > > + if (tick_is_expired(grace, now_ms))
> > > + break;
> > > + if (next == TICK_ETERNITY || tick_is_expired(next, grace))
> > > + next = grace;
> > > + }
> >
> > I could be wrong but I fear that it can cause some longer poll() pauses
> > because here you're postponing "next" if it was supposed to trigger
> > earlier.
>
> I'm not sure, because "next" is modified only if it's greater than "grace".
> The goal was exactly the contrary to wake up earlier if the grace time
> triggers before "next".

Ah yes indeed you're right. That's a good example of something not
really easy to get at first glance that we have to be careful about.

> But using a task, this is now obsolete and less error-prone :-)

Definitely!

Cheers,
Willy
Hey,

Am 16.03.2017 um 01:27 schrieb Willy Tarreau:
>
>> Thanks for raising that point. The choice was intended and may be
subject to
>> discussion.
>>
>> timeout keywords are (most of them, except maybe "timeout mail")
defined in
>> defaults/frontend/backend/listen sections, whereas this one is in
the global
>> one. I wanted to clearly differentiate that timeout to prevent some
>> misconfigurations.
>>
>> But I'm definitely not closed to add a "timeout" prefix. Also, I was not
>> very decided about the "grace" name but I didn't spend a lot of time to
>> write the patch (hence the [RFC] tag ;-)). You suggested "timeout
shutdown",
>> this one may be a better choice, indeed.
>
> In my opinion it's irrelevant to the section, but to the role. It's not
> a timeout but a grace time. We do already have "grace" in frontends for
> a similar purpose. In fact in my opinion a timeout is very different in
> that it's a maximum time waiting for an event to appear. Here we're not
> waiting for an event, we offer a grace time after the event (the reload
> signal). So in my opinion your choice of "grace" is perfectly suited
here.

I don't disagree with the use of the word grace in general (as opposed
timeout), but isn't it misleading to have a global grace option (this one
here, closing active, long-running sessions) and grace option in defaults/
frontend/listen/backend [1], which does something different (keeping the
listening socket open).

This sounds highly confusing to me and could become a source of
misunderstandings ("try setting grace to 5000", "ok, done", "which one?").

nginx calls this (also brandnew) feature worker_shutdown_timeout [2].

Maybe something like shutdown-maxgrace or shutdown-grace? I'm not sure,
but I'd definitely avoid a name clash with the existing grace option,
even if it's configured in a different section.


I understand this is for both TCP and HTTP mode? Maybe we can mention
this in the doc (" will remain alive *in both TCP and HTTP mode* when
a soft-stop ").


Otherwise, thumbs up, this is very useful!



cheers,
lukas


[1] https://cbonte.github.io/haproxy-dconv/1.8/configuration.html#grace
[2] http://nginx.org/en/docs/ngx_core_module.html#worker_shutdown_timeout
Hi Lukas,

On Wed, Mar 22, 2017 at 08:40:53PM +0100, Lukas Tribus wrote:
> > It's not
> > a timeout but a grace time. We do already have "grace" in frontends for
> > a similar purpose. In fact in my opinion a timeout is very different in
> > that it's a maximum time waiting for an event to appear. Here we're not
> > waiting for an event, we offer a grace time after the event (the reload
> > signal). So in my opinion your choice of "grace" is perfectly suited here.
>
> I don't disagree with the use of the word grace in general (as opposed
> timeout), but isn't it misleading to have a global grace option (this one
> here, closing active, long-running sessions) and grace option in defaults/
> frontend/listen/backend [1], which does something different (keeping the
> listening socket open).

Well we already have this with maxconn (global vs frontend). I don't really
see it as a timeout because the purpose (for users) is to ensure the old
process will stay in place for some time before dying.

> This sounds highly confusing to me and could become a source of
> misunderstandings ("try setting grace to 5000", "ok, done", "which one?").
>
> nginx calls this (also brandnew) feature worker_shutdown_timeout [2].

Ah interesting :-)

> Maybe something like shutdown-maxgrace or shutdown-grace? I'm not sure,
> but I'd definitely avoid a name clash with the existing grace option,
> even if it's configured in a different section.

I'm not much worried for the clash but I think that something like
"shutdown-grace" or something like this is probably a good idea.

> I understand this is for both TCP and HTTP mode? Maybe we can mention
> this in the doc (" will remain alive *in both TCP and HTTP mode* when
> a soft-stop ").

Probably yes. That just makes me think that we could involve the word
"delay" or "after" in the option name to remove any possible ambiguity
with grace or timeout. For example I think that "hard-stop-after 60s"
makes it clear that it will terminate a soft-stop period. What do you
(and others) think ?

> Otherwise, thumbs up, this is very useful!

Yes I totally agree.

Thanks,
Willy
Hi,

Le 22/03/2017 à 21:13, Willy Tarreau a écrit :
> Hi Lukas,
>
> On Wed, Mar 22, 2017 at 08:40:53PM +0100, Lukas Tribus wrote:
>> I understand this is for both TCP and HTTP mode? Maybe we can mention
>> this in the doc (" will remain alive *in both TCP and HTTP mode* when
>> a soft-stop ").

Yes, this is for both TCP and HTTP mode. I'll complete the documentation.

> Probably yes. That just makes me think that we could involve the word
> "delay" or "after" in the option name to remove any possible ambiguity
> with grace or timeout. For example I think that "hard-stop-after 60s"
> makes it clear that it will terminate a soft-stop period. What do you
> (and others) think ?

I'm OK with "hard-stop-after", I'll try to send the new patch tonight ;-)


--
Cyril Bonté
Am 22.03.2017 um 22:28 schrieb Cyril Bonté:
>
> I'm OK with "hard-stop-after", I'll try to send the new patch tonight ;-)
>

Great, thanks!


Lukas
Sorry, only registered users may post in this forum.

Click here to login