On Dec 16, 2014, at 10:32 AM, Ben Nemec <openst...@nemebean.com> wrote:
> 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 :-/). 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@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 _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
