On 8/14/26 4:24 PM, Tim Rozet via dev wrote:
> Commit 8e6f9a835 added a logical switch stage that extracts the
> original conntrack protocol and destination port for every new IP
> connection whenever the switch has a load balancer. This makes
> unrelated traffic depend on conntrack tuple fields even though the
> fields are only consumed by port-bearing load balancer rules.
>
> Some hardware offload implementations cannot match these fields.
> This is especially visible for unidirectional UDP, which may remain
> in ct.new while still being eligible for hardware offload.
>
> Generate extraction flows per port-bearing VIP instead. Match the VIP
> address and load balancer protocol so unrelated traffic takes the
> fallback flow without accessing conntrack tuple fields.
>
> Add coverage for direct and grouped load balancers, removal, and
> portless VIPs.
>
> Fixes: 8e6f9a8355e2 ("northd: Fix HW offload problem related to ct_tuple.")
>
> Assisted-by: OpenAI Codex
> Signed-off-by: Tim Rozet <[email protected]>
> ---
Hi Tim,
Thanks for the patch!
I also cc-ed Han, the original author of 8e6f9a8355e2 ("northd: Fix HW
offload problem related to ct_tuple."), for his view on this.
> Documentation/ref/ovn-logical-flows.7.rst | 11 ++++++----
> northd/northd.c | 26 ++++++++++++++---------
> tests/ovn-northd.at | 15 +++++++++++++
> 3 files changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/ref/ovn-logical-flows.7.rst
> b/Documentation/ref/ovn-logical-flows.7.rst
> index 0ec69e39e..f58feee70 100644
> --- a/Documentation/ref/ovn-logical-flows.7.rst
> +++ b/Documentation/ref/ovn-logical-flows.7.rst
> @@ -551,10 +551,13 @@ Ingress Table 13: Connection Tracking Field Extraction
> This table extracts connection tracking fields for new connections to be
> used by
> subsequent load balancing stages.
>
> -- A priority-100 flow matches ``ct.new && ip`` and extracts connection
> tracking
> - protocol and destination port information into registers ``reg1[16..23]``
> - (protocol) and ``reg1[0..15]`` (destination port) using the actions
> - ``reg1[16..23] = ct_proto(); reg1[0..15] = ct_tp_dst(); next;``.
> +- For every load balancer VIP on the switch that includes an L4 port *PORT*
> of
> + protocol *P* and IP address *VIP*, a priority-100 flow is added. For IPv4
> + *VIPs*, the flow matches ``ct.new && ip4.dst == VIP && P``. For IPv6
> *VIPs*,
> + the flow matches ``ct.new && ip6.dst == VIP && P``. It extracts connection
> + tracking protocol and destination port information into registers
> + ``reg1[16..23]`` (protocol) and ``reg1[0..15]`` (destination port) using
> the
> + actions ``reg1[16..23] = ct_proto(); reg1[0..15] = ct_tp_dst(); next;``.
>
> - A priority-0 flow matches all packets and advances to the next table.
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 8cd85a328..8a16793f1 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -9154,6 +9154,22 @@ build_lb_rules(struct lflow_table *lflows, struct
> ovn_lb_datapaths *lb_dps,
This function is for load balancers applied to switches. Don't we have
the exact same issue for load balancers applied to routers? I.e., in
function build_lrouter_in_ct_extract_flows().
> ds_clear(action);
> ds_clear(match);
>
> + /* Extract the original L4 tuple only for traffic that can match this
> + * port-bearing VIP. Accessing conntrack tuple fields for every new
> + * connection prevents unrelated traffic from being offloaded by some
> + * hardware implementations. */
> + if (lb_vip->port_str) {
> + ds_put_format(match, "ct.new && %s.dst == %s && %s", ip_match,
> + lb_vip->vip_str, lb->proto);
> + ovn_lflow_add_with_dp_group(
> + lflows, lb_dps->nb_ls_map.map, ods_size(ls_datapaths),
> + S_SWITCH_IN_CT_EXTRACT, 100, ds_cstr(match),
> + REG_CT_PROTO " = ct_proto(); "
> + REG_CT_TP_DST " = ct_tp_dst(); next;",
> + lb_dps->lflow_ref, WITH_HINT(&lb->nlb->header_));
> + ds_clear(match);
> + }
We now get one additional logical flow per load balancer VIP. It might
be acceptable but we should probably double check this at scale too.
Can we run ovn-kubernetes perf & scale jobs with a custom OVN version
including this fix (or a v2 of it)?
> +
> /* New connections in Ingress table. */
> const char *meter = NULL;
> bool reject = build_lb_vip_actions(lb, lb_vip, lb_vip_nb, action,
> @@ -19248,16 +19264,6 @@ build_ls_stateful_flows(const struct
> ls_stateful_record *ls_stateful_rec,
> sampling_apps, features, ls_stateful_rec->lflow_ref,
> sbrec_acl_id_table);
>
> - /* Build CT extraction flows - only needed if this datapath has load
> - * balancers. */
> - if (ls_stateful_rec->has_lb_vip) {
> - ovn_lflow_add(lflows, od, S_SWITCH_IN_CT_EXTRACT, 100,
> - "ct.new && ip",
> - REG_CT_PROTO " = ct_proto(); "
> - REG_CT_TP_DST " = ct_tp_dst(); next;",
> - ls_stateful_rec->lflow_ref);
> - }
> -
> build_lb_hairpin(ls_stateful_rec, od, lflows,
> ls_stateful_rec->lflow_ref);
> }
>
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 13400ff5e..a5d2092b6 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -5124,6 +5124,12 @@ check_stateful_flows() {
> table=??(ls_in_pre_stateful ), priority=120 , match=(reg0[[2]] == 1 &&
> ip4.dst == 10.0.0.20 && tcp.dst == 80), action=(reg4 = 10.0.0.20;
> reg2[[0..15]] = 80; ct_lb_mark;)
> ])
>
> + AT_CHECK([grep "ls_in_ct_extract" sw0flows | ovn_strip_lflows], [0], [dnl
> + table=??(ls_in_ct_extract ), priority=0 , match=(1), action=(next;)
> + table=??(ls_in_ct_extract ), priority=100 , match=(ct.new && ip4.dst ==
> 10.0.0.10 && tcp), action=(reg1[[16..23]] = ct_proto(); reg1[[0..15]] =
> ct_tp_dst(); next;)
> + table=??(ls_in_ct_extract ), priority=100 , match=(ct.new && ip4.dst ==
> 10.0.0.20 && tcp), action=(reg1[[16..23]] = ct_proto(); reg1[[0..15]] =
> ct_tp_dst(); next;)
> +])
> +
> AT_CHECK([grep "ls_in_lb " sw0flows | ovn_strip_lflows], [0], [dnl
> table=??(ls_in_lb ), priority=0 , match=(1), action=(next;)
> table=??(ls_in_lb ), priority=110 , match=(ct.trk && ct.est &&
> !ct.rpl && reg0[[19]] == 1 && ip4), action=(reg4 = ct_nw_dst(); reg2[[0..15]]
> = ct_tp_dst(); next;)
> @@ -5200,6 +5206,10 @@ AT_CHECK([grep "ls_in_pre_stateful" sw0flows |
> ovn_strip_lflows], [0], [dnl
> table=??(ls_in_pre_stateful ), priority=115 , match=(reg0[[2]] == 1 &&
> ip.is_frag), action=(reg0[[19]] = 1; ct_lb_mark;)
> ])
>
> +AT_CHECK([grep "ls_in_ct_extract" sw0flows | ovn_strip_lflows], [0], [dnl
> + table=??(ls_in_ct_extract ), priority=0 , match=(1), action=(next;)
> +])
> +
> AT_CHECK([grep "ls_in_lb " sw0flows | ovn_strip_lflows], [0], [dnl
> table=??(ls_in_lb ), priority=0 , match=(1), action=(next;)
> ])
> @@ -5246,6 +5256,11 @@ AT_CHECK([ovn-sbctl dump-flows sw0 | grep "ls_in_lb "
> | ovn_strip_lflows ], [0],
> table=??(ls_in_lb ), priority=110 , match=(ct.trk && ct.est &&
> !ct.rpl && reg0[[19]] == 1 && ip6), action=(xxreg1 = ct_ip6_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> ])
>
> +# A VIP without an L4 port does not use the extracted fields.
> +AT_CHECK([ovn-sbctl dump-flows sw0 | grep "ls_in_ct_extract" |
> ovn_strip_lflows], [0], [dnl
> + table=??(ls_in_ct_extract ), priority=0 , match=(1), action=(next;)
> +])
> +
> OVN_CLEANUP_NORTHD
> AT_CLEANUP
> ])
It would be nice to have a system test that validates that
unidirectional UDP traffic that's not destined to a LB VIP doesn't
generate ct_orig_tuple datapath flows. Essentially a test for the
scenario you describe in the commit log.
Could you please add one?
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
