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.
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
