Merge lp:~bigkevmcd/landscape-client/218388-limit-free-space-messages into lp:~landscape/landscape-client/trunk

[Kevin McDermott]

[Kevin McDermott (bigkevmcd)]

This provides a hard limit on the number of free space items...

I picked a fairly arbitrary number to limit it to...

[Free Ekanayaka (free.ekanayaka)]

Nice fix, +1!

[1]

+ MAX_FREE_SPACE_ITEMS_TO_EXCHANGE = 200
+

This is only about cosmetics, but in general I don't think we upper-case class attributes, even if meant to be constant (well, and that's a bit already implied by the fact that they are class attributes, or at least it should be a hint). So I'd suggest to lower-case it to max_free_space_items_to_exchange.

review: Approve

[lp:~bigkevmcd/landscape-client/218388-limit-free-space-messages]

201. By Kevin McDermott on 2010-01-29

Change MAX_FREE_SPACE... to max_free_space... from Free's review.

[Thomas Herve (therve)]

Looks good, +1.

review: Approve

=== modified file 'landscape/monitor/mountinfo.py'
--- landscape/monitor/mountinfo.py	2009-10-06 16:27:23 +0000
+++ landscape/monitor/mountinfo.py	2010-01-29 12:20:31 +0000
@@ -12,6 +12,8 @@
 
     persist_name = "mount-info"
 
+    max_free_space_items_to_exchange = 200
+
     def __init__(self, interval=300, monitor_interval=60*60,
                  mounts_file="/proc/mounts", create_time=time.time,
                  statvfs=None, hal_manager=None, mtab_file="/etc/mtab"):
@@ -69,8 +71,12 @@
 
     def create_free_space_message(self):
         if self._free_space:
-            message = {"type": "free-space", "free-space": self._free_space}
-            self._free_space = []
+            items_to_exchange = self._free_space[
+                :self.max_free_space_items_to_exchange]
+            message = {"type": "free-space",
+                       "free-space": items_to_exchange}
+            self._free_space = self._free_space[
+                self.max_free_space_items_to_exchange:]
             return message
         return None
 
@@ -214,4 +220,3 @@
             if "bind" in options.split(","):
                 bound_points.add(mount_point)
         return bound_points
-

=== modified file 'landscape/monitor/tests/test_mountinfo.py'
--- landscape/monitor/tests/test_mountinfo.py	2009-10-06 14:20:01 +0000
+++ landscape/monitor/tests/test_mountinfo.py	2010-01-29 12:20:31 +0000
@@ -240,8 +240,8 @@
 
         message = [d for d in messages if d["type"] == "free-space"][0]
         free_space = message["free-space"]
-        for i in range(1, 1):
-            self.assertEquals(free_space[i][0], i * step_size)
+        for i in range(len(free_space)):
+            self.assertEquals(free_space[i][0], (i + 1) * step_size)
             self.assertEquals(free_space[i][1], "/")
             self.assertEquals(free_space[i][2], 409600)
 
@@ -728,3 +728,67 @@
         plugin.run()
         message3 = plugin.create_mount_info_message()
         self.assertIdentical(message3, None)
+
+    def test_exchange_limits_exchanged_free_space_messages(self):
+        """
+        In order not to overload the server, the client should stagger the
+        exchange of free-space messages.
+        """
+        def statvfs(path):
+            return (4096, 0, mb(1000), mb(100), 0, 0, 0, 0, 0)
+
+        filename = self.makeFile("""\
+/dev/hda1 / ext3 rw 0 0
+""")
+        plugin = self.get_mount_info(mounts_file=filename, statvfs=statvfs,
+                                     create_time=self.reactor.time,
+                                     mtab_file=filename)
+        # Limit the test exchange to 5 items.
+        plugin.max_free_space_items_to_exchange = 5
+        step_size = self.monitor.step_size
+        self.monitor.add(plugin)
+
+        # Exchange should trigger a flush of the persist database
+        registry_mocker = self.mocker.replace(plugin.registry)
+        registry_mocker.flush()
+        self.mocker.result(None)
+        self.mocker.replay()
+
+        # Generate 10 data points
+        self.reactor.advance(step_size * 10)
+        self.monitor.exchange()
+
+        messages = self.mstore.get_pending_messages()
+        self.assertEquals(len(messages), 3)
+
+        message = [d for d in messages if d["type"] == "free-space"][0]
+        free_space = message["free-space"]
+        free_space_items = len(free_space)
+        self.assertEquals(free_space_items, 5)
+        for i in range(free_space_items):
+            self.assertEquals(free_space[i][0], (i + 1) * step_size)
+            self.assertEquals(free_space[i][1], "/")
+            self.assertEquals(free_space[i][2], 409600)
+
+        # The second exchange should pick up the remaining items.
+        self.mstore.delete_all_messages()
+        self.monitor.exchange()
+
+        messages = self.mstore.get_pending_messages()
+        self.assertEquals(len(messages), 1)
+
+        message = [d for d in messages if d["type"] == "free-space"][0]
+        free_space = message["free-space"]
+        free_space_items = len(free_space)
+        self.assertEquals(free_space_items, 5)
+        for i in range(free_space_items):
+            # Note (i+6) we've already retrieved the first 5 items.
+            self.assertEquals(free_space[i][0], (i + 6) * step_size)
+            self.assertEquals(free_space[i][1], "/")
+            self.assertEquals(free_space[i][2], 409600)
+
+        # Third exchange should not get any further free-space messages
+        self.mstore.delete_all_messages()
+        self.monitor.exchange()
+        messages = self.mstore.get_pending_messages()
+        self.assertEquals(len(messages), 0)