On 8/14/25 7:48 AM, Ales Musil wrote:
> On Wed, Aug 13, 2025 at 7:21 PM Mark Michelson via dev <
> ovs-dev@openvswitch.org> wrote:
>
>> 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 <mmich...@redhat.com>
>> ---
>>
>
> Hi Mark and Dumitru,
>
> I have just one comment down below.
>
Hi Mark, Ales,
>
>> V17:
>> - cherry picked from v15
>> ---
>> northd/datapath-sync.c | 53 ++++++++
>> northd/datapath-sync.h | 13 ++
>> northd/en-datapath-logical-router.c | 182 ++++++++++++++++++++--------
>> northd/en-datapath-logical-router.h | 5 +
>> northd/inc-proc-northd.c | 5 +-
>> tests/ovn-northd.at | 45 +++++++
>> 6 files changed, 250 insertions(+), 53 deletions(-)
>>
>> diff --git a/northd/datapath-sync.c b/northd/datapath-sync.c
>> index 5de5b8439..40f1761db 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,
>> };
>> }
>> @@ -82,9 +87,57 @@ void
>> ovn_unsynced_datapath_map_destroy(struct ovn_unsynced_datapath_map *map)
>> {
>> struct ovn_unsynced_datapath *dp;
>> + struct hmapx_node *node;
>> +
>> + hmapx_destroy(&map->new);
>> + hmapx_destroy(&map->updated);
>> + HMAPX_FOR_EACH_SAFE (node, &map->deleted) {
>> + /* Items in the deleted hmapx need to be freed individually since
>> + * they are not in map->dps.
>> + */
>> + dp = node->data;
>> + ovn_unsynced_datapath_destroy(dp);
>> + free(dp);
>> + }
>> + hmapx_destroy(&map->deleted);
>> +
>> HMAP_FOR_EACH_POP (dp, hmap_node, &map->dps) {
>> ovn_unsynced_datapath_destroy(dp);
>> free(dp);
>> }
>> 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 246500c7b..081d0af93 100644
>> --- a/northd/datapath-sync.h
>> +++ b/northd/datapath-sync.h
>> @@ -18,6 +18,7 @@
>>
>> #include "openvswitch/hmap.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.
>> @@ -62,6 +63,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 {
>> @@ -83,4 +89,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 86c3eb365..a033e4520 100644
>> --- a/northd/en-datapath-logical-router.c
>> +++ b/northd/en-datapath-logical-router.c
>> @@ -37,6 +37,80 @@ en_datapath_logical_router_init(struct engine_node
>> *node OVS_UNUSED,
>> return map;
>> }
>>
>> +static void
>> +gather_external_ids(const struct nbrec_logical_router *nbr,
>> + struct smap *external_ids)
>> +{
>> + smap_add(external_ids, "name", nbr->name);
>> + const char *neutron_router = smap_get(&nbr->options,
>> + "neutron:router_name");
>> + if (neutron_router && neutron_router[0]) {
>> + smap_add(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(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(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(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(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(external_ids, "logical-router", UUID_FMT,
>> + UUID_ARGS(&nbr->header_.uuid));
>> +}
>> +
>> +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_);
>> +
>> + gather_external_ids(nbr, &dp->external_ids);
>> + 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)
>> {
>> @@ -50,69 +124,75 @@ 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)
>> +{
>> + ovn_unsynced_datapath_map_clear_tracked_data(data);
>> +}
>>
>> - 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));
>> +
>> + struct ovn_unsynced_datapath_map *map = data;
>> +
>> + struct ovn_unsynced_datapath *udp;
>> + const struct nbrec_logical_router *nbr;
>> + NBREC_LOGICAL_ROUTER_TABLE_FOR_EACH_TRACKED (nbr, nb_lr_table) {
>> + udp = ovn_unsynced_datapath_find(map, &nbr->header_.uuid);
>> +
>> + if (nbrec_logical_router_is_deleted(nbr) && !udp) {
>> + return EN_UNHANDLED;
>> }
>>
>> - /* 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);
>> + if (nbrec_logical_router_is_new(nbr) && udp) {
>> + return EN_UNHANDLED;
>> }
>>
>> - /* 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));
>> + if (udp) {
>> + if (nbrec_logical_router_is_deleted(nbr) ||
>> + !logical_router_is_enabled(nbr)) {
>> + hmap_remove(&map->dps, &udp->hmap_node);
>> + hmapx_add(&map->deleted, udp);
>> + } else {
>> + /* 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.
>> + */
>>
>
> This comment isn't accurate, we are actually modifying it in
> place. There is only one thing that is created from scratch
> and that's the external_ids.
>
Good point, I changed it to:
/* We could try to modify the unsynced datapath external_ids
* in place based on the new logical router, but it's easier to
* just create a new map.
*/
And applied the patch to main.
Thanks a lot for the hard work on this one!
Regards,
Dumitru
>
>> + udp->requested_tunnel_key =
>> + smap_get_int(&nbr->options, "requested-tnl-key", 0);
>> + smap_destroy(&udp->external_ids);
>> + smap_init(&udp->external_ids);
>> + gather_external_ids(nbr, &udp->external_ids);
>> + hmapx_add(&map->updated, udp);
>> + }
>> + } else if (logical_router_is_enabled(nbr)) {
>> + udp = allocate_unsynced_router(nbr);
>> + hmap_insert(&map->dps, &udp->hmap_node,
>> + uuid_hash(&nbr->header_.uuid));
>> + hmapx_add(&map->new, udp);
>> + }
>> + }
>>
>> - hmap_insert(&map->dps, &dp->hmap_node,
>> uuid_hash(&nbr->header_.uuid));
>> + if (!(hmapx_is_empty(&map->new) && hmapx_is_empty(&map->updated) &&
>> + hmapx_is_empty(&map->deleted))) {
>> + return EN_HANDLED_UPDATED;
>> }
>>
>> - return EN_UPDATED;
>> + return EN_HANDLED_UNCHANGED;
>> }
>>
>> void
>> diff --git a/northd/en-datapath-logical-router.h
>> b/northd/en-datapath-logical-router.h
>> index 31e70fbf5..fa2bf798f 100644
>> --- a/northd/en-datapath-logical-router.h
>> +++ b/northd/en-datapath-logical-router.h
>> @@ -25,6 +25,7 @@ void *en_datapath_logical_router_init(struct engine_node
>> *,
>>
>> enum engine_node_state en_datapath_logical_router_run(struct engine_node
>> *,
>> void *data);
>> +void en_datapath_logical_router_clear_tracked_data(void *data);
>> void en_datapath_logical_router_cleanup(void *data);
>>
>> struct ovn_synced_logical_router {
>> @@ -45,4 +46,8 @@ enum engine_node_state
>> en_datapath_synced_logical_router_run(
>>
>> void en_datapath_synced_logical_router_cleanup(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 f34110eb8..ff309c402 100644
>> --- a/northd/inc-proc-northd.c
>> +++ b/northd/inc-proc-northd.c
>> @@ -182,7 +182,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);
>> @@ -220,7 +220,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 0ab1384f6..a85f75a2f 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -17641,3 +17641,48 @@ 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 --wait=sb lr-add lr1
>> +check_engine_stats datapath_logical_router norecompute compute
>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>> +
>> +# Update router options
>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>> +check ovn-nbctl --wait=sb set Logical_Router lr1 options:ct-zone-limit=10
>> +check_engine_stats datapath_logical_router norecompute compute
>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>> +
>> +# Disable logical router.
>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>> +check ovn-nbctl --wait=sb set Logical_Router lr1 enabled=false
>> +check_engine_stats datapath_logical_router norecompute compute
>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>> +
>> +# Re-enable logical router.
>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>> +check ovn-nbctl --wait=sb set Logical_Router lr1 enabled=true
>> +check_engine_stats datapath_logical_router norecompute compute
>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>> +
>> +# Delete logical router
>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
>> +check ovn-nbctl --wait=sb lr-del lr1
>> +check_engine_stats datapath_logical_router norecompute compute
>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>> +
>> +AT_CLEANUP
>> +])
>> --
>> 2.49.0
>>
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
> With that addressed:
> Acked-by: Ales Musil <amu...@redhat.com>
>
> Thanks,
> Ales
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
