Cinder: Add support to extend attached volumes

While creating an image, if we want to extend the volume
(to accomodate the image), we need to first detach the volume,
perform the extend and attach it again. This is inefficient
for backends that support extending attached volumes since we
are performing 3 cinder operations (the attachment call
includes 3 API calls which sum to a total of 5 API calls for 3
operations) instead of directly extending it which requires only
1 API call to cinder.
The support for extending attached volumes was added in cinder
microversion 3.42.

This patch adds a new boolean config option
``cinder_do_extend_attached`` which allows operators to specify
if the cinder backend they are using supports extending
attached (in-use) volumes. By default this will be ``false``.
Based on the parameter, we will perform the extend operation,
online (if cinder_do_extend_attached is true) and offline otherwise.

Spec: https://review.opendev.org/c/openstack/glance-specs/+/868901

Depends-On: https://review.opendev.org/c/openstack/glance/+/869021
Depends-On: https://review.opendev.org/c/openstack/cinder/+/869051

Change-Id: I5e70824e9abc5277ea25ba95704b358fe3686037

11 files changed, 212 insertions, 50 deletions

diff --git a/glance_store/_drivers/cinder/base.py b/glance_store/_drivers/cinder/base.py
index 69bb5ee..20fb897 100644
--- a/glance_store/_drivers/cinder/base.py
+++ b/glance_store/_drivers/cinder/base.py
@@ -52,6 +52,9 @@ class BaseBrickConnectorInterface(object):
     def disconnect_volume(self, device):
         self.conn.disconnect_volume(self.connection_info, device)
 
+    def extend_volume(self):
+        self.conn.extend_volume(self.connection_info)
+
     def yield_path(self, volume, volume_path):
         """
         This method returns the volume file path.
diff --git a/glance_store/_drivers/cinder/nfs.py b/glance_store/_drivers/cinder/nfs.py
index 7ae17ab..0e092d2 100644
--- a/glance_store/_drivers/cinder/nfs.py
+++ b/glance_store/_drivers/cinder/nfs.py
@@ -101,3 +101,6 @@ class NfsBrickConnector(base.BaseBrickConnectorInterface):
             mount.umount(vol_name, path, self.host,
                          self.root_helper)
         disconnect_volume_nfs()
+
+    def extend_volume(self):
+        raise NotImplementedError
diff --git a/glance_store/_drivers/cinder/store.py b/glance_store/_drivers/cinder/store.py
index fae6600..17fb96b 100644
--- a/glance_store/_drivers/cinder/store.py
+++ b/glance_store/_drivers/cinder/store.py
@@ -404,6 +404,19 @@ Possible values:
 
 * A string representing absolute path of mount point.
 """),
+    cfg.BoolOpt('cinder_do_extend_attached',
+                default=False,
+                help="""
+If this is set to True, glance will perform an extend operation
+on the attached volume. Only enable this option if the cinder
+backend driver supports the functionality of extending online
+(in-use) volumes. Supported from cinder microversion 3.42 and
+onwards. By default, it is set to False.
+
+Possible values:
+    * True or False
+
+"""),
 ]
 
 CINDER_SESSION = None
@@ -483,6 +496,13 @@ class Store(glance_store.driver.Store):
             self.store_conf = self.conf.glance_store
         self.volume_api = cinder_utils.API()
         getattr(os_brick, 'setup', lambda x: None)(CONF)
+        # The purpose of this map is to store the connector object for a
+        # particular volume as we will need to call os-brick extend_volume
+        # method for the kernel to realize the new size change after cinder
+        # extends the volume
+        # We only use it when creating the image so a volume will only have
+        # one mapping to a particular connector
+        self.volume_connector_map = {}
 
     def _set_url_prefix(self):
         self._url_prefix = "cinder://"
@@ -730,6 +750,8 @@ class Store(glance_store.driver.Store):
             self.volume_api.attachment_complete(client, attachment.id)
             LOG.debug('Attachment %(attachment_id)s completed successfully.',
                       {'attachment_id': attachment.id})
+
+            self.volume_connector_map[volume.id] = conn
             if (connection_info['driver_volume_type'] == 'rbd' and
                not conn.conn.do_local_attach):
                 yield device['path']
@@ -750,7 +772,8 @@ class Store(glance_store.driver.Store):
                             connection_info, device)
                     else:
                         conn.disconnect_volume(device)
-
+                        if self.volume_connector_map.get(volume.id):
+                            del self.volume_connector_map[volume.id]
                 except Exception:
                     LOG.exception(_LE('Failed to disconnect volume '
                                       '%(volume_id)s.'),
@@ -848,6 +871,100 @@ class Store(glance_store.driver.Store):
                               "internal error."))
             return 0
 
+    def _call_offline_extend(self, volume, size_gb):
+        size_gb += 1
+        LOG.debug("Extending (offline) volume %(volume_id)s to %(size)s GB.",
+                  {'volume_id': volume.id, 'size': size_gb})
+        volume.extend(volume, size_gb)
+        try:
+            volume = self._wait_volume_status(volume,
+                                              'extending',
+                                              'available')
+            size_gb = volume.size
+            return size_gb
+        except exceptions.BackendException:
+            raise exceptions.StorageFull()
+
+    def _call_online_extend(self, client, volume, size_gb):
+        size_gb += 1
+        LOG.debug("Extending (online) volume %(volume_id)s to %(size)s GB.",
+                  {'volume_id': volume.id, 'size': size_gb})
+        self.volume_api.extend_volume(client, volume, size_gb)
+        try:
+            volume = self._wait_volume_status(volume,
+                                              'extending',
+                                              'in-use')
+            size_gb = volume.size
+            return size_gb
+        except exceptions.BackendException:
+            raise exceptions.StorageFull()
+
+    def _write_data(self, f, write_props):
+        LOG.debug('Writing data to volume with write properties: '
+                  'bytes_written: %s, size_gb: %s, need_extend: %s, '
+                  'image_size: %s' %
+                  (write_props.bytes_written, write_props.size_gb,
+                   write_props.need_extend, write_props.image_size))
+        f.seek(write_props.bytes_written)
+        if write_props.buf:
+            f.write(write_props.buf)
+            write_props.bytes_written += len(write_props.buf)
+        while True:
+            write_props.buf = write_props.image_file.read(
+                self.WRITE_CHUNKSIZE)
+            if not write_props.buf:
+                write_props.need_extend = False
+                return
+            write_props.os_hash_value.update(write_props.buf)
+            write_props.checksum.update(write_props.buf)
+            if write_props.verifier:
+                write_props.verifier.update(write_props.buf)
+            if ((write_props.bytes_written + len(write_props.buf)) > (
+                write_props.size_gb * units.Gi) and
+                    (write_props.image_size == 0)):
+                return
+            f.write(write_props.buf)
+            write_props.bytes_written += len(write_props.buf)
+
+    def _offline_extend(self, client, volume, write_props):
+        while write_props.need_extend:
+            with self._open_cinder_volume(client, volume, 'wb') as f:
+                self._write_data(f, write_props)
+            if write_props.need_extend:
+                write_props.size_gb = self._call_offline_extend(
+                    volume, write_props.size_gb)
+
+    def _online_extend(self, client, volume, write_props):
+        with self._open_cinder_volume(client, volume, 'wb') as f:
+            # Th connector is initialized in _open_cinder_volume method
+            # and by mapping it with the volume ID, we are able to fetch
+            # it here
+            conn = self.volume_connector_map[volume.id]
+            while write_props.need_extend:
+                self._write_data(f, write_props)
+                if write_props.need_extend:
+                    # we already initialize a client with MV 3.54 and
+                    # we require 3.42 for online extend so we should
+                    # be good here.
+                    write_props.size_gb = self._call_online_extend(
+                        client, volume, write_props.size_gb)
+                    # Call os-brick to resize the LUN on the host
+                    conn.extend_volume()
+
+    # WriteProperties class is useful to allow us to modify immutable
+    # objects in the called methods
+    class WriteProperties:
+        def __init__(self, *args, **kwargs):
+            self.bytes_written = kwargs.get('bytes_written')
+            self.size_gb = kwargs.get('size_gb')
+            self.buf = kwargs.get('buf')
+            self.image_file = kwargs.get('image_file')
+            self.need_extend = kwargs.get('need_extend')
+            self.image_size = kwargs.get('image_size')
+            self.verifier = kwargs.get('verifier')
+            self.checksum = kwargs.get('checksum')
+            self.os_hash_value = kwargs.get('os_hash_value')
+
     @glance_store.driver.back_compat_add
     @capabilities.check
     def add(self, image_id, image_file, image_size, hashing_algo, context=None,
@@ -904,41 +1021,20 @@ class Store(glance_store.driver.Store):
         failed = True
         need_extend = True
         buf = None
+        online_extend = self.store_conf.cinder_do_extend_attached
+        write_props = self.WriteProperties(
+            bytes_written=bytes_written, size_gb=size_gb, buf=buf,
+            image_file=image_file, need_extend=need_extend,
+            image_size=image_size, verifier=verifier, checksum=checksum,
+            os_hash_value=os_hash_value)
         try:
-            while need_extend:
-                with self._open_cinder_volume(client, volume, 'wb') as f:
-                    f.seek(bytes_written)
-                    if buf:
-                        f.write(buf)
-                        bytes_written += len(buf)
-                    while True:
-                        buf = image_file.read(self.WRITE_CHUNKSIZE)
-                        if not buf:
-                            need_extend = False
-                            break
-                        os_hash_value.update(buf)
-                        checksum.update(buf)
-                        if verifier:
-                            verifier.update(buf)
-                        if (bytes_written + len(buf) > size_gb * units.Gi and
-                                image_size == 0):
-                            break
-                        f.write(buf)
-                        bytes_written += len(buf)
-
-                if need_extend:
-                    size_gb += 1
-                    LOG.debug("Extending volume %(volume_id)s to %(size)s GB.",
-                              {'volume_id': volume.id, 'size': size_gb})
-                    volume.extend(volume, size_gb)
-                    try:
-                        volume = self._wait_volume_status(volume,
-                                                          'extending',
-                                                          'available')
-                        size_gb = volume.size
-                    except exceptions.BackendException:
-                        raise exceptions.StorageFull()
-
+            if online_extend:
+                # we already initialize a client with MV 3.54 and
+                # we require 3.42 for online extend so we should
+                # be good here.
+                self._online_extend(client, volume, write_props)
+            else:
+                self._offline_extend(client, volume, write_props)
             failed = False
         except IOError as e:
             # Convert IOError reasons to Glance Store exceptions
@@ -957,17 +1053,17 @@ class Store(glance_store.driver.Store):
                                       '%(volume_id)s.'),
                                   {'volume_id': volume.id})
 
-        if image_size == 0:
-            metadata.update({'image_size': str(bytes_written)})
+        if write_props.image_size == 0:
+            metadata.update({'image_size': str(write_props.bytes_written)})
             volume.update_all_metadata(metadata)
         volume.update_readonly_flag(volume, True)
 
-        hash_hex = os_hash_value.hexdigest()
-        checksum_hex = checksum.hexdigest()
+        hash_hex = write_props.os_hash_value.hexdigest()
+        checksum_hex = write_props.checksum.hexdigest()
 
         LOG.debug("Wrote %(bytes_written)d bytes to volume %(volume_id)s "
                   "with checksum %(checksum_hex)s.",
-                  {'bytes_written': bytes_written,
+                  {'bytes_written': write_props.bytes_written,
                    'volume_id': volume.id,
                    'checksum_hex': checksum_hex})
 
@@ -979,7 +1075,7 @@ class Store(glance_store.driver.Store):
                                                volume.id)
 
         return (location_url,
-                bytes_written,
+                write_props.bytes_written,
                 checksum_hex,
                 hash_hex,
                 image_metadata)
diff --git a/glance_store/common/cinder_utils.py b/glance_store/common/cinder_utils.py
index b14aa23..c1c4186 100644
--- a/glance_store/common/cinder_utils.py
+++ b/glance_store/common/cinder_utils.py
@@ -208,3 +208,21 @@ class API(object):
                           {'id': attachment_id,
                            'msg': str(ex),
                            'code': getattr(ex, 'code', None)})
+
+    @handle_exceptions
+    def extend_volume(self, client, volume, new_size):
+        """Extend volume
+
+        :param client: cinderclient object
+        :param volume: UUID of the volume to extend
+        :param new_size: new size of the volume after extend
+        """
+        try:
+            client.volumes.extend(volume, new_size)
+        except cinder_exception.ClientException as ex:
+            with excutils.save_and_reraise_exception():
+                LOG.error(_LE('Extend volume failed for volume '
+                              '%(id)s. Error: %(msg)s Code: %(code)s'),
+                          {'id': volume.id,
+                           'msg': str(ex),
+                           'code': getattr(ex, 'code', None)})
diff --git a/glance_store/tests/unit/cinder/test_base.py b/glance_store/tests/unit/cinder/test_base.py
index cc57f56..488cf1a 100644
--- a/glance_store/tests/unit/cinder/test_base.py
+++ b/glance_store/tests/unit/cinder/test_base.py
@@ -104,6 +104,12 @@ class TestBaseBrickConnectorInterface(test_base.StoreBaseTest):
         self.connector.conn.disconnect_volume.assert_called_once_with(
             self.connection_info, fake_device)
 
+    def test_extend_volume(self):
+        self.mock_object(self.connector.conn, 'extend_volume')
+        self.connector.extend_volume()
+        self.connector.conn.extend_volume.assert_called_once_with(
+            self.connection_info)
+
     def test_yield_path(self):
         fake_vol = mock.MagicMock()
         fake_device = 'fake_dev_path'
diff --git a/glance_store/tests/unit/cinder/test_cinder_base.py b/glance_store/tests/unit/cinder/test_cinder_base.py
index d94782f..acdb70b 100644
--- a/glance_store/tests/unit/cinder/test_cinder_base.py
+++ b/glance_store/tests/unit/cinder/test_cinder_base.py
@@ -582,7 +582,7 @@ class TestCinderStoreBase(object):
                 fake_image_id, image_file, expected_size, self.hash_algo,
                 self.context, None)
 
-    def _test_cinder_add_extend(self, is_multi_store=False):
+    def _test_cinder_add_extend(self, is_multi_store=False, online=False):
 
         expected_volume_size = 2 * units.Gi
         expected_multihash = 'fake_hash'
@@ -619,6 +619,11 @@ class TestCinderStoreBase(object):
             backend = 'cinder1'
             expected_location = 'cinder://%s/%s' % (backend, fake_volume.id)
         self.config(cinder_volume_type='some_type', group=backend)
+        if online:
+            self.config(cinder_do_extend_attached=True, group=backend)
+            fake_connector = mock.MagicMock()
+            fake_vol_connector_map = {expected_volume_id: fake_connector}
+            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
@@ -635,9 +640,12 @@ class TestCinderStoreBase(object):
                                   side_effect=fake_open), \
                 mock.patch.object(cinder.utils, 'get_hasher') as fake_hasher, \
                 mock.patch.object(cinder.Store, '_wait_volume_status',
-                                  return_value=fake_volume) as mock_wait:
-            mock_cc.return_value = mock.MagicMock(client=fake_client,
-                                                  volumes=fake_volumes)
+                                  return_value=fake_volume) as mock_wait, \
+                mock.patch.object(cinder_utils.API,
+                                  'extend_volume') as extend_vol:
+            mock_cc_return_val = mock.MagicMock(client=fake_client,
+                                                volumes=fake_volumes)
+            mock_cc.return_value = mock_cc_return_val
 
             fake_hasher.side_effect = get_fake_hash
             loc, size, checksum, multihash, metadata = self.store.add(
@@ -656,11 +664,19 @@ class TestCinderStoreBase(object):
                 volume_type='some_type')
             if is_multi_store:
                 self.assertEqual(backend, metadata["store"])
