feat(elasticsearch): add node maintenance entry points for external orchestration - #190
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds Elasticsearch node maintenance start and end entry points. The change defines defaults and argument specifications, documents the workflow, restores cluster state after maintenance, extends the fake API, and adds integration contract scenarios. ChangesNode maintenance lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 4
🧹 Nitpick comments (1)
tests/integration/node_maintenance_contract.yml (1)
134-150: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the final persistent settings state.
The log checks prove that restore requests were sent. They do not prove that the final state is correct. Query
/_cluster/settingsafternode_maintenance_endand assert thatindices.recovery.max_bytes_per_secis200mbandcluster.routing.allocation.node_concurrent_recoveriesis absent.🤖 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 `@tests/integration/node_maintenance_contract.yml` around lines 134 - 150, Extend the node_maintenance_end validation to query /_cluster/settings after the restore sequence, then assert the final persistent settings state: indices.recovery.max_bytes_per_sec must be 200mb and cluster.routing.allocation.node_concurrent_recoveries must be absent. Keep the existing _log endpoint assertions and include the queried response in failure diagnostics.
🤖 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 `@docs/reference/elasticsearch.md`:
- Around line 446-465: Update the maintenance example around the
`node_maintenance_start` and `node_maintenance_end` includes so the restore
include runs inside an Ansible `always` block, even when stop, patch, reboot, or
start tasks fail. Keep the existing `elasticsearch_maintenance_wait_for_node`
variable and restore role parameters unchanged.
In `@roles/elasticsearch/tasks/node_maintenance_end.yml`:
- Around line 42-57: Update the “node_maintenance_end | Re-enable shard
allocation” task to restore the allocation baseline from
elasticsearch_cluster_settings rather than always clearing the setting. Use the
configured cluster.routing.allocation.enable value when present, and retain null
only when no baseline exists.
- Around line 25-40: Add ignore_errors: true to the retried best-effort
ansible.builtin.uri tasks in the node maintenance restore sequence, including
the task shown and the tasks at the referenced ranges. Keep their existing
until, retry, and failed_when behavior unchanged so exhausted retries do not
halt subsequent restoration steps.
In `@roles/elasticsearch/tasks/node_maintenance_start.yml`:
- Around line 26-32: Update the URL resolution in
roles/elasticsearch/tasks/node_maintenance_start.yml lines 26-32 and
roles/elasticsearch/tasks/node_maintenance_end.yml lines 17-23 to use HTTPS when
HTTP security is enabled; otherwise require an explicit HTTPS
elasticsearch_maintenance_api_url instead of constructing an insecure HTTP
default. Keep both maintenance entry points’ resolution logic consistent.
---
Nitpick comments:
In `@tests/integration/node_maintenance_contract.yml`:
- Around line 134-150: Extend the node_maintenance_end validation to query
/_cluster/settings after the restore sequence, then assert the final persistent
settings state: indices.recovery.max_bytes_per_sec must be 200mb and
cluster.routing.allocation.node_concurrent_recoveries must be absent. Keep the
existing _log endpoint assertions and include the queried response in failure
diagnostics.
🪄 Autofix
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 Plus
Run ID: fc66c93e-f920-49ad-a23d-72cc4f517c6c
📒 Files selected for processing (8)
.gitignoredocs/reference/elasticsearch.mdroles/elasticsearch/defaults/main.ymlroles/elasticsearch/meta/argument_specs.ymlroles/elasticsearch/tasks/node_maintenance_end.ymlroles/elasticsearch/tasks/node_maintenance_start.ymltests/fakes/fake_es_rolling_api.pytests/integration/node_maintenance_contract.yml
Comment on lines
+446
to
+465
| ```yaml | ||
| - name: Drain this node | ||
| ansible.builtin.include_role: | ||
| name: oddly.elasticstack.elasticsearch | ||
| tasks_from: node_maintenance_start | ||
| vars: | ||
| elasticsearch_maintenance_password: "{{ elastic_password }}" | ||
|
|
||
| # ... stop the service, patch, reboot, start the service ... | ||
|
|
||
| - name: Restore cluster state | ||
| ansible.builtin.include_role: | ||
| name: oddly.elasticstack.elasticsearch | ||
| tasks_from: node_maintenance_end | ||
| vars: | ||
| elasticsearch_maintenance_password: "{{ elastic_password }}" | ||
| # Wait matches on node.name, which the role sets from elasticsearch_nodename | ||
| # (defaults to the short hostname, may differ from inventory_hostname). | ||
| elasticsearch_maintenance_wait_for_node: "{{ elasticsearch_nodename | default(inventory_hostname) }}" | ||
| ``` |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Put the restore include in an always block.
The example runs node_maintenance_end only after successful maintenance. If the stop, patch, reboot, or start task fails, Ansible does not restore allocation, ML mode, or voting exclusions.
Proposed documentation change
-- name: Drain this node
- ansible.builtin.include_role:
- name: oddly.elasticstack.elasticsearch
- tasks_from: node_maintenance_start
- vars:
- elasticsearch_maintenance_password: "{{ elastic_password }}"
-
-# ... stop the service, patch, reboot, start the service ...
-
-- name: Restore cluster state
- ansible.builtin.include_role:
- name: oddly.elasticstack.elasticsearch
- tasks_from: node_maintenance_end
- vars:
- elasticsearch_maintenance_password: "{{ elastic_password }}"
- elasticsearch_maintenance_wait_for_node: "{{ elasticsearch_nodename | default(inventory_hostname) }}"
+- name: Maintain this node
+ block:
+ - name: Drain this node
+ ansible.builtin.include_role:
+ name: oddly.elasticstack.elasticsearch
+ tasks_from: node_maintenance_start
+ vars:
+ elasticsearch_maintenance_password: "{{ elastic_password }}"
+
+ # ... stop the service, patch, reboot, start the service ...
+ always:
+ - name: Restore cluster state
+ ansible.builtin.include_role:
+ name: oddly.elasticstack.elasticsearch
+ tasks_from: node_maintenance_end
+ vars:
+ elasticsearch_maintenance_password: "{{ elastic_password }}"
+ elasticsearch_maintenance_wait_for_node: "{{ elasticsearch_nodename | default(inventory_hostname) }}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ```yaml | |
| - name: Drain this node | |
| ansible.builtin.include_role: | |
| name: oddly.elasticstack.elasticsearch | |
| tasks_from: node_maintenance_start | |
| vars: | |
| elasticsearch_maintenance_password: "{{ elastic_password }}" | |
| # ... stop the service, patch, reboot, start the service ... | |
| - name: Restore cluster state | |
| ansible.builtin.include_role: | |
| name: oddly.elasticstack.elasticsearch | |
| tasks_from: node_maintenance_end | |
| vars: | |
| elasticsearch_maintenance_password: "{{ elastic_password }}" | |
| # Wait matches on node.name, which the role sets from elasticsearch_nodename | |
| # (defaults to the short hostname, may differ from inventory_hostname). | |
| elasticsearch_maintenance_wait_for_node: "{{ elasticsearch_nodename | default(inventory_hostname) }}" | |
| ``` |
🤖 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 `@docs/reference/elasticsearch.md` around lines 446 - 465, Update the
maintenance example around the `node_maintenance_start` and
`node_maintenance_end` includes so the restore include runs inside an Ansible
`always` block, even when stop, patch, reboot, or start tasks fail. Keep the
existing `elasticsearch_maintenance_wait_for_node` variable and restore role
parameters unchanged.
Comment on lines
+25
to
+40
| - name: node_maintenance_end | Wait for cluster API | ||
| ansible.builtin.uri: | ||
| url: "{{ _elasticsearch_maintenance_url }}/_cluster/health" | ||
| method: GET | ||
| status_code: [200, 503] | ||
| user: elastic | ||
| password: "{{ elasticsearch_maintenance_password }}" | ||
| force_basic_auth: true | ||
| validate_certs: "{{ elasticsearch_validate_api_certs }}" | ||
| register: _elasticsearch_maintenance_api | ||
| until: (_elasticsearch_maintenance_api.status | default(0)) == 200 | ||
| retries: 12 | ||
| delay: 10 | ||
| changed_when: false | ||
| failed_when: false | ||
| no_log: "{{ elasticstack_no_log }}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use ignore_errors: true for retried best-effort requests.
In Ansible 2.19+, failed_when: false does not prevent failure after until retries are exhausted. A failed API check, allocation restore, drain-setting restore, or health poll can stop the remaining restore sequence. Add ignore_errors: true to these retried best-effort URI tasks.
As per path instructions, failed_when: false does NOT survive until/retries exhaustion in Ansible 2.19+ — use ignore_errors: true instead.
Also applies to: 42-57, 86-109, 128-145
🧰 Tools
🪛 Checkov (3.3.9)
[medium] 25-43: Ensure that certificate validation isn't disabled with uri
(CKV_ANSIBLE_1)
[medium] 25-43: Ensure that HTTPS url is used with uri
(CKV2_ANSIBLE_1)
🤖 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 `@roles/elasticsearch/tasks/node_maintenance_end.yml` around lines 25 - 40, Add
ignore_errors: true to the retried best-effort ansible.builtin.uri tasks in the
node maintenance restore sequence, including the task shown and the tasks at the
referenced ranges. Keep their existing until, retry, and failed_when behavior
unchanged so exhausted retries do not halt subsequent restoration steps.
Source: Path instructions
Comment on lines
+42
to
+57
| - name: node_maintenance_end | Re-enable shard allocation | ||
| ansible.builtin.uri: | ||
| url: "{{ _elasticsearch_maintenance_url }}/_cluster/settings" | ||
| method: PUT | ||
| body: '{ "persistent": { "cluster.routing.allocation.enable": null } }' | ||
| body_format: json | ||
| user: elastic | ||
| password: "{{ elasticsearch_maintenance_password }}" | ||
| force_basic_auth: true | ||
| validate_certs: "{{ elasticsearch_validate_api_certs }}" | ||
| register: _elasticsearch_maintenance_alloc | ||
| until: (_elasticsearch_maintenance_alloc.json | default({})).acknowledged | default(false) | ||
| retries: 10 | ||
| delay: 30 | ||
| failed_when: false | ||
| no_log: "{{ elasticstack_no_log }}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Restore the configured allocation baseline.
node_maintenance_start persists cluster.routing.allocation.enable: primaries, but this task always clears the key. If elasticsearch_cluster_settings declares a persistent allocation policy, this removes that policy instead of restoring it. Derive this key from elasticsearch_cluster_settings, with null only when no baseline exists.
🧰 Tools
🪛 Checkov (3.3.9)
[medium] 42-60: Ensure that certificate validation isn't disabled with uri
(CKV_ANSIBLE_1)
[medium] 42-60: Ensure that HTTPS url is used with uri
(CKV2_ANSIBLE_1)
🤖 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 `@roles/elasticsearch/tasks/node_maintenance_end.yml` around lines 42 - 57,
Update the “node_maintenance_end | Re-enable shard allocation” task to restore
the allocation baseline from elasticsearch_cluster_settings rather than always
clearing the setting. Use the configured cluster.routing.allocation.enable value
when present, and retain null only when no baseline exists.
Comment on lines
+26
to
+32
| - name: node_maintenance_start | Resolve API URL | ||
| ansible.builtin.set_fact: | ||
| _elasticsearch_maintenance_url: >- | ||
| {{ elasticsearch_maintenance_api_url | ||
| if elasticsearch_maintenance_api_url is defined | ||
| else elasticsearch_http_protocol ~ '://' ~ elasticsearch_api_host | ||
| ~ ':' ~ elasticstack_elasticsearch_http_port }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Use a secure default maintenance API URL. Both entry points construct an HTTP URL from the role defaults and then send the required maintenance password with Basic authentication.
roles/elasticsearch/tasks/node_maintenance_start.yml#L26-L32: resolve to HTTPS when HTTP security is enabled, or require an explicit HTTPS API URL.roles/elasticsearch/tasks/node_maintenance_end.yml#L17-L23: use the same secure URL resolution asroles/elasticsearch/tasks/node_maintenance_start.yml.
📍 Affects 2 files
roles/elasticsearch/tasks/node_maintenance_start.yml#L26-L32(this comment)roles/elasticsearch/tasks/node_maintenance_end.yml#L17-L23
🤖 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 `@roles/elasticsearch/tasks/node_maintenance_start.yml` around lines 26 - 32,
Update the URL resolution in
roles/elasticsearch/tasks/node_maintenance_start.yml lines 26-32 and
roles/elasticsearch/tasks/node_maintenance_end.yml lines 17-23 to use HTTPS when
HTTP security is enabled; otherwise require an explicit HTTPS
elasticsearch_maintenance_api_url instead of constructing an insecure HTTP
default. Keep both maintenance entry points’ resolution logic consistent.
Source: Linters/SAST tools
… the elasticstack role defaults
…ew nits The PR body promised a contract test at tests/integration/node_maintenance_contract.yml but the file was never in the diff. This adds it (unblocked via the same .gitignore whitelist the other contract tests use), plus a couple of nits the review flagged: - node_maintenance_contract.yml: three plays against the fake ES API. Play 1 asserts the drain sequence hits every expected endpoint (voting exclusion for the correct nodename, allocation=primaries, ML upgrade mode ON, boost applied at 500mb, flush), then that the restore reverses each one (allocation reset, ML OFF, voting exclusion DELETE, baseline restored to 200mb from elasticsearch_cluster_settings, boosted-but-baseline-less keys explicitly nulled). Play 2 pins elasticsearch_ml_enabled: false and asserts no _ml/set_upgrade_mode call fires — catches the ML gating consistency fix below. - Docs example: elasticsearch_maintenance_wait_for_node now matches the internal _cat/nodes?h=name lookup by using elasticsearch_nodename | default(inventory_hostname). The old value silently spun 60x10s when node.name diverged from inventory_hostname and swallowed the fail. - ML gating: both node_maintenance_start and _end now guard the ML upgrade_mode call with when: elasticsearch_ml_enabled | bool, matching elasticsearch-rolling-upgrade.yml. - Voting exclusion: added a comment explaining why the maintenance entry points use it even though rolling-upgrade doesn't — the reboot/OS-patch window can be long, and the exclusion is always cleared in node_maintenance_end. Duplication with elasticsearch-rolling-upgrade.yml stays as-is — the sibling PRs #187 and #188 change the same shape of code but in the rolling-upgrade file, so unifying now would collide messily. Leaving the two implementations aligned by convention (both gate on status only, both take an operator-choosable wait_status) and opening a follow-up to fold rolling-upgrade onto these entry points after the current PR train settles.
Oddly
force-pushed
the
feat/elasticsearch-node-maintenance-entry-points
branch
from
August 10, 2026 11:38
4114367 to
df8fc29
Compare
…ML doesn't see the JSON colons The search() patterns held "method": "POST" etc. as bare plain scalars, which YAML treated as key: value mappings mid-item and rejected the file with 'did not find expected key'. Fixed by wrapping each list item in double quotes and escaping the inner double quotes. The file wasn't picked up by contracts CI until PR #187 landed; that's why the parse error only surfaced on the rebase pass.
…ch the fake API log
The fake API emits {"body":…, "method":…, "path":…, "port":…}
alphabetically per request, so content that lives in the body (primaries,
500mb, null values) shows up in the log line BEFORE the "method"/"path"
markers. The old assertions expected the opposite ordering and could not
match under regex '.' semantics (which don't cross newlines). Verified
against a copy of the actual fake API log dump from the failed run.
Oddly
added a commit
that referenced
this pull request
Aug 11, 2026
…ce defaults Two follow-ups the #190 merge surfaced against #186's own drift check: scripts/check_argspecs.py: only compare the role's 'main' entry point options against defaults/main.yml. Task-file entry points (node_maintenance_start / _end) take per-invocation parameters like elasticsearch_maintenance_password and _api_url that don't need role-wide defaults — they're inputs to a specific action, not defaults. The wider check was flagging those as spurious drift. roles/elasticsearch/meta/argument_specs.yml: the six role-wide maintenance defaults (elasticsearch_maintenance_wait_status, _health_retries, _health_delay, _wait_health, _require_green, elasticsearch_drain_cluster_settings) were declared under the two task-file entry points but not under 'main'. defaults/main.yml lists them, so the (correctly narrower) drift check now catches that gap and this backfill closes it. Also drops the pre-existing 'role_prefix | Verb' name[casing] warnings that were tripping lint_full for any PR whose diff pulled in the cert_detect_content_mode tasks. The convention is intentional (it matches the commit-message style), so it's moved from warn_list to skip_list rather than papered over with a noqa on every task.
Oddly
added a commit
that referenced
this pull request
Aug 11, 2026
…yml (#186) * chore(ci): fail CI when defaults/main.yml drifts from argument_specs.yml Adds scripts/check_argspecs.py — for every role that ships a meta/argument_specs.yml, compare the set of top-level (non-underscored) variables in defaults/main.yml against the options declared in the spec. Any mismatch either way (var in defaults but not spec, option in spec but no longer in defaults) exits non-zero with the offending names and a pointer to scripts/gen_argspecs.py for regeneration. Wired into test_linting.yml as the last step of the whole-collection lint job (skipped for the per-role invocation so we do not run five duplicate checks per PR). Also promotes gen_argspecs.py from /tmp/ into scripts/ so the fix path is self-contained, and uncomments elasticstack_cert_pass in defaults (one commented line the initial argument_specs PR left as spec-only drift — flipping to elasticstack_cert_pass: "" matches the actual role behaviour and clears the check). * chore(ci): scope drift check to main entry point + backfill maintenance defaults Two follow-ups the #190 merge surfaced against #186's own drift check: scripts/check_argspecs.py: only compare the role's 'main' entry point options against defaults/main.yml. Task-file entry points (node_maintenance_start / _end) take per-invocation parameters like elasticsearch_maintenance_password and _api_url that don't need role-wide defaults — they're inputs to a specific action, not defaults. The wider check was flagging those as spurious drift. roles/elasticsearch/meta/argument_specs.yml: the six role-wide maintenance defaults (elasticsearch_maintenance_wait_status, _health_retries, _health_delay, _wait_health, _require_green, elasticsearch_drain_cluster_settings) were declared under the two task-file entry points but not under 'main'. defaults/main.yml lists them, so the (correctly narrower) drift check now catches that gap and this backfill closes it. Also drops the pre-existing 'role_prefix | Verb' name[casing] warnings that were tripping lint_full for any PR whose diff pulled in the cert_detect_content_mode tasks. The convention is intentional (it matches the commit-message style), so it's moved from warn_list to skip_list rather than papered over with a noqa on every task. * fix(elasticsearch): treat empty elasticstack_cert_pass as unset The 'Set common password for common certificates' set_fact gated on elasticstack_cert_pass being defined, which was safe as long as the role's own default kept the variable commented out. When the drift check work uncommented it to a default of "" (so the arg spec and defaults stay in sync), the gate started firing and blanked elasticsearch_tls_key_passphrase. That empty string cascaded into elasticsearch-keystore.yml's http.ssl.keystore.secure_password Set step, which pipes it as stdin to `elasticsearch-keystore add` — and that binary rejects empty passphrases with a non-zero exit. Reproduced as: elasticsearch_custom passing on main (var commented, gate false) but failing on the drift branch (var defined empty, gate true, empty stdin) even though no keystore code changed between them. The docstring already promised 'Leave empty to use the per-role passphrases', so this changes the gate from 'is defined' to a non-empty check, which matches the documented behaviour and keeps downstream unaffected whether the default line is present or absent. * chore(ci): halve full_stack matrix concurrency to 3 to give heavy scenarios room Committed memory with 6 concurrent slots landed close to 100 GB out of ~120 GB usable on incus-ci. That was fine while the memory-gate timeout was 15 min, but repeated real-world runs with the 45-min timeout still expose the 20 GB elasticstack_default and 13.8 GB es_kibana scenarios to steady starvation: they lose the retry race against smaller peers that release+reacquire faster. Dropping to 3 slots keeps peak committed memory well within reach and lets the heavy jobs actually acquire on the first attempt. Wall clock roughly doubles for the full matrix but every scenario finishes. * fix(kibana): treat empty elasticstack_cert_pass as unset Same regression as the elasticsearch fix: setting elasticstack_cert_pass to "" (which happens as soon as the defaults line is uncommented for argument-spec parity) tripped the 'is defined' gate and silently blanked kibana_tls_key_passphrase. That empty string then reached the _cert_pass argument in cert_generate.yml, so elasticsearch-certutil ran with a bare '--pass' followed by '--out …', which the tool interprets as 'prompt for a password on stdin' — and throws IllegalStateException because CI has no TTY. Change the gate to match the elasticsearch fix (non-empty check).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Playbooks that take a node down outside the role's own flows (OS updates, reboots, storage work) end up reimplementing the drain procedure — health gate, voting exclusion, primaries-only allocation, ML upgrade mode, flush — and each copy grows its own bugs. This adds two documented task entry points so external orchestration can reuse the role's implementation:
node_maintenance_start: health gate → voting exclusion → allocation primaries → ML upgrade mode → optional recovery boost (elasticsearch_drain_cluster_settings) → flush.node_maintenance_end: reverses all of it, restores every boosted key to its baseline inelasticsearch_cluster_settings(instead of nulling it, which silently drops declared settings back to ES defaults), optionally waits for the node to rejoin, and gates on cluster health. Restore steps are best-effort so it can run from analwaysblock and double as a defensive state reset.Argument specs for both entry points included; reference docs updated.
Covered in
tests/integration/node_maintenance_contract.yml: the fake ES API learned voting exclusions and ML upgrade mode, and the test asserts the full drain/restore conversation plus the settings round-trip (boost applied, baseline restored, allocation freed).Follow-up candidate: the internal rolling-upgrade and config-restart flows can be refactored onto these same entry points to remove the remaining duplication.
Summary by CodeRabbit