Merge lp:~jml/launchpad/sftp-poppy into lp:launchpad

[Jonathan Lange]

Add two_stage_kill, speed up tachandler and kill by pidfile. Clean up poppy tests.

Contrary to what you might expect, this branch actually has nothing to do with SFTP. Instead, it does a bunch of cleanups and fixes to testing code that I had to touch in order to start thinking about with Poppy.

 canonical/launchpad/daemons/tachandler.py | 36 -----------------

Use osutils.kill_by_pidfile, which appeared to a direct copy and paste from this file.

 lp/services/osutils.py | 61 +++++++++++++++++++-----------

Extract the two-stage kill part of osutils.kill_by_pidfile into a separate function called two_stage_kill.

Fix a bug in the two-stage kill algorithm. If you terminate a process and don't give it the chance to report its results back to the parent process, the terminated process remains as a zombie. Without the waitpid, the polling loop will *always* run to completion and then do the SIGKILL, thus making the call take over 5s guaranteed.

Fixing this may well make some layers tear down faster.

 lp/poppy/filesystem.py | 1

Remove flakes.

 lp/poppy/tests/__init__.py | 5 ++
 lp/poppy/tests/helpers.py | 24 +++--------

Move all of the helper code out of __init__ and into helpers.py. Add a stub __init__.py.

Fix up some of the formatting in helpers, and re-use two_stage_kill, getting rid of the slow (wait for 2s) two-stage kill implementation here.

 lp/poppy/tests/test_poppy.py | 40 ++++++++-----------

Bits and pieces of cleanup:
 * Use lp.testing.TestCase
 * Fix some flakes
 * Rename tests to test_foo from testFoo, according to the standard
 * Delete the useless layer
 * Fix spelling mistakes
 * Use addCleanup instead of tearDown
 * Use makeTemporaryDirectory
 * Whitespace cleanups

 lp/soyuz/doc/soyuz-upload.txt.disabled | 2

The return value of killPoppy isn't useful and isn't used.

 lp/testing/__init__.py | 7 +--

Avoid using lambdas in addCleanup -- this is a throwback to an old addCleanup API. Re-use makeTemporaryDirectory in useTempDir.

Thanks,
jml

[Gavin Panella (allenap)]

Looks good. Two comments:

* It's not necessary to do waitpid(pid, os.WNOHANG) followed by
  kill(pid, 0); the return value from waitpid() will be (0, 0) when
  the process has not exited yet, and it will error the same as kill()
  if the process does not exist.

* It might be nice to be able to pass a subprocess.Popen into
  two_stage_kill, so that its poll() method is used, and so the
  returncode attribute is set correctly to avoid confusion. But,
  really, this is probably not worth the effort.

review: Approve

[Michael Nelson (michael.nelson)]

Review: Approve
Nothing I can add to Gavin's (which you're investigating).

On Thu, Mar 18, 2010 at 11:35 AM, Jonathan Lange <email address hidden> wrote:
> Jonathan Lange has proposed merging lp:~jml/launchpad/sftp-poppy into lp:launchpad/devel.
>
> Requested reviews:
>  Canonical Launchpad Engineering (launchpad)
>
>
> Contrary to what you might expect, this branch actually has nothing to do with SFTP. Instead, it does a bunch of cleanups and fixes to testing code that I had to touch in order to start thinking about with Poppy.
>
>  canonical/launchpad/daemons/tachandler.py |   36 -----------------
>
> Use osutils.kill_by_pidfile, which appeared to a direct copy and paste from this file.
>
>  lp/services/osutils.py                    |   61 +++++++++++++++++++-----------
>
> Extract the two-stage kill part of osutils.kill_by_pidfile into a separate function called two_stage_kill.
>
> Fix a bug in the two-stage kill algorithm. If you terminate a process and don't give it the chance to report its results back to the parent process, the terminated process remains as a zombie. Without the waitpid, the polling loop will *always* run to completion and then do the SIGKILL, thus making the call take over 5s guaranteed.

Nice.

