On 9/16/26 1:41 PM, Rukomoinikova Aleksandra wrote:
> 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.
>
Hi Alexandra,
> 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 ?
>
I didn't test it but maybe you're right, that seems OK.
Regards,
Dumitru
>>
>> 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
