Le 08/02/2021 à 23:53, William Dauchy a écrit :
Hello Christopher,
Here is the v2 addressing the points raised yesterday.
The patch 4/6 clearly looks scary but I made sure to not change anything
crazy apart from adding support for a version 2. I will probably start
to dream about a server-state-file burning every night.
I hope this will be good enough, but otherwise, don't hesitate to put
more comments, I will come back with a v3.
William Dauchy (6):
MEDIUM: cli: add check-addr command
MEDIUM: cli: add agent-port command
BUG/MEDIUM: server: re-align state file fields number
MEDIUM: server: add server-states version 2
MEDIUM: server: support {check,agent}_addr, agent_port in server state
CLEANUP: server: add missing space in server-state error output
doc/management.txt | 15 +-
include/haproxy/server-t.h | 16 +-
.../checks/1be_40srv_odd_health_checks.vtc | 2 +-
.../checks/40be_2srv_odd_health_checks.vtc | 2 +-
reg-tests/checks/4be_1srv_health_checks.vtc | 6 +-
src/proxy.c | 41 +-
src/server.c | 929 ++++++++++--------
7 files changed, 573 insertions(+), 438 deletions(-)
William,
Thanks for this new series. There are problems in the 5th patch. When a
server-state file is loaded, if health-check or agent-check are disabled for
a server, a warning is always emitted, independently on the parameters.
Especially if the parameters are both unset (NULL or "-" for addresses and
NULL and "0" for port). The format of this warning is also not really
convenient. While the warning emitted on the CLI is good, when the
server-state file is loaded, we have such warning :
[WARNING] 040/150958 (3623566) : server-state application failed for server
'back-http/www' health checks are not enabled on this server.
agent checks are not enabled on this server.
I suppose it also explains your last patch ("CLEANUP: server: add missing
space in server-state error output").
In addition, the warning is a bit misleading because most of the server
state is loaded at this stage. Note this comment is also true for the
warnings about the SRV resolution. I guess we should instead emit a
different warning depending the server state is not loaded at all or
partially loaded with the list of skipped part. But at least if the warning
is well formatted, it should be good for this patch. Having 2 types of
warnings may be handled in another patch.
Finally, there is a more annoying bug, but pretty easy to fix. You must be
careful to not use chunk_appendf() with a variable format string (2nd arg).
Just like with the printf family functions, you must not call it with a
non-constant format string. Here, there is a problem if I manually update
the server-state file to set "%s%s%s%s%s%s" as health-check or agent-check
address.
Thus replace this line :
chunk_appendf(msg, warning);
by
chunk_appendf(msg, "%s", warning);
or better :
chunk_strcat(msg, warning);
To conclude, I may merge the first 4 patches if you want, but I guess you
will have to adapt the first ones to produce better warnings. So I will
wait your confirmation to proceed. However, I will merge the bug fix.
There is no reason to not take it.
--
Christopher Faulet
