[ovs-dev,v3,2/7] northd: Introduce incremental processing for northd

Initial implementation adds a single node (northd). This single
node executes the northd processing pipeline but does not do so
incrementally.

In order to develop incremental processing for northd, the code
will be organised with a .c/.h file for each I-P node following
the naming convention en-<node name>.c/.h. These files will
contain definition of the node data, the main node processing
functions and change handlers (if any). The purpose of these nodes
will be coordination of the nodes work and implemention of the
relevant interfaces to plugin to the I-P framework. The actual
work that will be executed by the node will be organised into
a companion file or files. Ideally this file will follow the
naming convention of the node: e.g. en-<node name>.c is
associated with <node name>.c.

Initial node topology sees the northd node dependent on all DB
nodes. This will evolve over time.

Co-authored-by: Numan Siddique <numans@ovn.org>
Signed-off-by: Numan Siddique <numans@ovn.org>
Signed-off-by: Mark Gray <mark.d.gray@redhat.com>
---
 lib/inc-proc-eng.h       |  16 +++
 northd/automake.mk       |   4 +
 northd/en-northd.c       |  45 +++++++
 northd/en-northd.h       |  17 +++
 northd/inc-proc-northd.c | 254 +++++++++++++++++++++++++++++++++++++++
 northd/inc-proc-northd.h |  15 +++
 northd/northd.c          |  12 +-
 northd/northd.h          |   9 +-
 northd/ovn-northd.c      | 201 ++++++++++++++++++++-----------
 9 files changed, 490 insertions(+), 83 deletions(-)
 create mode 100644 northd/en-northd.c
 create mode 100644 northd/en-northd.h
 create mode 100644 northd/inc-proc-northd.c
 create mode 100644 northd/inc-proc-northd.h

On Mon, Oct 18, 2021 at 5:14 AM Mark Gray <mark.d.gray@redhat.com> wrote:
>
> Initial implementation adds a single node (northd). This single
> node executes the northd processing pipeline but does not do so
> incrementally.
>
> In order to develop incremental processing for northd, the code
> will be organised with a .c/.h file for each I-P node following
> the naming convention en-<node name>.c/.h. These files will
> contain definition of the node data, the main node processing
> functions and change handlers (if any). The purpose of these nodes
> will be coordination of the nodes work and implemention of the
> relevant interfaces to plugin to the I-P framework. The actual
> work that will be executed by the node will be organised into
> a companion file or files. Ideally this file will follow the
> naming convention of the node: e.g. en-<node name>.c is
> associated with <node name>.c.
>
> Initial node topology sees the northd node dependent on all DB
> nodes. This will evolve over time.
>
> Co-authored-by: Numan Siddique <numans@ovn.org>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> Signed-off-by: Mark Gray <mark.d.gray@redhat.com>
> ---
>  lib/inc-proc-eng.h       |  16 +++
>  northd/automake.mk       |   4 +
>  northd/en-northd.c       |  45 +++++++
>  northd/en-northd.h       |  17 +++
>  northd/inc-proc-northd.c | 254 +++++++++++++++++++++++++++++++++++++++
>  northd/inc-proc-northd.h |  15 +++
>  northd/northd.c          |  12 +-
>  northd/northd.h          |   9 +-
>  northd/ovn-northd.c      | 201 ++++++++++++++++++++-----------
>  9 files changed, 490 insertions(+), 83 deletions(-)
>  create mode 100644 northd/en-northd.c
>  create mode 100644 northd/en-northd.h
>  create mode 100644 northd/inc-proc-northd.c
>  create mode 100644 northd/inc-proc-northd.h
>
> diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h
> index 1ccae559dff6..a3f5a7e64287 100644
> --- a/lib/inc-proc-eng.h
> +++ b/lib/inc-proc-eng.h
> @@ -63,15 +63,22 @@
>  #define ENGINE_MAX_INPUT 256
>  #define ENGINE_MAX_OVSDB_INDEX 256
>
> +#include <stdbool.h>
> +#include <stdint.h>
> +
> +#include "compiler.h"
> +
>  struct engine_context {
>      struct ovsdb_idl_txn *ovs_idl_txn;
>      struct ovsdb_idl_txn *ovnsb_idl_txn;
> +    struct ovsdb_idl_txn *ovnnb_idl_txn;

It would be better to add comments to clarify for the fields used by only
one component but not the other.

>      void *client_ctx;
>  };
>
>  /* Arguments to be passed to the engine at engine_init(). */
>  struct engine_arg {
>      struct ovsdb_idl *sb_idl;
> +    struct ovsdb_idl *nb_idl;
>      struct ovsdb_idl *ovs_idl;
>  };
>
> @@ -347,6 +354,11 @@ static void en_##DB_NAME##_##TBL_NAME##_cleanup(void
*data OVS_UNUSED) \
>  #define ENGINE_FUNC_SB(TBL_NAME) \
>      ENGINE_FUNC_OVSDB(sb, TBL_NAME)
>
> +/* Macro to define member functions of an engine node which represents
> + * a table of OVN NB DB */
> +#define ENGINE_FUNC_NB(TBL_NAME) \
> +    ENGINE_FUNC_OVSDB(nb, TBL_NAME)
> +
>  /* Macro to define member functions of an engine node which represents
>   * a table of open_vswitch DB */
>  #define ENGINE_FUNC_OVS(TBL_NAME) \
> @@ -360,6 +372,10 @@ static void en_##DB_NAME##_##TBL_NAME##_cleanup(void
*data OVS_UNUSED) \
>  #define ENGINE_NODE_SB(TBL_NAME, TBL_NAME_STR) \
>      ENGINE_NODE_OVSDB(sb, "SB", TBL_NAME, TBL_NAME_STR);
>
> +/* Macro to define an engine node which represents a table of OVN NB DB
*/
> +#define ENGINE_NODE_NB(TBL_NAME, TBL_NAME_STR) \
> +    ENGINE_NODE_OVSDB(nb, "NB", TBL_NAME, TBL_NAME_STR);
> +
>  /* Macro to define an engine node which represents a table of
open_vswitch
>   * DB */
>  #define ENGINE_NODE_OVS(TBL_NAME, TBL_NAME_STR) \
> diff --git a/northd/automake.mk b/northd/automake.mk
> index 35ad8c09d9ba..f0c1fb11c83a 100644
> --- a/northd/automake.mk
> +++ b/northd/automake.mk
> @@ -4,6 +4,10 @@ northd_ovn_northd_SOURCES = \
>         northd/northd.c \
>         northd/northd.h \
>         northd/ovn-northd.c \
> +       northd/en-northd.c \
> +       northd/en-northd.h \
> +       northd/inc-proc-northd.c \
> +       northd/inc-proc-northd.h \
>         northd/ipam.c \
>         northd/ipam.h
>  northd_ovn_northd_LDADD = \
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> new file mode 100644
> index 000000000000..d310fa4dd31f
> --- /dev/null
> +++ b/northd/en-northd.c
> @@ -0,0 +1,45 @@
> +/*
> + * 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 <getopt.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "en-northd.h"
> +#include "lib/inc-proc-eng.h"
> +#include "northd.h"
> +#include "openvswitch/vlog.h"
> +
> +VLOG_DEFINE_THIS_MODULE(en_northd);
> +
> +void en_northd_run(struct engine_node *node, void *data OVS_UNUSED)
> +{
> +    const struct engine_context *eng_ctx = engine_get_context();
> +    struct northd_context *ctx = eng_ctx->client_ctx;

We should define a separate context structure for northd's engine
client_ctx, which should only include necessary global vars for executing
DB txn. Or maybe client_ctx is not needed for this first node. Basically,
we shouldn't assign the existing northd_context to the I-P engine's
client_ctx, because otherwise when there are more engine nodes, all the
nodes will be able to access the data in the northd_context, and it would
be a mess if a node carelessly accesses data that doesn't belong to its
input.

For each single node, all its input should be retrieved within the _run()
or _handler() functions through the engine APIs. So here for this single
node, the input of ovn_db_run() should be constructed here instead of
getting it from engine_context.

(After writing up the above comments I saw that in the patch "northd:
Introduce struct northd_data" you did split the structure, but the above
comment still applies. 1) the engine ctx shouldn't include
ovnnb_idl/ovnsb_idl, 2) the NB/SB tables and index required by the node
should belong to the input instead of the node's data itself.)

Other than this (and the parts related to this), the rest of the patch
looks good to me. I will spend some more time on the other patches of the
series.

Thanks,
Han

> +    ovn_db_run(ctx);
> +
> +    engine_set_node_state(node, EN_UPDATED);
> +
> +}
> +void *en_northd_init(struct engine_node *node OVS_UNUSED,
> +                     struct engine_arg *arg OVS_UNUSED)
> +{
> +    return NULL;
> +}
> +
> +void en_northd_cleanup(void *data OVS_UNUSED)
> +{
> +}
> diff --git a/northd/en-northd.h b/northd/en-northd.h
> new file mode 100644
> index 000000000000..0e7f76245e69
> --- /dev/null
> +++ b/northd/en-northd.h
> @@ -0,0 +1,17 @@
> +#ifndef EN_NORTHD_H
> +#define EN_NORTHD_H 1
> +
> +#include <config.h>
> +
> +#include <getopt.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "lib/inc-proc-eng.h"
> +
> +void en_northd_run(struct engine_node *node OVS_UNUSED, void *data
OVS_UNUSED);
> +void *en_northd_init(struct engine_node *node OVS_UNUSED,
> +                     struct engine_arg *arg);
> +void en_northd_cleanup(void *data);
> +
> +#endif /* EN_NORTHD_H */
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> new file mode 100644
> index 000000000000..85baeb07d3d9
> --- /dev/null
> +++ b/northd/inc-proc-northd.c
> @@ -0,0 +1,254 @@
> +/*
> + * 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 <getopt.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "lib/inc-proc-eng.h"
> +#include "lib/ovn-nb-idl.h"
> +#include "lib/ovn-sb-idl.h"
> +#include "openvswitch/poll-loop.h"
> +#include "openvswitch/vlog.h"
> +#include "inc-proc-northd.h"
> +#include "en-northd.h"
> +#include "util.h"
> +
> +VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
> +
> +#define NB_NODES \
> +    NB_NODE(nb_global, "nb_global") \
> +    NB_NODE(copp, "copp") \
> +    NB_NODE(logical_switch, "logical_switch") \
> +    NB_NODE(logical_switch_port, "logical_switch_port") \
> +    NB_NODE(forwarding_group, "forwarding_group") \
> +    NB_NODE(address_set, "address_set") \
> +    NB_NODE(port_group, "port_group") \
> +    NB_NODE(load_balancer, "load_balancer") \
> +    NB_NODE(load_balancer_health_check, "load_balancer_health_check") \
> +    NB_NODE(acl, "acl") \
> +    NB_NODE(logical_router, "logical_router") \
> +    NB_NODE(qos, "qos") \
> +    NB_NODE(meter, "meter") \
> +    NB_NODE(meter_band, "meter_band") \
> +    NB_NODE(logical_router_port, "logical_router_port") \
> +    NB_NODE(logical_router_static_route, "logical_router_static_route") \
> +    NB_NODE(logical_router_policy, "logical_router_policy") \
> +    NB_NODE(nat, "nat") \
> +    NB_NODE(dhcp_options, "dhcp_options") \
> +    NB_NODE(connection, "connection") \
> +    NB_NODE(dns, "dns") \
> +    NB_NODE(ssl, "ssl") \
> +    NB_NODE(gateway_chassis, "gateway_chassis") \
> +    NB_NODE(ha_chassis_group, "ha_chassis_group") \
> +    NB_NODE(ha_chassis, "ha_chassis") \
> +    NB_NODE(bfd, "bfd")
> +
> +    enum nb_engine_node {
> +#define NB_NODE(NAME, NAME_STR) NB_##NAME,
> +    NB_NODES
> +#undef NB_NODE
> +    };
> +
> +/* Define engine node functions for nodes that represent NB tables
> + *
> + * en_nb_<TABLE_NAME>_run()
> + * en_nb_<TABLE_NAME>_init()
> + * en_nb_<TABLE_NAME>_cleanup()
> + */
> +#define NB_NODE(NAME, NAME_STR) ENGINE_FUNC_NB(NAME);
> +    NB_NODES
> +#undef NB_NODE
> +
> +#define SB_NODES \
> +    SB_NODE(sb_global, "sb_global") \
> +    SB_NODE(chassis, "chassis") \
> +    SB_NODE(chassis_private, "chassis_private") \
> +    SB_NODE(encap, "encap") \
> +    SB_NODE(address_set, "address_set") \
> +    SB_NODE(port_group, "port_group") \
> +    SB_NODE(logical_flow, "logical_flow") \
> +    SB_NODE(logical_dp_group, "logical_DP_group") \
> +    SB_NODE(multicast_group, "multicast_group") \
> +    SB_NODE(meter, "meter") \
> +    SB_NODE(meter_band, "meter_band") \
> +    SB_NODE(datapath_binding, "datapath_binding") \
> +    SB_NODE(port_binding, "port_binding") \
> +    SB_NODE(mac_binding, "mac_binding") \
> +    SB_NODE(dhcp_options, "dhcp_options") \
> +    SB_NODE(dhcpv6_options, "dhcpv6_options") \
> +    SB_NODE(connection, "connection") \
> +    SB_NODE(ssl, "ssl") \
> +    SB_NODE(dns, "dns") \
> +    SB_NODE(rbac_role, "rbac_role") \
> +    SB_NODE(rbac_permission, "rbac_permission") \
> +    SB_NODE(gateway_chassis, "gateway_chassis") \
> +    SB_NODE(ha_chassis, "ha_chassis") \
> +    SB_NODE(ha_chassis_group, "ha_chassis_group") \
> +    SB_NODE(controller_event, "controller_event") \
> +    SB_NODE(ip_multicast, "ip_multicast") \
> +    SB_NODE(igmp_group, "igmp_group") \
> +    SB_NODE(service_monitor, "service_monitor") \
> +    SB_NODE(load_balancer, "load_balancer") \
> +    SB_NODE(bfd, "bfd") \
> +    SB_NODE(fdb, "fdb")
> +
> +enum sb_engine_node {
> +#define SB_NODE(NAME, NAME_STR) SB_##NAME,
> +    SB_NODES
> +#undef SB_NODE
> +};
> +
> +/* Define engine node functions for nodes that represent SB tables
> + *
> + * en_sb_<TABLE_NAME>_run()
> + * en_sb_<TABLE_NAME>_init()
> + * en_sb_<TABLE_NAME>_cleanup()
> + */
> +#define SB_NODE(NAME, NAME_STR) ENGINE_FUNC_SB(NAME);
> +    SB_NODES
> +#undef SB_NODE
> +
> +/* Define engine nodes for NB and SB tables
> + *
> + * struct engine_node en_nb_<TABLE_NAME>
> + * struct engine_node en_sb_<TABLE_NAME>
> + *
> + * Define nodes as static to avoid sparse errors.
> + */
> +#define NB_NODE(NAME, NAME_STR) static ENGINE_NODE_NB(NAME, NAME_STR);
> +    NB_NODES
> +#undef NB_NODE
> +
> +#define SB_NODE(NAME, NAME_STR) static ENGINE_NODE_SB(NAME, NAME_STR);
> +    SB_NODES
> +#undef SB_NODE
> +
> +/* Define engine nodes for other nodes. They should be defined as static
to
> + * avoid sparse errors. */
> +static ENGINE_NODE(northd, "northd");
> +
> +void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> +                          struct ovsdb_idl_loop *sb)
> +{
> +    /* Define relationships between nodes where first argument is
dependent
> +     * on the second argument */
> +    engine_add_input(&en_northd, &en_nb_nb_global, NULL);
> +    engine_add_input(&en_northd, &en_nb_copp, NULL);
> +    engine_add_input(&en_northd, &en_nb_logical_switch, NULL);
> +    engine_add_input(&en_northd, &en_nb_logical_switch_port, NULL);
> +    engine_add_input(&en_northd, &en_nb_forwarding_group, NULL);
> +    engine_add_input(&en_northd, &en_nb_address_set, NULL);
> +    engine_add_input(&en_northd, &en_nb_port_group, NULL);
> +    engine_add_input(&en_northd, &en_nb_load_balancer, NULL);
> +    engine_add_input(&en_northd, &en_nb_load_balancer_health_check,
NULL);
> +    engine_add_input(&en_northd, &en_nb_acl, NULL);
> +    engine_add_input(&en_northd, &en_nb_logical_router, NULL);
> +    engine_add_input(&en_northd, &en_nb_qos, NULL);
> +    engine_add_input(&en_northd, &en_nb_meter, NULL);
> +    engine_add_input(&en_northd, &en_nb_meter_band, NULL);
> +    engine_add_input(&en_northd, &en_nb_logical_router_port, NULL);
> +    engine_add_input(&en_northd, &en_nb_logical_router_static_route,
NULL);
> +    engine_add_input(&en_northd, &en_nb_logical_router_policy, NULL);
> +    engine_add_input(&en_northd, &en_nb_nat, NULL);
> +    engine_add_input(&en_northd, &en_nb_dhcp_options, NULL);
> +    engine_add_input(&en_northd, &en_nb_connection, NULL);
> +    engine_add_input(&en_northd, &en_nb_dns, NULL);
> +    engine_add_input(&en_northd, &en_nb_ssl, NULL);
> +    engine_add_input(&en_northd, &en_nb_gateway_chassis, NULL);
> +    engine_add_input(&en_northd, &en_nb_ha_chassis_group, NULL);
> +    engine_add_input(&en_northd, &en_nb_ha_chassis, NULL);
> +    engine_add_input(&en_northd, &en_nb_bfd, NULL);
> +
> +    engine_add_input(&en_northd, &en_sb_sb_global, NULL);
> +    engine_add_input(&en_northd, &en_sb_chassis, NULL);
> +    engine_add_input(&en_northd, &en_sb_chassis_private, NULL);
> +    engine_add_input(&en_northd, &en_sb_encap, NULL);
> +    engine_add_input(&en_northd, &en_sb_address_set, NULL);
> +    engine_add_input(&en_northd, &en_sb_port_group, NULL);
> +    engine_add_input(&en_northd, &en_sb_logical_flow, NULL);
> +    engine_add_input(&en_northd, &en_sb_logical_dp_group, NULL);
> +    engine_add_input(&en_northd, &en_sb_multicast_group, NULL);
> +    engine_add_input(&en_northd, &en_sb_meter, NULL);
> +    engine_add_input(&en_northd, &en_sb_meter_band, NULL);
> +    engine_add_input(&en_northd, &en_sb_datapath_binding, NULL);
> +    engine_add_input(&en_northd, &en_sb_port_binding, NULL);
> +    engine_add_input(&en_northd, &en_sb_mac_binding, NULL);
> +    engine_add_input(&en_northd, &en_sb_dhcp_options, NULL);
> +    engine_add_input(&en_northd, &en_sb_dhcpv6_options, NULL);
> +    engine_add_input(&en_northd, &en_sb_connection, NULL);
> +    engine_add_input(&en_northd, &en_sb_ssl, NULL);
> +    engine_add_input(&en_northd, &en_sb_dns, NULL);
> +    engine_add_input(&en_northd, &en_sb_rbac_role, NULL);
> +    engine_add_input(&en_northd, &en_sb_rbac_permission, NULL);
> +    engine_add_input(&en_northd, &en_sb_gateway_chassis, NULL);
> +    engine_add_input(&en_northd, &en_sb_ha_chassis, NULL);
> +    engine_add_input(&en_northd, &en_sb_ha_chassis_group, NULL);
> +    engine_add_input(&en_northd, &en_sb_controller_event, NULL);
> +    engine_add_input(&en_northd, &en_sb_ip_multicast, NULL);
> +    engine_add_input(&en_northd, &en_sb_igmp_group, NULL);
> +    engine_add_input(&en_northd, &en_sb_service_monitor, NULL);
> +    engine_add_input(&en_northd, &en_sb_load_balancer, NULL);
> +    engine_add_input(&en_northd, &en_sb_bfd, NULL);
> +    engine_add_input(&en_northd, &en_sb_fdb, NULL);
> +
> +    struct engine_arg engine_arg = {
> +        .nb_idl = nb->idl,
> +        .sb_idl = sb->idl,
> +    };
> +
> +    engine_init(&en_northd, &engine_arg);
> +}
> +
> +void inc_proc_northd_run(struct northd_context *ctx,
> +                         bool recompute) {
> +    engine_set_force_recompute(recompute);
> +    engine_init_run();
> +
> +    struct engine_context eng_ctx = {
> +        .ovnnb_idl_txn = ctx->ovnnb_txn,
> +        .ovnsb_idl_txn = ctx->ovnsb_txn,
> +        .client_ctx = ctx,
> +    };
> +
> +    engine_set_context(&eng_ctx);
> +
> +    if (ctx->ovnnb_txn && ctx->ovnsb_txn) {
> +        engine_run(true);
> +    }
> +
> +    if (!engine_has_run()) {
> +        if (engine_need_run()) {
> +            VLOG_DBG("engine did not run, force recompute next time.");
> +            engine_set_force_recompute(true);
> +            poll_immediate_wake();
> +        } else {
> +            VLOG_DBG("engine did not run, and it was not needed");
> +        }
> +    } else if (engine_aborted()) {
> +        VLOG_DBG("engine was aborted, force recompute next time.");
> +        engine_set_force_recompute(true);
> +        poll_immediate_wake();
> +    } else {
> +        engine_set_force_recompute(false);
> +    }
> +}
> +
> +void inc_proc_northd_cleanup(void)
> +{
> +    engine_cleanup();
> +    engine_set_context(NULL);
> +}
> diff --git a/northd/inc-proc-northd.h b/northd/inc-proc-northd.h
> new file mode 100644
> index 000000000000..09cb8f3b3a80
> --- /dev/null
> +++ b/northd/inc-proc-northd.h
> @@ -0,0 +1,15 @@
> +#ifndef INC_PROC_NORTHD_H
> +#define INC_PROC_NORTHD_H 1
> +
> +#include <config.h>
> +
> +#include "northd.h"
> +#include "ovsdb-idl.h"
> +
> +void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> +                          struct ovsdb_idl_loop *sb);
> +void inc_proc_northd_run(struct northd_context *ctx,
> +                         bool recompute);
> +void inc_proc_northd_cleanup(void);
> +
> +#endif /* INC_PROC_NORTHD */
> diff --git a/northd/northd.c b/northd/northd.c
> index 32ab3baf3b9c..1321e26faa9d 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -14800,10 +14800,7 @@ ovnsb_db_run(struct northd_context *ctx,
>  }
>
>  void
> -ovn_db_run(struct northd_context *ctx,
> -           struct ovsdb_idl_index *sbrec_chassis_by_name,
> -           struct ovsdb_idl_loop *ovnsb_idl_loop,
> -           const char *ovn_internal_version)
> +ovn_db_run(struct northd_context *ctx)
>  {
>      struct hmap datapaths, ports;
>      struct ovs_list lr_list;
> @@ -14813,13 +14810,14 @@ ovn_db_run(struct northd_context *ctx,
>      use_parallel_build = ctx->use_parallel_build;
>
>      int64_t start_time = time_wall_msec();
> +
>      stopwatch_start(OVNNB_DB_RUN_STOPWATCH_NAME, time_msec());
> -    ovnnb_db_run(ctx, sbrec_chassis_by_name, ovnsb_idl_loop,
> +    ovnnb_db_run(ctx, ctx->sbrec_chassis_by_name, ctx->ovnsb_idl_loop,
>                   &datapaths, &ports, &lr_list, start_time,
> -                 ovn_internal_version);
> +                 ctx->ovn_internal_version);
>      stopwatch_stop(OVNNB_DB_RUN_STOPWATCH_NAME, time_msec());
>      stopwatch_start(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
> -    ovnsb_db_run(ctx, ovnsb_idl_loop, &ports, start_time);
> +    ovnsb_db_run(ctx, ctx->ovnsb_idl_loop, &ports, start_time);
>      stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
>      destroy_datapaths_and_ports(&datapaths, &ports, &lr_list);
>  }
> diff --git a/northd/northd.h b/northd/northd.h
> index ffa2bbb4e88b..c0380ae60871 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -21,6 +21,8 @@ struct northd_context {
>      const char *ovnsb_db;
>      struct ovsdb_idl *ovnnb_idl;
>      struct ovsdb_idl *ovnsb_idl;
> +    struct ovsdb_idl_loop *ovnnb_idl_loop;
> +    struct ovsdb_idl_loop *ovnsb_idl_loop;
>      struct ovsdb_idl_txn *ovnnb_txn;
>      struct ovsdb_idl_txn *ovnsb_txn;
>      struct ovsdb_idl_index *sbrec_chassis_by_name;
> @@ -28,13 +30,10 @@ struct northd_context {
>      struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp;
>      struct ovsdb_idl_index *sbrec_ip_mcast_by_dp;
>
> +    const char *ovn_internal_version;
>      bool use_parallel_build;
>  };
>
> -void
> -ovn_db_run(struct northd_context *ctx,
> -           struct ovsdb_idl_index *sbrec_chassis_by_name,
> -           struct ovsdb_idl_loop *ovnsb_idl_loop,
> -           const char *ovn_internal_version);
> +void ovn_db_run(struct northd_context *ctx);
>
>  #endif /* NORTHD_H */
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 39aa960559dc..0c94afddb484 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -22,6 +22,7 @@
>  #include "command-line.h"
>  #include "daemon.h"
>  #include "fatal-signal.h"
> +#include "inc-proc-northd.h"
>  #include "lib/ip-mcast-index.h"
>  #include "lib/mcast-group-index.h"
>  #include "memory.h"
> @@ -439,6 +440,14 @@ check_and_add_supported_dhcpv6_opts_to_sb_db(struct
northd_context *ctx)
>  }
>
>  static void
> +add_column_noalert(struct ovsdb_idl *idl,
> +                   const struct ovsdb_idl_column *column)
> +{
> +    ovsdb_idl_add_column(idl, column);
> +    ovsdb_idl_omit_alert(idl, column);
> +}
> +
> +static void
>  usage(void)
>  {
>      printf("\
> @@ -560,14 +569,6 @@ parse_options(int argc OVS_UNUSED, char *argv[]
OVS_UNUSED,
>      free(short_options);
>  }
>
> -static void
> -add_column_noalert(struct ovsdb_idl *idl,
> -                   const struct ovsdb_idl_column *column)
> -{
> -    ovsdb_idl_add_column(idl, column);
> -    ovsdb_idl_omit_alert(idl, column);
> -}
> -
>  static void
>  update_ssl_config(void)
>  {
> @@ -645,6 +646,7 @@ main(int argc, char *argv[])
>      /* We want to detect (almost) all changes to the ovn-nb db. */
>      struct ovsdb_idl_loop ovnnb_idl_loop = OVSDB_IDL_LOOP_INITIALIZER(
>          ovsdb_idl_create(ovnnb_db, &nbrec_idl_class, true, true));
> +    ovsdb_idl_track_add_all(ovnnb_idl_loop.idl);
>      ovsdb_idl_omit_alert(ovnnb_idl_loop.idl,
>                           &nbrec_nb_global_col_nb_cfg_timestamp);
>      ovsdb_idl_omit_alert(ovnnb_idl_loop.idl,
&nbrec_nb_global_col_sb_cfg);
> @@ -659,12 +661,13 @@ main(int argc, char *argv[])
>
>      /* We want to detect only selected changes to the ovn-sb db. */
>      struct ovsdb_idl_loop ovnsb_idl_loop = OVSDB_IDL_LOOP_INITIALIZER(
> -        ovsdb_idl_create(ovnsb_db, &sbrec_idl_class, false, true));
> -
> +        ovsdb_idl_create(ovnsb_db, &sbrec_idl_class, true, true));
>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_sb_global);
>      add_column_noalert(ovnsb_idl_loop.idl, &sbrec_sb_global_col_nb_cfg);
>      add_column_noalert(ovnsb_idl_loop.idl, &sbrec_sb_global_col_options);
>      add_column_noalert(ovnsb_idl_loop.idl, &sbrec_sb_global_col_ipsec);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_sb_global_col_connections);
>
>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_logical_flow);
>      add_column_noalert(ovnsb_idl_loop.idl,
> @@ -716,24 +719,26 @@ main(int argc, char *argv[])
>      add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_mac);
>      add_column_noalert(ovnsb_idl_loop.idl,
>                         &sbrec_port_binding_col_nat_addresses);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
&sbrec_port_binding_col_chassis);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> -                         &sbrec_port_binding_col_gateway_chassis);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> -                         &sbrec_port_binding_col_ha_chassis_group);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> -                         &sbrec_port_binding_col_virtual_parent);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> -                         &sbrec_port_binding_col_up);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> -                         &sbrec_gateway_chassis_col_chassis);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
&sbrec_gateway_chassis_col_name);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> -                         &sbrec_gateway_chassis_col_priority);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> -                         &sbrec_gateway_chassis_col_external_ids);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> -                         &sbrec_gateway_chassis_col_options);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_port_binding_col_chassis);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_port_binding_col_gateway_chassis);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_port_binding_col_ha_chassis_group);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_port_binding_col_virtual_parent);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_port_binding_col_up);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_gateway_chassis_col_chassis);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_gateway_chassis_col_name);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_gateway_chassis_col_priority);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_gateway_chassis_col_external_ids);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_gateway_chassis_col_options);
>      add_column_noalert(ovnsb_idl_loop.idl,
>                         &sbrec_port_binding_col_external_ids);
>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_mac_binding);
> @@ -776,32 +781,35 @@ main(int argc, char *argv[])
>      add_column_noalert(ovnsb_idl_loop.idl,
&sbrec_rbac_permission_col_update);
>
>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_meter);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_meter_col_name);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_meter_col_unit);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_meter_col_bands);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
&sbrec_meter_col_name);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
&sbrec_meter_col_unit);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
&sbrec_meter_col_bands);
>
>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_meter_band);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
&sbrec_meter_band_col_action);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_meter_band_col_rate);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
&sbrec_meter_band_col_burst_size);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_meter_band_col_action);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
&sbrec_meter_band_col_rate);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_meter_band_col_burst_size);
>
>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_chassis);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_chassis_col_name);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
&sbrec_chassis_col_other_config);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_chassis_col_encaps);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
&sbrec_chassis_col_name);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_chassis_col_other_config);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
&sbrec_chassis_col_encaps);
>
>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_encap);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_encap_col_type);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
&sbrec_encap_col_type);
>
>      ovsdb_idl_add_table(ovnsb_idl_loop.idl,
&sbrec_table_chassis_private);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> -                         &sbrec_chassis_private_col_name);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> -                         &sbrec_chassis_private_col_chassis);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> -                         &sbrec_chassis_private_col_nb_cfg);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> -                         &sbrec_chassis_private_col_nb_cfg_timestamp);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_chassis_private_col_name);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_chassis_private_col_chassis);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_chassis_private_col_nb_cfg);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +
&sbrec_chassis_private_col_nb_cfg_timestamp);
>
>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_ha_chassis);
>      add_column_noalert(ovnsb_idl_loop.idl,
> @@ -822,10 +830,14 @@ main(int argc, char *argv[])
>                         &sbrec_ha_chassis_group_col_ref_chassis);
>
>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_igmp_group);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
&sbrec_igmp_group_col_address);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
&sbrec_igmp_group_col_datapath);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
&sbrec_igmp_group_col_chassis);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
&sbrec_igmp_group_col_ports);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_igmp_group_col_address);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_igmp_group_col_datapath);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_igmp_group_col_chassis);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_igmp_group_col_ports);
>
>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_ip_multicast);
>      add_column_noalert(ovnsb_idl_loop.idl,
> @@ -857,8 +869,8 @@ main(int argc, char *argv[])
>                         &sbrec_service_monitor_col_port);
>      add_column_noalert(ovnsb_idl_loop.idl,
>                         &sbrec_service_monitor_col_options);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> -                         &sbrec_service_monitor_col_status);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_service_monitor_col_status);
>      add_column_noalert(ovnsb_idl_loop.idl,
>                         &sbrec_service_monitor_col_protocol);
>      add_column_noalert(ovnsb_idl_loop.idl,
> @@ -878,19 +890,20 @@ main(int argc, char *argv[])
>                         &sbrec_load_balancer_col_external_ids);
>
>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_bfd);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
&sbrec_bfd_col_logical_port);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_dst_ip);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_status);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_min_tx);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_min_rx);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_detect_mult);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_disc);
> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_src_port);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_bfd_col_logical_port);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
&sbrec_bfd_col_dst_ip);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
&sbrec_bfd_col_status);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
&sbrec_bfd_col_min_tx);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
&sbrec_bfd_col_min_rx);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
&sbrec_bfd_col_detect_mult);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_disc);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
&sbrec_bfd_col_src_port);
>
>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_fdb);
> -    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_fdb_col_mac);
> -    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_fdb_col_dp_key);
> -    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_fdb_col_port_key);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl, &sbrec_fdb_col_mac);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
&sbrec_fdb_col_dp_key);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
&sbrec_fdb_col_port_key);
>
>      struct ovsdb_idl_index *sbrec_chassis_by_name
>          = chassis_index_create(ovnsb_idl_loop.idl);
> @@ -922,9 +935,16 @@ main(int argc, char *argv[])
>      stopwatch_create(LFLOWS_IGMP_STOPWATCH_NAME, SW_MS);
>      stopwatch_create(LFLOWS_DP_GROUPS_STOPWATCH_NAME, SW_MS);
>
> +    /* Initialize incremental processing engine for ovn-northd */
> +    inc_proc_northd_init(&ovnnb_idl_loop, &ovnsb_idl_loop);
> +
> +    unsigned int ovnnb_cond_seqno = UINT_MAX;
> +    unsigned int ovnsb_cond_seqno = UINT_MAX;
> +
>      /* Main loop. */
>      exiting = false;
>
> +    bool recompute = false;
>      while (!exiting) {
>          update_ssl_config();
>          memory_run();
> @@ -948,18 +968,46 @@ main(int argc, char *argv[])
>                  ovsdb_idl_set_lock(ovnsb_idl_loop.idl, "ovn_northd");
>              }
>
> +
> +            struct ovsdb_idl_txn *ovnnb_txn =
> +                        ovsdb_idl_loop_run(&ovnnb_idl_loop);
> +            unsigned int new_ovnnb_cond_seqno =
> +
 ovsdb_idl_get_condition_seqno(ovnnb_idl_loop.idl);
