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

BlockStorage: apiversions #458

Merged
merged 5 commits into from
Jan 13, 2020
Merged

Conversation

jtopjian
Copy link
Contributor

For #457

If there's concern about leaving v1/apiversions for backwards compatibility, I can easily drop the deletion of v1/apiversions here. I'm on the fence about whether that would be worthwhile or not. The original apiversions seems to be expecting an old response format, but I suppose that might be useful for some very old environments.

/cc @j-griffith

@coveralls
Copy link

coveralls commented Aug 11, 2017

Coverage Status

Coverage decreased (-0.1%) to 71.383% when pulling 2c650e6 on jtopjian:bs-apiversions into 4ecdf7c on gophercloud:master.

@j-griffith
Copy link
Contributor

Honestly V1 in Cinder has been deprecated for well over a year now; my response here would be that if you need V1 compatability, then use glide or godep and freeze your gophercloud component; but if there's an opportunity for us to make versionless work here and this stands in the way jetison it.

@jtopjian
Copy link
Contributor Author

Just to clarify, having apiversions under v1 was a bit of a misnomer in the first place. v1/apiversions has the ability to query for v2, etc, but it's expecting a different format of the results (which can be seen in the unit tests). Keeping v1/apiversions as-is might be considered a safe place to tuck this old version away.

My vote is to get rid of it as well, but I'm usually the one arguing to keep stuff, so that's why I was asking for a sanity check :)

@jtopjian
Copy link
Contributor Author

Too early in the morning and I didn't finish my thought.

@j-griffith given the result format that v1/apiversions is expecting (notably the Get request that expects a version rather than versions), do you think there is a likely chance that there are enough OpenStack environments around that warrants keeping the old version of v1/apiversions as-is?

The idea would be that someone could choose to use the v1/apiversions package as a legacy package.

@jtopjian
Copy link
Contributor Author

I mulled this over today and decided to keep v1/apiversions as-is and introduce a new apiversions package. I added a deprecation note to v1/apiversions to clarify the status.

The topic of handling deprecated APIs has come up before. Maybe this package can sit and wait until a decision has been made.

@coveralls
Copy link

coveralls commented Aug 12, 2017

Coverage Status

Coverage decreased (-0.02%) to 71.471% when pulling ebdd135 on jtopjian:bs-apiversions into 8a1afad on gophercloud:master.

@coveralls
Copy link

coveralls commented Aug 12, 2017

Coverage Status

Coverage decreased (-0.02%) to 71.471% when pulling 6e14a2c on jtopjian:bs-apiversions into 8a1afad on gophercloud:master.

@j-griffith
Copy link
Contributor

So this seems reasonable to me, and leaving V1 while painful I guess is reasonable with a deprecation marking and hopefully some day it does actually go away.

I'm a little mixed about a couple of things though, like the objection to wholesale copy/paste in the V3 patch when this just does the same thing. Also I noticed that the apiversions.Get call doesn't seem to work, errors on "Resource not found", and I haven't spent any time trying to look at why.

Anyway... I think the discovery methods should definitely be independent like you have here so seems like a great idea to me.

@coveralls
Copy link

coveralls commented Aug 17, 2017

Coverage Status

Coverage increased (+0.01%) to 71.503% when pulling a1296cc on jtopjian:bs-apiversions into 8a1afad on gophercloud:master.

@jtopjian
Copy link
Contributor Author

I'm a little mixed about a couple of things though, like the objection to wholesale copy/paste in the V3 patch when this just does the same thing.

