Merge lp:~allenap/launchpad/ec2-test-race-bug-422433-the-revenge into lp:launchpad

[Gavin Panella]

[Gavin Panella (allenap)]

This is a resubmission of an earlier branch:

  https://code.edge.launchpad.net/~allenap/launchpad/ec2-test-race-bug-422433/+merge/12778

It got merged, but had to be reverted because of failures in buildbot. This branch restores it, with some additional fixes to make it pass in buildbot:

  http://pastebin.ubuntu.com/288542/

No need to review the whole branch :)

This depends on https://code.edge.launchpad.net/~allenap/lp-source-dependencies/ec2-test-race-bug-422433-the-revenge/+merge/13063.

[Michael Nelson (michael.nelson)]

> This is a resubmission of an earlier branch:
>
> https://code.edge.launchpad.net/~allenap/launchpad/ec2-test-race-
> bug-422433/+merge/12778
>
> It got merged, but had to be reverted because of failures in buildbot.

<noodles775> allenap: so the previous mp failed on buildbot because it didn't have paramiko?
* andrea-bs (n=andrea@ubuntu/member/beeseek.developer.andrea-bs) has joined #launchpad-reviews
* henninge has quit (Read error: 113 (No route to host))
<allenap> noodles775: Yes, the buildbot AMI seems to be out of date, which is another bug. However, I thought it was a good idea to put the dep into the tree anyway.
<noodles775> allenap: ok, and where is pycrypto used?
<allenap> noodles775: It's required by paramiko :)
<noodles775> allenap: r=me
<allenap> noodles775: Thanks :)

review: Approve (code)

=== modified file 'lib/devscripts/ec2test/account.py'
--- lib/devscripts/ec2test/account.py	2009-10-03 08:19:54 +0000
+++ lib/devscripts/ec2test/account.py	2009-10-08 14:53:12 +0000
@@ -13,7 +13,10 @@
 import sys
 import urllib
 
+from datetime import datetime
+
 from boto.exception import EC2ResponseError
+from devscripts.ec2test.session import EC2SessionName
 
 import paramiko
 
@@ -24,6 +27,13 @@
     # ...anyone else want in on the fun?
     )
 
+AUTH_FAILURE_MESSAGE = """\
+POSSIBLE CAUSES OF ERROR:
+- Did you sign up for EC2?
+- Did you put a credit card number in your AWS account?
+Please double-check before reporting a problem.
+"""
+
 
 def get_ip():
     """Uses AWS checkip to obtain this machine's IP address.
@@ -64,44 +74,29 @@
         sys.stdout.write(msg)
         sys.stdout.flush()
 
+    def _find_expired_artifacts(self, artifacts):
+        now = datetime.utcnow()
+        for artifact in artifacts:
+            session_name = EC2SessionName(artifact.name)
+            if (session_name in (self.name, self.name.base) or (
+                    session_name.base == self.name.base and
+                    session_name.expires is not None and
+                    session_name.expires < now)):
+                yield artifact
+
     def acquire_security_group(self, demo_networks=None):
         """Get a security group with the appropriate configuration.
 
         "Appropriate" means configured to allow this machine to connect via
         SSH, HTTP and HTTPS.
 
