On Tue, Feb 11, 2025 at 12:11:57PM +0100, Dumitru Ceara wrote:
> + Felix (explicitly)
>
> On 2/11/25 11:22 AM, Dumitru Ceara wrote:
> > On 2/11/25 9:35 AM, 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,
> >
> >> v7->v8:
> >> * rework header file based on suggestions by Dumitru
> >> 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 | 11 ++++
> >> northd/northd.c | 41 +++++++++++++++
> >> northd/northd.h | 33 ++++++++++++
> >> ovn-nb.xml | 51 ++++++++++++++++++
> >> tests/ovn-northd.at | 87 ++++++++++++++++++++++++++++++-
> >> 6 files changed, 227 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/NEWS b/NEWS
> >> index 8c1c5c863..94358417a 100644
> >> --- a/NEWS
> >> +++ b/NEWS
> >> @@ -55,9 +55,13 @@ Post v24.09.0
> >> - Added vxlan_mode parameter in IC_NB_GLOBAL option column to enable or
> >> disable VXLAN encapsulation type in OVN-interconnect mode.
> >> - 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..7da76491d 100644
> >> --- a/northd/en-advertised-route-sync.c
> >> +++ b/northd/en-advertised-route-sync.c
> >> @@ -149,6 +149,17 @@ advertised_route_table_sync(
> >> continue;
> >> }
> >>
> >> + enum dynamic_routing_redistribute_mode drr =
> >> + route->out_port->dynamic_routing_redistribute;
> >> + if (route->source == ROUTE_SOURCE_CONNECTED &&
> >> + !drr_mode_CONNECTED_is_set(drr)) {
> >> + continue;
> >> + }
> >> + if (route->source == ROUTE_SOURCE_STATIC &&
> >> + !drr_mode_STATIC_is_set(drr)) {
> >> + 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 261a02e17..087332d05 100644
> >> --- a/northd/northd.c
> >> +++ b/northd/northd.c
> >> @@ -812,6 +812,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,
> >> + enum dynamic_routing_redistribute_mode default_dynamic_mode)
> >> +{
> >> + 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)) {
> >> +
> >
> > I didn't go through all the patches in this version yet but I'm assuming
> > it's probably ready to be pushed. I would however like to make a change
> > when applying it. Would you mind if I change the separator to ","
> > instead of ";"? Like that it would be more in sync with the
> > dynamic-routing-port-mapping option format you're adding in patch 12/16.
> > That one doesn't support ";" as separator.
> >
> > Please let me know what you think.
Hi Dumitru,
yes that makes a lot of sense.
It will then also need to be changed on ovn-nb.xml and the testcases.
Thanks a lot,
Felix
> >
> > I'll review the rest of the series in the meantime.
> >
> > Thanks,
> > Dumitru
> >
> >> + 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,
> >> @@ -912,6 +949,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);
> >> }
> >> }
> >> @@ -2264,6 +2303,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 9aa0934fa..d63b8013a 100644
> >> --- a/northd/northd.h
> >> +++ b/northd/northd.h
> >> @@ -307,6 +307,32 @@ struct mcast_port_info {
> >> * (e.g., IGMP join/leave). */
> >> };
> >>
> >> +#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
> >> +
> >> /* The 'key' comes from nbs->header_.uuid or nbr->header_.uuid or
> >> * sb->external_ids:logical-switch. */
> >> struct ovn_datapath {
> >> @@ -373,6 +399,8 @@ struct ovn_datapath {
> >> bool redirect_bridged;
> >> /* nbr has the option "dynamic-routing" set to true. */
> >> bool dynamic_routing;
> >> + /* The modes contained in the nbr option
> >> "dynamic-routing-redistribute". */
> >> + enum dynamic_routing_redistribute_mode dynamic_routing_redistribute;
> >>
> >> struct ovn_port **localnet_ports;
> >> size_t n_localnet_ports;
> >> @@ -628,6 +656,11 @@ struct ovn_port {
> >> struct lport_addresses lrp_networks;
> >> bool prefix_delegation; /* True if IPv6 prefix delegation enabled. */
> >>
> >> + /* The modes contained in the nbrp option
> >> "dynamic-routing-redistribute".
> >> + * If the option is unset it will be initialized based on the nbr
> >> + * option. */
> >> + enum dynamic_routing_redistribute_mode dynamic_routing_redistribute;
> >> +
> >> /* Logical port multicast data. */
> >> struct mcast_port_info mcast_info;
> >>
> >> diff --git a/ovn-nb.xml b/ovn-nb.xml
> >> index 2a178ab06..ae5e483d1 100644
> >> --- a/ovn-nb.xml
> >> +++ b/ovn-nb.xml
> >> @@ -3041,6 +3041,36 @@ or
> >> applied to this Logical Router
> >> </li>
> >> </ul>
> >> +
> >> + Users will need to use the following settings to opt into
> >> individual
> >> + route types that should be advertised. See:
> >> + <ul>
> >> + <li><ref column="options" key="dynamic-routing-redistribute"
> >> + table="Logical_Router"/></li>
> >> + <li><ref column="options" key="dynamic-routing-redistribute"
> >> + table="Logical_Router_Port"/></li>
> >> + </ul>
> >> + </column>
> >> +
> >> + <column name="options" key="dynamic-routing-redistribute"
> >> + type='{"type": "string"}'>
> >> + Only relevant if <ref column="options" key="dynamic-routing"
> >> + table="Logical_Router"/> is set to <code>true</code>.
> >> +
> >> + This is a list of elements separated by <code>;</code>.
> >> +
> >> + If <code>connected</code> is in the list then northd will
> >> synchronize
> >> + all "connected" routes to the southbound <ref table="Route"
> >> + db="OVN_SB"/> table. "Connected" here means routes implicitly
> >> created
> >> + by networks associated with the LRPs.
> >> +
> >> + If <code>static</code> is in the list then northd will
> >> synchronize all
> >> + <ref table="Logical_Router_Static_Route"/> to the southbound
> >> + <ref table="Route" db="OVN_SB"/> table.
> >> +
> >> + This value can be overwritten on a per LRP basis using
> >> + <ref column="options" key="dynamic-routing-redistribute"
> >> + table="Logical_Router_Port"/>.
> >> </column>
> >> </group>
> >>
> >> @@ -3808,7 +3838,28 @@ or
> >> status of <code>Transit Router</code> see
> >> <ref table="Logical_Router"/> table for more details.
> >> </p>
> >> + </column>
> >>
> >> + <column name="options" key="dynamic-routing-redistribute"
> >> + type='{"type": "string"}'>
> >> + Only relevant if <ref column="options" key="dynamic-routing"
> >> + table="Logical_Router"/> on the respective Logical_Router is set
> >> + to <code>true</code>.
> >> +
> >> + This is a list of elements separated by <code>;</code>.
> >> +
> >> + If <code>connected</code> is in the list then northd will
> >> synchronize
> >> + all "connected" routes to the southbound <ref table="Route"
> >> + db="OVN_SB"/> table. "Connected" here means routes implicitly
> >> created
> >> + by networks associated with the LRPs.
> >> +
> >> + If <code>static</code> is in the list then northd will
> >> synchronize all
> >> + <ref table="Logical_Router_Static_Route"/> to the southbound
> >> + <ref table="Route" db="OVN_SB"/> table.
> >> +
> >> + If not set the value from <ref column="options"
> >> + key="dynamic-routing-redistribute" table="Logical_Router"/> will
> >> be
> >> + used.
> >> </column>
> >> </group>
> >>
> >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> >> index 850c6aeb7..6cf08c8ea 100644
> >> --- a/tests/ovn-northd.at
> >> +++ b/tests/ovn-northd.at
> >> @@ -14925,7 +14925,8 @@ ovn_start
> >>
> >> # Adding a router - no route advertised.
> >> check ovn-nbctl lr-add lr0
> >> -check ovn-nbctl --wait=sb set Logical_Router lr0
> >> option:dynamic-routing=true
> >> +check ovn-nbctl --wait=sb set Logical_Router lr0
> >> option:dynamic-routing=true \
> >> +
> >> option:dynamic-routing-redistribute="connected;static"
> >> check_row_count Advertised_Route 0
> >> datapath=$(fetch_column datapath_binding _uuid external_ids:name=lr0)
> >>
> >> @@ -14978,6 +14979,59 @@ check_row_count Advertised_Route 0
> >> AT_CLEANUP
> >> ])
> >>
> >> +OVN_FOR_EACH_NORTHD_NO_HV([
> >> +AT_SETUP([dynamic-routing - sync to sb filtering])
> >> +AT_KEYWORDS([dynamic-routing])
> >> +ovn_start
> >> +
> >> +# We start with announcing everything on a lr with 2 lrps and 2 static
> >> routes.
> >> +check ovn-nbctl lr-add lr0
> >> +check ovn-nbctl --wait=sb set Logical_Router lr0
> >> option:dynamic-routing=true \
> >> +
> >> option:dynamic-routing-redistribute="connected;static"
> >> +check ovn-nbctl --wait=sb lrp-add lr0 lr0-sw0 00:00:00:00:ff:01
> >> 10.0.0.1/24
> >> +sw0=$(fetch_column port_binding _uuid logical_port=lr0-sw0)
> >> +check ovn-nbctl --wait=sb lrp-add lr0 lr0-sw1 00:00:00:00:ff:02
> >> 2001:db8::1/64
> >> +sw1=$(fetch_column port_binding _uuid logical_port=lr0-sw1)
> >> +check ovn-nbctl --wait=sb lr-route-add lr0 192.168.0.0/24 10.0.0.10
> >> +check ovn-nbctl --wait=sb lr-route-add lr0 2001:db8:1::/64 2001:db8::10
> >> +check_row_count Advertised_Route 4
> >> +datapath=$(fetch_column datapath_binding _uuid external_ids:name=lr0)
> >> +
> >> +# Disabling connected routes just keeps the static ones.
> >> +check ovn-nbctl --wait=sb set Logical_Router lr0
> >> option:dynamic-routing-redistribute="static"
> >> +check_row_count Advertised_Route 2
> >> +check_column 192.168.0.0/24 Advertised_Route ip_prefix datapath=$datapath
> >> logical_port=$sw0
> >> +check_column 2001:db8:1::/64 Advertised_Route ip_prefix
> >> datapath=$datapath logical_port=$sw1
> >> +
> >> +# Enabling it on lr0-sw0 will just bring this one route back.
> >> +check ovn-nbctl --wait=sb set Logical_Router_Port lr0-sw0
> >> option:dynamic-routing-redistribute="connected;static"
> >> +check_row_count Advertised_Route 3
> >> +check_row_count Advertised_Route 2 logical_port=$sw0
> >> +check_row_count Advertised_Route 1 logical_port=$sw0 ip_prefix=10.0.0.0/24
> >> +check_row_count Advertised_Route 1 logical_port=$sw0
> >> ip_prefix=192.168.0.0/24
> >> +
> >> +# Disabling static routes just keeps the one explicit connected route.
> >> +check ovn-nbctl --wait=sb remove Logical_Router lr0 option
> >> dynamic-routing-redistribute
> >> +check ovn-nbctl --wait=sb set Logical_Router_Port lr0-sw0
> >> option:dynamic-routing-redistribute="connected"
> >> +check_row_count Advertised_Route 1
> >> +check_column 10.0.0.0/24 Advertised_Route ip_prefix datapath=$datapath
> >> logical_port=$sw0
> >> +
> >> +# Enabling static routes on the LR, but disabeling them on lr0-sw0 also
> >> works.
> >> +check ovn-nbctl --wait=sb set Logical_Router lr0
> >> option:dynamic-routing-redistribute="static"
> >> +check ovn-nbctl --wait=sb set Logical_Router_Port lr0-sw0
> >> option:dynamic-routing-redistribute="connected"
> >> +check_row_count Advertised_Route 2
> >> +check_column 10.0.0.0/24 Advertised_Route ip_prefix datapath=$datapath
> >> logical_port=$sw0
> >> +check_column 2001:db8:1::/64 Advertised_Route ip_prefix
> >> datapath=$datapath logical_port=$sw1
> >> +
> >> +# Setting an empty dynamic-routing-redistribute will block advertisements
> >> from
> >> +# this interface.
> >> +check ovn-nbctl --wait=sb set Logical_Router_Port lr0-sw0
> >> option:dynamic-routing-redistribute='""'
> >> +check_row_count Advertised_Route 1
> >> +check_column 2001:db8:1::/64 Advertised_Route ip_prefix
> >> datapath=$datapath logical_port=$sw1
> >> +
> >> +AT_CLEANUP
> >> +])
> >> +
> >> OVN_FOR_EACH_NORTHD_NO_HV([
> >> AT_SETUP([dynamic-routing incremental processing])
> >> AT_KEYWORDS([dynamic-routing])
> >> @@ -14989,7 +15043,8 @@ ovn_start
> >> check ovn-nbctl --wait=sb sync
> >> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >> check ovn-nbctl lr-add lr0
> >> -check ovn-nbctl --wait=sb set Logical_Router lr0
> >> option:dynamic-routing=true
> >> +check ovn-nbctl --wait=sb set Logical_Router lr0
> >> option:dynamic-routing=true \
> >> +
> >> option:dynamic-routing-redistribute="connected;static"
> >>
> >> check_engine_stats northd recompute nocompute
> >> check_engine_stats routes recompute nocompute
> >> @@ -15038,6 +15093,34 @@ check_engine_stats routes recompute nocompute
> >> check_engine_stats advertised_route_sync recompute nocompute
> >> CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >>
> >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >> +check ovn-nbctl --wait=sb set Logical_Router lr0
> >> option:dynamic-routing-redistribute="static"
> >> +check_engine_stats northd recompute nocompute
> >> +check_engine_stats routes recompute nocompute
> >> +check_engine_stats advertised_route_sync recompute nocompute
> >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >> +
> >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >> +check ovn-nbctl --wait=sb remove Logical_Router lr0 option
> >> dynamic-routing-redistribute
> >> +check_engine_stats northd recompute nocompute
> >> +check_engine_stats routes recompute nocompute
> >> +check_engine_stats advertised_route_sync recompute nocompute
> >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >> +
> >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >> +check ovn-nbctl --wait=sb set Logical_Router_Port lr0-sw1
> >> option:dynamic-routing-redistribute="connected"
> >> +check_engine_stats northd recompute nocompute
> >> +check_engine_stats routes recompute nocompute
> >> +check_engine_stats advertised_route_sync recompute nocompute
> >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >> +
> >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >> +check ovn-nbctl --wait=sb set Logical_Router_Port lr0-sw0
> >> option:dynamic-routing-redistribute="connected"
> >> +check_engine_stats northd recompute nocompute
> >> +check_engine_stats routes recompute nocompute
> >> +check_engine_stats advertised_route_sync recompute nocompute
> >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >> +
> >> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >> check ovn-nbctl --wait=sb lrp-del lr0-sw0
> >> check_engine_stats northd recompute compute
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
