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 +118,7 @@
>     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)

Yay for code removal.

>
>     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 11:35:30 +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 11:35:30 +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 LICEN
I'm guessing you don't want to included this in this branch?
SE).
> +
> +"""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 11:35:30 +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()

OK, you said you checked all the call-sites and no-one used the return value.

>
>     @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 11:35:30 +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=""):

Right, I'd misunderstood our test style guides to mean that all
methods in our test-cases that we define should_user_under, but it's
just test methods.