Refactor nodepool apps into base app

There is a lot of commonality between the nodepool apps that is
replicated between each of the files. This will mean adding more again
when new apps are added.

Separate a base NodepoolApp and NodepoolDaemonApp that handle the common
parts of setting up a nodepool app and use them in the existing apps.

First Proposed as: I098b0a1d749e9dc400d45355f9295ca07058768b
Then reverted: I7d8f1cd866b768326157f4503c1f729ebb703c0c

Change-Id: I81e80af8e2c18e56db8b6dca59ceb279dc9d591c

nodepool/cmd/__init__.py

@@ -14,6 +14,10 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+import argparse
+import daemon
+import errno
+import extras
 import logging
 import logging.config
 import os
@@ -22,6 +26,35 @@ import sys
 import threading
 import traceback
 
+from nodepool.version import version_info as npd_version_info
+
+
+# as of python-daemon 1.6 it doesn't bundle pidlockfile anymore
+# instead it depends on lockfile-0.9.1 which uses pidfile.
+pid_file_module = extras.try_imports(['daemon.pidlockfile', 'daemon.pidfile'])
+
+
+def is_pidfile_stale(pidfile):
+    """ Determine whether a PID file is stale.
+
+        Return 'True' ("stale") if the contents of the PID file are
+        valid but do not match the PID of a currently-running process;
+        otherwise return 'False'.
+
+        """
+    result = False
+
+    pidfile_pid = pidfile.read_pid()
+    if pidfile_pid is not None:
+        try:
+            os.kill(pidfile_pid, 0)
+        except OSError as exc:
+            if exc.errno == errno.ESRCH:
+                # The specified PID does not exist
+                result = True
+
+    return result
+
 
 def stack_dump_handler(signum, frame):
     signal.signal(signal.SIGUSR2, signal.SIG_IGN)
@@ -45,17 +78,92 @@ def stack_dump_handler(signum, frame):
 
 class NodepoolApp(object):
 
+    app_name = None
+    app_description = 'Node pool.'
+
     def __init__(self):
         self.args = None
 
+    def create_parser(self):
+        parser = argparse.ArgumentParser(description=self.app_description)
+
+        parser.add_argument('-l',
+                            dest='logconfig',
+                            help='path to log config file')
+
+        parser.add_argument('--version',
+                            action='version',
+                            version=npd_version_info.version_string())
+
+        return parser
+
     def setup_logging(self):
         if self.args.logconfig:
             fp = os.path.expanduser(self.args.logconfig)
+
             if not os.path.exists(fp):
-                raise Exception("Unable to read logging config file at %s" %
+                m = "Unable to read logging config file at %s" % fp
-                                fp)
+                raise Exception(m)
+
             logging.config.fileConfig(fp)
+
         else:
-            logging.basicConfig(level=logging.DEBUG,
+            m = '%(asctime)s %(levelname)s %(name)s: %(message)s'
-                                format='%(asctime)s %(levelname)s %(name)s: '
+            logging.basicConfig(level=logging.DEBUG, format=m)
-                                       '%(message)s')
+
+    def _main(self, argv=None):
+        if argv is None:
+            argv = sys.argv[1:]
+
+        self.args = self.create_parser().parse_args()
+        return self._do_run()
+
+    def _do_run(self):
+        # NOTE(jamielennox): setup logging a bit late so it's not done until
+        # after a DaemonContext is created.
+        self.setup_logging()
+        return self.run()
+
+    @classmethod
+    def main(cls, argv=None):
+        return cls()._main(argv=argv)
+
+    def run(self):
+        """The app's primary function, override it with your logic."""
+        raise NotImplementedError()
+
+
+class NodepoolDaemonApp(NodepoolApp):
+
+    def create_parser(self):
+        parser = super(NodepoolDaemonApp, self).create_parser()
+
+        parser.add_argument('-p',
+                            dest='pidfile',
+                            help='path to pid file',
+                            default='/var/run/nodepool/%s.pid' % self.app_name)
+
+        parser.add_argument('-d',
+                            dest='nodaemon',
+                            action='store_true',
+                            help='do not run as a daemon')
+
+        return parser
+
+    def _do_run(self):
+        if self.args.nodaemon:
+            return super(NodepoolDaemonApp, self)._do_run()
+
+        else:
+            pid = pid_file_module.TimeoutPIDLockFile(self.args.pidfile, 10)
+
+            if is_pidfile_stale(pid):
+                pid.break_lock()
+
+            with daemon.DaemonContext(pidfile=pid):
+                return super(NodepoolDaemonApp, self)._do_run()
+
+    @classmethod
+    def main(cls, argv=None):
+        signal.signal(signal.SIGUSR2, stack_dump_handler)
+        return super(NodepoolDaemonApp, cls).main(argv)

nodepool/cmd/builder.py

@@ -12,40 +12,28 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
-import argparse
-import extras
 import signal
 import sys
 
-import daemon
-
 from nodepool import builder
 import nodepool.cmd
 
 
