Networking v2: Add Get Detailed Quota for project + 'trunk' extension support#2061
Networking v2: Add Get Detailed Quota for project + 'trunk' extension support#2061jtopjian merged 2 commits intogophercloud:masterfrom EmilienM:networking/details
Conversation
|
@EmilienM Thank you for submitting this! For the code review, we also need to see the actual Neutron Service API code that defines this API call. Please see here for more details: The most important part is providing a reference that the Let me know if you have any questions or need any help. |
Hey, long time no see! ;-)
Ack. I updated the PR body, let me know if it's not enough. |
Yes indeed. I hope you're doing well! 🙂
I think it is - the extension definition should be good enough to trace everything else. One thing I noticed immediately is that both the API docs and the code are using the word |
|
Build succeeded.
|
* Networking v2: Add Get Detailed Quota for Tenant
* Add GetDetail function for the quotas extension in Gophercloud's OpenStack Networking v2 API
- Allows a caller to get the list of quota types with in_use, reserved, and limit values for each
- Effectively wraps OpenStack Networking v2 API function /v2.0/quotas/{project_id}/details.json
- Inspired from the Compute API implementation.
For #2060
API reference: https://docs.openstack.org/api-ref/network/v2/index.html?expanded=show-quota-details-for-a-tenant-detail#quotas-extension-quotas
Code references:
* (extension) https://opendev.org/openstack/neutron/src/branch/master/neutron/extensions/quotasv2_detail.py
* (options) https://opendev.org/openstack/neutron/src/branch/master/neutron/conf/quota.py
You're right, it was an error on my side and in my testing I didn't have used resources so I didn't catch it. |
|
Build succeeded.
|
mdbooth
left a comment
There was a problem hiding this comment.
QuotaDetail.InUse is inconsistent with the api field, which is called user. However, it is consistent with QuotaUsage in blockstorage, and QuotaDetail in compute.
Whether we should rename the field comes down to the goals of the gophercloud project: does it aim to 'fix' inconsistencies in the OpenStack API such as this, or faithfully reproduce them? I don't know the answer.
See inline discussion about dynamic generation of quota resources by neutron. 'Request changes' is specifically for the missing trunk field. Everything else is for discussion.
|
|
||
| // QuotaDetailSet represents details of both operational limits of Networking resources for a project | ||
| // and the current usage of those resources. | ||
| type QuotaDetailSet struct { |
There was a problem hiding this comment.
This set is potentially problematic, because it's generated dynamically. It can certainly differ between clouds. E.g. my cloud outputs a 'trunk' resource:
{
"quota": {
"network": {
"limit": -1,
"used": 2,
"reserved": 0
},
"subnet": {
"limit": -1,
"used": 2,
"reserved": 0
},
"subnetpool": {
"limit": -1,
"used": 0,
"reserved": 0
},
"port": {
"limit": -1,
"used": 14,
"reserved": 0
},
"router": {
"limit": -1,
"used": 2,
"reserved": 0
},
"floatingip": {
"limit": -1,
"used": 2,
"reserved": 0
},
"rbac_policy": {
"limit": -1,
"used": 0,
"reserved": 0
},
"security_group": {
"limit": -1,
"used": 4,
"reserved": 0
},
"security_group_rule": {
"limit": -1,
"used": 46,
"reserved": 0
},
"trunk": {
"limit": -1,
"used": 6,
"reserved": 0
}
}
}
'trunk' originates here:
https://opendev.org/openstack/neutron-lib/src/branch/master/neutron_lib/api/definitions/trunk.py#L49
It is registered as a quota resource in neutron here:
https://opendev.org/openstack/neutron/src/branch/master/neutron/extensions/trunk.py
I did a quick audit of other resources which are registered in the same way and only found 'router', 'floatingip', and 'subnetpool', all of which you already have.
At a minimum you should add 'trunk'. It would again be down to the style goals of gophercloud whether this data should be returned as a struct or a map.
Something else to call out: if we continue to return a struct, any field in this struct which wasn't returned by the server will be filled with {InUse: 0, Reserved: 0, Limit: 0}. This is valid quota, so the caller wouldn't be able to distinguish this from the resource not being present at all. This might be ok, but we should probably make this decision deliberately.
There was a problem hiding this comment.
[...]
At a minimum you should add 'trunk'. It would again be down to the style goals of gophercloud whether this data should be returned as a struct or a map.
I'll add trunk (since we'll need it for our needs), but I suspect there are more extensions that register a resource in the quotas, they can be added later.
There was a problem hiding this comment.
Every field in this struct registered dynamically, btw, not just trunk. router, floatingip, and subnetpool are also registered in the same way. Also, the whole of neutron seems to be extensions. I wouldn't call out trunk as being different: it's probably just the most recent extension to be added. With an architecture like this you can almost guarantee that the api docs for quota detail will often/usually be out of date.
There was a problem hiding this comment.
What is the expected QuotaDetail value when the resource is not in the response?
If this question has a satisfying answer, then the struct would be much more usable than a map on the consumer side.
|
Questions for a regular gophercloud maintainer:
Map advantages:
|
We always stick to what the API is returning. If Gophercloud is inconsistent, it's because the APIs are inconsistent 🙂
Good question. I think this all depends on how often the resources in the returned result will differ. As an extreme example, if a cloud admin was able to create arbitrary network resources via the API, then yes, this absolutely should be a map. But on the other side of the spectrum, if Neutron (as a collection of extensions) ultimately only has a finite set of resources, then a struct probably works better. If it's common for very large OpenStack environments or forks of OpenStack to add additional Neutron extensions that register in-house/site-specific resources, then that shouldn't be taken into consideration here. Gophercloud only ensures compatibility with "vanilla" OpenStack. For this situation, the in-house environment would need to create a Go package that redefines the This is not set in stone - just the general guidelines used in Gophercloud. Happy to discuss more. |
|
Build succeeded.
|
Both of those are the answers I was expecting :) I'm happy with this patch if InUse is renamed to Used and a Trunk field is added to the struct. |
|
Build succeeded.
|
When the 'trunk' extension is enabled, it's possible to set quotas for trunks resources, the same way as other networking resources. 'trunk' originates here: https://opendev.org/openstack/neutron-lib/src/branch/master/neutron_lib/api/definitions/trunk.py#L49 It is registered as a quota resource in neutron here: https://opendev.org/openstack/neutron/src/branch/master/neutron/extensions/trunk.py Closes #2064
Done. Thanks for the reviews everyone! |
|
Build succeeded.
|
trunks resources, the same way as other networking resources.
For #2060 (detailed quota)
For #2064 (trunk support)
API reference: https://docs.openstack.org/api-ref/network/v2/index.html?expanded=show-quota-details-for-a-tenant-detail#quotas-extension-quotas
Code references: