Support negative minHealthy to easily express N-1, N-2 scenarios #371
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tiraboschi The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @tiraboschi. Thanks for your PR. I'm waiting for a medik8s member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
WalkthroughThe changes introduce support for negative values in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API Server
participant NodeHealthCheck Controller
User->>API Server: Create/Update NodeHealthCheck (MinHealthy: -1)
API Server->>NodeHealthCheck Controller: Reconcile event
NodeHealthCheck Controller->>NodeHealthCheck Controller: getAbsoluteMinHealthy(MinHealthy, totalNodes)
NodeHealthCheck Controller->>NodeHealthCheck Controller: Calculate threshold (totalNodes + MinHealthy)
NodeHealthCheck Controller-->>API Server: Update status/remediation as needed
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (13)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (11)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
The minHealthy parameter already allows cluster admins to prevent NHC from remediating too many nodes at once, protecting overall compute capacity. However, expressing N-1 or N-2 scenarios, where only one or two nodes should be remediated at a time, currently requires knowing the exact number of nodes in advance and updating the configuration as nodes are added. This change introduces support for negative minHealthy values, which are interpreted as "total number of selected nodes minus the provided value". For example, -1 means "all but one node must remain healthy". This change: - Does not introduce a new API, ensuring full backward compatibility and avoiding ambiguity. - Only alters the logic used to compute the absolute minHealthy value (similar to how percentages are handled). - Keeps the internal processing and validation unchanged. Signed-off-by: Simone Tiraboschi <[email protected]>
tiraboschi
force-pushed
the
negative_minHealthy
branch
from
d5193b4 to
048346d
Compare
| @@ -60,7 +60,10 @@ Remediation is not always the correct response to a failure. Especially in | |||
| larger clusters, we want to protect against failures that appear to take out | |||
| large portions of compute capacity but are really the result of failures on or | |||
| near the control plane. For this reason, the NHC CR includes the ability to | |||
| define a minimum number of healthy nodes, by percentage or absolute number. | |||
| define a minimum number of healthy nodes, by percentage or integer number. | |||
There was a problem hiding this comment.
I think the explanation given in the type definition looks clearer. Could we have something similar also here?
| absMinHealthy = total + minHealthy | ||
| } | ||
| if absMinHealthy < 0 { | ||
| err = fmt.Errorf("absolute minHealthy is negative: %d", absMinHealthy) |
There was a problem hiding this comment.
IIUC, the error message isn't catching completely the problem, unless you know the code, which is that the total number of selected Node is lower than the nodes we want to keep healthy, isn't it?
| 8, | ||
| nil, | ||
| ), | ||
| Entry("positive percentage value with round-up", |
|
closing in favor of #372 that got a wider consensus |
Why we need this PR
The
minHealthyparameter already allows cluster admins to prevent NHC from remediating too many nodes at once, protecting overall compute capacity.However, expressing N-1 or N-2 scenarios, where only one or two nodes should be remediated at a time,
currently requires knowing the exact number of nodes in advance and updating the configuration as nodes are added.
This change introduces support for negative minHealthy values, which are interpreted as
total number of selected nodes minus the provided value.
For example, -1 means ll but one node must remain healthy.
Changes made
This change:
Which issue(s) this PR fixes
Fixes https://issues.redhat.com/browse/RHWA-88
Test plan
minHealthyare properly handledSummary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests