On 1/20/25 11:32 AM, Felix Huettner wrote:
> On Mon, Jan 13, 2025 at 01:30:06PM +0100, Dumitru Ceara wrote:
>> Hi Felix,
>>
>> On 12/18/24 11:24 AM, Felix Huettner via dev wrote:
>>> in order to exchange routes between OVN and the network fabric we
>>> introduce a new southbound table. This is used by northd to write in the
>>> routes which should be announced from a given Logical Router.
>>>
>>
>> This commit message is a bit outdated. We already merged the schema change.
>>
>>> ovn-controller will later use this table to share these routes to the
>>> outside.
>>>
>>> Additionally this table will be used as a way for ovn-controller to
>>> share learned routes back to northd.
>>>
>>> Users must explicitly opt-in to advertise the routes using this table.
>>>
>>> Signed-off-by: Felix Huettner <[email protected]>
>>> ---
>>
>> Overall the changes look correct but I have some comments/suggestions.
>> Please see below.
>
> Hi Dumitru,
>
> thanks a lot for the review. All of the comments will be addressed in
> v3. I'll just comment below on the larger ones.
>>
>>> NEWS | 3 +
>>> 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 | 202 ++++++++++++++++++++++++++++++
>>> northd/en-advertised-route-sync.h | 27 ++++
>>> northd/en-northd-output.c | 8 ++
>>> northd/en-northd-output.h | 2 +
>>> northd/inc-proc-northd.c | 11 +-
>>> northd/northd.c | 24 ++--
>>> northd/northd.h | 4 +-
>>> ovn-nb.xml | 13 ++
>>> tests/ovn-northd.at | 57 +++++++++
>>> 15 files changed, 365 insertions(+), 34 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 d2b3ee460..342b91e96 100644
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -12,6 +12,9 @@ Post v24.09.0
>>> and "routing-protocols" are now also usable on distributed gateway
>>> ports.
>>> - ovn-nb: Changed schema of ovn-nb to make networks optional within
>>> Logical
>>> Router Ports.
>>> + - 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 c95b556f8..659b571ce 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));
>>> +}
>>> +
>>> 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..46ae3adf8
>>> --- /dev/null
>>> +++ b/northd/en-advertised-route-sync.c
>>> @@ -0,0 +1,202 @@
>>> +/*
>>
>> Missing copyright.
>>
>>> + * 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 "openvswitch/vlog.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"
>>> +
>>> +VLOG_DEFINE_THIS_MODULE(en_advertised_route_sync);
>>
>> We don't really log anything in this module. Should we remove this?
>>
>>> +
>>> +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());
>>
>> We're missing a stopwatch_create() in main().
>>
>>> +
>>> + 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_advertised_route *sb_route;
>>> + const struct sbrec_datapath_binding *sb_db;
>>> +
>>> + const struct sbrec_port_binding *logical_port;
>>> + char *ip_prefix;
>>> + bool stale;
>>> +};
>>> +
>>> +static struct ar_entry *
>>> +ar_alloc_entry(struct hmap *routes,
>>> + const struct sbrec_datapath_binding *sb_db,
>>> + const struct sbrec_port_binding *logical_port,
>>> + const 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 = xstrdup(ip_prefix);
>>> + route_e->stale = false;
>>> + 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);
>>> +
>>> + return route_e;
>>> +}
>>> +
>>> +static struct ar_entry *
>>> +ar_lookup_or_add(struct hmap *route_map,
>>> + const struct sbrec_datapath_binding *sb_db,
>>> + const struct sbrec_port_binding *logical_port,
>>> + const char *ip_prefix)
>>> +{
>>> + 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;
>>> + }
>>> +
>>> + route_e = ar_alloc_entry(route_map, sb_db,
>>> + logical_port, ip_prefix);
>>> + return route_e;
>>> +}
>>> +
>>> +static void
>>> +route_erase_entry(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)
>>> +{
>>> + if (!ovnsb_txn) {
>>
>> ovnsb_txn can't be NULL here. We already check in inc_proc_northd_run().
>>
>>> + return;
>>> + }
>>> +
>>> + struct hmap sync_routes = HMAP_INITIALIZER(&sync_routes);
>>> +
>>> + const struct parsed_route *route;
>>> +
>>> + struct ar_entry *route_e;
>>
>> We can avoid the need for the route_e->stale field if we change this
>> function to:
>>
>> - create a temporary map of routes to be advertised (iterate through
>> parsed_routes)
>> - walk all SB.Advertised_Routes (use SBREC_*_FOR_EACH_SAFE) and look up
>> the corresponding entry in the temporary map built above:
>> a. the entry is found - update it if needed
>> b. the entry is not found - delete from the SB table and remove from the
>> temporary map
>> - walk all remaining entries in the temporary map (these are all items
>> that don't have a corresponding SB record) and create the
>> SB.Advertised_Route entries for them
>>
>> You can use sb_lb_table_build_and_sync() as an example of how to do things.
>
> This is nicer, thanks a lot.
>>
>>> + const struct sbrec_advertised_route *sb_route;
>>> + SBREC_ADVERTISED_ROUTE_TABLE_FOR_EACH (sb_route,
>>> + sbrec_advertised_route_table) {
>>> + route_e = ar_alloc_entry(&sync_routes,
>>> + sb_route->datapath,
>>> + sb_route->logical_port,
>>> + sb_route->ip_prefix);
>>> + route_e->stale = true;
>>> + route_e->sb_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)) {
>>
>> Nit: could we store the parsed value of this option in the 'struct
>> ovn_datapath' record? Like that we could avoid calling smap_get_bool()
>> for each and every route of a single datapath.
>>
>>> + continue;
>>> + }
>>> +
>>> + char *ip_prefix = normalize_v46_prefix(&route->prefix,
>>> + route->plen);
>>> + route_e = ar_lookup_or_add(&sync_routes, route->od->sb,
>>> + route->out_port->sb, ip_prefix);
>>> + route_e->stale = false;
>>> +
>>> + if (!route_e->sb_route) {
>>> + 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);
>>> + route_e->sb_route = sr;
>>> + }
>>> +
>>> + free(ip_prefix);
>>> + }
>>> +
>>> + HMAP_FOR_EACH_POP (route_e, hmap_node, &sync_routes) {
>>> + if (route_e->stale) {
>>> + sbrec_advertised_route_delete(route_e->sb_route);
>>> + }
>>> + route_erase_entry(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..c6a41c713
>>> --- /dev/null
>>> +++ b/northd/en-advertised-route-sync.h
>>> @@ -0,0 +1,27 @@
>>> +/*
>>
>> Missing copyright.
>>
>>> + * 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);
>>> +
>>> +
>>> +#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..77a7d637c 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,
>>> + engine_noop_handler);
>>> +
>>
>> ovn-northd is the only writer of the Advertised_Route table so we can
>> probably also ignore all updates for SB.Advertised_Route columns (this
>> improves performance slightly). See similar uses of
>> ovsdb_idl_omit_alert() in main() for Logical_Flow, Multicast_Group,
>> Meter, etc.
>>
>> In that case we can safely replace the noop_handler with 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 b01e40ecd..0b495a2b6 100644
>>> --- a/northd/northd.c
>>> +++ b/northd/northd.c
>>> @@ -11127,7 +11127,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,
>>
>> I didn't test it but now that we pass the 'od' as argument instead of
>> the NB.Logical_Router can't we get rid of using the global lr_ports map
>> and instead lookup ports in the &od->ports map?
>
> It seems like we still need lr_ports. In some cases od->ports seems to
> be null which causes multiple test errors.
> I did not investigate it further, but it seems strange to me that is can
> be NULL.
>
I see. We should definitely try to figure out why that's the case.
However we can do that as a follow up. Maybe leave a "XXX" (TODO)
comment there?
>>
>>> 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 +11230,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,7 +11256,8 @@ parsed_route_add(const struct ovn_datapath *od,
>>> }
>>>
>>> static void
>>> -parsed_routes_add_static(struct ovn_datapath *od, const struct hmap
>>> *lr_ports,
>>> +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,
>>> @@ -11381,7 +11383,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 +11413,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,
>>> - const struct hmap *bfd_connections, struct hmap
>>> *routes,
>>> - struct simap *route_tables,
>>> - struct hmap *bfd_active_connections)
>>> +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,7 +11644,8 @@ 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)
>>> {
>>> diff --git a/northd/northd.h b/northd/northd.h
>>> index 9457a7be6..2be34e249 100644
>>> --- a/northd/northd.h
>>> +++ b/northd/northd.h
>>> @@ -714,7 +714,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 +745,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/ovn-nb.xml b/ovn-nb.xml
>>> index 8373ddb99..7f17c8059 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
>>> + </column>
>>> </group>
>>>
>>> <group title="Common Columns">
>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>>> index 21d9d63ab..b677c4b87 100644
>>> --- a/tests/ovn-northd.at
>>> +++ b/tests/ovn-northd.at
>>> @@ -14384,3 +14384,60 @@ 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
>>> +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=$(ovn-sbctl --bare --columns _uuid list datapath_binding lr0)
>>
>> IMO it's cleaner if we use fetch_column, e.g.:
>>
>> 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=$(ovn-sbctl --bare --columns _uuid list port_binding lr0-sw0)
>>
>> Same comment about fetch_column here.
>>
>>> +check_row_count Advertised_Route 1
>>> +AT_CHECK([ovn-sbctl --columns ip_prefix --bare find Advertised_Route
>>> datapath=$datapath logical_port=$pb], [0], [dnl
>>> +10.0.0.0/24
>>> +])
>>
>> The check_column/check_row_count is the cleaner way of performing such
>> checks.
>>
>>> +
>>> +# 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=$(ovn-sbctl --bare --columns _uuid list port_binding lr0-sw1)
>>> +check_row_count Advertised_Route 2
>>> +AT_CHECK([ovn-sbctl --columns ip_prefix --bare find Advertised_Route
>>> datapath=$datapath logical_port=$pb], [0], [dnl
>>> +10.0.0.0/24
>>> +])
>>
>> check_column/check_row_count
>>
>>> +AT_CHECK([ovn-sbctl --columns ip_prefix --bare find Advertised_Route
>>> datapath=$datapath logical_port=$pb2], [0], [dnl
>>> +10.0.1.0/24
>>> +])
>>
>> check_column/check_row_count
>>
>>> +
>>> +# 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
>>> +
>>> +# 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 3
>>> +
>>> +# 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 1
>>> +check_row_count Advertised_Route 1 logical_port=$pb2
>>> +
>>> +# removing the lr will remove all routes
>>> +check ovn-nbctl --wait=sb lr-del lr0
>>> +check_row_count Advertised_Route 0
>>> +
>>
>> What about IPv6?
>>
>> Also, we should probably add a test for link local as we skip
>> advertising those.
>
> I added these.
>
>>
>>> +AT_CLEANUP
>>> +])
>>> +
>>
>> We're also missing incremental processing tests. Could you please add
>> some too? See AT_SETUP([Port group incremental processing]) or
>> AT_SETUP([Load balancer incremental processing]) for some examples.
>
> I added incremental processing testing. However since there currently is
> no real incremental processing on lr/lrp changes the test is currently
> not validating too much.
>
That's fine, I think. We're validating that there's no I-P for these
but we can update the test when we add incremental processing support.
>
> Thanks a lot for the review,
> Felix
>
No problem.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
