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 | 167 +++++++++++++++++++++++++++++++++++++++++++++++ ic/en-tr.h | 5 ++ ic/inc-proc-ic.c | 19 +++++- ic/ovn-ic.c | 88 +------------------------ ic/ovn-ic.h | 6 +- tests/ovn-ic.at | 115 ++++++++++++++++++++++++++++++++ 6 files changed, 309 insertions(+), 91 deletions(-) diff --git a/ic/en-tr.c b/ic/en-tr.c index 524fe6ad6..c4fef700d 100644 --- a/ic/en-tr.c +++ b/ic/en-tr.c @@ -17,11 +17,102 @@ #include "en-dp-enum.h" #include "en-tr.h" #include "lib/inc-proc-eng.h" +#include "lib/ovn-ic-nb-idl.h" +#include "lib/ovn-ic-sb-idl.h" +#include "lib/ovn-nb-idl.h" +#include "openvswitch/shash.h" #include "openvswitch/vlog.h" #include "ovn-ic.h" +#include "ovsdb-idl.h" +#include "smap.h" VLOG_DEFINE_THIS_MODULE(en_ic_tr); +static void +tr_run(struct ic_context *ctx, struct hmap *dp_tnlids, + struct shash *isb_tr_dps) +{ + /* + * Warning: ovnisb_unlocked should not be used to insert data on IC_SB + * which can cause a constraint violation, as an example, inserting data to + * IC-SB datapath_binding. + */ + const struct nbrec_logical_router *lr; + + if (ctx->ovnnb_txn) { + struct shash nb_tres = SHASH_INITIALIZER(&nb_tres); + NBREC_LOGICAL_ROUTER_FOR_EACH (lr, ctx->ovnnb_idl) { + const char *tr_name = smap_get(&lr->options, "interconn-tr"); + if (tr_name) { + shash_add(&nb_tres, tr_name, lr); + } + } + + const struct icnbrec_transit_router *tr; + ICNBREC_TRANSIT_ROUTER_FOR_EACH (tr, ctx->ovninb_idl) { + lr = shash_find_and_delete(&nb_tres, tr->name); + if (!lr) { + lr = nbrec_logical_router_insert(ctx->ovnnb_txn); + nbrec_logical_router_set_name(lr, tr->name); + nbrec_logical_router_update_options_setkey( + lr, "interconn-tr", tr->name); + } + char *uuid_str = uuid_to_string(&tr->header_.uuid); + struct icsbrec_datapath_binding *isb_dp = shash_find_data( + isb_tr_dps, uuid_str); + free(uuid_str); + + if (isb_dp) { + char *tnl_key_str = xasprintf("%"PRId64, isb_dp->tunnel_key); + nbrec_logical_router_update_options_setkey( + lr, "requested-tnl-key", tnl_key_str); + free(tnl_key_str); + } + } + + struct shash_node *node; + SHASH_FOR_EACH (node, &nb_tres) { + nbrec_logical_router_delete(node->data); + } + shash_destroy(&nb_tres); + } + + /* Sync TR between INB and ISB. This is performed after syncing with AZ + * SB, to avoid uncommitted ISB datapath tunnel key to be synced back to + * AZ. */ + if (ctx->ovnisb_txn && + is_az_leader(ctx->ovnisb_txn)) { + /* Create ISB Datapath_Binding */ + const struct icnbrec_transit_router *tr; + ICNBREC_TRANSIT_ROUTER_FOR_EACH (tr, ctx->ovninb_idl) { + char *uuid_str = uuid_to_string(&tr->header_.uuid); + struct icsbrec_datapath_binding *isb_dp = + shash_find_and_delete(isb_tr_dps, uuid_str); + free(uuid_str); + + if (!isb_dp) { + int dp_key = allocate_dp_key(dp_tnlids, false, + "transit router datapath"); + if (!dp_key) { + continue; + } + + isb_dp = icsbrec_datapath_binding_insert(ctx->ovnisb_txn); + icsbrec_datapath_binding_set_tunnel_key(isb_dp, dp_key); + icsbrec_datapath_binding_set_nb_ic_uuid(isb_dp, + &tr->header_.uuid, 1); + icsbrec_datapath_binding_set_type(isb_dp, "transit-router"); + } + } + + struct shash_node *node; + SHASH_FOR_EACH (node, isb_tr_dps) { + icsbrec_datapath_binding_delete(node->data); + } + } +} + + enum engine_node_state en_tr_run(struct engine_node *node, void *data OVS_UNUSED) { @@ -46,6 +137,82 @@ 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 engine_context *eng_ctx = engine_get_context(); + struct ic_context *ctx = eng_ctx->client_ctx; + + if (!ctx->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, via en_dp_enum's isb_tr_dps map, and deletes orphans). + * Transit-switch datapath bindings - the bulk of the churn here - do not + * affect en_tr and are a no-op. This is the trigger that the en_dp_enum edge + * used to provide before it was reduced to an ordering-only (no-op) input: a + * transit-router binding change still forces a recompute so the freshly + * en_dp_enum-rebuilt isb_tr_dps is consumed. 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 engine_context *eng_ctx = engine_get_context(); + struct ic_context *ctx = eng_ctx->client_ctx; + + if (!ctx->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 0a29de2e5..a4824f5a8 100644 --- a/ic/en-tr.h +++ b/ic/en-tr.h @@ -9,4 +9,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 3e0f87118..5a193872f 100644 --- a/ic/inc-proc-ic.c +++ b/ic/inc-proc-ic.c @@ -278,11 +278,24 @@ void inc_proc_ic_init(struct ovsdb_idl_loop *nb, en_ts_nb_logical_switch_handler); engine_add_input(&en_ts, &en_icsb_encap, NULL); - /* en_tr: sync transit routers to NB and IC-SB datapath bindings. */ + /* en_tr: sync transit routers to NB and IC-SB datapath bindings. + * + * Like en_ts, en_dp_enum is an ordering dependency only: en_tr reads its + * shared tunnel-key allocator (dp_tnlids) and transit-router datapath map + * (isb_tr_dps) live in en_tr_run(), so a datapath-binding change it + * reports - including transit-switch churn that does not concern en_tr, + * and the binding en_tr itself just created - must not force a full + * recompute; the edge therefore uses a no-op handler. The real re-run + * trigger is the en_icsb_datapath_binding handler, which recomputes only + * for transit-router datapath bindings (en_dp_enum, being upstream, has + * already refreshed isb_tr_dps by then). */ engine_add_input(&en_tr, &en_az, NULL); - engine_add_input(&en_tr, &en_dp_enum, NULL); + engine_add_input(&en_tr, &en_dp_enum, engine_noop_handler); + 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_port_binding: sync cross-AZ port bindings. */ engine_add_input(&en_port_binding, &en_az, NULL); diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c index c1798b549..e096a818e 100644 --- a/ic/ovn-ic.c +++ b/ic/ovn-ic.c @@ -101,7 +101,7 @@ Options:\n\ stream_usage("database", true, true, false); } -static uint32_t +uint32_t allocate_dp_key(struct hmap *dp_tnlids, bool vxlan_mode, const char *name) { uint32_t hint = vxlan_mode ? OVN_MIN_DP_VXLAN_KEY_GLOBAL @@ -134,7 +134,7 @@ ic_pb_get_type(const struct icsbrec_port_binding *isb_pb) /* * Check if the AZ is the leader by checking the lock. */ -static bool +bool is_az_leader(struct ovsdb_idl_txn *txn) { struct ovsdb_idl *idl = ovsdb_idl_txn_get_idl(txn); @@ -375,90 +375,6 @@ ts_sync_scope(struct ic_context *ctx, struct hmap *dp_tnlids, shash_destroy(&isb_dps_scoped); } -void -tr_run(struct ic_context *ctx, struct hmap *dp_tnlids, - struct shash *isb_tr_dps) -{ - /* - * Warning: ovnisb_unlocked should not be used to insert data on IC_SB - * which can cause a constraint violation, as an example, inserting data to - * IC-SB datapath_binding. - */ - const struct nbrec_logical_router *lr; - - if (ctx->ovnnb_txn) { - struct shash nb_tres = SHASH_INITIALIZER(&nb_tres); - NBREC_LOGICAL_ROUTER_FOR_EACH (lr, ctx->ovnnb_idl) { - const char *tr_name = smap_get(&lr->options, "interconn-tr"); - if (tr_name) { - shash_add(&nb_tres, tr_name, lr); - } - } - - const struct icnbrec_transit_router *tr; - ICNBREC_TRANSIT_ROUTER_FOR_EACH (tr, ctx->ovninb_idl) { - lr = shash_find_and_delete(&nb_tres, tr->name); - if (!lr) { - lr = nbrec_logical_router_insert(ctx->ovnnb_txn); - nbrec_logical_router_set_name(lr, tr->name); - nbrec_logical_router_update_options_setkey( - lr, "interconn-tr", tr->name); - } - char *uuid_str = uuid_to_string(&tr->header_.uuid); - struct icsbrec_datapath_binding *isb_dp = shash_find_data( - isb_tr_dps, uuid_str); - free(uuid_str); - - if (isb_dp) { - char *tnl_key_str = xasprintf("%"PRId64, isb_dp->tunnel_key); - nbrec_logical_router_update_options_setkey( - lr, "requested-tnl-key", tnl_key_str); - free(tnl_key_str); - } - } - - struct shash_node *node; - SHASH_FOR_EACH (node, &nb_tres) { - nbrec_logical_router_delete(node->data); - } - shash_destroy(&nb_tres); - } - - /* Sync TR between INB and ISB. This is performed after syncing with AZ - * SB, to avoid uncommitted ISB datapath tunnel key to be synced back to - * AZ. */ - if (ctx->ovnisb_txn && - is_az_leader(ctx->ovnisb_txn)) { - /* Create ISB Datapath_Binding */ - const struct icnbrec_transit_router *tr; - ICNBREC_TRANSIT_ROUTER_FOR_EACH (tr, ctx->ovninb_idl) { - char *uuid_str = uuid_to_string(&tr->header_.uuid); - struct icsbrec_datapath_binding *isb_dp = - shash_find_and_delete(isb_tr_dps, uuid_str); - free(uuid_str); - - if (!isb_dp) { - int dp_key = allocate_dp_key(dp_tnlids, false, - "transit router datapath"); - if (!dp_key) { - continue; - } - - isb_dp = icsbrec_datapath_binding_insert(ctx->ovnisb_txn); - icsbrec_datapath_binding_set_tunnel_key(isb_dp, dp_key); - icsbrec_datapath_binding_set_nb_ic_uuid(isb_dp, - &tr->header_.uuid, 1); - icsbrec_datapath_binding_set_type(isb_dp, "transit-router"); - } - } - - struct shash_node *node; - SHASH_FOR_EACH (node, isb_tr_dps) { - icsbrec_datapath_binding_delete(node->data); - } - } -} - static void nb_addr_set_apply_diff(const void *arg, const char *item, bool add) { diff --git a/ic/ovn-ic.h b/ic/ovn-ic.h index 2e020db9d..8bca1340a 100644 --- a/ic/ovn-ic.h +++ b/ic/ovn-ic.h @@ -71,8 +71,6 @@ enum ic_datapath_type ic_dp_get_type( const struct icsbrec_datapath_binding *isb_dp); void address_set_run(struct ic_context *ctx); -void tr_run(struct ic_context *ctx, struct hmap *dp_tnlids, - struct shash *isb_tr_dps); void port_binding_run(struct ic_context *ctx); struct sset; @@ -89,7 +87,11 @@ void sync_service_monitor(struct ic_context *ctx); /* Shared IC helpers used by more than one engine node. */ +uint32_t +allocate_dp_key(struct hmap *dp_tnlids, bool vxlan_mode, const char *name); const struct sbrec_chassis * find_sb_chassis(struct ic_context *ctx, const char *name); +bool +is_az_leader(struct ovsdb_idl_txn *txn); #endif /* OVN_IC_H */ diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at index 2cf63c2c3..5ed04aeb1 100644 --- a/tests/ovn-ic.at +++ b/tests/ovn-ic.at @@ -881,6 +881,121 @@ 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 recompute)" = 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 recompute)" = 0]) +# The transit-switch IC-SB datapath binding it creates must not churn en_tr: +# en_tr only consumes transit-router datapath bindings, so its +# en_icsb_datapath_binding handler treats this as a no-op (the en_dp_enum edge +# is ordering-only). +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \ + inc-engine/show-stats tr recompute)" = 0]) +# en_dp_enum derives its maps from IC-SB datapath bindings, not IC-NB transit +# switches: a transit-switch add is an ordering-only edge, so it must not +# re-enumerate every datapath binding (no full recompute). +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \ + inc-engine/show-stats dp_enum 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 recompute)" = 0]) +# A transit-switch delete is likewise reconciled incrementally in en_dp_enum +# (via the IC-SB datapath binding delete), not by a full re-enumeration. +AT_CHECK([test "$(ovn-appctl -t ic/ovn-ic \ + inc-engine/show-stats dp_enum 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 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
