Merge lp:~tealeg/landscape-client/package-monitor-scoped-resynch into lp:~landscape/landscape-client/trunk

[Geoff Teale]

This branch makes the PackageMonitor plugin only respond to resynchronize-clients events when they are either "package" or global scope.

[Jerry Seutter (jseutter)]

+1, looks good.

landscape/monitor/tests/test_packagemonitor.py:233:80: E501 line too long (80 characters)

review: Approve

[Jerry Seutter (jseutter)]

In other branches the call signature for _resynchronize uses scopes:
def _resynchronize(self, scopes):

Can you update this changeset to use scopes instead of scope?

[Geoff Teale (tealeg)]

> +1, looks good.
>
> landscape/monitor/tests/test_packagemonitor.py:233:80: E501 line too long (80
> characters)

Done.

[Geoff Teale (tealeg)]

> In other branches the call signature for _resynchronize uses scopes:
> def _resynchronize(self, scopes):
>
> Can you update this changeset to use scopes instead of scope?

Done.

[Björn Tillenius (bjornt)]

[1]

+ if len(scopes) == 0 or self.scope in scopes:

This makes me a bit sad. Having this in every _resynchronize() doesn't
doesn't look good.

I see that we register the resynchronize method with something like
this:

  self.registry.reactor.call_on("resynchronize", self._resynchronize)

I'd suggest doing something like this instead:

  self.registry.register_resynchronize(self.scope, self._resynchronize)

That way self._resynchronize would be called only for the relevant
scope, which makes the method easier to read.

review: Needs Fixing

[Geoff Teale (tealeg)]

> [1]
>
> + if len(scopes) == 0 or self.scope in scopes:
>
> This makes me a bit sad. Having this in every _resynchronize() doesn't
> doesn't look good.
>
> I see that we register the resynchronize method with something like
> this:
>
> self.registry.reactor.call_on("resynchronize", self._resynchronize)
>
> I'd suggest doing something like this instead:
>
> self.registry.register_resynchronize(self.scope, self._resynchronize)
>
> That way self._resynchronize would be called only for the relevant
> scope, which makes the method easier to read.

Hi Bjorn, would you be happy for me to make that change as a follow on branch? It effects multiple branches here and it would be cleaner to tidy it all up in one swoop.

[Björn Tillenius (bjornt)]

On Wed, Jul 10, 2013 at 05:37:53PM -0000, Geoff Teale wrote:
> > [1]
> >
> > + if len(scopes) == 0 or self.scope in scopes:
> >
> > This makes me a bit sad. Having this in every _resynchronize() doesn't
> > doesn't look good.
> >
> > I see that we register the resynchronize method with something like
> > this:
> >
> > self.registry.reactor.call_on("resynchronize", self._resynchronize)
> >
> > I'd suggest doing something like this instead:
> >
> > self.registry.register_resynchronize(self.scope, self._resynchronize)
> >
> > That way self._resynchronize would be called only for the relevant
> > scope, which makes the method easier to read.
>
> Hi Bjorn, would you be happy for me to make that change as a follow on
> branch? It effects multiple branches here and it would be cleaner to
> tidy it all up in one swoop.

Sure, that would be fine, if it's easier.

[Geoff Teale (tealeg)]

> > Hi Bjorn, would you be happy for me to make that change as a follow on
> > branch? It effects multiple branches here and it would be cleaner to
> > tidy it all up in one swoop.
>
> Sure, that would be fine, if it's easier.

OK, I'll add a clean up branch at the end, I have some other calls to tidy up across all branches too.

[lp:~tealeg/landscape-client/package-monitor-scoped-resynch]

720. By Geoff Teale on 2013-07-16

Merge forwards and fix conflicts.

721. By Geoff Teale on 2013-07-16

Merge forwards

722. By Geoff Teale on 2013-07-16

merge forwards and resolve conflicts.

723. By Geoff Teale on 2013-07-25

Make test more readable by returning the task from createReporterTask.