I'm not following about the copy/paste. The original version that is in v1/apiversions shouldn't be thought of as being restricted to Block Storage v1 (at least I don't think, anyway). The version that is stored in v1 could/should have been in a versionless package to begin with.

However, upon moving it from blockstorage/v1/apiversions to blockstorage/apiversions, I discovered that the response formats are very different than what a current OpenStack Cinder environment is returning.

The "old versions":

You can see in the "List" example that v2 is is also being retrieved (at least as a unit test fixture). So these fixtures were applicable back when v2 existed (hence why this package could have always been in blockstorage/apiversions).

Somewhere along the way of Cinder's history, the "Get" response changed significantly to this. Ignore that the fixture is for "v3" because v1 and v2 look similar. If you compare that structure with the original Get, there's differences.

I don't have enough understanding as to why that changed, who is using it, etc. So I decided to just keep it in v1 as that's not harming anything (except for it being utterly confusing that it's a feature of v1).

Does that make sense? I absolutely agree that it's a horribly confusing situation.

Also I noticed that the apiversions.Get call doesn't seem to work, errors on "Resource not found"

That's actually a simple one: it was a bug. :)

Thank you for pointing it out - it's been fixed.

@jtopjian
Copy link
Contributor Author

OK, I've made some progress in figuring this out.

I have access to a cloud that is still running the Icehouse release of Cinder. cinder.conf has enable_v2_api set to true.

$ curl http://localhost:8776/ | python -mjson.tool

{
    "versions": [
        {
            "id": "v1.0",
            "links": [
                {
                    "href": "http://localhost:8776/v1/",
                    "rel": "self"
                }
            ],
            "status": "CURRENT",
            "updated": "2012-01-04T11:33:21Z"
        },
        {
            "id": "v2.0",
            "links": [
                {
                    "href": "http://localhost:8776/v2/",
                    "rel": "self"
                }
            ],
            "status": "CURRENT",
            "updated": "2012-11-21T11:33:21Z"
        }
    ]
}
$ curl -s -H "X-Auth-Token:$token" http://localhost:8776/v2/ | python -mjson.tool

{
    "version": {
        "id": "v1.0",
        "links": [
            {
                "href": "http://localhost:8776/v2/",
                "rel": "self"
            },
            {
                "href": "http://jorgew.github.com/block-storage-api/content/os-block-storage-1.0.pdf",
                "rel": "describedby",
                "type": "application/pdf"
            },
            {
                "href": "http://docs.rackspacecloud.com/servers/api/v1.1/application.wadl",
                "rel": "describedby",
                "type": "application/vnd.sun.wadl+xml"
            }
        ],
        "media-types": [
            {
                "base": "application/xml",
                "type": "application/vnd.openstack.volume+xml;version=1"
            },
            {
                "base": "application/json",
                "type": "application/vnd.openstack.volume+json;version=1"
            }
        ],
        "status": "CURRENT",
        "updated": "2012-01-04T11:33:21Z"
    }
}

I have no idea why v1 is being reported when I specifically requested v2. But, the format of the response is what the blockstorage/v1/apiversions package is looking for. Namely, a single JSON object of version.

Now, if I do the same request on a cloud running Mitaka:

$ oscurl http://localhost:8776/v2/ | python -mjson.tool
{
    "versions": [
        {
            "id": "v2.0",
            "links": [
                {
                    "href": "http://docs.openstack.org/",
                    "rel": "describedby",
                    "type": "text/html"
                },
                {
                    "href": "http://localhost:8776/v2/",
                    "rel": "self"
                }
            ],
            "media-types": [
                {
                    "base": "application/json",
                    "type": "application/vnd.openstack.volume+json;version=1"
                },
                {
                    "base": "application/xml",
                    "type": "application/vnd.openstack.volume+xml;version=1"
                }
            ],
            "min_version": "",
            "status": "SUPPORTED",
            "updated": "2014-06-28T12:20:21Z",
            "version": ""
        }
    ]
}

I see the "new" response format which is looking for a versions array (even though there's only one version).

Going back to the Icehouse environment, regardless of v1 being reported when I query http://localhost:8776/v2/, v2 features definitely work.

So while I've made progress in at least being able to reproduce old responses that the original blockstorage/v1/apiversions package was built for, I'm unsure as to when and why the response formats changed (somewhere between Icehouse and Mitaka) and what should be the best method of handling this:

  1. Make blockstorage/apiversions account for both types.
  2. Continue with keeping blockstorage/v1/apiversions as a "legacy" package and blockstorage/apiversions be the new one.

This commit removes the Get request and adds the ability to
extract a single API version via a ExtractAPIVersion function.

This is done to handle cases where older Block Storage v2 API
endpoints return a different style format than found in newer
releases of Cinder. Since the result of a List request to the
base API URL is the same across all releases, a consistent
response can be retrieved.
@coveralls
Copy link

coveralls commented Aug 18, 2017

Coverage Status

Coverage increased (+0.002%) to 71.494% when pulling cc8c488 on jtopjian:bs-apiversions into 8a1afad on gophercloud:master.

@jtopjian
Copy link
Contributor Author

Alright, here's another variation of this.

Since querying a specific versioned endpoint (such as http://localhost:8776/v2/) can return a different format depending on the OpenStack release, that makes support across older releases difficult.

However, querying the base API endpoint (http://localhost:8776) has shown to be consistent and contains all required information that would have also been found in the versioned endpoint.

So the variation I committed removes the Get request entirely and provides a function to extract a single version from a list of versions.

I think the idea has merit but I'm not sure about the way I implemented this. I would kind of like to see a faux-Get request in order to be consistent with normal Gophercloud style. Also, ExtractAPIVersion and ExtractAPIVersions sound too similar. Maybe ExtractAllAPIVersions might be a better name. But, I'm pushing this to get feedback before proceeding further.

@jtopjian jtopjian merged commit 82e93e0 into gophercloud:master Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants