Merge lp:~psusi/landscape-client/correct-free-mem into lp:~landscape/landscape-client/trunk

Proposed by Phillip Susi
Status: Merged
Approved by: Chris Glass
Approved revision: 594
Merged at revision: 602
Proposed branch: lp:~psusi/landscape-client/correct-free-mem
Merge into: lp:~landscape/landscape-client/trunk
Diff against target: 77 lines (+12/-10)
4 files modified
landscape/lib/sysstats.py (+4/-2)
landscape/lib/tests/test_sysstats.py (+4/-4)
landscape/monitor/tests/test_memoryinfo.py (+3/-3)
landscape/sysinfo/tests/test_memory.py (+1/-1)
To merge this branch: bzr merge lp:~psusi/landscape-client/correct-free-mem
Reviewer Review Type Date Requested Status
Jerry Seutter (community) Approve
Geoff Teale (community) Approve
Review via email: mp+140541@code.launchpad.net

This proposal supersedes a proposal from 2012-12-06.

Commit message

landscape-sysinfo was reporting memory usage values that
were entirely incorrect. This was because it was calculating free
memory to be all memory not on the active list. Changed to use
free + cached + buffers, just like the free command does.

Description of the change

Corrected test suite.

To post a comment you must log in.
Revision history for this message
Andreas Hasenack (ahasenack) wrote : Posted in a previous version of this proposal
Download full text (4.9 KiB)

There are a bunch of test failures, can you take a look at those?

[FAIL]
Traceback (most recent call last):
  File "/home/andreas/canonical/source/landscape-client/psusi/correct-free-mem/landscape/tests/mocker.py", line 146, in test_method_wrapper
    result = test_method()
  File "/home/andreas/canonical/source/landscape-client/psusi/correct-free-mem/landscape/lib/tests/test_sysstats.py", line 42, in test_get_memory_info
    self.assertEqual(memstats.free_memory, 503)
  File "/usr/lib/python2.7/dist-packages/twisted/trial/unittest.py", line 271, in assertEqual
    % (msg, pformat(first), pformat(second)))
twisted.trial.unittest.FailTest: not equal:
a = 852
b = 503

landscape.lib.tests.test_sysstats.MemoryStatsTest.test_get_memory_info
===============================================================================
[FAIL]
Traceback (most recent call last):
  File "/home/andreas/canonical/source/landscape-client/psusi/correct-free-mem/landscape/tests/mocker.py", line 146, in test_method_wrapper
    result = test_method()
  File "/home/andreas/canonical/source/landscape-client/psusi/correct-free-mem/landscape/monitor/tests/test_memoryinfo.py", line 154, in test_exchange_messages
    (step_size * 2, 503, 1567)]}])
  File "/home/andreas/canonical/source/landscape-client/psusi/correct-free-mem/landscape/tests/helpers.py", line 82, in assertMessages
    self.assertMessage(obtained_message, expected_message)
  File "/home/andreas/canonical/source/landscape-client/psusi/correct-free-mem/landscape/tests/helpers.py", line 76, in assertMessage
    pprint.pformat(obtained)))
twisted.trial.unittest.FailTest: Messages don't match.
Expected:
{'memory-info': [(300, 503, 1567), (600, 503, 1567)], 'type': 'memory-info'}
Obtained:
{'memory-info': [(300, 852, 1567), (600, 852, 1567)], 'type': 'memory-info'}

landscape.monitor.tests.test_memoryinfo.MemoryInfoTest.test_exchange_messages
===============================================================================
[FAIL]
Traceback (most recent call last):
  File "/home/andreas/canonical/source/landscape-client/psusi/correct-free-mem/landscape/tests/mocker.py", line 146, in test_method_wrapper
    result = test_method()
  File "/home/andreas/canonical/source/landscape-client/psusi/correct-free-mem/landscape/monitor/tests/test_memoryinfo.py", line 78, in test_read_sample_data
    self.assertEqual(message["memory-info"][0], (step_size, 503, 1567))
  File "/usr/lib/python2.7/dist-packages/twisted/trial/unittest.py", line 271, in assertEqual
    % (msg, pformat(first), pformat(second)))
twisted.trial.unittest.FailTest: not equal:
a = (300, 852, 1567)
b = (300, 503, 1567)

landscape.monitor.tests.test_memoryinfo.MemoryInfoTest.test_read_sample_data
===============================================================================
[FAIL]
Traceback (most recent call last):
  File "/home/andreas/canonical/source/landscape-client/psusi/correct-free-mem/landscape/tests/mocker.py", line 146, in test_method_wrapper
    result = test_method()
  File "/home/andreas/canonical/source/landscape-client/psusi/correct-free-mem/landscape/sysinfo/tests/test_memory.py", line 53, in test_run_adds_header
    ("Swap usage", "39%")])
  File...

Read more...

Revision history for this message
Jerry Seutter (jseutter) wrote :

+1, this makes landscape-sysinfo report more accurate memory usage.

Without the change:
jseutter@perth:~/src/landscape-client/trunk$ ./scripts/landscape-sysinfo
  System load: 0.82 Users logged in: 1
  Usage of /: 58.1% of 228.21GB IP address for eth0: 192.168.2.138
  Memory usage: 42% IP address for lxcbr0: 10.0.3.1
  Swap usage: 0% IP address for wlan0: 192.168.2.100
  Processes: 238