Unmerged revisions

=== modified file 'landscape/monitor/packagemonitor.py'
--- landscape/monitor/packagemonitor.py	2013-07-12 08:18:35 +0000
+++ landscape/monitor/packagemonitor.py	2013-07-16 13:16:33 +0000
@@ -11,6 +11,7 @@
 class PackageMonitor(MonitorPlugin):
 
     run_interval = 1800
+    scope = "package"
 
     def __init__(self, package_store_filename=None):
         super(PackageMonitor, self).__init__()

=== modified file 'landscape/monitor/plugin.py'
--- landscape/monitor/plugin.py	2013-07-12 14:12:48 +0000
+++ landscape/monitor/plugin.py	2013-07-16 13:16:33 +0000
@@ -23,9 +23,14 @@
         else:
             self._persist = None
 
+<<<<<<< TREE
     def _reset(self):
         if self.persist_name is not None:
             self.registry.persist.remove(self.persist_name)
+=======
+    def _reset(self):
+        self.registry.persist.remove(self.persist_name)
+>>>>>>> MERGE-SOURCE
 
     @property
     def persist(self):

=== modified file 'landscape/monitor/tests/test_aptpreferences.py'
--- landscape/monitor/tests/test_aptpreferences.py	2013-07-12 13:41:07 +0000
+++ landscape/monitor/tests/test_aptpreferences.py	2013-07-16 13:16:33 +0000
@@ -171,7 +171,12 @@
         self.makeFile(path=preferences_filename, content="crap")
         self.mstore.set_accepted_types(["apt-preferences"])
         self.plugin.run()
+<<<<<<< TREE
         self.reactor.fire("resynchronize", scopes=["package"])
+=======
+        package_scope = ["package"]
+        self.reactor.fire("resynchronize", package_scope)
+>>>>>>> MERGE-SOURCE
         self.plugin.run()
         messages = self.mstore.get_pending_messages()
         self.assertEqual(len(messages), 2)

=== modified file 'landscape/monitor/tests/test_computerinfo.py'
--- landscape/monitor/tests/test_computerinfo.py	2013-07-12 13:41:07 +0000
+++ landscape/monitor/tests/test_computerinfo.py	2013-07-16 13:16:33 +0000
@@ -278,7 +278,12 @@
                               root_path=self.makeDir())
         self.monitor.add(plugin)
         plugin.exchange()
+<<<<<<< TREE
         self.reactor.fire("resynchronize", scopes=["computer"])
+=======
+        computer_scope = ["computer"]
+        self.reactor.fire("resynchronize", computer_scope)
+>>>>>>> MERGE-SOURCE
         plugin.exchange()
         computer_info = {"type": "computer-info", "hostname": "ooga.local",
                          "timestamp": 0, "total-memory": 1510,

=== modified file 'landscape/monitor/tests/test_mountinfo.py'
--- landscape/monitor/tests/test_mountinfo.py	2013-07-12 13:41:07 +0000
+++ landscape/monitor/tests/test_mountinfo.py	2013-07-16 13:16:33 +0000
@@ -422,7 +422,12 @@
 
         plugin.run()
         plugin.exchange()
+<<<<<<< TREE
         self.reactor.fire("resynchronize", scopes=["disk"])
+=======
+        disk_scope = ["disk"]
+        self.reactor.fire("resynchronize", disk_scope)
+>>>>>>> MERGE-SOURCE
         plugin.run()
         plugin.exchange()
         messages = self.mstore.get_pending_messages()

=== modified file 'landscape/monitor/tests/test_packagemonitor.py'
--- landscape/monitor/tests/test_packagemonitor.py	2013-07-16 08:30:55 +0000
+++ landscape/monitor/tests/test_packagemonitor.py	2013-07-16 13:16:33 +0000
@@ -23,6 +23,33 @@
 
         self.package_monitor = PackageMonitor(self.package_store_filename)
 
+    def createReporterTask(self):
+        """
+        Put a task for the package reported into the package store.
+        """
+        message = {"type": "package-ids", "ids": [None], "request-id": 1}
+        self.package_store.add_task("reporter", message)
+
+    def assertSingleReporterTask(self, data, task_id):
+        """
+        Check that we have exactly one task, that it contains the right data
+        and that it's ID matches our expectation.
+        """
+        # The next task should contain the passed data.
+        task = self.package_store.get_next_task("reporter")
+        self.assertEqual(task.data, data)
+
+        # We want to make sure it has the correct id of 2 so that we
+        # know it's not a new task that the reporter could possibly
+        # remove by accident.
+        self.assertEqual(task.id, task_id)
+
+        # Let's remove that task and make sure there are no more tasks
+        # in the queue.
+        task.remove()
+        task = self.package_store.get_next_task("reporter")
+        self.assertEqual(task, None)
+
     def test_create_default_store_upon_message_handling(self):
         """
         If the package sqlite database file doesn't exist yet, it is created
@@ -200,17 +227,46 @@
 
     def test_resynchronize(self):
         """
-        If a 'resynchronize' reactor event is fired, the package
-        monitor should clear all queued tasks and queue a task that
+        If a 'resynchronize' reactor event is fired with 'package' scope, the
+        package monitor should clear all queued tasks and queue a task that
         tells the report to clear out the rest of the package data.
         """
         self.monitor.add(self.package_monitor)
-        message = {"type": "package-ids", "ids": [None], "request-id": 1}
-        self.package_store.add_task("reporter", message)
+        self.createReporterTask()
+
+        # The server doesn't currently send 'package' scope, but we should
+        # support it in case we change that in the future.
+        package_scope = ["package"]
+        self.monitor.reactor.fire("resynchronize", package_scope)
+
+        self.assertSingleReporterTask({"type": "resynchronize"}, 2)
+
+    def test_resynchronize_gets_new_session_id(self):
+        """
+        When a 'resynchronize' reactor event is fired, the C{PackageMonitor}
+        acquires a new session ID (as the old one will be blocked).
+        """
+        self.monitor.add(self.package_monitor)
+        session_id = self.package_monitor._session_id
+        self.createReporterTask()
+
+        self.package_monitor.client.broker.message_store.drop_session_ids()
+        self.monitor.reactor.fire("resynchronize")
+        self.assertNotEqual(session_id, self.package_monitor._session_id)
+
+    def test_resynchronize_on_global_scope(self):
+        """
+        If a 'resynchronize' reactor event is fired with global scope (the
+        empty list) , the package monitor should act as if it were an event
+        with 'package' scope.
+        """
+        self.monitor.add(self.package_monitor)
+        self.createReporterTask()
 
         self.monitor.reactor.fire("resynchronize")
 
         # The next task should be the resynchronize message.
+<<<<<<< TREE
         task = self.package_store.get_next_task("reporter")
         self.assertEqual(task.data, {"type": "resynchronize"})
 
@@ -222,6 +278,24 @@
         task.remove()
         task = self.package_store.get_next_task("reporter")
         self.assertEqual(task, None)
+=======
+        self.assertSingleReporterTask({"type": "resynchronize"}, 2)
+
+    def test_not_resynchronize_with_other_scope(self):
+        """
+        If a 'resynchronize' reactor event is fired with an irrelevant scope,
+        the package monitor should not respond to this.
+        """
+        self.monitor.add(self.package_monitor)
+        self.createReporterTask()
+
+        disk_scope = ["disk"]
+        self.monitor.reactor.fire("resynchronize", disk_scope)
+
+        # The next task should *not* be the resynchronize message.
+        self.assertSingleReporterTask(
+            {'ids': [None], 'request-id': 1, 'type': 'package-ids'}, 1)
+>>>>>>> MERGE-SOURCE
 
     def test_spawn_reporter_doesnt_chdir(self):
         command = self.makeFile("#!/bin/sh\necho RUN\n")

=== modified file 'landscape/monitor/tests/test_plugin.py'
--- landscape/monitor/tests/test_plugin.py	2013-07-12 13:41:07 +0000
+++ landscape/monitor/tests/test_plugin.py	2013-07-16 13:16:33 +0000
@@ -65,6 +65,7 @@
         plugin.call_on_accepted("type", callback)
         self.reactor.fire(("message-type-acceptance-changed", "type"), False)
 
+<<<<<<< TREE
     def test_resynchronize_with_global_scope(self):
         """
         If a 'resynchronize' event fires with global scope, we clear down the
@@ -106,6 +107,51 @@
         self.reactor.fire("resynchronize", scopes=["chrutfup"])
         self.assertEqual(self.monitor.persist.get("wubble"), {"hi": "there"})
 
+=======
+    def test_resynchronize_with_global_scope(self):
+        """
+        If a 'resynchronize' event fires with global scope, we clear down the
+        persist.
+        """
+        plugin = MonitorPlugin()
+        plugin.persist_name = "wubble"
+        plugin.register(self.monitor)
+        plugin.persist.set("hi", "there")
+        self.assertEqual(self.monitor.persist.get("wubble"), {"hi": "there"})
+        self.reactor.fire("resynchronize")
+        self.assertIsNone(self.monitor.persist.get("wubble"))
+
+    def test_resynchronize_with_provided_scope(self):
+        """
+        If a 'resynchronize' event fires with the provided scope, we clear down
+        the persist.
+        """
+        plugin = MonitorPlugin()
+        plugin.persist_name = "wubble"
+        plugin.scope = "frujical"
+        plugin.register(self.monitor)
+        plugin.persist.set("hi", "there")
+        self.assertEqual(self.monitor.persist.get("wubble"), {"hi": "there"})
+        frujical_scope = ["frujical"]
+        self.reactor.fire("resynchronize", frujical_scope)
+        self.assertIsNone(self.monitor.persist.get("wubble"))
+
+    def test_do_not_resynchronize_with_other_scope(self):
+        """
+        If a 'resynchronize' event fires with an irrelevant scope, we do
+        nothing.
+        """
+        plugin = MonitorPlugin()
+        plugin.persist_name = "wubble"
+        plugin.scope = "frujical"
+        plugin.register(self.monitor)
+        plugin.persist.set("hi", "there")
+        self.assertEqual(self.monitor.persist.get("wubble"), {"hi": "there"})
+        chrutfup_scope = ["chrutfup"]
+        self.reactor.fire("resynchronize", chrutfup_scope)
+        self.assertEqual(self.monitor.persist.get("wubble"), {"hi": "there"})
+
+>>>>>>> MERGE-SOURCE
 
 class StubDataWatchingPlugin(DataWatcher):
 

=== modified file 'landscape/monitor/tests/test_processorinfo.py'
--- landscape/monitor/tests/test_processorinfo.py	2013-07-12 13:41:07 +0000
+++ landscape/monitor/tests/test_processorinfo.py	2013-07-16 13:16:33 +0000
@@ -53,7 +53,12 @@
         plugin = ProcessorInfo()
         self.monitor.add(plugin)
         plugin.run()
+<<<<<<< TREE
         self.reactor.fire("resynchronize", scopes=["cpu"])
+=======
+        cpu_scope = ["cpu"]
+        self.reactor.fire("resynchronize", cpu_scope)
+>>>>>>> MERGE-SOURCE
         plugin.run()
         messages = self.mstore.get_pending_messages()
         self.assertEqual(len(messages), 2)

=== modified file 'landscape/monitor/tests/test_rebootrequired.py'
--- landscape/monitor/tests/test_rebootrequired.py	2013-07-12 13:41:07 +0000
+++ landscape/monitor/tests/test_rebootrequired.py	2013-07-16 13:16:33 +0000
@@ -117,7 +117,12 @@
         data.
         """
         self.plugin.run()
+<<<<<<< TREE
         self.reactor.fire("resynchronize", scopes=["package"])
+=======
+        package_scope = ["package"]
+        self.reactor.fire("resynchronize", package_scope)
+>>>>>>> MERGE-SOURCE
         self.plugin.run()
         messages = self.mstore.get_pending_messages()
         self.assertEqual(len(messages), 2)

=== modified file 'landscape/monitor/tests/test_swiftdeviceinfo.py'
--- landscape/monitor/tests/test_swiftdeviceinfo.py	2013-07-12 13:41:07 +0000
+++ landscape/monitor/tests/test_swiftdeviceinfo.py	2013-07-16 13:16:33 +0000
@@ -136,7 +136,12 @@
 
         plugin.run()
         plugin.exchange()
+<<<<<<< TREE
         self.reactor.fire("resynchronize", scopes=["storage"])
+=======
+        storage_scope = ["storage"]
+        self.reactor.fire("resynchronize", storage_scope)
+>>>>>>> MERGE-SOURCE
         plugin.run()
         plugin.exchange()
         messages = self.mstore.get_pending_messages()

=== modified file 'landscape/monitor/tests/test_updatemanager.py'
--- landscape/monitor/tests/test_updatemanager.py	2013-07-12 13:41:07 +0000
+++ landscape/monitor/tests/test_updatemanager.py	2013-07-16 13:16:33 +0000
@@ -103,7 +103,12 @@
         data.
         """
         self.plugin.run()
+<<<<<<< TREE
         self.reactor.fire("resynchronize", scopes=["package"])
+=======
+        package_scope = ["package"]
+        self.reactor.fire("resynchronize", package_scope)
+>>>>>>> MERGE-SOURCE
         self.plugin.run()
         messages = self.mstore.get_pending_messages()
         self.assertEqual(len(messages), 2)

=== modified file 'landscape/monitor/tests/test_usermonitor.py'
--- landscape/monitor/tests/test_usermonitor.py	2013-07-16 09:01:54 +0000
+++ landscape/monitor/tests/test_usermonitor.py	2013-07-16 13:16:33 +0000
@@ -76,6 +76,7 @@
 
     def test_wb_resynchronize_event(self):
         """
+<<<<<<< TREE
         When a C{resynchronize} event, with 'users' scope, occurs any cached
         L{UserChange} snapshots should be cleared and a new message with users
         generated.
@@ -219,6 +220,114 @@
 #                                 "work-phone": None}],
 #               "type": "users"}])
 # >>>>>>> MERGE-SOURCE
+=======
+        When a C{resynchronize} event occurs any cached L{UserChange}
+        snapshots should be cleared and a new message with users generated.
+        """
+        self.provider.users = [("jdoe", "x", 1000, 1000, "JD,,,,",
+                                "/home/jdoe", "/bin/sh")]
+        self.provider.groups = [("webdev", "x", 1000, ["jdoe"])]
+        self.broker_service.message_store.set_accepted_types(["users"])
+        self.monitor.add(self.plugin)
+        self.successResultOf(self.plugin.run())
+
+        persist = self.plugin._persist
+        self.assertTrue(persist.get("users"))
+        self.assertTrue(persist.get("groups"))
+        self.assertMessages(
+            self.broker_service.message_store.get_pending_messages(),
+            [{"create-group-members": {u"webdev":[u"jdoe"]},
+              "create-groups": [{"gid": 1000, "name": u"webdev"}],
+              "create-users": [{"enabled": True, "home-phone": None,
+                                "location": None, "name": u"JD",
+                                "primary-gid": 1000, "uid": 1000,
+                                "username": u"jdoe", "work-phone": None}],
+              "type": "users"}])
+
+        # Clear all the messages from the message store
+        self.broker_service.message_store.delete_all_messages()
+
+        self.monitor.reactor.fire("resynchronize")
+
+        self.successResultOf(self.plugin.run())
+        self.assertMessages(
+            self.broker_service.message_store.get_pending_messages(),
+            [{"create-group-members": {u"webdev":[u"jdoe"]},
+              "create-groups": [{"gid": 1000, "name": u"webdev"}],
+              "create-users": [{"enabled": True, "home-phone": None,
+                                "location": None, "name": u"JD",
+                                "primary-gid": 1000, "uid": 1000,
+                                "username": u"jdoe",
+                                "work-phone": None}],
+              "type": "users"}])
+
+    def test_wb_resynchronize_event_with_global_scope(self):
+        """
+        When a C{resynchronize} event, with global scope, occurs we act exactly
+        as if it had 'users' scope.
+        """
+        self.provider.users = [("jdoe", "x", 1000, 1000, "JD,,,,",
+                                "/home/jdoe", "/bin/sh")]
+        self.provider.groups = [("webdev", "x", 1000, ["jdoe"])]
+        self.broker_service.message_store.set_accepted_types(["users"])
+        self.monitor.add(self.plugin)
+        self.successResultOf(self.plugin.run())
+        persist = self.plugin._persist
+        self.assertTrue(persist.get("users"))
+        self.assertTrue(persist.get("groups"))
+        self.assertMessages(
+                self.broker_service.message_store.get_pending_messages(),
+                [{"create-group-members": {u"webdev":[u"jdoe"]},
+                  "create-groups": [{"gid": 1000, "name": u"webdev"}],
+                  "create-users": [{"enabled": True, "home-phone": None,
+                                    "location": None, "name": u"JD",
+                                    "primary-gid": 1000, "uid": 1000,
+                                    "username": u"jdoe", "work-phone": None}],
+                                    "type": "users"}])
+        self.broker_service.message_store.delete_all_messages()
+        deferred = self.monitor.reactor.fire("resynchronize")[0]
+        self.successResultOf(deferred)
+        self.assertMessages(
+            self.broker_service.message_store.get_pending_messages(),
+            [{"create-group-members": {u"webdev":[u"jdoe"]},
+              "create-groups": [{"gid": 1000, "name": u"webdev"}],
+              "create-users": [{"enabled": True, "home-phone": None,
+                                "location": None, "name": u"JD",
+                                "primary-gid": 1000, "uid": 1000,
+                                "username": u"jdoe",
+                                "work-phone": None}],
+              "type": "users"}])
+
+    def test_do_not_resynchronize_with_other_scope(self):
+        """
+        When a C{resynchronize} event, with an irrelevant scope, occurs we do
+        nothing.
+        """
+        self.provider.users = [("jdoe", "x", 1000, 1000, "JD,,,,",
+                                "/home/jdoe", "/bin/sh")]
+        self.provider.groups = [("webdev", "x", 1000, ["jdoe"])]
+        self.broker_service.message_store.set_accepted_types(["users"])
+        self.monitor.add(self.plugin)
+        self.successResultOf(self.plugin.run())
+        persist = self.plugin._persist
+        self.assertTrue(persist.get("users"))
+        self.assertTrue(persist.get("groups"))
+        self.assertMessages(
+                self.broker_service.message_store.get_pending_messages(),
+                [{"create-group-members": {u"webdev":[u"jdoe"]},
+                  "create-groups": [{"gid": 1000, "name": u"webdev"}],
+                  "create-users": [{"enabled": True, "home-phone": None,
+                                    "location": None, "name": u"JD",
+                                    "primary-gid": 1000, "uid": 1000,
+                                    "username": u"jdoe", "work-phone": None}],
+                  "type": "users"}])
+        self.broker_service.message_store.delete_all_messages()
+        disk_scope = ["disk"]
+        self.monitor.reactor.fire("resynchronize", disk_scope)[0]
+        self.assertMessages(
+            self.broker_service.message_store.get_pending_messages(),
+            [])
+>>>>>>> MERGE-SOURCE
 
     def test_run(self):
         """