Hi, first, thanks Marno for sharing your work.
On Mon, Jun 28, 2021 at 07:32:19AM +0000, Marno Krahmer wrote: > Hello, > > I would like to add a path to HAProxy. > This patch is supposed to change how stats are handled for disabled proxies. > > Prior to this patch, when outputting stats information, disabled proxies were > ignored / skipped. > This was an issue with old processes after a reload of HAProxy. > > It caused "the old process", that was still holding active sessions, to not > report stats any more. > This made it impossible for any monitoring solution to figure out, how many > currently active sessions exist. > > While this issue might barely be noticeable when using HAProxy for > HTTP-Traffic, for long-running TCP-Sessions, this can become an issue. > > This patch will now not only check, if a proxy is disabled, but also if it > still holds active sessions. And as long as it does, it will still output > statistics. > > Initially I opened the following Issue on GitHub: > https://github.com/haproxy/haproxy/issues/1307 For those not having followed the discussion there, this patch introduces a trade-off which seems very reasonable to me. The goal is to have as little behavior change as possible so as not to confuse those who are used to monitor their stats sockets during reloads. I tend to consider that the benefits of still seeing some proxies that still have active connections substantially outweight the surprise of the change here from the old situation where all old process stats instantly disappear. I'd like to go further so that we could always keep the old process stats available during reloads (i.e. even after the last connection completes on a frontend, it remains visible). But Marno's tests exhibited some unexpected behaviors making the master frontend suddenly appear, so we thought we could postpone that to after we've figured what causes this to happen. I'd personally be in favor of merging this trivial patch into 2.5-dev ASAP, and would not be opposed to backporting it to 2.4 after some time if there is some demand for this. I'd rather do it after we've figured why the alternate solution doesn't work though, as it might uncover a hidden bug elsewhere. If anyone thinks that listing stopping frontends/backends on a leaving process could have annoying side effects that we've overlooked, please detail them here so that a better solution could be designed. I consider that the current situation is mostly an accident resulting from the same state being used for a proxy being disabled in the config and one being disabled during reload. But some might rely on this :-/ And similary if you think you're having a better idea which shouldn't break deployed stuff, feel free to suggest. thanks, Willy > The patch: > > From 0648fc0c148fe463ea9f0c77f34beeb484688eac Mon Sep 17 00:00:00 2001 > From: Marno Krahmer <[email protected]> > Date: Thu, 24 Jun 2021 16:51:08 +0200 > Subject: [PATCH] MEDIUM: stats: include disabled proxies that hold active > sessions to stats > > After reloading HAProxy, the old process may still hold active sessions. > Currently there is no way to gather information, how many sessions such > a process still holds. This patch will not exclude disabled proxies from > stats output when they hold at least one active session. This will allow > sending `!@<PID> show stat` through a master socket to the disabled > process and have it returning its stats data. > --- > src/stats.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/src/stats.c b/src/stats.c > index 3458924b7..a5620577a 100644 > --- a/src/stats.c > +++ b/src/stats.c > @@ -3575,8 +3575,11 @@ static int stats_dump_proxies(struct stream_interface > *si, > } > > px = appctx->ctx.stats.obj1; > - /* skip the disabled proxies, global frontend > and non-networked ones */ > - if (!px->disabled && px->uuid > 0 && (px->cap > & (PR_CAP_FE | PR_CAP_BE))) { > + /* skip the global frontend proxies and > non-networked ones > + * also skip disabled proxies unless they are > still holding active sessions. > + * This change allows retrieving stats from > "old" proxies after a reload. > + */ > + if ((!px->disabled || px->served > 0) && > px->uuid > 0 && (px->cap & (PR_CAP_FE | PR_CAP_BE))) { > if > (stats_dump_proxy_to_buffer(si, htx, px, uri) == 0) > return 0; > } > -- > 2.17.0 > > > Thanks a lot > Marno >
