Revert "Refactor nodepool apps into base app"

Logging is currently broken due to this change. Nothing is logged.

This reverts commit 03c41ccc35.

Change-Id: I7d8f1cd866b768326157f4503c1f729ebb703c0c

nodepool/cmd/__init__.py

@@ -14,10 +14,6 @@
 # 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
@@ -26,35 +22,6 @@ 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)
@@ -78,91 +45,17 @@ 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):
-                m = "Unable to read logging config file at %s" % fp
+                raise Exception("Unable to read logging config file at %s" %
-                raise Exception(m)
+                                fp)
-
             logging.config.fileConfig(fp)
-
         else:
-            m = '%(asctime)s %(levelname)s %(name)s: %(message)s'
+            logging.basicConfig(level=logging.DEBUG,
-            logging.basicConfig(level=logging.DEBUG, format=m)
+                                format='%(asctime)s %(levelname)s %(name)s: '
-
+                                       '%(message)s')
-    def _main(self, argv=None):
-        if argv is None:
-            argv = sys.argv[1:]
-
-        self.args = self.create_parser().parse_args()
-        self.setup_logging()
-
-        return self._do_run()
-
-    def _do_run(self):
-        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,28 +12,40 @@
 # 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
 
 
-class NodePoolBuilderApp(nodepool.cmd.NodepoolDaemonApp):
+# 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'])
 
-    app_name = 'nodepool-builder'
+class NodePoolBuilderApp(nodepool.cmd.NodepoolApp):
-    app_description = 'NodePool Image Builder.'
 
     def sigint_handler(self, signal, frame):
         self.nb.stop()
         sys.exit(0)
 
-    def create_parser(self):
+    def parse_arguments(self):
-        parser = super(NodePoolBuilderApp, self).create_parser()
+        parser = argparse.ArgumentParser(description='NodePool Image Builder.')
-
         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)
@@ -43,16 +55,16 @@ class NodePoolBuilderApp(nodepool.cmd.NodepoolDaemonApp):
         parser.add_argument('--fake', action='store_true',
                             help='Do not actually run diskimage-builder '
                             '(used for testing)')
-        return parser
+        self.args = parser.parse_args()
 
-    def run(self):
+    def main(self):
-        self.nb = builder.NodePoolBuilder(self.args.config,
+        self.setup_logging()
-                                          self.args.build_workers,
+        self.nb = builder.NodePoolBuilder(
-                                          self.args.upload_workers,
+            self.args.config, self.args.build_workers,
-                                          self.args.fake)
+            self.args.upload_workers, self.args.fake)
 
         signal.signal(signal.SIGINT, self.sigint_handler)
-
+        signal.signal(signal.SIGUSR2, nodepool.cmd.stack_dump_handler)
         self.nb.start()
 
         while True:
@@ -60,7 +72,15 @@ class NodePoolBuilderApp(nodepool.cmd.NodepoolDaemonApp):
 
 
 def main():
-    return NodePoolBuilderApp.main()
+    app = NodePoolBuilderApp()
+    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,6 +14,7 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+import argparse
 import logging.config
 import sys
 
@@ -22,6 +23,7 @@ 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
 
@@ -30,15 +32,19 @@ log = logging.getLogger(__name__)
 
 class NodePoolCmd(NodepoolApp):
 
-    def create_parser(self):
+    def parse_arguments(self):
-        parser = super(NodePoolCmd, self).create_parser()
+        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('-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')
 
@@ -83,8 +89,7 @@ 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',
+        cmd_hold.add_argument('--reason', help='Reason this node is held',
-                              help='Reason this node is held',
                               required=True)
 
         cmd_delete = subparsers.add_parser(
@@ -125,21 +130,19 @@ class NodePoolCmd(NodepoolApp):
             help='list the current node requests')
         cmd_request_list.set_defaults(func=self.request_list)
 
-        return parser
+        self.args = parser.parse_args()
 
     def setup_logging(self):
-        # NOTE(jamielennox): This should just be the same as other apps
         if self.args.debug:
-            m = '%(asctime)s %(levelname)s %(name)s: %(message)s'
+            logging.basicConfig(level=logging.DEBUG,
-            logging.basicConfig(level=logging.DEBUG, format=m)
+                                format='%(asctime)s %(levelname)s %(name)s: '
-
+                                       '%(message)s')
         elif self.args.logconfig:
-            super(NodePoolCmd, self).setup_logging()
+            NodepoolApp.setup_logging(self)
-
         else:
-            m = '%(asctime)s %(levelname)s %(name)s: %(message)s'
+            logging.basicConfig(level=logging.INFO,
-            logging.basicConfig(level=logging.INFO, format=m)
+                                format='%(asctime)s %(levelname)s %(name)s: '
-
+                                       '%(message)s')
             l = logging.getLogger('kazoo')
             l.setLevel(logging.WARNING)
 
@@ -316,7 +319,7 @@ class NodePoolCmd(NodepoolApp):
             if t:
                 t.join()
 
-    def run(self):
+    def main(self):
         self.zk = None
 
         # commands which do not need to start-up or parse config
@@ -341,9 +344,11 @@ class NodePoolCmd(NodepoolApp):
         if self.zk:
             self.zk.disconnect()
 
-
 def main():
-    return NodePoolCmd.main()
+    npc = NodePoolCmd()
+    npc.parse_arguments()
+    npc.setup_logging()
+    return npc.main()
 
 
 if __name__ == "__main__":

nodepool/cmd/nodepoold.py

@@ -14,6 +14,15 @@
 # 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
@@ -26,21 +35,49 @@ import nodepool.webapp
 log = logging.getLogger(__name__)
 
 
-class NodePoolDaemon(nodepool.cmd.NodepoolDaemonApp):
+def is_pidfile_stale(pidfile):
+    """ Determine whether a PID file is stale.
 
-    app_name = 'nodepool'
+        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'.
 
-    def create_parser(self):
+        """
-        parser = super(NodePoolDaemon, self).create_parser()
+    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
+
+
+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')
-        return parser
+        parser.add_argument('--version', dest='version', action='store_true',
+                            help='show version')
+        self.args = parser.parse_args()
 
     def exit_handler(self, signum, frame):
         self.pool.stop()
@@ -51,7 +88,8 @@ class NodePoolDaemon(nodepool.cmd.NodepoolDaemonApp):
     def term_handler(self, signum, frame):
         os._exit(0)
 
-    def run(self):
+    def main(self):
+        self.setup_logging()
         self.pool = nodepool.nodepool.NodePool(self.args.secure,
                                                self.args.config)
         if not self.args.no_webapp:
@@ -61,6 +99,7 @@ class NodePoolDaemon(nodepool.cmd.NodepoolDaemonApp):
         # 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()
@@ -73,7 +112,23 @@ class NodePoolDaemon(nodepool.cmd.NodepoolDaemonApp):
 
 
 def main():
-    return NodePoolDaemon.main()
+    npd = NodePoolDaemon()
+    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__":