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 > <[email protected]> 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 <[email protected]> >> ---
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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
