VSAN policy support for storage volume provisioning inside kubernetes #42974

BaluDontu · 2017-03-13T03:40:25Z

The vsphere users will have the ability to specify custom Virtual SAN Storage Capabilities during dynamic volume provisioning. You can now define storage requirements, such as performance and availability, in the form of storage capabilities during dynamic volume provisioning. The storage capability requirements are converted into a Virtual SAN policy which are then pushed down to the Virtual SAN layer when a storage volume (virtual disk) is being created. The virtual disk is distributed across the Virtual SAN datastore to meet the requirements.

For example, User creates a storage class with VSAN storage capabilities:

kind: StorageClass
apiVersion: storage.k8s.io/v1beta1
metadata:
name: slow
provisioner: kubernetes.io/vsphere-volume
parameters:
hostFailuresToTolerate: "2"
diskStripes: "1"
cacheReservation: "20"
datastore: VSANDatastore

The vSphere Cloud provider provisions a virtual disk (VMDK) on VSAN with the policy configured to the disk.

When you know storage requirements of your application that is being deployed on a container, you can specify these storage capabilities when you create a storage class inside Kubernetes.

@pdhamdhere @tthole @abrarshivani @divyenpatel

Release note:

vSphere cloud provider: Allow specifying VSAN Storage Capabilities during dynamic volume provisioning.

k8s-ci-robot · 2017-03-13T03:40:26Z

Hi @BaluDontu. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-reviewable · 2017-03-13T03:40:30Z

This change is

BaluDontu · 2017-03-13T21:14:05Z

/approve

jingxu97 · 2017-03-22T22:07:15Z

@BaluDontu Does this PR require a release note? If it does, please add one

kerneltime · 2017-03-22T22:58:36Z

@k8s-bot ok to test

kerneltime · 2017-03-22T23:01:42Z

examples/volumes/vsphere/README.md

+
+  The policy settings can be one or more of the following:
+
+  * hostFailuresToTolerate: represents NumberOfFailuresToTolerate


The rendering of this is not clear. Either quote it as fixed width or bold the text strings.

I will make it to italics.

kerneltime · 2017-03-22T23:02:12Z

examples/volumes/vsphere/README.md

