SPBM policy integration for dynamic volume provisioning inside kubernetes #142

BaluDontu · 2017-05-11T02:21:32Z

Till now, vSphere CP provides support to configure persistent volume with VSAN storage capabilities - kubernetes#42974. Right now this only works with VSAN.

Also there might be other use cases:

But the user might need a way to configure a policy on other datastores like VMFS, NFS etc.
How to use SIOC, VMCrypt policies for a persistent disk.

We can achieve about 2 use cases by using existing storage policies which are already created on vCenter. The user will specify the SPBM policy ID as part of dynamic provisioning and volume will have the policy configured with it.

This feature will allow you to specify the SPBM policy ID as part of dynamic volume provisioning. The created persistent volume will have the SPBM policy ID associated with it when u create a PVC.

For example,

kind: StorageClass
apiVersion: storage.k8s.io/v1beta1
metadata:
  name: fast
provisioner: kubernetes.io/vsphere-volume
parameters:
    diskformat: zeroedthick
    storagepolicyName: policy1
    datastore: VMFSDatastore

When you deploy a pod with the PVC referring to the above mentioned storageclass, you will see the volume association on the vCenter for the node where the volume is attached to.

divyenpatel · 2017-05-11T18:38:07Z

pkg/volume/vsphere_volume/vsphere_volume_util.go

@@ -101,39 +110,50 @@ func (util *VsphereDiskUtil) CreateVolume(v *vsphereVolumeProvisioner) (vmDiskPa
 		case datastore:
 			volumeOptions.Datastore = value
 		case Fstype:
-			fstype = value
+			fstype := value


remove :=. Change this to
fstype = value

Good observation. I will fix it in next commit.

divyenpatel · 2017-05-11T18:47:22Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+		deviceConfigSpec.Profile = append(deviceConfigSpec.Profile, profileSpec)
+	}
+	virtualMachineConfigSpec.DeviceChange = append(virtualMachineConfigSpec.DeviceChange, deviceConfigSpec)
+	task, err := vm.Reconfigure(ctx, virtualMachineConfigSpec)


Do we want add check for nil task to avoid NPE? we have observed nil task when user does not have read permission.

Not needed i guess.

SandeepPissay · 2017-05-13T00:25:44Z

pkg/cloudprovider/providers/vsphere/vsphere.go

