On Thu, Feb 06, 2025 at 08:33:57AM +0100, Frode Nordahl wrote:
> On Wed, Feb 5, 2025 at 3:53 PM Felix Huettner via dev
> <[email protected]> wrote:
> >
> > On Wed, Feb 05, 2025 at 01:38:02PM +0100, Dumitru Ceara wrote:
> > > On 2/4/25 2:59 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,
> > >
> > > Thanks for v6!
> >
> > Hi Dumitru,
> >
> > thanks a lot for the review.
> >
> > >
> > > > 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 | 10 ++--
> > > > northd/en-advertised-route-sync.c | 8 +++
> > > > northd/northd.c | 47 +++++++++++++++++
> > > > northd/northd.h | 13 +++++
> > > > ovn-nb.xml | 53 +++++++++++++++++++
> > > > tests/ovn-northd.at | 87 ++++++++++++++++++++++++++++++-
> > > > 6 files changed, 213 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/NEWS b/NEWS
> > > > index 0460868e4..af46560b3 100644
> > > > --- a/NEWS
> > > > +++ b/NEWS
> > > > @@ -39,9 +39,13 @@ Post v24.09.0
> > > > in OVS for both IPv4 and IPv6 addresses, whenever possible,
> > > > reducing
> > > > the amount of IPv6 datapath flows.
> > > > - 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 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 5126348a8..d4360763f 100644
> > > > --- a/northd/en-advertised-route-sync.c
> > > > +++ b/northd/en-advertised-route-sync.c
> > > > @@ -148,6 +148,14 @@ advertised_route_table_sync(
> > > > 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;
> > > > + }
> > > >
> > > > char *ip_prefix = normalize_v46_prefix(&route->prefix,
> > > > route->plen);
> > > > diff --git a/northd/northd.c b/northd/northd.c
> > > > index 594060bdd..b1a81f985 100644
> > > > --- a/northd/northd.c
> > > > +++ b/northd/northd.c
> > > > @@ -801,6 +801,44 @@ ovn_datapath_update_external_ids(struct
> > > > ovn_datapath *od)
> > > > smap_destroy(&ids);
> > > > }
> > > >
> > > > +static void
> > > > +parse_dynamic_routing_redistribute(const struct smap *options,
> > > > + bool *dynamic_routing_connected,
> > > > + bool
> > > > default_dynamic_routing_connected,
> > > > + bool *dynamic_routing_static,
> > > > + bool default_dynamic_routing_static)
> > >
> > > Instead of passing booleans everywhere should we use an enum?
> > >
> > > e.g.:
> > >
> > > 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),
> > > };
> > >
> > > And we could change this function's signature to:
> > >
> > > static enum dynamic_routing_redistribute_mode
> > > parse_dynamic_routing_redistribute(
> > > const char *dynamic_routing_str,
> > > enum dynamic_routing_redistribute_mode default_dynamic_mode);
> > >
> > > We could also store a single field (bitwise OR between modes) in the
> > > struct ovn_datapath/ovn_port records instead of multiple booleans.
> > >
> > > What do you think?
> >
> > Yes that is nicer.
> > I'll adapt this in the next version.
> >
> > >
> > > > +{
> > > > + char *cur, *next, *start;
> > > > +
> > > > + *dynamic_routing_connected = false;
> > > > + *dynamic_routing_static = false;
> > > > +
> > > > + const char *dynamic_routing_redistribute = smap_get(
> > > > + options, "dynamic-routing-redistribute");
> > > > + if (!dynamic_routing_redistribute) {
> > > > + *dynamic_routing_connected = default_dynamic_routing_connected;
> > > > + *dynamic_routing_static = default_dynamic_routing_static;
> > > > + return;
> > > > + }
> > > > +
> > > > + start = next = xstrdup(dynamic_routing_redistribute);
> > > > + while ((cur = strsep(&next, ";")) && *cur) {
>
> Since a v7 is needed anyway:
> While the implementation looks sane to me, it struck me as being
> different from other parts of northd dealing with the same kinds of
> options. It appears we are using strtok_r() elsewhere.
>
> Is there any reason not to use strtok_r() here, and should we keep the
> implementation aligned with the rest of the code base?
Hi Frode,
I will switch to strtok_r if that is generally used.
Thanks for pointing that out.
Thanks,
Felix
>
> --
> Frode Nordahl
>
> > > > + if (!strcmp(cur, "connected")) {
> > > > + *dynamic_routing_connected = true;
> > > > + continue;
> > > > + }
> > > > + if (!strcmp(cur, "static")) {
> > > > + *dynamic_routing_static = true;
> > > > + continue;
> > > > + }
> > > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > > > + VLOG_WARN_RL(&rl, "unkown dynamic-routing-redistribute option
> > > > '%s'",
> > > > + cur);
> > > > + }
> > > > +
> > > > + free(start);
> > > > +}
> > > > +
> > > > static void
> > > > join_datapaths(const struct nbrec_logical_switch_table *nbrec_ls_table,
> > > > const struct nbrec_logical_router_table *nbrec_lr_table,
> > > > @@ -896,6 +934,10 @@ join_datapaths(const struct
> > > > nbrec_logical_switch_table *nbrec_ls_table,
> > > > }
> > > > od->dynamic_routing = smap_get_bool(&od->nbr->options,
> > > > "dynamic-routing", false);
> > > > + parse_dynamic_routing_redistribute(&od->nbr->options,
> > > > +
> > > > &od->dynamic_routing_connected,
> > > > + false,
> > > > +
> > > > &od->dynamic_routing_static, false);
> > > > ovs_list_push_back(lr_list, &od->lr_list);
> > > > }
> > > > }
> > > > @@ -2222,6 +2264,11 @@ join_logical_ports_lrp(struct hmap *ports,
> > > >
> > > > op->prefix_delegation = smap_get_bool(&op->nbrp->options,
> > > > "prefix_delegation", false);
> > > > + parse_dynamic_routing_redistribute(&op->nbrp->options,
> > > > + &op->dynamic_routing_connected,
> > > > + od->dynamic_routing_connected,
> > > > + &op->dynamic_routing_static,
> > > > + od->dynamic_routing_static);
> > > >
> > > > 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 1a843a627..a70a10e0c 100644
> > > > --- a/northd/northd.h
> > > > +++ b/northd/northd.h
> > > > @@ -362,6 +362,10 @@ struct ovn_datapath {
> > > > bool redirect_bridged;
> > > > /* nbr has the option "dynamic-routing" set to true. */
> > > > bool dynamic_routing;
> > > > + /* nbr option "dynamic-routing-redistribute" contains "connected".
> > > > */
> > > > + bool dynamic_routing_connected;
> > > > + /* nbr option "dynamic-routing-redistribute" contains "static". */
> > > > + bool dynamic_routing_static;
> > > >
> > > > struct ovn_port **localnet_ports;
> > > > size_t n_localnet_ports;
> > > > @@ -617,6 +621,15 @@ struct ovn_port {
> > > > struct lport_addresses lrp_networks;
> > > > bool prefix_delegation; /* True if IPv6 prefix delegation enabled.
> > > > */
> > > >
> > > > + /* nbrp option "dynamic-routing-redistribute" contains "connected".
> > > > + * If the option is unset it will be initialized based on the nbr
> > > > + * option. */
> > > > + bool dynamic_routing_connected;
> > > > + /* nbrp option "dynamic-routing-redistribute" contains "static".
> > > > + * If the option 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 aad014cb3..bf785e4d3 100644
> > > > --- a/ovn-nb.xml
> > > > +++ b/ovn-nb.xml
> > > > @@ -2973,6 +2973,37 @@ 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"/>.
> > > > +>>>>>>> 20721b41b (northd: Add filtering which routes to advertise.)
> > >
> > > I think the rebase conflict resolution was a bit wrong here.
> >
> > I'll fix that.
> >
> > Thanks a lot,
> > Felix
> >
> > >
> > > > </column>
> > > > </group>
> > > >
> > > > @@ -3699,6 +3730,28 @@ or
> > > > learned by the <code>ovn-ic</code> daemon.
> > > > </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>
> > > >
> > > > <group title="Attachment">
> > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > > > index 4c2e2713a..474e78163 100644
> > > > --- a/tests/ovn-northd.at
> > > > +++ b/tests/ovn-northd.at
> > > > @@ -14587,7 +14587,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)
> > > >
> > > > @@ -14640,6 +14641,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])
> > > > @@ -14651,7 +14705,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
> > > > @@ -14700,6 +14755,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
> > >
> > > Thanks,
> > > 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