-# as of python-daemon 1.6 it doesn't bundle pidlockfile anymore
+class NodePoolBuilderApp(nodepool.cmd.NodepoolDaemonApp):
-# instead it depends on lockfile-0.9.1 which uses pidfile.
-pid_file_module = extras.try_imports(['daemon.pidlockfile', 'daemon.pidfile'])
 
-class NodePoolBuilderApp(nodepool.cmd.NodepoolApp):
+    app_name = 'nodepool-builder'
+    app_description = 'NodePool Image Builder.'
 
     def sigint_handler(self, signal, frame):
         self.nb.stop()
         sys.exit(0)
 
-    def parse_arguments(self):
+    def create_parser(self):
-        parser = argparse.ArgumentParser(description='NodePool Image Builder.')
+        parser = super(NodePoolBuilderApp, self).create_parser()
+
         parser.add_argument('-c', dest='config',
                             default='/etc/nodepool/nodepool.yaml',
                             help='path to config file')
-        parser.add_argument('-l', dest='logconfig',
-                            help='path to log config file')
-        parser.add_argument('-p', dest='pidfile',
-                            help='path to pid file',
-                            default='/var/run/nodepool-builder/'
-                                    'nodepool-builder.pid')
-        parser.add_argument('-d', dest='nodaemon', action='store_true',
-                            help='do not run as a daemon')
         parser.add_argument('--build-workers', dest='build_workers',
                             default=1, help='number of build workers',
                             type=int)
@@ -55,16 +43,16 @@ class NodePoolBuilderApp(nodepool.cmd.NodepoolApp):
         parser.add_argument('--fake', action='store_true',
                             help='Do not actually run diskimage-builder '
                             '(used for testing)')
-        self.args = parser.parse_args()
+        return parser
 
-    def main(self):
+    def run(self):
-        self.setup_logging()
+        self.nb = builder.NodePoolBuilder(self.args.config,
-        self.nb = builder.NodePoolBuilder(
+                                          self.args.build_workers,
-            self.args.config, self.args.build_workers,
+                                          self.args.upload_workers,
-            self.args.upload_workers, self.args.fake)
+                                          self.args.fake)
 
         signal.signal(signal.SIGINT, self.sigint_handler)
-        signal.signal(signal.SIGUSR2, nodepool.cmd.stack_dump_handler)
+
         self.nb.start()
 
         while True:
@@ -72,15 +60,7 @@ class NodePoolBuilderApp(nodepool.cmd.NodepoolApp):
 
 
 def main():
-    app = NodePoolBuilderApp()
+    return NodePoolBuilderApp.main()
-    app.parse_arguments()
-
-    if app.args.nodaemon:
-        app.main()
-    else:
-        pid = pid_file_module.TimeoutPIDLockFile(app.args.pidfile, 10)
-        with daemon.DaemonContext(pidfile=pid):
-            app.main()
 
 
 if __name__ == "__main__":

nodepool/cmd/nodepoolcmd.py

@@ -14,7 +14,6 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
-import argparse
 import logging.config
 import sys
 
@@ -23,7 +22,6 @@ from nodepool import nodepool
 from nodepool import status
 from nodepool import zk
 from nodepool.cmd import NodepoolApp
-from nodepool.version import version_info as npc_version_info
 from config_validator import ConfigValidator
 from prettytable import PrettyTable
 
@@ -32,19 +30,15 @@ log = logging.getLogger(__name__)
 
 class NodePoolCmd(NodepoolApp):
 
-    def parse_arguments(self):
+    def create_parser(self):
-        parser = argparse.ArgumentParser(description='Node pool.')
+        parser = super(NodePoolCmd, self).create_parser()
+
         parser.add_argument('-c', dest='config',
                             default='/etc/nodepool/nodepool.yaml',
                             help='path to config file')
         parser.add_argument('-s', dest='secure',
                             default='/etc/nodepool/secure.conf',
                             help='path to secure file')
-        parser.add_argument('-l', dest='logconfig',
-                            help='path to log config file')
-        parser.add_argument('--version', action='version',
-                            version=npc_version_info.version_string(),
-                            help='show version')
         parser.add_argument('--debug', dest='debug', action='store_true',
                             help='show DEBUG level logging')
 
@@ -89,7 +83,8 @@ class NodePoolCmd(NodepoolApp):
             help='place a node in the HOLD state')
         cmd_hold.set_defaults(func=self.hold)
         cmd_hold.add_argument('id', help='node id')
