Cinder: Standardize volume GET calls

The logic to fetch a volume details is scattered all over which
makes it hard to read as well as maintain.
There are currently two ways we fetch volume details:
1. client.volumes.get
2. volume.manager.get

The first one being the correct way maintaining abstraction and
using the cinderclient for API calls.
The problem with current code is:
1. Hard to read and maintain
2. Any change in volume GET logic will require changes across
whole store
3. Not handling exceptions in a common place
4. vulnerable to bugs as issue in one place cannot be detected
easily by other code paths and needs to be isolately tested

This patch aims to standardize the volume GET call so we can
avoid the above stated issues.
This patch has been tested with the following operations massaging
every place modified in this patch:
1. Create image - _wait_volume_status
2. Create > 1 GB image
2.1 offline - _call_offline_extend
2.2 online - _call_online_extend
3. upload volume to image - is_image_associated_with_store

Depends-On: https://review.opendev.org/c/openstack/glance/+/962445

Signed-off-by: Rajat Dhasmana <rajatdhasmana@gmail.com>
Change-Id: I5a4bad40ea03b0d746aaad392781f1b0fe78c8bc

glance_store/_drivers/cinder/store.py

@@ -569,16 +569,20 @@ class Store(glance_store.driver.Store):
             # image-volume to modification by users which might lead
             # to corruption of image.
             try:
-                volume = cinder_client.volumes.get(volume_id)
-            except cinder_exception.NotFound:
-                reason = (_LW("Image-Volume %s not found. If you have "
-                              "upgraded your environment from single store "
-                              "to multi store, transfer all your "
-                              "Image-Volumes from user projects to service "
-                              "project."
-                          % volume_id))
-                LOG.warning(reason)
-                return False
+                volume = self.volume_api.get(cinder_client, volume_id)
+            # NotFound Exception is translated to BackendException
+            except exceptions.BackendException as e:
+                # 'Volume <vol-id> could not be found.
+                # (HTTP 404) (Request-ID: <req-id>)'
+                if 'HTTP 404' in str(e):
+                    reason = (_LW("Image-Volume %s not found. If you have "
+                                  "upgraded your environment from single "
+                                  "store to multi store, transfer all your "
+                                  "Image-Volumes from user projects to "
+                                  "service project."
+                              % volume_id))
+                    LOG.warning(reason)
+                    return False
             if cinder_volume_type and volume.volume_type == cinder_volume_type:
                 return True
             elif not cinder_volume_type:
@@ -687,10 +691,12 @@ class Store(glance_store.driver.Store):
             raise exceptions.BadStoreConfiguration(store_name="cinder",
                                                    reason=reason)
 
-    def _wait_volume_status(self, volume, status_transition, status_expected):
+    def _wait_volume_status(self, volume, status_transition, status_expected,
+                            client):
+        volume_id = volume.id
         max_recheck_wait = 15
         timeout = self.store_conf.cinder_state_transition_timeout
-        volume = volume.manager.get(volume.id)
+        volume = self.volume_api.get(client, volume_id)
         tries = 0
         elapsed = 0
         while volume.status == status_transition:
@@ -705,7 +711,7 @@ class Store(glance_store.driver.Store):
             time.sleep(wait)
             tries += 1
             elapsed += wait
-            volume = volume.manager.get(volume.id)
+            volume = self.volume_api.get(client, volume_id)
         if volume.status != status_expected:
             msg = (_('The status of volume %(volume_id)s is unexpected: '
                      'status = %(status)s, expected = %(expected)s.')
@@ -746,7 +752,7 @@ class Store(glance_store.driver.Store):
         LOG.debug('Attachment %(attachment_id)s created successfully.',
                   {'attachment_id': attachment['id']})
 
-        volume = volume.manager.get(volume_id)
+        volume = self.volume_api.get(client, volume_id)
         attachment_id = attachment['id']
         connection_info = None
         try:
@@ -853,7 +859,7 @@ class Store(glance_store.driver.Store):
         self._check_context(context)
         try:
             client = self.get_cinderclient(context, version='3.54')
-            volume = client.volumes.get(loc.volume_id)
+            volume = self.volume_api.get(client, loc.volume_id)
             size = int(volume.metadata.get('image_size',
                                            volume.size * units.Gi))
             iterator = self._cinder_volume_data_iterator(
@@ -896,7 +902,7 @@ class Store(glance_store.driver.Store):
                               "internal error."))
             return 0
 
-    def _call_offline_extend(self, volume, size_gb):
+    def _call_offline_extend(self, volume, size_gb, client):
         size_gb += 1
         LOG.debug("Extending (offline) volume %(volume_id)s to %(size)s GB.",
                   {'volume_id': volume.id, 'size': size_gb})
@@ -904,7 +910,8 @@ class Store(glance_store.driver.Store):
         try:
             volume = self._wait_volume_status(volume,
                                               'extending',
-                                              'available')
+                                              'available',
+                                              client)
             size_gb = volume.size
             return size_gb
         except exceptions.BackendException:
