sent too fast
On Thu, Nov 03, 2016 at 06:33:03PM +0100, Willy Tarreau wrote:
> On Thu, Nov 03, 2016 at 05:45:34PM +0100, Willy Tarreau wrote:
> > Hi Thomas,
> >
> > On Sun, Oct 30, 2016 at 09:14:53PM +0200, Thomas Raehalme wrote:
> > > Hi,
> > >
> > > In January[1] and June[2] there were discussions regarding the DRAIN state
> > > not being preserved when loading server state from file. Looking at the
> > > source it looks to me as the issue has not been resolved.
> > >
> > > Have you come to a conclusion if the current behaviour is how it will be
> > > or
> > > if a fix can be expected at some time?
> >
> > By looking at the code, I think there is a bug. I'm not yet certain because
> > there are various combinations but I think one of them is not correct. In
> > fact the goal is to ensure that :
> > - when the config doesn't change, the state file always has precedence ;
> > - when the config changes, it takes precedence, otherwise it's impossible
> > to take config changes into account, and the first purpose of the server
> > state file is to keep state across reloads caused by config updates!
> >
> > I need to continue to study this part to understand if the difference
> > between
> > the code and the comment is intentional or a result of another corner case
> > we
> > initially missed.
>
> OK now after writing down the various cases and reviewing the design notes
> we exchanged last year with Baptiste on this subject, it's clear that the
> comment is right and the code wrong (it does the exact opposite of the code).
>
> This patch fixes it for me. Could you please confirm as well ? If so I'll
> backport it into 1.6 so that 1.6.10 gets it (ie I'm interested in knowing
> more or less quickly :-)).
>
> Thanks!
> Willy
>From 69000df4055a1fc6061192fa205ab2b53e6798d3 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w...@1wt.eu>
Date: Thu, 3 Nov 2016 18:19:49 +0100
Subject: BUG/MEDIUM: srv-state: properly restore the DRAIN state
X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4
There were seveal reports about the DRAIN state not being properly
restored upon reload.
It happens that the condition in the code does exactly the opposite
of what the comment says, and the comment is right so the code is
wrong.
It's worth noting that the conditions are complex here due to the 2
available methods to set the drain state (CLI/agent, and config's
weight). To paraphrase the updated comment in the code, there are
two possible reasons for FDRAIN to have been present :
- previous config weight was zero
- "set server b/s drain" was sent to the CLI
In the first case, we simply want to drop this drain state if the new
weight is not zero anymore, meaning the administrator has intentionally
turned the weight back to a positive value to enable the server again
after an operation. In the second case, the drain state was forced on
the CLI regardless of the config's weight so we don't want a change to
the config weight to lose this status. What this means is :
- if previous weight was 0 and new one is >0, drop the DRAIN state.
- if the previous weight was >0, keep it.
This fix must be backported to 1.6.
---
src/server.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/src/server.c b/src/server.c
index 68fa800..340bba7 100644
--- a/src/server.c
+++ b/src/server.c
@@ -2161,15 +2161,23 @@ static void srv_update_state(struct server *srv, int
version, char **params)
/* apply drain mode if server is currently enabled */
if (!(srv->admin & SRV_ADMF_FMAINT) && (srv_admin_state
& SRV_ADMF_FDRAIN)) {
/* The SRV_ADMF_FDRAIN flag is inherited when
srv->iweight is 0
- * (srv->iweight is the weight set up in
configuration)
- * so we don't want to apply it when
srv_iweight is 0 and
- * srv->iweight is greater than 0. Purpose is
to give the
- * chance to the admin to re-enable this server
from configuration
- * file by setting a new weight > 0.
+ * (srv->iweight is the weight set up in
configuration).
+ * There are two possible reasons for FDRAIN to
have been present :
+ * - previous config weight was zero
+ * - "set server b/s drain" was sent to the
CLI
+ *
+ * In the first case, we simply want to drop
this drain state
+ * if the new weight is not zero anymore,
meaning the administrator
+ * has intentionally turned the weight back to
a positive value to
+ * enable the server again after an operation.
In the second case,
+ * the drain state was forced on the CLI
regardless of the config's
+ * weight so we don't want a change to the
config weight to lose this
+ * status. What this means is :
+ * - if previous weight was 0 and new one is
>0, drop the DRAIN state.
+ * - if the previous weight was >0, keep it.
*/
- if ((srv_iweight == 0) && (srv->iweight > 0)) {
+ if (srv_iweight > 0 || srv->iweight == 0)
srv_adm_set_drain(srv);
- }
}
srv->last_change = date.tv_sec - srv_last_time_change;
--
1.7.12.1
