vSphere Refactoring code - datastore object #158

divyenpatel · 2017-06-14T06:58:26Z

This PR is part of refactoring changes for datastore object.

It includes volume creation and volume deletion methods. Implemented factory pattern and kept the code for each approach in separate files.

@luomiao @rohitjogvmw @tusharnt @BaluDontu

rjog · 2017-06-14T18:42:53Z

pkg/cloudprovider/providers/vsphere/vclib/datastore.go

+}
+
+// IsVSANDatastore checks the Datastore object is of the type VSAN
+func (ds Datastore) IsVSANDatastore(ctx context.Context) (bool, error) {


IMHO, instead of having IsVSANDatastore lets change it to GetType and consumers can use it like

if (Datastore.GetTyoe() == VSANDatastore) {
.
.
.
}

In future I am sure Datastore. GetType() will be required.

changed it to

// GetType returns the type of datastore func (ds *Datastore) GetType(ctx context.Context) (string, error) {

rjog · 2017-06-14T21:12:55Z

pkg/cloudprovider/providers/vsphere/vclib/datastore.go

+}
+
+// Disk defines interfaces for creating disk
+type Disk interface {


IMHO, Disk APIs needs to be in Disk file and not in datastore.
Datastore should be a parameter to create.
For delete VolumeID / Path is sufficient.

Created new Object VirtualDisk and moved methods there

type VirtualDisk struct { DiskPath string VolumeOptions VolumeOptions VMOptions VMOptions }

func (disk *VirtualDisk) CreateVirtualDisk(ctx context.Context, datastore *Datastore) (err error) {

func (disk *VirtualDisk) DeleteVolume(ctx context.Context, datastore *Datastore) error {

divyenpatel · 2017-06-14T22:35:54Z

@rohitjogvmw Addressed review comments.

rjog · 2017-06-14T23:14:46Z

pkg/cloudprovider/providers/vsphere/vclib/virtualdisk.go

+}
+
+// GetDiskManager returns vmDiskManager or vdmDiskManager based on given volumeoptions
+func GetDiskManager(disk *VirtualDisk) Disk {


I think if you pass operation along with virtualDisk then for delete also you can make use of the factory.
IMHO below parameters in factory with dummy implementation will be extensible..

func GetDiskManager(disk *VirtualDisk, op VirtualDiskOperation) Disk {
if op == "create" {
// your current code
} else if op == "delete" {
// currently just return VDM but very soon VStorageObjectManager needs to be returned based on disk.diskId
}
}

updated as below

// GetDiskManager returns vmDiskManager or vdmDiskManager based on given volumeoptions func GetDiskManager(disk *VirtualDisk, diskOperation string) Disk { var diskManager Disk switch diskOperation { case VirtualDeleteOperation: diskManager = vdmDiskManager{disk.DiskPath, disk.VolumeOptions} case VirtualDiskCreateOperation: if disk.VolumeOptions.StoragePolicyName != "" || disk.VolumeOptions.VSANStorageProfileData != "" || disk.VolumeOptions.StoragePolicyID != "" { diskManager = vmDiskManager{disk.DiskPath, disk.VolumeOptions, disk.VMOptions} } else { diskManager = vdmDiskManager{disk.DiskPath, disk.VolumeOptions} } } return diskManager }

rjog · 2017-06-14T23:20:03Z

pkg/cloudprovider/providers/vsphere/vclib/virtualdisk.go

+// DeleteVolume deletes a disk at given disk path.
+func (disk *VirtualDisk) DeleteVolume(ctx context.Context, datastore *Datastore) error {
+	// Create a virtual disk manager
+	virtualDiskManager := object.NewVirtualDiskManager(datastore.Client())


I will suggest the diskManager from the factory and implement delete only in vdm.go

moved to vdm.go

rjog · 2017-06-14T23:27:29Z

pkg/cloudprovider/providers/vsphere/vclib/vdmDiskManager.go

+	"github.com/vmware/govmomi/vim25/types"
+)
+
+type vdmDiskManager struct {


vdm itself stands for VirtualDiskManager.
Can I suggest 2 things:

Put all disk managers in vclib/diskManagers

Define one interface for diskManager havig APIs like create, delete etc. and vdm and vmDiskManager will implement them or throw exception if operation is not supported. e.g delete is not supported in vmDiskManager

rename vdmDiskManager.go to vdm.go

created diskmanagers package under vclib and moved files there.

divyenp-m01:diskmanagers divyenp$ ls vdm.go virtualdisk.go vmdm.go

rjog · 2017-06-14T23:39:56Z

pkg/cloudprovider/providers/vsphere/vclib/vmDiskManager.go

+			ExtensionKey: "com.vmware.vim.sps",
+			ObjectData:   vmdisk.volumeOptions.VSANStorageProfileData,
+		}
+	}


please return err in else.
if storagePolicyId != "" {
} else if VSANStorageProfileData != "" {
} else {
return err;
}

VM based disk creation need to be used if no policy / vsan policy data is not provided

divyenpatel · 2017-06-15T06:07:03Z

@rohitjogvmw addressed 2nd round of review comments.

rjog · 2017-06-15T17:32:38Z

pkg/cloudprovider/providers/vsphere/vclib/diskmanagers/virtualdisk.go

+)
+
+// Disk defines interfaces for creating disk
+type Disk interface {


Shouldn't it be VirtualDiskProvider and not Disk?

changed Disk to VirtualDiskProvider

rjog · 2017-06-15T17:34:34Z

pkg/cloudprovider/providers/vsphere/vclib/diskmanagers/vdm.go

+	"k8s.io/kubernetes/pkg/cloudprovider/providers/vsphere/vclib"
+)
+
+type vdmDiskManager struct {


I still think VirtualDiskManager is better than vdmDiskManager.
vdm itself stands for VirtualDiskManager. so vdmDiskManager will be read as "VirtualDiskManagerDiskManager" isn't it?

Yes. let me change it to virtualDiskManager.
will be read as diskManager virtualDiskManager

rjog · 2017-06-15T17:40:40Z

pkg/cloudprovider/providers/vsphere/vclib/datastore.go

+
+// GetType returns the type of datastore
+func (ds *Datastore) GetType(ctx context.Context) (string, error) {
+	var mds mo.Datastore


what is mds?
Is there any particular convention being followed for MOs?

IMHO mds is not good for reading instead "dsMo" tells exactly what it holds.
But are we using Mo suffix for all MOs in code? I don't think so. We need to stick to some convention.
At other places I see name of that object with no "Mo" suffix (e.g. VirtualDiskManager).
If so please change input parameter of this function from "ds" to "dsRef" and use ds instead of mds.

Other alternatives are welcome.

what is mds?
Is there any particular convention being followed for MOs?

Govmomi follows this to create instance of Datastore struct declared under package mo.
Used this to differentiate variable from object.Datastore and mo.Datastore

rjog

LGTM

luomiao

Updated:
General feedback is, I am not sure if we should use this VirtualDiskProvider interface for vdm and vmdm... Maybe my following understanding is wrong but still want to raise the concern.

The whole point of using interface is generalization. We create this interface X and there are two instances A and B. At some point, we initialize A or B and return it back as an instance of interface X. Later the upper layer just need to call X.something and it will land in the desired functions A.something or B.something automatically.

However in our case, for EVERY disk operation, we need to call GetDiskManager to create an instance of VirtualDiskProvider. And after this, it goes into vdm or vmdm's function, and it's done... The instances are never reused... This seems lost the whole point of using interface. If anyway we need to do this check every time, why not just make a check inside a Create function, and then add two sub functions? Using interface here is just a overkill (or not the right use case...)

luomiao · 2017-06-15T21:47:31Z

pkg/cloudprovider/providers/vsphere/vclib/vmoptions.go

+	"github.com/vmware/govmomi/object"
+)
+
+// VMOptions provides helper objects for provisioning volume with SPBM Policy


Do we plan to add more functions to it?
If not, why not directly put the following three fields directly into vmDiskManager and VirtualDisk？

luomiao · 2017-06-15T21:51:31Z

pkg/cloudprovider/providers/vsphere/vclib/diskmanagers/vdm.go

+
+type virtualDiskManager struct {
+	diskPath      string
+	volumeOptions vclib.VolumeOptions


Capitalizing? DiskPath, VolumeOptions...

since virtualDiskManager and vmDiskManager are private and just implementing VirtualDiskProvider, so kept it private.

Data can be accessed using

VirtualDisk.DiskPath VirtualDisk.VolumeOptions VirtualDisk.VMOptions

luomiao · 2017-06-15T21:52:12Z

pkg/cloudprovider/providers/vsphere/vclib/diskmanagers/vmdm.go

+	"k8s.io/kubernetes/pkg/cloudprovider/providers/vsphere/vclib"
+)
+
+type vmDiskManager struct {


Capitalizing again. DiskPath, VolumeOptions, VmOptions...

luomiao · 2017-06-15T22:10:10Z

pkg/cloudprovider/providers/vsphere/vclib/diskmanagers/vdm.go

+	"github.com/vmware/govmomi/vim25/types"
+	"k8s.io/kubernetes/pkg/cloudprovider/providers/vsphere/vclib"
+)
+


There is vdm and vmdm. Maybe add one line description about them at the beginning to help ppl understand what's the difference.

Added one line description for each.

// virtualDiskManager implements VirtualDiskProvider Interface for creating and deleting volume using VirtualDiskManager

// vmDiskManager implements VirtualDiskProvider interface for creating volume using Virtual Machine Reconfigure approach

divyenpatel · 2017-06-15T23:52:07Z

Addressing @luomiao 's review comments.
Will keep getDiskManager private
func getDiskManager(disk *VirtualDisk, diskOperation string) VirtualDiskProvider {

and VirtualDisk will now internally get DiskManager, instead of exposing to the cloud provider.

func (virtualDisk *VirtualDisk) Create(ctx context.Context, datastore *vclib.Datastore) error {
	return getDiskManager(virtualDisk,VirtualDiskCreateOperation).Create(ctx, datastore)
}

func (virtualDisk *VirtualDisk) Delete(ctx context.Context, datastore *vclib.Datastore) error {
	return getDiskManager(virtualDisk,VirtualDeleteOperation).Delete(ctx, datastore)
}

divyenpatel · 2017-06-16T00:56:09Z

@luomiao Addressed review comments.

luomiao · 2017-06-16T00:57:35Z

LGTM

* adding VCLib package - Datastore object * adding vmoptions.go * addressed Rohit's review comments * addressed rohit's review comments * addressed 3rd round of review comments * addressed Miao's review comments

divyenpatel added 2 commits June 13, 2017 23:51

adding VCLib package - Datastore object

8935b7d

adding vmoptions.go

8c76970

rjog reviewed Jun 14, 2017

View reviewed changes

addressed Rohit's review comments

483160d

divyenpatel mentioned this pull request Jun 14, 2017

vSphere Refactoring code - datastore object #163

Closed

rjog reviewed Jun 14, 2017

View reviewed changes

addressed rohit's review comments

cd259b9

rjog reviewed Jun 15, 2017

View reviewed changes

addressed 3rd round of review comments

d00f401

rjog approved these changes Jun 15, 2017

View reviewed changes

luomiao reviewed Jun 15, 2017

View reviewed changes

addressed Miao's review comments

7343fc7

divyenpatel merged commit 3f6b55a into refactored-code Jun 16, 2017

divyenpatel mentioned this pull request Aug 2, 2017

VSphere cloud provider code refactoring kubernetes/kubernetes#49164

Merged

divyenpatel deleted the divyen-refactoring-datastore branch September 5, 2017 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vSphere Refactoring code - datastore object #158

vSphere Refactoring code - datastore object #158

divyenpatel commented Jun 14, 2017

rjog Jun 14, 2017

divyenpatel Jun 14, 2017

rjog Jun 14, 2017

divyenpatel Jun 14, 2017

divyenpatel commented Jun 14, 2017

rjog Jun 14, 2017

divyenpatel Jun 15, 2017

rjog Jun 14, 2017

divyenpatel Jun 15, 2017

rjog Jun 14, 2017

divyenpatel Jun 15, 2017

rjog Jun 14, 2017

divyenpatel Jun 15, 2017

divyenpatel commented Jun 15, 2017

rjog Jun 15, 2017

divyenpatel Jun 15, 2017

rjog Jun 15, 2017

divyenpatel Jun 15, 2017

rjog Jun 15, 2017

divyenpatel Jun 15, 2017

rjog left a comment

luomiao left a comment •

edited

Loading

luomiao Jun 15, 2017

luomiao Jun 15, 2017

divyenpatel Jun 16, 2017

luomiao Jun 15, 2017

luomiao Jun 15, 2017

divyenpatel Jun 16, 2017

divyenpatel commented Jun 15, 2017

divyenpatel commented Jun 16, 2017

luomiao commented Jun 16, 2017

vSphere Refactoring code - datastore object #158

vSphere Refactoring code - datastore object #158

Conversation

divyenpatel commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divyenpatel commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divyenpatel commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjog left a comment

Choose a reason for hiding this comment

luomiao left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divyenpatel commented Jun 15, 2017

divyenpatel commented Jun 16, 2017

luomiao commented Jun 16, 2017

luomiao left a comment •

edited

Loading