On Thu, Oct 15, 2026 at 7:11 PM Paulo Guilherme Silva via dev <
[email protected]> wrote:
> The ovn-ic instance holding the IC-SB lock writes, among others, the
> transit switch and router Datapath_Bindings through the locked IC-SB
> connection. Its transaction isn't available while the previous one is
> being committed, but ovn-ic runs the engine anyway. The nodes then skip
> their IC-SB writes: the en_tunnel_key handlers fall back to a recompute,
> which skips them as well, and the tracked changes are cleared, so e.g.
> the Datapath_Binding of a deleted transit switch isn't garbage collected
> until a later recompute.
>
> Postpone the engine run while the locked IC-SB transaction isn't
> available, keeping the tracked changes, as is already done for the other
> connections.
>
> Also, ignore the IC-SB Datapath_Binding deletions whose IC-NB transit
> switch or router is gone in en_tunnel_key. They are the echo of its own
> garbage collection and there is nothing to reconcile for them.
>
> Both made the "transit switch handler scoping" test flaky, as deleting
> a transit switch could trigger a recompute of en_tunnel_key.
>
> Fixes: 0ad737079717 ("ic: Add the en_tunnel_key node.")
> Signed-off-by: Paulo Guilherme Silva <[email protected]>
> ---
> ic/en-tunnel-key.c | 30 ++++++++++++++++++++++++++----
> ic/ovn-ic.c | 11 ++++++++++-
> 2 files changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/ic/en-tunnel-key.c b/ic/en-tunnel-key.c
> index 0a63a6c90..834c2143e 100644
> --- a/ic/en-tunnel-key.c
> +++ b/ic/en-tunnel-key.c
> @@ -377,11 +377,28 @@ en_tunnel_key_icnb_transit_router_handler(struct
> engine_node *node,
> return ret;
> }
>
> +static bool
> +icnb_transit_switch_exists(struct ic_context *ctx, const char *name)
> +{
> + const struct icnbrec_transit_switch *key =
> + icnbrec_transit_switch_index_init_row(
> + ctx->icnbrec_transit_switch_by_name);
> + icnbrec_transit_switch_index_set_name(key, name);
> + const struct icnbrec_transit_switch *ts =
> +
> icnbrec_transit_switch_index_find(ctx->icnbrec_transit_switch_by_name,
> + key);
> + icnbrec_transit_switch_index_destroy_row(key);
> + return ts != NULL;
> +}
> +
> /* IC-SB Datapath_Binding: react only to a binding *deletion* by
> re-creating it
> * for the still-present IC-NB entity (self-healing after an out-of-band
> - * delete). Inserts and modifies are this node's own writes (or already
> - * consistent), so they are a no-op here - the requested-tnl-key
> follow-up is
> - * driven by en_ts/en_tr's own handlers on this same table. */
> + * delete). The deletion of a binding whose IC-NB entity is gone is the
> echo
> + * of this node's own garbage collection and needs nothing, so it must not
> + * depend on the IC-SB transaction being available. Inserts and modifies
> are
> + * this node's own writes (or already consistent), so they are a no-op
> here -
> + * the requested-tnl-key follow-up is driven by en_ts/en_tr's own
> handlers on
> + * this same table. */
> enum engine_input_handler_result
> en_tunnel_key_icsb_datapath_binding_handler(struct engine_node *node,
> void *data_)
> @@ -402,10 +419,15 @@ en_tunnel_key_icsb_datapath_binding_handler(struct
> engine_node *node,
> /* Not inserted by ovn-ic: nothing to reconcile. */
> continue;
> }
> + if (!icnbrec_transit_router_get_for_uuid(ctx->ovninb_idl,
> + isb_dp->nb_ic_uuid))
> {
> + continue;
> + }
> char *uuid_str = uuid_to_string(isb_dp->nb_ic_uuid);
> sset_add(&tr_scope, uuid_str);
> free(uuid_str);
> - } else {
> + } else if (icnb_transit_switch_exists(ctx,
> + isb_dp->transit_switch)) {
> sset_add(&ts_scope, isb_dp->transit_switch);
> }
> }
> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> index 7e9b3139f..e65256577 100644
> --- a/ic/ovn-ic.c
> +++ b/ic/ovn-ic.c
> @@ -1449,8 +1449,17 @@ main(int argc, char *argv[])
> ovsdb_idl_has_ever_connected(ctx.ovninb_idl) &&
> ovsdb_idl_has_ever_connected(ctx.ovnisb_idl) &&
> ovsdb_idl_has_ever_connected(ctx.ovnisb_unlocked_idl)) {
> + /* The AZ holding the IC-SB lock writes through the locked
> + * connection, whose transaction isn't available while the
> + * previous one is being committed. Postpone the engine
> run
> + * in that case, keeping the tracked changes, as the nodes
> + * would otherwise skip their writes and the changes they
> + * handle incrementally would be lost. */
> + bool isb_txn_ready = ctx.ovnisb_txn ||
> + !ovsdb_idl_has_lock(ctx.ovnisb_idl);
> if (ctx.ovnnb_txn && ctx.ovnsb_txn && ctx.ovninb_txn &&
> - ctx.ovnisb_unlocked_txn &&
> inc_proc_ic_can_run(&eng_ctx)) {
> + ctx.ovnisb_unlocked_txn && isb_txn_ready &&
> + inc_proc_ic_can_run(&eng_ctx)) {
> /* The availability zone is now resolved by the en_az
> * engine node. The subsystem nodes skip their work
> when
> * there is no AZ; the sequence-number bookkeeping
> below
> --
> 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
>
>
Thank you Paulo,
applied to main.
Regards,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
