Hi Lucas! This isn't really a full review, but I pointed out two things
I noticed below.
On 10/27/26 1:21 PM, Lucas Vargas Dias via dev wrote:
> If two AZs connect ports with the same name to a transit switch,
> ovn-ic retries a failing transaction forever. Port names are unique
> in both the NB (Logical_Switch_Port) and the IC-SB (Port_Binding):
> one AZ tries to create a remote LSP that clashes with its local one,
> and the other AZ tries to bind a port name that is already bound.
> The failed commit also blocks every other change in the same
> transaction.
>
> Check the name before inserting. On a conflict, skip the port and
> log a rate-limited warning that names the port and AZs, so the
> operator knows which port to rename.
>
> Fixes: aa3e4436c232 ("ovn-ic: Interconnection port controller.")
> Assisted-by: Claude Opus 5.5, Claude Code
> Signed-off-by: Lucas Vargas Dias <[email protected]>
> ---
> ic/en-port-binding.c | 75 +++++++++++++++++++++++++++++++++++++++-----
> ic/ovn-ic.c | 5 +++
> ic/ovn-ic.h | 1 +
> tests/ovn-ic.at | 54 +++++++++++++++++++++++++++++++
> 4 files changed, 127 insertions(+), 8 deletions(-)
>
> diff --git a/ic/en-port-binding.c b/ic/en-port-binding.c
> index 5a2490b6e..638f819e0 100644
> --- a/ic/en-port-binding.c
> +++ b/ic/en-port-binding.c
> @@ -465,6 +465,28 @@ create_nb_lsp(struct ic_context *ctx,
> const struct icsbrec_port_binding *isb_pb,
> const struct nbrec_logical_switch *ls)
> {
> + /* The NB Logical_Switch_Port name is unique across the whole NB, but a
> + * remote AZ may bind a port whose name is already used here, e.g. by a
> + * local port with the same name on this transit switch or by a port of
> + * an unrelated logical switch. Inserting it would violate the name
> + * index, abort the whole NB transaction and retry it forever, so skip
> the
> + * remote port instead. A row inserted earlier in this same transaction
> + * (see lsp_create()) is also found here and is silently reused. */
> + const struct nbrec_logical_switch_port *existing =
> + get_lsp_by_ts_port_name(ctx, isb_pb->logical_port);
> + if (existing) {
> + if (!nbrec_logical_switch_port_is_new(existing)) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> + VLOG_WARN_RL(&rl, "Port %s of transit switch %s in availability "
> + "zone %s conflicts with an existing logical switch "
> + "port with the same name, skipping it.",
> + isb_pb->logical_port, isb_pb->transit_switch,
> + isb_pb->availability_zone
> + ? isb_pb->availability_zone->name : "<unknown>");
> + }
> + return;
> + }
> +
> const struct nbrec_logical_switch_port *lsp =
> nbrec_logical_switch_port_insert(ctx->ovnnb_txn);
> nbrec_logical_switch_port_set_name(lsp, isb_pb->logical_port);
> @@ -521,11 +543,45 @@ find_transit_router_by_name(struct ic_context *ctx,
> const char *name)
> }
>
> static const struct icsbrec_port_binding *
> -create_isb_pb(struct ovsdb_idl_txn *isb_txn, const char *logical_port,
> +find_isb_pb_by_name(struct ic_context *ctx, const char *logical_port)
> +{
> + const struct icsbrec_port_binding *key =
> + icsbrec_port_binding_index_init_row(
> + ctx->icsbrec_port_binding_by_name);
> + icsbrec_port_binding_index_set_logical_port(key, logical_port);
> + const struct icsbrec_port_binding *isb_pb =
> + icsbrec_port_binding_index_find(ctx->icsbrec_port_binding_by_name,
> + key);
> + icsbrec_port_binding_index_destroy_row(key);
> + return isb_pb;
> +}
> +
> +static const struct icsbrec_port_binding *
> +create_isb_pb(struct ic_context *ctx, struct ovsdb_idl_txn *isb_txn,
Nit: Since create_isb_pb() already accepted ctx->ovnisb_txn and now you
added ctx as a parameter, you could just grab ctx->ovnisb_txn inside of
the function instead of having an extra redundant argument.
> + const char *logical_port,
> const struct icsbrec_availability_zone *az, const char
> *ts_name,
> const struct uuid *nb_ic_uuid, const char *type,
> struct hmap *pb_tnlids)
> {
> + /* The IC-SB Port_Binding logical_port is unique across all AZs and
> + * transit switches/routers. If another AZ (or another transit
> + * switch/router) already binds a port with this name, inserting would
> + * violate the index, abort the whole IC-SB transaction and retry it
> + * forever, so leave this port unbound instead. */
> + const struct icsbrec_port_binding *existing =
> + find_isb_pb_by_name(ctx, logical_port);
> + if (existing) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> + VLOG_WARN_RL(&rl, "Port %s of %s in availability zone %s conflicts "
> + "with a port with the same name of %s in availability "
> + "zone %s, not binding it.",
> + logical_port, ts_name, az->name,
> + existing->transit_switch,
> + existing->availability_zone
> + ? existing->availability_zone->name : "<unknown>");
> + return NULL;
> + }
> +
> uint32_t pb_tnl_key = allocate_port_key(pb_tnlids);
> if (!pb_tnl_key) {
> return NULL;
> @@ -720,7 +776,7 @@ port_binding_sync_ts(struct ic_context *ctx,
> if (ctx->ovnisb_txn && is_az_leader(ctx->ovnisb_txn)) {
> if (!isb_pb) {
> isb_pb = create_isb_pb(
> - ctx->ovnisb_txn, tsp->name, runned_az,
> + ctx, ctx->ovnisb_txn, tsp->name, runned_az,
> ts->name, &ts->header_.uuid, "transit-switch-port",
> &pb_tnlids);
> }
> @@ -730,8 +786,8 @@ port_binding_sync_ts(struct ic_context *ctx,
> /* Create ISB port_binding as its chassis is local. */
> isb_pb = shash_find_and_delete(&local_pbs, tsp->name);
> if (!isb_pb) {
> - isb_pb = create_isb_pb(ctx->ovnisb_unlocked_txn, tsp->name,
> - runned_az,
> + isb_pb = create_isb_pb(ctx, ctx->ovnisb_unlocked_txn,
> + tsp->name, runned_az,
> ts->name, &ts->header_.uuid,
> "transit-switch-port", &pb_tnlids);
> }
> @@ -773,9 +829,12 @@ port_binding_sync_ts(struct ic_context *ctx,
> isb_pb = shash_find_and_delete(&local_pbs, lsp->name);
> if (!isb_pb) {
> isb_pb = create_isb_pb(
> - ctx->ovnisb_unlocked_txn, sb_pb->logical_port,
> + ctx, ctx->ovnisb_unlocked_txn, sb_pb->logical_port,
> runned_az, ts->name, &ts->header_.uuid,
> "transit-switch-port", &pb_tnlids);
> + if (!isb_pb) {
> + continue;
I'm curious why you added a check for isb_pb being NULL here but not at
any of the other call sites edited in this patch.
> + }
> sync_ts_isb_pb(ctx, sb_pb, isb_pb);
> } else {
> sync_local_port(ctx, isb_pb, sb_pb, lsp);
> @@ -899,7 +958,7 @@ port_binding_sync_tr(struct ic_context *ctx,
>
> if (ctx->ovnisb_txn && is_az_leader(ctx->ovnisb_txn)) {
> if (!isb_pb) {
> - isb_pb = create_isb_pb(ctx->ovnisb_txn, trp->name,
> + isb_pb = create_isb_pb(ctx, ctx->ovnisb_txn, trp->name,
> runned_az,
> tr->name, &tr->header_.uuid,
> "transit-router-port",
> @@ -912,8 +971,8 @@ port_binding_sync_tr(struct ic_context *ctx,
> } else {
> isb_pb = shash_find_and_delete(&local_pbs, trp->name);
> if (!isb_pb) {
> - isb_pb = create_isb_pb(ctx->ovnisb_unlocked_txn, trp->name,
> - runned_az,
> + isb_pb = create_isb_pb(ctx, ctx->ovnisb_unlocked_txn,
> + trp->name, runned_az,
> tr->name, &tr->header_.uuid,
> "transit-router-port", &pb_tnlids);
> }
> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> index e65256577..87a266b44 100644
> --- a/ic/ovn-ic.c
> +++ b/ic/ovn-ic.c
> @@ -1191,6 +1191,10 @@ main(int argc, char *argv[])
> = ovsdb_idl_index_create1(ovnisb_unlocked_idl_loop.idl,
>
> &icsbrec_port_binding_col_availability_zone);
>
> + struct ovsdb_idl_index *icsbrec_port_binding_by_name
> + = ovsdb_idl_index_create1(ovnisb_unlocked_idl_loop.idl,
> + &icsbrec_port_binding_col_logical_port);
> +
> struct ovsdb_idl_index *icsbrec_port_binding_by_ts
> = ovsdb_idl_index_create1(ovnisb_unlocked_idl_loop.idl,
> &icsbrec_port_binding_col_transit_switch);
> @@ -1408,6 +1412,7 @@ main(int argc, char *argv[])
> .icnbrec_transit_router_by_name =
> icnbrec_transit_router_by_name,
> .icsbrec_port_binding_by_az = icsbrec_port_binding_by_az,
> + .icsbrec_port_binding_by_name = icsbrec_port_binding_by_name,
> .icsbrec_port_binding_by_ts = icsbrec_port_binding_by_ts,
> .icsbrec_port_binding_by_ts_az =
> icsbrec_port_binding_by_ts_az,
> .icsbrec_route_by_az = icsbrec_route_by_az,
> diff --git a/ic/ovn-ic.h b/ic/ovn-ic.h
> index 8adb17ad7..b1cfad08f 100644
> --- a/ic/ovn-ic.h
> +++ b/ic/ovn-ic.h
> @@ -43,6 +43,7 @@ struct ic_context {
> struct ovsdb_idl_index *icnbrec_transit_switch_by_name;
> struct ovsdb_idl_index *icnbrec_transit_router_by_name;
> struct ovsdb_idl_index *icsbrec_port_binding_by_az;
> + struct ovsdb_idl_index *icsbrec_port_binding_by_name;
> struct ovsdb_idl_index *icsbrec_port_binding_by_ts;
> struct ovsdb_idl_index *icsbrec_port_binding_by_ts_az;
> struct ovsdb_idl_index *icsbrec_route_by_az;
> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
> index 646026817..4576c725e 100644
> --- a/tests/ovn-ic.at
> +++ b/tests/ovn-ic.at
> @@ -1689,6 +1689,60 @@ OVN_CLEANUP_IC([az1], [az2])
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn-ic -- same port name in two AZs])
> +
> +ovn_init_ic_db
> +ovn-ic-nbctl ts-add ts1
> +ovn_start az1
> +ovn_start az2
> +check ovn-ic-nbctl --wait=sb sync
> +
> +dnl Each AZ connects a router to ts1 through a legacy port that has the
> +dnl same name in both AZs. Pause ovn-ic so that neither AZ learns about the
> +dnl other's port before creating its own.
> +for az in az1 az2; do
> + as $az check ovn-appctl -t ic/ovn-ic pause
> +done
> +for az in az1 az2; do
> + ovn_as $az
> + check ovn-nbctl lr-add lr1
> + check ovn-nbctl lrp-add lr1 lrp-lr1-ts1 aa:aa:aa:aa:aa:01
> 169.254.100.1/24
> + check ovn-nbctl --wait=sb lsp-add-router-port ts1 lsp-dup lrp-lr1-ts1
> +done
> +for az in az1 az2; do
> + as $az check ovn-appctl -t ic/ovn-ic resume
> +done
> +
> +dnl IC-SB port names are unique, so only one AZ gets to bind the port; the
> +dnl other one must give up instead of retrying the insert forever.
> +wait_row_count ic-sb:Port_Binding 1 logical_port=lsp-dup
> +OVS_WAIT_UNTIL([grep -q "Port lsp-dup of ts1 .* conflicts with a port" \
> + az1/ic/ovn-ic.log az2/ic/ovn-ic.log])
> +
> +dnl And the AZ that got the binding must not try to create a remote NB port
> +dnl over the other AZ's local port with the same name.
> +OVS_WAIT_UNTIL([grep -q "Port lsp-dup of transit switch ts1 .* conflicts" \
> + az1/ic/ovn-ic.log az2/ic/ovn-ic.log])
> +for az in az1 az2; do
> + ovn_as $az
> + check_row_count nb:Logical_Switch_Port 1 name=lsp-dup type=router
> +done
> +
> +dnl ovn-ic must keep committing NB transactions: a port added in az1 shows
> +dnl up as remote in az2.
> +ovn_as az1
> +check ovn-nbctl lrp-add lr1 lrp-lr1-ts1-2 aa:aa:aa:aa:aa:02 169.254.101.1/24
> +check ovn-nbctl --wait=sb lsp-add-router-port ts1 lsp-az1-only lrp-lr1-ts1-2
> +ovn_as az2
> +wait_row_count nb:Logical_Switch_Port 1 name=lsp-az1-only type=remote
> +
> +AT_CHECK([grep -q "constraint violation" az1/ic/ovn-ic.log
> az2/ic/ovn-ic.log], [1])
> +
> +OVN_CLEANUP_IC([az1], [az2])
> +AT_CLEANUP
> +])
> +
> OVN_FOR_EACH_NORTHD([
> AT_SETUP([ovn-ic -- address-set sync])
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
