Hi Alexandra,

I have several notes inline below.

On Fri, Dec 19, 2025 at 4:49 AM Alexandra Rukomoinikova
<[email protected]> wrote:
>
> 1) Added tables for further implementation logic of service monitors for
> logical switch ports:
>
> New table:
>     - Logical_Switch_Port_Health_Check: Health check configuration
>       for logical switch port.
>
> Modified tables:
>     - Logical_Switch_Port: Add 'health_checks' column referencing
>       health checks configuration.
>
> 2) Added commands to create, delete, and describe health checks for logical 
> switch ports.
>
> Signed-off-by: Alexandra Rukomoinikova <[email protected]>
> ---
>  lib/ovn-util.c            |  17 ++
>  lib/ovn-util.h            |   2 +
>  ovn-nb.ovsschema          |  28 ++-
>  ovn-nb.xml                |  54 ++++
>  tests/ovn-nbctl.at        | 186 ++++++++++++++
>  utilities/ovn-nbctl.8.xml |  54 ++++
>  utilities/ovn-nbctl.c     | 512 ++++++++++++++++++++++++++++++++++++++
>  7 files changed, 851 insertions(+), 2 deletions(-)
>
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index cec029e42..16f7eb2b9 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -1704,3 +1704,20 @@ is_partial_uuid_match(const struct uuid *uuid, const 
> char *match)
>      s2 = strip_leading_zero(s2);
>      return !strncmp(s1, s2, strlen(s2));
>  }
> +
> +char *
> +skip_mac_address_from_lsp_address(const char *address_with_mac)
> +{
> +    if (!address_with_mac) {
> +        return NULL;
> +    }
> +    const char *ptr = address_with_mac;
> +    while (*ptr == ' ') {
> +        ptr++;
> +    }
> +    ptr += (char) 17;
> +    while (*ptr == ' ') {
> +        ptr++;
> +    }
> +    return (char *) ptr;
> +}
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index daff01995..7685118e6 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -775,4 +775,6 @@ NEIGH_REDISTRIBUTE_MODES
>  enum neigh_redistribute_mode
>  parse_neigh_dynamic_redistribute(const struct smap *options);
>
> +char *skip_mac_address_from_lsp_address(const char *address_with_mac);
> +
>  #endif /* OVN_UTIL_H */
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index 8c2c1d861..5373e6f59 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>  {
>      "name": "OVN_Northbound",
> -    "version": "7.15.0",
> -    "cksum": "4060410729 43708",
> +    "version": "7.16.0",
> +    "cksum": "3363473672 44948",
>      "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": "weak"},
> +                                 "min": 0,
> +                                 "max": "unlimited"}},
>                  "external_ids": {
>                      "type": {"key": "string", "value": "string",
>                               "min": 0, "max": "unlimited"}}},
> @@ -246,6 +251,25 @@
>                               "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}},
> +                "src_ip": {"type": "string"},
> +                "port": {"type": {"key": {"type": "integer",
> +                                          "minInteger": 0,
> +                                          "maxInteger": 65535}}},
> +                "addresses": {"type": {"key": "string",
> +                                       "min": 0,
> +                                       "max": "unlimited"}},
> +                "options": {
> +                     "type": {"key": "string",
> +                              "value": "string",
> +                              "min": 0,
> +                              "max": "unlimited"}}},
> +            "isRoot": true},

IMO, it makes sense for this to have isRoot: false. I can't see a
reason for a logical switch port health check to exist without being
attached to a logical switch port.

