Repository: vmware/govmomi
Pull request #1744 (2019-12-15 08:25:08)

vcsim: avoid possible panic in VirtualMachine.Destroy_Task
It is possible the VM's parent folder is destroyed in the middle of a VM Destroy call,
return ManagedObjectNotFound in that case.


Patch:
diff --git a/simulator/registry.go b/simulator/registry.go
index 91be876d..c73ba9e3 100644
--- a/simulator/registry.go
+++ b/simulator/registry.go
@@ -287,11 +287,14 @@ func (r *Registry) Update(obj mo.Reference, changes []types.PropertyChange) {
 // If no object of type kind is found, the method will panic when it reaches the
 // inventory root Folder where the Parent field is nil.
 func (r *Registry) getEntityParent(item mo.Entity, kind string) mo.Entity {
+	var ok bool
 	for {
 		parent := item.Entity().Parent
 
-		item = r.Get(*parent).(mo.Entity)
-
+		item, ok = r.Get(*parent).(mo.Entity)
+		if !ok {
+			return nil
+		}
 		if item.Reference().Type == kind {
 			return item
 		}
@@ -300,7 +303,11 @@ func (r *Registry) getEntityParent(item mo.Entity, kind string) mo.Entity {
 
 // getEntityDatacenter returns the Datacenter containing the given item
 func (r *Registry) getEntityDatacenter(item mo.Entity) *Datacenter {
-	return r.getEntityParent(item, "Datacenter").(*Datacenter)
+	dc, ok := r.getEntityParent(item, "Datacenter").(*Datacenter)
+	if ok {
+		return dc
+	}
+	return nil
 }
 
 func (r *Registry) getEntityFolder(item mo.Entity, kind string) *Folder {
diff --git a/simulator/virtual_machine.go b/simulator/virtual_machine.go
index aa739525..cfc19a64 100644
--- a/simulator/virtual_machine.go
+++ b/simulator/virtual_machine.go
@@ -1060,9 +1060,11 @@ func (vm *VirtualMachine) removeDevice(devices object.VirtualDeviceList, spec *t
 				}
 
 				if file != "" {
-					dc := Map.getEntityDatacenter(Map.Get(*vm.Parent).(mo.Entity))
+					dc := Map.getEntityDatacenter(vm)
 					dm := Map.VirtualDiskManager()
-
+					if dc == nil {
+						continue // parent was destroyed
+					}
 					dm.DeleteVirtualDiskTask(&types.DeleteVirtualDisk_Task{
 						Name:       file,
 						Datacenter: &dc.Self,
@@ -1383,7 +1385,13 @@ func (vm *VirtualMachine) UpgradeVMTask(req *types.UpgradeVM_Task) soap.HasFault
 }
 
 func (vm *VirtualMachine) DestroyTask(ctx *Context, req *types.Destroy_Task) soap.HasFault {
+	dc := ctx.Map.getEntityDatacenter(vm)
+
 	task := CreateTask(vm, "destroy", func(t *Task) (types.AnyType, types.BaseMethodFault) {
+		if dc == nil {
+			return nil, &types.ManagedObjectNotFound{Obj: vm.Self} // If our Parent was destroyed, so were we.
+		}
+
 		r := vm.UnregisterVM(ctx, &types.UnregisterVM{
 			This: req.This,
 		})
@@ -1399,12 +1407,11 @@ func (vm *VirtualMachine) DestroyTask(ctx *Context, req *types.Destroy_Task) soa
 
 		// Delete VM files from the datastore (ignoring result for now)
 		m := Map.FileManager()
-		dc := Map.getEntityDatacenter(vm).Reference()
 
 		_ = m.DeleteDatastoreFileTask(&types.DeleteDatastoreFile_Task{
 			This:       m.Reference(),
 			Name:       vm.Config.Files.LogDirectory,
-			Datacenter: &dc,
+			Datacenter: &dc.Self,
 		})
 
 		vm.run.remove(vm)


Test patch:
diff --git a/simulator/race_test.go b/simulator/race_test.go
index 7167a8a6..be8d5685 100644
--- a/simulator/race_test.go
+++ b/simulator/race_test.go
@@ -27,6 +27,7 @@ import (
 	"github.com/vmware/govmomi/event"
 	"github.com/vmware/govmomi/find"
 	"github.com/vmware/govmomi/property"
+	"github.com/vmware/govmomi/task"
 	"github.com/vmware/govmomi/view"
 	"github.com/vmware/govmomi/vim25/types"
 )
@@ -208,3 +209,83 @@ func TestRace(t *testing.T) {
 		t.Error("no tasks collected")
 	}
 }
+
+func isManagedObjectNotFound(err error) bool {
+	_, ok := err.(task.Error).Fault().(*types.ManagedObjectNotFound)
+	return ok
+}
+
+// Race VirtualMachine.Destroy vs Folder.Destroy; the latter removes the VM parent and the former should not panic in that case
+func TestRaceDestroy(t *testing.T) {
+	ctx := context.Background()
+
+	m := VPX()
+	m.Folder = 1
+	m.Autostart = false
+	defer m.Remove()
+
+	err := m.Create()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	s := m.Service.NewServer()
+	defer s.Close()
+
+	c, err := govmomi.NewClient(ctx, s.URL, true)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	finder := find.NewFinder(c.Client)
+	vms, err := finder.VirtualMachineList(ctx, "*")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	folder, err := finder.Folder(ctx, "vm/F0")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	notFound := false
+	var wg sync.WaitGroup
+
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		for _, vm := range vms {
+			task, err := vm.Destroy(ctx)
+			if err != nil {
+				t.Error(err)
+			}
+			err = task.Wait(ctx)
+			if err != nil {
+				if isManagedObjectNotFound(err) {
+					notFound = true
+				} else {
+					t.Error(err)
+				}
+			}
+		}
+	}()
+
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		task, err := folder.Destroy(ctx)
+		if err != nil {
+			t.Error(err)
+		}
+		err = task.Wait(ctx)
+		if err != nil {
+			t.Error(err)
+		}
+	}()
+
+	wg.Wait()
+
+	if !notFound {
+		t.Error("expected ManagedObjectNotFound")
+	}
+}