-        cmd_hold.add_argument('--reason', help='Reason this node is held',
+        cmd_hold.add_argument('--reason',
+                              help='Reason this node is held',
                               required=True)
 
         cmd_delete = subparsers.add_parser(
@@ -130,19 +125,21 @@ class NodePoolCmd(NodepoolApp):
             help='list the current node requests')
         cmd_request_list.set_defaults(func=self.request_list)
 
-        self.args = parser.parse_args()
+        return parser
 
     def setup_logging(self):
+        # NOTE(jamielennox): This should just be the same as other apps
         if self.args.debug:
-            logging.basicConfig(level=logging.DEBUG,
+            m = '%(asctime)s %(levelname)s %(name)s: %(message)s'
-                                format='%(asctime)s %(levelname)s %(name)s: '
+            logging.basicConfig(level=logging.DEBUG, format=m)
-                                       '%(message)s')
+
         elif self.args.logconfig:
-            NodepoolApp.setup_logging(self)
+            super(NodePoolCmd, self).setup_logging()
+
         else:
-            logging.basicConfig(level=logging.INFO,
+            m = '%(asctime)s %(levelname)s %(name)s: %(message)s'
-                                format='%(asctime)s %(levelname)s %(name)s: '
+            logging.basicConfig(level=logging.INFO, format=m)
-                                       '%(message)s')
+
             l = logging.getLogger('kazoo')
             l.setLevel(logging.WARNING)
 
@@ -319,7 +316,7 @@ class NodePoolCmd(NodepoolApp):
             if t:
                 t.join()
 
-    def main(self):
+    def run(self):
         self.zk = None
 
         # commands which do not need to start-up or parse config
@@ -344,11 +341,9 @@ class NodePoolCmd(NodepoolApp):
         if self.zk:
             self.zk.disconnect()
 
+
 def main():
-    npc = NodePoolCmd()
+    return NodePoolCmd.main()
-    npc.parse_arguments()
-    npc.setup_logging()
-    return npc.main()
 
 
 if __name__ == "__main__":

nodepool/cmd/nodepoold.py

@@ -14,15 +14,6 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
-import argparse
-import daemon
-import errno
-import extras
-
-# as of python-daemon 1.6 it doesn't bundle pidlockfile anymore
-# instead it depends on lockfile-0.9.1 which uses pidfile.
-pid_file_module = extras.try_imports(['daemon.pidlockfile', 'daemon.pidfile'])
-
 import logging
 import os
 import sys
@@ -35,49 +26,21 @@ import nodepool.webapp
 log = logging.getLogger(__name__)
 
 
-def is_pidfile_stale(pidfile):
+class NodePoolDaemon(nodepool.cmd.NodepoolDaemonApp):
-    """ Determine whether a PID file is stale.
 
-        Return 'True' ("stale") if the contents of the PID file are
+    app_name = 'nodepool'
-        valid but do not match the PID of a currently-running process;
-        otherwise return 'False'.
 
-        """
+    def create_parser(self):
-    result = False
+        parser = super(NodePoolDaemon, self).create_parser()
 
-    pidfile_pid = pidfile.read_pid()
-    if pidfile_pid is not None:
-        try:
-            os.kill(pidfile_pid, 0)
-        except OSError as exc:
-            if exc.errno == errno.ESRCH:
-                # The specified PID does not exist
-                result = True
-
-    return result
-
-
-class NodePoolDaemon(nodepool.cmd.NodepoolApp):
-
-    def parse_arguments(self):
-        parser = argparse.ArgumentParser(description='Node pool.')
         parser.add_argument('-c', dest='config',
                             default='/etc/nodepool/nodepool.yaml',
                             help='path to config file')
         parser.add_argument('-s', dest='secure',
                             default='/etc/nodepool/secure.conf',
                             help='path to secure file')
-        parser.add_argument('-d', dest='nodaemon', action='store_true',
-                            help='do not run as a daemon')
-        parser.add_argument('-l', dest='logconfig',
-                            help='path to log config file')
-        parser.add_argument('-p', dest='pidfile',
-                            help='path to pid file',
-                            default='/var/run/nodepool/nodepool.pid')
         parser.add_argument('--no-webapp', action='store_true')
-        parser.add_argument('--version', dest='version', action='store_true',
+        return parser
-                            help='show version')
-        self.args = parser.parse_args()
 
     def exit_handler(self, signum, frame):
         self.pool.stop()
@@ -88,8 +51,7 @@ class NodePoolDaemon(nodepool.cmd.NodepoolApp):
     def term_handler(self, signum, frame):
         os._exit(0)
 
-    def main(self):
+    def run(self):
-        self.setup_logging()
         self.pool = nodepool.nodepool.NodePool(self.args.secure,
                                                self.args.config)
         if not self.args.no_webapp:
@@ -99,7 +61,6 @@ class NodePoolDaemon(nodepool.cmd.NodepoolApp):
         # For back compatibility:
         signal.signal(signal.SIGUSR1, self.exit_handler)
 
-        signal.signal(signal.SIGUSR2, nodepool.cmd.stack_dump_handler)
         signal.signal(signal.SIGTERM, self.term_handler)
 
         self.pool.start()
@@ -112,23 +73,7 @@ class NodePoolDaemon(nodepool.cmd.NodepoolApp):
 
 
 def main():
-    npd = NodePoolDaemon()
+    return NodePoolDaemon.main()
-    npd.parse_arguments()
-
-    if npd.args.version:
-        from nodepool.version import version_info as npd_version_info
-        print "Nodepool version: %s" % npd_version_info.version_string()
-        return(0)
-
-    pid = pid_file_module.TimeoutPIDLockFile(npd.args.pidfile, 10)
-    if is_pidfile_stale(pid):
-        pid.break_lock()
-
-    if npd.args.nodaemon:
-        npd.main()
-    else:
-        with daemon.DaemonContext(pidfile=pid):
-            npd.main()
 
 
 if __name__ == "__main__":