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
=== modified file 'lib/canonical/buildd/tests/harness.py'
--- lib/canonical/buildd/tests/harness.py 2010-02-18 14:36:15 +0000
+++ lib/canonical/buildd/tests/harness.py 2010-02-19 13:29:17 +0000
@@ -1,4 +1,4 @@
1# Copyright 2009 Canonical Ltd. This software is licensed under the1# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4__metaclass__ = type4__metaclass__ = type
@@ -8,7 +8,6 @@
8 ]8 ]
99
10import os10import os
11import shutil
12import tempfile11import tempfile
13import unittest12import unittest
14from ConfigParser import SafeConfigParser13from ConfigParser import SafeConfigParser
@@ -18,6 +17,8 @@
18from canonical.buildd.slave import BuildDSlave17from canonical.buildd.slave import BuildDSlave
19from canonical.launchpad.daemons.tachandler import TacTestSetup18from canonical.launchpad.daemons.tachandler import TacTestSetup
2019
20from lp.services.osutils import remove_tree
21
2122
22test_conffile = os.path.join(23test_conffile = os.path.join(
23 os.path.dirname(__file__), 'buildd-slave-test.conf')24 os.path.dirname(__file__), 'buildd-slave-test.conf')
@@ -48,8 +49,7 @@
4849
49 def tearDown(self):50 def tearDown(self):
50 """Remove the 'filecache' directory used for the tests."""51 """Remove the 'filecache' directory used for the tests."""
51 if os.path.isdir(self.slave._cachepath):52 remove_tree(self.slave._cachepath)
52 shutil.rmtree(self.slave._cachepath)
5353
54 def makeLog(self, size):54 def makeLog(self, size):
55 """Inject data into the default buildlog file."""55 """Inject data into the default buildlog file."""
@@ -99,8 +99,7 @@
99 """99 """
100 def setUpRoot(self):100 def setUpRoot(self):
101 """Recreate empty root directory to avoid problems."""101 """Recreate empty root directory to avoid problems."""
102 if os.path.isdir(self.root):102 remove_tree(self.root)
103 shutil.rmtree(self.root)
104 os.mkdir(self.root)103 os.mkdir(self.root)
105 filecache = os.path.join(self.root, 'filecache')104 filecache = os.path.join(self.root, 'filecache')
106 os.mkdir(filecache)105 os.mkdir(filecache)
@@ -114,8 +113,7 @@
114 def tearDown(self):113 def tearDown(self):
115 """Tear down the system normally and additionaly remove the root."""114 """Tear down the system normally and additionaly remove the root."""
116 TacTestSetup.tearDown(self)115 TacTestSetup.tearDown(self)
117 if os.path.isdir(self.root):116 remove_tree(self.root)
118 shutil.rmtree(self.root)
119117
120 @property118 @property
121 def root(self):119 def root(self):
@@ -134,5 +132,4 @@
134132
135 @property133 @property
136 def logfile(self):134 def logfile(self):
137 return os.path.join(self.root, 'build-slave.log')135 return '/var/tmp/build-slave.log'
138
139136
=== modified file 'lib/canonical/launchpad/daemons/tachandler.py'
--- lib/canonical/launchpad/daemons/tachandler.py 2010-01-11 21:42:25 +0000
+++ lib/canonical/launchpad/daemons/tachandler.py 2010-02-19 13:29:17 +0000
@@ -1,8 +1,9 @@
1# Copyright 2009 Canonical Ltd. This software is licensed under the1# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4"""Test harness for TAC (Twisted Application Configuration) files.4from __future__ import with_statement
5"""5
6"""Test harness for TAC (Twisted Application Configuration) files."""
67
7__metaclass__ = type8__metaclass__ = type
89
@@ -42,6 +43,13 @@
42 # previous runs. Although tearDown() should have been called already,43 # previous runs. Although tearDown() should have been called already,
43 # we can't guarantee it.44 # we can't guarantee it.
44 self.tearDown()45 self.tearDown()
46
47 # setUp() watches the logfile to determine when the daemon has fully
48 # started. If it sees an old logfile, then it will find the LOG_MAGIC
49 # string and return immediately, provoking hard-to-diagnose race
50 # conditions. Delete the logfile to make sure this does not happen.
51 self._removeFile(self.logfile)
52
45 self.setUpRoot()53 self.setUpRoot()
46 args = [sys.executable, twistd_script, '-o', '-y', self.tacfile,54 args = [sys.executable, twistd_script, '-o', '-y', self.tacfile,
47 '--pidfile', self.pidfile, '--logfile', self.logfile]55 '--pidfile', self.pidfile, '--logfile', self.logfile]
@@ -62,27 +70,42 @@
62 if rv != 0:70 if rv != 0:
63 raise TacException('Error %d running %s' % (rv, args))71 raise TacException('Error %d running %s' % (rv, args))
6472
65 # Wait for the daemon to fully start (as determined by watching the73 self._waitForDaemonStartup()
66 # log file). If it takes more than 20 seconds, we assume it's gone74
67 # wrong, and raise TacException.75 def _hasDaemonStarted(self):
68 start = time.time()76 """Has the daemon started?
69 while True:77
70 if time.time() > start + 20:78 Startup is recognized by the appearance of LOG_MAGIC in the log
71 raise TacException(79 file.
72 'Unable to start %s. Check %s.'80 """
73 % (self.tacfile, self.logfile))81 if os.path.exists(self.logfile):
74 if os.path.exists(self.logfile):82 with open(self.logfile, 'r') as logfile:
75 if LOG_MAGIC in open(self.logfile, 'r').read():83 return LOG_MAGIC in logfile.read()
76 break84 else:
85 return False
86
87 def _waitForDaemonStartup(self):
88 """ Wait for the daemon to fully start.
89
90 Times out after 20 seconds. If that happens, the log file will
91 not be cleaned up so the user can post-mortem it.
92
93 :raises TacException: Timeout.
94 """
95 # Watch the log file for LOG_MAGIC to signal that startup has
96 # completed.
97 now = time.time()
98 deadline = now + 20
99 while now < deadline and not self._hasDaemonStarted():
77 time.sleep(0.1)100 time.sleep(0.1)
101 now = time.time()
102
103 if now >= deadline:
104 raise TacException('Unable to start %s. Check %s.' % (
105 self.tacfile, self.logfile))
78106
79 def tearDown(self):107 def tearDown(self):
80 self.killTac()108 self.killTac()
81 # setUp() watches the logfile to determine when the daemon has fully
82 # started. If it sees an old logfile, then it will find the LOG_MAGIC
83 # string and return immediately, provoking hard-to-diagnose race
84 # conditions. Delete the logfile to make sure this does not happen.
85 self._removeFile(self.logfile)
86109
87 def _removeFile(self, filename):110 def _removeFile(self, filename):
88 """Remove the given file if it exists."""111 """Remove the given file if it exists."""
@@ -93,7 +116,7 @@
93 raise116 raise
94117
95 def killTac(self):118 def killTac(self):
96 """Kill the TAC file, if it is running, and clean up any mess"""119 """Kill the TAC file if it is running."""
97 pidfile = self.pidfile120 pidfile = self.pidfile
98 if not os.path.exists(pidfile):121 if not os.path.exists(pidfile):
99 return122 return
@@ -128,7 +151,6 @@
128 except OSError:151 except OSError:
129 # Already terminated152 # Already terminated
130 pass153 pass
131 self._removeFile(self.logfile)
132154
133 def setUpRoot(self):155 def setUpRoot(self):
134 """Override this.156 """Override this.
@@ -139,11 +161,6 @@
139 """161 """
140 raise NotImplementedError162 raise NotImplementedError
141163
142 # XXX cprov 2005-07-08:
143 # We don't really need those information as property,
144 # they can be implmented as simple attributes since they
145 # store static information. Sort it out soon.
146
147 @property164 @property
148 def root(self):165 def root(self):
149 raise NotImplementedError166 raise NotImplementedError
150167
=== modified file 'lib/lp/buildmaster/tests/harness.py'
--- lib/lp/buildmaster/tests/harness.py 2009-06-25 00:46:29 +0000
+++ lib/lp/buildmaster/tests/harness.py 2010-02-19 13:29:17 +0000
@@ -11,11 +11,12 @@
1111
1212
13import os13import os
14import shutil
1514
16import canonical15import canonical
17from canonical.launchpad.daemons.tachandler import TacTestSetup16from canonical.launchpad.daemons.tachandler import TacTestSetup
1817
18from lp.services.osutils import remove_tree
19
1920
20class BuilddManagerTestSetup(TacTestSetup):21class BuilddManagerTestSetup(TacTestSetup):
21 """Setup BuilddManager for use by functional tests."""22 """Setup BuilddManager for use by functional tests."""
@@ -25,8 +26,7 @@
2526
26 Remove the directory and create a new one if it exists.27 Remove the directory and create a new one if it exists.
27 """28 """
28 if os.path.exists(self.root):29 remove_tree(self.root)
29 shutil.rmtree(self.root)
30 os.makedirs(self.root)30 os.makedirs(self.root)
3131
32 @property32 @property