Some first steps towards resolving some of the issues brought up on the mailing list related to documenting flags.

This patch helps expose some of the features of gflags that we were sort of skipping past, such as organizing flags by the file that defined them in help.

There are still some things to fix, for example all the flags defined in nova.flags should be moved to other files, and the ones that are there currently show up under nova.log, but previously they were all in one spot anyway so this is still fully a step forward.

Manager import was moved into Service's init so that the dynamic flags get loaded earlier so we can show them in the help.

3 files changed, 62 insertions, 25 deletions

diff --git a/nova/flags.py b/nova/flags.py
index f64a62da9..24bca0caf 100644
--- a/nova/flags.py
+++ b/nova/flags.py
@@ -160,9 +160,45 @@ class StrWrapper(object):
         raise KeyError(name)
 
 
-FLAGS = FlagValues()
-gflags.FLAGS = FLAGS
-gflags.DEFINE_flag(gflags.HelpFlag(), FLAGS)
+# Copied from gflags with small mods to get the naming correct.
+# Originally gflags checks for the first module that is not gflags that is
+# in the call chain, we want to check for the first module that is not gflags
+# and not this module.
+def _GetCallingModule():
+    """Returns the name of the module that's calling into this module.
+
+    We generally use this function to get the name of the module calling a
+    DEFINE_foo... function.
+    """
+    # Walk down the stack to find the first globals dict that's not ours.
+    for depth in range(1, sys.getrecursionlimit()):
+        if not sys._getframe(depth).f_globals is globals():
+            module_name = __GetModuleName(sys._getframe(depth).f_globals)
+            if module_name == 'gflags':
+                continue
+            if module_name is not None:
+                return module_name
+    raise AssertionError("No module was found")
+
+
+# Copied from gflags because it is a private function
+def __GetModuleName(globals_dict):
+    """Given a globals dict, returns the name of the module that defines it.
+
+    Args:
+    globals_dict: A dictionary that should correspond to an environment
+      providing the values of the globals.
+
+    Returns:
+    A string (the name of the module) or None (if the module could not
+    be identified.
+    """
+    for name, module in sys.modules.iteritems():
+        if getattr(module, '__dict__', None) is globals_dict:
+            if name == '__main__':
+                return sys.argv[0]
+            return name
+    return None
 
 
 def _wrapper(func):
@@ -173,6 +209,11 @@ def _wrapper(func):
     return _wrapped
 
 
+FLAGS = FlagValues()
+gflags.FLAGS = FLAGS
+gflags._GetCallingModule = _GetCallingModule
+
+
 DEFINE = _wrapper(gflags.DEFINE)
 DEFINE_string = _wrapper(gflags.DEFINE_string)
 DEFINE_integer = _wrapper(gflags.DEFINE_integer)
@@ -185,8 +226,6 @@ DEFINE_spaceseplist = _wrapper(gflags.DEFINE_spaceseplist)
 DEFINE_multistring = _wrapper(gflags.DEFINE_multistring)
 DEFINE_multi_int = _wrapper(gflags.DEFINE_multi_int)
 DEFINE_flag = _wrapper(gflags.DEFINE_flag)
-
-
 HelpFlag = gflags.HelpFlag
 HelpshortFlag = gflags.HelpshortFlag
 HelpXMLFlag = gflags.HelpXMLFlag
diff --git a/nova/service.py b/nova/service.py
index cc88ac233..f47358089 100644
--- a/nova/service.py
+++ b/nova/service.py
@@ -45,15 +45,10 @@ FLAGS = flags.FLAGS
 flags.DEFINE_integer('report_interval', 10,
                      'seconds between nodes reporting state to datastore',
                      lower_bound=1)
-
 flags.DEFINE_integer('periodic_interval', 60,
                      'seconds between running periodic tasks',
                      lower_bound=1)
 
-flags.DEFINE_flag(flags.HelpFlag())
-flags.DEFINE_flag(flags.HelpshortFlag())
-flags.DEFINE_flag(flags.HelpXMLFlag())
-
 
 class Service(object):
     """Base class for workers that run on hosts."""
@@ -64,6 +59,8 @@ class Service(object):
         self.binary = binary
         self.topic = topic
         self.manager_class_name = manager
+        manager_class = utils.import_class(self.manager_class_name)
+        self.manager = manager_class(host=self.host, *args, **kwargs)
         self.report_interval = report_interval
         self.periodic_interval = periodic_interval
         super(Service, self).__init__(*args, **kwargs)
@@ -71,9 +68,9 @@ class Service(object):
         self.timers = []
 
     def start(self):
-        manager_class = utils.import_class(self.manager_class_name)
-        self.manager = manager_class(host=self.host, *self.saved_args,
-                                                     **self.saved_kwargs)
+        vcs_string = version.version_string_with_vcs()
+        logging.audit(_("Starting %(topic)s node (version %(vcs_string)s)"),
+                      {'topic': self.topic, 'vcs_string': vcs_string})
         self.manager.init_host()
         self.model_disconnected = False
         ctxt = context.get_admin_context()
@@ -153,9 +150,6 @@ class Service(object):
             report_interval = FLAGS.report_interval
         if not periodic_interval:
             periodic_interval = FLAGS.periodic_interval
-        vcs_string = version.version_string_with_vcs()
-        logging.audit(_("Starting %(topic)s node (version %(vcs_string)s)")
-                % locals())
         service_obj = cls(host, binary, topic, manager,
                           report_interval, periodic_interval)
 
@@ -217,8 +211,19 @@ class Service(object):
 
 
 def serve(*services):
-    if not services:
-        services = [Service.create()]
+    try:
+        if not services:
+            services = [Service.create()]
+    except Exception:
+        logging.exception('in Service.create()')
+        raise
+    finally:
+        # After we've loaded up all our dynamic bits, check
+        # whether we should print help
+        flags.DEFINE_flag(flags.HelpFlag())
+        flags.DEFINE_flag(flags.HelpshortFlag())
+        flags.DEFINE_flag(flags.HelpXMLFlag())
+        FLAGS.ParseNewFlags()
 
     name = '_'.join(x.binary for x in services)
     logging.debug(_("Serving %s"), name)
diff --git a/nova/tests/test_service.py b/nova/tests/test_service.py
index a67c8d1e8..45d9afa6c 100644
--- a/nova/tests/test_service.py
+++ b/nova/tests/test_service.py
@@ -50,13 +50,6 @@ class ExtendedService(service.Service):
 class ServiceManagerTestCase(test.TestCase):
     """Test cases for Services"""
 
-    def test_attribute_error_for_no_manager(self):
-        serv = service.Service('test',
-                               'test',
-                               'test',
-                               'nova.tests.test_service.FakeManager')
-        self.assertRaises(AttributeError, getattr, serv, 'test_method')
-
     def test_message_gets_to_manager(self):
         serv = service.Service('test',
                                'test',