Repository: vmware/govmomi
Pull request #3408 (2024-04-11 03:25:11)

api: change RetrieveProperties to collect results in batches
'RetrieveProperties' is deprecated since 4.1
Use RetrievePropertiesEx + ContinueRetrievePropertiesEx to collect in batches.


akutz: I opened https://github.com/dougm/govmomi/pull/2 against this PR to pull in some tests. I will also be honest, one thing has me worried, the documentation around `RetrievePropertiesEx`'s `MaxObjects` field:

> The maximum number of [ObjectContent](https://vdc-download.vmware.com/vmwb-repository/dcr-public/184bb3ba-6fa8-4574-a767-d0c96e2a38f4/ba9422ef-405c-47dd-8553-e11b619185b2/SDK/vsphere-ws/docs/ReferenceGuide/vmodl.query.PropertyCollector.ObjectContent.html) data objects that should be returned in a single result from [RetrievePropertiesEx](https://vdc-download.vmware.com/vmwb-repository/dcr-public/184bb3ba-6fa8-4574-a767-d0c96e2a38f4/ba9422ef-405c-47dd-8553-e11b619185b2/SDK/vsphere-ws/docs/ReferenceGuide/vmodl.query.PropertyCollector.html#retrievePropertiesEx).
>
> An unset value indicates that there is no maximum. In this case [PropertyCollector](https://vdc-download.vmware.com/vmwb-repository/dcr-public/184bb3ba-6fa8-4574-a767-d0c96e2a38f4/ba9422ef-405c-47dd-8553-e11b619185b2/SDK/vsphere-ws/docs/ReferenceGuide/vmodl.query.PropertyCollector.html) policy may still limit the number of objects. Any remaining objects may be retrieved with [ContinueRetrievePropertiesEx](https://vdc-download.vmware.com/vmwb-repository/dcr-public/184bb3ba-6fa8-4574-a767-d0c96e2a38f4/ba9422ef-405c-47dd-8553-e11b619185b2/SDK/vsphere-ws/docs/ReferenceGuide/vmodl.query.PropertyCollector.html#continueRetrievePropertiesEx).
>
> A positive value causes [RetrievePropertiesEx](https://vdc-download.vmware.com/vmwb-repository/dcr-public/184bb3ba-6fa8-4574-a767-d0c96e2a38f4/ba9422ef-405c-47dd-8553-e11b619185b2/SDK/vsphere-ws/docs/ReferenceGuide/vmodl.query.PropertyCollector.html#retrievePropertiesEx) to suspend the retrieval when the count of objects reaches the specified maximum. [PropertyCollector](https://vdc-download.vmware.com/vmwb-repository/dcr-public/184bb3ba-6fa8-4574-a767-d0c96e2a38f4/ba9422ef-405c-47dd-8553-e11b619185b2/SDK/vsphere-ws/docs/ReferenceGuide/vmodl.query.PropertyCollector.html) policy may still limit the count to something less than [maxObjects](https://vdc-download.vmware.com/vmwb-repository/dcr-public/184bb3ba-6fa8-4574-a767-d0c96e2a38f4/ba9422ef-405c-47dd-8553-e11b619185b2/SDK/vsphere-ws/docs/ReferenceGuide/vmodl.query.PropertyCollector.RetrieveOptions.html#maxObjects). Any remaining objects may be retrieved with [ContinueRetrievePropertiesEx](https://vdc-download.vmware.com/vmwb-repository/dcr-public/184bb3ba-6fa8-4574-a767-d0c96e2a38f4/ba9422ef-405c-47dd-8553-e11b619185b2/SDK/vsphere-ws/docs/ReferenceGuide/vmodl.query.PropertyCollector.html#continueRetrievePropertiesEx).
>
> A value less than or equal to 0 is illegal.

Specifically the last line -- `A value less than or equal to 0 is illegal.` I checked, and neither the `options` property for [RetrievePropertiesEx](https://vdc-download.vmware.com/vmwb-repository/dcr-public/184bb3ba-6fa8-4574-a767-d0c96e2a38f4/ba9422ef-405c-47dd-8553-e11b619185b2/SDK/vsphere-ws/docs/ReferenceGuide/vmodl.query.PropertyCollector.html#retrievePropertiesEx) is nillable, nor is the `maxObjects` property for the [`RetrieveOptions`](https://vdc-download.vmware.com/vmwb-repository/dcr-public/184bb3ba-6fa8-4574-a767-d0c96e2a38f4/ba9422ef-405c-47dd-8553-e11b619185b2/SDK/vsphere-ws/docs/ReferenceGuide/vmodl.query.PropertyCollector.RetrieveOptions.html) struct. Will those note mean that a zero value is _always_ sent over the wire if no non-empty value is specified?

Patch:
diff --git a/property/collector.go b/property/collector.go
index ec426725..263621a0 100644
--- a/property/collector.go
+++ b/property/collector.go
@@ -142,9 +142,29 @@ func (p *Collector) CancelWaitForUpdates(ctx context.Context) error {
 	return err
 }
 
-func (p *Collector) RetrieveProperties(ctx context.Context, req types.RetrieveProperties) (*types.RetrievePropertiesResponse, error) {
-	req.This = p.Reference()
-	return methods.RetrieveProperties(ctx, p.roundTripper, &req)
+// RetrieveProperties wraps RetrievePropertiesEx and ContinueRetrievePropertiesEx to collect properties in batches.
+func (p *Collector) RetrieveProperties(
+	ctx context.Context,
+	req types.RetrieveProperties,
+	maxObjectsArgs ...int32) (*types.RetrievePropertiesResponse, error) {
+
+	var opts types.RetrieveOptions
+	if l := len(maxObjectsArgs); l > 1 {
+		return nil, fmt.Errorf("maxObjectsArgs accepts a single value")
+	} else if l == 1 {
+		opts.MaxObjects = maxObjectsArgs[0]
+	}
+
+	objects, err := mo.RetrievePropertiesEx(ctx, p.roundTripper, types.RetrievePropertiesEx{
+		This:    p.Reference(),
+		SpecSet: req.SpecSet,
+		Options: opts,
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	return &types.RetrievePropertiesResponse{Returnval: objects}, nil
 }
 
 // Retrieve loads properties for a slice of managed objects. The dst argument
diff --git a/simulator/property_collector.go b/simulator/property_collector.go
index 36ad1e93..c7c60895 100644
--- a/simulator/property_collector.go
+++ b/simulator/property_collector.go
@@ -26,6 +26,8 @@ import (
 	"sync"
 	"time"
 
+	"github.com/google/uuid"
+
 	"github.com/vmware/govmomi/object"
 	"github.com/vmware/govmomi/simulator/internal"
 	"github.com/vmware/govmomi/vim25"
@@ -523,6 +525,62 @@ func (pc *PropertyCollector) DestroyPropertyCollector(ctx *Context, c *types.Des
 	return body
 }
 
+var retrievePropertiesExBook sync.Map
+
+type retrievePropertiesExPage struct {
+	MaxObjects int32
+	Objects    []types.ObjectContent
+}
+
+func (pc *PropertyCollector) ContinueRetrievePropertiesEx(ctx *Context, r *types.ContinueRetrievePropertiesEx) soap.HasFault {
+	body := &methods.ContinueRetrievePropertiesExBody{}
+
+	if r.Token == "" {
+		body.Fault_ = Fault("", &types.InvalidPropertyFault{Name: "token"})
+		return body
+	}
+
+	obj, ok := retrievePropertiesExBook.LoadAndDelete(r.Token)
+	if !ok {
+		body.Fault_ = Fault("", &types.InvalidPropertyFault{Name: "token"})
+		return body
+	}
+
+	page := obj.(retrievePropertiesExPage)
+
+	var (
+		objsToStore  []types.ObjectContent
+		objsToReturn []types.ObjectContent
+	)
+	for i := range page.Objects {
+		if page.MaxObjects <= 0 || i < int(page.MaxObjects) {
+			objsToReturn = append(objsToReturn, page.Objects[i])
+		} else {
+			objsToStore = append(objsToStore, page.Objects[i])
+		}
+	}
+
+	if len(objsToStore) > 0 {
+		body.Res = &types.ContinueRetrievePropertiesExResponse{}
+		body.Res.Returnval.Token = uuid.NewString()
+		retrievePropertiesExBook.Store(
+			body.Res.Returnval.Token,
+			retrievePropertiesExPage{
+				MaxObjects: page.MaxObjects,
+				Objects:    objsToStore,
+			})
+	}
+
+	if len(objsToReturn) > 0 {
+		if body.Res == nil {
+			body.Res = &types.ContinueRetrievePropertiesExResponse{}
+		}
+		body.Res.Returnval.Objects = objsToReturn
+	}
+
+	return body
+}
+
 func (pc *PropertyCollector) RetrievePropertiesEx(ctx *Context, r *types.RetrievePropertiesEx) soap.HasFault {
 	body := &methods.RetrievePropertiesExBody{}
 
@@ -537,7 +595,28 @@ func (pc *PropertyCollector) RetrievePropertiesEx(ctx *Context, r *types.Retriev
 		}
 	} else {
 		objects := res.Objects[:0]
-		for _, o := range res.Objects {
+
+		var (
+			objsToStore  []types.ObjectContent
+			objsToReturn []types.ObjectContent
+		)
+		for i := range res.Objects {
+			if r.Options.MaxObjects <= 0 || i < int(r.Options.MaxObjects) {
+				objsToReturn = append(objsToReturn, res.Objects[i])
+			} else {
+				objsToStore = append(objsToStore, res.Objects[i])
+			}
+		}
+
+		if len(objsToStore) > 0 {
+			res.Token = uuid.NewString()
+			retrievePropertiesExBook.Store(res.Token, retrievePropertiesExPage{
+				MaxObjects: r.Options.MaxObjects,
+				Objects:    objsToStore,
+			})
+		}
+
+		for _, o := range objsToReturn {
 			propSet := o.PropSet[:0]
 			for _, p := range o.PropSet {
 				if p.Val != nil {
diff --git a/simulator/virtual_machine.go b/simulator/virtual_machine.go
index 6070b24b..7f192990 100644
--- a/simulator/virtual_machine.go
+++ b/simulator/virtual_machine.go
@@ -2184,10 +2184,6 @@ func (vm *VirtualMachine) CloneVMTask(ctx *Context, req *types.CloneVM_Task) soa
 				VmPathName: vmx.String(),
 			},
 		}
-		if req.Spec.Config != nil {
-			config.ExtraConfig = req.Spec.Config.ExtraConfig
-			config.InstanceUuid = req.Spec.Config.InstanceUuid
-		}
 
 		// Copying hardware properties
 		config.NumCPUs = vm.Config.Hardware.NumCPU
@@ -2224,6 +2220,14 @@ func (vm *VirtualMachine) CloneVMTask(ctx *Context, req *types.CloneVM_Task) soa
 			})
 		}
 
+		if dst, src := config, req.Spec.Config; src != nil {
+			dst.ExtraConfig = src.ExtraConfig
+			copyNonEmptyValue(&dst.Uuid, &src.Uuid)
+			copyNonEmptyValue(&dst.InstanceUuid, &src.InstanceUuid)
+			copyNonEmptyValue(&dst.NumCPUs, &src.NumCPUs)
+			copyNonEmptyValue(&dst.MemoryMB, &src.MemoryMB)
+		}
+
 		res := ctx.Map.Get(req.Folder).(vmFolder).CreateVMTask(ctx, &types.CreateVM_Task{
 			This:   folder.Self,
 			Config: config,
@@ -2264,6 +2268,17 @@ func (vm *VirtualMachine) CloneVMTask(ctx *Context, req *types.CloneVM_Task) soa
 	}
 }
 
+func copyNonEmptyValue[T comparable](dst, src *T) {
+	if dst == nil || src == nil {
+		return
+	}
+	var t T
+	if *src == t {
+		return
+	}
+	*dst = *src
+}
+
 func (vm *VirtualMachine) RelocateVMTask(ctx *Context, req *types.RelocateVM_Task) soap.HasFault {
 	task := CreateTask(vm, "relocateVm", func(t *Task) (types.AnyType, types.BaseMethodFault) {
 		var changes []types.PropertyChange
diff --git a/vim25/mo/retrieve.go b/vim25/mo/retrieve.go
index 96be376f..9f2b3248 100644
--- a/vim25/mo/retrieve.go
+++ b/vim25/mo/retrieve.go
@@ -158,16 +158,49 @@ func LoadObjectContent(content []types.ObjectContent, dst interface{}) error {
 	return nil
 }
 
+// RetrievePropertiesEx wraps RetrievePropertiesEx and ContinueRetrievePropertiesEx to collect properties in batches.
+func RetrievePropertiesEx(ctx context.Context, r soap.RoundTripper, req types.RetrievePropertiesEx) ([]types.ObjectContent, error) {
+	rx, err := methods.RetrievePropertiesEx(ctx, r, &req)
+	if err != nil {
+		return nil, err
+	}
+
+	if rx.Returnval == nil {
+		return nil, nil
+	}
+
+	objects := rx.Returnval.Objects
+	token := rx.Returnval.Token
+
+	for token != "" {
+		cx, err := methods.ContinueRetrievePropertiesEx(ctx, r, &types.ContinueRetrievePropertiesEx{
+			This:  req.This,
+			Token: token,
+		})
+		if err != nil {
+			return nil, err
+		}
+
+		token = cx.Returnval.Token
+		objects = append(objects, cx.Returnval.Objects...)
+	}
+
+	return objects, nil
+}
+
 // RetrievePropertiesForRequest calls the RetrieveProperties method with the
 // specified request and decodes the response struct into the value pointed to
 // by dst.
 func RetrievePropertiesForRequest(ctx context.Context, r soap.RoundTripper, req types.RetrieveProperties, dst interface{}) error {
-	res, err := methods.RetrieveProperties(ctx, r, &req)
+	objects, err := RetrievePropertiesEx(ctx, r, types.RetrievePropertiesEx{
+		This:    req.This,
+		SpecSet: req.SpecSet,
+	})
 	if err != nil {
 		return err
 	}
 
-	return LoadObjectContent(res.Returnval, dst)
+	return LoadObjectContent(objects, dst)
 }
 
 // RetrieveProperties retrieves the properties of the managed object specified


Test patch:
diff --git a/property/collector_test.go b/property/collector_test.go
index 00134c7f..06ca8c26 100644
--- a/property/collector_test.go
+++ b/property/collector_test.go
@@ -66,7 +66,9 @@ func TestWaitForUpdatesEx(t *testing.T) {
 				cancelCtx,
 				pc,
 				&property.WaitFilter{
-					CreateFilter: getDatacenterToVMFolderFilter(datacenter),
+					CreateFilter: types.CreateFilter{
+						Spec: getDatacenterToVMFolderFilter(datacenter),
+					},
 					WaitOptions: property.WaitOptions{
 						Options: &types.WaitOptions{
 							MaxWaitSeconds: addrOf(int32(3)),
@@ -108,6 +110,45 @@ func TestWaitForUpdatesEx(t *testing.T) {
 	}, model)
 }
 
+func TestRetrievePropertiesOneAtATime(t *testing.T) {
+	model := simulator.VPX()
+	model.Datacenter = 1
+	model.Cluster = 0
+	model.Pool = 0
+	model.Machine = 3
+	model.Autostart = false
+
+	simulator.Test(func(ctx context.Context, c *vim25.Client) {
+		finder := find.NewFinder(c, true)
+		datacenter, err := finder.DefaultDatacenter(ctx)
+		if err != nil {
+			t.Fatalf("default datacenter not found: %s", err)
+		}
+		finder.SetDatacenter(datacenter)
+		pc := property.DefaultCollector(c)
+
+		resp, err := pc.RetrieveProperties(ctx, types.RetrieveProperties{
+			SpecSet: []types.PropertyFilterSpec{
+				getDatacenterToVMFolderFilter(datacenter),
+			},
+		}, 1)
+		if err != nil {
+			t.Fatalf("failed to retrieve properties one object at a time: %s", err)
+		}
+
+		vmRefs := map[types.ManagedObjectReference]struct{}{}
+		for i := range resp.Returnval {
+			oc := resp.Returnval[i]
+			vmRefs[oc.Obj] = struct{}{}
+		}
+
+		if a, e := len(vmRefs), 3; a != 3 {
+			t.Fatalf("unexpected number of vms: a=%d, e=%d", a, e)
+		}
+
+	}, model)
+}
+
 func waitForPowerStateChanges(
 	ctx context.Context,
 	vm *object.VirtualMachine,
@@ -139,52 +180,50 @@ func waitForPowerStateChanges(
 	return false
 }
 
-func getDatacenterToVMFolderFilter(dc *object.Datacenter) types.CreateFilter {
+func getDatacenterToVMFolderFilter(dc *object.Datacenter) types.PropertyFilterSpec {
 	// Define a wait filter that looks for updates to VM power
 	// states for VMs under the specified datacenter.
-	return types.CreateFilter{
-		Spec: types.PropertyFilterSpec{
-			ObjectSet: []types.ObjectSpec{
-				{
-					Obj:  dc.Reference(),
-					Skip: addrOf(true),
-					SelectSet: []types.BaseSelectionSpec{
-						// Datacenter --> VM folder
-						&types.TraversalSpec{
-							SelectionSpec: types.SelectionSpec{
-								Name: "dcToVMFolder",
-							},
-							Type: "Datacenter",
-							Path: "vmFolder",
-							SelectSet: []types.BaseSelectionSpec{
-								&types.SelectionSpec{
-									Name: "visitFolders",
-								},
+	return types.PropertyFilterSpec{
+		ObjectSet: []types.ObjectSpec{
+			{
+				Obj:  dc.Reference(),
+				Skip: addrOf(true),
+				SelectSet: []types.BaseSelectionSpec{
+					// Datacenter --> VM folder
+					&types.TraversalSpec{
+						SelectionSpec: types.SelectionSpec{
+							Name: "dcToVMFolder",
+						},
+						Type: "Datacenter",
+						Path: "vmFolder",
+						SelectSet: []types.BaseSelectionSpec{
+							&types.SelectionSpec{
+								Name: "visitFolders",
 							},
 						},
+					},
+					// Folder --> children (folder / VM)
+					&types.TraversalSpec{
+						SelectionSpec: types.SelectionSpec{
+							Name: "visitFolders",
+						},
+						Type: "Folder",
 						// Folder --> children (folder / VM)
-						&types.TraversalSpec{
-							SelectionSpec: types.SelectionSpec{
+						Path: "childEntity",
+						SelectSet: []types.BaseSelectionSpec{
+							// Folder --> child folder
+							&types.SelectionSpec{
 								Name: "visitFolders",
 							},
-							Type: "Folder",
-							// Folder --> children (folder / VM)
-							Path: "childEntity",
-							SelectSet: []types.BaseSelectionSpec{
-								// Folder --> child folder
-								&types.SelectionSpec{
-									Name: "visitFolders",
-								},
-							},
 						},
 					},
 				},
 			},
-			PropSet: []types.PropertySpec{
-				{
-					Type:    "VirtualMachine",
-					PathSet: []string{"runtime.powerState"},
-				},
+		},
+		PropSet: []types.PropertySpec{
+			{
+				Type:    "VirtualMachine",
+				PathSet: []string{"runtime.powerState"},
 			},
 		},
 	}
diff --git a/property/example_test.go b/property/example_test.go
index 507e7436..5621e400 100644
--- a/property/example_test.go
+++ b/property/example_test.go
@@ -187,7 +187,10 @@ func ExampleCollector_WaitForUpdatesEx_addingRemovingPropertyFilters() {
 		}
 
 		// Now create a property filter that will catch the update.
-		pf, err := pc.CreateFilter(ctx, getDatacenterToVMFolderFilter(datacenter))
+		pf, err := pc.CreateFilter(
+			ctx,
+			types.CreateFilter{Spec: getDatacenterToVMFolderFilter(datacenter)},
+		)
 		if err != nil {
 			return fmt.Errorf("failed to create dc2vm property filter: %w", err)
 		}