Launchpad itself

Code review comment for lp:~jml/launchpad/sftp-poppy

sftp-poppy
Merge into devel

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2010-03-18:

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.

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

On Thu, Mar 18, 2010 at 11:35 AM, Jonathan Lange <jml@canonical.com> 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.

« Back to merge proposal