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.

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]

722. By Geoff Teale on 2013-07-16

merge forwards and resolve conflicts.

[Jerry Seutter (jseutter)]

+1

review: Approve

[Björn Tillenius (bjornt)]

Looks good, +1

[1]

+ 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)

Where does that dict on the last line come from? Ok, after reading the
diff I can see that it's from createReporterTask(), but if someone looks
at the test only, it's hard to see.

It would be better to either explicitly add that task you expect to be
there, or make createReportTask() return the task and id, so that it's
clear that you expect the existing task to still be there, and no a new
one.

review: Approve

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

723. By Geoff Teale on 2013-07-25

Make test more readable by returning the task from createReporterTask.

[Geoff Teale (tealeg)]

> Looks good, +1
>
> [1]
>

Agreed, I've tidied that up.

=== modified file 'landscape/monitor/packagemonitor.py'
--- landscape/monitor/packagemonitor.py	2013-07-12 08:18:35 +0000
+++ landscape/monitor/packagemonitor.py	2013-07-25 13:19:26 +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/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-25 13:19:26 +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}
+        return 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,28 +227,61 @@
 
     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.
-        task = self.package_store.get_next_task("reporter")
-        self.assertEqual(task.data, {"type": "resynchronize"})
-
-        # We want to make sure it is not the first message
-        self.assertNotEqual(task.id, 1)
-
-        # 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)
+        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)
+        task = self.createReporterTask()
+
+        disk_scope = ["disk"]
+        self.monitor.reactor.fire("resynchronize", disk_scope)
+
+        # The next task should *not* be the resynchronize message, but instead
+        # the original task we created.
+        self.assertSingleReporterTask(task.data, task.id)
 
     def test_spawn_reporter_doesnt_chdir(self):
         command = self.makeFile("#!/bin/sh\necho RUN\n")

=== 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-25 13:19:26 +0000
@@ -178,47 +178,6 @@
         self.assertMessages(
             self.broker_service.message_store.get_pending_messages(),
             [])
-# =======
-#         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"}])
-# >>>>>>> MERGE-SOURCE
 
     def test_run(self):
         """