launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/async-cobbler-login into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/async-cobbler-login/+merge/90282

This makes the provisioning server's login to Cobbler asynchronous.

The session can still be shared for multiple concurrent requests, and the existing optimistic re-authentication check still detects concurrent re-authentications.

I couldn't think of a usefully simple way to test for concurrency hazards.  In a nutshell, there's a lock around authentication.  Any re-authentication *from the time you notice* that you need to re-authenticate, up to a time when you already have that lock and can request authentication, will stop a redundant re-authentication.  And that's how different ongoing requests still back off from overwriting each other's auth tokens.

This does mean that authentication returns a Deferred now.

Jeroen
-- 
https://code.launchpad.net/~jtv/maas/async-cobbler-login/+merge/90282
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/async-cobbler-login into lp:maas.

=== modified file 'src/provisioningserver/cobblerclient.py'
--- src/provisioningserver/cobblerclient.py	2012-01-26 11:35:34 +0000
+++ src/provisioningserver/cobblerclient.py	2012-01-26 16:18:36 +0000
@@ -24,6 +24,7 @@
 import xmlrpclib
 
 from twisted.internet.defer import (
+    DeferredLock,
     inlineCallbacks,
     returnValue,
     )
@@ -55,6 +56,7 @@
         self.proxy = self._make_twisted_proxy()
         self.token = None
         self.connection_count = 0
+        self.authentication_lock = DeferredLock()
 
     def _make_twisted_proxy(self):
         """Create a Twisted XMRLPC proxy.
@@ -76,14 +78,7 @@
         """
         return (self.connection_count, self.token)
 
-    def _login(self):
-        """Log in."""
-        # Doing this synchronously for simplicity.  The authentication token
-        # can be shared, making this a rare operation that multiple
-        # concurrent requests may be waiting on.
-        self.token = xmlrpclib.Server(self.url).login(
-            self.user, self.password)
-
+    @inlineCallbacks
     def authenticate(self, previous_state=None):
         """Log in synchronously.
 
@@ -97,11 +92,21 @@
             method will assume that a concurrent authentication request has
             completed and the failed request can be retried without logging
             in again.
+            If no `previous_state` is given, authentication will happen
+            regardless.
         """
-        if previous_state is None or self.record_state() == previous_state:
-            # No concurrent login has completed since we last issued a
-            # request that required authentication; try a fresh one.
-            self._login()
+        if previous_state is None:
+            previous_state = self.record_state()
+
+        yield self.authentication_lock.acquire()
+        try:
+            if self.record_state() == previous_state:
+                # No concurrent login has completed since we last issued a
+                # request that required authentication; try a fresh one.
+                self.token = yield self.proxy.callRemote(
+                    'login', self.user, self.password)
+        finally:
+            self.authentication_lock.release()
 
     def substitute_token(self, arg):
         """Return `arg`, or the current auth token for `token_placeholder`."""
@@ -149,7 +154,7 @@
                 raise
 
         if authentication_expired:
-            self.authenticate(original_state)
+            yield self.authenticate(original_state)
             result = yield self._issue_call(method, *args)
         returnValue(result)
 

=== modified file 'src/provisioningserver/tests/test_cobblersession.py'
--- src/provisioningserver/tests/test_cobblersession.py	2012-01-26 11:54:37 +0000
+++ src/provisioningserver/tests/test_cobblersession.py	2012-01-26 16:18:36 +0000
@@ -58,7 +58,8 @@
 
     Records XMLRPC calls, and returns predetermined values.
     """
-    def __init__(self):
+    def __init__(self, fake_token=None):
+        self.fake_token = fake_token
         self.calls = []
         self.return_values = None
 
@@ -71,6 +72,9 @@
 
     @inlineCallbacks
     def callRemote(self, method, *args):
+        if method == 'login':
+            returnValue(self.fake_token)
+
         self.calls.append((method, ) + args)
         if self.return_values:
             value = self.return_values.pop(0)
@@ -97,16 +101,13 @@
         will use for its proxy; and `fake_token` to provide a login token
         that the session should pretend it gets from the server on login.
         """
-        self.fake_proxy = RecordingFakeProxy()
-        self.fake_token = kwargs.pop('fake_token')
+        fake_token = kwargs.pop('fake_token')
+        self.fake_proxy = RecordingFakeProxy(fake_token)
         super(RecordingSession, self).__init__(*args, **kwargs)
 
     def _make_twisted_proxy(self):
         return self.fake_proxy
 
-    def _login(self):
-        self.token = self.fake_token
-
 
 class TestCobblerSession(TestCase):
     """Test session management against a fake XMLRPC session."""
@@ -135,11 +136,12 @@
             token=fake_token(user, 'not-yet-authenticated'))
         self.assertEqual(None, session.token)
 
