Replace maxUnavailable auto-correction with validation that fails NNCP when value is 0 - #1550
Replace maxUnavailable auto-correction with validation that fails NNCP when value is 0#1550emy wants to merge 9 commits into
Conversation
kubevirt-prow
Bot
added
kind/bug
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
…P when value is 0 Signed-off-by: Emilia Desch <emy@redhat.com>
kubevirt-prow
Bot
added
dco-signoff: yes
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the NodeNetworkConfigurationPolicy controller to enforce that the computed maxUnavailable value must result in at least 1 node. If maxUnavailable is less than 1, the policy fails with FailedToConfigure. The feedback highlights a critical issue where returning a standard error on validation failure in both incrementUnavailableNodeCount and shouldAbortReconcile causes an infinite requeue loop and redundant retries. It is recommended to use a custom non-retryable error type to prevent this. Additionally, clarifying the API documentation for MaxUnavailable is suggested to avoid confusion regarding node availability during updates.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if maxUnavailable < 1 { | ||
| errMsg := fmt.Sprintf("invalid maxUnavailable configuration: computed value is %d. The maxUnavailable field must result in at least 1 node. Either increase the value (e.g., set to 1 or higher) or use a higher percentage (e.g., \"1%%\")", maxUnavailable) | ||
| r.Log.Error(errors.New(errMsg), "Invalid maxUnavailable configuration") | ||
| policyconditions.SetPolicyFailedToConfigure(&policy.Status.Conditions, errMsg) | ||
| updateErr := r.Client.Status().Update(ctx, policy) | ||
| if updateErr != nil { | ||
| return updateErr | ||
| } | ||
| return errors.New(errMsg) | ||
| } |
There was a problem hiding this comment.
Issue: Infinite Requeue Loop and Redundant Retries on Validation Failure
Returning a standard error from incrementUnavailableNodeCount when maxUnavailable < 1 causes two major issues:
- Infinite Requeue Loop: The error propagates back to
Reconcile, which returns it directly to the controller-runtime manager. This causes the controller to requeue the request indefinitely with exponential backoff, leading to high CPU usage, API server thrashing, and log spam. Since a validation error is terminal for the current policy generation, the reconciliation should stop without requeuing (i.e., returnctrl.Result{}, nil). - Redundant Retries inside
retry.OnError: The validation check is executed inside aretry.OnErrorblock with a classifier that retries on all errors (func(error) bool { return true }). Consequently, whenmaxUnavailable < 1is detected, the controller will log the error, update the policy status, and retry 5 times (the default retry limit) for a single reconcile attempt, resulting in redundant API calls and log spam.
Recommendation
To resolve this, you should:
- Define a custom non-retryable error type (e.g.,
InvalidMaxUnavailableError). - Update the
retry.OnErrorclassifier to not retry when the error is of this type. - Modify
Reconcileto catch this error and returnctrl.Result{}, nilto prevent requeuing.
| if maxUnavailable < 1 { | ||
| errMsg := fmt.Sprintf("invalid maxUnavailable configuration: computed value is %d. The maxUnavailable field must result in at least 1 node. Either increase the value (e.g., set to 1 or higher) or use a higher percentage (e.g., \"1%%\")", maxUnavailable) | ||
| logger.Error(errors.New(errMsg), "Invalid maxUnavailable configuration") | ||
| policyconditions.SetPolicyFailedToConfigure(&instance.Status.Conditions, errMsg) | ||
| updateErr := r.Client.Status().Update(ctx, instance) | ||
| if updateErr != nil { | ||
| return false, updateErr | ||
| } | ||
| return false, errors.New(errMsg) | ||
| } |
There was a problem hiding this comment.
Issue: Infinite Requeue Loop on Validation Failure
Similar to the check in incrementUnavailableNodeCount, returning a standard error here when maxUnavailable < 1 propagates to Reconcile, which returns it directly to the controller-runtime manager. This causes the controller to requeue the request indefinitely.
Since this is a terminal validation error, it should be returned as a non-retryable error type, and Reconcile should handle it by returning ctrl.Result{}, nil to stop reconciliation without requeuing.
| // The computed value must result in at least 1 node being available for updates. | ||
| // If set to 0 or a percentage that rounds to 0, the policy will fail with FailedToConfigure. |
There was a problem hiding this comment.
Suggestion: Clarify API Documentation Comment
The phrase "at least 1 node being available for updates" is confusing because in Kubernetes parlance, "available" typically refers to nodes that are ready and serving, whereas maxUnavailable specifies the number of nodes that can be unavailable (i.e., undergoing updates) at a time.
To prevent confusion, consider rephrasing this to clarify that the computed value must allow at least 1 node to be updated (or be unavailable) at a time.
Example:
// The computed value must result in at least 1 node being allowed to undergo updates at a time.…ailableError type Signed-off-by: Emilia Desch <emy@redhat.com>
Signed-off-by: Emilia Desch <emy@redhat.com>
|
@emy why we don't implement this as a CEL validation of > 1 for MaxUnavailable ? |
You cannot make CEL validation stricter once you introduced API because your already existing objects won't be forward-compatible. Correct me where I am wrong |
well same thing will happend if we do it at the controller, the old NNCP will have NNCP status with failed to configure, doing it at CEL level is more aparent (maybe customer's script are not checking the status so they have a fail possitive). |
|
Yeah actually over all those years I never saw anyone using |
Let's go with CEL it will make issue more aparent and there wil not be false possitive of users not checking status but finding that networking is not "networketing". |
Signed-off-by: Emilia Desch <emy@redhat.com>
Signed-off-by: Emilia Desch <emy@redhat.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR changes maxUnavailable behavior by removing the previous “auto-correct to minimum” logic and instead enforcing an API-level validation that rejects maxUnavailable: 0 (and 0%), with accompanying test updates.
Changes:
- Add CRD/API validation to reject
spec.maxUnavailablewhen set to zero (int or percent). - Remove
maxUnavailableauto-correction behavior in scaling logic so zero stays zero. - Add/adjust E2E and unit tests to reflect the new validation and scaling behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/handler/utils.go | Adds a helper to create/update NNCP with explicit maxUnavailable for validation tests. |
| test/e2e/handler/nodes_test.go | Updates call site for renamed helper (deleteNodeNetworkStates). |
| test/e2e/handler/nnce_conditions_test.go | Adds E2E coverage ensuring API rejects maxUnavailable values of 0 and 0%. |
| pkg/node/nodes_test.go | Updates unit test expectations for zero maxUnavailable scaling results. |
| pkg/node/nodes.go | Removes auto-correction/clamping of scaled maxUnavailable values. |
| api/shared/nodenetworkconfigurationpolicy_types.go | Adds kubebuilder XValidation rule/message for maxUnavailable > 0. |
| deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml | Propagates the new CRD validation rule/message into deploy CRDs. |
| bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml | Propagates the new CRD validation rule/message into bundle manifests. |
| controllers/handler/nodenetworkconfigurationpolicy_controller_test.go | Adds enactment label used by controller logic/predicates in tests. |
| controllers/handler/nodenetworkconfigurationpolicy_controller.go | Minor formatting-only change (blank line). |
Comments suppressed due to low confidence (1)
pkg/node/nodes.go:1
- With the clamping/auto-correction removed, this function now can return
0for inputs like0or0%. Even if new NNCPs are blocked by CRD validation going forward, clusters upgrading from older versions may still have existing persisted NNCPs withmaxUnavailable=0; returning0can cause the controller’s “max unavailable” gating to behave pathologically (e.g., permanently block progress if the logic requires allowing at least one node). Consider adding a backward-compatibility safeguard at reconciliation time for existing objects (e.g., treat 0 as invalid and mark policy degraded with a clear reason, or clamp to 1 with a warning), so upgrades don’t create stuck policies.
/*
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Must be greater than 0. When set as a percentage, it must be a positive percentage. | ||
| // +optional | ||
| //nolint:lll | ||
| // +kubebuilder:validation:XValidation:rule="!has(self) || (self.type == 0 && self.intVal > 0) || (self.type == 1 && int(self.strVal.replace('%', '')) > 0)",message="maxUnavailable must be greater than 0" | ||
| MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty"` |
| It("should be rejected by API validation when maxUnavailable is 0", func() { | ||
| By("Attempt to apply policy with maxUnavailable: 0") | ||
| err := setDesiredStateWithPolicyMaxUnavailableAndNodeSelector( | ||
| TestPolicy, | ||
| linuxBrUp(bridge1), | ||
| intstr.FromInt(0), | ||
| map[string]string{"node-role.kubernetes.io/worker": ""}, | ||
| ) | ||
| Expect(err).To(HaveOccurred(), "API should reject policy with maxUnavailable: 0") | ||
| Expect(err.Error()).To(ContainSubstring("maxUnavailable must be greater than 0")) | ||
| }) | ||
|
|
||
| It("should be rejected by API validation when maxUnavailable is 0%", func() { | ||
| By("Attempt to apply policy with maxUnavailable: 0%") | ||
| err := setDesiredStateWithPolicyMaxUnavailableAndNodeSelector( | ||
| TestPolicy, | ||
| linuxBrUp(bridge1), | ||
| intstr.FromString("0%"), | ||
| map[string]string{"node-role.kubernetes.io/worker": ""}, | ||
| ) | ||
| Expect(err).To(HaveOccurred(), "API should reject policy with maxUnavailable: 0%") | ||
| Expect(err.Error()).To(ContainSubstring("maxUnavailable must be greater than 0")) | ||
| }) |
| Context("when maxUnavailable configuration is invalid", func() { | ||
| It("should be rejected by API validation when maxUnavailable is 0", func() { | ||
| By("Attempt to apply policy with maxUnavailable: 0") | ||
| err := setDesiredStateWithPolicyMaxUnavailableAndNodeSelector( | ||
| TestPolicy, | ||
| linuxBrUp(bridge1), | ||
| intstr.FromInt(0), | ||
| map[string]string{"node-role.kubernetes.io/worker": ""}, | ||
| ) | ||
| Expect(err).To(HaveOccurred(), "API should reject policy with maxUnavailable: 0") | ||
| Expect(err.Error()).To(ContainSubstring("maxUnavailable must be greater than 0")) | ||
| }) | ||
|
|
||
| It("should be rejected by API validation when maxUnavailable is 0%", func() { | ||
| By("Attempt to apply policy with maxUnavailable: 0%") | ||
| err := setDesiredStateWithPolicyMaxUnavailableAndNodeSelector( | ||
| TestPolicy, | ||
| linuxBrUp(bridge1), | ||
| intstr.FromString("0%"), | ||
| map[string]string{"node-role.kubernetes.io/worker": ""}, | ||
| ) | ||
| Expect(err).To(HaveOccurred(), "API should reject policy with maxUnavailable: 0%") | ||
| Expect(err.Error()).To(ContainSubstring("maxUnavailable must be greater than 0")) | ||
| }) | ||
| }) |
Signed-off-by: Emilia Desch <emy@redhat.com>
… value Signed-off-by: Emilia Desch <emy@redhat.com>
… value Signed-off-by: Emilia Desch <emy@redhat.com>
… value Signed-off-by: Emilia Desch <emy@redhat.com>
|
@emy: The following tests failed, say
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. |
Is this a BUG FIX or a FEATURE?:
/kind bug
What this PR does / why we need it:
Replace maxUnavailable auto-correction with validation that fails NNCP when value is 0
Special notes for your reviewer:
Release note: