compute: Add ServerGroup property to Server#2217
Conversation
|
Tested against OpenStack 16: computeClient.Microversion = "2.71"
s, err := servers.Get(computeClient, serverUUID).Extract()
if err != nil {
panic(err)
}
fmt.Println(s.ServerGroups) |
jtopjian
left a comment
There was a problem hiding this comment.
@pierreprinetti Thanks for submitting this. Just one change - let me know if you have any questions.
In addition, for updates like this, we need to see the upstream Nova code that implements this feature. For this case, I believe this is the following:
- https://github.com/openstack/nova/blob/d2a5fe5621d6ff1ae8ba5087049e0c4347592cf6/nova/api/openstack/compute/servers.py#L454-L455
- https://github.com/openstack/nova/blob/52cae8801de9229d6cfb7871c2073b83b3e41b81/nova/api/openstack/compute/views/servers.py#L420-L423
Does that look correct to you?
| // server groups to which the server belongs. Currently this can | ||
| // contain at most one entry. | ||
| // New in microversion 2.71 | ||
| ServerGroups []string `json:"server_groups"` |
There was a problem hiding this comment.
This should be *[]string per: https://github.com/gophercloud/gophercloud/blob/master/docs/MICROVERSIONS.md#new-response-fields
There was a problem hiding this comment.
I am not a fan of indirection, but I am happy to do the change in the name of consistency.
However I wonder what case does this "pointer rule" address? When setting the microversion to a high enough value, Nova becomes pretty loud:
$ go build ./cmd/test && ./test
panic: Expected HTTP response code [200 203] when accessing [GET https://compute.mycloud.example.com/v2.1/0693e2bb538c92b79a49fe6d2e61b0fc/servers/d7f95d73-7a68-423d-89a7-1bf0031f7016], but got 406 instead
{"computeFault": {"code": 406, "message": "Version 2.71 is not supported by the API. Minimum is 2.1 and maximum is 2.60."}}
Here's the matrix of my results (before adding the pointer) when targeting OpenStack ~16 (code):
| call microversion | server has SG | ServerGroups is nil | len(ServerGroups) |
|---|---|---|---|
| 2.60 | false | true | 0 |
| 2.60 | true | true | 0 |
| 2.71 | false | false | 0 |
| 2.71 | true | false | 1 |
My OpenStack13 cloud is currently down, but based on what I've seen yesterday, this should be the result matrix for OpenStack 13 (before adding the pointer):
| call microversion | server has SG | ServerGroups is nil | len(ServerGroups) |
|---|---|---|---|
| 2.60 | false | true | 0 |
| 2.60 | true | true | 0 |
| 2.71 | false | error | error |
| 2.71 | true | error | error |
It seems that the results are distinct and deterministic based on the microversion in the Gophercloud call.
Incidentally, this slice is only nil when the microversion is below threshold (but I understand that this nil distinction can not be generalised to any type).
As I have already mentioned, however, this is just a thought and I am happy to conform to the rule.
There was a problem hiding this comment.
However I wonder what case does this "pointer rule" address?
Incidentally, this slice is only nil when the microversion is below threshold
Yes, this is the reason to have these fields as pointers. It provides some kind of way to denote that a sufficient microversion wasn't used. It's not great, but it's the most approachable way of balancing a typed language like Go and a dynamic result set like what "microversions" are doing.
|
Build succeeded.
|
0b8c74a to
d527cb1
Compare
This looks perfectly correct to me, thank you for the research 🙏 I have updated the commit message accordingly. |
Add to the Server type the list of server groups the server is part of, as exposed in microversions 1.71 and above as "server_groups". The new `ServerGroups` property is a pointer to slice (`*[]string`), as mandated by the Gophercloud ["New Response Fields" convention][1]. Nova code implementing the feature: * https://github.com/openstack/nova/blob/d2a5fe5621d6ff1ae8ba5087049e0c4347592cf6/nova/api/openstack/compute/servers.py#L454-L455 * https://github.com/openstack/nova/blob/52cae8801de9229d6cfb7871c2073b83b3e41b81/nova/api/openstack/compute/views/servers.py#L420-L423 [1]: https://github.com/gophercloud/gophercloud/blob/master/docs/MICROVERSIONS.md#new-response-fields
d527cb1 to
c36f263
Compare
|
Build succeeded.
|
Add to the Server type the list of server groups the server is part of,
as exposed in microversions 1.71 and above as "server_groups".
For #2216
Here are the docs describing the property of the API object "server": https://docs.openstack.org/api-ref/compute/?expanded=show-server-details-detail#show-server-details