Fix race during session creation

Concurrent VIM API invocations after session expiration would result
in concurrent login attempts. In such cases, if a login attempt is
made after a successful one, it would result in InvalidLogin. This
is because vCenter server won't allow login within an active session.
On the other hand, if two or more login attempts reach vCenter at
the same, it would result in session leak-- the last one returned
overwrite any of the previous sessions. This patch prevents the
race condition by making session creation synchronized and checking
for an active session before login attempt. We can also remove the
call to terminate previous session after login since a login is
attempted only if there is no active session.

Change-Id: Ib4ca3553ce14c80ab722092907d797767072741c
Closes-Bug: #1409014

oslo_vmware/api.py

@@ -23,6 +23,7 @@
 
 import logging
 
+from oslo_concurrency import lockutils
 import six
 
 from oslo.utils import excutils
@@ -211,15 +212,23 @@ def pbm(self):
         return self._pbm
 
     @RetryDecorator(exceptions=(exceptions.VimConnectionException,))
+    @lockutils.synchronized('oslo_vmware_api_lock')
     def _create_session(self):
         """Establish session with the server."""
+        # Another thread might have created the session while the current one
+        # was waiting for the lock.
+        if self._session_id and self.is_current_session_active():
+            LOG.debug("Current session: %s is active.",
+                      _trunc_id(self._session_id))
+            return
+
         session_manager = self.vim.service_content.sessionManager
         # Login and create new session with the server for making API calls.
         LOG.debug("Logging in with username = %s.", self._server_username)
         session = self.vim.Login(session_manager,
                                  userName=self._server_username,
                                  password=self._server_password)
-        prev_session_id, self._session_id = self._session_id, session.key
+        self._session_id = session.key
         # We need to save the username in the session since we may need it
         # later to check active session. The SessionIsActive method requires
         # the username parameter to be exactly same as that in the session
@@ -230,25 +239,6 @@ def _create_session(self):
                      "%s."),
                  _trunc_id(self._session_id))
 
-        # Terminate the previous session (if exists) for preserving sessions
-        # as there is a limit on the number of sessions we can have.
-        if prev_session_id:
-            try:
-                LOG.info(_LI("Terminating the previous session with ID = %s"),
-                         _trunc_id(prev_session_id))
-                self.vim.TerminateSession(session_manager,
-                                          sessionId=[prev_session_id])
-            except Exception:
-                # This exception is something we can live with. It is
-                # just an extra caution on our side. The session might
-                # have been cleared already. We could have made a call to
-                # SessionIsActive, but that is an overhead because we
-                # anyway would have to call TerminateSession.
-                LOG.warn(_LW("Error occurred while terminating the previous "
-                             "session with ID = %s."),
-                         _trunc_id(prev_session_id),
-                         exc_info=True)
-
         # Set PBM client cookie.
         if self._pbm is not None:
             self._pbm.set_soap_cookie(self._vim.get_http_cookie())

oslo_vmware/tests/test_api.py

@@ -170,25 +170,44 @@ def test_create_session(self):
         self.assertEqual(session.key, api_session._session_id)
         pbm.set_soap_cookie.assert_called_once_with(cookie)
 
-    def test_create_session_with_existing_session(self):
+    def test_create_session_with_existing_inactive_session(self):
         old_session_key = '12345'
         new_session_key = '67890'
         session = mock.Mock()
         session.key = new_session_key
         api_session = self._create_api_session(False)
         api_session._session_id = old_session_key
+        api_session._session_username = api_session._server_username
         vim_obj = api_session.vim
         vim_obj.Login.return_value = session
+        vim_obj.SessionIsActive.return_value = False
 
         api_session._create_session()
         session_manager = vim_obj.service_content.sessionManager
+        vim_obj.SessionIsActive.assert_called_once_with(
+            session_manager, sessionID=old_session_key,
+            userName=VMwareAPISessionTest.USERNAME)
         vim_obj.Login.assert_called_once_with(
             session_manager, userName=VMwareAPISessionTest.USERNAME,
             password=VMwareAPISessionTest.PASSWORD)
-        vim_obj.TerminateSession.assert_called_once_with(
-            session_manager, sessionId=[old_session_key])
         self.assertEqual(new_session_key, api_session._session_id)
 
+    def test_create_session_with_existing_active_session(self):
+        old_session_key = '12345'
+        api_session = self._create_api_session(False)
+        api_session._session_id = old_session_key
+        api_session._session_username = api_session._server_username
+        vim_obj = api_session.vim
+        vim_obj.SessionIsActive.return_value = True
+
+        api_session._create_session()
+        session_manager = vim_obj.service_content.sessionManager
+        vim_obj.SessionIsActive.assert_called_once_with(
+            session_manager, sessionID=old_session_key,
+            userName=VMwareAPISessionTest.USERNAME)
+        self.assertFalse(vim_obj.Login.called)
+        self.assertEqual(old_session_key, api_session._session_id)
+
     def test_invoke_api(self):
         api_session = self._create_api_session(True)
         response = mock.Mock()

requirements.txt

@@ -24,3 +24,4 @@ suds>=0.4
 eventlet>=0.15.2
 requests>=2.2.0,!=2.4.0
 urllib3>=1.8.3
+oslo.concurrency>=0.3.0,!=0.4.0         # Apache-2.0

tests/test_api.py

@@ -169,25 +169,44 @@ def test_create_session(self):
         self.assertEqual(session.key, api_session._session_id)
         pbm.set_soap_cookie.assert_called_once_with(cookie)
 
-    def test_create_session_with_existing_session(self):
+    def test_create_session_with_existing_inactive_session(self):
         old_session_key = '12345'
         new_session_key = '67890'
         session = mock.Mock()
         session.key = new_session_key
         api_session = self._create_api_session(False)
         api_session._session_id = old_session_key
+        api_session._session_username = api_session._server_username
         vim_obj = api_session.vim
         vim_obj.Login.return_value = session
+        vim_obj.SessionIsActive.return_value = False
 
         api_session._create_session()
         session_manager = vim_obj.service_content.sessionManager
+        vim_obj.SessionIsActive.assert_called_once_with(
+            session_manager, sessionID=old_session_key,
+            userName=VMwareAPISessionTest.USERNAME)
         vim_obj.Login.assert_called_once_with(
             session_manager, userName=VMwareAPISessionTest.USERNAME,
             password=VMwareAPISessionTest.PASSWORD)
-        vim_obj.TerminateSession.assert_called_once_with(
-            session_manager, sessionId=[old_session_key])
         self.assertEqual(new_session_key, api_session._session_id)
 
+    def test_create_session_with_existing_active_session(self):
+        old_session_key = '12345'
+        api_session = self._create_api_session(False)
+        api_session._session_id = old_session_key
+        api_session._session_username = api_session._server_username
+        vim_obj = api_session.vim
+        vim_obj.SessionIsActive.return_value = True
+
+        api_session._create_session()
+        session_manager = vim_obj.service_content.sessionManager
+        vim_obj.SessionIsActive.assert_called_once_with(
+            session_manager, sessionID=old_session_key,
+            userName=VMwareAPISessionTest.USERNAME)
+        self.assertFalse(vim_obj.Login.called)
+        self.assertEqual(old_session_key, api_session._session_id)
+
     def test_invoke_api(self):
         api_session = self._create_api_session(True)
         response = mock.Mock()