>          "Forwarding_Group": {
>              "columns": {
>                  "name": {"type": "string"},
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 730293e97..3cc46e60c 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -2115,6 +2115,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.
> @@ -2171,6 +2176,55 @@
>      </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="addresses">
> +      A set of IP addresses to monitor. If no specific addresses are 
> provided,
> +      health check will use all addresses configured on logical switch port.
> +    </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/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index dccf30758..62b89945b 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -3499,3 +3499,189 @@ 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
> +
> +# 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 icmp health check with the specified addresses
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 icmp 192.168.0.255 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     :  192.168.0.255
> +  Addresses     :  192.168.1.10, 192.168.1.11
> +])
> +
> +# 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 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
> +  Addresses     :  192.168.1.10
> +
> +  Protocol      :  icmp
> +  Source IP     :  192.168.0.255
> +  Addresses     :  192.168.1.10, 192.168.1.11
> +])
> +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
> +  Addresses     :  192.168.1.10
> +
> +  Protocol      :  udp
> +  Source IP     :  10.0.0.3
> +  Port          :  53
> +  Addresses     :  192.168.1.11
> +
> +  Protocol      :  icmp
> +  Source IP     :  192.168.0.255
> +  Addresses     :  192.168.1.10, 192.168.1.11
> +])
> +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
> +  Addresses     :  192.168.1.10
> +
> +  Protocol      :  udp
> +  Source IP     :  10.0.0.3
> +  Port          :  53
> +  Addresses     :  192.168.1.11
> +])
> +
> +AT_CHECK([ovn-nbctl lsp-hc-del lport0])
> +AT_CHECK([ovn-nbctl lsp-hc-list lport0], [0], [dnl])
> +
> +# Create health check without specified addresses
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 icmp 10.0.0.4])
> +AT_CHECK([ovn-nbctl lsp-hc-list lport0], [0], [dnl
> +Logical Switch Port lport0:
> +  Protocol      :  icmp
> +  Source IP     :  10.0.0.4
> +])
> +
> +# Check invalid protocol
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 stcp 10.0.0.1], [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], [1], [], [stderr])
> +AT_CHECK([grep "Not a valid IPv4 or IPv6 address" stderr], [0], [ignore])
> +
> +# Check TCP/UDP without destination port
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 tcp 10.0.0.1], [1], [], [stderr])
> +AT_CHECK([grep "Destination port required for tcp health check" stderr], 
> [0], [ignore])
> +
> +# Check invalid 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])
> +
> +# Check Duplicate health check configuration
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 icmp 10.0.0.4], [1], [], [stderr])
> +AT_CHECK([grep "Health check already exists" stderr], [0], [ignore])
> +
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 icmp 10.0.0.1 192.168.1.10])
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 icmp 10.0.0.1 192.168.1.10], [1], [], 
> [stderr])
> +AT_CHECK([grep "Health check already exists" stderr], [0], [ignore])
> +
> +# Check same protocol with different ports - it's ok
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 tcp 10.0.0.1 90 192.168.1.10])
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 tcp 10.0.0.1 443 192.168.1.10], [0], 
> [], [ignore])
> +
> +# Check different protocol with same ports - it's ok
> +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 udp 10.0.0.1 80 192.168.1.10], [0], 
> [], [ignore])
> +
> +# Check different source ip with same ports and addresses - it's ok
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 tcp 10.0.0.1 92 192.168.1.10])
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 tcp 10.0.0.2 92 192.168.1.10], [0], 
> [], [ignore])
> +
> +# Check multipal same addresses
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 tcp 10.0.0.1 93 192.168.1.10 
> 192.168.1.11])
> +AT_CHECK([ovn-nbctl lsp-hc-add lport0 tcp 10.0.0.1 93 192.168.1.10 
> 192.168.1.11], [1], [], [stderr])
> +AT_CHECK([grep "Health check already exists" stderr], [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], [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
> +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-list lport3], [0], [dnl
> +Logical Switch Port lport3:
> +  Protocol      :  icmp
> +  Source IP     :  2001:db8::2
> +  Addresses     :  2001:db8::1
> +])
> +
> +# 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
> +  Addresses     :  192.168.1.10, 192.168.1.11
> +])
> +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
> +  Addresses     :  192.168.1.10, 192.168.1.11
> +  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 7df902944..efc781d77 100644
> --- a/utilities/ovn-nbctl.8.xml
> +++ b/utilities/ovn-nbctl.8.xml
> @@ -1822,6 +1822,60 @@
>        </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 one or more IP addresses to monitor.
> +          If no addresses are provided, the health check will monitor all IP
> +          addresses configured on the logical switch port.
> +        </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 cdf6b578a..b8802ab30 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_range(const char *arg, int64_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\
> +\n\
>  %s\
>  %s\
>  \n\
> @@ -8677,6 +8695,490 @@ 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 bool
> +parse_health_check_protocol(struct ctl_context *ctx, const char *type,
> +                            enum health_check_protocol *protocol)
> +{
> +    if (!strcmp(type, "icmp")) {
> +        *protocol = LSP_ICMP_HEALTH_CHECK;
> +        return true;
> +    } else if (!strcmp(type, "tcp")) {
> +        *protocol = LSP_TCP_HEALTH_CHECK;
> +        return true;
> +    } else if (!strcmp(type, "udp")) {
> +        *protocol = LSP_UDP_HEALTH_CHECK;
> +        return true;
> +    } else {
> +        ctl_error(ctx, "%s: Type must be icmp, tcp or udp", type);
> +        return false;
> +    }
> +}
> +
> +static void
> +lsp_health_check_get_duplicate(
> +    int proto, const char *src_ip,
> +    int64_t port, struct sset *addresses,
> +    const struct nbrec_logical_switch_port *lsp,
> +    const struct nbrec_logical_switch_port_health_check **lsp_hc)
> +{

Instead of returning void, this could return a const struct
nbrec_logical_switch_port_health_check pointer. A NULL return means no
duplicate was found. A non-NULL return means we found a duplicate.

> +    *lsp_hc = NULL;
> +
> +    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 (src_ip && strcmp(lsp_hc_p->src_ip, src_ip)) {
> +            continue;
> +        }
> +
> +        const char *target_proto =
> +            (proto == LSP_ICMP_HEALTH_CHECK) ? "icmp" :
> +            (proto == LSP_TCP_HEALTH_CHECK) ? "tcp" : "udp";
> +        if (strcmp(lsp_hc_p->protocol, target_proto)) {
> +            continue;
> +        }
> +
> +        if (proto != LSP_ICMP_HEALTH_CHECK && lsp_hc_p->port != port) {
> +            continue;
> +        }
> +
> +        if (!addresses || sset_is_empty(addresses)) {
> +            *lsp_hc = lsp_hc_p;
> +            return;
> +        }

This seems incorrect to me. Imagine that we have a logical switch port
with IP addresses A, B, and C. We start by adding a health check that
specifies target addresses A and B. Then we add a new health check
with no target addresses. With this code, we would return the existing
health check that only monitors addresses A and B. However, since the
new health check has no target addresses, the intent likely was for
the new health check to monitor addresses A, B, and C. This means that
address C will not be monitored.

> +
> +        bool addresses_found = true;
> +        for (size_t j = 0; j < lsp_hc_p->n_addresses; j++) {
> +            if (!sset_contains(addresses, lsp_hc_p->addresses[j])) {
> +                addresses_found = false;
> +                break;
> +            }
> +        }
> +
> +        if (addresses_found) {
> +            *lsp_hc = lsp_hc_p;
> +            return;
> +        }
> +    }
> +}
> +
> +static char **
> +_get_lsp_ip_addresses(char **lsp_addresses,
> +                      int lsp_n_addresses,
> +                      int *lsp_n_ip_addresses)
> +{

This function is unnecessary. You can just use extract_lsp_addresses()
in ovn-util to get the IP addresses from a logical switch port. It's
also safer to use, since skip_mac_address_from_lsp_address() has the
possibility to return a pointer past the end of the string if the LSP
MAC is malformed.

> +    char **lsp_ip_addresses_p = NULL;
> +    *lsp_n_ip_addresses = 0;
> +    size_t n_capacity = 0;
> +    size_t count = 0;
> +
> +    for (size_t i = 0; i < lsp_n_addresses; i++) {
> +        char *address_without_mac =
> +            skip_mac_address_from_lsp_address(lsp_addresses[i]);
> +
> +        if (address_without_mac && address_without_mac[0]) {
> +            if (count == n_capacity) {
> +                lsp_ip_addresses_p = x2nrealloc(lsp_ip_addresses_p,
> +                                                &n_capacity,
> +                                                sizeof *lsp_ip_addresses_p);
> +            }
> +            lsp_ip_addresses_p[count] = xstrdup(address_without_mac);
> +            count++;
> +        }
> +    }
> +
> +    *lsp_n_ip_addresses = count;
> +    return lsp_ip_addresses_p;
> +}
> +
> +static bool
> +_lsp_contains_ip_address(char **lsp_ip_addresses,
> +                         size_t lsp_n_ip_addresses,
> +                         char *ip_address)
> +{
> +    for (size_t i = 0; i < lsp_n_ip_addresses; i++) {
> +        if (!strcmp(lsp_ip_addresses[i], ip_address)) {
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +
> +static char * OVS_WARN_UNUSED_RESULT
> +find_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_add(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_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_addresses);
> +}
> +
> +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 *port = ctx->argv[1];
> +    const char *type = ctx->argv[2];
> +    const char *src_ip = ctx->argv[3];
> +    enum health_check_protocol protocol;
> +    char **lsp_ip_addresses = NULL;
> +    int lsp_n_ip_addresses = 0;
> +    char *validated_src_ip = NULL;
> +    struct sset target_ips;
> +    sset_init(&target_ips);
> +
> +    char *error;
> +    error = lsp_by_name_or_uuid(ctx, port, true, &lsp);
> +    if (error) {
> +        ctx->error = error;
> +        return;
> +    }
> +
> +    if (lsp->type[0]) {
> +        ctl_error(ctx, "%s: Health check monitoring supported only for"
> +                  " port with vif type", lsp->type);
> +        goto cleanup;
> +    }
> +
> +    if (!parse_health_check_protocol(ctx, type, &protocol)) {
> +       goto cleanup;
> +    }
> +
> +    validated_src_ip = normalize_addr_str(src_ip);
> +    if (!validated_src_ip) {
> +        ctl_error(ctx, "%s: Not a valid IPv4 or IPv6 address",
> +                  ctx->argv[3]);
> +        goto cleanup;
> +    }
> +
> +    int64_t destination_port = 0;
> +    if (protocol == LSP_TCP_HEALTH_CHECK ||
> +        protocol == LSP_UDP_HEALTH_CHECK) {
> +        if (ctx->argc < 5) {
> +            ctl_error(ctx, "Destination port required for %s health check",
> +                      type);
> +            goto cleanup;
> +        }
> +
> +        error = parse_l4_port_range(ctx->argv[4], &destination_port);
> +        if (error) {
> +            ctx->error = error;
> +            goto cleanup;
> +        }
> +    }
> +
> +    size_t target_ips_start_index;
> +    if (protocol == LSP_ICMP_HEALTH_CHECK) {
> +        target_ips_start_index = 4;
> +    } else {
> +        target_ips_start_index = 5;
> +    }
> +
> +    if (ctx->argc > target_ips_start_index) {
> +        lsp_ip_addresses = _get_lsp_ip_addresses(lsp->addresses,
> +                                                 lsp->n_addresses,
> +                                                 &lsp_n_ip_addresses);
> +
> +        for (int i = target_ips_start_index; i < ctx->argc; i++) {
> +            char *ip_addr = ctx->argv[i];
> +
> +            char *validated_ip = normalize_addr_str(ip_addr);
> +            if (!validated_ip) {
> +                free(validated_ip);
> +                ctl_error(ctx, "%s: Not a valid IPv4 or IPv6 address",
> +                          ip_addr);
> +                goto cleanup;
> +            }
> +
> +            if (!_lsp_contains_ip_address(lsp_ip_addresses,
> +                                          lsp_n_ip_addresses,
> +                                          ip_addr)) {
> +                free(validated_ip);
> +                ctl_error(ctx, "%s: Address %s not configured on port",
> +                          lsp->name, ip_addr);
> +                goto cleanup;
> +            }
> +
> +            sset_add(&target_ips, validated_ip);
> +            free(validated_ip);
> +        }
> +    }
> +
> +    lsp_health_check_get_duplicate(protocol,
> +                                   src_ip,
> +                                   destination_port,
> +                                   &target_ips,
> +                                   lsp,
> +                                   &lsp_hc);
> +
> +    if (lsp_hc) {
> +        ctl_error(ctx, "Health check already exists");
> +        goto cleanup;
> +    }
> +
> +    lsp_hc = nbrec_logical_switch_port_health_check_insert(ctx->txn);
> +    nbrec_logical_switch_port_health_check_set_protocol(lsp_hc, type);
> +    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);
> +
> +    if (ctx->argc > target_ips_start_index) {
> +        size_t num_target_ips = ctx->argc - target_ips_start_index;
> +         nbrec_logical_switch_port_health_check_set_addresses(
> +            lsp_hc, (const char **) ctx->argv + target_ips_start_index,
> +            num_target_ips);
> +    } else {
> +        nbrec_logical_switch_port_health_check_set_addresses(lsp_hc, NULL, 
> 0);
> +    }
> +
> +    nbrec_logical_switch_port_update_health_checks_addvalue(lsp, lsp_hc);
> +
> +cleanup:
> +    if (lsp_ip_addresses) {
> +        for (size_t i = 0; i < lsp_n_ip_addresses; i++) {
> +            free(lsp_ip_addresses[i]);
> +        }
> +        free(lsp_ip_addresses);
> +    }
> +    sset_destroy(&target_ips);
> +    free(validated_src_ip);
> +}
> +
> +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 (!lsp) {
> +        ctl_error(ctx, "Logical Switch Port with id %s not found",
> +                  port_id);
> +        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
> +cmp_lsp_hc(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 != 0) {
> +        return protocol_cmp;
> +    }
> +
> +    if (strcmp(lsp_hc_1->protocol, "icmp") != 0) {
> +        if (lsp_hc_1->port != lsp_hc_2->port) {
> +            return lsp_hc_1->port < lsp_hc_2->port ? -1 : 1;
> +        }
> +    }
> +
> +    if (lsp_hc_1->n_addresses != lsp_hc_2->n_addresses) {
> +        return lsp_hc_1->n_addresses < lsp_hc_2->n_addresses ? -1 : 1;
> +    }
> +
> +    size_t min_n_addresses = lsp_hc_1->n_addresses < lsp_hc_2->n_addresses ?
> +                             lsp_hc_1->n_addresses : lsp_hc_2->n_addresses;

min_n_addresses is unnecessary. The previous if statement already
ensured that lsp_hc_1->n_addresses must be equal to
lsp_hc_2->n_addresses.


> +
> +    for (size_t i = 0; i < min_n_addresses; i++) {
> +        int ip_cmp = strcmp(lsp_hc_1->addresses[i], lsp_hc_2->addresses[i]);
> +        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_addresses);
> +}
> +
> +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);
> +    for (size_t i = 0; i < lsp->n_health_checks; i++) {
> +        lsp_hcs[i] = lsp->health_checks[i];
> +    }
> +
> +    qsort(lsp_hcs, lsp->n_health_checks, sizeof *lsp_hcs, cmp_lsp_hc);
> +
> +    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);
> +        }
> +        if (hc->n_addresses) {
> +            ds_put_format(&ctx->output, "  Addresses     :  ");
> +            for (size_t j = 0; j < hc->n_addresses; j++) {
> +                if (j > 0) {
> +                    ds_put_format(&ctx->output, ", ");
> +                }
> +                ds_put_format(&ctx->output, "%s", hc->addresses[j]);
> +            }
> +            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");
> +        }
> +    }
> +}
> +
>  static const struct ctl_table_class tables[NBREC_N_TABLES] = {
>      [NBREC_TABLE_DHCP_OPTIONS].row_ids
>      = {{&nbrec_logical_switch_port_col_name, NULL,
> @@ -9064,6 +9566,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", 2, INT_MAX, "PORT TYPE SRC_IP [DST_PORT] [ADDRESS]",
> +     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},
>  };
>
> --
> 2.48.1
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to