Hi,
Thanks for your review.
Em qua., 28 de out. de 2026 às 16:16, Rosemarie O'Riorden via dev <
[email protected]> escreveu:
> 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.
>
Some calls for create_isb_pb use ctx->ovnisb_unlocked_txn
instead ctx->ovnisb_txn.
>
> > + 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.
>
create_isb_pb() could already return NULL before this patch, when no
tunnel key is left. The other call sites already handle that:
sync_tsp_pb() and sync_trp_pb() return early on NULL, and the callers
then skip the port ("if (!isb_pb) continue;" / "if (isb_pb)"). The
legacy path was the only one that dereferences the result right away
(sync_ts_isb_pb() and isb_pb->type), so it was the only one that needed
a check.
Regards,
Lucas
> > + }
> > 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
>
--
_‘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