> +            if (new_ovnnb_cond_seqno != ovnnb_cond_seqno) {
> +                if (!new_ovnnb_cond_seqno) {
> +                    VLOG_INFO("OVN NB IDL reconnected, force
recompute.");
> +                    recompute = true;
> +                }
> +                ovnnb_cond_seqno = new_ovnnb_cond_seqno;
> +            }
> +
> +            struct ovsdb_idl_txn *ovnsb_txn =
> +                        ovsdb_idl_loop_run(&ovnsb_idl_loop);
> +            unsigned int new_ovnsb_cond_seqno =
> +
 ovsdb_idl_get_condition_seqno(ovnsb_idl_loop.idl);
> +            if (new_ovnsb_cond_seqno != ovnsb_cond_seqno) {
> +                if (!new_ovnsb_cond_seqno) {
> +                    VLOG_INFO("OVN SB IDL reconnected, force
recompute.");
> +                    recompute = true;
> +                }
> +                ovnsb_cond_seqno = new_ovnsb_cond_seqno;
> +            }
> +
>              struct northd_context ctx = {
>                  .ovnnb_db = ovnnb_db,
>                  .ovnsb_db = ovnsb_db,
>                  .ovnnb_idl = ovnnb_idl_loop.idl,
> -                .ovnnb_txn = ovsdb_idl_loop_run(&ovnnb_idl_loop),
> +                .ovnnb_idl_loop = &ovnnb_idl_loop,
> +                .ovnnb_txn = ovnnb_txn,
>                  .ovnsb_idl = ovnsb_idl_loop.idl,
> -                .ovnsb_txn = ovsdb_idl_loop_run(&ovnsb_idl_loop),
> +                .ovnsb_idl_loop = &ovnsb_idl_loop,
> +                .ovnsb_txn = ovnsb_txn,
>                  .sbrec_chassis_by_name = sbrec_chassis_by_name,
>                  .sbrec_ha_chassis_grp_by_name =
sbrec_ha_chassis_grp_by_name,
>                  .sbrec_mcast_group_by_name_dp =
sbrec_mcast_group_by_name_dp,
>                  .sbrec_ip_mcast_by_dp = sbrec_ip_mcast_by_dp,
>                  .use_parallel_build = use_parallel_build,
> +                .ovn_internal_version = ovn_internal_version,
>              };
>
>              if (!state.had_lock &&
ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
> @@ -975,17 +1023,15 @@ main(int argc, char *argv[])
>              }
>
>              if (ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
> -                ovn_db_run(&ctx, sbrec_chassis_by_name, &ovnsb_idl_loop,
> -                           ovn_internal_version);
> +                inc_proc_northd_run(&ctx, recompute);
> +                recompute = false;
>                  if (ctx.ovnsb_txn) {
>                      check_and_add_supported_dhcp_opts_to_sb_db(&ctx);
>                      check_and_add_supported_dhcpv6_opts_to_sb_db(&ctx);
>                      check_and_update_rbac(&ctx);
>                  }
> -            }
>
> -            ovsdb_idl_loop_commit_and_wait(&ovnnb_idl_loop);
> -            ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop);
> +            }
>          } else {
>              /* ovn-northd is paused
>               *    - we still want to handle any db updates and update the
> @@ -1008,6 +1054,19 @@ main(int argc, char *argv[])
>              ovsdb_idl_wait(ovnsb_idl_loop.idl);
>          }
>
> +        /* If there are any errors, we force a full recompute in order to
> +           ensure we handle any new tracked changes. */
> +        if (ovsdb_idl_loop_commit_and_wait(&ovnnb_idl_loop) != 1) {
> +            recompute = true;
> +        } else  {
> +            ovsdb_idl_track_clear(ovnnb_idl_loop.idl);
> +        }
> +        if (ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop) != 1) {
> +            recompute = true;
> +        } else {
> +            ovsdb_idl_track_clear(ovnsb_idl_loop.idl);
> +        }
> +
>          unixctl_server_run(unixctl);
>          unixctl_server_wait(unixctl);
>          memory_wait();
> @@ -1046,7 +1105,7 @@ main(int argc, char *argv[])
>          }
>          stopwatch_start(NORTHD_LOOP_STOPWATCH_NAME, time_msec());
>      }
> -
> +    inc_proc_northd_cleanup();
>
>      free(ovn_internal_version);
>      unixctl_server_destroy(unixctl);
> --
> 2.27.0
>