@@ -918,7 +925,8 @@ class Store(glance_store.driver.Store):
         try:
             volume = self._wait_volume_status(volume,
                                               'extending',
-                                              'in-use')
+                                              'in-use',
+                                              client)
             size_gb = volume.size
             return size_gb
         except exceptions.BackendException:
@@ -965,7 +973,7 @@ class Store(glance_store.driver.Store):
                 self._write_data(f, write_props)
             if write_props.need_extend:
                 write_props.size_gb = self._call_offline_extend(
-                    volume, write_props.size_gb)
+                    volume, write_props.size_gb, client)
 
     def _online_extend(self, client, volume, write_props):
         with self._open_cinder_volume(client, volume, 'wb') as f:
@@ -1048,7 +1056,8 @@ class Store(glance_store.driver.Store):
                                         metadata=metadata,
                                         volume_type=volume_type)
 
-        volume = self._wait_volume_status(volume, 'creating', 'available')
+        volume = self._wait_volume_status(volume, 'creating', 'available',
+                                          client)
         size_gb = volume.size
 
         failed = True

glance_store/common/cinder_utils.py

@@ -67,6 +67,15 @@ class API(object):
         volume = client.volumes.create(size, **kwargs)
         return volume
 
+    @handle_exceptions
+    def get(self, client, volume_id):
+        """Get the details about a volume given it's ID.
+
+        :param client: cinderclient object
+        :param volume_id: UUID of the volume to get
+        """
+        return client.volumes.get(volume_id)
+
     def delete(self, client, volume_id):
         client.volumes.delete(volume_id)
 

glance_store/tests/unit/cinder/test_cinder_base.py

@@ -163,43 +163,45 @@ class TestCinderStoreBase(object):
 
     @mock.patch.object(time, 'sleep')
     def test_wait_volume_status(self, mock_sleep):
-        fake_manager = mock.MagicMock(get=mock.Mock())
-        volume_available = mock.MagicMock(manager=fake_manager,
-                                          id='fake-id',
+        volume_available = mock.MagicMock(id='fake-id',
                                           status='available')
-        volume_in_use = mock.MagicMock(manager=fake_manager,
-                                       id='fake-id',
+        volume_in_use = mock.MagicMock(id='fake-id',
                                        status='in-use')
-        fake_manager.get.side_effect = [volume_available, volume_in_use]
+        fake_volumes = mock.MagicMock()
+        fake_volumes.get.side_effect = [volume_available, volume_in_use]
+        fake_client = mock.MagicMock(volumes=fake_volumes)
         self.assertEqual(volume_in_use,
                          self.store._wait_volume_status(
-                             volume_available, 'available', 'in-use'))
-        fake_manager.get.assert_called_with('fake-id')
+                             volume_available, 'available', 'in-use',
+                             fake_client))
+        fake_volumes.get.assert_called_with('fake-id')
         mock_sleep.assert_called_once_with(0.5)
 
     @mock.patch.object(time, 'sleep')
     def test_wait_volume_status_unexpected(self, mock_sleep):
-        fake_manager = mock.MagicMock(get=mock.Mock())
-        volume_available = mock.MagicMock(manager=fake_manager,
-                                          id='fake-id',
+        volume_available = mock.MagicMock(id='fake-id',
                                           status='error')
-        fake_manager.get.return_value = volume_available
+        fake_volumes = mock.MagicMock(
+            get=mock.Mock(return_value=volume_available))
+        fake_client = mock.MagicMock(volumes=fake_volumes)
         self.assertRaises(exceptions.BackendException,
                           self.store._wait_volume_status,
-                          volume_available, 'available', 'in-use')
-        fake_manager.get.assert_called_with('fake-id')
+                          volume_available, 'available', 'in-use',
+                          fake_client)
+        fake_volumes.get.assert_called_with('fake-id')
 
     @mock.patch.object(time, 'sleep')
     def test_wait_volume_status_timeout(self, mock_sleep):
-        fake_manager = mock.MagicMock(get=mock.Mock())
-        volume_available = mock.MagicMock(manager=fake_manager,
-                                          id='fake-id',
+        volume_available = mock.MagicMock(id='fake-id',
                                           status='available')
-        fake_manager.get.return_value = volume_available
+        fake_volumes = mock.MagicMock(
+            get=mock.Mock(return_value=volume_available))
+        fake_client = mock.MagicMock(volumes=fake_volumes)
         self.assertRaises(exceptions.BackendException,
                           self.store._wait_volume_status,
-                          volume_available, 'available', 'in-use')
-        fake_manager.get.assert_called_with('fake-id')
+                          volume_available, 'available', 'in-use',
+                          fake_client)
+        fake_volumes.get.assert_called_with('fake-id')
 
     def _test_open_cinder_volume(self, open_mode, attach_mode, error,
                                  multipath_supported=False,
@@ -209,7 +211,6 @@ class TestCinderStoreBase(object):
                                  update_attachment_error=None):
         fake_volume = mock.MagicMock(id=str(uuid.uuid4()), status='available',
                                      multiattach=multiattach)
