On Mon, Jan 20, 2025 at 01:49:24PM +0100, Dumitru Ceara wrote:
> 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?
Will do that.
Thanks.
>
> >>
> >>> 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
