Launchpad itself

Merge lp:~jtv/launchpad/bug-488695 into lp:launchpad

  1. bug-488695
  2. Merge into devel
Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Michael Hudson-Doyle
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~jtv/launchpad/bug-488695
Merge into: lp:launchpad
Diff against target: 233 lines (+195/-2)
2 files modified
lib/devscripts/ec2test/instance.py (+10/-2)
lib/devscripts/ec2test/tests/test_ec2instance.py (+185/-0)
To merge this branch: bzr merge lp:~jtv/launchpad/bug-488695
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Graham Binns code Pending
Review via email: mp+15279@code.launchpad.net

Commit message

Fix premature shutdowns in headless ec2 images.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

= Bug 488695 =

See bug for details. Headless EC2 images were shutting down prematurely.

The method that needed fixing did, in a shell:

def set_up_and_run(self, shutdown):
    # *Always* shut down unless & until we finish our work.
    really_shutdown = True
    try:
        try:
            return real_work()
        except Exception:
            handle_error()
        else:
            # No error; now we obey the caller's wishes.
            really_shutdown = shutdown
    finally:
        if really_shutdown:
            shutdown()

Fair enough, but as it turns out, the "return real_work()" in the inner "try" bypasses the "else" attached to the "try." Thus really_shutdown never becomes False.

Why does this matter for headless runs? Because in those cases, real_work() sets up the instance and then returns while the instance starts work on the actual tests. Shutting down at that point will kill the EC2 instance before it gets a fair chance to run them!

There didn't seem to be any tests for this, so with lots of stubbing and mocking, I added one. In fact this produced a nice little reusable class to mock up, disable, or inject all sorts of simple methods. We may want to make it a standard helper, except it currently has a nice short name that happens to match the IRC nick of one of our teammates.

So I added a test. That doesn't mean I'm a saint. In this review you'll see some of my darker moments:

 * I mocked whatever I had to, God knows how deep into the call tree. Obviously that makes for more brittle tests. Changes in the EC2 setup may require lots of changes to the mock objects in the future.

 * I suppressed a pylint warning. Apparently pylint isn't quite smart enough that "if self.simulated_failure is not None: raise self.simulated_failure" never raises None.

 * I moved a global import into a method, and it wasn't to avoid a circular import (I think). For whatever reason, I couldn't seem to import bzrlib.plugins.launchpad.account from instance.py when running the test. Since I was in a hurry, I moved it into the one method that used it—and which my test doesn't exercise.

To test:
{{{
./bin/test -vv -t test_ec2instance
}}}

As for Q/A... any "ec2 test --headless" or "ec2 land --headless" should do. Just make sure the instance keeps running independently after detaching, and that it shuts down after completion.

There's one piece of lint:

lib/devscripts/ec2test/instance.py
    422: [W0703, EC2Instance.set_up_and_run] Catch "Exception"

I didn't touch that. In this case catching Exception may actually make sense, but I'm not silencing the warning because python 2.5 and later change the behaviour for this situation and I don't think it'd hurt to remind more experienced people to think about it.

Jeroen

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

> The method that needed fixing did, in a shell:

Nutshell. In a nutshell.

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Looks fine, thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/devscripts/ec2test/instance.py'
--- lib/devscripts/ec2test/instance.py 2009-11-26 19:49:27 +0000
+++ lib/devscripts/ec2test/instance.py 2009-11-26 21:38:14 +0000
@@ -19,7 +19,6 @@
19import traceback19import traceback
2020
21from bzrlib.errors import BzrCommandError21from bzrlib.errors import BzrCommandError
22from bzrlib.plugins.launchpad.account import get_lp_login
2322
24import paramiko23import paramiko
2524
@@ -194,6 +193,11 @@
194 to allow access to the instance.193 to allow access to the instance.
195 :param credentials: An `EC2Credentials` object.194 :param credentials: An `EC2Credentials` object.
196 """195 """
196 # XXX JeroenVermeulen 2009-11-26: This import fails when
197 # testing without a real EC2 instance. Do it here so the test
198 # can still import this class.
199 from bzrlib.plugins.launchpad.account import get_lp_login
200
197 assert isinstance(name, EC2SessionName)201 assert isinstance(name, EC2SessionName)
198 if instance_type not in AVAILABLE_INSTANCE_TYPES:202 if instance_type not in AVAILABLE_INSTANCE_TYPES:
199 raise ValueError('unknown instance_type %s' % (instance_type,))203 raise ValueError('unknown instance_type %s' % (instance_type,))
@@ -388,6 +392,10 @@
388 self._ensure_ec2test_user_has_keys()392 self._ensure_ec2test_user_has_keys()
389 return self._connect('ec2test')393 return self._connect('ec2test')
390394
395 def _report_traceback(self):
396 """Print traceback."""
397 traceback.print_exc()
398
391 def set_up_and_run(self, postmortem, shutdown, func, *args, **kw):399 def set_up_and_run(self, postmortem, shutdown, func, *args, **kw):
392 """Start, run `func` and then maybe shut down.400 """Start, run `func` and then maybe shut down.
393401
@@ -415,7 +423,7 @@
415 # if there are any exceptions before it waits in the console423 # if there are any exceptions before it waits in the console
416 # (in the finally block), and you can't figure out why it's424 # (in the finally block), and you can't figure out why it's
417 # broken.425 # broken.
418 traceback.print_exc()426 self._report_traceback()
419 else:427 else:
420 really_shutdown = shutdown428 really_shutdown = shutdown
421 finally:429 finally:
422430
=== added file 'lib/devscripts/ec2test/tests/test_ec2instance.py'
--- lib/devscripts/ec2test/tests/test_ec2instance.py 1970-01-01 00:00:00 +0000
+++ lib/devscripts/ec2test/tests/test_ec2instance.py 2009-11-26 21:38:14 +0000
@@ -0,0 +1,185 @@
1# Copyright 2009 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3# pylint: disable-msg=E0702
4
5"""Test handling of EC2 machine images."""
6
7__metaclass__ = type
8
9from unittest import TestCase, TestLoader
10
11from devscripts.ec2test.instance import EC2Instance
12
13
14class Stub:
15 """Generic recipient of invocations.
16
17 Use this to:
18 - Stub methods that should do nothing.
19 - Inject failures into methods.
20 - Record whether a method is being called.
21 """
22 # XXX JeroenVermeulen 2009-11-26: This probably exists somewhere
23 # already. Or if not, maybe it should. But with a name that won't
24 # turn Stuart Bishop's IRC client into a disco simulator.
25
26 call_count = 0
27
28 def __init__(self, return_value=None, simulated_failure=None):
29 """Define what to do when this stub gets invoked.
30
31 :param return_value: Something to return from the invocation.
32 :parma simulated_failure: Something to raise in the invocation.
33 """
34 assert return_value is None or simulated_failure is None, (
35 "Stub can raise an exception or return a value, but not both.")
36 self.return_value = return_value
37 self.simulated_failure = simulated_failure
38
39 def __call__(self, *args, **kwargs):
40 """Catch a call.
41
42 Records the fact that an invocation was made in
43 `call_count`.
44
45 If you passed a `simulated_failure` to the constructor, that
46 object is raised.
47
48 :return: The `return_value` you passed to the constructor.
49 """
50 self.call_count += 1
51
52 if self.simulated_failure is not None:
53 raise self.simulated_failure
54
55 return self.return_value
56
57
58class FakeAccount:
59 """Helper for setting up an `EC2Instance` without EC2."""
60 acquire_private_key = Stub()
61 acquire_security_group = Stub()
62
63
64class FakeOutput:
65 """Pretend stdout/stderr output from EC2 instance."""
66 output = "Fake output."
67
68
69class FakeBotoInstance:
70 """Helper for setting up an `EC2Instance` without EC2."""
71 id = 0
72 state = 'running'
73 public_dns_name = 'fake-instance'
74
75 update = Stub()
76 stop = Stub()
77 get_console_output = FakeOutput
78
79
80class FakeReservation:
81 """Helper for setting up an `EC2Instance` without EC2."""
82 def __init__(self):
83 self.instances = [FakeBotoInstance()]
84
85
86class FakeImage:
87 """Helper for setting up an `EC2Instance` without EC2."""
88 run = Stub(return_value=FakeReservation())
89
90
91class FakeFailure(Exception):
92 """A pretend failure from the test runner."""
93
94
95class TestEC2Instance(TestCase):
96 """Test running of an `EC2Instance` without EC2."""
97
98 def _makeInstance(self):
99 """Set up an `EC2Instance`, with stubbing where needed.
100
101 `EC2Instance.shutdown` is replaced with a `Stub`, so check its
102 call_count to see whether it's been invoked.
103 """
104 session_name = None
105 image = FakeImage()
106 instance_type = 'c1.xlarge'
107 demo_networks = None
108 account = FakeAccount()
109 from_scratch = None
110 user_key = None
111 login = None
112
113 instance = EC2Instance(
114 session_name, image, instance_type, demo_networks, account,
115 from_scratch, user_key, login)
116
117 instance.shutdown = Stub()
118 instance._report_traceback = Stub()
119 instance.log = Stub()
120
121 return instance
122
123 def _runInstance(self, instance, runnee=None, headless=False):
124 """Set up and run an `EC2Instance` (but without EC2)."""
125 if runnee is None:
126 runnee = Stub()
127
128 instance.set_up_and_run(False, not headless, runnee)
129
130 def test_EC2Instance_test_baseline(self):
131 # The EC2 instances we set up have neither started nor been shut
132 # down. After running, they have started.
133 # Not a very useful test, except it establishes the basic
134 # assumptions for the other tests.
135 instance = self._makeInstance()
136 runnee = Stub()
137
138 self.assertEqual(0, runnee.call_count)
139 self.assertEqual(0, instance.shutdown.call_count)
140
141 self._runInstance(instance, runnee=runnee)
142
143 self.assertEqual(1, runnee.call_count)
144
145 def test_set_up_and_run_headful(self):
146 # A non-headless run executes all tests in the instance, then
147 # shuts down.
148 instance = self._makeInstance()
149
150 self._runInstance(instance, headless=False)
151
152 self.assertEqual(1, instance.shutdown.call_count)
153
154 def test_set_up_and_run_headless(self):
155 # An asynchronous, headless run kicks off the tests on the
156 # instance but does not shut it down.
157 instance = self._makeInstance()
158
159 self._runInstance(instance, headless=True)
160
161 self.assertEqual(0, instance.shutdown.call_count)
162
163 def test_set_up_and_run_headful_failure(self):
164 # If the test runner barfs, the instance swallows the exception
165 # and shuts down.
166 instance = self._makeInstance()
167 runnee = Stub(simulated_failure=FakeFailure("Headful barfage."))
168
169 self._runInstance(instance, runnee=runnee, headless=False)
170
171 self.assertEqual(1, instance.shutdown.call_count)
172
173 def test_set_up_and_run_headless_failure(self):
174 # If the instance's test runner fails to set up for a headless
175 # run, the instance swallows the exception and shuts down.
176 instance = self._makeInstance()
177 runnee = Stub(simulated_failure=FakeFailure("Headless boom."))
178
179 self._runInstance(instance, runnee=runnee, headless=True)
180
181 self.assertEqual(1, instance.shutdown.call_count)
182
183
184def test_suite():
185 return TestLoader().loadTestsFromName(__name__)