Merge lp:~fcorrea/landscape-client/wait-for-valid-session-id-on-resynchronize into lp:~landscape/landscape-client/trunk

[Fernando Correa Neto]

Changes the user monitor plugin to wait on a valid session id.

With the scope resync work, the user monitor plugin stopped working.
Since now we rely on a valid session id in order to send a message to the server, the user monitor plugin must wait until it gets a valid session.
The fix was provided by Free and a test and some cleanups were added as well.

[Benji York (benji)]

Thanks for the branch, it looks good.

review: Approve

[Free Ekanayaka (free.ekanayaka)]

review: Needs Fixing

[lp:~fcorrea/landscape-client/wait-for-valid-session-id-on-resynchronize]

828. By Fernando Correa Neto on 2016-01-15

- fix the unit test so it can actually fail if the fix is removed.
- improve test coveration for BrokerClientPlugin base class

[Fernando Correa Neto (fcorrea)]

Free

Thanks for reviewing it.
I've changed the test as we discussed to make sure it will fail when the fix is reverted. Please, take a look.

[Free Ekanayaka (free.ekanayaka)]

+1 with a nit

review: Approve

[lp:~fcorrea/landscape-client/wait-for-valid-session-id-on-resynchronize]

829. By Fernando Correa Neto on 2016-01-15

- rename test and address review comment

[Chad Smith (chad.smith)]

Good work here and additional unit tests. It took a while for me to go through upgrade path testing w/ broken released client TO patched trunk client.

Looks like a plain upgrade of a broken existing released client will still sit missing the initial delta of all system users. But, those broken clients will properly send any users or groups that were modified after the initial registration of that client.

To truly "fix" the broken clients, the customer would have to upgrade landscape-client and click the resynch button for that computer as a support person.

+1. This was tough.

review: Approve

=== modified file 'landscape/broker/client.py'
--- landscape/broker/client.py	2013-07-25 13:54:29 +0000
+++ landscape/broker/client.py	2016-01-15 16:29:39 +0000
@@ -1,6 +1,6 @@
 from logging import info, exception
 
-from twisted.internet.defer import maybeDeferred
+from twisted.internet.defer import maybeDeferred, succeed
 
 from landscape.log import format_object
 from landscape.lib.twisted_util import gather_results
@@ -67,7 +67,7 @@
         """
         if not (scopes is None or self.scope in scopes):
             # This resynchronize event is out of scope for us. Do nothing
-            return
+            return succeed(None)
 
         # Because the broker will drop session IDs already associated to scope
         # of the resynchronisation, it isn't safe to send messages until the

=== modified file 'landscape/broker/tests/test_client.py'
--- landscape/broker/tests/test_client.py	2013-07-19 09:57:57 +0000
+++ landscape/broker/tests/test_client.py	2016-01-15 16:29:39 +0000
@@ -47,6 +47,17 @@
         self.client.add(plugin)
         self.assertEqual(test_session_id, plugin._session_id)
 
+    def test_resynchronizing_out_of_scope(self):
+        """
+        When a 'reysnchronize' event happens and the plugin scope is not part
+        of the scopes that were passed, BrokerClientPlugin succeeds.
+        """
+        plugin = BrokerClientPlugin()
+        plugin.scope = "foo"
+        self.client.add(plugin)
+        deferred = self.client_reactor.fire("resynchronize", scopes=["bar"])[0]
+        self.assertIsNone(self.successResultOf(deferred))
+
     def test_resynchronizing_refreshes_session_id(self):
         """
         When a 'reysnchronize' event fires a new session ID is acquired as the

=== modified file 'landscape/monitor/tests/test_usermonitor.py'
--- landscape/monitor/tests/test_usermonitor.py	2016-01-11 18:34:49 +0000
+++ landscape/monitor/tests/test_usermonitor.py	2016-01-15 16:29:39 +0000
@@ -23,13 +23,13 @@
         def got_result(result):
             self.assertMessages(
                 self.broker_service.message_store.get_pending_messages(),
-                [{"create-group-members": {u"webdev":[u"jdoe"]},
+                [{"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"}])
+                  "type": "users"}])
             plugin.stop()
 
         users = [("jdoe", "x", 1000, 1000, "JD,,,,", "/home/jdoe", "/bin/sh")]
