On 6/23/26 10:59 AM, Matteo Perin via dev wrote: > Load balancer (and other) health checks previously treated a backend as > available until its first health check probe completed: a newly created > Service_Monitor row had an empty status, which backend_is_available() > considers available. This means traffic could be forwarded to a backend > whose health is still unknown (i.e. right after it is added to a load > balancer). > > Fix this by initializing new Service_Monitor rows to "offline" instead > of leaving the status empty. > > For load balancers, this changes behavior from fail-open to fail-closed > for un-probed backends. > Network-function health checks already treat any non-"online" status > (including empty) as unhealthy, so this only normalizes their stored > status and does not change NF behavior. > > Documentation, NEWS and tests are updated accordingly. > > Signed-off-by: Matteo Perin <[email protected]> > ---
Hi Matteo, Thanks for the patch! > This is a rework of a previous patch, > "northd: Add forward_unknown option to LB health checks." > and it follows up on the discussion on the current behavior of the LB > health check monitors: > https://mail.openvswitch.org/pipermail/ovs-dev/2026-June/433080.html > > Unfortunately, while the changes in the actual northd code were minimal > with this approach, quite a few tests adjustments were required. > Most of these are quite easy and just require the Service Monitors to > be brought online, but this change also made a race condition happen > during the UDP service teardown of the load balancer health checks > system tests, which required a small change but a lot of debugging. > Sorry to hear that but thanks for figuring it out! > The added benefit of this approach is also that when listing the > service monitors with something like `ovn-sbctl list service_monitor`, > not-yet probed ones will be listed as offline instead of being in an > unknown state (previously ""). > Neat! > Best Regards, > Matteo > > Documentation/ref/ovn-logical-flows.7.rst | 7 +- > NEWS | 5 ++ > northd/northd.c | 6 ++ > tests/ovn-ic.at | 20 +++++- > tests/ovn-northd.at | 81 +++++++++++++++++++++-- > tests/ovn.at | 18 ++++- > tests/system-common-macros.at | 7 +- > tests/system-ovn.at | 14 ++-- > 8 files changed, 143 insertions(+), 15 deletions(-) > > diff --git a/Documentation/ref/ovn-logical-flows.7.rst > b/Documentation/ref/ovn-logical-flows.7.rst > index 2c13478a7..de6faa13d 100644 > --- a/Documentation/ref/ovn-logical-flows.7.rst > +++ b/Documentation/ref/ovn-logical-flows.7.rst > @@ -612,6 +612,9 @@ Ingress Table 15: LB > addresses of *args* is the same as the address family of *VIP*. If health > check is enabled, then *args* will only contain those endpoints whose > service > monitor status entry in ``OVN_Southbound`` db is either ``online`` or > empty. > + Newly created service monitors are initialized to the ``offline`` state by > + ovn-northd, so a backend is not used until its first health check probe > + succeeds. > For IPv4 traffic the flow also loads the original destination IP and > transport > port in registers ``reg1`` and ``reg2``. For IPv6 traffic the flow also > loads > the original destination IP and transport port in registers ``xxreg1`` and > @@ -2762,7 +2765,9 @@ flows do not get programmed for load balancers with > IPv6 *VIPs*. > will be replaced by ``flags.skip_snat_for_lb = 1; ct_lb_mark(args; > skip_snat);``. If health check is enabled, then *args* will only contain > those > endpoints whose service monitor status entry in ``OVN_Southbound`` db is > - either ``online`` or empty. > + either ``online`` or empty. Newly created service monitors are initialized > + to the ``offline`` state by ovn-northd, so a backend is not used until its > + first health check probe succeeds. > > - For all the configured load balancing rules for a router in > ``OVN_Northbound`` > database that includes just an IP address *VIP* to match on, a priority-110 > diff --git a/NEWS b/NEWS > index 5f67556cb..7a9c501b0 100644 > --- a/NEWS > +++ b/NEWS > @@ -1,5 +1,10 @@ > Post v26.03.0 > ------------- > + - Load balancer health checks now "fail closed" for newly added backends: > + ovn-northd initializes a newly created Service_Monitor row to the > + "offline" state, so traffic is not forwarded to a backend until its > first > + health check probe succeeds. Previously, such backends were treated as > + available until the first probe completed. > - Added Transit Switch port support with new ovn-ic-nbctl 'tsp-add', > 'tsp-del' and 'tsp-set-addr' commands. > - Added ability to set any "ipsec_*" NB_Global option to configure the > diff --git a/northd/northd.c b/northd/northd.c > index aa332c449..36e9be093 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -3278,6 +3278,12 @@ create_or_get_service_mon(struct ovsdb_idl_txn > *ovnsb_txn, > sbrec_service_monitor_set_remote(sbrec_mon, remote_backend); > sbrec_service_monitor_set_ic_learned(sbrec_mon, false); > > + /* A freshly created monitor has not been probed yet. Start it as > + * "offline" so that traffic is not forwarded to a backend whose health > + * is still unknown. ovn-controller updates the status to "online" once > + * the first health check probe succeeds. */ > + sbrec_service_monitor_set_status(sbrec_mon, "offline"); > + > mon_info = xzalloc(sizeof *mon_info); > mon_info->sbrec_mon = sbrec_mon; > mon_info->required = true; > diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at > index cf42a5cd0..aeae54d20 100644 > --- a/tests/ovn-ic.at > +++ b/tests/ovn-ic.at > @@ -4341,7 +4341,25 @@ AT_CHECK([ovn-sbctl lflow-list az3_ls1 | grep > 'ls_in_arp_rsp' | ovn_strip_lflows > # az2_lb2 20.20.20.2:80 2.2.2.1:10880,5.5.5.1:10880,6.6.6.1:10880 > # (5.5.5.1 - does not have a service monitor) > > -# Check that if the service monitors are not initialized, balancing is > performed on all but the 4th > +# Service monitors are created in the "offline" state by default, so local > +# backends are not used until their first health check succeeds. This is a > +# mock test, so bring the local backends' ports up and mark their monitors > +# online to emulate a successful health check before validating load > balancing. > +ovn_as az1 > +pb_lport1=$(ovn-sbctl --bare --no-headings --columns _uuid find Port_Binding > logical_port=lport1_az1) > +check ovn-sbctl set Port_Binding $pb_lport1 up=true We can directly: check ovn-sbctl set Port_Binding lport1_az1 up=true > +sm_lport1=$(ovn-sbctl --bare --no-headings --columns _uuid find > Service_Monitor logical_port=lport1_az1) We should use fetch_column, i.e.: sm_lport1=$(fetch_column Service_Monitor _uuid logical_port="lport1_az1") > +check ovn-sbctl set Service_Monitor $sm_lport1 status=online > +check ovn-nbctl --wait=sb sync > + > +ovn_as az2 > +pb_lport3=$(ovn-sbctl --bare --no-headings --columns _uuid find Port_Binding > logical_port=lport3_az2) > +check ovn-sbctl set Port_Binding $pb_lport3 up=true > +sm_lport3=$(ovn-sbctl --bare --no-headings --columns _uuid find > Service_Monitor logical_port=lport3_az2) Same here. > +check ovn-sbctl set Service_Monitor $sm_lport3 status=online > +check ovn-nbctl --wait=sb sync > + > +# Check that with the local backends initialized, balancing is performed on > all but the 4th > ovn_as az1 > AT_CHECK([ovn-sbctl lflow-list az1_ls1 | grep ls_in_lb | grep backends | > ovn_strip_lflows], [0], [dnl > table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == > 10.10.10.1 && reg1[[16..23]] == 6 && reg1[[0..15]] == 80), action=(reg4 = > 10.10.10.1; reg2[[0..15]] = 80; > ct_lb_mark(backends=1.1.1.1:10880,5.5.5.1:10880,3.3.3.1:10880);) > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 7d008a8fc..a6ac1371c 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -1469,6 +1469,11 @@ wait_row_count Service_Monitor 2 > > check ovn-nbctl --wait=sb ls-lb-add sw0 lb1 > > +# Service monitors are created in the "offline" state, so set them online to > +# verify that both backends are programmed. > +check ovn-sbctl set service_monitor sw0-p1 status=online > +check ovn-sbctl set service_monitor sw1-p1 status=online > + > AT_CAPTURE_FILE([sbflows]) > OVS_WAIT_FOR_OUTPUT( > [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | > ovn_strip_lflows], 0, [dnl > @@ -1486,6 +1491,10 @@ OVS_WAIT_FOR_OUTPUT( > wait_row_count Service_Monitor 1 > > check ovn-nbctl lsp-set-enabled sw0-p1 enabled > +wait_row_count Service_Monitor 2 > + > +# The re-created service monitor for sw0-p1 starts "offline"; set it online. > +check ovn-sbctl set service_monitor sw0-p1 status=online > > AT_CAPTURE_FILE([sbflows]) > OVS_WAIT_FOR_OUTPUT( > @@ -1509,6 +1518,14 @@ check_uuid ovn-nbctl --wait=sb -- --id=@hc create \ > Load_Balancer_Health_Check vip="10.0.0.10\:80" -- add Load_Balancer . \ > health_check @hc > wait_row_count Service_Monitor 2 > + > +AS_BOX([Get the uuid of both the service_monitor]) > +sm_sw0_p1=$(fetch_column Service_Monitor _uuid logical_port=sw0-p1) > +sm_sw1_p1=$(fetch_column Service_Monitor _uuid logical_port=sw1-p1) > + > +# The re-created service monitors start "offline"; set them online. > +check ovn-sbctl set service_monitor $sm_sw0_p1 status=online > +check ovn-sbctl set service_monitor $sm_sw1_p1 status=online > check ovn-nbctl --wait=sb sync > > ovn-sbctl dump-flows sw0 | grep backends | grep priority=120 > lflows.txt > @@ -1516,10 +1533,6 @@ AT_CHECK([cat lflows.txt | ovn_strip_lflows], [0], [dnl > table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == > 10.0.0.10 && reg1[[16..23]] == 6 && reg1[[0..15]] == 80), action=(reg4 = > 10.0.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > ]) > > -AS_BOX([Get the uuid of both the service_monitor]) > -sm_sw0_p1=$(fetch_column Service_Monitor _uuid logical_port=sw0-p1) > -sm_sw1_p1=$(fetch_column Service_Monitor _uuid logical_port=sw1-p1) > - > AT_CAPTURE_FILE([sbflows3]) > OVS_WAIT_FOR_OUTPUT( > [ovn-sbctl dump-flows sw0 | tee sbflows3 | grep 'priority=120.*backends' | > ovn_strip_lflows], [0], > @@ -1596,6 +1609,12 @@ wait_row_count Service_Monitor 3 > wait_row_count Service_Monitor 2 logical_port=sw0-p1 > wait_row_count Service_Monitor 1 port=1000 > > +# The new service monitor for the 10.0.0.40 vip backend (10.0.0.3:1000) > +# starts "offline"; set it online. > +sm_sw0_p1_1000=$(fetch_column Service_Monitor _uuid logical_port=sw0-p1 > port=1000) > +check ovn-sbctl set service_monitor $sm_sw0_p1_1000 status=online > +check ovn-nbctl --wait=sb sync > + > AT_CAPTURE_FILE([sbflows9]) > OVS_WAIT_FOR_OUTPUT( > [ovn-sbctl dump-flows sw0 | tee sbflows9 | grep backends | grep > priority=120 | ovn_strip_lflows], > @@ -1689,6 +1708,54 @@ OVN_CLEANUP_NORTHD > AT_CLEANUP > ]) > > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([Load balancer health check unprobed backends fail closed]) > +ovn_start > + > +check ovn-nbctl lb-add lb1 10.0.0.10:80 10.0.0.3:80,20.0.0.3:80 > +check ovn-nbctl --wait=sb set load_balancer . > ip_port_mappings:10.0.0.3=sw0-p1:10.0.0.2 > +check ovn-nbctl --wait=sb set load_balancer . > ip_port_mappings:20.0.0.3=sw1-p1:20.0.0.2 > + > +check ovn-nbctl ls-add sw0 > +check ovn-nbctl --wait=sb lsp-add sw0 sw0-p1 -- lsp-set-addresses sw0-p1 \ > +"00:00:00:00:00:03 10.0.0.3" > +check ovn-nbctl ls-add sw1 > +check ovn-nbctl --wait=sb lsp-add sw1 sw1-p1 -- lsp-set-addresses sw1-p1 \ > +"02:00:00:00:00:03 20.0.0.3" > + > +check ovn-sbctl chassis-add hv1 geneve 127.0.0.1 > +check ovn-sbctl lsp-bind sw0-p1 hv1 > +check ovn-sbctl lsp-bind sw1-p1 hv1 > +wait_row_count nb:Logical_Switch_Port 1 name=sw0-p1 'up=true' > +wait_row_count nb:Logical_Switch_Port 1 name=sw1-p1 'up=true' > + > +check ovn-nbctl --wait=sb ls-lb-add sw0 lb1 > +check_uuid ovn-nbctl --wait=sb -- --id=@hc create \ > +Load_Balancer_Health_Check vip="10.0.0.10\:80" -- add Load_Balancer . \ > +health_check @hc > +wait_row_count Service_Monitor 2 > + > +AS_BOX([Newly created monitors start offline, so the VIP drops by default.]) > +wait_row_count Service_Monitor 2 status=offline > +AT_CAPTURE_FILE([sbflows_initial]) > +OVS_WAIT_FOR_OUTPUT( > + [ovn-sbctl dump-flows sw0 | tee sbflows_initial | grep "ip4.dst == > 10.0.0.10.*reg1.*== 6.*reg1.*== 80" | grep priority=120 | grep ls_in_lb | > ovn_strip_lflows], [0], [dnl > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == > 10.0.0.10 && reg1[[16..23]] == 6 && reg1[[0..15]] == 80), action=(drop;) > +]) > + > +AS_BOX([Once a backend is probed online, it is added to the VIP.]) > +check ovn-sbctl set service_monitor sw0-p1 status=online > +wait_row_count Service_Monitor 1 logical_port=sw0-p1 status=online > +AT_CAPTURE_FILE([sbflows_online]) > +OVS_WAIT_FOR_OUTPUT( > + [ovn-sbctl dump-flows sw0 | tee sbflows_online | grep > 'priority=120.*backends' | ovn_strip_lflows], 0, [dnl > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == > 10.0.0.10 && reg1[[16..23]] == 6 && reg1[[0..15]] == 80), action=(reg4 = > 10.0.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.3:80);) > +]) > + > +OVN_CLEANUP_NORTHD > +AT_CLEANUP > +]) > + > OVN_FOR_EACH_NORTHD_NO_HV_PARALLELIZATION([ > AT_SETUP([Load balancer health check reply lflow for type=external backend > on localnet LS]) > ovn_start > @@ -19404,9 +19471,13 @@ AT_CHECK([cat lr1_lflow | grep lr_in_dnat | grep > priority=120 | ovn_strip_lflows > ]) > > check ovn-nbctl --wait=sb set load_balancer . > ip_port_mappings:\"[[2001::3]]\"=\"lport1:[[2001::2]]\" > + > +# The service monitor is created in the "offline" state and the backend's > port > +# is not up in this no-hv test, so the backend is not used (fail closed) > until > +# a health check probe succeeds. > ovn-sbctl lflow-list lr1 > lr1_lflow > AT_CHECK([cat lr1_lflow | grep lr_in_dnat | grep priority=120 | > ovn_strip_lflows], [0], [dnl > - table=??(lr_in_dnat ), priority=120 , match=(ct.new && !ct.rel && > ip6 && ip6.dst == 2001::a && reg1[[16..23]] == 6 && reg1[[0..15]] == 80), > action=(ct_lb_mark_local(backends="lport1":[[2001::3]]:80);) > + table=??(lr_in_dnat ), priority=120 , match=(ct.new && !ct.rel && > ip6 && ip6.dst == 2001::a && reg1[[16..23]] == 6 && reg1[[0..15]] == 80), > action=(drop;) > ]) > > check ovn-nbctl lb-del lb_distubuted > diff --git a/tests/ovn.at b/tests/ovn.at > index c6be58288..319cd378d 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -27123,6 +27123,11 @@ check ovn-nbctl --wait=hv sync > > wait_row_count Service_Monitor 2 > > +# Service monitors are created in the "offline" state. Mark both backends > +# online to verify the load balancer forwards to healthy backends. > +check ovn-sbctl set service_monitor sw0-p1 status=online > +check ovn-sbctl set service_monitor sw1-p1 status=online > + > AT_CAPTURE_FILE([sbflows]) > OVS_WAIT_FOR_OUTPUT( > [ovn-sbctl dump-flows > sbflows > @@ -27202,7 +27207,9 @@ wait_row_count Service_Monitor 0 > check ovn-nbctl lsp-add sw0 sw0-p1 > > wait_row_count Service_Monitor 1 > -wait_row_count Service_Monitor 0 status=offline > +# The monitor for the freshly re-added port has not been probed yet, so it > +# starts in the "offline" state (fail closed). > +wait_row_count Service_Monitor 1 status=offline > wait_row_count Service_Monitor 0 status=online > > # lr0_public is on hv1. Hence lr0 & public should not be local datapaths on > hv2, and ln_public not a related port for hv2. > @@ -27317,6 +27324,11 @@ check ovn-nbctl --wait=hv sync > > wait_row_count Service_Monitor 2 > > +# Service monitors are created in the "offline" state. Mark both backends > +# online to verify the load balancer forwards to healthy backends. > +check ovn-sbctl set service_monitor sw0-p1 status=online > +check ovn-sbctl set service_monitor sw1-p1 status=online > + > AT_CAPTURE_FILE([sbflows]) > OVS_WAIT_FOR_OUTPUT( > [ovn-sbctl dump-flows > sbflows > @@ -27395,7 +27407,9 @@ wait_row_count Service_Monitor 0 > check ovn-nbctl lsp-add sw0 sw0-p1 > > wait_row_count Service_Monitor 1 > -wait_row_count Service_Monitor 0 status=offline > +# The monitor for the freshly re-added port has not been probed yet, so it > +# starts in the "offline" state (fail closed). > +wait_row_count Service_Monitor 1 status=offline > wait_row_count Service_Monitor 0 status=online > > # lr0_public is on hv1. Hence lr0 & public should not be local datapaths on > hv2, and ln_public not a related port for hv2. > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > index 16c59c3ce..e599956c1 100644 > --- a/tests/system-common-macros.at > +++ b/tests/system-common-macros.at > @@ -191,14 +191,17 @@ m4_define([NETNS_START_DHCPD], > # killing only the parent leaves those children running and hangs the whole > # test cleanup (especially on failures). This macro therefore installs a > # cleanup hook that kills the child processes (pkill -P) before the parent. > +# The parent is killed unconditionally (";" rather than "&&") because there > +# might not be any child process at that point, but the parent still holds > the > +# listening socket open and must always be stopped. > # > # The parent PID is recorded in the 'pidfile' and the user can employ the > same > -# (pkill -P && kill) technique to cleanly stop the server at will during the > +# (pkill -P; kill) technique to cleanly stop the server at will during the > # test. > # > m4_define([NETNS_START_UDP_ECHO], > [ip netns exec $1 nc -$2 -k -l -u -p $3 -d 0.1 -c '/bin/cat' & echo $! > > $4 > - echo "pkill -P \`cat $4\` && kill \`cat $4\`" >> cleanup > + echo "pkill -P \`cat $4\`; kill \`cat $4\`" >> cleanup > ] > ) > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index a4afab4e5..522d2429f 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -4586,8 +4586,11 @@ udp_data 5 > > # Stop UDP service in sw0-p1. > # Due to the way UDP nc with "-k" forks, we need to kill all > -# children processes and then then the parent. > -NS_CHECK_EXEC([sw0-p1], [pkill -P $udp1_pid && kill $udp1_pid]) > +# children processes and then the parent. The parent is killed > +# unconditionally (";" rather than "&&") because there might not be any child > +# process at that point, but the parent still holds the listening socket open > +# and must always be stopped so the backend becomes unreachable. > +NS_CHECK_EXEC([sw0-p1], [pkill -P $udp1_pid; kill $udp1_pid]) > > wait_row_count Service_Monitor 1 logical_port=sw0-p1 status=offline > wait_row_count Service_Monitor 1 logical_port=sw0-p2 status=online > @@ -4778,8 +4781,11 @@ udp_data 5 > > # Stop UDP service in sw0-p1. > # Due to the way UDP nc with "-k" forks, we need to kill all > -# children processes and then then the parent. > -NS_CHECK_EXEC([sw0-p1], [pkill -P $udp1_pid && kill $udp1_pid]) > +# children processes and then the parent. The parent is killed > +# unconditionally (";" rather than "&&") because there might not be any child > +# process at that point, but the parent still holds the listening socket open > +# and must always be stopped so the backend becomes unreachable. > +NS_CHECK_EXEC([sw0-p1], [pkill -P $udp1_pid; kill $udp1_pid]) > > wait_row_count Service_Monitor 1 logical_port=sw0-p1 status=offline > wait_row_count Service_Monitor 1 logical_port=sw0-p2 status=online I took care of the small test comment I had above and applied the patch to the main branch. I also added you to the AUTHORS.rst list. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
