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.
>
> 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?
>
>
> Wdyt about these two points?
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.
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;
}
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?
>
>> + }
>> +
>> + 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
>
>> +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
