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?
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
