Merge lp:~free.ekanayaka/landscape-client/release-upgrade-logs into lp:~landscape/landscape-client/trunk

Proposed by Free Ekanayaka on 2010-03-15
Status: Merged
Approved by: Sidnei da Silva on 2010-03-16
Approved revision: 216
Merge reported by: Free Ekanayaka
Merged at revision: not available
Proposed branch: lp:~free.ekanayaka/landscape-client/release-upgrade-logs
Merge into: lp:~landscape/landscape-client/trunk
Diff against target: 352 lines (+203/-10)
5 files modified
debian/changelog (+6/-0)
landscape/lib/fs.py (+22/-0)
landscape/lib/tests/test_fs.py (+36/-0)
landscape/package/releaseupgrader.py (+40/-3)
landscape/package/tests/test_releaseupgrader.py (+99/-7)
To merge this branch: bzr merge lp:~free.ekanayaka/landscape-client/release-upgrade-logs
Reviewer Review Type Date Requested Status
Sidnei da Silva (community) 2010-03-15 Approve on 2010-03-16
Jamu Kakar (community) 2010-03-15 Approve on 2010-03-16
Review via email: mp+21366@code.launchpad.net

Description of the change

This branch includes the full upgrade-tool logs in the operation result text. Maybe we want also to trim the content of each file if it's too big, like reporting only the *last* N lines. Comments are welcome.

To post a comment you must log in.
Sidnei da Silva (sidnei) wrote :

[1] Are we really reading whole files into memory like that? It gives me shivers. We should optimally be generating the message in temporary files and streaming them out on upload if possible, as reading a file of potentially unknown size into memory is a sure way of running into OOM bugs. Failing that, I would go for the 'last N lines' approach right now, instead of waiting until we hit the bug.

review: Needs Fixing
Jamu Kakar (jkakar) wrote :

[1]

+ def make_operation_result_text(self, out, err):

This method should be using a StringIO internally to avoid copying
strings. This will help with memory use.

[2]

As I recall, Andreas mentioned that the log files are fairly small,
less than 60kb. I think it's probably okay to load them into
memory. That said, as Sidnei suggests, it would be good to put a
safety check in the read_file function and only loads the last N
bytes. N can default to a fairly large number, like 100kb.

review: Needs Fixing
216. By Free Ekanayaka on 2010-03-16

Read only the last 100kb of the log file and use StringIO (Sidnei [1], Jamu [1] and [2])

Free Ekanayaka (free.ekanayaka) wrote :

Sidnei, Jamu thanks for the feedback.

The branch now reads only up to the last 100000 bytes of the file, and uses StringIO.

Jamu Kakar (jkakar) wrote :

Nice work, +1!

review: Approve
Sidnei da Silva (sidnei) wrote :

Looks good, but you're defaulting to read 100k from the *beginning* of the file (logs_limit is positive)? I think it would be more useful to read from the end of the file. With that change, +1!

review: Approve
Free Ekanayaka (free.ekanayaka) wrote :

Sidnei, maybe the code is confusing, but it's actually the *end* of the file:

+ content = read_file(filename, - self.logs_limit)

I set logs_limit to a positive value so that it's more user friendly, and the idea is that the fact that internally read_file is used with a negative value should be an implementation detail.

The docstring for logs_limit sounds clear enough to me:

+ @cvar logs_limit: When reporting upgrade-tool logs to the server, only the
+ last C{logs_limit} lines will be sent.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2010-02-09 09:45:00 +0000
3+++ debian/changelog 2010-03-16 10:39:30 +0000
4@@ -1,3 +1,9 @@
5+landscape-client (1.5.0-0ubuntu0.10.04) UNRELEASED; urgency=low
6+
7+ * New upstream version
8+
9+ -- Free Ekanayaka <free.ekanayaka@canonical.com> Mon, 15 Mar 2010 12:58:44 +0100
10+
11 landscape-client (1.4.4-0ubuntu0.10.04.0) lucid; urgency=low
12
13 * New upstream release (LP: #519200):
14
15=== modified file 'landscape/lib/fs.py'
16--- landscape/lib/fs.py 2010-02-05 19:12:42 +0000
17+++ landscape/lib/fs.py 2010-03-16 10:39:30 +0000
18@@ -1,5 +1,7 @@
19 """File-system utils"""
20
21+import os
22+
23
24 def create_file(path, content):
25 """Create a file with the given content.
26@@ -10,3 +12,23 @@
27 fd = open(path, "w")
28 fd.write(content)
29 fd.close()
30+
31+
32+def read_file(path, limit=None):
33+ """Return the content of the given file.
34+
35+ @param path: The path to the file.
36+ @param limit: An optional read limit. If positive, read up to that number
37+ of bytes from the beginning of the file. If negative, read up to that
38+ number of bytes from the end of the file.
39+ @return content: The content of the file, possibly trimmed to C{limit}.
40+ """
41+ fd = open(path, "r")
42+ if limit and os.path.getsize(path) > abs(limit):
43+ whence = 0
44+ if limit < 0:
45+ whence = 2
46+ fd.seek(limit, whence)
47+ content = fd.read()
48+ fd.close()
49+ return content
50
51=== added file 'landscape/lib/tests/test_fs.py'
52--- landscape/lib/tests/test_fs.py 1970-01-01 00:00:00 +0000
53+++ landscape/lib/tests/test_fs.py 2010-03-16 10:39:30 +0000
54@@ -0,0 +1,36 @@
55+from landscape.tests.helpers import LandscapeTest
56+
57+from landscape.lib.fs import read_file
58+
59+
60+class ReadFileTest(LandscapeTest):
61+
62+ def test_read_file(self):
63+ """
64+ With no options L{read_file} reads the whole file passed as argument.
65+ """
66+ path = self.makeFile("foo")
67+ self.assertEquals(read_file(path), "foo")
68+
69+ def test_read_file_with_limit(self):
70+ """
71+ With a positive limit L{read_file} reads only the bytes after the
72+ given limit.
73+ """
74+ path = self.makeFile("foo bar")
75+ self.assertEquals(read_file(path, limit=3), " bar")
76+
77+ def test_read_file_with_negative_limit(self):
78+ """
79+ With a negative limit L{read_file} reads only the tail of the file.
80+ """
81+ path = self.makeFile("foo bar from end")
82+ self.assertEquals(read_file(path, limit=-3), "end")
83+
84+ def test_read_file_with_limit_bigger_than_file(self):
85+ """
86+ If the limit is bigger than the file L{read_file} reads the entire file.
87+ """
88+ path = self.makeFile("foo bar from end")
89+ self.assertEquals(read_file(path, limit=100), "foo bar from end")
90+ self.assertEquals(read_file(path, limit=-100), "foo bar from end")
91
92=== modified file 'landscape/package/releaseupgrader.py'
93--- landscape/package/releaseupgrader.py 2009-12-15 11:03:31 +0000
94+++ landscape/package/releaseupgrader.py 2010-03-16 10:39:30 +0000
95@@ -15,6 +15,7 @@
96 from landscape.lib.fetch import url_to_filename, fetch_to_files
97 from landscape.lib.lsb_release import parse_lsb_release, LSB_RELEASE_FILENAME
98 from landscape.lib.gpg import gpg_verify
99+from landscape.lib.fs import read_file
100 from landscape.package.taskhandler import (
101 PackageTaskHandlerConfiguration, PackageTaskHandler, run_task_handler)
102 from landscape.manager.manager import SUCCEEDED, FAILED
103@@ -33,12 +34,25 @@
104
105
106 class ReleaseUpgrader(PackageTaskHandler):
107- """Perform release upgrades."""
108+ """Perform release upgrades.
109+
110+ @cvar config_factory: The configuration class to use to build configuration
111+ objects to be passed to our constructor.
112+ @cvar queue_name: The queue we pick tasks from.
113+ @cvar lsb_release_filename: The path to the LSB data on the file system.
114+ @cvar landscape_ppa_url: The URL of the Landscape PPA, if it is present
115+ in the computer's sources.list it won't be commented out.
116+ @cvar logs_directory: Path to the directory holding the upgrade-tool logs.
117+ @cvar logs_limit: When reporting upgrade-tool logs to the server, only the
118+ last C{logs_limit} lines will be sent.
119+ """
120
121 config_factory = ReleaseUpgraderConfiguration
122 queue_name = "release-upgrader"
123 lsb_release_filename = LSB_RELEASE_FILENAME
124 landscape_ppa_url = "http://ppa.launchpad.net/landscape/ppa/ubuntu/"
125+ logs_directory = "/var/log/dist-upgrade"
126+ logs_limit = 100000
127
128 def make_operation_result_message(self, operation_id, status, text, code):
129 """Convenience to create messages of type C{"operation-result"}."""
130@@ -210,6 +224,28 @@
131
132 return succeed(None)
133
134+ def make_operation_result_text(self, out, err):
135+ """Return the operation result text to be sent to the server.
136+
137+ @param out: The standard output of the upgrade-tool process.
138+ @param err: The standard error of the upgrade-tool process.
139+ @return: A text aggregating the process output, error and log files.
140+ """
141+ buf = cStringIO.StringIO()
142+
143+ for label, content in [("output", out), ("error", err)]:
144+ if content:
145+ buf.write("=== Standard %s ===\n\n%s\n\n" % (label, content))
146+
147+ for basename in os.listdir(self.logs_directory):
148+ if not basename.endswith(".log"):
149+ continue
150+ filename = os.path.join(self.logs_directory, basename)
151+ content = read_file(filename, - self.logs_limit)
152+ buf.write("=== %s ===\n\n%s\n\n" % (basename, content))
153+
154+ return buf.getvalue()
155+
156 def upgrade(self, code_name, operation_id, allow_third_party=False,
157 debug=False, mode=None):
158 """Run the upgrade-tool command and send a report of the results.
159@@ -267,8 +303,9 @@
160 status = SUCCEEDED
161 else:
162 status = FAILED
163- message = self.make_operation_result_message(
164- operation_id, status, "%s%s" % (out, err), code)
165+ text = self.make_operation_result_text(out, err)
166+ message = self.make_operation_result_message(operation_id, status,
167+ text, code)
168 logging.info("Queuing message with release upgrade results to "
169 "exchange urgently.")
170 return self._broker.send_message(message, True)
171
172=== modified file 'landscape/package/tests/test_releaseupgrader.py'
173--- landscape/package/tests/test_releaseupgrader.py 2010-01-21 08:47:03 +0000
174+++ landscape/package/tests/test_releaseupgrader.py 2010-03-16 10:39:30 +0000
175@@ -270,11 +270,93 @@
176 result.addCallback(check_result)
177 return result
178
179+ def test_default_logs_directory(self):
180+ """
181+ The default directory for the upgrade-tool logs is the system one.
182+ """
183+ self.assertEquals(self.upgrader.logs_directory,
184+ "/var/log/dist-upgrade")
185+
186+ def test_default_logs_limit(self):
187+ """
188+ The default read limit for the upgrade-tool logs is 100000 bytes.
189+ """
190+ self.assertEquals(self.upgrader.logs_limit, 100000)
191+
192+ def test_make_operation_result_text(self):
193+ """
194+ L{ReleaseUpgrade.make_operation_result_text} aggregates the contents of
195+ the process standard output, error and log files.
196+ """
197+ self.upgrader.logs_directory = self.makeDir()
198+ self.makeFile(basename="main.log",
199+ dirname=self.upgrader.logs_directory,
200+ content="main log")
201+ self.makeFile(basename="apt.log",
202+ dirname=self.upgrader.logs_directory,
203+ content="apt log")
204+ text = self.upgrader.make_operation_result_text("stdout", "stderr")
205+ self.assertEquals(text,
206+ "=== Standard output ===\n\n"
207+ "stdout\n\n"
208+ "=== Standard error ===\n\n"
209+ "stderr\n\n"
210+ "=== main.log ===\n\n"
211+ "main log\n\n"
212+ "=== apt.log ===\n\n"
213+ "apt log\n\n")
214+
215+ def test_make_operation_result_text_with_no_stderr(self):
216+ """
217+ L{ReleaseUpgrade.make_operation_result_text} skips the standard error
218+ if it's empty.
219+ """
220+ self.upgrader.logs_directory = self.makeDir()
221+ text = self.upgrader.make_operation_result_text("stdout", "")
222+ self.assertEquals(text,
223+ "=== Standard output ===\n\n"
224+ "stdout\n\n")
225+
226+ def test_make_operation_result_text_only_considers_log_files(self):
227+ """
228+ L{ReleaseUpgrade.make_operation_result_text} only considers log files
229+ from the last upgrade-tool run, directories containing log files from
230+ an older run are skipped.
231+ """
232+ self.upgrader.logs_directory = self.makeDir()
233+ self.makeDir(dirname=self.upgrader.logs_directory)
234+ text = self.upgrader.make_operation_result_text("stdout", "stderr")
235+ self.assertEquals(text,
236+ "=== Standard output ===\n\n"
237+ "stdout\n\n"
238+ "=== Standard error ===\n\n"
239+ "stderr\n\n")
240+
241+ def test_make_operation_result_text_trims_long_files(self):
242+ """
243+ L{ReleaseUpgrade.make_operation_result_text} only reads the last
244+ L{logs_limit} lines of a log file.
245+ """
246+ self.upgrader.logs_directory = self.makeDir()
247+ self.upgrader.logs_limit = 8
248+ self.makeFile(basename="main.log",
249+ dirname=self.upgrader.logs_directory,
250+ content="very long log")
251+ text = self.upgrader.make_operation_result_text("stdout", "stderr")
252+ self.assertEquals(text,
253+ "=== Standard output ===\n\n"
254+ "stdout\n\n"
255+ "=== Standard error ===\n\n"
256+ "stderr\n\n"
257+ "=== main.log ===\n\n"
258+ "long log\n\n")
259+
260 def test_upgrade(self):
261 """
262 The L{ReleaseUpgrader.upgrade} method spawns the appropropriate
263 upgrade-tool script and reports the result.
264 """
265+ self.upgrader.logs_directory = self.makeDir()
266 upgrade_tool_directory = self.config.upgrade_tool_directory
267 upgrade_tool_filename = os.path.join(upgrade_tool_directory, "karmic")
268 fd = open(upgrade_tool_filename, "w")
269@@ -298,8 +380,10 @@
270 self.assertIn("INFO: Queuing message with release upgrade "
271 "results to exchange urgently.",
272 self.logfile.getvalue())
273- result_text = u"--frontend DistUpgradeViewNonInteractive\n" \
274- "FOO=bar\nPWD=%s\nout\n" % upgrade_tool_directory
275+ result_text = (u"=== Standard output ===\n\n"
276+ "--frontend DistUpgradeViewNonInteractive\n"
277+ "FOO=bar\n"
278+ "PWD=%s\nout\n\n\n" % upgrade_tool_directory)
279 self.assertMessages(self.get_pending_messages(),
280 [{"type": "operation-result",
281 "operation-id": 100,
282@@ -324,6 +408,7 @@
283 which gets passed to the upgrade-tool script as argument for the
284 C{--mode} command line option.
285 """
286+ self.upgrader.logs_directory = self.makeDir()
287 upgrade_tool_directory = self.config.upgrade_tool_directory
288 upgrade_tool_filename = os.path.join(upgrade_tool_directory, "hardy")
289 self.makeFile(path=upgrade_tool_filename,
290@@ -337,8 +422,9 @@
291 result = self.upgrader.upgrade("hardy", 100, mode="server")
292
293 def check_result(ignored):
294- result_text = ("--frontend DistUpgradeViewNonInteractive "
295- "--mode server\n")
296+ result_text = (u"=== Standard output ===\n\n"
297+ "--frontend DistUpgradeViewNonInteractive "
298+ "--mode server\n\n\n")
299 self.assertMessages(self.get_pending_messages(),
300 [{"type": "operation-result",
301 "operation-id": 100,
302@@ -357,6 +443,7 @@
303 The L{ReleaseUpgrader.upgrade} method optionally sets environment
304 variables to be passed to the upgrade-tool process.
305 """
306+ self.upgrader.logs_directory = self.makeDir()
307 upgrade_tool_directory = self.config.upgrade_tool_directory
308 upgrade_tool_filename = os.path.join(upgrade_tool_directory, "karmic")
309 fd = open(upgrade_tool_filename, "w")
310@@ -377,8 +464,9 @@
311 debug=True)
312
313 def check_result(ignored):
314- result_text = u"DEBUG_UPDATE_MANAGER=True\n" \
315- u"RELEASE_UPRADER_ALLOW_THIRD_PARTY=True\n"
316+ result_text = (u"=== Standard output ===\n\n"
317+ "DEBUG_UPDATE_MANAGER=True\n"
318+ "RELEASE_UPRADER_ALLOW_THIRD_PARTY=True\n\n\n")
319 self.assertMessages(self.get_pending_messages(),
320 [{"type": "operation-result",
321 "operation-id": 100,
322@@ -402,6 +490,7 @@
323 The L{ReleaseUpgrader.upgrade} sends a message with failed status
324 field if the upgrade-tool exits with non-zero code.
325 """
326+ self.upgrader.logs_directory = self.makeDir()
327 upgrade_tool_directory = self.config.upgrade_tool_directory
328 upgrade_tool_filename = os.path.join(upgrade_tool_directory, "karmic")
329 fd = open(upgrade_tool_filename, "w")
330@@ -419,11 +508,13 @@
331 result = self.upgrader.upgrade("karmic", 100)
332
333 def check_result(ignored):
334+ result_text = (u"=== Standard output ===\n\nout\n\n\n"
335+ "=== Standard error ===\n\nerr\n\n\n")
336 self.assertMessages(self.get_pending_messages(),
337 [{"type": "operation-result",
338 "operation-id": 100,
339 "status": FAILED,
340- "result-text": "out\nerr\n",
341+ "result-text": result_text,
342 "result-code": 3}])
343
344 result.addCallback(check_result)
345@@ -439,6 +530,7 @@
346 and passes its files descriptors over to child processes we don't know
347 about.
348 """
349+ self.upgrader.logs_directory = self.makeDir()
350 upgrade_tool_directory = self.config.upgrade_tool_directory
351 upgrade_tool_filename = os.path.join(upgrade_tool_directory, "karmic")
352 child_pid_filename = self.makeFile()

Subscribers

People subscribed via source and target branches

to all changes: