Merge pull request #866 from bryanv/bryanv/getvm-invalid-vc-session

🐛 Handle invalid VC Session in GetVirtualMachine()
vmware-tanzu · Jan 24, 2025 · 2b75142 · 2b75142
2 parents 3c5dffc + 526031f
commit 2b75142
Show file tree

Hide file tree

Showing 3 changed files with 103 additions and 18 deletions.
diff --git a/pkg/providers/vsphere/vcenter/getvm.go b/pkg/providers/vsphere/vcenter/getvm.go
@@ -5,41 +5,55 @@
 package vcenter
 
 import (
+	"errors"
 	"fmt"
 
-	"github.com/vmware/govmomi/find"
+	"github.com/vmware/govmomi/fault"
 	"github.com/vmware/govmomi/object"
+	"github.com/vmware/govmomi/property"
 	"github.com/vmware/govmomi/vim25"
+	"github.com/vmware/govmomi/vim25/mo"
 	vimtypes "github.com/vmware/govmomi/vim25/types"
 
 	pkgctx "github.com/vmware-tanzu/vm-operator/pkg/context"
 )
 
+type getVMNotFoundError struct{}
+
+func (n getVMNotFoundError) Error() string {
+	return "vm not found"
+}
+
 // GetVirtualMachine gets the VM from VC, either by the Instance UUID, BIOS UUID, or MoID.
 func GetVirtualMachine(
 	vmCtx pkgctx.VirtualMachineContext,
 	vimClient *vim25.Client,
-	datacenter *object.Datacenter,
-	finder *find.Finder) (*object.VirtualMachine, error) {
+	datacenter *object.Datacenter) (*object.VirtualMachine, error) {
 
 	// 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
+		} else if !errors.Is(err, getVMNotFoundError{}) {
+			return nil, err
 		}
 	}
 
 	// Find by BIOS UUID.
 	if id := vmCtx.VM.Spec.BiosUUID; id != "" {
 		if vm, err := findVMByUUID(vmCtx, vimClient, datacenter, id, false); err == nil {
 			return vm, nil
+		} else if !errors.Is(err, getVMNotFoundError{}) {
+			return nil, err
 		}
 	}
 
 	// Find by MoRef.
 	if id := vmCtx.VM.Status.UniqueID; id != "" {
-		if vm, err := findVMByMoID(vmCtx, finder, id); err == nil {
+		if vm, err := findVMByMoID(vmCtx, vimClient, id); err == nil {
 			return vm, nil
+		} else if !errors.Is(err, getVMNotFoundError{}) {
+			return nil, err
 		}
 	}
 
@@ -48,21 +62,25 @@ func GetVirtualMachine(
 
 func findVMByMoID(
 	vmCtx pkgctx.VirtualMachineContext,
-	finder *find.Finder,
+	vimClient *vim25.Client,
 	moID string) (*object.VirtualMachine, error) {
 
-	ref, err := finder.ObjectReference(vmCtx, vimtypes.ManagedObjectReference{Type: "VirtualMachine", Value: moID})
-	if err != nil {
-		return nil, err
+	moRef := vimtypes.ManagedObjectReference{
+		Type:  "VirtualMachine",
+		Value: moID,
 	}
 
-	vm, ok := ref.(*object.VirtualMachine)
-	if !ok {
-		return nil, fmt.Errorf("found VM reference was not a VM but a %T", ref)
+	vm := mo.VirtualMachine{}
+	if err := property.DefaultCollector(vimClient).RetrieveOne(vmCtx, moRef, []string{"name"}, &vm); err != nil {
+		var f *vimtypes.ManagedObjectNotFound
+		if _, ok := fault.As(err, &f); ok {
+			return nil, getVMNotFoundError{}
+		}
+		return nil, fmt.Errorf("error retreiving VM via MoID: %w", err)
 	}
 
-	vmCtx.Logger.V(4).Info("Found VM via MoID", "path", vm.InventoryPath, "moID", moID)
-	return vm, nil
+	vmCtx.Logger.V(4).Info("Found VM via MoID", "moID", moID)
+	return object.NewVirtualMachine(vimClient, moRef), nil
 }
 
 func findVMByUUID(
@@ -74,9 +92,9 @@ func findVMByUUID(
 
 	ref, err := object.NewSearchIndex(vimClient).FindByUuid(vmCtx, datacenter, uuid, true, &isInstanceUUID)
 	if err != nil {
-		return nil, fmt.Errorf("error finding object by UUID %q: %w", uuid, err)
+		return nil, fmt.Errorf("error finding VM by UUID %q: %w", uuid, err)
 	} else if ref == nil {
-		return nil, fmt.Errorf("no VM found for UUID %q (instanceUUID: %v)", uuid, isInstanceUUID)
+		return nil, getVMNotFoundError{}
 	}
 
 	vm, ok := ref.(*object.VirtualMachine)

diff --git a/pkg/providers/vsphere/vcenter/getvm_test.go b/pkg/providers/vsphere/vcenter/getvm_test.go
@@ -56,11 +56,23 @@ func getVM() {
 		})
 
 		It("returns success", func() {
-			vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter, ctx.Finder)
+			vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(vm).ToNot(BeNil())
 			Expect(vm.Reference().Value).To(Equal(vmCtx.VM.Status.UniqueID))
 		})
+
+		Context("VC client is logged out", func() {
+			BeforeEach(func() {
+				Expect(ctx.VCClient.Logout(ctx)).To(Succeed())
+			})
+
+			It("returns error", func() {
+				vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter)
+				Expect(err).To(HaveOccurred())
+				Expect(vm).To(BeNil())
+			})
+		})
 	})
 
 	Context("Gets VM by UUID", func() {
@@ -74,9 +86,64 @@ func getVM() {
 		})
 
 		It("returns success", func() {
-			vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter, ctx.Finder)
+			vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(vm).ToNot(BeNil())
+		})
+
+		Context("VC client is logged out", func() {
+			BeforeEach(func() {
+				Expect(ctx.VCClient.Logout(ctx)).To(Succeed())
+			})
+
+			It("returns error", func() {
+				vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter)
+				Expect(err).To(HaveOccurred())
+				Expect(vm).To(BeNil())
+			})
+		})
+	})
+
+	Context("Gets VM by BiosUUID", func() {
+		BeforeEach(func() {
+			vm, err := ctx.Finder.VirtualMachine(ctx, vcVMName)
+			Expect(err).ToNot(HaveOccurred())
+
+			var o mo.VirtualMachine
+			Expect(vm.Properties(ctx, vm.Reference(), nil, &o)).To(Succeed())
+			vmCtx.VM.Spec.BiosUUID = o.Config.Uuid
+		})
+
+		It("returns success", func() {
+			vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(vm).ToNot(BeNil())
 		})
+
+		Context("VC client is logged out", func() {
+			BeforeEach(func() {
+				Expect(ctx.VCClient.Logout(ctx)).To(Succeed())
+			})
+
+			It("returns error", func() {
+				vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter)
+				Expect(err).To(HaveOccurred())
+				Expect(vm).To(BeNil())
+			})
+		})
+	})
+
+	Context("VM does not exist", func() {
+		BeforeEach(func() {
+			vmCtx.VM.UID = "bogus-uid"
+			vmCtx.VM.Spec.BiosUUID = "bogus-bios-uuid"
+			vmCtx.VM.Status.UniqueID = "bogus-moid"
+		})
+
+		It("returns success with nil vm", func() {
+			vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(vm).To(BeNil())
+		})
 	})
 }
diff --git a/pkg/providers/vsphere/vmprovider.go b/pkg/providers/vsphere/vmprovider.go
@@ -356,7 +356,7 @@ func (vs *vSphereVMProvider) getVM(
 	client *vcclient.Client,
 	notFoundReturnErr bool) (*object.VirtualMachine, error) {
 
-	vcVM, err := vcenter.GetVirtualMachine(vmCtx, client.VimClient(), client.Datacenter(), client.Finder())
+	vcVM, err := vcenter.GetVirtualMachine(vmCtx, client.VimClient(), client.Datacenter())
 	if err != nil {
 		return nil, err
 	}