Re: [PATCH v2 0/5] fix check port/addr consistency
Le 03/02/2021 à 12:19, William Dauchy a écrit : On Wed, Feb 3, 2021 at 9:59 AM Christopher Faulet wrote: At first glance, I'm just a bit annoyed with the patch 5. In the documentation, it is stated that "addr" option will be used for agent-check too. And there is no info about interactions between "addr" and "agent-addr" options when both are configured. For me, for an agent-check, the "agent-addr" option must be used in priority, regardless the options order. If not defined, the "addr" option must be used, if defined. And at the end, we use the server address by default if none is specified. ok I missed that part. it is a bit sad honestly, it makes things less explicit. There is another problem with "port" and "agent-port" options. It is mentioned in the documentation that "agent-port" is required to perform agent-checks. And "port" option is not used for agent-check. It is not really consistent. I propose to deal with port/agent-port options in the same way than addr/agent-addr ones. And we keep the test to be sure to have a dedicated port for the agent-check to not use the server's one. This way, we keep backward compatibility and improve consistency. Thus, if you agree with these changes, I guess we should keep SRV_F_AGENTADDR flag and add SRV_F_AGENTPORT. ok I will come back with a v3. I honestly don't like to have port/addr used for agent, as stated previously because it is creating some non explicit things. But indeed as we need to keep backward compat, I will fix that. About the CLI. I agree, the "check-addr" parameter on the "set server" command must be added. And the "agent-port" parameter must bee added too. For me, it is a consistency matter. But I understand it is also mandatory for dynamic environments. thanks, let me come back with a v3, so we can move forward for the next improvements. For the record, the series was finally fixed and pushed. Thanks William ! -- Christopher Faulet
Re: [PATCH v2 0/5] fix check port/addr consistency
On Wed, Feb 3, 2021 at 9:59 AM Christopher Faulet wrote: > At first glance, I'm just a bit annoyed with the patch 5. In the > documentation, > it is stated that "addr" option will be used for agent-check too. And there is > no info about interactions between "addr" and "agent-addr" options when both > are > configured. For me, for an agent-check, the "agent-addr" option must be used > in > priority, regardless the options order. If not defined, the "addr" option must > be used, if defined. And at the end, we use the server address by default if > none is specified. ok I missed that part. it is a bit sad honestly, it makes things less explicit. > There is another problem with "port" and "agent-port" options. It is mentioned > in the documentation that "agent-port" is required to perform agent-checks. > And > "port" option is not used for agent-check. It is not really consistent. I > propose to deal with port/agent-port options in the same way than > addr/agent-addr ones. And we keep the test to be sure to have a dedicated port > for the agent-check to not use the server's one. This way, we keep backward > compatibility and improve consistency. > Thus, if you agree with these changes, I guess we should keep SRV_F_AGENTADDR > flag and add SRV_F_AGENTPORT. ok I will come back with a v3. I honestly don't like to have port/addr used for agent, as stated previously because it is creating some non explicit things. But indeed as we need to keep backward compat, I will fix that. > About the CLI. I agree, the "check-addr" parameter on the "set server" command > must be added. And the "agent-port" parameter must bee added too. For me, it > is > a consistency matter. But I understand it is also mandatory for dynamic > environments. thanks, let me come back with a v3, so we can move forward for the next improvements. -- William
Re: [PATCH v2 0/5] fix check port/addr consistency
Le 02/02/2021 à 22:56, William Dauchy a écrit : Hello Christopher, As discussed, I revisited my previous series regarding check addr and port consistency. I don't think I missed anything. I won't hide my aim here, I would like to add support to set `check.addr` on the cli like it is possible for the service port. More and more people have a setup with containers having their own IP, but with a health check on the host (e.g. on the consul side for example). So it becomes more and more needed. That's mostly why I wanted to clarify the situation first with the different things I've seen while revisting this part of the code. Thanks William, I will review the series soon. At first glance, I'm just a bit annoyed with the patch 5. In the documentation, it is stated that "addr" option will be used for agent-check too. And there is no info about interactions between "addr" and "agent-addr" options when both are configured. For me, for an agent-check, the "agent-addr" option must be used in priority, regardless the options order. If not defined, the "addr" option must be used, if defined. And at the end, we use the server address by default if none is specified. There is another problem with "port" and "agent-port" options. It is mentioned in the documentation that "agent-port" is required to perform agent-checks. And "port" option is not used for agent-check. It is not really consistent. I propose to deal with port/agent-port options in the same way than addr/agent-addr ones. And we keep the test to be sure to have a dedicated port for the agent-check to not use the server's one. This way, we keep backward compatibility and improve consistency. Thus, if you agree with these changes, I guess we should keep SRV_F_AGENTADDR flag and add SRV_F_AGENTPORT. About the CLI. I agree, the "check-addr" parameter on the "set server" command must be added. And the "agent-port" parameter must bee added too. For me, it is a consistency matter. But I understand it is also mandatory for dynamic environments. Once that said, I don't really know how all of this is interacting with the servers state file. I don't really how this part works, so I may missed something. -- Christopher Faulet
[PATCH v2 0/5] fix check port/addr consistency
Hello Christopher, As discussed, I revisited my previous series regarding check addr and port consistency. I don't think I missed anything. I won't hide my aim here, I would like to add support to set `check.addr` on the cli like it is possible for the service port. More and more people have a setup with containers having their own IP, but with a health check on the host (e.g. on the consul side for example). So it becomes more and more needed. That's mostly why I wanted to clarify the situation first with the different things I've seen while revisting this part of the code. 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: server: get rid of checkport flag MEDIUM: get rid of checkaddr and agentaddr flag BUG/MINOR: server: don't set agent addr for addr parsing doc/management.txt| 1 + 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 | 5 +- src/dns.c | 3 +- src/proxy.c | 4 +- src/server.c | 57 --- 9 files changed, 39 insertions(+), 51 deletions(-) -- 2.29.2
