On 11/6/26 10:11 AM, Rukomoinikova Aleksandra wrote: > On 05.11.2026 02:01, Rosemarie O'Riorden wrote: >> Hi Alexandra, thanks for the patch :) I have a few comments below. >> >> Overall the design and intent of this feature seems good. > HI Rosemarie! Thanks for the review, and sorry for the delay in > responding to your previous reviews —they all somehow ended up in my > spam folder! =) I’m not sure how that happened; sorry again! It's > definitely not me, I hope 😂
Oh, is that what you think of my reviews? :D Nah I'm totally kidding, no worries :) >> >> On 10/28/26 6:53 PM, Alexandra Rukomoinikova via dev wrote: >>> If all the health-checked backends of a VIP are down, the VIP traffic >>> is dropped (or rejected), even when it is the health check itself that >>> is broken. >>> >>> Add "options:fail-open" to the NB Load_Balancer_Health_Check table. If >>> set to true and all the backends of the VIP are down, traffic is load >>> balanced across all of them. As soon as one backend is back online, >>> only the online backends are used. >>> >>> No new I-P handling is needed: load balancers with health checks always >>> fall back to a full northd recompute. >>> >>> Signed-off-by: Alexandra Rukomoinikova <[email protected]> >>> Assisted-by: Claude Opus 5 <[email protected]> >>> --- >>> NEWS | 5 +++ >>> northd/northd.c | 98 ++++++++++++++++++++++++++--------------- >>> tests/ovn-northd.at | 104 ++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 172 insertions(+), 35 deletions(-) >>> >>> diff --git a/NEWS b/NEWS >>> index 4176f8d7f..4eb058cb5 100644 >>> --- a/NEWS >>> +++ b/NEWS >>> @@ -170,6 +170,11 @@ OVN v26.09.0 - xxx xx xxxx >>> change handlers, so a change is reconciled in place instead of >>> re-running >>> the whole ovn-ic computation. The "inc-engine/show-stats" command >>> reports the per-node recompute counters. >>> + - Added a new "options:fail-open" key to the Load_Balancer_Health_Check >>> + table. If set to true and all the backends of the VIP are detected as >>> + down, traffic is load balanced across all of them instead of being >>> + dropped (or rejected). As soon as at least one backend is online >>> again, >>> + only the online backends are used. >>> >>> OVN v26.03.0 - xxx xx xxxx >>> -------------------------- >>> diff --git a/northd/northd.c b/northd/northd.c >>> index 3f59894bd..b7123941d 100644 >>> --- a/northd/northd.c >>> +++ b/northd/northd.c >>> @@ -3568,6 +3568,53 @@ backend_is_available(const struct ovn_northd_lb *lb, >>> false : true; >>> } >>> >>> +static size_t >>> +build_lb_vip_backends(const struct ovn_northd_lb *lb, >>> + const struct ovn_lb_vip *lb_vip, >>> + const struct ovn_northd_lb_vip *lb_vip_nb, >>> + struct ds *action, >>> + const struct svc_monitors_map_data *svc_mons_data, >>> + bool ignore_health_status) >>> +{ >>> + const struct ovn_lb_backend *backend; >>> + size_t n_backends = 0; >>> + size_t i = 0; >>> + >>> + VECTOR_FOR_EACH_PTR (&lb_vip->backends, backend) { >>> + struct ovn_northd_lb_backend *backend_nb = >>> + &lb_vip_nb->backends_nb[i++]; >>> + bool ipv6_backend = !IN6_IS_ADDR_V4MAPPED(&backend->ip); >>> + >>> + if (lb_vip_nb->lb_health_check && >>> + !backend_nb->health_check) { >>> + continue; >> First of all there was an "XXX" comment here suggesting an iteration >> refactor which was actually added by you in commit 7b0eb4d9. Do you >> still want to address that or did you change your mind? >> >> >> Also, I believe this case is impossible because backend_nb->health_check >> always has the same value as lb_vip_nb->lb_health_check. >> >> It's initialized as: >> backend_nb->health_check = lb_vip_nb->lb_health_check; >> (In that same commit where you introduced distributed load balancers.) >> And then its value is never changed. >> And I guess it never would be, because health checks are not configured >> per-backend. >> >> Did you include this extra check just in case its value is changed >> somewhere in the future? > > The check is actually reachable. backends_nb is xcalloc'ed, and > health_check is only assigned at the init_backend label in > ovn_lb_vip_backends_ip_port_mappings_init(), after the backend's > ip_port_mappings entry has been parsed successfully. If the entry is > missing or malformed, the backend keeps health_check == false even > though the VIP has a health check configured. > > For example, with vips 10.0.0.10:80=10.0.0.3:80,20.0.0.3:80, a health > check on the VIP and a mapping only for 10.0.0.3, the check is what > keeps 20.0.0.3 out of the action. Without it, 20.0.0.3 would receive > traffic without being monitored, even with 10.0.0.3 down. This is not > new behavior, the backend was already skipped this way before commit > > 7b0eb4d9. Ack, thank you for the clarification. > > And about the comment: > I shouldn't have dropped that comment silently. Inside this branch, both > conditions mean the same thing: the backend has no valid mapping. So in > v2 I'd like to replace them with a single check: > > /* Backends without a valid ip_port_mappings entry are never used. */ > if (!backend_nb->logical_port) { > continue; > } Nice, and I assume you'd do the same for the distibuted backend check? > > This should resolve the XXX too, as I don't think a separate list of > selected backends would be worth it just to avoid one condition. I'd > also add a test with a non-distributed load balancer where only some of > the backends have a mapping, since I didn't find one that checks the > resulting action. What do you think if i will replace this check i have > > suggested? Ok sure that sounds like a good idea. Thanks! > > >> >> >> Wdyt about these two points? >>> + } >>> + >>> + if (lb->is_distributed && >>> + !backend_nb->distributed_backend) { >>> + continue; >>> + } >> Same question here. >> >>> + >>> + if (!ignore_health_status && backend_nb->health_check && >>> + !backend_is_available(lb, >>> + backend, >>> + backend_nb, >>> + svc_mons_data)) { >>> + continue; >>> + } >>> + >>> + if (backend_nb->distributed_backend) { >>> + ds_put_format(action, "\"%s\":", backend_nb->logical_port); >>> + } >>> + ds_put_format(action, >>> + ipv6_backend ? "[%s]:%"PRIu16"," : "%s:%"PRIu16",", >>> + backend->ip_str, backend->port); >>> + n_backends++; >>> + } >>> + >>> + return n_backends; >>> +} >>> + >>> static bool >>> build_lb_vip_actions(const struct ovn_northd_lb *lb, >>> const struct ovn_lb_vip *lb_vip, >>> @@ -3598,41 +3645,22 @@ build_lb_vip_actions(const struct ovn_northd_lb *lb, >>> : "ct_lb_mark(backends="); >>> >>> if (lb_vip_nb->lb_health_check || lb->is_distributed) { >>> - size_t i = 0; >>> - size_t n_active_backends = 0; >>> - const struct ovn_lb_backend *backend; >>> - VECTOR_FOR_EACH_PTR (&lb_vip->backends, backend) { >>> - struct ovn_northd_lb_backend *backend_nb = >>> - &lb_vip_nb->backends_nb[i++]; >>> - bool ipv6_backend = !IN6_IS_ADDR_V4MAPPED(&backend->ip); >>> - >>> - /* XXX: Remove these checks: by changing the iteration >>> - * only for selected backends. */ >> The "XXX" comment I mentioned is here btw. >> >>> - if (lb_vip_nb->lb_health_check && >>> - !backend_nb->health_check) { >>> - continue; >>> - } >>> - >>> - if (lb->is_distributed && >>> - !backend_nb->distributed_backend) { >>> - continue; >>> - } >>> - >>> - if (backend_nb->health_check && >>> - !backend_is_available(lb, >>> - backend, >>> - backend_nb, >>> - svc_mons_data)) { >>> - continue; >>> - } >>> - >>> - if (backend_nb->distributed_backend) { >>> - ds_put_format(action, "\"%s\":", backend_nb->logical_port); >>> - } >>> - ds_put_format(action, >>> - ipv6_backend ? "[%s]:%"PRIu16"," : >>> "%s:%"PRIu16",", >>> - backend->ip_str, backend->port); >>> - n_active_backends++; >>> + size_t backends_start = action->length; >>> + size_t n_active_backends = >>> + build_lb_vip_backends(lb, lb_vip, lb_vip_nb, action, >>> + svc_mons_data, false); >>> + >>> + /* If all the health-checked backends are offline and "fail-open" >>> is >>> + * enabled, use all of them instead of dropping (or rejecting) the >>> + * traffic. As soon as at least one backend is reported online >>> + * again, only the online ones are used. */ >>> + if (!n_active_backends && lb_vip_nb->lb_health_check >>> + && smap_get_bool(&lb_vip_nb->lb_health_check->options, >>> + "fail-open", false)) { >>> + ds_truncate(action, backends_start); >>> + n_active_backends = >>> + build_lb_vip_backends(lb, lb_vip, lb_vip_nb, action, >>> + svc_mons_data, true); >>> } >>> ds_chomp(action, ','); >>> >>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >>> index 93af177ab..1f212f695 100644 >>> --- a/tests/ovn-northd.at >>> +++ b/tests/ovn-northd.at >>> @@ -25077,3 +25077,107 @@ OVN_CLEANUP_NORTHD >>> AT_CLEANUP >>> ]) >>> >>> +OVN_FOR_EACH_NORTHD_NO_HV([ >>> +AT_SETUP([Load balancer health check fail_open]) >> Nit: This can be "fail-open" to match the rest of the patch. >> >> Take care, >> Rosemarie O'Riorden > thanks, will fix it >> >>> +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 ls-add sw0 >>> +check ovn-nbctl 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" >>> + >>> +# service_monitors state online requires corresponding port_binding to be >>> "up" >>> +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 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_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 >>> + >>> +check ovn-nbctl --wait=sb ls-lb-add sw0 lb1 >>> + >>> +sm_sw0_p1=$(fetch_column Service_Monitor _uuid logical_port=sw0-p1) >>> +sm_sw1_p1=$(fetch_column Service_Monitor _uuid logical_port=sw1-p1) >>> + >>> +AS_BOX([Both backends online]) >>> +check ovn-sbctl set service_monitor $sm_sw0_p1 status=online >>> +check ovn-sbctl set service_monitor $sm_sw1_p1 status=online >>> + >>> +AT_CAPTURE_FILE([sbflows1]) >>> +OVS_WAIT_FOR_OUTPUT( >>> + [ovn-sbctl dump-flows sw0 | tee sbflows1 | 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,20.0.0.3:80);) >>> +]) >>> + >>> +AS_BOX([All backends offline without fail-open - traffic is dropped]) >>> +check ovn-sbctl set service_monitor $sm_sw0_p1 status=offline >>> +check ovn-sbctl set service_monitor $sm_sw1_p1 status=offline >>> + >>> +AT_CAPTURE_FILE([sbflows2]) >>> +OVS_WAIT_FOR_OUTPUT( >>> + [ovn-sbctl dump-flows sw0 | tee sbflows2 | grep 'ls_in_lb.*priority=120' >>> | 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([All backends offline with fail-open - all backends are used]) >>> +check ovn-nbctl --wait=sb set load_balancer_health_check . \ >>> +options:fail-open=true >>> + >>> +AT_CAPTURE_FILE([sbflows3]) >>> +OVS_WAIT_FOR_OUTPUT( >>> + [ovn-sbctl dump-flows sw0 | tee sbflows3 | 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,20.0.0.3:80);) >>> +]) >>> + >>> +AS_BOX([fail-open takes precedence over the reject option]) >>> +check ovn-nbctl --wait=sb set load_balancer . options:reject=true >>> + >>> +AT_CAPTURE_FILE([sbflows4]) >>> +OVS_WAIT_FOR_OUTPUT( >>> + [ovn-sbctl dump-flows sw0 | tee sbflows4 | 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,20.0.0.3:80);) >>> +]) >>> +check ovn-nbctl --wait=sb remove load_balancer . options reject >>> + >>> +AS_BOX([One backend back online - only the online one is used]) >>> +check ovn-sbctl set service_monitor $sm_sw1_p1 status=online >>> + >>> +AT_CAPTURE_FILE([sbflows5]) >>> +OVS_WAIT_FOR_OUTPUT( >>> + [ovn-sbctl dump-flows sw0 | tee sbflows5 | 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=20.0.0.3:80);) >>> +]) >>> + >>> +AS_BOX([Last backend goes down again - all backends are used]) >>> +check ovn-sbctl set service_monitor $sm_sw1_p1 status=offline >>> + >>> +AT_CAPTURE_FILE([sbflows6]) >>> +OVS_WAIT_FOR_OUTPUT( >>> + [ovn-sbctl dump-flows sw0 | tee sbflows6 | 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,20.0.0.3:80);) >>> +]) >>> + >>> +AS_BOX([Disabling fail-open restores the drop behaviour]) >>> +check ovn-nbctl --wait=sb set load_balancer_health_check . \ >>> +options:fail-open=false >>> + >>> +AT_CAPTURE_FILE([sbflows7]) >>> +OVS_WAIT_FOR_OUTPUT( >>> + [ovn-sbctl dump-flows sw0 | tee sbflows7 | grep 'ls_in_lb.*priority=120' >>> | 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;) >>> +]) >>> + >>> +OVN_CLEANUP_NORTHD >>> +AT_CLEANUP >>> +]) > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