@@ -91,20 +91,20 @@
         self.assertTrue(persist.get("groups"))
         self.assertMessages(
                 self.broker_service.message_store.get_pending_messages(),
-                [{"create-group-members": {u"webdev":[u"jdoe"]},
+                [{"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"}])
+                  "type": "users"}])
         self.broker_service.message_store.delete_all_messages()
         deferred = self.monitor.reactor.fire(
             "resynchronize", scopes=["users"])[0]
         self.successResultOf(deferred)
         self.assertMessages(
             self.broker_service.message_store.get_pending_messages(),
-            [{"create-group-members": {u"webdev":[u"jdoe"]},
+            [{"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",
@@ -113,6 +113,29 @@
                                 "work-phone": None}],
               "type": "users"}])
 
+    def test_new_message_after_resynchronize_event(self):
+        """
+        When a 'resynchronize' reactor event is fired, a new session is
+        created and the UserMonitor creates a new message.
+        """
+        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.plugin.client.broker.message_store.drop_session_ids()
+        deferred = self.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_wb_resynchronize_event_with_global_scope(self):
         """
         When a C{resynchronize} event, with global scope, occurs we act exactly
@@ -129,19 +152,19 @@
         self.assertTrue(persist.get("groups"))
         self.assertMessages(
                 self.broker_service.message_store.get_pending_messages(),
-                [{"create-group-members": {u"webdev":[u"jdoe"]},
+                [{"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"}])
+                  "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-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",
@@ -166,7 +189,7 @@
         self.assertTrue(persist.get("groups"))
         self.assertMessages(
                 self.broker_service.message_store.get_pending_messages(),
-                [{"create-group-members": {u"webdev":[u"jdoe"]},
+                [{"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",
@@ -189,13 +212,13 @@
         def got_result(result):
             self.assertMessages(
                 self.broker_service.message_store.get_pending_messages(),
-                [{"create-group-members": {u"webdev":[u"jdoe"]},
+                [{"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"}])
+                  "type": "users"}])
 
         self.provider.users = [("jdoe", "x", 1000, 1000, "JD,,,,",
                                 "/home/jdoe", "/bin/sh")]
@@ -230,14 +253,14 @@
         def got_result(result):
             self.assertMessages(
                 self.broker_service.message_store.get_pending_messages(),
-                [{"create-group-members": {u"webdev":[u"jdoe"]},
+                [{"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}],
-                                    "operation-id": 1001,
-                                    "type": "users"}])
+                  "operation-id": 1001,
+                  "type": "users"}])
 
         self.provider.users = [("jdoe", "x", 1000, 1000, "JD,,,,",
                                 "/home/jdoe", "/bin/sh")]
@@ -253,12 +276,12 @@
         def got_result(result):
             self.assertMessages(
                 self.broker_service.message_store.get_pending_messages(),
-                [{"create-group-members": {u"webdev":[u"jdoe"]},
+                [{"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}],
+                                    "username": u"jdoe", "work-phone": None}],
                   "type": "users"}])
 
         self.broker_service.message_store.set_accepted_types(["users"])
@@ -283,7 +306,7 @@
         def got_result(result):
             self.assertMessages(
                 self.broker_service.message_store.get_pending_messages(),
-                [{"create-group-members": {u"webdev":[u"jdoe"]},
+                [{"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",
@@ -332,8 +355,9 @@
 
         def got_result(result):
             mstore = self.broker_service.message_store
-            self.assertMessages(mstore.get_pending_messages(),
-                [{"create-group-members": {u"webdev":[u"jdoe"]},
+            self.assertMessages(
+                mstore.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",
@@ -376,7 +400,7 @@
         self.mocker.replay()
 
         self.provider.users = [("jdoe", "x", 1000, 1000, "JD,,,,",
-                       "/home/jdoe", "/bin/sh")]
+                                "/home/jdoe", "/bin/sh")]
         self.provider.groups = [("webdev", "x", 1000, ["jdoe"])]
         self.monitor.add(self.plugin)
         connector = RemoteUserMonitorConnector(self.reactor, self.config)

=== modified file 'landscape/monitor/usermonitor.py'
--- landscape/monitor/usermonitor.py	2016-01-11 18:34:49 +0000
+++ landscape/monitor/usermonitor.py	2016-01-15 16:29:39 +0000
@@ -39,10 +39,14 @@
             self._publisher.stop()
             self._publisher = None
 
-    def _reset(self):
+    def _resynchronize(self, scopes=None):
         """Reset user and group data."""
-        super(UserMonitor, self)._reset()
-        return self._run_detect_changes()
+        deferred = super(UserMonitor, self)._resynchronize(scopes=scopes)
+        # Wait for the superclass' asynchronous _resynchronize method to
+        # complete, so we have a new session ID at hand and we can craft a
+        # valid message (l.broker.client.BrokerClientPlugin._resynchronize).
+        deferred.addCallback(lambda _: self._run_detect_changes())
+        return deferred
 
     @remote
     def detect_changes(self, operation_id=None):