On Fri, Feb 07, 2025 at 03:45:19PM +0100, Dumitru Ceara wrote: > On 2/6/25 3:19 PM, Felix Huettner via dev wrote: > > Previously all routes of a logical router where announced. However in > > some cases it makes more sense to only announce static or connected > > routes. Therefor we add options to LR and LRP to define which routes to > > advertise. > > > > Acked-by: Lorenzo Bianconi <[email protected]> > > Signed-off-by: Felix Huettner <[email protected]> > > --- > > Hi Felix, > > > v6->v7: > > * addressed review comments > > v5->v6: > > * addressed review comments > > * changed option to "dynamic-routing-redistribute" > > v4->v5: skipped > > v2->v3: > > * A lot of minor review comments. > > * Reworked NEWs entry to make default more clear > > > > NEWS | 8 ++- > > northd/en-advertised-route-sync.c | 10 ++++ > > northd/northd.c | 41 +++++++++++++++ > > northd/northd.h | 18 +++++++ > > ovn-nb.xml | 51 ++++++++++++++++++ > > tests/ovn-northd.at | 87 ++++++++++++++++++++++++++++++- > > 6 files changed, 211 insertions(+), 4 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index dd17a579b..94a0a558c 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -42,9 +42,13 @@ Post v24.09.0 > > options:requested-chassis for router ports; if the chassis is remote > > then the router port will behave as a remote port. > > - Dynamic Routing: > > - * Add the option "dynamic-routing" to Logical Routers. If set to true > > all > > - static and connected routes attached to the router are shared to the > > + * Add the option "dynamic-routing" to Logical Routers. If set to true > > + static and connected routes matching the below filter are shared to > > the > > southbound "Advertised_Route" table for sharing outside of OVN. > > + The routes can further be configured by setting > > + `dynamic-routing-redistribute` on the LR or LRP. The LRP settings > > + overwrite the LR settings for all routes using this interface to > > + forward traffic on. > > > > OVN v24.09.0 - 13 Sep 2024 > > -------------------------- > > diff --git a/northd/en-advertised-route-sync.c > > b/northd/en-advertised-route-sync.c > > index 3c5065154..0fa91f5aa 100644 > > --- a/northd/en-advertised-route-sync.c > > +++ b/northd/en-advertised-route-sync.c > > @@ -149,6 +149,16 @@ advertised_route_table_sync( > > continue; > > } > > > > + enum dynamic_routing_redistribute_mode drr = > > + route->out_port->dynamic_routing_redistribute; > > + if (route->source == ROUTE_SOURCE_CONNECTED && > > + (drr & DRRM_CONNECTED) == 0) { > > If we go for the "magic" suggestion I'm mentioning below this could become: > > !drr_mode_CONNECTED_is_set(drr) > > > + continue; > > + } > > + if (route->source == ROUTE_SOURCE_STATIC && (drr & DRRM_STATIC) == > > 0) { > > + continue; > > + } > > + > > char *ip_prefix = normalize_v46_prefix(&route->prefix, > > route->plen); > > route_e = ar_add_entry(&sync_routes, route->od->sb, > > route->out_port->sb, ip_prefix); > > diff --git a/northd/northd.c b/northd/northd.c > > index ce39cee6c..3ba0e69c2 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -801,6 +801,43 @@ ovn_datapath_update_external_ids(struct ovn_datapath > > *od) > > smap_destroy(&ids); > > } > > > > +static enum dynamic_routing_redistribute_mode > > +parse_dynamic_routing_redistribute( > > + const struct smap *options, > > + const enum dynamic_routing_redistribute_mode default_dynamic_mode) > > No need for const enum. Arguments are passed by value. > > > +{ > > + char *save_ptr = NULL; > > + enum dynamic_routing_redistribute_mode out = DRRM_NONE; > > + > > + const char *dynamic_routing_redistribute = smap_get( > > + options, "dynamic-routing-redistribute"); > > + if (!dynamic_routing_redistribute) { > > + return default_dynamic_mode; > > + } > > + > > + char *tokstr = xstrdup(dynamic_routing_redistribute); > > + > > + for (char *token = strtok_r(tokstr, ";", &save_ptr); > > + token != NULL; > > + token = strtok_r(NULL, ";", &save_ptr)) { > > + > > + if (!strcmp(token, "connected")) { > > + out |= DRRM_CONNECTED; > > + continue; > > + } > > + if (!strcmp(token, "static")) { > > + out |= DRRM_STATIC; > > + continue; > > + } > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > > + VLOG_WARN_RL(&rl, "unkown dynamic-routing-redistribute option > > '%s'", > > + token); > > + } > > + > > + free(tokstr); > > + return out; > > +} > > + > > static void > > join_datapaths(const struct nbrec_logical_switch_table *nbrec_ls_table, > > const struct nbrec_logical_router_table *nbrec_lr_table, > > @@ -896,6 +933,8 @@ join_datapaths(const struct nbrec_logical_switch_table > > *nbrec_ls_table, > > } > > od->dynamic_routing = smap_get_bool(&od->nbr->options, > > "dynamic-routing", false); > > + od->dynamic_routing_redistribute = > > parse_dynamic_routing_redistribute( > > + &od->nbr->options, DRRM_NONE); > > ovs_list_push_back(lr_list, &od->lr_list); > > } > > } > > @@ -2233,6 +2272,8 @@ join_logical_ports_lrp(struct hmap *ports, > > > > op->prefix_delegation = smap_get_bool(&op->nbrp->options, > > "prefix_delegation", false); > > + op->dynamic_routing_redistribute = parse_dynamic_routing_redistribute( > > + &op->nbrp->options, od->dynamic_routing_redistribute); > > > > for (size_t j = 0; j < op->lrp_networks.n_ipv4_addrs; j++) { > > sset_add(&op->od->router_ips, > > diff --git a/northd/northd.h b/northd/northd.h > > index 7d0cfa9ef..06314730d 100644 > > --- a/northd/northd.h > > +++ b/northd/northd.h > > @@ -300,6 +300,17 @@ struct mcast_port_info { > > * (e.g., IGMP join/leave). */ > > }; > > > > +enum dynamic_routing_redistribute_mode_bits { > > + DRRM_CONNECTED_BIT = 0, > > + DRRM_STATIC_BIT = 1, > > +}; > > + > > +enum dynamic_routing_redistribute_mode { > > + DRRM_NONE = 0, > > + DRRM_CONNECTED = (1 << DRRM_CONNECTED_BIT), > > + DRRM_STATIC = (1 << DRRM_STATIC_BIT), > > +}; > > We could use some "magic" here: > > #define DRR_MODES \ > DRR_MODE(CONNECTED, 0) \ > DRR_MODE(STATIC, 1) > > enum dynamic_routing_redistribute_mode_bits { > #define DRR_MODE(PROTOCOL, BIT) DRRM_##PROTOCOL##_BIT = BIT, > DRR_MODES > #undef DRR_MODE > }; > > enum dynamic_routing_redistribute_mode { > DRRM_NONE = 0, > #define DRR_MODE(PROTOCOL, BIT) DRRM_##PROTOCOL = (1 << > DRRM_##PROTOCOL##_BIT), > DRR_MODES > #undef DRR_MODE > }; > > #define DRR_MODE(PROTOCOL, BIT) \ > static inline bool drr_mode_##PROTOCOL##_is_set( \ > enum dynamic_routing_redistribute_mode value) \ > { \ > return !!(value & DRRM_##PROTOCOL); \ > } > DRR_MODES > #undef DRR_MODE > > This generates: > > enum dynamic_routing_redistribute_mode_bits { > DRRM_CONNECTED_BIT = 0, > DRRM_STATIC_BIT = 1, > }; > > enum dynamic_routing_redistribute_mode { > DRRM_NONE = 0, > DRRM_CONNECTED = (1 << DRRM_CONNECTED_BIT), > DRRM_STATIC = (1 << DRRM_STATIC_BIT), > }; > > But also helpers: > > static inline bool drr_mode_CONNECTED_is_set(value) > static inline bool drr_mode_STATIC_is_set(value) > > What do you think?
Hi Dumitru, thanks a lot. Magic is a nice wording for this :) I'll add it in v8 and also address the others above. Thanks a lot, Felix > > Regards, > Dumitru > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
