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
