Repository: vmware/govmomi
Pull request #3546 (2024-09-06 22:19:56)

vcsim: Support PlaceVm with relocate placement type
## Description

This patch introduces basic support for `ClusterComputeResource.PlaceVm` with the `relocate` placement type in vcsim. Since relocate is a supported placement type in real vSphere environments, it is essential to ensure that our simulator environment reflects this capability for accurate and comprehensive testing.

Closes: #3540 

## Type of change

Please mark options that are relevant:

- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to
  not work as expected)
- [ ] This change requires a documentation update
- [ ] Build related change

## How Has This Been Tested?

All of the added unit tests simulate behavior (i.e., expected Faults/Recommendations) when testing the same `PlacementSpec` against a real vSphere environment.
```
% go test -v -count 1 -run 'TestPlaceVm' -coverprofile cover.out ./simulator

=== RUN   TestPlaceVmReconfigure
2024/09/06 14:56:08 unsupported placement type: unsupported
--- PASS: TestPlaceVmReconfigure (1.76s)
=== RUN   TestPlaceVmRelocate
--- PASS: TestPlaceVmRelocate (0.39s)
=== RUN   TestPlaceVmsXCluster
--- PASS: TestPlaceVmsXCluster (0.87s)
PASS
coverage: 27.4% of statements
ok      github.com/vmware/govmomi/simulator     3.455s  coverage: 27.4% of statements
```