On 21/10/2021 07:30, Han Zhou wrote:
> On Mon, Oct 18, 2021 at 5:14 AM Mark Gray <mark.d.gray@redhat.com> wrote:
>>
>> Initial implementation adds a single node (northd). This single
>> node executes the northd processing pipeline but does not do so
>> incrementally.
>>
>> In order to develop incremental processing for northd, the code
>> will be organised with a .c/.h file for each I-P node following
>> the naming convention en-<node name>.c/.h. These files will
>> contain definition of the node data, the main node processing
>> functions and change handlers (if any). The purpose of these nodes
>> will be coordination of the nodes work and implemention of the
>> relevant interfaces to plugin to the I-P framework. The actual
>> work that will be executed by the node will be organised into
>> a companion file or files. Ideally this file will follow the
>> naming convention of the node: e.g. en-<node name>.c is
>> associated with <node name>.c.
>>
>> Initial node topology sees the northd node dependent on all DB
>> nodes. This will evolve over time.
>>
>> Co-authored-by: Numan Siddique <numans@ovn.org>
>> Signed-off-by: Numan Siddique <numans@ovn.org>
>> Signed-off-by: Mark Gray <mark.d.gray@redhat.com>
>> ---
>>  lib/inc-proc-eng.h       |  16 +++
>>  northd/automake.mk       |   4 +
>>  northd/en-northd.c       |  45 +++++++
>>  northd/en-northd.h       |  17 +++
>>  northd/inc-proc-northd.c | 254 +++++++++++++++++++++++++++++++++++++++
>>  northd/inc-proc-northd.h |  15 +++
>>  northd/northd.c          |  12 +-
>>  northd/northd.h          |   9 +-
>>  northd/ovn-northd.c      | 201 ++++++++++++++++++++-----------
>>  9 files changed, 490 insertions(+), 83 deletions(-)
>>  create mode 100644 northd/en-northd.c
>>  create mode 100644 northd/en-northd.h
>>  create mode 100644 northd/inc-proc-northd.c
>>  create mode 100644 northd/inc-proc-northd.h
>>
>> diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h
>> index 1ccae559dff6..a3f5a7e64287 100644
>> --- a/lib/inc-proc-eng.h
>> +++ b/lib/inc-proc-eng.h
>> @@ -63,15 +63,22 @@
>>  #define ENGINE_MAX_INPUT 256
>>  #define ENGINE_MAX_OVSDB_INDEX 256
>>
>> +#include <stdbool.h>
>> +#include <stdint.h>
>> +
>> +#include "compiler.h"
>> +
>>  struct engine_context {
>>      struct ovsdb_idl_txn *ovs_idl_txn;
>>      struct ovsdb_idl_txn *ovnsb_idl_txn;
>> +    struct ovsdb_idl_txn *ovnnb_idl_txn;
> 
> It would be better to add comments to clarify for the fields used by only
> one component but not the other.
> 
>>      void *client_ctx;
>>  };
>>
>>  /* Arguments to be passed to the engine at engine_init(). */
>>  struct engine_arg {
>>      struct ovsdb_idl *sb_idl;
>> +    struct ovsdb_idl *nb_idl;
>>      struct ovsdb_idl *ovs_idl;
>>  };
>>
>> @@ -347,6 +354,11 @@ static void en_##DB_NAME##_##TBL_NAME##_cleanup(void
> *data OVS_UNUSED) \
>>  #define ENGINE_FUNC_SB(TBL_NAME) \
>>      ENGINE_FUNC_OVSDB(sb, TBL_NAME)
>>
>> +/* Macro to define member functions of an engine node which represents
>> + * a table of OVN NB DB */
>> +#define ENGINE_FUNC_NB(TBL_NAME) \
>> +    ENGINE_FUNC_OVSDB(nb, TBL_NAME)
>> +
>>  /* Macro to define member functions of an engine node which represents
>>   * a table of open_vswitch DB */
>>  #define ENGINE_FUNC_OVS(TBL_NAME) \
>> @@ -360,6 +372,10 @@ static void en_##DB_NAME##_##TBL_NAME##_cleanup(void
> *data OVS_UNUSED) \
>>  #define ENGINE_NODE_SB(TBL_NAME, TBL_NAME_STR) \
>>      ENGINE_NODE_OVSDB(sb, "SB", TBL_NAME, TBL_NAME_STR);
>>
>> +/* Macro to define an engine node which represents a table of OVN NB DB
> */
>> +#define ENGINE_NODE_NB(TBL_NAME, TBL_NAME_STR) \
>> +    ENGINE_NODE_OVSDB(nb, "NB", TBL_NAME, TBL_NAME_STR);
>> +
>>  /* Macro to define an engine node which represents a table of
> open_vswitch
>>   * DB */
>>  #define ENGINE_NODE_OVS(TBL_NAME, TBL_NAME_STR) \
>> diff --git a/northd/automake.mk b/northd/automake.mk
>> index 35ad8c09d9ba..f0c1fb11c83a 100644
>> --- a/northd/automake.mk
>> +++ b/northd/automake.mk
>> @@ -4,6 +4,10 @@ northd_ovn_northd_SOURCES = \
>>         northd/northd.c \
>>         northd/northd.h \
>>         northd/ovn-northd.c \
>> +       northd/en-northd.c \
>> +       northd/en-northd.h \
>> +       northd/inc-proc-northd.c \
>> +       northd/inc-proc-northd.h \
>>         northd/ipam.c \
>>         northd/ipam.h
>>  northd_ovn_northd_LDADD = \
>> diff --git a/northd/en-northd.c b/northd/en-northd.c
>> new file mode 100644
>> index 000000000000..d310fa4dd31f
>> --- /dev/null
>> +++ b/northd/en-northd.c
>> @@ -0,0 +1,45 @@
>> +/*
>> + * 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 <getopt.h>
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +
>> +#include "en-northd.h"
>> +#include "lib/inc-proc-eng.h"
>> +#include "northd.h"
>> +#include "openvswitch/vlog.h"
>> +
>> +VLOG_DEFINE_THIS_MODULE(en_northd);
>> +
>> +void en_northd_run(struct engine_node *node, void *data OVS_UNUSED)
>> +{
>> +    const struct engine_context *eng_ctx = engine_get_context();
>> +    struct northd_context *ctx = eng_ctx->client_ctx;
> 

I should have something tomorrow that resolves your comments but I do
have a few questions for clarification.

> We should define a separate context structure for northd's engine
> client_ctx, which should only include necessary global vars for executing
> DB txn. Or maybe client_ctx is not needed for this first node. Basically,
> we shouldn't assign the existing northd_context to the I-P engine's
> client_ctx, because otherwise when there are more engine nodes, all the
> nodes will be able to access the data in the northd_context, and it would
> be a mess if a node carelessly accesses data that doesn't belong to its
> input.

I am going to remove the client context as I think it simplifies things.
However, I will need to add ovsdb_idl_loop for SB into the engine
context as this is needed by northd to update and read 'cur_cfg' and
'next_cfg' (update_northbound_cfg()). I can't think of a way around
this? What do you think?

> 
> For each single node, all its input should be retrieved within the _run()
> or _handler() functions through the engine APIs. So here for this single
> node, the input of ovn_db_run() should be constructed here instead of
> getting it from engine_context.
> 
> (After writing up the above comments I saw that in the patch "northd:
> Introduce struct northd_data" you did split the structure, but the above
> comment still applies. 1) the engine ctx shouldn't include
> ovnnb_idl/ovnsb_idl, 2) the NB/SB tables and index required by the node
> should belong to the input instead of the node's data itself.)
>

Yeah, this makes sense. It wasn't clear to me what should be included
where - but I think I have got it now. Also, I didn't know there was a
way to retrieve the index and tables from the OVSDB nodes (I didn't
notice when I was looking through the API).

What I plan to do is to get all these indexes and tables in the
en_northd.c file (via en_northd_run() on each iteration) and then assign
them to variables in 'struct northd_data' before running northd_run().
In this way, I maintain a dependency flow like:

inc-proc-northd.c -> en_northd.c -> northd.c

I don't want to include any I-P code in northd.c.

> Other than this (and the parts related to this), the rest of the patch
> looks good to me. I will spend some more time on the other patches of the
> series.
> 
> Thanks,
> Han
> 
>> +    ovn_db_run(ctx);
>> +
>> +    engine_set_node_state(node, EN_UPDATED);
>> +
>> +}
>> +void *en_northd_init(struct engine_node *node OVS_UNUSED,
>> +                     struct engine_arg *arg OVS_UNUSED)
>> +{
>> +    return NULL;
>> +}
>> +
>> +void en_northd_cleanup(void *data OVS_UNUSED)
>> +{
>> +}
>> diff --git a/northd/en-northd.h b/northd/en-northd.h
>> new file mode 100644
>> index 000000000000..0e7f76245e69
>> --- /dev/null
>> +++ b/northd/en-northd.h
>> @@ -0,0 +1,17 @@
>> +#ifndef EN_NORTHD_H
>> +#define EN_NORTHD_H 1
>> +
>> +#include <config.h>
>> +
>> +#include <getopt.h>
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +
>> +#include "lib/inc-proc-eng.h"
>> +
>> +void en_northd_run(struct engine_node *node OVS_UNUSED, void *data
> OVS_UNUSED);
>> +void *en_northd_init(struct engine_node *node OVS_UNUSED,
>> +                     struct engine_arg *arg);
>> +void en_northd_cleanup(void *data);
>> +
>> +#endif /* EN_NORTHD_H */
>> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
>> new file mode 100644
>> index 000000000000..85baeb07d3d9
>> --- /dev/null
>> +++ b/northd/inc-proc-northd.c
>> @@ -0,0 +1,254 @@
>> +/*
>> + * 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 <getopt.h>
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +
>> +#include "lib/inc-proc-eng.h"
>> +#include "lib/ovn-nb-idl.h"
>> +#include "lib/ovn-sb-idl.h"
>> +#include "openvswitch/poll-loop.h"
>> +#include "openvswitch/vlog.h"
>> +#include "inc-proc-northd.h"
>> +#include "en-northd.h"
>> +#include "util.h"
>> +
>> +VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
>> +
>> +#define NB_NODES \
>> +    NB_NODE(nb_global, "nb_global") \
>> +    NB_NODE(copp, "copp") \
>> +    NB_NODE(logical_switch, "logical_switch") \
>> +    NB_NODE(logical_switch_port, "logical_switch_port") \
>> +    NB_NODE(forwarding_group, "forwarding_group") \
>> +    NB_NODE(address_set, "address_set") \
>> +    NB_NODE(port_group, "port_group") \
>> +    NB_NODE(load_balancer, "load_balancer") \
>> +    NB_NODE(load_balancer_health_check, "load_balancer_health_check") \
>> +    NB_NODE(acl, "acl") \
>> +    NB_NODE(logical_router, "logical_router") \
>> +    NB_NODE(qos, "qos") \
>> +    NB_NODE(meter, "meter") \
>> +    NB_NODE(meter_band, "meter_band") \
>> +    NB_NODE(logical_router_port, "logical_router_port") \
>> +    NB_NODE(logical_router_static_route, "logical_router_static_route") \
>> +    NB_NODE(logical_router_policy, "logical_router_policy") \
>> +    NB_NODE(nat, "nat") \
>> +    NB_NODE(dhcp_options, "dhcp_options") \
>> +    NB_NODE(connection, "connection") \
>> +    NB_NODE(dns, "dns") \
>> +    NB_NODE(ssl, "ssl") \
>> +    NB_NODE(gateway_chassis, "gateway_chassis") \
>> +    NB_NODE(ha_chassis_group, "ha_chassis_group") \
>> +    NB_NODE(ha_chassis, "ha_chassis") \
>> +    NB_NODE(bfd, "bfd")
>> +
>> +    enum nb_engine_node {
>> +#define NB_NODE(NAME, NAME_STR) NB_##NAME,
>> +    NB_NODES
>> +#undef NB_NODE
>> +    };
>> +
>> +/* Define engine node functions for nodes that represent NB tables
>> + *
>> + * en_nb_<TABLE_NAME>_run()
>> + * en_nb_<TABLE_NAME>_init()
>> + * en_nb_<TABLE_NAME>_cleanup()
>> + */
>> +#define NB_NODE(NAME, NAME_STR) ENGINE_FUNC_NB(NAME);
>> +    NB_NODES
>> +#undef NB_NODE
>> +
>> +#define SB_NODES \
>> +    SB_NODE(sb_global, "sb_global") \
>> +    SB_NODE(chassis, "chassis") \
>> +    SB_NODE(chassis_private, "chassis_private") \
>> +    SB_NODE(encap, "encap") \
>> +    SB_NODE(address_set, "address_set") \
>> +    SB_NODE(port_group, "port_group") \
>> +    SB_NODE(logical_flow, "logical_flow") \
>> +    SB_NODE(logical_dp_group, "logical_DP_group") \
>> +    SB_NODE(multicast_group, "multicast_group") \
>> +    SB_NODE(meter, "meter") \
>> +    SB_NODE(meter_band, "meter_band") \
>> +    SB_NODE(datapath_binding, "datapath_binding") \
>> +    SB_NODE(port_binding, "port_binding") \
>> +    SB_NODE(mac_binding, "mac_binding") \
>> +    SB_NODE(dhcp_options, "dhcp_options") \
>> +    SB_NODE(dhcpv6_options, "dhcpv6_options") \
>> +    SB_NODE(connection, "connection") \
>> +    SB_NODE(ssl, "ssl") \
>> +    SB_NODE(dns, "dns") \
>> +    SB_NODE(rbac_role, "rbac_role") \
>> +    SB_NODE(rbac_permission, "rbac_permission") \
>> +    SB_NODE(gateway_chassis, "gateway_chassis") \
>> +    SB_NODE(ha_chassis, "ha_chassis") \
>> +    SB_NODE(ha_chassis_group, "ha_chassis_group") \
>> +    SB_NODE(controller_event, "controller_event") \
>> +    SB_NODE(ip_multicast, "ip_multicast") \
>> +    SB_NODE(igmp_group, "igmp_group") \
>> +    SB_NODE(service_monitor, "service_monitor") \
>> +    SB_NODE(load_balancer, "load_balancer") \
>> +    SB_NODE(bfd, "bfd") \
>> +    SB_NODE(fdb, "fdb")
>> +
>> +enum sb_engine_node {
>> +#define SB_NODE(NAME, NAME_STR) SB_##NAME,
>> +    SB_NODES
>> +#undef SB_NODE
>> +};
>> +
>> +/* Define engine node functions for nodes that represent SB tables
>> + *
>> + * en_sb_<TABLE_NAME>_run()
>> + * en_sb_<TABLE_NAME>_init()
>> + * en_sb_<TABLE_NAME>_cleanup()
>> + */
>> +#define SB_NODE(NAME, NAME_STR) ENGINE_FUNC_SB(NAME);
>> +    SB_NODES
>> +#undef SB_NODE
>> +
>> +/* Define engine nodes for NB and SB tables
>> + *
>> + * struct engine_node en_nb_<TABLE_NAME>
>> + * struct engine_node en_sb_<TABLE_NAME>
>> + *
>> + * Define nodes as static to avoid sparse errors.
>> + */
>> +#define NB_NODE(NAME, NAME_STR) static ENGINE_NODE_NB(NAME, NAME_STR);
>> +    NB_NODES
>> +#undef NB_NODE
>> +
>> +#define SB_NODE(NAME, NAME_STR) static ENGINE_NODE_SB(NAME, NAME_STR);
>> +    SB_NODES
>> +#undef SB_NODE
>> +
>> +/* Define engine nodes for other nodes. They should be defined as static
> to
>> + * avoid sparse errors. */
>> +static ENGINE_NODE(northd, "northd");
>> +
>> +void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>> +                          struct ovsdb_idl_loop *sb)
>> +{
>> +    /* Define relationships between nodes where first argument is
> dependent
>> +     * on the second argument */
>> +    engine_add_input(&en_northd, &en_nb_nb_global, NULL);
>> +    engine_add_input(&en_northd, &en_nb_copp, NULL);
>> +    engine_add_input(&en_northd, &en_nb_logical_switch, NULL);
>> +    engine_add_input(&en_northd, &en_nb_logical_switch_port, NULL);
>> +    engine_add_input(&en_northd, &en_nb_forwarding_group, NULL);
>> +    engine_add_input(&en_northd, &en_nb_address_set, NULL);
>> +    engine_add_input(&en_northd, &en_nb_port_group, NULL);
>> +    engine_add_input(&en_northd, &en_nb_load_balancer, NULL);
>> +    engine_add_input(&en_northd, &en_nb_load_balancer_health_check,
> NULL);
>> +    engine_add_input(&en_northd, &en_nb_acl, NULL);
>> +    engine_add_input(&en_northd, &en_nb_logical_router, NULL);
>> +    engine_add_input(&en_northd, &en_nb_qos, NULL);
>> +    engine_add_input(&en_northd, &en_nb_meter, NULL);
>> +    engine_add_input(&en_northd, &en_nb_meter_band, NULL);
>> +    engine_add_input(&en_northd, &en_nb_logical_router_port, NULL);
>> +    engine_add_input(&en_northd, &en_nb_logical_router_static_route,
> NULL);
>> +    engine_add_input(&en_northd, &en_nb_logical_router_policy, NULL);
>> +    engine_add_input(&en_northd, &en_nb_nat, NULL);
>> +    engine_add_input(&en_northd, &en_nb_dhcp_options, NULL);
>> +    engine_add_input(&en_northd, &en_nb_connection, NULL);
>> +    engine_add_input(&en_northd, &en_nb_dns, NULL);
>> +    engine_add_input(&en_northd, &en_nb_ssl, NULL);
>> +    engine_add_input(&en_northd, &en_nb_gateway_chassis, NULL);
>> +    engine_add_input(&en_northd, &en_nb_ha_chassis_group, NULL);
>> +    engine_add_input(&en_northd, &en_nb_ha_chassis, NULL);
>> +    engine_add_input(&en_northd, &en_nb_bfd, NULL);
>> +
>> +    engine_add_input(&en_northd, &en_sb_sb_global, NULL);
>> +    engine_add_input(&en_northd, &en_sb_chassis, NULL);
>> +    engine_add_input(&en_northd, &en_sb_chassis_private, NULL);
>> +    engine_add_input(&en_northd, &en_sb_encap, NULL);
>> +    engine_add_input(&en_northd, &en_sb_address_set, NULL);
>> +    engine_add_input(&en_northd, &en_sb_port_group, NULL);
>> +    engine_add_input(&en_northd, &en_sb_logical_flow, NULL);
>> +    engine_add_input(&en_northd, &en_sb_logical_dp_group, NULL);
>> +    engine_add_input(&en_northd, &en_sb_multicast_group, NULL);
>> +    engine_add_input(&en_northd, &en_sb_meter, NULL);
>> +    engine_add_input(&en_northd, &en_sb_meter_band, NULL);
>> +    engine_add_input(&en_northd, &en_sb_datapath_binding, NULL);
>> +    engine_add_input(&en_northd, &en_sb_port_binding, NULL);
>> +    engine_add_input(&en_northd, &en_sb_mac_binding, NULL);
>> +    engine_add_input(&en_northd, &en_sb_dhcp_options, NULL);
>> +    engine_add_input(&en_northd, &en_sb_dhcpv6_options, NULL);
>> +    engine_add_input(&en_northd, &en_sb_connection, NULL);
>> +    engine_add_input(&en_northd, &en_sb_ssl, NULL);
>> +    engine_add_input(&en_northd, &en_sb_dns, NULL);
>> +    engine_add_input(&en_northd, &en_sb_rbac_role, NULL);
>> +    engine_add_input(&en_northd, &en_sb_rbac_permission, NULL);
>> +    engine_add_input(&en_northd, &en_sb_gateway_chassis, NULL);
>> +    engine_add_input(&en_northd, &en_sb_ha_chassis, NULL);
>> +    engine_add_input(&en_northd, &en_sb_ha_chassis_group, NULL);
>> +    engine_add_input(&en_northd, &en_sb_controller_event, NULL);
>> +    engine_add_input(&en_northd, &en_sb_ip_multicast, NULL);
>> +    engine_add_input(&en_northd, &en_sb_igmp_group, NULL);
>> +    engine_add_input(&en_northd, &en_sb_service_monitor, NULL);
>> +    engine_add_input(&en_northd, &en_sb_load_balancer, NULL);
>> +    engine_add_input(&en_northd, &en_sb_bfd, NULL);
>> +    engine_add_input(&en_northd, &en_sb_fdb, NULL);
>> +
>> +    struct engine_arg engine_arg = {
>> +        .nb_idl = nb->idl,
>> +        .sb_idl = sb->idl,
>> +    };
>> +
>> +    engine_init(&en_northd, &engine_arg);
>> +}
>> +
>> +void inc_proc_northd_run(struct northd_context *ctx,
>> +                         bool recompute) {
>> +    engine_set_force_recompute(recompute);
>> +    engine_init_run();
>> +
>> +    struct engine_context eng_ctx = {
>> +        .ovnnb_idl_txn = ctx->ovnnb_txn,
>> +        .ovnsb_idl_txn = ctx->ovnsb_txn,
>> +        .client_ctx = ctx,
>> +    };
>> +
>> +    engine_set_context(&eng_ctx);
>> +
>> +    if (ctx->ovnnb_txn && ctx->ovnsb_txn) {
>> +        engine_run(true);
>> +    }
>> +
>> +    if (!engine_has_run()) {
>> +        if (engine_need_run()) {
>> +            VLOG_DBG("engine did not run, force recompute next time.");
>> +            engine_set_force_recompute(true);
>> +            poll_immediate_wake();
>> +        } else {
>> +            VLOG_DBG("engine did not run, and it was not needed");
>> +        }
>> +    } else if (engine_aborted()) {
>> +        VLOG_DBG("engine was aborted, force recompute next time.");
>> +        engine_set_force_recompute(true);
>> +        poll_immediate_wake();
>> +    } else {
>> +        engine_set_force_recompute(false);
>> +    }
>> +}
>> +
>> +void inc_proc_northd_cleanup(void)
>> +{
>> +    engine_cleanup();
>> +    engine_set_context(NULL);
>> +}
>> diff --git a/northd/inc-proc-northd.h b/northd/inc-proc-northd.h
>> new file mode 100644
>> index 000000000000..09cb8f3b3a80
>> --- /dev/null
>> +++ b/northd/inc-proc-northd.h
>> @@ -0,0 +1,15 @@
>> +#ifndef INC_PROC_NORTHD_H
>> +#define INC_PROC_NORTHD_H 1
>> +
>> +#include <config.h>
>> +
>> +#include "northd.h"
>> +#include "ovsdb-idl.h"
>> +
>> +void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>> +                          struct ovsdb_idl_loop *sb);
>> +void inc_proc_northd_run(struct northd_context *ctx,
>> +                         bool recompute);
>> +void inc_proc_northd_cleanup(void);
>> +
>> +#endif /* INC_PROC_NORTHD */
>> diff --git a/northd/northd.c b/northd/northd.c
>> index 32ab3baf3b9c..1321e26faa9d 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -14800,10 +14800,7 @@ ovnsb_db_run(struct northd_context *ctx,
>>  }
>>
>>  void
>> -ovn_db_run(struct northd_context *ctx,
>> -           struct ovsdb_idl_index *sbrec_chassis_by_name,
>> -           struct ovsdb_idl_loop *ovnsb_idl_loop,
>> -           const char *ovn_internal_version)
>> +ovn_db_run(struct northd_context *ctx)
>>  {
>>      struct hmap datapaths, ports;
>>      struct ovs_list lr_list;
>> @@ -14813,13 +14810,14 @@ ovn_db_run(struct northd_context *ctx,
>>      use_parallel_build = ctx->use_parallel_build;
>>
>>      int64_t start_time = time_wall_msec();
>> +
>>      stopwatch_start(OVNNB_DB_RUN_STOPWATCH_NAME, time_msec());
>> -    ovnnb_db_run(ctx, sbrec_chassis_by_name, ovnsb_idl_loop,
>> +    ovnnb_db_run(ctx, ctx->sbrec_chassis_by_name, ctx->ovnsb_idl_loop,
>>                   &datapaths, &ports, &lr_list, start_time,
>> -                 ovn_internal_version);
>> +                 ctx->ovn_internal_version);
>>      stopwatch_stop(OVNNB_DB_RUN_STOPWATCH_NAME, time_msec());
>>      stopwatch_start(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
>> -    ovnsb_db_run(ctx, ovnsb_idl_loop, &ports, start_time);
>> +    ovnsb_db_run(ctx, ctx->ovnsb_idl_loop, &ports, start_time);
>>      stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
>>      destroy_datapaths_and_ports(&datapaths, &ports, &lr_list);
>>  }
>> diff --git a/northd/northd.h b/northd/northd.h
>> index ffa2bbb4e88b..c0380ae60871 100644
>> --- a/northd/northd.h
>> +++ b/northd/northd.h
>> @@ -21,6 +21,8 @@ struct northd_context {
>>      const char *ovnsb_db;
>>      struct ovsdb_idl *ovnnb_idl;
>>      struct ovsdb_idl *ovnsb_idl;
>> +    struct ovsdb_idl_loop *ovnnb_idl_loop;
>> +    struct ovsdb_idl_loop *ovnsb_idl_loop;
>>      struct ovsdb_idl_txn *ovnnb_txn;
>>      struct ovsdb_idl_txn *ovnsb_txn;
>>      struct ovsdb_idl_index *sbrec_chassis_by_name;
>> @@ -28,13 +30,10 @@ struct northd_context {
>>      struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp;
>>      struct ovsdb_idl_index *sbrec_ip_mcast_by_dp;
>>
>> +    const char *ovn_internal_version;
>>      bool use_parallel_build;
>>  };
>>
>> -void
>> -ovn_db_run(struct northd_context *ctx,
>> -           struct ovsdb_idl_index *sbrec_chassis_by_name,
>> -           struct ovsdb_idl_loop *ovnsb_idl_loop,
>> -           const char *ovn_internal_version);
>> +void ovn_db_run(struct northd_context *ctx);
>>
>>  #endif /* NORTHD_H */
>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>> index 39aa960559dc..0c94afddb484 100644
>> --- a/northd/ovn-northd.c
>> +++ b/northd/ovn-northd.c
>> @@ -22,6 +22,7 @@
>>  #include "command-line.h"
>>  #include "daemon.h"
>>  #include "fatal-signal.h"
>> +#include "inc-proc-northd.h"
>>  #include "lib/ip-mcast-index.h"
>>  #include "lib/mcast-group-index.h"
>>  #include "memory.h"
>> @@ -439,6 +440,14 @@ check_and_add_supported_dhcpv6_opts_to_sb_db(struct
> northd_context *ctx)
>>  }
>>
>>  static void
>> +add_column_noalert(struct ovsdb_idl *idl,
>> +                   const struct ovsdb_idl_column *column)
>> +{
>> +    ovsdb_idl_add_column(idl, column);
>> +    ovsdb_idl_omit_alert(idl, column);
>> +}
>> +
>> +static void
>>  usage(void)
>>  {
>>      printf("\
>> @@ -560,14 +569,6 @@ parse_options(int argc OVS_UNUSED, char *argv[]
> OVS_UNUSED,
>>      free(short_options);
>>  }
>>
>> -static void
>> -add_column_noalert(struct ovsdb_idl *idl,
>> -                   const struct ovsdb_idl_column *column)
>> -{
>> -    ovsdb_idl_add_column(idl, column);
>> -    ovsdb_idl_omit_alert(idl, column);
>> -}
>> -
>>  static void
>>  update_ssl_config(void)
>>  {
>> @@ -645,6 +646,7 @@ main(int argc, char *argv[])
>>      /* We want to detect (almost) all changes to the ovn-nb db. */
>>      struct ovsdb_idl_loop ovnnb_idl_loop = OVSDB_IDL_LOOP_INITIALIZER(
>>          ovsdb_idl_create(ovnnb_db, &nbrec_idl_class, true, true));
>> +    ovsdb_idl_track_add_all(ovnnb_idl_loop.idl);
>>      ovsdb_idl_omit_alert(ovnnb_idl_loop.idl,
>>                           &nbrec_nb_global_col_nb_cfg_timestamp);
>>      ovsdb_idl_omit_alert(ovnnb_idl_loop.idl,
> &nbrec_nb_global_col_sb_cfg);
>> @@ -659,12 +661,13 @@ main(int argc, char *argv[])
>>
>>      /* We want to detect only selected changes to the ovn-sb db. */
>>      struct ovsdb_idl_loop ovnsb_idl_loop = OVSDB_IDL_LOOP_INITIALIZER(
>> -        ovsdb_idl_create(ovnsb_db, &sbrec_idl_class, false, true));
>> -
>> +        ovsdb_idl_create(ovnsb_db, &sbrec_idl_class, true, true));
>>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_sb_global);
>>      add_column_noalert(ovnsb_idl_loop.idl, &sbrec_sb_global_col_nb_cfg);
>>      add_column_noalert(ovnsb_idl_loop.idl, &sbrec_sb_global_col_options);
>>      add_column_noalert(ovnsb_idl_loop.idl, &sbrec_sb_global_col_ipsec);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_sb_global_col_connections);
>>
>>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_logical_flow);
>>      add_column_noalert(ovnsb_idl_loop.idl,
>> @@ -716,24 +719,26 @@ main(int argc, char *argv[])
>>      add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_mac);
>>      add_column_noalert(ovnsb_idl_loop.idl,
>>                         &sbrec_port_binding_col_nat_addresses);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> &sbrec_port_binding_col_chassis);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
>> -                         &sbrec_port_binding_col_gateway_chassis);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
>> -                         &sbrec_port_binding_col_ha_chassis_group);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
>> -                         &sbrec_port_binding_col_virtual_parent);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
>> -                         &sbrec_port_binding_col_up);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
>> -                         &sbrec_gateway_chassis_col_chassis);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> &sbrec_gateway_chassis_col_name);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
>> -                         &sbrec_gateway_chassis_col_priority);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
>> -                         &sbrec_gateway_chassis_col_external_ids);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
>> -                         &sbrec_gateway_chassis_col_options);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_port_binding_col_chassis);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_port_binding_col_gateway_chassis);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_port_binding_col_ha_chassis_group);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_port_binding_col_virtual_parent);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_port_binding_col_up);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_gateway_chassis_col_chassis);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_gateway_chassis_col_name);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_gateway_chassis_col_priority);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_gateway_chassis_col_external_ids);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_gateway_chassis_col_options);
>>      add_column_noalert(ovnsb_idl_loop.idl,
>>                         &sbrec_port_binding_col_external_ids);
>>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_mac_binding);
>> @@ -776,32 +781,35 @@ main(int argc, char *argv[])
>>      add_column_noalert(ovnsb_idl_loop.idl,
> &sbrec_rbac_permission_col_update);
>>
>>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_meter);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_meter_col_name);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_meter_col_unit);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_meter_col_bands);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> &sbrec_meter_col_name);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> &sbrec_meter_col_unit);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> &sbrec_meter_col_bands);
>>
>>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_meter_band);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> &sbrec_meter_band_col_action);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_meter_band_col_rate);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> &sbrec_meter_band_col_burst_size);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_meter_band_col_action);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> &sbrec_meter_band_col_rate);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_meter_band_col_burst_size);
>>
>>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_chassis);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_chassis_col_name);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> &sbrec_chassis_col_other_config);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_chassis_col_encaps);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> &sbrec_chassis_col_name);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_chassis_col_other_config);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> &sbrec_chassis_col_encaps);
>>
>>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_encap);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_encap_col_type);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> &sbrec_encap_col_type);
>>
>>      ovsdb_idl_add_table(ovnsb_idl_loop.idl,
> &sbrec_table_chassis_private);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
>> -                         &sbrec_chassis_private_col_name);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
>> -                         &sbrec_chassis_private_col_chassis);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
>> -                         &sbrec_chassis_private_col_nb_cfg);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
>> -                         &sbrec_chassis_private_col_nb_cfg_timestamp);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_chassis_private_col_name);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_chassis_private_col_chassis);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_chassis_private_col_nb_cfg);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +
> &sbrec_chassis_private_col_nb_cfg_timestamp);
>>
>>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_ha_chassis);
>>      add_column_noalert(ovnsb_idl_loop.idl,
>> @@ -822,10 +830,14 @@ main(int argc, char *argv[])
>>                         &sbrec_ha_chassis_group_col_ref_chassis);
>>
>>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_igmp_group);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> &sbrec_igmp_group_col_address);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> &sbrec_igmp_group_col_datapath);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> &sbrec_igmp_group_col_chassis);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> &sbrec_igmp_group_col_ports);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_igmp_group_col_address);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_igmp_group_col_datapath);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_igmp_group_col_chassis);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_igmp_group_col_ports);
>>
>>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_ip_multicast);
>>      add_column_noalert(ovnsb_idl_loop.idl,
>> @@ -857,8 +869,8 @@ main(int argc, char *argv[])
>>                         &sbrec_service_monitor_col_port);
>>      add_column_noalert(ovnsb_idl_loop.idl,
>>                         &sbrec_service_monitor_col_options);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
>> -                         &sbrec_service_monitor_col_status);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_service_monitor_col_status);
>>      add_column_noalert(ovnsb_idl_loop.idl,
>>                         &sbrec_service_monitor_col_protocol);
>>      add_column_noalert(ovnsb_idl_loop.idl,
>> @@ -878,19 +890,20 @@ main(int argc, char *argv[])
>>                         &sbrec_load_balancer_col_external_ids);
>>
>>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_bfd);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> &sbrec_bfd_col_logical_port);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_dst_ip);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_status);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_min_tx);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_min_rx);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_detect_mult);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_disc);
>> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_src_port);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
>> +                               &sbrec_bfd_col_logical_port);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> &sbrec_bfd_col_dst_ip);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> &sbrec_bfd_col_status);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> &sbrec_bfd_col_min_tx);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> &sbrec_bfd_col_min_rx);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> &sbrec_bfd_col_detect_mult);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_disc);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> &sbrec_bfd_col_src_port);
>>
>>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_fdb);
>> -    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_fdb_col_mac);
>> -    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_fdb_col_dp_key);
>> -    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_fdb_col_port_key);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl, &sbrec_fdb_col_mac);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> &sbrec_fdb_col_dp_key);
>> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> &sbrec_fdb_col_port_key);
>>
>>      struct ovsdb_idl_index *sbrec_chassis_by_name
>>          = chassis_index_create(ovnsb_idl_loop.idl);
>> @@ -922,9 +935,16 @@ main(int argc, char *argv[])
>>      stopwatch_create(LFLOWS_IGMP_STOPWATCH_NAME, SW_MS);
>>      stopwatch_create(LFLOWS_DP_GROUPS_STOPWATCH_NAME, SW_MS);
>>
>> +    /* Initialize incremental processing engine for ovn-northd */
>> +    inc_proc_northd_init(&ovnnb_idl_loop, &ovnsb_idl_loop);
>> +
>> +    unsigned int ovnnb_cond_seqno = UINT_MAX;
>> +    unsigned int ovnsb_cond_seqno = UINT_MAX;
>> +
>>      /* Main loop. */
>>      exiting = false;
>>
>> +    bool recompute = false;
>>      while (!exiting) {
>>          update_ssl_config();
>>          memory_run();
>> @@ -948,18 +968,46 @@ main(int argc, char *argv[])
>>                  ovsdb_idl_set_lock(ovnsb_idl_loop.idl, "ovn_northd");
>>              }
>>
>> +
>> +            struct ovsdb_idl_txn *ovnnb_txn =
>> +                        ovsdb_idl_loop_run(&ovnnb_idl_loop);
>> +            unsigned int new_ovnnb_cond_seqno =
>> +
>  ovsdb_idl_get_condition_seqno(ovnnb_idl_loop.idl);
>> +            if (new_ovnnb_cond_seqno != ovnnb_cond_seqno) {
>> +                if (!new_ovnnb_cond_seqno) {
>> +                    VLOG_INFO("OVN NB IDL reconnected, force
> recompute.");
>> +                    recompute = true;
>> +                }
>> +                ovnnb_cond_seqno = new_ovnnb_cond_seqno;
>> +            }
>> +
>> +            struct ovsdb_idl_txn *ovnsb_txn =
>> +                        ovsdb_idl_loop_run(&ovnsb_idl_loop);
>> +            unsigned int new_ovnsb_cond_seqno =
>> +
>  ovsdb_idl_get_condition_seqno(ovnsb_idl_loop.idl);
>> +            if (new_ovnsb_cond_seqno != ovnsb_cond_seqno) {
>> +                if (!new_ovnsb_cond_seqno) {
>> +                    VLOG_INFO("OVN SB IDL reconnected, force
> recompute.");
>> +                    recompute = true;
>> +                }
>> +                ovnsb_cond_seqno = new_ovnsb_cond_seqno;
>> +            }
>> +
>>              struct northd_context ctx = {
>>                  .ovnnb_db = ovnnb_db,
>>                  .ovnsb_db = ovnsb_db,
>>                  .ovnnb_idl = ovnnb_idl_loop.idl,
>> -                .ovnnb_txn = ovsdb_idl_loop_run(&ovnnb_idl_loop),
>> +                .ovnnb_idl_loop = &ovnnb_idl_loop,
>> +                .ovnnb_txn = ovnnb_txn,
>>                  .ovnsb_idl = ovnsb_idl_loop.idl,
>> -                .ovnsb_txn = ovsdb_idl_loop_run(&ovnsb_idl_loop),
>> +                .ovnsb_idl_loop = &ovnsb_idl_loop,
>> +                .ovnsb_txn = ovnsb_txn,
>>                  .sbrec_chassis_by_name = sbrec_chassis_by_name,
>>                  .sbrec_ha_chassis_grp_by_name =
> sbrec_ha_chassis_grp_by_name,
>>                  .sbrec_mcast_group_by_name_dp =
> sbrec_mcast_group_by_name_dp,
>>                  .sbrec_ip_mcast_by_dp = sbrec_ip_mcast_by_dp,
>>                  .use_parallel_build = use_parallel_build,
>> +                .ovn_internal_version = ovn_internal_version,
>>              };
>>
>>              if (!state.had_lock &&
> ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
>> @@ -975,17 +1023,15 @@ main(int argc, char *argv[])
>>              }
>>
>>              if (ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
>> -                ovn_db_run(&ctx, sbrec_chassis_by_name, &ovnsb_idl_loop,
>> -                           ovn_internal_version);
>> +                inc_proc_northd_run(&ctx, recompute);
>> +                recompute = false;
>>                  if (ctx.ovnsb_txn) {
>>                      check_and_add_supported_dhcp_opts_to_sb_db(&ctx);
>>                      check_and_add_supported_dhcpv6_opts_to_sb_db(&ctx);
>>                      check_and_update_rbac(&ctx);
>>                  }
>> -            }
>>
>> -            ovsdb_idl_loop_commit_and_wait(&ovnnb_idl_loop);
>> -            ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop);
>> +            }
>>          } else {
>>              /* ovn-northd is paused
>>               *    - we still want to handle any db updates and update the
>> @@ -1008,6 +1054,19 @@ main(int argc, char *argv[])
>>              ovsdb_idl_wait(ovnsb_idl_loop.idl);
>>          }
>>
>> +        /* If there are any errors, we force a full recompute in order to
>> +           ensure we handle any new tracked changes. */
>> +        if (ovsdb_idl_loop_commit_and_wait(&ovnnb_idl_loop) != 1) {
>> +            recompute = true;
>> +        } else  {
>> +            ovsdb_idl_track_clear(ovnnb_idl_loop.idl);
>> +        }
>> +        if (ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop) != 1) {
>> +            recompute = true;
>> +        } else {
>> +            ovsdb_idl_track_clear(ovnsb_idl_loop.idl);
>> +        }
>> +
>>          unixctl_server_run(unixctl);
>>          unixctl_server_wait(unixctl);
>>          memory_wait();
>> @@ -1046,7 +1105,7 @@ main(int argc, char *argv[])
>>          }
>>          stopwatch_start(NORTHD_LOOP_STOPWATCH_NAME, time_msec());
>>      }
>> -
>> +    inc_proc_northd_cleanup();
>>
>>      free(ovn_internal_version);
>>      unixctl_server_destroy(unixctl);
>> --
>> 2.27.0
>>
>

