Launchpad itself

Merge lp:~lifeless/launchpad/run into lp:launchpad

  1. run
  2. Merge into devel
Proposed by Robert Collins
Status: Merged
Approved by: Michael Hudson-Doyle
Approved revision: no longer in the source branch.
Merged at revision: 11696
Proposed branch: lp:~lifeless/launchpad/run
Merge into: lp:launchpad
Diff against target: 282 lines (+50/-75)
3 files modified
Makefile (+0/-1)
lib/canonical/launchpad/scripts/runlaunchpad.py (+45/-62)
lib/canonical/launchpad/scripts/tests/test_runlaunchpad.py (+5/-12)
To merge this branch: bzr merge lp:~lifeless/launchpad/run
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Review via email: mp+38091@code.launchpad.net

Commit message

Improvements and prep work for consolidating Service with TacTestSetup.

Description of the change

Improvements and prep work for consolidating Service with TacTestSetup

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

Note that the changes to ignore should_launch are because some services deliberately ignore it - they don't run by default but when explicitly chosen they need to run; their 'should_launch' is defined as 'return False' : we already filter so that only wanted services are attempted, so checking for should_launch on setUp is a harmful (breaks consistency, causes more code) belts-and-braces.

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

166 + """kill process and BLOCK until process dies.

Should have an initial capital.

I also wonder if stop_process should get more forceful than TERM, but well, that's something for another branch.

The changes look nice, btw.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'Makefile'
2--- Makefile 2010-09-07 18:15:01 +0000
3+++ Makefile 2010-10-11 04:11:44 +0000
4@@ -255,7 +255,6 @@
5 $(RM) thread*.request
6 bin/run -r librarian,sftp,codebrowse -i $(LPCONFIG)
7
8-
9 start_librarian: compile
10 bin/start_librarian
11
12
13=== modified file 'lib/canonical/launchpad/scripts/runlaunchpad.py'
14--- lib/canonical/launchpad/scripts/runlaunchpad.py 2010-09-17 20:46:58 +0000
15+++ lib/canonical/launchpad/scripts/runlaunchpad.py 2010-10-11 04:11:44 +0000
16@@ -7,16 +7,18 @@
17 __all__ = ['start_launchpad']
18
19
20-import atexit
21+from contextlib import nested
22 import os
23 import signal
24 import subprocess
25 import sys
26
27+import fixtures
28 from zope.app.server.main import main
29
30 from canonical.config import config
31 from lp.services.mailman import runmailman
32+from canonical.launchpad.daemons import tachandler
33 from canonical.launchpad.testing import googletestservice
34 from canonical.lazr.pidfile import (
35 make_pidfile,
36@@ -28,20 +30,25 @@
37 return os.path.abspath(os.path.join(config.root, *path.split('/')))
38
39
40-TWISTD_SCRIPT = make_abspath('bin/twistd')
41-
42-
43-class Service(object):
44+class Service(fixtures.Fixture):
45
46 @property
47 def should_launch(self):
48- """Return true if this service should be launched."""
49+ """Return true if this service should be launched by default."""
50 return False
51
52 def launch(self):
53- """Launch the service, but do not block."""
54+ """Run the service in a thread or external process.
55+
56+ May block long enough to kick it off, but must return control to
57+ the caller without waiting for it to shutdown.
58+ """
59 raise NotImplementedError
60
61+ def setUp(self):
62+ super(Service, self).setUp()
63+ self.launch()
64+
65
66 class TacFile(Service):
67
68@@ -56,8 +63,7 @@
69 :param pre_launch: A callable that is called before the launch
70 process.
71 """
72- # No point calling super's __init__.
73- # pylint: disable-msg=W0231
74+ super(TacFile, self).__init__()
75 self.name = name
76 self.tac_filename = tac_filename
77 self.section_name = section_name
78@@ -80,10 +86,6 @@
79 return config[self.section_name].logfile
80
81 def launch(self):
82- # Don't run the server if it wasn't asked for.
83- if not self.should_launch:
84- return
85-
86 self.pre_launch()
87
88 pidfile = pidfile_path(self.name)
89@@ -91,7 +93,7 @@
90 tacfile = make_abspath(self.tac_filename)
91
92 args = [
93- TWISTD_SCRIPT,
94+ tachandler.twistd_script,
95 "--no_save",
96 "--nodaemon",
97 "--python", tacfile,
98@@ -109,15 +111,8 @@
99 # ability to cycle the log files by sending a signal to the twisted
100 # process.
101 process = subprocess.Popen(args, stdin=subprocess.PIPE)
102+ self.addCleanup(stop_process, process)
103 process.stdin.close()
104- # I've left this off - we still check at termination and we can
105- # avoid the startup delay. -- StuartBishop 20050525
106- #time.sleep(1)
107- #if process.poll() != None:
108- # raise RuntimeError(
109- # "%s did not start: %d"
110- # % (self.name, process.returncode))
111- stop_at_exit(process)
112
113
114 class MailmanService(Service):
115@@ -127,11 +122,8 @@
116 return config.mailman.launch
117
118 def launch(self):
119- # Don't run the server if it wasn't asked for. Also, don't attempt to
120- # shut it down at exit.
121- if self.should_launch:
122- runmailman.start_mailman()
123- atexit.register(runmailman.stop_mailman)
124+ runmailman.start_mailman()
125+ self.addCleanup(runmailman.stop_mailman)
126
127
128 class CodebrowseService(Service):
129@@ -144,8 +136,8 @@
130 process = subprocess.Popen(
131 ['make', 'run_codebrowse'],
132 stdin=subprocess.PIPE)
133+ self.addCleanup(stop_process, process)
134 process.stdin.close()
135- stop_at_exit(process)
136
137
138 class GoogleWebService(Service):
139@@ -155,9 +147,7 @@
140 return config.google_test_service.launch
141
142 def launch(self):
143- if self.should_launch:
144- process = googletestservice.start_as_process()
145- stop_at_exit(process)
146+ self.addCleanup(stop_process, googletestservice.start_as_process())
147
148
149 class MemcachedService(Service):
150@@ -180,23 +170,18 @@
151 else:
152 cmd.append('-v')
153 process = subprocess.Popen(cmd, stdin=subprocess.PIPE)
154+ self.addCleanup(stop_process, process)
155 process.stdin.close()
156- stop_at_exit(process)
157-
158-
159-def stop_at_exit(process):
160- """Create and register an atexit hook for killing a process.
161-
162- The hook will BLOCK until the process dies.
163+
164+
165+def stop_process(process):
166+ """kill process and BLOCK until process dies.
167
168 :param process: An instance of subprocess.Popen.
169 """
170-
171- def stop_process():
172- if process.poll() is None:
173- os.kill(process.pid, signal.SIGTERM)
174- process.wait()
175- atexit.register(stop_process)
176+ if process.poll() is None:
177+ os.kill(process.pid, signal.SIGTERM)
178+ process.wait()
179
180
181 def prepare_for_librarian():
182@@ -273,26 +258,24 @@
183 services, argv = split_out_runlaunchpad_arguments(argv[1:])
184 argv = process_config_arguments(argv)
185 services = get_services_to_run(services)
186- for service in services:
187- service.launch()
188-
189- # Store our process id somewhere
190- make_pidfile('launchpad')
191-
192 # Create the ZCML override file based on the instance.
193 config.generate_overrides()
194
195- if config.launchpad.launch:
196- main(argv)
197- else:
198- # We just need the foreground process to sit around forever waiting
199- # for the signal to shut everything down. Normally, Zope itself would
200- # be this master process, but we're not starting that up, so we need
201- # to do something else.
202- try:
203- signal.pause()
204- except KeyboardInterrupt:
205- pass
206+ with nested(*services):
207+ # Store our process id somewhere
208+ make_pidfile('launchpad')
209+
210+ if config.launchpad.launch:
211+ main(argv)
212+ else:
213+ # We just need the foreground process to sit around forever waiting
214+ # for the signal to shut everything down. Normally, Zope itself would
215+ # be this master process, but we're not starting that up, so we need
216+ # to do something else.
217+ try:
218+ signal.pause()
219+ except KeyboardInterrupt:
220+ pass
221
222
223 def start_librarian():
224@@ -303,7 +286,7 @@
225 prepare_for_librarian()
226 pidfile = pidfile_path('librarian')
227 cmd = [
228- TWISTD_SCRIPT,
229+ tachandler.twistd_script,
230 "--python", 'daemons/librarian.tac',
231 "--pidfile", pidfile,
232 "--prefix", 'Librarian',
233
234=== modified file 'lib/canonical/launchpad/scripts/tests/test_runlaunchpad.py'
235--- lib/canonical/launchpad/scripts/tests/test_runlaunchpad.py 2010-08-20 20:31:18 +0000
236+++ lib/canonical/launchpad/scripts/tests/test_runlaunchpad.py 2010-10-11 04:11:44 +0000
237@@ -13,7 +13,8 @@
238 import os
239 import shutil
240 import tempfile
241-import unittest
242+
243+import testtools
244
245 import canonical.config
246 from canonical.config import config
247@@ -119,12 +120,12 @@
248 self.assertEquals('test', config.instance_name)
249
250
251-class ServersToStart(unittest.TestCase):
252+class ServersToStart(testtools.TestCase):
253 """Test server startup control."""
254
255 def setUp(self):
256 """Make sure that only the Librarian is configured to launch."""
257- unittest.TestCase.setUp(self)
258+ testtools.TestCase.setUp(self)
259 launch_data = """
260 [librarian_server]
261 launch: True
262@@ -134,11 +135,7 @@
263 launch: False
264 """
265 config.push('launch_data', launch_data)
266-
267- def tearDown(self):
268- """Restore the default configuration."""
269- config.pop('launch_data')
270- unittest.TestCase.tearDown(self)
271+ self.addCleanup(config.pop, 'launch_data')
272
273 def test_nothing_explictly_requested(self):
274 """Implicitly start services based on the config.*.launch property.
275@@ -167,7 +164,3 @@
276
277 def test_launchpad_systems_red(self):
278 self.failIf(config.launchpad.launch)
279-
280-
281-def test_suite():
282- return unittest.TestLoader().loadTestsFromName(__name__)