<div>This is going to be a somewhat verbose reply.</div><div><br></div><div>TL;DR:</div><div><br></div><div>Summary, I'd like cfg.py to be config.py (pipe dream, I'm sure, but I don't</div><div>really agree with the abbreviation), to provide a global configuration object,</div>
<div>and to have shortcuts for registration of options, that projects should</div><div>standardize on. And I'm willing to help make those changes.</div><div><br></div><br><div class="gmail_quote">On Tue, Mar 6, 2012 at 2:10 AM, Mark McLoughlin <span dir="ltr"><<a href="mailto:markmc@redhat.com">markmc@redhat.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hey,<br>
<br>
The original cfg design[1] assumed certain usage patterns that I hoped<br>
would be adopted by all projects using it. In gerrit, we're debating a<br>
set of patch to make keystone use these patterns:<br>
<br>
<a href="https://review.openstack.org/4547" target="_blank">https://review.openstack.org/4547</a><br>
<br>
I thought it was best to move some of that discussion here since I'm<br>
hoping we can get some rough consensus across projects. I really think<br>
it will be beneficial if we can share common idioms and patterns across<br>
projects, rather than just using the same library in different ways.<br>
<br>
<br>
So, what I'm proposing is:<br>
<br>
1) We stop using global objects like FLAGS or CONF and instead pass<br>
the config object to code which uses it.<br>
<br>
On an evilness scale, these global objects may be relatively<br>
benign but they do still harm modularity. Any module that depends<br>
on these global objects cannot be re-used without ensuring the<br>
global object exists in the user e.g. doing this:<br>
<br>
from nova import flags<br>
<br>
FLAGS = flags.FLAGS<br>
<br>
is a nice way of ensuring code is nova specific, even if there is<br>
no other reason for it to be tied to nova.<br></blockquote><div><br></div><div><br></div><div>This is not true if the cfg stuff is providing the global object,</div><div>as then it will be provided by the library rather than the project. We only</div>
<div>aren't at this point yet because we haven't moved to openstack-common fully.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Also, these global objects force us to do a bunch of hacks in unit<br>
tests. We need to do tricks to ensure the object is initialized as<br>
we want. We also need to save and restore its state between runs.<br></blockquote><div><br></div><div><div>I don't agree with the statement that they force "a bunch of hacks," clearing</div><div>state is a perfectly normal thing to do, it is done for any servers that get</div>
<div>started, any mocks that are made, and every test database. Making sure that</div><div>modifications to a configuration object are cleaned up is no different: there</div><div>is no "save and restore" just always start from a blank slate and set things</div>
<div>as required, same as in the non-global model.</div></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Subtle mistakes here might mean that a test passes when all the<br>
tests are run together, but fail when run individually[2].<br>
<br></blockquote><div><br></div><div><div>Subtle mistakes are still subtle mistakes, though the commit you reference</div><div>was really a pretty large one (and the solution is incorrect as well). The</div><div>original developer created an unnecessary tearDown call (the base class does</div>
<div>all those things already) and neglected to call the parent class, this is a</div><div>complete bug in all situations considering how much stuff the parent class</div><div>is already doing and is akin to not calling the parent class for setUp (which</div>
<div>would likely result in no database being available amongst other issues).</div><div>That the original code or the solution made it past review is a significant</div><div>oversight.</div></div><div><br></div><div><div>
(The correct solution would have been to delete the assignment of self.stubs</div><div>in setUp and delete the tearDown method)</div><div><br></div><div>There are plenty of ways to mitigate risk, but few outside of metaclasses</div>
<div>are going to solve the problem of somebody not calling tearDown, though</div><div>simply moving some code to setUp would probably solve the symptom.</div><div><br></div><div>...</div><div><br></div><div>On the benefits of using global configuration:</div>
<div><br></div><div> - simple to use, easy concept</div><div> - different components do not get different configurations</div><div> - more concise</div><div> - no additional patterns required besides standard top-of-file boilerplate</div>
<div><br></div><div>Now, there are a couple changes we'll want to make to cfg.py to support</div><div>global use more robustly, but there really aren't many. Additionally, it will</div><div>be very easy to move glance to global conf.</div>
</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
If you want to get a feel for the end result when avoiding using<br>
global objects, look at glance or my cfg-cleanup[3] keystone<br>
branch.<br>
<br>
2) We define options close to where they are used.<br>
<br>
Again, this is for modularity. If some code relies on code in<br>
another module registering an option, it creates a dependence<br>
between the modules.<br></blockquote><div><br></div><div>This has never been an issue in my mind, so if you thought it was I may have</div><div>miscommunicated.</div><div><br></div><div>Anyway, I'm in full agreement about defining options close to where they</div>
<div>are used and always have been, in Keystone specifically the only reason</div><div>they aren't in those spots has just been because things have moved so much</div><div>in a short time that it was easier not having to move config options, too.</div>
<div>I support moving them to appropriate places at anybody's leisure.</div><div><br></div><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
In practice, this resulted in two patterns. Firstly, where a set of<br>
options are used exclusively within a class:<br>
<br>
class ExtensionManager(object):<br>
<br>
enabled_apis_opt = cfg.ListOpt('enabled_apis',<br>
default=['ec2', 'osapi'],<br>
help='List of APIs to enable by default')<br>
<br>
def __init__(self, conf):<br>
self.conf = conf<br>
self.conf.register_opt(enabled_apis_opt)<br>
...<br>
<br>
def _load_extensions(self):<br>
for ext_factory in self.conf.osapi_extension:<br>
....<br>
<br>
Here you have the options used by the class clearly declared at the<br>
beginning of the class definition and a config instance passed to the<br>
constructor.<br>
<br>
Secondly, where options are used by functions within a module:<br>
<br>
crypt_strength_opt = cfg.IntOpt('crypt_strength', default=40000)<br>
<br>
def hash_password(conf, password):<br>
"""Hash a password. Hard."""<br>
password_utf8 = password.encode('utf-8')<br>
if passlib.hash.sha512_crypt.identify(password_utf8):<br>
return password_utf8<br>
conf.register_opt(crypt_strength_opt)<br>
h = passlib.hash.sha512_crypt.encrypt(password_utf8,<br>
rounds=conf.crypt_strength)<br>
return h<br>
<br>
Here you have the options declared at the beginning of the module and a<br>
config instance passed to functions which use options. The option schema<br>
is registered by such a function just before the option is used.<br></blockquote><div><br></div><div><div>My preference here is to define all options at the top of the file, largely</div><div>because one is often looking for a config option, possibly to figure out</div>
<div>what its default is, usually to figure out where to change its default, and</div><div>having them defined at the top of the file makes sure they are always where</div><div>you expect them to be.</div><div><br></div></div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
One complaint about the above pattern is that it's more verbose than<br>
simply doing e.g.<br>
<br>
CONF = config.CONF<br>
<br>
config.register_opt('crypt_strength', default=40000)<br>
<br>
def hash_password(password):<br>
...<br>
h = passlib.hash.sha512_crypt.encrypt(password_utf8,<br>
rounds=CONF.crypt_strength)<br>
return h<br>
<br>
<br>
The obvious way of making it less verbose while still removing the<br>
dependence on global objects would be:<br>
<br>
def hash_password(conf, password):<br>
...<br>
conf.register_opt('crypt_strength', default=40000)<br>
h = passlib.hash.sha512_crypt.encrypt(password_utf8,<br>
rounds=conf.crypt_strength)<br>
return h<br>
<br>
However, that does have the effect of obscuring the option definitions<br>
somewhat.<br></blockquote><div><br></div><div><div><br></div><div>Since I am for the global config option with all options defined at the top</div><div>of the file, I don't see a problem here :p Though I would have the</div>
<div>registration options be named register_str and register_int (just a typo</div><div>in your code above, I assume).</div><div><br></div></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
Any thoughts or ideas on all of this?<br></blockquote><div><br></div><div><br></div><div>(I'll post this to the top as a tl;dr)</div><div><br></div><div>Summary, I'd like cfg.py to be config.py (pipe dream, I'm sure, but I don't</div>
<div>really agree with the abbreviation), to provide a global configuration object,</div><div>and to have shortcuts for registration of options, that projects should</div><div>standardize on. And I'm willing to help make those changes.</div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks,<br>
Mark.<br>
<br>
[1] - <a href="http://wiki.openstack.org/CommonConfigModule" target="_blank">http://wiki.openstack.org/CommonConfigModule</a><br>
[2] - <a href="https://github.com/openstack/nova/commit/4eeb0b96fc" target="_blank">https://github.com/openstack/nova/commit/4eeb0b96fc</a><br>
[3] - <a href="https://github.com/markmc/keystone/tree/cfg-cleanup" target="_blank">https://github.com/markmc/keystone/tree/cfg-cleanup</a><br>
<br>
</blockquote></div><br>