Glance cinder nfs: Block creating qcow2 volumes

There's an issue when cinder nfs is configured as glance backend and
the image is > 1GB i.e. extend operation is performed on volume.
Currently glance writes the qcow2 header in the raw volume and
cinder nfs driver used to work on format autodetection which is
changed to manual detection here[1]
This fixes the extend volume problem for raw volumes, however, if
cinder nfs is configured to create qcow2 volumes then we will run into
the same problem.
This patch blocks creating images when the nfs volume is qcow2.

Refactoring work:
1) attachment_delete call is removed in nfs related code as it was
already done in the finally block
2) handle_exceptions decorator is removed from volume delete method
as it raises BackendException and incase when volume is not found
(possibly deleted manually), the image deletion fails so we don't
want to raise any error in this case

[1] https://review.opendev.org/c/openstack/cinder/+/761152

Closes-Bug: #1901138
Change-Id: I8ce6f36f1cb4b0ed6bcc5f3869fab3bb64fe3390

2 files changed, 110 insertions, 33 deletions

diff --git a/glance_store/tests/unit/test_cinder_store.py b/glance_store/tests/unit/test_cinder_store.py
index e87f129..ad25486 100644
--- a/glance_store/tests/unit/test_cinder_store.py
+++ b/glance_store/tests/unit/test_cinder_store.py
@@ -152,13 +152,19 @@ class TestCinderStore(base.StoreBaseTest,
     def _test_open_cinder_volume(self, open_mode, attach_mode, error,
                                  multipath_supported=False,
                                  enforce_multipath=False,
-                                 encrypted_nfs=False):
+                                 encrypted_nfs=False, qcow2_vol=False):
         self.config(cinder_mount_point_base=None)
         fake_volume = mock.MagicMock(id=str(uuid.uuid4()), status='available')
+        fake_volume.manager.get.return_value = fake_volume
         fake_volumes = FakeObject(get=lambda id: fake_volume)
         fake_attachment_id = str(uuid.uuid4())
         fake_attachment_create = {'id': fake_attachment_id}
-        fake_attachment_update = mock.MagicMock(id=fake_attachment_id)
+        if encrypted_nfs or qcow2_vol:
+            fake_attachment_update = mock.MagicMock(
+                id=fake_attachment_id,
+                connection_info={'driver_volume_type': 'nfs'})
+        else:
+            fake_attachment_update = mock.MagicMock(id=fake_attachment_id)
         fake_conn_info = mock.MagicMock(connector={})
         fake_client = FakeObject(volumes=fake_volumes)
         _, fake_dev_path = tempfile.mkstemp(dir=self.test_dir)