>
> Fixing this may well make some layers tear down faster.
>
>  lp/poppy/filesystem.py                    |    1
>
> Remove flakes.
>
>  lp/poppy/tests/__init__.py                |    5 ++
>  lp/poppy/tests/helpers.py                 |   24 +++--------
>
> Move all of the helper code out of __init__ and into helpers.py. Add a stub __init__.py.
>
> Fix up some of the formatting in helpers, and re-use two_stage_kill, getting rid of the slow (wait for 2s) two-stage kill implementation here.
>
>  lp/poppy/tests/test_poppy.py              |   40 ++++++++-----------
>
> Bits and pieces of cleanup:
>  * Use lp.testing.TestCase
>  * Fix some flakes
>  * Rename tests to test_foo from testFoo, according to the standard
>  * Delete the useless layer
>  * Fix spelling mistakes
>  * Use addCleanup instead of tearDown
>  * Use makeTemporaryDirectory
>  * Whitespace cleanups
>
>  lp/soyuz/doc/soyuz-upload.txt.disabled    |    2
>
> The return value of killPoppy isn't useful and isn't used.
>
>  lp/testing/__init__.py                    |    7 +--
>
> Avoid using lambdas in addCleanup -- this is a throwback to an old addCleanup API. Re-use makeTemporaryDirectory in useTempDir.
>
> Thanks,
> jml
> --
> https://code.launchpad.net/~jml/launchpad/sftp-poppy/+merge/21627
> Your team Launchpad code reviewers from Canonical is subscribed to branch lp:launchpad/devel.
>
> === modified file 'lib/canonical/launchpad/daemons/tachandler.py'
> --- lib/canonical/launchpad/daemons/tachandler.py       2010-02-19 13:27:29 +0000
> +++ lib/canonical/launchpad/daemons/tachandler.py       2010-03-18 11:35:30 +0000
> @@ -16,12 +16,12 @@
>  import sys
>  import os
>  import time
> -from signal import SIGTERM, SIGKILL
>  import subprocess
>
>  from twisted.application import service
>  from twisted.python import log
>
> +from lp.services.osutils import kill_by_pidfile
>
>  twistd_script = os.path.abspath(os.path.join(
>     os.path.dirname(__file__),
> @@ -118,39 +11...

[Jonathan Lange (jml)]

On Thu, Mar 18, 2010 at 11:57 AM, Gavin Panella
<email address hidden> wrote:
> Review: Approve
> Looks good. Two comments:
>
> * It's not necessary to do waitpid(pid, os.WNOHANG) followed by
>  kill(pid, 0); the return value from waitpid() will be (0, 0) when
>  the process has not exited yet, and it will error the same as kill()
>  if the process does not exist.
>

Hmm. I see. I wasn't quite sure what the behaviour actually is, so I
did Science.

$ ps ax | grep 3711
 7702 pts/2 R+ 0:00 grep 3711
$ python
Python 2.6.4 (r264:75706, Dec 7 2009, 18:43:55)
[GCC 4.4.1] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import os

Spawn a long-running process

>>> pid = os.spawnv(os.P_NOWAIT, '/bin/sleep', ['/bin/sleep', '10'])
>>> pid
7820

waitpid on-existent process raises errors

>>> os.waitpid(3711, os.WNOHANG)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 10] No child processes

waitpid returns (0, 0) while it's running for (pid, result)
>>> os.waitpid(7820, os.WNOHANG)
(0, 0)

Once it's terminated, returns (pid, result) with a non-zero pid.
>>> os.waitpid(7820, os.WNOHANG)
(7820, 0)

Now that it's terminated and we've reaped it, it'll raise an exception
>>> os.waitpid(7820, os.WNOHANG)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 10] No child processes

I'll fix the code accordingly, making sure that we don't force a 0.1s
sleep if the SIGTERM was successful.

> * It might be nice to be able to pass a subprocess.Popen into
>  two_stage_kill, so that its poll() method is used, and so the
>  returncode attribute is set correctly to avoid confusion. But,
>  really, this is probably not worth the effort.

It's valuable having a low-level function that takes a pid. I agree
that what you describe would also be useful, but maybe not right now.
I wonder what we have to do to encourage others who might need this to
stick code into this module and re-use two-stage kill.

Thanks!
jml

[Gavin Panella (allenap)]

jml wrote:
> I wonder what we have to do to encourage others who might need this to
> stick code into this module and re-use two-stage kill.

Get it into the standard library! :)

=== modified file 'lib/canonical/launchpad/daemons/tachandler.py'
--- lib/canonical/launchpad/daemons/tachandler.py	2010-02-19 13:27:29 +0000
+++ lib/canonical/launchpad/daemons/tachandler.py	2010-03-18 19:22:29 +0000
@@ -7,11 +7,16 @@
 
 __metaclass__ = type
 
-__all__ = ['TacTestSetup', 'ReadyService', 'TacException']
-
-
-# This file is used by launchpad-buildd, so it cannot import any
-# Launchpad code!
+__all__ = [
+    'TacTestSetup',
+    'ReadyService',
+    'TacException',
+    'kill_by_pidfile',
+    'remove_if_exists',
+    'two_stage_kill',
+    ]
+
+
 import errno
 import sys
 import os
@@ -22,6 +27,77 @@
 from twisted.application import service
 from twisted.python import log
 
