placement inadvertently imports many python modules it does not need

Bug #1743120 reported by Chris Dent
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Compute (nova)
Fix Released
Low
Chris Dent

Bug Description

The placement WSGI app has its own WSGI framework and is the only script/binary that placement uses. From the outset, it has been intended that a deployment could run multiple placement servers, distributed across multiple hosts/pods/whatever, with some form of load balancing proxy in front.

In that model, it would be ideal for the app to require as few python module as possible, and have as small a footprint, at least starting up, as possible.

Currently, that's not the case, for two reasons:

* Placement makes use of the FaultWrapper WSGI middleware to catch unexpected exceptions and turn them into status 500 HTTP responses. FaultWrapper imports nova.utils and nova.utils imports a lot of stuff.

* Placement is within the nova/api/openstack package hierarchy and nova/api/openstack/__init__.py contains active code and imports which placement does not need.

These problems can be addressed:

* FaultWrapper is overkill for the placement WSGI app. FaultWrapper is capable of decoding a variety of exceptions into a variety of status responses. This is not required by placement. The only exceptions that can make it to FaultWrapper in placement are those that would result in a 500. A simpler middleware can handle this.

* nova/api/openstack/__init__.py should be emptied, the contents moved to a file which is explicitly imported by those modules that actually want it. Presumably, this could also provide an avenue for lightening the metadata API.

Note that this is not the only case of inadvertent imports, but is one of the main vectors. Others will be identified and additional bugs created.

Tags: placement
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to nova (master)

Fix proposed to branch: master
Review: https://review.openstack.org/533752

Changed in nova:
status: Triaged → In Progress
Revision history for this message
Chris Dent (cdent) wrote :

This going to be fairly complicated to make much headway on, unless the resource_provider objects are moved out from under nova/objects. __init__.py there registers all those objects, each of which imports things from all over the nova hierarchy. One notable example are the ec2 objects which import nova/api/ec2/ec2utils which imports dogpile so it can memoize some operations (are they ever called?).

Also, nova/__init__.py imports oslo_service and eventlet, neither of which placement wants to see. This could perhaps be moved back to nova/cmd and wherever else is actually required.

Revision history for this message
Chris Dent (cdent) wrote :

I messed up the bug reference, so there's also:
 https://review.openstack.org/#/c/533797/

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to nova (master)

Reviewed: https://review.openstack.org/533752
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=5f881ff030c119b50bf1929b9829f52493ac447c
Submitter: Zuul
Branch: master

commit 5f881ff030c119b50bf1929b9829f52493ac447c
Author: Chris Dent <email address hidden>
Date: Sat Jan 13 16:11:51 2018 +0000

    [placement] use simple FaultWrapper

    Prior to this patch placement was using the nova FaultWrapper
    middleware as a final guard against unexpected exceptions. That
    middleware does more than required and also imports stuff that
    placement does not otherwise require.

    The new middleware is only for exceptions that have fallen through
    previous handling and need to be turned into 500 responses, formatted
    to JSON, and logged.

    While doing this it became clear that the limited Exception catching
    in handler.py was redundant if FaultWrapper and the Microversion
    middlewares (both required for the operation of the system) are
    involved, so that has been removed, with a comment.

    This should not require a microversion as the structure of successful
    responses and expected error responses does not change. What does
    change is that a 500 response will be formatted more as the api-wg
    errors guideline[1] desires.

    [1] http://specs.openstack.org/openstack/api-wg/guidelines/errors.html

    Change-Id: Ib2c3250b964a30239f298cdf13ea2020f205f67d
    Partial-Bug: #1743120

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/533797
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=ef6f4e4c8ec82e2c9f9988fe2e04591ee01220e6
Submitter: Zuul
Branch: master

commit ef6f4e4c8ec82e2c9f9988fe2e04591ee01220e6
Author: Chris Dent <email address hidden>
Date: Mon Jan 15 20:58:48 2018 +0000

    Refactor WSGI apps and utils to limit imports

    The file nova/api/openstack/__init__.py had imported a lot of
    modules, notably nova.utils. This means that any code which
    runs within that package, notably the placement service, imports
    all those modules, even if it is not going to use them. This
    results in scripts/binaries that are heavier than they need
    to be and in some cases including modules, like eventlet, that
    it would feel safe to not have in the stack.

    Unfortunately we cannot sinply rename nova/api/openstack/__init__.py
    to another name because it contains FaultWrapper and FaultWrapper
    is referred to, by package path, from the paste.ini file and that
    file is out there in config land, and something we prefer not to
    change. Therefore alternate methods of cleaning up were explored
    and this has led to some useful changes:

    Fault wrapper is the only consumer of walk_class_hierarchy so
    there is no reason for it it to be in nova.utils.

    nova.wsgi contains a mismash of WSGI middleware and applications,
    which need only a small number of imports, and Server classes
    which are more complex and not required by the WSGI wares.

    Therefore nova.wsgi was split into nova.wsgi and nova.api.wsgi.
    The name choices may not be ideal, but they were chosen to limit
    the cascades of changes that are needed across code and tests.

    Where utils.utf8 was used it has been replaced with the similar (but not
    exactly equivalient) method from oslo_utils.encodeutils.

    Change-Id: I297f30aa6eb01fe3b53fd8c9b7853949be31156d
    Partial-Bug: #1743120

Revision history for this message
Chris Dent (cdent) wrote :

We can call this done now that openstack/placement exists

Changed in nova:
status: In Progress → Fix Committed
Changed in nova:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.