Thanks Paulo, I have comments inline below.
On Mon, Feb 9, 2026 at 1:29 PM Paulo Guilherme Silva via dev
<[email protected]> wrote:
>
> This new engine now maintains the enum_datapath related data for ovn-ic
> daemon which was earlier maintained by the ic engine node invoked the
> enumerate_datapaths() function. The inputs to this engine node are
> en_icnb_transit_switch;
> en_icsb_datapath_binding';
> In order to achieve this, we refactor in the following way:
> * Introduce enum_datapaths_init() which initializes this data.
> * Introduce enum_datapaths_destroy() which clears this data for a
> new iteration.
> * Introduce enum_datapaths_run() which invokes the full recompute
> of the engine.
>
> This engine node becomes an input to 'ic' node.
>
> Signed-off-by: Paulo Guilherme Silva <[email protected]>
> ---
> ic/automake.mk | 2 +
> ic/en-enum-datapaths.c | 141 +++++++++++++++++++++++++++++++++++++++++
> ic/en-enum-datapaths.h | 30 +++++++++
> ic/en-ic.c | 29 +++++----
> ic/inc-proc-ic.c | 6 ++
> ic/ovn-ic.c | 82 ++++++++++++------------
> ic/ovn-ic.h | 8 +--
> lib/stopwatch-names.h | 1 +
> 8 files changed, 244 insertions(+), 55 deletions(-)
> create mode 100644 ic/en-enum-datapaths.c
> create mode 100644 ic/en-enum-datapaths.h
>
> diff --git a/ic/automake.mk b/ic/automake.mk
> index a69b1030d..2766483b7 100644
> --- a/ic/automake.mk
> +++ b/ic/automake.mk
> @@ -4,6 +4,8 @@ ic_ovn_ic_SOURCES = ic/ovn-ic.c \
> ic/ovn-ic.h \
> ic/en-ic.c \
> ic/en-ic.h \
> + ic/en-enum-datapaths.c \
> + ic/en-enum-datapaths.h \
> ic/inc-proc-ic.c \
> ic/inc-proc-ic.h
> ic_ovn_ic_LDADD = \
> diff --git a/ic/en-enum-datapaths.c b/ic/en-enum-datapaths.c
> new file mode 100644
> index 000000000..7515c0b85
> --- /dev/null
> +++ b/ic/en-enum-datapaths.c
> @@ -0,0 +1,141 @@
> +/*
> + * 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>
> +
> +/* OVS includes. */
> +#include "openvswitch/vlog.h"
> +#include "openvswitch/shash.h"
> +#include "openvswitch/hmap.h"
> +
> +/* OVN includes. */
> +#include "ovn-ic.h"
> +#include "en-enum-datapaths.h"
> +#include "inc-proc-ic.h"
> +#include "lib/inc-proc-eng.h"
> +#include "lib/ovn-ic-sb-idl.h"
> +#include "lib/ovn-util.h"
> +#include "lib/stopwatch-names.h"
> +#include "coverage.h"
> +#include "stopwatch.h"
> +#include "stopwatch-names.h"
> +
> +VLOG_DEFINE_THIS_MODULE(en_enum_datapaths);
> +COVERAGE_DEFINE(enum_datapaths_run);
> +
> +static void
> +enum_datapath_run(const struct icsbrec_datapath_binding_table *dp_table,
> + struct ed_type_enum_datapaths *dp_data);
> +static enum ic_datapath_type
> + ic_dp_get_type(const struct icsbrec_datapath_binding *isb_dp);
> +static void enum_datapaths_init(struct ed_type_enum_datapaths *data);
> +static void enum_datapaths_destroy(struct ed_type_enum_datapaths *data);
> +static void enum_datapaths_clear(struct ed_type_enum_datapaths *data);
> +
> +void *
> +en_enum_datapaths_init(struct engine_node *node OVS_UNUSED,
> + struct engine_arg *arg OVS_UNUSED)
> +{
> + struct ed_type_enum_datapaths *data = xzalloc(sizeof *data);
> + enum_datapaths_init(data);
> + return data;
> +}
> +
> +void
> +en_enum_datapaths_cleanup(void *data)
> +{
> + enum_datapaths_destroy(data);
> +}
> +
> +enum engine_node_state
> +en_enum_datapaths_run(struct engine_node *node, void *data)
> +{
> + struct ed_type_enum_datapaths *dp_data = data;
> +
> + enum_datapaths_clear(dp_data);
> +
> + const struct icsbrec_datapath_binding_table *dp_table =
> + EN_OVSDB_GET(engine_get_input("ICSB_datapath_binding", node));
> +
> + COVERAGE_INC(enum_datapaths_run);
> + stopwatch_start(OVN_IC_ENUM_DATAPATHS_RUN_STOPWATCH_NAME, time_msec());
> + enum_datapath_run(dp_table, dp_data);
> + stopwatch_stop(OVN_IC_ENUM_DATAPATHS_RUN_STOPWATCH_NAME, time_msec());
> +
> + return EN_UPDATED;
> +}
> +
> +static void
> +enum_datapath_run(const struct icsbrec_datapath_binding_table *dp_table,
> + struct ed_type_enum_datapaths *dp_data)
> +{
> + const struct icsbrec_datapath_binding *isb_dp;
> + ICSBREC_DATAPATH_BINDING_TABLE_FOR_EACH (isb_dp, dp_table) {
> + /* 1. Adiciona Tunnel ID */
> + ovn_add_tnlid(&dp_data->dp_tnlids, isb_dp->tunnel_key);
> +
> + /* 2. Classifica o Datapath */
It appears some Portuguese has entered the code here :)
> + enum ic_datapath_type dp_type = ic_dp_get_type(isb_dp);
> + if (dp_type == IC_ROUTER) {
> + char *uuid_str = uuid_to_string(isb_dp->nb_ic_uuid);
> + shash_add(&dp_data->isb_tr_dps, uuid_str, (void *) isb_dp);
> + free(uuid_str);
> + } else {
> + shash_add(&dp_data->isb_ts_dps, isb_dp->transit_switch,
> + (void *) isb_dp);
> + }
> + }
> +}
> +
> +static enum ic_datapath_type
> +ic_dp_get_type(const struct icsbrec_datapath_binding *isb_dp)
> +{
> + if (isb_dp->type && !strcmp(isb_dp->type, "transit-router")) {
> + return IC_ROUTER;
> + }
> +
> + return IC_SWITCH;
> +}
> +
> +static void
> +enum_datapaths_init(struct ed_type_enum_datapaths *data)
> +{
> + hmap_init(&data->dp_tnlids);
> + shash_init(&data->isb_ts_dps);
> + shash_init(&data->isb_tr_dps);
> +}
> +
> +static void
> +enum_datapaths_destroy(struct ed_type_enum_datapaths *data)
> +{
> + enum_datapaths_clear(data);
There is no need to call enum_datapaths_clear() here. There is a
similar pattern employed in patches 3, 4, and 5 as well.
> + ovn_destroy_tnlids(&data->dp_tnlids);
> +
> + shash_destroy(&data->isb_ts_dps);
> + shash_destroy(&data->isb_tr_dps);
> +}
> +
> +static void
> +enum_datapaths_clear(struct ed_type_enum_datapaths *data)
> +{
> + ovn_destroy_tnlids(&data->dp_tnlids);
> + hmap_init(&data->dp_tnlids);
> +
> + shash_clear(&data->isb_ts_dps);
> + shash_clear(&data->isb_tr_dps);
> +}
> diff --git a/ic/en-enum-datapaths.h b/ic/en-enum-datapaths.h
> new file mode 100644
> index 000000000..0aff9fc89
> --- /dev/null
> +++ b/ic/en-enum-datapaths.h
> @@ -0,0 +1,30 @@
> +#ifndef EN_IC_ENUM_DATAPATHS_H
> +#define EN_IC_ENUM_DATAPATHS_H 1
> +
> +#include <config.h>
> +
> +#include <stdbool.h>
> +#include <getopt.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +/* OVS includes. */
> +#include "lib/hmapx.h"
> +#include "openvswitch/hmap.h"
> +#include "openvswitch/shash.h"
> +
> +/* OVN includes. */
> +#include "lib/inc-proc-eng.h"
> +
> +/* struct which maintains the data of the engine node enumerate datapaths. */
> +struct ed_type_enum_datapaths {
> + struct hmap dp_tnlids;
> + struct shash isb_ts_dps;
> + struct shash isb_tr_dps;
> +};
> +
> +void *en_enum_datapaths_init(struct engine_node *, struct engine_arg *);
> +enum engine_node_state en_enum_datapaths_run(struct engine_node *, void
> *data);
> +void en_enum_datapaths_cleanup(void *data);
> +
> +#endif
> \ No newline at end of file
> diff --git a/ic/en-ic.c b/ic/en-ic.c
> index ce7d5de76..16b0e12bd 100644
> --- a/ic/en-ic.c
> +++ b/ic/en-ic.c
> @@ -24,6 +24,8 @@
> /* OVN includes. */
> #include "ovn-ic.h"
> #include "en-ic.h"
> +#include "en-enum-datapaths.h"
> +#include "lib/ovn-ic-sb-idl.h"
> #include "lib/inc-proc-eng.h"
> #include "lib/ovn-util.h"
> #include "lib/stopwatch-names.h"
> @@ -152,18 +154,26 @@ enum engine_node_state
> en_ic_run(struct engine_node *node, void *data)
> {
> const struct engine_context *eng_ctx = engine_get_context();
> -
> + struct ic_data *ic_data = data;
> struct ic_input input_data;
>
> - ic_destroy(data);
> - ic_init(data);
> + struct ed_type_enum_datapaths *dp_node_data =
> + engine_get_input_data("enum_datapaths", node);
> +
> + if (!dp_node_data) {
> + return EN_UNCHANGED;
> + }
The NULL check above is unnecessary. The enum_datapaths node always
returns EN_UPDATED as its status, so the dp_node_data will always be
non-NULL.
> +
> + ic_data->dp_tnlids = &dp_node_data->dp_tnlids;
> + ic_data->isb_ts_dps = &dp_node_data->isb_ts_dps;
> + ic_data->isb_tr_dps = &dp_node_data->isb_tr_dps;
>
> ic_get_input_data(node, &input_data);
> input_data.runned_az = eng_ctx->client_ctx;
>
> COVERAGE_INC(ic_run);
> stopwatch_start(IC_OVN_DB_RUN_STOPWATCH_NAME, time_msec());
> - ovn_db_run(&input_data, data, (struct engine_context *) eng_ctx);
> + ovn_db_run(&input_data, ic_data, (struct engine_context *) eng_ctx);
> stopwatch_stop(IC_OVN_DB_RUN_STOPWATCH_NAME, time_msec());
> return EN_UPDATED;
> }
> @@ -186,17 +196,14 @@ en_ic_cleanup(void *data)
> }
>
> void
> -ic_destroy(struct ic_data *data)
> +ic_destroy(struct ic_data *data OVS_UNUSED)
> {
> - ovn_destroy_tnlids(&data->dp_tnlids);
> - shash_destroy(&data->isb_ts_dps);
> - shash_destroy(&data->isb_tr_dps);
> }
>
> void
> ic_init(struct ic_data *data)
> {
> - hmap_init(&data->dp_tnlids);
> - shash_init(&data->isb_ts_dps);
> - shash_init(&data->isb_tr_dps);
> + data->dp_tnlids = NULL;
> + data->isb_ts_dps = NULL;
> + data->isb_tr_dps = NULL;
> }
> diff --git a/ic/inc-proc-ic.c b/ic/inc-proc-ic.c
> index 0160c3a3e..d7d247e7a 100644
> --- a/ic/inc-proc-ic.c
> +++ b/ic/inc-proc-ic.c
> @@ -27,6 +27,7 @@
> #include "openvswitch/vlog.h"
> #include "inc-proc-ic.h"
> #include "en-ic.h"
> +#include "en-enum-datapaths.h"
> #include "unixctl.h"
> #include "util.h"
>
> @@ -158,6 +159,7 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_ic);
> /* Define engine nodes for other nodes. They should be defined as static to
> * avoid sparse errors. */
> static ENGINE_NODE(ic, SB_WRITE);
> +static ENGINE_NODE(enum_datapaths);
>
> void
> inc_proc_ic_init(struct ovsdb_idl_loop *nb,
> @@ -167,6 +169,10 @@ inc_proc_ic_init(struct ovsdb_idl_loop *nb,
> {
> /* Define relationships between nodes where first argument is dependent
> * on the second argument */
> + engine_add_input(&en_enum_datapaths, &en_icnb_transit_switch, NULL);
> + engine_add_input(&en_enum_datapaths, &en_icsb_datapath_binding, NULL);
> +
> + engine_add_input(&en_ic, &en_enum_datapaths, NULL);
> engine_add_input(&en_ic, &en_nb_nb_global, NULL);
> engine_add_input(&en_ic, &en_nb_logical_router_static_route, NULL);
> engine_add_input(&en_ic, &en_nb_logical_router, NULL);
> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> index 8327054de..724201616 100644
> --- a/ic/ovn-ic.c
> +++ b/ic/ovn-ic.c
> @@ -172,16 +172,6 @@ allocate_dp_key(struct hmap *dp_tnlids, bool vxlan_mode,
> const char *name)
> &hint);
> }
>
> -static enum ic_datapath_type
> -ic_dp_get_type(const struct icsbrec_datapath_binding *isb_dp)
> -{
> - if (isb_dp->type && !strcmp(isb_dp->type, "transit-router")) {
> - return IC_ROUTER;
> - }
> -
> - return IC_SWITCH;
> -}
> -
> static enum ic_port_binding_type
> ic_pb_get_type(const struct icsbrec_port_binding *isb_pb)
> {
> @@ -192,28 +182,6 @@ ic_pb_get_type(const struct icsbrec_port_binding *isb_pb)
> return IC_SWITCH_PORT;
> }
>
> -static void
> -enumerate_datapaths(struct ic_input *ic,
> - struct hmap *dp_tnlids,
> - struct shash *isb_ts_dps,
> - struct shash *isb_tr_dps)
> -{
> - const struct icsbrec_datapath_binding *isb_dp;
> - ICSBREC_DATAPATH_BINDING_TABLE_FOR_EACH (isb_dp,
> - ic->icsbrec_datapath_binding_table) {
> - ovn_add_tnlid(dp_tnlids, isb_dp->tunnel_key);
> -
> - enum ic_datapath_type dp_type = ic_dp_get_type(isb_dp);
> - if (dp_type == IC_ROUTER) {
> - char *uuid_str = uuid_to_string(isb_dp->nb_ic_uuid);
> - shash_add(isb_tr_dps, uuid_str, isb_dp);
> - free(uuid_str);
> - } else {
> - shash_add(isb_ts_dps, isb_dp->transit_switch, isb_dp);
> - }
> - }
> -}
> -
> static void
> ts_run(struct engine_context *ctx,
> struct ic_input *ic,
> @@ -275,7 +243,17 @@ ts_run(struct engine_context *ctx,
>
> const struct icsbrec_datapath_binding *isb_dp;
> isb_dp = shash_find_data(isb_ts_dps, ts->name);
> - if (isb_dp) {
> + if (!isb_dp) {
> + const struct icsbrec_datapath_binding *raw;
> + ICSBREC_DATAPATH_BINDING_TABLE_FOR_EACH (raw,
> + ic->icsbrec_datapath_binding_table) {
> + if (raw->transit_switch && !strcmp(raw->transit_switch,
> + ts->name)) {
> + isb_dp = raw;
> + break;
> + }
> + }
I'm having trouble understanding the need for this addition.
enum_datapaths should have filled in isb_ts_dps with all of the
transit switches from the datapath binding table, so I don't
understand how this loop helps.
If this loop does find a transit switch, then the else block below
will not be run. Therefore, the NB DB will not have the requested
tunnel key updated based on the transit switch config.
> + } else {
> int64_t nb_tnl_key = smap_get_int(&ls->other_config,
> "requested-tnl-key",
> 0);
> @@ -309,6 +287,24 @@ ts_run(struct engine_context *ctx,
> ic->icnbrec_transit_switch_table) {
> const struct icsbrec_datapath_binding *isb_dp =
> shash_find_and_delete(isb_ts_dps, ts->name);
> +
> + if (!isb_dp) {
> + const struct icsbrec_datapath_binding *raw_isb;
> + ICSBREC_DATAPATH_BINDING_TABLE_FOR_EACH (raw_isb,
> + ic->icsbrec_datapath_binding_table) {
> + if (raw_isb->n_nb_ic_uuid > 0 &&
> + uuid_equals(&raw_isb->nb_ic_uuid[0],
> + &ts->header_.uuid)) {
> + isb_dp = raw_isb;
> + if (isb_dp->transit_switch) {
> + shash_find_and_delete(isb_ts_dps,
> + isb_dp->transit_switch);
> + }
> + break;
> + }
> + }
> + }
I have the same question here: why would this loop be necessary if the
enum_datapaths node has populated the isb_ts_dps with all transit
switches?
> +
> if (!isb_dp) {
> /* Allocate tunnel key */
> int64_t dp_key = allocate_dp_key(dp_tnlids, vxlan_mode,
> @@ -320,6 +316,9 @@ ts_run(struct engine_context *ctx,
> isb_dp =
> icsbrec_datapath_binding_insert(ctx->ovnisb_idl_txn);
> icsbrec_datapath_binding_set_transit_switch(isb_dp,
> ts->name);
> icsbrec_datapath_binding_set_tunnel_key(isb_dp, dp_key);
> + icsbrec_datapath_binding_set_nb_ic_uuid(isb_dp,
> + &ts->header_.uuid,
> 1);
> + icsbrec_datapath_binding_set_type(isb_dp, "transit-switch");
> } else if (dp_key_refresh) {
> /* Refresh tunnel key since encap mode has changed. */
> int64_t dp_key = allocate_dp_key(dp_tnlids, vxlan_mode,
> @@ -339,9 +338,13 @@ ts_run(struct engine_context *ctx,
> }
> }
>
> - struct shash_node *node;
> - SHASH_FOR_EACH (node, isb_ts_dps) {
> - icsbrec_datapath_binding_delete(node->data);
> + struct shash_node *node, *next;
> + SHASH_FOR_EACH_SAFE (node, next, isb_ts_dps) {
> + struct icsbrec_datapath_binding *isb_dp_to_del = node->data;
> + if (isb_dp_to_del->n_nb_ic_uuid > 0) {
> + icsbrec_datapath_binding_delete(isb_dp_to_del);
> + }
> + shash_delete(isb_ts_dps, node);
> }
> }
> }
> @@ -3114,10 +3117,8 @@ ovn_db_run(struct ic_input *input_data,
> struct engine_context *eng_ctx)
> {
> gateway_run(eng_ctx, input_data);
> - enumerate_datapaths(input_data, &ic_data->dp_tnlids,
> - &ic_data->isb_ts_dps, &ic_data->isb_tr_dps);
> - ts_run(eng_ctx, input_data, &ic_data->dp_tnlids, &ic_data->isb_ts_dps);
> - tr_run(eng_ctx, input_data, &ic_data->dp_tnlids, &ic_data->isb_tr_dps);
> + ts_run(eng_ctx, input_data, ic_data->dp_tnlids, ic_data->isb_ts_dps);
> + tr_run(eng_ctx, input_data, ic_data->dp_tnlids, ic_data->isb_tr_dps);
> port_binding_run(eng_ctx, input_data);
> route_run(eng_ctx, input_data);
> sync_service_monitor(eng_ctx, input_data);
> @@ -3522,6 +3523,7 @@ main(int argc, char *argv[])
> ovn_conn_show, ovnisb_idl_loop.idl);
>
> stopwatch_create(IC_OVN_DB_RUN_STOPWATCH_NAME, SW_MS);
> + stopwatch_create(OVN_IC_ENUM_DATAPATHS_RUN_STOPWATCH_NAME, SW_MS);
>
> /* Initialize incremental processing engine for ovn-northd */
> inc_proc_ic_init(&ovnnb_idl_loop, &ovnsb_idl_loop,
> diff --git a/ic/ovn-ic.h b/ic/ovn-ic.h
> index 2c2efc046..9b0009274 100644
> --- a/ic/ovn-ic.h
> +++ b/ic/ovn-ic.h
> @@ -66,10 +66,10 @@ struct ic_input {
> };
>
> struct ic_data {
> - /* Global state for 'en-ic'. */
> - struct hmap dp_tnlids;
> - struct shash isb_ts_dps;
> - struct shash isb_tr_dps;
> + /* Global state for 'en-enum-datapaths'. */
> + struct hmap *dp_tnlids;
> + struct shash *isb_ts_dps;
> + struct shash *isb_tr_dps;
If at all possible, these three fields should be const. These fields
are created and managed by the enum_datapaths engine node, so any
other engine node should treat this data as read-only.
In this particular case, the ts_run() and tr_run() functions are
specifically not treating the isb_ts_dps and isb_tr_dps as const.
Those functions will remove items from the shash. If this can be
avoided, that would be for the best.
> };
> struct ic_state {
> bool had_lock;
> diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h
> index e933213d8..21572f023 100644
> --- a/lib/stopwatch-names.h
> +++ b/lib/stopwatch-names.h
> @@ -41,5 +41,6 @@
> #define GROUP_ECMP_ROUTE_RUN_STOPWATCH_NAME "group_ecmp_route"
>
> #define IC_OVN_DB_RUN_STOPWATCH_NAME "ovn_db_run"
> +#define OVN_IC_ENUM_DATAPATHS_RUN_STOPWATCH_NAME "enum_datapaths_run"
>
> #endif
> --
> 2.34.1
>
>
> --
>
>
>
>
> _'Esta mensagem é direcionada apenas para os endereços constantes no
> cabeçalho inicial. Se você não está listado nos endereços constantes no
> cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa
> mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas estão
> imediatamente anuladas e proibidas'._
>
>
> * **'Apesar do Magazine Luiza tomar
> todas as precauções razoáveis para assegurar que nenhum vírus esteja
> presente nesse e-mail, a empresa não poderá aceitar a responsabilidade por
> quaisquer perdas ou danos causados por esse e-mail ou por seus anexos'.*
>
>
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
