All changes to northbound logical routers can be incrementally processed
by en-datapath-logical-router. This change ensures that it is done and
the changes can then be propagated to the next nodes. A new test in
ovn-northd.at ensures the behavior.
Signed-off-by: Mark Michelson <[email protected]>
---
northd/datapath-sync.c | 39 ++++++
northd/datapath-sync.h | 13 ++
northd/en-datapath-logical-router.c | 209 +++++++++++++++++++++-------
northd/en-datapath-logical-router.h | 6 +
northd/inc-proc-northd.c | 5 +-
tests/ovn-northd.at | 40 ++++++
6 files changed, 257 insertions(+), 55 deletions(-)
diff --git a/northd/datapath-sync.c b/northd/datapath-sync.c
index 4916ed647..eea878ee8 100644
--- a/northd/datapath-sync.c
+++ b/northd/datapath-sync.c
@@ -16,6 +16,8 @@
#include <config.h>
#include "datapath-sync.h"
+#include "ovsdb-idl-provider.h"
+#include "uuid.h"
static const char *ovn_datapath_strings[] = {
[DP_SWITCH] = "logical-switch",
@@ -74,6 +76,9 @@ ovn_unsynced_datapath_map_init(struct
ovn_unsynced_datapath_map *map,
{
*map = (struct ovn_unsynced_datapath_map) {
.dps = HMAP_INITIALIZER(&map->dps),
+ .new = HMAPX_INITIALIZER(&map->new),
+ .deleted = HMAPX_INITIALIZER(&map->deleted),
+ .updated = HMAPX_INITIALIZER(&map->updated),
.dp_type = dp_type,
};
}
@@ -88,3 +93,37 @@ ovn_unsynced_datapath_map_destroy(struct
ovn_unsynced_datapath_map *map)
}
hmap_destroy(&map->dps);
}
+
+struct ovn_unsynced_datapath *
+ovn_unsynced_datapath_find(const struct ovn_unsynced_datapath_map *map,
+ const struct uuid *datapath_uuid)
+{
+ uint32_t hash = uuid_hash(datapath_uuid);
+
+ struct ovn_unsynced_datapath *udp;
+ HMAP_FOR_EACH_WITH_HASH (udp, hmap_node, hash, &map->dps) {
+ if (uuid_equals(&udp->nb_row->uuid, datapath_uuid)) {
+ return udp;
+ }
+ }
+ return NULL;
+}
+
+void
+ovn_unsynced_datapath_map_clear_tracked_data(
+ struct ovn_unsynced_datapath_map *map)
+{
+ hmapx_clear(&map->new);
+ hmapx_clear(&map->updated);
+
+ /* Deleted entries need to be freed since they don't
+ * exist in map->dps.
+ */
+ struct hmapx_node *node;
+ HMAPX_FOR_EACH_SAFE (node, &map->deleted) {
+ struct ovn_unsynced_datapath *udp = node->data;
+ ovn_unsynced_datapath_destroy(udp);
+ free(udp);
+ hmapx_delete(&map->deleted, node);
+ }
+}
diff --git a/northd/datapath-sync.h b/northd/datapath-sync.h
index 19b6e0fd0..8503132e6 100644
--- a/northd/datapath-sync.h
+++ b/northd/datapath-sync.h
@@ -19,6 +19,7 @@
#include "openvswitch/hmap.h"
#include "openvswitch/list.h"
#include "smap.h"
+#include "hmapx.h"
/* Datapath syncing API. This file consists of utility functions
* that can be used when syncing northbound datapath types (e.g.
@@ -63,6 +64,11 @@ struct ovn_unsynced_datapath_map {
/* ovn_unsynced_datapath */
struct hmap dps;
enum ovn_datapath_type dp_type;
+
+ /* Data for incremental case */
+ struct hmapx new;
+ struct hmapx updated;
+ struct hmapx deleted;
};
struct ovn_synced_datapath {
@@ -84,4 +90,11 @@ void ovn_unsynced_datapath_map_init(struct
ovn_unsynced_datapath_map *,
enum ovn_datapath_type);
void ovn_unsynced_datapath_map_destroy(struct ovn_unsynced_datapath_map *);
+struct ovn_unsynced_datapath *
+ovn_unsynced_datapath_find(const struct ovn_unsynced_datapath_map *,
+ const struct uuid *);
+
+void ovn_unsynced_datapath_map_clear_tracked_data(
+ struct ovn_unsynced_datapath_map *);
+
#endif /* DATAPATH_SYNC_H */
diff --git a/northd/en-datapath-logical-router.c
b/northd/en-datapath-logical-router.c
index 8f86b7291..2cef34faf 100644
--- a/northd/en-datapath-logical-router.c
+++ b/northd/en-datapath-logical-router.c
@@ -34,6 +34,74 @@ en_datapath_logical_router_init(struct engine_node *node
OVS_UNUSED,
return map;
}
+static struct ovn_unsynced_datapath *
+allocate_unsynced_router(const struct nbrec_logical_router *nbr)
+{
+ struct ovn_unsynced_datapath *dp =
+ ovn_unsynced_datapath_alloc(nbr->name, DP_ROUTER,
+ smap_get_int(&nbr->options,
+ "requested-tnl-key", 0),
+ &nbr->header_);
+
+ 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);
+ }
+
+ 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);
+ }
+
+ /* For backwards-compatibility, also store the NB UUID in
+ * external-ids:logical-router. This is useful if ovn-controller
+ * has not updated and expects this to be where to find the
+ * UUID.
+ */
+ smap_add_format(&dp->external_ids, "logical-router", UUID_FMT,
+ UUID_ARGS(&nbr->header_.uuid));
+
+ return dp;
+}
+
+static bool
+logical_router_is_enabled(const struct nbrec_logical_router *nbr)
+{
+ return !nbr->enabled || *nbr->enabled;
+}
+
enum engine_node_state
en_datapath_logical_router_run(struct engine_node *node , void *data)
{
@@ -47,69 +115,104 @@ en_datapath_logical_router_run(struct engine_node *node ,
void *data)
const struct nbrec_logical_router *nbr;
NBREC_LOGICAL_ROUTER_TABLE_FOR_EACH (nbr, nb_lr_table) {
- if (nbr->enabled && !(*nbr->enabled)) {
+ if (!logical_router_is_enabled(nbr)) {
continue;
}
- struct ovn_unsynced_datapath *dp =
- ovn_unsynced_datapath_alloc(nbr->name, DP_ROUTER,
- smap_get_int(&nbr->options,
- "requested-tnl-key", 0),
- &nbr->header_);
-
- 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);
- }
+ struct ovn_unsynced_datapath *dp = allocate_unsynced_router(nbr);
+ hmap_insert(&map->dps, &dp->hmap_node, uuid_hash(&nbr->header_.uuid));
+ }
- 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);
- }
+ return EN_UPDATED;
+}
- 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);
- }
+void
+en_datapath_logical_router_clear_tracked_data(void *data)
+{
+ struct ovn_unsynced_datapath_map *map = data;
+ ovn_unsynced_datapath_map_clear_tracked_data(map);
+}
- 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");
- }
+enum engine_input_handler_result
+en_datapath_logical_router_logical_router_handler(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));
- /* 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);
- }
+ struct ovn_unsynced_datapath_map *map = data;
+ enum engine_input_handler_result result = EN_HANDLED_UNCHANGED;
- /* For backwards-compatibility, also store the NB UUID in
- * external-ids:logical-router. This is useful if ovn-controller
- * has not updated and expects this to be where to find the
- * UUID.
- */
- smap_add_format(&dp->external_ids, "logical-router", UUID_FMT,
- UUID_ARGS(&nbr->header_.uuid));
+ const struct nbrec_logical_router *nbr;
+ bool is_deleted;
+ bool is_new;
+ bool is_updated;
+ struct ovn_unsynced_datapath *udp;
+ NBREC_LOGICAL_ROUTER_TABLE_FOR_EACH_TRACKED (nbr, nb_lr_table) {
+ udp = NULL;
+ is_deleted = false;
+ is_new = false;
+ is_updated = false;
+ if (nbrec_logical_router_is_deleted(nbr)) {
+ udp = ovn_unsynced_datapath_find(map, &nbr->header_.uuid);
+ if (udp) {
+ is_deleted = true;
+ }
+ } else if (nbrec_logical_router_is_new(nbr)) {
+ if (logical_router_is_enabled(nbr)) {
+ is_new = true;
+ }
+ } else {
+ /* Updated logical routers may need to be treated as "new" or
+ * "deleted" if the "enabled" setting changed on the router.
+ */
+ udp = ovn_unsynced_datapath_find(map, &nbr->header_.uuid);
+ if (!udp) {
+ if (logical_router_is_enabled(nbr)) {
+ /* The router was previously not enabled but now
+ * is. Treat it as new.
+ */
+ is_new = true;
+ }
+ } else if (!logical_router_is_enabled(nbr)) {
+ /* The router was previously enabled but now is not
+ * enabled. Treat it as if it were deleted.
+ */
+ is_deleted = true;
+ } else {
+ is_updated = true;
+ }
+ }
- hmap_insert(&map->dps, &dp->hmap_node, uuid_hash(&nbr->header_.uuid));
+ if (is_new) {
+ ovs_assert(!udp);
+ udp = allocate_unsynced_router(nbr);
+ hmap_insert(&map->dps, &udp->hmap_node,
+ uuid_hash(&nbr->header_.uuid));
+ hmapx_add(&map->new, udp);
+ result = EN_HANDLED_UPDATED;
+ } else if (is_deleted) {
+ ovs_assert(udp);
+ hmap_remove(&map->dps, &udp->hmap_node);
+ hmapx_add(&map->deleted, udp);
+ result = EN_HANDLED_UPDATED;
+ } else if (is_updated) {
+ /* We could try to modify the unsynced datapath in place
+ * based on the new logical router, but it's easier to
+ * just allocate a new one.
+ */
+ ovs_assert(udp);
+ hmap_remove(&map->dps, &udp->hmap_node);
+ ovn_unsynced_datapath_destroy(udp);
+ free(udp);
+ udp = allocate_unsynced_router(nbr);
+ hmap_insert(&map->dps, &udp->hmap_node,
+ uuid_hash(&nbr->header_.uuid));
+ hmapx_add(&map->updated, udp);
+ result = EN_HANDLED_UPDATED;
+ }
}
- return EN_UPDATED;
+ return result;
}
void
diff --git a/northd/en-datapath-logical-router.h
b/northd/en-datapath-logical-router.h
index 31e70fbf5..6ab719830 100644
--- a/northd/en-datapath-logical-router.h
+++ b/northd/en-datapath-logical-router.h
@@ -45,4 +45,10 @@ enum engine_node_state en_datapath_synced_logical_router_run(
void en_datapath_synced_logical_router_cleanup(void *data);
+void en_datapath_logical_router_clear_tracked_data(void *data);
+
+enum engine_input_handler_result
+en_datapath_logical_router_logical_router_handler(struct engine_node *,
+ void *);
+
#endif /* EN_DATAPATH_LOGICAL_ROUTER_H */
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 2e9c33d17..26cfc7069 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -187,7 +187,7 @@ static ENGINE_NODE(advertised_route_sync);
static ENGINE_NODE(learned_route_sync, CLEAR_TRACKED_DATA);
static ENGINE_NODE(dynamic_routes);
static ENGINE_NODE(group_ecmp_route, CLEAR_TRACKED_DATA);
-static ENGINE_NODE(datapath_logical_router);
+static ENGINE_NODE(datapath_logical_router, CLEAR_TRACKED_DATA);
static ENGINE_NODE(datapath_logical_switch);
static ENGINE_NODE(datapath_sync);
static ENGINE_NODE(datapath_synced_logical_router);
@@ -234,7 +234,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
engine_add_input(&en_datapath_logical_switch, &en_nb_logical_switch,
NULL);
engine_add_input(&en_datapath_logical_switch, &en_global_config, NULL);
- engine_add_input(&en_datapath_logical_router, &en_nb_logical_router, NULL);
+ engine_add_input(&en_datapath_logical_router, &en_nb_logical_router,
+ en_datapath_logical_router_logical_router_handler);
engine_add_input(&en_datapath_sync, &en_datapath_logical_switch, NULL);
engine_add_input(&en_datapath_sync, &en_datapath_logical_router, NULL);
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 33cd8a4ae..a0bfda993 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -17730,3 +17730,43 @@ check as northd ovn-appctl -t ovn-northd
inc-engine/clear-stats
AT_CLEANUP
])
+
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([Unsynced Logical Router Incremental Processing])
+ovn_start
+
+# Start by issuing a recompute just so we know that the engine
+# has run at least once.
+check as northd ovn-appctl -t ovn-northd inc-engine/recompute
+
+# Now all of our router changes should be handled incrementally.
+# There is no circumstance under which en-datapath-logical-router
+# should have to recompute.
+
+# Add a router
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+check ovn-nbctl lr-add lr1
+check_engine_stats datapath_logical_router norecompute compute
+
+# Update router options
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+check ovn-nbctl set Logical_Router lr1 options:ct-zone-limit=10
+check_engine_stats datapath_logical_router norecompute compute
+
+# Disable logical router.
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+check ovn-nbctl set Logical_Router lr1 enabled=false
+check_engine_stats datapath_logical_router norecompute compute
+
+# Re-enable logical router.
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+check ovn-nbctl set Logical_Router lr1 enabled=true
+check_engine_stats datapath_logical_router norecompute compute
+
+# Delete logical router
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+check ovn-nbctl lr-del lr1
+check_engine_stats datapath_logical_router norecompute compute
+
+AT_CLEANUP
+])
