Re: [PATCH v3 0/5] fix check port/addr consistency
Hello Christopher, Thanks for your continuous support on this review :) On Thu, Feb 4, 2021 at 10:09 AM Christopher Faulet wrote: > Thanks William, it seems good. I've just a question (sorry :). When the server > state file is loaded, why the check port is set only if there is a DNS > resolution ? I know it was done this way before the support of check_port > parameter in the server state. But I suppose that now we support it, it should > always be set, isn't it ? yes you are totally right. > And is there any usage to add "agent-addr" in the server state file? Because > it > can also be set on the CLI. And later, same question will appear for > "check-addr" and "agent-port". the general rule is indeed, anything which can be changed through the CLI should be loaded through server states. Truth is server states becomes a nightmare to manage; I don't remember if you were in the discussions a few months ago, but we concluded we should change that long term by https://github.com/haproxy/haproxy/issues/953 So while waiting for a proper solution, we are indeed supposed to keep server states up to date. > I don't want to bother you again. So, I propose you to merge the first patch > and > to add a new one to not set the check port when the server state file is > loaded. > Then I can merge the third patch and amend the second one to move the check > port > assignment before merging it. And finally I can merge the fourth and fifth > patches. Don't worry, I can send you a v4. -- William
Re: [PATCH v3 0/5] fix check port/addr consistency
Le 03/02/2021 à 22:30, William Dauchy a écrit : Hello Christopher, Here is my last update on port/addr consistency. I addressed all the point you mentioned. I hope I did not forgot anything. I will come back with `check-addr` and `agent-port` on the cli once those patches are accepted. William Dauchy (5): BUG/MINOR: cli: fix set server addr/port coherency with health checks MEDIUM: server: adding support for check_port in server state MEDIUM: check: remove checkport checkaddr flag BUG/MINOR: check: consitent way to set agentaddr MEDIUM: check: align agentaddr and agentport behaviour Thanks William, it seems good. I've just a question (sorry :). When the server state file is loaded, why the check port is set only if there is a DNS resolution ? I know it was done this way before the support of check_port parameter in the server state. But I suppose that now we support it, it should always be set, isn't it ? And is there any usage to add "agent-addr" in the server state file? Because it can also be set on the CLI. And later, same question will appear for "check-addr" and "agent-port". I don't want to bother you again. So, I propose you to merge the first patch and to add a new one to not set the check port when the server state file is loaded. Then I can merge the third patch and amend the second one to move the check port assignment before merging it. And finally I can merge the fourth and fifth patches. -- Christopher Faulet
[PATCH v3 0/5] fix check port/addr consistency
Hello Christopher, Here is my last update on port/addr consistency. I addressed all the point you mentioned. I hope I did not forgot anything. I will come back with `check-addr` and `agent-port` on the cli once those patches are accepted. William Dauchy (5): BUG/MINOR: cli: fix set server addr/port coherency with health checks MEDIUM: server: adding support for check_port in server state MEDIUM: check: remove checkport checkaddr flag BUG/MINOR: check: consitent way to set agentaddr MEDIUM: check: align agentaddr and agentport behaviour doc/configuration.txt | 10 +-- doc/management.txt| 1 + include/haproxy/check.h | 2 + include/haproxy/server-t.h| 10 +-- .../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 | 33 -- src/dns.c | 3 +- src/proxy.c | 4 +- src/server.c | 64 +-- 11 files changed, 78 insertions(+), 59 deletions(-) -- 2.30.0
