On 30.10.2025 23:43, Mark Michelson wrote: > Hi Alexandra, HI Mark, thanks for review, my last letter came out strange =) > 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]> 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]> >> Signed-off-by: Alexandra Rukomoinikova <[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 "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] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> -- regards, Alexandra. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