+    @inlineCallbacks
     def test_authenticate_authenticates_initially(self):
         token = fake_token('authenticated')
         session = self.make_recording_session(token=token)
         self.assertEqual(None, session.token)
-        session.authenticate()
+        yield session.authenticate()
         self.assertEqual(token, session.token)
 
     @inlineCallbacks
@@ -150,12 +152,14 @@
         yield session.call("some_method")
         self.assertEqual(state, session.record_state())
 
+    @inlineCallbacks
     def test_authentication_changes_state_cookie(self):
         session = self.make_recording_session()
         old_cookie = session.record_state()
-        session.authenticate()
+        yield session.authenticate()
         self.assertNotEqual(old_cookie, session.record_state())
 
+    @inlineCallbacks
     def test_authenticate_backs_off_from_overwriting_concurrent_auth(self):
         session = self.make_recording_session()
         # Two requests are made concurrently.
@@ -163,12 +167,12 @@
         cookie_before_request_2 = session.record_state()
         # Request 1 comes back with an authentication failure, and its
         # callback refreshes the session's auth token.
-        session.authenticate(cookie_before_request_1)
+        yield session.authenticate(cookie_before_request_1)
         token_for_retrying_request_1 = session.token
         # Request 2 also comes back an authentication failure, and its
         # callback also asks the session to ensure that it is
         # authenticated.
-        session.authenticate(cookie_before_request_2)
+        yield session.authenticate(cookie_before_request_2)
         token_for_retrying_request_2 = session.token
 
         # The double authentication does not confuse the session; both
@@ -180,10 +184,11 @@
         self.assertNotEqual(cookie_before_request_1, session.token)
         self.assertNotEqual(cookie_before_request_2, session.token)
 
+    @inlineCallbacks
     def test_substitute_token_substitutes_only_placeholder(self):
         token = fake_token('for-substitution')
         session = self.make_recording_session(token=token)
-        session.authenticate()
+        yield session.authenticate()
         arbitrary_number = pick_number()
         arbitrary_string = 'string-%d' % pick_number()
         inputs = [

=== modified file 'src/provisioningserver/tests/test_fakecobbler.py'
--- src/provisioningserver/tests/test_fakecobbler.py	2012-01-26 10:06:41 +0000
+++ src/provisioningserver/tests/test_fakecobbler.py	2012-01-26 16:18:36 +0000
@@ -21,8 +21,13 @@
     FakeCobbler,
     FakeTwistedProxy,
     )
+from testtools.content import text_content
 from testtools.deferredruntest import AsynchronousDeferredRunTest
-from twisted.internet.defer import inlineCallbacks
+from testtools.testcase import ExpectedException
+from twisted.internet.defer import (
+    inlineCallbacks,
+    returnValue,
+    )
 
 
 class FakeCobblerSession(CobblerSession):
@@ -39,13 +44,14 @@
         self.token = self.proxy.fake_cobbler.login(self.user, self.password)
 
 
+@inlineCallbacks
 def fake_cobbler_session(url=None, user=None, password=None,
                          fake_cobbler=None):
     """Fake a CobblerSession."""
     session = FakeCobblerSession(
         url, user, password, fake_cobbler=fake_cobbler)
-    session.authenticate()
-    return session
+    yield session.authenticate()
+    returnValue(session)
 
 
 class TestFakeCobbler(TestCase):
@@ -56,17 +62,18 @@
 
     run_tests_with = AsynchronousDeferredRunTest.make_factory()
 
+    @inlineCallbacks
     def test_login_failure_raises_failure(self):
         cobbler = FakeCobbler(passwords={'moi': 'potahto'})
-        self.assertRaises(
-            Exception,
-            fake_cobbler_session,
-            user='moi', password='potayto', fake_cobbler=cobbler)
+        with ExpectedException(Exception):
+            return_value = yield fake_cobbler_session(
+                user='moi', password='potayto', fake_cobbler=cobbler)
+            self.addDetail('return_value', text_content(repr(return_value)))
 
     @inlineCallbacks
     def test_expired_token_triggers_retry(self):
         cobbler = FakeCobbler(passwords={'user': 'pw'})
-        session = fake_cobbler_session(
+        session = yield fake_cobbler_session(
             user='user', password='pw', fake_cobbler=cobbler)
         # When an auth token expires, the server just forgets about it.
         old_token = session.token
@@ -83,7 +90,7 @@
     @inlineCallbacks
     def test_valid_token_does_not_raise_auth_error(self):
         cobbler = FakeCobbler(passwords={'user': 'password'})
-        session = fake_cobbler_session(
+        session = yield fake_cobbler_session(
             user='user', password='password', fake_cobbler=cobbler)
         old_token = session.token
         yield CobblerSystem.new(session)