When a load balancer with options:distributed=true is attached to a
logical router that has a chassis-redirect port, the incremental
engine path in northd_handle_lb_data_changes calls
handle_od_lb_datapath_modes, which sets od->is_distributed=true on
that LR datapath. However, the LRP-level lr_in_admission flows
generated by build_adm_ctrl_flows_for_lrouter_port check
od_is_centralized(op->od) (== !od->is_distributed) via
consider_l3dgw_port_is_centralized, and those flows are not rebuilt
on LB-association changes. The result is that lr_in_admission keeps a
stale is_chassis_resident("cr-lrp-X") guard on the gateway-LRP MAC
match, so ingress traffic for the LB VIP landing on any chassis other
than the cr-port host gets dropped at admission (priority-50/55
flows).
Toggling options:distributed=false then back to true masks the bug
because the second update path in en_lb_data returns EN_UNHANDLED
(lb_data_load_balancer_handler), forcing a full recompute which
re-runs the LRP-level admission flow generator with the now-correct
od->is_distributed.
When has_distributed_lb is set, northd_handle_lb_data_changes falls
back to a full recompute without trying to identify which specific LR
datapaths had their is_distributed bit transition.
Also fix has_routable_lb and has_distributed_lb tracking in
handle_od_lb_changes (guard prevented flags beyond the first LB) and
add has_distributed_lb reset in destroy_tracked_data (replacing a
duplicate has_health_checks reset).
Reproducer:
ovn-nbctl lb-add lb0 VIP:80 BE1:8080,BE2:8080 tcp
ovn-nbctl set load_balancer lb0 options:distributed=true
ovn-nbctl lr-lb-add ROUTER_WITH_DGP lb0
ovn-nbctl ls-lb-add LS lb0
Without this fix the priority-50/55 admission flows on the DGP LRP
retain the is_chassis_resident("cr-lrp-...") guard and ingress LB
traffic from non-gateway chassis is dropped.
Fixes: 7b0eb4d9ed05 ("northd: Add distributed load balancer support.")
Signed-off-by: Dmitrii Shcherbakov <[email protected]>
---
northd/en-lb-data.c | 8 +++-----
northd/northd.c | 19 ++++++++++++++++--
tests/ovn-northd.at | 48 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 68 insertions(+), 7 deletions(-)
diff --git a/northd/en-lb-data.c b/northd/en-lb-data.c
index 04ef20bcc..fd4f0d42f 100644
--- a/northd/en-lb-data.c
+++ b/northd/en-lb-data.c
@@ -698,10 +698,8 @@ handle_od_lb_changes(struct nbrec_load_balancer
**nbrec_lbs,
ovs_assert(lb);
trk_lb_data->has_health_checks |= lb->health_checks;
- if (!trk_lb_data->has_routable_lb) {
- trk_lb_data->has_routable_lb |= lb->routable;
- trk_lb_data->has_distributed_lb |= lb->is_distributed;
- }
+ trk_lb_data->has_routable_lb |= lb->routable;
+ trk_lb_data->has_distributed_lb |= lb->is_distributed;
}
if (unode) {
@@ -762,8 +760,8 @@ destroy_tracked_data(struct ed_type_lb_data *lb_data)
lb_data->tracked_lb_data.has_dissassoc_lbs_from_lbgrps = false;
lb_data->tracked_lb_data.has_dissassoc_lbs_from_od = false;
lb_data->tracked_lb_data.has_dissassoc_lbgrps_from_od = false;
- lb_data->tracked_lb_data.has_health_checks = false;
lb_data->tracked_lb_data.has_routable_lb = false;
+ lb_data->tracked_lb_data.has_distributed_lb = false;
struct hmapx_node *node;
HMAPX_FOR_EACH_SAFE (node, &lb_data->tracked_lb_data.deleted_lbs) {
diff --git a/northd/northd.c b/northd/northd.c
index 0dbf17426..1fb4cc01d 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3587,7 +3587,7 @@ build_lb_vip_actions(const struct ovn_northd_lb *lb,
return reject;
}
-static inline void
+static inline bool
handle_od_lb_datapath_modes(struct ovn_datapath *od,
struct ovn_lb_datapaths *lb_dps)
{
@@ -3595,9 +3595,14 @@ handle_od_lb_datapath_modes(struct ovn_datapath *od,
hmapx_add(&lb_dps->ls_lb_with_stateless_mode, od);
}
+ bool transition = false;
if (od->nbr && lb_dps->lb->is_distributed) {
+ if (!od->is_distributed) {
+ transition = true;
+ }
od->is_distributed = true;
}
+ return transition;
}
static void
@@ -5695,6 +5700,10 @@ northd_handle_lb_data_changes(struct tracked_lb_data
*trk_lb_data,
return false;
}
+ if (trk_lb_data->has_distributed_lb) {
+ return false;
+ }
+
struct ovn_lb_datapaths *lb_dps;
struct ovn_northd_lb *lb;
struct ovn_datapath *od;
@@ -5803,7 +5812,13 @@ northd_handle_lb_data_changes(struct tracked_lb_data
*trk_lb_data,
lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, &uuidnode->uuid);
ovs_assert(lb_dps);
ovn_lb_datapaths_add_lr(lb_dps, 1, &od, ods_size(lr_datapaths));
- handle_od_lb_datapath_modes(od, lb_dps);
+ if (handle_od_lb_datapath_modes(od, lb_dps)) {
+ /* od->is_distributed transitioned to true. LRP-level
+ * lr_in_admission and other od_is_centralized()-gated
+ * flows are not rebuilt on LB-association changes, so
+ * fall back to a full recompute. */
+ return false;
+ }
/* Add the lb to the northd tracked data. */
hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps);
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index f87b14c9a..d5b8f393c 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -19174,6 +19174,54 @@ OVN_CLEANUP_NORTHD
AT_CLEANUP
])
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([lr_in_admission cr-port guard dropped on distributed LB attach to
DGP LR])
+ovn_start
+
+check ovn-sbctl chassis-add ch1 geneve 127.0.0.1
+
+check ovn-nbctl lr-add lr1
+check ovn-nbctl lrp-add lr1 lrp1 02:ac:10:01:00:01 172.16.1.1/24
+# Set gateway_mtu so build_gateway_mtu_flow() emits both the priority-50
+# (unicast, with check_pkt_larger) and priority-55 (ARP) lr_in_admission
+# flows on the LRP. Both code paths gate their is_chassis_resident
+# guard on consider_l3dgw_port_is_centralized().
+check ovn-nbctl set logical_router_port lrp1 options:gateway_mtu=1400
+check ovn-nbctl ls-add ls1
+check ovn-nbctl lsp-add-router-port ls1 ls1-lrp1 lrp1
+# Make lrp1 a distributed gateway port with a chassis-redirect.
+check ovn-nbctl lrp-set-gateway-chassis lrp1 ch1
+check ovn-nbctl --wait=sb sync
+
+# Attach a distributed LB to a router that already has a chassis-redirect
+# port. The incremental path must rebuild admission
+# flows when is_distributed flipped, allowing LB traffic on
+# non-gateway chassis.
+check ovn-nbctl lb-add lb1 1.1.1.1:80 192.168.0.1:8080
+check ovn-nbctl set load_balancer lb1 options:distributed=true
+check ovn-nbctl --wait=sb lr-lb-add lr1 lb1
+
+ovn-sbctl lflow-list lr1 > lr1_lflows
+
+# Unicast (priority-50) and ARP (priority-55) admission flows on the
+# DGP LRP must not be gated by is_chassis_resident("cr-lrp1") once a
+# distributed LB is attached.
+AT_CHECK([grep lr_in_admission lr1_lflows | grep -E 'priority=5[[05]]' | grep
'02:ac:10:01:00:01' | grep -v eth.mcast | ovn_strip_lflows], [0], [dnl
+ table=??(lr_in_admission ), priority=50 , match=(eth.dst ==
02:ac:10:01:00:01 && inport == "lrp1"), action=(reg9[[1]] =
check_pkt_larger(1414); xreg0[[0..47]] = 02:ac:10:01:00:01; next;)
+ table=??(lr_in_admission ), priority=55 , match=(eth.dst ==
02:ac:10:01:00:01 && inport == "lrp1" && (arp)), action=(xreg0[[0..47]] =
02:ac:10:01:00:01; next;)
+])
+
+# As a cross-check, the cr-port's always-redirect option should also
+# be off, because has_distributed_lb on the LR's lr_stateful record is
+# now true.
+AT_CHECK([ovn-sbctl --bare --columns options find Port_Binding
logical_port=cr-lrp1 | tr ' ' '\n' | sort], [0], [dnl
+distributed-port=lrp1
+])
+
+OVN_CLEANUP_NORTHD
+AT_CLEANUP
+])
+
OVN_FOR_EACH_NORTHD_NO_HV([
AT_SETUP([ip_port_mappings validation: IPv6])
ovn_start
--
2.53.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
