Repository navigation
fix(storage-cassandra): apply max_connections and override CDRS default reconnect policy - #457
jcshepherd wants to merge 5 commits into
Conversation
…able broadcast address (#403) Two connection-resilience problems on the Cassandra backend: 1. create_session never applied config.max_connections, so the driver ran one connection per node regardless of config. Now wired through with_connection_pool_config, so the pool opens max_connections connections and scatters requests across them. 2. Containerized nodes advertise their internal bridge IP as broadcast_rpc_address. The driver reconnects to the advertised address, which was not reachable outside the container. devtools/cassandra-node and the CI service containers now advertise 127.0.0.1; the deployment guide documents the requirement for production nodes. Test: connection_pool_size.rs asserts the pool size follows max_connections (red when it was ignored, green now). Recovery-after-drop verified manually via nodetool disablebinary/enablebinary. Partially addresses #403: the underlying cdrs-tokio stream-id reuse is a driver defect, mitigated here (more connections divide the id space) and tracked upstream.
LeeroyHannigan
left a comment
LeeroyHannigan
left a comment
There was a problem hiding this comment.
I ran the new test against a single Cassandra 4.1 node. The pool size follows max_connections: 1, 2 and 8 give 2, 3 and 9 clients on the node, and the test is red with engine.rs reverted to main. fmt and clippy are clean. The broadcast address half does not reproduce: with max_connections=8 every socket the process holds goes to the contact point, none to the advertised address, and after nodetool disablebinary then enablebinary the pool answers "No nodes available in query plan!" for 105 seconds with 127.0.0.1 advertised and without it. Please drop "Closes #403", correct the two documentation statements, and count only this engine's own connections in the test. The commit title also carries a stray bullet.
| // Before the fix, create_session ignored max_connections and opened one | ||
| // connection per engine, so both engines together added ~2 and this | ||
| // assertion fails. With the fix, the large engine alone adds >= 8. | ||
| let large_delta = both_count - small_count; |
There was a problem hiding this comment.
Major: both counts come from system_views.clients, which holds every client on the node, so an unrelated connection closing between them subtracts from the delta and fails the assertion with a message that blames the fix. I held a separate 8-connection client, started this test, and closed that client after the first count: "large pool (max_connections=8) added only 7 connections (small_count=5, both_count=12); expected at least 8". The CI job runs this suite without --test-threads=1 (integration-cassandra.yml:96) while the other 149 tests build and drop their own engines, so please take a baseline immediately around each engine's own construction, or isolate this test the way ttl_integration is isolated at line 89.
There was a problem hiding this comment.
This test was doing more than it needed to, in terms of (essentially) testing driver and node behavior, not just the basic configuration change.
…e completing migration process
jcshepherd
changed the title
I don't think your test reproduced the problem, if I'm understanding your test correctly. The problem is not with initial connection, it is with reconnection. That's where the broadcast address comes into play.
I'm not able to replicate the "no nodes available in query plan" error, but the basic point is correct. The broadcast address was a red herring: the issues were more basic connection management. See below. |
* Broadcast address was a red herring and doesn't play into the client connection process: removed from code and docs. * Retaining change to enable configurable connection pool size, rather than letting CDRS default to 1. * CDRS defaulted to a connection retry policy that would exponentially back off to 60s delay, and then try forever, once a minute. An ill-timed reconnection attempt could experience a delay of up to 2 minutes, giving an appearance of a permanent connection failure, even with a pool size > 1. Reconnection policy is now explicitly configured with a maximum delay of 5s. The "try forever" behavior is not configurable. The "max_attempts" configuration specifies how many attempts are required before the backoff meets/exceeds the maximum delay, not the total number of reconnection attempts. Testing: I'm not including a test to connect/disconnect but in local testing, repeated connection failures (via rapidly bouncing the node) were able to recover in < 1.5s over 15 consecutive restarts. With the default reconnection policy, reconnect attempts appeated to stall after 12 consecutive restarts (as the driver side backoff delay had reached the max of 60s, causing long delays between attempts).
jcshepherd
changed the title
There was a problem hiding this comment.
Thanks Joel. I re-ran the round 1 items against b4cf251 and tested the reconnect policy and the schema agreement wait, on one node and on a two node cluster.
"Closes #403" is gone and so is the broadcast address text in the guide. The pool size reaches the driver, max_connections of 1, 2, 8 and 10 open 2, 3, 9 and 11 sockets. There's still no test that fails if it stops reaching the driver, I've put a sketch of one inline.
The schema agreement wait needs to come out. On two nodes with one stopped, account creation timed out after 51s and a fresh start after 37s, both with Timed out waiting for Cassandra schema agreement after a migration. Main did both in 5.5s and 7.7s with the same node down. It also doesn't fix the schema_history does not exist failure, your last CI run hit it with the wait in place (cassandra-storage).
That failure is one we missed in round 1. With more than one pooled connection, cdrs-tokio sends USE to the other connections from a background task, and the caller's own USE returns before that task finishes (connection_pool.rs:212-251 in 9.0.2), so the next unqualified statement can run in the previous keyspace. In a loop switching between two keyspaces on one node, 9% of statements ran in the wrong keyspace at pool size 10 and none did at pool size 1. Round 1 looked for this and didn't reproduce it, so I won't ask you to fix it here. I'll open an issue and a fix, and since the pool change is what exposes it I'd like that fix to land before this one.
The 5s reconnect cap is fine to keep. When the session did recover from a docker restart, it took 3.7 to 5.4s at head against 7.5 to 13s on main.
I couldn't get the recovery numbers in the commit message behind a published Docker port though, which is how CI and devtools/run-cassandra-tests expose the node. After nodetool disablebinary and enablebinary the first query comes back in about 0.9s, then in most runs the driver drops the node and doesn't pick it up again within 3 minutes. Head got a stable recovery in 1 of 10 of those runs and 2 of 10 restarts, main in 1 and 3.
With the contact point set to the container's own address, head recovered in 0.1 to 4.8s over 7 runs, which is in line with your numbers. The cause is in the driver's metadata refresh and it's on main too, so it's not for this PR. It looks like the cause of the #403 hang, I'll write it up there.
The first commit still has the bullet and the broadcast address text, and the squash puts commit messages in the body, so they'd land in main.
Once these are done I'll approve:
- Remove the schema agreement wait and its test.
- Add a test that fails when the pool size doesn't reach the driver.
- Fix the reconnect comment and the recovery claims in the commit message and PR body.
- Squash to one commit with a message that matches the change.
| // the multi-connection pool (issue #403) exposes it. Wait for the cluster | ||
| // schema to agree before the next statement so the new object is visible | ||
| // everywhere. This is startup/DDL-only, so the poll cost is irrelevant. | ||
| wait_for_schema_agreement(session).await |
There was a problem hiding this comment.
On two nodes with node 2 stopped, this times out on the first migration and fails it. A down node keeps its old schema_version row in system.peers, so the check can't agree until the node is back. Account creation failed after 51s and a fresh start after 37s, main did both in 5.5s and 7.7s.
With both nodes up it doesn't guarantee the other node has the new table either. The system.local and system.peers reads are two separate queries and round robin sends them to different nodes, so the check never reads node 2's own version. A migration still failed with Table 'transaction_ledger' doesn't exist in 1 of 13 runs at head, 2 of 13 on main.
On one node there's no race for it to close. I sent 800 statements on other pooled connections right after their CREATE TABLE and all 800 found the table. I'd remove it.
| use extenddb_storage_cassandra::migrations::run_data_migrations; | ||
|
|
||
| #[tokio::test] | ||
| async fn migrations_survive_concurrent_keyspace_creation() { |
There was a problem hiding this comment.
This passed 20 of 20 runs with the wait removed, so it doesn't test the wait.
The 12 tasks share one session, so one task's USE moves another task's unqualified DDL into its keyspace. When I listed the tables afterwards, 11 of 12 keyspaces held only schema_history, at head and on main, and nothing reported an error. The test only reads schema_history, which each task creates right after its own USE, so it passes. That's #408, which #453 fixes. I'd drop this with the wait.
| let lb = RoundRobinLoadBalancingStrategy::new(); | ||
| let session_future = TcpSessionBuilder::new(lb, cluster_config).build(); | ||
| let session_future = TcpSessionBuilder::new(lb, cluster_config) | ||
| .with_connection_pool_config(pool_config) |
There was a problem hiding this comment.
Deleting this line still passes all 63 unit tests and the 150 direct integration tests, the unit tests only cover pool_size_for. Something like this fails without it, and other clients on the node can't change the result because it counts the driver's pooled connections:
#[tokio::test]
async fn max_connections_reaches_the_driver_pool() {
if crate::helpers::skip_without_cassandra() { return; }
let mut cfg = crate::helpers::test_config();
cfg.max_connections = 4;
cfg.ensure_cached_connection_string();
let engine = extenddb_storage_cassandra::CassandraEngine::new(&cfg, "us-east-1").await.unwrap();
// The driver creates a node's pool on first use.
engine.session().query("SELECT release_version FROM system.local").await.unwrap();
let nodes = engine.session().cluster_metadata().unignored_nodes();
let mut seen = std::collections::HashSet::new();
for _ in 0..16 {
let conn = nodes[0].persistent_connection().await.unwrap();
seen.insert(std::sync::Arc::as_ptr(&conn) as usize);
}
assert_eq!(seen.len(), 4);
}It passes at head and fails with this line removed or with pool_size_for returning 1.
| /// Cap on concurrently in-flight detached TTL claim releases. | ||
| const TTL_RELEASE_MAX_IN_FLIGHT: usize = 1_024; | ||
|
|
||
| // Reconnection backoff bounds for the driver's connection pool (issue #403). |
There was a problem hiding this comment.
RECONNECT_BASE_DELAY is 1s, the comment says 2s. The delays come out at about 2s, about 4s, 4.25 to 5s, then exactly 5s with no jitter (reconnection_policy.rs:124-126 in cdrs-tokio 9.0.2). The same policy also drives the control connection.
A new schedule starts each time the pool starts reconnecting (connection_pool.rs:364), so with the default policy the delay only gets to 60s after about a minute of failed attempts in one outage. The 12 restarts in the commit message would only get there if the pool never reconnected in between.
|
Joel, same as on #412, I've pulled the round 2 additions back. Items 1, 3 and 4 from my last comment are no longer conditions on this PR. The schema agreement measurements (51s and 37s with one of two nodes down, against 5.5s and 7.7s on main) are recorded on #637, which is where that problem lives, and whether the wait stays in this PR is your call. What's left is the round 1 list: a test that fails when the pool size stops reaching the driver (the sketch on engine.rs line 228 is there if you want it), and the stray bullet in the first commit title. Those two and I approve. Nothing else gets added. |
|
Joel, I was wrong to pull back items 1, 3 and 4 of my 28 October comment this morning, so that list stands as posted, and nothing gets added to it. The schema agreement wait is new in this PR. It isn't in the round 1 commit or on main, and with one of two nodes stopped it fails account creation after 51s and a fresh start after 37s, against 5.5s and 7.7s on main. It needs to come out, or wait only on peers that are up. Items 3 and 4 are about this PR's own reconnect comment and commit messages, which land on main with the squash. |
Mitigates (#403)
What
Plumb config.max_connections through to the driver config, so the driver can use more than the default single connection per node (typically Cassandra drivers are configured to use multiple connections per node). This has a side effect of improving driver stability due to an issue in the cdr-tokio driver where "stream ids" (like request ids") rollover at 2^16 and get re-used without checking to see if the id is already in use (e.g., by a slow-running query). This should largely mitigate the 'hung connection' effect reported in #403.
The driver issue itself is out-of-scope of this PR.
The driver defaulted to a reconnect policy that would back off to a 60s delay, and hold there. In an unstable environment, this could quickly lead to the driver take up to 2 minutes to reconnect, giving the appearance of a persistent connection issue. With this change, connection retries back off to a maximum of 5s between attempts.
Recovery-after-drop verified manually via nodetool disablebinary/enablebinary.
Why
Two connection-resilience problems on the Cassandra backend:
These changes do not address the stream id issues in the CDRS driver itself, but provide more stability and responsiveness for ExtendDB when under load and/or Cassandra is exhibiting instability.
Testing done
Full pre-merge workflow (fmt, clippy, license, unit, pytest, rust-tests)
Local testing with rapid, repeated node bounces showed connections being re-established reliably with an average reconnect time of 1.26s once the node itself was ready to accept requests.
Checklist
cargo test --workspace)cargo fmt --check)cargo clippy -- -W clippy::pedantic)Storagetrait, auth model, on-diskformat, or public CLI surface, an RFC has been accepted or is linked
below. Otherwise, an ADR captures the decision (link below).
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.