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.
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
> +
> +OVN_CLEANUP_CONTROLLER([hv1])
> +
> +OVN_CLEANUP_NORTHD
> +
> +as
> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> +/connection dropped.*/d"])
> +AT_CLEANUP
> +])
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
