OCPBUGS-86075: docs(nodepool): fixing incomplete stuck node drain documentation in section Scaling To Zero - #8544
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates the NodePool "Scaling To Zero" documentation: it explains why node drains can block when protected pods cannot be rescheduled (including when all nodes are removed at once), adds an explicit important warning, expands prevention guidance with a NodePool YAML example setting .spec.nodeDrainTimeout and .spec.nodeVolumeDetachTimeout to values > 0s, updates the API reference wording, and notes an alternative non-draining approach using machine annotations. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
PoornimaSingour
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@PoornimaSingour: This pull request references Jira Issue OCPBUGS-86075, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@PoornimaSingour: This pull request references Jira Issue OCPBUGS-86075, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
area/documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/content/how-to/automated-machine-management/nodepool-lifecycle.md (1)
70-104: ⚡ Quick winConsider adding a diagram to illustrate the drain blocking scenario.
While the textual explanation is clear, a diagram could help visualize the drain process and why it blocks when all nodes are removed simultaneously. This could be a simple flowchart or state diagram showing:
- Initial state: Multiple nodes with PDB-protected pods
- Drain attempt: Pods cannot be rescheduled (no available nodes)
- Outcome: Drain blocks vs. timeout-based removal
As per coding guidelines, markdown files should provide service architecture diagrams using mermaid or ASCII format where applicable.
📊 Example mermaid diagram
```mermaid graph TD A[Scale NodePool to 0] --> B{All nodes being removed?} B -->|Yes| C{PDB-protected pods present?} B -->|No| D[Normal drain process] C -->|Yes| E{Drain timeout configured?} C -->|No| D E -->|Yes| F[Wait for timeout, then remove nodes] E -->|No| G[Drain blocks indefinitely] D --> H[Nodes removed successfully] F --> H style G fill:`#ffcccc` style F fill:`#ccffcc` style H fill:`#ccffcc````
🤖 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/content/how-to/automated-machine-management/nodepool-lifecycle.md` around lines 70 - 104, Add a simple mermaid diagram illustrating the drain-blocking scenario into the "Scaling To Zero" section to complement the text; place it after the paragraph that lists conditions preventing drains and before the "Prevention" heading, and reference the decision points shown in the reviewer example (e.g., "Scale NodePool to 0", "PDB-protected pods present?", "Drain timeout configured?") so readers can visually connect to the existing fields .spec.nodeDrainTimeout and .spec.nodeVolumeDetachTimeout in the NodePool example.
🤖 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.
Nitpick comments:
In `@docs/content/how-to/automated-machine-management/nodepool-lifecycle.md`:
- Around line 70-104: Add a simple mermaid diagram illustrating the
drain-blocking scenario into the "Scaling To Zero" section to complement the
text; place it after the paragraph that lists conditions preventing drains and
before the "Prevention" heading, and reference the decision points shown in the
reviewer example (e.g., "Scale NodePool to 0", "PDB-protected pods present?",
"Drain timeout configured?") so readers can visually connect to the existing
fields .spec.nodeDrainTimeout and .spec.nodeVolumeDetachTimeout in the NodePool
example.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0fb94c7f-e1db-41e2-a2ba-a74f4568f2c2
⛔ Files ignored due to path filters (1)
docs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.md
📒 Files selected for processing (1)
docs/content/how-to/automated-machine-management/nodepool-lifecycle.md
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Thanks for fixing the broken documentation — the truncated bullet points and typo fix are clearly needed. A few comments inline.
|
|
||
| - The hosted cluster contains `PodDisruptionBudgets` that require at least | ||
| - The hosted cluster contains pods that use `PersistentVolumes`` | ||
| - The hosted cluster contains `PodDisruptionBudgets` that require at least one healthy pod, preventing eviction when there are no other nodes to reschedule onto. |
There was a problem hiding this comment.
[blocking] aggregated-docs.md is auto-generated by make docs-aggregate (via hack/tools/docs-aggregator/main.go). Manual edits here will be overwritten the next time anyone runs make update.
Please revert all changes to this file and regenerate it instead:
make docs-aggregateThere was a problem hiding this comment.
@bryan-cox ,Good learning here for me. Reverted manual edits and regenerated with make docs-aggregate. Thank you !!!
| namespace: clusters | ||
| spec: | ||
| nodeDrainTimeout: 1m | ||
| nodeVolumeDetachTimeout: 5m |
There was a problem hiding this comment.
[suggestion] These timeout values are very aggressive for a documentation example that users will copy-paste:
nodeDrainTimeout: 1m— in production, graceful drain can legitimately take several minutes (pods with longterminationGracePeriodSeconds, slow preStop hooks). A 1-minute timeout risks data loss.- The relative ordering is inverted — drain typically takes longer than volume detach, so
nodeDrainTimeoutshould generally be >=nodeVolumeDetachTimeout.
Consider more conservative values:
nodeDrainTimeout: 30m
nodeVolumeDetachTimeout: 10mThere was a problem hiding this comment.
Updated the values, nodeDrainTimeout: 30m and nodeVolumeDetachTimeout: 10m. Agreed that 1m is too aggressive and few customer might just copy paste it.
|
|
||
| !!! important | ||
|
|
||
| This is expected behavior. When all nodes are being removed simultaneously, pods protected by PDBs have nowhere to be rescheduled, so the drain operation blocks indefinitely. Configure drain timeouts to ensure nodes are removed after a bounded period. |
There was a problem hiding this comment.
[nit] The text says pods have nowhere to be rescheduled — but the drain blocks because eviction is refused by the PDB admission check, not because rescheduling fails.
Suggested rewording:
This is expected behavior. When all nodes are removed simultaneously, pods protected by PodDisruptionBudgets cannot be evicted because the PDB constraints cannot be satisfied with no remaining nodes. As a result, the drain operation blocks indefinitely. Configure
nodeDrainTimeoutto ensure nodes are eventually removed after a bounded period.
There was a problem hiding this comment.
Thanks, updated the wording to clarify that eviction.
| !!! note | ||
| See the [Hypershift API reference page](../../reference/api.md) for more details. | ||
| See the [HyperShift API reference page](../../reference/api.md) for more details on these fields. | ||
| For an alternative approach that skips draining entirely via machine annotations, see [Scaling down data plane to Zero](scale-to-zero-dataplane.md). |
There was a problem hiding this comment.
[nit] These two lines will render as a single dense paragraph in MkDocs since there is no blank line between them. Add a blank indented line between them for better readability.
There was a problem hiding this comment.
@bryan-cox I have fixed it. Also aligned the !!! note block format to match the !!! important blocks in the same file.
There was a problem hiding this comment.
There is a issue here if we are adding blank line then hyperlinks are not showing in the doc preview see https://github.com/PoornimaSingour/hypershift/blob/f82366f09df63b6b2a990a8a7854a8e07c963331/docs/content/how-to/automated-machine-management/nodepool-lifecycle.md commit.
The issue is how GitHub renders this — GitHub doesn't understand MkDocs !!! note admonition syntax.
It treats the indented content as a code block or plain text, so the markdown links inside don't render as hyperlinks.
There was a problem hiding this comment.
Seems like it will look like this in the Github but in upstream it will come in hyperlinks. Changes are done
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/content/how-to/automated-machine-management/nodepool-lifecycle.md`:
- Around line 89-99: Add a language identifier to the fenced code block that
begins with "apiVersion: hypershift.openshift.io/v1beta1" so the block is marked
as YAML (e.g., change the opening ``` to ```yaml) to satisfy markdownlint MD040
and improve rendering; locate the block that contains the NodePool spec
(includes nodeDrainTimeout and nodeVolumeDetachTimeout) and update its fence
accordingly.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5a1ad9ef-2920-42d5-9120-b39f3e075307
⛔ Files ignored due to path filters (1)
docs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.md
📒 Files selected for processing (1)
docs/content/how-to/automated-machine-management/nodepool-lifecycle.md
012fb10 to
04a05dc
Compare
04a05dc to
373fdb9
Compare
373fdb9 to
f82366f
Compare
f82366f to
8ade07c
Compare
Complete truncated bullet points in the Scaling To Zero section, fix PDB eviction explanation, add NodePool YAML example with conservative drain timeout values, and improve MkDocs admonition formatting. Regenerate aggregated-docs.md via make docs-aggregate. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
8ade07c to
7f54bba
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, PoornimaSingour The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/lgtm |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
/test |
|
@PoornimaSingour: The The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test ? |
|
@PoornimaSingour: The following commands are available to trigger required jobs: The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/verified This is a documentation-only change (NodePool lifecycle docs). All CI checks pass — lint, verify, gitlint, build docs, |
|
@PoornimaSingour: The DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/verified by me |
openshift-ci-robot
added
the
verified
|
@PoornimaSingour: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@PoornimaSingour: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@PoornimaSingour: Jira Issue Verification Checks: Jira Issue OCPBUGS-86075 Jira Issue OCPBUGS-86075 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Fix included in release 5.0.0-0.nightly-2026-05-25-135947 |
This branch was successfully deployed
5 participants
What this PR does / why we need it:
As a part of this PR below has been fixed in upstream document of NodePool lifecycle in Scaling To Zero section :
Which issue(s) this PR fixes:
Fixes : https://redhat.atlassian.net/browse/OCPBUGS-86075
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit