openstack team mailing list archive

[Mark McLoughlin <markmc@xxxxxxxxxx>]

Hey,

On Tue, 2012-03-06 at 14:44 -0800, Andy Smith wrote:
> This is going to be a somewhat verbose reply.

Positively terse by my standards :)

> 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.

AFAICT, the only real issue here is the use of a global object.

Personally, I think such global objects are in very poor taste.

But this isn't so much about my taste as agreeing on a pattern which all
projects are happy to follow. If I thought there was consensus around
using a global object, I'd happily grit my teeth and make it happen.

Here's the problem, though - I can't imagine glance-core folks welcoming
a patch to adopt a global config object. it seems like a completely
backwards move. Maybe that's just me?

> 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
[..]
> >  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.

Making the library provide a global object is certainly a great way of
ratcheting up the evilness :)

Say I've two independent modules in the same process which want to parse
two separate config files? One of them uses the global object and the
other does something different? How about if the author of the both
modules never considered that it would be combined in the same process
with other cfg using code?

I could see us wanting to do something like this with the keystone auth
middleware, for example.

[..]
> >     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.

Creating mocks, DB connection objects or re-starting servers on each
test run is different from trying to restore a global object to its
state before a test run.

The CONF object is initialized the first time the config module is
imported. Each test run you need to attempt to reset the object back to
its original pristine state. Compared to just creating a config object
for each run, I do think that's a hack.

> >     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 issue was subtle enough that it went unnoticed for 3.5 months. And
previously, before the code was moved out of test_servers.py, there was
an even hackier attempt to restore the allow_admin_api flag after each
test. The global object has quite clearly been a source of confusion and
subtle brokenness.

[Snip discussion of tearDown() method strategies which I don't disagree
with]

> On the benefits of using global configuration:
> 
>  - simple to use, easy concept

Simple or not, global objects are probably only second to goto
statements in "considered harmful"[1] stakes.

Like goto statements, they can be an elegant solution in certain
restricted cases - e.g. goto can be good for error handling, globals can
be good for constants - but I think this case definitely crosses the
line over into evilness.

>  - different components do not get different configurations

Different components may want different configurations.

>  - more concise
>  - no additional patterns required besides standard top-of-file boilerplate

The top-of-file boilerplate may be slightly more concise, but it's
inelegant because of the dependence on globals.

[..]

> >  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.

Yep, I understood that. I guess I was just raising it because it's the
combination of this and removing the global object which results in the
usage patterns I wanted to discuss.

[..]
> > 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.
> >
[..]
> > 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.

I think the same is equally true of having them at the top of a class,
but by putting them in the scope of the class (where possible) makes it
easier to refactor the class into another module i.e.

  foo
  bar
  blaa

  class A:
    ..
  class B:
    ...

vs

  class A:
    foo
    bar

  class B:
    blaa

The latter definitely makes more sense to me.

[...]

Cheers,
Mark.

[1] - http://en.wikipedia.org/wiki/Considered_harmful