On 2/6/25 8:33 AM, 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?
>
Well, we use both throughout the code base. I personally prefer
strsep() though. I don't really think using strtok*() ever has an
advantage over strsep().
But because we use both versions in the code base I won't oppose a
change. I'll let you guys decide. :)
Regards,
Dumitru
> --
> 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
