OCPBUGS-90077: Fix infinite reboot on SSH exit command status#4284
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughThis change adds reboot-attempt tracking for nodes. New constants and helper functions in the metadata package read, increment, and clear a reboot-attempts annotation on nodes, alongside the existing reboot-required annotation. The node reconciler now checks the attempt count during reconciliation: reaching the configured maximum stops further reboot processing, clears annotations, and emits a warning event, while attempts below the maximum are incremented before proceeding. Separately, the Windows reboot routine now tolerates a specific SSH command-exit condition during restart, treating it as expected rather than a fatal error, with the remaining reboot/reinitialization sequence unchanged. 🚥 Pre-merge checks | ✅ 5 | ❌ 15❌ Failed checks (15 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
openshift-ci
Bot
added
the
approved
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@controllers/node_controller.go`:
- Around line 97-111: The reboot flow in node reconciliation counts failures too
early and logs a non-error with Error, so update the logic around
`IncrementRebootAttempts`, `SafeReboot`, and the `r.log` call in the node
controller. Move the reboot-attempt increment so it happens only after an actual
reboot attempt fails (after `SafeReboot` returns an error), and leave setup
failures from `signer.Create`, `instanceFromNode`, or `NewNodeConfig` uncounted.
Also replace the `r.log.Error(nil, ...)` call for the max-attempts condition
with an appropriate `log.Info(...)` message since the warning event already
records the failure condition.
🪄 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: 9a67611e-a642-4189-9a4d-0f4db2e69d4b
📒 Files selected for processing (3)
controllers/node_controller.gopkg/metadata/metadata.gopkg/windows/windows.go
| attempts := metadata.GetRebootAttempts(node) | ||
| if attempts >= metadata.MaxRebootAttempts { | ||
| r.log.Error(nil, "max reboot attempts reached", "node", node.Name, "attempts", attempts) | ||
| r.recorder.Eventf(node, core.EventTypeWarning, "RebootFailed", | ||
| "Node %s failed to reboot after %d attempts", node.Name, attempts) | ||
| if err := metadata.RemoveRebootAnnotation(ctx, r.client, *node); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to remove reboot annotation: %w", err) | ||
| } | ||
| return ctrl.Result{}, nil | ||
| } | ||
|
|
||
| if err := metadata.IncrementRebootAttempts(ctx, r.client, node); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to increment reboot attempts: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reboot attempt counter is incremented before the reboot is attempted.
IncrementRebootAttempts (line 108) runs before signer.Create, instanceFromNode, NewNodeConfig, and SafeReboot. If any of these setup steps fail, the counter is incremented but no reboot was attempted. After 3 such failures (e.g., transient API errors during signer.Create), the node gives up without a single reboot attempt.
Consider moving the increment to after SafeReboot fails so only actual reboot failures are counted:
🔧 Suggested fix: increment after reboot failure
if _, ok := node.GetAnnotations()[metadata.RebootAnnotation]; ok {
attempts := metadata.GetRebootAttempts(node)
if attempts >= metadata.MaxRebootAttempts {
- r.log.Error(nil, "max reboot attempts reached", "node", node.Name, "attempts", attempts)
+ r.log.Info("max reboot attempts reached, giving up", "node", node.Name, "attempts", attempts)
r.recorder.Eventf(node, core.EventTypeWarning, "RebootFailed",
"Node %s failed to reboot after %d attempts", node.Name, attempts)
if err := metadata.RemoveRebootAnnotation(ctx, r.client, *node); err != nil {
return ctrl.Result{}, fmt.Errorf("failed to remove reboot annotation: %w", err)
}
return ctrl.Result{}, nil
}
-
- if err := metadata.IncrementRebootAttempts(ctx, r.client, node); err != nil {
- return ctrl.Result{}, fmt.Errorf("failed to increment reboot attempts: %w", err)
- }
signer, err := signer.Create(ctx, types.NamespacedName{Namespace: r.watchNamespace,
Name: secrets.PrivateKeySecret}, r.client)
if err != nil {
return ctrl.Result{}, fmt.Errorf("unable to create signer from private key secret: %w", err)
}
instanceInfo, err := r.instanceFromNode(ctx, node)
if err != nil {
return ctrl.Result{}, err
}
nc, err := nodeconfig.NewNodeConfig(ctx, r.client, r.k8sclientset, r.clusterServiceCIDR, r.watchNamespace,
instanceInfo, signer, nil, nil, r.platform)
if err != nil {
return ctrl.Result{}, fmt.Errorf("failed to create new nodeconfig: %w", err)
}
defer func() {
err := nc.Close()
if err != nil {
r.log.Info("WARNING: error closing nodeconfig", "error", err.Error())
}
}()
if err := nc.SafeReboot(ctx); err != nil {
+ if incErr := metadata.IncrementRebootAttempts(ctx, r.client, node); incErr != nil {
+ r.log.Error(incErr, "failed to increment reboot attempts", "node", node.Name)
+ }
return ctrl.Result{}, fmt.Errorf("full instance reboot failed: %w", err)
}
}
return ctrl.Result{}, nil
}Also, r.log.Error(nil, ...) on line 99 is semantically incorrect — log.Error expects an error. Use log.Info for this condition since the RebootFailed warning event already provides error-level visibility. As per coding guidelines, use logr.Logger appropriately: log.Error(err, ...) is for errors, log.Info(...) for conditions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| attempts := metadata.GetRebootAttempts(node) | |
| if attempts >= metadata.MaxRebootAttempts { | |
| r.log.Error(nil, "max reboot attempts reached", "node", node.Name, "attempts", attempts) | |
| r.recorder.Eventf(node, core.EventTypeWarning, "RebootFailed", | |
| "Node %s failed to reboot after %d attempts", node.Name, attempts) | |
| if err := metadata.RemoveRebootAnnotation(ctx, r.client, *node); err != nil { | |
| return ctrl.Result{}, fmt.Errorf("failed to remove reboot annotation: %w", err) | |
| } | |
| return ctrl.Result{}, nil | |
| } | |
| if err := metadata.IncrementRebootAttempts(ctx, r.client, node); err != nil { | |
| return ctrl.Result{}, fmt.Errorf("failed to increment reboot attempts: %w", err) | |
| } | |
| attempts := metadata.GetRebootAttempts(node) | |
| if attempts >= metadata.MaxRebootAttempts { | |
| r.log.Info("max reboot attempts reached, giving up", "node", node.Name, "attempts", attempts) | |
| r.recorder.Eventf(node, core.EventTypeWarning, "RebootFailed", | |
| "Node %s failed to reboot after %d attempts", node.Name, attempts) | |
| if err := metadata.RemoveRebootAnnotation(ctx, r.client, *node); err != nil { | |
| return ctrl.Result{}, fmt.Errorf("failed to remove reboot annotation: %w", err) | |
| } | |
| return ctrl.Result{}, nil | |
| } | |
| signer, err := signer.Create(ctx, types.NamespacedName{Namespace: r.watchNamespace, | |
| Name: secrets.PrivateKeySecret}, r.client) | |
| if err != nil { | |
| return ctrl.Result{}, fmt.Errorf("unable to create signer from private key secret: %w", err) | |
| } | |
| instanceInfo, err := r.instanceFromNode(ctx, node) | |
| if err != nil { | |
| return ctrl.Result{}, err | |
| } | |
| nc, err := nodeconfig.NewNodeConfig(ctx, r.client, r.k8sclientset, r.clusterServiceCIDR, r.watchNamespace, | |
| instanceInfo, signer, nil, nil, r.platform) | |
| if err != nil { | |
| return ctrl.Result{}, fmt.Errorf("failed to create new nodeconfig: %w", err) | |
| } | |
| defer func() { | |
| err := nc.Close() | |
| if err != nil { | |
| r.log.Info("WARNING: error closing nodeconfig", "error", err.Error()) | |
| } | |
| }() | |
| if err := nc.SafeReboot(ctx); err != nil { | |
| if incErr := metadata.IncrementRebootAttempts(ctx, r.client, node); incErr != nil { | |
| r.log.Error(incErr, "failed to increment reboot attempts", "node", node.Name) | |
| } | |
| return ctrl.Result{}, fmt.Errorf("full instance reboot failed: %w", err) | |
| } |
🤖 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 `@controllers/node_controller.go` around lines 97 - 111, The reboot flow in
node reconciliation counts failures too early and logs a non-error with Error,
so update the logic around `IncrementRebootAttempts`, `SafeReboot`, and the
`r.log` call in the node controller. Move the reboot-attempt increment so it
happens only after an actual reboot attempt fails (after `SafeReboot` returns an
error), and leave setup failures from `signer.Create`, `instanceFromNode`, or
`NewNodeConfig` uncounted. Also replace the `r.log.Error(nil, ...)` call for the
max-attempts condition with an appropriate `log.Info(...)` message since the
warning event already records the failure condition.
Source: Coding guidelines
| // DesiredVersionAnnotation is a Node annotation, indicating the Service ConfigMap that should be used to configure it | ||
| DesiredVersionAnnotation = "windowsmachineconfig.openshift.io/desired-version" | ||
| // RebootAnnotation indicates the node's underlying instance needs to be restarted | ||
| RebootAnnotation = "windowsmachineconfig.openshift.io/reboot-required" |
There was a problem hiding this comment.
Kubernetes annotations are just map[string]string, so a value like a reboot-required having the counter is perfectly valid. for example reboot-required: "2"
Nothing anywhere inspects the value, so overloading it with the attempt count is a non-breaking change.
I'd lean toward consolidating to the single annotation, since it also reduces the total annotation surface area on the Node object
|
/test unit |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jrvaldes, mansikulkarni96 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
removed
the
do-not-merge/work-in-progress
mansikulkarni96
changed the title
openshift-ci-robot
added
jira/severity-important
|
@mansikulkarni96: This pull request references Jira Issue OCPBUGS-90077, which is invalid:
Comment 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. |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@mansikulkarni96: This pull request references Jira Issue OCPBUGS-90077, which is valid. The bug has been moved to the POST state. 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-robot
removed
the
jira/invalid-bug
|
/cherry-pick release-4.22 |
|
@mansikulkarni96: once the present PR merges, I will cherry-pick it on top of 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. |
|
/override ci/prow/vsphere-disconnected-e2e-operator |
|
@jrvaldes: Overrode contexts on behalf of jrvaldes: ci/prow/vsphere-disconnected-e2e-operator 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 vsphere-proxy-e2e-operator |
|
/override ci/prow/vsphere-proxy-e2e-operator |
|
@mansikulkarni96: Overrode contexts on behalf of mansikulkarni96: ci/prow/vsphere-proxy-e2e-operator 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. |
|
/override ci/prow/vsphere-disconnected-e2e-operator |
|
@mansikulkarni96: Overrode contexts on behalf of mansikulkarni96: ci/prow/vsphere-disconnected-e2e-operator 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. |
|
@mansikulkarni96: 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. |
|
@mansikulkarni96: Jira Issue OCPBUGS-90077: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-90077 has been moved to the MODIFIED state. 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. |
|
@mansikulkarni96: new pull request created: #4296 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. |
|
@mansikulkarni96: new pull request created: #4297 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. |
|
@mansikulkarni96: new pull request created: #4298 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. |
4 participants
Summary by CodeRabbit
New Features
Bug Fixes