vcsim: fix RelocateVM_Task related races

Closes #3565
vmware · Sep 30, 2024 · 6cea86e · 6cea86e
1 parent 6c79c2b
commit 6cea86e
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 8 deletions.
diff --git a/simulator/folder.go b/simulator/folder.go
@@ -89,13 +89,13 @@ func networkSummary(n *mo.Network) types.BaseNetworkSummary {
 
 func folderPutChild(ctx *Context, f *mo.Folder, o mo.Entity) {
 	ctx.WithLock(f, func() {
-		// Need to update ChildEntity before Map.Put for ContainerView updates to work properly
-		f.ChildEntity = append(f.ChildEntity, ctx.Map.reference(o))
-		ctx.Map.PutEntity(f, o)
+		ctx.WithLock(o, func() {
+			// Need to update ChildEntity before Map.Put for ContainerView updates to work properly
+			f.ChildEntity = append(f.ChildEntity, ctx.Map.reference(o))
+			ctx.Map.PutEntity(f, o)
 
-		folderUpdate(ctx, f, o, ctx.Map.AddReference)
+			folderUpdate(ctx, f, o, ctx.Map.AddReference)
 
-		ctx.WithLock(o, func() {
 			switch e := o.(type) {
 			case *mo.Network:
 				e.Summary = networkSummary(e)
@@ -110,7 +110,10 @@ func folderPutChild(ctx *Context, f *mo.Folder, o mo.Entity) {
 
 func folderRemoveChild(ctx *Context, f *mo.Folder, o mo.Reference) {
 	ctx.Map.Remove(ctx, o.Reference())
+	folderRemoveReference(ctx, f, o)
+}
 
+func folderRemoveReference(ctx *Context, f *mo.Folder, o mo.Reference) {
 	ctx.WithLock(f, func() {
 		RemoveReference(&f.ChildEntity, o.Reference())
 
@@ -596,7 +599,7 @@ func (f *Folder) MoveIntoFolderTask(ctx *Context, c *types.MoveIntoFolder_Task)
 				return nil, &types.NotSupported{}
 			}
 
-			folderRemoveChild(ctx, &parent.Folder, ref)
+			folderRemoveReference(ctx, &parent.Folder, ref)
 			folderPutChild(ctx, &f.Folder, obj)
 		}
 

diff --git a/simulator/race_test.go b/simulator/race_test.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -29,6 +30,7 @@ import (
 	"github.com/vmware/govmomi/find"
 	"github.com/vmware/govmomi/property"
 	"github.com/vmware/govmomi/view"
+	"github.com/vmware/govmomi/vim25"
 	"github.com/vmware/govmomi/vim25/types"
 )
 
@@ -328,3 +330,60 @@ func TestRaceDestroy(t *testing.T) {
 		t.Error("expected ManagedObjectNotFound")
 	}
 }
+
+func TestRaceVmRelocate(t *testing.T) {
+	Test(func(ctx context.Context, c *vim25.Client) {
+		finder := find.NewFinder(c)
+
+		dc, err := finder.DefaultDatacenter(ctx)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		finder.SetDatacenter(dc)
+
+		vm, err := finder.VirtualMachine(ctx, "DC0_H0_VM0")
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		folders, err := dc.Folders(ctx)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		vmFolder := folders.VmFolder
+
+		var failed atomic.Int32
+		var wg sync.WaitGroup
+
+		for i := 0; i < 10; i++ {
+			spec := types.VirtualMachineRelocateSpec{
+				Folder: types.NewReference(vmFolder.Reference()),
+			}
+
+			wg.Add(1)
+			go func() {
+				defer wg.Done()
+				task, err := vm.Relocate(ctx, spec, types.VirtualMachineMovePriorityDefaultPriority)
+				if err != nil {
+					panic(err)
+				}
+				if err = task.Wait(ctx); err != nil {
+					failed.Add(1)
+				}
+			}()
+
+			vmFolder, err = vmFolder.CreateFolder(ctx, "child")
+			if err != nil {
+				t.Fatal("Failed to create folder", err)
+			}
+		}
+
+		wg.Wait()
+
+		if n := failed.Load(); n != 0 {
+			t.Errorf("%d relocate calls failed", n)
+		}
+	})
+}
diff --git a/simulator/virtual_machine.go b/simulator/virtual_machine.go
@@ -2509,8 +2509,12 @@ func (vm *VirtualMachine) RelocateVMTask(ctx *Context, req *types.RelocateVM_Tas
 
 		if ref := req.Spec.Folder; ref != nil {
 			folder := ctx.Map.Get(*ref).(*Folder)
-			folder.MoveIntoFolderTask(ctx, &types.MoveIntoFolder_Task{
-				List: []types.ManagedObjectReference{vm.Self},
+			ctx.WithLock(folder, func() {
+				res := folder.MoveIntoFolderTask(ctx, &types.MoveIntoFolder_Task{
+					List: []types.ManagedObjectReference{vm.Self},
+				}).(*methods.MoveIntoFolder_TaskBody).Res
+				// Wait for task to complete while we hold the Folder lock
+				ctx.Map.Get(res.Returnval).(*Task).Wait()
 			})
 		}