| Message ID | 20251219094839.78689-4-arukomoinikova@k2.cloud |
|---|---|
| State | Changes Requested |
| Delegated to: | Dumitru Ceara |
| Headers | show |
| Series | [ovs-dev,1/6] ovn-nb, ovn-nbctl: Add LSP Health Check schema support. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | fail | apply and check: fail |
Bleep bloop. Greetings Alexandra Rukomoinikova, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: error: Failed to merge in the changes. hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0001 controller: Add LSP health check handling with ICMP probes. When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
Hi Alexandra, I have some notes below. On Fri, Dec 19, 2025 at 4:49 AM Alexandra Rukomoinikova <arukomoinikova@k2.cloud> wrote: > > * Add service monitor type validation during creation to prevent > invalid types and avoid subsequent type checks in runtime processing. > * Implement ICMP probe handling for logical switch ports health checks. > * Add unit tests for the new functionality. > > Signed-off-by: Alexandra Rukomoinikova <arukomoinikova@k2.cloud> > --- > controller/pinctrl.c | 90 ++++++++++++++++++----------- > tests/ovn-northd.at | 132 +++++++++++++++++++++++++++++++++++++++++++ > tests/system-ovn.at | 68 ++++++++++++++++++++++ > 3 files changed, 257 insertions(+), 33 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 3ccacfa2d..c788acb16 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -6856,6 +6856,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. */ > @@ -6980,20 +6982,26 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn, > const struct sbrec_service_monitor *sb_svc_mon; > SBREC_SERVICE_MONITOR_TABLE_FOR_EACH (sb_svc_mon, svc_mon_table) { > enum svc_monitor_type mon_type; > - if (sb_svc_mon->type && !strcmp(sb_svc_mon->type, > - "network-function")) { > + enum svc_monitor_protocol protocol; Like with the previous patch, moving this declaration earlier in the function hinders readability. > + > + 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")) { > + mon_type = SVC_MON_TYPE_LSP; > } else { > mon_type = SVC_MON_TYPE_LB; > } > > - enum svc_monitor_protocol protocol; > if (!strcmp(sb_svc_mon->protocol, "udp")) { > - protocol = SVC_MON_PROTO_UDP; > + protocol = (mon_type == SVC_MON_TYPE_NF) ? > + SVC_MON_PROTO_ICMP : SVC_MON_PROTO_UDP; This doesn't seem right to me. This is now coercing NF service monitors that are configured as type UDP to be type ICMP. Currently, northd creates all NF service monitors to use "icmp" as the protocol, so it's actually impossible to trigger this condition. However, if NF service monitors were to expand to allow other protocols, the existing code would work as written. I think this change should be removed. > } else if (!strcmp(sb_svc_mon->protocol, "icmp")) { > protocol = SVC_MON_PROTO_ICMP; > } else { > - protocol = SVC_MON_PROTO_TCP; > + protocol = (mon_type == SVC_MON_TYPE_NF) ? > + SVC_MON_PROTO_ICMP : SVC_MON_PROTO_TCP; I have the same note here as I did in the UDP case. > } > > const struct sbrec_port_binding *pb > @@ -7030,9 +7038,6 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn, > bool mac_found = false; > > if (mon_type == SVC_MON_TYPE_NF) { > - if (protocol != SVC_MON_PROTO_ICMP) { > - continue; > - } And this should be restored. If NF service monitors get expanded to allow other protocols than ICMP, then we can remove this line. > input_pb = lport_lookup_by_name(sbrec_port_binding_by_name, > sb_svc_mon->logical_input_port); > if (!input_pb) { > @@ -7047,11 +7052,6 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn, > } > } > } else { > - if (protocol != SVC_MON_PROTO_TCP && > - protocol != SVC_MON_PROTO_UDP) { > - continue; > - } This change needs to be made differently. I think the idea you had was that LSP health checks do not have to be restricted to TCP and UDP, so we should remove this check. However, LB health checks *are* restricted to TCP and UDP. By removing this if statement, it allows for non-TCP and non-UDP LB health checks to be attempted by pinctrl. > - > for (size_t i = 0; i < pb->n_mac && !mac_found; i++) { > struct lport_addresses laddrs; > > @@ -8010,6 +8010,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; > uint64_t packet_stub[128 / 8]; > struct dp_packet packet; > dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub); > @@ -8056,7 +8057,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; > @@ -8338,6 +8340,7 @@ pinctrl_handle_svc_check(struct rconn *swconn, const struct flow *ip_flow, > "not found"); > return; > } > + > pinctrl_handle_tcp_svc_check(swconn, pkt_in, svc_mon); > } else { > const char *end = > @@ -8350,48 +8353,69 @@ 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); > + 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, 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"); > return; > } > - if (svc_mon->type == SVC_MON_TYPE_NF) { > - pinctrl_handle_icmp_svc_check(pkt_in, svc_mon); > - } > + > + /* Type validation done during creation - > + * asserts on unsupported types. */ > + ovs_assert(svc_mon->type != SVC_MON_TYPE_NF || > + svc_mon->type != SVC_MON_TYPE_LSP); This assertion is impossible to fail. I think what you meant to do was: ovs_assert(svc_mon->type == SVC_MON_TYPE_NF || svc_mon->type == SVC_MON_TYPE_LSP); > + > + pinctrl_handle_icmp_svc_check(pkt_in, svc_mon); > + > return; > } > } 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"); > return; > } > - if (svc_mon->type == SVC_MON_TYPE_NF) { > - pinctrl_handle_icmp_svc_check(pkt_in, svc_mon); > - } > + > + /* Type validation done during creation > + * - asserts on unsupported types. */ > + ovs_assert(svc_mon->type != SVC_MON_TYPE_NF || > + svc_mon->type != SVC_MON_TYPE_LSP); I have the same note on this assertion. > + > + pinctrl_handle_icmp_svc_check(pkt_in, svc_mon); > + > return; > } > } > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index d425cfb7a..9225f0646 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -19030,3 +19030,135 @@ check ovn-nbctl --wait=sb sync > OVN_CLEANUP_NORTHD > AT_CLEANUP > ]) > + > +AT_SETUP([Logical Switch Port Health Check - lflow/service monitor synchronization]) > +ovn_start > + > +check ovn-nbctl ls-add ls1 > +check ovn-nbctl lsp-add ls1 lport1 -- \ > + lsp-set-addresses lport1 "f0:00:0f:01:02:04 192.168.0.10" "f0:00:0f:01:02:06 192.168.0.11" > + > +check ovn-nbctl lsp-add ls1 lport2 -- \ > + lsp-set-addresses lport2 "f0:00:0f:01:02:08 192.168.0.12" > + > +check ovn-nbctl set NB_Global . options:svc_monitor_mac="11:11:11:11:11:11" > + > +# Create service monitor for all lsp addresses. > +check ovn-nbctl lsp-hc-add lport1 icmp 192.168.0.255 > +check_row_count nb:Logical_Switch_Port_Health_Check 1 > +lport1_uuid1=$(fetch_column nb:Logical_Switch_Port_Health_Check _uuid protocol=icmp) > + > +# Check lflow and service monitor synchronization > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep 'ls_in_arp_rsp'| grep "priority=110" | ovn_strip_lflows], [0], [dnl > + table=??(ls_in_arp_rsp ), priority=110 , match=(arp.tpa == 192.168.0.255 && arp.op == 1), action=(eth.dst = eth.src; eth.src = 11:11:11:11:11:11; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 11:11:11:11:11:11; arp.tpa = arp.spa; arp.spa = 192.168.0.255; outport = inport; flags.loopback = 1; output;) > +]) > + > +check_row_count sb:Service_Monitor 2 > +check_column "false false" sb:Service_Monitor ic_learned logical_port=lport1 > +check_column "false false" sb:Service_Monitor remote logical_port=lport1 > +check_column "192.168.0.10 192.168.0.11" sb:Service_Monitor ip logical_port=lport1 > +check_column "icmp icmp" sb:Service_Monitor protocol logical_port=lport1 > +check_column "192.168.0.255 192.168.0.255" sb:Service_Monitor src_ip logical_port=lport1 > +check_column "0 0" sb:Service_Monitor port logical_port=lport1 > +check_column "logical-switch-port logical-switch-port" sb:Service_Monitor type logical_port=lport1 > +check_column "11:11:11:11:11:11 11:11:11:11:11:11" sb:Service_Monitor src_mac logical_port=lport1 > + > +# Create one more service monitor for all lsp addresses. > +check ovn-nbctl lsp-hc-add lport1 tcp 192.168.0.254 80 > +check_row_count nb:Logical_Switch_Port_Health_Check 2 > +lport1_uuid2=$(fetch_column nb:Logical_Switch_Port_Health_Check _uuid protocol=tcp) > + > +# Check that 2 records (2 addresses) have been created for each protocol. > +check_row_count sb:Service_Monitor 4 > + > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep 'ls_in_arp_rsp'| grep "priority=110" | ovn_strip_lflows], [0], [dnl > + table=??(ls_in_arp_rsp ), priority=110 , match=(arp.tpa == 192.168.0.254 && arp.op == 1), action=(eth.dst = eth.src; eth.src = 11:11:11:11:11:11; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 11:11:11:11:11:11; arp.tpa = arp.spa; arp.spa = 192.168.0.254; outport = inport; flags.loopback = 1; output;) > + table=??(ls_in_arp_rsp ), priority=110 , match=(arp.tpa == 192.168.0.255 && arp.op == 1), action=(eth.dst = eth.src; eth.src = 11:11:11:11:11:11; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 11:11:11:11:11:11; arp.tpa = arp.spa; arp.spa = 192.168.0.255; outport = inport; flags.loopback = 1; output;) > +]) > + > +# Change addresses on lport - addresses for service monitors should be changed > +check ovn-nbctl lsp-set-addresses lport1 "f0:00:0f:01:02:04 192.168.0.20" > + > +check_row_count sb:Service_Monitor 2 > +check_column "false false" sb:Service_Monitor ic_learned logical_port=lport1 > +check_column "false false " sb:Service_Monitor remote logical_port=lport1 > +check_column "192.168.0.20 192.168.0.20" sb:Service_Monitor ip logical_port=lport1 > +check_column "icmp tcp" sb:Service_Monitor protocol logical_port=lport1 > +check_column "192.168.0.255 192.168.0.254" sb:Service_Monitor src_ip logical_port=lport1 > +check_column "0 80" sb:Service_Monitor port logical_port=lport1 > +check_column "logical-switch-port logical-switch-port" sb:Service_Monitor type logical_port=lport1 > +check_column "11:11:11:11:11:11 11:11:11:11:11:11" sb:Service_Monitor src_mac logical_port=lport1 > + > +# Check options propogations > +hc_lport1_uuid=$(fetch_column nb:logical_switch_port_health_check _uuid src_ip="192.168.0.255") > + > +check ovn-nbctl set logical_switch_port_health_check $hc_lport1_uuid options:interval=3 > +check ovn-nbctl set logical_switch_port_health_check $hc_lport1_uuid options:timeout=30 > +check ovn-nbctl set logical_switch_port_health_check $hc_lport1_uuid options:success_count=1 > +check ovn-nbctl set logical_switch_port_health_check $hc_lport1_uuid options:failure_count=2 > + > + > +sm_lport1_uuid=$(fetch_column sb:service_monitor _uuid protocol=icmp) > + > +AT_CHECK([ovn-sbctl get Service_Monitor $sm_lport1_uuid options:interval], > +[0], ["3" > +]) > +AT_CHECK([ovn-sbctl get Service_Monitor $sm_lport1_uuid options:failure_count], > +[0], ["2" > +]) > +AT_CHECK([ovn-sbctl get Service_Monitor $sm_lport1_uuid options:success_count], > +[0], ["1" > +]) > +AT_CHECK([ovn-sbctl get Service_Monitor $sm_lport1_uuid options:timeout], > +[0], ["30" > +]) > + > +ovn-nbctl list logical_switch_port > + > +check ovn-nbctl lsp-hc-del lport1 > + > +check_row_count nb:Logical_Switch_Port_Health_Check 0 > + > +# Change the service monitor to monitor only one address. > +check ovn-nbctl lsp-hc-add lport1 icmp 192.168.0.255 192.168.0.20 > +check_row_count nb:Logical_Switch_Port_Health_Check 1 > +lport1_uuid3=$(fetch_column nb:Logical_Switch_Port_Health_Check _uuid protocol=icmp) > + > +check_row_count sb:Service_Monitor 1 > + > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep 'ls_in_arp_rsp'| grep "priority=110" | ovn_strip_lflows], [0], [dnl > + table=??(ls_in_arp_rsp ), priority=110 , match=(arp.tpa == 192.168.0.255 && arp.op == 1), action=(eth.dst = eth.src; eth.src = 11:11:11:11:11:11; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 11:11:11:11:11:11; arp.tpa = arp.spa; arp.spa = 192.168.0.255; outport = inport; flags.loopback = 1; output;) > +]) > + > +check_column "false" sb:Service_Monitor ic_learned logical_port=lport1 > +check_column "false" sb:Service_Monitor remote logical_port=lport1 > +check_column "192.168.0.20" sb:Service_Monitor ip logical_port=lport1 > +check_column "icmp" sb:Service_Monitor protocol logical_port=lport1 > +check_column "192.168.0.255" sb:Service_Monitor src_ip logical_port=lport1 > +check_column "0" sb:Service_Monitor port logical_port=lport1 > +check_column "logical-switch-port" sb:Service_Monitor type logical_port=lport1 > +check_column "11:11:11:11:11:11" sb:Service_Monitor src_mac logical_port=lport1 > + > +# Create one more monitor > +check ovn-nbctl lsp-hc-add lport2 icmp 192.168.0.255 192.168.0.12 > +lport1_uuid4=$(fetch_column nb:Logical_Switch_Port_Health_Check _uuid addresses="192.168.0.12") > +check_row_count nb:Logical_Switch_Port_Health_Check 2 > + > +check_row_count sb:Service_Monitor 2 > + > +# One record for arp replay > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep 'ls_in_arp_rsp'| grep "priority=110" | ovn_strip_lflows], [0], [dnl > + table=??(ls_in_arp_rsp ), priority=110 , match=(arp.tpa == 192.168.0.255 && arp.op == 1), action=(eth.dst = eth.src; eth.src = 11:11:11:11:11:11; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 11:11:11:11:11:11; arp.tpa = arp.spa; arp.spa = 192.168.0.255; outport = inport; flags.loopback = 1; output;) > +]) > + > +ovn-nbctl list logical_switch_port_health_check > + > +check ovn-nbctl lsp-hc-del lport1 > +check ovn-nbctl lsp-hc-del lport2 > +check_row_count nb:Logical_Switch_Port_Health_Check 0 > +check_row_count sb:Service_Monitor 0 > + > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep 'ls_in_arp_rsp'| grep "priority=110" | ovn_strip_lflows], [0], []) > + > +AT_CLEANUP > +]) > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 303b10894..15b7491bb 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -19046,3 +19046,71 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > /Couldn't parse IPv6 prefix nexthop.*/d"]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([Logical Switch Port Health Check]) > + > +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]) > +NS_EXEC([lport], [ip 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" > + > +check ovn-nbctl --wait=hv sync > + > +check ovn-nbctl lsp-hc-add lport icmp 192.168.0.255 > +lport1_uuid1=$(fetch_column nb:Logical_Switch_Port_Health_Check _uuid protocol=icmp) > + > +check_row_count sb:Service_Monitor 1 > + > +NETNS_START_TCPDUMP([lport], [-n -c 2 -i lport], [lport]) > +OVS_WAIT_UNTIL([ > + total_queries=`grep "ICMP" -c lport.tcpdump` > + test "${total_queries}" = "2" > +]) > + > +# Wait until the services are set to online. > +wait_row_count Service_Monitor 1 status=online > + > +# Create one more health check on logical switch port > +NETNS_DAEMONIZE([lport], [nc -l -k 192.168.0.10 4041], [lport_tcp.pid]) > + > +check ovn-nbctl lsp-hc-add lport tcp 192.168.0.255 4041 > +lport1_uuid2=$(fetch_column nb:Logical_Switch_Port_Health_Check _uuid protocol=tcp) > + > +check_row_count sb:Service_Monitor 2 > + > +# Wait until the services are set to online. > +wait_row_count Service_Monitor 2 status=online > + > +NETNS_DAEMONIZE([lport], [nc -ulp 4042], [lport_udp.pid]) > + > +check ovn-nbctl lsp-hc-add lport udp 192.168.0.255 4042 > +lport1_uuid3=$(fetch_column nb:Logical_Switch_Port_Health_Check _uuid protocol=udp) > + > +check_row_count sb:Service_Monitor 3 > +# Wait until the services are set to online. > +wait_row_count Service_Monitor 3 status=online > + > +AT_CLEANUP > +]) > -- > 2.48.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 1/21/26 8:31 PM, Mark Michelson via dev wrote: > Hi Alexandra, I have some notes below. > > On Fri, Dec 19, 2025 at 4:49 AM Alexandra Rukomoinikova > <arukomoinikova@k2.cloud> wrote: >> >> * Add service monitor type validation during creation to prevent >> invalid types and avoid subsequent type checks in runtime processing. >> * Implement ICMP probe handling for logical switch ports health checks. >> * Add unit tests for the new functionality. >> >> Signed-off-by: Alexandra Rukomoinikova <arukomoinikova@k2.cloud> >> --- Hi Alexandra, Mark, Thanks for the patch! On top of Mark's comments I have a few of my own. [...] >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> index d425cfb7a..9225f0646 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -19030,3 +19030,135 @@ check ovn-nbctl --wait=sb sync >> OVN_CLEANUP_NORTHD >> AT_CLEANUP >> ]) >> + >> +AT_SETUP([Logical Switch Port Health Check - lflow/service monitor synchronization]) >> +ovn_start >> + >> +check ovn-nbctl ls-add ls1 >> +check ovn-nbctl lsp-add ls1 lport1 -- \ >> + lsp-set-addresses lport1 "f0:00:0f:01:02:04 192.168.0.10" "f0:00:0f:01:02:06 192.168.0.11" >> + >> +check ovn-nbctl lsp-add ls1 lport2 -- \ >> + lsp-set-addresses lport2 "f0:00:0f:01:02:08 192.168.0.12" >> + >> +check ovn-nbctl set NB_Global . options:svc_monitor_mac="11:11:11:11:11:11" >> + >> +# Create service monitor for all lsp addresses. >> +check ovn-nbctl lsp-hc-add lport1 icmp 192.168.0.255 >> +check_row_count nb:Logical_Switch_Port_Health_Check 1 >> +lport1_uuid1=$(fetch_column nb:Logical_Switch_Port_Health_Check _uuid protocol=icmp) >> + >> +# Check lflow and service monitor synchronization >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep 'ls_in_arp_rsp'| grep "priority=110" | ovn_strip_lflows], [0], [dnl >> + table=??(ls_in_arp_rsp ), priority=110 , match=(arp.tpa == 192.168.0.255 && arp.op == 1), action=(eth.dst = eth.src; eth.src = 11:11:11:11:11:11; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 11:11:11:11:11:11; arp.tpa = arp.spa; arp.spa = 192.168.0.255; outport = inport; flags.loopback = 1; output;) >> +]) >> + >> +check_row_count sb:Service_Monitor 2 >> +check_column "false false" sb:Service_Monitor ic_learned logical_port=lport1 >> +check_column "false false" sb:Service_Monitor remote logical_port=lport1 >> +check_column "192.168.0.10 192.168.0.11" sb:Service_Monitor ip logical_port=lport1 >> +check_column "icmp icmp" sb:Service_Monitor protocol logical_port=lport1 >> +check_column "192.168.0.255 192.168.0.255" sb:Service_Monitor src_ip logical_port=lport1 >> +check_column "0 0" sb:Service_Monitor port logical_port=lport1 >> +check_column "logical-switch-port logical-switch-port" sb:Service_Monitor type logical_port=lport1 >> +check_column "11:11:11:11:11:11 11:11:11:11:11:11" sb:Service_Monitor src_mac logical_port=lport1 >> + >> +# Create one more service monitor for all lsp addresses. >> +check ovn-nbctl lsp-hc-add lport1 tcp 192.168.0.254 80 >> +check_row_count nb:Logical_Switch_Port_Health_Check 2 >> +lport1_uuid2=$(fetch_column nb:Logical_Switch_Port_Health_Check _uuid protocol=tcp) >> + >> +# Check that 2 records (2 addresses) have been created for each protocol. >> +check_row_count sb:Service_Monitor 4 >> + >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep 'ls_in_arp_rsp'| grep "priority=110" | ovn_strip_lflows], [0], [dnl >> + table=??(ls_in_arp_rsp ), priority=110 , match=(arp.tpa == 192.168.0.254 && arp.op == 1), action=(eth.dst = eth.src; eth.src = 11:11:11:11:11:11; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 11:11:11:11:11:11; arp.tpa = arp.spa; arp.spa = 192.168.0.254; outport = inport; flags.loopback = 1; output;) >> + table=??(ls_in_arp_rsp ), priority=110 , match=(arp.tpa == 192.168.0.255 && arp.op == 1), action=(eth.dst = eth.src; eth.src = 11:11:11:11:11:11; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 11:11:11:11:11:11; arp.tpa = arp.spa; arp.spa = 192.168.0.255; outport = inport; flags.loopback = 1; output;) >> +]) >> + >> +# Change addresses on lport - addresses for service monitors should be changed >> +check ovn-nbctl lsp-set-addresses lport1 "f0:00:0f:01:02:04 192.168.0.20" >> + >> +check_row_count sb:Service_Monitor 2 >> +check_column "false false" sb:Service_Monitor ic_learned logical_port=lport1 >> +check_column "false false " sb:Service_Monitor remote logical_port=lport1 >> +check_column "192.168.0.20 192.168.0.20" sb:Service_Monitor ip logical_port=lport1 >> +check_column "icmp tcp" sb:Service_Monitor protocol logical_port=lport1 >> +check_column "192.168.0.255 192.168.0.254" sb:Service_Monitor src_ip logical_port=lport1 >> +check_column "0 80" sb:Service_Monitor port logical_port=lport1 >> +check_column "logical-switch-port logical-switch-port" sb:Service_Monitor type logical_port=lport1 >> +check_column "11:11:11:11:11:11 11:11:11:11:11:11" sb:Service_Monitor src_mac logical_port=lport1 >> + >> +# Check options propogations >> +hc_lport1_uuid=$(fetch_column nb:logical_switch_port_health_check _uuid src_ip="192.168.0.255") >> + >> +check ovn-nbctl set logical_switch_port_health_check $hc_lport1_uuid options:interval=3 >> +check ovn-nbctl set logical_switch_port_health_check $hc_lport1_uuid options:timeout=30 >> +check ovn-nbctl set logical_switch_port_health_check $hc_lport1_uuid options:success_count=1 >> +check ovn-nbctl set logical_switch_port_health_check $hc_lport1_uuid options:failure_count=2 >> + >> + >> +sm_lport1_uuid=$(fetch_column sb:service_monitor _uuid protocol=icmp) >> + >> +AT_CHECK([ovn-sbctl get Service_Monitor $sm_lport1_uuid options:interval], >> +[0], ["3" >> +]) >> +AT_CHECK([ovn-sbctl get Service_Monitor $sm_lport1_uuid options:failure_count], >> +[0], ["2" >> +]) >> +AT_CHECK([ovn-sbctl get Service_Monitor $sm_lport1_uuid options:success_count], >> +[0], ["1" >> +]) >> +AT_CHECK([ovn-sbctl get Service_Monitor $sm_lport1_uuid options:timeout], >> +[0], ["30" >> +]) >> + >> +ovn-nbctl list logical_switch_port Missing check? Or should we just remove this one? >> + >> +check ovn-nbctl lsp-hc-del lport1 >> + >> +check_row_count nb:Logical_Switch_Port_Health_Check 0 >> + >> +# Change the service monitor to monitor only one address. >> +check ovn-nbctl lsp-hc-add lport1 icmp 192.168.0.255 192.168.0.20 >> +check_row_count nb:Logical_Switch_Port_Health_Check 1 >> +lport1_uuid3=$(fetch_column nb:Logical_Switch_Port_Health_Check _uuid protocol=icmp) >> + >> +check_row_count sb:Service_Monitor 1 >> + >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep 'ls_in_arp_rsp'| grep "priority=110" | ovn_strip_lflows], [0], [dnl >> + table=??(ls_in_arp_rsp ), priority=110 , match=(arp.tpa == 192.168.0.255 && arp.op == 1), action=(eth.dst = eth.src; eth.src = 11:11:11:11:11:11; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 11:11:11:11:11:11; arp.tpa = arp.spa; arp.spa = 192.168.0.255; outport = inport; flags.loopback = 1; output;) >> +]) >> + >> +check_column "false" sb:Service_Monitor ic_learned logical_port=lport1 >> +check_column "false" sb:Service_Monitor remote logical_port=lport1 >> +check_column "192.168.0.20" sb:Service_Monitor ip logical_port=lport1 >> +check_column "icmp" sb:Service_Monitor protocol logical_port=lport1 >> +check_column "192.168.0.255" sb:Service_Monitor src_ip logical_port=lport1 >> +check_column "0" sb:Service_Monitor port logical_port=lport1 >> +check_column "logical-switch-port" sb:Service_Monitor type logical_port=lport1 >> +check_column "11:11:11:11:11:11" sb:Service_Monitor src_mac logical_port=lport1 >> + >> +# Create one more monitor >> +check ovn-nbctl lsp-hc-add lport2 icmp 192.168.0.255 192.168.0.12 >> +lport1_uuid4=$(fetch_column nb:Logical_Switch_Port_Health_Check _uuid addresses="192.168.0.12") >> +check_row_count nb:Logical_Switch_Port_Health_Check 2 >> + >> +check_row_count sb:Service_Monitor 2 >> + >> +# One record for arp replay >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep 'ls_in_arp_rsp'| grep "priority=110" | ovn_strip_lflows], [0], [dnl >> + table=??(ls_in_arp_rsp ), priority=110 , match=(arp.tpa == 192.168.0.255 && arp.op == 1), action=(eth.dst = eth.src; eth.src = 11:11:11:11:11:11; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 11:11:11:11:11:11; arp.tpa = arp.spa; arp.spa = 192.168.0.255; outport = inport; flags.loopback = 1; output;) >> +]) >> + >> +ovn-nbctl list logical_switch_port_health_check Missing check? Or should we just remove this one? >> + >> +check ovn-nbctl lsp-hc-del lport1 >> +check ovn-nbctl lsp-hc-del lport2 >> +check_row_count nb:Logical_Switch_Port_Health_Check 0 >> +check_row_count sb:Service_Monitor 0 >> + >> +AT_CHECK([ovn-sbctl lflow-list ls1 | grep 'ls_in_arp_rsp'| grep "priority=110" | ovn_strip_lflows], [0], []) >> + >> +AT_CLEANUP >> +]) These ovn-northd.at changes should be moved to the previous patch. Regards, Dumitru
diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 3ccacfa2d..c788acb16 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -6856,6 +6856,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. */ @@ -6980,20 +6982,26 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn, const struct sbrec_service_monitor *sb_svc_mon; SBREC_SERVICE_MONITOR_TABLE_FOR_EACH (sb_svc_mon, svc_mon_table) { enum svc_monitor_type mon_type; - if (sb_svc_mon->type && !strcmp(sb_svc_mon->type, - "network-function")) { + enum svc_monitor_protocol protocol; + + 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")) { + mon_type = SVC_MON_TYPE_LSP; } else { mon_type = SVC_MON_TYPE_LB; } - enum svc_monitor_protocol protocol; if (!strcmp(sb_svc_mon->protocol, "udp")) { - protocol = SVC_MON_PROTO_UDP; + protocol = (mon_type == SVC_MON_TYPE_NF) ? + SVC_MON_PROTO_ICMP : SVC_MON_PROTO_UDP; } else if (!strcmp(sb_svc_mon->protocol, "icmp")) { protocol = SVC_MON_PROTO_ICMP; } else { - protocol = SVC_MON_PROTO_TCP; + protocol = (mon_type == SVC_MON_TYPE_NF) ? + SVC_MON_PROTO_ICMP : SVC_MON_PROTO_TCP; } const struct sbrec_port_binding *pb @@ -7030,9 +7038,6 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn, bool mac_found = false; if (mon_type == SVC_MON_TYPE_NF) { - if (protocol != SVC_MON_PROTO_ICMP) { - continue; - } input_pb = lport_lookup_by_name(sbrec_port_binding_by_name, sb_svc_mon->logical_input_port); if (!input_pb) { @@ -7047,11 +7052,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; @@ -8010,6 +8010,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; uint64_t packet_stub[128 / 8]; struct dp_packet packet; dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub); @@ -8056,7 +8057,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; @@ -8338,6 +8340,7 @@ pinctrl_handle_svc_check(struct rconn *swconn, const struct flow *ip_flow, "not found"); return; } + pinctrl_handle_tcp_svc_check(swconn, pkt_in, svc_mon); } else { const char *end = @@ -8350,48 +8353,69 @@ 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); + 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, 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"); return; } - if (svc_mon->type == SVC_MON_TYPE_NF) { - pinctrl_handle_icmp_svc_check(pkt_in, svc_mon); - } + + /* Type validation done during creation - + * asserts on unsupported types. */ + ovs_assert(svc_mon->type != SVC_MON_TYPE_NF || + svc_mon->type != SVC_MON_TYPE_LSP); + + pinctrl_handle_icmp_svc_check(pkt_in, svc_mon); + return; } } 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"); return; } - if (svc_mon->type == SVC_MON_TYPE_NF) { - pinctrl_handle_icmp_svc_check(pkt_in, svc_mon); - } + + /* Type validation done during creation + * - asserts on unsupported types. */ + ovs_assert(svc_mon->type != SVC_MON_TYPE_NF || + svc_mon->type != SVC_MON_TYPE_LSP); + + pinctrl_handle_icmp_svc_check(pkt_in, svc_mon); + return; } } diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index d425cfb7a..9225f0646 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -19030,3 +19030,135 @@ check ovn-nbctl --wait=sb sync OVN_CLEANUP_NORTHD AT_CLEANUP ]) + +AT_SETUP([Logical Switch Port Health Check - lflow/service monitor synchronization]) +ovn_start + +check ovn-nbctl ls-add ls1 +check ovn-nbctl lsp-add ls1 lport1 -- \ + lsp-set-addresses lport1 "f0:00:0f:01:02:04 192.168.0.10" "f0:00:0f:01:02:06 192.168.0.11" + +check ovn-nbctl lsp-add ls1 lport2 -- \ + lsp-set-addresses lport2 "f0:00:0f:01:02:08 192.168.0.12" + +check ovn-nbctl set NB_Global . options:svc_monitor_mac="11:11:11:11:11:11" + +# Create service monitor for all lsp addresses. +check ovn-nbctl lsp-hc-add lport1 icmp 192.168.0.255 +check_row_count nb:Logical_Switch_Port_Health_Check 1 +lport1_uuid1=$(fetch_column nb:Logical_Switch_Port_Health_Check _uuid protocol=icmp) + +# Check lflow and service monitor synchronization +AT_CHECK([ovn-sbctl lflow-list ls1 | grep 'ls_in_arp_rsp'| grep "priority=110" | ovn_strip_lflows], [0], [dnl + table=??(ls_in_arp_rsp ), priority=110 , match=(arp.tpa == 192.168.0.255 && arp.op == 1), action=(eth.dst = eth.src; eth.src = 11:11:11:11:11:11; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 11:11:11:11:11:11; arp.tpa = arp.spa; arp.spa = 192.168.0.255; outport = inport; flags.loopback = 1; output;) +]) + +check_row_count sb:Service_Monitor 2 +check_column "false false" sb:Service_Monitor ic_learned logical_port=lport1 +check_column "false false" sb:Service_Monitor remote logical_port=lport1 +check_column "192.168.0.10 192.168.0.11" sb:Service_Monitor ip logical_port=lport1 +check_column "icmp icmp" sb:Service_Monitor protocol logical_port=lport1 +check_column "192.168.0.255 192.168.0.255" sb:Service_Monitor src_ip logical_port=lport1 +check_column "0 0" sb:Service_Monitor port logical_port=lport1 +check_column "logical-switch-port logical-switch-port" sb:Service_Monitor type logical_port=lport1 +check_column "11:11:11:11:11:11 11:11:11:11:11:11" sb:Service_Monitor src_mac logical_port=lport1 + +# Create one more service monitor for all lsp addresses. +check ovn-nbctl lsp-hc-add lport1 tcp 192.168.0.254 80 +check_row_count nb:Logical_Switch_Port_Health_Check 2 +lport1_uuid2=$(fetch_column nb:Logical_Switch_Port_Health_Check _uuid protocol=tcp) + +# Check that 2 records (2 addresses) have been created for each protocol. +check_row_count sb:Service_Monitor 4 + +AT_CHECK([ovn-sbctl lflow-list ls1 | grep 'ls_in_arp_rsp'| grep "priority=110" | ovn_strip_lflows], [0], [dnl + table=??(ls_in_arp_rsp ), priority=110 , match=(arp.tpa == 192.168.0.254 && arp.op == 1), action=(eth.dst = eth.src; eth.src = 11:11:11:11:11:11; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 11:11:11:11:11:11; arp.tpa = arp.spa; arp.spa = 192.168.0.254; outport = inport; flags.loopback = 1; output;) + table=??(ls_in_arp_rsp ), priority=110 , match=(arp.tpa == 192.168.0.255 && arp.op == 1), action=(eth.dst = eth.src; eth.src = 11:11:11:11:11:11; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 11:11:11:11:11:11; arp.tpa = arp.spa; arp.spa = 192.168.0.255; outport = inport; flags.loopback = 1; output;) +]) + +# Change addresses on lport - addresses for service monitors should be changed +check ovn-nbctl lsp-set-addresses lport1 "f0:00:0f:01:02:04 192.168.0.20" + +check_row_count sb:Service_Monitor 2 +check_column "false false" sb:Service_Monitor ic_learned logical_port=lport1 +check_column "false false " sb:Service_Monitor remote logical_port=lport1 +check_column "192.168.0.20 192.168.0.20" sb:Service_Monitor ip logical_port=lport1 +check_column "icmp tcp" sb:Service_Monitor protocol logical_port=lport1 +check_column "192.168.0.255 192.168.0.254" sb:Service_Monitor src_ip logical_port=lport1 +check_column "0 80" sb:Service_Monitor port logical_port=lport1 +check_column "logical-switch-port logical-switch-port" sb:Service_Monitor type logical_port=lport1 +check_column "11:11:11:11:11:11 11:11:11:11:11:11" sb:Service_Monitor src_mac logical_port=lport1 + +# Check options propogations +hc_lport1_uuid=$(fetch_column nb:logical_switch_port_health_check _uuid src_ip="192.168.0.255") + +check ovn-nbctl set logical_switch_port_health_check $hc_lport1_uuid options:interval=3 +check ovn-nbctl set logical_switch_port_health_check $hc_lport1_uuid options:timeout=30 +check ovn-nbctl set logical_switch_port_health_check $hc_lport1_uuid options:success_count=1 +check ovn-nbctl set logical_switch_port_health_check $hc_lport1_uuid options:failure_count=2 + + +sm_lport1_uuid=$(fetch_column sb:service_monitor _uuid protocol=icmp) + +AT_CHECK([ovn-sbctl get Service_Monitor $sm_lport1_uuid options:interval], +[0], ["3" +]) +AT_CHECK([ovn-sbctl get Service_Monitor $sm_lport1_uuid options:failure_count], +[0], ["2" +]) +AT_CHECK([ovn-sbctl get Service_Monitor $sm_lport1_uuid options:success_count], +[0], ["1" +]) +AT_CHECK([ovn-sbctl get Service_Monitor $sm_lport1_uuid options:timeout], +[0], ["30" +]) + +ovn-nbctl list logical_switch_port + +check ovn-nbctl lsp-hc-del lport1 + +check_row_count nb:Logical_Switch_Port_Health_Check 0 + +# Change the service monitor to monitor only one address. +check ovn-nbctl lsp-hc-add lport1 icmp 192.168.0.255 192.168.0.20 +check_row_count nb:Logical_Switch_Port_Health_Check 1 +lport1_uuid3=$(fetch_column nb:Logical_Switch_Port_Health_Check _uuid protocol=icmp) + +check_row_count sb:Service_Monitor 1 + +AT_CHECK([ovn-sbctl lflow-list ls1 | grep 'ls_in_arp_rsp'| grep "priority=110" | ovn_strip_lflows], [0], [dnl + table=??(ls_in_arp_rsp ), priority=110 , match=(arp.tpa == 192.168.0.255 && arp.op == 1), action=(eth.dst = eth.src; eth.src = 11:11:11:11:11:11; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 11:11:11:11:11:11; arp.tpa = arp.spa; arp.spa = 192.168.0.255; outport = inport; flags.loopback = 1; output;) +]) + +check_column "false" sb:Service_Monitor ic_learned logical_port=lport1 +check_column "false" sb:Service_Monitor remote logical_port=lport1 +check_column "192.168.0.20" sb:Service_Monitor ip logical_port=lport1 +check_column "icmp" sb:Service_Monitor protocol logical_port=lport1 +check_column "192.168.0.255" sb:Service_Monitor src_ip logical_port=lport1 +check_column "0" sb:Service_Monitor port logical_port=lport1 +check_column "logical-switch-port" sb:Service_Monitor type logical_port=lport1 +check_column "11:11:11:11:11:11" sb:Service_Monitor src_mac logical_port=lport1 + +# Create one more monitor +check ovn-nbctl lsp-hc-add lport2 icmp 192.168.0.255 192.168.0.12 +lport1_uuid4=$(fetch_column nb:Logical_Switch_Port_Health_Check _uuid addresses="192.168.0.12") +check_row_count nb:Logical_Switch_Port_Health_Check 2 + +check_row_count sb:Service_Monitor 2 + +# One record for arp replay +AT_CHECK([ovn-sbctl lflow-list ls1 | grep 'ls_in_arp_rsp'| grep "priority=110" | ovn_strip_lflows], [0], [dnl + table=??(ls_in_arp_rsp ), priority=110 , match=(arp.tpa == 192.168.0.255 && arp.op == 1), action=(eth.dst = eth.src; eth.src = 11:11:11:11:11:11; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 11:11:11:11:11:11; arp.tpa = arp.spa; arp.spa = 192.168.0.255; outport = inport; flags.loopback = 1; output;) +]) + +ovn-nbctl list logical_switch_port_health_check + +check ovn-nbctl lsp-hc-del lport1 +check ovn-nbctl lsp-hc-del lport2 +check_row_count nb:Logical_Switch_Port_Health_Check 0 +check_row_count sb:Service_Monitor 0 + +AT_CHECK([ovn-sbctl lflow-list ls1 | grep 'ls_in_arp_rsp'| grep "priority=110" | ovn_strip_lflows], [0], []) + +AT_CLEANUP +]) diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 303b10894..15b7491bb 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -19046,3 +19046,71 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d /Couldn't parse IPv6 prefix nexthop.*/d"]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([Logical Switch Port Health Check]) + +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]) +NS_EXEC([lport], [ip 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" + +check ovn-nbctl --wait=hv sync + +check ovn-nbctl lsp-hc-add lport icmp 192.168.0.255 +lport1_uuid1=$(fetch_column nb:Logical_Switch_Port_Health_Check _uuid protocol=icmp) + +check_row_count sb:Service_Monitor 1 + +NETNS_START_TCPDUMP([lport], [-n -c 2 -i lport], [lport]) +OVS_WAIT_UNTIL([ + total_queries=`grep "ICMP" -c lport.tcpdump` + test "${total_queries}" = "2" +]) + +# Wait until the services are set to online. +wait_row_count Service_Monitor 1 status=online + +# Create one more health check on logical switch port +NETNS_DAEMONIZE([lport], [nc -l -k 192.168.0.10 4041], [lport_tcp.pid]) + +check ovn-nbctl lsp-hc-add lport tcp 192.168.0.255 4041 +lport1_uuid2=$(fetch_column nb:Logical_Switch_Port_Health_Check _uuid protocol=tcp) + +check_row_count sb:Service_Monitor 2 + +# Wait until the services are set to online. +wait_row_count Service_Monitor 2 status=online + +NETNS_DAEMONIZE([lport], [nc -ulp 4042], [lport_udp.pid]) + +check ovn-nbctl lsp-hc-add lport udp 192.168.0.255 4042 +lport1_uuid3=$(fetch_column nb:Logical_Switch_Port_Health_Check _uuid protocol=udp) + +check_row_count sb:Service_Monitor 3 +# Wait until the services are set to online. +wait_row_count Service_Monitor 3 status=online + +AT_CLEANUP +])
* Add service monitor type validation during creation to prevent invalid types and avoid subsequent type checks in runtime processing. * Implement ICMP probe handling for logical switch ports health checks. * Add unit tests for the new functionality. Signed-off-by: Alexandra Rukomoinikova <arukomoinikova@k2.cloud> --- controller/pinctrl.c | 90 ++++++++++++++++++----------- tests/ovn-northd.at | 132 +++++++++++++++++++++++++++++++++++++++++++ tests/system-ovn.at | 68 ++++++++++++++++++++++ 3 files changed, 257 insertions(+), 33 deletions(-)