> On 5/13/22 12:54, Dumitru Ceara wrote:
> > 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.
> >
>
> Patch 4/5 "fixes" this by skipping the second part of the loop for
> gw-routers. But we still have this problem for non-gw-routers on which
> the LB is applied.
ack, I agree. In a different form we already a similar issue even in ovn
master. I will fix it in v2.
Regards,
Lorenzo
>
> Thanks,
> Dumitru
>
> >> 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