-        fake_volume.manager.get.return_value = fake_volume
         fake_attachment_id = str(uuid.uuid4())
         fake_attachment_create = {'id': fake_attachment_id}
         if encrypted_nfs or qcow2_vol:
@@ -457,8 +458,10 @@ class TestCinderStoreBase(object):
 
         with mock.patch.object(cinder.Store, 'get_cinderclient') as mocked_cc:
             mocked_cc.return_value = mock.MagicMock(volumes=fake_volumes)
-            self.assertRaises(exceptions.NotFound, method_call, loc,
-                              context=self.context)
+            exc = exceptions.NotFound
+            if method_call == self.store.get:
+                exc = exceptions.BackendException
+            self.assertRaises(exc, method_call, loc, context=self.context)
 
     def test_cinder_get_volume_not_found(self):
         self._test_cinder_volume_not_found(self.store.get, 'get')
@@ -546,9 +549,9 @@ class TestCinderStoreBase(object):
         self.config(cinder_volume_type='some_type', group=backend)
 
         fake_client = mock.MagicMock(auth_token=None, management_url=None)
-        fake_volume.manager.get.return_value = fake_volume
-        fake_volumes = mock.MagicMock(create=mock.Mock(
-            return_value=fake_volume))
+        fake_volumes = mock.MagicMock(
+            create=mock.Mock(return_value=fake_volume),
+            get=mock.Mock(return_value=fake_volume))
 
         @contextlib.contextmanager
         def fake_open(client, volume, mode):
@@ -603,9 +606,9 @@ class TestCinderStoreBase(object):
         self.config(cinder_volume_type='some_type', group=backend)
 
         fake_client = mock.MagicMock(auth_token=None, management_url=None)
-        fake_volume.manager.get.return_value = fake_volume
-        fake_volumes = mock.MagicMock(create=mock.Mock(
-            return_value=fake_volume))
+        fake_volumes = mock.MagicMock(
+            create=mock.Mock(return_value=fake_volume),
+            get=mock.Mock(return_value=fake_volume))
 
         @contextlib.contextmanager
         def fake_open(client, volume, mode):
@@ -684,9 +687,9 @@ class TestCinderStoreBase(object):
             self.store.volume_connector_map = fake_vol_connector_map
 
         fake_client = mock.MagicMock(auth_token=None, management_url=None)
-        fake_volume.manager.get.return_value = fake_volume
-        fake_volumes = mock.MagicMock(create=mock.Mock(
-            return_value=fake_volume))
+        fake_volumes = mock.MagicMock(
+            create=mock.Mock(return_value=fake_volume),
+            get=mock.Mock(return_value=fake_volume))
 
         @contextlib.contextmanager
         def fake_open(client, volume, mode):
@@ -727,14 +730,18 @@ class TestCinderStoreBase(object):
                     mock_cc_return_val, fake_volume,
                     expected_volume_size // units.Gi)
                 mock_wait.assert_has_calls(
-                    [mock.call(fake_volume, 'creating', 'available'),
-                     mock.call(fake_volume, 'extending', 'in-use')])
+                    [mock.call(fake_volume, 'creating', 'available',
+                               mock_cc_return_val),
+                     mock.call(fake_volume, 'extending', 'in-use',
+                               mock_cc_return_val)])
             else:
                 fake_volume.extend.assert_called_once_with(
                     fake_volume, expected_volume_size // units.Gi)
                 mock_wait.assert_has_calls(
-                    [mock.call(fake_volume, 'creating', 'available'),
-                     mock.call(fake_volume, 'extending', 'available')])
+                    [mock.call(fake_volume, 'creating', 'available',
+                               mock_cc_return_val),
+                     mock.call(fake_volume, 'extending', 'available',
+                               mock_cc_return_val)])
 
     def test_cinder_add_extend_storage_full(self):
 
@@ -757,9 +764,9 @@ class TestCinderStoreBase(object):
         verifier = None
 
         fake_client = mock.MagicMock()
-        fake_volume.manager.get.return_value = fake_volume
-        fake_volumes = mock.MagicMock(create=mock.Mock(
-            return_value=fake_volume))
+        fake_volumes = mock.MagicMock(
+            create=mock.Mock(return_value=fake_volume),
+            get=mock.Mock(return_value=fake_volume))
 
         with mock.patch.object(cinder.Store, 'get_cinderclient') as mock_cc, \
                 mock.patch.object(self.store, '_open_cinder_volume'), \
@@ -797,9 +804,9 @@ class TestCinderStoreBase(object):
             delete=mock.MagicMock(side_effect=Exception()))
 
         fake_client = mock.MagicMock()
-        fake_volume.manager.get.return_value = fake_volume
-        fake_volumes = mock.MagicMock(create=mock.Mock(
-            return_value=fake_volume))
+        fake_volumes = mock.MagicMock(
+            create=mock.Mock(return_value=fake_volume),
+            get=mock.Mock(return_value=fake_volume))
         verifier = None
 
         with mock.patch.object(cinder.Store, 'get_cinderclient') as mock_cc, \