OCPBUGS-16118: fix vsphere machine stuck in infinite requeue when PatchMachine fails after clone

[jcpowermac]

…fails after clone

When a VM clone is initiated, the task ID is saved to an in-memory TaskIDCache. If the subsequent PatchMachine() call fails (e.g. due to a transient ValidatingAdmissionPolicy rejection), the task ID is never persisted to the machine object. On every subsequent reconcile, the cache guard sees a mismatch between the cached task ID and the empty TaskRef in the machine's providerStatus, causing an infinite RequeueAfterError loop. The recovery code in reconciler.create() that handles "cloned but lost taskRef" is never reached.

Fix by clearing the TaskIDCache entry when PatchMachine() fails, so the next Create() call can proceed past the cache guard and reach the existing recovery path.

Summary by CodeRabbit

• Bug Fixes
  • Improved machine creation handling by clearing stale task information before updates.
  • Ensured failed creation attempts do not retain outdated task data.
  • Added reliable requeue behavior when task status does not match the machine’s provider status.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[coderabbitai]

Walkthrough

Actuator.Create now clears the machine’s TaskIDCache entry before patching. Tests cover cache population after success, cleanup after patch failure, and requeue behavior when cached and provider task IDs differ.

Changes

vSphere TaskIDCache behavior

Layer / File(s)	Summary
Create cache cleanup and validation pkg/controller/vsphere/actuator.go, pkg/controller/vsphere/actuator_test.go	Actuator.Create clears the machine’s cached task ID before patching, with tests covering successful cache population, cleanup after patch failure, and RequeueAfterError handling for mismatched task IDs.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: vr4manta

🚥 Pre-merge checks | ✅ 15

✅ Passed checks (15 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	The new test titles are static t.Run strings and contain no dynamic runtime data or generated identifiers.
Test Structure And Quality	✅ Passed	The added subtests are single-purpose, use explicit 10s Eventually timeouts, and clean up created resources consistently with the package’s existing test style.
Microshift Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the new test is a plain Go envtest/unit test, so MicroShift-specific API checks are not applicable.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the new coverage is a Go unit test with envtest/simulator, and it makes no SNO-relevant multi-node assumptions.
Topology-Aware Scheduling Compatibility	✅ Passed	Only vSphere actuator cache cleanup and tests changed; no manifests or scheduling/topology constraints were added.
Ote Binary Stdout Contract	✅ Passed	No process-level stdout writes were added; the only init() in actuator_test.go just registers schemes, and the new test code isn’t suite/bootstrap code.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	PASS: the added coverage is plain Go unit tests using local envtest/simulator; it adds no new Ginkgo e2e tests or public-network dependencies.
No-Weak-Crypto	✅ Passed	Touched files only add TaskIDCache logic/tests; no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB or secret/token comparisons found.
Container-Privileges	✅ Passed	PASS: The PR only changes actuator logic and tests; no container/K8s manifests or privilege flags (privileged, hostPID/Network/IPC, SYS_ADMIN, allowPrivilegeEscalation) are added.
No-Sensitive-Data-In-Logs	✅ Passed	No new logging was added in the touched code; existing logs only mention machine names and generic errors, not secrets or PII.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main fix: clearing the vSphere task cache to prevent infinite requeue when PatchMachine fails.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[jcpowermac]

/test unit

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign racheljpg for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci-robot]

@jcpowermac: This pull request references Jira Issue OCPBUGS-16118, which is invalid:

• expected the bug to be open, but it isn't
• expected the bug to target the "5.0.0" version, but no target version was set
• expected the bug to be in one of the following states: NEW, ASSIGNED, POST, but it is Closed (Duplicate) instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

…fails after clone

When a VM clone is initiated, the task ID is saved to an in-memory TaskIDCache. If the subsequent PatchMachine() call fails (e.g. due to a transient ValidatingAdmissionPolicy rejection), the task ID is never persisted to the machine object. On every subsequent reconcile, the cache guard sees a mismatch between the cached task ID and the empty TaskRef in the machine's providerStatus, causing an infinite RequeueAfterError loop. The recovery code in reconciler.create() that handles "cloned but lost taskRef" is never reached.

Fix by clearing the TaskIDCache entry when PatchMachine() fails, so the next Create() call can proceed past the cache guard and reach the existing recovery path.

Summary by CodeRabbit

• Bug Fixes
• Improved machine creation handling by clearing stale task information before updates.
• Ensured failed creation attempts do not retain outdated task data.
• Added reliable requeue behavior when task status does not match the machine’s provider status.

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.

[coderabbitai]

🧹 Nitpick comments (1)

pkg/controller/vsphere/actuator.go (1)

100-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Fix looks correct; update the now-stale comment.

Clearing the cache entry on PatchMachine() failure correctly breaks the infinite-requeue loop described in the PR objective. However, the comment at Line 100 ("save the taskRef in our cache in case of any error with patch") no longer fully applies — the entry is now immediately deleted again if PatchMachine() fails in the same call, so the cache no longer survives a patch failure. Consider updating the comment to mention that the entry is purged on patch failure and only persists across failures... intentionally left for cases where saving succeeds but a subsequent operation still needs it.

🤖 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 `@pkg/controller/vsphere/actuator.go` around lines 100 - 114, Update the
comment above the TaskIDCache assignment in the reconciliation flow to state
that the task reference is cached for later errors only when patching succeeds,
and that it is removed if PatchMachine() fails. Keep the existing cache
assignment and deletion behavior unchanged.

🤖 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.

Nitpick comments:
In `@pkg/controller/vsphere/actuator.go`:
- Around line 100-114: Update the comment above the TaskIDCache assignment in
the reconciliation flow to state that the task reference is cached for later
errors only when patching succeeds, and that it is removed if PatchMachine()
fails. Keep the existing cache assignment and deletion behavior unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5bc34bbe-b89a-4aa8-b752-8dff17cbb153

📥 Commits

Reviewing files that changed from the base of the PR and between c0b2d2c and edf4f67.

📒 Files selected for processing (2)

• pkg/controller/vsphere/actuator.go
• pkg/controller/vsphere/actuator_test.go

[jcpowermac]

/jira refresh

[openshift-ci-robot]

@jcpowermac: This pull request references Jira Issue OCPBUGS-16118, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (5.0.0) matches configured target version for branch (5.0.0)
• bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @huali9

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

/jira refresh

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]

@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: huali9.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

@jcpowermac: This pull request references Jira Issue OCPBUGS-16118, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (5.0.0) matches configured target version for branch (5.0.0)
• bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @huali9

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

/jira refresh

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.

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.

[jcpowermac]

/test ?

[jcpowermac]

/test e2e-vsphere-ovn-multi-vcenter

[jcpowermac]

/assign @racheljpg

/cc @vr4manta

[openshift-ci]

@jcpowermac: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

[mdbooth]

@jcpowermac Do you have logs (preferrably a must-gather) from a cluster where you saw:

(e.g. due to a transient ValidatingAdmissionPolicy rejection)

Independent of this fix, I'm very interested in that.