Update indexer pool on all node updates; Fix is_indexed for empty shards#6518
Update indexer pool on all node updates; Fix is_indexed for empty shards#6518nadav-govari wants to merge 12 commits into
Conversation
nadav-govari
force-pushed
the
nadav/decommissioning-bug
branch
from
34d09c9 to
dda63bb
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb0ed0b861
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9198367963
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let shutdown_handle = sandbox | ||
| .remove_node(&retiring_node_id) | ||
| .expect("the retiring node should be in the sandbox"); | ||
| tokio::time::timeout(Duration::from_secs(30), shutdown_handle.shutdown()) |
There was a problem hiding this comment.
At this shutdown await, the retiring_client used for node_stats() above is still in scope and has an idle reqwest connection to the node being shut down. The node task waits for the REST server's graceful shutdown before returning, and ClusterSandbox::shutdown_services explicitly avoids this class of hang by dropping REST clients before shutdown; keeping this client alive can make the new test time out even when decommissioning succeeded. Drop the retiring-node client before awaiting shutdown_handle.shutdown().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3dcf0ebca
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 531af99230
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Err(_) => false, | ||
| } | ||
| }, | ||
| Duration::from_millis(200), |
There was a problem hiding this comment.
In this new test, the second wait_until_predicate argument is the total timeout, so this gives CI only 200 ms for index creation, plan propagation, pipeline startup, and node_stats() visibility before failing. A slow but correct run can miss this precondition and never exercise the decommission path; keep these precondition waits at seconds scale and use the millisecond value only for the retry interval.
Useful? React with 👍 / 👎.
| .expect("all 6 documents should be searchable after decommission"); | ||
|
|
||
| // Clean shutdown of the remaining nodes (also exercises decommissioning the last indexer). | ||
| tokio::time::timeout(Duration::from_secs(3), sandbox.shutdown()) |
There was a problem hiding this comment.
At this shutdown, surviving_client is still in scope after its node_stats() request, so its reqwest keep-alive connection can keep the surviving node's REST server in graceful.shutdown().await and make this 3-second timeout fail even though decommission succeeded. Scope or drop the pinned node clients before calling sandbox.shutdown().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36362fa8c2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
nadav-govari
force-pushed
the
nadav/decommissioning-bug
branch
2 times, most recently
from
58ec844 to
3ecff0d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6137161ba0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| self.inner | ||
| .shards | ||
| .get_mut(queue_id) | ||
| .expect("shard was present above and is only removed on the missing-queue path") | ||
| .truncation_position_inclusive = truncate_up_to_position_inclusive; |
There was a problem hiding this comment.
Fresh evidence in the current diff is this new path that records Position::Eof(None) as the shard truncation even though the WAL branch is skipped. For an empty shard, IngestSource::truncate publishes the EOF gossip asynchronously and then sends the best-effort TruncateShards RPC; if this RPC reaches a decommissioning ingester first, truncate_shards_inner immediately calls check_decommissioning_status and can mark the node Decommissioned before the ShardPositionsUpdate has deleted the shard from the ingester/control plane/metastore. When the retiring indexer/ingester then shuts down, the async gossip can be lost and the closed shard remains assigned to a dead node, so empty EOFs should keep waiting for the gossip deletion rather than making the shard is_indexed via this RPC path.
Useful? React with 👍 / 👎.
guilload
left a comment
guilload
left a comment
There was a problem hiding this comment.
Approved but address comments.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c71b965c3b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Err(_) => false, | ||
| } | ||
| }, | ||
| Duration::from_secs(3), |
There was a problem hiding this comment.
In this post-decommission check, a 3-second total wait can fail on a correct but slow CI run because Auto-committed ingest still has to finish publishing splits, update the metastore, and become visible to the searcher after the retiring indexer shuts down; the existing graceful-shutdown/no-data-loss coverage in this file allows a much longer window for the same visibility path. Please keep this timeout at the longer integration-test scale so the new regression test does not become flaky under load.
Useful? React with 👍 / 👎.
2 participants
Description
Fixes two bugs:
Rebalance shards when ingester status changes #6185 made it so that cluster updates only fire when ingester status changes. This made it so that the reported indexing plan coming from indexers wasn't being updated. Specifically, this appeared when a node was decommissioned, its shards got reassigned, and the new indexer the shards got reassigned finished indexing the shard, and deleted it; in these cases, the control plane running plan loop would see that it wasn't up to date and fire a bunch of reapply plan requests.
is_indexedis broken for empty shards as of Delete shards only when notified via gossip #6504 -self.truncation_position_inclusive.is_eof()can't hit because it's necessarilyPosition::Beginning.How was this PR tested?
Unit tested and integration tested.
I ran a test on a local cluster with all services on one node, and then one service per node with multiple indexers; the test decommissioned one node at a time the way a deployment would, and eventually closed the last one too. In these tests, the number of searchable docs was the same as the number acked on ingest. I also set min shards to be higher than the number of indexers and sent less docs than the number of shards to confirm that empty shards truncate and delete gracefully; everything worked as intended in the test.