AZ and decommissioning-aware indexing planning, plus optimizations - #6694
Conversation
![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
https://github.com/quickwit-oss/quickwit/blob/43f7a4624f2893725b0a749d0908a631fc15eddd/quickwit-control-plane/src/indexing_scheduler/scheduling/scheduling_logic.rs#L477-L478
When every shard of a source is already assigned to ready peers, unassigned_sources contains no entry for that source, so this loop never calls reclaim_self_hosted_shards_from_peers. For example, a draining indexer with free capacity that hosts a shard currently indexed by a ready indexer in the same AZ will never reclaim it; the physical-plan pass removes and then reassigns it to the ready indexer's unchanged quota, while the locality threshold sees it as nearby and does not rebuild from scratch. This defeats the decommissioning optimization for settled plans, so reclaim candidates need to include fully assigned sources too.
https://github.com/quickwit-oss/quickwit/blob/43f7a4624f2893725b0a749d0908a631fc15eddd/quickwit-control-plane/src/indexing_scheduler/scheduling/mod.rs#L603-L604
For a shard replicated across AZs, selecting only the first hosting node makes is_shard_nearby ignore the AZs of all other replicas. Consequently, an assignment to a non-hosting indexer colocated with the second replica is misclassified as remote, and find_nearby_indexer may choose an actually cross-AZ indexer instead. The nearby check should succeed when the candidate shares an availability zone with any shard location, matching is_shard_local's treatment of replicas.
https://github.com/quickwit-oss/quickwit/blob/43f7a4624f2893725b0a749d0908a631fc15eddd/quickwit-control-plane/src/indexing_scheduler/mod.rs#L493-L495
When locality remains below the threshold but a scratch plan is no better, this return leaves next_plan_from_scratch_timestamp unset. Every subsequent scheduling event therefore solves the complete placement problem twice again, despite the documented 30-minute cooldown; on large clusters with persistently unavoidable low locality, ordinary model changes can repeatedly incur the expensive scratch solve. Record the next-attempt timestamp whenever a scratch rebuild is attempted, not only when its plan is selected.
https://github.com/quickwit-oss/quickwit/blob/43f7a4624f2893725b0a749d0908a631fc15eddd/quickwit-control-plane/src/indexing_scheduler/mod.rs#L319-L323
If the pool contains a Ready indexer with zero indexing capacity plus positive-capacity draining indexers, this check marks every drainer SelfHostedOnly, after which build_indexer_infos drops the only ready indexer. Any shard not hosted by a drainer then has no eligible destination: capacity inflation cannot change eligibility, so minimal_feasible_inflation_attempt exhausts its attempts and panics. Treat a ready indexer as available here only when its indexing capacity is nonzero, consistent with the subsequent filtering.
https://github.com/quickwit-oss/quickwit/blob/43f7a4624f2893725b0a749d0908a631fc15eddd/quickwit-control-plane/src/indexing_scheduler/scheduling/mod.rs#L698-L706
The physical allocator does not filter remaining_num_shards_per_node by eligibility, so a SelfHostedOnly indexer can receive a foreign shard when its logical quota cannot be matched to distinct local shard IDs. This occurs with replicated shards, for example when two draining indexers both host the same shard: the count-based solver can assign each a local quota from the overlapping affinities, but after one gets that shard, this fallback assigns another, non-hosted shard to the other drainer. Enforce self-hosting while resolving shard IDs, or make the logical quotas account for overlapping replicas.
https://github.com/quickwit-oss/quickwit/blob/43f7a4624f2893725b0a749d0908a631fc15eddd/quickwit-serve/src/lib.rs#L1518
Although the pool entry now caches availability_zone, indexer_node_changed still filters updates using only status, capacity, and tasks. If a ClusterChange::Update changes or supplies the AZ without changing those other fields, the update is discarded and the scheduler continues using the stale AZ for placement and locality metrics until the node is removed and re-added. Include availability-zone changes in the pool's meaningful-update predicate.
ℹ️ 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".
|
I don't see in the PR description the fact that now you can rebuild from scratch periodically. |
43f7a46 to
6c79a45
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c79a45df0
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| let eligibility = match indexer.ingester_status { | ||
| IngesterStatus::Ready => Eligibility::Any, | ||
| _ => Eligibility::SelfHostedOnly, |
There was a problem hiding this comment.
When QW_ENABLE_LOCALITY_AWARE_SCHEDULING is enabled and the cluster has no Ready indexer, such as during single-node shutdown, every selected retiring or decommissioning indexer becomes SelfHostedOnly. An enabled Kafka or other non-sharded source has no hosted-shard affinity, so all final-placement candidates are filtered out on every inflation attempt and solve panics, repeatedly restarting the control plane instead of allowing decommissioning to finish. Retain an explicit all-draining fallback or otherwise exclude/drop non-sharded work before invoking the solver.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,352 @@ | |||
| // Copyright 2021-Present Datadog, Inc. | |||
There was a problem hiding this comment.
A repo-wide search for churn_tests finds no module declaration or other reference to this newly added file; scheduling/mod.rs declares only scale_tests. Rust therefore never compiles or runs these 352 lines, so the claimed successive-rebuild and rolling-decommission churn coverage is absent. Add a #[cfg(test)] mod churn_tests; declaration alongside the scale test module.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9585c81b8f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| if best_attempt >= MIN_INFLATION_ATTEMPT_FOR_WARNING { | ||
| let best_attempt = minimal_feasible_inflation_attempt(&base_problem); | ||
| if best_attempt > MIN_INFLATION_ATTEMPT_FOR_WARNING { |
There was a problem hiding this comment.
When placement requires exactly two inflation attempts, this condition emits only info, even though the threshold's contract says two attempts add 44% virtual capacity and warrant operator attention. In deployments that do not collect info-level control-plane logs, the first materially unbalanced scheduling result is therefore invisible; use >= MIN_INFLATION_ATTEMPT_FOR_WARNING so attempt 2 is warned as intended.
Useful? React with 👍 / 👎.
| for shard_id in &task.shard_ids { | ||
| pipeline_per_shard.insert(shard_id.clone(), pipeline.clone()); |
There was a problem hiding this comment.
When two different sources reuse the same ShardId—which the protocol permits because ShardPKey also includes index_uid and source_id—this insertion overwrites one source's assignment with the other. Subsequent plan comparisons can therefore report false shard moves and pipeline resets, or miss real ones, depending on hash-map iteration order; include the task's source identity in the shard key instead of relying on undocumented global shard-ID uniqueness.
AGENTS.md reference: AGENTS.md:L125-L126
Useful? React with 👍 / 👎.
a6ce9cf to
57e9995
Compare
nadav-govari
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e061f5fda
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let can_optimize_plan = is_plan_eligible_for_optimization( | ||
| &indexers, | ||
| &indexer_statuses, | ||
| is_locality_aware, | ||
| &mut self.state, | ||
| ); |
There was a problem hiding this comment.
Density optimization eligibility is evaluated only while handling an externally triggered plan rebuild. When a shard or topology change applies a fragmented-but-valid plan, conditionally_optimize_plan skips repair because that plan differs from the previous one; after the indexers converge, the periodic ControlPlaneLoop calls control_running_plan, whose stable-plan path does nothing. With no further cluster event, the optimizer is therefore never invoked again, so mergeable underfilled pipelines remain indefinitely instead of converging at the documented cooldown cadence. Schedule a rebuild/repair when the running plan is stable and the repair interval has elapsed.
Useful? React with 👍 / 👎.
* Remove empty pipelines from physical plan
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b208d6d20d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let is_locality_aware = is_locality_aware_scheduling_enabled() | ||
| && all_indexers_advertise_availability_zone(&self.indexer_pool); | ||
|
|
||
| let indexer_id_to_cpu_capacities: FnvHashMap<String, CpuCapacity> = indexers | ||
| .iter() | ||
| .filter_map(|indexer| { | ||
| if indexer.indexing_capacity.cpu_millis() > 0 { | ||
| Some((indexer.node_id.to_string(), indexer.indexing_capacity)) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .collect(); | ||
| let indexer_infos: FnvHashMap<NodeId, IndexerInfo> = | ||
| build_indexer_infos(&indexers, is_locality_aware); |
There was a problem hiding this comment.
When the feature flag is enabled but any pool entry lacks an availability zone, such as during a partially configured rollout, this disables the effective locality mode even though select_available_indexers_for_scheduling has already included retiring and decommissioning nodes. build_indexer_infos consequently marks those nodes as Eligibility::Any, allowing foreign shards and non-sharded pipelines to be assigned to nodes that are shutting down. Either retain the self-hosted-only restriction independently of AZ awareness or fall back to the legacy indexer selection as well.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| let unassigned_sources: Vec<Source> = compute_unassigned_sources(problem, solution); | ||
| for source in &unassigned_sources { |
There was a problem hiding this comment.
When a source is already fully assigned and one of its physical hosts begins draining, compute_unassigned_sources omits that source entirely. Thus, if its shard is currently indexed by a ready peer, this function never invokes the reclaim logic; the later physical conversion removes the shard from the peer as draining-hosted but fills the unchanged peer quota by assigning it straight back. The draining node therefore never takes ownership of that shard, defeating the decommissioning behavior and potentially delaying shutdown until the shard is migrated for another reason.
Useful? React with 👍 / 👎.
3 participants
Description
This change adds support for availability zone aware indexing plans, and for allowing decommissioning ingesters to index their own shards to help speed up decommissioning.
The change sits behind a flag, default off, that falls back to the existing flow if it's off. The existing unit tests were not touched (other than struct stuff for compilation). New tests were added, including static tests at huge scale, to ensure that the plans produced are reasonable.
How was this PR tested?
Extensive unit testing and cluster testing. At one index, it achieves just about 100% same-az locality. At multiple indexes (I tested up to 9), it generally hovers above 90%, usually above 95%. Density is usually in the high 80s. There's additional optimizations coming in a follow-up PR that get us there.
High level summary of changes
First, we strip foreign shards off retiring indexers, which are only eligible to index their own shards. This part is relatively straightforward.
To assign shards back to retiring indexers, we reclaim foreign hosted shards from peers. This is done with a general accounting of self-hosted vs foreign shards; from there, we're able to figure out what should go where in the physical step.
Finally, to ensure locality and density, we perform the original density assignment step in each AZ; then, we assign the leftover shards the same as we did before.