Add Cinder V3 and MicroVersion Support #415
Conversation
|
Hopefully that makes it much more clear. I'll add some comments regarding any differences, I took the liberty of adding some missing comments on public structs and removed occurences of naked-returns just because they bug me, and that's about it. |
| pagination.SinglePageBase | ||
| } | ||
|
|
||
| // UnmarshalJSON converts our JSON API response into our snapshot struct |
There was a problem hiding this comment.
Added missing comment on public struct here
| _, r.Err = client.Delete(deleteURL(client, id), nil) | ||
| return r | ||
| } | ||
|
|
There was a problem hiding this comment.
In Create and Delete above I made the returns explicit "return r" instead of the naked returns that were in the old version.
There was a problem hiding this comment.
I'm not sure how familiar you are with Go (and I'm no expert, either), so I apologize if you already know this: the "naked" return is known as a "named return". It comes off as a little odd at first. I've only recently started to appreciate it and am going through a phase of "name all the returns".
If you wanted to make this an explicit return, I think it'd be best to also change the function signature, too. From:
func Create(client *gophercloud.ServiceClient, opts CreateOptsBuilder) (r CreateResult) {to
func Create(client *gophercloud.ServiceClient, opts CreateOptsBuilder) CreateResult {However, I think it'd be best to leave the returns as they are, just because it would break consistency from how the other files are set up:
$ cd ~/go/src/github.com/gophercloud/gophercloud/openstack
$ grep -R return$ * | wc
441 888 21740There was a problem hiding this comment.
@jtopjian I'm pretty familiar with Golang, but I think it's always good to make suggestions and sync up terminology etc. One of my greatest pet peeves has become "naked" return statements in functions, initially I thought they were "awesome", but after a while I find them to be annoying, vague and often times confusing. It's not very "idomatic" as they say, but if you would prefer it in the interest of keeping things consistent that's fine, it is your project after all. It would be great if you'd reconsider though, just because it's "consistent" isn't necessarily a great argument IMO.
Your point about removing the name altogether is a good one, and I agree. Let me know if you're willing to reconsider at least for the BlockStorage modules going further, if not like I said that's completly fair. I can change it all back.
There was a problem hiding this comment.
I think the consistency is the most important thing here. There are parts of Gophercloud that not everyone agrees with, but adhering to a strict set of rules has helped keep it simple and relatively easy to implement additions. So I'm going to have to be the bad guy and say "keep the named return".
| }) | ||
| return r | ||
| } | ||
|
|
There was a problem hiding this comment.
Same modification to Update, remove the naked returns and explicitly return 'r'
| "github.com/gophercloud/gophercloud/pagination" | ||
| ) | ||
|
|
||
| // Attachment represents a Volume Attachment record |
There was a problem hiding this comment.
Added the public struct comment
| ) | ||
|
|
||
| // Attachment represents a Volume Attachment record | ||
| type Attachment struct { |
There was a problem hiding this comment.
Another public struct comment added here
| VolumeID string `json:"volume_id"` | ||
| } | ||
|
|
||
| // UnmarshalJSON is our unmarshalling helper |
There was a problem hiding this comment.
Public function so added the comment above
| return len(volumes) == 0, err | ||
| } | ||
|
|
||
| // ExtractVolumes extracts and returns Volumes. It is used while iterating over a volumes.List call. |
There was a problem hiding this comment.
Public function, needed above comment
| return &s, err | ||
| } | ||
|
|
||
| // ExtractInto converts our response data into a volume struct |
There was a problem hiding this comment.
Public function, needed above comment
| return r.Result.ExtractIntoStructPtr(v, "volume") | ||
| } | ||
|
|
||
| // ExtractVolumesInto similar to ExtractInto but operates on a `list` of volumes |
There was a problem hiding this comment.
Public function, needed above comment
|
@j-griffith I'm so sorry! I thought there would have been more changes to the actual API interaction between v2 and v3 than... zero :) I didn't mean for you to spend time pointing out comments and such. Is it safe to say that communicating with the Block Storage v3 API will be the same as v2? Or will v3, at some point, deviate from v2? The reason I ask is because if v2 and v3 would remain the same, we could probably just rename the But if v3 will, at some point, take off from v2, then it'd be best to make this copy. edit: Maybe a better way of framing my question: do you think it would be possible to have one shared // This field only exists in v3
if v3 {or // This is actually a string in v2
if v2 {where it would make sense to have separate v2 and v3 packages? |
|
@jtopjian So V3 "will" diverge but only when you start tacking on microversions. The base 3.0 version is in fact truly a 1:1 mapping to V2. That was actually intentional as part of the microversion adoption in Cinder. I'm actually not convinced of the best way to handle microversions from an SDK perspective, but that's beside the point for this I think. One thing I did think about when working on this was actually starting the V3 modules off as just interfaces that call directly into V2 if there is no microversion specified? That might actually make more sense here. You're suggestion of "making V2" something more generic makes sense too, the only thing is we've now got this nice pattern of using the directory structure to build the URL; that's certainly easy to change. Maybe we could sync up on IRC and chat? I'd like to get your input (and anybody else that has opinions). Maybe I'll put together a gist of some different options and post it to see what you and others think? |
That's probably the best option - I usually can't find the time to jump on IRC throughout the day. But yeah, I'm definitely interested in any ideas you have. On one hand, like you say the directory structure is a nice pattern. And I think that should be the default option. But since v2 support isn't fully implemented, I'm curious if there's a way that a change applicable to both v2 and v3 can be done in one spot. However, if making that happen will make overall management more complicated (meaning it would take more time to maintain the combined packages than just copying and pasting a change applicable to both v2 and v3), let's just defer to separate packages. |
|
Also, will |
|
Sounds good, I'll see if I can find some time in the next day or two to come up with some different ideas. The fact that there's only 2 modules (other than the contrib [volumeactions]) does make it a good time to consider this). I hated the copy/paste in V2 of Cinder so I totally agree it's annoying to repeat it here. The contrib (extension) items like volumeactions for now are pretty static and don't fall under either versioning or microversioning. That being said however one of the goals we're working on right now is deprecating and removing the current volume-actions attach related methods. They'll certainly be around for at least a couple more releases but they really need to go away. That's actually why I added this change was to have a foundation to pull in the new attachment API's. They've landed in Cinder last release, and we're currently working on updating Nova here: The idea is that when that lands we can deprecate and move forward and get things like multi-attach support finally (both Nova and BareMetal). |
|
Cool - thank you for all of the background! Also, if discussing / designing a long-term structure for v2/v3/contrib starts preventing you from actually using Gophercloud and v3, I'm more than happy to merge a straight copy first and then work on the long-term design. |
| } | ||
|
|
||
| return openstack.NewBlockStorageV3(client, gophercloud.EndpointOpts{ | ||
| Region: os.Getenv("OS_REGION_NAME"), |
There was a problem hiding this comment.
Does it make sense to have a resonable default in case OS_REGION_NAME is not set?
There was a problem hiding this comment.
Having an empty region name is completely valid.
| @@ -114,6 +114,8 @@ func (client *ServiceClient) setMicroversionHeader(opts *RequestOpts) { | |||
| opts.MoreHeaders["X-OpenStack-Nova-API-Version"] = client.Microversion | |||
| case "sharev2": | |||
| opts.MoreHeaders["X-OpenStack-Manila-API-Version"] = client.Microversion | |||
| case "volume": | |||
| opts.MoreHeaders["X-OpenStack-Volume-API-Version"] = client.Microversion | |||
There was a problem hiding this comment.
The client.Microversion is not set anywhere currently so it's an empty string. Shouldn't CinderV3 API reject packets that contain an empty string in the X-OpenStack-Volume-API-Version field.
There was a problem hiding this comment.
If client.Microversion is an empty string, this piece of code will never be reached. See here: https://github.com/j-griffith/gophercloud/blob/2af0cfeba7c8f0e894f945fb89ab8b0b0f7096a6/service_client.go#L61-L63
There was a problem hiding this comment.
@jtopjian yes, you're right, this piece of code will never be reached if the string is empty. I've overlooked the condition. I'm sorry for that.
However, how does CinderV3 behave in case the X-OpenStack-Volume-API-Version is not present? Are such packets rejected?
There was a problem hiding this comment.
No problem.
My understanding is that it will act like the other services (Nova, Manila) that implement microversions. A user won't be able to use a feature that requires a specific microversion. So the request won't be outright rejected, but certain features won't be usable.
There was a problem hiding this comment.
Yes, you're are right. Everything that is available in version 3.0 is available. Anything that requires 3.1+ version is not available.
|
LGTM |
|
@rootfs I think you may be interested. |
|
If reasonable, I'd like to see something in-between what's here now: I like the idea of creating a |
|
@j-griffith will Attachment API be here too? |
|
@rootfs Yes, and that was in fact that motivation for this PR. I know the wholesale copy/paste is lame; but sadly that's what we did in Cinder, Nova etc as well to get to that initial compatible microversion. I've considered trying something clever to get some re-use but honestly I'm not sure that's such a good idea. One thing to keep in mind, V3 should be it; there should never be another wholesale copy again, only additions for new microversions within the V3 endpoint functions. |
|
If in the future v3 and v2 do diverge, delegation is probably less desirable. @jrperritt is there any release plan to pick up this pr? thanks. |
|
@j-griffith Understood. I'm totally fine doing a copy/paste for v3. IIRC, I think the only issue with this PR was the named returned stuff. After that, it should be good to go. |
|
@jtopjian see my comment above for someone who is not yet on board with copy/paste |
|
@jrperritt Yeah, I'm not saying your idea isn't better. The problem is that I haven't been able to come up with a clean way of doing that without doing some sort of trickery which IMHO is never good. That being said, if you have an idea of a clean way to point back until updates that is easy to follow and doesn't involve "magic" I'd be thrilled to learn and implement it. I know this has sat a while because I was going to go off and try to come up with a happy medium, I didn't have much luck though without making a mess, the best I came up with was just interfaces in the V3 dir that called V2 and that seemed lame... but maybe that's ok?? The other option (and probably the right option) was to move all of the requests and not have the V2, V3 namespace be the controlling factor here. That's certainly doable, and makes a lot of sense I think; but I didn't like it because I was concerned about introducing compatibility issues for folks already using Gophercloud after performing upgrades. Maybe there's a way around that... I can look at it some more but if you have something in mind let me know. |
I apologize. I didn't interpret what you said as being against copy/paste. I was under the impression that if a better solution wasn't put forward, a copy/paste solution would be fine. |
@j-griffith I agree with the approach. I assume that in the |
|
Been away for a bit, looks like this approach is dead no? Was there ever an agreement on the MV topic? FWIW I still think the version independent resource structure is worth while. |
|
Hi all, Apologies for the delay with this. The microversion discussion is still ongoing but we wanted to at least get Cinder v3 moving. @j-griffith Go ahead and do a straight copy of v2 to v3. IIRC the only request I had from the previous copy was to leave the named returns in place. That can all be housed under one PR. Either separate commits per resource or a single commit is fine with me. Next, you can add whatever new v3 features you need on top of that (I've seen "multi-attach" mentioned a bunch). Our only request is to keep the additions minimal and stay within the same release/microversion as they will most likely be modified when a microversion solution has been reached. Each feature should follow the normal house rules: Please create an issue (similar to this) and open one PR per method. I'll gladly make it a priority to review them. Our hope with this is to unblock Cinder v3 usage while the microversion discussion is still ongoing. Please let me know if you have questions or concerns. Again, we do apologize for the delay. |
|
@jtopjian Thanks for the update, and no worries on the delay; there's a lot to be figured out here. I'll pick back up on the copy/update approach and hit the items you referenced. Thanks!! |
| b, err := opts.ToVolumeCreateMap() | ||
| if err != nil { | ||
| r.Err = err | ||
| return r |
j-griffith
force-pushed
the
cinder-v3-microversions
branch
from
f60d512 to
2a794b5
Compare
|
@jtopjian if you're good with this, feel free to merge |
|
Apologies for the delay -- I was waiting for Travis to pass. The error it is reporting is: I've confirmed only Once that's fixed up, the next error is:
Once those two changes are made, all tests should pass. Further, doing: $ diff -r openstack/blockstorage/v2 openstack/blockstorage/v3Looks clean. |
|
Thanks all, my bad on this; I haven't been looking for the last couple weeks. I'll go ahead and rework things with a general NewBlockStorageClient in noauth and fix up the other issue as well. Sorry I've been MIA for a bit. |
This change pulls the existing V2 implementations into V3. Note that
the base V3 version (3.0) is a direct map back to V2. As a result to
use the base V3 endpoint for Cinder we can don't need to change anything
other than the URL. Also clean up some simple lint errors in
requests.go
One thing that the base V3 version does buy us here though is
Microversions (for better or worse). So, this change also adds in the
ability to specify MicroVersion in the additional-header of the
ServiceClient. This will be important going forward and so we can now
add things like the new standalone Attachment API.
Volume V3 just points to V2 unless a microversioned call is issued:
https://github.com/openstack/cinder/blob/master/cinder/api/v3/volumes.py#L25
https://github.com/openstack/cinder/blob/master/cinder/api/v3/volumes.py#L50
An example of a micro versioned call (not implemented in gophercloud):
https://github.com/openstack/cinder/blob/master/cinder/api/v3/volumes.py#L212
Even the base V3 view builder just utilizes V2 (only adding additional
info for specific versions):
https://github.com/openstack/cinder/blob/master/cinder/api/v3/views/volumes.py#L19
https://github.com/openstack/cinder/blob/master/cinder/api/v3/views/snapshots.py#L19
j-griffith
force-pushed
the
cinder-v3-microversions
branch
from
2a794b5 to
94e1cbc
Compare
|
Build succeeded.
|
|
@jtopjian @jrperritt @pospispa I think over all this time I've lost track of some things. I thought we decided against the non-versioned resource approach here: https://github.com/j-griffith/gophercloud/tree/non-versioned-cinder-objects/openstack/blockstorage But want to check with all of you one last time. I'd like to get something for V3 wrapped up shortly and can make either approach work and get it finalized depending on what other think. |
|
@j-griffith The non-versioned approach may very well be the best long-term solution. The problem is that it implies a microversion solution, for which there isn't one yet. I wanted to proceed with a straight copy of v2-to-v3 because we know it works and it'll get v3 support into applications now. It buys us some time to get a solution for microversions. I realize that this means there might be a change in the way Cinder v3 (maybe even v2) is used within Gophercloud. However, that could be in 1 month or 6... The caveat here is to not add too many features to the v3 copy that would introduce breakage within the v3 API (meaning: let's try to keep this as v3.0 compatible as possible). |
|
Alright, here we go. |
|
@j-griffith Thank you for your work with this - especially for being part of the Cinder v3 design and the microversion design. It's really appreciated and I do hope you continue :) |
|
@jtopjian Absolutely!!! Thank you, looking forward to working together more in the near future. |
|
@j-griffith I was away on PTOs. I'm glad to see Cinder V3 merged. |
This commit is only a memo of what need to happen for the feature to work. 1. Import gophercloud/openstack/blockstorage/extensions/volumeactions - needed for ExtendSize function. 2. Update vendored version of gophercloud to include gophercloud/gophercloud#415. - needed for `blockStorageV3Client` and to enable microversions support in Cinder. 3. In `resourceBlockStorageVolumeV2` remove `ForceNew=true` from "size" field in schema. 4. In `resourceBlockStorageVolumeV2Update` detect changes in "size" field and call `volumeactions.ExtendSize` with V3Client and Microversion set to "3.42". Cater for error="EOF" bug.
ghost
mentioned this pull request
* ContainerInfraV1: add Cluster template datasource Add new datasource for the OpenStack Magnum cluster template. Provide acceptance test and website documentation. * ContainerInfraV1: check env prior datasource test Check if "OS_CONTAINER_INFRA_ENVIRONMENT" is set prior running datasource test for the OpenStack Container Infra cluster template. * Docs: add clustertemplate datasource web link Add Container Infra V1 clustertemplate datasource link to the website sidebar.
This change pulls the existing V2 implementations into V3. Note that
the base V3 version (3.0) is a direct map back to V2. As a result to
use the base V3 endpoint for Cinder we can don't need to change anything
other than the URL. Also clean up some simple lint errors in
requests.go
One thing that the base V3 version does buy us here though is
Microversions (for better or worse). So, this change also adds in the
ability to specify MicroVersion in the additional-header of the
ServiceClient. This will be important going forward and so we can now
add things like the new standalone Attachment API.
For #412
Volume V3 just points to V2 unless a microversioned call is issued:
https://github.com/openstack/cinder/blob/master/cinder/api/v3/volumes.py#L25
https://github.com/openstack/cinder/blob/master/cinder/api/v3/volumes.py#L50
An example of a micro versioned call (not implemented in gophercloud):
https://github.com/openstack/cinder/blob/master/cinder/api/v3/volumes.py#L212
Even the base V3 view builder just utilizes V2 (only adding additional
info for specific versions):
https://github.com/openstack/cinder/blob/master/cinder/api/v3/views/volumes.py#L19
https://github.com/openstack/cinder/blob/master/cinder/api/v3/views/snapshots.py#L19