@@ -200,6 +206,8 @@ class TestCinderStore(base.StoreBaseTest,
                 mock.patch.object(cinder_utils.API,
                                   'attachment_delete') as attach_delete, \
                 mock.patch.object(cinder_utils.API,
+                                  'attachment_get') as attach_get, \
+                mock.patch.object(cinder_utils.API,
                                   'attachment_complete') as attach_complete:
 
             with mock.patch.object(connector,
@@ -207,24 +215,24 @@ class TestCinderStore(base.StoreBaseTest,
                                    return_value=fake_conn_info) as mock_conn:
                 if error:
                     self.assertRaises(error, do_open)
-                elif encrypted_nfs:
-                    fake_volume.initialize_connection.return_value = {
-                        'driver_volume_type': 'nfs'
-                    }
-                    fake_volume.encrypted = True
+                elif encrypted_nfs or qcow2_vol:
+                    fake_volume.encrypted = False
+                    if encrypted_nfs:
+                        fake_volume.encrypted = True
+                    elif qcow2_vol:
+                        attach_get.return_value = mock.MagicMock(
+                            connection_info={'format': 'qcow2'})
                     try:
                         with self.store._open_cinder_volume(
                                 fake_client, fake_volume, open_mode):
                             pass
                     except exceptions.BackendException:
-                        self.assertEqual(1,
-                                         fake_volume.unreserve.call_count)
-                        self.assertEqual(1,
-                                         fake_volume.delete.call_count)
+                        attach_delete.assert_called_once_with(
+                            fake_client, fake_attachment_id)
                 else:
                     do_open()
 
-                if not encrypted_nfs:
+                if not (encrypted_nfs or qcow2_vol):
                     mock_conn.assert_called_once_with(
                         root_helper, socket.gethostname(),
                         multipath_supported, enforce_multipath)
@@ -244,6 +252,24 @@ class TestCinderStore(base.StoreBaseTest,
                         fake_client, fake_attachment_id)
                     attach_delete.assert_called_once_with(
                         fake_client, fake_attachment_id)
+                else:
+                    mock_conn.assert_called_once_with(
+                        root_helper, socket.gethostname(),
+                        multipath_supported, enforce_multipath)
+                    fake_connector.connect_volume.assert_not_called()
+                    fake_connector.disconnect_volume.assert_not_called()
+                    fake_conn_obj.assert_called_once_with(
+                        mock.ANY, root_helper, conn=mock.ANY,
+                        use_multipath=multipath_supported)
+                    attach_create.assert_called_once_with(
+                        fake_client, fake_volume.id, mode=attach_mode)
+                    attach_update.assert_called_once_with(
+                        fake_client, fake_attachment_id,
+                        fake_conn_info, mountpoint='glance_store')
+                    attach_complete.assert_called_once_with(
+                        fake_client, fake_attachment_id)
+                    attach_delete.assert_called_once_with(
+                        fake_client, fake_attachment_id)
 
     def test_open_cinder_volume_rw(self):
         self._test_open_cinder_volume('wb', 'rw', None)
@@ -269,6 +295,9 @@ class TestCinderStore(base.StoreBaseTest,
     def test_open_cinder_volume_nfs_encrypted(self):
         self._test_open_cinder_volume('rb', 'ro', None, encrypted_nfs=True)
 
+    def test_open_cinder_volume_nfs_qcow2_volume(self):
+        self._test_open_cinder_volume('rb', 'ro', None, qcow2_vol=True)
+
     def test_cinder_configure_add(self):
         self.assertRaises(exceptions.BadStoreConfiguration,
                           self.store._check_context, None)
diff --git a/glance_store/tests/unit/test_multistore_cinder.py b/glance_store/tests/unit/test_multistore_cinder.py
index 2cc1e58..ebe69ee 100644
--- a/glance_store/tests/unit/test_multistore_cinder.py
+++ b/glance_store/tests/unit/test_multistore_cinder.py
@@ -183,12 +183,19 @@ class TestMultiCinderStore(base.MultiStoreBaseTest,
 
     def _test_open_cinder_volume(self, open_mode, attach_mode, error,
                                  multipath_supported=False,
-                                 enforce_multipath=False):
+                                 enforce_multipath=False,
+                                 encrypted_nfs=False, qcow2_vol=False):
         self.config(cinder_mount_point_base=None, group='cinder1')
         fake_volume = mock.MagicMock(id=str(uuid.uuid4()), status='available')
+        fake_volume.manager.get.return_value = fake_volume
         fake_attachment_id = str(uuid.uuid4())
         fake_attachment_create = {'id': fake_attachment_id}
-        fake_attachment_update = mock.MagicMock(id=fake_attachment_id)
+        if encrypted_nfs or qcow2_vol:
+            fake_attachment_update = mock.MagicMock(
+                id=fake_attachment_id,
+                connection_info={'driver_volume_type': 'nfs'})
+        else:
+            fake_attachment_update = mock.MagicMock(id=fake_attachment_id)
         fake_conn_info = mock.MagicMock(connector={})
         fake_volumes = FakeObject(get=lambda id: fake_volume)
         fake_client = FakeObject(volumes=fake_volumes)
@@ -233,6 +240,8 @@ class TestMultiCinderStore(base.MultiStoreBaseTest,
                 mock.patch.object(cinder_utils.API,
                                   'attachment_delete') as attach_delete, \
                 mock.patch.object(cinder_utils.API,
+                                  'attachment_get') as attach_get, \
+                mock.patch.object(cinder_utils.API,
                                   'attachment_complete') as attach_complete:
 
             with mock.patch.object(connector,
@@ -240,27 +249,60 @@ class TestMultiCinderStore(base.MultiStoreBaseTest,
                                    return_value=fake_conn_info) as mock_conn:
                 if error:
                     self.assertRaises(error, do_open)
+                elif encrypted_nfs or qcow2_vol:
+                    fake_volume.encrypted = False
+                    if encrypted_nfs:
+                        fake_volume.encrypted = True
+                    elif qcow2_vol:
+                        attach_get.return_value = mock.MagicMock(
+                            connection_info={'format': 'qcow2'})
+                    try:
+                        with self.store._open_cinder_volume(
+                                fake_client, fake_volume, open_mode):
+                            pass
+                    except exceptions.BackendException:
+                        attach_delete.assert_called_once_with(
+                            fake_client, fake_attachment_id)
                 else:
                     do_open()
-
-                mock_conn.assert_called_once_with(
-                    root_helper, socket.gethostname(), multipath_supported,
-                    enforce_multipath)
-                fake_connector.connect_volume.assert_called_once_with(mock.ANY)
-                fake_connector.disconnect_volume.assert_called_once_with(
-                    mock.ANY, fake_devinfo)
-                fake_conn_obj.assert_called_once_with(
-                    mock.ANY, root_helper, conn=mock.ANY,
-                    use_multipath=multipath_supported)
-                attach_create.assert_called_once_with(
-                    fake_client, fake_volume.id, mode=attach_mode)
-                attach_update.assert_called_once_with(
-                    fake_client, fake_attachment_id,
-                    fake_conn_info, mountpoint='glance_store')
-                attach_complete.assert_called_once_with(fake_client,
-                                                        fake_attachment_id)
-                attach_delete.assert_called_once_with(fake_client,
-                                                      fake_attachment_id)
+                if not (encrypted_nfs or qcow2_vol):
+                    mock_conn.assert_called_once_with(
+                        root_helper, socket.gethostname(),
+                        multipath_supported, enforce_multipath)
+                    fake_connector.connect_volume.assert_called_once_with(
+                        mock.ANY)
+                    fake_connector.disconnect_volume.assert_called_once_with(
+                        mock.ANY, fake_devinfo)
+                    fake_conn_obj.assert_called_once_with(
+                        mock.ANY, root_helper, conn=mock.ANY,
+                        use_multipath=multipath_supported)
+                    attach_create.assert_called_once_with(
+                        fake_client, fake_volume.id, mode=attach_mode)
+                    attach_update.assert_called_once_with(
+                        fake_client, fake_attachment_id,
+                        fake_conn_info, mountpoint='glance_store')
+                    attach_complete.assert_called_once_with(
+                        fake_client, fake_attachment_id)
+                    attach_delete.assert_called_once_with(fake_client,
+                                                          fake_attachment_id)
+                else:
+                    mock_conn.assert_called_once_with(
+                        root_helper, socket.gethostname(),
+                        multipath_supported, enforce_multipath)
+                    fake_connector.connect_volume.assert_not_called()
+                    fake_connector.disconnect_volume.assert_not_called()
+                    fake_conn_obj.assert_called_once_with(
+                        mock.ANY, root_helper, conn=mock.ANY,
+                        use_multipath=multipath_supported)
+                    attach_create.assert_called_once_with(
+                        fake_client, fake_volume.id, mode=attach_mode)
+                    attach_update.assert_called_once_with(
+                        fake_client, fake_attachment_id,
+                        fake_conn_info, mountpoint='glance_store')
+                    attach_complete.assert_called_once_with(
+                        fake_client, fake_attachment_id)
+                    attach_delete.assert_called_once_with(
+                        fake_client, fake_attachment_id)
 
     def test_open_cinder_volume_rw(self):
         self._test_open_cinder_volume('wb', 'rw', None)
@@ -284,6 +326,12 @@ class TestMultiCinderStore(base.MultiStoreBaseTest,
                                       multipath_supported=True,
                                       enforce_multipath=True)
 
+    def test_open_cinder_volume_nfs_encrypted(self):
+        self._test_open_cinder_volume('rb', 'ro', None, encrypted_nfs=True)
+
+    def test_open_cinder_volume_nfs_qcow2_volume(self):
+        self._test_open_cinder_volume('rb', 'ro', None, qcow2_vol=True)
+
     def test_cinder_check_context(self):
         self.assertRaises(exceptions.BadStoreConfiguration,
                           self.store._check_context, None)