On Fri, Jan 13, 2023 at 7:51 AM Ales Musil <[email protected]> wrote:
> > > On Thu, Jan 12, 2023 at 8:41 PM Mark Michelson <[email protected]> > wrote: > >> Acked-by: Mark Michelson <[email protected]> >> >> While reading this, I kept thinking this looked oddly familiar. It turns >> out I gave an ack to v1 of this patch :) >> >> Anyway, I took a close look at the differences between v2 and v3, and >> I'm now giving this version an ack too. Thanks a bunch, Ales. >> >> On 12/16/22 09:47, Ales Musil wrote: >> > To make it easier to add flows to this stage, refactor the function. >> > This also has the benefit that we should see fewer allocations due to >> > rearranging how we create flows and how we manipulate the match string. >> > >> > Signed-off-by: Ales Musil <[email protected]> >> > --- >> > northd/northd.c | 456 ++++++++++++++++++++++-------------------------- >> > 1 file changed, 207 insertions(+), 249 deletions(-) >> > >> > diff --git a/northd/northd.c b/northd/northd.c >> > index 4751feab4..7d1425180 100644 >> > --- a/northd/northd.c >> > +++ b/northd/northd.c >> > @@ -3857,10 +3857,12 @@ static bool >> > build_lb_vip_actions(struct ovn_lb_vip *lb_vip, >> > struct ovn_northd_lb_vip *lb_vip_nb, >> > struct ds *action, char *selection_fields, >> > + struct ds *skip_snat_action, struct ds >> *force_snat_action, >> > bool ls_dp, bool ct_lb_mark) >> > { >> > const char *ct_lb_action = ct_lb_mark ? "ct_lb_mark" : "ct_lb"; >> > - bool skip_hash_fields = false, reject = false; >> > + bool reject = !lb_vip->n_backends && lb_vip->empty_backend_rej; >> > + bool drop = false; >> > >> > if (lb_vip_nb->lb_health_check) { >> > ds_put_format(action, "%s(backends=", ct_lb_action); >> > @@ -3881,23 +3883,12 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip, >> > ds_put_format(action, "%s:%"PRIu16",", >> > backend->ip_str, backend->port); >> > } >> > + ds_chomp(action, ','); >> > >> > - if (!n_active_backends) { >> > - if (!lb_vip->empty_backend_rej) { >> > - ds_clear(action); >> > - ds_put_cstr(action, debug_drop_action()); >> > - skip_hash_fields = true; >> > - } else { >> > - reject = true; >> > - } >> > - } else { >> > - ds_chomp(action, ','); >> > - ds_put_cstr(action, ");"); >> > - } >> > - } else if (lb_vip->empty_backend_rej && !lb_vip->n_backends) { >> > - reject = true; >> > + drop = !n_active_backends && !lb_vip->empty_backend_rej; >> > + reject = !n_active_backends && lb_vip->empty_backend_rej; >> > } else { >> > - ds_put_format(action, "%s(backends=%s);", ct_lb_action, >> > + ds_put_format(action, "%s(backends=%s", ct_lb_action, >> > lb_vip_nb->backend_ips); >> > } >> > >> > @@ -3907,12 +3898,26 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip, >> > ds_clear(action); >> > ds_put_format(action, "reg0 = 0; reject { outport <-> inport; >> " >> > "next(pipeline=egress,table=%d);};", stage); >> > - } else if (!skip_hash_fields && selection_fields && >> selection_fields[0]) { >> > - ds_chomp(action, ';'); >> > - ds_chomp(action, ')'); >> > - ds_put_format(action, "; hash_fields=\"%s\");", >> selection_fields); >> > + } else if (drop) { >> > + ds_clear(action); >> > + ds_put_cstr(action, debug_drop_action()); >> > + } else if (selection_fields && selection_fields[0]) { >> > + ds_put_format(action, "; hash_fields=\"%s\"", >> selection_fields); >> > + } >> > + >> > + bool is_lb_action = !(reject || drop); >> > + >> > + if (!ls_dp) { >> > + ds_put_format(skip_snat_action, "flags.skip_snat_for_lb = 1; >> %s%s", >> > + ds_cstr(action), is_lb_action ? "; skip_snat);" >> : ""); >> > + ds_put_format(force_snat_action, "flags.force_snat_for_lb = 1; >> %s%s", >> > + ds_cstr(action), is_lb_action ? "; force_snat);" >> : ""); >> > } >> > - return reject; >> > + if (is_lb_action) { >> > + ds_put_cstr(action, ");"); >> > + } >> > + >> > + return false; >> > } >> > >> > static void >> > @@ -7493,8 +7498,8 @@ build_lb_rules(struct hmap *lflows, struct >> ovn_northd_lb *lb, bool ct_lb_mark, >> > /* New connections in Ingress table. */ >> > const char *meter = NULL; >> > bool reject = build_lb_vip_actions(lb_vip, lb_vip_nb, action, >> > - lb->selection_fields, true, >> > - ct_lb_mark); >> > + lb->selection_fields, NULL, >> > + NULL, true, ct_lb_mark); >> > >> > ds_put_format(match, "ct.new && %s.dst == %s", ip_match, >> > lb_vip->vip_str); >> > @@ -10431,49 +10436,123 @@ get_force_snat_ip(struct ovn_datapath *od, >> const char *key_type, >> > return true; >> > } >> > >> > +#define LROUTER_NAT_LB_FLOW_INIT(MATCH, ACTION, PRIO) \ >> > + (struct lrouter_nat_lb_flow) \ >> > + { .action = (ACTION), .lflow_ref = NULL, \ >> > + .hash = ovn_logical_flow_hash( \ >> > + ovn_stage_get_table(S_ROUTER_IN_DNAT), \ >> > + ovn_stage_get_pipeline(S_ROUTER_IN_DNAT), \ >> > + (PRIO), ds_cstr(MATCH), (ACTION)) } >> > + >> > +enum lrouter_nat_lb_flow_type { >> > + LROUTER_NAT_LB_FLOW_NORMAL = 0, >> > + LROUTER_NAT_LB_FLOW_SKIP_SNAT, >> > + LROUTER_NAT_LB_FLOW_FORCE_SNAT, >> > + LROUTER_NAT_LB_FLOW_MAX, >> > +}; >> > + >> > +struct lrouter_nat_lb_flow { >> > + char *action; >> > + struct ovn_lflow *lflow_ref; >> > + >> > + uint32_t hash; >> > +}; >> > + >> > +struct lrouter_nat_lb_flows_ctx { >> > + struct lrouter_nat_lb_flow new[LROUTER_NAT_LB_FLOW_MAX]; >> > + struct lrouter_nat_lb_flow est[LROUTER_NAT_LB_FLOW_MAX]; >> > + >> > + struct ds *new_match; >> > + struct ds *est_match; >> > + struct ds *undnat_match; >> > + >> > + struct ovn_lb_vip *lb_vip; >> > + struct ovn_northd_lb *lb; >> > + bool reject; >> > + >> > + int prio; >> > + >> > + struct hmap *lflows; >> > + const struct shash *meter_groups; >> > +}; >> > + >> > 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) >> > -{ >> > - if (!n_dplist) { >> > +build_distr_lrouter_nat_flows_for_lb(struct lrouter_nat_lb_flows_ctx >> *ctx, >> > + enum lrouter_nat_lb_flow_type >> type, >> > + struct ovn_datapath *od) >> > +{ >> > + char *gw_action = od->is_gw_router ? "ct_dnat;" : >> "ct_dnat_in_czone;"; >> > + /* Store the match lengths, so we can reuse the ds buffer. */ >> > + size_t new_match_len = ctx->new_match->length; >> > + size_t est_match_len = ctx->est_match->length; >> > + size_t undnat_match_len = ctx->undnat_match->length; >> > + >> > + >> > + const char *meter = NULL; >> > + >> > + if (ctx->reject) { >> > + meter = copp_meter_get(COPP_REJECT, od->nbr->copp, >> ctx->meter_groups); >> > + } >> > + >> > + if (ctx->lb_vip->n_backends || !ctx->lb_vip->empty_backend_rej) { >> > + ds_put_format(ctx->new_match, " && is_chassis_resident(%s)", >> > + od->l3dgw_ports[0]->cr_port->json_key); >> > + ds_put_format(ctx->est_match, " && is_chassis_resident(%s)", >> > + od->l3dgw_ports[0]->cr_port->json_key); >> > + } >> > + >> > + ovn_lflow_add_with_hint__(ctx->lflows, od, S_ROUTER_IN_DNAT, >> ctx->prio, >> > + ds_cstr(ctx->new_match), >> ctx->new[type].action, >> > + NULL, meter, &ctx->lb->nlb->header_); >> > + ovn_lflow_add_with_hint(ctx->lflows, od, S_ROUTER_IN_DNAT, >> ctx->prio, >> > + ds_cstr(ctx->est_match), >> ctx->est[type].action, >> > + &ctx->lb->nlb->header_); >> > + >> > + ds_truncate(ctx->new_match, new_match_len); >> > + ds_truncate(ctx->est_match, est_match_len); >> > + >> > + if (!ctx->lb_vip->n_backends) { >> > return; >> > } >> > >> > - struct ovn_lflow *lflow_ref_new = NULL, *lflow_ref_est = NULL; >> > - uint32_t hash_new = ovn_logical_flow_hash( >> > - ovn_stage_get_table(S_ROUTER_IN_DNAT), >> > - ovn_stage_get_pipeline(S_ROUTER_IN_DNAT), >> > - prio, new_match, new_action); >> > - uint32_t hash_est = ovn_logical_flow_hash( >> > - ovn_stage_get_table(S_ROUTER_IN_DNAT), >> > - ovn_stage_get_pipeline(S_ROUTER_IN_DNAT), >> > - prio, est_match, est_action); >> > + char *action = type == LROUTER_NAT_LB_FLOW_NORMAL >> > + ? gw_action : ctx->est[type].action; >> > >> > - for (size_t i = 0; i < n_dplist; i++) { >> > - struct ovn_datapath *od = dplist[i]; >> > - const char *meter = NULL; >> > + ds_put_format(ctx->undnat_match, >> > + ") && outport == %s && is_chassis_resident(%s)", >> > + od->l3dgw_ports[0]->json_key, >> > + od->l3dgw_ports[0]->cr_port->json_key); >> > + ovn_lflow_add_with_hint(ctx->lflows, od, S_ROUTER_OUT_UNDNAT, 120, >> > + ds_cstr(ctx->undnat_match), action, >> > + &ctx->lb->nlb->header_); >> > + ds_truncate(ctx->undnat_match, undnat_match_len); >> > +} >> > >> > - if (reject) { >> > - meter = copp_meter_get(COPP_REJECT, od->nbr->copp, >> meter_groups); >> > - } >> > - if (meter || !ovn_dp_group_add_with_reference(lflow_ref_new, >> od)) { >> > - struct ovn_lflow *lflow = >> ovn_lflow_add_at_with_hash(lflows, od, >> > - S_ROUTER_IN_DNAT, prio, new_match, new_action, >> > - NULL, meter, &lb->nlb->header_, OVS_SOURCE_LOCATOR, >> > - hash_new); >> > - lflow_ref_new = meter ? NULL : lflow; >> > - } >> > +static void >> > +build_gw_lrouter_nat_flows_for_lb(struct lrouter_nat_lb_flows_ctx *ctx, >> > + enum lrouter_nat_lb_flow_type type, >> > + struct ovn_datapath *od) >> > +{ >> > + const char *meter = NULL; >> > + struct lrouter_nat_lb_flow *new_flow = &ctx->new[type]; >> > + struct lrouter_nat_lb_flow *est_flow = &ctx->est[type]; >> > >> > - if (!ovn_dp_group_add_with_reference(lflow_ref_est, od)) { >> > - lflow_ref_est = ovn_lflow_add_at_with_hash(lflows, od, >> > - S_ROUTER_IN_DNAT, prio, est_match, est_action, >> > - NULL, NULL, &lb->nlb->header_, >> > - OVS_SOURCE_LOCATOR, hash_est); >> > - } >> > + if (ctx->reject) { >> > + meter = copp_meter_get(COPP_REJECT, od->nbr->copp, >> ctx->meter_groups); >> > + } >> > + if (meter || !ovn_dp_group_add_with_reference(new_flow->lflow_ref, >> od)) { >> > + struct ovn_lflow *lflow = >> ovn_lflow_add_at_with_hash(ctx->lflows, od, >> > + S_ROUTER_IN_DNAT, ctx->prio, ds_cstr(ctx->new_match), >> > + new_flow->action, NULL, meter, &ctx->lb->nlb->header_, >> > + OVS_SOURCE_LOCATOR, new_flow->hash); >> > + new_flow->lflow_ref = meter ? NULL : lflow; >> > + } >> > + >> > + if (!ovn_dp_group_add_with_reference(est_flow->lflow_ref, od)) { >> > + est_flow->lflow_ref = ovn_lflow_add_at_with_hash(ctx->lflows, >> od, >> > + S_ROUTER_IN_DNAT, ctx->prio, ds_cstr(ctx->est_match), >> > + est_flow->action, NULL, NULL, &ctx->lb->nlb->header_, >> > + OVS_SOURCE_LOCATOR, est_flow->hash); >> > } >> > } >> > >> > @@ -10487,22 +10566,25 @@ build_lrouter_nat_flows_for_lb(struct >> ovn_lb_vip *lb_vip, >> > bool ct_lb_mark) >> > { >> > const char *ct_natted = ct_lb_mark ? "ct_mark.natted" : >> "ct_label.natted"; >> > - char *skip_snat_new_action = NULL; >> > - char *skip_snat_est_action = NULL; >> > - char *new_match; >> > - char *est_match; >> > + >> > + bool ipv4 = lb_vip->address_family == AF_INET; >> > + const char *ip_match = ipv4 ? "ip4" : "ip6"; >> > + const char *ip_reg = ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6; >> > + >> > + int prio = 110; >> > + >> > + struct ds skip_snat_act = DS_EMPTY_INITIALIZER; >> > + struct ds force_snat_act = DS_EMPTY_INITIALIZER; >> > + struct ds est_match = DS_EMPTY_INITIALIZER; >> > + struct ds undnat_match = DS_EMPTY_INITIALIZER; >> > + struct ds unsnat_match = DS_EMPTY_INITIALIZER; >> > >> > ds_clear(match); >> > ds_clear(action); >> > >> > 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); >> > - } >> > + lb->selection_fields, >> &skip_snat_act, >> > + &force_snat_act, false, >> ct_lb_mark); >> > >> > /* Higher priority rules are added for load-balancing in DNAT >> > * table. For every match (on a VIP[:port]), we add two flows. >> > @@ -10510,50 +10592,23 @@ build_lrouter_nat_flows_for_lb(struct >> ovn_lb_vip *lb_vip, >> > * of "ct_lb_mark($targets);". The other flow is for ct.est with >> > * an action of "next;". >> > */ >> > - if (lb_vip->address_family == AF_INET) { >> > - ds_put_format(match, "ip4 && "REG_NEXT_HOP_IPV4" == %s", >> > - lb_vip->vip_str); >> > - } else { >> > - ds_put_format(match, "ip6 && "REG_NEXT_HOP_IPV6" == %s", >> > - lb_vip->vip_str); >> > - } >> > - >> > - if (lb->skip_snat) { >> > - 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;"); >> > - } >> > - >> > - int prio = 110; >> > - if (lb_vip->port_str) { >> > + ds_put_format(match, "ct.new && !ct.rel && %s && %s == %s", >> > + ip_match, ip_reg, lb_vip->vip_str); >> > + if (lb_vip->vip_port) { >> > prio = 120; >> > - new_match = xasprintf("ct.new && !ct.rel && %s && %s && " >> > - REG_ORIG_TP_DPORT_ROUTER" == %s", >> > - ds_cstr(match), lb->proto, >> lb_vip->port_str); >> > - est_match = xasprintf("ct.est && !ct.rel && %s && %s && " >> > - REG_ORIG_TP_DPORT_ROUTER" == %s && %s == >> 1", >> > - ds_cstr(match), lb->proto, >> lb_vip->port_str, >> > - ct_natted); >> > - } else { >> > - new_match = xasprintf("ct.new && !ct.rel && %s", >> ds_cstr(match)); >> > - est_match = xasprintf("ct.est && !ct.rel && %s && %s == 1", >> > - ds_cstr(match), ct_natted); >> > + ds_put_format(match, " && %s && "REG_ORIG_TP_DPORT_ROUTER" == >> %d", >> > + lb->proto, lb_vip->vip_port); >> > } >> > >> > - const char *ip_match = NULL; >> > - if (lb_vip->address_family == AF_INET) { >> > - ip_match = "ip4"; >> > - } else { >> > - ip_match = "ip6"; >> > - } >> > + ds_put_cstr(&est_match, "ct.est"); >> > + /* Clone the match after initial "ct.new" (6 bytes). */ >> > + ds_put_cstr(&est_match, ds_cstr(match) + 6); >> > + ds_put_format(&est_match, " && %s == 1", ct_natted); >> > >> > /* Add logical flows to UNDNAT the load balanced reverse traffic >> in >> > * the router egress pipleine stage - S_ROUTER_OUT_UNDNAT if the >> logical >> > * router has a gateway router port associated. >> > */ >> > - struct ds undnat_match = DS_EMPTY_INITIALIZER; >> > ds_put_format(&undnat_match, "%s && (", ip_match); >> > >> > for (size_t i = 0; i < lb_vip->n_backends; i++) { >> > @@ -10568,12 +10623,9 @@ build_lrouter_nat_flows_for_lb(struct >> ovn_lb_vip *lb_vip, >> > ds_put_cstr(&undnat_match, ") || "); >> > } >> > } >> > - ds_chomp(&undnat_match, ' '); >> > - ds_chomp(&undnat_match, '|'); >> > - ds_chomp(&undnat_match, '|'); >> > - ds_chomp(&undnat_match, ' '); >> > + /* Remove the trailing " || ". */ >> > + ds_truncate(&undnat_match, undnat_match.length - 4); >> > >> > - struct ds unsnat_match = DS_EMPTY_INITIALIZER; >> > ds_put_format(&unsnat_match, "%s && %s.dst == %s && %s", >> > ip_match, ip_match, lb_vip->vip_str, lb->proto); >> > if (lb_vip->port_str) { >> > @@ -10581,21 +10633,34 @@ build_lrouter_nat_flows_for_lb(struct >> ovn_lb_vip *lb_vip, >> > lb_vip->port_str); >> > } >> > >> > - 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 lrouter_nat_lb_flows_ctx ctx = { >> > + .lb_vip = lb_vip, >> > + .lb = lb, >> > + .reject = reject, >> > + .prio = prio, >> > + .lflows = lflows, >> > + .meter_groups = meter_groups, >> > + .new_match = match, >> > + .est_match = &est_match, >> > + .undnat_match = &undnat_match >> > + }; >> > >> > - struct ovn_datapath **gw_router_force_snat = >> > - xcalloc(lb->n_nb_lr, sizeof *gw_router_force_snat); >> > - int n_gw_router_force_snat = 0; >> > + ctx.new[LROUTER_NAT_LB_FLOW_NORMAL] = >> > + LROUTER_NAT_LB_FLOW_INIT(match, ds_cstr(action), prio); >> > + ctx.est[LROUTER_NAT_LB_FLOW_NORMAL] = >> > + LROUTER_NAT_LB_FLOW_INIT(&est_match, "next;", prio); >> > >> > - struct ovn_datapath **gw_router = >> > - xcalloc(lb->n_nb_lr, sizeof *gw_router); >> > - int n_gw_router = 0; >> > + ctx.new[LROUTER_NAT_LB_FLOW_SKIP_SNAT] = >> > + LROUTER_NAT_LB_FLOW_INIT(match, ds_cstr(&skip_snat_act), prio); >> > + ctx.est[LROUTER_NAT_LB_FLOW_SKIP_SNAT] = >> > + LROUTER_NAT_LB_FLOW_INIT(&est_match, >> > + "flags.skip_snat_for_lb = 1; next;", >> prio); >> > >> > - struct ovn_datapath **distributed_router = >> > - xcalloc(lb->n_nb_lr, sizeof *distributed_router); >> > - int n_distributed_router = 0; >> > + ctx.new[LROUTER_NAT_LB_FLOW_FORCE_SNAT] = >> > + LROUTER_NAT_LB_FLOW_INIT(match, ds_cstr(&force_snat_act), >> prio); >> > + ctx.est[LROUTER_NAT_LB_FLOW_FORCE_SNAT] = >> > + LROUTER_NAT_LB_FLOW_INIT(&est_match, >> > + "flags.force_snat_for_lb = 1; next;", >> prio); >> > >> > struct ovn_datapath **lb_aff_force_snat_router = >> > xcalloc(lb->n_nb_lr, sizeof *lb_aff_force_snat_router); >> > @@ -10609,18 +10674,22 @@ build_lrouter_nat_flows_for_lb(struct >> ovn_lb_vip *lb_vip, >> > * lflow generation for them. >> > */ >> > for (size_t i = 0; i < lb->n_nb_lr; i++) { >> > + enum lrouter_nat_lb_flow_type type; >> > struct ovn_datapath *od = lb->nb_lr[i]; >> > + >> > + if (lb->skip_snat) { >> > + type = LROUTER_NAT_LB_FLOW_SKIP_SNAT; >> > + } else if (!lport_addresses_is_empty(&od->lb_force_snat_addrs) >> || >> > + od->lb_force_snat_router_ip) { >> > + type = LROUTER_NAT_LB_FLOW_FORCE_SNAT; >> > + } else { >> > + type = LROUTER_NAT_LB_FLOW_NORMAL; >> > + } >> > + >> > if (!od->n_l3dgw_ports) { >> > - if (lb->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; >> > - } else { >> > - gw_router[n_gw_router++] = od; >> > - } >> > + build_gw_lrouter_nat_flows_for_lb(&ctx, type, od); >> > } else { >> > - distributed_router[n_distributed_router++] = od; >> > + build_distr_lrouter_nat_flows_for_lb(&ctx, type, od); >> > } >> > >> > if (!lport_addresses_is_empty(&od->lb_force_snat_addrs) || >> > @@ -10648,141 +10717,30 @@ build_lrouter_nat_flows_for_lb(struct >> ovn_lb_vip *lb_vip, >> > } >> > } >> > >> > - /* GW router logic */ >> > - 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); >> > - >> > - 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, >> > - "flags.force_snat_for_lb = 1; next;", >> > - lflows, prio, meter_groups); >> > - >> > /* LB affinity flows for datapaths where CMS has specified >> > * force_snat_for_lb floag option. >> > */ >> > - build_lb_affinity_lr_flows(lflows, lb, lb_vip, new_match, >> > + build_lb_affinity_lr_flows(lflows, lb, lb_vip, ds_cstr(match), >> > "flags.force_snat_for_lb = 1; ", >> > lb_aff_force_snat_router, >> > n_lb_aff_force_snat_router); >> > >> > - if (!drop) { >> > - ds_put_cstr(action, ");"); >> > - } >> > - >> > - build_gw_lrouter_nat_flows_for_lb(lb, gw_router, n_gw_router, >> > - reject, new_match, ds_cstr(action), est_match, >> > - "next;", lflows, prio, meter_groups); >> > - >> > /* LB affinity flows for datapaths where CMS has specified >> > * skip_snat_for_lb floag option or regular datapaths. >> > */ >> > char *lb_aff_action = >> > lb->skip_snat ? "flags.skip_snat_for_lb = 1; " : NULL; >> > - build_lb_affinity_lr_flows(lflows, lb, lb_vip, new_match, >> lb_aff_action, >> > - lb_aff_router, n_lb_aff_router); >> > - >> > - /* Distributed router logic */ >> > - for (size_t i = 0; i < n_distributed_router; i++) { >> > - struct ovn_datapath *od = distributed_router[i]; >> > - char *new_match_p = new_match; >> > - char *est_match_p = est_match; >> > - const char *meter = NULL; >> > - bool is_dp_lb_force_snat = >> > - !lport_addresses_is_empty(&od->lb_force_snat_addrs) || >> > - od->lb_force_snat_router_ip; >> > - >> > - if (reject) { >> > - meter = copp_meter_get(COPP_REJECT, od->nbr->copp, >> meter_groups); >> > - } >> > - >> > - if (lb_vip->n_backends || !lb_vip->empty_backend_rej) { >> > - new_match_p = xasprintf("%s && is_chassis_resident(%s)", >> > - new_match, >> > - >> od->l3dgw_ports[0]->cr_port->json_key); >> > - est_match_p = xasprintf("%s && is_chassis_resident(%s)", >> > - est_match, >> > - >> od->l3dgw_ports[0]->cr_port->json_key); >> > - } >> > - >> > - if (lb->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_); >> > - } else if (is_dp_lb_force_snat) { >> > - ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT, >> prio, >> > - new_match_p, new_actions, NULL, >> > - meter, &lb->nlb->header_); >> > - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, prio, >> > - est_match_p, >> > - "flags.force_snat_for_lb = 1; >> next;", >> > - &lb->nlb->header_); >> > - } else { >> > - ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT, >> prio, >> > - new_match_p, ds_cstr(action), >> NULL, >> > - meter, &lb->nlb->header_); >> > - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, prio, >> > - est_match_p, "next;", >> > - &lb->nlb->header_); >> > - } >> > - >> > - if (new_match_p != new_match) { >> > - free(new_match_p); >> > - } >> > - if (est_match_p != est_match) { >> > - free(est_match_p); >> > - } >> > - >> > - if (!lb_vip->n_backends) { >> > - continue; >> > - } >> > - >> > - char *undnat_match_p = xasprintf( >> > - "%s) && outport == %s && is_chassis_resident(%s)", >> > - ds_cstr(&undnat_match), >> > - od->l3dgw_ports[0]->json_key, >> > - od->l3dgw_ports[0]->cr_port->json_key); >> > - if (lb->skip_snat) { >> > - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, >> 120, >> > - undnat_match_p, >> skip_snat_est_action, >> > - &lb->nlb->header_); >> > - } else if (is_dp_lb_force_snat) { >> > - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, >> 120, >> > - undnat_match_p, >> > - "flags.force_snat_for_lb = 1; >> next;", >> > - &lb->nlb->header_); >> > - } else { >> > - ovn_lflow_add_with_hint( >> > - lflows, od, S_ROUTER_OUT_UNDNAT, 120, undnat_match_p, >> > - od->is_gw_router ? "ct_dnat;" : "ct_dnat_in_czone;", >> > - &lb->nlb->header_); >> > - } >> > - free(undnat_match_p); >> > - } >> > + build_lb_affinity_lr_flows(lflows, lb, lb_vip, ds_cstr(match), >> > + lb_aff_action, lb_aff_router, >> n_lb_aff_router); >> > >> > ds_destroy(&unsnat_match); >> > ds_destroy(&undnat_match); >> > + ds_destroy(&est_match); >> > + ds_destroy(&skip_snat_act); >> > + ds_destroy(&force_snat_act); >> > >> > - free(skip_snat_new_action); >> > - free(skip_snat_est_action); >> > - free(est_match); >> > - free(new_match); >> > - free(new_actions); >> > - >> > - free(gw_router_force_snat); >> > - free(gw_router_skip_snat); >> > - free(distributed_router); >> > free(lb_aff_force_snat_router); >> > free(lb_aff_router); >> > - free(gw_router); >> > } >> > >> > static void >> >> > Thank you Mark, there will be slight changes needed if we merge the ICMP > related feature flag first (which we should for backport convenience). > I will share the diff here before posting v4 so you or any other reviwer > don't have to go > over the whole v4. > > Thanks, > Ales > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > [email protected] IM: amusil > <https://red.ht/sig> > As promised the diff between v3 and v4: diff --git a/northd/northd.c b/northd/northd.c index 7d1425180..83d0d4872 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3858,9 +3867,10 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip, struct ovn_northd_lb_vip *lb_vip_nb, struct ds *action, char *selection_fields, struct ds *skip_snat_action, struct ds *force_snat_action, - bool ls_dp, bool ct_lb_mark) + bool ls_dp, const struct chassis_features *features) { - const char *ct_lb_action = ct_lb_mark ? "ct_lb_mark" : "ct_lb"; + const char *ct_lb_action = + features->ct_no_masked_label ? "ct_lb_mark" : "ct_lb"; bool reject = !lb_vip->n_backends && lb_vip->empty_backend_rej; bool drop = false; @@ -3908,16 +3918,17 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip, bool is_lb_action = !(reject || drop); if (!ls_dp) { + bool flag_supported = is_lb_action && features->ct_lb_related; ds_put_format(skip_snat_action, "flags.skip_snat_for_lb = 1; %s%s", - ds_cstr(action), is_lb_action ? "; skip_snat);" : ""); + ds_cstr(action), flag_supported ? "; skip_snat);" : ""); ds_put_format(force_snat_action, "flags.force_snat_for_lb = 1; %s%s", - ds_cstr(action), is_lb_action ? "; force_snat);" : ""); + ds_cstr(action), flag_supported ? "; force_snat);" : ""); } if (is_lb_action) { ds_put_cstr(action, ");"); } - return false; + return reject; } static void @@ -6791,14 +6802,17 @@ build_acls(struct ovn_datapath *od, const struct chassis_features *features, * a dynamically negotiated FTP data channel), but will allow * related traffic such as an ICMP Port Unreachable through * that's generated from a non-listening UDP port. */ + const char *ct_acl_action = features->ct_lb_related + ? "ct_commit_nat;" + : "next;"; ds_clear(&match); ds_put_format(&match, "!ct.est && ct.rel && !ct.new%s && %s == 0", use_ct_inv_match ? " && !ct.inv" : "", ct_blocked_match); ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX - 3, - ds_cstr(&match), "ct_commit_nat;"); + ds_cstr(&match), ct_acl_action); ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX - 3, - ds_cstr(&match), "ct_commit_nat;"); + ds_cstr(&match), ct_acl_action); /* Ingress and Egress ACL Table (Priority 65532). * @@ -7473,9 +7487,9 @@ build_lb_affinity_default_flows(struct ovn_datapath *od, struct hmap *lflows) } static void -build_lb_rules(struct hmap *lflows, struct ovn_northd_lb *lb, bool ct_lb_mark, - struct ds *match, struct ds *action, - const struct shash *meter_groups) +build_lb_rules(struct hmap *lflows, struct ovn_northd_lb *lb, + const struct chassis_features *features, struct ds *match, + struct ds *action, const struct shash *meter_groups) { for (size_t i = 0; i < lb->n_vips; i++) { struct ovn_lb_vip *lb_vip = &lb->vips[i]; @@ -7499,7 +7513,7 @@ build_lb_rules(struct hmap *lflows, struct ovn_northd_lb *lb, bool ct_lb_mark, const char *meter = NULL; bool reject = build_lb_vip_actions(lb_vip, lb_vip_nb, action, lb->selection_fields, NULL, - NULL, true, ct_lb_mark); + NULL, true, features); ds_put_format(match, "ct.new && %s.dst == %s", ip_match, lb_vip->vip_str); @@ -10563,9 +10577,11 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, struct hmap *lflows, struct ds *match, struct ds *action, const struct shash *meter_groups, - bool ct_lb_mark) + const struct chassis_features *features) { - const char *ct_natted = ct_lb_mark ? "ct_mark.natted" : "ct_label.natted"; + const char *ct_natted = features->ct_no_masked_label + ? "ct_mark.natted" + : "ct_label.natted"; bool ipv4 = lb_vip->address_family == AF_INET; const char *ip_match = ipv4 ? "ip4" : "ip6"; @@ -10584,7 +10600,7 @@ 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, &skip_snat_act, - &force_snat_act, false, ct_lb_mark); + &force_snat_act, false, features); /* Higher priority rules are added for load-balancing in DNAT * table. For every match (on a VIP[:port]), we add two flows. @@ -10785,8 +10801,7 @@ build_lswitch_flows_for_lb(struct ovn_northd_lb *lb, struct hmap *lflows, * REGBIT_CONNTRACK_COMMIT. */ build_lb_rules_pre_stateful(lflows, lb, features->ct_no_masked_label, match, action); - build_lb_rules(lflows, lb, features->ct_no_masked_label, - match, action, meter_groups); + build_lb_rules(lflows, lb, features, match, action, meter_groups); } /* If there are any load balancing rules, we should send the packet to @@ -10869,7 +10884,7 @@ build_lrouter_flows_for_lb(struct ovn_northd_lb *lb, struct hmap *lflows, build_lrouter_nat_flows_for_lb(lb_vip, lb, &lb->vips_nb[i], lflows, match, action, meter_groups, - features->ct_no_masked_label); + features); if (!build_empty_lb_event_flow(lb_vip, lb, match, action)) { continue; @@ -14179,7 +14194,7 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, struct hmap *lflows, const struct hmap *ports, struct ds *match, struct ds *actions, const struct shash *meter_groups, - bool ct_lb_mark) + const struct chassis_features *features) { if (!od->nbr) { return; @@ -14210,9 +14225,11 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, struct hmap *lflows, * a dynamically negotiated FTP data channel), but will allow * related traffic such as an ICMP Port Unreachable through * that's generated from a non-listening UDP port. */ - if (od->has_lb_vip) { + if (od->has_lb_vip && features->ct_lb_related) { ds_clear(match); - const char *ct_flag_reg = ct_lb_mark ? "ct_mark" : "ct_label"; + const char *ct_flag_reg = features->ct_no_masked_label + ? "ct_mark" + : "ct_label"; ds_put_cstr(match, "ct.rel && !ct.est && !ct.new"); size_t match_len = match->length; @@ -14435,7 +14452,7 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, struct hmap *lflows, if (od->nbr->n_nat) { ds_clear(match); - const char *ct_natted = ct_lb_mark ? + const char *ct_natted = features->ct_no_masked_label ? "ct_mark.natted" : "ct_label.natted"; ds_put_format(match, "ip && %s == 1", ct_natted); @@ -14552,7 +14569,7 @@ build_lswitch_and_lrouter_iterate_by_od(struct ovn_datapath *od, build_lrouter_arp_nd_for_datapath(od, lsi->lflows, lsi->meter_groups); build_lrouter_nat_defrag_and_lb(od, lsi->lflows, lsi->ports, &lsi->match, &lsi->actions, lsi->meter_groups, - lsi->features->ct_no_masked_label); + lsi->features); build_lb_affinity_default_flows(od, lsi->lflows); } 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
