Support maxUnhealthy to easily express N-1, N-2 scenarios #372
Conversation
|
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. |
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API Server
participant NHC Validator
participant NHC Controller
User->>API Server: Create/Update NodeHealthCheck (with MinHealthy or MaxUnhealthy)
API Server->>NHC Validator: Validate NHC spec
NHC Validator->>API Server: Accept/Reject (enforces mutual exclusivity, value checks)
API Server->>NHC Controller: Store/Update NHC
NHC Controller->>NHC Controller: getMinHealthy(spec, totalNodes)
NHC Controller->>NHC Controller: Determine remediation eligibility
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (12)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
README.md (1)
64-67: Fix grammar and spelling issues in the documentation.The documentation correctly explains the new
maxUnhealthyparameter, but there are language issues that need to be addressed.Apply this diff to fix the grammar and spelling:
-In alternative a complementary parameter can be used to specify the maximum +As an alternative, a complementary parameter can be used to specify the maximum -number of unhealthy nodes simplifying the configuration of N-1, N-2 scenario +number of unhealthy nodes, simplifying the configuration of N-1, N-2 scenarios -when the total number of selected nodes is not known in advance or is exected +when the total number of selected nodes is not known in advance or is expectedconfig/crd/bases/remediation.medik8s.io_nodehealthchecks.yaml (1)
141-142: In-spec references should use camelCase for field namesThe description refers to
MinHealthyandMaxUnhealthywith an initial capital letter, whereas the CRD field names areminHealthyandmaxUnhealthy. Using the exact casing makes grep-based searches and generated docs clearer.- MinHealthy and MaxUnhealthy are configuring the same aspect, + minHealthy and maxUnhealthy configure the same aspect,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
README.md(1 hunks)api/v1alpha1/nodehealthcheck_types.go(1 hunks)api/v1alpha1/nodehealthcheck_webhook.go(3 hunks)api/v1alpha1/nodehealthcheck_webhook_test.go(1 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)bundle/manifests/node-healthcheck-operator.clusterserviceversion.yaml(1 hunks)bundle/manifests/remediation.medik8s.io_nodehealthchecks.yaml(1 hunks)config/crd/bases/remediation.medik8s.io_nodehealthchecks.yaml(1 hunks)config/manifests/base/bases/node-healthcheck-operator.clusterserviceversion.yaml(1 hunks)controllers/nodehealthcheck_controller.go(2 hunks)controllers/nodehealthcheck_controller_test.go(3 hunks)docs/configuration.md(1 hunks)e2e/nhc_e2e_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
api/v1alpha1/nodehealthcheck_webhook.go (1)
api/v1alpha1/nodehealthcheck_types.go (1)
NodeHealthCheck(319-325)
controllers/nodehealthcheck_controller.go (1)
api/v1alpha1/nodehealthcheck_types.go (1)
NodeHealthCheckSpec(57-136)
controllers/nodehealthcheck_controller_test.go (1)
api/v1alpha1/nodehealthcheck_types.go (1)
NodeHealthCheckSpec(57-136)
🪛 LanguageTool
README.md
[uncategorized] ~64-~64: A comma might be missing here.
Context: ...s, by percentage or absolute number. In alternative a complementary parameter can be used t...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
🔇 Additional comments (24)
e2e/nhc_e2e_test.go (1)
39-45: LGTM! Correct usage of the new maxUnhealthy field.The test properly demonstrates the new
maxUnhealthyparameter as an alternative tominHealthy. Usingintstr.FromInt(1)formaxUnhealthyeffectively limits remediation to one unhealthy node at a time, which aligns with the PR's goal of simplifying N-1 scenarios.api/v1alpha1/zz_generated.deepcopy.go (1)
120-124: LGTM! Correctly generated deep copy logic.The auto-generated deep copy code properly handles the new
MaxUnhealthyfield, following the same pattern as other pointer fields in the struct. The nil check and new allocation are implemented correctly.docs/configuration.md (1)
52-60: Excellent documentation update!The table clearly documents the new
maxUnhealthyfield and its relationship withminHealthy. The mutual exclusivity is well explained, and the formatting improvements make the documentation more readable. The change from a default value to "n/a" correctly reflects that at least one of the fields must be explicitly set.api/v1alpha1/nodehealthcheck_types.go (2)
86-87: LGTM! Clear documentation of mutual exclusivity.The updated documentation for
MinHealthycorrectly explains its relationship with the newMaxUnhealthyfield, making it clear that they cannot be used simultaneously.
94-104: Excellent implementation of the MaxUnhealthy field!The new field is properly defined with:
- Consistent validation annotations matching
MinHealthy- Clear documentation explaining the 0% edge case
- Proper kubebuilder and operator-sdk annotations
- Explicit mention of mutual exclusivity with
MinHealthyThe field definition aligns perfectly with the PR objectives of providing an alternative to
MinHealthyfor N-1, N-2 scenarios.api/v1alpha1/nodehealthcheck_webhook.go (3)
43-43: LGTM: Consistent error constant additionThe new
maxUnhealthyErrorconstant follows the same pattern as the existingminHealthyError, maintaining consistency in the codebase.
109-109: LGTM: Function rename reflects expanded scopeThe function call correctly reflects the expanded validation that now handles both
MinHealthyandMaxUnhealthyfields.
129-144: LGTM: Comprehensive validation logicThe unified validation function correctly implements all requirements:
- Enforces mutual exclusivity between
MinHealthyandMaxUnhealthy- Requires at least one field to be specified
- Validates both fields for negative integer values
- Maintains consistent error messaging
The implementation follows established patterns and handles all edge cases properly.
config/manifests/base/bases/node-healthcheck-operator.clusterserviceversion.yaml (2)
59-66: LGTM: Clear and comprehensive field descriptorThe new
maxUnhealthyspec descriptor provides excellent documentation:
- Clearly explains the field's purpose and behavior
- Documents value constraints (positive integers/percentages, 100% cap)
- Explicitly mentions mutual exclusivity with
minHealthy- Consistent formatting with other descriptors
70-72: LGTM: Updated description maintains consistencyThe updated
minHealthydescription correctly documents the mutual exclusivity constraint withmaxUnhealthy, ensuring users understand the relationship between these fields.bundle/manifests/remediation.medik8s.io_nodehealthchecks.yaml (2)
134-146: LGTM: Well-structured CRD schema definitionThe
maxUnhealthyfield schema is correctly implemented:
- Uses appropriate
anyOfpattern for integer/string values- Includes comprehensive description with clear constraints
- Validation pattern matches
minHealthyfor consistency- Properly configured with
x-kubernetes-int-or-string: true- Documents mutual exclusivity relationship
156-157: LGTM: Consistent documentation updateThe updated
minHealthydescription correctly documents the mutual exclusivity constraint, maintaining consistency with themaxUnhealthyfield description.bundle/manifests/node-healthcheck-operator.clusterserviceversion.yaml (2)
99-106: LGTM: Consistent spec descriptor across manifestsThe
maxUnhealthyspec descriptor matches the base manifest, ensuring consistent documentation across all ClusterServiceVersion files. The description is clear and comprehensive.
110-112: LGTM: Maintains manifest consistencyThe
minHealthydescription update ensures consistency across all ClusterServiceVersion manifests, properly documenting the mutual exclusivity constraint.api/v1alpha1/nodehealthcheck_webhook_test.go (4)
75-76: LGTM: Proper test isolationSetting
MaxUnhealthytonilensures the test properly isolates theMinHealthynegative value validation scenario.
83-93: LGTM: Comprehensive negative value validation testThe test properly validates negative
maxUnhealthyvalues:
- Correctly isolates the test by setting
MinHealthyto nil- Uses the appropriate error constant
- Follows the established test pattern
95-105: LGTM: Proper mutual exclusivity validation testThe test correctly validates that
minHealthyandmaxUnhealthycannot be specified simultaneously, checking for the appropriate error message.
107-116: LGTM: Complete requirement validation testThe test correctly validates that at least one of
minHealthyormaxUnhealthymust be specified, checking for the appropriate error message.controllers/nodehealthcheck_controller_test.go (4)
122-148: LGTM: Well-organized test structure for minHealthy validation.The reorganization of minHealthy tests into a proper
Context("minHealthy")block improves test readability and structure. The new test cases appropriately cover percentage validation scenarios and maintain consistency with the existing skipped test for negative values.
150-176: LGTM: Comprehensive test coverage for maxUnhealthy validation.The maxUnhealthy test cases appropriately mirror the minHealthy tests, which makes sense given their similar validation requirements. The tests cover the key scenarios: invalid percentages over 100%, negative values (properly skipped with explanation), and valid percentage ranges.
277-413: LGTM: Excellent table-driven test coverage for getMinHealthy function.This comprehensive table test covers all the critical scenarios for the new
getMinHealthyfunction:✅ Positive cases: Both absolute values and percentages for
minHealthyandmaxUnhealthy
✅ Edge cases: 0%, 100%, and rounding scenarios
✅ Error cases: Negative values, mutual exclusivity, neither field set, and maxUnhealthy exceeding total nodes
✅ Calculation verification: Correct percentage calculations and minHealthy derivation from maxUnhealthyThe test entries correctly validate the mathematical relationships (e.g., 75% of 10 nodes = 8 after rounding, maxUnhealthy of 8 → minHealthy of 2).
684-687: LGTM: Clean helper function for test data creation.The
ptrToIntStrhelper function provides a concise way to create*intstr.IntOrStringpointers from string values, improving the readability of the table test entries.controllers/nodehealthcheck_controller.go (2)
332-335: LGTM: Clean refactoring with enhanced error context.The refactoring from direct
intstr.GetScaledValueFromIntOrPercentcall to the newgetMinHealthyhelper function is well-implemented. The enhanced error logging now includes bothminHealthyandmaxUnhealthyvalues, providing better debugging context for administrators.
883-905: LGTM: Robust implementation with comprehensive validation.The
getMinHealthyhelper function correctly implements the mutual exclusivity requirement and provides thorough validation:✅ Mutual exclusivity enforcement (lines 884-886): Prevents both parameters from being set simultaneously
✅ Negative value validation: Applied to bothMinHealthyandMaxUnhealthycalculations
✅ Upper bound validation (lines 899-901): EnsuresMaxUnhealthydoesn't exceed total node count
✅ Mandatory field validation (line 904): Requires at least one parameter to be specified
✅ Correct mathematical conversion (line 902):total - maxUnhealthyaccurately represents the minimum healthy nodesThe implementation aligns perfectly with the PR objectives of supporting N-1, N-2 scenarios through the
MaxUnhealthyparameter.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
64-67: Polish wording & fix typos in the new ‘maxUnhealthy’ paragraph
Current phrasing is a bit awkward (“In alternative”), has a comma missing, plural/typo issues (“scenario”, “exected”). The paragraph reads more smoothly and professionally with the tweaks below.-In alternative a complementary parameter can be used to specify the maximum -number of unhealthy nodes simplifying the configuration of N-1, N-2 scenario -when the total number of selected nodes is not known in advance or is exected -to change. +Alternatively, a complementary `maxUnhealthy` parameter can be used to specify +the maximum number of unhealthy nodes, simplifying the configuration of N-1 or +N-2 scenarios when the total number of selected nodes is not known in advance +or is expected to change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
README.md(1 hunks)api/v1alpha1/nodehealthcheck_types.go(1 hunks)api/v1alpha1/nodehealthcheck_webhook.go(3 hunks)api/v1alpha1/nodehealthcheck_webhook_test.go(1 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)bundle/manifests/node-healthcheck-operator.clusterserviceversion.yaml(1 hunks)bundle/manifests/remediation.medik8s.io_nodehealthchecks.yaml(1 hunks)config/crd/bases/remediation.medik8s.io_nodehealthchecks.yaml(1 hunks)config/manifests/base/bases/node-healthcheck-operator.clusterserviceversion.yaml(1 hunks)controllers/nodehealthcheck_controller.go(2 hunks)controllers/nodehealthcheck_controller_test.go(3 hunks)docs/configuration.md(1 hunks)e2e/nhc_e2e_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- e2e/nhc_e2e_test.go
- api/v1alpha1/zz_generated.deepcopy.go
- docs/configuration.md
- config/manifests/base/bases/node-healthcheck-operator.clusterserviceversion.yaml
- api/v1alpha1/nodehealthcheck_types.go
- bundle/manifests/remediation.medik8s.io_nodehealthchecks.yaml
- api/v1alpha1/nodehealthcheck_webhook.go
- controllers/nodehealthcheck_controller.go
- bundle/manifests/node-healthcheck-operator.clusterserviceversion.yaml
- config/crd/bases/remediation.medik8s.io_nodehealthchecks.yaml
- controllers/nodehealthcheck_controller_test.go
- api/v1alpha1/nodehealthcheck_webhook_test.go
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~64-~64: A comma might be missing here.
Context: ...s, by percentage or absolute number. In alternative a complementary parameter can be used t...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
| @@ -2243,6 +2273,144 @@ var _ = Describe("Node Health Check CR", func() { | |||
| }) | |||
|
|
|||
| }) | |||
|
|
|||
| Context("getAbsoluteMinHealthy", func() { | |||
| DescribeTable("should return absolute minHealthy", | |||
docs/configuration.md
Outdated
| | _remediationTemplate_ | yes but mutually exclusive with below | n/a | A [ObjectReference](https://kubernetes.io/docs/reference/kubernetes-api/common-definitions/object-reference/) to a remediation template provided by a remediation provider. See details below. | | ||
| | _escalatingRemediations_ | yes but mutually exclusive with above | n/a | A list of ObjectReferences to a remediation template with order and timeout. See details below. | | ||
| | _minHealthy_ | one of _minHealthy_ or _maxUnhealthy_ should be set | n/a | The minimum number of healthy nodes selected by this CR for allowing further remediation. Percentage or absolute number. _minHealthy_ and _maxUnhealthy_ are mutually exclusive. | | ||
| | _maxUnhealthy_ | one of _minHealthy_ or _maxUnhealthy_ should be set | n/a | The maximum number of unhealthy nodes selected by this CR for allowing further remediation. Percentage or absolute number. _minHealthy_ and _maxUnhealthy_ are mutually exclusive. | |
There was a problem hiding this comment.
can we please note somewhere that maxUnhealthy should not be used with remediators which delete nodes, e.g. MachineDeletionRemediation, because deleting unhealthy nodes breaks counting (un)healthy nodes? Maybe here and also at new the field in types.go?
slintes
changed the title
|
/ok-to-test |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
README.md(1 hunks)api/v1alpha1/nodehealthcheck_types.go(1 hunks)api/v1alpha1/nodehealthcheck_webhook.go(4 hunks)api/v1alpha1/nodehealthcheck_webhook_test.go(1 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)bundle/manifests/node-healthcheck-operator.clusterserviceversion.yaml(1 hunks)bundle/manifests/remediation.medik8s.io_nodehealthchecks.yaml(1 hunks)config/crd/bases/remediation.medik8s.io_nodehealthchecks.yaml(1 hunks)config/manager/kustomization.yaml(1 hunks)config/manifests/base/bases/node-healthcheck-operator.clusterserviceversion.yaml(1 hunks)controllers/nodehealthcheck_controller.go(2 hunks)controllers/nodehealthcheck_controller_test.go(3 hunks)docs/configuration.md(1 hunks)e2e/nhc_e2e_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- api/v1alpha1/zz_generated.deepcopy.go
- api/v1alpha1/nodehealthcheck_webhook_test.go
- bundle/manifests/remediation.medik8s.io_nodehealthchecks.yaml
- docs/configuration.md
- api/v1alpha1/nodehealthcheck_webhook.go
- config/crd/bases/remediation.medik8s.io_nodehealthchecks.yaml
- api/v1alpha1/nodehealthcheck_types.go
- config/manifests/base/bases/node-healthcheck-operator.clusterserviceversion.yaml
- controllers/nodehealthcheck_controller.go
- e2e/nhc_e2e_test.go
- README.md
- bundle/manifests/node-healthcheck-operator.clusterserviceversion.yaml
- controllers/nodehealthcheck_controller_test.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mshitrit
PR: medik8s/node-healthcheck-operator#365
File: controllers/resources/manager.go:319-364
Timestamp: 2025-05-28T07:55:11.390Z
Learning: In the node-healthcheck-operator HandleHealthyNode method, when calcCrDeletionDelay fails with an error, the intended behavior is to log the error and proceed with CR deletion (treating it as "no delay configured") rather than aborting reconciliation. This prevents the system from getting stuck when delay calculations fail due to issues like malformed annotations.
Learnt from: mshitrit
PR: medik8s/node-healthcheck-operator#365
File: controllers/resources/manager.go:319-335
Timestamp: 2025-05-28T08:18:35.543Z
Learning: In the node-healthcheck-operator HandleHealthyNode method, the UpdateStatusNodeDelayedHealthy call with unsafe type cast to *NodeHealthCheck is actually safe because HealthyDelayContextKey is only set for NodeHealthCheck controllers, not MachineHealthCheck controllers. This means shortestDelay will always be 0 for MachineHealthCheck, preventing the unsafe cast line from being reached.
Learnt from: mshitrit
PR: medik8s/node-healthcheck-operator#365
File: controllers/resources/status.go:74-81
Timestamp: 2025-05-28T07:42:13.767Z
Learning: In the Node Healthcheck Operator's status management for controllers/resources/status.go, when a node's healthy delay period expires, the entire unhealthy node entry is removed from the status via UpdateStatusNodeHealthy rather than just resetting the HealthyDelayed flag to false. The state transition flow is: unhealthy -> healthy with delay (HealthyDelayed=true) -> completely healthy (removed from unhealthy nodes list).
config/manager/kustomization.yaml
Outdated
| images: | ||
| - name: controller | ||
| newName: quay.io/medik8s/node-healthcheck-operator | ||
| newTag: latest |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin the controller image to a fixed tag or digest instead of latest
Using latest makes deployments non-deterministic and risks pulling incompatible images when new versions are pushed. Pin to a released semantic version or, preferably, an immutable digest.
- newName: quay.io/medik8s/node-healthcheck-operator
- newTag: latest
+ newName: quay.io/medik8s/node-healthcheck-operator
+ # pin to a specific release tag or digest for reproducibility
+ newTag: v0.8.1 # example – update to the actual release
+# digest: sha256:xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # optional but safer🤖 Prompt for AI Agents
In config/manager/kustomization.yaml lines 12 to 15, the controller image tag is
set to 'latest', which causes non-deterministic deployments. Replace 'latest'
with a fixed semantic version tag or an immutable image digest to ensure
consistent and reliable deployments.
|
/test 4.18-openshift-e2e |
|
/test 4.17-openshift-e2e |
|
/test 4.16-openshift-e2e |
tiraboschi
force-pushed
the
maxUnhealthy
branch
2 times, most recently
from
e9b1e57 to
43db298
Compare
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 maxUnhealthy as an alternative to minHealthy. This change: - introduces a new maxUnhealthy API as a mutually exclusive alternative to minHealthy. - Only alters the logic used to compute the minHealthy value (similar to how percentages are handled) when the value is expressed with maxUnhealthy. - Keeps the internal processing and validation unchanged. Signed-off-by: Simone Tiraboschi <[email protected]>
|
/test 4.18-openshift-e2e |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: slintes, tiraboschi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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
maxUnhealthyas an alternative tominHealthy.Changes made
This change:
maxUnhealthyAPI as a mutually exclusive alternative tominHealthy.minHealthyvalue (similar to how percentages are handled) when the value is expressed withmaxUnhealthy.Which issue(s) this PR fixes
Fixes https://issues.redhat.com/browse/RHWA-88
It's an alternative to #371
Test plan
maxUnhealthy.Summary by CodeRabbit
New Features
maxUnhealthyfield for NodeHealthCheck resources, allowing remediation based on the maximum number or percentage of unhealthy nodes.minHealthyandmaxUnhealthyfields, which are mutually exclusive.Bug Fixes
minHealthyormaxUnhealthycan be set, and neither can have negative values.Documentation
maxUnhealthyoption and its usage constraints.Tests