On 8/30/23 13:10, Ales Musil wrote:
> The affinity code was differentiating between force_snat
> and skip snat. However, if both parameters were set at the
> same time the force_snat would be preferred, which shouldn't
> be the case.
>
> Make sure the skip_snat is preferred if both parameters are
> present.
>
> Fixes: d3926b4 ("northd: rely on new actions for lb affinity")
> Reported-at: https://bugzilla.redhat.com/2224260
> Signed-off-by: Ales Musil <[email protected]>
> Signed-off-by: Dumitru Ceara <[email protected]>
> (cherry picked from commit 0f4df76)
> ---
> northd/northd.c | 20 +++++++++++++++-----
> tests/ovn-northd.at | 19 +++++++++++++++++++
> 2 files changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 00a9013dd..fd1f9c629 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -10669,6 +10669,10 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
> xcalloc(lb->n_nb_lr, sizeof *distributed_router);
> int n_distributed_router = 0;
>
> + struct ovn_datapath **lb_aff_skip_snat_router =
> + xcalloc(lb->n_nb_lr, sizeof *lb_aff_skip_snat_router);
> + int n_lb_aff_skip_snat_router = 0;
> +
> struct ovn_datapath **lb_aff_force_snat_router =
> xcalloc(lb->n_nb_lr, sizeof *lb_aff_force_snat_router);
> int n_lb_aff_force_snat_router = 0;
> @@ -10695,7 +10699,9 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
> distributed_router[n_distributed_router++] = od;
> }
>
> - if (!lport_addresses_is_empty(&od->lb_force_snat_addrs) ||
> + if (lb->skip_snat) {
> + lb_aff_skip_snat_router[n_lb_aff_skip_snat_router++] = od;
> + } else if (!lport_addresses_is_empty(&od->lb_force_snat_addrs) ||
> od->lb_force_snat_router_ip) {
> lb_aff_force_snat_router[n_lb_aff_force_snat_router++] = od;
> } else {
> @@ -10752,11 +10758,14 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
> "next;", lflows, prio, meter_groups);
>
> /* LB affinity flows for datapaths where CMS has specified
> - * skip_snat_for_lb floag option or regular datapaths.
> + * skip_snat_for_lb flag option.
> */
> - char *lb_aff_action =
> - lb->skip_snat ? "flags.skip_snat_for_lb = 1; " : NULL;
> - build_lb_affinity_lr_flows(lflows, lb, lb_vip, new_match, lb_aff_action,
> + build_lb_affinity_lr_flows(lflows, lb, lb_vip, new_match,
> + "flags.skip_snat_for_lb = 1; ",
> + lb_aff_skip_snat_router,
> + n_lb_aff_skip_snat_router);
> +
> + build_lb_affinity_lr_flows(lflows, lb, lb_vip, new_match, NULL,
> lb_aff_router, n_lb_aff_router);
>
> /* Distributed router logic */
> @@ -10852,6 +10861,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
> free(gw_router_force_snat);
> free(gw_router_skip_snat);
> free(distributed_router);
> + free(lb_aff_skip_snat_router);
> free(lb_aff_force_snat_router);
> free(lb_aff_router);
> free(gw_router);
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index b50f23e19..4e8eb38b8 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -8334,6 +8334,25 @@ AT_CHECK([grep "lr_in_dnat " R1flows_force_snat |
> sort], [0], [dnl
> table=7 (lr_in_dnat ), priority=70 , match=(ct.rel && !ct.est &&
> !ct.new && ct_mark.skip_snat == 1), action=(flags.skip_snat_for_lb = 1;
> ct_commit_nat;)
> ])
>
> +AS_BOX([Test LR flows - lb_force_snat_ip="172.16.0.1" + skip_snat=true])
> +check ovn-nbctl --wait=sb set logical_router R1
> options:lb_force_snat_ip="172.16.0.1"
> +check ovn-nbctl --wait=sb set load_balancer lb0 options:skip_snat=true
> +
> +ovn-sbctl dump-flows R1 > R1flows_force_skip_snat
> +AT_CAPTURE_FILE([R1flows_force_skip_snat])
> +
> +grep "lr_in_dnat " R1flows_force_skip_snat | sed 's/table=../table=??/' |
> sort
I removed this unrelated "grep" and then applied the patch to branch
22.12. Thanks for the backport, Ales!
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
