> 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.
same for me. After addressing Dumitru's comments: Acked-by: Lorenzo Bianconi <[email protected]> Regards, Lorenzo > > > 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 >
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
