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?
--
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
