On 5/3/22 21:20, Lorenzo Bianconi wrote:
> Group together gw routers where a given load balancer is installed
> dividing them according to snat_type flag (skip_snat, force_snat).
> This is a preliminary patch to reduce load balancer logical flows
> computation cost for gw routers.
>
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
Hi Lorenzo,
I think the idea in this patch (and following ones) is OK. There is
however an issue we need to fix. Please see below.
> northd/northd.c | 83 ++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 71 insertions(+), 12 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index bcb470043..d90879eeb 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -9832,6 +9832,29 @@ enum lb_snat_type {
> SKIP_SNAT,
> };
>
> +static void
> +build_gw_lrouter_nat_flows_for_lb(struct ovn_northd_lb *lb,
> + struct ovn_datapath **dplist, int n_dplist,
> + bool reject, char *new_match,
> + char *new_action, char *est_match,
> + char *est_action, struct hmap *lflows,
> + int prio, const struct shash *meter_groups)
> +{
> + for (size_t i = 0; i < n_dplist; i++) {
> + struct ovn_datapath *od = dplist[i];
> + const char *meter = NULL;
> +
> + if (reject) {
> + meter = copp_meter_get(COPP_REJECT, od->nbr->copp, meter_groups);
> + }
> + ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT, prio,
> + new_match, new_action, NULL, meter,
> + &lb->nlb->header_);
> + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, prio,
> + est_match, est_action, &lb->nlb->header_);
> + }
> +}
> +
> static void
> build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip,
> struct ovn_northd_lb *lb,
> @@ -9929,6 +9952,42 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
> lb_vip->vip_port);
> }
>
> + struct ovn_datapath **gw_router_skip_snat =
> + xcalloc(lb->n_nb_lr, sizeof *gw_router_skip_snat);
> + int n_gw_router_skip_snat = 0;
> +
> + struct ovn_datapath **gw_router_force_snat =
> + xcalloc(lb->n_nb_lr, sizeof *gw_router_force_snat);
> + int n_gw_router_force_snat = 0;
> +
> + struct ovn_datapath **gw_router =
> + xcalloc(lb->n_nb_lr, sizeof *gw_router);
> + int n_gw_router = 0;
> +
> + /* Group gw router since we do not have datapath dependency in
> + * lflow generation for them.
> + */
> + for (size_t i = 0; i < lb->n_nb_lr; i++) {
> + struct ovn_datapath *od = lb->nb_lr[i];
> + if (!od->n_l3dgw_ports) {
> + if (snat_type == SKIP_SNAT) {
> + gw_router_skip_snat[n_gw_router_skip_snat++] = od;
> + } else if (!lport_addresses_is_empty(&od->lb_force_snat_addrs) ||
> + od->lb_force_snat_router_ip) {
> + gw_router_force_snat[n_gw_router_force_snat++] = od;
> + snat_type = FORCE_SNAT;
> + } else {
> + gw_router[n_gw_router++] = od;
> + }
> + }
> + }
At this point, if at least one router has lb_force_snat_router_ip or
lb_force_snat_addrs set, then snat_type is FORCE_SNAT.
> +
> + build_gw_lrouter_nat_flows_for_lb(lb, gw_router_skip_snat,
> + n_gw_router_skip_snat, reject, new_match,
> + skip_snat_new_action, est_match,
> + skip_snat_est_action, lflows, prio, meter_groups);
> +
> +
> for (size_t i = 0; i < lb->n_nb_lr; i++) {
Here we iterate again through all routers on which the LB is applied.
Each of these might need a different "force snat" behavior.
> struct ovn_datapath *od = lb->nb_lr[i];
> char *new_match_p = new_match;
> @@ -9967,19 +10026,15 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
> od->l3dgw_ports[0]->cr_port->json_key);
> }
>
> - if (snat_type == NO_FORCE_SNAT &&
> - (!lport_addresses_is_empty(&od->lb_force_snat_addrs) ||
> - od->lb_force_snat_router_ip)) {
> - snat_type = FORCE_SNAT;
> - }
> -
> if (snat_type == SKIP_SNAT) {
> - ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT, prio,
> - new_match_p, skip_snat_new_action,
> NULL,
> - meter, &lb->nlb->header_);
> - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, prio,
> - est_match_p, skip_snat_est_action,
> - &lb->nlb->header_);
> + if (od->n_l3dgw_ports) {
> + ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT, prio,
> + new_match_p, skip_snat_new_action,
> + NULL, meter, &lb->nlb->header_);
> + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, prio,
> + est_match_p, skip_snat_est_action,
> + &lb->nlb->header_);
> + }
> } else if (snat_type == FORCE_SNAT) {
But if snat_type was FORCE_SNAT before the loop then we will always
enter this "if" which is not correct.
I think the way to correctly implement this is to actually build 6 sets
of routers:
1. gw_router_skip_snat
2. gw_router_force_snat
3. gw_router_no_snat
These first 3 are the ones you already built above. We need the
following 3 extra ones:
4. distrib_router_skip_snat
5. distrib_router_force_snat
6. distrib_router_no_snat
Maybe we can change the snat type enum to:
enum lb_snat_type {
GW_RTR_NO_FORCE_SNAT,
GW_RTR_FORCE_SNAT,
GW_RTR_SKIP_SNAT,
DIST_RTR_NO_FORCE_SNAT,
DIST_RTR_FORCE_SNAT,
DIST_RTR_SKIP_SNAT,
LB_SNAT_TYPE_MAX,
};
And then build an array of routers per snat type, e.g.:
struct ovn_datapath **rtr_by_snat_type[LB_SNAT_TYPE_MAX];
size_t n_rtr_by_snat_type[LB_SNAT_TYPE_MAX];
We could then factor out the body of this second loop into a function,
e.g., build_lrouter_snat_flows_for_lb(), and use it like:
for (snat_type = 0; snat_type < LB_SNAT_TYPE_MAX; snat_type++) {
for each od in rtr_by_snat_type[snat_type] {
build_lrouter_snat_flows_for_lb(od, snat_type, ...);
}
}
What do you think?
Thanks,
Dumitru
> char *new_actions = xasprintf("flags.force_snat_for_lb = 1; %s",
> ds_cstr(action));
> @@ -10044,6 +10099,10 @@ next:
> free(skip_snat_est_action);
> free(est_match);
> free(new_match);
> +
> + free(gw_router_force_snat);
> + free(gw_router_skip_snat);
> + free(gw_router);
> }
>
> static void
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
