Repository: vmware/govmomi
Pull request #2768 (2022-02-21 12:10:26)

vcsim: Fix NFS datastore moid collision
## Description

Fix NFS datastore moids in vcsim so that they are unique to each datastore.
This fixes the issue that multiple `CreateNfsDatastore` requests except the first one does not work.

Closes: #2767

## Type of change

Please mark options that are relevant:

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] 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?

Please describe the tests that you ran to verify your changes. Provide
instructions so we can reproduce. If applicable, please also list any relevant
details for your test configuration.

1. Start vcsim
1. create NFS Datastore `ds1` on `DC0_C0_H0` by govc
1. create NFS Datastore `ds2` on `DC0_C0_H0` by govc

```bash
$ govc datastore.create -name /ds1 -type nfs DC0_C0_H0
$ govc datastore.create -name /ds2 -type nfs DC0_C0_H0
$ govc datastore.info
Name:        LocalDS_0
  Path:      /DC0/datastore/LocalDS_0
  Type:      OTHER
  URL:       /tmp/govcsim-DC0-LocalDS_0-2611405272
  Capacity:  10240.0 GB
  Free:      10200.0 GB
Name:        ds1
  Path:      /DC0/datastore/ds1
  Type:      NFS
  URL:       /ds1
  Capacity:  10240.0 GB
  Free:      10240.0 GB
  Remote:    :
Name:        ds2
  Path:      /DC0/datastore/ds2
  Type:      NFS
  URL:       /ds2
  Capacity:  10240.0 GB
  Free:      10240.0 GB
  Remote:    :
# every datastore has unique moid
govc datastore.info -json | jq .Datastores[].Self
{
  "Type": "Datastore",
  "Value": "datastore-52"
}
{
  "Type": "Datastore",
  "Value": "datastore-66"
}
{
  "Type": "Datastore",
  "Value": "datastore-68"
}
```

## Checklist:

- [x] My code follows the CONTRIBUTION [guidelines](./../CONTRIBUTING.md) of
  this project
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] 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
- [ ] Any dependent changes have been merged


dougm: Thanks @Syuparn , I will take a closer look at this, but I think that vcsim needs to validate that `RemoteHost` and `RemotePath` are specified as real vCenter does.  When specified the moid would be unique:
```console
% govc datastore.create -type nfs -path "$dir1" -name nfs1 -remote-host nfs.example.com -remote-path /ds1 DC0_C0_H0

% govc datastore.create -type nfs -path "$dir2" -name nfs2 -remote-host nfs.example.com -remote-path /ds2 DC0_C0_H0

% govc datastore.info -json | jq .Datastores[].Self
{
  "Type": "Datastore",
  "Value": "/Users/dougm/tmp/govcsim-DC0-LocalDS_0-105522338@group-5"
}
{
  "Type": "Datastore",
  "Value": "nfs.example.com:/ds1@group-5"
}
{
  "Type": "Datastore",
  "Value": "nfs.example.com:/ds2@group-5"
}
```

Patch:
diff --git a/gen/Gemfile.lock b/gen/Gemfile.lock
index 1c40743c..4e7ec80b 100644
--- a/gen/Gemfile.lock
+++ b/gen/Gemfile.lock
@@ -1,12 +1,12 @@
 GEM
   remote: https://rubygems.org/
   specs:
-    mini_portile2 (2.8.0)
-    nokogiri (1.13.2)
-      mini_portile2 (~> 2.8.0)
+    mini_portile2 (2.6.1)
+    nokogiri (1.12.5)
+      mini_portile2 (~> 2.6.1)
       racc (~> 1.4)
     power_assert (0.4.1)
-    racc (1.6.0)
+    racc (1.5.2)
     test-unit (3.2.3)
       power_assert
 
