[controllers] Fix maxUnhealthyCount bypass - #4420
Draft
jrvaldes wants to merge 1 commit into
Draft
Conversation
The authentication-failure remediation path in the Machine controller deleted Machines unconditionally, bypassing the maxUnhealthyCount safety check already enforced on the private-key-rotation path. This commit lets a MachineSet exceed its disruption budget when both remediation paths triggered deletion around the same time. Route both paths through a single deleteMachineIfAllowed helper so the gate applies consistently, and back off with RequeueAfter instead of an immediate requeue while a restricted deletion waits on a sibling's replacement to become healthy. Also fixes a nil pointer panic in isWindowsMachineHealthy for a Running Machine with no NodeRef, adds unit coverage for the gate, and widens e2e deletion/reconfiguration timeouts to account for the gate delaying deletion until a sibling's replacement finishes configuring.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughWindows Machine remediation now routes stale-key and authentication-failure deletions through a shared safety gate. Restricted deletions emit events and requeue after 30 seconds. Health checks reject invalid status or missing Node references. Tests cover deletion limits, events, backoff, and nil Node references. End-to-end tests update secret replacement, timeout, logging, and remediation handling. Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (17 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
/test ? |
Contributor
Author
|
/test unit |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jrvaldes 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
added
the
approved
Contributor
Author
|
/test lint |
Contributor
Author
|
/test images |
Contributor
Author
|
/test vsphere-e2e-operator |
This was referenced Aug 4, 2026
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Contributor
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
Contributor
Author
|
@coderabbitai rate limit |
|
Your plan includes PR reviews subject to rate limits. Reviews are available now. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/windowsmachine_controller_deletion_test.go`:
- Line 1: Add the //go:build !windows constraint to the top of the
WindowsMachine deletion test file, before the package declaration, matching the
constraint used by WindowsMachineReconciler in windowsmachine_controller.go.
In `@controllers/windowsmachine_controller.go`:
- Line 339: Update deleteMachineIfAllowed and its authentication-failure caller
to evaluate the unhealthy budget after removing the target: exclude the target
from the sibling health count and permit deletion when the resulting count is <=
maxUnhealthyCount, including Provisioned targets. Add a regression case to
TestDeleteMachineIfAllowed covering a Provisioned target with reason
"authentication failure".
- Around line 494-498: Update the condition in the loop using ma so it checks
len(ma.OwnerReferences) before accessing ma.OwnerReferences[0], replacing the
redundant machine.OwnerReferences guard while preserving the existing
owner-name, health, and deletion checks. Add a regression fixture covering an
ownerless sibling Windows-labeled Machine during deletion evaluation.
- Line 371: Update the reconciliation return around deleteMachine to wrap its
error with fmt.Errorf using %w, including context that identifies the
safety-gated machine deletion operation. Preserve the existing ctrl.Result{}
return and deleteMachine(ctx, machine) behavior.
- Around line 355-371: Update deleteMachineIfAllowed to return successfully
before calling isAllowedDeletion when machine.DeletionTimestamp is already set,
preserving deleteMachine’s no-op behavior for in-progress deletion. Add a
wrapper-level test covering an already-deleting Machine and verifying it neither
checks the deletion budget nor emits a restriction event or requeues.
In `@test/e2e/secrets_test.go`:
- Around line 253-275: Update the helper containing the Secret Get/Create/Update
calls to create one context.WithTimeout context with an appropriate deadline,
ensure it is canceled, and pass that context to all three Kubernetes API
operations instead of context.TODO().
🪄 Autofix
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: 91512654-ad60-4a11-aec8-2623a620bc14
⛔ Files ignored due to path filters (36)
vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1/awsfailuredomain.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1/awsfailuredomainplacement.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1/awsresourcefilter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1/awsresourcereference.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1/azurefailuredomain.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1/controlplanemachineset.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1/controlplanemachinesetspec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1/controlplanemachinesetstatus.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1/controlplanemachinesetstrategy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1/controlplanemachinesettemplate.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1/controlplanemachinesettemplateobjectmeta.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1/failuredomains.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1/gcpfailuredomain.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1/nutanixfailuredomainreference.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1/openshiftmachinev1beta1machinetemplate.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1/openstackfailuredomain.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1/rootvolume.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1/vspherefailuredomain.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/utils.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/clientset/versioned/clientset.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/clientset/versioned/fake/clientset_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/clientset/versioned/fake/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/clientset/versioned/fake/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/clientset/versioned/typed/machine/v1/controlplanemachineset.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/clientset/versioned/typed/machine/v1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/clientset/versioned/typed/machine/v1/fake/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/clientset/versioned/typed/machine/v1/fake/fake_controlplanemachineset.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/clientset/versioned/typed/machine/v1/fake/fake_machine_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/clientset/versioned/typed/machine/v1/generated_expansion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/clientset/versioned/typed/machine/v1/machine_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/clientset/versioned/typed/machine/v1beta1/fake/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/clientset/versioned/typed/machine/v1beta1/fake/fake_machine.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/clientset/versioned/typed/machine/v1beta1/fake/fake_machine_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/clientset/versioned/typed/machine/v1beta1/fake/fake_machinehealthcheck.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/machine/clientset/versioned/typed/machine/v1beta1/fake/fake_machineset.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (4)
controllers/windowsmachine_controller.gocontrollers/windowsmachine_controller_deletion_test.gotest/e2e/create_test.gotest/e2e/secrets_test.go
| @@ -0,0 +1,317 @@ | |||
| package controllers | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add the Linux build constraint to this controller test.
This file has no //go:build !windows constraint. A Windows-targeted test build can select this test while excluding Linux-only controller code that defines WindowsMachineReconciler.
Proposed fix
+//go:build !windows
+
package controllersAs per path instructions, “Controllers must have //go:build !windows tag.”
#!/bin/bash
set -euo pipefail
for file in \
controllers/windowsmachine_controller.go \
controllers/windowsmachine_controller_deletion_test.go; do
echo "== $file =="
sed -n '1,8p' "$file"
done🤖 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/windowsmachine_controller_deletion_test.go` at line 1, Add the
//go:build !windows constraint to the top of the WindowsMachine deletion test
file, before the package declaration, matching the constraint used by
WindowsMachineReconciler in windowsmachine_controller.go.
| r.recorder.Eventf(machine, core.EventTypeWarning, "MachineSetupFailure", | ||
| "Machine %s authentication failure", machine.Name) | ||
| return ctrl.Result{}, r.deleteMachine(ctx, machine) | ||
| return r.deleteMachineIfAllowed(ctx, machine, "authentication failure") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Evaluate the deletion budget after deletion.
The authentication-failure path can run for a Provisioned Machine. That target is already unhealthy. With two replicas, one healthy sibling, and maxUnhealthyCount == 1, the gate returns RequeueAfter instead of deleting the failed target. The target then cannot be replaced.
Calculate the post-deletion unhealthy count. Exclude the target from the sibling health count, then allow deletion when the post-deletion count is <= maxUnhealthyCount. Add a regression case in TestDeleteMachineIfAllowed for a Provisioned target and reason "authentication failure".
Also applies to: 355-371
🤖 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/windowsmachine_controller.go` at line 339, Update
deleteMachineIfAllowed and its authentication-failure caller to evaluate the
unhealthy budget after removing the target: exclude the target from the sibling
health count and permit deletion when the resulting count is <=
maxUnhealthyCount, including Provisioned targets. Add a regression case to
TestDeleteMachineIfAllowed covering a Provisioned target with reason
"authentication failure".
Contributor
Author
There was a problem hiding this comment.
@coderabbitai create a commit; 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/windowsmachine_controller.go at line 339, Update
deleteMachineIfAllowed and its authentication-failure caller to evaluate the
unhealthy budget after removing the target: exclude the target from the sibling
health count and permit deletion when the resulting count is <=
maxUnhealthyCount, including Provisioned targets. Add a regression case to
TestDeleteMachineIfAllowed covering a Provisioned target with reason
"authentication failure".
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline controllers/windowsmachine_controller.go --items all --type function --match 'deleteMachineIfAllowed|isAllowedDeletion|isWindowsMachineHealthy|Reconcile'
ast-grep outline controllers/windowsmachine_controller_deletion_test.go --items all --type function --match 'TestDeleteMachineIfAllowed|TestIsAllowedDeletion|new|fixture|event'
rg -n -C 8 'func \(r \*WindowsMachineReconciler\) (deleteMachineIfAllowed|isAllowedDeletion)|authentication failure' \
controllers/windowsmachine_controller.go
rg -n -C 12 'func TestDeleteMachineIfAllowed|authentication failure|Provisioned|MachineDeletionRestricted' \
controllers/windowsmachine_controller_deletion_test.goLength of output: 9277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '36,138p' controllers/windowsmachine_controller_deletion_test.go
sed -n '139,282p' controllers/windowsmachine_controller_deletion_test.go
sed -n '463,525p' controllers/windowsmachine_controller.go
rg -n -C 4 'OwnerReferences\[0\]|UID|ObjectKey|newTestMachine\(' \
controllers/windowsmachine_controller_deletion_test.go controllers/windowsmachine_controller.goLength of output: 31207
@jrvaldes, I verified the finding. It remains valid.
The authentication-failure caller at Line 339 already calls deleteMachineIfAllowed. Do not change that caller.
The minimal fix is in isAllowedDeletion:
- Exclude the target from the healthy sibling count.
- Check
ma.OwnerReferencesbefore readingma.OwnerReferences[0]. - Calculate the post-deletion unhealthy count as
MachineSet replicas - healthy siblings. - Allow deletion when that count is
<= maxUnhealthyCount.
This preserves the existing restricted case where a healthy target has an unhealthy sibling. Deleting that target produces two unhealthy Machines. It allows the requested Provisioned target case because the healthy sibling remains and the post-deletion unhealthy count is one.
I cannot create a commit directly. The verified patch should also add one focused TestDeleteMachineIfAllowed case:
reason:"authentication failure"- target phase:
Provisioned - sibling: healthy and
Running - MachineSet replicas:
2 - expected result: no requeue and target deletion
- expected events: no
MachineDeletionRestrictedevent
You are interacting with an AI system.
Comment on lines
+355
to
+371
| func (r *WindowsMachineReconciler) deleteMachineIfAllowed(ctx context.Context, machine *mapi.Machine, | ||
| reason string) (ctrl.Result, error) { | ||
| deletionAllowed, err := r.isAllowedDeletion(ctx, machine) | ||
| if err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("unable to determine if Machine can be deleted: %w", err) | ||
| } | ||
| if !deletionAllowed { | ||
| r.log.Info("machine deletion restricted", "name", machine.GetName(), "reason", reason, | ||
| "maxUnhealthyCount", maxUnhealthyCount) | ||
| r.recorder.Eventf(machine, core.EventTypeWarning, "MachineDeletionRestricted", | ||
| "Machine %v deletion restricted (%s) as the maximum unhealthy machines can`t exceed %v count", | ||
| machine.Name, reason, maxUnhealthyCount) | ||
| // Requeue with a backoff rather than immediately, to avoid a tight reconcile loop and event spam while | ||
| // waiting for a sibling Machine's remediation to complete. | ||
| return ctrl.Result{RequeueAfter: machineDeletionRestrictedRequeueInterval}, nil | ||
| } | ||
| return ctrl.Result{}, r.deleteMachine(ctx, machine) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Preserve the no-op for a Machine already deleting.
deleteMachine returns successfully when DeletionTimestamp is set. This wrapper checks the budget first. If the budget is full, an already-deleting Machine emits MachineDeletionRestricted and requeues instead of returning success.
Return before isAllowedDeletion when deletion has already started. Add a wrapper-level test for this case.
Proposed fix
func (r *WindowsMachineReconciler) deleteMachineIfAllowed(ctx context.Context, machine *mapi.Machine,
reason string) (ctrl.Result, error) {
+ if !machine.GetDeletionTimestamp().IsZero() {
+ return ctrl.Result{}, nil
+ }
deletionAllowed, err := r.isAllowedDeletion(ctx, machine)As per path instructions, “Check reconciliation loop logic and idempotency.”
📝 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.
Suggested change
| func (r *WindowsMachineReconciler) deleteMachineIfAllowed(ctx context.Context, machine *mapi.Machine, | |
| reason string) (ctrl.Result, error) { | |
| deletionAllowed, err := r.isAllowedDeletion(ctx, machine) | |
| if err != nil { | |
| return ctrl.Result{}, fmt.Errorf("unable to determine if Machine can be deleted: %w", err) | |
| } | |
| if !deletionAllowed { | |
| r.log.Info("machine deletion restricted", "name", machine.GetName(), "reason", reason, | |
| "maxUnhealthyCount", maxUnhealthyCount) | |
| r.recorder.Eventf(machine, core.EventTypeWarning, "MachineDeletionRestricted", | |
| "Machine %v deletion restricted (%s) as the maximum unhealthy machines can`t exceed %v count", | |
| machine.Name, reason, maxUnhealthyCount) | |
| // Requeue with a backoff rather than immediately, to avoid a tight reconcile loop and event spam while | |
| // waiting for a sibling Machine's remediation to complete. | |
| return ctrl.Result{RequeueAfter: machineDeletionRestrictedRequeueInterval}, nil | |
| } | |
| return ctrl.Result{}, r.deleteMachine(ctx, machine) | |
| func (r *WindowsMachineReconciler) deleteMachineIfAllowed(ctx context.Context, machine *mapi.Machine, | |
| reason string) (ctrl.Result, error) { | |
| if !machine.GetDeletionTimestamp().IsZero() { | |
| return ctrl.Result{}, nil | |
| } | |
| deletionAllowed, err := r.isAllowedDeletion(ctx, machine) | |
| if err != nil { | |
| return ctrl.Result{}, fmt.Errorf("unable to determine if Machine can be deleted: %w", err) | |
| } | |
| if !deletionAllowed { | |
| r.log.Info("machine deletion restricted", "name", machine.GetName(), "reason", reason, | |
| "maxUnhealthyCount", maxUnhealthyCount) | |
| r.recorder.Eventf(machine, core.EventTypeWarning, "MachineDeletionRestricted", | |
| "Machine %v deletion restricted (%s) as the maximum unhealthy machines can`t exceed %v count", | |
| machine.Name, reason, maxUnhealthyCount) | |
| // Requeue with a backoff rather than immediately, to avoid a tight reconcile loop and event spam while | |
| // waiting for a sibling Machine's remediation to complete. | |
| return ctrl.Result{RequeueAfter: machineDeletionRestrictedRequeueInterval}, nil | |
| } | |
| return ctrl.Result{}, r.deleteMachine(ctx, machine) | |
| } |
🤖 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/windowsmachine_controller.go` around lines 355 - 371, Update
deleteMachineIfAllowed to return successfully before calling isAllowedDeletion
when machine.DeletionTimestamp is already set, preserving deleteMachine’s no-op
behavior for in-progress deletion. Add a wrapper-level test covering an
already-deleting Machine and verifying it neither checks the deletion budget nor
emits a restriction event or requeues.
Source: Path instructions
| // waiting for a sibling Machine's remediation to complete. | ||
| return ctrl.Result{RequeueAfter: machineDeletionRestrictedRequeueInterval}, nil | ||
| } | ||
| return ctrl.Result{}, r.deleteMachine(ctx, machine) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrap the delete error with operation context.
Line 371 returns the raw deleteMachine error. Wrap it so the reconciliation error identifies the safety-gated delete operation.
Proposed fix
- return ctrl.Result{}, r.deleteMachine(ctx, machine)
+ if err := r.deleteMachine(ctx, machine); err != nil {
+ return ctrl.Result{}, fmt.Errorf("delete machine %q: %w", machine.Name, err)
+ }
+ return ctrl.Result{}, nilAs per coding guidelines, “In Go code, wrap errors with context using fmt.Errorf with %w.”
📝 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.
Suggested change
| return ctrl.Result{}, r.deleteMachine(ctx, machine) | |
| if err := r.deleteMachine(ctx, machine); err != nil { | |
| return ctrl.Result{}, fmt.Errorf("delete machine %q: %w", machine.Name, err) | |
| } | |
| return ctrl.Result{}, nil |
🤖 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/windowsmachine_controller.go` at line 371, Update the
reconciliation return around deleteMachine to wrap its error with fmt.Errorf
using %w, including context that identifies the safety-gated machine deletion
operation. Preserve the existing ctrl.Result{} return and deleteMachine(ctx,
machine) behavior.
Source: Coding guidelines
Comment on lines
+494
to
498
| // Note: `len(machine.OwnerReferences) != 0` here refers to the outer `machine` parameter (already validated | ||
| // non-empty at the top of this function), not the loop variable `ma`. It is redundant but kept for | ||
| // clarity/safety in case this function is refactored again in the future. | ||
| if len(machine.OwnerReferences) != 0 && ma.OwnerReferences[0].Name == machinesetName && | ||
| r.isWindowsMachineHealthy(ctx, &ma) && ma.DeletionTimestamp.IsZero() { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the loop item's owner references.
The condition validates machine.OwnerReferences, but it indexes ma.OwnerReferences[0]. A Windows-labeled Machine without owner references panics the controller during deletion evaluation.
Check len(ma.OwnerReferences) before indexing. Add a regression fixture for an ownerless sibling Machine.
Proposed fix
- if len(machine.OwnerReferences) != 0 && ma.OwnerReferences[0].Name == machinesetName &&
+ if len(ma.OwnerReferences) != 0 && ma.OwnerReferences[0].Name == machinesetName &&As per path instructions, “Verify ... owner references.”
📝 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.
Suggested change
| // Note: `len(machine.OwnerReferences) != 0` here refers to the outer `machine` parameter (already validated | |
| // non-empty at the top of this function), not the loop variable `ma`. It is redundant but kept for | |
| // clarity/safety in case this function is refactored again in the future. | |
| if len(machine.OwnerReferences) != 0 && ma.OwnerReferences[0].Name == machinesetName && | |
| r.isWindowsMachineHealthy(ctx, &ma) && ma.DeletionTimestamp.IsZero() { | |
| // Note: `len(machine.OwnerReferences) != 0` here refers to the outer `machine` parameter (already validated | |
| // non-empty at the top of this function), not the loop variable `ma`. It is redundant but kept for | |
| // clarity/safety in case this function is refactored again in the future. | |
| if len(ma.OwnerReferences) != 0 && ma.OwnerReferences[0].Name == machinesetName && | |
| r.isWindowsMachineHealthy(ctx, &ma) && ma.DeletionTimestamp.IsZero() { |
🤖 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/windowsmachine_controller.go` around lines 494 - 498, Update the
condition in the loop using ma so it checks len(ma.OwnerReferences) before
accessing ma.OwnerReferences[0], replacing the redundant machine.OwnerReferences
guard while preserving the existing owner-name, health, and deletion checks. Add
a regression fixture covering an ownerless sibling Windows-labeled Machine
during deletion evaluation.
Source: Path instructions
Comment on lines
253
to
+275
| for _, ns := range []string{wmcoNamespace, tc.workloadNamespace} { | ||
| _, err := tc.client.K8s.CoreV1().Secrets(ns).Create(context.TODO(), &privateKeySecret, meta.CreateOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("could not create private key secret in namespace %s: %w", ns, err) | ||
| existing, getErr := tc.client.K8s.CoreV1().Secrets(ns).Get(context.TODO(), secrets.PrivateKeySecret, | ||
| meta.GetOptions{}) | ||
| if getErr != nil { | ||
| if !apierrors.IsNotFound(getErr) { | ||
| return fmt.Errorf("error getting private key secret in namespace %s: %w", ns, getErr) | ||
| } | ||
| newSecret := &core.Secret{ | ||
| Data: map[string][]byte{secrets.PrivateKeySecretKey: keyData}, | ||
| ObjectMeta: meta.ObjectMeta{ | ||
| Name: secrets.PrivateKeySecret, | ||
| }, | ||
| } | ||
| if _, err := tc.client.K8s.CoreV1().Secrets(ns).Create(context.TODO(), newSecret, | ||
| meta.CreateOptions{}); err != nil { | ||
| return fmt.Errorf("could not create private key secret in namespace %s: %w", ns, err) | ||
| } | ||
| continue | ||
| } | ||
| existing.Data = map[string][]byte{secrets.PrivateKeySecretKey: keyData} | ||
| if _, err := tc.client.K8s.CoreV1().Secrets(ns).Update(context.TODO(), existing, | ||
| meta.UpdateOptions{}); err != nil { | ||
| return fmt.Errorf("could not update private key secret in namespace %s: %w", ns, err) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use a deadline-bound context for Secret API calls.
context.TODO() does not cancel or time-limit the new Get, Create, and Update calls. If the API server stops responding, createPrivateKeySecret can block without a request deadline.
Create a context.WithTimeout context for this helper and pass it to all three calls.
🤖 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 `@test/e2e/secrets_test.go` around lines 253 - 275, Update the helper
containing the Secret Get/Create/Update calls to create one context.WithTimeout
context with an appropriate deadline, ensure it is canceled, and pass that
context to all three Kubernetes API operations instead of context.TODO().
Source: Coding guidelines
Contributor
|
@jrvaldes: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The authentication-failure remediation path in the Machine controller deleted Machines unconditionally, bypassing the maxUnhealthyCount safety check already enforced on the private-key-rotation path. This commit lets a MachineSet exceed its disruption budget when both remediation paths triggered deletion around the same time.
Route both paths through a single deleteMachineIfAllowed helper so the gate applies consistently, and back off with RequeueAfter instead of an immediate requeue while a restricted deletion waits on a sibling's replacement to become healthy. Also fixes a nil pointer panic in isWindowsMachineHealthy for a Running Machine with no NodeRef, adds unit coverage for the gate, and widens e2e deletion/reconfiguration timeouts to account for the gate delaying deletion until a sibling's replacement finishes configuring.
Summary by CodeRabbit