With the change:
jseutter@perth:~/src/landscape-client/correct-free-mem$ ./scripts/landscape-sysinfo
  System load: 0.92 Users logged in: 1
  Usage of /: 58.1% of 228.21GB IP address for eth0: 192.168.2.138
  Memory usage: 22% IP address for lxcbr0: 10.0.3.1
  Swap usage: 0% IP address for wlan0: 192.168.2.100
  Processes: 238

This matches the output of top, free and The Internet. It differs from the free command by 1%, but I assume this is because we truncate the value somewhere.

I'm getting two PEP8 warnings, can you clean these up?
=== pep8 ===

landscape/lib/sysstats.py:16:80: E501 line too long (96 characters)
landscape/lib/sysstats.py:20:80: E501 line too long (87 characters)

594. By Phillip Susi

landscape-sysinfo was reporting memory usage values that
were entirely incorrect. This was because it was calculating free
memory to be all memory not on the active list. Changed to use
free + cached + buffers, just like the free command does.

Revision history for this message
Phillip Susi (psusi) wrote :

How's this?

Revision history for this message
Jerry Seutter (jseutter) wrote :

> How's this?

Looks good. Thanks!

Revision history for this message
Geoff Teale (tealeg) wrote :

+1 Nice change!

review: Approve
Revision history for this message
Jerry Seutter (jseutter) wrote :

+1

review: Approve
Revision history for this message
Chris Glass (tribaal) wrote :

Marking this as Approved to let tarmac pick this up and merge it in.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'landscape/lib/sysstats.py'
2--- landscape/lib/sysstats.py 2010-04-28 13:07:53 +0000
3+++ landscape/lib/sysstats.py 2012-12-27 20:12:21 +0000
4@@ -13,11 +13,13 @@
5 for line in open(filename):
6 if ":" in line:
7 key, value = line.split(":", 1)
8- if key in ["Active", "MemTotal", "SwapFree", "SwapTotal"]:
9+ if key in ["MemTotal", "SwapFree", "SwapTotal", "MemFree",
10+ "Buffers", "Cached"]:
11 data[key] = int(value.split()[0])
12
13 self.total_memory = data["MemTotal"] // 1024
14- self.free_memory = (data["MemTotal"] - data["Active"]) // 1024
15+ self.free_memory = (data["MemFree"] + data["Buffers"] +
16+ data["Cached"]) // 1024
17 self.total_swap = data["SwapTotal"] // 1024
18 self.free_swap = data["SwapFree"] // 1024
19
20
21=== modified file 'landscape/lib/tests/test_sysstats.py'
22--- landscape/lib/tests/test_sysstats.py 2011-07-05 05:09:11 +0000
23+++ landscape/lib/tests/test_sysstats.py 2012-12-27 20:12:21 +0000
24@@ -39,14 +39,14 @@
25 filename = self.makeFile(SAMPLE_MEMORY_INFO)
26 memstats = MemoryStats(filename)
27 self.assertEqual(memstats.total_memory, 1510)
28- self.assertEqual(memstats.free_memory, 503)
29- self.assertEqual(memstats.used_memory, 1007)
30+ self.assertEqual(memstats.free_memory, 852)
31+ self.assertEqual(memstats.used_memory, 658)
32 self.assertEqual(memstats.total_swap, 1584)
33 self.assertEqual(memstats.free_swap, 1567)
34 self.assertEqual(memstats.used_swap, 17)
35- self.assertEqual("%.2f" % memstats.free_memory_percentage, "33.31")
36+ self.assertEqual("%.2f" % memstats.free_memory_percentage, "56.42")
37 self.assertEqual("%.2f" % memstats.free_swap_percentage, "98.93")
38- self.assertEqual("%.2f" % memstats.used_memory_percentage, "66.69")
39+ self.assertEqual("%.2f" % memstats.used_memory_percentage, "43.58")
40 self.assertEqual("%.2f" % memstats.used_swap_percentage, "1.07")
41
42 def test_get_memory_info_without_swap(self):
43
44=== modified file 'landscape/monitor/tests/test_memoryinfo.py'
45--- landscape/monitor/tests/test_memoryinfo.py 2011-07-05 05:09:11 +0000
46+++ landscape/monitor/tests/test_memoryinfo.py 2012-12-27 20:12:21 +0000
47@@ -75,7 +75,7 @@
48 self.reactor.advance(step_size)
49
50 message = plugin.create_message()
51- self.assertEqual(message["memory-info"][0], (step_size, 503, 1567))
52+ self.assertEqual(message["memory-info"][0], (step_size, 852, 1567))
53
54 def test_messaging_flushes(self):
55 """
56@@ -150,8 +150,8 @@
57
58 self.assertMessages(self.mstore.get_pending_messages(),
59 [{"type": "memory-info",
60- "memory-info": [(step_size, 503, 1567),
61- (step_size * 2, 503, 1567)]}])
62+ "memory-info": [(step_size, 852, 1567),
63+ (step_size * 2, 852, 1567)]}])
64
65 def test_call_on_accepted(self):
66 plugin = MemoryInfo(source_filename=self.makeFile(self.SAMPLE_DATA),
67
68=== modified file 'landscape/sysinfo/tests/test_memory.py'
69--- landscape/sysinfo/tests/test_memory.py 2011-07-05 05:09:11 +0000
70+++ landscape/sysinfo/tests/test_memory.py 2012-12-27 20:12:21 +0000
71@@ -49,5 +49,5 @@
72 def test_run_adds_header(self):
73 self.memory.run()
74 self.assertEqual(self.sysinfo.get_headers(),
75- [("Memory usage", "34%"),
76+ [("Memory usage", "27%"),
77 ("Swap usage", "39%")])

Subscribers

People subscribed via source and target branches

to all changes: