On 18.02.2026 23:23, Dumitru Ceara wrote:
> 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).
Hi! Thank you so much for the review, your time, and all fixes! I just
wanted to say I'm worried that you spent so much time on all the changes
during the review—I understand time is short right now due to branching,
but I have no problem making any fixes myself. Thanks again for everything)
> 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?
Yeah, i will
>
> 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
>
--
regards,
Alexandra.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
