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?
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
