Launchpad itself

Merge lp://staging/~jtv/launchpad/bug-523926 into lp://staging/launchpad

  1. bug-523926
  2. Merge into devel
Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: not available
Merged at revision: not available
Proposed branch: lp://staging/~jtv/launchpad/bug-523926
Merge into: lp://staging/launchpad
Diff against target: 211 lines (+54/-40)
3 files modified
lib/canonical/buildd/tests/harness.py (+7/-10)
lib/canonical/launchpad/daemons/tachandler.py (+44/-27)
lib/lp/buildmaster/tests/harness.py (+3/-3)
To merge this branch: bzr merge lp://staging/~jtv/launchpad/bug-523926
Reviewer Review Type Date Requested Status
Abel Deuring (community) Approve
Canonical Launchpad Engineering code Pending
Review via email: mp+19704@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

= Bug 520651 =

Sometimes when running tests, a daemon fails to start up. In that case you'll get a message referring to the daemon's log file—which would usually help except it does not exist.

This happens because TacHandler's tearDown deletes the logfile. Yes, log files do need to be deleted before the same dæmon is started again later. Magic in the way TacHandler polls for daemon startup makes that necessary. But there's no real need to clean up the logfile at the end of a run. The amount of garbage is limited, with a single fixed filename, and it's in /var/tmp/ where it won't usually bother anyone.

So I fixed this by deleting the log file in setUp, but not in tearDown. In addition to this I did some drive-by cleanups. In the buildd test harness, which stubbornly refused to leave logfiles behind, I moved the logfile out of /var/tmp/buildd/ which was being deleted during both setup and teardown.

I also rewrote that daemon startup polling loop to be, I hope, a bit easier to maintain.

There is one pylint notice:
{{{
lib/canonical/launchpad/daemons/tachandler.py
    6: [W0105] String statement has no effect
}}}

This seems to be because the docstring has to come after the import for the "with" statement. Not quite enough to disable the warning; the extra import will go away as we upgrade python further anyway.