-        If a group is already configured with this name for this connection,
-        then re-use that. Otherwise, create a new security group and configure
-        it appropriately.
-
         The name of the security group is the `EC2Account.name` attribute.
 
         :return: A boto security group.
         """
         if demo_networks is None:
             demo_networks = []
-        try:
-            group = self.conn.get_all_security_groups(self.name)[0]
-        except EC2ResponseError, e:
-            if e.code != 'InvalidGroup.NotFound':
-                raise
-        else:
-            # If an existing security group was configured, try deleting it
-            # since our external IP might have changed.
-            try:
-                group.delete()
-            except EC2ResponseError, e:
-                if e.code != 'InvalidGroup.InUse':
-                    raise
-                # Otherwise, it means that an instance is already using
-                # it, so simply re-use it. It's unlikely that our IP changed!
-                #
-                # XXX: JonathanLange 2009-06-05: If the security group exists
-                # already, verify that the current IP is permitted; if it is
-                # not, make an INFO log and add the current IP.
-                self.log("Security group already in use, so reusing.\n")
-                return group
-
+        # Create the security group.
         security_group = self.conn.create_security_group(
             self.name, 'Authorization to access the test runner instance.')
         # Authorize SSH and HTTP.
@@ -117,34 +112,49 @@
             security_group.authorize('tcp', 443, 443, network)
         return security_group
 
+    def delete_previous_security_groups(self):
+        """Delete previously used security groups, if found."""
+        expired_groups = self._find_expired_artifacts(
+            self.conn.get_all_security_groups())
+        for group in expired_groups:
+            try:
+                group.delete()
+            except EC2ResponseError, e:
+                if e.code != 'InvalidGroup.InUse':
+                    raise
+                self.log('Cannot delete; security group '
+                         '%r in use.\n' % group.name)
+            else:
+                self.log('Deleted security group %r.\n' % group.name)
+
     def acquire_private_key(self):
         """Create & return a new key pair for the test runner."""
         key_pair = self.conn.create_key_pair(self.name)
         return paramiko.RSAKey.from_private_key(
             cStringIO.StringIO(key_pair.material.encode('ascii')))
 
-    def delete_previous_key_pair(self):
-        """Delete previously used keypair, if it exists."""
-        try:
-            # Only one keypair will match 'self.name' since it's a unique
-            # identifier.
-            key_pairs = self.conn.get_all_key_pairs(self.name)
-            assert len(key_pairs) == 1, (
-                "Should be only one keypair, found %d (%s)"
-                % (len(key_pairs), key_pairs))
-            key_pair = key_pairs[0]
-            key_pair.delete()
-        except EC2ResponseError, e:
-            if e.code != 'InvalidKeyPair.NotFound':
-                if e.code == 'AuthFailure':
-                    # Inserted because of previous support issue.
-                    self.log(
-                        'POSSIBLE CAUSES OF ERROR:\n'
-                        '  Did you sign up for EC2?\n'
-                        '  Did you put a credit card number in your AWS '
-                        'account?\n'
-                        'Please doublecheck before reporting a problem.\n')
-                raise
+    def delete_previous_key_pairs(self):
+        """Delete previously used keypairs, if found."""
+        expired_key_pairs = self._find_expired_artifacts(
+            self.conn.get_all_key_pairs())
+        for key_pair in expired_key_pairs:
+            try:
+                key_pair.delete()
+            except EC2ResponseError, e:
+                if e.code != 'InvalidKeyPair.NotFound':
+                    if e.code == 'AuthFailure':
+                        # Inserted because of previous support issue.
+                        self.log(AUTH_FAILURE_MESSAGE)
+                    raise
+                self.log('Cannot delete; key pair not '
+                         'found %r\n' % key_pair.name)
+            else:
+                self.log('Deleted key pair %r.\n' % key_pair.name)
+
+    def collect_garbage(self):
+        """Remove any old keys and security groups."""
+        self.delete_previous_security_groups()
+        self.delete_previous_key_pairs()
 
     def acquire_image(self, machine_id):
         """Get the image.

=== modified file 'lib/devscripts/ec2test/builtins.py'
--- lib/devscripts/ec2test/builtins.py	2009-10-05 13:31:40 +0000
+++ lib/devscripts/ec2test/builtins.py	2009-10-08 14:53:12 +0000
@@ -22,6 +22,7 @@
 from devscripts.ec2test.credentials import EC2Credentials
 from devscripts.ec2test.instance import (
     AVAILABLE_INSTANCE_TYPES, DEFAULT_INSTANCE_TYPE, EC2Instance)
+from devscripts.ec2test.session import EC2SessionName
 from devscripts.ec2test.testrunner import EC2TestRunner, TRUNK_BRANCH
 
 
