chore: add node/manager/coordinating count checks to OpenSearch readiness probe - #1959
Conversation
…1913) * add node count check * add manager node check * style: ruff autofix (auto) * add coordinating nodes * style: ruff autofix (auto) * Update opensearch_utils.py * style: ruff autofix (auto) * Update opensearch_utils.py * add max retries to wait for opensearch * style: ruff autofix (auto) * updating count to 3 for nodes * add check nodes false for OSS and for tests ---------
github-actions
Bot
added
backend
WalkthroughAdds an optional node-count readiness check to ChangesOpenSearch node-count readiness check with configurable retries
Sequence Diagram(s)sequenceDiagram
participant lifespan as lifespan.py
participant init as opensearch_init
participant utils as opensearch_utils
participant client as OpenSearch Client
lifespan->>init: wait_for_opensearch(max_retries=OPENSEARCH_WAIT_MAX_RETRIES)
init->>utils: _wait_for_opensearch(client, max_retries)
loop up to max_retries
utils->>client: cluster.health()
alt OPENSEARCH_NODE_COUNT_CHECK_ENABLED=true
utils->>client: transport GET _nodes/data
utils->>client: transport GET _nodes/cluster_manager
utils->>client: transport GET _nodes/coordinating_only
alt all counts met
utils-->>init: return (ready)
else count short
utils->>utils: log warning, retry with backoff
end
else OPENSEARCH_NODE_COUNT_CHECK_ENABLED=false
utils-->>init: return (health-based ready)
end
end
init-->>lifespan: OpenSearch is ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app/lifespan.py (1)
296-296: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConfirm the worst-case startup wait is acceptable.
OPENSEARCH_WAIT_MAX_RETRIESdefaults to 100 and the underlying backoff caps each delay atmax_delay=30s, so an unavailable cluster can block this synchronous bootstrap for up to ~50 minutes before raising. This runs before the rest of startup (the comment at Line 246-250 notes it's intentionally synchronous). Confirm this upper bound is acceptable for your orchestration/liveness timeouts, or consider a tighter cap for the bootstrap path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/lifespan.py` at line 296, The synchronous OpenSearch bootstrap in wait_for_opensearch can block startup for far too long with the current OPENSEARCH_WAIT_MAX_RETRIES and capped backoff. Review the startup path in lifespan.py around the await wait_for_opensearch call and either reduce the bootstrap retry/max-delay settings for this path or make them configurable so the worst-case wait is bounded to match your orchestration and liveness timeouts.src/utils/opensearch_utils.py (1)
166-184: 🚀 Performance & Scalability | 🔵 TrivialMove the
config.settingsimport out of the retry loop. The_nodes.successfulvalues already match the filtered node requests, so the node-count check is fine as-is.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/opensearch_utils.py` around lines 166 - 184, Move the config.settings import out of the retry loop in the OpenSearch node-count check, keeping the logic in the health-check path unchanged. Update the code around the node-count validation in opensearch_utils so the OPENSEARCH_EXPECTED_* constants and OPENSEARCH_NODE_COUNT_CHECK_ENABLED are imported once at module scope or before the retry block, and leave the existing _nodes.successful handling for cluster_manager and coordinating_only as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config/settings.py`:
- Around line 44-57: The OpenSearch node-count readiness gate in settings is too
strict by default and can block startup for valid topologies. Update the
defaults around OPENSEARCH_NODE_COUNT_CHECK_ENABLED,
OPENSEARCH_EXPECTED_CLUSTER_MANAGER_COUNT, and
OPENSEARCH_EXPECTED_COORDINATING_NODE_COUNT so the check is non-blocking unless
explicitly configured, or make the expected counts safe for optional node types.
Also review wait_for_opensearch and the related config constants in settings.py
to ensure the gate only enforces counts when the deployment actually requires
dedicated cluster-manager or coordinating-only nodes, and document the required
tuning.
---
Nitpick comments:
In `@src/app/lifespan.py`:
- Line 296: The synchronous OpenSearch bootstrap in wait_for_opensearch can
block startup for far too long with the current OPENSEARCH_WAIT_MAX_RETRIES and
capped backoff. Review the startup path in lifespan.py around the await
wait_for_opensearch call and either reduce the bootstrap retry/max-delay
settings for this path or make them configurable so the worst-case wait is
bounded to match your orchestration and liveness timeouts.
In `@src/utils/opensearch_utils.py`:
- Around line 166-184: Move the config.settings import out of the retry loop in
the OpenSearch node-count check, keeping the logic in the health-check path
unchanged. Update the code around the node-count validation in opensearch_utils
so the OPENSEARCH_EXPECTED_* constants and OPENSEARCH_NODE_COUNT_CHECK_ENABLED
are imported once at module scope or before the retry block, and leave the
existing _nodes.successful handling for cluster_manager and coordinating_only
as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 337381f3-aee6-4af7-9e15-032a4b5bb57d
📒 Files selected for processing (9)
.github/workflows/test-e2e.yml.github/workflows/test-integration.ymldocker-compose.ymlsrc/app/lifespan.pysrc/config/settings.pysrc/utils/opensearch_init.pysrc/utils/opensearch_utils.pytests/unit/test_opensearch_init_wait_retries.pytests/unit/test_opensearch_wait_node_count.py
| OPENSEARCH_NODE_COUNT_CHECK_ENABLED = os.getenv( | ||
| "OPENSEARCH_NODE_COUNT_CHECK", "true" | ||
| ).strip().lower() in ("true", "1", "yes") | ||
|
|
||
| # Expected cluster size, used only when the node-count check is enabled. | ||
| OPENSEARCH_EXPECTED_DATA_NODE_COUNT = get_env_int("OPENSEARCH_EXPECTED_DATA_NODE_COUNT", 3) | ||
| # Minimum reachable cluster-manager (master) nodes, gated by the same flag. | ||
| OPENSEARCH_EXPECTED_CLUSTER_MANAGER_COUNT = get_env_int( | ||
| "OPENSEARCH_EXPECTED_CLUSTER_MANAGER_COUNT", 3 | ||
| ) | ||
| # Minimum reachable coordinating-only nodes, gated by the same flag. | ||
| OPENSEARCH_EXPECTED_COORDINATING_NODE_COUNT = get_env_int( | ||
| "OPENSEARCH_EXPECTED_COORDINATING_NODE_COUNT", 3 | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Default-enabled node-count gate with expected counts of 3 can permanently block startup.
OPENSEARCH_NODE_COUNT_CHECK_ENABLED defaults to true, while OPENSEARCH_EXPECTED_CLUSTER_MANAGER_COUNT and OPENSEARCH_EXPECTED_COORDINATING_NODE_COUNT default to 3. Coordinating-only nodes and dedicated cluster-manager nodes are optional in many OpenSearch topologies. A cluster that does not deploy 3 dedicated coordinating-only nodes (or 3 dedicated cluster-managers) will report coordinating_count/cluster_manager_count below the threshold forever, so wait_for_opensearch will exhaust all retries and raise OpenSearchNotReadyError, failing startup.
The CI/compose overrides disable the flag, but any deployment that doesn't explicitly set OPENSEARCH_NODE_COUNT_CHECK=false inherits this risk. Consider defaulting the flag to false, or defaulting the coordinating/cluster-manager expected counts to a non-blocking value, and documenting that these must be tuned to the actual topology.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config/settings.py` around lines 44 - 57, The OpenSearch node-count
readiness gate in settings is too strict by default and can block startup for
valid topologies. Update the defaults around
OPENSEARCH_NODE_COUNT_CHECK_ENABLED, OPENSEARCH_EXPECTED_CLUSTER_MANAGER_COUNT,
and OPENSEARCH_EXPECTED_COORDINATING_NODE_COUNT so the check is non-blocking
unless explicitly configured, or make the expected counts safe for optional node
types. Also review wait_for_opensearch and the related config constants in
settings.py to ensure the gate only enforces counts when the deployment actually
requires dedicated cluster-manager or coordinating-only nodes, and document the
required tuning.
mpawlow
left a comment
mpawlow
left a comment
There was a problem hiding this comment.
Code Review 1
- ✅ Approved / LGTM 🚀
2 participants
Summary
Cherry-pick of #1913 onto
main.The OpenSearch readiness probe (
wait_for_opensearch) only checked cluster health status (green/yellow) but not whether the expected number of nodes had actually joined the cluster. On multi-node deployments this caused the startup bootstrap to proceed before the full cluster was formed, leading to flaky behaviour under load.Changes:
src/utils/opensearch_utils.py— after a green/yellow health status, gate on data-node count, cluster-manager count, and coordinating-node count matching configurable thresholds; fall through to retry/backoff if any are shortsrc/config/settings.py— new env vars:OPENSEARCH_NODE_COUNT_CHECK(defaulttrue),OPENSEARCH_EXPECTED_DATA_NODE_COUNT(default3),OPENSEARCH_EXPECTED_CLUSTER_MANAGER_COUNT(default3),OPENSEARCH_EXPECTED_COORDINATING_NODE_COUNT(default3),OPENSEARCH_WAIT_MAX_RETRIES(default100)src/utils/opensearch_init.py— forwardmax_retriesfrom lifespan callerdocker-compose.yml, CI workflows — setOPENSEARCH_NODE_COUNT_CHECK=falsefor single-node local/test clusterstests/unit/test_opensearch_wait_node_count.py— unit tests covering all count-check branches (flag on/off, each count type short)tests/unit/test_opensearch_init_wait_retries.py— unit tests verifyingmax_retriesis forwarded correctlySummary by CodeRabbit
New Features
Bug Fixes