@@ -1266,19 +1282,43 @@ func (vs *VSphere) CreateVolume(volumeOptions *VolumeOptions) (volumePath string
 	dc, err := f.Datacenter(ctx, vs.cfg.Global.Datacenter)
 	f.SetDatacenter(dc)

+	if volumeOptions.StoragePolicyName != "" {


Shouldn't you fail early if storagePolicyName and VSANStorageProfileData are set? If we are already doing it, can you point to the code where we fail early?

You can see that code in vspherE_volume_util.go file - 6d41087#diff-482fa05b285b59ab0bc70641f984ce98R131

SandeepPissay · 2017-05-13T00:27:02Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+
+		if volumeOptions.Datastore != "" {
+			if !IsUserSpecifiedDatastoreCompatible(dsRefs, volumeOptions.Datastore) {
+				return "", fmt.Errorf("User specified datastore: %q is not compatible with the StoragePolicy: %q requirements", volumeOptions.Datastore, volumeOptions.StoragePolicyName)


Don't we want to say why it is not compatible? SPBM gives reasons why a particular datastore is incompatible.

Yes, I have made sure it shows messages why a datastore is incompatible.

Can you paste an example of what message the k8s user would see? SPBM does return CompatibilityResult.error that says what are the expected capabilities and what the storage can offer (actual capability). Do we show this to k8s user?

SandeepPissay · 2017-05-13T00:38:36Z

pkg/cloudprovider/providers/vsphere/vsphere_util.go

+	compatibleHubs := res.CompatibleDatastores()
+	// Return an error if there are no compatible datastores.
+	if len(compatibleHubs) < 1 {
+		return nil, fmt.Errorf("There are no compatible datastores: %+v that satisfy the storage policy: %+q requirements", datastores, storagePolicyID)


Returning 0 compatible datastores with nil error is a perfectly fine behavior for this method. The caller can decide if it was expecting 0 or 1 or 2 or more etc. Based on the caller's requirements, it can treat it as error or perfectly fine condition.

Makes sense. Moved it to the original caller function.

SandeepPissay · 2017-05-13T00:41:20Z

pkg/cloudprovider/providers/vsphere/vsphere_util.go

+}
+
+// Get the datastore morefs.
+func (vs *VSphere) getDatastoreMorefs(ctx context.Context, dsRefs []types.ManagedObjectReference) ([]mo.Datastore, error) {


Rename this method to getDatastoreMo since you are returning the Datastore manaject objects here and not really a managed object references. You may want to consider renaming the local variables in this method for the same reasons.

SandeepPissay · 2017-05-13T00:42:47Z

pkg/cloudprovider/providers/vsphere/vsphere_util.go

+}
+
+// Verify if the user specified datastore is in the list of compatible datastores.
+func IsUserSpecifiedDatastoreCompatible(dsRefs []mo.Datastore, dsName string) bool {


Rename the function according to the implemented logic. This method seems to be just checking if the given dsName is in the given Datastore managed object array. The method has nothing to do with compatibility or incompatibility checks.

Restructured the code slightly here to make sure IsUserSpecifiedDatastoreCompatible() does what it needs to do.

SandeepPissay · 2017-05-13T00:45:15Z

pkg/cloudprovider/providers/vsphere/vsphere_util.go

+}
+
+// Get the best fit compatible datastore by free space.
+func GetBestFitCompatibleDatastore(dsRefs []mo.Datastore) string {


Rename to GetMostFreeDatastore and return the Datastore managed object. Also I think we should not use Refs here as these are not managed object references, instead they are managed objects. I see this misnaming used a lot in your change.

I have renamed it all places.

SandeepPissay · 2017-05-13T00:48:26Z

pkg/cloudprovider/providers/vsphere/vsphere_util.go

+		return nil, err
+	}
+
+	// The K8s cluster might be deployed inside a cluster or a host.


I guess we cannot assume this. The k8s cluster could be created in a child resource pool that may be within one or many resource pool/child resource pools.

Removed this logic altogether.

SandeepPissay · 2017-05-13T00:50:30Z

pkg/cloudprovider/providers/vsphere/vsphere_util.go

+}
+
+// Get all datastores accessible inside the current Kubernetes cluster.
+func (vs *VSphere) getAllAccessibleDatastoresForK8sCluster(ctx context.Context, resourcePool *object.ResourcePool) ([]types.ManagedObjectReference, error) {


You are assuming that all the datastores within the cluster are accessible to all k8s nodes. This may not be true! You will also have to check if the ESX hosts running the k8s nodes have access to the datastores.

Yes. You are correct! I have retrieved datastores from the esx host.

SandeepPissay · 2017-05-13T01:05:26Z

examples/volumes/vsphere/README.md

@@ -374,7 +376,47 @@
      pvpod       1/1     Running   0          48m
      ```

-### Virtual SAN policy support inside Kubernetes
+### Storage Policy Management inside kubernetes
+#### Using existing vCenter SPBM policy


As we discussed there are some known issues with the user specifying the policy name instead of policy ID, and I think we should document this so that the VI admin/k8s admin/k8s user knows about it and how to deal with it. I'm just listing down our discussion points

*** Admin updating the policy name in vCenter could cause confusions/inconsistencies ***
This happens when VI admin changes the policy name without the knowledge of k8s users or letting the k8s users know about it. The k8s users will not be able to create volume with storage class that uses old policy name. k8s user and VI admin has to resolve such issues by finding out the latest policy name and updating the existing storage classes with latest policy name.

*** Two or more PVs could show different policy names but with the same policy ID ***
The k8s user can see the policy name and policy ID while describing PVs. If a storage policy name in vCenter is updated, then this is not reflected in the policy names shown for the existing PVs (the policy ID will be shown correctly). The k8s user has to update the storage class to use the latest policy name, and any PVs created after this point will show the correct policy name and policy ID. However, if the old PVs and new PVs are described, the old PVs will continue to show the old policy name which could cause confusions.

True. This would not be the right place to describe these known issues. When we announce a release with this feature that's when we would have this information written up.

So, how are you tracking that these known issues are not lost?

BaluDontu

Addressed sandeep's comments.

BaluDontu · 2017-05-15T18:17:27Z

examples/volumes/vsphere/README.md

@@ -374,7 +376,47 @@
      pvpod       1/1     Running   0          48m
      ```

-### Virtual SAN policy support inside Kubernetes
+### Storage Policy Management inside kubernetes
+#### Using existing vCenter SPBM policy


True. This would not be the right place to describe these known issues. When we announce a release with this feature that's when we would have this information written up.

BaluDontu · 2017-05-15T18:19:01Z

pkg/cloudprovider/providers/vsphere/vsphere.go

@@ -1266,19 +1282,43 @@ func (vs *VSphere) CreateVolume(volumeOptions *VolumeOptions) (volumePath string
 	dc, err := f.Datacenter(ctx, vs.cfg.Global.Datacenter)
 	f.SetDatacenter(dc)

+	if volumeOptions.StoragePolicyName != "" {


You can see that code in vspherE_volume_util.go file - 6d41087#diff-482fa05b285b59ab0bc70641f984ce98R131

BaluDontu · 2017-05-15T18:32:35Z

pkg/cloudprovider/providers/vsphere/vsphere_util.go

+	compatibleHubs := res.CompatibleDatastores()
+	// Return an error if there are no compatible datastores.
+	if len(compatibleHubs) < 1 {
+		return nil, fmt.Errorf("There are no compatible datastores: %+v that satisfy the storage policy: %+q requirements", datastores, storagePolicyID)


Makes sense. Moved it to the original caller function.

BaluDontu · 2017-05-15T18:33:37Z

pkg/cloudprovider/providers/vsphere/vsphere_util.go

+}
+
+// Get the best fit compatible datastore by free space.
+func GetBestFitCompatibleDatastore(dsRefs []mo.Datastore) string {


I have renamed it all places.

BaluDontu · 2017-05-15T18:35:41Z

pkg/cloudprovider/providers/vsphere/vsphere_util.go

+}
+
+// Get the datastore morefs.
+func (vs *VSphere) getDatastoreMorefs(ctx context.Context, dsRefs []types.ManagedObjectReference) ([]mo.Datastore, error) {


BaluDontu · 2017-05-16T21:49:24Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+
+		if volumeOptions.Datastore != "" {
+			if !IsUserSpecifiedDatastoreCompatible(dsRefs, volumeOptions.Datastore) {
+				return "", fmt.Errorf("User specified datastore: %q is not compatible with the StoragePolicy: %q requirements", volumeOptions.Datastore, volumeOptions.StoragePolicyName)


Yes, I have made sure it shows messages why a datastore is incompatible.

BaluDontu · 2017-05-16T21:50:28Z

pkg/cloudprovider/providers/vsphere/vsphere_util.go

+}
+
+// Verify if the user specified datastore is in the list of compatible datastores.
+func IsUserSpecifiedDatastoreCompatible(dsRefs []mo.Datastore, dsName string) bool {


Restructured the code slightly here to make sure IsUserSpecifiedDatastoreCompatible() does what it needs to do.

BaluDontu · 2017-05-16T21:51:17Z

pkg/cloudprovider/providers/vsphere/vsphere_util.go

+}
+
+// Get all datastores accessible inside the current Kubernetes cluster.
+func (vs *VSphere) getAllAccessibleDatastoresForK8sCluster(ctx context.Context, resourcePool *object.ResourcePool) ([]types.ManagedObjectReference, error) {


Yes. You are correct! I have retrieved datastores from the esx host.

BaluDontu · 2017-05-16T21:51:30Z

pkg/cloudprovider/providers/vsphere/vsphere_util.go

+		return nil, err
+	}
+
+	// The K8s cluster might be deployed inside a cluster or a host.


Removed this logic altogether.

SandeepPissay · 2017-05-19T17:51:01Z

pkg/cloudprovider/providers/vsphere/vsphere_util.go

+	if err != nil {
+		return nil, err
+	}
+	dsMorefs := make(map[int][]types.ManagedObjectReference)


Comment on line#210-219

This entire block can be written in a simple and efficient way. There is no need to create multiple maps to arrive at the shared datastore list and also I found it a bit complex to understand the logic. Here's how we can write this block (pseudo code only):

Set sharedDs;
for (i = 0; i < vmList.size(); i++) {
Set accessibleDs = getAccesibleDatastoresForNode(vm.get(i))
if (i == 0) {
sharedDs.addAll(accesibleDs);
} else {
sharedDs = intersect(sharedDs, accessibleDs);
if (sharedDs.size() == 0) {
break;
}
}
}

sharedDs intersect(sharedDs, accessibleDs) {
for ds : sharedDs {
if (accessibleDs.get(ds) == null) {
sharedDs.remove(ds)
}
}
return sharedDs
}

SandeepPissay · 2017-05-19T18:36:03Z

pkg/cloudprovider/providers/vsphere/vsphere.go

@@ -1535,20 +1533,27 @@ func (vs *VSphere) cleanUpDummyVMs(dummyVMPrefix string) {
 		f.SetDatacenter(dc)

 		// Get the folder reference for global working directory where the dummy VM needs to be created.
-		vmFolder, err := getFolder(ctx, vs.client, vs.cfg.Global.Datacenter, vs.cfg.Global.WorkingDir)
+		vmFolder, err := f.Folder(ctx, strings.TrimSuffix(vs.cfg.Global.WorkingDir, "/"))


I did not quite get why you had to modify this method. Can you explain what is this method doing, when it is invoked and why this needs change?

This is just to get folder. I don't need a function, I can get folder reference from working directory path.

SandeepPissay · 2017-05-20T20:06:17Z

pkg/cloudprovider/providers/vsphere/vsphere_util.go

+	var sharedList []string
+	for _, val1 := range list1 {
+		// Check if val1 is found in list2
+		for _, val2 := range list2 {


This search will be expensive if the number of entries are a lot but may not be so inefficient if the number of entries are less. I was expecting you to use "map" since "set" data structure is not there in go std library. But the logic seems correct, see if you can use map instead.

Since the datastores will be very minimum for a VM, this logic would be not so expensive.

SandeepPissay · 2017-05-20T20:13:49Z

The code changes looks ok to me. I dont see any "testing done" mentioned in this review. Could you explain what tests have you done on the latest code to verify that it works fine?

BaluDontu · 2017-05-20T20:27:39Z

SandeepPissay · 2017-05-21T04:52:05Z

LGTM

BaluDontu · 2017-06-08T06:56:34Z

The PR is already merged on Kubernetes - kubernetes#46176

BaluDontu added 3 commits May 10, 2017 18:08

PBM govmomi dependencies

f74e27e

Kubernetes core API changes for vSphere

5bee771

Open API and swagger spec changes

28a1071

divyenpatel reviewed May 11, 2017

View reviewed changes

BaluDontu mentioned this pull request May 11, 2017

vCenter SPBM policy integration for dynamic volume provisioning inside kubernetes #131

Closed

BaluDontu force-pushed the SPBMPolicySupportVsphere branch from af26171 to 6d41087 Compare May 12, 2017 23:08

SPBM policy ID support in vsphere cloud provider

6d41087

SandeepPissay reviewed May 13, 2017

View reviewed changes

Addresses review comments from team

82f5736

BaluDontu commented May 16, 2017

View reviewed changes

Find a shared datastore for all my kubernetes cluster VM's

ccbf878

SandeepPissay reviewed May 19, 2017

View reviewed changes

Get shared datastores made more code readable

ddbbe3c

SandeepPissay reviewed May 20, 2017

View reviewed changes

BaluDontu closed this Jun 8, 2017

BaluDontu deleted the SPBMPolicySupportVsphere branch July 13, 2017 23:22

SPBM policy integration for dynamic volume provisioning inside kubernetes #142

SPBM policy integration for dynamic volume provisioning inside kubernetes #142

Conversation

BaluDontu commented May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BaluDontu May 11, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SandeepPissay May 13, 2017 • 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

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

BaluDontu left a 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

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

SandeepPissay commented May 20, 2017

BaluDontu commented May 20, 2017 • edited Loading

SandeepPissay commented May 21, 2017

BaluDontu commented Jun 8, 2017

BaluDontu commented May 11, 2017 •

edited

Loading

BaluDontu May 11, 2017 •

edited

Loading

SandeepPissay May 13, 2017 •

edited

Loading

BaluDontu commented May 20, 2017 •

edited

Loading