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,
+ 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;
+ }
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])
--
2.43.0
--
_'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
