Open Stack

Excerpts from Daniel P. Berrange's message of 2015-07-24 15:11:19 +0100:
> On Fri, Jul 24, 2015 at 09:56:41AM -0400, Doug Hellmann wrote:
> > Excerpts from Daniel P. Berrange's message of 2015-07-24 09:48:15 +0100:
> > > On Thu, Jul 23, 2015 at 05:55:36PM +0300, mhorban wrote:
> > > > Hi all,
> > > > 
> > > > During development process in nova I faced with an issue related with config
> > > > options. Now we have lists of config options and registering options mixed
> > > > with source code in regular files.
> > > > From one side it can be convenient: to have module-encapsulated config
> > > > options. But problems appear when we need to use some config option in
> > > > different modules/packages.
> > > > 
> > > > If some option is registered in module X and module X imports module Y for
> > > > some reasons...
> > > > and in one day we need to import this option in module Y we will get
> > > > exception
> > > > NoSuchOptError on import_opt in module Y.
> > > > Because of circular dependency.
> > > > To resolve it we can move registering of this option in Y module(in the
> > > > inappropriate place) or use other tricks.
> > > > 
> > > > I offer to create file options.py in each package and move all package's
> > > > config options and registration code there.
> > > > Such approach allows us to import any option in any place of nova without
> > > > problems.
> > > > 
> > > > Implementations of this refactoring can be done piece by piece where piece
> > > > is
> > > > one package.
> > > > 
> > > > What is your opinion about this idea?
> > > 
> > > I tend to think that focusing on problems with dependancy ordering when
> > > modules import each others config options is merely attacking a symptom
> > > of the real root cause problem.
> > > 
> > > The way we use config options is really entirely wrong. We have gone
> > > to the trouble of creating (or trying to create) structured code with
> > > isolated functional areas, files and object classes, and then we throw
> > > in these config options which are essentially global variables which are
> > > allowed to be accessed by any code anywhere. This destroys the isolation
> > > of the various classes we've created, and means their behaviour often
> > > based on side effects of config options from unrelated pieces of code.
> > > It is total madness in terms of good design practices to have such use
> > > of global variables.
> > > 
> > > So IMHO, if we want to fix the real big problem with config options, we
> > > need to be looking to solution where we stop using config options as
> > > global variables. We should change our various classes so that the
> > > neccessary configurable options as passed into object constructors
> > > and/or methods as parameters.
> > 
> > We've tried to do this in a lot of places throughout Oslo. It mostly
> > works, until you hit a driver that uses options that the other drivers
> > don't (oslo.messaging has this problem especially).
> > 
> > We had a ConfigFilter class in oslo.config to enforce the isolation (an
> > option registered on a filter object is only visible to the code that
> > owns the filter). However, that causes challenges for value
> > interpolation. A deployer doesn't know which options are visible to each
> > other, and has a flat file where they can clearly see all of the
> > values together, so they expect %(foo)s to work no matter where "foo" is
> > defined. Until we solve those issues, the ConfigFilter isn't really
> > supported.
> > 
> > The best solution we've come up with so far is to isolate the options
> > into groups based on the driver name, and then not allow code outside of
> > the driver to access those options for any reason.
> > 
> > To deal with import ordering issues, we pass ConfigObj instance
> > around, and register options at runtime instead of import time,
> > usually in an __init__ method.  Registering an option more than
> > once is fine.
> 
> Yep, I would imagine that the nova/cmd/*.py command line entry
> points would trigger loading of the config file and pass the
> object into the service they run. The service may pass that
> config object on down to some classes, eg I'd expect the nova
> ComputeDriver to accept a config object in its constructor.
> Likewise the various manager classes like nova/compute/manager.py
> These would read the various config option values they care about
> and pass those into the various methods / objects that need them
> 
> > > As an example in the libvirt driver.
> > > 
> > > I would set it up so that /only/ the LibvirtDriver class in driver.py
> > > was allowed to access the CONF config options. In its constructor it
> > > would load all the various config options it needs, and either set
> > > class attributes for them, or pass them into other methods it calls.
> > > So in the driver.py, instead of calling CONF.libvirt.libvirt_migration_uri
> > > everywhere in the code,  in the constructor we'd save that config param
> > > value to an attribute 'self.mig_uri = CONF.libvirt.libvirt_migration_uri'
> > > and then where needed, we'd just call "self.mig_uri".
> > 
> > There are, from time to time, specs proposed to provide ways for
> > applications to reload their configuration settings without restarting.
> > So far we've said that was an application issue, because oslo.config
> > can reload the files, but it doesn't know when (that's an app signal
> > handling thing) and it doesn't know where values might have been
> > saved like you describe.  I don't know if that's something the nova
> > team wants to support.
> 
> I don't think you can magically reload stuff on the fly regardless
> of how we store or access the configuration settings, and in fact
> I think our current global variable approach makes it even harder
> to deal with that too.
> 
> It is not uncommon to read a config parameter and use that to
> configure some state somewhere. The CONF object is magically
> updated to have the new config value when the user editted the
> file. Now we have some code reading CONF.foo directly and acting
> on it, and other code using the previously setup state based on
> the old CONF.foo value. This will certainly crash & burn in a
> horrible way.
> 
> To be able to support live updates of config parameters, we need
> to make the code using those parameters aware of it and ensure the
> change takes effect everywhere that's needed at the same time.
> If the config options are merely used to set attributes on classes
> or passed in as method parameters, we can clearly understand the
> implications of changing the config value on the fly, and call a
> suitable method to change it. This is really all just standard
> good design practice for object oriented programming and why mixing
> in use of global variables with OO design is a horrible idea in
> general.

Yep, that's why we consider it an application concern, and not a library
concern. I only raise the point in case you hadn't seen those requests,
since it would mean doing some work either way.

Doug