Open Stack

On Dec 16, 2014, at 10:32 AM, Ben Nemec <openstack at nemebean.com> wrote:

> On 12/16/2014 07:20 AM, Doug Hellmann wrote:
>> 
>> 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.
> 
> 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 :-/).  

If it does, we should probably change the interpolation code to use any option values it finds as a literal string without interpreting or validating it. That means changing the implementation to go through a different lookup path, but it sounds like we need that anyway.

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

https://review.openstack.org/#/c/140143/

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