On 12/16/2014 07:20 AM, Doug Hellmann wrote: > > On Dec 16, 2014, at 7:41 AM, Mark McLoughlin <[email protected]> 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 > [email protected] > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > _______________________________________________ OpenStack-dev mailing list [email protected] http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