On Tue, Oct 26, 2021 at 12:16 PM Mark Gray <mark.d.gray@redhat.com> wrote:
>
> On 21/10/2021 07:30, Han Zhou wrote:
> > On Mon, Oct 18, 2021 at 5:14 AM Mark Gray <mark.d.gray@redhat.com>
wrote:
> >>
> >> Initial implementation adds a single node (northd). This single
> >> node executes the northd processing pipeline but does not do so
> >> incrementally.
> >>
> >> In order to develop incremental processing for northd, the code
> >> will be organised with a .c/.h file for each I-P node following
> >> the naming convention en-<node name>.c/.h. These files will
> >> contain definition of the node data, the main node processing
> >> functions and change handlers (if any). The purpose of these nodes
> >> will be coordination of the nodes work and implemention of the
> >> relevant interfaces to plugin to the I-P framework. The actual
> >> work that will be executed by the node will be organised into
> >> a companion file or files. Ideally this file will follow the
> >> naming convention of the node: e.g. en-<node name>.c is
> >> associated with <node name>.c.
> >>
> >> Initial node topology sees the northd node dependent on all DB
> >> nodes. This will evolve over time.
> >>
> >> Co-authored-by: Numan Siddique <numans@ovn.org>
> >> Signed-off-by: Numan Siddique <numans@ovn.org>
> >> Signed-off-by: Mark Gray <mark.d.gray@redhat.com>
> >> ---
> >>  lib/inc-proc-eng.h       |  16 +++
> >>  northd/automake.mk       |   4 +
> >>  northd/en-northd.c       |  45 +++++++
> >>  northd/en-northd.h       |  17 +++
> >>  northd/inc-proc-northd.c | 254 +++++++++++++++++++++++++++++++++++++++
> >>  northd/inc-proc-northd.h |  15 +++
> >>  northd/northd.c          |  12 +-
> >>  northd/northd.h          |   9 +-
> >>  northd/ovn-northd.c      | 201 ++++++++++++++++++++-----------
> >>  9 files changed, 490 insertions(+), 83 deletions(-)
> >>  create mode 100644 northd/en-northd.c
> >>  create mode 100644 northd/en-northd.h
> >>  create mode 100644 northd/inc-proc-northd.c
> >>  create mode 100644 northd/inc-proc-northd.h
> >>
> >> diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h
> >> index 1ccae559dff6..a3f5a7e64287 100644
> >> --- a/lib/inc-proc-eng.h
> >> +++ b/lib/inc-proc-eng.h
> >> @@ -63,15 +63,22 @@
> >>  #define ENGINE_MAX_INPUT 256
> >>  #define ENGINE_MAX_OVSDB_INDEX 256
> >>
> >> +#include <stdbool.h>
> >> +#include <stdint.h>
> >> +
> >> +#include "compiler.h"
> >> +
> >>  struct engine_context {
> >>      struct ovsdb_idl_txn *ovs_idl_txn;
> >>      struct ovsdb_idl_txn *ovnsb_idl_txn;
> >> +    struct ovsdb_idl_txn *ovnnb_idl_txn;
> >
> > It would be better to add comments to clarify for the fields used by
only
> > one component but not the other.
> >
> >>      void *client_ctx;
> >>  };
> >>
> >>  /* Arguments to be passed to the engine at engine_init(). */
> >>  struct engine_arg {
> >>      struct ovsdb_idl *sb_idl;
> >> +    struct ovsdb_idl *nb_idl;
> >>      struct ovsdb_idl *ovs_idl;
> >>  };
> >>
> >> @@ -347,6 +354,11 @@ static void
en_##DB_NAME##_##TBL_NAME##_cleanup(void
> > *data OVS_UNUSED) \
> >>  #define ENGINE_FUNC_SB(TBL_NAME) \
> >>      ENGINE_FUNC_OVSDB(sb, TBL_NAME)
> >>
> >> +/* Macro to define member functions of an engine node which represents
> >> + * a table of OVN NB DB */
> >> +#define ENGINE_FUNC_NB(TBL_NAME) \
> >> +    ENGINE_FUNC_OVSDB(nb, TBL_NAME)
> >> +
> >>  /* Macro to define member functions of an engine node which represents
> >>   * a table of open_vswitch DB */
> >>  #define ENGINE_FUNC_OVS(TBL_NAME) \
> >> @@ -360,6 +372,10 @@ static void
en_##DB_NAME##_##TBL_NAME##_cleanup(void
> > *data OVS_UNUSED) \
> >>  #define ENGINE_NODE_SB(TBL_NAME, TBL_NAME_STR) \
> >>      ENGINE_NODE_OVSDB(sb, "SB", TBL_NAME, TBL_NAME_STR);
> >>
> >> +/* Macro to define an engine node which represents a table of OVN NB
DB
> > */
> >> +#define ENGINE_NODE_NB(TBL_NAME, TBL_NAME_STR) \
> >> +    ENGINE_NODE_OVSDB(nb, "NB", TBL_NAME, TBL_NAME_STR);
> >> +
> >>  /* Macro to define an engine node which represents a table of
> > open_vswitch
> >>   * DB */
> >>  #define ENGINE_NODE_OVS(TBL_NAME, TBL_NAME_STR) \
> >> diff --git a/northd/automake.mk b/northd/automake.mk
> >> index 35ad8c09d9ba..f0c1fb11c83a 100644
> >> --- a/northd/automake.mk
> >> +++ b/northd/automake.mk
> >> @@ -4,6 +4,10 @@ northd_ovn_northd_SOURCES = \
> >>         northd/northd.c \
> >>         northd/northd.h \
> >>         northd/ovn-northd.c \
> >> +       northd/en-northd.c \
> >> +       northd/en-northd.h \
> >> +       northd/inc-proc-northd.c \
> >> +       northd/inc-proc-northd.h \
> >>         northd/ipam.c \
> >>         northd/ipam.h
> >>  northd_ovn_northd_LDADD = \
> >> diff --git a/northd/en-northd.c b/northd/en-northd.c
> >> new file mode 100644
> >> index 000000000000..d310fa4dd31f
> >> --- /dev/null
> >> +++ b/northd/en-northd.c
> >> @@ -0,0 +1,45 @@
> >> +/*
> >> + * 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 <getopt.h>
> >> +#include <stdlib.h>
> >> +#include <stdio.h>
> >> +
> >> +#include "en-northd.h"
> >> +#include "lib/inc-proc-eng.h"
> >> +#include "northd.h"
> >> +#include "openvswitch/vlog.h"
> >> +
> >> +VLOG_DEFINE_THIS_MODULE(en_northd);
> >> +
> >> +void en_northd_run(struct engine_node *node, void *data OVS_UNUSED)
> >> +{
> >> +    const struct engine_context *eng_ctx = engine_get_context();
> >> +    struct northd_context *ctx = eng_ctx->client_ctx;
> >
>
> I should have something tomorrow that resolves your comments but I do
> have a few questions for clarification.
>
> > We should define a separate context structure for northd's engine
> > client_ctx, which should only include necessary global vars for
executing
> > DB txn. Or maybe client_ctx is not needed for this first node.
Basically,
> > we shouldn't assign the existing northd_context to the I-P engine's
> > client_ctx, because otherwise when there are more engine nodes, all the
> > nodes will be able to access the data in the northd_context, and it
would
> > be a mess if a node carelessly accesses data that doesn't belong to its
> > input.
>
> I am going to remove the client context as I think it simplifies things.
> However, I will need to add ovsdb_idl_loop for SB into the engine
> context as this is needed by northd to update and read 'cur_cfg' and
> 'next_cfg' (update_northbound_cfg()). I can't think of a way around
> this? What do you think?
>
One way to avoid ovsdb_idl_loop in the engine context is not including
update_northbound_cfg() in the I-P engine. This part shouldn't have
performance pressure so doesn't need incremental processing.

> >
> > For each single node, all its input should be retrieved within the
_run()
> > or _handler() functions through the engine APIs. So here for this single
> > node, the input of ovn_db_run() should be constructed here instead of
> > getting it from engine_context.
> >
> > (After writing up the above comments I saw that in the patch "northd:
> > Introduce struct northd_data" you did split the structure, but the above
> > comment still applies. 1) the engine ctx shouldn't include
> > ovnnb_idl/ovnsb_idl, 2) the NB/SB tables and index required by the node
> > should belong to the input instead of the node's data itself.)
> >
>
> Yeah, this makes sense. It wasn't clear to me what should be included
> where - but I think I have got it now. Also, I didn't know there was a
> way to retrieve the index and tables from the OVSDB nodes (I didn't
> notice when I was looking through the API).
>
> What I plan to do is to get all these indexes and tables in the
> en_northd.c file (via en_northd_run() on each iteration) and then assign
> them to variables in 'struct northd_data' before running northd_run().
> In this way, I maintain a dependency flow like:
>
> inc-proc-northd.c -> en_northd.c -> northd.c
>
> I don't want to include any I-P code in northd.c.
>
I would suggest using a separate structure, such as "northd_input" (or a
better name) to include the index and tables. The data of en_northd should
only contain the data generated by the engine node (i.e. output data).

Thanks,
Han

> > Other than this (and the parts related to this), the rest of the patch
> > looks good to me. I will spend some more time on the other patches of
the
> > series.
> >
> > Thanks,
> > Han
> >
> >> +    ovn_db_run(ctx);
> >> +
> >> +    engine_set_node_state(node, EN_UPDATED);
> >> +
> >> +}
> >> +void *en_northd_init(struct engine_node *node OVS_UNUSED,
> >> +                     struct engine_arg *arg OVS_UNUSED)
> >> +{
> >> +    return NULL;
> >> +}
> >> +
> >> +void en_northd_cleanup(void *data OVS_UNUSED)
> >> +{
> >> +}
> >> diff --git a/northd/en-northd.h b/northd/en-northd.h
> >> new file mode 100644
> >> index 000000000000..0e7f76245e69
> >> --- /dev/null
> >> +++ b/northd/en-northd.h
> >> @@ -0,0 +1,17 @@
> >> +#ifndef EN_NORTHD_H
> >> +#define EN_NORTHD_H 1
> >> +
> >> +#include <config.h>
> >> +
> >> +#include <getopt.h>
> >> +#include <stdlib.h>
> >> +#include <stdio.h>
> >> +
> >> +#include "lib/inc-proc-eng.h"
> >> +
> >> +void en_northd_run(struct engine_node *node OVS_UNUSED, void *data
> > OVS_UNUSED);
> >> +void *en_northd_init(struct engine_node *node OVS_UNUSED,
> >> +                     struct engine_arg *arg);
> >> +void en_northd_cleanup(void *data);
> >> +
> >> +#endif /* EN_NORTHD_H */
> >> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> >> new file mode 100644
> >> index 000000000000..85baeb07d3d9
> >> --- /dev/null
> >> +++ b/northd/inc-proc-northd.c
> >> @@ -0,0 +1,254 @@
> >> +/*
> >> + * 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 <getopt.h>
> >> +#include <stdlib.h>
> >> +#include <stdio.h>
> >> +
> >> +#include "lib/inc-proc-eng.h"
> >> +#include "lib/ovn-nb-idl.h"
> >> +#include "lib/ovn-sb-idl.h"
> >> +#include "openvswitch/poll-loop.h"
> >> +#include "openvswitch/vlog.h"
> >> +#include "inc-proc-northd.h"
> >> +#include "en-northd.h"
> >> +#include "util.h"
> >> +
> >> +VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
> >> +
> >> +#define NB_NODES \
> >> +    NB_NODE(nb_global, "nb_global") \
> >> +    NB_NODE(copp, "copp") \
> >> +    NB_NODE(logical_switch, "logical_switch") \
> >> +    NB_NODE(logical_switch_port, "logical_switch_port") \
> >> +    NB_NODE(forwarding_group, "forwarding_group") \
> >> +    NB_NODE(address_set, "address_set") \
> >> +    NB_NODE(port_group, "port_group") \
> >> +    NB_NODE(load_balancer, "load_balancer") \
> >> +    NB_NODE(load_balancer_health_check, "load_balancer_health_check")
\
> >> +    NB_NODE(acl, "acl") \
> >> +    NB_NODE(logical_router, "logical_router") \
> >> +    NB_NODE(qos, "qos") \
> >> +    NB_NODE(meter, "meter") \
> >> +    NB_NODE(meter_band, "meter_band") \
> >> +    NB_NODE(logical_router_port, "logical_router_port") \
> >> +    NB_NODE(logical_router_static_route,
"logical_router_static_route") \
> >> +    NB_NODE(logical_router_policy, "logical_router_policy") \
> >> +    NB_NODE(nat, "nat") \
> >> +    NB_NODE(dhcp_options, "dhcp_options") \
> >> +    NB_NODE(connection, "connection") \
> >> +    NB_NODE(dns, "dns") \
> >> +    NB_NODE(ssl, "ssl") \
> >> +    NB_NODE(gateway_chassis, "gateway_chassis") \
> >> +    NB_NODE(ha_chassis_group, "ha_chassis_group") \
> >> +    NB_NODE(ha_chassis, "ha_chassis") \
> >> +    NB_NODE(bfd, "bfd")
> >> +
> >> +    enum nb_engine_node {
> >> +#define NB_NODE(NAME, NAME_STR) NB_##NAME,
> >> +    NB_NODES
> >> +#undef NB_NODE
> >> +    };
> >> +
> >> +/* Define engine node functions for nodes that represent NB tables
> >> + *
> >> + * en_nb_<TABLE_NAME>_run()
> >> + * en_nb_<TABLE_NAME>_init()
> >> + * en_nb_<TABLE_NAME>_cleanup()
> >> + */
> >> +#define NB_NODE(NAME, NAME_STR) ENGINE_FUNC_NB(NAME);
> >> +    NB_NODES
> >> +#undef NB_NODE
> >> +
> >> +#define SB_NODES \
> >> +    SB_NODE(sb_global, "sb_global") \
> >> +    SB_NODE(chassis, "chassis") \
> >> +    SB_NODE(chassis_private, "chassis_private") \
> >> +    SB_NODE(encap, "encap") \
> >> +    SB_NODE(address_set, "address_set") \
> >> +    SB_NODE(port_group, "port_group") \
> >> +    SB_NODE(logical_flow, "logical_flow") \
> >> +    SB_NODE(logical_dp_group, "logical_DP_group") \
> >> +    SB_NODE(multicast_group, "multicast_group") \
> >> +    SB_NODE(meter, "meter") \
> >> +    SB_NODE(meter_band, "meter_band") \
> >> +    SB_NODE(datapath_binding, "datapath_binding") \
> >> +    SB_NODE(port_binding, "port_binding") \
> >> +    SB_NODE(mac_binding, "mac_binding") \
> >> +    SB_NODE(dhcp_options, "dhcp_options") \
> >> +    SB_NODE(dhcpv6_options, "dhcpv6_options") \
> >> +    SB_NODE(connection, "connection") \
> >> +    SB_NODE(ssl, "ssl") \
> >> +    SB_NODE(dns, "dns") \
> >> +    SB_NODE(rbac_role, "rbac_role") \
> >> +    SB_NODE(rbac_permission, "rbac_permission") \
> >> +    SB_NODE(gateway_chassis, "gateway_chassis") \
> >> +    SB_NODE(ha_chassis, "ha_chassis") \
> >> +    SB_NODE(ha_chassis_group, "ha_chassis_group") \
> >> +    SB_NODE(controller_event, "controller_event") \
> >> +    SB_NODE(ip_multicast, "ip_multicast") \
> >> +    SB_NODE(igmp_group, "igmp_group") \
> >> +    SB_NODE(service_monitor, "service_monitor") \
> >> +    SB_NODE(load_balancer, "load_balancer") \
> >> +    SB_NODE(bfd, "bfd") \
> >> +    SB_NODE(fdb, "fdb")
> >> +
> >> +enum sb_engine_node {
> >> +#define SB_NODE(NAME, NAME_STR) SB_##NAME,
> >> +    SB_NODES
> >> +#undef SB_NODE
> >> +};
> >> +
> >> +/* Define engine node functions for nodes that represent SB tables
> >> + *
> >> + * en_sb_<TABLE_NAME>_run()
> >> + * en_sb_<TABLE_NAME>_init()
> >> + * en_sb_<TABLE_NAME>_cleanup()
> >> + */
> >> +#define SB_NODE(NAME, NAME_STR) ENGINE_FUNC_SB(NAME);
> >> +    SB_NODES
> >> +#undef SB_NODE
> >> +
> >> +/* Define engine nodes for NB and SB tables
> >> + *
> >> + * struct engine_node en_nb_<TABLE_NAME>
> >> + * struct engine_node en_sb_<TABLE_NAME>
> >> + *
> >> + * Define nodes as static to avoid sparse errors.
> >> + */
> >> +#define NB_NODE(NAME, NAME_STR) static ENGINE_NODE_NB(NAME, NAME_STR);
> >> +    NB_NODES
> >> +#undef NB_NODE
> >> +
> >> +#define SB_NODE(NAME, NAME_STR) static ENGINE_NODE_SB(NAME, NAME_STR);
> >> +    SB_NODES
> >> +#undef SB_NODE
> >> +
> >> +/* Define engine nodes for other nodes. They should be defined as
static
> > to
> >> + * avoid sparse errors. */
> >> +static ENGINE_NODE(northd, "northd");
> >> +
> >> +void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> >> +                          struct ovsdb_idl_loop *sb)
> >> +{
> >> +    /* Define relationships between nodes where first argument is
> > dependent
> >> +     * on the second argument */
> >> +    engine_add_input(&en_northd, &en_nb_nb_global, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_copp, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_logical_switch, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_logical_switch_port, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_forwarding_group, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_address_set, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_port_group, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_load_balancer, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_load_balancer_health_check,
> > NULL);
> >> +    engine_add_input(&en_northd, &en_nb_acl, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_logical_router, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_qos, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_meter, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_meter_band, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_logical_router_port, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_logical_router_static_route,
> > NULL);
> >> +    engine_add_input(&en_northd, &en_nb_logical_router_policy, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_nat, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_dhcp_options, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_connection, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_dns, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_ssl, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_gateway_chassis, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_ha_chassis_group, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_ha_chassis, NULL);
> >> +    engine_add_input(&en_northd, &en_nb_bfd, NULL);
> >> +
> >> +    engine_add_input(&en_northd, &en_sb_sb_global, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_chassis, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_chassis_private, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_encap, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_address_set, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_port_group, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_logical_flow, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_logical_dp_group, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_multicast_group, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_meter, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_meter_band, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_datapath_binding, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_port_binding, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_mac_binding, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_dhcp_options, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_dhcpv6_options, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_connection, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_ssl, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_dns, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_rbac_role, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_rbac_permission, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_gateway_chassis, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_ha_chassis, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_ha_chassis_group, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_controller_event, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_ip_multicast, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_igmp_group, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_service_monitor, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_load_balancer, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_bfd, NULL);
> >> +    engine_add_input(&en_northd, &en_sb_fdb, NULL);
> >> +
> >> +    struct engine_arg engine_arg = {
> >> +        .nb_idl = nb->idl,
> >> +        .sb_idl = sb->idl,
> >> +    };
> >> +
> >> +    engine_init(&en_northd, &engine_arg);
> >> +}
> >> +
> >> +void inc_proc_northd_run(struct northd_context *ctx,
> >> +                         bool recompute) {
> >> +    engine_set_force_recompute(recompute);
> >> +    engine_init_run();
> >> +
> >> +    struct engine_context eng_ctx = {
> >> +        .ovnnb_idl_txn = ctx->ovnnb_txn,
> >> +        .ovnsb_idl_txn = ctx->ovnsb_txn,
> >> +        .client_ctx = ctx,
> >> +    };
> >> +
> >> +    engine_set_context(&eng_ctx);
> >> +
> >> +    if (ctx->ovnnb_txn && ctx->ovnsb_txn) {
> >> +        engine_run(true);
> >> +    }
> >> +
> >> +    if (!engine_has_run()) {
> >> +        if (engine_need_run()) {
> >> +            VLOG_DBG("engine did not run, force recompute next
time.");
> >> +            engine_set_force_recompute(true);
> >> +            poll_immediate_wake();
> >> +        } else {
> >> +            VLOG_DBG("engine did not run, and it was not needed");
> >> +        }
> >> +    } else if (engine_aborted()) {
> >> +        VLOG_DBG("engine was aborted, force recompute next time.");
> >> +        engine_set_force_recompute(true);
> >> +        poll_immediate_wake();
> >> +    } else {
> >> +        engine_set_force_recompute(false);
> >> +    }
> >> +}
> >> +
> >> +void inc_proc_northd_cleanup(void)
> >> +{
> >> +    engine_cleanup();
> >> +    engine_set_context(NULL);
> >> +}
> >> diff --git a/northd/inc-proc-northd.h b/northd/inc-proc-northd.h
> >> new file mode 100644
> >> index 000000000000..09cb8f3b3a80
> >> --- /dev/null
> >> +++ b/northd/inc-proc-northd.h
> >> @@ -0,0 +1,15 @@
> >> +#ifndef INC_PROC_NORTHD_H
> >> +#define INC_PROC_NORTHD_H 1
> >> +
> >> +#include <config.h>
> >> +
> >> +#include "northd.h"
> >> +#include "ovsdb-idl.h"
> >> +
> >> +void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> >> +                          struct ovsdb_idl_loop *sb);
> >> +void inc_proc_northd_run(struct northd_context *ctx,
> >> +                         bool recompute);
> >> +void inc_proc_northd_cleanup(void);
> >> +
> >> +#endif /* INC_PROC_NORTHD */
> >> diff --git a/northd/northd.c b/northd/northd.c
> >> index 32ab3baf3b9c..1321e26faa9d 100644
> >> --- a/northd/northd.c
> >> +++ b/northd/northd.c
> >> @@ -14800,10 +14800,7 @@ ovnsb_db_run(struct northd_context *ctx,
> >>  }
> >>
> >>  void
> >> -ovn_db_run(struct northd_context *ctx,
> >> -           struct ovsdb_idl_index *sbrec_chassis_by_name,
> >> -           struct ovsdb_idl_loop *ovnsb_idl_loop,
> >> -           const char *ovn_internal_version)
> >> +ovn_db_run(struct northd_context *ctx)
> >>  {
> >>      struct hmap datapaths, ports;
> >>      struct ovs_list lr_list;
> >> @@ -14813,13 +14810,14 @@ ovn_db_run(struct northd_context *ctx,
> >>      use_parallel_build = ctx->use_parallel_build;
> >>
> >>      int64_t start_time = time_wall_msec();
> >> +
> >>      stopwatch_start(OVNNB_DB_RUN_STOPWATCH_NAME, time_msec());
> >> -    ovnnb_db_run(ctx, sbrec_chassis_by_name, ovnsb_idl_loop,
> >> +    ovnnb_db_run(ctx, ctx->sbrec_chassis_by_name, ctx->ovnsb_idl_loop,
> >>                   &datapaths, &ports, &lr_list, start_time,
> >> -                 ovn_internal_version);
> >> +                 ctx->ovn_internal_version);
> >>      stopwatch_stop(OVNNB_DB_RUN_STOPWATCH_NAME, time_msec());
> >>      stopwatch_start(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
> >> -    ovnsb_db_run(ctx, ovnsb_idl_loop, &ports, start_time);
> >> +    ovnsb_db_run(ctx, ctx->ovnsb_idl_loop, &ports, start_time);
> >>      stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
> >>      destroy_datapaths_and_ports(&datapaths, &ports, &lr_list);
> >>  }
> >> diff --git a/northd/northd.h b/northd/northd.h
> >> index ffa2bbb4e88b..c0380ae60871 100644
> >> --- a/northd/northd.h
> >> +++ b/northd/northd.h
> >> @@ -21,6 +21,8 @@ struct northd_context {
> >>      const char *ovnsb_db;
> >>      struct ovsdb_idl *ovnnb_idl;
> >>      struct ovsdb_idl *ovnsb_idl;
> >> +    struct ovsdb_idl_loop *ovnnb_idl_loop;
> >> +    struct ovsdb_idl_loop *ovnsb_idl_loop;
> >>      struct ovsdb_idl_txn *ovnnb_txn;
> >>      struct ovsdb_idl_txn *ovnsb_txn;
> >>      struct ovsdb_idl_index *sbrec_chassis_by_name;
> >> @@ -28,13 +30,10 @@ struct northd_context {
> >>      struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp;
> >>      struct ovsdb_idl_index *sbrec_ip_mcast_by_dp;
> >>
> >> +    const char *ovn_internal_version;
> >>      bool use_parallel_build;
> >>  };
> >>
> >> -void
> >> -ovn_db_run(struct northd_context *ctx,
> >> -           struct ovsdb_idl_index *sbrec_chassis_by_name,
> >> -           struct ovsdb_idl_loop *ovnsb_idl_loop,
> >> -           const char *ovn_internal_version);
> >> +void ovn_db_run(struct northd_context *ctx);
> >>
> >>  #endif /* NORTHD_H */
> >> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> >> index 39aa960559dc..0c94afddb484 100644
> >> --- a/northd/ovn-northd.c
> >> +++ b/northd/ovn-northd.c
> >> @@ -22,6 +22,7 @@
> >>  #include "command-line.h"
> >>  #include "daemon.h"
> >>  #include "fatal-signal.h"
> >> +#include "inc-proc-northd.h"
> >>  #include "lib/ip-mcast-index.h"
> >>  #include "lib/mcast-group-index.h"
> >>  #include "memory.h"
> >> @@ -439,6 +440,14 @@
check_and_add_supported_dhcpv6_opts_to_sb_db(struct
> > northd_context *ctx)
> >>  }
> >>
> >>  static void
> >> +add_column_noalert(struct ovsdb_idl *idl,
> >> +                   const struct ovsdb_idl_column *column)
> >> +{
> >> +    ovsdb_idl_add_column(idl, column);
> >> +    ovsdb_idl_omit_alert(idl, column);
> >> +}
> >> +
> >> +static void
> >>  usage(void)
> >>  {
> >>      printf("\
> >> @@ -560,14 +569,6 @@ parse_options(int argc OVS_UNUSED, char *argv[]
> > OVS_UNUSED,
> >>      free(short_options);
> >>  }
> >>
> >> -static void
> >> -add_column_noalert(struct ovsdb_idl *idl,
> >> -                   const struct ovsdb_idl_column *column)
> >> -{
> >> -    ovsdb_idl_add_column(idl, column);
> >> -    ovsdb_idl_omit_alert(idl, column);
> >> -}
> >> -
> >>  static void
> >>  update_ssl_config(void)
> >>  {
> >> @@ -645,6 +646,7 @@ main(int argc, char *argv[])
> >>      /* We want to detect (almost) all changes to the ovn-nb db. */
> >>      struct ovsdb_idl_loop ovnnb_idl_loop = OVSDB_IDL_LOOP_INITIALIZER(
> >>          ovsdb_idl_create(ovnnb_db, &nbrec_idl_class, true, true));
> >> +    ovsdb_idl_track_add_all(ovnnb_idl_loop.idl);
> >>      ovsdb_idl_omit_alert(ovnnb_idl_loop.idl,
> >>                           &nbrec_nb_global_col_nb_cfg_timestamp);
> >>      ovsdb_idl_omit_alert(ovnnb_idl_loop.idl,
> > &nbrec_nb_global_col_sb_cfg);
> >> @@ -659,12 +661,13 @@ main(int argc, char *argv[])
> >>
> >>      /* We want to detect only selected changes to the ovn-sb db. */
> >>      struct ovsdb_idl_loop ovnsb_idl_loop = OVSDB_IDL_LOOP_INITIALIZER(
> >> -        ovsdb_idl_create(ovnsb_db, &sbrec_idl_class, false, true));
> >> -
> >> +        ovsdb_idl_create(ovnsb_db, &sbrec_idl_class, true, true));
> >>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_sb_global);
> >>      add_column_noalert(ovnsb_idl_loop.idl,
&sbrec_sb_global_col_nb_cfg);
> >>      add_column_noalert(ovnsb_idl_loop.idl,
&sbrec_sb_global_col_options);
> >>      add_column_noalert(ovnsb_idl_loop.idl,
&sbrec_sb_global_col_ipsec);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_sb_global_col_connections);
> >>
> >>      ovsdb_idl_add_table(ovnsb_idl_loop.idl,
&sbrec_table_logical_flow);
> >>      add_column_noalert(ovnsb_idl_loop.idl,
> >> @@ -716,24 +719,26 @@ main(int argc, char *argv[])
> >>      add_column_noalert(ovnsb_idl_loop.idl,
&sbrec_port_binding_col_mac);
> >>      add_column_noalert(ovnsb_idl_loop.idl,
> >>                         &sbrec_port_binding_col_nat_addresses);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> > &sbrec_port_binding_col_chassis);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> >> -                         &sbrec_port_binding_col_gateway_chassis);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> >> -                         &sbrec_port_binding_col_ha_chassis_group);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> >> -                         &sbrec_port_binding_col_virtual_parent);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> >> -                         &sbrec_port_binding_col_up);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> >> -                         &sbrec_gateway_chassis_col_chassis);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> > &sbrec_gateway_chassis_col_name);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> >> -                         &sbrec_gateway_chassis_col_priority);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> >> -                         &sbrec_gateway_chassis_col_external_ids);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> >> -                         &sbrec_gateway_chassis_col_options);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_port_binding_col_chassis);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +
&sbrec_port_binding_col_gateway_chassis);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +
&sbrec_port_binding_col_ha_chassis_group);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +
&sbrec_port_binding_col_virtual_parent);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_port_binding_col_up);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_gateway_chassis_col_chassis);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_gateway_chassis_col_name);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_gateway_chassis_col_priority);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +
&sbrec_gateway_chassis_col_external_ids);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_gateway_chassis_col_options);
> >>      add_column_noalert(ovnsb_idl_loop.idl,
> >>                         &sbrec_port_binding_col_external_ids);
> >>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_mac_binding);
> >> @@ -776,32 +781,35 @@ main(int argc, char *argv[])
> >>      add_column_noalert(ovnsb_idl_loop.idl,
> > &sbrec_rbac_permission_col_update);
> >>
> >>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_meter);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_meter_col_name);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_meter_col_unit);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_meter_col_bands);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > &sbrec_meter_col_name);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > &sbrec_meter_col_unit);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > &sbrec_meter_col_bands);
> >>
> >>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_meter_band);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> > &sbrec_meter_band_col_action);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
&sbrec_meter_band_col_rate);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> > &sbrec_meter_band_col_burst_size);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_meter_band_col_action);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > &sbrec_meter_band_col_rate);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_meter_band_col_burst_size);
> >>
> >>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_chassis);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_chassis_col_name);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> > &sbrec_chassis_col_other_config);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
&sbrec_chassis_col_encaps);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > &sbrec_chassis_col_name);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_chassis_col_other_config);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > &sbrec_chassis_col_encaps);
> >>
> >>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_encap);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_encap_col_type);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > &sbrec_encap_col_type);
> >>
> >>      ovsdb_idl_add_table(ovnsb_idl_loop.idl,
> > &sbrec_table_chassis_private);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> >> -                         &sbrec_chassis_private_col_name);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> >> -                         &sbrec_chassis_private_col_chassis);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> >> -                         &sbrec_chassis_private_col_nb_cfg);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> >> -                         &sbrec_chassis_private_col_nb_cfg_timestamp);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_chassis_private_col_name);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_chassis_private_col_chassis);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_chassis_private_col_nb_cfg);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +
> > &sbrec_chassis_private_col_nb_cfg_timestamp);
> >>
> >>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_ha_chassis);
> >>      add_column_noalert(ovnsb_idl_loop.idl,
> >> @@ -822,10 +830,14 @@ main(int argc, char *argv[])
> >>                         &sbrec_ha_chassis_group_col_ref_chassis);
> >>
> >>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_igmp_group);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> > &sbrec_igmp_group_col_address);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> > &sbrec_igmp_group_col_datapath);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> > &sbrec_igmp_group_col_chassis);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> > &sbrec_igmp_group_col_ports);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_igmp_group_col_address);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_igmp_group_col_datapath);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_igmp_group_col_chassis);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_igmp_group_col_ports);
> >>
> >>      ovsdb_idl_add_table(ovnsb_idl_loop.idl,
&sbrec_table_ip_multicast);
> >>      add_column_noalert(ovnsb_idl_loop.idl,
> >> @@ -857,8 +869,8 @@ main(int argc, char *argv[])
> >>                         &sbrec_service_monitor_col_port);
> >>      add_column_noalert(ovnsb_idl_loop.idl,
> >>                         &sbrec_service_monitor_col_options);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> >> -                         &sbrec_service_monitor_col_status);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_service_monitor_col_status);
> >>      add_column_noalert(ovnsb_idl_loop.idl,
> >>                         &sbrec_service_monitor_col_protocol);
> >>      add_column_noalert(ovnsb_idl_loop.idl,
> >> @@ -878,19 +890,20 @@ main(int argc, char *argv[])
> >>                         &sbrec_load_balancer_col_external_ids);
> >>
> >>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_bfd);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> > &sbrec_bfd_col_logical_port);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_dst_ip);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_status);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_min_tx);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_min_rx);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
&sbrec_bfd_col_detect_mult);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_disc);
> >> -    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_bfd_col_src_port);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> >> +                               &sbrec_bfd_col_logical_port);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > &sbrec_bfd_col_dst_ip);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > &sbrec_bfd_col_status);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > &sbrec_bfd_col_min_tx);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > &sbrec_bfd_col_min_rx);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > &sbrec_bfd_col_detect_mult);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
&sbrec_bfd_col_disc);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > &sbrec_bfd_col_src_port);
> >>
> >>      ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_fdb);
> >> -    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_fdb_col_mac);
> >> -    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_fdb_col_dp_key);
> >> -    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_fdb_col_port_key);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
&sbrec_fdb_col_mac);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > &sbrec_fdb_col_dp_key);
> >> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > &sbrec_fdb_col_port_key);
> >>
> >>      struct ovsdb_idl_index *sbrec_chassis_by_name
> >>          = chassis_index_create(ovnsb_idl_loop.idl);
> >> @@ -922,9 +935,16 @@ main(int argc, char *argv[])
> >>      stopwatch_create(LFLOWS_IGMP_STOPWATCH_NAME, SW_MS);
> >>      stopwatch_create(LFLOWS_DP_GROUPS_STOPWATCH_NAME, SW_MS);
> >>
> >> +    /* Initialize incremental processing engine for ovn-northd */
> >> +    inc_proc_northd_init(&ovnnb_idl_loop, &ovnsb_idl_loop);
> >> +
> >> +    unsigned int ovnnb_cond_seqno = UINT_MAX;
> >> +    unsigned int ovnsb_cond_seqno = UINT_MAX;
> >> +
> >>      /* Main loop. */
> >>      exiting = false;
> >>
> >> +    bool recompute = false;
> >>      while (!exiting) {
> >>          update_ssl_config();
> >>          memory_run();
> >> @@ -948,18 +968,46 @@ main(int argc, char *argv[])
> >>                  ovsdb_idl_set_lock(ovnsb_idl_loop.idl, "ovn_northd");
> >>              }
> >>
> >> +
> >> +            struct ovsdb_idl_txn *ovnnb_txn =
> >> +                        ovsdb_idl_loop_run(&ovnnb_idl_loop);
> >> +            unsigned int new_ovnnb_cond_seqno =
> >> +
> >  ovsdb_idl_get_condition_seqno(ovnnb_idl_loop.idl);
> >> +            if (new_ovnnb_cond_seqno != ovnnb_cond_seqno) {
> >> +                if (!new_ovnnb_cond_seqno) {
> >> +                    VLOG_INFO("OVN NB IDL reconnected, force
> > recompute.");
> >> +                    recompute = true;
> >> +                }
> >> +                ovnnb_cond_seqno = new_ovnnb_cond_seqno;
> >> +            }
> >> +
> >> +            struct ovsdb_idl_txn *ovnsb_txn =
> >> +                        ovsdb_idl_loop_run(&ovnsb_idl_loop);
> >> +            unsigned int new_ovnsb_cond_seqno =
> >> +
> >  ovsdb_idl_get_condition_seqno(ovnsb_idl_loop.idl);
> >> +            if (new_ovnsb_cond_seqno != ovnsb_cond_seqno) {
> >> +                if (!new_ovnsb_cond_seqno) {
> >> +                    VLOG_INFO("OVN SB IDL reconnected, force
> > recompute.");
> >> +                    recompute = true;
> >> +                }
> >> +                ovnsb_cond_seqno = new_ovnsb_cond_seqno;
> >> +            }
> >> +
> >>              struct northd_context ctx = {
> >>                  .ovnnb_db = ovnnb_db,
> >>                  .ovnsb_db = ovnsb_db,
> >>                  .ovnnb_idl = ovnnb_idl_loop.idl,
> >> -                .ovnnb_txn = ovsdb_idl_loop_run(&ovnnb_idl_loop),
> >> +                .ovnnb_idl_loop = &ovnnb_idl_loop,
> >> +                .ovnnb_txn = ovnnb_txn,
> >>                  .ovnsb_idl = ovnsb_idl_loop.idl,
> >> -                .ovnsb_txn = ovsdb_idl_loop_run(&ovnsb_idl_loop),
> >> +                .ovnsb_idl_loop = &ovnsb_idl_loop,
> >> +                .ovnsb_txn = ovnsb_txn,
> >>                  .sbrec_chassis_by_name = sbrec_chassis_by_name,
> >>                  .sbrec_ha_chassis_grp_by_name =
> > sbrec_ha_chassis_grp_by_name,
> >>                  .sbrec_mcast_group_by_name_dp =
> > sbrec_mcast_group_by_name_dp,
> >>                  .sbrec_ip_mcast_by_dp = sbrec_ip_mcast_by_dp,
> >>                  .use_parallel_build = use_parallel_build,
> >> +                .ovn_internal_version = ovn_internal_version,
> >>              };
> >>
> >>              if (!state.had_lock &&
> > ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
> >> @@ -975,17 +1023,15 @@ main(int argc, char *argv[])
> >>              }
> >>
> >>              if (ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
> >> -                ovn_db_run(&ctx, sbrec_chassis_by_name,
&ovnsb_idl_loop,
> >> -                           ovn_internal_version);
> >> +                inc_proc_northd_run(&ctx, recompute);
> >> +                recompute = false;
> >>                  if (ctx.ovnsb_txn) {
> >>                      check_and_add_supported_dhcp_opts_to_sb_db(&ctx);
> >>
 check_and_add_supported_dhcpv6_opts_to_sb_db(&ctx);
> >>                      check_and_update_rbac(&ctx);
> >>                  }
> >> -            }
> >>
> >> -            ovsdb_idl_loop_commit_and_wait(&ovnnb_idl_loop);
> >> -            ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop);
> >> +            }
> >>          } else {
> >>              /* ovn-northd is paused
> >>               *    - we still want to handle any db updates and update
the
> >> @@ -1008,6 +1054,19 @@ main(int argc, char *argv[])
> >>              ovsdb_idl_wait(ovnsb_idl_loop.idl);
> >>          }
> >>
> >> +        /* If there are any errors, we force a full recompute in
order to
> >> +           ensure we handle any new tracked changes. */
> >> +        if (ovsdb_idl_loop_commit_and_wait(&ovnnb_idl_loop) != 1) {
> >> +            recompute = true;
> >> +        } else  {
> >> +            ovsdb_idl_track_clear(ovnnb_idl_loop.idl);
> >> +        }
> >> +        if (ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop) != 1) {
> >> +            recompute = true;
> >> +        } else {
> >> +            ovsdb_idl_track_clear(ovnsb_idl_loop.idl);
> >> +        }
> >> +
> >>          unixctl_server_run(unixctl);
> >>          unixctl_server_wait(unixctl);
> >>          memory_wait();
> >> @@ -1046,7 +1105,7 @@ main(int argc, char *argv[])
> >>          }
> >>          stopwatch_start(NORTHD_LOOP_STOPWATCH_NAME, time_msec());
> >>      }
> >> -
> >> +    inc_proc_northd_cleanup();
> >>
> >>      free(ovn_internal_version);
> >>      unixctl_server_destroy(unixctl);
> >> --
> >> 2.27.0
> >>
> >
>

On 27/10/2021 07:00, Han Zhou wrote:
> I would suggest using a separate structure, such as "northd_input" (or a
> better name) to include the index and tables. The data of en_northd should
> only contain the data generated by the engine node (i.e. output data).
> 
> Thanks,
> Han

Thanks for all your suggestions. I took the onboard. New series at:
https://patchwork.ozlabs.org/project/ovn/list/?series=269213