diff --git a/object/virtual_device_list.go b/object/virtual_device_list.go
index 65265453..c45f553a 100644
--- a/object/virtual_device_list.go
+++ b/object/virtual_device_list.go
@@ -64,7 +64,6 @@ func EthernetCardTypes() VirtualDeviceList {
 		&types.VirtualE1000e{},
 		&types.VirtualVmxnet2{},
 		&types.VirtualVmxnet3{},
-		&types.VirtualVmxnet3Vrdma{},
 		&types.VirtualPCNet32{},
 		&types.VirtualSriovEthernetCard{},
 	}).Select(func(device types.BaseVirtualDevice) bool {
diff --git a/simulator/host_datastore_system.go b/simulator/host_datastore_system.go
index a7f6780e..da643691 100644
--- a/simulator/host_datastore_system.go
+++ b/simulator/host_datastore_system.go
@@ -60,11 +60,23 @@ func (dss *HostDatastoreSystem) add(ctx *Context, ds *Datastore) *soap.Fault {
 	}
 
 	folder := ctx.Map.getEntityFolder(dss.Host, "datastore")
-	ds.Self.Type = typeName(ds)
-	// Datastore is the only type where create methods do not include the parent (Folder in this case),
-	// but we need the moref to be unique per DC/datastoreFolder, but not per-HostSystem.
-	ds.Self.Value += "@" + folder.Self.Value
-	// TODO: name should be made unique in the case of Local ds type
+
+	found := false
+	if e := ctx.Map.FindByName(ds.Name, folder.ChildEntity); e != nil {
+		if e.Reference().Type != "Datastore" {
+			return Fault(e.Reference().Value, &types.DuplicateName{
+				Name:   ds.Name,
+				Object: e.Reference(),
+			})
+		}
+
+		// if datastore already exists, use current reference
+		found = true
+		ds.Self = e.Reference()
+	} else {
+		// put datastore to folder and generate reference
+		folderPutChild(ctx, folder, ds)
+	}
 
 	ds.Summary.Datastore = &ds.Self
 	ds.Summary.Name = ds.Name
@@ -84,7 +96,8 @@ func (dss *HostDatastoreSystem) add(ctx *Context, ds *Datastore) *soap.Fault {
 	parent := hostParent(dss.Host)
 	ctx.Map.AddReference(ctx, parent, &parent.Datastore, ds.Self)
 
-	if ctx.Map.Get(ds.Self) == nil {
+	// NOTE: browser must be created after ds is appended to dss.Datastore
+	if !found {
 		browser := &HostDatastoreBrowser{}
 		browser.Datastore = dss.Datastore
 		ds.Browser = ctx.Map.Put(browser).Reference()
@@ -95,8 +108,6 @@ func (dss *HostDatastoreSystem) add(ctx *Context, ds *Datastore) *soap.Fault {
 		info.FreeSpace = ds.Summary.FreeSpace
 		info.MaxMemoryFileSize = ds.Summary.Capacity
 		info.MaxFileSize = ds.Summary.Capacity
-
-		folderPutChild(ctx, folder, ds)
 	}
 
 	return nil
@@ -107,7 +118,6 @@ func (dss *HostDatastoreSystem) CreateLocalDatastore(ctx *Context, c *types.Crea
 
 	ds := &Datastore{}
 	ds.Name = c.Name
-	ds.Self.Value = c.Path
 
 	ds.Info = &types.LocalDatastoreInfo{
 		DatastoreInfo: types.DatastoreInfo{
@@ -147,9 +157,24 @@ func (dss *HostDatastoreSystem) CreateLocalDatastore(ctx *Context, c *types.Crea
 func (dss *HostDatastoreSystem) CreateNasDatastore(ctx *Context, c *types.CreateNasDatastore) soap.HasFault {
 	r := &methods.CreateNasDatastoreBody{}
 
+	// validate RemoteHost and RemotePath are specified
+	if c.Spec.RemoteHost == "" {
+		r.Fault_ = Fault(
+			"A specified parameter was not correct: Spec.RemoteHost",
+			&types.InvalidArgument{InvalidProperty: "RemoteHost"},
+		)
+		return r
+	}
+	if c.Spec.RemotePath == "" {
+		r.Fault_ = Fault(
+			"A specified parameter was not correct: Spec.RemotePath",
+			&types.InvalidArgument{InvalidProperty: "RemotePath"},
+		)
+		return r
+	}
+
 	ds := &Datastore{}
 	ds.Name = path.Base(c.Spec.LocalPath)
-	ds.Self.Value = c.Spec.RemoteHost + ":" + c.Spec.RemotePath
 
 	ds.Info = &types.NasDatastoreInfo{
 		DatastoreInfo: types.DatastoreInfo{
diff --git a/vapi/simulator/simulator.go b/vapi/simulator/simulator.go
index d3c2b3e9..6f44d7b2 100644
--- a/vapi/simulator/simulator.go
+++ b/vapi/simulator/simulator.go
@@ -1523,10 +1523,13 @@ func (s *handler) updateFileInfo(id string) *update {
 
 // libraryPath returns the local Datastore fs path for a Library or Item if id is specified.
 func libraryPath(l *library.Library, id string) string {
-	// DatastoreID (moref) format is "$local-path@$ds-folder-id",
-	// see simulator.HostDatastoreSystem.CreateLocalDatastore
-	ds := strings.SplitN(l.Storage[0].DatastoreID, "@", 2)[0]
-	return path.Join(append([]string{ds, "contentlib-" + l.ID}, id)...)
+	dsref := types.ManagedObjectReference{
+		Type:  "Datastore",
+		Value: l.Storage[0].DatastoreID,
+	}
+	ds := simulator.Map.Get(dsref).(*simulator.Datastore)
+
+	return path.Join(append([]string{ds.Info.GetDatastoreInfo().Url, "contentlib-" + l.ID}, id)...)
 }
 
 func (s *handler) libraryItemFileCreate(up *update, name string, body io.ReadCloser) error {


Test patch:
diff --git a/simulator/host_datastore_system_test.go b/simulator/host_datastore_system_test.go
index 7e53de78..5a6a6e99 100644
--- a/simulator/host_datastore_system_test.go
+++ b/simulator/host_datastore_system_test.go
@@ -98,3 +98,61 @@ func TestHostDatastoreSystem(t *testing.T) {
 		}
 	}
 }
+
+func TestCreateNasDatastoreValidation(t *testing.T) {
+	s := New(NewServiceInstance(SpoofContext(), esx.ServiceContent, esx.RootFolder))
+
+	ts := s.NewServer()
+	defer ts.Close()
+
+	ctx := context.Background()
+
+	c, err := govmomi.NewClient(ctx, ts.URL, true)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	host := object.NewHostSystem(c.Client, esx.HostSystem.Reference())
+
+	dss, err := host.ConfigManager().DatastoreSystem(ctx)
+	if err != nil {
+		t.Error(err)
+	}
+
+	pwd, err := os.Getwd()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	tests := []struct {
+		name string
+		spec types.HostNasVolumeSpec
+	}{
+		{
+			"RemotePath is not specified",
+			types.HostNasVolumeSpec{
+				Type:       string(types.HostFileSystemVolumeFileSystemTypeNFS),
+				LocalPath:  pwd,
+				RemoteHost: "localhost",
+			},
+		},
+		{
+			"RemoteHost is not specified",
+			types.HostNasVolumeSpec{
+				Type:       string(types.HostFileSystemVolumeFileSystemTypeNFS),
+				LocalPath:  pwd,
+				RemotePath: pwd,
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			_, err := dss.CreateNasDatastore(ctx, tt.spec)
+
+			if err == nil {
+				t.Error("expected error")
+			}
+		})
+	}
+}