Le 06/02/2021 à 20:47, William Dauchy a écrit :
Hello,This is a followup from last week cleaning regarding check and agent check. This patch series brings some more coherency on the CLI side. I also put some minor cleaning. William Dauchy (6): CLEANUP: check: fix some typo in comments CLEANUP: tools: typo in `strl2irc` mention MEDIUM: cli: add check-addr command MEDIUM: cli: add agent-port command 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 | 9 +- .../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/check.c | 18 +- src/proxy.c | 41 ++-- src/server.c | 192 ++++++++++++++---- src/tools.c | 2 +- 9 files changed, 213 insertions(+), 74 deletions(-)
Thanks William ! I merged the first two patches. For the others, I've some questions/comments.
First, there is a test to be sure the agent-check is enabled before updating the agent address and/or port. Do you think it should also be done for the health-check? Because, for now, it is possible to set an address on a disabled (or even not configured) health-check. But it is not possible for an agent-check. Given that it is not possible to enable/disable an agent-check or an health-check from the CLI, I guess the warning for both makes sense.
Then, there are some problems when the server-state file is loaded. The trash chunk used by srv_update_state() is overwritten when update_server_agent_addr_port() is called. It is not obvious, but the get_trash_chunk() function returns cyclically one of two trash chunks. srv_update_state() uses a trash chunk. When called, update_server_check_addr_port() uses the other one. So, update_server_agent_addr_port() re-uses the first trash chunk, the same than srv_update_state(). A solution may be to allocate a dedicated chunk in srv_update_state(), using alloc_trash_chunk().
A more annoying problem happens when an old state file is loaded (prior these changes). Last parameters are not defined, params[18] (check-addr), params[19] (agent-addr) and params[20] (agent-port) are set to NULL. Thus, HAProxy crashes when the health-check address is compared to "-". In fact, this comes from the SRV_STATE_FILE_NB_FIELDS_VERSION_1 macro. It should be set to 24. You added 3 new fields but just incremented it by 1. Hum... No, there is also another problem. In srv_state_parse_line(). SRV_STATE_FILE_NB_FIELDS_VERSION_1 must be equal to SRV_STATE_FILE_MAX_FIELDS. Or better, it should really reflect the argument number of the version 1, so 21. And it must be compared to srv_arg instead of arg. It is a bug since the 1.8 (commit 316947196).
However, this means with these changes, a cold restart is required. In the 2.4, 5 new fields were added to the server-state file. Is it expected to perform a cold restart when upgrading to the 2.4 from a prior major version ? If so, I'm ok. But, it may be a good idea to change the state file version then to not silently ignore the state file. Otherwise, I guess it is possible to make these 5 new fields optional, isn't it ?
William, I'm sorry if I'm not really clear but the subject is a bit foggy for me :) Do you feel confident to handle all the changes ?
-- Christopher Faulet
