On 2/11/26 4:45 PM, Dumitru Ceara wrote:
> On 2/7/26 10:48 PM, Alexandra Rukomoinikova wrote:
>> Add LSP service monitor type and handle ICMP health checks for
>> logical switch ports in addition to network functions.
>>
>> Signed-off-by: Alexandra Rukomoinikova <[email protected]>
>> ---
>> v1 --> v2: fixed comments
>> ---
>
> Hi Alexandra,
>
> Overall this looks OK, just some comments below.
>
Hi Alexandra,
I had missed a thing yesterday, please see below.
> I'll continue with the review of the rest of your v2 series tomorrow.
>
> Regards,
> Dumitru
>
>> controller/pinctrl.c | 63 ++++++++++++++++++----------
>> tests/system-ovn.at | 99 ++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 139 insertions(+), 23 deletions(-)
>>
>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
>> index 7baf6b488..7b80f4725 100644
>> --- a/controller/pinctrl.c
>> +++ b/controller/pinctrl.c
>> @@ -6912,6 +6912,8 @@ enum svc_monitor_type {
>> SVC_MON_TYPE_LB,
>> /* network function */
>> SVC_MON_TYPE_NF,
>> + /* logical switch port */
>> + SVC_MON_TYPE_LSP,
>> };
>>
>> /* Service monitor health checks. */
>> @@ -7039,6 +7041,9 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn,
>> if (sb_svc_mon->type && !strcmp(sb_svc_mon->type,
>> "network-function")) {
>> mon_type = SVC_MON_TYPE_NF;
>> + } else if (sb_svc_mon->type && !strcmp(sb_svc_mon->type,
>> + "logical-switch-port")) {
>
> Nit: this should be aligned under the strcmp's first argument.
>
>> + mon_type = SVC_MON_TYPE_LSP;
>> } else {
>> mon_type = SVC_MON_TYPE_LB;
>> }
>> @@ -7103,11 +7108,6 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn,
>> }
>> }
>> } else {
>> - if (protocol != SVC_MON_PROTO_TCP &&
>> - protocol != SVC_MON_PROTO_UDP) {
>> - continue;
>> - }
>> -
>> for (size_t i = 0; i < pb->n_mac && !mac_found; i++) {
>> struct lport_addresses laddrs;
>>
>> @@ -8066,6 +8066,7 @@ static void
>> svc_monitor_send_icmp_health_check__(struct rconn *swconn,
>> struct svc_monitor *svc_mon)
>> {
>> + bool svc_mon_nf = (svc_mon->type == SVC_MON_TYPE_NF) ? true : false;
>
> Nit: this can be:
>
> bool svc_mon_nf = svc_mon->type == SVC_MON_TYPE_NF;
>
>> uint64_t packet_stub[128 / 8];
>> struct dp_packet packet;
>> dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
>> @@ -8112,7 +8113,8 @@ svc_monitor_send_icmp_health_check__(struct rconn
>> *swconn,
>> struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub);
>> enum ofp_version version = rconn_get_version(swconn);
>> put_load(svc_mon->dp_key, MFF_LOG_DATAPATH, 0, 64, &ofpacts);
>> - put_load(svc_mon->input_port_key, MFF_LOG_OUTPORT, 0, 32, &ofpacts);
>> + put_load(svc_mon_nf ? svc_mon->input_port_key : svc_mon->port_key,
>> + MFF_LOG_OUTPORT, 0, 32, &ofpacts);
>> put_load(1, MFF_LOG_FLAGS, MLF_LOCAL_ONLY, 1, &ofpacts);
>> struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&ofpacts);
>> resubmit->in_port = OFPP_CONTROLLER;
>> @@ -8406,24 +8408,32 @@ pinctrl_handle_svc_check(struct rconn *swconn, const
>> struct flow *ip_flow,
>> return;
>> }
>>
>> - /* Handle ICMP ECHO REQUEST probes for Network Function services */
>> + /* Handle ICMP ECHO REQUEST probes for Network Function and
>> + * Logical Switch Port services */
>> if (in_eth->eth_type == htons(ETH_TYPE_IP)) {
>> struct icmp_header *ih = l4h;
>> /* It's ICMP packet. */
>> - if (ih->icmp_type == ICMP4_ECHO_REQUEST && ih->icmp_code == 0) {
>> - uint32_t hash = hash_bytes(&dst_ip_addr, sizeof dst_ip_addr,
>> - hash_3words(dp_key, port_key,
>> 0));
>> - struct svc_monitor *svc_mon =
>> - pinctrl_find_svc_monitor(dp_key, port_key,
>> &dst_ip_addr, 0,
>> + if ((ih->icmp_type == ICMP4_ECHO_REQUEST ||
>> + ih->icmp_type == ICMP4_ECHO_REPLY) && ih->icmp_code == 0) {
>> + int is_echo_request = (ih->icmp_type == ICMP4_ECHO_REQUEST);
>
> Nit: no need for the parenthesis.
>
>> + struct in6_addr *target_addr = is_echo_request
>> + ? &dst_ip_addr : &ip_addr;
>> + uint32_t hash =
>> + hash_bytes(target_addr, sizeof(*target_addr),
>> + hash_3words(dp_key, port_key, 0));
>> + struct svc_monitor *svc_mon =
>
> Nit: one space too many before 'struct'.
>
>> + pinctrl_find_svc_monitor(dp_key, port_key, target_addr,
>> 0,
>> SVC_MON_PROTO_ICMP, hash);
>> if (!svc_mon) {
>> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(
>> - 1, 5);
>> + static struct vlog_rate_limit rl =
>> + VLOG_RATE_LIMIT_INIT(1, 5);
>> VLOG_WARN_RL(&rl, "handle service check: Service
>> monitor "
>> - "not found for ICMP request");
>> + "not found for ICMP %s", is_echo_request ?
>> + "request" : "reply");
>> return;
>> }
>> - if (svc_mon->type == SVC_MON_TYPE_NF) {
>> + if (svc_mon->type == SVC_MON_TYPE_NF ||
>> + svc_mon->type == SVC_MON_TYPE_LSP) {
>
> Does this check make sense? Why make it match on type NF or type LSP
> when all we care about is that this is a monitor for ICMP protocol?
>
> And we already lookup based on protocol above so I'd just skip this
> check all together and call pinctrl_handle_icmp_svc_check() directly.
>
>> pinctrl_handle_icmp_svc_check(pkt_in, svc_mon);
>> }
>> return;
>> @@ -8431,21 +8441,28 @@ pinctrl_handle_svc_check(struct rconn *swconn, const
>> struct flow *ip_flow,
>> } else if (in_eth->eth_type == htons(ETH_TYPE_IPV6)) {
>> struct icmp6_data_header *ih6 = l4h;
>> /* It's ICMPv6 packet. */
>> - if (ih6->icmp6_base.icmp6_type == ICMP6_ECHO_REQUEST &&
>> + if ((ih6->icmp6_base.icmp6_type == ICMP6_ECHO_REQUEST ||
>> + ih6->icmp6_base.icmp6_type == ICMP6_ECHO_REPLY) &&
>> ih6->icmp6_base.icmp6_code == 0) {
>> - uint32_t hash = hash_bytes(&dst_ip_addr, sizeof dst_ip_addr,
>> + int is_echo_request =
>> + (ih6->icmp6_base.icmp6_type == ICMP6_ECHO_REQUEST);
>> + struct in6_addr *target_addr = is_echo_request
>> + ? &dst_ip_addr : &ip_addr;
>> + uint32_t hash = hash_bytes(target_addr,
>> sizeof(*target_addr),
>> hash_3words(dp_key, port_key,
>> 0));
>> struct svc_monitor *svc_mon =
>> - pinctrl_find_svc_monitor(dp_key, port_key,
>> &dst_ip_addr, 0,
>> + pinctrl_find_svc_monitor(dp_key, port_key, target_addr,
>> 0,
>> SVC_MON_PROTO_ICMP, hash);
>> if (!svc_mon) {
>> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(
>> - 1, 5);
>> + static struct vlog_rate_limit rl =
>> + VLOG_RATE_LIMIT_INIT(1, 5);
>> VLOG_WARN_RL(&rl, "handle service check: Service
>> monitor "
>> - "not found for ICMPv6 request");
>> + "not found for ICMPv6 %s", is_echo_request
>> ?
>> + "request" : "reply");
>> return;
>> }
>> - if (svc_mon->type == SVC_MON_TYPE_NF) {
>> + if (svc_mon->type == SVC_MON_TYPE_NF ||
>> + svc_mon->type == SVC_MON_TYPE_LSP) {
>
> Same comment as for IPv4.
>
>> pinctrl_handle_icmp_svc_check(pkt_in, svc_mon);
>> }
>> return;
>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
>> index 82b7e7db5..14c0996a0 100644
>> --- a/tests/system-ovn.at
>> +++ b/tests/system-ovn.at
>> @@ -20993,3 +20993,102 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port
>> patch-.*/d
>> /connection dropped.*/d"])
>> AT_CLEANUP
>> ])
>> +
>> +OVN_FOR_EACH_NORTHD_NO_HV([
>> +AT_SETUP([Logical Switch Port Health Check])
>> +AT_SKIP_IF([test $HAVE_NC = no])
>> +
>> +ovn_start
>> +OVS_TRAFFIC_VSWITCHD_START()
>> +ADD_BR([br-int])
>> +
>> +# Set external-ids in br-int needed for ovn-controller
>> +ovs-vsctl \
>> + -- set Open_vSwitch . external-ids:system-id=hv1 \
>> + -- set Open_vSwitch .
>> external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
>> + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
>> + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
>> + -- set bridge br-int fail-mode=secure
>> other-config:disable-in-band=true
>> +
>> +# Start ovn-controller
>> +start_daemon ovn-controller
>> +
>> +check ovn-nbctl ls-add ls1
>> +
>> +ADD_NAMESPACES(lport)
>> +ADD_VETH(lport, lport, br-int, "192.168.0.10", "f0:00:0f:01:02:04", \
>> + "192.168.0.1")
>> +NS_EXEC([lport], [ip r del default via 192.168.0.1 dev lport])
>
> Don't you achieve the same thing if you just don't pass any 6th argument
> to ADD_VETH() above?
>
>> +NS_EXEC([lport], [ip r add default dev lport])
>> +NS_EXEC([lport], [ip addr add 2001:db8::10/64 dev lport])
>> +NS_EXEC([lport], [ip -6 r add default dev lport])
>> +
>> +check ovn-nbctl lsp-add ls1 lport -- \
>> + lsp-set-addresses lport "f0:00:0f:01:02:04 192.168.0.10 2001:db8::10"
>> +
>> +check ovn-nbctl --wait=hv sync
>> +
>> +check ovn-nbctl lsp-hc-add lport icmp 192.168.0.255 192.168.0.10
>> +check_row_count sb:Service_Monitor 1
>> +
>> +# Wait until the services are set to online.
>> +wait_row_count Service_Monitor 1 status=online
>> +
>> +check ovn-nbctl lsp-hc-add lport tcp 192.168.0.255 4041 192.168.0.10
>> +
>> +check_row_count sb:Service_Monitor 2
>> +
>> +# Create one more health check on logical switch port
>> +NETNS_DAEMONIZE([lport], [nc -l -k 192.168.0.10 4041], [lport_tcp.pid])
>> +
>> +# Wait until the services are set to online.
>> +wait_row_count Service_Monitor 2 status=online
>> +
>> +check ovn-nbctl lsp-hc-add lport udp 192.168.0.255 4042 192.168.0.10
>> +
>> +NETNS_DAEMONIZE([lport], [nc -ulp 4042], [lport_udp.pid])
>> +
>> +# Wait until the services are set to online.
>> +wait_row_count Service_Monitor 3 status=online
>> +
>> +check ovn-nbctl lsp-hc-del lport
>> +
>> +# IPv6 ICMP health check (ping6)
>> +check ovn-nbctl lsp-hc-add lport icmp 2001:db8::ff 2001:db8::10
>> +check_row_count sb:Service_Monitor 1
>> +
>> +# Wait until the services are set to online.
>> +wait_row_count Service_Monitor 1 status=online
>> +
>> +# IPv6 TCP health check
>> +check ovn-nbctl lsp-hc-add lport tcp 2001:db8::ff 4043 2001:db8::10
>> +
>> +check_row_count sb:Service_Monitor 2
>> +
>> +# Start IPv6 TCP server
>> +NETNS_DAEMONIZE([lport], [nc -6 -l -k 2001:db8::10 4043],
>> [lport_ipv6_tcp.pid])
>> +
>> +# Wait until the services are set to online.
>> +wait_row_count Service_Monitor 2 status=online
>> +
>> +# IPv6 UDP health check
>> +check ovn-nbctl lsp-hc-add lport udp 2001:db8::ff 4044 2001:db8::10
>> +check_row_count sb:Service_Monitor 3
>> +
>> +# Start IPv6 UDP server
>> +NETNS_DAEMONIZE([lport], [nc -6 -u -l 2001:db8::10 4044],
>> [lport_ipv6_udp.pid])
>> +
>> +# Wait until the services are set to online.
>> +wait_row_count Service_Monitor 3 status=online
>> +
>> +killall nc 2>/dev/null || true
This shouldn't be needed because we start the nc servers with
NETNS_DAEMONIZE. It also is not great to run "killall" as it might
affect other nc instances not started by the test.
>> +
>> +OVN_CLEANUP_CONTROLLER([hv1])
>> +
>> +OVN_CLEANUP_NORTHD
>> +
>> +as
>> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
>> +/connection dropped.*/d"])
>> +AT_CLEANUP
>> +])
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
