Commit 6de8868d (reconnect: Fix broken inactivity probe if there is no
other reason to wake up.) enables poll_loop to run immediately on the
next millisecond when the probe interval has expired with no receive
attempted. Its reasoning notes that "in a correctly written application
we should not fall into this case more than once in a row".
ovsdb-server falls into it forever on a session whose peer stops
reading. It does not call jsonrpc_session_recv() while a session has a
send backlog, so reconnect_receive_attempted() is never called for it,
the probe expiration can never be passed, and the FSM asks for the
'now + 1' wake-up on every pass. The session is neither probed nor
disconnected, and every wake-up walks every connection, so the cost
grows with the session count: 10.5% of a core with the stalled session
alone, 21.9% alongside 50 healthy sessions, 42.3% alongside 300. This
has been observed in production, where ovn-octavia-provider clients
raised an exception but did not die, keeping their backlog frozen and
degrading the ovsdb-server service.
Let a jsonrpc_session user declare that it does not receive while there
is a backlog, and have ovsdb-server do so for its sessions. For such a
session, jsonrpc_session_run() records a receive attempt whenever a
backlog remains after trying to send it: that send attempt is the
session's look at the connection. The receive gate still prevents a
disconnection while ovsdb-server itself is busy, and stops producing the
'now + 1' wake-ups. A backlog that moves still counts as activity, so a
peer that keeps taking data is kept; one whose queue does not move for a
probe interval is probed and, with the probe queued behind the backlog,
disconnected one interval later.
Testing. A client reads an 8 MB monitor reply at a fixed rate and never
answers the probe, against the 5 s inactivity_probe default. Passes/s
is the mean (and peak) of voluntary_ctxt_switches from /proc/PID/status,
which poll_block() advances once per pass, sampled once a second over
72 s past the first probe interval.
unpatched patched
rate passes/s session passes/s session
tcp
0 kB/s 830.1 (933) kept 1.6 (7) dropped t+12 s
4 kB/s 667.1 (918) kept 0.3 (7) dropped t+27 s
20 kB/s 32.8 (832) kept 0.6 (7) kept
50 kB/s 0.6 (6) kept 0.6 (7) kept
ssl
0 kB/s 810.3 (906) kept 1.5 (7) dropped t+12 s
50 kB/s 796.7 (878) kept 1.4 (6) dropped t+12 s
225 kB/s 344.7 (883) read, t+37 s 1.5 (6) dropped t+12 s
500 kB/s 0.7 (7) read, t+22 s 1.5 (6) read, t+19 s *
kept: still connected when the 84 s run ends. read: read the
whole reply, then dropped for not answering the probe.
* 1 run in 3 dropped at the first timeout, t+10 s.
A real octavia-driver-agent (ovn-octavia-provider 2.0.0) over ssl,
stopped with SIGSTOP in the middle of a 25.6M-cell northbound dump: the
unpatched server runs at 823 passes/s and never drops the session; the
patched one runs at 1.9 passes/s and drops it at t+10.2 s.
Fixes: 6de8868d19ea ("reconnect: Fix broken inactivity probe if there is no
other reason to wake up.")
Suggested-by: Ilya Maximets <[email protected]>
Signed-off-by: Aeliton G. Silva <[email protected]>
Assisted-by: Claude Opus 5.5, Claude Code
---
Notes:
v3:
- Rework following review: instead of teaching the reconnect FSM
about queued bytes, a jsonrpc_session user declares that it does
not receive while there is a backlog
(jsonrpc_session_set_backlog_blocks_recv()), and such a session
records a receive attempt whenever a backlog remains after trying
to send it. reconnect.c and the Python library are unchanged.
- Squash v2 2/2 into this patch: the same rule now falls out of the
existing probe and timeout.
- Tests: the two ovsdb-server checks use ovsdb-client/block instead
of a new Python client, and also check that the stalled session is
disconnected.
lib/jsonrpc.c | 28 +++++++++++
lib/jsonrpc.h | 2 +
ovsdb/jsonrpc-server.c | 4 ++
tests/ovsdb-server.at | 110 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 144 insertions(+)
diff --git a/lib/jsonrpc.c b/lib/jsonrpc.c
index f01a2e56f..f80a9a947 100644
--- a/lib/jsonrpc.c
+++ b/lib/jsonrpc.c
@@ -833,6 +833,10 @@ struct jsonrpc_session {
/* Limits for jsonrpc. */
size_t max_n_msgs;
size_t max_backlog_bytes;
+
+ /* The user does not call jsonrpc_session_recv() while there is a send
+ * backlog. See jsonrpc_session_set_backlog_blocks_recv(). */
+ bool backlog_blocks_recv;
};
static void
@@ -887,6 +891,7 @@ jsonrpc_session_open_multiple(const struct svec *remotes,
bool retry)
s->seqno = 0;
s->dscp = 0;
s->last_error = 0;
+ s->backlog_blocks_recv = false;
jsonrpc_session_set_backlog_threshold(s, 0, 0);
@@ -934,6 +939,7 @@ jsonrpc_session_open_unreliably(struct jsonrpc *jsonrpc,
uint8_t dscp)
s->stream = NULL;
s->pstream = NULL;
s->seqno = 1;
+ s->backlog_blocks_recv = false;
jsonrpc_session_set_backlog_threshold(s, 0, 0);
return s;
@@ -1065,6 +1071,17 @@ jsonrpc_session_run(struct jsonrpc_session *s)
reconnect_activity(s->reconnect, time_msec());
}
+ if (s->backlog_blocks_recv && jsonrpc_get_backlog(s->rpc)) {
+ /* Our user will not try to receive until the backlog is sent, so
+ * the receive attempt the inactivity probe waits for would never
+ * happen, and the FSM would ask to be woken up every millisecond
+ * to make it. Trying to send the backlog is this session's look
+ * at the connection, so count it as one: a peer that takes
+ * nothing for a probe interval is probed and then disconnected,
+ * the same as a peer that sends nothing. */
+ reconnect_receive_attempted(s->reconnect, time_msec());
+ }
+
error = jsonrpc_get_status(s->rpc);
if (error) {
reconnect_disconnected(s->reconnect, time_msec(), error);
@@ -1355,6 +1372,17 @@ jsonrpc_session_set_options(struct jsonrpc_session *s,
jsonrpc_session_set_dscp(s, options->dscp);
}
+/* Declares whether a send backlog on 's' blocks its user from calling
+ * jsonrpc_session_recv(). A user that does not receive while 's' has a
+ * backlog should set 'backlog_blocks_recv' to true, so that a peer which stops
+ * taking data is still subject to the inactivity probe. */
+void
+jsonrpc_session_set_backlog_blocks_recv(struct jsonrpc_session *s,
+ bool backlog_blocks_recv)
+{
+ s->backlog_blocks_recv = backlog_blocks_recv;
+}
+
/* Sets thresholds for send backlog. If send backlog contains more than
* 'max_n_msgs' messages or is larger than 'max_backlog_bytes' bytes,
* connection will be closed (then reconnected, if that feature is enabled). */
diff --git a/lib/jsonrpc.h b/lib/jsonrpc.h
index 1baffcd80..f38ef8ca9 100644
--- a/lib/jsonrpc.h
+++ b/lib/jsonrpc.h
@@ -153,6 +153,8 @@ void jsonrpc_session_set_probe_interval(struct
jsonrpc_session *,
int probe_interval);
void jsonrpc_session_set_dscp(struct jsonrpc_session *,
uint8_t dscp);
+void jsonrpc_session_set_backlog_blocks_recv(struct jsonrpc_session *,
+ bool backlog_blocks_recv);
void jsonrpc_session_set_backlog_threshold(struct jsonrpc_session *,
size_t max_n_msgs,
size_t max_backlog_bytes);
diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
index 92e82228f..599d27c7f 100644
--- a/ovsdb/jsonrpc-server.c
+++ b/ovsdb/jsonrpc-server.c
@@ -617,6 +617,10 @@ ovsdb_jsonrpc_session_create(struct ovsdb_jsonrpc_remote
*remote,
s->js_seqno = jsonrpc_session_get_seqno(js);
s->read_only = read_only;
+ /* ovsdb_jsonrpc_session_run() does not receive while there is a
+ * backlog. */
+ jsonrpc_session_set_backlog_blocks_recv(js, true);
+
remote->server->n_sessions++;
return s;
diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at
index 7042b1fd3..a4b1a840b 100644
--- a/tests/ovsdb-server.at
+++ b/tests/ovsdb-server.at
@@ -3161,3 +3161,113 @@ TEST_CONFIG_FILE([complex config], [
}
}
], [0])
+
+AT_BANNER([OVSDB -- ovsdb-server peers that stop reading])
+
+dnl One field of ovsdb-server's memory/show, which omits a zero field.
+m4_define([MEMORY_SHOW_FIELD],
+ [ovs-appctl -t ovsdb-server memory/show dnl
+ | tr ' ' '\n' | sed -n 's/^$1:\(.*\)/\1/p'])
+
+dnl The cumulative poll_create_node count: one per fd the poll loop
+dnl registers, per pass, so its rate tracks the pass rate.
+m4_define([POLL_CREATE_NODE_TOTAL],
+ [ovs-appctl -t ovsdb-server coverage/show dnl
+ | awk '/^poll_create_node/ { for (i = 1; i <= NF; i++) dnl
+ if ($i == "total:") { print $(i + 1); exit } }'])
+
+# OVSDB_SERVER_CHECK_STALLED_PEER(PROTOCOL)
+#
+# Monitors a database over a PROTOCOL ("tcp" or "ssl") remote from an
+# ovsdb-client that then stops reading (ovsdb-client/block), and makes
+# ovsdb-server queue more for it than the sockets hold, so a backlog stands
+# in ovsdb-server's own queue. ovsdb-server does not receive on a session
+# with a backlog, so the inactivity probe, which waits for a receive
+# attempt, used to make the poll loop wake up every millisecond to make
+# one, forever, and the session was neither probed nor disconnected.
+#
+# Checks that the poll loop stays near its idle rate past the probe
+# interval, and that the stalled session is disconnected by the ordinary
+# inactivity probe. Over ssl the payload sits in the SSL layer and only
+# a few bytes stand in ovsdb-server's queue, which is the shape seen in
+# production.
+m4_define([OVSDB_SERVER_CHECK_STALLED_PEER],
+ [AT_SETUP([ovsdb-server drops a $1 peer that stops reading])
+ AT_KEYWORDS([ovsdb server positive $1 backlog probe])
+ m4_if([$1], [ssl],
+ [AT_SKIP_IF([test "$HAVE_OPENSSL" = no])
+ PKIDIR=$abs_top_builddir/tests
+ server_ssl="--private-key=$PKIDIR/testpki-privkey2.pem"
+ server_ssl="$server_ssl --certificate=$PKIDIR/testpki-cert2.pem"
+ server_ssl="$server_ssl --ca-cert=$PKIDIR/testpki-cacert.pem"
+ client_ssl="--private-key=$PKIDIR/testpki-privkey.pem"
+ client_ssl="$client_ssl --certificate=$PKIDIR/testpki-cert.pem"
+ client_ssl="$client_ssl --ca-cert=$PKIDIR/testpki-cacert.pem"],
+ [server_ssl= client_ssl=])
+ ordinal_schema > schema
+ AT_CHECK([ovsdb-tool create db schema], [0], [ignore], [ignore])
+ on_exit 'kill `cat *.pid`'
+ AT_CHECK([ovsdb-server --log-file --detach --no-chdir --pidfile dnl
+ $server_ssl --remote=p$1:0:127.0.0.1 db], [0], [ignore], [ignore])
+ PARSE_LISTENING_PORT([ovsdb-server.log], [PORT])
+ target=$1:127.0.0.1:$PORT
+
+ idle_before=`POLL_CREATE_NODE_TOTAL`
+ sleep 2
+ idle_after=`POLL_CREATE_NODE_TOTAL`
+ : ${idle_before:=0} ${idle_after:=0}
+ idle_rate=`expr $idle_after - $idle_before`
+ idle_rate=`expr $idle_rate / 2`
+
+ dnl The monitor detaches once it has the (empty) initial contents, and
+ dnl from here on reads nothing at all.
+ AT_CHECK([ovsdb-client --detach --no-chdir --pidfile $client_ssl dnl
+ monitor $target ordinals ordinals > monitor.out], [0], [ignore],
+ [ignore])
+ AT_CHECK([ovs-appctl -t ovsdb-client ovsdb-client/block], [0], [ignore])
+
+ dnl Insert 60 kB rows until updates for the blocked monitor stand in
+ dnl ovsdb-server's queue. Stop there: the probe interval runs from the
+ dnl last time that queue moved.
+ name=`printf '%060000d' 0`
+ for i in `seq 1 500`; do
+ ovsdb-client $client_ssl transact $target dnl
+ "@<:@\"ordinals\", {\"op\": \"insert\", \"table\": \"ordinals\", dnl
+ \"row\": {\"number\": $i, \"name\": \"$name\"}}@:>@" > /dev/null
+ backlog=`MEMORY_SHOW_FIELD([backlog])`
+ test -n "$backlog" && break
+ done
+
+ dnl Hard preconditions, not assumptions: without a standing backlog the
+ dnl checks below would pass on a server that still spins, and with any
+ dnl other session the disconnect below might not be the stalled one.
+ OVS_WAIT_UNTIL([test "`MEMORY_SHOW_FIELD([backlog])`" -gt 0])
+ OVS_WAIT_UNTIL([test "`MEMORY_SHOW_FIELD([sessions])`" = 1])
+
+ dnl Past the 5 s default probe interval of a command-line remote.
+ sleep 6
+ stalled_before=`POLL_CREATE_NODE_TOTAL`
+ sleep 2
+ stalled_after=`POLL_CREATE_NODE_TOTAL`
+ : ${stalled_before:=0} ${stalled_after:=0}
+ stalled_rate=`expr $stalled_after - $stalled_before`
+ stalled_rate=`expr $stalled_rate / 2`
+
+ dnl Before the fix this measured thousands per second against an idle
+ dnl rate of about 10. Allow ten times the idle rate.
+ limit=`expr $idle_rate + 10`
+ limit=`expr $limit \* 10`
+ echo "poll_create_node/s: idle $idle_rate, stalled $stalled_rate, dnl
+limit $limit"
+ AT_CHECK([test $stalled_rate -lt $limit])
+
+ dnl Nothing reaches the peer for a probe interval, so it is probed, and
+ dnl the probe cannot get past the backlog either, so it is disconnected
+ dnl one interval later.
+ OVS_WAIT_UNTIL([grep -q 'no response to inactivity probe' ovsdb-server.log])
+ OVS_WAIT_UNTIL([test -z "`MEMORY_SHOW_FIELD([sessions])`"])
+ OVSDB_SERVER_SHUTDOWN(["/no response to inactivity probe/d"])
+ AT_CLEANUP])
+
+OVSDB_SERVER_CHECK_STALLED_PEER([tcp])
+OVSDB_SERVER_CHECK_STALLED_PEER([ssl])
--
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
