Rackspace improvements (OpenStack Cinder) #22023

mkulke · 2016-02-25T23:18:28Z

This adds PV support via Cinder on Rackspace clusters. Rackspace Cloud Block Storage is pretty much vanilla OpenStack Cinder, so there is no need for a separate Volume Plugin. Instead I refactored the Cinder/OpenStack interaction a bit (by introducing a CinderProvider Interface and moving the device path detection logic to the OpenStack part).

Right now this is limited to AttachDisk and DetachDisk. Creation and deletion of Block Storage is not in scope of this PR.

Also the ExternalID and InstanceID cloud provider methods have been implemented for Rackspace.

k8s-bot · 2016-02-25T23:19:10Z

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.

k8s-bot · 2016-02-25T23:20:13Z

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.

k8s-bot · 2016-02-25T23:20:39Z

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.

bgrant0607 · 2016-02-27T06:30:45Z

cc @anguslees @thommay

bgrant0607 · 2016-02-27T06:31:23Z

@markturansky

mkulke · 2016-02-28T00:08:21Z

I'm currently considering a change in the Instance detection heuristics:

Currently it uses node.Spec.Name to find an Instance in the Rackspace API.
The node name by default is the hostname. On Rackspace the hostname is derived from the Server Name, but it's no 1:1 match (e.g. _ is converted to - to have a valid hostname). In those cases (or when you override the node name via kubelet --hostname-overide=${CLOUD_NETWORK_IPV4}) the server will not be found.
Rackspace does not provide a meta-data endpoint like other OpenStack flavors (169.254.169.254), but when you provide --config-drive=true via HEAT or nova, you get /media/configdrive/openstack/latest/meta_data.json. Not by default, though.
A heuristic to get a Server ID via the config drive could be added.

mkulke · 2016-02-29T17:02:05Z

The aforementioned heuristic has been added to node address discovery.

bgrant0607 · 2016-03-13T18:07:09Z

@metral Do you know someone who could help review this?

metral · 2016-03-13T18:13:22Z

@bgrant0607 I can definitely help find someone, if not I can take a stab at it as well

metral · 2016-03-15T01:06:49Z

pkg/cloudprovider/providers/openstack/openstack.go

@@ -1070,6 +1071,21 @@ func (os *OpenStack) CreateVolume(name string, size int, tags *map[string]string
 	return vol.ID, err
 }

