[nexus] tune libpq keepalive to catch dead CockroachDB connections fa…#10744
Closed
jam-mad wants to merge 1 commit into
Closed
[nexus] tune libpq keepalive to catch dead CockroachDB connections fa…#10744jam-mad wants to merge 1 commit into
jam-mad wants to merge 1 commit into
Conversation
…ster
Nexus used libpq's default TCP keepalive settings. On illumos, a dead
connection could go undetected for about 2 hours before the OS even
sent a probe, then another 8 minutes before giving up.
davepacheco tracked this down in a real incident: a Nexus instance
hung a rack update for 45+ minutes holding two dead connections to a
CockroachDB node that had reset. The cause: keepalives_idle,
keepalives_interval, and keepalives_count were never set, so Nexus
just used the OS defaults.
This sets those four parameters in make_postgres_connector()
(nexus/db-queries/src/db/pool.rs), cutting worst-case detection time
to about 2 minutes:
keepalives=1
keepalives_idle=10
keepalives_interval=10
keepalives_count=12
The connection args list already existed
(previously just sslmode=disable) and already flows through
DieselPgConnector::to_url() into libpq.
Added two tests in nexus/db-queries/src/db/pool_connection.rs (had no
coverage before):
- to_url_includes_keepalive_args: checks the connection string has the
new parameters.
- connection_actually_gets_tcp_keepalive_settings: opens a real
connection and reads back the actual kernel TCP settings, the
automated version of the mdb -k check davepacheco did by hand. Works
on Linux, macOS, and illumos via socket2's keepalive accessors, which
handle each OS's different option names. Only run on Linux so far.
Also re-ran the existing db::pool::test tests against a real
CockroachDB. Both still pass.
These timing values are a starting point, not final. Worth a second
look before merging.
Closes oxidecomputer#10668
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tune libpq keepalive so dead CockroachDB connections are detected in minutes, not hours
Fixes #10668.
Closes #10668
Summary
Nexus's connections to CockroachDB use libpq's default TCP keepalive
settings, which on illumos means a dead connection can go undetected for
about 2 hours before the OS even sends a probe, then another 8 minutes
before it gives up. This adds four keepalive parameters to the existing
connection args in
make_postgres_connector(), cutting that detectionwindow to roughly 2 minutes.
All of the diagnostic work here is @davepacheco's, from a real incident
where this hung a rack update for 45+ minutes. This PR implements the
fix he identified and adds test coverage for it, including an automated
test that opens a real connection and reads back the actual kernel TCP
keepalive settings, the same check davepacheco did by hand with
mdb -kduring the original investigation. It works on Linux, macOS, and
illumos.
The problem
During a self-service rack update, one Nexus instance hung during
handoff, holding two CockroachDB connections for 47+ minutes with zero
sagas running.
omdb nexus quiesce show --stacksshowed both threadsblocked inside libpq's
PQgetResult, waiting on a socket read that wasnever going to complete.
davepacheco tracked this down (full details in #10668).
The change
make_postgres_connector()innexus/db-queries/src/db/pool.rsalreadybuilds a list of extra libpq connection parameters (previously just
sslmode=disable). That list gets appended as query parameters onto thePostgres connection URL in
DieselPgConnector::to_url()(
nexus/db-queries/src/db/pool_connection.rs), which Diesel handsstraight to libpq. Nothing about that plumbing needed to change; this
just adds four more entries to the same list:
keepalives_idle=10sends the first probe 10 seconds after theconnection goes quiet.
keepalives_interval=10andkeepalives_count=12give up after 120 more seconds of unanswered probes. Total worst-case
detection time: about 2 minutes, instead of roughly 2 hours.
These numbers are a starting point in the same ballpark davepacheco
suggested (10 seconds to first probe, 120 seconds to abort), not a final
answer. Worth a second opinion before merging, since this is a production
reliability configuration. If it's too aggressive it risks tearing down connections
during a legitimate CockroachDB pause, too lax doesn't fix the problem.
Testing
nexus/db-queries/src/db/pool_connection.rshad no test coverage at allon
DieselPgConnector's URL-building logic before this. Added twotests, in increasing order of how much they actually prove:
to_url_includes_keepalive_args: builds a connector with the new argsand asserts the resulting connection string contains all four
keepalive parameters. Fast, offline, no database needed. This only
proves we build the right string.
connection_actually_gets_tcp_keepalive_settings: opens a realconnection to a real (test) CockroachDB with that exact connection
string, then reads back the TCP socket's actual kernel keepalive
settings and asserts they match what we configured. This is the
automated version of the
mdb -kcheck davepacheco did on alive connection while diagnosing the original incident.
Diesel's
PgConnectionhides the underlying socket, so the test can'tinspect the connector's connection directly. It opens a second,
plain libpq connection with the same connection string instead. libpq
is what actually applies these settings via
setsockopt, so thischecks real behavior. It gets the raw socket via libpq's own
PQsocket()(a supported, public part of libpq's C API),
dup()s the fd so nothingdouble-closes it, and reads it back with
socket2.This works on Linux, macOS, and illumos (production?) with no
platform-specific code.
socket2'skeepalive_time/keepalive_interval/keepalive_retriesalready handle each OS'sdifferent option names. For example, macOS reports
TCP_KEEPIDLEasTCP_KEEPALIVE. illumos has its own olderTCP_KEEPALIVE_THRESHOLD,but also supports the same
TCP_KEEPIDLE/TCP_KEEPINTVL/TCP_KEEPCNTnames Linux uses (confirmed by checking illumos's actualconstants in the
libccrate). I've only run this on Linux myself;illumos should work the same way, gated by
socket2's owncfg(target_os = "illumos"), but I haven't confirmed it theredirectly.
Also ran the pre-existing
db::pool::testtests(
test_pool_can_be_terminated,test_pool_drop_does_not_panic), whichconnect to a real ephemeral CockroachDB instance. Both still pass with
the new connection args in place.
New dev-dependencies:
socket2(already a workspace dependency with theallfeature enabled, just not previously used by this crate) andlibc(also already workspace-managed).pq-syswas already a directdependency of this crate. Nothing new added to the crates.io dependency
graph; this just wires up existing workspace dependencies for the first
time here, and only in test code.
How to re-run
Risk / non-goals
(
pool.rs,pool_connection.rs), plus aCargo.tomlupdate to wireup two already-workspace-managed dependencies for the first time in
this crate. The connection-string code already existed. This only adds
parameters to an existing list.
work unmodified on illumos given
socket2's own platform gatingincludes it, but that hasn't been confirmed by actually running it
there. Worth a CI run on the real target before merging.
detail sunshowers raised in CockroachDB lost quorum on critical ranges during racklette update #10658. That looked like a different issue.