+# This file is used by launchpad-buildd, so it cannot import any
+# Launchpad code!
+
+
+def two_stage_kill(pid, poll_interval=0.1, num_polls=50):
+    """Kill process 'pid' with SIGTERM. If it doesn't die, SIGKILL it.
+
+    :param pid: The pid of the process to kill.
+    :param poll_interval: The polling interval used to check if the
+        process is still around.
+    :param num_polls: The number of polls to do before doing a SIGKILL.
+    """
+    # Kill the process.
+    try:
+        os.kill(pid, SIGTERM)
+    except OSError, e:
+        if e.errno in (errno.ESRCH, errno.ECHILD):
+            # Process has already been killed.
+            return
+
+    # Poll until the process has ended.
+    for i in range(num_polls):
+        try:
+            # Reap the child process and get its return value. If it's not
+            # gone yet, continue.
+            new_pid, result = os.waitpid(pid, os.WNOHANG)
+            if new_pid:
+                return result
+            time.sleep(poll_interval)
+        except OSError, e:
+            # Raised if the process is gone by the time we try to get the
+            # return value.
+            return
+
+    # The process is still around, so terminate it violently.
+    try:
+        os.kill(pid, SIGKILL)
+    except OSError:
+        # Already terminated
+        pass
+
+
+def kill_by_pidfile(pidfile_path):
+    """Kill a process identified by the pid stored in a file.
+
+    The pid file is removed from disk.
+    """
+    if not os.path.exists(pidfile_path):
+        return
+    try:
+        # Get the pid.
+        pid = open(pidfile_path, 'r').read().split()[0]
+        try:
+            pid = int(pid)
+        except ValueError:
+            # pidfile contains rubbish
+            return
+
+        two_stage_kill(pid)
+    finally:
+        remove_if_exists(pidfile_path)
+
+
+def remove_if_exists(path):
+    """Remove the given file if it exists."""
+    try:
+        os.remove(path)
+    except OSError, e:
+        if e.errno != errno.ENOENT:
+            raise
+
 
 twistd_script = os.path.abspath(os.path.join(
     os.path.dirname(__file__),
@@ -48,7 +124,7 @@
         # started. If it sees an old logfile, then it will find the LOG_MAGIC
         # string and return immediately, provoking hard-to-diagnose race
         # conditions. Delete the logfile to make sure this does not happen.
-        self._removeFile(self.logfile)
+        remove_if_exists(self.logfile)
 
         self.setUpRoot()
         args = [sys.executable, twistd_script, '-o', '-y', self.tacfile,
@@ -107,50 +183,10 @@
     def tearDown(self):
         self.killTac()
 
-    def _removeFile(self, filename):
-        """Remove the given file if it exists."""
-        try:
-            os.remove(filename)
-        except OSError, e:
-            if e.errno != errno.ENOENT:
-                raise
-
     def killTac(self):
         """Kill the TAC file if it is running."""
         pidfile = self.pidfile
-        if not os.path.exists(pidfile):
-            return
-
-        # Get the pid.
-        pid = open(pidfile, 'r').read().strip()
-        try:
-            pid = int(pid)
-        except ValueError:
-            # pidfile contains rubbish
-            return
-
-        # Kill the process.
-        try:
-            os.kill(pid, SIGTERM)
-        except OSError, e:
-            if e.errno in (errno.ESRCH, errno.ECHILD):
-                # Process has already been killed.
-                return
-
-        # Poll until the process has ended.
-        for i in range(50):
-            try:
-                os.kill(pid, 0)
-                time.sleep(0.1)
-            except OSError, e:
-                break
-        else:
-            # The process is still around, so terminate it violently.
-            try:
-                os.kill(pid, SIGKILL)
-            except OSError:
-                # Already terminated
-                pass
+        kill_by_pidfile(pidfile)
 
     def setUpRoot(self):
         """Override this.

=== modified file 'lib/lp/poppy/filesystem.py'
--- lib/lp/poppy/filesystem.py	2009-06-25 05:30:52 +0000
+++ lib/lp/poppy/filesystem.py	2010-03-18 19:22:29 +0000
@@ -6,7 +6,6 @@
 import datetime
 import os
 import stat
-import shutil
 from zope.security.interfaces import Unauthorized
 from zope.server.interfaces.ftp import IFileSystem
 from zope.interface import implements

=== added file 'lib/lp/poppy/tests/__init__.py'
--- lib/lp/poppy/tests/__init__.py	1970-01-01 00:00:00 +0000
+++ lib/lp/poppy/tests/__init__.py	2010-03-18 19:22:29 +0000
@@ -0,0 +1,5 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for Poppy."""
+

=== renamed file 'lib/lp/poppy/tests/__init__.py' => 'lib/lp/poppy/tests/helpers.py'
--- lib/lp/poppy/tests/__init__.py	2009-06-25 05:39:50 +0000
+++ lib/lp/poppy/tests/helpers.py	2010-03-18 19:22:29 +0000
@@ -5,16 +5,18 @@
 
 import os
 import select
-import signal
 import subprocess
 import sys
 import time
 
 from canonical.config import config
+from lp.services.osutils import two_stage_kill
+
 
 class SoyuzUploadError(Exception):
     """Used in the soyuz-upload test."""
 
+
 class PoppyTestSetup:
     """Provides a setup and teardown mechanism for a poppy subprocess instance.
 
@@ -34,28 +36,18 @@
     def startPoppy(self):
         """Start the poppy instance."""
         script = os.path.join(config.root, "daemons/poppy-upload.py")
-        self.process = subprocess.Popen([sys.executable, script,
-                                         "--allow-user", self.user,
-                                         "--cmd", self.cmd,
-                                         self.fsroot, self.port],
-                                        stdout=subprocess.PIPE)
+        self.process = subprocess.Popen(
+            [sys.executable, script, "--allow-user", self.user,
+             "--cmd", self.cmd, self.fsroot, self.port],
+            stdout=subprocess.PIPE)
         self.running = True
 
     def killPoppy(self):
         """Kill the poppy instance dead."""
         if not self.alive:
             return
-        os.kill(self.process.pid, signal.SIGTERM)
-        # Give poppy a chance to die
-        time.sleep(2)
-        # Look to see if it has died yet
-        ret = self.process.poll()
-        if ret is None:
-            # Poppy had not died, so send it SIGKILL
-            os.kill(self.process.pid, signal.SIGKILL)
-        # Finally return the result.
+        two_stage_kill(self.process.pid)
         self.running = False
-        return self.process.wait()
 
     @property
     def alive(self):

=== modified file 'lib/lp/poppy/tests/test_poppy.py'
--- lib/lp/poppy/tests/test_poppy.py	2010-03-17 11:04:20 +0000
+++ lib/lp/poppy/tests/test_poppy.py	2010-03-18 19:22:29 +0000
@@ -7,45 +7,38 @@
 
 import ftplib
 import os
-import shutil
 import socket
-import tempfile
 import unittest
 import StringIO
 
-from canonical.testing import LaunchpadZopelessLayer
-from lp.poppy.tests import PoppyTestSetup
-
-
-class TestPoppy(unittest.TestCase):
+from lp.poppy.tests.helpers import PoppyTestSetup
+from lp.testing import TestCase
+
+
+class TestPoppy(TestCase):
     """Test if poppy.py daemon works properly."""
 
-    layer = LaunchpadZopelessLayer
-
     def setUp(self):
         """Set up poppy in a temp dir."""
-        self.root_dir = tempfile.mkdtemp()
+        super(TestPoppy, self).setUp()
+        self.root_dir = self.makeTemporaryDirectory()
         self.port = 3421
         self.poppy = PoppyTestSetup(self.root_dir, port=self.port,
                                     cmd='echo CLOSED')
         self.poppy.startPoppy()
-
-    def tearDown(self):
-        """Purge poppy root directory."""
-        self.poppy.killPoppy()
-        shutil.rmtree(self.root_dir)
+        self.addCleanup(self.poppy.killPoppy)
 
     def getFTPConnection(self, login=True, user="ubuntu", password=""):
         """Build and return a FTP connection to the current poppy.
 
-        Optionally log in with as 'annonymous' & empty password, or passed
+        Optionally log in with as 'anonymous' & empty password, or passed
         user/password.
         """
         conn = ftplib.FTP()
         # poppy usually takes sometime to come up, we need to wait, or insist.
         while True:
             try:
-                reply = conn.connect("localhost", self.port)
+                conn.connect("localhost", self.port)
             except socket.error:
                 if not self.poppy.alive:
                     raise
@@ -75,7 +68,7 @@
         upload_dir = contents[1]
         return os.path.join(self.root_dir, upload_dir, path)
 
-    def testLOGIN(self):
+    def test_LOGIN(self):
         """Check login procedure and the state of the toplevel lockfile.
 
         The toplevel lockfile should allow the process-upload runner (lp_queue,
@@ -90,7 +83,7 @@
         lockfile_path = os.path.join(self.root_dir, '.lock')
         self.assertEqual(os.stat(lockfile_path).st_mode, 0100664)
 
-    def testCWD(self):
+    def test_CWD(self):
         """Check automatic creation of directories 'cwd'ed in.
 
         Also ensure they are created with proper permission (g+rwxs)
@@ -107,7 +100,7 @@
         self.assertTrue(os.path.exists(wanted_path))
         self.assertEqual(os.stat(wanted_path).st_mode, 042775)
 
-    def testMKD(self):
+    def test_MKD(self):
         """Check recursive MKD (aka mkdir -p).
 
         Also ensure they are created with proper permission (g+rwxs)
@@ -128,7 +121,7 @@
         self.assertTrue(os.path.exists(wanted_path))
         self.assertEqual(os.stat(wanted_path).st_mode, 042775)
 
-    def testRMD(self):
+    def test_RMD(self):
         """Check recursive RMD (aka rmdir)"""
         conn = self.getFTPConnection()
         self.assertEqual(
@@ -143,7 +136,7 @@
         wanted_path = self._uploadPath('foo')
         self.assertFalse(os.path.exists(wanted_path))
 
-    def testSTOR(self):
+    def test_STOR(self):
         """Check if the parent directories are created during file upload.
 
         The uploaded file permissions are also special (g+rwxs).
@@ -160,7 +153,7 @@
         self.assertEqual(fs_content, "fake contents")
         self.assertEqual(os.stat(wanted_path).st_mode, 0102674)
 
-    def testUploadIsolation(self):
+    def test_uploadIsolation(self):
         """Check if poppy isolates the uploads properly.
 
         Upload should be done atomically, i.e., poppy should isolate the
@@ -216,5 +209,6 @@
                 self.root_dir, upload_dirs[index], "test")).read()
             self.assertEqual(content, expected_contents[index])
 
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/services/osutils.py'
--- lib/lp/services/osutils.py	2009-12-21 02:50:08 +0000
+++ lib/lp/services/osutils.py	2010-03-18 19:22:29 +0000
@@ -8,67 +8,21 @@
     'remove_tree',
     'kill_by_pidfile',
     'remove_if_exists',
+    'two_stage_kill',
     ]
 
-import errno
 import os.path
-from signal import SIGTERM, SIGKILL
 import shutil
-import time
+
+
+from canonical.launchpad.daemons.tachandler import (
+    kill_by_pidfile,
+    remove_if_exists,
+    two_stage_kill,
+    )
 
 
 def remove_tree(path):
     """Remove the tree at 'path' from disk."""
     if os.path.exists(path):
         shutil.rmtree(path)
-
-
-def kill_by_pidfile(pidfile_path):
-    """Kill a process identified by the pid stored in a file.
-
-    The pid file is removed from disk.
-    """
-    if not os.path.exists(pidfile_path):
-        return
-    try:
-        # Get the pid.
-        pid = open(pidfile_path, 'r').read().split()[0]
-        try:
-            pid = int(pid)
-        except ValueError:
-            # pidfile contains rubbish
-            return
-
-        # Kill the process.
-        try:
-            os.kill(pid, SIGTERM)
-        except OSError, e:
-            if e.errno in (errno.ESRCH, errno.ECHILD):
-                # Process has already been killed.
-                return
-
-        # Poll until the process has ended.
-        for i in range(50):
-            try:
-                os.kill(pid, 0)
-                time.sleep(0.1)
-            except OSError, e:
-                break
-        else:
-            # The process is still around, so terminate it violently.
-            try:
-                os.kill(pid, SIGKILL)
-            except OSError:
-                # Already terminated
-                pass
-    finally:
-        remove_if_exists(pidfile_path)
-
-
-def remove_if_exists(path):
-    """Remove the given file if it exists."""
-    try:
-        os.remove(path)
-    except OSError, e:
-        if e.errno != errno.ENOENT:
-            raise

=== modified file 'lib/lp/soyuz/doc/soyuz-upload.txt.disabled'
--- lib/lp/soyuz/doc/soyuz-upload.txt.disabled	2010-03-17 11:04:20 +0000
+++ lib/lp/soyuz/doc/soyuz-upload.txt.disabled	2010-03-18 19:22:29 +0000
@@ -186,7 +186,7 @@
 Right, that's all we need from the FTP server. We don't need it anymore,
 so we'll just kill the process.
 
-  >>> status = poppy.killPoppy()
+  >>> poppy.killPoppy()
 
 Finally, we'll just create an entirely empty upload folder. We rely for
 our tests on a poppy-like naming system, ie. that the upload folder

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2010-03-16 18:03:34 +0000
+++ lib/lp/testing/__init__.py	2010-03-18 19:22:29 +0000
@@ -215,7 +215,7 @@
     def makeTemporaryDirectory(self):
         """Create a temporary directory, and return its path."""
         tempdir = tempfile.mkdtemp()
-        self.addCleanup(lambda: shutil.rmtree(tempdir))
+        self.addCleanup(shutil.rmtree, tempdir)
         return tempdir
 
     def assertProvides(self, obj, interface):
@@ -380,11 +380,10 @@
 
     def useTempDir(self):
         """Use a temporary directory for this test."""
-        tempdir = tempfile.mkdtemp()
-        self.addCleanup(lambda: shutil.rmtree(tempdir))
+        tempdir = self.makeTemporaryDirectory()
         cwd = os.getcwd()
         os.chdir(tempdir)
-        self.addCleanup(lambda: os.chdir(cwd))
+        self.addCleanup(os.chdir, cwd)
 
 
 class TestCaseWithFactory(TestCase):