From 6e206087df06ca094e1b03fa5efd566d341a6fd6 Mon Sep 17 00:00:00 2001 From: Jamie Lennox Date: Tue, 14 Mar 2017 11:35:04 +1100 Subject: [PATCH] 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 | 118 ++++++++++++++++++++++++++++++++++-- nodepool/cmd/builder.py | 48 +++++---------- nodepool/cmd/nodepoolcmd.py | 41 ++++++------- nodepool/cmd/nodepoold.py | 69 +++------------------ 4 files changed, 152 insertions(+), 124 deletions(-) diff --git a/nodepool/cmd/__init__.py b/nodepool/cmd/__init__.py index 3d388e74c..15e136c62 100644 --- a/nodepool/cmd/__init__.py +++ b/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" % - fp) + m = "Unable to read logging config file at %s" % fp + raise Exception(m) + logging.config.fileConfig(fp) + else: - logging.basicConfig(level=logging.DEBUG, - format='%(asctime)s %(levelname)s %(name)s: ' - '%(message)s') + m = '%(asctime)s %(levelname)s %(name)s: %(message)s' + logging.basicConfig(level=logging.DEBUG, format=m) + + 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) diff --git a/nodepool/cmd/builder.py b/nodepool/cmd/builder.py index 55d3a4370..1138cba5f 100644 --- a/nodepool/cmd/builder.py +++ b/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 -# 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.NodepoolDaemonApp): -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): - parser = argparse.ArgumentParser(description='NodePool Image Builder.') + def create_parser(self): + 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): - self.setup_logging() - self.nb = builder.NodePoolBuilder( - self.args.config, self.args.build_workers, - self.args.upload_workers, self.args.fake) + def run(self): + self.nb = builder.NodePoolBuilder(self.args.config, + self.args.build_workers, + 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: @@ -72,15 +60,7 @@ class NodePoolBuilderApp(nodepool.cmd.NodepoolApp): def 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() + return NodePoolBuilderApp.main() if __name__ == "__main__": diff --git a/nodepool/cmd/nodepoolcmd.py b/nodepool/cmd/nodepoolcmd.py index 608a9358a..294517f42 100644 --- a/nodepool/cmd/nodepoolcmd.py +++ b/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): - parser = argparse.ArgumentParser(description='Node pool.') + def create_parser(self): + 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, - format='%(asctime)s %(levelname)s %(name)s: ' - '%(message)s') + m = '%(asctime)s %(levelname)s %(name)s: %(message)s' + logging.basicConfig(level=logging.DEBUG, format=m) + elif self.args.logconfig: - NodepoolApp.setup_logging(self) + super(NodePoolCmd, self).setup_logging() + else: - logging.basicConfig(level=logging.INFO, - format='%(asctime)s %(levelname)s %(name)s: ' - '%(message)s') + m = '%(asctime)s %(levelname)s %(name)s: %(message)s' + logging.basicConfig(level=logging.INFO, format=m) + 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() - npc.parse_arguments() - npc.setup_logging() - return npc.main() + return NodePoolCmd.main() if __name__ == "__main__": diff --git a/nodepool/cmd/nodepoold.py b/nodepool/cmd/nodepoold.py index 6a623b9ad..00b7861bb 100644 --- a/nodepool/cmd/nodepoold.py +++ b/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): - """ Determine whether a PID file is stale. +class NodePoolDaemon(nodepool.cmd.NodepoolDaemonApp): - 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'. + app_name = 'nodepool' - """ - result = False + def create_parser(self): + 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', - help='show version') - self.args = parser.parse_args() + return parser 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): - self.setup_logging() + def run(self): 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() - 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() + return NodePoolDaemon.main() if __name__ == "__main__":