openstack team mailing list archive

[Andy Smith <andyster@xxxxxxxxx>]

This is going to be a somewhat verbose reply.

TL;DR:

Summary, I'd like cfg.py to be config.py (pipe dream, I'm sure, but I don't
really agree with the abbreviation), to provide a global configuration
object,
and to have shortcuts for registration of options, that projects should
standardize on. And I'm willing to help make those changes.


On Tue, Mar 6, 2012 at 2:10 AM, Mark McLoughlin <markmc@xxxxxxxxxx> wrote:

> Hey,
>
> The original cfg design[1] assumed certain usage patterns that I hoped
> would be adopted by all projects using it. In gerrit, we're debating a
> set of patch to make keystone use these patterns:
>
>  https://review.openstack.org/4547
>
> I thought it was best to move some of that discussion here since I'm
> hoping we can get some rough consensus across projects. I really think
> it will be beneficial if we can share common idioms and patterns across
> projects, rather than just using the same library in different ways.
>
>
> So, what I'm proposing is:
>
>  1) We stop using global objects like FLAGS or CONF and instead pass
>     the config object to code which uses it.
>
>     On an evilness scale, these global objects may be relatively
>     benign but they do still harm modularity. Any module that depends
>     on these global objects cannot be re-used without ensuring the
>     global object exists in the user e.g. doing this:
>
>       from nova import flags
>
>       FLAGS = flags.FLAGS
>
>     is a nice way of ensuring code is nova specific, even if there is
>     no other reason for it to be tied to nova.
>


This is not true if the cfg stuff is providing the global object,
as then it will be provided by the library rather than the project. We only
aren't at this point yet because we haven't moved to openstack-common fully.


>
>     Also, these global objects force us to do a bunch of hacks in unit
>     tests. We need to do tricks to ensure the object is initialized as
>     we want. We also need to save and restore its state between runs.
>

I don't agree with the statement that they force "a bunch of hacks,"
clearing
state is a perfectly normal thing to do, it is done for any servers that get
started, any mocks that are made, and every test database. Making sure that
modifications to a configuration object are cleaned up is no different:
there
is no "save and restore" just always start from a blank slate and set things
as required, same as in the non-global model.



>     Subtle mistakes here might mean that a test passes when all the
>     tests are run together, but fail when run individually[2].
>
>
Subtle mistakes are still subtle mistakes, though the commit you reference
was really a pretty large one (and the solution is incorrect as well). The
original developer created an unnecessary tearDown call (the base class does
all those things already) and neglected to call the parent class, this is a
complete bug in all situations considering how much stuff the parent class
is already doing and is akin to not calling the parent class for setUp
(which
would likely result in no database being available amongst other issues).
That the original code or the solution made it past review is a significant
oversight.

(The correct solution would have been to delete the assignment of self.stubs
in setUp and delete the tearDown method)

There are plenty of ways to mitigate risk, but few outside of metaclasses
are going to solve the problem of somebody not calling tearDown, though
simply moving some code to setUp would probably solve the symptom.

...

On the benefits of using global configuration:

 - simple to use, easy concept
 - different components do not get different configurations
 - more concise
 - no additional patterns required besides standard top-of-file boilerplate

Now, there are a couple changes we'll want to make to cfg.py to support
global use more robustly, but there really aren't many. Additionally, it
will
be very easy to move glance to global conf.



>     If you want to get a feel for the end result when avoiding using
>     global objects, look at glance or my cfg-cleanup[3] keystone
>     branch.
>
>  2) We define options close to where they are used.
>
>     Again, this is for modularity. If some code relies on code in
>     another module registering an option, it creates a dependence
>     between the modules.
>

This has never been an issue in my mind, so if you thought it was I may have
miscommunicated.

Anyway, I'm in full agreement about defining options close to where they
are used and always have been, in Keystone specifically the only reason
they aren't in those spots has just been because things have moved so much
in a short time that it was easier not having to move config options, too.
I support moving them to appropriate places at anybody's leisure.




>
> In practice, this resulted in two patterns. Firstly, where a set of
> options are used exclusively within a class:
>
>  class ExtensionManager(object):
>
>      enabled_apis_opt = cfg.ListOpt('enabled_apis',
>                                     default=['ec2', 'osapi'],
>                                     help='List of APIs to enable by
> default')
>
>      def __init__(self, conf):
>          self.conf = conf
>          self.conf.register_opt(enabled_apis_opt)
>          ...
>
>      def _load_extensions(self):
>          for ext_factory in self.conf.osapi_extension:
>              ....
>
> Here you have the options used by the class clearly declared at the
> beginning of the class definition and a config instance passed to the
> constructor.
>
> Secondly, where options are used by functions within a module:
>
>    crypt_strength_opt = cfg.IntOpt('crypt_strength', default=40000)
>
>    def hash_password(conf, password):
>        """Hash a password. Hard."""
>        password_utf8 = password.encode('utf-8')
>        if passlib.hash.sha512_crypt.identify(password_utf8):
>            return password_utf8
>        conf.register_opt(crypt_strength_opt)
>        h = passlib.hash.sha512_crypt.encrypt(password_utf8,
>                                              rounds=conf.crypt_strength)
>        return h
>
> Here you have the options declared at the beginning of the module and a
> config instance passed to functions which use options. The option schema
> is registered by such a function just before the option is used.
>

My preference here is to define all options at the top of the file, largely
because one is often looking for a config option, possibly to figure out
what its default is, usually to figure out where to change its default, and
having them defined at the top of the file makes sure they are always where
you expect them to be.



>
>
> One complaint about the above pattern is that it's more verbose than
> simply doing e.g.
>
>    CONF = config.CONF
>
>    config.register_opt('crypt_strength', default=40000)
>
>    def hash_password(password):
>        ...
>        h = passlib.hash.sha512_crypt.encrypt(password_utf8,
>                                              rounds=CONF.crypt_strength)
>        return h
>
>
> The obvious way of making it less verbose while still removing the
> dependence on global objects would be:
>
>    def hash_password(conf, password):
>        ...
>        conf.register_opt('crypt_strength', default=40000)
>        h = passlib.hash.sha512_crypt.encrypt(password_utf8,
>                                              rounds=conf.crypt_strength)
>        return h
>
> However, that does have the effect of obscuring the option definitions
> somewhat.
>


Since I am for the global config option with all options defined at the top
of the file, I don't see a problem here :p Though I would have the
registration options be named register_str and register_int (just a typo
in your code above, I assume).



>
>
> Any thoughts or ideas on all of this?
>


(I'll post this to the top as a tl;dr)

Summary, I'd like cfg.py to be config.py (pipe dream, I'm sure, but I don't
really agree with the abbreviation), to provide a global configuration
object,
and to have shortcuts for registration of options, that projects should
standardize on. And I'm willing to help make those changes.



>
> Thanks,
> Mark.
>
> [1] - http://wiki.openstack.org/CommonConfigModule
> [2] - https://github.com/openstack/nova/commit/4eeb0b96fc
> [3] - https://github.com/markmc/keystone/tree/cfg-cleanup
>
>