[RFE] Extended resources should not always calculate all attributes

Bug #1751040 reported by Tom Stappaerts
14
This bug affects 2 people
Affects Status Importance Assigned to Milestone
neutron
Confirmed
Wishlist
Unassigned

Bug Description

Currently [1] resource extensions are asked to retrieve all attributes when calling resource_extend.

This poses some problems:
- Extensions need to gather all attributes for the resource they extend
- ML2 mechanism drivers cannot make a difference between list and show of an object

We propose to:
A) Add a 'fields' argument to the extensions functions. This 'fields' argument is already present when creating the object dictionary, typically passed from ?fields=x parameters through the api.

Passing it to resource extensions would prevent them from calculating attributes that will be discarded anyway.
B) Let the clients when calling the API for list functions, use the 'fields' parameters to restrict the fields that are returned.

We think this is a positive improvement as it:
1) Prevents unneeded data transfer between api and client.
2) Provides the ability for extensions that are interfacing with external components (like an SDN controller) to only process the requested attributes.

The purpose of this RFE is to gather your comments before we start any devwork.

Components that would need changing:
- Neutron: resource_extend and all calling functions
- Ml2: extension drivers
(Both needing full backwards compatibility)
- openstackclient & openstack sdk: list api calls need to include ?fields parameters
- neutronclient: list api calls need to include ?fields parameters
- neutron-tempest-plugin: new test cases

[1]https://github.com/openstack/neutron/blob/master/neutron/db/_resource_extend.py

Changed in neutron:
assignee: nobody → Tom Stappaerts (tstappae)
information type: Public → Public Security
information type: Public Security → Private Security
information type: Private Security → Public
tags: added: rfe
Revision history for this message
Ihar Hrachyshka (ihar-hrachyshka) wrote :

Agreed that we may want to filter out extensions that won't participate in returned payload formation. One thing worth noticing is that I don't think we actually send all the data to the client that then filters out unneeded fields, as you seem to imply. Instead, we call db_utils.resource_fields(res, fields) before passing the payload dict to the root api layer, the function that filters out fields not explicitly requested. If that's the case, I am not sure which new API tests you suggest to implement. (The current API behaviour is already as expected; of course additional coverage for the 'fields' feature is welcome if it's not good enough, but this matter is independent of the proposal).

My understanding is that in case of what you propose, we would need to know which fields are generated by each registered extension, to know which extension to call to. Then extensions could also do additional filtering by themselves in case when they serve multiple fields. This probably requires a new extension registration API that would support both of these features. Is it what you envision?

Changed in neutron:
status: New → Confirmed
importance: Undecided → Wishlist
tags: added: loadimpact
Revision history for this message
Tom Stappaerts (tstappae) wrote :

Hi

The proposal is twofold:
1) At the moment, unless I am missing some configuration, CLI client does not specify which 'fields' it wants to see. So they are not filtered out by the db_utils.resource_fields(res, fields). I do agree that when the 'fields' argument is passed in API that also the fields of the extensions are filtered out. However they are still calculated, that is point 2.

2) I would only do the second part that you propose, "Then extensions could also do additional filtering by themselves in case when they serve multiple fields.", which would leave it in the hands of each extension if they need to act or not. If you deem it necessary I could also look at "we would need to know which fields are generated by each registered extension," but I believe that would require more extensive changes.

The architecture that I had in mind was to change the signature of extended functions:
resolved_func(response, db_object, fields) instead of resolved_func(response, db_object). This would mean either wrapping around legacy functions based on inspection (this would be on registration) or try-catch on execution. A new registration api is also a possibility, I do not know what the ideal situation here would be.
To be effective it would also mean changes in ML2 extension drivers, but I believe that requires fewer changes as it uses inheritance instead of registration.

Revision history for this message
Slawek Kaplonski (slaweq) wrote :

Are You still want to work on this? If so we can discuss that during one of our next drivers meeting.

Revision history for this message
Slawek Kaplonski (slaweq) wrote :

As we discussed during the PTG this idea makes sense for us.
I will add it to the next drivers meeting agenda to discuss it once again in more details (and hopefully approve officially there).
@Tom, also if You are not going to work on implementation of this, please unassign your self from it so someone else interested in working on it may take it :)

tags: added: rfe-triaged
removed: rfe
tags: added: api
Revision history for this message
Tom Stappaerts (tstappae) wrote :

Hi,
I will attend the driver meeting this friday. Unfortunately we do not have the bandwidth to do this right now. I do however have a patchset of february 2018 (!! so outdated) if anyone wants to have a look at that feel free to email me.

Changed in neutron:
assignee: Tom Stappaerts (tstappae) → nobody
Revision history for this message
Slawek Kaplonski (slaweq) wrote :

During last drivers meeting we approved this RFE. But as there is no volunteer for now to work on implementation, lets keep it approved but not target it for any specific release.
If there is anyone who wants to work on this, please reach out to us here, on OpenStack ML or on #openstack-neutron channel @freenode.

tags: added: rfe-approved
removed: rfe-triaged
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers