[nexus] tune libpq keepalive to catch dead CockroachDB connections faster

[jam-mad]

Tune libpq keepalive so dead CockroachDB connections are caught in minutes, not hours

Fixes #10668.

Summary

Nexus used libpq's default TCP keepalive settings. On illumos, that
means a dead CockroachDB connection can sit undetected for about 2
hours. This sets four keepalive parameters so detection takes about 2
minutes instead.

All credit for finding this goes to @davepacheco, from a real incident
where it hung a rack update for 45+ minutes. This PR implements his fix
and adds tests, including one that opens a real connection and reads
back the actual kernel TCP settings, the same check @davepacheco did by
hand with mdb -k.

The problem

A Nexus instance hung during a rack update, holding two dead
CockroachDB connections for 47+ minutes. @davepacheco traced it (full
details in #10668): the CockroachDB sled had reset, but Nexus never
noticed. Why: keepalives_idle, keepalives_interval, and
keepalives_count were never set, so Nexus used the OS defaults, 2
hours to the first probe and 8 more minutes to give up.

The change

make_postgres_connector() in pool.rs already builds a list of libpq
connection parameters (just sslmode=disable before). This adds four
more:

let args = vec![
    ("sslmode", "disable"),
    ("keepalives", "1"),
    ("keepalives_idle", "10"),
    ("keepalives_interval", "10"),
    ("keepalives_count", "12"),
];

10 seconds to the first probe, 120 more seconds to give up. Worst case:
about 2 minutes instead of 2 hours.

These numbers are a starting point, close to what @davepacheco
suggested, not final. Worth a second look before merging: too
aggressive and we might drop connections during a normal CockroachDB
pause; too lax and this doesn't fix anything.

Testing

pool_connection.rs had zero test coverage before this. Added two
tests:

• to_url_includes_keepalive_args: checks the connection string has
  the right parameters. Fast, no database needed.

• connection_actually_gets_tcp_keepalive_settings: opens a real
  connection to a real (test) CockroachDB and reads back the actual
  kernel TCP settings. The automated version of the mdb -k check
  @davepacheco did by hand.

  Diesel hides the raw socket, so the test opens a second, plain libpq
  connection with the same connection string and reads that one
  instead. Works on Linux, macOS, and illumos with no extra code, since
  socket2's keepalive getters already handle each OS's different
  option names. I've only run this on Linux; illumos should work the
  same way but I haven't confirmed it there.

Also re-ran the existing db::pool tests against a real CockroachDB.
Both still pass.

New dev-dependencies (socket2, libc) were already workspace-managed,
just not used by this crate before. Nothing new added to the dependency
graph.

How to re-run

cargo test -p nexus-db-queries --lib pool_connection
cargo nextest run -p nexus-db-queries -E 'test(pool_connection)'
cargo nextest run -p nexus-db-queries -E 'test(db::pool::test)'

Risk / non-goals

• Small change: pool.rs, pool_connection.rs, and a Cargo.toml
  update. No schema, no API change.
• Kernel-state test only ran on Linux so far. Should work on illumos
  too; not yet confirmed there.
• Doesn't touch the separate duplicate-connection issue sunshowers
  raised in CockroachDB lost quorum on critical ranges during racklette update #10658.
• Timing values are open to change; see above.

[davepacheco]

I appreciate the offer here, but this isn't that helpful. The change itself is structurally fine (if tiny), but the specific tunable values don't seem right. The tests here give confidence in the direction of the fix, but they don't actually test the fix and I don't think they're worth including. (They don't replicate the mdb -k work I did because that involved looking at the actual connection created by the program, not one created in a way that mimics what we think the program does.) Also, the description here is pretty verbose. There's no need to restate the bug when you're linking to it. I'm planning to create a new PR with the fix and I'll test it by hand.

I'd welcome approaches for actually testing what we want here automatically.

[davepacheco]

Suggested change

	// Tune libpq's TCP keepalive so a dead CockroachDB connection is caught
	// in about 2 minutes, not the OS default of 2 hours. See
	// oxidecomputer/omicron#10668: this gap once made Nexus hold a dead
	// connection for 45+ minutes and hang a rack update.
	let args = vec![
	("sslmode", "disable"),
	("keepalives", "1"),
	("keepalives_idle", "10"),
	("keepalives_interval", "10"),
	("keepalives_count", "12"),
	];
	// Tune libpq's TCP keepalive so a dead CockroachDB connection is caught
	// in one minute, not the OS default of 2 hours.
	let args = vec![
	("sslmode", "disable"),
	("keepalives", "1"),
	("keepalives_idle", "30"),
	("keepalives_interval", "5"),
	("keepalives_count", "6"),
	];

[davepacheco]

I don't find it helpful to reference history in this way. It's sufficient to just say "Verifies that database connections have TCP keepalive configured".

Except this test doesn't do that. It just tests that the URL gets constructed the way we think it does. I don't think this is that useful a test.

[davepacheco]

I'm also not sure this test is worthwhile. Again, it doesn't test that the connections that we create have keepalive set on them. It tests that if we were to create a connection in the same way that Diesel currently does, with the same arguments that we're providing to it, then our connection would have keepalive enabled. But if its implementation changed for whatever reason, this test could still pass while the behavior was broken.

[jam-mad]

I appreciate the offer here, but this isn't that helpful. The change itself is structurally fine (if tiny), but the specific tunable values don't seem right. The tests here give confidence in the direction of the fix, but they don't actually test the fix and I don't think they're worth including. (They don't replicate the mdb -k work I did because that involved looking at the actual connection created by the program, not one created in a way that mimics what we think the program does.) Also, the description here is pretty verbose. There's no need to restate the bug when you're linking to it. I'm planning to create a new PR with the fix and I'll test it by hand.

I'd welcome approaches for actually testing what we want here automatically.

Thanks for the review, especially given the team’s limited bandwidth for outside contributions. I tend to err on the side of verbosity because I context-switch a lot and am used to being thorough when working in unfamiliar repos. I’m happy to relax that a bit.

I proposed a simple test in PR #11086. I tested it on linux using the cargo nextest instructions. I didn’t get a chance to test it on the helios VM.