Skip to content

Replace maxUnavailable auto-correction with validation that fails NNCP when value is 0 #1550

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
emy wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Replace maxUnavailable auto-correction with validation that fails NNCP when value is 0 #1550

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9257efd
Replace maxUnavailable auto-correction with validation that fails NNC…
emy Jul 9, 2026
2e947fe
Fix infinite requeue and redundant retries with custom InvalidMaxUnav…
emy Jul 9, 2026
6c65e00
Update bundle manifests with new maxUnavailable documentation
emy Jul 9, 2026
480e072
Replace controller-based maxUnavailable validation with CEL validation
emy Jul 28, 2026
1438a15
regenerate bundle files
emy Jul 28, 2026
b3e7b40
Added BeforeEach hook
emy Jul 29, 2026
fdf68ca
Fix CEL validation rule to treat maxUnavailable IntOrString as scalar…
emy Jul 29, 2026
26ab708
Fix CEL validation rule to treat maxUnavailable IntOrString as scalar…
emy Jul 29, 2026
f543260
Fix CEL validation rule to treat maxUnavailable IntOrString as scalar…
emy Jul 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions api/shared/nodenetworkconfigurationpolicy_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ type NodeNetworkConfigurationPolicySpec struct {

// MaxUnavailable specifies percentage or number
// of machines that can be updating at a time. Default is "50%".
// 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.

@gemini-code-assist gemini-code-assist Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

// +optional
MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty"`
Comment on lines +49 to 53
}
Expand Down
23 changes: 23 additions & 0 deletions controllers/handler/nodenetworkconfigurationpolicy_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,17 @@ func (r *NodeNetworkConfigurationPolicyReconciler) incrementUnavailableNodeCount
)
}

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)
}

@gemini-code-assist gemini-code-assist Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Issue: Infinite Requeue Loop and Redundant Retries on Validation Failure

Returning a standard error from incrementUnavailableNodeCount when maxUnavailable < 1 causes two major issues:

  1. 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., return ctrl.Result{}, nil).
  2. Redundant Retries inside retry.OnError: The validation check is executed inside a retry.OnError block with a classifier that retries on all errors (func(error) bool { return true }). Consequently, when maxUnavailable < 1 is 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:

  1. Define a custom non-retryable error type (e.g., InvalidMaxUnavailableError).
  2. Update the retry.OnError classifier to not retry when the error is of this type.
  3. Modify Reconcile to catch this error and return ctrl.Result{}, nil to prevent requeuing.


if policy.Status.UnavailableNodeCountMap == nil {
policy.Status.UnavailableNodeCountMap = map[string]int{}
}
Expand Down Expand Up @@ -682,6 +693,18 @@ func (r *NodeNetworkConfigurationPolicyReconciler) shouldAbortReconcile(
logger.Info("Error getting max unavailable count")
return false, err
}

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)
}

@gemini-code-assist gemini-code-assist Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.


filter := enactmentconditions.LogicalConditionCountFilter{
nmstateapi.NodeNetworkConfigurationEnactmentConditionFailing: corev1.ConditionTrue,
nmstateapi.NodeNetworkConfigurationEnactmentConditionProgressing: corev1.ConditionFalse,
Expand Down
3 changes: 3 additions & 0 deletions controllers/handler/nodenetworkconfigurationpolicy_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,9 @@ var _ = Describe("NodeNetworkConfigurationPolicy controller predicates", func()
nnce := nmstatev1beta1.NodeNetworkConfigurationEnactment{
ObjectMeta: metav1.ObjectMeta{
Name: shared.EnactmentKey(nodeName, nncp.Name).Name,
Labels: map[string]string{
shared.EnactmentPolicyLabel: nncp.Name,
},
},
Status: shared.NodeNetworkConfigurationEnactmentStatus{},
}
Expand Down
10 changes: 2 additions & 8 deletions pkg/node/nodes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,6 @@ func MaxUnavailableNodeCount(ctx context.Context, cli client.Reader, policy *nms
}

func ScaledMaxUnavailableNodeCount(matchingNodes int, intOrPercent intstr.IntOrString) (int, error) {
correctMaxUnavailable := func(maxUnavailable int) int {
if maxUnavailable < 1 {
return MinMaxunavailable
}
return maxUnavailable
}
maxUnavailable, err := intstr.GetScaledValueFromIntOrPercent(&intOrPercent, matchingNodes, true)
if err != nil {
defaultMaxUnavailable := intstr.FromString(DefaultMaxunavailable)
Expand All @@ -122,9 +116,9 @@ func ScaledMaxUnavailableNodeCount(matchingNodes int, intOrPercent intstr.IntOrS
matchingNodes,
true,
)
return correctMaxUnavailable(maxUnavailable), err
return maxUnavailable, err
}
return correctMaxUnavailable(maxUnavailable), nil
return maxUnavailable, nil
}

// Return true if the event name is the name of
Expand Down
6 changes: 4 additions & 2 deletions pkg/node/nodes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ var _ = Describe("MaxUnavailable nodes", func() {
expectedScaledMaxUnavailable int
expectedError types.GomegaMatcher
}
// Note: ValidateMaxUnavailable is tested at the controller integration test level
// since it requires a full client context with policies and enactments
DescribeTable("testing ScaledMaxUnavailableNodeCount",
func(c maxUnavailableCase) {
maxUnavailable, err := ScaledMaxUnavailableNodeCount(c.nmstateEnabledNodes, c.maxUnavailable)
Expand Down Expand Up @@ -84,14 +86,14 @@ var _ = Describe("MaxUnavailable nodes", func() {
maxUnavailableCase{
nmstateEnabledNodes: 5,
maxUnavailable: intstr.FromString("0%"),
expectedScaledMaxUnavailable: 1,
expectedScaledMaxUnavailable: 0,
expectedError: Not(HaveOccurred()),
}),
Entry("Zero value",
maxUnavailableCase{
nmstateEnabledNodes: 5,
maxUnavailable: intstr.FromInt(0),
expectedScaledMaxUnavailable: 1,
expectedScaledMaxUnavailable: 0,
expectedError: Not(HaveOccurred()),
}))
})