OCPBUGS-105426: OCPBUGS-105430: OCPBUGS-105431: OCPBUGS-105315: Fix duplicate template error and e2es

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, &notFoundErr) {
+			// 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
@@ -362,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
@@ -383,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 {
@@ -742,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 {

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))
+		})
+	}
+}

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

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()
 
@@ -102,6 +111,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)
@@ -957,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)