Re: [PATCH 0/6] cli commands coherency
Hi Christopher, On Mon, Feb 8, 2021 at 12:21 PM Christopher Faulet wrote: > 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. yes ok. > 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(). ah ok, I thought it was ok as long as there was no reset, but I indeed overlooked the second chunk. I will fix that. > 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). ah true I overlooked SRV_STATE_FILE_NB_FIELDS_VERSION_1 meaning. let me fix that as well. > 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 ? good idea. We never did it in the past, but that's probably a good thing to do. > 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 ? Thanks for the review. I will come back with a v2. -- William
Re: [PATCH 0/6] cli commands coherency
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
[PATCH 0/6] cli commands coherency
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(-) -- 2.30.0