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
=== modified file 'debian/changelog'
--- debian/changelog 2010-02-09 09:45:00 +0000
+++ debian/changelog 2010-03-16 10:39:30 +0000
@@ -1,3 +1,9 @@
1landscape-client (1.5.0-0ubuntu0.10.04) UNRELEASED; urgency=low
2
3 * New upstream version
4
5 -- Free Ekanayaka <free.ekanayaka@canonical.com> Mon, 15 Mar 2010 12:58:44 +0100
6
1landscape-client (1.4.4-0ubuntu0.10.04.0) lucid; urgency=low7landscape-client (1.4.4-0ubuntu0.10.04.0) lucid; urgency=low
28
3 * New upstream release (LP: #519200):9 * New upstream release (LP: #519200):
410
=== modified file 'landscape/lib/fs.py'
--- landscape/lib/fs.py 2010-02-05 19:12:42 +0000
+++ landscape/lib/fs.py 2010-03-16 10:39:30 +0000
@@ -1,5 +1,7 @@
1"""File-system utils"""1"""File-system utils"""
22
3import os
4
35
4def create_file(path, content):6def create_file(path, content):
5 """Create a file with the given content.7 """Create a file with the given content.
@@ -10,3 +12,23 @@
10 fd = open(path, "w")12 fd = open(path, "w")
11 fd.write(content)13 fd.write(content)
12 fd.close()14 fd.close()
15
16
17def read_file(path, limit=None):
18 """Return the content of the given file.
19
20 @param path: The path to the file.
21 @param limit: An optional read limit. If positive, read up to that number
22 of bytes from the beginning of the file. If negative, read up to that
23 number of bytes from the end of the file.
24 @return content: The content of the file, possibly trimmed to C{limit}.
25 """
26 fd = open(path, "r")
27 if limit and os.path.getsize(path) > abs(limit):
28 whence = 0
29 if limit < 0:
30 whence = 2
31 fd.seek(limit, whence)
32 content = fd.read()
33 fd.close()
34 return content
1335
=== added file 'landscape/lib/tests/test_fs.py'
--- landscape/lib/tests/test_fs.py 1970-01-01 00:00:00 +0000
+++ landscape/lib/tests/test_fs.py 2010-03-16 10:39:30 +0000
@@ -0,0 +1,36 @@
1from landscape.tests.helpers import LandscapeTest
2
3from landscape.lib.fs import read_file
4
5
6class ReadFileTest(LandscapeTest):
7
8 def test_read_file(self):
9 """
10 With no options L{read_file} reads the whole file passed as argument.
11 """
12 path = self.makeFile("foo")
13 self.assertEquals(read_file(path), "foo")
14
15 def test_read_file_with_limit(self):
16 """
17 With a positive limit L{read_file} reads only the bytes after the
18 given limit.
19 """
20 path = self.makeFile("foo bar")
21 self.assertEquals(read_file(path, limit=3), " bar")
22
23 def test_read_file_with_negative_limit(self):
24 """
25 With a negative limit L{read_file} reads only the tail of the file.
26 """
27 path = self.makeFile("foo bar from end")
28 self.assertEquals(read_file(path, limit=-3), "end")
29
30 def test_read_file_with_limit_bigger_than_file(self):
31 """
32 If the limit is bigger than the file L{read_file} reads the entire file.
33 """
34 path = self.makeFile("foo bar from end")
35 self.assertEquals(read_file(path, limit=100), "foo bar from end")
36 self.assertEquals(read_file(path, limit=-100), "foo bar from end")
037
=== modified file 'landscape/package/releaseupgrader.py'
--- landscape/package/releaseupgrader.py 2009-12-15 11:03:31 +0000
+++ landscape/package/releaseupgrader.py 2010-03-16 10:39:30 +0000
@@ -15,6 +15,7 @@
15from landscape.lib.fetch import url_to_filename, fetch_to_files15from landscape.lib.fetch import url_to_filename, fetch_to_files
16from landscape.lib.lsb_release import parse_lsb_release, LSB_RELEASE_FILENAME16from landscape.lib.lsb_release import parse_lsb_release, LSB_RELEASE_FILENAME
17from landscape.lib.gpg import gpg_verify17from landscape.lib.gpg import gpg_verify
18from landscape.lib.fs import read_file
18from landscape.package.taskhandler import (19from landscape.package.taskhandler import (
19 PackageTaskHandlerConfiguration, PackageTaskHandler, run_task_handler)20 PackageTaskHandlerConfiguration, PackageTaskHandler, run_task_handler)
20from landscape.manager.manager import SUCCEEDED, FAILED21from landscape.manager.manager import SUCCEEDED, FAILED
@@ -33,12 +34,25 @@
3334
3435
35class ReleaseUpgrader(PackageTaskHandler):36class ReleaseUpgrader(PackageTaskHandler):
36 """Perform release upgrades."""37 """Perform release upgrades.
38
39 @cvar config_factory: The configuration class to use to build configuration
40 objects to be passed to our constructor.
41 @cvar queue_name: The queue we pick tasks from.
42 @cvar lsb_release_filename: The path to the LSB data on the file system.
43 @cvar landscape_ppa_url: The URL of the Landscape PPA, if it is present
44 in the computer's sources.list it won't be commented out.
45 @cvar logs_directory: Path to the directory holding the upgrade-tool logs.
46 @cvar logs_limit: When reporting upgrade-tool logs to the server, only the
47 last C{logs_limit} lines will be sent.
48 """
3749
38 config_factory = ReleaseUpgraderConfiguration50 config_factory = ReleaseUpgraderConfiguration
39 queue_name = "release-upgrader"51 queue_name = "release-upgrader"
40 lsb_release_filename = LSB_RELEASE_FILENAME52 lsb_release_filename = LSB_RELEASE_FILENAME
41 landscape_ppa_url = "http://ppa.launchpad.net/landscape/ppa/ubuntu/"53 landscape_ppa_url = "http://ppa.launchpad.net/landscape/ppa/ubuntu/"
54 logs_directory = "/var/log/dist-upgrade"
55 logs_limit = 100000
4256
43 def make_operation_result_message(self, operation_id, status, text, code):57 def make_operation_result_message(self, operation_id, status, text, code):
44 """Convenience to create messages of type C{"operation-result"}."""58 """Convenience to create messages of type C{"operation-result"}."""
@@ -210,6 +224,28 @@
210224
211 return succeed(None)225 return succeed(None)
212226
227 def make_operation_result_text(self, out, err):
228 """Return the operation result text to be sent to the server.
229
230 @param out: The standard output of the upgrade-tool process.
231 @param err: The standard error of the upgrade-tool process.
232 @return: A text aggregating the process output, error and log files.
233 """
234 buf = cStringIO.StringIO()
235
236 for label, content in [("output", out), ("error", err)]:
237 if content:
238 buf.write("=== Standard %s ===\n\n%s\n\n" % (label, content))
239
240 for basename in os.listdir(self.logs_directory):
241 if not basename.endswith(".log"):
242 continue
243 filename = os.path.join(self.logs_directory, basename)
244 content = read_file(filename, - self.logs_limit)
245 buf.write("=== %s ===\n\n%s\n\n" % (basename, content))
246
247 return buf.getvalue()
248
213 def upgrade(self, code_name, operation_id, allow_third_party=False,249 def upgrade(self, code_name, operation_id, allow_third_party=False,
214 debug=False, mode=None):250 debug=False, mode=None):
215 """Run the upgrade-tool command and send a report of the results.251 """Run the upgrade-tool command and send a report of the results.
@@ -267,8 +303,9 @@
267 status = SUCCEEDED303 status = SUCCEEDED
268 else:304 else:
269 status = FAILED305 status = FAILED
270 message = self.make_operation_result_message(306 text = self.make_operation_result_text(out, err)
271 operation_id, status, "%s%s" % (out, err), code)307 message = self.make_operation_result_message(operation_id, status,
308 text, code)
272 logging.info("Queuing message with release upgrade results to "309 logging.info("Queuing message with release upgrade results to "
273 "exchange urgently.")310 "exchange urgently.")
274 return self._broker.send_message(message, True)311 return self._broker.send_message(message, True)
275312
=== modified file 'landscape/package/tests/test_releaseupgrader.py'
--- landscape/package/tests/test_releaseupgrader.py 2010-01-21 08:47:03 +0000
+++ landscape/package/tests/test_releaseupgrader.py 2010-03-16 10:39:30 +0000
@@ -270,11 +270,93 @@
270 result.addCallback(check_result)270 result.addCallback(check_result)
271 return result271 return result
272272
273 def test_default_logs_directory(self):
274 """
275 The default directory for the upgrade-tool logs is the system one.
276 """
277 self.assertEquals(self.upgrader.logs_directory,
278 "/var/log/dist-upgrade")
279
280 def test_default_logs_limit(self):
281 """
282 The default read limit for the upgrade-tool logs is 100000 bytes.
283 """
284 self.assertEquals(self.upgrader.logs_limit, 100000)
285
286 def test_make_operation_result_text(self):
287 """
288 L{ReleaseUpgrade.make_operation_result_text} aggregates the contents of
289 the process standard output, error and log files.
290 """
291 self.upgrader.logs_directory = self.makeDir()
292 self.makeFile(basename="main.log",
293 dirname=self.upgrader.logs_directory,
294 content="main log")
295 self.makeFile(basename="apt.log",
296 dirname=self.upgrader.logs_directory,
297 content="apt log")
298 text = self.upgrader.make_operation_result_text("stdout", "stderr")
299 self.assertEquals(text,
300 "=== Standard output ===\n\n"
301 "stdout\n\n"
302 "=== Standard error ===\n\n"
303 "stderr\n\n"
304 "=== main.log ===\n\n"
305 "main log\n\n"
306 "=== apt.log ===\n\n"
307 "apt log\n\n")
308
309 def test_make_operation_result_text_with_no_stderr(self):
310 """
311 L{ReleaseUpgrade.make_operation_result_text} skips the standard error
312 if it's empty.
313 """
314 self.upgrader.logs_directory = self.makeDir()
315 text = self.upgrader.make_operation_result_text("stdout", "")
316 self.assertEquals(text,
317 "=== Standard output ===\n\n"
318 "stdout\n\n")
319
320 def test_make_operation_result_text_only_considers_log_files(self):
321 """
322 L{ReleaseUpgrade.make_operation_result_text} only considers log files
323 from the last upgrade-tool run, directories containing log files from
324 an older run are skipped.
325 """
326 self.upgrader.logs_directory = self.makeDir()
327 self.makeDir(dirname=self.upgrader.logs_directory)
328 text = self.upgrader.make_operation_result_text("stdout", "stderr")
329 self.assertEquals(text,
330 "=== Standard output ===\n\n"
331 "stdout\n\n"
332 "=== Standard error ===\n\n"
333 "stderr\n\n")
334
335 def test_make_operation_result_text_trims_long_files(self):
336 """
337 L{ReleaseUpgrade.make_operation_result_text} only reads the last
338 L{logs_limit} lines of a log file.
339 """
340 self.upgrader.logs_directory = self.makeDir()
341 self.upgrader.logs_limit = 8
342 self.makeFile(basename="main.log",
343 dirname=self.upgrader.logs_directory,
344 content="very long log")
345 text = self.upgrader.make_operation_result_text("stdout", "stderr")
346 self.assertEquals(text,
347 "=== Standard output ===\n\n"
348 "stdout\n\n"
349 "=== Standard error ===\n\n"
350 "stderr\n\n"
351 "=== main.log ===\n\n"
352 "long log\n\n")
353
273 def test_upgrade(self):354 def test_upgrade(self):
274 """355 """
275 The L{ReleaseUpgrader.upgrade} method spawns the appropropriate356 The L{ReleaseUpgrader.upgrade} method spawns the appropropriate
276 upgrade-tool script and reports the result.357 upgrade-tool script and reports the result.
277 """358 """
359 self.upgrader.logs_directory = self.makeDir()
278 upgrade_tool_directory = self.config.upgrade_tool_directory360 upgrade_tool_directory = self.config.upgrade_tool_directory
279 upgrade_tool_filename = os.path.join(upgrade_tool_directory, "karmic")361 upgrade_tool_filename = os.path.join(upgrade_tool_directory, "karmic")
280 fd = open(upgrade_tool_filename, "w")362 fd = open(upgrade_tool_filename, "w")
@@ -298,8 +380,10 @@
298 self.assertIn("INFO: Queuing message with release upgrade "380 self.assertIn("INFO: Queuing message with release upgrade "
299 "results to exchange urgently.",381 "results to exchange urgently.",
300 self.logfile.getvalue())382 self.logfile.getvalue())
301 result_text = u"--frontend DistUpgradeViewNonInteractive\n" \383 result_text = (u"=== Standard output ===\n\n"
302 "FOO=bar\nPWD=%s\nout\n" % upgrade_tool_directory384 "--frontend DistUpgradeViewNonInteractive\n"
385 "FOO=bar\n"
386 "PWD=%s\nout\n\n\n" % upgrade_tool_directory)
303 self.assertMessages(self.get_pending_messages(),387 self.assertMessages(self.get_pending_messages(),
304 [{"type": "operation-result",388 [{"type": "operation-result",
305 "operation-id": 100,389 "operation-id": 100,
@@ -324,6 +408,7 @@
324 which gets passed to the upgrade-tool script as argument for the408 which gets passed to the upgrade-tool script as argument for the
325 C{--mode} command line option.409 C{--mode} command line option.
326 """410 """
411 self.upgrader.logs_directory = self.makeDir()
327 upgrade_tool_directory = self.config.upgrade_tool_directory412 upgrade_tool_directory = self.config.upgrade_tool_directory
328 upgrade_tool_filename = os.path.join(upgrade_tool_directory, "hardy")413 upgrade_tool_filename = os.path.join(upgrade_tool_directory, "hardy")
329 self.makeFile(path=upgrade_tool_filename,414 self.makeFile(path=upgrade_tool_filename,
@@ -337,8 +422,9 @@
337 result = self.upgrader.upgrade("hardy", 100, mode="server")422 result = self.upgrader.upgrade("hardy", 100, mode="server")
338423
339 def check_result(ignored):424 def check_result(ignored):
340 result_text = ("--frontend DistUpgradeViewNonInteractive "425 result_text = (u"=== Standard output ===\n\n"
341 "--mode server\n")426 "--frontend DistUpgradeViewNonInteractive "
427 "--mode server\n\n\n")
342 self.assertMessages(self.get_pending_messages(),428 self.assertMessages(self.get_pending_messages(),
343 [{"type": "operation-result",429 [{"type": "operation-result",
344 "operation-id": 100,430 "operation-id": 100,
@@ -357,6 +443,7 @@
357 The L{ReleaseUpgrader.upgrade} method optionally sets environment443 The L{ReleaseUpgrader.upgrade} method optionally sets environment
358 variables to be passed to the upgrade-tool process.444 variables to be passed to the upgrade-tool process.
359 """445 """
446 self.upgrader.logs_directory = self.makeDir()
360 upgrade_tool_directory = self.config.upgrade_tool_directory447 upgrade_tool_directory = self.config.upgrade_tool_directory
361 upgrade_tool_filename = os.path.join(upgrade_tool_directory, "karmic")448 upgrade_tool_filename = os.path.join(upgrade_tool_directory, "karmic")
362 fd = open(upgrade_tool_filename, "w")449 fd = open(upgrade_tool_filename, "w")
@@ -377,8 +464,9 @@
377 debug=True)464 debug=True)
378465
379 def check_result(ignored):466 def check_result(ignored):
380 result_text = u"DEBUG_UPDATE_MANAGER=True\n" \467 result_text = (u"=== Standard output ===\n\n"
381 u"RELEASE_UPRADER_ALLOW_THIRD_PARTY=True\n"468 "DEBUG_UPDATE_MANAGER=True\n"
469 "RELEASE_UPRADER_ALLOW_THIRD_PARTY=True\n\n\n")
382 self.assertMessages(self.get_pending_messages(),470 self.assertMessages(self.get_pending_messages(),
383 [{"type": "operation-result",471 [{"type": "operation-result",
384 "operation-id": 100,472 "operation-id": 100,
@@ -402,6 +490,7 @@
402 The L{ReleaseUpgrader.upgrade} sends a message with failed status490 The L{ReleaseUpgrader.upgrade} sends a message with failed status
403 field if the upgrade-tool exits with non-zero code.491 field if the upgrade-tool exits with non-zero code.
404 """492 """
493 self.upgrader.logs_directory = self.makeDir()
405 upgrade_tool_directory = self.config.upgrade_tool_directory494 upgrade_tool_directory = self.config.upgrade_tool_directory
406 upgrade_tool_filename = os.path.join(upgrade_tool_directory, "karmic")495 upgrade_tool_filename = os.path.join(upgrade_tool_directory, "karmic")
407 fd = open(upgrade_tool_filename, "w")496 fd = open(upgrade_tool_filename, "w")
@@ -419,11 +508,13 @@
419 result = self.upgrader.upgrade("karmic", 100)508 result = self.upgrader.upgrade("karmic", 100)
420509
421 def check_result(ignored):510 def check_result(ignored):
511 result_text = (u"=== Standard output ===\n\nout\n\n\n"
512 "=== Standard error ===\n\nerr\n\n\n")
422 self.assertMessages(self.get_pending_messages(),513 self.assertMessages(self.get_pending_messages(),
423 [{"type": "operation-result",514 [{"type": "operation-result",
424 "operation-id": 100,515 "operation-id": 100,
425 "status": FAILED,516 "status": FAILED,
426 "result-text": "out\nerr\n",517 "result-text": result_text,
427 "result-code": 3}])518 "result-code": 3}])
428519
429 result.addCallback(check_result)520 result.addCallback(check_result)
@@ -439,6 +530,7 @@
439 and passes its files descriptors over to child processes we don't know530 and passes its files descriptors over to child processes we don't know
440 about.531 about.
441 """532 """
533 self.upgrader.logs_directory = self.makeDir()
442 upgrade_tool_directory = self.config.upgrade_tool_directory534 upgrade_tool_directory = self.config.upgrade_tool_directory
443 upgrade_tool_filename = os.path.join(upgrade_tool_directory, "karmic")535 upgrade_tool_filename = os.path.join(upgrade_tool_directory, "karmic")
444 child_pid_filename = self.makeFile()536 child_pid_filename = self.makeFile()

Subscribers

People subscribed via source and target branches

to all changes: