Merge pull request #818 from bryanv/bryanv/remove-inventory-path-vm-l…

…ookup

🐛 Remove Inventory path fallback for GetVM

pkg/providers/vsphere/vcenter/getvm.go

@@ -10,22 +10,18 @@ import (
 	"github.com/vmware/govmomi/object"
 	"github.com/vmware/govmomi/vim25"
 	vimtypes "github.com/vmware/govmomi/vim25/types"
-	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
 
-	vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha3"
 	pkgctx "github.com/vmware-tanzu/vm-operator/pkg/context"
-	"github.com/vmware-tanzu/vm-operator/pkg/topology"
 )
 
-// GetVirtualMachine gets the VM from VC, either by the MoID, UUID, or the inventory path.
+// GetVirtualMachine gets the VM from VC, either by the Instance UUID, BIOS UUID, or MoID.
 func GetVirtualMachine(
 	vmCtx pkgctx.VirtualMachineContext,
-	k8sClient ctrlclient.Client,
 	vimClient *vim25.Client,
 	datacenter *object.Datacenter,
 	finder *find.Finder) (*object.VirtualMachine, error) {
 
-	// Find by instance UUID.
+	// Find by Instance UUID.
 	if id := vmCtx.VM.UID; id != "" {
 		if vm, err := findVMByUUID(vmCtx, vimClient, datacenter, string(id), true); err == nil {
 			return vm, nil
@@ -46,8 +42,7 @@ func GetVirtualMachine(
 		}
 	}
 
-	// Find by inventory path.
-	return findVMByInventory(vmCtx, k8sClient, vimClient, finder)
+	return nil, nil
 }
 
 func findVMByMoID(
@@ -91,72 +86,3 @@ func findVMByUUID(
 	vmCtx.Logger.V(4).Info("Found VM via UUID", "uuid", uuid, "isInstanceUUID", isInstanceUUID)
 	return vm, nil
 }
-
-func findVMByInventory(
-	vmCtx pkgctx.VirtualMachineContext,
-	k8sClient ctrlclient.Client,
-	vimClient *vim25.Client,
-	finder *find.Finder) (*object.VirtualMachine, error) {
-
-	// Note that we'll usually only get here to find the VM via its inventory path when we're first
-	// creating the VM. To determine the path, we need the NS Folder MoID and the VM's ResourcePolicy,
-	// if set, and we'll fetch these again as a part of createVirtualMachine(). For now, just re-fetch
-	// but we could pass the Folder MoID and ResourcePolicy to save a bit of duplicated work.
-
-	folderMoID, err := topology.GetNamespaceFolderMoID(vmCtx, k8sClient, vmCtx.VM.Namespace)
-	if err != nil {
-		return nil, err
-	}
-
-	// While we strictly only need the Folder's ManagedObjectReference below, use the Finder
-	// here to check if it actually exists.
-	folder, err := GetFolderByMoID(vmCtx, finder, folderMoID)
-	if err != nil {
-		return nil, fmt.Errorf("failed to get namespace Folder: %w", err)
-	}
-
-	// When the VM has a ResourcePolicy, the VM is placed in a child folder under the namespace's folder.
-	var policyName string
-	if reserved := vmCtx.VM.Spec.Reserved; reserved != nil {
-		policyName = reserved.ResourcePolicyName
-	}
-	if policyName != "" {
-		resourcePolicy := &vmopv1.VirtualMachineSetResourcePolicy{}
-
-		key := ctrlclient.ObjectKey{Name: policyName, Namespace: vmCtx.VM.Namespace}
-		if err := k8sClient.Get(vmCtx, key, resourcePolicy); err != nil {
-			// Note that if VM does not exist, and we're about to create it, the ResourcePolicy is Get()
-			// again so the corresponding condition is almost always true if we don't hit an error here.
-			// Creating the VM with an explicit InstanceUUID is the easiest way out to avoid this.
-			return nil, fmt.Errorf("failed to get VirtualMachineSetResourcePolicy: %w", err)
-		}
-
-		if folderName := resourcePolicy.Spec.Folder; folderName != "" {
-			childFolder, err := GetChildFolder(vmCtx, folder, folderName)
-			if err != nil {
-				vmCtx.Logger.Error(err, "Failed to get VirtualMachineSetResourcePolicy child Folder",
-					"parentPath", folder.InventoryPath, "folderName", folderName, "policyName", policyName)
-				return nil, err
-			}
-
-			folder = childFolder
-		}
-	}
-
-	ref, err := object.NewSearchIndex(vimClient).FindChild(vmCtx, folder.Reference(), vmCtx.VM.Name)
-	if err != nil {
-		return nil, err
-	} else if ref == nil {
-		// VM does not exist.
-		return nil, nil
-	}
-
-	vm, ok := ref.(*object.VirtualMachine)
-	if !ok {
-		return nil, fmt.Errorf("found VM reference was not a VM but a %T", ref)
-	}
-
-	vmCtx.Logger.V(4).Info("Found VM via inventory",
-		"parentFolderMoID", folder.Reference().Value, "moID", vm.Reference().Value)
-	return vm, nil
-}

pkg/providers/vsphere/vcenter/getvm_test.go

@@ -8,10 +8,8 @@ import (
 	. "github.com/onsi/gomega"
 
 	"github.com/vmware/govmomi/vim25/mo"
-	vimtypes "github.com/vmware/govmomi/vim25/types"
 	"k8s.io/apimachinery/pkg/types"
 
-	vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha3"
 	pkgctx "github.com/vmware-tanzu/vm-operator/pkg/context"
 	"github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/vcenter"
 	"github.com/vmware-tanzu/vm-operator/test/builder"
@@ -52,54 +50,6 @@ func getVM() {
 		}
 	})
 
-	Context("Gets VM by inventory", func() {
-		BeforeEach(func() {
-			vm, err := ctx.Finder.VirtualMachine(ctx, vcVMName)
-			Expect(err).ToNot(HaveOccurred())
-
-			task, err := vm.Clone(ctx, nsInfo.Folder, vmCtx.VM.Name, vimtypes.VirtualMachineCloneSpec{})
-			Expect(err).ToNot(HaveOccurred())
-			Expect(task.Wait(ctx)).To(Succeed())
-		})
-
-		It("returns success", func() {
-			vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.Client, ctx.VCClient.Client, ctx.Datacenter, ctx.Finder)
-			Expect(err).ToNot(HaveOccurred())
-			Expect(vm).ToNot(BeNil())
-		})
-
-		It("returns nil if VM does not exist", func() {
-			vmCtx.VM.Name = "bogus"
-			vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.Client, ctx.VCClient.Client, ctx.Datacenter, ctx.Finder)
-			Expect(err).ToNot(HaveOccurred())
-			Expect(vm).To(BeNil())
-		})
-
-		Context("Namespace Folder does not exist", func() {
-			BeforeEach(func() {
-				task, err := nsInfo.Folder.Destroy(vmCtx)
-				Expect(err).ToNot(HaveOccurred())
-				Expect(task.Wait(vmCtx)).To(Succeed())
-			})
-
-			It("returns error", func() {
-				vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.Client, ctx.VCClient.Client, ctx.Datacenter, ctx.Finder)
-				Expect(err).To(HaveOccurred())
-				Expect(err.Error()).To(HavePrefix("failed to get namespace Folder"))
-				Expect(vm).To(BeNil())
-			})
-		})
-
-		It("returns success when MoID is invalid", func() {
-			// Expect fallback to inventory.
-			vmCtx.VM.Status.UniqueID = "vm-bogus"
-
-			vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.Client, ctx.VCClient.Client, ctx.Datacenter, ctx.Finder)
-			Expect(err).ToNot(HaveOccurred())
-			Expect(vm).ToNot(BeNil())
-		})
-	})
-
 	Context("Gets VM when MoID is set", func() {
 		BeforeEach(func() {
 			vm, err := ctx.Finder.VirtualMachine(ctx, vcVMName)
@@ -108,7 +58,7 @@ func getVM() {
 		})
 
 		It("returns success", func() {
-			vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.Client, ctx.VCClient.Client, ctx.Datacenter, ctx.Finder)
+			vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter, ctx.Finder)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(vm).ToNot(BeNil())
 			Expect(vm.Reference().Value).To(Equal(vmCtx.VM.Status.UniqueID))
@@ -126,44 +76,9 @@ func getVM() {
 		})
 
 		It("returns success", func() {
-			vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.Client, ctx.VCClient.Client, ctx.Datacenter, ctx.Finder)
-			Expect(err).ToNot(HaveOccurred())
-			Expect(vm).ToNot(BeNil())
-		})
-	})
-
-	Context("Gets VM with ResourcePolicy by inventory", func() {
-		BeforeEach(func() {
-			resourcePolicy, folder := ctx.CreateVirtualMachineSetResourcePolicy("getvm-test", nsInfo)
-			if vmCtx.VM.Spec.Reserved == nil {
-				vmCtx.VM.Spec.Reserved = &vmopv1.VirtualMachineReservedSpec{}
-			}
-			vmCtx.VM.Spec.Reserved.ResourcePolicyName = resourcePolicy.Name
-
-			vm, err := ctx.Finder.VirtualMachine(ctx, vcVMName)
-			Expect(err).ToNot(HaveOccurred())
-
-			task, err := vm.Clone(ctx, folder, vmCtx.VM.Name, vimtypes.VirtualMachineCloneSpec{})
-			Expect(err).ToNot(HaveOccurred())
-			Expect(task.Wait(ctx)).To(Succeed())
-		})
-
-		It("returns success", func() {
-			vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.Client, ctx.VCClient.Client, ctx.Datacenter, ctx.Finder)
+			vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter, ctx.Finder)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(vm).ToNot(BeNil())
 		})
-
-		It("returns error when ResourcePolicy does not exist", func() {
-			if vmCtx.VM.Spec.Reserved == nil {
-				vmCtx.VM.Spec.Reserved = &vmopv1.VirtualMachineReservedSpec{}
-			}
-			vmCtx.VM.Spec.Reserved.ResourcePolicyName = "bogus"
-
-			vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.Client, ctx.VCClient.Client, ctx.Datacenter, ctx.Finder)
-			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(HavePrefix("failed to get VirtualMachineSetResourcePolicy"))
-			Expect(vm).To(BeNil())
-		})
 	})
 }

pkg/providers/vsphere/vmprovider.go

@@ -242,7 +242,7 @@ func (vs *vSphereVMProvider) getVM(
 	client *vcclient.Client,
 	notFoundReturnErr bool) (*object.VirtualMachine, error) {
 
-	vcVM, err := vcenter.GetVirtualMachine(vmCtx, vs.k8sClient, client.VimClient(), client.Datacenter(), client.Finder())
+	vcVM, err := vcenter.GetVirtualMachine(vmCtx, client.VimClient(), client.Datacenter(), client.Finder())
 	if err != nil {
 		return nil, err
 	}