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]> --- 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. 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 ""). 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 d426d671a..2abe230b5 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 ability to set any "ipsec_*" NB_Global option to configure the IPsec backend. - Documented missing ovn-nbctl commands: "mirror-rule-add", diff --git a/northd/northd.c b/northd/northd.c index 30fdd9d39..f2f604766 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 9fc2bdec6..6969c233d 100644 --- a/tests/ovn-ic.at +++ b/tests/ovn-ic.at @@ -3907,7 +3907,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 +sm_lport1=$(ovn-sbctl --bare --no-headings --columns _uuid find Service_Monitor 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) +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 4ef2c8946..8b51c8ba5 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -1468,6 +1468,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 @@ -1485,6 +1490,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( @@ -1508,6 +1517,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 @@ -1515,10 +1532,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], @@ -1595,6 +1608,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], @@ -1688,6 +1707,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 @@ -19308,9 +19375,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 -- 2.43.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
