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

[Geoff Teale]

This branch makes the keystone token plugin only respond
to resynchronize-clients events when they are either
"openstack" or global scope. Note, the server doesn't
currently send "openstack" scope, but I decided that all
plugins should have their scope defined so that we can
easily add scoped resynchs on the server when we need to.

This branch makes the keystone token plugin only respond to resynchronize-clients events when they are either "openstack" or global scope. Note, the server doesn't currently send "openstack" scope, but I decided that all plugins should have their scope defined so that we can easily add scoped resynchs on the server when we need to.

Note - this branch has now been re-based on the centralise-resynch-handling and will need re-review on that basis.

[Jerry Seutter (jseutter)]

+1, looks good.

review: Approve

[Jerry Seutter (jseutter)]

Some lintian issues:

landscape/manager/tests/test_keystonetoken.py:9:1: E303 too many blank lines (3)
landscape/manager/tests/test_keystonetoken.py:125:80: E501 line too long (87 characters)
landscape/broker/tests/test_server.py:368:80: E501 line too long (80 characters)

[Geoff Teale (tealeg)]

> Some lintian issues:
>
> landscape/manager/tests/test_keystonetoken.py:9:1: E303 too many blank lines
> (3)
> landscape/manager/tests/test_keystonetoken.py:125:80: E501 line too long (87
> characters)
> landscape/broker/tests/test_server.py:368:80: E501 line too long (80
> characters)

Cheers, all dealt with.

[Chris Glass (tribaal)]

[1]
+class FakePersist(object):
+
+ def __init__(self):
+ self.called = False
+
+ def remove(self, key):
+ self.called = True

This should probably go to someplace else, like landscape/lib/persist or landscape/tests/helpers as I assume it can be reused by all other plugins using the persist mechanism?

(ongoing review, saving just because I'm afraid chromium will die on me again)

[Chris Glass (tribaal)]

Alright, +1!

review: Approve

[Geoff Teale (tealeg)]

Chris, I addressed your previous review point.

[Chris Glass (tribaal)]

Looks good! Thanks!

review: Approve

[Jerry Seutter (jseutter)]

+1

landscape/tests/helpers.py:655:1: W391 blank line at end of file

review: Approve

[🤖 Landscape Builder (landscape-builder)]

No proposals found for merge of https://code.launchpad.net/~tealeg/landscape-client/centralise-resynch-handling into https://code.launchpad.net/~landscape/landscape-client/trunk.

=== modified file 'landscape/manager/keystonetoken.py'
--- landscape/manager/keystonetoken.py	2013-01-25 17:28:02 +0000
+++ landscape/manager/keystonetoken.py	2013-07-17 07:49:27 +0000
@@ -17,6 +17,7 @@
     message_type = "keystone-token"
     message_key = "data"
     run_interval = 60 * 15
+    scope = "openstack"
 
     def __init__(self, keystone_config_file=KEYSTONE_CONFIG_FILE):
         self._keystone_config_file = keystone_config_file
@@ -26,13 +27,12 @@
         self._persist_filename = os.path.join(self.registry.config.data_path,
                                               "keystone.bpickle")
         self._persist = Persist(filename=self._persist_filename)
-        self.registry.reactor.call_on("resynchronize", self._resynchronize)
         self.registry.reactor.call_every(self.registry.config.flush_interval,
                                          self.flush)
 
-    def _resynchronize(self):
+    def _reset(self):
         """
-        Recreate the persist upon C{_resynchronize}.
+        Reset the persist.
         """
         self._persist.remove("data")
 

=== modified file 'landscape/manager/tests/test_keystonetoken.py'
--- landscape/manager/tests/test_keystonetoken.py	2013-01-25 17:28:02 +0000
+++ landscape/manager/tests/test_keystonetoken.py	2013-07-17 07:49:27 +0000
@@ -2,7 +2,7 @@
 from landscape.tests.helpers import LandscapeTest
 
 from landscape.manager.keystonetoken import KeystoneToken
-from landscape.tests.helpers import ManagerHelper
+from landscape.tests.helpers import ManagerHelper, FakePersist
 
 
 class KeystoneTokenTest(LandscapeTest):
@@ -88,20 +88,49 @@
         self.reactor.advance(flush_interval)
         self.assertTrue(os.path.exists(persist_filename))
 
-    def test_resynchronize_message_calls_resynchronize_method(self):
-        """
-        If the reactor fires a "resynchronize" even the C{_resynchronize}
-        method on the keystone plugin object is called.
-        """
-        self.called = False
-
-        def stub_resynchronize():
-            self.called = True
-        self.plugin._resynchronize = stub_resynchronize
-
-        self.manager.add(self.plugin)
-        self.reactor.fire("resynchronize")
-        self.assertTrue(self.called)
+    def test_resynchronize_message_calls_reset_method(self):
+        """
+        If the reactor fires a "resynchronize", with 'openstack' scope, the
+        C{_reset} method on the keystone plugin object is called.
+        """
+        self.manager.add(self.plugin)
+        self.plugin._persist = FakePersist()
+        openstack_scope = ["openstack"]
+        self.reactor.fire("resynchronize", openstack_scope)
+        self.assertTrue(self.plugin._persist.called)
+
+    def test_resynchronize_gets_new_session_id(self):
+        """
+        If L{KeystoneToken} reacts to a "resynchronize" event it should get a
+        new session id as part of the process.
+        """
+        self.manager.add(self.plugin)
+        session_id = self.plugin._session_id
+        self.plugin._persist = FakePersist()
+        self.plugin.client.broker.message_store.drop_session_ids()
+        self.reactor.fire("resynchronize")
+        self.assertNotEqual(session_id, self.plugin._session_id)
+
+    def test_resynchronize_with_global_scope(self):
+        """
+        If the reactor fires a "resynchronize", with global scope, we act as if
+        it had 'openstack' scope.
+        """
+        self.manager.add(self.plugin)
+        self.plugin._persist = FakePersist()
+        self.reactor.fire("resynchronize")
+        self.assertTrue(self.plugin._persist.called)
+
+    def test_do_not_resynchronize_with_other_scope(self):
+        """
+        If the reactor fires a "resynchronize", with an irrelevant scope, we do
+        nothing.
+        """
+        self.manager.add(self.plugin)
+        self.plugin._persist = FakePersist()
+        disk_scope = ["disk"]
+        self.reactor.fire("resynchronize", disk_scope)
+        self.assertFalse(self.plugin._persist.called)
 
     def test_send_message_with_no_data(self):
         """

=== modified file 'landscape/tests/helpers.py'
--- landscape/tests/helpers.py	2013-04-30 14:02:42 +0000
+++ landscape/tests/helpers.py	2013-07-17 07:49:27 +0000
@@ -639,3 +639,16 @@
         """Remove sample data for the process that matches C{process_id}."""
         process_dir = os.path.join(self._sample_dir, str(process_id))
         shutil.rmtree(process_dir)
+
+
+class FakePersist(object):
+    """
+    Incompletely fake a C{landscape.lib.Persist} to simplify higher level tests
+    that result in an attempt to clear down persisted data.
+    """
+
+    def __init__(self):
+        self.called = False
+
+    def remove(self, key):
+        self.called = True