Openstack cinder v1/v2/auto API support #40423

mkutsevol · 2017-01-25T14:29:38Z

What this PR does / why we need it:
It adds support for v2 cinder API + autodetection of available cinder API level (as in LBs).
Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #39572

Special notes for your reviewer:
Based on work by @anguslees. The first two commits are just rebased from #36344 which already had a lgtm by @jbeda

Release note:

Add support for v2 cinder API for openstack cloud provider. By default it autodetects the available version.

k8s-ci-robot · 2017-01-25T14:29:39Z

Hi @mkutsevol. 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-reviewable · 2017-01-25T14:29:46Z

This change is

xrl · 2017-01-28T18:31:51Z

My team is migrating to Mitaka and will lose Cinder V1 support. We are running this code and it's been functional and stable.

xrl · 2017-01-31T00:46:40Z

Can I request a bump in the gophercloud version? I am having issues with expiring tokens in my build and a gophercloud update might help.

mkutsevol · 2017-01-31T17:55:32Z

Sure @xrl, why not. Will update and push it out tomorrow.

xsgordon · 2017-02-06T21:29:25Z

@idvoretskyi can you add the sig/openstack label?

xrl · 2017-02-10T18:52:27Z

@jbeda @bgrant0607 can this work be reviewed and merged? it's significant and important to continue support with modern openstack installations.

bgrant0607 · 2017-02-28T16:47:14Z

@mkutsevol @xrl This PR needs to be reviewed by someone familiar with Openstack.

@xsgordon @idvoretskyi Please find someone to review.

idvoretskyi · 2017-02-28T17:36:57Z

cc @Quentin-M @anguslees

rootfs · 2017-03-08T15:35:30Z

pkg/cloudprovider/providers/openstack/openstack.go

+		})
+		if err != nil || sClient == nil {
+			glog.Errorf("Unable to initialize cinder client for region: %s", os.region)
+			return nil, err


This return is suspicious. If v1 API is not enabled, we should keep trying v2.

And we will, creating a ServiceClient doesn't depend on supported blockstorage api versions

This seems fine: either the user explicitly set "v1" in their config (in which case it should error) or it was determined that "v1" was current.

rootfs · 2017-03-08T15:36:17Z

pkg/cloudprovider/providers/openstack/openstack.go

+			return nil, err
+		}
+
+		apiversions_v1.List(sClient).EachPage(func(page pagination.Page) (bool, error) {


what if block v1 api is disabled?

There is no v2 apiversions at all in the gophercloud library.
It is used only to probe available versions.
So it will work flawlessly if v1 is absent.

rootfs · 2017-03-08T15:37:03Z

pkg/cloudprovider/providers/openstack/openstack.go

+			return true, nil
+		})
+		// Autoprobing failed, no supported versions available
+		if os.bsOpts.BSVersion == "auto" {


this is redundant, the statement is already under auto case.

It is not, it will be (if probe is successful) switched to highest supported and available version on lines 589/592,
If it is left in "auto" that means that probe failed and it errors out with a corresponding error message.

rootfs · 2017-03-08T15:47:23Z

pkg/cloudprovider/providers/openstack/openstack_volumes.go

+	return vol.ID, err
+}
+
+func (volumes *VolumesV2) Create(opts VolumeCreateOpts) (string, error) {


It looks to me, except in getVolume where ServerID and Device are new in V2, CreateDisk, DeleteDisk and rest of getVolume are identical in both V1 and V2.

In such case, why not just pass version as a new parameter into these functions and call the right package based on version, rather than copying code?

I tend to agree that duplicate code is not good, but in this particular case, please consider why I did that way. I implement the IVolumes interface. Now, the Create/Delete methods are similar. But when v3 arrives that might not be the case. If I'll have a structure like:

func (volumes *VolumesV1) Create(opts VolumeCreateOpts) (string, error) { return generalCreate('v1',...) } func (volumes *VolumesV2) Create(opts VolumeCreateOpts) (string, error) { return generalCreate('v2',...) }

... etc

I may force a developer, who will implement v3 support to adapt to that style, and no one knows what the lib will be like at that time. It may be not convenient.

I think that code organized this way (for this particular case) is more manageable.

When v3 arrives IVolumes implementations may be split into separate files, eg openstack_volumes_v#

Did I make you change your opinion? :D

I looked at v3 and v2 create volume APIs, so far they look the same.

But I buy your points that things could diverge later.

rootfs · 2017-03-21T17:30:13Z

I tested latest code on my setup. I created a storage class, a claim and a Pod. Eventually the claim and pod were created through v2 api.

@jrperritt any comments?

jrperritt · 2017-03-21T17:44:02Z

any comments?

This seems fine to me.

rootfs · 2017-03-21T17:46:08Z

@mkutsevol please make another (hopefully last) squash

xsgordon · 2017-03-21T18:02:29Z

@hogepodge yes I had been politely ignoring the need for v3 support as while it is ultimately going to be needed the situation with v1 versus v2 is more critical right now. We probably should have a separate issue tracking the need for v3 support eventually though.

Support for cinder v1/v2 api with the new gophercloud/gophercloud library. API version is configurable and defaulting autodetection.

mkutsevol · 2017-03-21T19:51:26Z

@rootfs, pushed and tests are ok.

idvoretskyi · 2017-03-21T19:59:02Z

cc @e0ne

rootfs · 2017-03-21T21:07:32Z

/lgtm

rootfs · 2017-03-21T21:08:09Z

@wojtek-t can you approve? Thanks.

e0ne · 2017-03-22T07:32:35Z

@xsgordon fair enough about v2 and v3. Anyway, from Cinder perspective both v1 and v2 versions are deprecated now. And as was said above, v3 and v2 versions are the same, so IMO it would be good to have v3 support. It will help to add microversions support in the future

xsgordon · 2017-03-22T12:51:25Z

@rootfs I couldn't find an open issue for gophercloud V3 support so I filed gophercloud/gophercloud#305 - patches welcome I'm sure ;).

