Hi Paulo, I have a few notes on this patch.
On Tue, Jul 21, 2026 at 1:44 PM Paulo Guilherme Silva <[email protected]> wrote: > > Introduce the en_az engine node: move the availability-zone computation > (previously done inline in ovn_db_run) into a dedicated node that resolves > ctx->runned_az, which the downstream subsystem nodes gate on. > > Also wire the ic-backoff-interval-ms option to throttle the engine loop > (IC_NB_Global:options:ic-backoff-interval-ms), so a busy IC-SB does not > spin ovn-ic at full speed. ic-backoff-interval-ms should be documented in ovn-ic-nb.xml > > Assisted-by: Claude Opus 4.8, Claude Code > Signed-off-by: Paulo Guilherme Silva <[email protected]> > --- > ic/automake.mk | 2 + > ic/en-az.c | 125 ++++++++++++++++++++++++++++++++++++++++ > ic/en-az.h | 26 +++++++++ > ic/en-gateway.c | 6 ++ > ic/en-port-binding.c | 6 ++ > ic/en-route.c | 6 ++ > ic/en-service-monitor.c | 6 ++ > ic/en-tr.c | 6 ++ > ic/en-ts.c | 6 ++ > ic/inc-proc-ic.c | 8 +++ > ic/ovn-ic.c | 82 +++++--------------------- > 11 files changed, 213 insertions(+), 66 deletions(-) > create mode 100644 ic/en-az.c > create mode 100644 ic/en-az.h > > diff --git a/ic/automake.mk b/ic/automake.mk > index fd888d60c..b730fc578 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-az.c \ > + ic/en-az.h \ > ic/en-dp-enum.c \ > ic/en-dp-enum.h \ > ic/en-gateway.c \ > diff --git a/ic/en-az.c b/ic/en-az.c > new file mode 100644 > index 000000000..1e51ec391 > --- /dev/null > +++ b/ic/en-az.c > @@ -0,0 +1,125 @@ > +/* > + * 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 "en-az.h" > +#include "lib/inc-proc-eng.h" > +#include "lib/ovn-ic-sb-idl.h" > +#include "lib/ovn-nb-idl.h" > +#include "openvswitch/vlog.h" > +#include "ovn-ic.h" > +#include "ovsdb-idl.h" > +#include "util.h" > + > +VLOG_DEFINE_THIS_MODULE(en_ic_az); > + > +/* Resolves (and, if needed, registers) the availability zone for this ovn-ic > + * instance. This node has no engine inputs: like the former az_run() helper > + * it runs on every engine iteration and reads the NB Global and IC-SB > + * Availability_Zone tables directly. It keeps ctx->runned_az populated for > + * the subsystem nodes (and for the main loop after inc_proc_ic_run()), and > + * reports EN_UPDATED only when the resolved AZ identity changes so dependent > + * nodes do not recompute on every iteration. */ > +enum engine_node_state > +en_az_run(struct engine_node *node OVS_UNUSED, void *data_) > +{ > + const struct engine_context *eng_ctx = engine_get_context(); > + struct ic_context *ctx = eng_ctx->client_ctx; > + struct ed_type_az *data = data_; > + const struct icsbrec_availability_zone *old_az = data->runned_az; > + const struct icsbrec_availability_zone *az; > + > + data->runned_az = NULL; > + > + const struct nbrec_nb_global *nb_global = > + nbrec_nb_global_first(ctx->ovnnb_idl); > + if (!nb_global) { > + VLOG_INFO("NB Global not exist."); A few notes here: * This should be rate-limited. * This should use the more grammatically-correct "NB Global does not exist." * Since this results in not being able to run en_az_run(), this seems like it should be VLOG_WARN instead of VLOG_INFO. > + goto out; > + } > + > + /* Update old AZ if name changes. Note: if name changed when ovn-ic > + * is not running, one has to manually delete/update the old AZ with: > + * "ovn-ic-sbctl destroy avail <az>". */ > + if (ctx->ovnisb_unlocked_txn && data->az_name > + && strcmp(data->az_name, nb_global->name)) { > + ICSBREC_AVAILABILITY_ZONE_FOR_EACH (az, ctx->ovnisb_unlocked_idl) { > + /* AZ name update locally need to update az in ISB. */ > + if (nb_global->name[0] && !strcmp(az->name, data->az_name)) { > + icsbrec_availability_zone_set_name(az, nb_global->name); > + break; > + } else if (!nb_global->name[0] > + && !strcmp(az->name, data->az_name)) { > + icsbrec_availability_zone_delete(az); > + break; > + } > + } > + free(data->az_name); > + data->az_name = NULL; > + } > + > + if (!nb_global->name[0]) { > + goto out; > + } > + > + if (!data->az_name) { > + data->az_name = xstrdup(nb_global->name); > + } > + > + if (ctx->ovnisb_unlocked_txn) { > + ovsdb_idl_txn_add_comment(ctx->ovnisb_unlocked_txn, "AZ %s", > + data->az_name); > + } > + > + ICSBREC_AVAILABILITY_ZONE_FOR_EACH (az, ctx->ovnisb_unlocked_idl) { > + if (!strcmp(az->name, data->az_name)) { > + data->runned_az = az; > + goto out; > + } > + } > + > + /* Create AZ in ISB */ > + if (ctx->ovnisb_unlocked_txn) { > + VLOG_INFO("Register AZ %s to interconnection DB.", data->az_name); > + az = icsbrec_availability_zone_insert(ctx->ovnisb_unlocked_txn); > + icsbrec_availability_zone_set_name(az, data->az_name); > + data->runned_az = az; > + } > + > +out: > + /* Bridge the result to ctx for the subsystem run()/helpers and the main > + * loop, which still read ctx->runned_az. */ > + ctx->runned_az = data->runned_az; I think storing runned_az in the ic_context is not a good idea. Any engine node that needs to know the runned_az should have en_az as an input node. Then they should use engine_get_input_data() to get a const pointer to en_az's data. With the current approach, nodes that rely on ctx->runned_az do not have to have en_az as an input node. If en_az returns EN_UPDATED, then those nodes that use ctx->runned_az but do not have en_az as an input node will not be run. As an example, this commit does not add en_az as an input node to en_address_set. However, en_address_set relies on ctx->runned_az as part of its processing. This means that en_address_set is not run when en_az updates, even though it should. > + VLOG_DBG("Availability zone: %s", > + data->runned_az ? data->runned_az->name : "not created yet."); This should be rate-limited. > + > + return data->runned_az == old_az ? EN_UNCHANGED : EN_UPDATED; > +} > + > +void * > +en_az_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + return xzalloc(sizeof(struct ed_type_az)); > +} > + > +void > +en_az_cleanup(void *data_) > +{ > + struct ed_type_az *data = data_; > + > + free(data->az_name); > + data->az_name = NULL; > +} > diff --git a/ic/en-az.h b/ic/en-az.h > new file mode 100644 > index 000000000..ea9b1a81c > --- /dev/null > +++ b/ic/en-az.h > @@ -0,0 +1,26 @@ > +#ifndef EN_IC_AZ_H > +#define EN_IC_AZ_H 1 > + > +#include <config.h> > + > +#include "lib/inc-proc-eng.h" > + > +struct icsbrec_availability_zone; > + > +/* Data maintained by the "az" engine node: the availability zone this ovn-ic > + * instance runs for. This replaces the former az_run() helper and its file > + * scope 'az_name' static variable. */ > +struct ed_type_az { > + /* The availability zone record for this instance, or NULL if it is not > + * known/registered yet. Owned by the IDL. */ > + const struct icsbrec_availability_zone *runned_az; > + /* The name this instance registered as, persisted across iterations so a > + * later rename can update the existing IC-SB record. */ > + char *az_name; > +}; > + > +enum engine_node_state en_az_run(struct engine_node *node, void *data); > +void *en_az_init(struct engine_node *node, struct engine_arg *arg); > +void en_az_cleanup(void *data); > + > +#endif /* EN_IC_AZ_H */ > diff --git a/ic/en-gateway.c b/ic/en-gateway.c > index f41166018..570bfea8d 100644 > --- a/ic/en-gateway.c > +++ b/ic/en-gateway.c > @@ -27,6 +27,12 @@ en_gateway_run(struct engine_node *node OVS_UNUSED, void > *data OVS_UNUSED) > const struct engine_context *eng_ctx = engine_get_context(); > struct ic_context *ctx = eng_ctx->client_ctx; > > + /* runned_az is resolved by the upstream en_az node. Without an AZ there > + * is nothing to sync (mirrors the previous main-loop gating). */ > + if (!ctx->runned_az) { > + return EN_UNCHANGED; > + } This guard is added to a bunch of run() functions. As noted above, these should get their data from en_az rather than from the ic_context. Notably, this guard is not added to en_address_set_run(). This actually has a two-pronged effect * en_address_set_run() may run with a NULL runned_az. * en_az keeps data->runned_az NULL if the nb_global table has no data in it. en_address_set_run() will assert if nb_global is NULL. Adding the runned_az guard will ensure that en_address_set_run() will exit early and not assert. > + > gateway_run(ctx); > > return EN_UPDATED; > diff --git a/ic/en-port-binding.c b/ic/en-port-binding.c > index 8843d6cef..0b8b9c5b2 100644 > --- a/ic/en-port-binding.c > +++ b/ic/en-port-binding.c > @@ -27,6 +27,12 @@ en_port_binding_run(struct engine_node *node OVS_UNUSED, > void *data OVS_UNUSED) > const struct engine_context *eng_ctx = engine_get_context(); > struct ic_context *ctx = eng_ctx->client_ctx; > > + /* runned_az is resolved by the upstream en_az node. Without an AZ there > + * is nothing to sync (mirrors the previous main-loop gating). */ > + if (!ctx->runned_az) { > + return EN_UNCHANGED; > + } > + > port_binding_run(ctx); > > return EN_UPDATED; > diff --git a/ic/en-route.c b/ic/en-route.c > index ec39d89d9..922f006c6 100644 > --- a/ic/en-route.c > +++ b/ic/en-route.c > @@ -27,6 +27,12 @@ en_route_run(struct engine_node *node OVS_UNUSED, void > *data OVS_UNUSED) > const struct engine_context *eng_ctx = engine_get_context(); > struct ic_context *ctx = eng_ctx->client_ctx; > > + /* runned_az is resolved by the upstream en_az node. Without an AZ there > + * is nothing to sync (mirrors the previous main-loop gating). */ > + if (!ctx->runned_az) { > + return EN_UNCHANGED; > + } > + > route_run(ctx); > > return EN_UPDATED; > diff --git a/ic/en-service-monitor.c b/ic/en-service-monitor.c > index b85a36dc7..8e49c03eb 100644 > --- a/ic/en-service-monitor.c > +++ b/ic/en-service-monitor.c > @@ -28,6 +28,12 @@ en_service_monitor_run(struct engine_node *node OVS_UNUSED, > const struct engine_context *eng_ctx = engine_get_context(); > struct ic_context *ctx = eng_ctx->client_ctx; > > + /* runned_az is resolved by the upstream en_az node. Without an AZ there > + * is nothing to sync (mirrors the previous main-loop gating). */ > + if (!ctx->runned_az) { > + return EN_UNCHANGED; > + } > + > sync_service_monitor(ctx); > > return EN_UPDATED; > diff --git a/ic/en-tr.c b/ic/en-tr.c > index 4f05a4325..8b46f89e6 100644 > --- a/ic/en-tr.c > +++ b/ic/en-tr.c > @@ -29,6 +29,12 @@ en_tr_run(struct engine_node *node, void *data OVS_UNUSED) > struct ic_context *ctx = eng_ctx->client_ctx; > struct ed_type_dp_enum *dp = engine_get_input_data("dp_enum", node); > > + /* runned_az is resolved by the upstream en_az node. Without an AZ there > + * is nothing to sync (mirrors the previous main-loop gating). */ > + if (!ctx->runned_az) { > + return EN_UNCHANGED; > + } > + > tr_run(ctx, &dp->dp_tnlids, &dp->isb_tr_dps); > > return EN_UPDATED; > diff --git a/ic/en-ts.c b/ic/en-ts.c > index e8df1888a..84e37a30a 100644 > --- a/ic/en-ts.c > +++ b/ic/en-ts.c > @@ -29,6 +29,12 @@ en_ts_run(struct engine_node *node, void *data OVS_UNUSED) > struct ic_context *ctx = eng_ctx->client_ctx; > struct ed_type_dp_enum *dp = engine_get_input_data("dp_enum", node); > > + /* runned_az is resolved by the upstream en_az node. Without an AZ there > + * is nothing to sync (mirrors the previous main-loop gating). */ > + if (!ctx->runned_az) { > + return EN_UNCHANGED; > + } > + > ts_run(ctx, &dp->dp_tnlids, &dp->isb_ts_dps); > > return EN_UPDATED; > diff --git a/ic/inc-proc-ic.c b/ic/inc-proc-ic.c > index 8721c5673..9bde8f1f8 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-az.h" > #include "en-dp-enum.h" > #include "en-gateway.h" > #include "en-ts.h" > @@ -173,6 +174,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(az); > static ENGINE_NODE(dp_enum); > static ENGINE_NODE(gateway); > static ENGINE_NODE(ts); > @@ -215,6 +217,7 @@ void inc_proc_ic_init(struct ovsdb_idl_loop *nb, > engine_add_input(&en_dp_enum, &en_icnb_ic_nb_global, NULL); > > /* en_gateway: sync gateways/chassis between SB and IC-SB. */ > + engine_add_input(&en_gateway, &en_az, NULL); > engine_add_input(&en_gateway, &en_icsb_availability_zone, NULL); > engine_add_input(&en_gateway, &en_icsb_gateway, NULL); > engine_add_input(&en_gateway, &en_icsb_encap, NULL); > @@ -222,6 +225,7 @@ void inc_proc_ic_init(struct ovsdb_idl_loop *nb, > engine_add_input(&en_gateway, &en_sb_encap, NULL); > > /* en_ts: sync transit switches to NB and IC-SB datapath bindings. */ > + engine_add_input(&en_ts, &en_az, NULL); > engine_add_input(&en_ts, &en_dp_enum, NULL); > engine_add_input(&en_ts, &en_icnb_ic_nb_global, NULL); > engine_add_input(&en_ts, &en_icnb_transit_switch, NULL); > @@ -229,11 +233,13 @@ void inc_proc_ic_init(struct ovsdb_idl_loop *nb, > engine_add_input(&en_ts, &en_icsb_encap, NULL); > > /* en_tr: sync transit routers to NB and IC-SB datapath bindings. */ > + engine_add_input(&en_tr, &en_az, NULL); > engine_add_input(&en_tr, &en_dp_enum, NULL); > engine_add_input(&en_tr, &en_icnb_transit_router, NULL); > engine_add_input(&en_tr, &en_nb_logical_router, NULL); > > /* en_port_binding: sync cross-AZ port bindings. */ > + engine_add_input(&en_port_binding, &en_az, NULL); > engine_add_input(&en_port_binding, &en_icsb_availability_zone, NULL); > engine_add_input(&en_port_binding, &en_icsb_port_binding, NULL); > engine_add_input(&en_port_binding, &en_icnb_transit_switch, NULL); > @@ -247,6 +253,7 @@ void inc_proc_ic_init(struct ovsdb_idl_loop *nb, > engine_add_input(&en_port_binding, &en_sb_chassis, NULL); > > /* en_route: advertise/learn cross-AZ routes. */ > + engine_add_input(&en_route, &en_az, NULL); > engine_add_input(&en_route, &en_icsb_availability_zone, NULL); > engine_add_input(&en_route, &en_icsb_port_binding, NULL); > engine_add_input(&en_route, &en_icsb_route, NULL); > @@ -262,6 +269,7 @@ void inc_proc_ic_init(struct ovsdb_idl_loop *nb, > engine_add_input(&en_route, &en_sb_learned_route, NULL); > > /* en_service_monitor: sync load-balancer health checks across AZs. */ > + engine_add_input(&en_service_monitor, &en_az, NULL); > engine_add_input(&en_service_monitor, &en_icsb_availability_zone, NULL); > engine_add_input(&en_service_monitor, &en_icsb_service_monitor, NULL); > engine_add_input(&en_service_monitor, &en_sb_sb_global, NULL); > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > index b36c52ea5..3970974bc 100644 > --- a/ic/ovn-ic.c > +++ b/ic/ovn-ic.c > @@ -99,68 +99,6 @@ Options:\n\ > stream_usage("database", true, true, false); > } > > -static const struct icsbrec_availability_zone * > -az_run(struct ic_context *ctx) > -{ > - const struct nbrec_nb_global *nb_global = > - nbrec_nb_global_first(ctx->ovnnb_idl); > - > - if (!nb_global) { > - VLOG_INFO("NB Global not exist."); > - return NULL; > - } > - > - /* Update old AZ if name changes. Note: if name changed when ovn-ic > - * is not running, one has to manually delete/update the old AZ with: > - * "ovn-ic-sbctl destroy avail <az>". */ > - static char *az_name; > - const struct icsbrec_availability_zone *az; > - if (ctx->ovnisb_unlocked_txn && az_name > - && strcmp(az_name, nb_global->name)) { > - ICSBREC_AVAILABILITY_ZONE_FOR_EACH (az, ctx->ovnisb_unlocked_idl) { > - /* AZ name update locally need to update az in ISB. */ > - if (nb_global->name[0] && !strcmp(az->name, az_name)) { > - icsbrec_availability_zone_set_name(az, nb_global->name); > - break; > - } else if (!nb_global->name[0] && !strcmp(az->name, az_name)) { > - icsbrec_availability_zone_delete(az); > - break; > - } > - } > - free(az_name); > - az_name = NULL; > - } > - > - if (!nb_global->name[0]) { > - return NULL; > - } > - > - if (!az_name) { > - az_name = xstrdup(nb_global->name); > - } > - > - if (ctx->ovnisb_unlocked_txn) { > - ovsdb_idl_txn_add_comment(ctx->ovnisb_unlocked_txn, "AZ %s", > az_name); > - } > - > - ICSBREC_AVAILABILITY_ZONE_FOR_EACH (az, ctx->ovnisb_unlocked_idl) { > - if (!strcmp(az->name, az_name)) { > - ctx->runned_az = az; > - return az; > - } > - } > - > - /* Create AZ in ISB */ > - if (ctx->ovnisb_unlocked_txn) { > - VLOG_INFO("Register AZ %s to interconnection DB.", az_name); > - az = icsbrec_availability_zone_insert(ctx->ovnisb_unlocked_txn); > - icsbrec_availability_zone_set_name(az, az_name); > - ctx->runned_az = az; > - return az; > - } > - return NULL; > -} > - > static uint32_t > allocate_dp_key(struct hmap *dp_tnlids, bool vxlan_mode, const char *name) > { > @@ -4248,6 +4186,17 @@ main(int argc, char *argv[]) > ovnisb_idl_loop.idl, > ovnisb_unlocked_idl_loop.idl, > ovninb_idl_loop.idl); > + > + /* Postpone the next engine run by the length of the previous one, up > + * to this interval, to coalesce bursts of change-driven runs. A > + * forced recompute (set_force_recompute) still runs immediately, so > + * this only throttles the natural, incremental path. */ > + const struct icnbrec_ic_nb_global *ic_nb_global = > + icnbrec_ic_nb_global_first(ovninb_idl_loop.idl); > + eng_ctx.backoff_ms = ic_nb_global > + ? smap_get_uint(&ic_nb_global->options, > + "ic-backoff-interval-ms", 0) : 0; > + > memory_run(); > if (memory_should_report()) { > struct simap usage = SIMAP_INITIALIZER(&usage); > @@ -4418,11 +4367,12 @@ main(int argc, char *argv[]) > ovsdb_idl_has_ever_connected(ctx.ovnisb_unlocked_idl)) { > if (ctx.ovnnb_txn && ctx.ovnsb_txn && ctx.ovninb_txn && > ctx.ovnisb_unlocked_txn && > inc_proc_ic_can_run(&eng_ctx)) { > - ctx.runned_az = az_run(&ctx); > - VLOG_DBG("Availability zone: %s", ctx.runned_az ? > - ctx.runned_az->name : "not created yet."); > + /* The availability zone is now resolved by the en_az > + * engine node, which populates ctx.runned_az during the > + * run. The subsystem nodes skip their work when there > is > + * no AZ. */ > + (void) inc_proc_ic_run(&ctx, &eng_ctx); > if (ctx.runned_az) { > - (void) inc_proc_ic_run(&ctx, &eng_ctx); > update_sequence_numbers(&ctx, > &ovnisb_unlocked_idl_loop); > } > -- > 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
