On 15.09.2026 23:38, Dumitru Ceara wrote:
> On 9/11/26 12:43 PM, Alexandra Rukomoinikova via dev wrote:
>> The "ovn_ic_sb" lock is requested by every non-paused ovn-ic instance,
>> regardless of whether it holds the local AZ "ovn_ic" lock. With several
>> ovn-ic instances per AZ, a standby one can win the race for it.
>>
>> That deadlocks IC-SB writes: the standby holds the lock but never reaches
>> inc_proc_ic_run(), which is gated on the local SB lock, while the active
>> instances run ts_run() with is_az_leader() false and skip datapath
>> creation and deletion entirely.
>>
>> Request the lock only while holding the local SB lock, and drop the
>> request when that lock is lost. Also report ovn-ic-sb lock in
>> ovn-appctl.
>>
>> Fixes: 052a298bb90e ("ovn-ic: Use dual IC-SB connections to prevent
>> constraint violations.")
>> Signed-off-by: Alexandra Rukomoinikova <[email protected]>
>> ---
> Hi Alexandra,
>
> Thanks for the fix! There's one issue though, please see below.
>
>> ic/ovn-ic.c | 27 ++++++++++++------
>> tests/ovn-ic.at | 73 +++++++++++++++++++++++++++++++++++++++++++++----
>> 2 files changed, 87 insertions(+), 13 deletions(-)
>>
>> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
>> index be03d1781..16682ccbe 100644
>> --- a/ic/ovn-ic.c
>> +++ b/ic/ovn-ic.c
>> @@ -4375,14 +4375,23 @@ main(int argc, char *argv[])
>> ovsdb_idl_set_lock(ovnsb_idl_loop.idl, "ovn_ic");
>> }
>>
>> - if (!ovsdb_idl_has_lock(ovnisb_idl_loop.idl) &&
>> - !ovsdb_idl_is_lock_contended(ovnisb_idl_loop.idl)) {
>> - /*
>> - * Ensure that only a single ovn-ic has the permission to
>> - * write to IC-SB.
>> - */
>> - VLOG_INFO("Acquiring OVN ISB lock.");
>> - ovsdb_idl_set_lock(ovnisb_idl_loop.idl, "ovn_ic_sb");
>> + if (ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
>> + if (!ovsdb_idl_has_lock(ovnisb_idl_loop.idl) &&
>> + !ovsdb_idl_is_lock_contended(ovnisb_idl_loop.idl)) {
>> + /*
>> + * Ensure that only a single ovn-ic has the permission
>> to
>> + * write to IC-SB. Only the instance that is active in
>> + * its own AZ may request it: a standby instance never
>> + * runs the engine, so if it held the IC-SB lock nobody
>> + * would ever write to IC-SB.
>> + */
>> + VLOG_INFO("Acquiring OVN ISB lock.");
>> + ovsdb_idl_set_lock(ovnisb_idl_loop.idl, "ovn_ic_sb");
>> + }
>> + } else {
>> + /* Release the IC-SB lock, or withdraw a pending request for
>> + * it, as soon as the local SB lock is gone. */
>> + ovsdb_idl_set_lock(ovnisb_idl_loop.idl, NULL);
>> }
>>
>> struct ovsdb_idl_txn *ovnnb_txn =
>> @@ -4720,6 +4729,8 @@ ovn_ic_status(struct unixctl_conn *conn, int argc
>> OVS_UNUSED,
>> */
>> struct ds s = DS_EMPTY_INITIALIZER;
>> ds_put_format(&s, "Status: %s\n", status);
>> + ds_put_format(&s, "IC-SB lock: %s\n",
>> + state->had_isb_lock ? "held" : "not held");
>> unixctl_command_reply(conn, ds_cstr(&s));
>> ds_destroy(&s);
>> }
>> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
>> index c9e07250b..68b981925 100644
>> --- a/tests/ovn-ic.at
>> +++ b/tests/ovn-ic.at
>> @@ -6152,13 +6152,13 @@ check_column "ts1 ts2 ts3" ic-sb:Datapath_Binding
>> transit_switch
>> as az2 check ovn-appctl -t ic/ovn-ic resume
>>
>> # Now do the same on a "slow" ovn-ic-sb
>> -OVS_WAIT_UNTIL([test "x$(as az2 ovn-appctl -t ic/ovn-ic status)" =
>> "xStatus: active"])
>> +OVS_WAIT_UNTIL([test "x$(as az2 ovn-appctl -t ic/ovn-ic status | grep
>> "Status")" = "xStatus: active"])
>>
>> AS_BOX([$(date +%H:%M:%S.%03N) Pausing ovn-ic-sb])
>> AT_CHECK([kill -STOP $(cat ovn-ic-sb/ovsdb-server.pid)])
>>
>> as az1 check ovn-appctl -t ic/ovn-ic pause
>> -OVS_WAIT_UNTIL([test "x$(as az1 ovn-appctl -t ic/ovn-ic status)" =
>> "xStatus: paused"])
>> +OVS_WAIT_UNTIL([test "x$(as az1 ovn-appctl -t ic/ovn-ic status | grep
>> "Status")" = "xStatus: paused"])
>>
>> check ovn-ic-nbctl ts-add ts4
>>
>> @@ -6353,21 +6353,21 @@ net_add n1
>>
>> check ovn-ic-nbctl ts-add ts1
>> ovn_start az1
>> -OVS_WAIT_UNTIL([test "x$(as az1 ovn-appctl -t ic/ovn-ic status)" =
>> "xStatus: active"])
>> +OVS_WAIT_UNTIL([test "x$(as az1 ovn-appctl -t ic/ovn-ic status | grep
>> "Status")" = "xStatus: active"])
>> OVS_WAIT_UNTIL([grep -q "OVN ISB lock acquired" az1/ic/ovn-ic.log])
>>
>> ovn_start az2
>>
>> AS_BOX([az2 paused])
>> check as az2 ovn-appctl -t ic/ovn-ic pause
>> -OVS_WAIT_UNTIL([test "x$(as az2 ovn-appctl -t ic/ovn-ic status)" =
>> "xStatus: paused"])
>> +OVS_WAIT_UNTIL([test "x$(as az2 ovn-appctl -t ic/ovn-ic status | grep
>> "Status")" = "xStatus: paused"])
>> # Wait for az1 receiving "locked" notification.
>> OVS_WAIT_UNTIL([grep -q 'send notification, method="locked"'
>> ovn-ic-sb/ovsdb-server.log])
>> n1_lock_notif=$(grep -c 'send notification, method="locked"'
>> ovn-ic-sb/ovsdb-server.log)
>>
>> AS_BOX([az1 paused])
>> check as az1 ovn-appctl -t ic/ovn-ic pause
>> -OVS_WAIT_UNTIL([test "x$(as az1 ovn-appctl -t ic/ovn-ic status)" =
>> "xStatus: paused"])
>> +OVS_WAIT_UNTIL([test "x$(as az1 ovn-appctl -t ic/ovn-ic status | | grep
>> "Status")" = "xStatus: paused"])
> There's a typo here "| | grep" (spotted by my AI helper) causing:
>
> /workspace/ovn-tmp/tests/testsuite.dir/at-groups/1293/test-source: line
> 73: syntax error near unexpected token `|'
> /workspace/ovn-tmp/tests/testsuite.dir/at-groups/1293/test-source: line
> 73: ` test "x$(as az1 ovn-appctl -t ic/ovn-ic status | | grep
> "Status")" = "xStatus: paused"'
>
> Yet CI was green on this patch which means we're doing something wrong
> when running our tests in GitHub CI. I'll dig into that separately.
>
> Now if I fix this we fail just below in the "ovn-ic - pause" test.
>
> Most likely the reason is because after we manage to take the SB lock we
> need a new ovn-ic run before we can try to take the ISB lock. But if
> there are no external changes nothing will trigger that run.
>
> The same AI helper suggests adding a poll_immediate_wake() as soon as we
> successfully take the SB lock. That seems reasonable to me, what do you
> think?
Hi Dumitru! Thanks for the review.
Maybe we should request the ovn_ic_sb lock after receiving an update
about ovn_iс lock—that is, after the Southbound database's run_idl_loop
call ?
>
> Regards,
> Dumitru
>
>> n2_lock_notif=$(grep -c 'send notification, method="locked"'
>> ovn-ic-sb/ovsdb-server.log)
>>
>> # Pausing az1 should not cause az2 to own the lock: az2 is paused.
>> @@ -6387,3 +6387,66 @@ OVN_CLEANUP_IC([az1], [az2])
>> AT_CLEANUP
>> ])
>>
>> +OVN_FOR_EACH_NORTHD([
>> +AT_SETUP([ovn-ic - IC-SB lock is only requested by the active instance])
>> +ovn_init_ic_db
>> +
>> +ovn_start az1
>> +ovn_start az2
>> +wait_row_count ic-sb:Availability_Zone 2
>> +
>> +check ovn-ic-nbctl ts-add ts1
>> +wait_row_count ic-sb:Datapath_Binding 1 transit_switch=ts1
>> +
>> +# Start a second ovn-ic in az1. It can't get the SB lock, so it must stay
>> +# standby and must not compete for the IC-SB lock either.
>> +ovn_as az1
>> +mkdir az1/ic2
>> +as az1/ic2 start_daemon ovn-ic -v \
>> + --ovnnb-db=$OVN_NB_DB --ovnsb-db=$OVN_SB_DB \
>> + --ic-nb-db=unix:"$ovs_base"/ovn-ic-nb/ovn-ic-nb.sock \
>> + --ic-sb-db=unix:"$ovs_base"/ovn-ic-sb/ovn-ic-sb.sock
>> +OVS_WAIT_UNTIL([as az1 ovn-appctl -t ic2/ovn-ic status | grep -q "^Status:
>> standby$"])
>> +OVS_WAIT_UNTIL([as az1 ovn-appctl -t ic2/ovn-ic ic-sb-connection-status |
>> grep -q "^connected$"])
>> +AT_CHECK([as az1 ovn-appctl -t ic2/ovn-ic status | grep -q "^IC-SB lock:
>> not held$"])
>> +
>> +# Simulate an IC-SB failover: freeze both active instances, drop every
>> +# IC-SB connection and let the standby reconnect first. A standby that
>> +# competes for the IC-SB lock wins it here and, never running the engine,
>> +# blocks IC-SB datapath creation for everyone.
>> +AS_BOX([Dropping IC-SB connections with the active instances frozen])
>> +AT_CHECK([kill -STOP $(cat az1/ic/ovn-ic.pid) $(cat az2/ic/ovn-ic.pid)])
>> +check as ovn-ic-sb ovs-appctl -t ovsdb-server ovsdb-server/reconnect
>> +OVS_WAIT_UNTIL([as az1 ovn-appctl -t ic2/ovn-ic ic-sb-connection-status |
>> grep -q "^not connected$"])
>> +OVS_WAIT_UNTIL([as az1 ovn-appctl -t ic2/ovn-ic ic-sb-connection-status |
>> grep -q "^connected$"])
>> +AT_CHECK([kill -CONT $(cat az1/ic/ovn-ic.pid) $(cat az2/ic/ovn-ic.pid)])
>> +
>> +check ovn-ic-nbctl ts-add ts2
>> +wait_row_count ic-sb:Datapath_Binding 1 transit_switch=ts2
>> +check_column "ts1 ts2" ic-sb:Datapath_Binding transit_switch
>> +
>> +AT_CHECK([as az1 ovn-appctl -t ic2/ovn-ic status | grep -q "^Status:
>> standby$"])
>> +AT_CHECK([as az1 ovn-appctl -t ic2/ovn-ic status | grep -q "^IC-SB lock:
>> not held$"])
>> +AT_CHECK([grep -q "OVN ISB lock acquired" az1/ic2/ovn-ic.log], [1])
>> +
>> +# Once the standby becomes active in its AZ it should take the IC-SB lock.
>> +AS_BOX([Pausing both original instances])
>> +check as az1 ovn-appctl -t ic/ovn-ic pause
>> +check as az2 ovn-appctl -t ic/ovn-ic pause
>> +OVS_WAIT_UNTIL([as az1 ovn-appctl -t ic2/ovn-ic status | grep -q "^Status:
>> active$"])
>> +OVS_WAIT_UNTIL([as az1 ovn-appctl -t ic2/ovn-ic status | grep -q "^IC-SB
>> lock: held$"])
>> +
>> +check ovn-ic-nbctl ts-add ts3
>> +wait_row_count ic-sb:Datapath_Binding 1 transit_switch=ts3
>> +check_column "ts1 ts2 ts3" ic-sb:Datapath_Binding transit_switch
>> +
>> +check as az1 ovn-appctl -t ic/ovn-ic resume
>> +check as az2 ovn-appctl -t ic/ovn-ic resume
>> +
>> +as az1/ic2
>> +OVS_APP_EXIT_AND_WAIT([ovn-ic])
>> +
>> +OVN_CLEANUP_IC([az1], [az2])
>> +
>> +AT_CLEANUP
>> +])
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
