OCPBUGS-92182: OCPBUGS-99219: Use providerSpec.Template in vSphere machineset reconciliation - #6234
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-important
|
@djoshy: This pull request references Jira Issue OCPBUGS-92182, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@djoshy: This pull request references Jira Issue OCPBUGS-92182, which is valid. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMachineConfigController now reads vSphere disk configuration from the install-config ConfigMap and gains RBAC for that access. vSphere template reconciliation resolves existing, rollback, or newly created templates and propagates the resolved name through divergence handling. ChangesvSphere template recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReconcileVSphereProviderSpec
participant KubeClient
participant ResolveExistingTemplateVM
participant VSphere
ReconcileVSphereProviderSpec->>KubeClient: Fetch cluster-config-v1 and vSphere credentials
KubeClient-->>ResolveExistingTemplateVM: Provide diskType and client access
ResolveExistingTemplateVM->>VSphere: Look up provider or infrastructure template
VSphere-->>ResolveExistingTemplateVM: Return existing VM or not found
ResolveExistingTemplateVM->>VSphere: Rename rollback VM or create template from OVA
ResolveExistingTemplateVM-->>ReconcileVSphereProviderSpec: Return resolvedName and created
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.github.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 `@pkg/controller/bootimage/vsphere_helpers.go`:
- Around line 262-283: The rollback VM lookup in createTemplateForFailureDomain
treats any oldErr from finder.VirtualMachine as if the VM does not exist, which
can hide real vCenter failures and incorrectly trigger OVA recreation. Update
this branch to only take the “no existing template” path for a
*find.NotFoundError, matching the computed-name lookup behavior, and return
other errors immediately so transient lookup failures are surfaced instead of
falling back to createNewVMTemplateWithNameForFailureDomain.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e445c5ba-c20b-48d5-9b12-d660d0468c8a
📒 Files selected for processing (1)
pkg/controller/bootimage/vsphere_helpers.go
|
/retest-required |
1 similar comment
|
/retest-required |
|
@djoshy surprised that there are no unit tests |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jcpowermac
left a comment
jcpowermac
left a comment
There was a problem hiding this comment.
Review by Claude (AI Reviewer)
This review was performed by Claude Code (claude-opus-4-6) on behalf of @jcpowermac. I am the reviewer.
The refactor into resolveExistingTemplateVM is clean — extracting the resolution logic reduces cyclomatic complexity and the errors.As fix correctly handles govmomi error types. The create-from-OVA path for new failure domains fills the missing gap.
However there is a critical issue with what happens after the providerSpec.Template lookup succeeds — the caller still returns the computed name, which will patch the MachineSet to point at a VM that doesn't exist. See inline comments for details.
Findings summary:
- 🔴 Critical: providerSpec.Template lookup succeeds but caller overwrites with non-existent computed name
- 🟡 Silent error swallowing on providerSpec.Template lookup failure
- 🟡 Hardcoded
"thin"disk type for new templates - ⚪ Typo in doc comment
| // already has a valid template doesn't fail just because the infra computed name isn't there yet. | ||
| if providerSpec.Template != "" && providerSpec.Template != name { | ||
| if tmplVM, tmplErr := finder.VirtualMachine(ctx, providerSpec.Template); tmplErr == nil { | ||
| return tmplVM, false, nil |
There was a problem hiding this comment.
[Claude Review — Critical] When this branch succeeds and returns the VM found via providerSpec.Template, the caller in createNewVMTemplate still has name set to the computed {infraID}-rhcos-{failureDomainName}. The caller proceeds to check the RHCOS version and at line 621-623 does:
if providerSpec.Template != name {
return name, true, nil
}This patches the MachineSet's template to the computed name — but no VM with that name exists in vSphere. The MachineSet gets updated to reference a non-existent template, which will cause the same failure on the next reconciliation.
The fix needs to either:
- Return the resolved name alongside the VM (e.g. add a
resolvedName stringreturn), so the caller uses the name that actually matched, or - Have the caller skip the
providerSpec.Template != namedivergence check when the template was resolved via providerSpec.
| if providerSpec.Template != "" && providerSpec.Template != name { | ||
| if tmplVM, tmplErr := finder.VirtualMachine(ctx, providerSpec.Template); tmplErr == nil { | ||
| return tmplVM, false, nil | ||
| } |
There was a problem hiding this comment.
[Claude Review] If finder.VirtualMachine fails with a non-NotFoundError (e.g., permission denied, network timeout), the error is silently swallowed and we fall through to the computed name lookup. This could mask real infrastructure issues and make debugging harder. Consider at least logging:
klog.Warningf("Failed to look up providerSpec.Template %s, falling back to computed name: %v", providerSpec.Template, tmplErr)There was a problem hiding this comment.
This is valid.
vCenter won't always be available (upgrade - maintenance, etc.)
There was a problem hiding this comment.
Updated with a log line.
| return nil, false, fmt.Errorf("failed to download %s: %w", ova.Location, ovaErr) | ||
| } | ||
| if createErr := createNewVMTemplateWithNameForFailureDomain(ctx, providerSpec, failureDomain, finder, client, tagManager, name, ovaPath, infraID, "thin"); createErr != nil { | ||
| return nil, false, createErr |
There was a problem hiding this comment.
[Claude Review] The existing update path uses getDiskTypeFromExistingVM(vmMo) to preserve the disk type from the current template. Hardcoding "thin" here for new failure domains may not match the cluster's convention (e.g., thick or eagerZeroedThick). Might be fine as a default but worth either a comment explaining why, or reading the disk type from a sibling failure domain's existing template if one was already processed in this loop.
There was a problem hiding this comment.
I suppose in this scenario no template exists anywhere. I am unsure if we should select disk type for a user.
There was a problem hiding this comment.
Yeah, should we just error out instead of creating a new VM?
There was a problem hiding this comment.
[Claude Review] I checked the API types — disk type is not persisted anywhere in the OpenShift API. It is not in VSphereMachineProviderSpec, Infrastructure spec, or the failure domain topology. The install-config diskType is consumed by the installer at template creation time but never written to a cluster object that MCO can read.
So getDiskTypeFromExistingVM() is the only way to determine disk type at runtime — by inspecting an existing VM backing. For a brand new failure domain with no template, the options are:
- Read it from a sibling failure domain template — createNewVMTemplate loops over all failure domains, so if one was already resolved earlier in the loop with an existing VM, its disk type could be captured and reused here. This preserves whatever the installer originally set.
- Error out rather than guessing — force the user to ensure the template exists before adding the failure domain, since MCO cannot know the intended disk type.
Given that picking the wrong disk type could have real storage/performance implications and MCO should not be making that choice for the user, erroring out (option 2) with a clear message seems safer. Alternatively, option 1 is reasonable if we can assume all failure domains in a cluster use the same disk type.
There was a problem hiding this comment.
@djoshy I am concerned about erroring, I have seen issues in testing when mco goes degraded for the entire cluster, other operators themselves become degraded as a result - I need to investigate this further.
There was a problem hiding this comment.
Hi @jcpowermac no rush - any additional thoughts on how to handle this?
It probably wasn't trivial to unit test govmomi calls without additional vendors when it was originally written. E2Es are required for feature promotion, so we probably decided to prioritize those instead. Although at this point, the amount of vSphere carveouts in the boot image controller warrants it 😅 ....but I'd rather not block this bug fix on that. |
|
/test ? |
|
/test e2e-vsphere e2e-vsphere-ovn-zones e2e-vsphere-ovn-upi |
jcpowermac
left a comment
jcpowermac
left a comment
There was a problem hiding this comment.
Review by @jcallen — two inline comments on error handling and disk type defaults.
b34fcbd to
04eedf6
Compare
|
/lgtm |
|
Scheduling tests matching the |
|
/retest-required |
|
/pipeline required |
|
Scheduling tests matching the |
|
|
||
| return "", false, nil | ||
| return "", false, fmt.Errorf("providerSpec workspace (server: %s, datacenter: %s, datastore: %s, resourcePool: %s, vmGroup: %s) does not match any vCenter/failure domain in the Infrastructure object", | ||
| providerSpec.Workspace.Server, providerSpec.Workspace.Datacenter, providerSpec.Workspace.Datastore, providerSpec.Workspace.ResourcePool, providerSpec.Workspace.VMGroup) |
There was a problem hiding this comment.
Not every failure domain will have a vmgroup, only when https://github.com/openshift/api/blob/72624b98ff3d5cd0c2ca8643544e1f60ee77ded5/config/v1/types_infrastructure.go#L1462 type is HostGroup
There was a problem hiding this comment.
Updated the message to account for this, I believe the underlying check was already in place:
machine-config-operator/pkg/controller/bootimage/vsphere_helpers.go
Lines 522 to 530 in 23687b4
| workspaceFolder, folderErr := finder.Folder(ctx, providerSpec.Workspace.Folder) | ||
| if folderErr != nil { | ||
| klog.Warningf("failed to resolve workspace folder %q; cannot verify template VM locality this reconcile, proceeding with name-based lookup only: %v", providerSpec.Workspace.Folder, folderErr) | ||
| } |
There was a problem hiding this comment.
Dropped this down to a warning
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djoshy, jcpowermac 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 |
|
/retest-required |
|
Verified using IPI on vspehere multi-vcenter cluster. The cluster was created using prow workflow periodic-ci-openshift-openshift-tests-private-release-5.0-amd64-nightly-vsphere-ipi-multi-vcenter-f28 The following verifications were executed using machinesets using both vcenters in the cluster: Verify that custom templates out of the Machineset's folder are not deleted
After update: Verify that non-existent templates can be updated when the default template doesn't existVerification steps:
Result: The default template is uploaded and the machineset is configured to use the default template. Verify that custom existing templates are updated when the default template doesn't exist and there is no machineset using the default template
Result: The default template atomically swapped and updated with the correct version. Verify that and error is raised when a machineset is configured to use a wrong failure domain
/verified by @sergiordlr |
openshift-ci-robot
added
the
verified
|
@sergiordlr: This PR has been marked as verified by 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. |
|
/retest-required |
|
@djoshy: The following test failed, say
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. |
|
/override ci/prow/bootstrap-unit Has passed on this commit before, seems to be flaking now |
|
@djoshy: Overrode contexts on behalf of djoshy: ci/prow/bootstrap-unit 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. |
|
@djoshy: Jira Issue Verification Checks: Jira Issue OCPBUGS-92182 Jira Issue OCPBUGS-92182 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
/cherry-pick release-4.22 release-4.21 release-4.20 |
|
@djoshy: #6234 failed to apply on top of branch "release-4.22": 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. |
…Center/multi-FD Covers the highest-bug-history function in the vSphere boot-image path: matching a providerSpec to the correct vCenter+failure domain while skipping others (including a real multi-vCenter, multi-failure-domain scenario), the version-match no-op path, a full real OVA rebuild via vcsim, providerSpec.Template divergence from the computed name (the PR openshift#6234 bug class - reconciles the name without touching vSphere), mid-swap crash recovery via the mco-old-* rollback VM, the two RHCOS-version-undeterminable error paths, and the 80-character name limit. Also fixes two govmomi v0.45.1 simulator quirks surfaced by running two vcsim instances at once in the same test binary: - vim25 SOAP request handling is routed through the package-level simulator.Map global regardless of which simulated vCenter's HTTP server received the request, so a second live vCenter can silently redirect lookups meant for the first. Added simulatedVCenter.activate() (vsphere_simulator_test.go) to explicitly re-pin the global to the intended vCenter before any vim25 call, and applied it wherever createTestVM or a cross-vCenter assertion runs. - createTestVM previously built its fixture VM via the real OVA import path (createNewVMTemplateWithNameForFailureDomain), which stages new VMs under a name-derived "mco-tmp-<hash>" temp name before renaming; since vcsim's Rename doesn't relocate the underlying datastore folder, a later real import reusing that same deterministic temp name (as the code under test does on a version-mismatch rebuild) would fail with FileAlreadyExists. Switched the fixture to build directly under its final name via folder.CreateVM, sidestepping the temp-name collision entirely (vsphere_object_helpers_test.go). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…te names Closes the vSphere e2e gap around PR openshift#6234's bug class: vSphere is the only platform where providerSpec.Template names a vCenter object directly, so it's the only platform where the boot image controller has to resolve/preserve an existing template referenced by an arbitrary, non-standard name rather than one it computed itself. Uploads the current RHCOS OVA under a custom template name, points a cloned MachineSet's providerSpec.Template directly at it, and asserts the controller recognizes it as already valid (no Degraded/Progressing conditions, CheckCurrentOSImageIsUpdated passes) and leaves the custom name in place rather than renaming it back to the name it would have computed itself. Verified with `go build ./test/extended-priv/...` (this test package builds as part of the main module) - not run against a live cluster. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Fix included in release 5.0.0-0.nightly-2026-08-05-033721 |
7 participants
When a new failure domain is added, the controller always looked up the template by the computed name, ignoring
providerSpec.Template, causing reconciliation to fail. Fix by checking providerSpec.Template first, using errors.As forNotFoundError, and creating the template from OVA when none exists. I broke out the resolution step into a separate function to reduce the cyclomatic complexity as the lint check failed(which is fair 😄).An additional RBAC manifest was added as the MCC needs to lookup the
cluster-config-v1configmap in the kube-system namespaceto recover the original install time configuration for the template.I also rolled in the fix for https://redhat.atlassian.net/browse/OCPBUGS-99219 to this PR, since it is somewhat related.
- How to verify it
A
machinesetwith a non standard vsphere template name(not matching the old name the MCO compute from the infra object) should be successfully reconciled.Existing vSphere boot image e2es should also continue to pass.
Summary by CodeRabbit
New Features
Permissions
Reliability