Possible improvements to config/initialization

Bug #1407649 reported by Stuart McLaren
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
osprofiler
New
Undecided
Unassigned

Bug Description

In Glance, currently the osprofiler initialization looks like this:

        if cfg.CONF.profiler.enabled:
            _notifier = osprofiler.notifier.create("Messaging",
                                                   notifier.messaging, {},
                                                   notifier.get_transport(),
                                                   "glance", "api",
                                                   cfg.CONF.bind_host)
            osprofiler.notifier.set(_notifier)
        else:
            osprofiler.web.disable()

The config sections in glance-api.conf and glance-registry.conf look like:

 [profiler]
 # If False fully disable profiling feature.
 #enabled = False

 # If False doesn't trace SQL requests.
 #trace_sqlalchemy = False

and the wsgi pipeline config looks like:

 # Use this pipeline for keystone auth
 [pipeline:glance-api-keystone]
 pipeline = versionnegotiation osprofiler authtoken context rootapp

The initialisation/configurization of osprofiler is different to other pipeline entries, such as keystone.

In keystone the initialization/configuration of the middleware is done as follows:

1) there is no explicit call to initialize the keystone middleware -- it bootstraps itself
2) the keystone middleware options are defined in the middleware itself

see https://github.com/openstack/keystonemiddleware/blob/master/keystonemiddleware/auth_token.py

it is *those* built-in options that are referenced in every projects' configuration files (including glance-api.conf):

 [keystone_authtoken]
 identity_uri = http://127.0.0.1:35357
 admin_tenant_name = %SERVICE_TENANT_NAME%
 admin_user = %SERVICE_USER%
 admin_password = %SERVICE_PASSWORD%
 revocation_cache_time = 10

As far as I see, the advantages to the keystone approach are:

1) keystone is enabled/disabled simply by placing it in or removing it from the wsgi pipeline

unlike keystone, osprofiler code is loaded even when it is not in the pipeline. It is possible to put osprofiler in an inconsistent state by having it 'enabled' but leaving it out of the pipeline -- that is not possible with keystone.

2) options are guaranteed to be consistent across projects

this is not the case with osprofiler.

3) projects do not have to implement their own versions of osprofiler options/stanza.

for osprofiler each project has to implement it's own options rather than inheriting them 'for free' from osprofiler.

I would propose that osprofiler provides a new config stanza [osprofiler] which is consistent across projects.

It should allow projects to spin up osprofiler by whether it is contained in the pipeline or not.

Glance would be able to remove completely the "if cfg.CONF.profiler.enabled" code block once the [osprofiler] stanza and the relevant options is available. (Other projects could ideally also remove their equivalent setup code).

Revision history for this message
Zhi Yan Liu (lzy-dev) wrote :

Thanks Stuart, I think this is a good point to discuss.

>>> The initialisation/configurization of osprofiler is different to other pipeline entries, such as keystone.

I think a key reason is middleware is only a min optional part of osprofiler, for example in proposed change of zarqa [0], it didn't use paste pipeline, but it still need "osprofiler.notifier.create()" call due to whatever we use web middleware of osprofiler, we all need to initialize notifier for osprofiler internal usage base on it integrated project, for example, in glance we could use oslo.messaging notifier directly due to glance integrated oslo.messaging, but for trove, currently oslo.messaging integration change is still wip (very close [2]) so we need to write a customized notifier in project, and create and set notifier to osprofiler. So overall, I mean osprofiler can't bootstraps itself without extra initialization step.

Another reason of why osprofiler way unlikes "there is no explicit call to initialize the keystone middleware" is that historically as the first osprofiler integrated project glance team asked to add a main 'enabled' option to switch osprofiler if disabled totally (and I think it's necessary/useful, especially for the concern from performance perspective).

>>> the keystone middleware options are defined in the middleware itself

I trend to agree this, we can move there essential options from integrated project to a proper place in osprofiler, e.g. 'enabled', 'hmac_keys', and 'trace_sqlalchemy', however project probably need to add own options for particular profiling requirement, for example, in zaqar we'd like to allow operator to configure profile switcher on different driver [3].

So in sort, I think the key to me is that, osprofiler is not just middleware, but it's a lib. So we can't just 'to remove completely the "if cfg.CONF.profiler.enabled" code block once the [osprofiler] stanza and the relevant options is available.' then to make it works.

[0] Change I32565de6c447cd5e95a0ef54a9fbd4e571c2d820
[1] Change I580cce8d2b3c4ec9ce625ac09de6f14e1249f6f5
[2] Change Ibd886f3cb4a45250c7c434b3af711abee266671c
[3] https://review.openstack.org/#/c/141356/6/zaqar/profile.py #L33-42

If I miss anything please feel free to point it out.

Revision history for this message
Stuart McLaren (stuart-mclaren) wrote :

Hi Zhi Yan,

Many thanks for your comments. You clearly know more about osprofiler than I do :-)

Some notes on why I'm interested in this:
Part of the reason I'm interested in this is the 'zero downtime configuration' stuff I've been working on.
I'd like some of the api/registry specific parts of the api.py or registry.py startup scripts to be removed if possible and make the scripts simpler. I'd like to be able to pick up any osprofiler changes make before a 'restart' of the service. Currently osprofiler makes this tricky as, once we are in wsgi.py there is no way to know (currently) if the process is an api or registry process (that asymmetry is handled in api.py/registry.py, but they won't be invoked on a restart)

Glance could probably work around osprofiler's current config/bootstrap, but it got me thinking about whether things could be improved.

> we need to write a customized notifier in project, and create and set notifier to osprofiler. So overall, I mean osprofiler can't bootstraps itself without extra initialization step.

Could that be done similarly to this?

 data_api = glance.db.registry.api
 (in glance-api.conf)

From a service restart point of view, it would be great if you just changed a 'native' osprofiler option and didn't need to make an 'init' call. However, if you need a one line "init_osprofiler()" that was called on service startup (or restart) somewhere inside glance's wsgi.py that wouldn't be too bad (as long as it could handle the api/registry case, ie the port number and service name were native osprofiler options).

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.