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.

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

Feature: Implements Raw Block Volume Support for in-tree vSphere plugin #68761

Merged
merged 1 commit into from
Nov 8, 2018

Conversation

fanzhangio
Copy link

@fanzhangio fanzhangio commented Sep 18, 2018

  • add volume mode checking and set fstype for any FSType
  • set VolumeMode in PV spec
  • implement BlockVolumePlugin, BlockVolumeMapper, BlockVolumeUnmapper interfaces.
  • add tests for vsphere block volume

What this PR does / why we need it:
vSphere should support raw block volume support
https://kubernetes.io/docs/concepts/storage/persistent-volumes/#raw-block-volume-support

Which issue(s) this PR fixes:
Fixes #68762

Special notes for your reviewer:

Release note:

vSphereVolume implements Raw Block Volume Support 

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 18, 2018
@k8s-ci-robot k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 18, 2018
@fanzhangio
Copy link
Author

/test pull-kubernetes-e2e-gce-100-performance
/test pull-kubernetes-kubemark-e2e-gce-big

@k8s-ci-robot
Copy link
Contributor

@fanzhangio: You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to set the milestone.

In response to this:

/milestone v1.12

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.

@fanzhangio
Copy link
Author

@vladimirvivien Could you plz take a look at this PR ?

@vladimirvivien
Copy link
Member

Hi @fanzhangio
For proper block volume implementation, you will need to implement a three interfaces:

Take a look at gce_pd_block to get an idea what is needed
https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/gce_pd/gce_pd_block.go

@k8s-ci-robot k8s-ci-robot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 25, 2018
@fanzhangio fanzhangio changed the title Add vsphere block volume support (WIP) Add vsphere block volume support Sep 25, 2018
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Sep 25, 2018
@fanzhangio fanzhangio force-pushed the vsphere-volume branch 7 times, most recently from 04b1d6e to 44a06bf Compare September 30, 2018 18:54
@fanzhangio fanzhangio changed the title (WIP) Add vsphere block volume support Feature: Implement vSphere block volume Sep 30, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 30, 2018
"k8s.io/kubernetes/pkg/volume/util/volumepathhandler"
)

var _ volume.BlockVolumePlugin = &vsphereVolumePlugin{}
Copy link
Contributor

Choose a reason for hiding this comment

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

The existing vsphere volume plugin already support creating/attaching/detaching/deleting block volumes. Could you explain what is the additional functionality specific to vSphere are we getting for block volumes with this PR?

Copy link
Member

@divyenpatel divyenpatel Oct 1, 2018

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @divyenpatel ! Yes, it is raw block volume support. I am going to modify the PR title for more clarity. @SandeepPissay

@fanzhangio fanzhangio changed the title Feature: Implement vSphere block volume Feature: Implement Raw vSphere Block Volume Support Oct 1, 2018
@vladimirvivien
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 5, 2018
@vladimirvivien
Copy link
Member

@SandeepPissay @divyenpatel PTAL, this PR needs approval. Thanks.

blkUtil := volumepathhandler.NewBlockVolumePathHandler()
globalMapPathUUID, err := blkUtil.FindGlobalMapPathUUIDFromPod(pluginDir, mapPath, podUID)
if err != nil {
return nil, err
Copy link
Member

Choose a reason for hiding this comment

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

Would be good for debugging if we can print error before returning.

glog.Errorf("Failed to find GlobalMapPathUUID from Pod: %s with error: %+v", podUID, err)
return nil, err

Copy link
Author

Choose a reason for hiding this comment

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

OK

pkg/volume/vsphere_volume/vsphere_volume_block.go Outdated Show resolved Hide resolved
func (plugin *vsphereVolumePlugin) newBlockVolumeMapperInternal(spec *volume.Spec, podUID types.UID, manager vdManager, mounter mount.Interface) (volume.BlockVolumeMapper, error) {
volumeSource, _, err := getVolumeSource(spec)
if err != nil {
return nil, err
Copy link
Member

Choose a reason for hiding this comment

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

log error before return.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@divyenpatel
Copy link
Member

@fanzhangio can you please provide testing details?
Can you share YAMLs that you have tried out to access raw block volume in the pod with vSphere cloud provider?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2018
@fanzhangio
Copy link
Author

fanzhangio commented Nov 7, 2018

Hi @divyenpatel, here is E2E test by steps:

  1. provision VC and Hosts.
  2. provision VMs as k8s nodes
  3. deploy k8s cluster enabling feature gates BlockVolume=true and cloud provider intree
  4. Run intree e2e tests towards this VC
  5. Test PV, PVC yaml on cluster

@fanzhangio
Copy link
Author

fanzhangio commented Nov 7, 2018

YAMLs

Persistent Volume Claim requesting a Raw Block Volume

apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: block-pvc
  annotations:
    volume.beta.kubernetes.io/storage-class: thin-disk
spec:
  accessModes:
    - ReadWriteOnce
  volumeMode: Block
  resources:
    requests:
      storage: 2Gi

Create Storage Class

apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
  name: thin-disk
provisioner: kubernetes.io/vsphere-volume
parameters:
    datastore: sharedVmfs-0

Persistent Volume

apiVersion: v1
kind: PersistentVolume
metadata:
  name: raw-pv
spec:
  volumeMode: Block
  capacity:
    storage: 2Gi
  accessModes:
    - ReadWriteOnce
  persistentVolumeReclaimPolicy: Retain
  vsphereVolume:
    volumePath: “[datasotre1] volumes/myDisk”

@divyenpatel
Copy link
Member

divyenpatel commented Nov 7, 2018

@fanzhangio I do not see in the diff, following check is removed pkg/volume/vsphere_volume/vsphere_volume.go

if util.CheckPersistentVolumeClaimModeBlock(v.options.PVC) {
 		return nil, fmt.Errorf("%s does not support block volume provisioning", v.plugin.GetPluginName())
 	}

- add raw block volume support for vsphere volume provisioner
- set VolumeMode and any FSType in vsphere volume dynamic provision if feature
BlockVolume enabled
- implement BlockVolumePlugin, BlockVolumeMapper, BlockVolumeUnmapper interfaces.
- add tests for vsphere block volume
@fanzhangio
Copy link
Author

/test pull-kubernetes-integration

@divyenpatel
Copy link
Member

/assign @divyenpatel

@divyenpatel
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: divyenpatel, fanzhangio

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2018
@divyenpatel
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2018
@fanzhangio
Copy link
Author

/kind feature

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Nov 7, 2018
@fanzhangio
Copy link
Author

/test pull-kubernetes-e2e-gce

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/feature Categorizes issue or PR as related to a new feature. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vSphere volume plugin needs to support block volume
5 participants