From 1bf032959fbd4845e2a957c083cf4a505bfe752a Mon Sep 17 00:00:00 2001 From: Lukas Pirl Date: Thu, 24 Aug 2017 14:56:00 +0200 Subject: [PATCH 1/3] delete invalid PID file on ``tahoe (re)start`` --- src/allmydata/scripts/startstop_node.py | 52 +++++++++++++++++++------ 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/src/allmydata/scripts/startstop_node.py b/src/allmydata/scripts/startstop_node.py index 75073a1e8..16a9693d7 100644 --- a/src/allmydata/scripts/startstop_node.py +++ b/src/allmydata/scripts/startstop_node.py @@ -86,6 +86,35 @@ class StartTahoeNodePlugin: return StatsGathererService(verbose=True) raise ValueError("unknown nodetype %s" % self.nodetype) +def get_pidfile(basedir): + """ + Returns the path to the PID file. + :param basedir: the node's base directory + :returns: the path to the PID file + """ + return os.path.join(basedir, u"twistd.pid") + +def get_pid_from_pidfile(pidfile): + """ + Tries to read and return the PID stored in the node's PID file + (twistd.pid). + :param pidfile: try to read this PID file + :returns: A numeric PID on success, ``None`` if PID file absent or + inaccessible, ``-1`` if PID file invalid. + """ + try: + with open(pidfile, "r") as f: + pid = f.read() + except EnvironmentError: + return None + + try: + pid = int(pid) + except ValueError: + return -1 + + return pid + def identify_node_type(basedir): for fn in listdir_unicode(basedir): if fn.endswith(u".tac"): @@ -135,6 +164,12 @@ def start(config): return 1 twistd_config.loadedPlugins = {"StartTahoeNode": StartTahoeNodePlugin(nodetype, basedir)} + # handle invalid PID file (twistd might not start otherwise) + pidfile = get_pidfile(basedir) + if get_pid_from_pidfile(pidfile) == -1: + print >>err, "found invalid PID file in %s - deleting it" % basedir + os.remove(pidfile) + # On Unix-like platforms: # Unless --nodaemon was provided, the twistd.runApp() below spawns off a # child process, and the parent calls os._exit(0), so there's no way for @@ -177,21 +212,16 @@ def stop(config): basedir = config['basedir'] quoted_basedir = quote_local_unicode_path(basedir) print >>out, "STOPPING", quoted_basedir - pidfile = os.path.join(basedir, u"twistd.pid") - if not os.path.exists(pidfile): + + pidfile = get_pidfile(basedir) + pid = get_pid_from_pidfile(pidfile) + if pid is None: print >>err, "%s does not look like a running node directory (no twistd.pid)" % quoted_basedir # we define rc=2 to mean "nothing is running, but it wasn't me who # stopped it" return 2 - with open(pidfile, "r") as f: - pid = f.read() - - try: - pid = int(pid) - except ValueError: - # The error message below mimics a Twisted error message, which is - # displayed when starting a node with an invalid pidfile. - print >>err, "Pidfile %s contains non-numeric value" % pidfile + elif pid == -1: + print >>err, "%s contains an invalid PID file" % basedir # we define rc=2 to mean "nothing is running, but it wasn't me who # stopped it" return 2 From 86f79e8111cbb764a5375b49c8b58a5a98161392 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 24 Aug 2017 10:34:58 -0400 Subject: [PATCH 2/3] Add a test for the non-numeric case --- src/allmydata/test/cli/test_cli.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/allmydata/test/cli/test_cli.py b/src/allmydata/test/cli/test_cli.py index 8eec32b3d..1f39e8836 100644 --- a/src/allmydata/test/cli/test_cli.py +++ b/src/allmydata/test/cli/test_cli.py @@ -7,6 +7,7 @@ import re from twisted.trial import unittest from twisted.python.monkey import MonkeyPatcher from twisted.internet import task +from twisted.python.filepath import FilePath import allmydata from allmydata.util import fileutil, hashutil, base32, keyutil @@ -1289,3 +1290,24 @@ class Options(ReallyEqualMixin, unittest.TestCase): ["--node-directory=there", "start", "--nodaemon"]) self.failUnlessRaises(usage.UsageError, self.parse, ["start", "--basedir=here", "--nodaemon"]) + + +class Stop(unittest.TestCase): + def test_non_numeric_pid(self): + """ + If the pidfile exists but does not contain a numeric value, a complaint to + this effect is written to stderr and the non-success result is + returned. + """ + basedir = FilePath(self.mktemp().decode("ascii")) + basedir.makedirs() + basedir.child(u"twistd.pid").setContent(b"foo") + + config = startstop_node.StopOptions() + config.stdout = StringIO() + config.stderr = StringIO() + config['basedir'] = basedir.path + + result_code = startstop_node.stop(config) + self.assertEqual(2, result_code) + self.assertIn("contains non-numeric value", config.stderr.getvalue()) From 46305c74e11e056dfb1bb78cefc615fe5f3e030b Mon Sep 17 00:00:00 2001 From: Lukas Pirl Date: Thu, 24 Aug 2017 17:56:57 +0200 Subject: [PATCH 3/3] added test for (ignoring an) invalid PID file when starting a node --- src/allmydata/test/cli/test_cli.py | 70 +++++++++++++++++++++++++----- 1 file changed, 60 insertions(+), 10 deletions(-) diff --git a/src/allmydata/test/cli/test_cli.py b/src/allmydata/test/cli/test_cli.py index 1f39e8836..e30778a73 100644 --- a/src/allmydata/test/cli/test_cli.py +++ b/src/allmydata/test/cli/test_cli.py @@ -1292,8 +1292,62 @@ class Options(ReallyEqualMixin, unittest.TestCase): ["start", "--basedir=here", "--nodaemon"]) -class Stop(unittest.TestCase): - def test_non_numeric_pid(self): +class StartStop(unittest.TestCase): + + @staticmethod + def create_config(basedir): + """ + Creates a node and returns it's create config. + Returns a minimal create config for ``basedir``. + """ + config = create_node.CreateNodeOptions() + config.stdout = StringIO() + config.stderr = StringIO() + config['basedir'] = basedir.path + config['hostname'] = "testnode" + return config + + @staticmethod + def start_config(basedir): + """ + Returns a minimal start config for the node in ``basedir``. + """ + config = startstop_node.StartOptions() + config.twistd_args = [] + config.stdout = StringIO() + config.stderr = StringIO() + config['basedir'] = basedir.path + return config + + @staticmethod + def stop_config(basedir): + """ + Returns a minimal stop config for the node in ``basedir``. + """ + config = startstop_node.StopOptions() + config.stdout = StringIO() + config.stderr = StringIO() + config['basedir'] = basedir.path + return config + + def test_non_numeric_pid_start(self): + """ + If the pidfile exists but does not contain an invalid value, + a complaint to this effect is written to stderr, the pid file + is removed and the command should exit successfully. + """ + basedir = FilePath(self.mktemp().decode("ascii")) + basedir.makedirs() + create_node.create_node(self.create_config(basedir)) + basedir.child(u"twistd.pid").setContent(b"foo") + start_config = self.start_config(basedir) + result_code = startstop_node.start(start_config) + self.assertEqual(0, result_code) + self.assertIn("found invalid PID file", + start_config.stderr.getvalue()) + startstop_node.stop(self.stop_config(basedir)) + + def test_non_numeric_pid_stop(self): """ If the pidfile exists but does not contain a numeric value, a complaint to this effect is written to stderr and the non-success result is @@ -1302,12 +1356,8 @@ class Stop(unittest.TestCase): basedir = FilePath(self.mktemp().decode("ascii")) basedir.makedirs() basedir.child(u"twistd.pid").setContent(b"foo") - - config = startstop_node.StopOptions() - config.stdout = StringIO() - config.stderr = StringIO() - config['basedir'] = basedir.path - - result_code = startstop_node.stop(config) + stop_config = self.stop_config(basedir) + result_code = startstop_node.stop(stop_config) self.assertEqual(2, result_code) - self.assertIn("contains non-numeric value", config.stderr.getvalue()) + self.assertIn("contains an invalid PID file", + stop_config.stderr.getvalue())