On 1/21/25 4:47 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. > > Signed-off-by: Felix Huettner <[email protected]> > ---
Hi Felix, This patch looks good to me in general. I just have a few small comments for the next revision. > v2->v3: > * A lot of minor review comments. > * Reworked NEWs entry to make default more clear > > NEWS | 10 ++-- > northd/en-advertised-route-sync.c | 12 ++++- > northd/northd.c | 12 +++++ > northd/northd.h | 11 ++++ > ovn-nb.xml | 80 ++++++++++++++++++++++++++--- > tests/ovn-northd.at | 84 ++++++++++++++++++++++++++++++- > 6 files changed, 195 insertions(+), 14 deletions(-) > > diff --git a/NEWS b/NEWS > index 4ff158014..2197d62db 100644 > --- a/NEWS > +++ b/NEWS > @@ -29,9 +29,13 @@ Post v24.09.0 > be configured via --ssl-ciphersuites (--ssl-ciphers only applies to > TLSv1.2 and earlier). > - 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 > - southbound "Route" table for sharing outside of OVN. > + * 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 filtered by setting > + `dynamic-routing-connected` and `dynamic-routing-static` on the LR or > + LRP. The LRP settings overwrite the LR settings for all routes using > + this interface to forward traffic on. I guess we're still discussing whether these should be separate options or if we should have different way of configuring redistribution: https://mail.openvswitch.org/pipermail/ovs-dev/2025-January/420316.html Just reiterating that my vote there was for something like: LR.options.dynamic-routing-redistribute="connected;nat;etc" But I don't have a very strong preference at the moment so I won't block this patch on it for now. > > OVN v24.09.0 - 13 Sep 2024 > -------------------------- > diff --git a/northd/en-advertised-route-sync.c > b/northd/en-advertised-route-sync.c > index 6ddd15512..b94bc0240 100644 > --- a/northd/en-advertised-route-sync.c > +++ b/northd/en-advertised-route-sync.c > @@ -16,6 +16,7 @@ > > #include <config.h> > > +#include "smap.h" This is not needed. > #include "stopwatch.h" > #include "northd.h" > > @@ -148,8 +149,15 @@ advertised_route_table_sync( > if (prefix_is_link_local(&route->prefix, route->plen)) { > continue; > } > - if (!smap_get_bool(&route->od->nbr->options, "dynamic-routing", > - false)) { > + if (!route->od->dynamic_routing) { > + continue; > + } > + if (route->source == ROUTE_SOURCE_CONNECTED && > + !route->out_port->dynamic_routing_connected) { > + continue; > + } > + if (route->source == ROUTE_SOURCE_STATIC && > + !route->out_port->dynamic_routing_static) { > continue; > } > > diff --git a/northd/northd.c b/northd/northd.c > index 1c32ef8f8..06fcb0bde 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -914,6 +914,12 @@ 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_connected = smap_get_bool(&od->nbr->options, > + "dynamic-routing-connected", > + false); > + od->dynamic_routing_static = smap_get_bool(&od->nbr->options, > + "dynamic-routing-static", > + false); The indentation is a bit off here. To be inline with most of the code base I'd write this as: od->dynamic_routing_connected = smap_get_bool(&od->nbr->options, "dynamic-routing-connected", false); od->dynamic_routing_static = smap_get_bool(&od->nbr->options, "dynamic-routing-static", false); > ovs_list_push_back(lr_list, &od->lr_list); > } > } > @@ -2262,6 +2268,12 @@ join_logical_ports_lrp(struct hmap *ports, > > op->prefix_delegation = smap_get_bool(&op->nbrp->options, > "prefix_delegation", false); > + op->dynamic_routing_connected = smap_get_bool( > + &op->nbrp->options, "dynamic-routing-connected", > + od->dynamic_routing_connected); > + op->dynamic_routing_static = smap_get_bool(&op->nbrp->options, > + "dynamic-routing-static", > + od->dynamic_routing_static); Here too, I guess. > > 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 98a17d733..e5a1674fc 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -369,6 +369,10 @@ struct ovn_datapath { > bool redirect_bridged; > /* nbr has the option "dynamic-routing" set to true. */ > bool dynamic_routing; > + /* nbr has the option "dynamic-routing-connected" set to true. */ > + bool dynamic_routing_connected; > + /* nbr has the option "dynamic-routing-static" set to true. */ > + bool dynamic_routing_static; > > struct ovn_port **localnet_ports; > size_t n_localnet_ports; > @@ -624,6 +628,13 @@ struct ovn_port { > struct lport_addresses lrp_networks; > bool prefix_delegation; /* True if IPv6 prefix delegation enabled. */ > > + /* nbrp has the option "dynamic-routing-connected" set to true. > + * If it is unset it will be initialized based on the nbr option. */ > + bool dynamic_routing_connected; > + /* nbrp has the option "dynamic-routing-static" set to true. > + * If it is unset it will be initialized based on the nbr option. */ > + bool dynamic_routing_static; > + > /* Logical port multicast data. */ > struct mcast_port_info mcast_info; > > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 718d0126e..4dddf4c60 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -2951,13 +2951,47 @@ or > If set to <code>true</code> then this <ref table="Logical_Router"/> > can participate in dynamic routing with components outside of OVN. > > - It will synchronize all routes to the soutbound > - <ref table="Route" db="OVN_SB"/> table that are relevant for the > - router. This includes: > - * all "connected" routes implicitly created by networks associated > with > - this Logical Router > - * all <ref table="Logical_Router_Static_Route"/> that are applied to > - this Logical Router > + 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-connected" > + table="Logical_Router"/></li> > + <li><ref column="options" key="dynamic-routing-static" > + table="Logical_Router"/></li> > + <li><ref column="options" key="dynamic-routing-connected" > + table="Logical_Router_Port"/></li> > + <li><ref column="options" key="dynamic-routing-static" > + table="Logical_Router_Port"/></li> > + </ul> > + </column> > + > + <column name="options" key="dynamic-routing-connected" > + type='{"type": "boolean"}'> > + Only relevant if <ref column="options" key="dynamic-routing" > + table="Logical_Router"/> is set to <code>true</code>. > + > + If this is <code>true</code> as well 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. > + > + This value can be overwritten on a per LRP basis using > + <ref column="options" key="dynamic-routing-connected" > + table="Logical_Router_Port"/>. > + </column> > + > + <column name="options" key="dynamic-routing-static" > + type='{"type": "boolean"}'> > + Only relevant if <ref column="options" key="dynamic-routing" > + table="Logical_Router"/> is set to <code>true</code>. > + > + If this is <code>true</code> as well 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-static" > + table="Logical_Router_Port"/>. > </column> > </group> > > @@ -3684,6 +3718,38 @@ or > learned by the <code>ovn-ic</code> daemon. > </p> > </column> > + > + <column name="options" key="dynamic-routing-connected" > + type='{"type": "boolean"}'> > + Only relevant if <ref column="options" key="dynamic-routing" > + table="Logical_Router"/> on the respective Logical_Router is set > + to <code>true</code>. > + > + If this is <code>true</code> as well then northd will synchronize all > + "connected" routes associated with this LRP to the southbound > + <ref table="Route" db="OVN_SB"/> table. "Connected" here means routes > + implicitly created by networks associated with this LRP. > + > + If not set the value from <ref column="options" > + key="dynamic-routing-connected" table="Logical_Router_Port"/> will be > + used. > + </column> > + > + <column name="options" key="dynamic-routing-static" > + type='{"type": "boolean"}'> > + Only relevant if <ref column="options" key="dynamic-routing" > + table="Logical_Router"/> on the respective Logical_Router is set > + to <code>true</code>. > + > + If this is <code>true</code> as well then northd will synchronize all > + <ref table="Logical_Router_Static_Route"/> to the southbound > + <ref table="Route" db="OVN_SB"/> table that use this LRP to forward > + traffic on. > + > + If not set the value from <ref column="options" > + key="dynamic-routing-static" table="Logical_Router_Port"/> will be > + used. > + </column> > </group> > > <group title="Attachment"> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 30ea23502..397a4ffe3 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -14427,7 +14427,9 @@ ovn_start > > # adding a router - still nothing here > 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-connected=true \ > + option:dynamic-routing-static=true > check_row_count Advertised_Route 0 > datapath=$(fetch_column datapath_binding _uuid external_ids:name=lr0) > > @@ -14480,6 +14482,53 @@ 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-connected=true \ > + option:dynamic-routing-static=true > +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) > + > +# Disabeling connected routes just keeps the static ones. Typo: Disabeling > +check ovn-nbctl --wait=sb remove Logical_Router lr0 option > dynamic-routing-connected > +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 > + > +# Enabeling it on lr0-sw0 will just bring this one route back. Typo: Enabeling > +check ovn-nbctl --wait=sb set Logical_Router_Port lr0-sw0 > option:dynamic-routing-connected=true > +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 > + > +# Disabeling static routes just keeps the one explicit connected route. Typo: Disabeling > +check ovn-nbctl --wait=sb remove Logical_Router lr0 option > dynamic-routing-static > +check_row_count Advertised_Route 1 > +check_column 10.0.0.0/24 Advertised_Route ip_prefix datapath=$datapath > logical_port=$sw0 > + > +# Enabeling static routes on the LR, but disabeling them on lr0-sw0 also > works. Typo: Enabeling > +check ovn-nbctl --wait=sb set Logical_Router lr0 > option:dynamic-routing-static=true > +check ovn-nbctl --wait=sb set Logical_Router_Port lr0-sw0 > option:dynamic-routing-static=false > +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 > + > +AT_CLEANUP > +]) > + > OVN_FOR_EACH_NORTHD_NO_HV([ > AT_SETUP([dynamic-routing incremental processing]) > AT_KEYWORDS([dynamic-routing]) > @@ -14491,7 +14540,9 @@ 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-connected=true \ > + option:dynamic-routing-static=true > > check_engine_stats northd recompute nocompute > check_engine_stats routes recompute nocompute > @@ -14540,6 +14591,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 remove Logical_Router lr0 option > dynamic-routing-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 remove Logical_Router lr0 option > dynamic-routing-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 set Logical_Router_Port lr0-sw1 > option:dynamic-routing-connected=true > +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-static=true > +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 > @@ -14556,3 +14635,4 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE > > AT_CLEANUP > ]) > + Unrelated newline, also not needed. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
