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