On 3/4/25 6:19 PM, Mark Michelson wrote: > In current OVN, the en-northd node (via code in northd.c) takes full > control of syncing logical switches and logical routers with southbound > datapath bindings. This is fine, so long as: > 1) These are the only datapath types to sync. > 2) northd will always be the arbiter of datapath syncing. > > However, future commits will introduce new types of datapaths. These are > not good fits for the en-northd node, since they have completely > independent processing rules, and trying to shoehorn them into a struct > ovn_datapath would be wasteful. > > This patch introduces a new way of syncing datapaths. Each datapath type > has a node that is responsible for creating an ovn_unsynced_datapath_map > for the type of datapath it creates. Then a type-agnostic datapath > syncing node syncs these datapaths with the southbound Datapath_Binding > type. Then, these synced datapaths are fed back into type-specific > datapath nodes, which translate these synced datapaths into specific > types. > > Nodes can then use these as inputs if they need synced datapaths (i.e. a > northbound type with its corresponding southbound type). In this case, > en_northd uses the synced logical switch and logical router types in > order to create its ovn_datapath structures. > > Doing this will provide an easy way to sync new datapath types to the > southbound database. > > Signed-off-by: Mark Michelson <[email protected]> > ---
Hi Mark, Thanks for the refactor! > TODO.rst | 11 + > northd/automake.mk | 8 + > northd/datapath_sync.c | 57 +++++ > northd/datapath_sync.h | 77 ++++++ > northd/en-datapath-logical-router.c | 172 ++++++++++++++ > northd/en-datapath-logical-router.h | 47 ++++ > northd/en-datapath-logical-switch.c | 173 ++++++++++++++ > northd/en-datapath-logical-switch.h | 47 ++++ > northd/en-datapath-sync.c | 314 +++++++++++++++++++++++++ > northd/en-datapath-sync.h | 30 +++ > northd/en-global-config.c | 11 + > northd/en-northd.c | 40 ++++ > northd/en-northd.h | 2 + > northd/inc-proc-northd.c | 26 +++ > northd/northd.c | 351 +++------------------------- > northd/northd.h | 6 +- > 16 files changed, 1054 insertions(+), 318 deletions(-) > create mode 100644 northd/datapath_sync.c > create mode 100644 northd/datapath_sync.h > create mode 100644 northd/en-datapath-logical-router.c > create mode 100644 northd/en-datapath-logical-router.h > create mode 100644 northd/en-datapath-logical-switch.c > create mode 100644 northd/en-datapath-logical-switch.h > create mode 100644 northd/en-datapath-sync.c > create mode 100644 northd/en-datapath-sync.h > > diff --git a/TODO.rst b/TODO.rst > index e50b1bd76..d062fe005 100644 > --- a/TODO.rst > +++ b/TODO.rst > @@ -156,3 +156,14 @@ OVN To-do List > monitoring conditions to update before we actually try to learn routes. > Otherwise we could try to add duplicated Learned_Routes and the ovnsb > commit would fail. > + > +* Datapath sync nodes > + > + * Add incremental processing to the en-datapath-logical-switch, > + en-datapath-logical-router, en-datapath-sync, and possibly the > + en-datapath-synced-logical-router and en-datapath-synced-logical-switch > + nodes. > + > + * Migrate data stored in the ovn\_datapath structure to > + ovn\_synced\_logical_router and ovn\_synced\_logical\_switch. This will > + allow for the eventual deletion of ovn\_datapath. Did you mean to say "this will allow for the eventual incremental processing of deletions of ovn_datapath"? Or something along those lines. > diff --git a/northd/automake.mk b/northd/automake.mk > index 9a7165529..bf9978dd2 100644 > --- a/northd/automake.mk > +++ b/northd/automake.mk > @@ -3,11 +3,19 @@ bin_PROGRAMS += northd/ovn-northd > northd_ovn_northd_SOURCES = \ > northd/aging.c \ > northd/aging.h \ > + northd/datapath_sync.c \ > + northd/datapath_sync.h \ > northd/debug.c \ > northd/debug.h \ > northd/northd.c \ > northd/northd.h \ > northd/ovn-northd.c \ > + northd/en-datapath-logical-switch.c \ > + northd/en-datapath-logical-switch.h \ > + northd/en-datapath-logical-router.c \ > + northd/en-datapath-logical-router.h \ > + northd/en-datapath-sync.c \ > + northd/en-datapath-sync.h \ > northd/en-ecmp-nexthop.c \ > northd/en-ecmp-nexthop.h \ > northd/en-global-config.c \ > diff --git a/northd/datapath_sync.c b/northd/datapath_sync.c > new file mode 100644 > index 000000000..fcffb71aa > --- /dev/null > +++ b/northd/datapath_sync.c > @@ -0,0 +1,57 @@ > +/* Copyright (c) 2025, Red Hat, Inc. > + * > + * 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 "datapath_sync.h" > + > +struct ovn_unsynced_datapath * > +ovn_unsynced_datapath_alloc(const char *name, uint32_t requested_tunnel_key, > + const struct ovsdb_idl_row *nb_row) > +{ > + struct ovn_unsynced_datapath *dp = xzalloc(sizeof *dp); > + dp->name = xstrdup(name); > + dp->requested_tunnel_key = requested_tunnel_key; > + dp->nb_row = nb_row; > + smap_init(&dp->external_ids); > + > + return dp; > +} > + > +void > +ovn_unsynced_datapath_destroy(struct ovn_unsynced_datapath *dp) > +{ > + free(dp->name); > + smap_destroy(&dp->external_ids); > +} > + > +void > +ovn_unsynced_datapath_map_init(struct ovn_unsynced_datapath_map *map, > + const char *sb_key_name) > +{ > + hmap_init(&map->dps); > + map->sb_key_name = sb_key_name; > +} > + > +void > +ovn_unsynced_datapath_map_destroy(struct ovn_unsynced_datapath_map *map) > +{ > + struct ovn_unsynced_datapath *dp; > + HMAP_FOR_EACH_POP (dp, hmap_node, &map->dps) { > + ovn_unsynced_datapath_destroy(dp); > + free(dp); > + } > + hmap_destroy(&map->dps); > +} > diff --git a/northd/datapath_sync.h b/northd/datapath_sync.h > new file mode 100644 > index 000000000..0b00cbace > --- /dev/null > +++ b/northd/datapath_sync.h > @@ -0,0 +1,77 @@ > +/* Copyright (c) 2025, Red Hat, Inc. > + * > + * 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 DATAPATH_SYNC_H > +#define DATAPATH_SYNC_H 1 > + > +#include "openvswitch/hmap.h" > +#include "openvswitch/list.h" > +#include "smap.h" > + > +/* Datapath syncing API. This file consists of utility functions > + * that can be used when syncing northbound datapath types (e.g. > + * Logical_Router and Logical_Switch) to southbound Port_Bindings. > + * > + * The basic flow of data is as such. > + * 1. A northbound type is converted into an ovn_unsynced_datapath. > + * All ovn_unsynced_datapaths are placed into an ovn_unsynced_datapath_map. > + * 2. The en_datapath_sync node takes all of the maps in as input and > + * syncs them with southbound datapath bindings. This includes allocating > + * tunnel keys across all datapath types. The output of this node is > + * ovn_synced_datapaths, which contains a list of all synced datapaths. > + * 3. A northbound type-aware node then takes the ovn_synced_datapaths, > + * and decodes the generic synced datapaths back into a type-specific > + * version (e.g. ovn_synced_logical_router). Later nodes can then consume > + * these type-specific synced datapath types in order to perform > + * further processing. > + */ > + > +/* Represents a datapath from the northbound database > + * that has not yet been synced with the southbound database. > + */ > +struct ovn_unsynced_datapath { > + struct hmap_node hmap_node; > + char *name; > + uint32_t requested_tunnel_key; > + struct smap external_ids; > + const struct ovsdb_idl_row *nb_row; > +}; > + > +struct ovn_unsynced_datapath_map { > + /* ovn_unsynced_datapath */ > + struct hmap dps; > + const char *sb_key_name; > +}; > + > +struct ovn_synced_datapath { > + struct ovs_list list_node; > + const struct ovsdb_idl_row *nb_row; > + const struct sbrec_datapath_binding *sb_dp; > +}; > + > +struct ovn_synced_datapaths { > + struct ovs_list synced_dps; > +}; > + > +struct ovn_unsynced_datapath *ovn_unsynced_datapath_alloc( > + const char *name, uint32_t requested_tunnel_key, > + const struct ovsdb_idl_row *nb_row); > +void ovn_unsynced_datapath_destroy(struct ovn_unsynced_datapath *dp); > + > +void ovn_unsynced_datapath_map_init(struct ovn_unsynced_datapath_map *map, > + const char *sb_key_name); > +void ovn_unsynced_datapath_map_destroy(struct ovn_unsynced_datapath_map > *map); > + > +#endif /* DATAPATH_SYNC_H */ > diff --git a/northd/en-datapath-logical-router.c > b/northd/en-datapath-logical-router.c > new file mode 100644 > index 000000000..b97c7c177 > --- /dev/null > +++ b/northd/en-datapath-logical-router.c > @@ -0,0 +1,172 @@ > +/* > + * Copyright (c) 2025, Red Hat, Inc. > + * > + * 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/hmap.h" > +#include "openvswitch/util.h" > + > +#include "ovn-nb-idl.h" > +#include "ovn-sb-idl.h" > +#include "aging.h" > +#include "datapath_sync.h" > +#include "en-datapath-logical-router.h" > +#include "ovn-util.h" > + > +#define LR_SB_KEY_NAME "logical-router" > + > +void * > +en_datapath_logical_router_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *args OVS_UNUSED) > +{ > + struct ovn_unsynced_datapath_map *map = xzalloc(sizeof *map); > + ovn_unsynced_datapath_map_init(map, LR_SB_KEY_NAME); > + return map; > +} > + > +void > +en_datapath_logical_router_run(struct engine_node *node , void *data) > +{ > + const struct nbrec_logical_router_table *nb_lr_table = > + EN_OVSDB_GET(engine_get_input("NB_logical_router", node)); > + > + struct ovn_unsynced_datapath_map *map = data; > + > + ovn_unsynced_datapath_map_destroy(map); > + ovn_unsynced_datapath_map_init(map, LR_SB_KEY_NAME); > + > + const struct nbrec_logical_router *nbr; > + NBREC_LOGICAL_ROUTER_TABLE_FOR_EACH (nbr, nb_lr_table) { > + struct ovn_unsynced_datapath *dp; > + dp = ovn_unsynced_datapath_alloc(nbr->name, > + smap_get_int(&nbr->options, > + "requested-tnl-key", > 0), > + &nbr->header_); > + > + smap_add_format(&dp->external_ids, LR_SB_KEY_NAME, UUID_FMT, > + UUID_ARGS(&nbr->header_.uuid)); > + smap_add(&dp->external_ids, "name", dp->name); > + const char *neutron_router = smap_get(&nbr->options, > + "neutron:router_name"); > + if (neutron_router && neutron_router[0]) { > + smap_add(&dp->external_ids, "name2", neutron_router); > + } Nit: I'd add a newline here. > + int64_t ct_zone_limit = ovn_smap_get_llong(&nbr->options, > + "ct-zone-limit", -1); > + if (ct_zone_limit > 0) { > + smap_add_format(&dp->external_ids, "ct-zone-limit", "%"PRId64, > + ct_zone_limit); > + } > + > + int nat_default_ct = smap_get_int(&nbr->options, > + "snat-ct-zone", -1); > + if (nat_default_ct >= 0) { > + smap_add_format(&dp->external_ids, "snat-ct-zone", "%d", > + nat_default_ct); > + } > + > + bool learn_from_arp_request = > + smap_get_bool(&nbr->options, "always_learn_from_arp_request", > + true); > + if (!learn_from_arp_request) { > + smap_add(&dp->external_ids, "always_learn_from_arp_request", > + "false"); > + } > + > + /* For timestamp refreshing, the smallest threshold of the option is > + * set to SB to make sure all entries are refreshed in time. > + * This approach simplifies processing in ovn-controller, but it > + * may be enhanced, if necessary, to parse the complete CIDR-based > + * threshold configurations to SB to reduce unnecessary refreshes. */ > + uint32_t age_threshold = min_mac_binding_age_threshold( > + smap_get(&nbr->options, > + "mac_binding_age_threshold")); > + if (age_threshold) { > + smap_add_format(&dp->external_ids, "mac_binding_age_threshold", > + "%u", age_threshold); > + } > + > + hmap_insert(&map->dps, &dp->hmap_node, > uuid_hash(&nbr->header_.uuid)); > + } > + > + engine_set_node_state(node, EN_UPDATED); > +} > + > + > +void > +en_datapath_logical_router_cleanup(void *data) > +{ > + struct ovn_unsynced_datapath_map *map = data; > + ovn_unsynced_datapath_map_destroy(map); > +} > + > +static void > +synced_logical_router_map_init( > + struct ovn_synced_logical_router_map *router_map) > +{ > + hmap_init(&router_map->synced_routers); > +} > + > +static void > +synced_logical_router_map_destroy( > + struct ovn_synced_logical_router_map *router_map) > +{ > + hmap_destroy(&router_map->synced_routers); We're leaking all ovn_synced_logical_router structs that might still be referenced by the map at this point. > +} > + > +void * > +en_datapath_synced_logical_router_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *args OVS_UNUSED) > +{ > + struct ovn_synced_logical_router_map *router_map; > + router_map = xzalloc(sizeof *router_map); > + synced_logical_router_map_init(router_map); > + > + return router_map; > +} > + > +void > +en_datapath_synced_logical_router_run(struct engine_node *node , void *data) > +{ > + const struct ovn_synced_datapaths *dps = > + engine_get_input_data("datapath_sync", node); > + struct ovn_synced_logical_router_map *router_map = data; > + > + synced_logical_router_map_destroy(router_map); > + synced_logical_router_map_init(router_map); > + > + struct ovn_synced_datapath *sdp; > + LIST_FOR_EACH (sdp, list_node, &dps->synced_dps) { > + struct uuid key; > + if (!smap_get_uuid(&sdp->sb_dp->external_ids, LR_SB_KEY_NAME, &key)) > { > + continue; > + } Do we lookup the NB uuid here as a means of checking whether this datapath is for a logical switch? If so we can do: if (sdp->nb_row->table->class_ != &nbrec_table_logical_router) { continue; } > + struct ovn_synced_logical_router *lr = xzalloc(sizeof *lr); > + lr->nb = CONTAINER_OF(sdp->nb_row, struct nbrec_logical_router, > + header_); > + lr->sb = sdp->sb_dp; > + hmap_insert(&router_map->synced_routers, &lr->hmap_node, > + uuid_hash(&key)); We don't need the 'key' here, we can do: uuid_hash(&lr->nb->header_.uuid). > + } > + > + engine_set_node_state(node, EN_UPDATED); > +} > + > +void en_datapath_synced_logical_router_cleanup(void *data) > +{ > + struct ovn_synced_logical_router_map *router_map = data; > + synced_logical_router_map_destroy(router_map); > +} > diff --git a/northd/en-datapath-logical-router.h > b/northd/en-datapath-logical-router.h > new file mode 100644 > index 000000000..9d749fb9b > --- /dev/null > +++ b/northd/en-datapath-logical-router.h > @@ -0,0 +1,47 @@ > +/* > + * Copyright (c) 2025, Red Hat, Inc. > + * > + * 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_DATAPATH_LOGICAL_ROUTER_H > +#define EN_DATAPATH_LOGICAL_ROUTER_H > + > +#include "lib/inc-proc-eng.h" > +#include "openvswitch/hmap.h" > + > + > +void *en_datapath_logical_router_init(struct engine_node *node, > + struct engine_arg *args); > + > +void en_datapath_logical_router_run(struct engine_node *node , void *data); > +void en_datapath_logical_router_cleanup(void *data); > + > +struct ovn_synced_logical_router { > + struct hmap_node hmap_node; > + const struct nbrec_logical_router *nb; > + const struct sbrec_datapath_binding *sb; > +}; > + > +struct ovn_synced_logical_router_map { > + struct hmap synced_routers; > +}; > + > +void *en_datapath_synced_logical_router_init(struct engine_node *node, > + struct engine_arg *args); > + > +void en_datapath_synced_logical_router_run(struct engine_node *node, > + void *data); > +void en_datapath_synced_logical_router_cleanup(void *data); > + > +#endif /* EN_DATAPATH_LOGICAL_ROUTER_H */ > diff --git a/northd/en-datapath-logical-switch.c > b/northd/en-datapath-logical-switch.c > new file mode 100644 > index 000000000..6cbf782c1 > --- /dev/null > +++ b/northd/en-datapath-logical-switch.c > @@ -0,0 +1,173 @@ > +/* > + * Copyright (c) 2025, Red Hat, Inc. > + * > + * 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/hmap.h" > +#include "openvswitch/util.h" > +#include "openvswitch/vlog.h" > + > +#include "inc-proc-eng.h" > +#include "ovn-nb-idl.h" > +#include "ovn-sb-idl.h" > +#include "datapath_sync.h" > +#include "en-datapath-logical-switch.h" > +#include "en-global-config.h" > +#include "ovn-util.h" > + > +#define LS_SB_KEY_NAME "logical-switch" > + > +VLOG_DEFINE_THIS_MODULE(en_datapath_logical_switch); > + > +void * > +en_datapath_logical_switch_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *args OVS_UNUSED) > +{ > + struct ovn_unsynced_datapath_map *map = xzalloc(sizeof *map); > + ovn_unsynced_datapath_map_init(map, LS_SB_KEY_NAME); > + return map; > +} > + > +void > +en_datapath_logical_switch_run(struct engine_node *node , void *data) > +{ > + const struct nbrec_logical_switch_table *nb_ls_table = > + EN_OVSDB_GET(engine_get_input("NB_logical_switch", node)); > + const struct ed_type_global_config *global_config = > + engine_get_input_data("global_config", node); > + > + struct ovn_unsynced_datapath_map *map = data; > + > + ovn_unsynced_datapath_map_destroy(map); > + ovn_unsynced_datapath_map_init(map, LS_SB_KEY_NAME); > + > + const struct nbrec_logical_switch *nbs; > + NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH (nbs, nb_ls_table) { > + uint32_t requested_tunnel_key = smap_get_int(&nbs->other_config, > + "requested-tnl-key", 0); > + const char *ts = smap_get(&nbs->other_config, "interconn-ts"); > + > + if (!ts && global_config->vxlan_mode && > + requested_tunnel_key >= 1 << 12) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_WARN_RL(&rl, "Tunnel key %"PRIu32" for datapath %s is " > + "incompatible with VXLAN", requested_tunnel_key, > + nbs->name); > + requested_tunnel_key = 0; > + } > + > + struct ovn_unsynced_datapath *dp; > + dp = ovn_unsynced_datapath_alloc(nbs->name, requested_tunnel_key, > + &nbs->header_); > + > + smap_init(&dp->external_ids); > + smap_add_format(&dp->external_ids, LS_SB_KEY_NAME, UUID_FMT, > + UUID_ARGS(&nbs->header_.uuid)); > + smap_add(&dp->external_ids, "name", dp->name); > + const char *neutron_network = smap_get(&nbs->other_config, > + "neutron:network_name"); > + if (neutron_network && neutron_network[0]) { > + smap_add(&dp->external_ids, "name2", neutron_network); > + } Nit: I'd add a newline here. > + int64_t ct_zone_limit = ovn_smap_get_llong(&nbs->other_config, > + "ct-zone-limit", -1); > + if (ct_zone_limit > 0) { > + smap_add_format(&dp->external_ids, "ct-zone-limit", "%"PRId64, > + ct_zone_limit); > + } > + > + if (ts) { > + smap_add(&dp->external_ids, "interconn-ts", ts); > + } > + > + uint32_t age_threshold = smap_get_uint(&nbs->other_config, > + "fdb_age_threshold", 0); > + if (age_threshold) { > + smap_add_format(&dp->external_ids, "fdb_age_threshold", > + "%u", age_threshold); > + } > + > + hmap_insert(&map->dps, &dp->hmap_node, > uuid_hash(&nbs->header_.uuid)); > + } > + > + engine_set_node_state(node, EN_UPDATED); > +} > + > + > +void > +en_datapath_logical_switch_cleanup(void *data) > +{ > + struct ovn_unsynced_datapath_map *map = data; > + ovn_unsynced_datapath_map_destroy(map); > +} > + > +static void > +synced_logical_switch_map_init( > + struct ovn_synced_logical_switch_map *switch_map) > +{ > + hmap_init(&switch_map->synced_switches); > +} > + > +static void > +synced_logical_switch_map_destroy( > + struct ovn_synced_logical_switch_map *switch_map) > +{ > + hmap_destroy(&switch_map->synced_switches); We're leaking all ovn_synced_logical_switch structs that might still be referenced by the map at this point. > +} > + > +void * > +en_datapath_synced_logical_switch_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *args OVS_UNUSED) > +{ > + struct ovn_synced_logical_switch_map *switch_map; > + switch_map = xzalloc(sizeof *switch_map); > + synced_logical_switch_map_init(switch_map); > + > + return switch_map; > +} > + > +void > +en_datapath_synced_logical_switch_run(struct engine_node *node , void *data) > +{ > + const struct ovn_synced_datapaths *dps = > + engine_get_input_data("datapath_sync", node); > + struct ovn_synced_logical_switch_map *switch_map = data; > + > + synced_logical_switch_map_destroy(switch_map); > + synced_logical_switch_map_init(switch_map); > + > + struct ovn_synced_datapath *sdp; > + LIST_FOR_EACH (sdp, list_node, &dps->synced_dps) { > + struct uuid key; > + if (!smap_get_uuid(&sdp->sb_dp->external_ids, LS_SB_KEY_NAME, &key)) > { > + continue; > + } Do we lookup the NB uuid here as a means of checking whether this datapath is for a logical switch? If so we can do: if (sdp->nb_row->table->class_ != &nbrec_table_logical_switch) { continue; } > + struct ovn_synced_logical_switch *lsw = xzalloc(sizeof *lsw); > + lsw->nb = CONTAINER_OF(sdp->nb_row, struct nbrec_logical_switch, > + header_); > + lsw->sb = sdp->sb_dp; > + hmap_insert(&switch_map->synced_switches, &lsw->hmap_node, > + uuid_hash(&key)); We don't really need the 'key' for this part, it's actually &sdp->nb_row.uuid. > + } > + > + engine_set_node_state(node, EN_UPDATED); > +} > + > +void en_datapath_synced_logical_switch_cleanup(void *data) > +{ > + struct ovn_synced_logical_switch_map *switch_map = data; > + synced_logical_switch_map_destroy(switch_map); > +} > diff --git a/northd/en-datapath-logical-switch.h > b/northd/en-datapath-logical-switch.h > new file mode 100644 > index 000000000..b2bfa2c36 > --- /dev/null > +++ b/northd/en-datapath-logical-switch.h > @@ -0,0 +1,47 @@ > +/* > + * Copyright (c) 2025, Red Hat, Inc. > + * > + * 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_DATAPATH_LOGICAL_SWITCH_H > +#define EN_DATAPATH_LOGICAL_SWITCH_H > + > +#include "lib/inc-proc-eng.h" > +#include "openvswitch/hmap.h" > + > + > +void *en_datapath_logical_switch_init(struct engine_node *node, > + struct engine_arg *args); > + > +void en_datapath_logical_switch_run(struct engine_node *node , void *data); > +void en_datapath_logical_switch_cleanup(void *data); > + > +struct ovn_synced_logical_switch { > + struct hmap_node hmap_node; > + const struct nbrec_logical_switch *nb; > + const struct sbrec_datapath_binding *sb; > +}; > + > +struct ovn_synced_logical_switch_map { > + struct hmap synced_switches; > +}; > + > +void *en_datapath_synced_logical_switch_init(struct engine_node *node, > + struct engine_arg *args); Nit: no need for explicit parameter names, the types are explicit enough. > + > +void en_datapath_synced_logical_switch_run(struct engine_node *node, Nit: no need for the explicit 'node' parameter name, the type is explicit enough. > + void *data); > +void en_datapath_synced_logical_switch_cleanup(void *data); > + > +#endif /* EN_DATAPATH_LOGICAL_SWITCH_H */ > diff --git a/northd/en-datapath-sync.c b/northd/en-datapath-sync.c > new file mode 100644 > index 000000000..7e797360c > --- /dev/null > +++ b/northd/en-datapath-sync.c > @@ -0,0 +1,314 @@ > +/* > + * Copyright (c) 2025, Red Hat, Inc. > + * > + * 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 "uuidset.h" > + > +#include "en-datapath-sync.h" > +#include "en-global-config.h" > +#include "datapath_sync.h" > +#include "ovn-sb-idl.h" > +#include "openvswitch/vlog.h" > + > +VLOG_DEFINE_THIS_MODULE(datapath_sync); > + > +void * > +en_datapath_sync_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *args OVS_UNUSED) > +{ > + struct ovn_synced_datapaths *synced_datapaths > + = xzalloc(sizeof *synced_datapaths); > + ovs_list_init(&synced_datapaths->synced_dps); > + > + return synced_datapaths; > +} > + > +static struct ovn_unsynced_datapath * > +find_unsynced_datapath(const struct ovn_unsynced_datapath_map **maps, > + size_t n_maps, > + const struct sbrec_datapath_binding *sb_dp, > + const char **map_key) > +{ > + struct uuid key; > + const struct ovn_unsynced_datapath_map *map; > + bool found_map = false; > + *map_key = NULL; > + for (size_t i = 0; i < n_maps; i++) { > + map = maps[i]; > + if (smap_get_uuid(&sb_dp->external_ids, map->sb_key_name, &key)) { > + found_map = true; > + break; > + } > + } > + > + if (!found_map) { > + return NULL; > + } > + ovs_assert(map); > + *map_key = map->sb_key_name; > + > + uint32_t hash = uuid_hash(&key); Nit: I'd inline this as argument to HMAP_FOR_EACH_WITH_HASH(). > + struct ovn_unsynced_datapath *dp; > + HMAP_FOR_EACH_WITH_HASH (dp, hmap_node, hash, &map->dps) { > + if (uuid_equals(&key, &dp->nb_row->uuid)) { > + return dp; > + } > + } > + > + return NULL; > +} > + > +struct candidate_sdp { > + struct ovs_list list_node; > + struct ovn_synced_datapath *sdp; > + uint32_t requested_tunnel_key; > + uint32_t existing_tunnel_key; > +}; > + > +static struct candidate_sdp * > +candidate_sdp_alloc(const struct ovn_unsynced_datapath *udp, > + const struct sbrec_datapath_binding *sb_dp) > +{ > + struct ovn_synced_datapath *sdp; > + sdp = xzalloc(sizeof *sdp); > + sdp->sb_dp = sb_dp; > + sdp->nb_row = udp->nb_row; > + sbrec_datapath_binding_set_external_ids(sb_dp, &udp->external_ids); > + > + struct candidate_sdp *candidate; > + candidate = xzalloc(sizeof *candidate); > + candidate->sdp = sdp; > + candidate->requested_tunnel_key = udp->requested_tunnel_key; > + candidate->existing_tunnel_key = sdp->sb_dp->tunnel_key; > + > + return candidate; > +} > + > +static void > +reset_synced_datapaths(struct ovn_synced_datapaths *synced_datapaths) > +{ > + struct ovn_synced_datapath *sdp; > + LIST_FOR_EACH_POP (sdp, list_node, &synced_datapaths->synced_dps) { > + free(sdp); > + } > + ovs_list_init(&synced_datapaths->synced_dps); > +} > + > +static void > +create_synced_datapath_candidates_from_sb( > + const struct sbrec_datapath_binding_table *sb_dp_table, > + struct uuidset *visited, > + const struct ovn_unsynced_datapath_map **input_maps, > + size_t n_input_maps, > + struct ovs_list *candidate_sdps) > +{ > + const struct sbrec_datapath_binding *sb_dp; > + SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_SAFE (sb_dp, sb_dp_table) { > + const char *map_key; > + struct ovn_unsynced_datapath *udp; > + udp = find_unsynced_datapath(input_maps, n_input_maps, sb_dp, > + &map_key); > + if (!udp) { > + sbrec_datapath_binding_delete(sb_dp); > + continue; > + } > + > + if (uuidset_find(visited, &udp->nb_row->uuid)) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_INFO_RL( > + &rl, "deleting Datapath_Binding "UUID_FMT" with " > + "duplicate external-ids:%s "UUID_FMT, > + UUID_ARGS(&sb_dp->header_.uuid), map_key, > + UUID_ARGS(&udp->nb_row->uuid)); > + sbrec_datapath_binding_delete(sb_dp); > + continue; > + } > + > + struct candidate_sdp *candidate; > + candidate = candidate_sdp_alloc(udp, sb_dp); > + ovs_list_push_back(candidate_sdps, &candidate->list_node); > + uuidset_insert(visited, &udp->nb_row->uuid); > + } > +} > + > +static void > +create_synced_datapath_candidates_from_nb( > + const struct ovn_unsynced_datapath_map **input_maps, > + size_t n_input_maps, > + struct ovsdb_idl_txn *ovnsb_idl_txn, > + struct uuidset *visited, > + struct ovs_list *candidate_sdps) > +{ > + for (size_t i = 0; i < n_input_maps; i++) { > + const struct ovn_unsynced_datapath_map *map = input_maps[i]; > + struct ovn_unsynced_datapath *udp; > + HMAP_FOR_EACH (udp, hmap_node, &map->dps) { > + if (uuidset_find(visited, &udp->nb_row->uuid)) { > + continue; > + } > + struct sbrec_datapath_binding *sb_dp; > + sb_dp = sbrec_datapath_binding_insert(ovnsb_idl_txn); > + struct candidate_sdp *candidate; > + candidate = candidate_sdp_alloc(udp, sb_dp); > + ovs_list_push_back(candidate_sdps, &candidate->list_node); > + } > + } > +} > + > +static void > +assign_requested_tunnel_keys(struct ovs_list *candidate_sdps, > + struct hmap *dp_tnlids, > + struct ovn_synced_datapaths *synced_datapaths) > +{ > + struct candidate_sdp *candidate; > + LIST_FOR_EACH_SAFE (candidate, list_node, candidate_sdps) { > + if (!candidate->requested_tunnel_key) { > + continue; > + } > + if (ovn_add_tnlid(dp_tnlids, candidate->requested_tunnel_key)) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_WARN_RL(&rl, "Logical datapath "UUID_FMT" requests same " > + "tunnel key %"PRIu32" as another logical datapath", > + UUID_ARGS(&candidate->sdp->nb_row->uuid), > + candidate->requested_tunnel_key); > + } > + sbrec_datapath_binding_set_tunnel_key(candidate->sdp->sb_dp, > + > candidate->requested_tunnel_key); > + ovs_list_remove(&candidate->list_node); > + ovs_list_push_back(&synced_datapaths->synced_dps, > + &candidate->sdp->list_node); > + free(candidate); > + } > +} > + > +static void > +assign_existing_tunnel_keys(struct ovs_list *candidate_sdps, > + struct hmap *dp_tnlids, > + struct ovn_synced_datapaths *synced_datapaths) > +{ > + struct candidate_sdp *candidate; > + LIST_FOR_EACH_SAFE (candidate, list_node, candidate_sdps) { > + if (!candidate->existing_tunnel_key) { > + continue; > + } > + /* Existing southbound DP. If this key is available, > + * reuse it. > + */ > + if (ovn_add_tnlid(dp_tnlids, candidate->existing_tunnel_key)) { > + ovs_list_remove(&candidate->list_node); > + ovs_list_push_back(&synced_datapaths->synced_dps, > + &candidate->sdp->list_node); > + free(candidate); > + } > + } > +} > + > +static void > +allocate_tunnel_keys(struct ovs_list *candidate_sdps, > + struct hmap *dp_tnlids, > + uint32_t max_dp_tunnel_id, > + struct ovn_synced_datapaths *synced_datapaths) > +{ > + uint32_t hint = 0; > + struct candidate_sdp *candidate; > + LIST_FOR_EACH_SAFE (candidate, list_node, candidate_sdps) { > + uint32_t tunnel_key = > + ovn_allocate_tnlid(dp_tnlids, "datapath", OVN_MIN_DP_KEY_LOCAL, > + max_dp_tunnel_id, &hint); > + if (!tunnel_key) { > + continue; > + } > + sbrec_datapath_binding_set_tunnel_key(candidate->sdp->sb_dp, > + tunnel_key); > + ovs_list_remove(&candidate->list_node); > + ovs_list_push_back(&synced_datapaths->synced_dps, > + &candidate->sdp->list_node); > + free(candidate); > + } > +} > + > +static void > +delete_candidates_with_no_tunnel_key(struct ovs_list *candidate_sdps) Nit: the name of the function confused me into thinking that we check something about tunnel_key here. In reality, we don't, we unconditionally destroy all items in the list. Maybe we should call this delete_candidates() and add a comment in its caller. What do you think? > +{ > + /* Anything from this list represents a datapath where a tunnel ID could > + * not be allocated. Delete the SB datapath binding for these. > + */ > + struct candidate_sdp *candidate; > + LIST_FOR_EACH_POP (candidate, list_node, candidate_sdps) { > + sbrec_datapath_binding_delete(candidate->sdp->sb_dp); > + free(candidate->sdp); > + free(candidate); > + } > +} > + > +void > +en_datapath_sync_run(struct engine_node *node , void *data) > +{ > + const struct sbrec_datapath_binding_table *sb_dp_table = > + EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node)); > + const struct ed_type_global_config *global_config = > + engine_get_input_data("global_config", node); > + /* The inputs are: > + * * Some number of input maps. > + * * Southbound Datapath Binding table. > + * * Global config data. > + * > + * Therefore, the number of inputs - 2 is the number of input > + * maps from the datapath-specific nodes. > + */ > + size_t n_input_maps = node->n_inputs - 2; > + const struct ovn_unsynced_datapath_map *input_maps[n_input_maps]; GCC complains in CI here with: northd/en-datapath-sync.c:274:56: error: Variable length array is used. The CI uses gcc-4:13.2.0-7ubuntu1. > + struct ovn_synced_datapaths *synced_datapaths = data; > + > + for (size_t i = 0; i < n_input_maps; i++) { > + input_maps[i] = engine_get_data(node->inputs[i].node); > + } > + > + reset_synced_datapaths(synced_datapaths); > + > + struct uuidset visited = UUIDSET_INITIALIZER(&visited); > + struct ovs_list candidate_sdps = OVS_LIST_INITIALIZER(&candidate_sdps); > + create_synced_datapath_candidates_from_sb(sb_dp_table, &visited, > + input_maps, n_input_maps, > + &candidate_sdps); > + > + const struct engine_context *eng_ctx = engine_get_context(); > + create_synced_datapath_candidates_from_nb(input_maps, n_input_maps, > + eng_ctx->ovnsb_idl_txn, > &visited, > + &candidate_sdps); > + uuidset_destroy(&visited); > + > + struct hmap dp_tnlids = HMAP_INITIALIZER(&dp_tnlids); > + assign_requested_tunnel_keys(&candidate_sdps, &dp_tnlids, > + synced_datapaths); > + assign_existing_tunnel_keys(&candidate_sdps, &dp_tnlids, > synced_datapaths); > + allocate_tunnel_keys(&candidate_sdps, &dp_tnlids, > + global_config->max_dp_tunnel_id, synced_datapaths); > + delete_candidates_with_no_tunnel_key(&candidate_sdps); > + We leak dp_tnlids. I think we need: ovn_destroy_tnlids(&dp_tnlids); > + engine_set_node_state(node, EN_UPDATED); > +} > + > +void en_datapath_sync_cleanup(void *data) > +{ > + struct ovn_synced_datapaths *synced_datapaths = data; > + struct ovn_synced_datapath *sdp; > + > + LIST_FOR_EACH_POP (sdp, list_node, &synced_datapaths->synced_dps) { > + free(sdp); > + } > +} > diff --git a/northd/en-datapath-sync.h b/northd/en-datapath-sync.h > new file mode 100644 > index 000000000..1c087a9e8 > --- /dev/null > +++ b/northd/en-datapath-sync.h > @@ -0,0 +1,30 @@ > +/* > + * Copyright (c) 2025, Red Hat, Inc. > + * > + * 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_DATAPATH_SYNC_H > +#define EN_DATAPATH_SYNC_H > + > +#include "inc-proc-eng.h" > + > +void *en_datapath_sync_init(struct engine_node *node, > + struct engine_arg *args); Nit: no need to specify the 'node' and 'args' parameter names here, the types are explicit enough. > + > + > +void en_datapath_sync_run(struct engine_node *node , void *data); Nit: no need to specify the 'node' parameter name here, the type is explicit enough. > + > +void en_datapath_sync_cleanup(void *data); Nit: I think I'd remove the empty lines between the function declarations above. > + > +#endif /* EN_DATAPATH_SYNC_H */ > diff --git a/northd/en-global-config.c b/northd/en-global-config.c > index 7e84d0da9..afb735128 100644 > --- a/northd/en-global-config.c > +++ b/northd/en-global-config.c > @@ -63,6 +63,17 @@ en_global_config_init(struct engine_node *node OVS_UNUSED, > return data; > } > > +static uint32_t > +get_ovn_max_dp_key_local(bool vxlan_mode, bool vxlan_ic_mode) > +{ > + if (vxlan_mode) { > + /* OVN_MAX_DP_GLOBAL_NUM doesn't apply for VXLAN mode. */ > + return vxlan_ic_mode ? OVN_MAX_DP_VXLAN_KEY_LOCAL > + : OVN_MAX_DP_VXLAN_KEY; > + } > + return vxlan_ic_mode ? OVN_MAX_DP_VXLAN_KEY_LOCAL : OVN_MAX_DP_KEY_LOCAL; > +} > + > void > en_global_config_run(struct engine_node *node , void *data) > { > diff --git a/northd/en-northd.c b/northd/en-northd.c > index ea946696c..ecb5bb032 100644 > --- a/northd/en-northd.c > +++ b/northd/en-northd.c > @@ -111,6 +111,14 @@ northd_get_input_data(struct engine_node *node, > input_data->svc_monitor_mac_ea = global_config->svc_monitor_mac_ea; > input_data->features = &global_config->features; > input_data->vxlan_mode = global_config->vxlan_mode; > + > + const struct ovn_synced_logical_switch_map *synced_lses = > + engine_get_input_data("datapath_synced_logical_switch", node); > + input_data->synced_lses = synced_lses; Nit: I'd avoid the intermediate local variable. > + > + const struct ovn_synced_logical_router_map *synced_lrs = > + engine_get_input_data("datapath_synced_logical_router", node); > + input_data->synced_lrs = synced_lrs; Nit: here too. > } > > void > @@ -501,6 +509,38 @@ northd_sb_fdb_change_handler(struct engine_node *node, > void *data) > return true; > } > > +bool northd_synced_lr_handler(struct engine_node *node OVS_UNUSED, > + void *data OVS_UNUSED) > +{ > + /* Currently, northd handles logical router changes in a node that > + * reads directly from the northbound logical router table. That node > + * will trigger a recompute if conditions on changed logical routers > + * cannot be handled. From en-northd's perspective, synced logical > + * router changes are always handled. > + * > + * Once datapath syncing has incremental processing added, then > + * en-northd can move its logical router change handling here and > + * remove the northbound logical router table as an input. > + */ > + return true; > +} > + > +bool northd_synced_ls_handler(struct engine_node *node OVS_UNUSED, > + void *data OVS_UNUSED) > +{ > + /* Currently, northd handles logical switch changes in a node that > + * reads directly from the northbound logical switch table. That node > + * will trigger a recompute if conditions on changed logical switches > + * cannot be handled. From en-northd's perspective, synced logical > + * switch changes are always handled. > + * > + * Once datapath syncing has incremental processing added, then > + * en-northd can move its logical switch change handling here and > + * remove the northbound logical switch table as an input. > + */ > + return true; > +} Another option is to move these comments to inc-proc-northd.c and use a engine_noop_handler(). What do you think? > + > void > en_route_policies_cleanup(void *data) > { > diff --git a/northd/en-northd.h b/northd/en-northd.h > index b676a0ad3..b6d9ca8b8 100644 > --- a/northd/en-northd.h > +++ b/northd/en-northd.h > @@ -20,6 +20,8 @@ bool northd_nb_logical_router_handler(struct engine_node *, > void *data); > bool northd_sb_port_binding_handler(struct engine_node *, void *data); > bool northd_lb_data_handler(struct engine_node *, void *data); > bool northd_sb_fdb_change_handler(struct engine_node *node, void *data); > +bool northd_synced_lr_handler(struct engine_node *node, void *data); > +bool northd_synced_ls_handler(struct engine_node *node, void *data); > void *en_routes_init(struct engine_node *node OVS_UNUSED, > struct engine_arg *arg OVS_UNUSED); > void en_route_policies_cleanup(void *data); > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index 7f92c0cb7..b6293d6e7 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -47,6 +47,9 @@ > #include "en-advertised-route-sync.h" > #include "en-learned-route-sync.h" > #include "en-group-ecmp-route.h" > +#include "en-datapath-logical-router.h" > +#include "en-datapath-logical-switch.h" > +#include "en-datapath-sync.h" > #include "unixctl.h" > #include "util.h" > > @@ -179,6 +182,13 @@ static > ENGINE_NODE_WITH_CLEAR_TRACK_DATA(learned_route_sync, > "learned_route_sync"); > static ENGINE_NODE(dynamic_routes, "dynamic_routes"); > static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(group_ecmp_route, > "group_ecmp_route"); > +static ENGINE_NODE(datapath_logical_router, "datapath_logical_router"); > +static ENGINE_NODE(datapath_logical_switch, "datapath_logical_switch"); > +static ENGINE_NODE(datapath_synced_logical_router, > + "datapath_synced_logical_router"); > +static ENGINE_NODE(datapath_synced_logical_switch, > + "datapath_synced_logical_switch"); > +static ENGINE_NODE(datapath_sync, "datapath_sync"); > > void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > struct ovsdb_idl_loop *sb) > @@ -209,6 +219,18 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_add_input(&en_acl_id, &en_nb_acl, NULL); > engine_add_input(&en_acl_id, &en_sb_acl_id, NULL); > > + engine_add_input(&en_datapath_logical_switch, &en_nb_logical_switch, > NULL); > + engine_add_input(&en_datapath_logical_switch, &en_global_config, NULL); Nit: I'd add an empty line before moving to the next I-P node. > + engine_add_input(&en_datapath_logical_router, &en_nb_logical_router, > NULL); Nit: here too. > + engine_add_input(&en_datapath_sync, &en_datapath_logical_switch, NULL); > + engine_add_input(&en_datapath_sync, &en_datapath_logical_router, NULL); > + engine_add_input(&en_datapath_sync, &en_sb_datapath_binding, NULL); > + engine_add_input(&en_datapath_sync, &en_global_config, NULL); Nit: here too. > + engine_add_input(&en_datapath_synced_logical_router, &en_datapath_sync, > + NULL); > + engine_add_input(&en_datapath_synced_logical_switch, &en_datapath_sync, > + NULL); > + > engine_add_input(&en_northd, &en_nb_mirror, NULL); > engine_add_input(&en_northd, &en_nb_static_mac_binding, NULL); > engine_add_input(&en_northd, &en_nb_chassis_template_var, NULL); > @@ -245,6 +267,10 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_add_input(&en_northd, &en_nb_logical_router, > northd_nb_logical_router_handler); > engine_add_input(&en_northd, &en_lb_data, northd_lb_data_handler); > + engine_add_input(&en_northd, &en_datapath_synced_logical_router, > + northd_synced_lr_handler); > + engine_add_input(&en_northd, &en_datapath_synced_logical_switch, > + northd_synced_ls_handler); > > engine_add_input(&en_lr_nat, &en_northd, lr_nat_northd_handler); > > diff --git a/northd/northd.c b/northd/northd.c > index 474d2018c..4c3715d39 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -52,6 +52,8 @@ > #include "en-ls-stateful.h" > #include "en-multicast.h" > #include "en-sampling-app.h" > +#include "en-datapath-logical-switch.h" > +#include "en-datapath-logical-router.h" > #include "lib/ovn-parallel-hmap.h" > #include "ovn/actions.h" > #include "ovn/features.h" > @@ -453,6 +455,7 @@ struct lrouter_group { > struct hmapx tmp_ha_ref_chassis; > }; > > +static void init_mcast_info_for_datapath(struct ovn_datapath *od); Nit: I'd add a newline here. > static struct ovn_datapath * > ovn_datapath_create(struct hmap *datapaths, const struct uuid *key, > const struct nbrec_logical_switch *nbs, > @@ -470,6 +473,8 @@ ovn_datapath_create(struct hmap *datapaths, const struct > uuid *key, > od->lr_group = NULL; > hmap_init(&od->ports); > sset_init(&od->router_ips); > + od->tunnel_key = sb->tunnel_key; > + init_mcast_info_for_datapath(od); > return od; > } > > @@ -733,87 +738,6 @@ store_mcast_info_for_switch_datapath(const struct > sbrec_ip_multicast *sb, > } > } > > -static void > -ovn_datapath_update_external_ids(struct ovn_datapath *od) > -{ > - /* Get the logical-switch or logical-router UUID to set in > - * external-ids. */ > - char uuid_s[UUID_LEN + 1]; > - sprintf(uuid_s, UUID_FMT, UUID_ARGS(&od->key)); > - const char *key = od->nbs ? "logical-switch" : "logical-router"; > - > - /* Get names to set in external-ids. */ > - const char *name = od->nbs ? od->nbs->name : od->nbr->name; > - const char *name2 = (od->nbs > - ? smap_get(&od->nbs->external_ids, > - "neutron:network_name") > - : smap_get(&od->nbr->external_ids, > - "neutron:router_name")); > - > - /* Set external-ids. */ > - struct smap ids = SMAP_INITIALIZER(&ids); > - smap_add(&ids, key, uuid_s); > - smap_add(&ids, "name", name); > - if (name2 && name2[0]) { > - smap_add(&ids, "name2", name2); > - } > - > - int64_t ct_zone_limit = ovn_smap_get_llong(od->nbs ? > - &od->nbs->other_config : > - &od->nbr->options, > - "ct-zone-limit", -1); > - if (ct_zone_limit > 0) { > - smap_add_format(&ids, "ct-zone-limit", "%"PRId64, ct_zone_limit); > - } > - > - /* Set interconn-ts. */ > - if (od->nbs) { > - const char *ts = smap_get(&od->nbs->other_config, "interconn-ts"); > - if (ts) { > - smap_add(&ids, "interconn-ts", ts); > - } > - > - uint32_t age_threshold = smap_get_uint(&od->nbs->other_config, > - "fdb_age_threshold", 0); > - if (age_threshold) { > - smap_add_format(&ids, "fdb_age_threshold", > - "%u", age_threshold); > - } > - } > - > - /* Set snat-ct-zone */ > - if (od->nbr) { > - int nat_default_ct = smap_get_int(&od->nbr->options, > - "snat-ct-zone", -1); > - if (nat_default_ct >= 0) { > - smap_add_format(&ids, "snat-ct-zone", "%d", nat_default_ct); > - } > - > - bool learn_from_arp_request = > - smap_get_bool(&od->nbr->options, "always_learn_from_arp_request", > - true); > - if (!learn_from_arp_request) { > - smap_add(&ids, "always_learn_from_arp_request", "false"); > - } > - > - /* For timestamp refreshing, the smallest threshold of the option is > - * set to SB to make sure all entries are refreshed in time. > - * XXX: This approach simplifies processing in ovn-controller, but it > - * may be enhanced, if necessary, to parse the complete CIDR-based > - * threshold configurations to SB to reduce unnecessary refreshes. */ > - uint32_t age_threshold = min_mac_binding_age_threshold( > - smap_get(&od->nbr->options, > - "mac_binding_age_threshold")); > - if (age_threshold) { > - smap_add_format(&ids, "mac_binding_age_threshold", > - "%u", age_threshold); > - } > - } > - > - sbrec_datapath_binding_set_external_ids(od->sb, &ids); > - smap_destroy(&ids); > -} > - > static enum dynamic_routing_redistribute_mode > parse_dynamic_routing_redistribute( > const struct smap *options, > @@ -865,170 +789,6 @@ parse_dynamic_routing_redistribute( > return out; > } > > -static void > -join_datapaths(const struct nbrec_logical_switch_table *nbrec_ls_table, > - const struct nbrec_logical_router_table *nbrec_lr_table, > - const struct sbrec_datapath_binding_table *sbrec_dp_table, > - struct ovsdb_idl_txn *ovnsb_txn, > - struct hmap *datapaths, struct ovs_list *sb_only, > - struct ovs_list *nb_only, struct ovs_list *both) > -{ > - ovs_list_init(sb_only); > - ovs_list_init(nb_only); > - ovs_list_init(both); > - > - const struct sbrec_datapath_binding *sb; > - SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_SAFE (sb, sbrec_dp_table) { > - struct uuid key; > - if (!smap_get_uuid(&sb->external_ids, "logical-switch", &key) && > - !smap_get_uuid(&sb->external_ids, "logical-router", &key)) { > - ovsdb_idl_txn_add_comment( > - ovnsb_txn, > - "deleting Datapath_Binding "UUID_FMT" that lacks " > - "external-ids:logical-switch and " > - "external-ids:logical-router", > - UUID_ARGS(&sb->header_.uuid)); > - sbrec_datapath_binding_delete(sb); > - continue; > - } > - > - if (ovn_datapath_find_(datapaths, &key)) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > - VLOG_INFO_RL( > - &rl, "deleting Datapath_Binding "UUID_FMT" with " > - "duplicate external-ids:logical-switch/router "UUID_FMT, > - UUID_ARGS(&sb->header_.uuid), UUID_ARGS(&key)); > - sbrec_datapath_binding_delete(sb); > - continue; > - } > - > - struct ovn_datapath *od = ovn_datapath_create(datapaths, &key, > - NULL, NULL, sb); > - ovs_list_push_back(sb_only, &od->list); > - } > - > - vxlan_ic_mode = false; > - const struct nbrec_logical_switch *nbs; > - NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH (nbs, nbrec_ls_table) { > - struct ovn_datapath *od = ovn_datapath_find_(datapaths, > - &nbs->header_.uuid); > - if (od) { > - od->nbs = nbs; > - ovs_list_remove(&od->list); > - ovs_list_push_back(both, &od->list); > - ovn_datapath_update_external_ids(od); > - } else { > - od = ovn_datapath_create(datapaths, &nbs->header_.uuid, > - nbs, NULL, NULL); > - ovs_list_push_back(nb_only, &od->list); > - } > - > - init_ipam_info_for_datapath(od); > - init_mcast_info_for_datapath(od); > - > - if (smap_get_bool(&nbs->other_config, "ic-vxlan_mode", false)) { > - vxlan_ic_mode = true; > - } > - } > - > - const struct nbrec_logical_router *nbr; > - NBREC_LOGICAL_ROUTER_TABLE_FOR_EACH (nbr, nbrec_lr_table) { > - if (!lrouter_is_enabled(nbr)) { > - continue; > - } > - > - struct ovn_datapath *od = ovn_datapath_find_(datapaths, > - &nbr->header_.uuid); > - if (od) { > - if (!od->nbs) { > - od->nbr = nbr; > - ovs_list_remove(&od->list); > - ovs_list_push_back(both, &od->list); > - ovn_datapath_update_external_ids(od); > - } else { > - /* Can't happen! */ > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, > 1); > - VLOG_WARN_RL(&rl, > - "duplicate UUID "UUID_FMT" in OVN_Northbound", > - UUID_ARGS(&nbr->header_.uuid)); > - continue; > - } > - } else { > - od = ovn_datapath_create(datapaths, &nbr->header_.uuid, > - NULL, nbr, NULL); > - ovs_list_push_back(nb_only, &od->list); > - } > - init_mcast_info_for_datapath(od); > - if (smap_get(&od->nbr->options, "chassis")) { > - od->is_gw_router = true; > - } > - od->dynamic_routing = smap_get_bool(&od->nbr->options, > - "dynamic-routing", false); > - od->dynamic_routing_redistribute = > - parse_dynamic_routing_redistribute(&od->nbr->options, DRRM_NONE, > - od->nbr->name); > - } > -} > - > - > -uint32_t > -get_ovn_max_dp_key_local(bool _vxlan_mode, bool _vxlan_ic_mode) > -{ > - if (_vxlan_mode) { > - /* OVN_MAX_DP_GLOBAL_NUM doesn't apply for VXLAN mode. */ > - return _vxlan_ic_mode ? OVN_MAX_DP_VXLAN_KEY_LOCAL > - : OVN_MAX_DP_VXLAN_KEY; > - } > - return _vxlan_ic_mode ? OVN_MAX_DP_VXLAN_KEY_LOCAL : > OVN_MAX_DP_KEY_LOCAL; > -} > - > -static void > -ovn_datapath_allocate_key(struct hmap *datapaths, struct hmap *dp_tnlids, > - struct ovn_datapath *od, uint32_t *hint) > -{ > - if (!od->tunnel_key) { > - od->tunnel_key = ovn_allocate_tnlid(dp_tnlids, "datapath", > - OVN_MIN_DP_KEY_LOCAL, > - get_ovn_max_dp_key_local(vxlan_mode, vxlan_ic_mode), hint); > - if (!od->tunnel_key) { > - if (od->sb) { > - sbrec_datapath_binding_delete(od->sb); > - } > - ovs_list_remove(&od->list); > - ovn_datapath_destroy(datapaths, od); > - } > - } > -} > - > -static void > -ovn_datapath_assign_requested_tnl_id( > - struct hmap *dp_tnlids, struct ovn_datapath *od) > -{ > - const struct smap *other_config = (od->nbs > - ? &od->nbs->other_config > - : &od->nbr->options); > - uint32_t tunnel_key = smap_get_int(other_config, "requested-tnl-key", 0); > - if (tunnel_key) { > - const char *interconn_ts = smap_get(other_config, "interconn-ts"); > - if (!interconn_ts && vxlan_mode && tunnel_key >= 1 << 12) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > - VLOG_WARN_RL(&rl, "Tunnel key %"PRIu32" for datapath %s is " > - "incompatible with VXLAN", tunnel_key, > - od->nbs ? od->nbs->name : od->nbr->name); > - return; > - } > - if (ovn_add_tnlid(dp_tnlids, tunnel_key)) { > - od->tunnel_key = tunnel_key; > - } else { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > - VLOG_WARN_RL(&rl, "Logical %s %s requests same tunnel key " > - "%"PRIu32" as another logical switch or router", > - od->nbs ? "switch" : "router", > - od->nbs ? od->nbs->name : od->nbr->name, > tunnel_key); > - } > - } > -} > - > static void > ods_build_array_index(struct ovn_datapaths *datapaths) > { > @@ -1048,82 +808,43 @@ ods_build_array_index(struct ovn_datapaths *datapaths) > } > } > > -/* Updates the southbound Datapath_Binding table so that it contains the > - * logical switches and routers specified by the northbound database. > - * > - * Initializes 'datapaths' to contain a "struct ovn_datapath" for every > logical > - * switch and router. */ > +/* Initializes 'ls_datapaths' to contain a "struct ovn_datapath" for every > + * logical switch, and initializes 'lr_datapaths' to contain a > + * "struct ovn_datapath" for every logical router. > + */ > static void > -build_datapaths(struct ovsdb_idl_txn *ovnsb_txn, > - const struct nbrec_logical_switch_table *nbrec_ls_table, > - const struct nbrec_logical_router_table *nbrec_lr_table, > - const struct sbrec_datapath_binding_table *sbrec_dp_table, > +build_datapaths(const struct ovn_synced_logical_switch_map *ls_map, > + const struct ovn_synced_logical_router_map *lr_map, > struct ovn_datapaths *ls_datapaths, > struct ovn_datapaths *lr_datapaths) > { > - struct ovs_list sb_only, nb_only, both; > - > - struct hmap *datapaths = &ls_datapaths->datapaths; > - join_datapaths(nbrec_ls_table, nbrec_lr_table, sbrec_dp_table, ovnsb_txn, > - datapaths, &sb_only, &nb_only, &both); > - > - /* Assign explicitly requested tunnel ids first. */ > - struct hmap dp_tnlids = HMAP_INITIALIZER(&dp_tnlids); > - struct ovn_datapath *od; > - LIST_FOR_EACH (od, list, &both) { > - ovn_datapath_assign_requested_tnl_id(&dp_tnlids, od); > - } > - LIST_FOR_EACH (od, list, &nb_only) { > - ovn_datapath_assign_requested_tnl_id(&dp_tnlids, od); > - } > - > - /* Keep nonconflicting tunnel IDs that are already assigned. */ > - LIST_FOR_EACH (od, list, &both) { > - if (!od->tunnel_key && ovn_add_tnlid(&dp_tnlids, > od->sb->tunnel_key)) { > - od->tunnel_key = od->sb->tunnel_key; > - } > - } > - > - /* Assign new tunnel ids where needed. */ > - uint32_t hint = 0; > - LIST_FOR_EACH_SAFE (od, list, &both) { > - ovn_datapath_allocate_key(datapaths, &dp_tnlids, od, &hint); > - } > - LIST_FOR_EACH_SAFE (od, list, &nb_only) { > - ovn_datapath_allocate_key(datapaths, &dp_tnlids, od, &hint); > + struct ovn_synced_logical_switch *ls; > + HMAP_FOR_EACH (ls, hmap_node, &ls_map->synced_switches) { > + struct ovn_datapath *od = > + ovn_datapath_create(&ls_datapaths->datapaths, > + &ls->nb->header_.uuid, > + ls->nb, NULL, ls->sb); > + init_ipam_info_for_datapath(od); > } > > - /* Sync tunnel ids from nb to sb. */ > - LIST_FOR_EACH (od, list, &both) { > - if (od->sb->tunnel_key != od->tunnel_key) { > - sbrec_datapath_binding_set_tunnel_key(od->sb, od->tunnel_key); > + struct ovn_synced_logical_router *lr; > + HMAP_FOR_EACH (lr, hmap_node, &lr_map->synced_routers) { > + if (!lrouter_is_enabled(lr->nb)) { > + continue; > } > - ovn_datapath_update_external_ids(od); > - } > - LIST_FOR_EACH (od, list, &nb_only) { > - od->sb = sbrec_datapath_binding_insert(ovnsb_txn); > - ovn_datapath_update_external_ids(od); > - sbrec_datapath_binding_set_tunnel_key(od->sb, od->tunnel_key); > - } > - ovn_destroy_tnlids(&dp_tnlids); > > - /* Delete southbound records without northbound matches. */ > - LIST_FOR_EACH_SAFE (od, list, &sb_only) { > - ovs_list_remove(&od->list); > - sbrec_datapath_binding_delete(od->sb); > - ovn_datapath_destroy(datapaths, od); > - } > - > - /* Move lr datapaths to lr_datapaths, and ls datapaths will > - * remain in datapaths/ls_datapaths. */ > - HMAP_FOR_EACH_SAFE (od, key_node, datapaths) { > - if (!od->nbr) { > - ovs_assert(od->nbs); > - continue; > + struct ovn_datapath *od = > + ovn_datapath_create(&lr_datapaths->datapaths, > + &lr->nb->header_.uuid, > + NULL, lr->nb, lr->sb); > + if (smap_get(&od->nbr->options, "chassis")) { > + od->is_gw_router = true; > } > - hmap_remove(datapaths, &od->key_node); > - hmap_insert(&lr_datapaths->datapaths, &od->key_node, > - od->key_node.hash); > + od->dynamic_routing = smap_get_bool(&od->nbr->options, > + "dynamic-routing", false); > + od->dynamic_routing_redistribute = > + parse_dynamic_routing_redistribute(&od->nbr->options, DRRM_NONE, > + od->nbr->name); > } > > ods_build_array_index(ls_datapaths); > @@ -18786,10 +18507,8 @@ ovnnb_db_run(struct northd_input *input_data, > > vxlan_mode = input_data->vxlan_mode; > > - build_datapaths(ovnsb_txn, > - input_data->nbrec_logical_switch_table, > - input_data->nbrec_logical_router_table, > - input_data->sbrec_datapath_binding_table, > + build_datapaths(input_data->synced_lses, > + input_data->synced_lrs, > &data->ls_datapaths, > &data->lr_datapaths); > build_lb_datapaths(input_data->lbs, input_data->lbgrps, > diff --git a/northd/northd.h b/northd/northd.h > index 5b4f3df5e..89cd5beee 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -68,6 +68,10 @@ struct northd_input { > /* ACL ID inputs. */ > const struct acl_id_data *acl_id_data; > > + /* Synced datapath inputs. */ > + const struct ovn_synced_logical_switch_map *synced_lses; > + const struct ovn_synced_logical_router_map *synced_lrs; > + > /* Indexes */ > struct ovsdb_idl_index *sbrec_chassis_by_name; > struct ovsdb_idl_index *sbrec_chassis_by_hostname; > @@ -964,8 +968,6 @@ lr_has_multiple_gw_ports(const struct ovn_datapath *od) > return od->n_l3dgw_ports > 1 && !od->is_gw_router; > } > > -uint32_t get_ovn_max_dp_key_local(bool _vxlan_mode, bool ic_mode); > - > /* Returns true if the logical router port 'enabled' column is empty or > * set to true. Otherwise, returns false. */ > static inline bool Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
