OCPBUGS-104569: vSphere boot image reconciler overwrites/renames current custom-named templates - #6354
Open
jcpowermac wants to merge 3 commits into
Open
OCPBUGS-104569: vSphere boot image reconciler overwrites/renames current custom-named templates#6354jcpowermac wants to merge 3 commits into
jcpowermac wants to merge 3 commits into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
Author
|
/test ? |
|
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:
WalkthroughThe change adds unit, simulator-backed, OVF import, and extended-privilege coverage for boot image caching, platform handling, vSphere template reconciliation, and workspace-specific vSphere workflows. ChangesBoot image and vSphere workflows
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MachineSet
participant Controller
participant VCenterClient
participant OVA
MachineSet->>Controller: reconcile workspace template
Controller->>VCenterClient: resolve server and datacenter
Controller->>OVA: upload selected OVA
OVA-->>VCenterClient: provide OVA content
VCenterClient-->>Controller: return template release
Controller-->>MachineSet: preserve or update template reference
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
/test unit |
Contributor
Author
|
/test e2e-vsphere-ovn-zones |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/controller/bootimage/vsphere_helpers.go (1)
232-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale sibling comment: "17 chars" still present elsewhere in this file.
This comment was correctly fixed to say 16 chars, but
createNewVMTemplateWithNameForFailureDomainin the same file still documentstempName/oldTempNameas "fixed-length (17 chars)", which is now inconsistent with both this fix andTestAtomicTempName'slen(nameA) != 16assertion. Please update that comment too for consistency.📝 Suggested fix
- // tempName is where the new OVA is imported; oldTempName holds the existing template during - // the swap. Both are fixed-length (17 chars) so they always fit within vSphere's 80-char limit. + // tempName is where the new OVA is imported; oldTempName holds the existing template during + // the swap. Both are fixed-length (16 chars) so they always fit within vSphere's 80-char limit.🤖 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/bootimage/vsphere_helpers.go` at line 232, Update the documentation in createNewVMTemplateWithNameForFailureDomain so tempName and oldTempName are described as fixed-length 16-character names, matching atomicTempName and the TestAtomicTempName assertion.pkg/controller/bootimage/vsphere_ova_import_spike_test.go (1)
145-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIs this spike test still needed now that full coverage exists?
This test self-documents as "a throwaway spike (see plan A0)", and its "vcsim supports OVF import" premise is now proven more thoroughly by
TestCreateNewVMTemplateinvsphere_create_template_test.go(which also exercises rollback/error paths this spike doesn't). It also re-implements ~50 lines of simulator bring-up already centralized innewSimulatedVCenter/newSimulatedVCenters(vsphere_simulator_test.go).Consider either removing
TestSpikeVcsimSupportsOvfImport(keepingbuildMinimalOVA, which is still used byvsphere_create_template_test.go) or refactoring it to reusenewSimulatedVCenterto cut the duplication if it's being kept intentionally as a standalone smoke test.🤖 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/bootimage/vsphere_ova_import_spike_test.go` around lines 145 - 268, The standalone TestSpikeVcsimSupportsOvfImport is now redundant with TestCreateNewVMTemplate and duplicates simulator setup. Remove the spike test while retaining buildMinimalOVA because it is still used elsewhere; if the test is intentionally preserved, refactor its setup to use newSimulatedVCenter or newSimulatedVCenters instead of creating the simulator inline.pkg/controller/bootimage/vsphere_create_template_test.go (1)
98-329: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLGTM! One small optional note: the
newSimulatedVCenters(t, 1)/buildFailureDomain/buildVSphereInfra/buildVSphereCredsSecretquartet is repeated verbatim across most subtests here — could be pulled into a smallnewSingleVCenterFixture(t)helper, but the current explicit style keeps each subtest self-contained, so this is purely optional.🤖 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/bootimage/vsphere_create_template_test.go` around lines 98 - 329, Optionally reduce repeated single-vCenter test setup by introducing a newSingleVCenterFixture helper that encapsulates newSimulatedVCenters, buildFailureDomain, buildVSphereInfra, and buildVSphereCredsSecret. Update applicable TestCreateNewVMTemplate subtests to use the helper while preserving their independent setup and behavior; this refactor is optional.
🤖 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/bootimage/vsphere_create_template_test.go`:
- Around line 98-329: Optionally reduce repeated single-vCenter test setup by
introducing a newSingleVCenterFixture helper that encapsulates
newSimulatedVCenters, buildFailureDomain, buildVSphereInfra, and
buildVSphereCredsSecret. Update applicable TestCreateNewVMTemplate subtests to
use the helper while preserving their independent setup and behavior; this
refactor is optional.
In `@pkg/controller/bootimage/vsphere_helpers.go`:
- Line 232: Update the documentation in
createNewVMTemplateWithNameForFailureDomain so tempName and oldTempName are
described as fixed-length 16-character names, matching atomicTempName and the
TestAtomicTempName assertion.
In `@pkg/controller/bootimage/vsphere_ova_import_spike_test.go`:
- Around line 145-268: The standalone TestSpikeVcsimSupportsOvfImport is now
redundant with TestCreateNewVMTemplate and duplicates simulator setup. Remove
the spike test while retaining buildMinimalOVA because it is still used
elsewhere; if the test is intentionally preserved, refactor its setup to use
newSimulatedVCenter or newSimulatedVCenters instead of creating the simulator
inline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c8939da4-5ac0-4168-b71f-c060c96fd52f
⛔ Files ignored due to path filters (113)
vendor/github.com/vmware/govmomi/simulator/alarm_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/authorization_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/cluster_compute_resource.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/container.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/container_host_system.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/container_virtual_machine.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/crypto_manager_kmip.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/custom_fields_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/customization_spec_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/datacenter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/dataset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/datastore.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/datastore_namespace_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/dvs.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/dvs_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/entity.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/environment_browser.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/authorization_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/datacenter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/event_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/host_capability.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/host_config_filesystemvolume.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/host_config_info.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/host_firewall_system.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/host_hardware_info.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/host_storage_device_info.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/host_system.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/host_vnic_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/performance_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/performance_manager_data.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/resource_pool.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/root_folder.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/service_content.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/setting.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/task_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/esx/virtual_device.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/event_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/extension_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/file_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/folder.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/guest_id.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/guest_operations_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/history_collector.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/host_certificate_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/host_datastore_browser.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/host_datastore_system.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/host_firewall_system.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/host_local_account_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/host_network_system.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/host_storage_system.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/host_system.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/host_vnic_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/http_nfc_lease.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/internal/object_lock.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/internal/server.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/internal/testcert.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/internal/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/ip_pool_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/license_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/model.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/object.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/option_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/ovf_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/performance_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/portgroup.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/property_collector.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/property_filter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/registry.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/resource_pool.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/search_index.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/service_instance.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/session_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/simulator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/snapshot.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/storage_resource_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/task.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/task_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/tenant_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/user_directory.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/view_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/virtual_disk_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/virtual_machine.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/vm_compatibility_checker.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/vm_provisioning_checker.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/vpx/alarm_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/vpx/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/vpx/performance_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/vpx/performance_manager_data.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/vpx/root_folder.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/vpx/service_content.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/vpx/setting.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/vpx/task_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/simulator/vstorage_object_manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/toolbox/hgfs/archive.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/toolbox/hgfs/encoding.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/toolbox/hgfs/hgfs_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/toolbox/hgfs/hgfs_other.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/toolbox/hgfs/protocol.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/toolbox/hgfs/server.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/toolbox/process/process.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/toolbox/vix/property.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/toolbox/vix/protocol.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/units/size.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/vapi/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/vapi/resource.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/vapi/simulator/simulator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/vapi/vcenter/vcenter_ovf.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/vapi/vcenter/vcenter_vmtx.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/vapi/vm/dataset/dataset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/vmware/govmomi/vapi/vm/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (12)
pkg/controller/bootimage/cache/cache_test.gopkg/controller/bootimage/ms_helpers_test.gopkg/controller/bootimage/platform_helpers_test.gopkg/controller/bootimage/vsphere_create_template_test.gopkg/controller/bootimage/vsphere_helpers.gopkg/controller/bootimage/vsphere_helpers_test.gopkg/controller/bootimage/vsphere_object_helpers_test.gopkg/controller/bootimage/vsphere_ova_import_spike_test.gopkg/controller/bootimage/vsphere_simulator_test.gopkg/controller/common/featuregates_test.gotest/extended-priv/mco_bootimages.gotest/extended-priv/util/vsphere.go
jcpowermac
added a commit
to jcpowermac/release
that referenced
this pull request
Jul 31, 2026
…ator main Adds three optional/on-demand presubmit jobs (plain IPI, zonal single-vCenter, multi-vCenter) scoped to the MCO Bootimages longduration spec group, so the new vSphere multi-vCenter/multi-failure-domain reconciliation e2e specs in openshift/machine-config-operator#6354 can be exercised via /test before they land in a periodic. Reuses the existing openshift-e2e-vsphere and openshift-e2e-vsphere-zones workflows, differentiated only by MAX_VCENTERS per SPLAT-2238 (openshift#81215), instead of a separate hardcoded-pool multi-vcenter workflow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jcpowermac
added a commit
to jcpowermac/release
that referenced
this pull request
Jul 31, 2026
…ator main Adds three optional/on-demand presubmit jobs (plain IPI, zonal single-vCenter, multi-vCenter) scoped to the MCO Bootimages longduration spec group, so the new vSphere multi-vCenter/multi-failure-domain reconciliation e2e specs in openshift/machine-config-operator#6354 can be exercised via /test before they land in a periodic. Reuses the existing openshift-e2e-vsphere and openshift-e2e-vsphere-zones workflows, differentiated only by MAX_VCENTERS per SPLAT-2238 (openshift#81215), instead of a separate hardcoded-pool multi-vcenter workflow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jcpowermac
added a commit
to jcpowermac/release
that referenced
this pull request
Jul 31, 2026
…ator main Adds three optional/on-demand presubmit jobs (plain IPI, zonal single-vCenter, multi-vCenter) scoped to the MCO Bootimages longduration spec group, so the new vSphere multi-vCenter/multi-failure-domain reconciliation e2e specs in openshift/machine-config-operator#6354 can be exercised via /test before they land in a periodic. Reuses the existing openshift-e2e-vsphere and openshift-e2e-vsphere-zones workflows, differentiated only by MAX_VCENTERS per SPLAT-2238 (openshift#81215), instead of a separate hardcoded-pool multi-vcenter workflow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
openshift-merge-bot Bot
pushed a commit
to openshift/release
that referenced
this pull request
Jul 31, 2026
…ator main (#82723) Adds three optional/on-demand presubmit jobs (plain IPI, zonal single-vCenter, multi-vCenter) scoped to the MCO Bootimages longduration spec group, so the new vSphere multi-vCenter/multi-failure-domain reconciliation e2e specs in openshift/machine-config-operator#6354 can be exercised via /test before they land in a periodic. Reuses the existing openshift-e2e-vsphere and openshift-e2e-vsphere-zones workflows, differentiated only by MAX_VCENTERS per SPLAT-2238 (#81215), instead of a separate hardcoded-pool multi-vcenter workflow. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
/test ? |
Contributor
Author
|
/test e2e-vsphere-multi-vcenter-mco-bootimages |
Contributor
Author
|
/test ? |
Contributor
Author
|
/test e2e-vsphere-mco-bootimages |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
test/extended-priv/mco_bootimages.go (1)
862-866: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
fdGrouptype.
fdGroupis declared but never instantiated; the code usesfdByServer/fdOrderinstead. This is dead code that a static-analysisunusedcheck could flag.🤖 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/extended-priv/mco_bootimages.go` around lines 862 - 866, Remove the unused fdGroup type declaration near fdOrder, leaving the fdByServer and fdOrder implementation 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.
Inline comments:
In `@pkg/controller/bootimage/vsphere_helpers.go`:
- Around line 622-630: Update the isTemplateAtRelease call in the
providerSpec.Template preservation check to capture its error instead of
discarding it. Ignore only *find.NotFoundError as the benign “not current” case;
propagate any other lookup or Properties() error, matching the existing
existingTemplateVM error handling, while preserving the current-template early
return.
In `@test/extended-priv/mco_bootimages.go`:
- Around line 1342-1352: Handle and propagate errors from both MachineSet
workspace lookups in the block before Eventually, rather than discarding them.
Update the surrounding verification flow so failures from ms.Get for
workspace.server or workspace.datacenter are surfaced and prevent
getReleaseFromVsphereTemplate from proceeding with empty values or the default
vCenter fallback.
- Around line 831-949: Fix buildWorkspaceGroupsAcrossVCenters so partial
failures clean up synthetic MachineSets created before the failure. Define the
cleanup closure before the server-processing loop, keep it referencing the
evolving syntheticMachineSets slice, and return that cleanup function on every
subsequent error path instead of func() {}.
- Around line 896-928: Update the CloneResource transformation callback to set
the synthetic MachineSet’s network.devices[0].networkName field to fd.Network,
alongside the existing workspace overrides. Preserve the donor clone behavior
for all other fields so scaling uses the requested topology network.
In `@test/extended-priv/util/vsphere.go`:
- Around line 409-444: Update GetAllVSphereFailureDomains to capture the first
base64 decoding error encountered while processing credsMap instead of silently
continuing. After the gjson ForEach loop, return that error before the
no-failure-domains check, matching GetVSphereConnectionInfo’s fail-fast behavior
and preventing incomplete credentials from being returned.
---
Nitpick comments:
In `@test/extended-priv/mco_bootimages.go`:
- Around line 862-866: Remove the unused fdGroup type declaration near fdOrder,
leaving the fdByServer and fdOrder implementation unchanged.
🪄 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: ac320457-3f72-45cc-89b8-a0ce03f1bb26
📒 Files selected for processing (5)
pkg/controller/bootimage/vsphere_create_template_test.gopkg/controller/bootimage/vsphere_helpers.gotest/extended-priv/mco_bootimages.gotest/extended-priv/mco_scale.gotest/extended-priv/util/vsphere.go
jcpowermac
force-pushed
the
vsphere-ova-test-coverage
branch
from
August 3, 2026 17:47
7380358 to
394bb7b
Compare
Contributor
Author
|
/test e2e-vsphere-mco-bootimages |
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 `@test/extended-priv/util/vsphere.go`:
- Around line 422-436: Update the credential resolution around
GetAllVSphereFailureDomains and its ForEach callback to decode credsMap once
before iterating failure domains, then use direct lookups for each domain’s
userKey and passKey. Explicitly detect either missing credential key and return
false with the existing error mechanism, while preserving decode-error handling
and assigning info.User and info.Password only from the resolved values.
🪄 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: 2a3727a2-a681-4546-8e40-0742ca362831
📒 Files selected for processing (6)
pkg/controller/bootimage/helpers.gopkg/controller/bootimage/vsphere_create_template_test.gopkg/controller/bootimage/vsphere_helpers.gotest/extended-priv/mco_bootimages.gotest/extended-priv/mco_scale.gotest/extended-priv/util/vsphere.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/controller/bootimage/vsphere_helpers.go
- test/extended-priv/mco_scale.go
- pkg/controller/bootimage/vsphere_create_template_test.go
- test/extended-priv/mco_bootimages.go
jcpowermac
force-pushed
the
vsphere-ova-test-coverage
branch
from
August 3, 2026 20:34
394bb7b to
05c2177
Compare
Contributor
Author
|
/test e2e-vsphere-multi-vcenter-mco-bootimages |
jcpowermac
force-pushed
the
vsphere-ova-test-coverage
branch
from
August 4, 2026 14:20
05c2177 to
7d804d9
Compare
Contributor
Author
|
/test e2e-vsphere-multi-vcenter-mco-bootimages |
Contributor
Author
|
/test unit |
openshift-ci
Bot
added
the
needs-rebase
jcpowermac
force-pushed
the
vsphere-ova-test-coverage
branch
from
August 4, 2026 16:45
7d804d9 to
0d7bb24
Compare
openshift-ci
Bot
removed
the
needs-rebase
Contributor
Author
|
/test e2e-vsphere-multi-vcenter-mco-bootimages |
openshift-ci-robot
added
jira/valid-bug
Contributor
|
@jcpowermac: This pull request references Jira Issue OCPBUGS-104569, 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. |
…mplates Bug: when a vSphere MachineSet's providerSpec.Template pointed at a non-standard (customer-managed) name, createNewVMTemplate treated any mismatch against the computed canonical name as drift and any RHCOS version mismatch as staleness, without checking whether the custom-named template already held the current release. This caused the controller to rename or rebuild valid, up-to-date custom templates on every reconcile, and rebuilding into the computed name could collide with a current template already sitting there under that name from before the custom name was adopted. createNewVMTemplate now calls the new isTemplateAtRelease helper to check the embedded RHCOS product version before acting: a custom name that already matches the target release is left alone, and an outdated custom template converges to the computed name only when the computed name doesn't already have its own current template (in which case the controller just switches back to it instead of rebuilding). Converging a custom-named template to the computed name during a rebuild requires swapping out a VM known under a name other than the one being created, so findAllRequiredResources and createNewVMTemplateWithNameForFailureDomain now take the already-resolved *object.VirtualMachine directly instead of re-resolving it by the (possibly different) target name. atomicTempName is exported as AtomicTempName so e2e coverage can compute the same "mco-old-<hash>" rollback name without duplicating the hashing logic. Also fixes an unrelated %wt -> %w format-verb typo in upgradeStubIgnitionIfRequired's error wrapping. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jcpowermac
force-pushed
the
vsphere-ova-test-coverage
branch
from
August 5, 2026 16:09
1f57c13 to
a83b588
Compare
djoshy
reviewed
Aug 5, 2026
| // (see the release-matches branch below). But the computed name may already have | ||
| // its own current template sitting there (e.g. left behind from before the | ||
| // custom name was adopted) — vSphere names are unique, so rebuilding straight | ||
| // into computedName would collide with it. Check for that first and, if found, |
Contributor
There was a problem hiding this comment.
Should we add a test case for the collision scenario? Perhaps in TestCreateNewVMTemplate()?
Contributor
|
Overall looks good to me! I read through the tests and they are all great additions and make sense - I just have one suggestion above. I've asked @sergiordlr to take a look at the extended tests additions and also to verify the first commit of the PR. |
djoshy
reviewed
Aug 5, 2026
Contributor
There was a problem hiding this comment.
Could we roll this into the main vendor commit?
Adds a govmomi simulator-backed unit test harness for the vSphere boot image path: pure-function coverage for the OVA/atomic-swap helpers, simulator-backed coverage for vSphere object lookups and createNewVMTemplate's decision tree (including the multi-vCenter and multi-failure-domain cases and the custom-template-preservation fix), TestReconcileVSphereProviderSpec, hot-loop-by-version coverage, and DownloadOva and CheckBootImagePlatform unit tests. Also adds e2e specs under test/extended-priv covering multi-vCenter and multi-failure-domain reconciliation and non-standard providerSpec.Template names, and supporting helpers for datacenter-aware template lookups and backdated image uploads scoped to a MachineSet's own workspace instead of failureDomains[0]. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rage
Vendors additional packages from the already-present
github.com/vmware/govmomi module (no version bump): the simulator
package (and its esx/vpx/internal subpackages) used to back the vSphere
boot image unit tests with a fake vCenter, plus toolbox, units, vapi,
and vapi/{simulator,vcenter,vm} needed transitively.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jcpowermac
force-pushed
the
vsphere-ova-test-coverage
branch
from
August 5, 2026 19:36
85960a9 to
35e5a95
Compare
Contributor
|
Hello, regarding the e2e tests it seems that we have been working on the same thing. We created this PR to adapt the vsphere bootimages tests to multi vcenter clusters. |
Contributor
Author
mine is tested and ready to go, is there any reason not to merge this pr? |
Contributor
Author
|
/test unit |
Contributor
Author
|
/verified by @jcpowermac via new unit and e2e tests. |
openshift-ci-robot
added
the
verified
Contributor
|
@jcpowermac: 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. |
Contributor
|
@jcpowermac: The following tests 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. |
sergiordlr
reviewed
Aug 7, 2026
| setArchitectureAndCheckStatus(clonedMS, machineConfiguration, arch.String()) | ||
| }) | ||
|
|
||
| g.It("[OTP] Boot image controller preserves a valid non-standard providerSpec.Template name", g.Label("Platform:vsphere"), func() { |
Contributor
There was a problem hiding this comment.
This test case looks like a duplicate of this other existing test case
"[PolarionID:74239][OTP] ManagedBootImages. Restore Partial MachineSet images"
https://github.com/openshift/machine-config-operator/pull/6317/changes#diff-c4d4c5caa7275da9e1aff5004390b5f6fc089e798f7eefb7a5151e4863154944L224
Is it like that?
- Configure machineconfiguration to patch only labeled machinesets
- Configure a backdated template using a non-canonical name
- Label the machineset to trigger the upgrade
- Check that the template is updated to the latest version, but the name of the template hasn't changed
Contributor
|
The fix to make the bootimages properly work with multivcenters broke many of the existing extended test cases, which expected the name of the template to change. Making the automation code able to properly handle the new scenarios took a while. We used this PR to fix the currently existing extended tests so that they can handle the new logic I would like to merge this PR and have all tests passing before modifying the code further and add new test cases. cc: @djoshy |
Contributor
|
We are in the process of merging #6317 I'm temporarily holding this PR /hold |
openshift-ci
Bot
added
the
do-not-merge/hold
TimurMP
pushed a commit
to TimurMP/release
that referenced
this pull request
Aug 9, 2026
…ator main (openshift#82723) Adds three optional/on-demand presubmit jobs (plain IPI, zonal single-vCenter, multi-vCenter) scoped to the MCO Bootimages longduration spec group, so the new vSphere multi-vCenter/multi-failure-domain reconciliation e2e specs in openshift/machine-config-operator#6354 can be exercised via /test before they land in a periodic. Reuses the existing openshift-e2e-vsphere and openshift-e2e-vsphere-zones workflows, differentiated only by MAX_VCENTERS per SPLAT-2238 (openshift#81215), instead of a separate hardcoded-pool multi-vcenter workflow. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
With #6387 merged, we should now be able to go ahead with this. Would you mind rebasing? |
Contributor
|
/unhold |
openshift-ci
Bot
added
needs-rebase
Contributor
|
PR needs rebase. 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. |
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
4 participants
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.
Will fill in this later
- What I did
- How to verify it
- Description for the changelog
Summary by CodeRabbit
Bug Fixes
Tests