Open Stack

On Fri, Jul 24, 2015 at 12:29:56PM -0400, Doug Hellmann wrote:
> Excerpts from Joshua Harlow's message of 2015-07-24 09:07:13 -0700:
> > Daniel P. Berrange wrote:
> > > 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.
> > 
> > Amen to that :)
> > 
> > >
> > > 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.
> > >
> > > 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".
> > >
> > > Now in the various other libvirt files, imagebackend.py, volume.py
> > > vif.py, etc. None of those files would /ever/ access CONF.*. Any time
> > > they needed a config parameter, it would be passed into their constructor
> > > or method, by the LibvirtDriver or whatever invoked them.
> > 
> > +1 and IMHO if some driver needs some 'funky' new parameter that isn't 
> > getting passed previously, probably the driver may be built wrong, or 
> > it's not conforming to the API that is provided (and therefore either 
> > the API needs adjustments or the driver needs to be fixed); all the 
> > above IMHO is pretty standard OOP practices and I'd like to see more of 
> > it honestly :)
> 
> While it's good to share common options as much as possible, there
> are a lot of perfectly legitimate reasons for drivers to need
> different configuration options. Different backends might use different
> authentication mechanisms, or need different ways to specify "where" the
> thing the driver is managing actually lives. The job of the driver API
> layer is to hide those differences so the application doesn't have to
> care about them. Pushing all of the options up to be API parameters
> defeats the purpose of that if not all drivers use all of the options.

Agreed, that's why I suggested the ComputeDriver constructor would
probably just accept a config object - each subclass has valid
reasons for wanting its own set of config parameters in this case.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|