@@ -240,8 +241,8 @@
         postmortem_option,
         Option(
             'headless',
-            help=('After building the instance and test, run the remote tests '
-                  'headless.  Cannot be used with postmortem '
+            help=('After building the instance and test, run the remote '
+                  'tests headless.  Cannot be used with postmortem '
                   'or file.')),
         debug_option,
         Option(
@@ -285,8 +286,9 @@
                 "Submitting to PQM with non-default test options isn't "
                 "supported")
 
+        session_name = EC2SessionName.make(EC2TestRunner.name)
         instance = EC2Instance.make(
-            EC2TestRunner.name, instance_type, machine)
+            session_name, instance_type, machine)
 
         runner = EC2TestRunner(
             test_branch, email=email, file=file,
@@ -426,8 +428,9 @@
         branches, test_branch = _get_branches_and_test_branch(
             trunk, branch, test_branch)
 
+        session_name = EC2SessionName.make(EC2TestRunner.name)
         instance = EC2Instance.make(
-            EC2TestRunner.name, instance_type, machine, demo)
+            session_name, instance_type, machine, demo)
 
         runner = EC2TestRunner(
             test_branch, branches=branches,
@@ -480,8 +483,9 @@
         ListOption(
             'extra-update-image-command', type=str,
             help=('Run this command (with an ssh agent) on the image before '
-                  'running the default update steps.  Can be passed more than '
-                  'once, the commands will be run in the order specified.')),
+                  'running the default update steps.  Can be passed more '
+                  'than once, the commands will be run in the order '
+                  'specified.')),
         Option(
             'public',
             help=('Remove proprietary code from the sourcecode directory '
@@ -498,8 +502,9 @@
 
         credentials = EC2Credentials.load_from_file()
 
+        session_name = EC2SessionName.make(EC2TestRunner.name)
         instance = EC2Instance.make(
-            EC2TestRunner.name, instance_type, machine,
+            session_name, instance_type, machine,
             credentials=credentials)
         instance.check_bundling_prerequisites()
 

=== modified file 'lib/devscripts/ec2test/instance.py'
--- lib/devscripts/ec2test/instance.py	2009-10-04 15:40:46 +0000
+++ lib/devscripts/ec2test/instance.py	2009-10-08 14:53:12 +0000
@@ -24,6 +24,7 @@
 import paramiko
 
 from devscripts.ec2test.credentials import EC2Credentials
+from devscripts.ec2test.session import EC2SessionName
 
 
 DEFAULT_INSTANCE_TYPE = 'c1.xlarge'
@@ -160,6 +161,18 @@
 """
 
 
+postmortem_banner = """\
+Postmortem Console. EC2 instance is not yet dead.
+It will shut down when you exit this prompt (CTRL-D)
+
+Tab-completion is enabled.
+EC2Instance is available as `instance`.
+Also try these:
+  http://%(dns)s/current_test.log
+  ssh -A %(dns)s
+"""
+
+
 class EC2Instance:
     """A single EC2 instance."""
 
@@ -170,6 +183,7 @@
 
         :param name: The name to use for the key pair and security group for
             the instance.
+        :type name: `EC2SessionName`
         :param instance_type: One of the AVAILABLE_INSTANCE_TYPES.
         :param machine_id: The AMI to use, or None to do the usual regexp
             matching.  If you put 'based-on:' before the AMI id, it is assumed
@@ -180,6 +194,7 @@
             to allow access to the instance.
         :param credentials: An `EC2Credentials` object.
         """
+        assert isinstance(name, EC2SessionName)
         if instance_type not in AVAILABLE_INSTANCE_TYPES:
             raise ValueError('unknown instance_type %s' % (instance_type,))
 
@@ -200,7 +215,7 @@
         # We always recreate the keypairs because there is no way to
         # programmatically retrieve the private key component, unless we
         # generate it.
-        account.delete_previous_key_pair()
+        account.collect_garbage()
 
         if machine_id and machine_id.startswith('based-on:'):
             from_scratch = True
@@ -345,8 +360,8 @@
             if our_connection:
                 connection.close()
             self.log(
-                'You can now use ssh -A ec2test@%s to log in the instance.\n' %
-                self.hostname)
+                'You can now use ssh -A ec2test@%s to '
+                'log in the instance.\n' % self.hostname)
             self._ec2test_user_has_keys = True
 
     def connect(self):
@@ -401,18 +416,8 @@
             try:
                 if postmortem:
                     console = code.InteractiveConsole(locals())
-                    console.interact((
-                        'Postmortem Console.  EC2 instance is not yet dead.\n'
-                        'It will shut down when you exit this prompt '
-                        '(CTRL-D).\n'
-                        '\n'
-                        'Tab-completion is enabled.'
-                        '\n'
-                        'EC2Instance is available as `instance`.\n'
-                        'Also try these:\n'
-                        '  http://%(dns)s/current_test.log\n'
-                        '  ssh -A %(dns)s') %
-                                     {'dns': self.hostname})
+                    console.interact(
+                        postmortem_banner % {'dns': self.hostname})
                     print 'Postmortem console closed.'
             finally:
                 if shutdown:

=== added file 'lib/devscripts/ec2test/session.py'
--- lib/devscripts/ec2test/session.py	1970-01-01 00:00:00 +0000
+++ lib/devscripts/ec2test/session.py	2009-10-08 14:53:12 +0000
@@ -0,0 +1,90 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Code to represent a single session of EC2 use."""
+
+__metaclass__ = type
+__all__ = [
+    'EC2SessionName',
+    ]
+
+from datetime import datetime, timedelta
+
+from devscripts.ec2test.utils import (
+    find_datetime_string, make_datetime_string, make_random_string)
+
+
+DEFAULT_LIFETIME = timedelta(hours=6)
+
+
+class EC2SessionName(str):
+    """A name for an EC2 session.
+
+    This is used when naming key pairs and security groups, so it's
+    useful to be unique. However, to aid garbage collection of old key
+    pairs and security groups, the name contains a common element and
+    an expiry timestamp. The form taken should always be:
+
+      <base-name>/<expires-timestamp>/<random-data>
+
+    None of the parts should contain forward-slashes, and the
+    timestamp should be acceptable input to `find_datetime_string`.
+
+    `EC2SessionName.make()` will generate a suitable name given a
+    suitable base name.
+    """
+
+    @classmethod
+    def make(cls, base, expires=None):
+        """Create an `EC2SessionName`.
+
+        This checks that `base` does not contain a forward-slash, and
+        provides some convenient functionality for `expires`:
+
+        - If `expires` is None, it defaults to now (UTC) plus
+          `DEFAULT_LIFETIME`.
+
+        - If `expires` is a `datetime`, it is converted to a timestamp
+          in the correct form.
+
+        - If `expires` is a `timedelta`, it is added to now (UTC) then
+          converted to a timestamp.
+
+        - Otherwise `expires` is assumed to be a string, so is checked
+          for the absense of forward-slashes, and that a correctly
+          formed timestamp can be discovered.
+
+        """
+        assert '/' not in base
+        if expires is None:
+            expires = DEFAULT_LIFETIME
+        if isinstance(expires, timedelta):
+            expires = datetime.utcnow() + expires
+        if isinstance(expires, datetime):
+            expires = make_datetime_string(expires)
+        else:
+            assert '/' not in expires
+            assert find_datetime_string(expires) is not None
+        rand = make_random_string(8)
+        return cls("%s/%s/%s" % (base, expires, rand))
+
+    @property
+    def base(self):
+        parts = self.split('/')
+        if len(parts) != 3:
+            return None
+        return parts[0]
+
+    @property
+    def expires(self):
+        parts = self.split('/')
+        if len(parts) != 3:
+            return None
+        return find_datetime_string(parts[1])
+
+    @property
+    def rand(self):
+        parts = self.split('/')
+        if len(parts) != 3:
+            return None
+        return parts[2]

=== added directory 'lib/devscripts/ec2test/tests'
=== added file 'lib/devscripts/ec2test/tests/__init__.py'
--- lib/devscripts/ec2test/tests/__init__.py	1970-01-01 00:00:00 +0000
+++ lib/devscripts/ec2test/tests/__init__.py	2009-10-08 14:53:12 +0000
@@ -0,0 +1,2 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).

=== added file 'lib/devscripts/ec2test/tests/test_session.py'
--- lib/devscripts/ec2test/tests/test_session.py	1970-01-01 00:00:00 +0000
+++ lib/devscripts/ec2test/tests/test_session.py	2009-10-08 14:53:12 +0000
@@ -0,0 +1,69 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test the session module."""
+
+__metaclass__ = type
+
+import re
+import unittest
+
+from datetime import datetime, timedelta
+
+from devscripts.ec2test import session
+
+
+class TestEC2SessionName(unittest.TestCase):
+    """Tests for EC2SessionName."""
+
+    def test_make(self):
+        # EC2SessionName.make() is the most convenient way to create
+        # valid names.
+        name = session.EC2SessionName.make("fred")
+        check = re.compile(
+            r'^fred/\d{4}-\d{2}-\d{2}-\d{4}/[0-9a-zA-Z]{8}$').match
+        self.failIf(check(name) is None, "Did not match %r" % name)
+        possible_expires = [
+            None, '1986-04-26-0123', timedelta(hours=10),
+            datetime(1986, 04, 26, 1, 23)
+            ]
+        for expires in possible_expires:
+            name = session.EC2SessionName.make("fred", expires)
+            self.failIf(check(name) is None, "Did not match %r" % name)
+
+    def test_properties(self):
+        # A valid EC2SessionName has properies to access the three
+        # components of its name.
+        base = "fred"
+        timestamp = datetime(1986, 4, 26, 1, 23)
+        timestamp_string = '1986-04-26-0123'
+        rand = 'abcdef123456'
+        name = session.EC2SessionName(
+            "%s/%s/%s" % (base, timestamp_string, rand))
+        self.failUnlessEqual(base, name.base)
+        self.failUnlessEqual(timestamp, name.expires)
+        self.failUnlessEqual(rand, name.rand)
+
+    def test_invalid_base(self):
+        # If the given base contains a forward-slash, an
+        # AssertionError should be raised.
+        self.failUnlessRaises(
+            AssertionError, session.EC2SessionName.make, "forward/slash")
+
+    def test_invalid_timestamp(self):
+        # If the given expiry timestamp contains a forward-slash, an
+        # AssertionError should be raised.
+        self.failUnlessRaises(
+            AssertionError, session.EC2SessionName.make, "fred", "/")
+        # If the given expiry timestamp does not contain a timestamp
+        # in the correct form, an AssertionError should be raised.
+        self.failUnlessRaises(
+            AssertionError, session.EC2SessionName.make, "fred", "1986.04.26")
+
+    def test_form_not_correct(self):
+        # If the form of the string is not base/timestamp/rand then
+        # the corresponding properties should all return None.
+        broken_name = session.EC2SessionName('bob')
+        self.failUnless(broken_name.base is None)
+        self.failUnless(broken_name.expires is None)
+        self.failUnless(broken_name.rand is None)

=== added file 'lib/devscripts/ec2test/tests/test_utils.py'
--- lib/devscripts/ec2test/tests/test_utils.py	1970-01-01 00:00:00 +0000
+++ lib/devscripts/ec2test/tests/test_utils.py	2009-10-08 14:53:12 +0000
@@ -0,0 +1,61 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test the utils module."""
+
+__metaclass__ = type
+
+import unittest
+
+from datetime import datetime
+
+from devscripts.ec2test import utils
+
+
+class TestDateTimeUtils(unittest.TestCase):
+    """Tests for date/time related utilities."""
+
+    example_date = datetime(1986, 4, 26, 1, 23)
+    example_date_string = '1986-04-26-0123'
+    example_date_text = (
+        'blah blah foo blah 23545 646 ' +
+        example_date_string + ' 435 blah')
+
+    def test_make_datetime_string(self):
+        self.failUnlessEqual(
+            self.example_date_string,
+            utils.make_datetime_string(self.example_date))
+
+    def test_find_datetime_string(self):
+        self.failUnlessEqual(
+            self.example_date,
+            utils.find_datetime_string(self.example_date_string))
+        self.failUnlessEqual(
+            self.example_date,
+            utils.find_datetime_string(self.example_date_text))
+
+
+class TestRandomUtils(unittest.TestCase):
+    """Tests for randomness related utilities."""
+
+    hex_chars = frozenset('0123456789abcdefABCDEF')
+
+    def test_make_random_string(self):
+        rand_a = utils.make_random_string()
+        rand_b = utils.make_random_string()
+        self.failIfEqual(rand_a, rand_b)
+        self.failUnlessEqual(32, len(rand_a))
+        self.failUnlessEqual(32, len(rand_b))
+        self.failUnless(self.hex_chars.issuperset(rand_a))
+        self.failUnless(self.hex_chars.issuperset(rand_b))
+
+    def test_make_random_string_with_length(self):
+        for length in (8, 16, 64):
+            rand = utils.make_random_string(length)
+            self.failUnlessEqual(length, len(rand))
+            self.failUnless(self.hex_chars.issuperset(rand))
+
+    def test_make_random_string_with_bad_length(self):
+        # length must be a multiple of 2.
+        self.failUnlessRaises(
+            AssertionError, utils.make_random_string, 15)

=== added file 'lib/devscripts/ec2test/utils.py'
--- lib/devscripts/ec2test/utils.py	1970-01-01 00:00:00 +0000
+++ lib/devscripts/ec2test/utils.py	2009-10-08 14:53:12 +0000
@@ -0,0 +1,55 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""General useful stuff."""
+
+__metaclass__ = type
+__all__ = [
+    'find_datetime_string',
+    'make_datetime_string',
+    'make_random_string',
+    ]
+
+
+import binascii
+import datetime
+import os
+import re
+
+
+def make_datetime_string(when=None):
+    """Generate a simple formatted date and time string.
+
+    This is intended to be embedded in text to be later found by
+    `find_datetime_string`.
+    """
+    if when is None:
+        when = datetime.datetime.utcnow()
+    return when.strftime('%Y-%m-%d-%H%M')
+
+
+re_find_datetime = re.compile(
+    r'(\d{4})-(\d{2})-(\d{2})-(\d{2})(\d{2})')
+
+def find_datetime_string(text):
+    """Search for a simple date and time in arbitrary text.
+
+    The format searched for is %Y-%m-%d-%H%M - the same as produced by
+    `make_datetime_string`.
+    """
+    match = re_find_datetime.search(text)
+    if match is None:
+        return None
+    else:
+        return datetime.datetime(
+            *(int(part) for part in match.groups()))
+
+
+def make_random_string(length=32):
+    """Return a simple random UUID.
+
+    The uuid module is only available in Python 2.5 and above, but a
+    simple non-RFC-compliant hack here is sufficient.
+    """
+    assert length % 2 == 0, "length must be a multiple of 2"
+    return binascii.hexlify(os.urandom(length/2))

=== modified file 'setup.py'
--- setup.py	2009-08-17 19:16:23 +0000
+++ setup.py	2009-10-08 14:53:12 +0000
@@ -41,6 +41,7 @@
         'mechanize',
         'mocker',
         'oauth',
+        'paramiko',
         'python-openid',
         'pytz',
         # This appears to be a broken indirect dependency from zope.security:

=== modified file 'versions.cfg'
--- versions.cfg	2009-10-01 16:38:03 +0000
+++ versions.cfg	2009-10-08 14:53:12 +0000
@@ -35,8 +35,10 @@
 mocker = 0.10.1
 mozrunner = 1.3.4
 oauth = 1.0
+paramiko = 1.7.4
 Paste = 1.7.2
 PasteDeploy = 1.3.3
+pycrypto = 2.0.1
 python-openid = 2.2.1
 pytz = 2009l
 RestrictedPython = 3.4.2