Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

vSphere storage policy support for dynamic volume provisioning #46176

Merged
merged 4 commits into from
May 23, 2017
Merged

vSphere storage policy support for dynamic volume provisioning #46176

merged 4 commits into from
May 23, 2017

Conversation

BaluDontu
Copy link
Contributor

@BaluDontu BaluDontu commented May 21, 2017 •
edited
Loading

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:

vSphere cloud provider: vSphere Storage policy Support for dynamic volume provisioning

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 21, 2017
@k8s-ci-robot
Copy link
Contributor

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-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 21, 2017
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels May 21, 2017
@BaluDontu
Copy link
Contributor Author

@k8s-bot ok to test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 21, 2017
@k8s-ci-robot
Copy link
Contributor

@BaluDontu: you can't request testing unless you are a kubernetes member.

In response to this:

@k8s-bot ok to test

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-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 21, 2017
@BaluDontu
Copy link
Contributor Author

/assign @saad-ali

@BaluDontu
Copy link
Contributor Author

@saad-ali @deads2k @sttts : Can anyone of you run the tests?

@msau42
Copy link
Member

msau42 commented May 21, 2017

@k8s-bot ok to test

@BaluDontu BaluDontu force-pushed the vSphereStoragePolicySupport branch from c326cf1 to 8e516d1 Compare May 21, 2017 19:12
divyenpatel
divyenpatel reviewed May 22, 2017
View reviewed changes
pkg/cloudprovider/providers/vsphere/vsphere_util.go
}
dsMoList, err := vs.getDatastoreMo(ctx, nonCompatibleHubs)
if err != nil {
return nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should return err

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, GetNonCompatibleDatastoresMo returns datastore list if there are any, otherwise nil. Check the return type.

@BaluDontu BaluDontu force-pushed the vSphereStoragePolicySupport branch 3 times, most recently from efdbb8d to 962717e Compare May 22, 2017 06:41
@deads2k
Copy link
Contributor

deads2k commented May 22, 2017

@kubernetes/sig-storage-api-reviews @kubernetes/sig-storage-feature-requests @kubernetes/sig-storage-pr-reviews

@k8s-ci-robot k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label May 22, 2017
@deads2k deads2k removed their assignment May 22, 2017
@BaluDontu BaluDontu force-pushed the vSphereStoragePolicySupport branch 3 times, most recently from f4b5c5d to 5c26f3b Compare May 22, 2017 18:21
@BaluDontu
Copy link
Contributor Author

@saad-ali @sttts : All the required tests passed successfully. Can I have an lgtm for it.

@divyenpatel
Copy link
Member

/approve

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 22, 2017
@BaluDontu
Copy link
Contributor Author

Rebased the PR and resolved any conflicts..

@msau42
Copy link
Member

msau42 commented May 22, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2017
@lavalamp
Copy link
Member

/approve

@lavalamp lavalamp self-assigned this May 22, 2017
@lavalamp
Copy link
Member

/approve

@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 22, 2017
@BaluDontu BaluDontu force-pushed the vSphereStoragePolicySupport branch from 3442a22 to eb3cf50 Compare May 23, 2017 02:45
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 23, 2017
@BaluDontu
Copy link
Contributor Author

Rebased the PR again.

@msau42
Copy link
Member

msau42 commented May 23, 2017

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@msau42
Copy link
Member

msau42 commented May 23, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BaluDontu, divyenpatel, jingxu97, lavalamp, msau42

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

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

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 23, 2017 •
edited
Loading

@BaluDontu: The following test(s) failed:

Test name Commit Details Rerun command
pull-kubernetes-federation-e2e-gce eb3cf50 link @k8s-bot pull-kubernetes-federation-e2e-gce test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 455e9ff into kubernetes:master May 23, 2017
@BaluDontu BaluDontu mentioned this pull request May 24, 2017
Merged
dixudx pushed a commit to dixudx/kubernetes that referenced this pull request May 26, 2017
Merge pull request kubernetes#46320 from vmware/e2evSphereStoragePoli…
b9416c2 
…cySupport

Automatic merge from submit-queue (batch tested with PRs 45949, 46009, 46320, 46423, 46437)

e2e tests for storage policy support in Kubernetes

This PR covers e2e test cases for vSphere storage policy support in Kubernetes - kubernetes#46176.

The following test scenario have been implemented.
- Specify only SPBM storage policy name.
     - Verify if the disk is provisioned on a compatible datastore with max free space.
- Specify a storage policy name which is not defined on VC.
    - Verify if PVC create errors out that no pbm profile with this policy is found.
- Specify both SPBM storage policy name and VSAN capabilities together.
    - Verify if PVC create errors out that you can't use both SPBM policy name with VSAN capabilities. You can only specify one.
- Specify SPBM storage policy name with user specified datastore which is non-compatible.
   - Verify if PVC create errors out that it can't provision a disk on a non-compatible datastore.

@jeffvance @divyenpatel

**Release note**:

```release-note
None
```
@BaluDontu BaluDontu mentioned this pull request May 26, 2017
Closed
@BaluDontu BaluDontu mentioned this pull request Jun 8, 2017
Closed
@BaluDontu BaluDontu deleted the vSphereStoragePolicySupport branch July 13, 2017 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msau42 msau42 msau42 left review comments

@divyenpatel divyenpatel divyenpatel left review comments

Assignees

@lavalamp lavalamp

@sttts sttts

@saad-ali saad-ali

@msau42 msau42

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@BaluDontu @k8s-ci-robot @msau42 @deads2k @divyenpatel @jingxu97 @lavalamp @k8s-github-robot @sttts @saad-ali