On Fri, Dec 16, 2022 at 12:32 AM Dumitru Ceara <[email protected]> wrote:
> On 12/7/22 16:34, Ales Musil wrote: > > In order to have the SNAT action available for > > related traffic store the flag in CT register. > > > > Extend the ct_lb and ct_lb_mark action to > > allow additional parameter that sets the > > corresponding flag if needed in ct_label/ct_mark > > register. This allows us to later on match on it > > for related traffic and set the corresponding flags > > fro additional flows. > > > > Currently only two flags are supported "skip_snat" > > and "force_snat" which are mutually exclusive. > > > > Reported-at: https://bugzilla.redhat.com/2126083 > > Signed-off-by: Ales Musil <[email protected]> > > --- > > Hi Ales, > > First of all thanks a lot for fixing this, I know it wasn't easy to > debug! > > [...] > > > diff --git a/northd/northd.c b/northd/northd.c > > index a16e8a8a7..cb5b14181 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > [...] > > > @@ -10411,6 +10422,11 @@ build_lrouter_nat_flows_for_lb(struct > ovn_lb_vip *lb_vip, > > bool reject = build_lb_vip_actions(lb_vip, vips_nb, action, > > lb->selection_fields, false, > > ct_lb_mark); > > + bool drop = !!strncmp(ds_cstr(action), "ct_lb", strlen("ct_lb")); > > + if (!drop) { > > + /* Remove the trailing ");". */ > > + ds_truncate(action, action->length - 2); > > + } > > > > /* Higher priority rules are added for load-balancing in DNAT > > * table. For every match (on a VIP[:port]), we add two flows. > > @@ -10427,8 +10443,9 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip > *lb_vip, > > } > > > > if (lb->skip_snat) { > > - skip_snat_new_action = xasprintf("flags.skip_snat_for_lb = 1; > %s", > > - ds_cstr(action)); > > + skip_snat_new_action = xasprintf("flags.skip_snat_for_lb = 1; > %s%s", > > + ds_cstr(action), > > + drop ? "" : "; skip_snat);"); > > skip_snat_est_action = xasprintf("flags.skip_snat_for_lb = 1; " > > "next;"); > > } > > @@ -10561,8 +10578,9 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip > *lb_vip, > > skip_snat_new_action, est_match, > > skip_snat_est_action, lflows, prio, meter_groups); > > > > - char *new_actions = xasprintf("flags.force_snat_for_lb = 1; %s", > > - ds_cstr(action)); > > + char *new_actions = xasprintf("flags.force_snat_for_lb = 1; %s%s", > > + ds_cstr(action), > > + drop ? "" : "; force_snat);"); > > build_gw_lrouter_nat_flows_for_lb(lb, gw_router_force_snat, > > n_gw_router_force_snat, reject, new_match, > > new_actions, est_match, > > @@ -10577,6 +10595,10 @@ build_lrouter_nat_flows_for_lb(struct > ovn_lb_vip *lb_vip, > > lb_aff_force_snat_router, > > n_lb_aff_force_snat_router); > > > > + if (!drop) { > > + ds_put_cstr(action, ");"); > > + } > > + > > I think I should've spoken earlier but I was a bit behind with reviewing > this patch until now: this whole part seems a bit fragile because we > make assumptions about how the string built by build_lb_vip_actions() > looks like. What if in the future the action string built by > build_lb_vip_actions() doesn't start with "ct_lb.."? What if it > doesn't end with ");"? > > I also don't really like the 'drop' variable: it's almost always equal > to 'reject' with one exception, when healthcheck is enabled and no > backends are active but the LB VIP is not configured to reject on > empty backend sets. > > But I won't block the release for this part. Instead, I think we > should find a way to refactor all this code in the near future so that > all LB actions are built in a single place. build_lb_vip_actions() was > supposed to do that but now part of it is done outside the function. > What do you think, does it sound feasible to you? > I agree it's not the best approach, the refactor I've prepared some time ago might address some of those concerns. The problem is that build_lb_vip_actions doesn't know the context so it's hard to make decision if there should be a flag or not. I don't know what would be the best solution for that, but I'll try to think about the solution. Hopefully I'll be able to include that in the refactor. > > I added Mark's ack and pushed the patch to main and to branch-22.12. > > Thanks, > Dumitru > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
