Take into consideration races in XtremIOClient3

When working with FC and xtremio is using XtremIOClient3 on FC's
terminate_connection we can get VolumeBackendAPIException saying a
resource could not be found when we shouldn't.

The cause is that Client3 is open to race conditions on 2 methods,
find_lunmap and num_of_mapped_volumes, where a list of mappings is first
retrieved and then we iterate this list to retrieved additional info on
the mappings.

The race would happen if one of the mappings is removed from the backend
in the time it takes to retrieve the additional info after we have
retrieved the list.

This patch fixes this issue by ignoring any mappings that have been
removed and are now NotFound when retrieving additional information for
a mapping in those 2 methods.

This patch also fixes this kind of race problems on volume creation
since it uses find_lumap method.

Closes-Bug: #1521143
Change-Id: I40831e04093ff475395870a333211dd0cb60440f

diff --git a/cinder/tests/unit/test_emc_xtremio.py b/cinder/tests/unit/test_emc_xtremio.py
index 0676522129cfc65d0608748f53915623091e0b2f..4aba0fcda9b8f62a6a1b08cfff153041bdde58f7 100644 (file)
--- a/cinder/tests/unit/test_emc_xtremio.py
+++ b/cinder/tests/unit/test_emc_xtremio.py
@@ -563,22 +563,20 @@ class EMCXIODriverFibreChannelTestCase(test.TestCase):
         super(EMCXIODriverFibreChannelTestCase, self).setUp()
         clean_xms_data()
 
-        config = mock.Mock()
-        config.san_login = ''
-        config.san_password = ''
-        config.san_ip = ''
-        config.xtremio_cluster_name = ''
-        config.xtremio_provisioning_factor = 20.0
+        self.config = mock.Mock(san_login='',
+                                san_password='',
+                                san_ip='',
+                                xtremio_cluster_name='',
+                                xtremio_provisioning_factor=20.0)
         self.driver = xtremio.XtremIOFibreChannelDriver(
-            configuration=config)
-        self.driver.client = xtremio.XtremIOClient4(config,
-                                                    config.
-                                                    xtremio_cluster_name)
-
+            configuration=self.config)
         self.data = CommonData()
 
     def test_initialize_terminate_connection(self, req):
         req.side_effect = xms_request
+        self.driver.client = xtremio.XtremIOClient4(
+            self.config, self.config.xtremio_cluster_name)
+
         self.driver.create_volume(self.data.test_volume)
         map_data = self.driver.initialize_connection(self.data.test_volume,
                                                      self.data.connector)
@@ -586,3 +584,43 @@ class EMCXIODriverFibreChannelTestCase(test.TestCase):
         self.driver.terminate_connection(self.data.test_volume,
                                          self.data.connector)
         self.driver.delete_volume(self.data.test_volume)
+
+    def test_race_on_terminate_connection(self, req):
+        """Test for race conditions on num_of_mapped_volumes.
+
+        This test confirms that num_of_mapped_volumes won't break even if we
+        receive a NotFound exception when retrieving info on a specific
+        mapping, as that specific mapping could have been deleted between
+        the request to get the list of exiting mappings and the request to get
+        the info on one of them.
+        """
+        req.side_effect = xms_request
+        self.driver.client = xtremio.XtremIOClient3(
+            self.config, self.config.xtremio_cluster_name)
+        # We'll wrap num_of_mapped_volumes, we'll store here original method
+        original_method = self.driver.client.num_of_mapped_volumes
+
+        def fake_num_of_mapped_volumes(*args, **kwargs):
+            # Add a nonexistent mapping
+            mappings = [{'href': 'volumes/1'}, {'href': 'volumes/12'}]
+
+            # Side effects will be: 1st call returns the list, then we return
+            # data for existing mappings, and on the nonexistent one we added
+            # we return NotFound
+            side_effect = [{'lun-maps': mappings},
+                           {'content': xms_data['lun-maps'][1]},
+                           exception.NotFound]
+
+            with mock.patch.object(self.driver.client, 'req',
+                                   side_effect=side_effect):
+                return original_method(*args, **kwargs)
+
+        self.driver.create_volume(self.data.test_volume)
+        map_data = self.driver.initialize_connection(self.data.test_volume,
+                                                     self.data.connector)
+        self.assertEqual(1, map_data['data']['target_lun'])
+        with mock.patch.object(self.driver.client, 'num_of_mapped_volumes',
+                               side_effect=fake_num_of_mapped_volumes):
+            self.driver.terminate_connection(self.data.test_volume,
+                                             self.data.connector)
+        self.driver.delete_volume(self.data.test_volume)

diff --git a/cinder/volume/drivers/emc/xtremio.py b/cinder/volume/drivers/emc/xtremio.py
index 6de8ca191fb0003bdc32a5d2ad5e69c8d1658f28..aacc6ad27a4df292918dca2e3bca2239e50cd987 100644 (file)
--- a/cinder/volume/drivers/emc/xtremio.py
+++ b/cinder/volume/drivers/emc/xtremio.py
@@ -198,21 +198,35 @@ class XtremIOClient3(XtremIOClient):
 
     def find_lunmap(self, ig_name, vol_name):
         try:
-            for lm_link in self.req('lun-maps')['lun-maps']:
-                idx = lm_link['href'].split('/')[-1]
-                lm = self.req('lun-maps', idx=int(idx))['content']
-                if lm['ig-name'] == ig_name and lm['vol-name'] == vol_name:
-                    return lm
+            lun_mappings = self.req('lun-maps')['lun-maps']
         except exception.NotFound:
             raise (exception.VolumeDriverException
                    (_("can't find lun-map, ig:%(ig)s vol:%(vol)s") %
                     {'ig': ig_name, 'vol': vol_name}))
 
+        for lm_link in lun_mappings:
+            idx = lm_link['href'].split('/')[-1]
+            # NOTE(geguileo): There can be races so mapped elements retrieved
+            # in the listing may no longer exist.
+            try:
+                lm = self.req('lun-maps', idx=int(idx))['content']
+            except exception.NotFound:
+                continue
+            if lm['ig-name'] == ig_name and lm['vol-name'] == vol_name:
+                return lm
+
+        return None
+
     def num_of_mapped_volumes(self, initiator):
         cnt = 0
         for lm_link in self.req('lun-maps')['lun-maps']:
             idx = lm_link['href'].split('/')[-1]
-            lm = self.req('lun-maps', idx=int(idx))['content']
+            # NOTE(geguileo): There can be races so mapped elements retrieved
+            # in the listing may no longer exist.
+            try:
+                lm = self.req('lun-maps', idx=int(idx))['content']
+            except exception.NotFound:
+                continue
             if lm['ig-name'] == initiator:
                 cnt += 1
         return cnt