openstack team mailing list archive

[Mark McLoughlin <markmc@xxxxxxxxxx>]

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.

     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. 
     Subtle mistakes here might mean that a test passes when all the 
     tests are run together, but fail when run individually[2].

     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.


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.


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.


Any thoughts or ideas on all of this?

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