On 2/3/25 4:12 PM, [email protected] wrote:
> On Fri, 2025-01-31 at 12:49 +0100, Dumitru Ceara wrote:
>> On 1/30/25 10:51 PM, Dumitru Ceara wrote:
>>> On 1/21/25 4:47 PM, Felix Huettner via dev wrote:
>>>> in order to exchange routes between OVN and the network fabric we
>>>> use the new Advertised_Route sb table. Northd here advertises all
>>>> routes
>>>> where the user explicitly opted-in.
>>>>
>>>> ovn-controller will later use this table to share these routes to
>>>> the
>>>> outside.
>>>>
>>>> Signed-off-by: Felix Huettner <[email protected]>
>>>> ---
>>>
>>> Hi Felix,
>>>
>>> I have a few small comments below. Aside from those this patch
>>> looks in
>>> good shape to me.
>>>
>>>> v3->v4:
>>>> * fix use after free
>>>> v2->v3:
>>>> * A lot of minor review comments.
>>>> * Sync logic reworked to no longer need a "stale" field.
>>>> * Stop watching Advertised_Route table for changes.
>>>>
>>>> NEWS | 4 +
>>>> ic/ovn-ic.c | 21 ----
>>>> lib/ovn-util.c | 22 ++++
>>>> lib/ovn-util.h | 2 +
>>>> lib/stopwatch-names.h | 1 +
>>>> northd/automake.mk | 2 +
>>>> northd/en-advertised-route-sync.c | 186
>>>> ++++++++++++++++++++++++++++++
>>>> northd/en-advertised-route-sync.h | 29 +++++
>>>> northd/en-northd-output.c | 8 ++
>>>> northd/en-northd-output.h | 2 +
>>>> northd/inc-proc-northd.c | 11 +-
>>>> northd/northd.c | 29 +++--
>>>> northd/northd.h | 6 +-
>>>> northd/ovn-northd.c | 5 +
>>>> ovn-nb.xml | 13 +++
>>>> tests/ovn-northd.at | 137 ++++++++++++++++++++++
>>>> 16 files changed, 443 insertions(+), 35 deletions(-)
>>>> create mode 100644 northd/en-advertised-route-sync.c
>>>> create mode 100644 northd/en-advertised-route-sync.h
>>>>
>>>> diff --git a/NEWS b/NEWS
>>>> index 72c5a6339..4ff158014 100644
>>>> --- a/NEWS
>>>> +++ b/NEWS
>>>> @@ -28,6 +28,10 @@ Post v24.09.0
>>>> this option was not set). TLS ciphersuites for TLSv1.3
>>>> and later can
>>>> 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.
>>>>
>>>> OVN v24.09.0 - 13 Sep 2024
>>>> --------------------------
>>>> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
>>>> index 8320cbea5..948c17f9b 100644
>>>> --- a/ic/ovn-ic.c
>>>> +++ b/ic/ovn-ic.c
>>>> @@ -1008,27 +1008,6 @@ get_nexthop_from_lport_addresses(bool
>>>> is_v4,
>>>> return true;
>>>> }
>>>>
>>>> -static bool
>>>> -prefix_is_link_local(struct in6_addr *prefix, unsigned int plen)
>>>> -{
>>>> - if (IN6_IS_ADDR_V4MAPPED(prefix)) {
>>>> - /* Link local range is "169.254.0.0/16". */
>>>> - if (plen < 16) {
>>>> - return false;
>>>> - }
>>>> - ovs_be32 lla;
>>>> - inet_pton(AF_INET, "169.254.0.0", &lla);
>>>> - return ((in6_addr_get_mapped_ipv4(prefix) &
>>>> htonl(0xffff0000)) == lla);
>>>> - }
>>>> -
>>>> - /* ipv6, link local range is "fe80::/10". */
>>>> - if (plen < 10) {
>>>> - return false;
>>>> - }
>>>> - return (((prefix->s6_addr[0] & 0xff) == 0xfe) &&
>>>> - ((prefix->s6_addr[1] & 0xc0) == 0x80));
>>>> -}
>>>> -
>>>> static bool
>>>> prefix_is_deny_listed(const struct smap *nb_options,
>>>> struct in6_addr *prefix,
>>>> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
>>>> index b78bdbfa1..ed847517a 100644
>>>> --- a/lib/ovn-util.c
>>>> +++ b/lib/ovn-util.c
>>>> @@ -1351,3 +1351,25 @@ ovn_update_swconn_at(struct rconn *swconn,
>>>> const char *target,
>>>>
>>>> return notify;
>>>> }
>>>> +
>>>> +bool
>>>> +prefix_is_link_local(const struct in6_addr *prefix, unsigned int
>>>> plen)
>>>> +{
>>>> + if (IN6_IS_ADDR_V4MAPPED(prefix)) {
>>>> + /* Link local range is "169.254.0.0/16". */
>>>> + if (plen < 16) {
>>>> + return false;
>>>> + }
>>>> + ovs_be32 lla;
>>>> + inet_pton(AF_INET, "169.254.0.0", &lla);
>>>> + return ((in6_addr_get_mapped_ipv4(prefix) &
>>>> htonl(0xffff0000)) == lla);
>>>> + }
>>>> +
>>>> + /* ipv6, link local range is "fe80::/10". */
>>>> + if (plen < 10) {
>>>> + return false;
>>>> + }
>>>> + return (((prefix->s6_addr[0] & 0xff) == 0xfe) &&
>>>> + ((prefix->s6_addr[1] & 0xc0) == 0x80));
>>>> +}
>>>> +
>>>
>>> Nit: one newline too many.
>>>
>>>> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
>>>> index 899bd9d12..31c2c68df 100644
>>>> --- a/lib/ovn-util.h
>>>> +++ b/lib/ovn-util.h
>>>> @@ -487,4 +487,6 @@ void ovn_exit_args_finish(struct
>>>> ovn_exit_args *exit_args);
>>>> bool ovn_update_swconn_at(struct rconn *swconn, const char
>>>> *target,
>>>> int probe_interval, const char
>>>> *where);
>>>>
>>>> +bool prefix_is_link_local(const struct in6_addr *prefix,
>>>> unsigned int plen);
>>>> +
>>>> #endif /* OVN_UTIL_H */
>>>> diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h
>>>> index 660c653fb..dc4129ee5 100644
>>>> --- a/lib/stopwatch-names.h
>>>> +++ b/lib/stopwatch-names.h
>>>> @@ -34,5 +34,6 @@
>>>> #define LR_NAT_RUN_STOPWATCH_NAME "lr_nat_run"
>>>> #define LR_STATEFUL_RUN_STOPWATCH_NAME "lr_stateful"
>>>> #define LS_STATEFUL_RUN_STOPWATCH_NAME "ls_stateful"
>>>> +#define ADVERTISED_ROUTE_SYNC_RUN_STOPWATCH_NAME
>>>> "advertised_route_sync"
>>>>
>>>> #endif
>>>> diff --git a/northd/automake.mk b/northd/automake.mk
>>>> index 6566ad299..a2797237a 100644
>>>> --- a/northd/automake.mk
>>>> +++ b/northd/automake.mk
>>>> @@ -34,6 +34,8 @@ northd_ovn_northd_SOURCES = \
>>>> northd/en-ls-stateful.h \
>>>> northd/en-sampling-app.c \
>>>> northd/en-sampling-app.h \
>>>> + northd/en-advertised-route-sync.c \
>>>> + northd/en-advertised-route-sync.h \
>>>> northd/inc-proc-northd.c \
>>>> northd/inc-proc-northd.h \
>>>> northd/ipam.c \
>>>> diff --git a/northd/en-advertised-route-sync.c b/northd/en-
>>>> advertised-route-sync.c
>>>> new file mode 100644
>>>> index 000000000..6ddd15512
>>>> --- /dev/null
>>>> +++ b/northd/en-advertised-route-sync.c
>>>> @@ -0,0 +1,186 @@
>>>> +/*
>>>> + * Copyright (c) 2024, STACKIT GmbH & Co. KG
>>>
>>> Nit: 2025
>>>
>>>> + *
>>>> + * Licensed under the Apache License, Version 2.0 (the
>>>> "License");
>>>> + * you may not use this file except in compliance with the
>>>> License.
>>>> + * You may obtain a copy of the License at:
>>>> + *
>>>> + * http://www.apache.org/licenses/LICENSE-2.0
>>>> + *
>>>> + * Unless required by applicable law or agreed to in writing,
>>>> software
>>>> + * distributed under the License is distributed on an "AS IS"
>>>> BASIS,
>>>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
>>>> or implied.
>>>> + * See the License for the specific language governing
>>>> permissions and
>>>> + * limitations under the License.
>>>> + */
>>>> +
>>>> +#include <config.h>
>>>> +
>>>> +#include "stopwatch.h"
>>>> +#include "northd.h"
>>>> +
>>>> +#include "en-advertised-route-sync.h"
>>>> +#include "lib/stopwatch-names.h"
>>>> +#include "openvswitch/hmap.h"
>>>> +#include "ovn-util.h"
>>>> +
>>>> +static void
>>>> +advertised_route_table_sync(
>>>> + struct ovsdb_idl_txn *ovnsb_txn,
>>>> + const struct sbrec_advertised_route_table
>>>> *sbrec_advertised_route_table,
>>>> + const struct hmap *parsed_routes);
>>>> +
>>>> +void
>>>> +*en_advertised_route_sync_init(struct engine_node *node
>>>> OVS_UNUSED,
>>>> + struct engine_arg *arg OVS_UNUSED)
>>>> +{
>>>> + return NULL;
>>>> +}
>>>> +
>>>> +void
>>>> +en_advertised_route_sync_cleanup(void *data OVS_UNUSED)
>>>> +{
>>>> +}
>>>> +
>>>> +void
>>>> +en_advertised_route_sync_run(struct engine_node *node, void
>>>> *data OVS_UNUSED)
>>>> +{
>>>> + struct routes_data *routes_data
>>>> + = engine_get_input_data("routes", node);
>>>> + const struct engine_context *eng_ctx = engine_get_context();
>>>> + const struct sbrec_advertised_route_table
>>>> *sbrec_advertised_route_table =
>>>> + EN_OVSDB_GET(engine_get_input("SB_advertised_route",
>>>> node));
>>>> +
>>>> + stopwatch_start(ADVERTISED_ROUTE_SYNC_RUN_STOPWATCH_NAME,
>>>> time_msec());
>>>> +
>>>> + advertised_route_table_sync(eng_ctx->ovnsb_idl_txn,
>>>> + sbrec_advertised_route_table,
>>>> + &routes_data->parsed_routes);
>>>
>>> Nit: indent under the parenthesis, i.e.:
>>>
>>> advertised_route_table_sync(eng_ctx->ovnsb_idl_txn,
>>> sbrec_advertised_route_table,
>>> &routes_data->parsed_routes);
>>>
>>>> +
>>>> + stopwatch_stop(ADVERTISED_ROUTE_SYNC_RUN_STOPWATCH_NAME,
>>>> time_msec());
>>>> + engine_set_node_state(node, EN_UPDATED);
>>>> +}
>>>> +
>>>> +struct ar_entry {
>>>> + struct hmap_node hmap_node;
>>>> +
>>>> + const struct sbrec_datapath_binding *sb_db;
>>>> +
>>>> + const struct sbrec_port_binding *logical_port;
>>>> + char *ip_prefix;
>>>> +};
>>>> +
>>>> +static struct ar_entry *
>>>> +ar_alloc_entry(struct hmap *routes,
>>>> + const struct sbrec_datapath_binding *sb_db,
>>>> + const struct sbrec_port_binding *logical_port,
>>>> + char *ip_prefix)
>>>> +{
>>>> + struct ar_entry *route_e = xzalloc(sizeof *route_e);
>>>> +
>>>> + route_e->sb_db = sb_db;
>>>> + route_e->logical_port = logical_port;
>>>> + route_e->ip_prefix = ip_prefix;
>>>> + uint32_t hash = uuid_hash(&sb_db->header_.uuid);
>>>> + hash = hash_string(logical_port->logical_port, hash);
>>>> + hash = hash_string(ip_prefix, hash);
>>>> + hmap_insert(routes, &route_e->hmap_node, hash);
>>>> +
>>
>> This does more than just allocating the entry. Let's call the
>> function
>> ar_add_entry() or similar. What do you think?
>
> Hi Felix, Dumitru,
Hi Martin,
> I have one more question/comment for ar_alloc_entry.
> If you 'free' memory at pointer passed as the '*ip_prefix' after
> calling this function, northd will crash some time later when trying to
> serialize request to SB. Example of code that would cause the crash:
>
> char *ip_prefix = normalize_v46_prefix(&route->prefix,
> route->plen);
> ar_alloc_entry(route_map,
> route->od->sb,
> route->out_port->sb,
> ip_prefix,
> tracked_pb);
> free(ip_prefix);
>
> This caught me quite off guard. Could the function copy the value from
> `*ip_prefix` instead? Or alternatively add a comment that outlines this
> behavior.
>
It kind of depends what semantics we want. In the current form
ar_alloc_entry() takes ownership of the ip_prefix you pass it. So
freeing ip_prefix outside afterwards would break the semantics.
> I also noticed that in later patches in this series, in function
> `publish_host_routes`, there's code like this:
>
> struct ovn_port_routable_addresses addrs = get_op_addresses(
> port->peer, lr_stateful_rec, false);
> for (size_t i = 0; i < addrs.n_addrs; i++) {
> publish_lport_addresses(sync_routes, route->od->sb,
> route->out_port,
> &addrs.laddrs[i],
> port->peer);
> }
> destroy_routable_addresses(&addrs);
>
> Here, the `&addrs.laddrs[i]` are passed to `publish_lport_addresses`
> which eventually passes `&addrs.laddrs[i]->ipv4_addrs[]` into
> `ar_alloc_entry` and then it destroys the `&addrs`. I'm bit usnsure if
> it's the way that `destroy_routable_addresses` destroys `&addrs`, or if
> it's just accidental, but in this case the north is not crashing.
>
The difference in this case is that the call is:
const struct ipv4_netaddr *addr = &addresses->ipv4_addrs[i];
char *addr_s = xasprintf("%s/32", addr->addr_s);
ar_alloc_entry(sync_routes, sb_db, logical_port->sb,
addr_s, tracking_port->sb);
So ar_alloc_entry() is getting again full ownership of the addr_s we
built (and allocated) with the xasprintf() call.
Hope this helps.
Dumitru
> Martin.
>
>>
>>>> + return route_e;
>>>> +}
>>>> +
>>>> +static struct ar_entry *
>>>> +ar_find(struct hmap *route_map,
>>>> + const struct sbrec_datapath_binding *sb_db,
>>>> + const struct sbrec_port_binding
>>>> *logical_port,
>>>> + const char *ip_prefix)
>>>
>>> Nit: indentation
>>>
>>>> +{
>>>> + struct ar_entry *route_e;
>>>> + uint32_t hash;
>>>> +
>>>> + hash = uuid_hash(&sb_db->header_.uuid);
>>>> + hash = hash_string(logical_port->logical_port, hash);
>>>> + hash = hash_string(ip_prefix, hash);
>>>> + HMAP_FOR_EACH_WITH_HASH (route_e, hmap_node, hash,
>>>> route_map) {
>>>> + if (!uuid_equals(&sb_db->header_.uuid,
>>>> + &route_e->sb_db->header_.uuid)) {
>>>> + continue;
>>>> + }
>>>> +
>>>> + if (!uuid_equals(&logical_port->header_.uuid,
>>>> + &route_e->logical_port->header_.uuid))
>>>> {
>>>> + continue;
>>>> + }
>>>> +
>>>> + if (strcmp(ip_prefix, route_e->ip_prefix)) {
>>>> + continue;
>>>> + }
>>>> +
>>>> + return route_e;
>>>> + }
>>>> +
>>>> + return NULL;
>>>> +}
>>>> +
>>>> +static void
>>>> +ar_entry_free(struct ar_entry *route_e)
>>>> +{
>>>> + free(route_e->ip_prefix);
>>>> + free(route_e);
>>>> +}
>>>> +
>>>> +static void
>>>> +advertised_route_table_sync(
>>>> + struct ovsdb_idl_txn *ovnsb_txn,
>>>> + const struct sbrec_advertised_route_table
>>>> *sbrec_advertised_route_table,
>>>> + const struct hmap *parsed_routes)
>>>> +{
>>>> + struct hmap sync_routes = HMAP_INITIALIZER(&sync_routes);
>>>> +
>>>
>>> Nit: no need for an empty line IMO.
>>>
>>>> + const struct parsed_route *route;
>>>> +
>>>> + struct ar_entry *route_e;
>>>> + const struct sbrec_advertised_route *sb_route;
>>>> + HMAP_FOR_EACH (route, key_node, parsed_routes) {
>>>> + if (route->is_discard_route) {
>>>> + continue;
>>>> + }
>>>> + if (prefix_is_link_local(&route->prefix, route->plen)) {
>>>> + continue;
>>>> + }
>>>> + if (!smap_get_bool(&route->od->nbr->options, "dynamic-
>>>> routing",
>>>> + false)) {
>>>> + continue;
>>>> + }
>>>
>>> This should be:
>>>
>>> if (!route->od->dynamic_routing) {
>>> continue;
>>> }
>>>
>>> instead.
>>>
>>>> +
>>>> + char *ip_prefix = normalize_v46_prefix(&route->prefix,
>>>> + route->plen);
>>>> + route_e = ar_alloc_entry(&sync_routes, route->od->sb,
>>>> + route->out_port->sb,
>>>> ip_prefix);
>>>> + }
>>>> +
>>>> + SBREC_ADVERTISED_ROUTE_TABLE_FOR_EACH_SAFE (sb_route,
>>>> +
>>>> sbrec_advertised_route_table) {
>>>> + route_e = ar_find(&sync_routes, sb_route->datapath,
>>>> + sb_route->logical_port,
>>>> + sb_route->ip_prefix);
>>>> + if (route_e) {
>>>> + hmap_remove(&sync_routes, &route_e->hmap_node);
>>>> + ar_entry_free(route_e);
>>>> + } else {
>>>> + sbrec_advertised_route_delete(sb_route);
>>>> + }
>>>> + }
>>>> +
>>>> + HMAP_FOR_EACH_POP (route_e, hmap_node, &sync_routes) {
>>>> + const struct sbrec_advertised_route *sr =
>>>> + sbrec_advertised_route_insert(ovnsb_txn);
>>>> + sbrec_advertised_route_set_datapath(sr, route_e->sb_db);
>>>> + sbrec_advertised_route_set_logical_port(sr, route_e-
>>>>> logical_port);
>>>> + sbrec_advertised_route_set_ip_prefix(sr, route_e-
>>>>> ip_prefix);
>>>> + ar_entry_free(route_e);
>>>> + }
>>>> +
>>>> + hmap_destroy(&sync_routes);
>>>> +}
>>>> +
>>>> diff --git a/northd/en-advertised-route-sync.h b/northd/en-
>>>> advertised-route-sync.h
>>>> new file mode 100644
>>>> index 000000000..bd27ecefa
>>>> --- /dev/null
>>>> +++ b/northd/en-advertised-route-sync.h
>>>> @@ -0,0 +1,29 @@
>>>> +/*
>>>> + * Copyright (c) 2024, STACKIT GmbH & Co. KG
>>>
>>> Nit: 2025
>>>
>>>> + *
>>>> + * Licensed under the Apache License, Version 2.0 (the
>>>> "License");
>>>> + * you may not use this file except in compliance with the
>>>> License.
>>>> + * You may obtain a copy of the License at:
>>>> + *
>>>> + * http://www.apache.org/licenses/LICENSE-2.0
>>>> + *
>>>> + * Unless required by applicable law or agreed to in writing,
>>>> software
>>>> + * distributed under the License is distributed on an "AS IS"
>>>> BASIS,
>>>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
>>>> or implied.
>>>> + * See the License for the specific language governing
>>>> permissions and
>>>> + * limitations under the License.
>>>> + */
>>>> +#ifndef EN_ADVERTISED_ROUTE_SYNC_H
>>>> +#define EN_ADVERTISED_ROUTE_SYNC_H 1
>>>> +
>>>> +#include "lib/inc-proc-eng.h"
>>>> +
>>>> +struct advertised_route_sync_data {
>>>> +};
>>>> +
>>>> +void *en_advertised_route_sync_init(struct engine_node *, struct
>>>> engine_arg *);
>>>> +void en_advertised_route_sync_cleanup(void *data);
>>>> +void en_advertised_route_sync_run(struct engine_node *, void
>>>> *data);
>>>> +
>>>> +
>>>
>>> Nit: one empty line too many.
>>>
>>>> +#endif /* EN_ADVERTISED_ROUTE_SYNC_H */
>>>> diff --git a/northd/en-northd-output.c b/northd/en-northd-
>>>> output.c
>>>> index 98098d974..555ed2b9e 100644
>>>> --- a/northd/en-northd-output.c
>>>> +++ b/northd/en-northd-output.c
>>>> @@ -72,3 +72,11 @@ northd_output_fdb_aging_handler(struct
>>>> engine_node *node,
>>>> engine_set_node_state(node, EN_UPDATED);
>>>> return true;
>>>> }
>>>> +
>>>> +bool
>>>> +northd_output_advertised_route_sync_handler(struct engine_node
>>>> *node,
>>>> + void *data
>>>> OVS_UNUSED)
>>>> +{
>>>> + engine_set_node_state(node, EN_UPDATED);
>>>> + return true;
>>>> +}
>>>> diff --git a/northd/en-northd-output.h b/northd/en-northd-
>>>> output.h
>>>> index 5f577b89c..00391ace3 100644
>>>> --- a/northd/en-northd-output.h
>>>> +++ b/northd/en-northd-output.h
>>>> @@ -17,5 +17,7 @@ bool
>>>> northd_output_mac_binding_aging_handler(struct engine_node *node,
>>>> void *data
>>>> OVS_UNUSED);
>>>> bool northd_output_fdb_aging_handler(struct engine_node *node,
>>>> void *data OVS_UNUSED);
>>>> +bool northd_output_advertised_route_sync_handler(struct
>>>> engine_node *node,
>>>> + void *data
>>>> OVS_UNUSED);
>>>>
>>>> #endif
>>>> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
>>>> index 6e0aa04c4..12956a5de 100644
>>>> --- a/northd/inc-proc-northd.c
>>>> +++ b/northd/inc-proc-northd.c
>>>> @@ -41,6 +41,7 @@
>>>> #include "en-sampling-app.h"
>>>> #include "en-sync-sb.h"
>>>> #include "en-sync-from-sb.h"
>>>> +#include "en-advertised-route-sync.h"
>>>> #include "unixctl.h"
>>>> #include "util.h"
>>>>
>>>> @@ -102,7 +103,8 @@ static unixctl_cb_func chassis_features_list;
>>>> SB_NODE(fdb, "fdb") \
>>>> SB_NODE(static_mac_binding, "static_mac_binding") \
>>>> SB_NODE(chassis_template_var, "chassis_template_var") \
>>>> - SB_NODE(logical_dp_group, "logical_dp_group")
>>>> + SB_NODE(logical_dp_group, "logical_dp_group") \
>>>> + SB_NODE(advertised_route, "advertised_route")
>>>>
>>>> enum sb_engine_node {
>>>> #define SB_NODE(NAME, NAME_STR) SB_##NAME,
>>>> @@ -161,6 +163,7 @@ static ENGINE_NODE(route_policies,
>>>> "route_policies");
>>>> static ENGINE_NODE(routes, "routes");
>>>> static ENGINE_NODE(bfd, "bfd");
>>>> static ENGINE_NODE(bfd_sync, "bfd_sync");
>>>> +static ENGINE_NODE(advertised_route_sync,
>>>> "advertised_route_sync");
>>>>
>>>> void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>>>> struct ovsdb_idl_loop *sb)
>>>> @@ -263,6 +266,10 @@ void inc_proc_northd_init(struct
>>>> ovsdb_idl_loop *nb,
>>>> engine_add_input(&en_bfd_sync, &en_route_policies, NULL);
>>>> engine_add_input(&en_bfd_sync, &en_northd,
>>>> bfd_sync_northd_change_handler);
>>>>
>>>> + engine_add_input(&en_advertised_route_sync, &en_routes,
>>>> NULL);
>>>> + engine_add_input(&en_advertised_route_sync,
>>>> &en_sb_advertised_route,
>>>> + NULL);
>>>> +
>>>> engine_add_input(&en_sync_meters, &en_nb_acl, NULL);
>>>> engine_add_input(&en_sync_meters, &en_nb_meter, NULL);
>>>> engine_add_input(&en_sync_meters, &en_sb_meter, NULL);
>>>> @@ -344,6 +351,8 @@ void inc_proc_northd_init(struct
>>>> ovsdb_idl_loop *nb,
>>>> northd_output_mac_binding_aging_handler);
>>>> engine_add_input(&en_northd_output, &en_fdb_aging,
>>>> northd_output_fdb_aging_handler);
>>>> + engine_add_input(&en_northd_output,
>>>> &en_advertised_route_sync,
>>>> +
>>>> northd_output_advertised_route_sync_handler);
>>>>
>>>> struct engine_arg engine_arg = {
>>>> .nb_idl = nb->idl,
>>>> diff --git a/northd/northd.c b/northd/northd.c
>>>> index 74014de32..1c32ef8f8 100644
>>>> --- a/northd/northd.c
>>>> +++ b/northd/northd.c
>>>> @@ -912,6 +912,8 @@ join_datapaths(const struct
>>>> nbrec_logical_switch_table *nbrec_ls_table,
>>>> if (smap_get(&od->nbr->options, "chassis")) {
>>>> od->is_gw_router = true;
>>>> }
>>>> + od->dynamic_routing = smap_get_bool(&od->nbr->options,
>>>> + "dynamic-routing",
>>>> false);
>>>> ovs_list_push_back(lr_list, &od->lr_list);
>>>> }
>>>> }
>>>> @@ -11127,7 +11129,8 @@ route_hash(struct parsed_route *route)
>>>> }
>>>>
>>>> static bool
>>>> -find_static_route_outport(struct ovn_datapath *od, const struct
>>>> hmap *lr_ports,
>>>> +find_static_route_outport(const struct ovn_datapath *od,
>>>> + const struct hmap *lr_ports,
>>>> const struct nbrec_logical_router_static_route *route, bool
>>>> is_ipv4,
>>>> const char **p_lrp_addr_s, struct ovn_port **p_out_port);
>>>>
>>>> @@ -11229,7 +11232,7 @@ parsed_route_add(const struct
>>>> ovn_datapath *od,
>>>> new_pr->route_table_id = route_table_id;
>>>> new_pr->is_src_route = is_src_route;
>>>> new_pr->hash = route_hash(new_pr);
>>>> - new_pr->nbr = od->nbr;
>>>> + new_pr->od = od;
>>>> new_pr->ecmp_symmetric_reply = ecmp_symmetric_reply;
>>>> new_pr->is_discard_route = is_discard_route;
>>>> if (!is_discard_route) {
>>>> @@ -11255,11 +11258,12 @@ parsed_route_add(const struct
>>>> ovn_datapath *od,
>>>> }
>>>>
>>>> static void
>>>> -parsed_routes_add_static(struct ovn_datapath *od, const struct
>>>> hmap *lr_ports,
>>>> - const struct nbrec_logical_router_static_route
>>>> *route,
>>>> - const struct hmap *bfd_connections,
>>>> - struct hmap *routes, struct simap
>>>> *route_tables,
>>>> - struct hmap *bfd_active_connections)
>>>> +parsed_routes_add_static(const struct ovn_datapath *od,
>>>> + const struct hmap *lr_ports,
>>>> + const struct
>>>> nbrec_logical_router_static_route *route,
>>>> + const struct hmap *bfd_connections,
>>>> + struct hmap *routes, struct simap
>>>> *route_tables,
>>>> + struct hmap *bfd_active_connections)
>>>> {
>>>> /* Verify that the next hop is an IP address with an all-
>>>> ones mask. */
>>>> struct in6_addr *nexthop = NULL;
>>>> @@ -11381,7 +11385,8 @@ parsed_routes_add_static(struct
>>>> ovn_datapath *od, const struct hmap *lr_ports,
>>>> }
>>>>
>>>> static void
>>>> -parsed_routes_add_connected(struct ovn_datapath *od, const
>>>> struct ovn_port *op,
>>>> +parsed_routes_add_connected(const struct ovn_datapath *od,
>>>> + const struct ovn_port *op,
>>>> struct hmap *routes)
>>>> {
>>>> for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
>>>> @@ -11410,14 +11415,14 @@ parsed_routes_add_connected(struct
>>>> ovn_datapath *od, const struct ovn_port *op,
>>>> }
>>>>
>>>> void
>>>> -build_parsed_routes(struct ovn_datapath *od, const struct hmap
>>>> *lr_ports,
>>>> +build_parsed_routes(const struct ovn_datapath *od, const struct
>>>> hmap *lr_ports,
>>>> const struct hmap *bfd_connections, struct
>>>> hmap *routes,
>>>> struct simap *route_tables,
>>>> struct hmap *bfd_active_connections)
>>>> {
>>>> struct parsed_route *pr;
>>>> HMAP_FOR_EACH (pr, key_node, routes) {
>>>> - if (pr->nbr == od->nbr) {
>>>> + if (pr->od == od) {
>>>> pr->stale = true;
>>>> }
>>>> }
>>>> @@ -11641,13 +11646,15 @@ build_route_match(const struct ovn_port
>>>> *op_inport, uint32_t rtb_id,
>>>>
>>>> /* Output: p_lrp_addr_s and p_out_port. */
>>>> static bool
>>>> -find_static_route_outport(struct ovn_datapath *od, const struct
>>>> hmap *lr_ports,
>>>> +find_static_route_outport(const struct ovn_datapath *od,
>>>> + const struct hmap *lr_ports,
>>>> const struct nbrec_logical_router_static_route *route, bool
>>>> is_ipv4,
>>>> const char **p_lrp_addr_s, struct ovn_port **p_out_port)
>>>> {
>>>> const char *lrp_addr_s = NULL;
>>>> struct ovn_port *out_port = NULL;
>>>> if (route->output_port) {
>>>> + /* XXX: we should be able to use &od->ports instead of
>>>> lr_ports. */
>>>> out_port = ovn_port_find(lr_ports, route->output_port);
>>>> if (!out_port) {
>>>> static struct vlog_rate_limit rl =
>>>> VLOG_RATE_LIMIT_INIT(5, 1);
>>>> diff --git a/northd/northd.h b/northd/northd.h
>>>> index 9457a7be6..98a17d733 100644
>>>> --- a/northd/northd.h
>>>> +++ b/northd/northd.h
>>>> @@ -367,6 +367,8 @@ struct ovn_datapath {
>>>>
>>>> /* router datapath has a logical port with redirect-type set
>>>> to bridged. */
>>>> bool redirect_bridged;
>>>> + /* nbr has the option "dynamic-routing" set to true. */
>>>> + bool dynamic_routing;
>>>>
>>>> struct ovn_port **localnet_ports;
>>>> size_t n_localnet_ports;
>>>> @@ -714,7 +716,7 @@ struct parsed_route {
>>>> const struct nbrec_logical_router_static_route *route;
>>>> bool ecmp_symmetric_reply;
>>>> bool is_discard_route;
>>>> - const struct nbrec_logical_router *nbr;
>>>> + const struct ovn_datapath *od;
>>>> bool stale;
>>>> struct sset ecmp_selection_fields;
>>>> enum route_source source;
>>>> @@ -745,7 +747,7 @@ void northd_indices_create(struct northd_data
>>>> *data,
>>>>
>>>> void route_policies_init(struct route_policies_data *);
>>>> void route_policies_destroy(struct route_policies_data *);
>>>> -void build_parsed_routes(struct ovn_datapath *, const struct
>>>> hmap *,
>>>> +void build_parsed_routes(const struct ovn_datapath *, const
>>>> struct hmap *,
>>>> const struct hmap *, struct hmap *,
>>>> struct simap *,
>>>> struct hmap *);
>>>> uint32_t get_route_table_id(struct simap *, const char *);
>>>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>>>> index aa4c57663..17d6145be 100644
>>>> --- a/northd/ovn-northd.c
>>>> +++ b/northd/ovn-northd.c
>>>> @@ -941,6 +941,10 @@ main(int argc, char *argv[])
>>>> ovsdb_idl_omit_alert(ovnsb_idl_loop.idl,
>>>>
>>>> &sbrec_logical_dp_group_columns[i]);
>>>> }
>>>> + for (size_t i = 0; i < SBREC_ADVERTISED_ROUTE_N_COLUMNS;
>>>> i++) {
>>>> + ovsdb_idl_omit_alert(ovnsb_idl_loop.idl,
>>>> +
>>>> &sbrec_advertised_route_columns[i]);
>>>> + }
>>>>
>>>> unixctl_command_register("sb-connection-status", "", 0, 0,
>>>> ovn_conn_show, ovnsb_idl_loop.idl);
>>>> @@ -968,6 +972,7 @@ main(int argc, char *argv[])
>>>> stopwatch_create(LR_NAT_RUN_STOPWATCH_NAME, SW_MS);
>>>> stopwatch_create(LR_STATEFUL_RUN_STOPWATCH_NAME, SW_MS);
>>>> stopwatch_create(LS_STATEFUL_RUN_STOPWATCH_NAME, SW_MS);
>>>> + stopwatch_create(ADVERTISED_ROUTE_SYNC_RUN_STOPWATCH_NAME,
>>>> SW_MS);
>>>>
>>>> /* Initialize incremental processing engine for ovn-northd
>>>> */
>>>> inc_proc_northd_init(&ovnnb_idl_loop, &ovnsb_idl_loop);
>>>> diff --git a/ovn-nb.xml b/ovn-nb.xml
>>>> index 24ef12f3b..718d0126e 100644
>>>> --- a/ovn-nb.xml
>>>> +++ b/ovn-nb.xml
>>>> @@ -2946,6 +2946,19 @@ or
>>>> option is not present the limit is not set and the zone
>>>> limit is
>>>> derived from OvS default datapath limit.
>>>> </column>
>>>> +
>>>> + <column name="options" key="dynamic-routing"
>>>> type='{"type": "boolean"}'>
>>>> + 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
>>>
>>> This doesn't render that nicely. The resulting man page is content
>>> is
>>> something like:
>>>
>>> includes: * all "connected" routes implicitly
>>> created by
>>> networks associated with this Logical Router * all
>>> Logical_Router_Static_Route that are applied to this
>>> Logical Router
>>>
>>> If you change this block to:
>>>
>>> <ul>
>>> <li>
>>> all "connected" routes implicitly created by networks
>>> associated with this Logical Router
>>> </li>
>>> <li>
>>> all <ref table="Logical_Router_Static_Route"/> that are
>>> applied to this Logical Router
>>> </li>
>>> </ul>
>>>
>>> The generated man page looks slightly better IMO:
>>>
>>> table that are relevant for the router. This
>>> includes:
>>>
>>> • all "connected" routes implicitly created by
>>> networks associated with this Logical Router
>>>
>>> • all Logical_Router_Static_Route that are
>>> applied
>>> to this Logical Router
>>>
>>>> + </column>
>>>> </group>
>>>>
>>>> <group title="Common Columns">
>>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>>>> index edfd5764b..30ea23502 100644
>>>> --- a/tests/ovn-northd.at
>>>> +++ b/tests/ovn-northd.at
>>>> @@ -14419,3 +14419,140 @@ AT_CHECK([ovn-sbctl lflow-list S1 |
>>>> grep ls_out_acl_action | grep priority=500 |
>>>>
>>>> AT_CLEANUP
>>>> ])
>>>> +
>>>> +OVN_FOR_EACH_NORTHD_NO_HV([
>>>> +AT_SETUP([dynamic-routing - sync to sb])
>>>> +AT_KEYWORDS([dynamic-routing])
>>>> +ovn_start
>>>> +
>>>> +# adding a router - still nothing here
>>>
>>> Nit: comments should be sentences.
>>>
>>>> +check ovn-nbctl lr-add lr0
>>>> +check ovn-nbctl --wait=sb set Logical_Router lr0 option:dynamic-
>>>> routing=true
>>>> +check_row_count Advertised_Route 0
>>>> +datapath=$(fetch_column datapath_binding _uuid
>>>> external_ids:name=lr0)
>>>> +
>>>> +# adding a LRP adds a route entry for the associated network
>>>> +check ovn-nbctl --wait=sb lrp-add lr0 lr0-sw0 00:00:00:00:ff:01
>>>> 10.0.0.1/24
>>>> +pb=$(fetch_column port_binding _uuid logical_port=lr0-sw0)
>>>> +check_row_count Advertised_Route 1
>>>> +check_column 10.0.0.0/24 Advertised_Route ip_prefix
>>>> datapath=$datapath logical_port=$pb
>>>> +
>>>> +# adding a second LRP adds an additional route entry
>>>> +check ovn-nbctl --wait=sb lrp-add lr0 lr0-sw1 00:00:00:00:ff:02
>>>> 10.0.1.1/24
>>>> +pb2=$(fetch_column port_binding _uuid logical_port=lr0-sw1)
>>>> +check_row_count Advertised_Route 2
>>>> +check_column 10.0.0.0/24 Advertised_Route ip_prefix
>>>> datapath=$datapath logical_port=$pb
>>>> +check_column 10.0.1.0/24 Advertised_Route ip_prefix
>>>> datapath=$datapath logical_port=$pb2
>>>> +
>>>> +# adding a static route adds an additional entry
>>>> +check ovn-nbctl --wait=sb lr-route-add lr0 192.168.0.0/24
>>>> 10.0.0.10
>>>> +check_row_count Advertised_Route 3
>>>> +check_row_count Advertised_Route 2 logical_port=$pb
>>>> +check_row_count Advertised_Route 1 logical_port=$pb
>>>> ip_prefix=192.168.0.0/24
>>>> +
>>>> +# adding an ipv6 LRP adds an addition route entry
>>>> +check ovn-nbctl --wait=sb lrp-add lr0 lr0-sw2 00:00:00:00:ff:03
>>>> 2001:db8::1/64 fe80::1/64
>>>> +pb3=$(fetch_column port_binding _uuid logical_port=lr0-sw2)
>>>> +check_row_count Advertised_Route 4
>>>> +check_row_count Advertised_Route 2 logical_port=$pb
>>>> +check_row_count Advertised_Route 1 logical_port=$pb
>>>> ip_prefix=192.168.0.0/24
>>>> +check_column 10.0.1.0/24 Advertised_Route ip_prefix
>>>> datapath=$datapath logical_port=$pb2
>>>> +check_column 2001:db8::/64 Advertised_Route ip_prefix
>>>> datapath=$datapath logical_port=$pb3
>>>> +
>>>> +# removing the option:dynamic-routing removes all routes
>>>> +check ovn-nbctl --wait=sb remove Logical_Router lr0 option
>>>> dynamic-routing
>>>> +check_row_count Advertised_Route 0
>>>> +
>>>> +# and setting it again adds them again
>>>> +check ovn-nbctl --wait=sb set Logical_Router lr0 option:dynamic-
>>>> routing=true
>>>> +check_row_count Advertised_Route 4
>>>> +
>>>> +# removing the lrp used for the static route removes both route
>>>> entries
>>>> +check ovn-nbctl --wait=sb lrp-del lr0-sw0
>>>> +check_row_count Advertised_Route 2
>>>> +check_row_count Advertised_Route 1 logical_port=$pb2
>>>> +check_row_count Advertised_Route 1 logical_port=$pb3
>>>> +
>>>> +# removing the lr will remove all routes
>>>> +check ovn-nbctl --wait=sb lr-del lr0
>>>> +check_row_count Advertised_Route 0
>>>> +
>>>> +AT_CLEANUP
>>>> +])
>>>> +
>>>> +OVN_FOR_EACH_NORTHD_NO_HV([
>>>> +AT_SETUP([dynamic-routing incremental processing])
>>>> +AT_KEYWORDS([dynamic-routing])
>>>> +ovn_start
>>>> +
>>>> +# Test I-P for dynamic-routing.
>>>> +# Presently ovn-northd has no I-P for Advertised_Route.
>>>> +# Wait for sb to be connected before clearing stats.
>>>> +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_engine_stats northd recompute nocompute
>>>> +check_engine_stats routes recompute nocompute
>>>> +check_engine_stats advertised_route_sync recompute nocompute
>>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
>>>
>>> CHECK_NO_CHANGE_AFTER_RECOMPUTE(1) also waits for logical ports to
>>> be
>>> up. We have no ports in the configuration at this point. This can
>>> be a
>>> plain:
>>>
>>> CHECK_NO_CHANGE_AFTER_RECOMPUTE
>>>
>>>> +
>>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>>>> +check ovn-nbctl --wait=sb lrp-add lr0 lr0-sw0 00:00:00:00:ff:01
>>>> 10.0.0.1/24
>>>> +check_engine_stats northd recompute compute
>>>> +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-add lr0 lr0-sw1 00:00:00:00:ff:02
>>>> 10.0.1.1/24
>>>> +check_engine_stats northd recompute compute
>>>> +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 lr-route-add lr0 192.168.0.0/24
>>>> 10.0.0.10
>>>> +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-add lr0 lr0-sw2 00:00:00:00:ff:03
>>>> 2001:db8::1/64 fe80::1/64
>>>> +check_engine_stats northd recompute compute
>>>> +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
>>>> +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 lr0 option:dynamic-
>>>> routing=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
>>>> +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 lr-del lr0
>>>> +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
>>>> +
>>>> +AT_CLEANUP
>>>> +])
>>>
>>> Regards,
>>> Dumitru
>>>
>>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
