On Mon, Jan 13, 2025 at 01:59:57PM +0100, Dumitru Ceara wrote: > Hi Felix, > > Overall looks OK. I left some comments below.
Hi Dumitru, thanks a lot for the review. All things will be adressed in v3. > > Regards, > Dumitru > > On 12/18/24 11:24 AM, Felix Huettner via dev wrote: > > previously all routes of a logical router where announced. However in > > Nit: Previously > > > 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]> > > --- > > NEWS | 3 ++ > > northd/en-advertised-route-sync.c | 18 +++++++ > > ovn-nb.xml | 78 ++++++++++++++++++++++++++++--- > > tests/ovn-northd.at | 61 +++++++++++++++++++++++- > > 4 files changed, 152 insertions(+), 8 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index 342b91e96..33cb2ca89 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -15,6 +15,9 @@ Post v24.09.0 > > - 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. > > + The routes can furthe be filtered by setting > > `dynamic-routing-connected` > > Typo: furthe > > > + and `dynamic-routing-static` on the LR or LRP. The LRP settings > > overwrite > > + the LR settings for all routes using this interface as an exit. > > I think I'd rephrase this last part as: > "... 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 46ae3adf8..33097ed72 100644 > > --- a/northd/en-advertised-route-sync.c > > +++ b/northd/en-advertised-route-sync.c > > @@ -15,6 +15,7 @@ > > #include <config.h> > > > > #include "openvswitch/vlog.h" > > +#include "smap.h" > > #include "stopwatch.h" > > #include "northd.h" > > > > @@ -135,6 +136,13 @@ route_erase_entry(struct ar_entry *route_e) > > free(route_e); > > } > > > > +static bool > > +get_nbrp_or_nbr_option(const struct ovn_port *op, const char *key) > > +{ > > + return smap_get_bool(&op->nbrp->options, key, > > + smap_get_bool(&op->od->nbr->options, key, false)); > > Nit: I'd indent this as: > > return smap_get_bool(&op->nbrp->options, key, > smap_get_bool(&op->od->nbr->options, > key, false)); > > Also, the NEWS entry seems to imply (at least to me) that if we don't > set any of the per LR/LRP options we advertise all routes. While in > practice we don't, we opt-in for each type of (static/connected) route. > Or am I reading this wrong? Yes it is opt-in. I'll update the NEWS entry to make that more clear. > > > +} > > + > > static void > > advertised_route_table_sync( > > struct ovsdb_idl_txn *ovnsb_txn, > > @@ -172,6 +180,16 @@ advertised_route_table_sync( > > false)) { > > continue; > > } > > + if (route->source == ROUTE_SOURCE_CONNECTED && > > + !get_nbrp_or_nbr_option(route->out_port, > > + "dynamic-routing-connected")) { > > Nit: should we pre-parse this option for the datapath and router ports? > > > + continue; > > + } > > + if (route->source == ROUTE_SOURCE_STATIC && > > + !get_nbrp_or_nbr_option(route->out_port, > > + "dynamic-routing-static")) { > > Same question here. > > > + continue; > > + } > > > > char *ip_prefix = normalize_v46_prefix(&route->prefix, > > route->plen); > > diff --git a/ovn-nb.xml b/ovn-nb.xml > > index 7f17c8059..fb178cbed 100644 > > --- a/ovn-nb.xml > > +++ b/ovn-nb.xml > > @@ -2951,13 +2951,45 @@ 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 > > + routes types that should be advertised. See: > > s/routes types/route types/ > > > + * <ref column="options" key="dynamic-routing-connected" > > + table="Logical_Router"/> > > + * <ref column="options" key="dynamic-routing-static" > > + table="Logical_Router"/> > > + * <ref column="options" key="dynamic-routing-connected" > > + table="Logical_Router_Port"/> > > + * <ref column="options" key="dynamic-routing-static" > > + table="Logical_Router_Port"/> > > This doesn't render too nicely in the resulting man page (all items end > up on the same line). I'm not sure of the best way to do this but > wrapping all this into a <ul></ul> and each of the items into a > <li></li> seems to give decent results: > > • options:dynamic-routing-connected > > • options:dynamic-routing-static > > • options:dynamic-routing-connected > > • options:dynamic-routing-static > > > + </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> > > > > @@ -3685,6 +3717,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 network associated with this LRP. > > s/network/networks/ > > > + > > + 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 as an > > outgoin > > Typo: "outgoin" but maybe we should rephrase as: > > "... that use this LRP to forward traffic on." > > > + interface. > > + > > + 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 b677c4b87..370f1d38d 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -14392,7 +14392,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=$(ovn-sbctl --bare --columns _uuid list datapath_binding lr0) > > > > @@ -14441,3 +14443,60 @@ 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 > > Please write comments as sentences - starting with a capital letter and > ending with dot. > > > +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=$(ovn-sbctl --bare --columns _uuid list port_binding lr0-sw0) > > +check ovn-nbctl --wait=sb lrp-add lr0 lr0-sw1 00:00:00:00:ff:02 10.0.1.1/24 > > +sw1=$(ovn-sbctl --bare --columns _uuid list port_binding 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 192.168.1.0/24 10.0.1.10 > > +check_row_count Advertised_Route 4 > > +datapath=$(ovn-sbctl --bare --columns _uuid list datapath_binding lr0) > > fetch_column > > > + > > +# disabeling connected routes just keeps the static ones > > Typo: disabeling (and not a sentence). > > > +check ovn-nbctl --wait=sb remove Logical_Router lr0 option > > dynamic-routing-connected > > +check_row_count Advertised_Route 2 > > +AT_CHECK([ovn-sbctl --columns ip_prefix --bare find Advertised_Route > > datapath=$datapath logical_port=$sw0], [0], [dnl > > +192.168.0.0/24 > > +]) > > +AT_CHECK([ovn-sbctl --columns ip_prefix --bare find Advertised_Route > > datapath=$datapath logical_port=$sw1], [0], [dnl > > +192.168.1.0/24 > > +]) > > + > > +# enabeling it on lr0-sw0 will just bring this one route back > > Typo: enabeling (and not a sentence). > > > +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 (and not a sentence). > > > +check ovn-nbctl --wait=sb remove Logical_Router lr0 option > > dynamic-routing-static > > +check_row_count Advertised_Route 1 > > +AT_CHECK([ovn-sbctl --columns ip_prefix --bare find Advertised_Route > > datapath=$datapath logical_port=$sw0], [0], [dnl > > check_column/check_row_count > > > +10.0.0.0/24 > > +]) > > + > > +# enabeling static routes on the LR, but disabeling them on lr0-sw0 also > > works > > enabeling and not a sentence > > > +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 > > +AT_CHECK([ovn-sbctl --columns ip_prefix --bare find Advertised_Route > > datapath=$datapath logical_port=$sw0], [0], [dnl > > +10.0.0.0/24 > > +]) > > +AT_CHECK([ovn-sbctl --columns ip_prefix --bare find Advertised_Route > > datapath=$datapath logical_port=$sw1], [0], [dnl > > +192.168.1.0/24 > > +]) > > check_column/check_row_count > > > + > > +AT_CLEANUP > > +]) > > + > > Maybe we should also test IPv6? > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
