On 30.10.2025 23:43, Mark Michelson wrote: Hi Alexandra,
HI Mark, thaks for review. I'm responding to the entire series here, rather than crafting individual replies to each patch. First, I have a couple of high level findings/questions. Looking at this, my assumption is that something outside of OVN is responsible for distributing the traffic to the logical routers that have the distributed load balancer. At the moment, yes. In the future, I'd like to complete this patch using native BGP support in OVN. This hasn't been done yet because I don't yet understand how the current BGP support can be conveniently distributed. The ct_lb_local_mark() action gets installed on all chassis. It's then up to ovn-controller to populate the OpenFlow group with only the local backends. What happens with a distributed load balancer if a chassis does not have any load balancer backends present? Nothing will be added to the openflow group. The code contains a check for the existence of backends on the node. In patch 3, you wrote: "If the balancer is running on a router with dgp, the router will no longer be centralized." Does this only apply to load balancer flows? How does this affect non-load balancing features that use conntrack. Does SNAT/DNAT still happen only on the gateway hv? Sorry, that's not what i meant, ll correct that. The rules for ARP/ND processing for DGP have become distributed, and for load balancing(dnat/snat), noting more. And finally, I have a note for this particular patch. Since this test is adding a new action, this should add new test cases to the "action parsing" test in ovn.at. oh, sorry, yeah, I forgot about that, I will resend the patch with the added test. On Tue, Oct 21, 2025 at 10:31 AM Alexandra Rukomoinikova <[email protected]><mailto:[email protected]> wrote: This commit introduces a new `ct_lb_mark_local` action that filters backends based on local chassis bindings for load balancing. By forming OpenFlow groups for balancing only with local backends. In the future, this will be used to implement distributed balancers. Suggested-by: Vladislav Odintsov <[email protected]><mailto:[email protected]> Signed-off-by: Alexandra Rukomoinikova <[email protected]><mailto:[email protected]> --- controller/lflow.c | 17 ++++ controller/lflow.h | 1 + controller/ovn-controller.c | 1 + include/ovn/actions.h | 11 +++ lib/actions.c | 149 ++++++++++++++++++++++++++++++------ lib/ovn-util.c | 2 +- utilities/ovn-trace.c | 2 + 7 files changed, 158 insertions(+), 25 deletions(-) diff --git a/controller/lflow.c b/controller/lflow.c index e84fb2486..9979508eb 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -64,6 +64,7 @@ struct lookup_port_aux { const struct sbrec_logical_flow *lflow; struct objdep_mgr *deps_mgr; const struct hmap *chassis_tunnels; + const struct shash *local_bindings; }; struct condition_aux { @@ -153,6 +154,20 @@ lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp) return false; } +/* Given the OVN port name, get true if port locates on local chassis, + * false otherwise. */ +static bool +lookup_local_port_cb(const void *aux_, const char *port_name) +{ + const struct lookup_port_aux *aux = aux_; + + if (local_binding_get_primary_pb(aux->local_bindings, port_name)) { + return true; + } + + return false; +} + /* Given the OVN port name, get its openflow port */ static bool tunnel_ofport_cb(const void *aux_, const char *port_name, ofp_port_t *ofport) @@ -857,6 +872,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, .lflow = lflow, .deps_mgr = l_ctx_out->lflow_deps_mgr, .chassis_tunnels = l_ctx_in->chassis_tunnels, + .local_bindings = l_ctx_in->lbinding_lports, }; /* Parse any meter to be used if this flow should punt packets to @@ -871,6 +887,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub); struct ovnact_encode_params ep = { .lookup_port = lookup_port_cb, + .lookup_local_port = lookup_local_port_cb, .tunnel_ofport = tunnel_ofport_cb, .aux = &aux, .is_switch = ldp->is_switch, diff --git a/controller/lflow.h b/controller/lflow.h index d82acc0d8..0cde1ebf4 100644 --- a/controller/lflow.h +++ b/controller/lflow.h @@ -138,6 +138,7 @@ struct lflow_ctx_in { const struct smap *template_vars; const struct flow_collector_ids *collector_ids; const struct hmap *local_lbs; + const struct shash *lbinding_lports; bool localnet_learn_fdb; bool localnet_learn_fdb_changed; bool explicit_arp_ns_output; diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 6fbf3a227..a76c77d49 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -3925,6 +3925,7 @@ init_lflow_ctx(struct engine_node *node, l_ctx_in->template_vars = &template_vars->local_templates; l_ctx_in->collector_ids = &fo->collector_ids; l_ctx_in->local_lbs = &lb_data->local_lbs; + l_ctx_in->lbinding_lports = &rt_data->lbinding_data.bindings; l_ctx_out->flow_table = &fo->flow_table; l_ctx_out->group_table = &fo->group_table; diff --git a/include/ovn/actions.h b/include/ovn/actions.h index d3f0bfd04..2ca8dac8f 100644 --- a/include/ovn/actions.h +++ b/include/ovn/actions.h @@ -75,6 +75,7 @@ struct collector_set_ids; OVNACT(CT_SNAT_IN_CZONE, ovnact_ct_nat) \ OVNACT(CT_LB, ovnact_ct_lb) \ OVNACT(CT_LB_MARK, ovnact_ct_lb) \ + OVNACT(CT_LB_MARK_LOCAL, ovnact_ct_lb) \ OVNACT(SELECT, ovnact_select) \ OVNACT(CT_CLEAR, ovnact_null) \ OVNACT(CT_COMMIT_NAT, ovnact_ct_commit_to_zone) \ @@ -311,6 +312,12 @@ struct ovnact_ct_commit_to_zone { uint8_t ltable; }; +enum ovnact_ct_lb_type { + OVNACT_CT_LB_TYPE_LABEL, + OVNACT_CT_LB_TYPE_MARK, + OVNACT_CT_LB_LOCAL_TYPE_MARK, +}; + enum ovnact_ct_lb_flag { OVNACT_CT_LB_FLAG_NONE, OVNACT_CT_LB_FLAG_SKIP_SNAT, @@ -324,6 +331,7 @@ struct ovnact_ct_lb_dst { ovs_be32 ipv4; }; uint16_t port; + char *port_name; }; /* OVNACT_CT_LB/OVNACT_CT_LB_MARK. */ @@ -891,6 +899,9 @@ struct ovnact_encode_params { bool (*lookup_port)(const void *aux, const char *port_name, unsigned int *portp); + /* Check if the logical port is bound to this chassis. */ + bool (*lookup_local_port)(const void *aux, const char *port_name); + /* Looks up tunnel port to a chassis by its port name. If found, stores * its openflow port number in '*ofport' and returns true; * otherwise, returns false. */ diff --git a/lib/actions.c b/lib/actions.c index 53f4d20a9..21ab4d83b 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -1187,8 +1187,25 @@ ovnact_ct_commit_to_zone_free(struct ovnact_ct_commit_to_zone *cn OVS_UNUSED) { } + +static bool +parse_ct_lb_logical_port_name(struct action_context *ctx, + struct ovnact_ct_lb_dst *dst) +{ + if (ctx->lexer->token.type != LEX_T_STRING) { + return false; + } + + dst->port_name = xstrdup(ctx->lexer->token.s); + + lexer_get(ctx->lexer); + + return true; +} + static void -parse_ct_lb_action(struct action_context *ctx, bool ct_lb_mark) +parse_ct_lb_action(struct action_context *ctx, + enum ovnact_ct_lb_type type) { if (ctx->pp->cur_ltable >= ctx->pp->n_tables) { lexer_error(ctx->lexer, "\"ct_lb\" action not allowed in last table."); @@ -1211,7 +1228,20 @@ parse_ct_lb_action(struct action_context *ctx, bool ct_lb_mark) while (!lexer_match(ctx->lexer, LEX_T_SEMICOLON) && !lexer_match(ctx->lexer, LEX_T_RPAREN)) { - struct ovnact_ct_lb_dst dst; + struct ovnact_ct_lb_dst dst = {0}; + + if (type == OVNACT_CT_LB_LOCAL_TYPE_MARK) { + + if (!parse_ct_lb_logical_port_name(ctx, &dst)) { + vector_destroy(&dsts); + lexer_syntax_error(ctx->lexer, + "expecting logicl port name " + "for distributed load balancer"); + return; + } + lexer_get(ctx->lexer); + } + if (lexer_match(ctx->lexer, LEX_T_LSQUARE)) { /* IPv6 address and port */ if (ctx->lexer->token.type != LEX_T_INTEGER @@ -1298,8 +1328,19 @@ parse_ct_lb_action(struct action_context *ctx, bool ct_lb_mark) } } - struct ovnact_ct_lb *cl = ct_lb_mark ? ovnact_put_CT_LB_MARK(ctx->ovnacts) - : ovnact_put_CT_LB(ctx->ovnacts); + struct ovnact_ct_lb *cl; + switch (type) { + case OVNACT_CT_LB_TYPE_LABEL: + cl = ovnact_put_CT_LB(ctx->ovnacts); + break; + case OVNACT_CT_LB_TYPE_MARK: + cl = ovnact_put_CT_LB_MARK(ctx->ovnacts); + break; + case OVNACT_CT_LB_LOCAL_TYPE_MARK: + cl = ovnact_put_CT_LB_MARK_LOCAL(ctx->ovnacts); + break; + } + cl->ltable = ctx->pp->cur_ltable + 1; cl->n_dsts = vector_len(&dsts); cl->dsts = vector_steal_array(&dsts); @@ -1308,13 +1349,16 @@ parse_ct_lb_action(struct action_context *ctx, bool ct_lb_mark) } static void -format_ct_lb(const struct ovnact_ct_lb *cl, struct ds *s, bool ct_lb_mark) +format_ct_lb(const struct ovnact_ct_lb *cl, struct ds *s, + enum ovnact_ct_lb_type type) { - if (ct_lb_mark) { - ds_put_cstr(s, "ct_lb_mark"); - } else { - ds_put_cstr(s, "ct_lb"); - } + static const char *const lb_action_strings[] = { + [OVNACT_CT_LB_TYPE_LABEL] = "ct_lb", + [OVNACT_CT_LB_TYPE_MARK] = "ct_lb_mark", + [OVNACT_CT_LB_LOCAL_TYPE_MARK] = "ct_lb_mark_local", + }; + ds_put_cstr(s, lb_action_strings[type]); + if (cl->n_dsts) { ds_put_cstr(s, "(backends="); for (size_t i = 0; i < cl->n_dsts; i++) { @@ -1337,6 +1381,9 @@ format_ct_lb(const struct ovnact_ct_lb *cl, struct ds *s, bool ct_lb_mark) ds_put_format(s, "]:%"PRIu16, dst->port); } } + if (type == OVNACT_CT_LB_LOCAL_TYPE_MARK) { + ds_put_format(s, ":%s", dst->port_name); + } } if (cl->hash_fields) { @@ -1363,20 +1410,36 @@ format_ct_lb(const struct ovnact_ct_lb *cl, struct ds *s, bool ct_lb_mark) static void format_CT_LB(const struct ovnact_ct_lb *cl, struct ds *s) { - format_ct_lb(cl, s, false); + format_ct_lb(cl, s, OVNACT_CT_LB_TYPE_LABEL); } static void format_CT_LB_MARK(const struct ovnact_ct_lb *cl, struct ds *s) { - format_ct_lb(cl, s, true); + format_ct_lb(cl, s, OVNACT_CT_LB_TYPE_MARK); +} + +static void +format_CT_LB_MARK_LOCAL(const struct ovnact_ct_lb *cl, struct ds *s) +{ + format_ct_lb(cl, s, OVNACT_CT_LB_LOCAL_TYPE_MARK); +} + +static inline void +append_nat_destination(struct ds *ds, const char *ip_addr, + bool needs_brackets) +{ + ds_put_format(ds, "ct(nat(dst=%s%s%s", + needs_brackets ? "[" : "", + ip_addr, + needs_brackets ? "]" : ""); } static void encode_ct_lb(const struct ovnact_ct_lb *cl, const struct ovnact_encode_params *ep, struct ofpbuf *ofpacts, - bool ct_lb_mark) + enum ovnact_ct_lb_type type) { uint8_t recirc_table = cl->ltable + first_ptable(ep, ep->pipeline); if (!cl->n_dsts) { @@ -1408,7 +1471,8 @@ encode_ct_lb(const struct ovnact_ct_lb *cl, struct ofpact_group *og; uint32_t zone_reg = ep->is_switch ? MFF_LOG_CT_ZONE - MFF_REG0 : MFF_LOG_DNAT_ZONE - MFF_REG0; - const char *flag_reg = ct_lb_mark ? "ct_mark" : "ct_label"; + const char *flag_reg = (type == OVNACT_CT_LB_TYPE_LABEL) + ? "ct_label" : "ct_mark"; const char *ct_flag_value; switch (cl->ct_flag) { @@ -1435,32 +1499,50 @@ encode_ct_lb(const struct ovnact_ct_lb *cl, BUILD_ASSERT(MFF_LOG_CT_ZONE < MFF_REG0 + FLOW_N_REGS); BUILD_ASSERT(MFF_LOG_DNAT_ZONE >= MFF_REG0); BUILD_ASSERT(MFF_LOG_DNAT_ZONE < MFF_REG0 + FLOW_N_REGS); + + size_t n_active_backends = 0; for (size_t bucket_id = 0; bucket_id < cl->n_dsts; bucket_id++) { const struct ovnact_ct_lb_dst *dst = &cl->dsts[bucket_id]; char ip_addr[INET6_ADDRSTRLEN]; + if (dst->family == AF_INET) { inet_ntop(AF_INET, &dst->ipv4, ip_addr, sizeof ip_addr); } else { inet_ntop(AF_INET6, &dst->ipv6, ip_addr, sizeof ip_addr); } - ds_put_format(&ds, ",bucket=bucket_id=%"PRIuSIZE",weight:100,actions=" - "ct(nat(dst=%s%s%s", bucket_id, - dst->family == AF_INET6 && dst->port ? "[" : "", - ip_addr, - dst->family == AF_INET6 && dst->port ? "]" : ""); + + if (type == OVNACT_CT_LB_LOCAL_TYPE_MARK + && !ep->lookup_local_port(ep->aux, dst->port_name)) { + continue; + } + + ds_put_format(&ds, ",bucket=bucket_id=%"PRIuSIZE",weight:100,actions=", + bucket_id); + + bool needs_brackets = (dst->family == AF_INET6 && dst->port); + append_nat_destination(&ds, ip_addr, needs_brackets); + if (dst->port) { ds_put_format(&ds, ":%"PRIu16, dst->port); } + ds_put_format(&ds, "),commit,table=%d,zone=NXM_NX_REG%d[0..15]," "exec(set_field:" OVN_CT_MASKED_STR(OVN_CT_NATTED) "->%s", recirc_table, zone_reg, flag_reg); + if (ct_flag_value) { ds_put_format(&ds, ",set_field:%s->%s", ct_flag_value, flag_reg); } ds_put_cstr(&ds, "))"); + + n_active_backends++; + } + + if (!n_active_backends) { + return; } table_id = ovn_extend_table_assign_id(ep->group_table, ds_cstr(&ds), @@ -1480,7 +1562,7 @@ encode_CT_LB(const struct ovnact_ct_lb *cl, const struct ovnact_encode_params *ep, struct ofpbuf *ofpacts) { - encode_ct_lb(cl, ep, ofpacts, false); + encode_ct_lb(cl, ep, ofpacts, OVNACT_CT_LB_TYPE_LABEL); } static void @@ -1488,13 +1570,30 @@ encode_CT_LB_MARK(const struct ovnact_ct_lb *cl, const struct ovnact_encode_params *ep, struct ofpbuf *ofpacts) { - encode_ct_lb(cl, ep, ofpacts, true); + encode_ct_lb(cl, ep, ofpacts, OVNACT_CT_LB_TYPE_MARK); } static void -ovnact_ct_lb_free(struct ovnact_ct_lb *ct_lb) +encode_CT_LB_MARK_LOCAL(const struct ovnact_ct_lb *cl, + const struct ovnact_encode_params *ep, + struct ofpbuf *ofpacts) +{ + encode_ct_lb(cl, ep, ofpacts, OVNACT_CT_LB_LOCAL_TYPE_MARK); +} + +static void +ovnact_ct_lb_free_dsts(struct ovnact_ct_lb *ct_lb) { + for (size_t i = 0; i < ct_lb->n_dsts; i++) { + free(ct_lb->dsts[i].port_name); + } free(ct_lb->dsts); +} + +static void +ovnact_ct_lb_free(struct ovnact_ct_lb *ct_lb) +{ + ovnact_ct_lb_free_dsts(ct_lb); free(ct_lb->hash_fields); } @@ -5904,9 +6003,11 @@ parse_action(struct action_context *ctx) } else if (lexer_match_id(ctx->lexer, "ct_lb")) { VLOG_WARN_RL(&rl, "The \"ct_lb\" action is deprecated please " "consider using a different action."); - parse_ct_lb_action(ctx, false); + parse_ct_lb_action(ctx, OVNACT_CT_LB_TYPE_LABEL); } else if (lexer_match_id(ctx->lexer, "ct_lb_mark")) { - parse_ct_lb_action(ctx, true); + parse_ct_lb_action(ctx, OVNACT_CT_LB_TYPE_MARK); + } else if (lexer_match_id(ctx->lexer, "ct_lb_mark_local")) { + parse_ct_lb_action(ctx, OVNACT_CT_LB_LOCAL_TYPE_MARK); } else if (lexer_match_id(ctx->lexer, "ct_clear")) { ovnact_put_CT_CLEAR(ctx->ovnacts); } else if (lexer_match_id(ctx->lexer, "ct_commit_nat")) { diff --git a/lib/ovn-util.c b/lib/ovn-util.c index 910f67304..369484c98 100644 --- a/lib/ovn-util.c +++ b/lib/ovn-util.c @@ -921,7 +921,7 @@ ip_address_and_port_from_lb_key(const char *key, char **ip_address, * * NOTE: If OVN_NORTHD_PIPELINE_CSUM is updated make sure to double check * whether an update of OVN_INTERNAL_MINOR_VER is required. */ -#define OVN_NORTHD_PIPELINE_CSUM "<mailto:)){diff--gita/lib/ovn-util.cb/lib/ovn-util.cindex910f67304..369484c98100644---a/lib/ovn-util.c+++b/lib/ovn-util.c@@-921,7+921,7@@ip_address_and_port_from_lb_key(constchar*key,char**ip_address,**NOTE:IfOVN_NORTHD_PIPELINE_CSUMisupdatedmakesuretodoublecheck*whetheranupdateofOVN_INTERNAL_MINOR_VERisrequired.*/-#defineOVN_NORTHD_PIPELINE_CSUM>2164662054 10958" +#define OVN_NORTHD_PIPELINE_CSUM "4239189964 11014" #define OVN_INTERNAL_MINOR_VER 11 /* Returns the OVN version. The caller must free the returned value. */ diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c index 9d9f915da..f288c8a8f 100644 --- a/utilities/ovn-trace.c +++ b/utilities/ovn-trace.c @@ -3590,6 +3590,8 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len, case OVNACT_CT_STATE_SAVE: execute_ct_save_state(ovnact_get_CT_STATE_SAVE(a), uflow, super); break; + case OVNACT_CT_LB_MARK_LOCAL: + break; } } ofpbuf_uninit(&stack); -- 2.48.1 _______________________________________________ dev mailing list [email protected]<mailto:[email protected]> https://mail.openvswitch.org/mailman/listinfo/ovs-dev -- regards, Alexandra. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
