Hi Christopher, On Mon, Feb 8, 2021 at 12:21 PM Christopher Faulet <cfau...@haproxy.com> 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