-            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')])
+            if online:
+                extend_vol.assert_called_once_with(
+                    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')])
+            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')])
 
     def test_cinder_add_extend_storage_full(self):
 
diff --git a/glance_store/tests/unit/cinder/test_cinder_store.py b/glance_store/tests/unit/cinder/test_cinder_store.py
index a2b5af7..184acde 100644
--- a/glance_store/tests/unit/cinder/test_cinder_store.py
+++ b/glance_store/tests/unit/cinder/test_cinder_store.py
@@ -138,6 +138,9 @@ class TestCinderStore(base.StoreBaseTest,
     def test_cinder_add_extend(self):
         self._test_cinder_add_extend()
 
+    def test_cinder_add_extend_online(self):
+        self._test_cinder_add_extend(online=True)
+
     def test_cinder_delete(self):
         self._test_cinder_delete()
 
diff --git a/glance_store/tests/unit/cinder/test_multistore_cinder.py b/glance_store/tests/unit/cinder/test_multistore_cinder.py
index 01b27f2..f40965b 100644
--- a/glance_store/tests/unit/cinder/test_multistore_cinder.py
+++ b/glance_store/tests/unit/cinder/test_multistore_cinder.py
@@ -276,6 +276,9 @@ class TestMultiCinderStore(base.MultiStoreBaseTest,
     def test_cinder_add_extend(self):
         self._test_cinder_add_extend(is_multi_store=True)
 
+    def test_cinder_add_extend_online(self):
+        self._test_cinder_add_extend(is_multi_store=True, online=True)
+
     def test_cinder_delete(self):
         self._test_cinder_delete(is_multi_store=True)
 
diff --git a/glance_store/tests/unit/cinder/test_nfs.py b/glance_store/tests/unit/cinder/test_nfs.py
index d0f7d91..aa040ce 100644
--- a/glance_store/tests/unit/cinder/test_nfs.py
+++ b/glance_store/tests/unit/cinder/test_nfs.py
@@ -91,3 +91,6 @@ class TestNfsBrickConnector(
         nfs.mount.umount.assert_called_once_with(
             vol_name, mount_path, self.connector.host,
             self.connector.root_helper)
+
+    def test_extend_volume(self):
+        self.assertRaises(NotImplementedError, self.connector.extend_volume)
diff --git a/glance_store/tests/unit/test_opts.py b/glance_store/tests/unit/test_opts.py
index bfb9bce..073b782 100644
--- a/glance_store/tests/unit/test_opts.py
+++ b/glance_store/tests/unit/test_opts.py
@@ -84,6 +84,7 @@ class OptsTestCase(base.StoreBaseTest):
             'cinder_volume_type',
             'cinder_use_multipath',
             'cinder_enforce_multipath',
+            'cinder_do_extend_attached',
             'default_swift_reference',
             'https_insecure',
             'filesystem_store_chunk_size',
diff --git a/releasenotes/notes/cinder-support-extend-in-use-volume-c6292f950ff75cca.yaml b/releasenotes/notes/cinder-support-extend-in-use-volume-c6292f950ff75cca.yaml
new file mode 100644
index 0000000..2b4a748
--- /dev/null
+++ b/releasenotes/notes/cinder-support-extend-in-use-volume-c6292f950ff75cca.yaml
@@ -0,0 +1,10 @@
+---
+features:
+  - |
+    Added support for extending in-use volumes in cinder store.
+    A new boolean config option ``cinder_do_extend_attached`` is
+    added which allows operators to enable/disable extending
+    in-use volume support when creating an image.
+    By default, ``cinder_do_extend_attached`` will be ``False``
+    i.e. old flow of detaching, extending and attaching will be
+    used.