feat(esxi): change parameters for NewDiskDev and avoid unnecessary deploy

1. The fields UnitNumber and Index should be independent
2. Avoid unnecessary deploy, such create vm with iso not image

pkg/hostman/storageman/storage_agent.go

@@ -134,36 +134,36 @@ func (as *SAgentStorage) agentRebuildRoot(ctx context.Context, data jsonutils.JS
 	return vm.DoRebuildRoot(ctx, newPath, diskId)
 }
 
-func (as *SAgentStorage) agentCreateGuest(ctx context.Context, data *jsonutils.JSONDict) error {
+func (as *SAgentStorage) agentCreateGuest(ctx context.Context, data *jsonutils.JSONDict) (bool, error) {
 	hd := SHostDatastore{}
 	err := data.Unmarshal(&hd)
 	if err != nil {
-		return errors.Wrap(err, hostutils.ParamsError.Error())
+		return false, errors.Wrap(err, hostutils.ParamsError.Error())
 	}
 	host, ds, err := as.getHostAndDatastore(ctx, hd)
 	if err != nil {
-		return err
+		return false, err
 	}
 	desc, _ := data.Get("desc")
 	descDict, ok := desc.(*jsonutils.JSONDict)
 	if !ok {
-		return errors.Wrap(hostutils.ParamsError, "agentCreateGuest data format error")
+		return false, errors.Wrap(hostutils.ParamsError, "agentCreateGuest data format error")
 	}
 	createParam := esxi.SCreateVMParam{}
 	err = descDict.Unmarshal(&createParam)
 	if err != nil {
-		return errors.Wrapf(err, "%s: fail to unmarshal to esxi.SCreateVMParam", hostutils.ParamsError)
+		return false, errors.Wrapf(err, "%s: fail to unmarshal to esxi.SCreateVMParam", hostutils.ParamsError)
 	}
-	vm, err := host.CreateVM2(ctx, ds, createParam)
+	needDeploy, vm, err := host.CreateVM2(ctx, ds, createParam)
 	if err != nil {
-		return errors.Wrap(err, "SHost.CreateVM2")
+		return false, errors.Wrap(err, "SHost.CreateVM2")
 	}
 	name, _ := descDict.GetString("name")
 	err = as.tryRenameVm(ctx, vm, name)
 	if err != nil {
-		return errors.Wrapf(err, "RenameVm name '%s'", name)
+		return false, errors.Wrapf(err, "RenameVm name '%s'", name)
 	}
-	return nil
+	return needDeploy, nil
 }
 
 func (as *SAgentStorage) tryRenameVm(ctx context.Context, vm *esxi.SVirtualMachine, name string) error {
@@ -194,8 +194,12 @@ func (as *SAgentStorage) AgentDeployGuest(ctx context.Context, data interface{})
 	init := false
 	dataDict := data.(*jsonutils.JSONDict)
 	action, _ := dataDict.GetString("action")
+	var (
+		needDeploy bool
+		err        error
+	)
 	if action == "create" {
-		err := as.agentCreateGuest(ctx, dataDict)
+		needDeploy, err = as.agentCreateGuest(ctx, dataDict)
 		if err != nil {
 			return nil, errors.Wrap(err, "agentCreateGuest")
 		}
@@ -275,49 +279,52 @@ func (as *SAgentStorage) AgentDeployGuest(ctx context.Context, data interface{})
 	}
 
 	log.Debugf("host: %s, port: %d, user: %s, passwd: %s", info.Host, info.Port, info.Account, info.Password)
-	vddkInfo := deployapi.VDDKConInfo{
-		Host:   info.Host,
-		Port:   int32(info.Port),
-		User:   info.Account,
-		Passwd: info.Password,
-		Vmref:  vmref,
-	}
-	guestDesc := deployapi.GuestDesc{}
-	err = dataDict.Unmarshal(&guestDesc, "desc")
-	if err != nil {
-		return nil, errors.Wrapf(err, "%s: unmarshal to guestDesc", hostutils.ParamsError.Error())
-	}
-
-	desc, _ := dataDict.Get("desc")
-	guestDesc.Hypervisor = api.HYPERVISOR_ESXI
-	deploy, err := deployclient.GetDeployClient().DeployGuestFs(ctx, &deployapi.DeployParams{
-		DiskPath:  rootPath,
-		GuestDesc: &guestDesc,
-		DeployInfo: &deployapi.DeployInfo{
-			PublicKey:               &key,
-			Deploys:                 deployArray,
-			Password:                passwd,
-			IsInit:                  init,
-			WindowsDefaultAdminUser: true,
-		},
-		VddkInfo: &vddkInfo,
-	})
-	customize := false
-	if err != nil {
-		log.Errorf("unable to DeployGuestFs: %v", err)
-		customize = true
-	} else if deploy == nil {
-		log.Errorf("unable to DeployGuestFs: deploy is nil")
-		customize = true
-	} else if len(deploy.Os) == 0 {
-		log.Errorf("unable to DeployGuestFs: os is empty")
-		customize = true
-	}
-	if customize == true {
-		as.waitVmToolsVersion(ctx, vm)
-		err = vm.DoCustomize(ctx, desc)
+	var deploy *deployapi.DeployGuestFsResponse
+	if needDeploy {
+		vddkInfo := deployapi.VDDKConInfo{
+			Host:   info.Host,
+			Port:   int32(info.Port),
+			User:   info.Account,
+			Passwd: info.Password,
+			Vmref:  vmref,
+		}
+		guestDesc := deployapi.GuestDesc{}
+		err = dataDict.Unmarshal(&guestDesc, "desc")
 		if err != nil {
-			log.Errorf("unable to DoCustomize for vm %s: %v", vm.GetId(), err)
+			return nil, errors.Wrapf(err, "%s: unmarshal to guestDesc", hostutils.ParamsError.Error())
+		}
+
+		desc, _ := dataDict.Get("desc")
+		guestDesc.Hypervisor = api.HYPERVISOR_ESXI
+		deploy, err = deployclient.GetDeployClient().DeployGuestFs(ctx, &deployapi.DeployParams{
+			DiskPath:  rootPath,
+			GuestDesc: &guestDesc,
+			DeployInfo: &deployapi.DeployInfo{
+				PublicKey:               &key,
+				Deploys:                 deployArray,
+				Password:                passwd,
+				IsInit:                  init,
+				WindowsDefaultAdminUser: true,
+			},
+			VddkInfo: &vddkInfo,
+		})
+		customize := false
+		if err != nil {
+			log.Errorf("unable to DeployGuestFs: %v", err)
+			customize = true
+		} else if deploy == nil {
+			log.Errorf("unable to DeployGuestFs: deploy is nil")
+			customize = true
+		} else if len(deploy.Os) == 0 {
+			log.Errorf("unable to DeployGuestFs: os is empty")
+			customize = true
+		}
+		if customize == true {
+			as.waitVmToolsVersion(ctx, vm)
+			err = vm.DoCustomize(ctx, desc)
+			if err != nil {
+				log.Errorf("unable to DoCustomize for vm %s: %v", vm.GetId(), err)
+			}
 		}
 	}
 

pkg/multicloud/esxi/devtools.go

@@ -24,15 +24,28 @@ import (
 	"yunion.io/x/pkg/errors"
 )
 
-func NewDiskDev(sizeMb int64, templatePath string, uuid string, index int32, keyBase int32, controlKey int32, key int32) *types.VirtualDisk {
+type SDiskConfig struct {
+	SizeMb        int64
+	Uuid          string
+	ControllerKey int32
+	UnitNumber    int32
+	Key           int32
+	ImagePath     string
+	IsRoot        bool
+}
+
+// In fact, it is the default lable of first one disk
+const rootDiskMark = "Hard disk 1"
+
+func NewDiskDev(sizeMb int64, config SDiskConfig) *types.VirtualDisk {
 	device := types.VirtualDisk{}
 	diskFile := types.VirtualDiskFlatVer2BackingInfo{}
 	diskFile.DiskMode = "persistent"
 	thinProvisioned := true
 	diskFile.ThinProvisioned = &thinProvisioned
-	diskFile.Uuid = uuid
-	if len(templatePath) > 0 {
-		diskFile.FileName = templatePath
+	diskFile.Uuid = config.Uuid
+	if len(config.ImagePath) > 0 {
+		diskFile.FileName = config.ImagePath
 	}
 	device.Backing = &diskFile
 
@@ -40,13 +53,15 @@ func NewDiskDev(sizeMb int64, templatePath string, uuid string, index int32, key
 		device.CapacityInKB = sizeMb * 1024
 	}
 
-	device.ControllerKey = controlKey
-	if key != 0 {
-		device.Key = key
-	} else {
-		device.Key = keyBase + index
+	device.ControllerKey = config.ControllerKey
+	device.Key = config.Key
+	device.UnitNumber = &config.UnitNumber
+
+	var label string
+	if config.IsRoot {
+		label = rootDiskMark
+		device.DeviceInfo = &types.Description{Label: label}
 	}
-	device.UnitNumber = &index
 
 	return &device
 }

pkg/multicloud/esxi/host.go

@@ -728,34 +728,174 @@ type SEsxiImageInfo struct {
 	StorageCacheHostIp string
 }
 
-func (self *SHost) CreateVM2(ctx context.Context, ds *SDatastore, params SCreateVMParam) (*SVirtualMachine, error) {
+func (self *SHost) CreateVM2(ctx context.Context, ds *SDatastore, params SCreateVMParam) (needDeploy bool, vm *SVirtualMachine, err error) {
+	needDeploy = true
+	var temvm *SVirtualMachine
 	if len(params.InstanceSnapshotInfo.InstanceSnapshotId) > 0 {
-		temvm, err := self.manager.SearchVM(params.InstanceSnapshotInfo.InstanceId)
+		temvm, err = self.manager.SearchVM(params.InstanceSnapshotInfo.InstanceId)
 		if err != nil {
-			return nil, errors.Wrapf(err, "can't find vm %q, please sync status for vm or sync cloudaccount", params.InstanceSnapshotInfo.InstanceId)
+			err = errors.Wrapf(err, "can't find vm %q, please sync status for vm or sync cloudaccount", params.InstanceSnapshotInfo.InstanceId)
 		}
-		isp, err := temvm.GetInstanceSnapshot(params.InstanceSnapshotInfo.InstanceSnapshotId)
+		var isp cloudprovider.ICloudInstanceSnapshot
+		isp, err = temvm.GetInstanceSnapshot(params.InstanceSnapshotInfo.InstanceSnapshotId)
 		if err != nil {
-			return nil, errors.Wrap(err, "unable to GetInstanceSnapshot")
+			err = errors.Wrap(err, "unable to GetInstanceSnapshot")
+			return
 		}
 		sp := isp.(*SVirtualMachineSnapshot)
-		return self.CloneVM(ctx, temvm, &sp.snapshotTree.Snapshot, ds, params)
+		vm, err = self.CloneVM(ctx, temvm, &sp.snapshotTree.Snapshot, ds, params)
+		return
 	}
 	if len(params.Disks) == 0 {
-		return nil, errors.Error("empty disk config")
+		err = errors.Error("empty disk config")
+		return
 	}
 	imageInfo := params.Disks[0].ImageInfo
 	if imageInfo.ImageType == string(cloudprovider.ImageTypeSystem) {
-		temvm, err := self.manager.SearchTemplateVM(imageInfo.ImageExternalId)
+		temvm, err = self.manager.SearchTemplateVM(imageInfo.ImageExternalId)
 		if err != nil {
-			return nil, errors.Wrapf(err, "SEsxiClient.SearchTemplateVM for image %q", imageInfo.ImageExternalId)
+			err = errors.Wrapf(err, "SEsxiClient.SearchTemplateVM for image %q", imageInfo.ImageExternalId)
+			return
 		}
-		return self.CloneVM(ctx, temvm, nil, ds, params)
+		vm, err = self.CloneVM(ctx, temvm, nil, ds, params)
+		return
 	}
 	return self.DoCreateVM(ctx, ds, params)
 }
 
-func (self *SHost) DoCreateVM(ctx context.Context, ds *SDatastore, params SCreateVMParam) (*SVirtualMachine, error) {
+func (self *SHost) needScsi(disks []SDiskInfo) bool {
+	if len(disks) == 0 {
+		return false
+	}
+	for i := range disks {
+		driver := disks[i].Driver
+		if driver == "" || driver == "scsi" || driver == "pvscsi" {
+			return true
+		}
+	}
+	return false
+}
+
+func (self *SHost) addDisks(ctx context.Context, dc *SDatacenter, ds *SDatastore, disks []SDiskInfo, uuid string, objectVm *object.VirtualMachine) (*SVirtualMachine, error) {
+	getVM := func() (*SVirtualMachine, error) {
+		var moVM mo.VirtualMachine
+		err := self.manager.reference2Object(objectVm.Reference(), VIRTUAL_MACHINE_PROPS, &moVM)
+		if err != nil {
+			return nil, errors.Wrap(err, "fail to fetch virtual machine just created")
+		}
+
+		evm := NewVirtualMachine(self.manager, &moVM, self.datacenter)
+		if evm == nil {
+			return nil, errors.Error("create successfully but unable to NewVirtualMachine")
+		}
+		return evm, nil
+	}
+
+	if len(disks) == 0 {
+		return getVM()
+	}
+
+	var (
+		scsiIdx    = 0
+		ideIdx     = 0
+		ide1un     = 0
+		ide2un     = 1
+		unitNumber = 0
+		ctrlKey    = 0
+	)
+	deviceChange := make([]types.BaseVirtualDeviceConfigSpec, 0, 1)
+	// add disks
+	var rootDiskSizeMb int64
+	for i, disk := range disks {
+		imagePath := disk.ImagePath
+		var size = disk.Size
+		if len(imagePath) == 0 {
+			if size == 0 {
+				size = 30 * 1024
+			}
+		} else {
+			imagePath, err := self.FileUrlPathToDsPath(imagePath)
+			if err != nil {
+				return nil, errors.Wrapf(err, "SHost.FileUrlPathToDsPath")
+			}
+			newImagePath := fmt.Sprintf("[%s] %s/%s.vmdk", ds.GetRelName(), uuid, uuid)
+			fm := ds.getDatastoreObj().NewFileManager(dc.getObjectDatacenter(), true)
+			err = fm.Copy(ctx, imagePath, newImagePath)
+			if err != nil {
+				return nil, errors.Wrap(err, "unable to copy system disk")
+			}
+			imagePath = newImagePath
+			rootDiskSizeMb = size
+		}
+		uuid, driver := disk.DiskId, "scsi"
+		if len(disk.Driver) > 0 {
+			driver = disk.Driver
+		}
+		if driver == "scsi" || driver == "pvscsi" {
+			if self.isVersion50() {
+				driver = "scsi"
+			}
+			ctrlKey = 1000
+			unitNumber = scsiIdx
+			scsiIdx += 1
+			if scsiIdx == 7 {
+				scsiIdx++
+			}
+		} else {
+			ideno := ideIdx % 2
+			if ideno == 0 {
+				unitNumber = ideIdx/2 + ide1un
+			} else {
+				unitNumber = ideIdx/2 + ide2un
+			}
+			ctrlKey = 200 + ideno
+			ideIdx += 1
+		}
+		log.Debugf("size: %d, image path: %s, uuid: %s, index: %d, ctrlKey: %d, driver: %s, key: %d.", size, imagePath, uuid, unitNumber, ctrlKey, disk.Driver, 2000+i)
+		spec := addDevSpec(NewDiskDev(size, SDiskConfig{
+			SizeMb:        size,
+			Uuid:          uuid,
+			ControllerKey: int32(ctrlKey),
+			UnitNumber:    int32(unitNumber),
+			Key:           int32(2000 + i),
+			ImagePath:     imagePath,
+			IsRoot:        i == 0,
+		}))
+		if len(imagePath) == 0 {
+			spec.FileOperation = "create"
+		}
+		deviceChange = append(deviceChange, spec)
+	}
+	log.Infof("deviceChange: %s", jsonutils.Marshal(deviceChange))
+
+	configSpec := types.VirtualMachineConfigSpec{}
+	configSpec.DeviceChange = deviceChange
+	task, err := objectVm.Reconfigure(ctx, configSpec)
+	if err != nil {
+		return nil, errors.Wrap(err, "unable to reconfigure")
+	}
+	err = task.Wait(ctx)
+	if err != nil {
+		return nil, errors.Wrap(err, "task.Wait")
+	}
+
+	evm, err := getVM()
+	if err != nil {
+		return nil, err
+	}
+
+	// resize root disk
+	if rootDiskSizeMb > 0 && int64(evm.vdisks[0].GetDiskSizeMB()) != rootDiskSizeMb {
+		err = evm.vdisks[0].Resize(ctx, rootDiskSizeMb)
+		if err != nil {
+			return evm, errors.Wrap(err, "resize for root disk")
+		}
+	}
+	return evm, nil
+}
+
+func (self *SHost) DoCreateVM(ctx context.Context, ds *SDatastore, params SCreateVMParam) (needDeploy bool, vm *SVirtualMachine, err error) {
+	needDeploy = true
 	deviceChange := make([]types.BaseVirtualDeviceConfigSpec, 0, 5)
 
 	// uuid first
@@ -800,35 +940,25 @@ func (self *SHost) DoCreateVM(ctx context.Context, ds *SDatastore, params SCreat
 	deviceChange = append(deviceChange, addDevSpec(NewIDEDev(200, 0)))
 	deviceChange = append(deviceChange, addDevSpec(NewIDEDev(200, 1)))
 	deviceChange = append(deviceChange, addDevSpec(NewSVGADev(500, 100)))
-	disks, driver := params.Disks, "scsi"
-	if len(disks) > 0 {
-		driver = disks[0].Driver
-	}
-	if driver == "scsi" || driver == "pvscsi" {
+
+	if self.needScsi(params.Disks) {
+		driver := "pvscsi"
 		if self.isVersion50() {
 			driver = "scsi"
 		}
 		deviceChange = append(deviceChange, addDevSpec(NewSCSIDev(1000, 100, driver)))
 	}
-	var err error
 	cdromPath := params.Cdrom.Path
 	if len(cdromPath) > 0 {
+		needDeploy = false
 		cdromPath, err = self.FileUrlPathToDsPath(cdromPath)
 		if err != nil {
-			return nil, errors.Wrapf(err, "SHost.FileUrlPathToDsPath for cdrom path")
+			err = errors.Wrapf(err, "SHost.FileUrlPathToDsPath for cdrom path")
+			return
 		}
 	}
 	deviceChange = append(deviceChange, addDevSpec(NewCDROMDev(cdromPath, 16000, 201)))
 
-	var (
-		scsiIdx = 0
-		ideIdx  = 0
-		ide1un  = 0
-		ide2un  = 1
-		index   = 0
-		ctrlKey = 0
-	)
-
 	// add usb to support mouse
 	usbController := addDevSpec(NewUSBController(nil))
 	deviceChange = append(deviceChange, usbController)
@@ -851,7 +981,7 @@ func (self *SHost) DoCreateVM(ctx context.Context, ds *SDatastore, params SCreat
 		}
 		dev, err := NewVNICDev(self, mac, driver, bridge, int32(vlanId), 4000, 100, int32(index))
 		if err != nil {
-			return nil, errors.Wrap(err, "NewVNICDev")
+			return needDeploy, nil, errors.Wrap(err, "NewVNICDev")
 		}
 		deviceChange = append(deviceChange, addDevSpec(dev))
 	}
@@ -859,118 +989,36 @@ func (self *SHost) DoCreateVM(ctx context.Context, ds *SDatastore, params SCreat
 	spec.DeviceChange = deviceChange
 	dc, err := self.GetDatacenter()
 	if err != nil {
-		return nil, errors.Wrapf(err, "SHost.GetDatacenter for host '%s'", self.GetId())
+		err = errors.Wrapf(err, "SHost.GetDatacenter for host '%s'", self.GetId())
+		return
 	}
 	// get vmFloder
 	folders, err := dc.getObjectDatacenter().Folders(ctx)
 	if err != nil {
-		return nil, errors.Wrap(err, "object.DataCenter.Folders")
+		err = errors.Wrap(err, "object.DataCenter.Folders")
+		return
 	}
 	vmFolder := folders.VmFolder
 	resourcePool, err := self.SyncResourcePool(params.ResourcePool)
 	if err != nil {
-		return nil, errors.Wrap(err, "SyncResourcePool")
+		err = errors.Wrap(err, "SyncResourcePool")
+		return
 	}
 	task, err := vmFolder.CreateVM(ctx, spec, resourcePool, self.GetoHostSystem())
 	if err != nil {
-		return nil, errors.Wrap(err, "VmFolder.Create")
+		err = errors.Wrap(err, "VmFolder.Create")
+		return
 	}
 
 	info, err := task.WaitForResult(ctx, nil)
 	if err != nil {
-		return nil, errors.Wrap(err, "Task.WaitForResult")
+		err = errors.Wrap(err, "Task.WaitForResult")
+		return
 	}
-
-	deviceChange = make([]types.BaseVirtualDeviceConfigSpec, 0, 1)
-	// add disks
-	var rootDiskSizeMb int64
-	for _, disk := range disks {
-		imagePath := disk.ImagePath
-		var size = disk.Size
-		if len(imagePath) == 0 {
-			if size == 0 {
-				size = 30 * 1024
-			}
-		} else {
-			imagePath, err = self.FileUrlPathToDsPath(imagePath)
-			if err != nil {
-				return nil, errors.Wrapf(err, "SHost.FileUrlPathToDsPath")
-			}
-			newImagePath := fmt.Sprintf("[%s] %s/%s.vmdk", ds.GetRelName(), params.Uuid, params.Uuid)
-			fm := ds.getDatastoreObj().NewFileManager(dc.getObjectDatacenter(), true)
-			err := fm.Copy(ctx, imagePath, newImagePath)
-			if err != nil {
-				return nil, errors.Wrap(err, "unable to copy system disk")
-			}
-			imagePath = newImagePath
-			rootDiskSizeMb = size
-		}
-		uuid, driver := disk.DiskId, "scsi"
-		if len(disk.Driver) > 0 {
-			driver = disk.Driver
-		}
-		if driver == "scsi" || driver == "pvscsi" {
-			if self.isVersion50() {
-				driver = "scsi"
-			}
-			ctrlKey = 1000
-			index = scsiIdx
-			scsiIdx += 1
-			if scsiIdx == 7 {
-				scsiIdx++
-			}
-		} else {
-			ideno := ideIdx % 2
-			if ideno == 0 {
-				index = ideIdx/2 + ide1un
-			} else {
-				index = ideIdx/2 + ide2un
-			}
-			ctrlKey = 200 + ideno
-			ideIdx += 1
-		}
-		log.Debugf("size: %d, image path: %s, uuid: %s, index: %d, ctrlKey: %d, driver: %s.", size, imagePath, uuid,
-			index, ctrlKey, disk.Driver)
-		spec := addDevSpec(NewDiskDev(size, imagePath, uuid, int32(index), 2000, int32(ctrlKey), 0))
-		if len(imagePath) == 0 {
-			spec.FileOperation = "create"
-		}
-		deviceChange = append(deviceChange, spec)
-	}
-
-	configSpec := types.VirtualMachineConfigSpec{}
-	configSpec.DeviceChange = deviceChange
-
 	vmRef := info.Result.(types.ManagedObjectReference)
 	objectVM := object.NewVirtualMachine(self.manager.client.Client, vmRef)
-	task, err = objectVM.Reconfigure(ctx, configSpec)
-	if err != nil {
-		return nil, errors.Wrap(err, "unable to reconfigure")
-	}
-	err = task.Wait(ctx)
-	if err != nil {
-		return nil, errors.Wrap(err, "task.Wait")
-	}
-
-	var moVM mo.VirtualMachine
-	err = self.manager.reference2Object(vmRef, VIRTUAL_MACHINE_PROPS, &moVM)
-	if err != nil {
-		return nil, errors.Wrap(err, "fail to fetch virtual machine just created")
-	}
-
-	evm := NewVirtualMachine(self.manager, &moVM, self.datacenter)
-	if evm == nil {
-		return nil, errors.Error("create successfully but unable to NewVirtualMachine")
-	}
-
-	// resize root disk
-	if rootDiskSizeMb > 0 && int64(evm.vdisks[0].GetDiskSizeMB()) != rootDiskSizeMb {
-		err = evm.vdisks[0].Resize(ctx, rootDiskSizeMb)
-		if err != nil {
-			return evm, errors.Wrap(err, "resize for root disk")
-		}
-	}
-	return evm, nil
+	vm, err = self.addDisks(ctx, dc, ds, params.Disks, params.Uuid, objectVM)
+	return
 }
 
 // If snapshot is not nil, params.Disks will be ignored

pkg/multicloud/esxi/virtualmachine.go

@@ -188,15 +188,23 @@ func (self *SVirtualMachine) DoRebuildRoot(ctx context.Context, imagePath string
 func (self *SVirtualMachine) rebuildDisk(ctx context.Context, disk *SVirtualDisk, imagePath string) error {
 	uuid := disk.GetId()
 	sizeMb := disk.GetDiskSizeMB()
-	index := disk.index
 	diskKey := disk.getKey()
 	ctlKey := disk.getControllerKey()
+	unitNumber := *disk.dev.GetVirtualDevice().UnitNumber
 
 	err := self.doDetachAndDeleteDisk(ctx, disk)
 	if err != nil {
 		return err
 	}
-	return self.createDiskInternal(ctx, sizeMb, uuid, int32(index), diskKey, ctlKey, imagePath, false)
+	return self.createDiskInternal(ctx, SDiskConfig{
+		SizeMb:        int64(sizeMb),
+		Uuid:          uuid,
+		ControllerKey: ctlKey,
+		UnitNumber:    unitNumber,
+		Key:           diskKey,
+		ImagePath:     imagePath,
+		IsRoot:        len(imagePath) > 0,
+	}, false)
 }
 
 func (self *SVirtualMachine) UpdateVM(ctx context.Context, name string) error {
@@ -918,7 +926,13 @@ func (self *SVirtualMachine) CreateDisk(ctx context.Context, sizeMb int, uuid st
 		unitNumber++
 	}
 
-	return self.createDiskInternal(ctx, sizeMb, uuid, int32(unitNumber), diskKey, ctrlKey, "", true)
+	return self.createDiskInternal(ctx, SDiskConfig{
+		SizeMb:        int64(sizeMb),
+		Uuid:          uuid,
+		UnitNumber:    int32(unitNumber),
+		ControllerKey: ctrlKey,
+		Key:           diskKey,
+	}, true)
 }
 
 // createDriverAndDisk will create a driver and disk associated with the driver
@@ -941,10 +955,19 @@ func (self *SVirtualMachine) createDriverAndDisk(ctx context.Context, sizeMb int
 		log.Errorf("there is no suitable key between 1000 and 2000???!")
 	}
 
-	return self.createDiskWithDeviceChange(ctx, deviceChange, sizeMb, uuid, 0, diskKey, scsiKey, "", true)
+	return self.createDiskWithDeviceChange(ctx, deviceChange,
+		SDiskConfig{
+			SizeMb:        int64(sizeMb),
+			Uuid:          uuid,
+			ControllerKey: scsiKey,
+			UnitNumber:    0,
+			Key:           scsiKey,
+			ImagePath:     "",
+			IsRoot:        false,
+		}, true)
 }
 
-func (self *SVirtualMachine) copyRootDisk(ctx context.Context, imagePath string, index int) (string, error) {
+func (self *SVirtualMachine) copyRootDisk(ctx context.Context, imagePath string) (string, error) {
 	movm := self.getVirtualMachine()
 	if movm.LayoutEx == nil || len(movm.LayoutEx.File) == 0 {
 		return "", fmt.Errorf("invalid LayoutEx")
@@ -969,7 +992,8 @@ func (self *SVirtualMachine) copyRootDisk(ctx context.Context, imagePath string,
 	}
 	path := datastore.cleanPath(file)
 	vmDir := strings.Split(path, "/")[0]
-	newImagePath := datastore.getPathString(fmt.Sprintf("%s/%s-%d.vmdk", vmDir, vmDir, index))
+	// TODO find a non-conflicting path
+	newImagePath := datastore.getPathString(fmt.Sprintf("%s/%s.vmdk", vmDir, vmDir))
 
 	fm := datastore.getDatastoreObj().NewFileManager(datastore.datacenter.getObjectDatacenter(), true)
 	err = fm.Copy(ctx, imagePath, newImagePath)
@@ -979,22 +1003,20 @@ func (self *SVirtualMachine) copyRootDisk(ctx context.Context, imagePath string,
 	return newImagePath, nil
 }
 
-func (self *SVirtualMachine) createDiskWithDeviceChange(ctx context.Context,
-	deviceChange []types.BaseVirtualDeviceConfigSpec, sizeMb int,
-	uuid string, index int32, diskKey int32, ctlKey int32, imagePath string, check bool) error {
-
+func (self *SVirtualMachine) createDiskWithDeviceChange(ctx context.Context, deviceChange []types.BaseVirtualDeviceConfigSpec, config SDiskConfig, check bool) error {
 	var err error
 	// copy disk
-	if len(imagePath) > 0 {
-		imagePath, err = self.copyRootDisk(ctx, imagePath, int(index))
+	if len(config.ImagePath) > 0 {
+		config.IsRoot = true
+		config.ImagePath, err = self.copyRootDisk(ctx, config.ImagePath)
 		if err != nil {
 			return errors.Wrap(err, "unable to copyRootDisk")
 		}
 	}
 
-	devSpec := NewDiskDev(int64(sizeMb), imagePath, uuid, index, 0, ctlKey, diskKey)
+	devSpec := NewDiskDev(int64(config.SizeMb), config)
 	spec := addDevSpec(devSpec)
-	if len(imagePath) == 0 {
+	if len(config.ImagePath) == 0 {
 		spec.FileOperation = types.VirtualDeviceConfigSpecFileOperationCreate
 	}
 	configSpec := types.VirtualMachineConfigSpec{}
@@ -1025,10 +1047,9 @@ func (self *SVirtualMachine) createDiskWithDeviceChange(ctx context.Context,
 	return cloudprovider.ErrTimeout
 }
 
-func (self *SVirtualMachine) createDiskInternal(ctx context.Context, sizeMb int, uuid string, index int32,
-	diskKey int32, ctlKey int32, imagePath string, check bool) error {
+func (self *SVirtualMachine) createDiskInternal(ctx context.Context, config SDiskConfig, check bool) error {
 
-	return self.createDiskWithDeviceChange(ctx, nil, sizeMb, uuid, index, diskKey, ctlKey, imagePath, check)
+	return self.createDiskWithDeviceChange(ctx, nil, config, check)
 }
 
 func (self *SVirtualMachine) Renew(bc billing.SBillingCycle) error {