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

Volume Plugin for Cinder; Openstack Block Storage #6689

Merged
merged 1 commit into from Aug 28, 2015

Conversation

Projects
None yet
@spothanis
Copy link
Contributor

spothanis commented Apr 10, 2015

The PR implements cinder as a volume plugin. Cinder is openstack's block storage component.
The functionality is very similar to GCE's pd

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Apr 10, 2015

To @thockin to find a reviewer.

// CinderPersistentDisk represents a Persistent Disk resource in Openstack.
// A Cinder volume must exist and be formatted before mounting to a container.
// The disk must also be in the same region as the kubelet.
type CinderPersistentDiskVolumeSource struct {

This comment has been minimized.

@markturansky

markturansky Apr 10, 2015

Member

Maybe just "CinderVolumeSource"? It will be present in both the VolumeSource and PersistentVolumeSource structs.

This comment has been minimized.

@spothanis

spothanis Apr 10, 2015

Contributor

Will update

This comment has been minimized.

@thockin

thockin Apr 10, 2015

Member

Yeah "PersistentDisk" is not a general concept name, it's literally the product name on Google Cloud Platform. If it's not part of the well-known name of this concept, don't include the words.

@@ -200,6 +200,8 @@ type VolumeSource struct {
ISCSI *ISCSIVolumeSource `json:"iscsi"`
// Glusterfs represents a Glusterfs mount on the host that shares a pod's lifetime
Glusterfs *GlusterfsVolumeSource `json:"glusterfs"`
// CinderPersistentDisk represents a cinder volume attached and mounted on kubelets host machine
CinderPersistentDisk *CinderPersistentDiskVolumeSource `json:"cinderPersistentDisk"`

This comment has been minimized.

@markturansky

markturansky Apr 10, 2015

Member

You'll also want this in the PersistentVolumeSource struct if Cinder is to be provisioned as a PersistentVolume.

This comment has been minimized.

@spothanis

spothanis Apr 10, 2015

Contributor

We want cinder to be in the PersistentVolumeSource, but I am still in the process of writing the provisioning volume functionality in openstack cloudprovider, I will update the PR.

This comment has been minimized.

@markturansky

markturansky Apr 10, 2015

Member

Provisioning the volumes themselves is an orthogonal task to this. So long as the volumes exist in the infrastructure (however they were provisioned), they can be PersistentVolumes. You just need to implement the PersistentVolumePlugin interface and make sure the pointer is in both structs.

This comment has been minimized.

@spothanis

spothanis Apr 10, 2015

Contributor

Sorry my bad. I will update the pr.

This comment has been minimized.

@thockin

thockin Apr 10, 2015

Member

I'm not sure we want this as a VolumeSource long term - PersistentVolumeSource is a better abstraction. In the short term, I'll review the rest of this PR and come back to this point :)

This comment has been minimized.

@thockin

thockin Apr 10, 2015

Member

I guess this falls into the same bucket as AWS and GCE - OK for now, to eventually be replaced with Persistent Voluems

limitations under the License.
*/

package cinder_pd

This comment has been minimized.

@markturansky

markturansky Apr 10, 2015

Member

Maybe just "cinder"? that would match the others (nfs, iscsi, glusterfs)

This comment has been minimized.

@spothanis

spothanis Apr 10, 2015

Contributor

Will rename.

This comment has been minimized.

@thockin

thockin Apr 10, 2015

Member

This rename should include the dir and files


type cinderPersistentDiskPlugin struct {
host volume.VolumeHost
legacyMode bool // if set, plugin answers to the legacy name

This comment has been minimized.

@markturansky

markturansky Apr 10, 2015

Member

The original 4 volumes still support legacy mode. Newer volumes like this one don't need this.

This comment has been minimized.

@spothanis

spothanis Apr 10, 2015

Contributor

Will remove. Thanks for pointing out.

@spothanis spothanis force-pushed the spothanis:cinder-vol-plugin branch from 3e7052c to 4dc48ac Apr 10, 2015

// The disk must also be in the same region as the kubelet.
type CinderPersistentDiskVolumeSource struct {
// Unique name of the PD resource. Used to identify the disk in cinder volume
PDName string `json:"pdName"`

This comment has been minimized.

@thockin

thockin Apr 10, 2015

Member

As above - use field names that make sense to someone who knows this concept.

return volume, err
}

func (os *OpenStack) getMinionIDbyHostname(cClient *gophercloud.ServiceClient) (string, error) {

This comment has been minimized.

@thockin

thockin Apr 10, 2015

Member

We've moved away from "minion" to "node" in general

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Apr 10, 2015

Thanks for taking point Mark. I had almost nothing to add.

@markturansky

This comment has been minimized.

Copy link
Member

markturansky commented Apr 10, 2015

I'm happy to contribute where I can :)

@spothanis spothanis force-pushed the spothanis:cinder-vol-plugin branch from 4dc48ac to 694e08d Apr 15, 2015

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Apr 15, 2015

@zmerlynn

This is another of the volume plugins for which we have no e2e, no way to add e2e in GCE, and no way to ingest 3rd party e2e. :(

@pmorie

This comment has been minimized.

Copy link
Member

pmorie commented Apr 15, 2015

This is another of the volume plugins for which we have no e2e, no way to add e2e in GCE, and no way to ingest 3rd party e2e. :(

+1 (-1?)

@spothanis spothanis force-pushed the spothanis:cinder-vol-plugin branch from 694e08d to e78d268 Apr 21, 2015

@wattsteve

This comment has been minimized.

Copy link
Contributor

wattsteve commented May 7, 2015

When the Pods are scheduled on Kubernetes Nodes, what are the package requirements in order for the Kubernetes Nodes to be able mount the Cinder Block Device? For example, with the current Kubernetes GlusterFS and Ceph Plugins, glusterfs-client and ceph-common have to be installed, respectively.

@spothanis

This comment has been minimized.

Copy link
Contributor

spothanis commented May 7, 2015

@wattsteve

This comment has been minimized.

Copy link
Contributor

wattsteve commented May 7, 2015

@spothanis thanks. The README is part of the PR so I had read it, but all it states is "Ensure cinder is installed and configured properly in the region in which kubelet is spun up". Does this plugin design assume that the kubelet is running in a Kubernetes deployment within an OpenStack cluster? Does it still work if Kubernetes is running adjacent to an OpenStack Cinder Deployment (a private cloud scenario)? What client libraries need to be installed on the Kubelet Hosts for the Volume Plugin to work?

@spothanis

This comment has been minimized.

Copy link
Contributor

spothanis commented May 7, 2015

The volume plugin uses gopher cloud's openstack client. It is already
packaged into kubelet, so there are no additional installation requirements
for any client libraries.

Does this plugin design assume that the kubelet is running in a Kubernetes
deployment within an OpenStack cluster?
Yes.

Does it still work if Kubernetes is running adjacent to an OpenStack
Cinder Deployment (a private cloud scenario)?
Kubelet hosts must be nova vm's and must be in a region where Cinder is
available, only then nova will be able to attach a volume to the VM on
which kubelet has been running.

On Thu, May 7, 2015 at 5:07 AM, Steve Watt notifications@github.com wrote:

@spothanis https://github.com/spothanis thanks. The README is part of
the PR so I had read it, but all it states is "Ensure cinder is installed
and configured properly in the region in which kubelet is spun up". Does
this plugin design assume that the kubelet is running in a Kubernetes
deployment within an OpenStack cluster? Does it still work if Kubernetes is
running adjacent to an OpenStack Cinder Deployment (a private cloud
scenario)? What client libraries need to be installed on the Kubernetes for
the Volume Plugin to work?


Reply to this email directly or view it on GitHub
#6689 (comment)
.

Thank you,
Sreekanth

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented May 22, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain @ixdy.

@erictune

This comment has been minimized.

Copy link
Member

erictune commented Jun 1, 2015

ok to test

@erictune erictune added this to the v1.0-post milestone Jun 1, 2015

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Jun 1, 2015

EXPERIMENTAL JENKINS PR BUILDER: e2e build failed.

@@ -205,6 +205,8 @@ type VolumeSource struct {
Glusterfs *GlusterfsVolumeSource `json:"glusterfs"`
// PersistentVolumeClaimVolumeSource represents a reference to a PersistentVolumeClaim in the same namespace
PersistentVolumeClaimVolumeSource *PersistentVolumeClaimVolumeSource `json:"persistentVolumeClaim,omitempty"`
// CinderVolume represents a cinder volume attached and mounted on kubelets host machine
CinderVolume *CinderVolumeSource `json:"cinderVolume"`

This comment has been minimized.

@thockin

thockin Jun 1, 2015

Member

do you really want "cinderVolume" to be the API or do you want "cinder" ?

This comment has been minimized.

@spothanis

spothanis Jun 2, 2015

Contributor

I will change it to cinder

// The disk must also be in the same region as the kubelet.
type CinderVolumeSource struct {
// Unique uuid of the volume resource. Used to identify the cinder volume
VolID string `json:"volId"`

This comment has been minimized.

@thockin

thockin Jun 1, 2015

Member

don't abbreviate Vol here - plain old "id" or "volumeID"

This comment has been minimized.

@spothanis

spothanis Jun 2, 2015

Contributor

makes sense will change

@@ -419,6 +423,17 @@ func validateGlusterfs(glusterfs *api.GlusterfsVolumeSource) errs.ValidationErro
return allErrs
}

func validateCinderVolumeSource(PD *api.CinderVolumeSource) errs.ValidationErrorList {

This comment has been minimized.

@thockin

thockin Jun 1, 2015

Member

rename the PD variable to something meaningful here. it's not a PD (and it should have been 'pd' anyway)

This comment has been minimized.

@spothanis

spothanis Jun 2, 2015

Contributor

ahh, this was an over sight, thanks for catching it.

This comment has been minimized.

@thockin

thockin Jun 2, 2015

Member

If you can turn this PR around in a day or two I can review it again before
the real feature freeze

On Mon, Jun 1, 2015 at 5:19 PM, spothanis notifications@github.com wrote:

In pkg/api/validation/validation.go
#6689 (comment)
:

@@ -419,6 +423,17 @@ func validateGlusterfs(glusterfs *api.GlusterfsVolumeSource) errs.ValidationErro
return allErrs
}

+func validateCinderVolumeSource(PD *api.CinderVolumeSource) errs.ValidationErrorList {

ahh, this was an over sight, thanks for catching it.


Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/6689/files#r31484207
.

if cd.readOnly {
flags = mount.FlagReadOnly
}
cloud := cd.plugin.host.GetCloudProvider()

This comment has been minimized.

@thockin

thockin Jun 1, 2015

Member

rather than plumbing CloudProvider into Volumes, can you encapsulate it as a "manager" object like PD does? Is that possible?

This comment has been minimized.

@thockin

thockin Jun 1, 2015

Member

I'm torn on this one - this is probably a better answer but I want to do it consistently across the various forms, and I am not sure if this is 100% there (just haven't thought deeply). Shortest path to merge is to do it the same way others do and then file a bug to fix them all.

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Jul 8, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@pmorie

This comment has been minimized.

Copy link
Member

pmorie commented Jul 8, 2015

ok to test

On Wed, Jul 8, 2015 at 6:55 PM, Kubernetes Bot notifications@github.com
wrote:

Can one of the admins verify that this patch is reasonable to test? (reply
"ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.


Reply to this email directly or view it on GitHub
#6689 (comment)
.

@pmorie pmorie added the lgtm label Aug 27, 2015

@saad-ali

This comment has been minimized.

Copy link
Member

saad-ali commented Aug 27, 2015

@spothanis If you plan to address @pmorie's feedback in a follow up PR, please open an issue and link it here for tracking. And thanks for all your hard work!

@spothanis

This comment has been minimized.

Copy link
Contributor

spothanis commented Aug 27, 2015

@saad-ali I am rebasing to include @pmorie suggestions, will have it shortly, but if it is better to do it in a separate PR, I can do that too

@pmorie

This comment has been minimized.

Copy link
Member

pmorie commented Aug 27, 2015

@spothanis It's your call -- I'll remove LGTM if you would prefer and add it again when you're ready, or I can review a follow up PR. For the record, I didn't mean to derail this so close to LGTM.

@spothanis

This comment has been minimized.

Copy link
Contributor

spothanis commented Aug 27, 2015

@pmorie I really appreciate your time and advice on the PR. I will send a follow up PR to address the feedback
@thockin @saad-ali @markturansky Thank you for all of your time and feedback.

@pmorie

This comment has been minimized.

Copy link
Member

pmorie commented Aug 27, 2015

@spothanis did you decide about a follow up or integrating comments now?

@spothanis

This comment has been minimized.

Copy link
Contributor

spothanis commented Aug 27, 2015

@pmorie do a follow up PR if it is ok

@pmorie

This comment has been minimized.

Copy link
Member

pmorie commented Aug 27, 2015

@spothanis sounds great.

@k8s-oncall Can oncall take a look at this PR?

@k8s-merge-robot

This comment has been minimized.

Copy link
Contributor

k8s-merge-robot commented Aug 27, 2015

Labelling this PR as size/XXL

@bprashanth

This comment has been minimized.

Copy link
Member

bprashanth commented Aug 28, 2015

Fyi: plan is to wait for the submit queue to pick this up

@pmorie

This comment has been minimized.

Copy link
Member

pmorie commented Aug 28, 2015

@bprashanth when do you expect that to happen?

@bprashanth

This comment has been minimized.

Copy link
Member

bprashanth commented Aug 28, 2015

Hopefully today, it's a function of how green the build is. Let me know if this is blocking you and we can do a manual merge, as it seems retty isolated.

@saad-ali

This comment has been minimized.

Copy link
Member

saad-ali commented Aug 28, 2015

@bprashanth Let's prioritize getting this in. Thanks!

bprashanth pushed a commit that referenced this pull request Aug 28, 2015

Prashanth B
Merge pull request #6689 from spothanis/cinder-vol-plugin
Volume Plugin for Cinder; Openstack Block Storage

@bprashanth bprashanth merged commit 8d0d54f into kubernetes:master Aug 28, 2015

4 checks passed

Jenkins GCE e2e 146 tests run, 62 skipped, 0 failed.
Details
Shippable Shippable builds completed
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bprashanth

This comment has been minimized.

Copy link
Member

bprashanth commented Aug 28, 2015

Grr, broke precommit, please /hack/verify-generated-conversions.sh again

@spothanis

This comment has been minimized.

Copy link
Contributor

spothanis commented Aug 28, 2015

@bprashanth looking into it

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Aug 28, 2015

how did shippable pass with broken conversions?

@bprashanth

This comment has been minimized.

Copy link
Member

bprashanth commented Aug 28, 2015

the results should expire. it passed before the breaking changes came in.

@pmorie

This comment has been minimized.

Copy link
Member

pmorie commented Aug 28, 2015

@bprashanth Did I read your comment correctly to mean that the precommit hook changed after the last shippable run?

@bprashanth

This comment has been minimized.

Copy link
Member

bprashanth commented Aug 28, 2015

the generated code changed beneath us in a way that didn't cause a merge conflict but caused the precommit to fail, but we didn't notice because shippable ran and passed before the change

@spothanis

This comment has been minimized.

Copy link
Contributor

spothanis commented Aug 28, 2015

I will open a new PR with the required changes

@wattsteve

This comment has been minimized.

Copy link
Contributor

wattsteve commented Aug 31, 2015

@spothanis et al. Thanks for all your hard work on this. This is an awesome feature to add!

@spothanis spothanis referenced this pull request Aug 31, 2015

Merged

Cinder Volume Plugin #13367

dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018

Prashanth B
Merge pull request kubernetes#6689 from spothanis/cinder-vol-plugin
Volume Plugin for Cinder; Openstack Block Storage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment