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.

7 files changed, 75 insertions, 26 deletions

diff --git a/bin/nova-ajax-console-proxy b/bin/nova-ajax-console-proxy
index 392b328b1..1e11c6d58 100755
--- a/bin/nova-ajax-console-proxy
+++ b/bin/nova-ajax-console-proxy
@@ -47,9 +47,11 @@ from nova import utils
 from nova import wsgi
 
 FLAGS = flags.FLAGS
-
 flags.DEFINE_integer('ajax_console_idle_timeout', 300,
                      'Seconds before idle connection destroyed')
+flags.DEFINE_flag(flags.HelpFlag())
+flags.DEFINE_flag(flags.HelpshortFlag())
+flags.DEFINE_flag(flags.HelpXMLFlag())
 
 LOG = logging.getLogger('nova.ajax_console_proxy')
 LOG.setLevel(logging.DEBUG)
diff --git a/bin/nova-api b/bin/nova-api
index d5efb4687..cf140570a 100755
--- a/bin/nova-api
+++ b/bin/nova-api
@@ -48,6 +48,9 @@ flags.DEFINE_integer('ec2_listen_port', 8773, 'port for ec2 api to listen')
 flags.DEFINE_string('osapi_listen', "0.0.0.0",
                     'IP address for OpenStack API to listen')
 flags.DEFINE_integer('osapi_listen_port', 8774, 'port for os api to listen')
+flags.DEFINE_flag(flags.HelpFlag())
+flags.DEFINE_flag(flags.HelpshortFlag())
+flags.DEFINE_flag(flags.HelpXMLFlag())
 
 API_ENDPOINTS = ['ec2', 'osapi']
 
diff --git a/bin/nova-direct-api b/bin/nova-direct-api
index 6c63bd26b..bf29d9a5e 100755
--- a/bin/nova-direct-api
+++ b/bin/nova-direct-api
@@ -45,6 +45,10 @@ from nova.compute import api as compute_api
 FLAGS = flags.FLAGS
 flags.DEFINE_integer('direct_port', 8001, 'Direct API port')
 flags.DEFINE_string('direct_host', '0.0.0.0', 'Direct API host')
+flags.DEFINE_flag(flags.HelpFlag())
+flags.DEFINE_flag(flags.HelpshortFlag())
+flags.DEFINE_flag(flags.HelpXMLFlag())
+
 
 if __name__ == '__main__':
     utils.default_flagfile()
diff --git a/bin/nova-manage b/bin/nova-manage
index 5189de0e1..b603c8b07 100755
--- a/bin/nova-manage
+++ b/bin/nova-manage
@@ -93,6 +93,9 @@ flags.DECLARE('network_size', 'nova.network.manager')
 flags.DECLARE('vlan_start', 'nova.network.manager')
 flags.DECLARE('vpn_start', 'nova.network.manager')
 flags.DECLARE('fixed_range_v6', 'nova.network.manager')
+flags.DEFINE_flag(flags.HelpFlag())
+flags.DEFINE_flag(flags.HelpshortFlag())
+flags.DEFINE_flag(flags.HelpXMLFlag())
 
 
 def param2id(object_id):
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',