+      ```
+      [Download example](vsphere-volume-sc-vsancapabilities.yaml?raw=true)
+
+      Here a persistent volume will be created with the Virtual SAN capabilities - hostFailuresToTolerate to 2 and cachereservation is 20% read cache reserved for storage object. Also the persistent volume will be zeroedthick disk.


zeroedthick vs zeroedthick

I will make it to italics.

kerneltime · 2017-03-22T23:04:34Z

pkg/cloudprovider/providers/vsphere/vsphere.go

@@ -66,6 +66,8 @@ const (
 	ZeroedThickDiskType       = "zeroedThick"
 	VolDir                    = "kubevols"
 	RoundTripperDefaultCount  = 3
+	DummyVMName               = "kubernetes-helper-vm"


Is a race condition possible if 2 volumes are being created for the same VM name? Should the name by dynamically generated?

We are keeping this Dummy VM around for all the create disk operations.
If 2 requests for createDisk come at same time, the VC would serialize these operations.

Does the second create disk succeed or fail since create vm fails?

First time when VM create fails it will return back and kubernetes would try again that it when it finds existing dummyVM and will create a disk successfully.

Ok. I guess since the VM is not deleted this is true only for the first and second volume creation race.

kerneltime · 2017-03-22T23:09:16Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+	// 2. Add a new disk to the VM by performing VM reconfigure.
+	// 3. Detach the new disk from the dummy VM.
+	if volumeOptions.StorageProfileData != "" {
+		// Check if the datastore is VSAN if any capability requirements are specified.


Move the code into it's own private function will keep attach disk function readable.

I just wanted to keep it there as it would easy to read all the steps if storageProfileData is not null.
The code seems to be minimum as well.

kerneltime · 2017-03-22T23:11:21Z

pkg/cloudprovider/providers/vsphere/vsphere.go

+		// Check if the DummyVM exists in kubernetes cluster folder.
+		// The kubernetes cluster folder - vs.cfg.Global.WorkingDir is where all the nodes in the kubernetes cluster are created.
+		vmRegex := vs.cfg.Global.WorkingDir + DummyVMName
+		dummyVM, err := f.VirtualMachine(ctx, vmRegex)


Check the serialization logic within the controller manager. It might be possible to have 2 requests try to create the same VM. Using a new VM name that is specific for a POD might work around the serialization problem or we might have to check on error to see if VM is created.

If 2 requests come to create the same VM at exactly same time, the 1st operations would succeed and 2nd would fail. If 2nd fails, kubernetes attempts to try again and it would eventually find the already created VM and would create a volume on that disk.

All PVC creations are retried by kubernetes. It assumes as if these are recoverable errors.

This is clarified to you after our discussion and hence not needed anymore.

kerneltime · 2017-03-22T23:13:34Z

Partial review done, will complete review once tests pass.

kerneltime · 2017-03-22T23:25:35Z

pkg/volume/vsphere_volume/vsphere_volume_util.go

 			volumeOptions.DiskFormat = value
 		case "datastore":
 			volumeOptions.Datastore = value
+		case Policy_HostFailuresToTolerate:


The cases enumerated here can be a bit hard to read, can these be moved into independent methods?

We would creating so many methods for all the VSAN capabilities. This way it is easy to understand as for each case it would validate the VSAN capability and concatenate it to volumeOptions.StorageProfileData.

On further inspection this case structure and the one below can be combined. Instead of the second nested case being only for validation you can combine them into one validate and update string.

Same opinion as Ritesh on this one.. The validation function just did another switch which you have already done here. Better to combine them.

kerneltime · 2017-03-22T23:26:59Z

pkg/volume/vsphere_volume/vsphere_volume_util.go

+		case Policy_HostFailuresToTolerate:
+			if !validateVSANCapability(Policy_HostFailuresToTolerate, value) {
+				return "", 0, fmt.Errorf(`Invalid value for hostFailuresToTolerate in volume plugin %s. 
+				The default value is 1, minimum value is 0 and maximum value is 3.`, v.plugin.GetPluginName())


The limits here and below should be constants that get put into the log.

U meant the "The default value is 1, minimum value is 0 and maximum value is 3.",

"The value can be either 0 or 1",

"The minimum percentage is 0 and maximum percentage is 100."

and so on to be constants?

The values for the numbers

kerneltime · 2017-03-22T23:29:12Z

@k8s-bot non-cri node e2e test this

kerneltime · 2017-03-23T23:19:24Z

pkg/volume/vsphere_volume/vsphere_volume_util.go

+
+// Validate the capability requirement for the user specified policy attributes.
+func validateVSANCapability(capabilityName string, capabilityValue string) bool {
+	switch strings.ToLower(capabilityName) {


This can be cleaned up. The conversion can be done once and then the validation instead of doing the validation for each case which is duplication.

kerneltime · 2017-03-23T23:24:48Z

pkg/cloudprovider/providers/vsphere/vsphere.go

@@ -1378,3 +1636,49 @@ func makeDirectoryInDatastore(c *govmomi.Client, dc *object.Datacenter, path str

 	return err
 }
+
+// Get the folder


Update the comment or rename method to say that get the folder for a given VM

kerneltime · 2017-03-23T23:28:09Z

pkg/cloudprovider/providers/vsphere/vsphere.go

@@ -1210,18 +1189,6 @@ func (vs *VSphere) CreateVolume(volumeOptions *VolumeOptions) (volumePath string
 		datastore = volumeOptions.Datastore
 	}

-	// Default diskformat as 'thin'
-	if volumeOptions.DiskFormat == "" {


Why not leave this at the top level to not replicate setting the default?

luomiao · 2017-03-27T20:18:39Z

/lgtm

kerneltime · 2017-03-27T20:30:30Z

/lgtm

BaluDontu · 2017-03-27T21:38:36Z

@thockin : I have received an lgtm from @kerneltime @luomiao . Can you please provide an lgtm if we are good to go. Thanks!

thockin · 2017-03-27T22:16:31Z

/approve

k8s-github-robot · 2017-03-27T22:16:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BaluDontu, kerneltime, luomiao, thockin

Needs approval from an approver in each of these OWNERS Files:

~~examples/OWNERS~~ [thockin]
~~pkg/cloudprovider/providers/vsphere/OWNERS~~ [kerneltime,luomiao,thockin]
~~pkg/volume/vsphere_volume/OWNERS~~ [thockin]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-03-28T00:00:22Z

Automatic merge from submit-queue (batch tested with PRs 42835, 42974)

jingxu97 · 2017-03-28T16:14:34Z

/approve

@saad-ali

…42974-upstream-release-1.6 Automatic merge from submit-queue Automated cherry pick of #41929 #42974 upstream release 1.6 Cherry pick of #41929 #42974 on release-1.6. #41929: vSphere Cloud Provider: Fstype in storage class #42974: VSAN policy support for storage volume provisioning inside kubernetes @saad-ali

@jeffvance

Automatic merge from submit-queue e2e tests for VSAN policy support in Kubernetes for vSphere Following e2e test cases have been implemented for VSAN policy support in Kubernetes for vSphere. These e2e tests are for PR #42974 which out for review. A total of 8 test cases for below mentioned 5 different scenarios are implemented. Test cases: 1. Validation of VSAN capabilities. - hostFailuresToTolerate : Minimum 1 and Max 3. Should be integer. - stripeWidth: Minimum is 1 and Maximum is 12. Should be integer. - proportionalCapacity: Expressed in percentage. Should be between 0 and 100. Should be integer. - iopsLimit: Should be greater than 0. Should be integer. 2. Use a VSAN testbed setup. Try valid VSAN capabilities which are supported by VSAN testbed. Make sure the disk is created with policy configured with it. - Ex: Using hostFailuresToTolerate=0 and cacheReservation=12 Ex: diskStripes=1 and objectSpaceReservation=30 3. Use a VSAN testbed setup. Try valid VSAN capabilities which are not supported by VSAN testbed. Make sure that the disk creation fails and PVC status is pending. 4. Try using VSAN capabilities on a non-VSAN datastore. PVC status will be pending and it errors to the user saying to VSAN capabilities are not supported on a non-VSAN testbed. 5. Try all 1 to 5 with custom datastore specified by the user. @jeffvance @divyenpatel **Release note**: ```release-note None ```

Automatic merge from submit-queue vSphere storage policy support for dynamic volume provisioning Till now, vSphere cloud provider provides support to configure persistent volume with VSAN storage capabilities - #42974. Right now this only works with VSAN. Also there might be other use cases: - The user might need a way to configure a policy on other datastores like VMFS, NFS etc. - Use Storage IO control, VMCrypt policies for a persistent disk. We can achieve about 2 use cases by using existing storage policies which are already created on vCenter using the Storage Policy Based Management service. The user will specify the SPBM policy ID as part of dynamic provisioning - resultant persistent volume will have the policy configured with it. - The persistent volume will be created on the compatible datastore that satisfies the storage policy requirements. - If there are multiple compatible datastores, the datastore with the max free space would be chosen by default. - If the user specifies the datastore along with the storage policy ID, the volume will created on this datastore if its compatible. In case if the user specified datastore is incompatible, it would error out the reasons for incompatibility to the user. - Also, the user will be able to see the associations of persistent volume object with the policy on the vCenter once the volume is attached to the node. For instance in the below example, the volume will created on a compatible datastore with max free space that satisfies the "Gold" storage policy requirements. ``` kind: StorageClass apiVersion: storage.k8s.io/v1beta1 metadata: name: fast provisioner: kubernetes.io/vsphere-volume parameters: diskformat: zeroedthick storagepolicyName: Gold ``` For instance in the below example, the vSphere CP checks if "VSANDatastore" is compatible with "Gold" storage policy requirements. If yes, volume will be provisioned on "VSANDatastore" else it will error that "VSANDatastore" is not compatible with the exact reason for failure. ``` kind: StorageClass apiVersion: storage.k8s.io/v1beta1 metadata: name: fast provisioner: kubernetes.io/vsphere-volume parameters: diskformat: zeroedthick storagepolicyName: Gold datastore: VSANDatastore ``` As a part of this change, 4 commits have been added to this PR. 1. Vendor changes for vmware/govmomi 2. Changes to the VsphereVirtualDiskVolumeSource in the Kubernetes API. Added 2 additional fields StoragePolicyName, StoragePolicyID 3. Swagger and Open spec API changes. 4. vSphere Cloud Provider changes to implement the storage policy support. **Release note**: ```release-note vSphere cloud provider: vSphere Storage policy Support for dynamic volume provisioning ```

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 13, 2017

k8s-github-robot assigned freehan Mar 13, 2017

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Mar 13, 2017

BaluDontu mentioned this pull request Mar 16, 2017

e2e tests for VSAN policy support in Kubernetes for vSphere #43188

Merged

BaluDontu force-pushed the VSANPolicyProvisioningForKubernetesOnKubernetesRepo branch from 42dd5c9 to f060456 Compare March 16, 2017 21:22

jingxu97 requested a review from luomiao March 22, 2017 22:04

jingxu97 assigned kerneltime and unassigned freehan Mar 22, 2017

kerneltime reviewed Mar 22, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2017

BaluDontu force-pushed the VSANPolicyProvisioningForKubernetesOnKubernetesRepo branch 2 times, most recently from c97a300 to a0a4351 Compare March 23, 2017 22:48

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2017

kerneltime reviewed Mar 23, 2017

View reviewed changes

VSAN policy support for storage volume provisioning inside kubernetes

dbe9483

BaluDontu force-pushed the VSANPolicyProvisioningForKubernetesOnKubernetesRepo branch from c372155 to dbe9483 Compare March 27, 2017 19:43

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2017

k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Mar 27, 2017

k8s-github-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Mar 27, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2017

thockin removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Mar 27, 2017

k8s-github-robot merged commit 3843108 into kubernetes:master Mar 28, 2017

enisoc added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 20, 2017

BaluDontu mentioned this pull request May 11, 2017

SPBM policy integration for dynamic volume provisioning inside kubernetes vmware-archive/kubernetes-archived#142

Closed

BaluDontu mentioned this pull request May 21, 2017

vSphere storage policy support for dynamic volume provisioning #46176

Merged

BaluDontu deleted the VSANPolicyProvisioningForKubernetesOnKubernetesRepo branch September 5, 2017 23:10


		The policy settings can be one or more of the following:

		* hostFailuresToTolerate: represents NumberOfFailuresToTolerate

VSAN policy support for storage volume provisioning inside kubernetes #42974

VSAN policy support for storage volume provisioning inside kubernetes #42974

Conversation

BaluDontu commented Mar 13, 2017 • edited by enisoc

k8s-ci-robot commented Mar 13, 2017

k8s-reviewable commented Mar 13, 2017

BaluDontu commented Mar 13, 2017

jingxu97 commented Mar 22, 2017

kerneltime commented Mar 22, 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

BaluDontu Mar 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerneltime Mar 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerneltime Mar 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BaluDontu Mar 24, 2017 • edited

Choose a reason for hiding this comment

kerneltime commented Mar 22, 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

BaluDontu Mar 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerneltime commented Mar 22, 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

luomiao commented Mar 27, 2017 • edited

kerneltime commented Mar 27, 2017

BaluDontu commented Mar 27, 2017

thockin commented Mar 27, 2017

k8s-github-robot commented Mar 27, 2017

k8s-github-robot commented Mar 28, 2017

jingxu97 commented Mar 28, 2017

BaluDontu commented Mar 13, 2017 •

edited by enisoc

BaluDontu Mar 23, 2017 •

edited

kerneltime Mar 22, 2017 •

edited

kerneltime Mar 22, 2017 •

edited

BaluDontu Mar 24, 2017 •

edited

BaluDontu Mar 23, 2017 •

edited

luomiao commented Mar 27, 2017 •

edited