Open Stack

On Dec 16, 2014, at 7:41 AM, Mark McLoughlin <markmc at redhat.com> wrote:

> Hi Doug,
> 
> On Mon, 2014-12-08 at 15:58 -0500, Doug Hellmann wrote:
>> As we’ve discussed a few times, we want to isolate applications from
>> the configuration options defined by libraries. One way we have of
>> doing that is the ConfigFilter class in oslo.config. When a regular
>> ConfigOpts instance is wrapped with a filter, a library can register
>> new options on the filter that are not visible to anything that
>> doesn’t have the filter object.
> 
> Or to put it more simply, the configuration options registered by the
> library should not be part of the public API of the library.
> 
>> Unfortunately, the Neutron team has identified an issue with this
>> approach. We have a bug report [1] from them about the way we’re using
>> config filters in oslo.concurrency specifically, but the issue applies
>> to their use everywhere. 
>> 
>> The neutron tests set the default for oslo.concurrency’s lock_path
>> variable to “$state_path/lock”, and the state_path option is defined
>> in their application. With the filter in place, interpolation of
>> $state_path to generate the lock_path value fails because state_path
>> is not known to the ConfigFilter instance.
> 
> It seems that Neutron sets this default in its etc/neutron.conf file in
> its git tree:
> 
>  lock_path = $state_path/lock
> 
> I think we should be aiming for defaults like this to be set in code,
> and for the sample config files to contain nothing but comments. So,
> neutron should do:
> 
>  lockutils.set_defaults(lock_path="$state_path/lock")
> 
> That's a side detail, however.
> 
>> The reverse would also happen (if the value of state_path was somehow
>> defined to depend on lock_path),
> 
> This dependency wouldn't/shouldn't be code - because Neutron *code*
> shouldn't know about the existence of library config options.
> Neutron deployers absolutely will be aware of lock_path however.
> 
>> and that’s actually a bigger concern to me. A deployer should be able
>> to use interpolation anywhere, and not worry about whether the options
>> are in parts of the code that can see each other. The values are all
>> in one file, as far as they know, and so interpolation should “just
>> work”.
> 
> Yes, if a deployer looks at a sample configuration file, all options
> listed in there seem like they're in-play for substitution use within
> the value of another option. For string substitution only, I'd say there
> should be a global namespace where all options are registered.
> 
> Now ... one caveat on all of this ... I do think the string substitution
> feature is pretty obscure and mostly just used in default values.
> 
>> I see a few solutions:
>> 
>> 1. Don’t use the config filter at all.
>> 2. Make the config filter able to add new options and still see
>> everything else that is already defined (only filter in one
>> direction).
>> 3. Leave things as they are, and make the error message better.
> 
> 4. Just tackle this specific case by making lock_path implicitly
> relative to a base path the application can set via an API, so Neutron
> would do:
> 
>  lockutils.set_base_path(CONF.state_path)
> 
> at startup.
> 
> 5. Make the toplevel ConfigOpts aware of all filters hanging off it, and
> somehow cycle through all of those filters just when doing string
> substitution.

We would have to allow the reverse as well, since the filter object doesn’t see options not explicitly imported by the code creating the filter.

In either case, it only works if the filter object has been instantiated. I wonder if we have a similar problem with runtime option registration. I’ll have to test that.


> 
>> Because of the deployment implications of using the filter, I’m
>> inclined to go with choice 1 or 2. However, choice 2 leaves open the
>> possibility of a deployer wanting to use the value of an option
>> defined by one filtered set of code when defining another. I don’t
>> know how frequently that might come up, but it seems like the error
>> would be very confusing, especially if both options are set in the
>> same config file.
>> 
>> I think that leaves option 1, which means our plans for hiding options
>> from applications need to be rethought.
>> 
>> Does anyone else see another solution that I’m missing?
> 
> I'd do something like (3) and (4), then wait to see if it crops up
> multiple times in the future before tackling a more general solution.

Option 3 prevents neutron from adopting oslo.concurrency, and option 4 is a backwards-incompatible change to the way lock path is set.

> 
> With option (1), the basic thing to think about is how to maintain API
> compatibility - if we expose the options through the API, how do we deal
> with future moves, removals, renames, and changing semantics of those
> config options.

The option is exposed through the existing set_defaults() method, so we can make that handle any backwards compatibility issues if we change it.

> 
> Mark.
>