Hi Dmitrii, thanks for the fix! Left some comments below
> 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 <dmitrii.s at protonmail.com
> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>>
> ---
> 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;
> + }
I don’t think this part is needed. In the function above you already added an
early fallback
to recompute when a distributed load balancer is present. It gets triggered
when we iterate
over all new/affected datapaths in lb-data IP-node, so we’d only hit this|if|
for updating
routers without distributed LBs, while the distributed LB itself is already
marked as affected.
That case can be handled incrementally anyway.
>
> /* 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
We usually don't add chassis manually in tests, you can use the non-existent
chassis below.
> +
> +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
