On Thu, Oct 15, 2026 at 7:12 PM Paulo Guilherme Silva
<[email protected]> wrote:
> Hi Ales,
>
Hi Paulo,
>
> I found the cause of the flakiness. The failing check in that run was
> "tunnel_key recompute = 0" right after "ts-del ts1".
>
> The ovn-ic instance holding the IC-SB lock writes the transit switch and
> router Datapath_Bindings through the locked IC-SB connection. That
> transaction isn't available while the previous one is still being
> committed, but ovn-ic ran the engine anyway. In that case the
> en_tunnel_key handlers fell back to a recompute. The recompute also
> skipped the IC-SB writes and the tracked changes were cleared, so the
> recompute wasn't only spurious: the incremental change could be lost
> (e.g. the binding of a deleted transit switch wasn't garbage collected).
> The deletion of a binding by en_tunnel_key could also come back while
> its transaction was still pending and trigger the same fallback. The
> test hit the recompute side of it, mostly with slow builds such as the
> sanitizers one.
>
> I sent a follow-up fix:
>
> ic: Wait for the IC-SB transaction before running the engine.
>
I will take a look.
>
> When the instance holds the IC-SB lock and the locked transaction isn't
> available, the engine run is postponed and the tracked changes are kept,
> as is already done for the other connections. en_tunnel_key also ignores
> the echo of its own binding deletions, as there is nothing to reconcile
> for them.
>
> I couldn't hit the race naturally, so to validate it I used a temporary
> local change that withholds the locked IC-SB transaction in half of the
> ovn-ic iterations (simulating the pending commit and its wakeup) and ran
> the whole ovn-ic.at with it. Without the fix, 10 tests failed, including
> this one and functional ones such as "transit switch handling" and
> "VXLAN tunnel key". With the fix, all 142 tests passed. The full
> testsuite also passes.
>
> Unrelated, but worth mentioning: one of the CI runs of the fix failed
> in "interconnection" because "tee" panicked:
>
> thread 'main' panicked at src/uu/tee/src/tee.rs:188:21:
> assertion `left == right` failed: 2nd pipe should have enough spare
>
> Since the move to Ubuntu 26.04, "tee" in the CI container is uutils
> 0.10.0. It assumes its 2nd internal pipe is never smaller than the 1st.
> But once the user's pipe buffers exceed fs.pipe-user-pages-soft, the
> kernel creates new pipes with only 2 pages, so when that limit is
> crossed between the two pipe() calls the assertion fails. That's likely
> with "make check -j4". I could reproduce it deterministically in an
> ubuntu:26.04 container by bringing the pipe usage to that point; GNU tee
> (gnutee, also in the image) is fine under the same conditions. It's
> fixed upstream in uutils commit e427814b99 ("tee: don't extend stdout if
> 1st pipe is small"). Until Ubuntu picks that up, any test going through
> ovn_trace (which pipes into tee) or another "| tee" can fail randomly,
> so it might be worth using GNU tee in the CI container.
>
Yeah I'm aware of both the failures and the fix, I'm hesitant to change
the tee for the Ubuntu container only, let's see how long it takes for
Ubuntu to pick up newer uutils.
> Thanks,
> Paulo
>
Thanks,
Ales
>
> Em ter., 13 de out. de 2026 às 10:02, Paulo Guilherme Silva
> <[email protected]> escreveu:
>
>> Hi Ales,
>>
>> Yes, I will investigate the flaky test and propose a fix.
>>
>> Thanks,
>> Paulo
>>
>> Em seg., 12 de out. de 2026 às 02:53, Ales Musil <[email protected]>
>> escreveu:
>>
>>>
>>>
>>> On Tue, Oct 6, 2026 at 10:09 PM Paulo Guilherme Silva via dev <
>>> [email protected]> wrote:
>>>
>>>> Add the incremental change handler for the en_tr node and scope its
>>>> incremental processing to the affected transit-router datapaths, so an
>>>> IC-NB transit-router (or its port) change reconciles only that router
>>>> instead of forcing a full recompute.
>>>>
>>>> Add a tests/ovn-ic.at test exercising the transit switch and router
>>>> incremental handlers.
>>>>
>>>> Assisted-by: Claude Opus 4.8, Claude Code
>>>> Co-authored-by: Tiago Matos <[email protected]>
>>>> Signed-off-by: Tiago Matos <[email protected]>
>>>> Signed-off-by: Paulo Guilherme Silva <[email protected]>
>>>> ---
>>>> ic/en-tr.c | 72 ++++++++++++++++++++++++++
>>>> ic/en-tr.h | 5 ++
>>>> ic/inc-proc-ic.c | 14 +++--
>>>> tests/ovn-ic.at | 131 +++++++++++++++++++++++++++++++++++++++++++++++
>>>> 4 files changed, 218 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/ic/en-tr.c b/ic/en-tr.c
>>>> index 9c1eed3de..d62883cc0 100644
>>>> --- a/ic/en-tr.c
>>>> +++ b/ic/en-tr.c
>>>> @@ -124,6 +124,78 @@ en_tr_run(struct engine_node *node, void *data
>>>> OVS_UNUSED)
>>>> return EN_UPDATED;
>>>> }
>>>>
>>>> +/* Only transit-router mirror logical routers (options:interconn-tr)
>>>> affect
>>>> + * en_tr. A change to such a logical router is left to a full
>>>> recompute
>>>> + * (returns EN_UNHANDLED); any other logical router is irrelevant to
>>>> en_tr, so
>>>> + * its change is handled as a no-op, avoiding a recompute on unrelated
>>>> NB
>>>> + * Logical_Router updates. */
>>>> +enum engine_input_handler_result
>>>> +en_tr_nb_logical_router_handler(struct engine_node *node,
>>>> + void *data OVS_UNUSED)
>>>> +{
>>>> + const struct ed_type_az *az = engine_get_input_data("az", node);
>>>> +
>>>> + if (!az->runned_az) {
>>>> + return EN_HANDLED_UNCHANGED;
>>>> + }
>>>> +
>>>> + const struct nbrec_logical_router_table *tbl =
>>>> + EN_OVSDB_GET(engine_get_input("NB_logical_router", node));
>>>> + const struct nbrec_logical_router *lr;
>>>> + NBREC_LOGICAL_ROUTER_TABLE_FOR_EACH_TRACKED (lr, tbl) {
>>>> + if (smap_get(&lr->options, "interconn-tr")) {
>>>> + return EN_UNHANDLED;
>>>> + }
>>>> + }
>>>> +
>>>> + return EN_HANDLED_UNCHANGED;
>>>> +}
>>>> +
>>>> +/* IC-SB Datapath_Binding: tr_run() reads only transit-router datapath
>>>> bindings
>>>> + * (it mirrors their tunnel_key into the NB logical router as
>>>> + * requested-tnl-key). A new/deleted/modified transit-router binding
>>>> - created
>>>> + * by en_tunnel_key - forces a recompute so tr_run() publishes the
>>>> committed
>>>> + * key. Transit-switch datapath bindings - the bulk of the churn here
>>>> - do not
>>>> + * affect en_tr and are a no-op. A change to the type column cannot be
>>>> + * classified (the old type is gone), so recompute to be safe. */
>>>> +enum engine_input_handler_result
>>>> +en_tr_icsb_datapath_binding_handler(struct engine_node *node,
>>>> + void *data OVS_UNUSED)
>>>> +{
>>>> + const struct ed_type_az *az = engine_get_input_data("az", node);
>>>> +
>>>> + if (!az->runned_az) {
>>>> + return EN_HANDLED_UNCHANGED;
>>>> + }
>>>> +
>>>> + const struct icsbrec_datapath_binding_table *tbl =
>>>> + EN_OVSDB_GET(engine_get_input("ICSB_datapath_binding", node));
>>>> + const struct icsbrec_datapath_binding *isb_dp;
>>>> + ICSBREC_DATAPATH_BINDING_TABLE_FOR_EACH_TRACKED (isb_dp, tbl) {
>>>> + if (icsbrec_datapath_binding_is_new(isb_dp) ||
>>>> + icsbrec_datapath_binding_is_deleted(isb_dp)) {
>>>> + /* A new/deleted transit-router binding is the trigger
>>>> en_tr needs;
>>>> + * a transit-switch binding (the common churn) is a no-op.
>>>> Classify
>>>> + * by the row's own type, which is available on tracked
>>>> deleted
>>>> + * rows too - unlike track_is_updated(), which reports
>>>> every
>>>> + * column as updated on insert. */
>>>> + if (ic_dp_get_type(isb_dp) == IC_ROUTER) {
>>>> + return EN_UNHANDLED;
>>>> + }
>>>> + } else if (ic_dp_get_type(isb_dp) == IC_ROUTER ||
>>>> + ovsdb_idl_track_is_updated(
>>>> + &isb_dp->header_,
>>>> + &icsbrec_datapath_binding_col_type)) {
>>>> + /* A modified transit-router binding (e.g.
>>>> tunnel_key/nb_ic_uuid),
>>>> + * or a type change that can no longer be classified,
>>>> forces a
>>>> + * recompute. */
>>>> + return EN_UNHANDLED;
>>>> + }
>>>> + }
>>>> +
>>>> + return EN_HANDLED_UNCHANGED;
>>>> +}
>>>> +
>>>> void *
>>>> en_tr_init(struct engine_node *node OVS_UNUSED,
>>>> struct engine_arg *arg OVS_UNUSED)
>>>> diff --git a/ic/en-tr.h b/ic/en-tr.h
>>>> index c558c0913..cc55a794f 100644
>>>> --- a/ic/en-tr.h
>>>> +++ b/ic/en-tr.h
>>>> @@ -21,4 +21,9 @@ enum engine_node_state en_tr_run(struct engine_node
>>>> *node, void *data);
>>>> void *en_tr_init(struct engine_node *node, struct engine_arg *arg);
>>>> void en_tr_cleanup(void *data);
>>>>
>>>> +enum engine_input_handler_result
>>>> +en_tr_nb_logical_router_handler(struct engine_node *node, void *data);
>>>> +enum engine_input_handler_result
>>>> +en_tr_icsb_datapath_binding_handler(struct engine_node *node, void
>>>> *data);
>>>> +
>>>> #endif /* EN_IC_TR_H */
>>>> diff --git a/ic/inc-proc-ic.c b/ic/inc-proc-ic.c
>>>> index 89135fc5d..88c866f4b 100644
>>>> --- a/ic/inc-proc-ic.c
>>>> +++ b/ic/inc-proc-ic.c
>>>> @@ -238,12 +238,18 @@ 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 their AZ NB Logical_Router
>>>> mirrors.
>>>> - * Like en_ts, IC-SB Datapath_Binding creation/keying is owned by
>>>> - * en_tunnel_key. */
>>>> + *
>>>> + * Like en_ts, en_tr builds its own transit-router IC-SB
>>>> Datapath_Binding
>>>> + * map each run and only maintains the NB mirror; IC-SB
>>>> Datapath_Binding
>>>> + * creation/keying is owned by en_tunnel_key. A transit-router
>>>> binding
>>>> + * change (created by en_tunnel_key) forces a recompute so en_tr
>>>> publishes
>>>> + * the committed key to requested-tnl-key. */
>>>> engine_add_input(&en_tr, &en_az, NULL);
>>>> - engine_add_input(&en_tr, &en_icsb_datapath_binding, NULL);
>>>> + engine_add_input(&en_tr, &en_icsb_datapath_binding,
>>>> + en_tr_icsb_datapath_binding_handler);
>>>> engine_add_input(&en_tr, &en_icnb_transit_router, NULL);
>>>> - engine_add_input(&en_tr, &en_nb_logical_router, NULL);
>>>> + engine_add_input(&en_tr, &en_nb_logical_router,
>>>> + en_tr_nb_logical_router_handler);
>>>>
>>>> /* en_tunnel_key: the single owner of IC-SB Datapath_Binding
>>>> creation,
>>>> * tunnel-key allocation, VXLAN-range refresh and GC, for both
>>>> transit
>>>> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
>>>> index 3a9755632..1e19b2d53 100644
>>>> --- a/tests/ovn-ic.at
>>>> +++ b/tests/ovn-ic.at
>>>> @@ -932,6 +932,137 @@ OVN_CLEANUP_IC([az1])
>>>> AT_CLEANUP
>>>> ])
>>>>
>>>> +OVN_FOR_EACH_NORTHD([
>>>> +AT_SETUP([ovn-ic -- incremental processing - transit switch and
>>>> router])
>>>> +
>>>> +# The en_ts/en_tr NB Logical_Switch/Logical_Router change handlers
>>>> must not
>>>> +# trigger a full recompute for logical switches/routers that are not
>>>> transit
>>>> +# switch/router mirrors.
>>>> +
>>>> +ovn_init_ic_db
>>>> +net_add n1
>>>> +ovn_start az1
>>>> +as az1
>>>> +check ovn-ic-nbctl --wait=sb ts-add ts1
>>>> +OVS_WAIT_UNTIL([test "$(ovn-nbctl --bare --columns=name \
>>>> + find logical_switch name=ts1)" = ts1])
>>>> +
>>>> +# Quiesce the engine and reset stats so only the change under test
>>>> counts.
>>>> +check ovn-ic-nbctl --wait=sb sync
>>>> +check ovn-appctl -t ic/ovn-ic inc-engine/recompute
>>>> +check ovn-ic-nbctl --wait=sb sync
>>>> +check ovn-appctl -t ic/ovn-ic inc-engine/clear-stats
>>>> +
>>>> +# Adding a regular logical switch and a regular logical router must be
>>>> handled
>>>> +# by the en_ts/en_tr handlers without a full recompute of those nodes.
>>>> +check ovn-nbctl --wait=sb ls-add sw0
>>>> +check ovn-nbctl --wait=sb lr-add lr0
>>>> +OVS_WAIT_UNTIL([test "$(ovn-nbctl --bare --columns=name \
>>>> + find logical_router name=lr0)" = lr0])
>>>> +check ovn-ic-nbctl --wait=sb sync
>>>> +
>>>> +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \
>>>> + inc-engine/show-stats ts compute)" -gt 0])
>>>> +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \
>>>> + inc-engine/show-stats ts recompute)" = 0])
>>>> +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \
>>>> + inc-engine/show-stats tr compute)" -gt 0])
>>>> +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \
>>>> + inc-engine/show-stats tr recompute)" = 0])
>>>> +
>>>> +# The transit switch mirror is still intact after the incremental
>>>> update.
>>>> +AT_CHECK([test "$(ovn-nbctl --bare --columns=name \
>>>> + find logical_switch name=ts1)" = ts1])
>>>> +
>>>> +OVN_CLEANUP_IC([az1])
>>>> +AT_CLEANUP
>>>> +])
>>>> +
>>>> +OVN_FOR_EACH_NORTHD([
>>>> +AT_SETUP([ovn-ic -- incremental processing - transit switch handler
>>>> scoping])
>>>> +
>>>> +# A transit-switch add/delete in IC-NB must be applied by the en_ts
>>>> +# en_icnb_transit_switch change handler without a full recompute of
>>>> en_ts,
>>>> +# keeping the NB Logical_Switch mirror and the IC-SB Datapath_Binding
>>>> in sync.
>>>> +
>>>> +ovn_init_ic_db
>>>> +net_add n1
>>>> +ovn_start az1
>>>> +as az1
>>>> +check ovn-ic-nbctl --wait=sb ts-add ts1
>>>> +OVS_WAIT_UNTIL([test "$(ovn-nbctl --bare --columns=name \
>>>> + find logical_switch name=ts1)" = ts1])
>>>> +
>>>> +# Quiesce the engine and reset stats so only the change under test
>>>> counts.
>>>> +check ovn-ic-nbctl --wait=sb sync
>>>> +check ovn-appctl -t ic/ovn-ic inc-engine/recompute
>>>> +check ovn-ic-nbctl --wait=sb sync
>>>> +check ovn-appctl -t ic/ovn-ic inc-engine/clear-stats
>>>> +
>>>> +# Adding a second transit switch is handled incrementally (no en_ts
>>>> recompute)
>>>> +# and creates its NB mirror and IC-SB datapath binding.
>>>> +check ovn-ic-nbctl --wait=sb ts-add ts2
>>>> +OVS_WAIT_UNTIL([test "$(ovn-nbctl --bare --columns=name \
>>>> + find logical_switch name=ts2)" = ts2])
>>>> +check ovn-ic-nbctl --wait=sb sync
>>>> +
>>>> +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \
>>>> + inc-engine/show-stats ts compute)" -gt 0])
>>>> +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \
>>>> + inc-engine/show-stats ts recompute)" = 0])
>>>> +# The transit-switch IC-SB datapath binding en_tunnel_key creates must
>>>> not
>>>> +# churn en_tr: en_tr only consumes transit-router datapath bindings,
>>>> so its
>>>> +# en_icsb_datapath_binding handler treats a transit-switch binding as
>>>> a no-op.
>>>> +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \
>>>> + inc-engine/show-stats tr compute)" -gt 0])
>>>> +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \
>>>> + inc-engine/show-stats tr recompute)" = 0])
>>>> +# en_tunnel_key owns IC-SB datapath binding creation: a transit-switch
>>>> add is
>>>> +# handled by its scoped en_icnb_transit_switch handler, so it must not
>>>> full
>>>> +# recompute.
>>>> +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \
>>>> + inc-engine/show-stats tunnel_key compute)" -gt
>>>> 0])
>>>> +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \
>>>> + inc-engine/show-stats tunnel_key recompute)" =
>>>> 0])
>>>> +AT_CHECK([test "$(ovn-ic-sbctl --bare --columns=type \
>>>> + find datapath_binding transit_switch=ts2)" =
>>>> transit-switch])
>>>> +
>>>> +# Deleting a transit switch is also handled incrementally: its NB
>>>> mirror and
>>>> +# IC-SB datapath binding are garbage-collected without a full
>>>> recompute.
>>>> +check ovn-ic-nbctl --wait=sb ts-del ts1
>>>> +OVS_WAIT_UNTIL([test -z "$(ovn-nbctl --bare --columns=name \
>>>> + find logical_switch name=ts1)"])
>>>> +check ovn-ic-nbctl --wait=sb sync
>>>> +
>>>> +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \
>>>> + inc-engine/show-stats ts compute)" -gt 0])
>>>> +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \
>>>> + inc-engine/show-stats ts recompute)" = 0])
>>>> +# A transit-switch delete is likewise reconciled incrementally in
>>>> en_tunnel_key
>>>> +# (its scoped en_icnb_transit_switch handler GCs the binding), not by
>>>> a full
>>>> +# recompute.
>>>> +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \
>>>> + inc-engine/show-stats tunnel_key compute)" -gt
>>>> 0])
>>>> +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \
>>>> + inc-engine/show-stats tunnel_key recompute)" =
>>>> 0])
>>>> +AT_CHECK([test -z "$(ovn-ic-sbctl --bare --columns=type \
>>>> + find datapath_binding transit_switch=ts1)"])
>>>> +
>>>> +# Deleting the NB mirror of a live transit switch out-of-band
>>>> recreates it via
>>>> +# the en_ts NB Logical_Switch handler, still without a full recompute.
>>>> +check ovn-nbctl --wait=sb ls-del ts2
>>>> +check ovn-ic-nbctl --wait=sb sync
>>>> +OVS_WAIT_UNTIL([test "$(ovn-nbctl --bare --columns=name \
>>>> + find logical_switch name=ts2)" = ts2])
>>>> +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \
>>>> + inc-engine/show-stats ts compute)" -gt 0])
>>>> +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \
>>>> + inc-engine/show-stats ts recompute)" = 0])
>>>> +
>>>> +OVN_CLEANUP_IC([az1])
>>>> +AT_CLEANUP
>>>> +])
>>>> +
>>>> OVN_FOR_EACH_NORTHD([
>>>> AT_SETUP([ovn-ic -- port sync])
>>>>
>>>> --
>>>> 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
>>>>
>>>>
>>> Hi Paulo,
>>>
>>> I see that the test added with this patch is a bit flaky.
>>> Could you please investigate and possibly send a follow up fix?
>>>
>>> Failure example: https://github.com/ovn-org/ovn/actions/runs/38096998339
>>> .
>>>
>>> Thanks,
>>> Ales
>>>
>>
>
> *‘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
