On 12/16/22 08:20, Ales Musil wrote: > 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. >
Great, thanks again! _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