```
go tool cover -html=cover.out -o=coverage.html 
```
100% code coverage on new code:
![Screenshot 2024-09-06 at 2 59 18 PM](https://github.com/user-attachments/assets/244ca43a-fb8f-42c9-8649-c18718adddec)
![Screenshot 2024-09-06 at 2 59 30 PM](https://github.com/user-attachments/assets/0adc9291-0bdb-42e1-9016-0358b6f2c53e)
![Screenshot 2024-09-06 at 2 59 41 PM](https://github.com/user-attachments/assets/3da01fbc-d500-43e9-a302-0e4e5f09bab4)
![Screenshot 2024-09-06 at 2 59 56 PM](https://github.com/user-attachments/assets/793fa942-ad30-478e-97a2-3633764fbf4b)

## Checklist:

- [x] My code follows the `CONTRIBUTION` [guidelines] of this project
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have made corresponding changes to the documentation
- [x] I have added tests that prove my fix is effective or that my feature works
- [x] New and existing unit tests pass locally with my changes
- [x] Any dependent changes have been merged

[guidelines]: https://github.com/vmware/govmomi/blob/main/CONTRIBUTING.md


yanleizhao-vmware: > Thanks @yanleizhao-vmware , Can you please [squash](https://github.com/vmware/govmomi/blob/main/CONTRIBUTING.md#updating-pull-requests) the review fixup commits first?

Of course, I wasn't sure if using squash and fixup is recommended, or tolerated 😅 

Patch:
diff --git a/simulator/cluster_compute_resource.go b/simulator/cluster_compute_resource.go
index bdac24a0..01c39362 100644
--- a/simulator/cluster_compute_resource.go
+++ b/simulator/cluster_compute_resource.go
@@ -24,6 +24,7 @@ import (
 
 	"github.com/google/uuid"
 
+	"github.com/vmware/govmomi/object"
 	"github.com/vmware/govmomi/simulator/esx"
 	"github.com/vmware/govmomi/vim25/methods"
 	"github.com/vmware/govmomi/vim25/mo"
@@ -455,10 +456,9 @@ func (c *ClusterComputeResource) PlaceVm(ctx *Context, req *types.PlaceVm) soap.
 	case types.PlacementSpecPlacementTypeReconfigure:
 		// Validate input.
 		if req.PlacementSpec.ConfigSpec == nil {
-			invalidArg := &types.InvalidArgument{
+			body.Fault_ = Fault("", &types.InvalidArgument{
 				InvalidProperty: "PlacementSpec.configSpec",
-			}
-			body.Fault_ = Fault("", invalidArg)
+			})
 			return body
 		}
 
@@ -475,6 +475,24 @@ func (c *ClusterComputeResource) PlaceVm(ctx *Context, req *types.PlaceVm) soap.
 			TargetHost:   spec.Host,
 			RelocateSpec: spec,
 		})
+	case types.PlacementSpecPlacementTypeRelocate:
+		// Validate fields of req.PlacementSpec, if explicitly provided.
+		if !validatePlacementSpecForPlaceVmRelocate(ctx, req, body) {
+			return body
+		}
+
+		// After validating req.PlacementSpec, we must have a valid req.PlacementSpec.Vm.
+		vmObj := ctx.Map.Get(*req.PlacementSpec.Vm).(*VirtualMachine)
+
+		// Populate RelocateSpec's common fields, if not explicitly provided.
+		populateRelocateSpecForPlaceVmRelocate(&req.PlacementSpec.RelocateSpec, vmObj)
+
+		// Update PlacementResult.
+		res.Action = append(res.Action, &types.PlacementAction{
+			Vm:           req.PlacementSpec.Vm,
+			TargetHost:   req.PlacementSpec.RelocateSpec.Host,
+			RelocateSpec: req.PlacementSpec.RelocateSpec,
+		})
 	default:
 		log.Printf("unsupported placement type: %s", req.PlacementSpec.PlacementType)
 		body.Fault_ = Fault("", new(types.NotSupported))
@@ -490,6 +508,134 @@ func (c *ClusterComputeResource) PlaceVm(ctx *Context, req *types.PlaceVm) soap.
 	return body
 }
 
+// validatePlacementSpecForPlaceVmRelocate validates the fields of req.PlacementSpec for a relocate placement type.
+// Returns true if the fields are valid, false otherwise.
+func validatePlacementSpecForPlaceVmRelocate(ctx *Context, req *types.PlaceVm, body *methods.PlaceVmBody) bool {
+	if req.PlacementSpec.Vm == nil {
+		body.Fault_ = Fault("", &types.InvalidArgument{
+			InvalidProperty: "PlacementSpec",
+		})
+		return false
+	}
+
+	// Oddly when the VM is not found, PlaceVm complains about configSpec being invalid, despite this being
+	// a relocate placement type. Possibly due to treating the missing VM as a create placement type
+	// internally, which requires the configSpec to be present.
+	vmObj, exist := ctx.Map.Get(*req.PlacementSpec.Vm).(*VirtualMachine)
+	if !exist {
+		body.Fault_ = Fault("", &types.InvalidArgument{
+			InvalidProperty: "PlacementSpec.configSpec",
+		})
+		return false
+	}
+
+	return validateRelocateSpecForPlaceVmRelocate(ctx, req.PlacementSpec.RelocateSpec, body, vmObj)
+}
+
+// validateRelocateSpecForPlaceVmRelocate validates the fields of req.PlacementSpec.RelocateSpec for a relocate
+// placement type. Returns true if the fields are valid, false otherwise.
+func validateRelocateSpecForPlaceVmRelocate(ctx *Context, spec *types.VirtualMachineRelocateSpec, body *methods.PlaceVmBody, vmObj *VirtualMachine) bool {
+	if spec == nil {
+		// An empty relocate spec is valid, as it will be populated with default values.
+		return true
+	}
+
+	if spec.Host != nil {
+		if _, exist := ctx.Map.Get(*spec.Host).(*HostSystem); !exist {
+			body.Fault_ = Fault("", &types.ManagedObjectNotFound{
+				Obj: *spec.Host,
+			})
+			return false
+		}
+	}
+
+	if spec.Datastore != nil {
+		if _, exist := ctx.Map.Get(*spec.Datastore).(*Datastore); !exist {
+			body.Fault_ = Fault("", &types.ManagedObjectNotFound{
+				Obj: *spec.Datastore,
+			})
+			return false
+		}
+	}
+
+	if spec.Pool != nil {
+		if _, exist := ctx.Map.Get(*spec.Pool).(*ResourcePool); !exist {
+			body.Fault_ = Fault("", &types.ManagedObjectNotFound{
+				Obj: *spec.Pool,
+			})
+			return false
+		}
+	}
+
+	if spec.Disk != nil {
+		deviceList := object.VirtualDeviceList(vmObj.Config.Hardware.Device)
+		vdiskList := deviceList.SelectByType(&types.VirtualDisk{})
+		for _, disk := range spec.Disk {
+			var diskFound bool
+			for _, vdisk := range vdiskList {
+				if disk.DiskId == vdisk.GetVirtualDevice().Key {
+					diskFound = true
+					break
+				}
+			}
+			if !diskFound {
+				body.Fault_ = Fault("", &types.InvalidArgument{
+					InvalidProperty: "PlacementSpec.vm",
+				})
+				return false
+			}
+
+			// Unlike a non-existing spec.Datastore that throws ManagedObjectNotFound, a non-existing disk.Datastore
+			// throws InvalidArgument.
+			if _, exist := ctx.Map.Get(disk.Datastore).(*Datastore); !exist {
+				body.Fault_ = Fault("", &types.InvalidArgument{
+					InvalidProperty: "RelocateSpec",
+				})
+				return false
+			}
+		}
+	}
+
+	return true
+}
+
+// populateRelocateSpecForPlaceVmRelocate populates the fields of req.PlacementSpec.RelocateSpec for a relocate
+// placement type, if not explicitly provided.
+func populateRelocateSpecForPlaceVmRelocate(specPtr **types.VirtualMachineRelocateSpec, vmObj *VirtualMachine) {
+	if *specPtr == nil {
+		*specPtr = &types.VirtualMachineRelocateSpec{}
+	}
+
+	spec := *specPtr
+
+	if spec.DiskMoveType == "" {
+		spec.DiskMoveType = string(types.VirtualMachineRelocateDiskMoveOptionsMoveAllDiskBackingsAndDisallowSharing)
+	}
+
+	if spec.Datastore == nil {
+		spec.Datastore = &vmObj.Datastore[0]
+	}
+
+	if spec.Host == nil {
+		spec.Host = vmObj.Runtime.Host
+	}
+
+	if spec.Pool == nil {
+		spec.Pool = vmObj.ResourcePool
+	}
+
+	if spec.Disk == nil {
+		deviceList := object.VirtualDeviceList(vmObj.Config.Hardware.Device)
+		for _, vdisk := range deviceList.SelectByType(&types.VirtualDisk{}) {
+			spec.Disk = append(spec.Disk, types.VirtualMachineRelocateSpecDiskLocator{
+				DiskId:       vdisk.GetVirtualDevice().Key,
+				Datastore:    *spec.Datastore,
+				DiskMoveType: spec.DiskMoveType,
+			})
+		}
+	}
+}
+
 func CreateClusterComputeResource(ctx *Context, f *Folder, name string, spec types.ClusterConfigSpecEx) (*ClusterComputeResource, types.BaseMethodFault) {
 	if e := ctx.Map.FindByName(name, f.ChildEntity); e != nil {
 		return nil, &types.DuplicateName{


Test patch:
diff --git a/simulator/cluster_compute_resource_test.go b/simulator/cluster_compute_resource_test.go
index 7f1aaba3..9ff0ad11 100644
--- a/simulator/cluster_compute_resource_test.go
+++ b/simulator/cluster_compute_resource_test.go
@@ -156,7 +156,7 @@ func TestPlaceVmReconfigure(t *testing.T) {
 		{
 			"unsupported placement type",
 			nil,
-			types.PlacementSpecPlacementTypeRelocate,
+			types.PlacementSpecPlacementType("unsupported"),
 			"NotSupported",
 		},
 		{
@@ -206,3 +206,163 @@ func TestPlaceVmReconfigure(t *testing.T) {
 		})
 	}
 }
+
+func TestPlaceVmRelocate(t *testing.T) {
+	Test(func(ctx context.Context, c *vim25.Client) {
+		// Test env setup.
+		finder := find.NewFinder(c, true)
+		datacenter, err := finder.DefaultDatacenter(ctx)
+		if err != nil {
+			t.Fatalf("failed to get default datacenter: %v", err)
+		}
+		finder.SetDatacenter(datacenter)
+
+		vmMoRef := Map.Any("VirtualMachine").(*VirtualMachine).Reference()
+		hostMoRef := Map.Any("HostSystem").(*HostSystem).Reference()
+		dsMoRef := Map.Any("Datastore").(*Datastore).Reference()
+
+		tests := []struct {
+			name         string
+			relocateSpec *types.VirtualMachineRelocateSpec
+			vmMoRef      *types.ManagedObjectReference
+			expectedErr  string
+		}{
+			{
+				"relocate without a spec",
+				nil,
+				&vmMoRef,
+				"",
+			},
+			{
+				"relocate with an empty spec",
+				&types.VirtualMachineRelocateSpec{},
+				&vmMoRef,
+				"",
+			},
+			{
+				"relocate without a vm in spec",
+				&types.VirtualMachineRelocateSpec{
+					Host: &hostMoRef,
+				},
+				nil,
+				"InvalidArgument",
+			},
+			{
+				"relocate with a non-existing vm in spec",
+				&types.VirtualMachineRelocateSpec{
+					Host: &hostMoRef,
+				},
+				&types.ManagedObjectReference{
+					Type:  "VirtualMachine",
+					Value: "fake-vm-999",
+				},
+				"InvalidArgument",
+			},
+			{
+				"relocate with a diskId in spec.dick that does not exist in the vm",
+				&types.VirtualMachineRelocateSpec{
+					Host: &hostMoRef,
+					Disk: []types.VirtualMachineRelocateSpecDiskLocator{
+						{
+							DiskId:    1,
+							Datastore: dsMoRef,
+						},
+					},
+				},
+				&vmMoRef,
+				"InvalidArgument",
+			},
+			{
+				"relocate with a non-existing datastore in spec.disk",
+				&types.VirtualMachineRelocateSpec{
+					Host: &hostMoRef,
+					Disk: []types.VirtualMachineRelocateSpecDiskLocator{
+						{
+							DiskId: 204, // The default diskId in simulator.
+							Datastore: types.ManagedObjectReference{
+								Type:  "Datastore",
+								Value: "fake-datastore-999",
+							},
+						},
+					},
+				},
+				&vmMoRef,
+				"InvalidArgument",
+			},
+			{
+				"relocate with a valid spec.disk",
+				&types.VirtualMachineRelocateSpec{
+					Host: &hostMoRef,
+					Disk: []types.VirtualMachineRelocateSpecDiskLocator{
+						{
+							DiskId:    204, // The default diskId in simulator.
+							Datastore: dsMoRef,
+						},
+					},
+				},
+				&vmMoRef,
+				"",
+			},
+			{
+				"relocate with a valid host in spec",
+				&types.VirtualMachineRelocateSpec{
+					Host: &hostMoRef,
+				},
+				&vmMoRef,
+				"",
+			},
+			{
+				"relocate with a non-existing host in spec",
+				&types.VirtualMachineRelocateSpec{
+					Host: &types.ManagedObjectReference{
+						Type:  "HostSystem",
+						Value: "fake-host-999",
+					},
+				},
+				&vmMoRef,
+				"ManagedObjectNotFound",
+			},
+			{
+				"relocate with a non-existing datastore in spec",
+				&types.VirtualMachineRelocateSpec{
+					Datastore: &types.ManagedObjectReference{
+						Type:  "Datastore",
+						Value: "fake-datastore-999",
+					},
+				},
+				&vmMoRef,
+				"ManagedObjectNotFound",
+			},
+			{
+				"relocate with a non-existing resource pool in spec",
+				&types.VirtualMachineRelocateSpec{
+					Pool: &types.ManagedObjectReference{
+						Type:  "ResourcePool",
+						Value: "fake-resource-pool-999",
+					},
+				},
+				&vmMoRef,
+				"ManagedObjectNotFound",
+			},
+		}
+
+		for _, test := range tests {
+			test := test // assign to local var since loop var is reused
+			// PlaceVm.
+			placementSpec := types.PlacementSpec{
+				Vm:            test.vmMoRef,
+				RelocateSpec:  test.relocateSpec,
+				PlacementType: string(types.PlacementSpecPlacementTypeRelocate),
+			}
+
+			clusterMoRef := Map.Any("ClusterComputeResource").(*ClusterComputeResource).Reference()
+			clusterObj := object.NewClusterComputeResource(c, clusterMoRef)
+			_, err = clusterObj.PlaceVm(ctx, placementSpec)
+			if err == nil && test.expectedErr != "" {
+				t.Fatalf("expected error %q, got nil", test.expectedErr)
+			} else if err != nil && !strings.Contains(err.Error(), test.expectedErr) {
+				t.Fatalf("expected error %q, got %v", test.expectedErr, err)
+			}
+		}
+	})
+}