From 3d55084cde7f30aba2ec41c003242d2762cf4413 Mon Sep 17 00:00:00 2001 From: Rajat Dhasmana Date: Fri, 26 Sep 2025 19:42:51 +0000 Subject: [PATCH] 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 Change-Id: I5a4bad40ea03b0d746aaad392781f1b0fe78c8bc --- glance_store/_drivers/cinder/store.py | 49 ++++++---- glance_store/common/cinder_utils.py | 9 ++ .../tests/unit/cinder/test_cinder_base.py | 91 ++++++++++--------- 3 files changed, 87 insertions(+), 62 deletions(-) diff --git a/glance_store/_drivers/cinder/store.py b/glance_store/_drivers/cinder/store.py index 6c208414..6281e923 100644 --- a/glance_store/_drivers/cinder/store.py +++ b/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 could not be found. + # (HTTP 404) (Request-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 diff --git a/glance_store/common/cinder_utils.py b/glance_store/common/cinder_utils.py index c1c4186b..5f7044d5 100644 --- a/glance_store/common/cinder_utils.py +++ b/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) diff --git a/glance_store/tests/unit/cinder/test_cinder_base.py b/glance_store/tests/unit/cinder/test_cinder_base.py index 971bde61..59169284 100644 --- a/glance_store/tests/unit/cinder/test_cinder_base.py +++ b/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, \