Merge pull request vmware#2768 from Syuparn/issue-2767

vcsim: Fix NFS datastore moid collision

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{

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")
+			}
+		})
+	}
+}

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 {