@e0ne I don't disagree that it would be good, but right now we are blocked on the above and in the meantime the situation is that while V2 is deprecated but still supported V1 (the only version the provider supports right now) is completely removed (not just deprecated), so without this patch the provider's volume integration is not usable at all on OpenStack clouds that use recent releases of OpenStack. V3 support should be added in a separate PR once it is supported via Gophercloud.

hogepodge · 2017-03-22T16:25:21Z

Yes please let's unblock on this PR and then work on cinder v3 in gophercloud, then getting it into a follow up on this patch.

rootfs · 2017-03-24T14:02:00Z

@anguslees can you approve?

e0ne · 2017-03-24T14:21:04Z

@xsgordon, @hogepodge sure, I won't block this patch anyway.

anguslees · 2017-03-27T15:54:15Z

pkg/cloudprovider/providers/openstack/openstack_volumes.go

-	if len(disk.Attachments) > 0 && disk.Attachments[0]["server_id"] != nil && instanceID == disk.Attachments[0]["server_id"] {
+	if volume.AttachedServerId != instanceID {
+		errMsg := fmt.Sprintf("Disk: %s has no attachments or is not attached to compute: %s", volume.Name, instanceID)
+		glog.Errorf(errMsg)


minor point: This is an openstack-python pattern, and not usual for golang. Usually you want to glog the error xor return the error to your caller, presumably so they might glog it later with some additional context.

@anguslees, thank you for the review, that is a direct port of logic of previous code. If/when time comes to add v3 support I will keep this in mind.

anguslees · 2017-03-27T15:55:40Z

I made a minor style comment, but I'm ok with code being merged as-is.

/approve

wojtek-t · 2017-03-27T18:04:25Z

/approve

k8s-github-robot · 2017-03-27T18:04:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anguslees, mkutsevol, rootfs, wojtek-t

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

~~OWNERS~~ [wojtek-t]

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

k8s-github-robot · 2017-03-27T19:49:22Z

Automatic merge from submit-queue (batch tested with PRs 43681, 40423, 43562, 43008, 43381)

xrl · 2017-03-27T20:14:05Z

How soon does this get in to the 1.5 series?

mkutsevol · 2017-03-27T20:20:55Z

@xrl, hi!
I doubt that it will land into 1.5. It should have severe merge conflicts with 1.5

xrl · 2017-03-29T03:26:34Z

OK, so how soon will it land in 1.6? 👍

rootfs · 2017-03-29T13:11:15Z

@ethernetdan @calebamiles can this fix be cherry picked into 1.6?

@anguslees

…cinder_v1_2_auto Automatic merge from submit-queue (batch tested with PRs 43681, 40423, 43562, 43008, 43381) Openstack cinder v1/v2/auto API support **What this PR does / why we need it**: It adds support for v2 cinder API + autodetection of available cinder API level (as in LBs). **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes kubernetes#39572 **Special notes for your reviewer**: Based on work by @anguslees. The first two commits are just rebased from kubernetes#36344 which already had a lgtm by @jbeda **Release note**: ``` Add support for v2 cinder API for openstack cloud provider. By default it autodetects the available version. ```

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 25, 2017

k8s-github-robot assigned lavalamp Jan 25, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 25, 2017

lavalamp assigned jbeda and unassigned lavalamp Jan 26, 2017

k8s-github-robot assigned bgrant0607 and unassigned jbeda Jan 30, 2017

apelisse assigned jbeda Jan 31, 2017

idvoretskyi added the area/provider/openstack Issues or PRs related to openstack provider label Feb 7, 2017

xrl mentioned this pull request Feb 16, 2017

Migrate rackspace/gophercloud -> gophercloud/gophercloud #36344

Merged

bgrant0607 assigned idvoretskyi and xsgordon and unassigned jbeda and bgrant0607 Feb 28, 2017

xsgordon mentioned this pull request Mar 7, 2017

Need Cinder V2 API support #42658

Closed

rootfs reviewed Mar 8, 2017

View reviewed changes

mkutsevol added 2 commits March 21, 2017 20:46

Support for v1/v2/autoprobe openstack cinder blockstorage

2c05bb5

Support for cinder v1/v2 api with the new gophercloud/gophercloud library. API version is configurable and defaulting autodetection.

Update deps

89f596f

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 21, 2017

anguslees approved these changes Mar 27, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2017

k8s-github-robot merged commit 31e596e into kubernetes:master Mar 27, 2017

mkutsevol deleted the feature/openstack_cinder_v1_2_auto branch March 30, 2017 14:44

rootfs mentioned this pull request May 5, 2017

Cinder volume attach does not work #45350

Closed

Openstack cinder v1/v2/auto API support #40423

Openstack cinder v1/v2/auto API support #40423

Conversation

mkutsevol commented Jan 25, 2017

k8s-ci-robot commented Jan 25, 2017

k8s-reviewable commented Jan 25, 2017

xrl commented Jan 28, 2017

xrl commented Jan 31, 2017

mkutsevol commented Jan 31, 2017

xsgordon commented Feb 6, 2017

xrl commented Feb 10, 2017

bgrant0607 commented Feb 28, 2017

idvoretskyi commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkutsevol Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootfs commented Mar 21, 2017

jrperritt commented Mar 21, 2017

rootfs commented Mar 21, 2017

xsgordon commented Mar 21, 2017

mkutsevol commented Mar 21, 2017

idvoretskyi commented Mar 21, 2017

rootfs commented Mar 21, 2017

rootfs commented Mar 21, 2017

e0ne commented Mar 22, 2017

xsgordon commented Mar 22, 2017 • edited Loading

hogepodge commented Mar 22, 2017 • edited Loading

rootfs commented Mar 24, 2017

e0ne commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anguslees commented Mar 27, 2017

wojtek-t commented Mar 27, 2017

k8s-github-robot commented Mar 27, 2017

k8s-github-robot commented Mar 27, 2017

xrl commented Mar 27, 2017

mkutsevol commented Mar 27, 2017

xrl commented Mar 29, 2017

rootfs commented Mar 29, 2017

mkutsevol Mar 10, 2017 •

edited

Loading

xsgordon commented Mar 22, 2017 •

edited

Loading

hogepodge commented Mar 22, 2017 •

edited

Loading