From 65fe8359b10bd34fa365f95f66e79ca2bf153268 Mon Sep 17 00:00:00 2001 From: Bryan Venteicher Date: Mon, 9 Dec 2024 11:55:43 -0600 Subject: [PATCH] Remove Inventory path fallback for GetVM Originally, we'd try to find a VM via its MoID, with a fallback to the Inventory by path. Since then, we've added the Instance and Bios UUIDs as preferred ways to find a VM. VMs are now created with a known Instance ID - that of the k8s VM UID - that we can use for lookup. This removes lookup depending on k8s objects to determine if the VM exists that have a separate lifecycle than the VM. --- pkg/providers/vsphere/vcenter/getvm.go | 80 +----------------- pkg/providers/vsphere/vcenter/getvm_test.go | 89 +-------------------- pkg/providers/vsphere/vmprovider.go | 2 +- 3 files changed, 6 insertions(+), 165 deletions(-) diff --git a/pkg/providers/vsphere/vcenter/getvm.go b/pkg/providers/vsphere/vcenter/getvm.go index 59bb54691..2704ef700 100644 --- a/pkg/providers/vsphere/vcenter/getvm.go +++ b/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 -} diff --git a/pkg/providers/vsphere/vcenter/getvm_test.go b/pkg/providers/vsphere/vcenter/getvm_test.go index fe1f75655..051ce330c 100644 --- a/pkg/providers/vsphere/vcenter/getvm_test.go +++ b/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()) - }) }) } diff --git a/pkg/providers/vsphere/vmprovider.go b/pkg/providers/vsphere/vmprovider.go index edf665c81..38669ebed 100644 --- a/pkg/providers/vsphere/vmprovider.go +++ b/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 }