On 12/16/2014 07:20 AM, Doug Hellmann wrote:
> 
> On Dec 16, 2014, at 7:41 AM, Mark McLoughlin <mar...@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.

This doesn't seem like it should be difficult to do though.  The
ConfigFilter already takes a conf object when it gets initialized so it
should have access to all of the globally registered opts.  I'm a little
surprised it doesn't already.

I'm actually not 100% sure it makes sense to allow application opts to
reference library opts since the application shouldn't depend on a
library setting, but since the config file is flat I don't know that we
can enforce that separation so _somebody_ is going to try to do it and
be confused why it doesn't work.

So I guess I feel like making opt interpolation work in both directions
is the "right" way to do this, but it's kind of a moot point if runtime
registration breaks this anyway (which it probably does :-/).  Improving
the error message to explain why a particular value can't be used for
interpolation might be the only not insanely complicated way to
completely address this interpolation issue.

> 
> 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.
>>
> 
> 
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 


_______________________________________________
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Reply via email to