+func (os *OpenStack) GetDevicePath(diskId string) string {
+	files, _ := ioutil.ReadDir("/dev/disk/by-id/")


The usage of the specific device path /dev/disk/by-id/virtio-<UUID> makes a couple of assumptions:

That the device in question is from a pure/upstream OpenStack install, and not a Rackspace Public Cloud block device, as Rackspace presents block devices under the path schema /dev/disk/by-uuid/<UUID>

And that the block device in question was formatted with a fs type after its creation

By default a new block device in OpenStack or Rackspace comes clean without a fs on it where the path is /dev/vd* on OpenStack and /dev/xvd* on Rackspace.

Not having a fs on the device at creation, but rather after a user specifically performs mkfs.ext* on it, means that the device will not exist in /dev/disk/by-uuid/<UUID> until after its been formatted with a fs

I confirmed this today on fresh installs of Ubuntu 15.10, as well as a CoreOS 835.9.0, on the Rackspace Public Cloud.

GetDevicePath would have to be altered to accommodate for this difference in environment.

Also, see: https://ask.openstack.org/en/question/50882/are-devdiskby-id-symlinks-unreliable/

@metral: thanks for looking at this, GetDevicePath is actually specific to vanilla OpenStack (os *OpenStack). It was part of the Cinder volume plugin, which I generalized to make it work with Rackspace as well: https://github.com/kubernetes/kubernetes/pull/22023/files#diff-dcc20522f7683cc42d2cd40a27c2c155L92

For Rackspace I did not find a way to deduct the device via volume-id from the device tree at all, so I am querying the Cinder API for the device, which works fine: https://github.com/kubernetes/kubernetes/pull/22023/files#diff-dcc20522f7683cc42d2cd40a27c2c155L92

For generic openstack: I think it's fine to stick with the same existing code in this change, but I share @metral's concerns about finding unformatted volumes by-uuid. I like the CinderProvider.GetDevicePath structure you've added to this change, and we should use that in a later change to replace the generic OpenStack logic with something that also uses the cinder API to find the device name (looks like this is GetVolume(diskId).attachments[i].device, where attachments[i].server_id==our_server_id in cinder v2 API).

It's probably feasible to try cinder API v2 or v1 depending on what appears to be available, and therefore combine the rackspace+openstack code again. I'm not fussed about whether we do that in this change or not (there are so many other places where the rackspace+openstack providers could share code, that it seems like that's a reasonable project in itself).

metral · 2016-03-15T01:45:17Z

@jamiehannaford, care to take a look?

jamiehannaford · 2016-03-15T10:31:37Z

pkg/cloudprovider/providers/rackspace/rackspace.go

+}
+
+// Attaches given cinder volume to the compute running kubelet
+func (os *Rackspace) AttachDisk(diskName string) (string, error) {


really minor, but maybe change os to rs to avoid ambiguity over providers

elsewhere in this module os is used, I kept it for consistency, as I don't want to touch a lot of unrelated lines. I'll name it rs where I touch code, anyway. The naming issues should be addressed in a separate PR i guess.

Yeah, I agree. If you could change os to rs in the functions you've added, though, that would be great.

jamiehannaford · 2016-03-15T10:51:33Z

@jrperritt is this something you could also have a look at? I've already left some feedback

jrperritt · 2016-03-16T10:26:01Z

Yes, if there's still interest, I can have a look tomorrow.

jrperritt · 2016-03-17T08:20:24Z

pkg/cloudprovider/providers/rackspace/rackspace.go

+		if instanceID == disk.Attachments[0]["server_id"] {
+			glog.V(4).Infof("Disk: %q is already attached to compute: %q", diskName, instanceID)
+			return disk.ID, nil
+		} else {


Since you're returning either way, you can remove this else line.

jrperritt · 2016-03-17T08:37:22Z

+1 from me. All my comments were stylistic changes (minor, to be sure).

saad-ali · 2016-04-14T03:32:22Z

pkg/cloudprovider/providers/openstack/openstack.go

@@ -1080,6 +1081,21 @@ func (os *OpenStack) CreateVolume(name string, size int, tags *map[string]string
 	return vol.ID, err
 }

+func (os *OpenStack) GetDevicePath(diskId string) string {


Add a GoDoc comment

k8s-bot · 2016-04-14T12:04:33Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in hack/jenkins/job-configs/kubernetes-jenkins-pull/ instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

saad-ali · 2016-04-18T21:00:26Z

LGTM
@mkulke Could you squash your commits and rebase.
@anguslees, @jamiehannaford, @jrperritt: if you all have no objections I'll add the LGTM label to get this merged.

saad-ali · 2016-04-20T23:46:47Z

@k8s-bot ok to test

k8s-bot · 2016-04-21T00:42:09Z

GCE e2e build/test passed for commit 07ecd3dce40768f5b001b5fef0097aab59624304.

…overy.

k8s-bot · 2016-04-21T09:06:17Z

GCE e2e build/test passed for commit ba4d74f.

mkulke · 2016-04-21T09:06:31Z

@saad-ali, sorry I forgot to stage adjusted openstack tests.

k8s-bot · 2016-04-21T21:23:21Z

GCE e2e build/test passed for commit ba4d74f.

k8s-github-robot · 2016-04-21T22:40:57Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-04-21T23:35:02Z

GCE e2e build/test passed for commit ba4d74f.

k8s-github-robot · 2016-04-21T23:38:13Z

Automatic merge from submit-queue

Automatic merge from submit-queue Rackspace improvements (OpenStack Cinder) This adds PV support via Cinder on Rackspace clusters. Rackspace Cloud Block Storage is pretty much vanilla OpenStack Cinder, so there is no need for a separate Volume Plugin. Instead I refactored the Cinder/OpenStack interaction a bit (by introducing a CinderProvider Interface and moving the device path detection logic to the OpenStack part). Right now this is limited to `AttachDisk` and `DetachDisk`. Creation and deletion of Block Storage is not in scope of this PR. Also the `ExternalID` and `InstanceID` cloud provider methods have been implemented for Rackspace.

googlebot added the cla: yes label Feb 25, 2016

k8s-github-robot assigned bgrant0607 Feb 25, 2016

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 25, 2016

mkulke changed the title ~~Rackspace improvements~~ Rackspace improvements (OpenStack Cinder) Feb 25, 2016

metral reviewed Mar 15, 2016
View reviewed changes

jamiehannaford reviewed Mar 15, 2016
View reviewed changes

jrperritt reviewed Mar 17, 2016
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 22, 2016

ghost assigned saad-ali and unassigned ghost Apr 4, 2016

saad-ali reviewed Apr 14, 2016
View reviewed changes

mkulke force-pushed the rackspace-improvements branch from b7f3a98 to 42f2b8b Compare April 15, 2016 12:57

mkulke force-pushed the rackspace-improvements branch from 189f01f to 07ecd3d Compare April 18, 2016 21:48

saad-ali added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Apr 20, 2016

Added Block Storage support to Rackspace provider, improved Node disc…

ba4d74f

…overy.

mkulke force-pushed the rackspace-improvements branch from 07ecd3d to ba4d74f Compare April 21, 2016 08:32

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 21, 2016

k8s-github-robot added the needs-ok-to-merge label Apr 21, 2016

saad-ali added lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-merge and removed needs-ok-to-merge labels Apr 21, 2016

k8s-github-robot merged commit 06160b6 into kubernetes:master Apr 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rackspace improvements (OpenStack Cinder) #22023

Rackspace improvements (OpenStack Cinder) #22023

mkulke commented Feb 25, 2016

k8s-bot commented Feb 25, 2016

k8s-bot commented Feb 25, 2016

k8s-bot commented Feb 25, 2016

bgrant0607 commented Feb 27, 2016

bgrant0607 commented Feb 27, 2016

mkulke commented Feb 28, 2016

mkulke commented Feb 29, 2016

bgrant0607 commented Mar 13, 2016

metral commented Mar 13, 2016

metral Mar 15, 2016

mkulke Mar 15, 2016

anguslees Mar 22, 2016

metral commented Mar 15, 2016

jamiehannaford Mar 15, 2016

mkulke Mar 15, 2016

jamiehannaford Mar 15, 2016

jamiehannaford commented Mar 15, 2016

jrperritt commented Mar 16, 2016

jrperritt Mar 17, 2016

jrperritt commented Mar 17, 2016

saad-ali Apr 14, 2016

k8s-bot commented Apr 14, 2016

saad-ali commented Apr 18, 2016

saad-ali commented Apr 20, 2016

k8s-bot commented Apr 21, 2016

k8s-bot commented Apr 21, 2016

mkulke commented Apr 21, 2016

k8s-bot commented Apr 21, 2016

k8s-github-robot commented Apr 21, 2016

k8s-bot commented Apr 21, 2016

k8s-github-robot commented Apr 21, 2016

Rackspace improvements (OpenStack Cinder) #22023

Rackspace improvements (OpenStack Cinder) #22023

Conversation

mkulke commented Feb 25, 2016

k8s-bot commented Feb 25, 2016

k8s-bot commented Feb 25, 2016

k8s-bot commented Feb 25, 2016

bgrant0607 commented Feb 27, 2016

bgrant0607 commented Feb 27, 2016

mkulke commented Feb 28, 2016

mkulke commented Feb 29, 2016

bgrant0607 commented Mar 13, 2016

metral commented Mar 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metral commented Mar 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamiehannaford commented Mar 15, 2016

jrperritt commented Mar 16, 2016

Choose a reason for hiding this comment

jrperritt commented Mar 17, 2016

Choose a reason for hiding this comment

k8s-bot commented Apr 14, 2016

saad-ali commented Apr 18, 2016

saad-ali commented Apr 20, 2016

k8s-bot commented Apr 21, 2016

k8s-bot commented Apr 21, 2016

mkulke commented Apr 21, 2016

k8s-bot commented Apr 21, 2016

k8s-github-robot commented Apr 21, 2016

k8s-bot commented Apr 21, 2016

k8s-github-robot commented Apr 21, 2016