To Q/A, run some tests that start daemons (e.g. librarian and/or buildd) twice in succession. You should see /var/tmp/librarian.log and /var/tmp/build-slave.log after each run that exercises the respective daemon. Run these log files through "wc" after the first run and the second. Their stats should remain about the same as the re-run cleans up the old logs and the tests then write the same logs again. (In my case the "wc" output is _exactly_ identical, with just the timestamps changed AFAICS, but I wouldn't promise it).

Jeroen

Revision history for this message
Abel Deuring (adeuring) wrote :

nice work!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/buildd/tests/harness.py'
2--- lib/canonical/buildd/tests/harness.py 2010-02-18 14:36:15 +0000
3+++ lib/canonical/buildd/tests/harness.py 2010-02-19 13:29:17 +0000
4@@ -1,4 +1,4 @@
5-# Copyright 2009 Canonical Ltd. This software is licensed under the
6+# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
7 # GNU Affero General Public License version 3 (see the file LICENSE).
8
9 __metaclass__ = type
10@@ -8,7 +8,6 @@
11 ]
12
13 import os
14-import shutil
15 import tempfile
16 import unittest
17 from ConfigParser import SafeConfigParser
18@@ -18,6 +17,8 @@
19 from canonical.buildd.slave import BuildDSlave
20 from canonical.launchpad.daemons.tachandler import TacTestSetup
21
22+from lp.services.osutils import remove_tree
23+
24
25 test_conffile = os.path.join(
26 os.path.dirname(__file__), 'buildd-slave-test.conf')
27@@ -48,8 +49,7 @@
28
29 def tearDown(self):
30 """Remove the 'filecache' directory used for the tests."""
31- if os.path.isdir(self.slave._cachepath):
32- shutil.rmtree(self.slave._cachepath)
33+ remove_tree(self.slave._cachepath)
34
35 def makeLog(self, size):
36 """Inject data into the default buildlog file."""
37@@ -99,8 +99,7 @@
38 """
39 def setUpRoot(self):
40 """Recreate empty root directory to avoid problems."""
41- if os.path.isdir(self.root):
42- shutil.rmtree(self.root)
43+ remove_tree(self.root)
44 os.mkdir(self.root)
45 filecache = os.path.join(self.root, 'filecache')
46 os.mkdir(filecache)
47@@ -114,8 +113,7 @@
48 def tearDown(self):
49 """Tear down the system normally and additionaly remove the root."""
50 TacTestSetup.tearDown(self)
51- if os.path.isdir(self.root):
52- shutil.rmtree(self.root)
53+ remove_tree(self.root)
54
55 @property
56 def root(self):
57@@ -134,5 +132,4 @@
58
59 @property
60 def logfile(self):
61- return os.path.join(self.root, 'build-slave.log')
62-
63+ return '/var/tmp/build-slave.log'
64
65=== modified file 'lib/canonical/launchpad/daemons/tachandler.py'
66--- lib/canonical/launchpad/daemons/tachandler.py 2010-01-11 21:42:25 +0000
67+++ lib/canonical/launchpad/daemons/tachandler.py 2010-02-19 13:29:17 +0000
68@@ -1,8 +1,9 @@
69-# Copyright 2009 Canonical Ltd. This software is licensed under the
70+# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
71 # GNU Affero General Public License version 3 (see the file LICENSE).
72
73-"""Test harness for TAC (Twisted Application Configuration) files.
74-"""
75+from __future__ import with_statement
76+
77+"""Test harness for TAC (Twisted Application Configuration) files."""
78
79 __metaclass__ = type
80
81@@ -42,6 +43,13 @@
82 # previous runs. Although tearDown() should have been called already,
83 # we can't guarantee it.
84 self.tearDown()
85+
86+ # setUp() watches the logfile to determine when the daemon has fully
87+ # started. If it sees an old logfile, then it will find the LOG_MAGIC
88+ # string and return immediately, provoking hard-to-diagnose race
89+ # conditions. Delete the logfile to make sure this does not happen.
90+ self._removeFile(self.logfile)
91+
92 self.setUpRoot()
93 args = [sys.executable, twistd_script, '-o', '-y', self.tacfile,
94 '--pidfile', self.pidfile, '--logfile', self.logfile]
95@@ -62,27 +70,42 @@
96 if rv != 0:
97 raise TacException('Error %d running %s' % (rv, args))
98
99- # Wait for the daemon to fully start (as determined by watching the
100- # log file). If it takes more than 20 seconds, we assume it's gone
101- # wrong, and raise TacException.
102- start = time.time()
103- while True:
104- if time.time() > start + 20:
105- raise TacException(
106- 'Unable to start %s. Check %s.'
107- % (self.tacfile, self.logfile))
108- if os.path.exists(self.logfile):
109- if LOG_MAGIC in open(self.logfile, 'r').read():
110- break
111+ self._waitForDaemonStartup()
112+
113+ def _hasDaemonStarted(self):
114+ """Has the daemon started?
115+
116+ Startup is recognized by the appearance of LOG_MAGIC in the log
117+ file.
118+ """
119+ if os.path.exists(self.logfile):
120+ with open(self.logfile, 'r') as logfile:
121+ return LOG_MAGIC in logfile.read()
122+ else:
123+ return False
124+
125+ def _waitForDaemonStartup(self):
126+ """ Wait for the daemon to fully start.
127+
128+ Times out after 20 seconds. If that happens, the log file will
129+ not be cleaned up so the user can post-mortem it.
130+
131+ :raises TacException: Timeout.
132+ """
133+ # Watch the log file for LOG_MAGIC to signal that startup has
134+ # completed.
135+ now = time.time()
136+ deadline = now + 20
137+ while now < deadline and not self._hasDaemonStarted():
138 time.sleep(0.1)
139+ now = time.time()
140+
141+ if now >= deadline:
142+ raise TacException('Unable to start %s. Check %s.' % (
143+ self.tacfile, self.logfile))
144
145 def tearDown(self):
146 self.killTac()
147- # setUp() watches the logfile to determine when the daemon has fully
148- # started. If it sees an old logfile, then it will find the LOG_MAGIC
149- # string and return immediately, provoking hard-to-diagnose race
150- # conditions. Delete the logfile to make sure this does not happen.
151- self._removeFile(self.logfile)
152
153 def _removeFile(self, filename):
154 """Remove the given file if it exists."""
155@@ -93,7 +116,7 @@
156 raise
157
158 def killTac(self):
159- """Kill the TAC file, if it is running, and clean up any mess"""
160+ """Kill the TAC file if it is running."""
161 pidfile = self.pidfile
162 if not os.path.exists(pidfile):
163 return
164@@ -128,7 +151,6 @@
165 except OSError:
166 # Already terminated
167 pass
168- self._removeFile(self.logfile)
169
170 def setUpRoot(self):
171 """Override this.
172@@ -139,11 +161,6 @@
173 """
174 raise NotImplementedError
175
176- # XXX cprov 2005-07-08:
177- # We don't really need those information as property,
178- # they can be implmented as simple attributes since they
179- # store static information. Sort it out soon.
180-
181 @property
182 def root(self):
183 raise NotImplementedError
184
185=== modified file 'lib/lp/buildmaster/tests/harness.py'
186--- lib/lp/buildmaster/tests/harness.py 2009-06-25 00:46:29 +0000
187+++ lib/lp/buildmaster/tests/harness.py 2010-02-19 13:29:17 +0000
188@@ -11,11 +11,12 @@
189
190
191 import os
192-import shutil
193
194 import canonical
195 from canonical.launchpad.daemons.tachandler import TacTestSetup
196
197+from lp.services.osutils import remove_tree
198+
199
200 class BuilddManagerTestSetup(TacTestSetup):
201 """Setup BuilddManager for use by functional tests."""
202@@ -25,8 +26,7 @@
203
204 Remove the directory and create a new one if it exists.
205 """
206- if os.path.exists(self.root):
207- shutil.rmtree(self.root)
208+ remove_tree(self.root)
209 os.makedirs(self.root)
210
211 @property