Le 31/01/2021 à 11:11, William Dauchy a écrit :
while reading `update_server_addr_port` I found out some things which
can be seen as incoherency. I hope I did not overlooked anything:
- one comment is stating check's address should be updated if it uses
the server one; however the condition checks if `SRV_F_CHECKADDR` is
set; this flag is set when a check address is set; result is that we
override the check address where I was not expecting it. In fact we
don't need to update anything here as server addr is used when check
addr is not set.
- same goes for check agent addr
- for port, it is a bit different, we update the check port if it is
unset. This is harmless because we also use server port if check port
is unset. However it creates some incoherency before/after using this
command, as check port should stay unset througout the life of the
process unless it is is set by `set server check-port` command.
quite hard to locate the origin of this this issue but the function was
introduced in commit d458adcc52b74608e2fe6a2a95f09ce5e94932b7 ("MINOR:
new update_server_addr_port() function to change both server's ADDR and
service PORT"). I was however not able to determine whether this is due
to a change of behavior along the years. So this patch can potentially
be backported up to v1.8.
Signed-off-by: William Dauchy
---
src/server.c | 15 ---
1 file changed, 15 deletions(-)
diff --git a/src/server.c b/src/server.c
index 10f528640..99b7e9181 100644
--- a/src/server.c
+++ b/src/server.c
@@ -3625,16 +3625,6 @@ const char *update_server_addr_port(struct server *s,
const char *addr, const ch
ipcpy(&sa, &s->addr);
changed = 1;
- /* we also need to update check's ADDR only if it uses the server's one */
- if ((s->check.state & CHK_ST_CONFIGURED) && (s->flags &
SRV_F_CHECKADDR)) {
- ipcpy(&sa, &s->check.addr);
- }
-
- /* we also need to update agent ADDR only if it use the
server's one */
- if ((s->agent.state & CHK_ST_CONFIGURED) && (s->flags &
SRV_F_AGENTADDR)) {
- ipcpy(&sa, &s->agent.addr);
- }
-
/* update report for caller */
chunk_printf(msg, "IP changed from '%s' to '%s'", current_addr,
addr);
}
@@ -3714,11 +3704,6 @@ const char *update_server_addr_port(struct server *s,
const char *addr, const ch
}
chunk_appendf(msg, "%d'", new_port);
-
- /* we also need to update health checks port only if it
uses server's realport */
- if ((s->check.state & CHK_ST_CONFIGURED) && !(s->flags
& SRV_F_CHECKPORT)) {
- s->check.port = new_port;
- }
}
else {
chunk_appendf(msg, "no need to change the port");
Hi William,
It seems you're right on this point. There is no reason to update health-check
or agent-check when the server address or port are updated. So this patch is
correct for me. But it also reveals a consistency problem. And I'm inclined to
wait a bit to review more deeply this part.
For instance, SRV_F_AGENTADDR flag is set when the "addr" option is set on a
server line but not for the "agent-addr" option. And for the both options, the
last one parsed is always used to set the agent-check addr. Thus these two lines
don't have the same behavior :
server ... addr agent-addr
server ... agent-addr addr
The same is true for "port" and "agent-port" options.
On the other hand "agent-inter" is properly handled. We inherit from the
health-check value during the post-init, only if it's not already set via a
"agent-inter" option.
In addition, there are SRV_F_CHECKADDR/SRV_F_CHECKPORT flags for health-checks
but only SRV_F_AGENTADDR for the agent-checks. For me, we can probably remove
all these flags and handle inheritance at the post-init stage. But if we must
keep some flags, only SRV_F_AGNTADDR/SRV_F_AGENTPORT are useful.
On the CLI, the inconsistency also exists. There is a way to change the agent
port and the health-check address but not the opposite. It is not really handy.
So, it may be good to take a global look at this stuff. I may missed something.
And be carefull for the backports because the health-checks were refactored in
the 2.2.
--
Christopher Faulet
