From f314ab7007ffb5eb90b84339e00a816a6bfbbfe7 Mon Sep 17 00:00:00 2001 From: David Joshy Date: Fri, 7 Aug 2026 14:53:20 -0400 Subject: [PATCH 1/5] vsphere: scope template lookup to folder --- pkg/controller/bootimage/vsphere_helpers.go | 26 ++++++++++- .../bootimage/vsphere_helpers_test.go | 44 +++++++++++++++++++ 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/pkg/controller/bootimage/vsphere_helpers.go b/pkg/controller/bootimage/vsphere_helpers.go index fd8d36342f..e8028efe3c 100644 --- a/pkg/controller/bootimage/vsphere_helpers.go +++ b/pkg/controller/bootimage/vsphere_helpers.go @@ -9,6 +9,7 @@ import ( "net/url" "os" "path" + "strings" "github.com/vmware/govmomi" "github.com/vmware/govmomi/find" @@ -198,7 +199,13 @@ func findAllRequiredResources(ctx context.Context, finder *find.Finder, provider if err != nil { return nil, fmt.Errorf("failed to find datastore: %w", err) } - vr.existingVM, err = finder.VirtualMachine(ctx, name) + scopedPath := templateSearchPath(providerSpec.Workspace.Folder, name) + vr.existingVM, err = finder.VirtualMachine(ctx, scopedPath) + if _, ok := err.(*find.NotFoundError); ok && scopedPath != name { + // Nothing at that path inside the workspace folder — fall back to a global search so we can + // still detect (and log) a customer-managed VM of the same name living elsewhere. + vr.existingVM, err = finder.VirtualMachine(ctx, name) + } if err != nil { if _, ok := err.(*find.NotFoundError); ok { klog.Infof("VM Template with name %s does not already exists", name) @@ -230,6 +237,15 @@ func isInFolder(vm *object.VirtualMachine, folder *object.Folder) bool { return path.Dir(vm.InventoryPath) == folder.InventoryPath } +// templateSearchPath scopes name to folder to avoid ambiguity when the same name exists in multiple +// folders. Left unscoped when there's no folder, or name is already an absolute inventory path. +func templateSearchPath(folder, name string) string { + if folder == "" || strings.HasPrefix(name, "/") { + return name + } + return path.Join(folder, name) +} + // getDiskTypeFromExistingVM inspects the given VM's disk backing configuration and returns its disk provisioning type (thin, thick, eagerZeroedThick). func getDiskTypeFromExistingVM(vmMo mo.VirtualMachine) string { diskType := "" @@ -325,7 +341,13 @@ func resolveExistingTemplateVM( // Check providerSpec.Template first so a freshly-added failure domain whose MachineSet // already has a valid template doesn't fail just because the infra computed name isn't a match. if providerSpec.Template != "" && providerSpec.Template != computedName { - tmplVM, tmplErr := finder.VirtualMachine(ctx, providerSpec.Template) + scopedPath := templateSearchPath(providerSpec.Workspace.Folder, providerSpec.Template) + tmplVM, tmplErr := finder.VirtualMachine(ctx, scopedPath) + if scopedPath != providerSpec.Template && errors.As(tmplErr, ¬FoundErr) { + // Nothing at that path inside the workspace folder — fall back to a global search so we + // can still detect (and log) a customer-managed VM of the same name living elsewhere. + tmplVM, tmplErr = finder.VirtualMachine(ctx, providerSpec.Template) + } switch { case tmplErr == nil && isInFolder(tmplVM, workspaceFolder): return tmplVM, providerSpec.Template, false, nil diff --git a/pkg/controller/bootimage/vsphere_helpers_test.go b/pkg/controller/bootimage/vsphere_helpers_test.go index 1f77db8bb7..70c541e191 100644 --- a/pkg/controller/bootimage/vsphere_helpers_test.go +++ b/pkg/controller/bootimage/vsphere_helpers_test.go @@ -112,3 +112,47 @@ func TestIsInFolder(t *testing.T) { }) } } + +// TestTemplateSearchPath verifies the folder-scoped path used to disambiguate templates that share a +// name across folders (OCPBUGS-105426): a bare-name finder.VirtualMachine search matches anywhere in +// vCenter and errors out with "resolves to multiple vms" if two same-named templates exist in different +// folders, even when one of them unambiguously lives in the workspace folder MCO manages. +func TestTemplateSearchPath(t *testing.T) { + tests := []struct { + name string + folder string + vmName string + want string + }{ + { + name: "scopes to the workspace folder", + folder: "/dc1/vm/openshift4-folder", + vmName: "rhcos-template", + want: "/dc1/vm/openshift4-folder/rhcos-template", + }, + { + name: "no folder configured stays unscoped", + folder: "", + vmName: "rhcos-template", + want: "rhcos-template", + }, + { + name: "template already an absolute inventory path stays unscoped", + folder: "/dc1/vm/openshift4-folder", + vmName: "/dc1/vm/customer-folder/rhcos-template", + want: "/dc1/vm/customer-folder/rhcos-template", + }, + { + name: "trailing slash on folder does not produce a double slash", + folder: "/dc1/vm/openshift4-folder/", + vmName: "rhcos-template", + want: "/dc1/vm/openshift4-folder/rhcos-template", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, templateSearchPath(tt.folder, tt.vmName)) + }) + } +} From 81cacd39413063a8d6a616bebdd4b74e1cf15cb6 Mon Sep 17 00:00:00 2001 From: David Joshy Date: Fri, 7 Aug 2026 14:55:59 -0400 Subject: [PATCH 2/5] test: match vsphere fd on datastore/pool too --- test/extended-priv/machineset.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/test/extended-priv/machineset.go b/test/extended-priv/machineset.go index 50cb0961cc..dee76332bb 100644 --- a/test/extended-priv/machineset.go +++ b/test/extended-priv/machineset.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "path" "strconv" "strings" "time" @@ -734,8 +735,9 @@ func (ms MachineSet) SetAutoscalerLabels(labels string) error { } // GetVSphereFailureDomain returns the failure domain from the infrastructure resource that matches -// the given MachineSet's workspace. It matches by comparing the workspace server and datacenter -// against each failure domain's server and topology.datacenter. +// the given MachineSet's workspace. Two failure domains can share the same server/datacenter but use +// different datastore/resourcePool values, so all four are compared — mirroring the matching logic in +// createNewVMTemplate (pkg/controller/bootimage/vsphere_helpers.go) — to avoid picking the wrong domain. func GetVSphereFailureDomain(ms *MachineSet) (string, error) { workspace, err := ms.Get(`{.spec.template.spec.providerSpec.value.workspace}`) if err != nil { @@ -747,6 +749,8 @@ func GetVSphereFailureDomain(ms *MachineSet) (string, error) { if wsServer == "" || wsDataCenter == "" { return "", fmt.Errorf("workspace in MachineSet %s is missing server or datacenter", ms.GetName()) } + wsDatastore := gjson.Get(workspace, "datastore").String() + wsResourcePool := gjson.Get(workspace, "resourcePool").String() infra := NewResource(ms.GetOC().AsAdmin(), "infrastructure", "cluster") failureDomains, err := infra.Get(`{.spec.platformSpec.vsphere.failureDomains}`) @@ -755,12 +759,15 @@ func GetVSphereFailureDomain(ms *MachineSet) (string, error) { } for _, fd := range gjson.Parse(failureDomains).Array() { - if fd.Get("server").String() == wsServer && fd.Get("topology.datacenter").String() == wsDataCenter { + if fd.Get("server").String() == wsServer && + fd.Get("topology.datacenter").String() == wsDataCenter && + fd.Get("topology.datastore").String() == wsDatastore && + path.Clean(fd.Get("topology.resourcePool").String()) == path.Clean(wsResourcePool) { return fd.Raw, nil } } - return "", fmt.Errorf("no failure domain found matching server=%s datacenter=%s for MachineSet %s", wsServer, wsDataCenter, ms.GetName()) + return "", fmt.Errorf("no failure domain found matching server=%s datacenter=%s datastore=%s resourcePool=%s for MachineSet %s", wsServer, wsDataCenter, wsDatastore, wsResourcePool, ms.GetName()) } // GetVSphereConnectionInfoForMachineSet returns the vSphere connection info for the failure domain From ec088424b5974645c33b9746b874c00cbdcb0194 Mon Sep 17 00:00:00 2001 From: David Joshy Date: Fri, 7 Aug 2026 15:02:51 -0400 Subject: [PATCH 3/5] test/extended: reupload vsphere image before reuse --- test/extended-priv/mco_bootimages.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/extended-priv/mco_bootimages.go b/test/extended-priv/mco_bootimages.go index 033581db88..07b60a0c6c 100644 --- a/test/extended-priv/mco_bootimages.go +++ b/test/extended-priv/mco_bootimages.go @@ -102,6 +102,22 @@ var _ = g.Describe("[sig-mco][Suite:openshift/machine-config-operator/longdurati ).To(o.Succeed(), "Error configuring Partial managedBootImages in the 'cluster' MachineConfiguration resource") logger.Infof("OK!\n") + // vSphere updates templates in-place (same name, new content), so the vSphere template behind + // backdatedImageName was likely already reconciled to the current release by the first update + // above. Re-upload it so this second use is genuinely backdated again, or MCO will see an + // already-current template and never trigger the update this check expects. + if exutil.CheckPlatform(oc) == VspherePlatform { + exutil.By("Re-upload the backdated vSphere template so it is genuinely backdated again") + vsInfo, vsErr := GetVSphereConnectionInfoForMachineSet(machineSet) + o.Expect(vsErr).NotTo(o.HaveOccurred(), "Error getting the vSphere connection info for %s", machineSet) + folder, fErr := machineSet.GetWorkspaceFolder() + o.Expect(fErr).NotTo(o.HaveOccurred(), "Error getting the workspace folder for %s", machineSet) + o.Expect(exutil.DeleteVsphereTemplate(backdatedImageName, folder, vsInfo)).To(o.Succeed(), + "Error deleting the already-updated vSphere template %s", backdatedImageName) + backdatedImageName = getBackdatedBootImage(oc.AsAdmin(), machineSet) + logger.Infof("OK!\n") + } + exutil.By("Patch coreos boot image in MachineSet") o.Expect(machineSet.SetCoreOsBootImage(backdatedImageName)).To(o.Succeed(), "Error patching the value of the coreos boot image in %s", machineSet) From 14ce75574bc8b2286be57b6288f4d0747875513e Mon Sep 17 00:00:00 2001 From: David Joshy Date: Mon, 10 Aug 2026 14:11:50 -0400 Subject: [PATCH 4/5] test: use per-run unique vsphere template name --- test/extended-priv/mco_bootimages.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/test/extended-priv/mco_bootimages.go b/test/extended-priv/mco_bootimages.go index 07b60a0c6c..a8df8b13cd 100644 --- a/test/extended-priv/mco_bootimages.go +++ b/test/extended-priv/mco_bootimages.go @@ -5,6 +5,7 @@ import ( "regexp" "strings" + "github.com/google/uuid" g "github.com/onsi/ginkgo/v2" o "github.com/onsi/gomega" exutil "github.com/openshift/machine-config-operator/test/extended-priv/util" @@ -18,6 +19,14 @@ import ( const mapiBaseErrorMessageTemplate = `1 Degraded MAPI MachineSets | 0 Degraded ControlPlaneMachineSets | 0 Degraded CAPI MachineSets | 0 Degraded CAPI MachineDeployments | Error(s):` + ` error syncing MAPI MachineSet %s: failed to reconcile machineset %s, err:` +// backdatedImageRunID is generated once per test binary process, so every vSphere backdated +// template this run uploads gets a name unique to that run. Without it, every run uploads under +// the exact same literal name regardless of which failure domain/folder it lands in — so a leftover +// template from an earlier (or crashed, uncleaned-up) run collides with the current run's upload, +// which is what caused both the MCO-side "resolves to multiple vms" bug and the machine-api-provider- +// vsphere actuator's own "multiple templates found" clone-time failure. +var backdatedImageRunID = strings.ReplaceAll(uuid.NewString(), "-", "")[:8] + var _ = g.Describe("[sig-mco][Suite:openshift/machine-config-operator/longduration][Serial][Disruptive] MCO Bootimages", func() { defer g.GinkgoRecover() @@ -973,8 +982,9 @@ func getBackdatedBootImage(oc *exutil.CLI, ms *MachineSet) string { baseImageURL, err := rhcosHandler.GetBaseImageURLFromRHCOSImageInfo(imageVersion, OSImageStreamRHEL9, arch) o.Expect(err).NotTo(o.HaveOccurred(), "Error getting the base image URL") - // To avoid collisions we will add prefix to identify our image - baseImage = "mcotest-" + baseImage + // To avoid collisions with other test runs (including leftovers from a crashed, uncleaned-up + // run) we prefix with a per-run-unique ID in addition to the "mcotest-" marker. + baseImage = fmt.Sprintf("mcotest-%s-%s", backdatedImageRunID, baseImage) o.Expect( uploadBaseImageToCloud(ms, platform, baseImageURL, baseImage), ).To(o.Succeed(), "Error uploading the base image %s to the cloud", baseImageURL) From 7a8747093244ceec8247a6d971c8d05a5eb1a0f1 Mon Sep 17 00:00:00 2001 From: David Joshy Date: Thu, 6 Aug 2026 13:32:30 -0400 Subject: [PATCH 5/5] vsphere: check secret before mutating template --- pkg/controller/bootimage/vsphere_helpers.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/pkg/controller/bootimage/vsphere_helpers.go b/pkg/controller/bootimage/vsphere_helpers.go index e8028efe3c..99eecf0368 100644 --- a/pkg/controller/bootimage/vsphere_helpers.go +++ b/pkg/controller/bootimage/vsphere_helpers.go @@ -384,6 +384,12 @@ func resolveExistingTemplateVM( if len(computedName) > 80 { return nil, "", false, fmt.Errorf("length of VM template name `%s` exceeds the permitted limit of 80 characters", computedName) } + // Validate/upgrade the ignition stub before creating the template in vSphere. If this fails, + // we must not have already mutated vSphere state, or a subsequent reconcile would find the + // template already in place and silently drop the error (see reconcileVSphereProviderSpec). + if err := upgradeStubIgnitionIfRequired(providerSpec.UserDataSecret.Name, kubeClient); err != nil { + return nil, "", false, err + } ova, ovaErr := streamData.QueryDisk(arch, "vmware", "ova") if ovaErr != nil { return nil, "", false, ovaErr @@ -405,6 +411,11 @@ func resolveExistingTemplateVM( } // Rollback: restore the old template renamed away during a crashed atomic swap. + // Validate/upgrade the ignition stub before this rename, so an invalid user-data secret blocks + // even this recovery mutation rather than only the OVA-driven create/swap paths. + if err := upgradeStubIgnitionIfRequired(providerSpec.UserDataSecret.Name, kubeClient); err != nil { + return nil, "", false, err + } klog.Infof("Recovering from mid-swap crash: renaming %s back to %s", oldTempName, computedName) renameTask, renameErr := oldVM.Rename(ctx, computedName) if renameErr != nil { @@ -764,6 +775,14 @@ func createNewVMTemplate(streamData *stream.Stream, providerSpec *machinev1beta1 if templateProductVersion != release { klog.Infof("Existing RHCOS v%s does not match current RHCOS v%s. Starting reconciliation process.", templateProductVersion, release) + // Validate/upgrade the ignition stub before swapping the template in vSphere. If this + // fails, we must not have already mutated vSphere state, or a subsequent reconcile would + // find the template already up to date and silently drop the error (see + // reconcileVSphereProviderSpec). + if err := upgradeStubIgnitionIfRequired(providerSpec.UserDataSecret.Name, kubeClient); err != nil { + return "", false, err + } + // Find and download the relevant OVA file ova, err := streamData.QueryDisk(arch, "vmware", "ova") if err != nil {