On 2/17/26 6:58 PM, Alexandra Rukomoinikova wrote:
> Add schema and CLI support for health checks on logical switch
> ports by introducing a new health check table and linking it
> from logical switch ports. Implement corresponding ovn-nbctl
> commands to manage LSP health checks. Also extend service
> monitoring to support the logical-switch-port type.
>
> Signed-off-by: Alexandra Rukomoinikova <[email protected]>
> ---
> v2 --> v3: fixed Dumitru's comments
> ---
Hi Alexandra,
Thanks for the new revision!
I had a few comments below but because they're relatively minor I went
ahead and took care of them and applied the patch to main. That's true
for the rest of the patches in the series (more details in the
individual replies).
However, one thing this series misses is a NEWS entry announcing the new
Health Check support for switch ports. As I couldn't decide on which
patch of your series should include one and as I thought that it might
be better for the author of the series to write it, I didn't add one either.
I also didn't want that your feature misses the 26.03 branch date (this
Friday).
Could you please post a follow up that adds a NEWS entry?
For the comments on this specific patch, please see below.
> ovn-nb.ovsschema | 27 +-
> ovn-nb.xml | 53 ++++
> ovn-sb.ovsschema | 7 +-
> ovn-sb.xml | 11 +
> tests/ovn-nbctl.at | 196 +++++++++++++++
> utilities/ovn-nbctl.8.xml | 52 ++++
> utilities/ovn-nbctl.c | 509 ++++++++++++++++++++++++++++++++++++--
> 7 files changed, 834 insertions(+), 21 deletions(-)
>
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index 2c60d000c..fea4cd130 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Northbound",
> - "version": "7.16.0",
> - "cksum": "3182666148 43912",
> + "version": "7.17.0",
> + "cksum": "2579043495 45129",
> "tables": {
> "NB_Global": {
> "columns": {
> @@ -179,6 +179,11 @@
> "refType": "strong"},
> "min": 0,
> "max": 1}},
> + "health_checks": {"type": {"key": {"type": "uuid",
> + "refTable":
> "Logical_Switch_Port_Health_Check",
> + "refType": "strong"},
> + "min": 0,
> + "max": "unlimited"}},
> "external_ids": {
> "type": {"key": "string", "value": "string",
> "min": 0, "max": "unlimited"}}},
> @@ -250,6 +255,24 @@
> "min": 0, "max": "unlimited"}}},
> "indexes": [["name"], ["id"]],
> "isRoot": true},
> + "Logical_Switch_Port_Health_Check": {
> + "columns": {
> + "protocol": {
> + "type": {"key": {"type": "string",
> + "enum": ["set", ["tcp", "udp", "icmp"]]},
> + "min": 0, "max": 1}},
As discussed in the other email thread, this should be mandatory so we
shouldn't specify "min"/"max".
> + "src_ip": {"type": "string"},
> + "port": {"type": {"key": {"type": "integer",
> + "minInteger": 0,
> + "maxInteger": 65535}}},
> + "address": {"type": {"key": "string",
> + "min": 0,
> + "max": 1}},
This should be mandatory too, so "type": "string".
> + "options": {
> + "type": {"key": "string",
> + "value": "string",
> + "min": 0, "max": "unlimited"}}},
> + "isRoot": false},
> "Forwarding_Group": {
> "columns": {
> "name": {"type": "string"},
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index ae37d03d1..627394cc4 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -2191,6 +2191,11 @@
> Please see the <ref table="Mirror"/> table.
> </column>
>
> + <column name="health_checks">
> + Health checks configuration for this logical switch port.
> + Please see the <ref table="Logical_Switch_Port_Health_Check"/> table.
> + </column>
> +
> <column name="ha_chassis_group">
> References a row in the OVN Northbound database's
> <ref table="HA_Chassis_Group" db="OVN_Northbound"/> table.
> @@ -2247,6 +2252,54 @@
> </group>
> </table>
>
> + <table name="Logical_Switch_Port_Health_Check"
> + title="logical switch port health check">
> + <p>
> + Each row represents health check configuration for logical switch port.
> + Health checks are used to monitor reachability and health of backend
> + endpoints associated with logical switch port. Health check can be
> + configured to use different protocols (TCP, UDP, or ICMP) to verify
> + availability of target IP addresses.
> + </p>
> +
> + <column name="protocol">
> + Valid protocols are <code>tcp</code>, <code>udp</code>, or
> + <code>icmp</code>. For <code>tcp</code> and <code>udp</code>
> + protocols - destination port must be specified.
> + </column>
> +
> + <column name="src_ip">
> + Source IP address used when sending health check probes.
> + </column>
> +
> + <column name="port">
> + Destination port number for TCP and UDP health checks.
> + </column>
> +
> + <column name="address">
> + IP address to monitor for the health check.
> + </column>
> +
> + <column name="options" key="interval" type='{"type": "integer"}'>
> + The interval, in seconds, between service monitor checks.
> + </column>
> +
> + <column name="options" key="timeout" type='{"type": "integer"}'>
> + The time, in seconds, after which the service monitor check times
> + out.
> + </column>
> +
> + <column name="options" key="success_count" type='{"type": "integer"}'>
> + The number of successful checks after which the service is
> + considered <code>online</code>.
> + </column>
> +
> + <column name="options" key="failure_count" type='{"type": "integer"}'>
> + The number of failure checks after which the service is considered
> + <code>offline</code>.
> + </column>
> + </table>
> +
> <table name="Forwarding_Group" title="forwarding group">
> <p>
> Each row represents one forwarding group.
> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> index 237330df3..d9a91739c 100644
> --- a/ovn-sb.ovsschema
> +++ b/ovn-sb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Southbound",
> - "version": "21.7.1",
> - "cksum": "3096184714 36646",
> + "version": "21.8.0",
> + "cksum": "614397313 36713",
> "tables": {
> "SB_Global": {
> "columns": {
> @@ -516,7 +516,8 @@
> "type": {"type": {"key": {
> "type": "string",
> "enum": ["set", ["load-balancer",
> - "network-function"]]},
> + "network-function",
> + "logical-switch-port"]]},
> "min": 0, "max": 1}},
> "ip": {"type": "string"},
> "mac": {"type": "string"},
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index d294a96ea..92496b911 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -5053,6 +5053,8 @@ tcp.flags = RST;
> icmp, and the health probe is done by injecting an icmp echo request
> packet into the <code>inport</code> of the Network_Function and then
> montoring the same packet coming out of the <code>outport</code>.
> + For <code>type</code> "logical-switch-port", supported protocols are
> + <code>tcp</code>, <code>udp</code> and <code>icmp</code>.
> </column>
>
> <column name="port">
> @@ -5135,6 +5137,15 @@ tcp.flags = RST;
> the service and doesn't expect any reply. If it receives an ICMP
> reply, then it considers the service to be <code>offline</code>.
> </p>
> +
> + <p>
> + For ICMP service, <code>ovn-controller</code> sends an ICMP
> + Echo Request and expects an ICMP Echo Reply to consider service
> + to be <code>online</code>. If no reply is received or if an ICMP
> + error message is received, the service is considered
> + <code>offline</code>.
> + </p>
> +
> </column>
> </group>
>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index f4cb89b82..e95cd9fa8 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -3519,3 +3519,199 @@ AT_CHECK([ovn-nbctl --may-exist lsp-add-localnet-port
> ls ln_port net1], [1], [],
> check ovn-nbctl lsp-set-options ln_port network_name=net1
> check ovn-nbctl --may-exist lsp-add-localnet-port ls ln_port net1
> ])
> +
> +dnl ---------------------------------------------------------------------
> +
> +AT_SETUP([ovn-nbctl - Logical Switch Port Health Check])
> +OVN_NBCTL_TEST_START daemon
> +
This only tests with ovn-nbctl in daemon mode. And that's not enough.
It actually misses the memory leak in lsp-hc-list, please see below.
It should be:
OVN_NBCTL_TEST([ovn_nbctl_lsp_health_check], [Logical Switch Port Health
Check], [
> +# Create logical switch port.
> +AT_CHECK([ovn-nbctl ls-add ls0])
> +AT_CHECK([ovn-nbctl lsp-add ls0 lport0])
> +AT_CHECK([ovn-nbctl lsp-set-addresses lport0 "00:11:22:33:44:55
> 192.168.1.10" "00:11:22:33:44:56 192.168.1.11"])
> +
> +# Create IPV4 ICMP health check.
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 icmp 192.168.0.255 192.168.1.10])
> +AT_CHECK([ovn-nbctl lsp-hc-list lport0], [0], [dnl
> +Logical Switch Port lport0:
> + Protocol : icmp
> + Source IP : 192.168.0.255
> + Address : 192.168.1.10
> +])
> +
> +# Check hc attaching to logical switch port.
> +hc_icmp_uuid=$(fetch_column nb:logical_switch_port_health_check _uuid
> src_ip="192.168.0.255")
> +check_column "$hc_icmp_uuid" nb:logical_switch_port health_checks
> +
> +# Create IPv4 TCP health check with the specified addresses.
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 tcp 10.0.0.2 80 192.168.1.10])
> +AT_CHECK([ovn-nbctl lsp-hc-list lport0], [0], [dnl
> +Logical Switch Port lport0:
> + Protocol : tcp
> + Source IP : 10.0.0.2
> + Port : 80
> + Address : 192.168.1.10
> +
> + Protocol : icmp
> + Source IP : 192.168.0.255
> + Address : 192.168.1.10
> +])
> +hc_tcp_uuid=$(fetch_column nb:logical_switch_port_health_check _uuid
> src_ip="10.0.0.2")
> +
> +# Create UDP health check with the specified addresses.
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 udp 10.0.0.3 53 192.168.1.11])
> +AT_CHECK([ovn-nbctl lsp-hc-list lport0], [0], [dnl
> +Logical Switch Port lport0:
> + Protocol : tcp
> + Source IP : 10.0.0.2
> + Port : 80
> + Address : 192.168.1.10
> +
> + Protocol : udp
> + Source IP : 10.0.0.3
> + Port : 53
> + Address : 192.168.1.11
> +
> + Protocol : icmp
> + Source IP : 192.168.0.255
> + Address : 192.168.1.10
> +])
> +hc_udp_uuid=$(fetch_column nb:logical_switch_port_health_check _uuid
> src_ip="10.0.0.3")
> +
> +check_column "$hc_icmp_uuid $hc_tcp_uuid $hc_udp_uuid"
> nb:logical_switch_port health_checks
> +
> +AT_CHECK([ovn-nbctl lsp-hc-del lport0 $hc_icmp_uuid])
> +# Check hcs detaching to logical switch port.
> +check_column "$hc_tcp_uuid $hc_udp_uuid" nb:logical_switch_port health_checks
> +
> +AT_CHECK([ovn-nbctl lsp-hc-list lport0], [0], [dnl
> +Logical Switch Port lport0:
> + Protocol : tcp
> + Source IP : 10.0.0.2
> + Port : 80
> + Address : 192.168.1.10
> +
> + Protocol : udp
> + Source IP : 10.0.0.3
> + Port : 53
> + Address : 192.168.1.11
> +])
> +
> +AT_CHECK([ovn-nbctl lsp-hc-del lport0])
> +check_row_count nb:Logical_Switch_Port_Health_Check 0
> +AT_CHECK([ovn-nbctl lsp-hc-list lport0], [0], [dnl])
> +
> +# Check invalid protocol.
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 stcp 10.0.0.1 50], [1], [], [stderr])
> +AT_CHECK([grep "Type must be icmp, tcp, or udp" stderr], [0], [ignore])
> +
> +# Check invalid source IP.
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 icmp invalid_ip invalid_ip], [1], [],
> [stderr])
> +AT_CHECK([grep "Not a valid IPv4 or IPv6 address" stderr], [0], [ignore])
> +
> +# Check TCP/UDP non-valid destination port.
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 tcp 10.0.0.1 70000], [1], [], [stderr])
> +AT_CHECK([grep "port must in range 0...65535" stderr], [0], [ignore])
> +
> +# Check IP address not configured on port.
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 icmp 10.0.0.1 192.168.99.99], [1], [],
> [stderr])
> +AT_CHECK([grep "Address 192.168.99.99 not configured on port" stderr], [0],
> [ignore])
> +
> +AT_CHECK([ovn-nbctl lsp-hc-del lport0])
> +
> +# Different source IP, same protocol, port and address - should be allowed.
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 tcp 10.0.0.1 80 192.168.1.10])
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 tcp 10.0.0.2 80 192.168.1.10], [0],
> [], [ignore])
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 tcp 10.0.0.3 80 192.168.1.10], [0],
> [], [ignore])
> +
> +# Same source IP, different ports, same protocol and address - should be
> allowed.
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 tcp 10.0.0.1 81 192.168.1.10], [0],
> [], [ignore])
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 tcp 10.0.0.1 82 192.168.1.10], [0],
> [], [ignore])
> +
> +# Same source IP and port, different protocols, same address - should be
> allowed.
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 udp 10.0.0.1 80 192.168.1.10], [0],
> [], [ignore])
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 icmp 10.0.0.1 192.168.1.10], [0], [],
> [ignore])
> +
> +# Check duplication.
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 tcp 10.0.0.5 90 192.168.1.10])
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 tcp 10.0.0.5 90 192.168.1.10], [1],
> [], [stderr])
> +AT_CHECK([grep "Health check with address 192.168.1.10 already exists on
> port lport0" stderr], [0], [ignore])
> +
> +# ICMP - same source IP, different addresses - should be allowed.
> +AT_CHECK([ovn-nbctl lsp-hc-del lport0])
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 icmp 10.0.0.9 192.168.1.10])
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 icmp 10.0.0.9 192.168.1.11], [0], [],
> [ignore])
> +
> +# ICMP - different source IP, same address - should be allowed.
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 icmp 10.0.0.10 192.168.1.10], [0], [],
> [ignore])
> +
> +# Check supported logical switch port type for monitoring.
> +AT_CHECK([ovn-nbctl lsp-add ls0 lport1])
> +AT_CHECK([ovn-nbctl lsp-set-type lport1 router])
> +AT_CHECK([ovn-nbctl lsp-hc-add lport1 icmp 10.0.0.1 10.0.0.2], [1], [],
> [stderr])
> +AT_CHECK([grep "Health check monitoring supported only for port with vif
> type" stderr], [0], [ignore])
> +
> +AT_CHECK([ovn-nbctl lsp-hc-del lport0])
> +
> +# Test IPv6 addresses support: icmp, tcp, udp.
> +AT_CHECK([ovn-nbctl lsp-add ls0 lport3])
> +AT_CHECK([ovn-nbctl lsp-set-addresses lport3 "00:11:22:33:44:57
> 2001:db8::1"])
> +AT_CHECK([ovn-nbctl lsp-hc-add lport3 icmp 2001:db8::2 2001:db8::1])
> +AT_CHECK([ovn-nbctl lsp-hc-add lport3 tcp 2001:db8::2 80 2001:db8::1])
> +AT_CHECK([ovn-nbctl lsp-hc-add lport3 udp 2001:db8::2 53 2001:db8::1])
> +AT_CHECK([ovn-nbctl lsp-hc-list lport3], [0], [dnl
> +Logical Switch Port lport3:
> + Protocol : icmp
> + Source IP : 2001:db8::2
> + Address : 2001:db8::1
> +
> + Protocol : tcp
> + Source IP : 2001:db8::2
> + Port : 80
> + Address : 2001:db8::1
> +
> + Protocol : udp
> + Source IP : 2001:db8::2
> + Port : 53
> + Address : 2001:db8::1
> +])
> +
> +AT_CHECK([ovn-nbctl lsp-hc-del lport3])
> +
> +# Duplicate IPv6 health check - should be disabled.
> +AT_CHECK([ovn-nbctl lsp-hc-add lport3 icmp 2001:db8::2 2001:db8::1])
> +AT_CHECK([ovn-nbctl lsp-hc-add lport3 icmp 2001:db8::2 2001:db8::1], [1],
> [], [stderr])
> +AT_CHECK([grep "Health check with address 2001:db8::1 already exists on port
> lport3" stderr], [0], [ignore])
> +
> +# Check options printing.
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 icmp 10.0.0.4 192.168.1.10
> 192.168.1.11])
> +AT_CHECK([ovn-nbctl lsp-hc-list lport0], [0], [dnl
> +Logical Switch Port lport0:
> + Protocol : icmp
> + Source IP : 10.0.0.4
> + Address : 192.168.1.10
> +])
> +hc_icmp_uuid=$(fetch_column nb:logical_switch_port_health_check _uuid
> src_ip="10.0.0.4")
> +
> +AT_CHECK([ovn-nbctl set logical_switch_port_health_check $hc_icmp_uuid
> options:interval=3])
> +AT_CHECK([ovn-nbctl set logical_switch_port_health_check $hc_icmp_uuid
> options:timeout=30])
> +AT_CHECK([ovn-nbctl set logical_switch_port_health_check $hc_icmp_uuid
> options:success_count=1])
> +AT_CHECK([ovn-nbctl set logical_switch_port_health_check $hc_icmp_uuid
> options:failure_count=2])
> +
> +AT_CHECK([ovn-nbctl lsp-hc-list lport0], [0], [dnl
> +Logical Switch Port lport0:
> + Protocol : icmp
> + Source IP : 10.0.0.4
> + Address : 192.168.1.10
> + Interval : 3
> + Timeout : 30
> + Success count : 1
> + Failure count : 2
> +])
> +
> +AT_CHECK([ovn-nbctl lsp-hc-del lport0])
> +AT_CHECK([ovn-nbctl lsp-hc-del lport3])
> +check_row_count nb:Logical_Switch_Port_Health_Check 0
> +
> +OVN_NBCTL_TEST_STOP "/terminating with signal 15/d"
> +AT_CLEANUP
> diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
> index 253f6d8fc..4fbd0bb0e 100644
> --- a/utilities/ovn-nbctl.8.xml
> +++ b/utilities/ovn-nbctl.8.xml
> @@ -1823,6 +1823,58 @@
> </dd>
> </dl>
>
> + <h2>Health Check commands</h2>
> + <dl>
> + <dt><code>lsp-hc-add</code> <var>PORT</var> <var>PROTOCOL</var>
> + <var>SOURCE_IP</var> [<var>DST_PORT</var>] <var>ADDRESS</var></dt>
> + <dd>
> + <p>
> + Creates a new health check configuration for the logical switch
> port
> + <code>PORT</code> with the below mandatory arguments.
> + </p>
> +
> + <p>
> + <var>PROTOCOL</var> specifies the health check protocol -
> + <code>tcp</code>, <code>udp</code>, or <code>icmp</code>.
> + </p>
> +
> + <p>
> + <var>SOURCE_IP</var> specifies the source IP address used when
> + sending health check probes.
> + </p>
> +
> + <p>
> + <var>DST_PORT</var> specifies the destination port number for
> + <code>tcp</code> and <code>udp</code> health checks. This parameter
> + is ignored for <code>icmp</code> protocol.
> + </p>
> +
> + <p>
> + <var>ADDRESS</var> specifies the IP address to monitor.
> + </p>
> +
> + <p>
> + Additional health check options such as interval, timeout,
> + success count, and failure count can be configured separately.
> + </p>
> + </dd>
> +
> + <dt><code>lsp-hc-del</code> <var>PORT</var> [<var>HC_UUID</var>]</dt>
> + <dd>
> + <p>
> + Deletes health check configuration for the logical switch port
> + <code>PORT</code>.
> + </p>
> + </dd>
> +
> + <dt><code>lsp-hc-list</code> <var>PORT</var></dt>
> + <dd>
> + Lists all health check configurations for the logical switch port
> + <code>PORT</code>, including protocol, source IP, destination port,
> + monitored addresses, and current status.
> + </dd>
> + </dl>
> +
> <h2>Synchronization Commands</h2>
>
> <dl>
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 2b674cad3..ac69688bd 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -66,6 +66,18 @@ string_ptr(char *ptr)
> return (ptr) ? ptr : s;
> }
>
> +static char *OVS_WARN_UNUSED_RESULT
> +parse_l4_port(const char *arg, uint16_t *port_p)
> +{
> + int64_t port;
> + if (!ovs_scan(arg, "%"SCNd64, &port)
> + || port < 0 || port > UINT16_MAX) {
> + return xasprintf("%s: port must in range 0...65535", arg);
> + }
> + *port_p = port;
> + return NULL;
> +}
> +
> static void
> nbctl_add_base_prerequisites(struct ovsdb_idl *idl,
> enum nbctl_wait_type wait_type)
> @@ -544,6 +556,12 @@ MAC_Binding commands:\n\
> Delete Static_MAC_Binding entry\n\
> static-mac-binding-list List all Static_MAC_Binding entries\n\
> \n\
> +Logical Switch Port Health Check:\n\
> + lsp-hc-add PORT PROTOCOL SOURCE_IP DST_PORT ADDRESS...\n\
> + add health check monitoring for PORT\n\
> + lsp-hc-del PORT HC_UUID delete health check monitoring for PORT\n\
> + lsp-hc-list PORT print health check for PORT\n\
Nit: I'd say "list health checks for PORT". Also, this should be
aligned under "delete".
> +\n\
> %s\
> %s\
> \n\
> @@ -1464,18 +1482,23 @@ nbctl_pre_lsp_set_addresses(struct ctl_context *ctx)
> &nbrec_logical_switch_port_col_dynamic_addresses);
> }
>
> -static char *
> +static bool
> lsp_contains_duplicate_ip(struct lport_addresses *laddrs1,
> struct lport_addresses *laddrs2,
> - const struct nbrec_logical_switch_port *lsp_test)
> + const struct nbrec_logical_switch_port *lsp_test,
> + char **error_str)
> {
> for (size_t i = 0; i < laddrs1->n_ipv4_addrs; i++) {
> for (size_t j = 0; j < laddrs2->n_ipv4_addrs; j++) {
> if (laddrs1->ipv4_addrs[i].addr == laddrs2->ipv4_addrs[j].addr) {
> - return xasprintf("duplicate IPv4 address '%s' found on "
> - "logical switch port '%s'",
> - laddrs1->ipv4_addrs[i].addr_s,
> - lsp_test->name);
> + if (error_str) {
> + *error_str = xasprintf("duplicate IPv4 address '%s' "
> + "found on logical switch "
> + "port '%s'",
> + laddrs1->ipv4_addrs[i].addr_s,
> + lsp_test->name);
> + }
> + return true;
> }
> }
> }
> @@ -1484,15 +1507,23 @@ lsp_contains_duplicate_ip(struct lport_addresses
> *laddrs1,
> for (size_t j = 0; j < laddrs2->n_ipv6_addrs; j++) {
> if (IN6_ARE_ADDR_EQUAL(&laddrs1->ipv6_addrs[i].addr,
> &laddrs2->ipv6_addrs[j].addr)) {
> - return xasprintf("duplicate IPv6 address '%s' found on "
> - "logical switch port '%s'",
> - laddrs1->ipv6_addrs[i].addr_s,
> - lsp_test->name);
> + if (error_str) {
> + *error_str = xasprintf("duplicate IPv6 address "
> + "'%s' found on logical "
> + "switch port '%s'",
> + laddrs1->ipv6_addrs[i].addr_s,
> + lsp_test->name);
> + }
> + return true;
> }
> }
> }
>
> - return NULL;
> + if (error_str) {
> + *error_str = NULL;
> + }
> +
> + return false;
> }
>
> static char *
> @@ -1500,12 +1531,13 @@ lsp_contains_duplicates(const struct
> nbrec_logical_switch *ls,
> const struct nbrec_logical_switch_port *lsp,
> const char *address)
> {
> + char *error = NULL;
> + char *sub_error = NULL;
> struct lport_addresses laddrs;
> if (!extract_lsp_addresses(address, &laddrs)) {
> return NULL;
> }
>
> - char *sub_error = NULL;
> for (size_t i = 0; i < ls->n_ports; i++) {
> struct nbrec_logical_switch_port *lsp_test = ls->ports[i];
> if (lsp_test == lsp) {
> @@ -1518,10 +1550,11 @@ lsp_contains_duplicates(const struct
> nbrec_logical_switch *ls,
> addr = lsp_test->dynamic_addresses;
> }
> if (extract_lsp_addresses(addr, &laddrs_test)) {
> - sub_error = lsp_contains_duplicate_ip(&laddrs, &laddrs_test,
> - lsp_test);
> + bool has_duplicate =
> + lsp_contains_duplicate_ip(&laddrs, &laddrs_test,
> + lsp_test, &sub_error);
> destroy_lport_addresses(&laddrs_test);
> - if (sub_error) {
> + if (has_duplicate) {
> goto err_out;
> }
> }
> @@ -1529,7 +1562,6 @@ lsp_contains_duplicates(const struct
> nbrec_logical_switch *ls,
> }
>
> err_out: ;
> - char *error = NULL;
This can stay here, no need to move it up in the beginning of the function.
> if (sub_error) {
> error = xasprintf("Error on switch %s: %s", ls->name, sub_error);
> free(sub_error);
> @@ -8690,6 +8722,441 @@ nbctl_lsp_add_misc_port(struct ctl_context *ctx)
> shash_add(&nbctx->lsp_to_ls_map, lsp_name, ls);
> }
>
> +/* Logical Switch Port Health Check Functions. */
> +enum health_check_protocol {
> + LSP_ICMP_HEALTH_CHECK,
> + LSP_TCP_HEALTH_CHECK,
> + LSP_UDP_HEALTH_CHECK,
> +};
> +
> +static char *
> +lsp_health_check_parse_protocol(const char *type,
> + enum health_check_protocol *protocol)
> +{
> + char *error = NULL;
> +
> + if (!strcmp(type, "icmp")) {
> + *protocol = LSP_ICMP_HEALTH_CHECK;
> + } else if (!strcmp(type, "tcp")) {
> + *protocol = LSP_TCP_HEALTH_CHECK;
> + } else if (!strcmp(type, "udp")) {
> + *protocol = LSP_UDP_HEALTH_CHECK;
> + } else {
> + error = xasprintf("%s: Type must be icmp, tcp, or udp.", type);
> + }
> +
> + return error;
> +}
> +
> +static char *
> +lsp_health_check_parse_dst_port(struct ctl_context *ctx,
> + enum health_check_protocol protocol,
> + const char *protocol_str,
> + uint16_t *destination_port,
> + int *target_ip_index)
> +{
> + char *error;
> +
> + switch (protocol) {
> + case LSP_TCP_HEALTH_CHECK:
> + case LSP_UDP_HEALTH_CHECK:
> + if (ctx->argc < 5) {
> + error = xasprintf("Destination port required for "
> + "%s health check.", protocol_str);
> + return error;
> + }
> +
> + error = parse_l4_port(ctx->argv[4], destination_port);
> + if (error) {
> + return error;
> + }
> +
> + *target_ip_index = 5;
> + break;
> +
> + case LSP_ICMP_HEALTH_CHECK:
> + *target_ip_index = 4;
> + break;
> +
> + default:
> + OVS_NOT_REACHED();
> + }
> +
> + if (*target_ip_index == ctx->argc) {
> + error = xasprintf("No addresses specified for health checking.");
> + return error;
> + }
This last check belongs in the caller, in nbctl_lsp_health_check_add().
> +
> + return NULL;
> +}
> +
> +static bool
> +lsp_health_check_get_duplicate_record(
> + int64_t destination_port, const char *protocol_str,
> + const char *source_ip_str, const char *target_ip_str,
> + const struct nbrec_logical_switch_port *lsp)
> +{
> + for (size_t i = 0; i < lsp->n_health_checks; i++) {
> + const struct nbrec_logical_switch_port_health_check *lsp_hc_p =
> + lsp->health_checks[i];
> +
> + if (!strcmp(lsp_hc_p->src_ip, source_ip_str) &&
> + !strcmp(lsp_hc_p->protocol, protocol_str)) {
> +
> + if (strcmp(protocol_str, "icmp") &&
> + lsp_hc_p->port != destination_port) {
> + continue;
> + }
> +
> + if (strcmp(lsp_hc_p->address, target_ip_str) == 0) {
> + return true;
> + }
> + }
> + }
> +
> + return false;
> +}
> +
> +static char *
> +lsp_health_check_parse_target_address(
> + const char *target_ip_str, const char *protocol_str,
> + const char *source_ip_str, const int64_t destination_port,
> + const struct nbrec_logical_switch_port *lsp)
> +{
> + bool ip_found_on_port = false;
> + char *error = NULL;
> +
> + struct lport_addresses target_address;
> + if (!extract_ip_address(target_ip_str, &target_address)) {
> + error = xasprintf("Not a valid IPv4 or IPv6 address %s.",
> + target_ip_str);
> + return error;
> + }
> +
> + struct lport_addresses lsp_address;
> + for (size_t i = 0; i < lsp->n_addresses; i++) {
> + if (!extract_lsp_addresses(lsp->addresses[i], &lsp_address)) {
> + error = xasprintf("Error extracting logical switch port
> address.");
> + goto cleanup;
> + }
> +
> + if (lsp_contains_duplicate_ip(&target_address,
> + &lsp_address, lsp, NULL)) {
> + ip_found_on_port = true;
> + }
> +
> + destroy_lport_addresses(&lsp_address);
> +
> + if (ip_found_on_port) {
> + break;
> + }
> + }
> +
> + if (!ip_found_on_port) {
> + error = xasprintf("Address %s not configured on port %s.",
> + target_ip_str, lsp->name);
> + goto cleanup;
> + }
> +
> + if (lsp_health_check_get_duplicate_record(destination_port,
> + protocol_str,
> + source_ip_str,
> + target_ip_str, lsp)) {
> + error = xasprintf("Health check with address %s already exists "
> + "on port %s.", target_ip_str, lsp->name);
> + goto cleanup;
> + }
> +
> +cleanup:
> + destroy_lport_addresses(&target_address);
> + return error;
> +}
> +
> +static void
> +nbctl_pre_lsp_health_check_add(struct ctl_context *ctx)
> +{
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_logical_switch_port_col_name);
We're missing:
ovsdb_idl_add_column(ctx->idl,
&nbrec_logical_switch_port_col_type);
That's why the nbctl.at test you added doesn't pass in "direct" mode.
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_logical_switch_port_col_addresses);
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_logical_switch_port_col_health_checks);
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_logical_switch_port_health_check_col_port);
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_logical_switch_port_health_check_col_protocol);
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_logical_switch_port_health_check_col_src_ip);
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_logical_switch_port_health_check_col_address);
> +}
> +
> +static void
> +nbctl_lsp_health_check_add(struct ctl_context *ctx)
> +{
> + const struct nbrec_logical_switch_port *lsp = NULL;
> + const struct nbrec_logical_switch_port_health_check *lsp_hc = NULL;
> +
> + const char *lsp_port_name = ctx->argv[1];
> + const char *protocol_str = ctx->argv[2];
> + const char *src_ip_str = ctx->argv[3];
> + char *error = NULL;
> +
> + error = lsp_by_name_or_uuid(ctx, lsp_port_name, true, &lsp);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> +
> + if (lsp->type && lsp->type[0]) {
> + ctl_error(ctx, "%s: Health check monitoring supported only for"
> + " port with vif type.", lsp->type);
> + return;
> + }
> +
> + char *src_ip = normalize_addr_str(src_ip_str);
> + if (!src_ip) {
> + ctl_error(ctx, "%s: Not a valid IPv4 or IPv6 address.", src_ip_str);
> + goto cleanup;
> + }
> +
> + enum health_check_protocol protocol;
> + error = lsp_health_check_parse_protocol(protocol_str, &protocol);
> + if (error) {
> + ctx->error = error;
> + goto cleanup;
> + }
> +
> + uint16_t destination_port = 0;
> + int target_ip_index = 0;
> + error = lsp_health_check_parse_dst_port(ctx, protocol,
> + protocol_str,
> + &destination_port,
> + &target_ip_index);
> + if (error) {
> + ctx->error = error;
> + goto cleanup;
> + }
> +
> + const char *hc_address = ctx->argv[target_ip_index];
> + error = lsp_health_check_parse_target_address(hc_address,
> + protocol_str,
> + src_ip,
> + destination_port,
> + lsp);
> + if (error) {
> + ctx->error = error;
> + goto cleanup;
> + }
> +
> + lsp_hc = nbrec_logical_switch_port_health_check_insert(ctx->txn);
> +
> + nbrec_logical_switch_port_health_check_set_protocol(lsp_hc,
> protocol_str);
> + nbrec_logical_switch_port_health_check_set_src_ip(lsp_hc, src_ip);
> + nbrec_logical_switch_port_health_check_set_port(lsp_hc,
> destination_port);
> + nbrec_logical_switch_port_health_check_set_address(lsp_hc, hc_address);
> + nbrec_logical_switch_port_verify_health_checks(lsp);
> + nbrec_logical_switch_port_update_health_checks_addvalue(lsp, lsp_hc);
> +
> +cleanup:
> + free(src_ip);
> +}
> +
> +static char * OVS_WARN_UNUSED_RESULT
> +find_logical_switch_port_health_check_by_uuid(
Nit: to be more aligned with the rest of the helpers in this file, I'd
call this logical_switch_port_health_check_by_uuid().
> + struct ctl_context *ctx,
> + const char *id,
> + const struct nbrec_logical_switch_port_health_check **lsp_hc_p)
> +{
> + const struct nbrec_logical_switch_port_health_check *lsp_hc;
> + *lsp_hc_p = NULL;
> +
> + struct uuid lsp_hc_uuid;
> + bool is_uuid = uuid_from_string(&lsp_hc_uuid, id);
> + if (!is_uuid) {
> + return xasprintf("%s: Invalid UUID format.", id);
> + }
> +
> + lsp_hc = nbrec_logical_switch_port_health_check_get_for_uuid(
> + ctx->idl, &lsp_hc_uuid);
> + if (!lsp_hc) {
> + return xasprintf("%s: Logical Switch Port Health Check "
> + "not found.", id);
> + }
> +
> + *lsp_hc_p = lsp_hc;
> +
> + return NULL;
> +}
> +
> +static void
> +nbctl_pre_lsp_health_check_del(struct ctl_context *ctx)
> +{
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_logical_switch_port_col_name);
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_logical_switch_port_col_health_checks);
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_logical_switch_port_health_check_col_protocol);
> +}
> +
> +static void
> +nbctl_lsp_health_check_del(struct ctl_context *ctx)
> +{
> + const struct nbrec_logical_switch_port_health_check *lsp_hc = NULL;
> + const struct nbrec_logical_switch_port *lsp = NULL;
> + const char *port_id = ctx->argv[1];
> + const char *hc_uuid = ctx->argv[2];
> +
> + char *error;
> + error = lsp_by_name_or_uuid(ctx, port_id, true, &lsp);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> +
> + if (hc_uuid) {
> + error = find_logical_switch_port_health_check_by_uuid(ctx, hc_uuid,
> + &lsp_hc);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> +
> + nbrec_logical_switch_port_update_health_checks_delvalue(lsp, lsp_hc);
> + nbrec_logical_switch_port_health_check_delete(lsp_hc);
> + return;
> + }
> +
> + for (size_t i = 0; i < lsp->n_health_checks; i++) {
> + nbrec_logical_switch_port_update_health_checks_delvalue(
> + lsp, lsp->health_checks[i]);
> + nbrec_logical_switch_port_health_check_delete(lsp->health_checks[i]);
> + }
> +}
> +
> +static int
> +lsp_health_check_cmp(const void *lsp_hc_1_, const void *lsp_hc_2_)
> +{
> + const struct nbrec_logical_switch_port_health_check *const *lsp_hc_1p =
> + lsp_hc_1_;
> + const struct nbrec_logical_switch_port_health_check *const *lsp_hc_2p =
> + lsp_hc_2_;
> + const struct nbrec_logical_switch_port_health_check *lsp_hc_1 =
> + *lsp_hc_1p;
> + const struct nbrec_logical_switch_port_health_check *lsp_hc_2 =
> + *lsp_hc_2p;
> +
> + int src_ip_cmp = strcmp(lsp_hc_1->src_ip, lsp_hc_2->src_ip);
> + if (src_ip_cmp) {
> + return src_ip_cmp;
> + }
> +
> + int protocol_cmp = strcmp(lsp_hc_1->protocol, lsp_hc_2->protocol);
> + if (protocol_cmp) {
> + return protocol_cmp;
> + }
> +
> + if (strcmp(lsp_hc_1->protocol, "icmp")) {
> + if (lsp_hc_1->port != lsp_hc_2->port) {
> + return lsp_hc_1->port < lsp_hc_2->port ? -1 : 1;
> + }
> + }
> +
> + int ip_cmp = strcmp(lsp_hc_1->address, lsp_hc_2->address);
> + if (ip_cmp) {
> + return ip_cmp;
> + }
> +
> + return 0;
> +}
> +
> +static void
> +nbctl_pre_lsp_health_check_list(struct ctl_context *ctx)
> +{
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_logical_switch_port_col_name);
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_logical_switch_port_col_health_checks);
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_logical_switch_port_health_check_col_port);
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_logical_switch_port_health_check_col_protocol);
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_logical_switch_port_health_check_col_src_ip);
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_logical_switch_port_health_check_col_address);
Same comment for options as in the pre-add function, we're missing:
ovsdb_idl_add_column(ctx->idl,
&nbrec_logical_switch_port_health_check_col_options);
> +}
> +
> +static void
> +nbctl_lsp_health_check_list(struct ctl_context *ctx)
> +{
> + const struct nbrec_logical_switch_port_health_check **lsp_hcs;
> + const struct nbrec_logical_switch_port *lsp = NULL;
> + const char *port = ctx->argv[1];
> +
> + char *error;
> + error = lsp_by_name_or_uuid(ctx, port, true, &lsp);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> +
> + if (!lsp->n_health_checks) {
> + return;
> + }
> +
> + lsp_hcs = xmalloc(sizeof *lsp_hcs * lsp->n_health_checks);
We leak 'lsp_hcs' when we exit this function.
> + memcpy(lsp_hcs, lsp->health_checks,
> + lsp->n_health_checks * sizeof(*lsp_hcs));
> +
> + qsort(lsp_hcs, lsp->n_health_checks,
> + sizeof *lsp_hcs, lsp_health_check_cmp);
Nit: I'd write this as:
qsort(lsp_hcs, lsp->n_health_checks, sizeof *lsp_hcs,
lsp_health_check_cmp);
> +
> + ds_put_format(&ctx->output, "Logical Switch Port %s:\n", port);
> + for (size_t i = 0; i < lsp->n_health_checks; i++) {
> + const struct nbrec_logical_switch_port_health_check *hc = lsp_hcs[i];
> +
> + ds_put_format(&ctx->output, " Protocol : %s\n",
> + hc->protocol);
> + ds_put_format(&ctx->output, " Source IP : %s\n",
> + hc->src_ip);
> +
> + if (strcmp(hc->protocol, "icmp")) {
> + ds_put_format(&ctx->output, " Port : %"PRId64"\n",
> + hc->port);
> + }
> +
> + ds_put_format(&ctx->output, " Address : ");
This needs two more spaces before ':', otherwise it's not aligned nicely
in the output, see the tests in ovn-nbctl.at.
> + ds_put_format(&ctx->output, "%s", hc->address);
> + ds_put_format(&ctx->output, "\n");
> +
> + int interval = smap_get_int(&hc->options, "interval", 0);
> + int timeout = smap_get_int(&hc->options, "timeout", 0);
> + int success_count = smap_get_int(&hc->options, "success_count", 0);
> + int failure_count = smap_get_int(&hc->options, "failure_count", 0);
> + if (interval) {
> + ds_put_format(&ctx->output, " Interval : %d\n",
> + interval);
> + }
> + if (timeout) {
> + ds_put_format(&ctx->output, " Timeout : %d\n",
> + timeout);
> + }
> + if (success_count) {
> + ds_put_format(&ctx->output, " Success count : %d\n",
> + success_count);
> + }
> + if (failure_count) {
> + ds_put_format(&ctx->output, " Failure count : %d\n",
> + failure_count);
> + }
> + if (i < lsp->n_health_checks - 1) {
> + ds_put_format(&ctx->output, "\n");
> + }
> + }
free(lsp_hcs);
> +}
> +
> static const struct ctl_table_class tables[NBREC_N_TABLES] = {
> [NBREC_TABLE_DHCP_OPTIONS].row_ids
> = {{&nbrec_logical_switch_port_col_name, NULL,
> @@ -9077,6 +9544,16 @@ static const struct ctl_command_syntax
> nbctl_commands[] = {
> nbctl_pre_static_mac_binding, nbctl_static_mac_binding_list, NULL,
> "", RO },
>
> + /* Health Check commands */
> + {"lsp-hc-add", 4, 5, "PORT TYPE SRC_IP [DST_PORT] ADDRESS",
s/TYPE/PROTOCOL/
> + nbctl_pre_lsp_health_check_add, nbctl_lsp_health_check_add,
> + NULL, "", RW },
> + {"lsp-hc-del", 1, INT_MAX, "PORT [HC_UUID]",
> + nbctl_pre_lsp_health_check_del, nbctl_lsp_health_check_del,
> + NULL, "", RW },
> + {"lsp-hc-list", 1, 1, "PORT", nbctl_pre_lsp_health_check_list,
> + nbctl_lsp_health_check_list, NULL, "", RW },
> +
> {NULL, 0, 0, NULL, NULL, NULL, NULL, "", RO},
> };
>
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
