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

CSI implementation of raw block volume support #64723

Merged
merged 1 commit into from Jun 6, 2018

Conversation

@vladimirvivien
Copy link
Member

vladimirvivien commented Jun 4, 2018

What this PR does / why we need it:
This PR implements support for block volumes feature.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #64722

Special notes for your reviewer:

Release note:

Provides API support for external CSI storage drivers to support block volumes.
@vladimirvivien

This comment has been minimized.

Copy link
Member Author

vladimirvivien commented Jun 4, 2018

/sig storage

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-block-support branch 2 times, most recently from f3a7c8d to dc35e59 Jun 4, 2018

@vladimirvivien vladimirvivien changed the title CSI block volumes support implementation CSI implementation of raw block volume support Jun 4, 2018

@vladimirvivien

This comment has been minimized.

Copy link
Member Author

vladimirvivien commented Jun 5, 2018

/test pull-kubernetes-e2e-gce

@saad-ali
Copy link
Member

saad-ali left a comment

Do you have a driver that you can test this code against?

attachID := getAttachmentName(csiSource.VolumeHandle, csiSource.Driver, nodeName)

// search for attachment by VolumeAttachment.Spec.Source.PersistentVolumeName
attachment, err := m.k8s.StorageV1beta1().VolumeAttachments().Get(attachID, meta.GetOptions{})

This comment has been minimized.

@saad-ali

saad-ali Jun 5, 2018

Member

It is unfortunate that we have this dependency between MountDevice (aka SetupDevice) and Attach. Ideally, these methods (along with NodeStage) should be able to be implement a local attach without a remote attach. But that is a larger refactor we can address in the future. + @davidz627

This comment has been minimized.

@davidz627

davidz627 Jun 5, 2018

Contributor

Issue tracked here: #64781

// request to remove pod volume map path also
podVolumePath, volumeName := m.GetPodDeviceMapPath()
podVolumeMapPath := filepath.Join(podVolumePath, volumeName)
if err := csi.NodeUnpublishVolume(ctx, m.volumeID, podVolumeMapPath); err != nil {

This comment has been minimized.

@saad-ali

saad-ali Jun 5, 2018

Member

Shouldn't NodeUnstageVolume and NodeUnpublishVolume be called by TearDownDevice and UnmapDevice instead of both in the same method?

This comment has been minimized.

@vladimirvivien

vladimirvivien Jun 5, 2018

Author Member

@saad-ali yeah the API probably should have an UnmapDevice method. But It does not. Similarly, the MapDevice was introduced in the API with the refactor PR. So, maybe later we do that.

@vladimirvivien

This comment has been minimized.

Copy link
Member Author

vladimirvivien commented Jun 5, 2018

@saad-ali Unfortunately I don't have something to test this.

@davidz627

This comment has been minimized.

Copy link
Contributor

davidz627 commented Jun 5, 2018

@vladimirvivien might be worth building out a little of the mock driver in the csi-test repo to test this with just to sanity check everything.
https://github.com/kubernetes-csi/csi-test

@saad-ali

This comment has been minimized.

Copy link
Member

saad-ali commented Jun 5, 2018

I'm not certain but my bet is that although SetupDevice and MapDevice both cleanly map to CSI calls, since all the cleanup logic is inside a single call TearDownDevice (and there is common non-plugin specific teardown logic as well), things may not work correctly when cleaning up.

Since we don't have any driver implementing block, we don't have a good way to test it. I'm ok merging this as is, putting a big "this is alpha comment" on top, and then revising once we have a real CSI block driver to test with.

@saad-ali

This comment has been minimized.

Copy link
Member

saad-ali commented Jun 5, 2018

Can you add a CSIBlockVolume feature gate, to ensure it is not enabled by default as alpha.

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-block-support branch from dc35e59 to eabc5b3 Jun 5, 2018

@vladimirvivien

This comment has been minimized.

Copy link
Member Author

vladimirvivien commented Jun 5, 2018

@saad-ali added CSIBlockVolume feature gate. PTAL

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-block-support branch from eabc5b3 to 5044a3d Jun 5, 2018

@saad-ali saad-ali added this to the v1.11 milestone Jun 5, 2018

@saad-ali

This comment has been minimized.

Copy link
Member

saad-ali commented Jun 5, 2018

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jun 5, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saad-ali, vladimirvivien

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

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 6, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@saad-ali @vladimirvivien

Pull Request Labels
  • sig/storage: Pull Request will be escalated to these SIGs if needed.
  • priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
  • kind/feature: New functionality.
Help
@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 6, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 6, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit e188271 into kubernetes:master Jun 6, 2018

17 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation vladimirvivien authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.