Merge "Add volume multiattach handling"

glance_store/_drivers/cinder.py

@@ -33,6 +33,7 @@ from oslo_config import cfg
 from oslo_utils import units
 
 from glance_store import capabilities
+from glance_store.common import attachment_state_manager
 from glance_store.common import cinder_utils
 from glance_store.common import utils
 import glance_store.driver
@@ -699,8 +700,13 @@ class Store(glance_store.driver.Store):
         connector_prop = connector.get_connector_properties(
             root_helper, host, use_multipath, enforce_multipath)
 
-        attachment = self.volume_api.attachment_create(client, volume_id,
-                                                       mode=attach_mode)
+        if volume.multiattach:
+            attachment = attachment_state_manager.attach(client, volume_id,
+                                                         host,
+                                                         mode=attach_mode)
+        else:
+            attachment = self.volume_api.attachment_create(client, volume_id,
+                                                           mode=attach_mode)
         attachment = self.volume_api.attachment_update(
             client, attachment['id'], connector_prop,
             mountpoint='glance_store')
@@ -767,13 +773,19 @@ class Store(glance_store.driver.Store):
                                               root_helper)
                         disconnect_volume_nfs()
                     else:
-                        conn.disconnect_volume(connection_info, device)
+                        if volume.multiattach:
+                            attachment_state_manager.detach(
+                                client, attachment.id, volume_id, host, conn,
+                                connection_info, device)
+                        else:
+                            conn.disconnect_volume(connection_info, device)
                 except Exception:
                     LOG.exception(_LE('Failed to disconnect volume '
                                       '%(volume_id)s.'),
                                   {'volume_id': volume.id})
 
-            self.volume_api.attachment_delete(client, attachment.id)
+            if not volume.multiattach:
+                self.volume_api.attachment_delete(client, attachment.id)
 
     def _cinder_volume_data_iterator(self, client, volume, max_size, offset=0,
                                      chunk_size=None, partial_length=None):

glance_store/common/attachment_state_manager.py

@@ -0,0 +1,261 @@
+# Copyright 2021 RedHat Inc.
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import collections
+import contextlib
+import logging
+import socket
+import threading
+
+from oslo_config import cfg
+
+from glance_store.common import cinder_utils
+from glance_store import exceptions
+from glance_store.i18n import _LE, _LW
+
+
+LOG = logging.getLogger(__name__)
+
+HOST = socket.gethostname()
+CONF = cfg.CONF
+
+
+class AttachmentStateManagerMeta(type):
+    _instance = {}
+
+    def __call__(cls, *args, **kwargs):
+        if cls not in cls._instance:
+            cls._instance[cls] = super(
+                AttachmentStateManagerMeta, cls).__call__(*args, **kwargs)
+        return cls._instance[cls]
+
+
+class _AttachmentStateManager(metaclass=AttachmentStateManagerMeta):
+    """A global manager of a volume's multiple attachments.
+
+    _AttachmentStateManager manages a _AttachmentState object for the current
+    glance node. Primarily it creates one on object initialization and returns
+    it via get_state().
+
+    _AttachmentStateManager manages concurrency itself. Independent callers do
+    not need to consider interactions between multiple _AttachmentStateManager
+    calls when designing their own locking.
+
+    """
+    # Reset state of global _AttachmentStateManager
+    state = None
+    use_count = 0
+
+    # Guards both state and use_count
+    cond = threading.Condition()
+
+    def __init__(self, host):
+        """Initialise a new _AttachmentState
+
+        We will block before creating a new state until all operations
+        using a previous state have completed.
+
+        :param host: host
+        """
+        # Wait until all operations using a previous state are
+        # complete before initialising a new one. Note that self.state is
+        # already None, set either by initialisation or by host_down. This
+        # means the current state will not be returned to any new callers,
+        # and use_count will eventually reach zero.
+        # We do this to avoid a race between _AttachmentState initialisation
+        # and an on-going attach/detach operation
+        self.host = host
+        while self.use_count != 0:
+            self.cond.wait()
+
+        # Another thread might have initialised state while we were
+        # waiting
+        if self.state is None:
+            LOG.debug('Initialising _HostMountState')
+            self.state = _AttachmentState()
+
+    @contextlib.contextmanager
+    def get_state(self):
+        """Return the current attachment state.
+
+        _AttachmentStateManager will not permit a new state object to be
+        created while any previous state object is still in use.
+
+        :rtype: _AttachmentState
+        """
+
+        # We hold the instance lock here so that if a _AttachmentState is
+        # currently initialising we'll wait for it to complete rather than
+        # fail.
+        with self.cond:
+            state = self.state
+            if state is None:
+                LOG.error('Host not initialized')
+                raise exceptions.HostNotInitialized(host=self.host)
+            self.use_count += 1
+        try:
+            LOG.debug('Got _AttachmentState')
+            yield state
+        finally:
+            with self.cond:
+                self.use_count -= 1
+                self.cond.notify_all()
+
+
+class _AttachmentState(object):
+    """A data structure recording all managed attachments. _AttachmentState
+    ensures that the glance node only attempts to a single multiattach volume
+    in use by multiple attachments once, and that it is not disconnected until
+    it is no longer in use by any attachments.
+
+    Callers should not create a _AttachmentState directly, but should obtain
+    it via:
+
+      with attachment.get_manager().get_state() as state:
+        state.attach(...)
+
+    _AttachmentState manages concurrency itself. Independent callers do not
+    need to consider interactions between multiple _AttachmentState calls when
+    designing their own locking.
+    """
+
+    class _Attachment(object):
+        # A single multiattach volume, and the set of attachments in use
+        # on it.
+        def __init__(self):
+            # A guard for operations on this volume
+            self.lock = threading.Lock()
+
+            # The set of attachments on this volume
+            self.attachments = set()
+
+        def add_attachment(self, attachment_id, host):
+            self.attachments.add((attachment_id, host))
+
+        def remove_attachment(self, attachment_id, host):
+            self.attachments.remove((attachment_id, host))
+
+        def in_use(self):
+            return len(self.attachments) > 0
+
+    def __init__(self):
+        """Initialise _AttachmentState"""
+
+        self.volumes = collections.defaultdict(self._Attachment)
+        self.volume_api = cinder_utils.API()
+
+    @contextlib.contextmanager
+    def _get_locked(self, volume):
+        """Get a locked attachment object
+
+        :param mountpoint: The path of the volume whose attachment we should
+                           return.
+        :rtype: _AttachmentState._Attachment
+        """
+        while True:
+            vol = self.volumes[volume]
+            with vol.lock:
+                if self.volumes[volume] is vol:
+                    yield vol
+                    break
+
+    def attach(self, client, volume_id, host, mode=None):
+        """Ensure a volume is available for an attachment and create an
+        attachment
+
+        :param client: Cinderclient object
+        :param volume_id: ID of the volume to attach
+        :param host: The host the volume will be attached to
+        :param mode: The attachment mode
+        """
+
+        LOG.debug('_AttachmentState.attach(volume_id=%(volume_id)s, '
+                  'host=%(host)s, mode=%(mode)s)',
+                  {'volume_id': volume_id, 'host': host, 'mode': mode})
+        with self._get_locked(volume_id) as vol_attachment:
+
+            try:
+                attachment = self.volume_api.attachment_create(
+                    client, volume_id, mode=mode)
+            except Exception:
+                LOG.exception(_LE('Error attaching volume %(volume_id)s',
+                              {'volume_id': volume_id}))
+                del self.volumes[volume_id]
+                raise
+
+            vol_attachment.add_attachment(attachment['id'], host)
+
+        LOG.debug('_AttachmentState.attach for volume_id=%(volume_id)s '
+                  'and attachment_id=%(attachment_id)s completed successfully',
+                  {'volume_id': volume_id, 'attachment_id': attachment['id']})
+        return attachment
+
+    def detach(self, client, attachment_id, volume_id, host, conn,
+               connection_info, device):
+        """Delete the attachment no longer in use, and disconnect volume
+        if necessary.
+
+        :param client: Cinderclient object
+        :param attachment_id: ID of the attachment between volume and host
+        :param volume_id: ID of the volume to attach
+        :param host: The host the volume was attached to
+        :param conn: connector object
+        :param connection_info: connection information of the volume we are
+                                detaching
+        :device: device used to write image
+
+        """
+        LOG.debug('_AttachmentState.detach(vol_id=%(volume_id)s, '
+                  'attachment_id=%(attachment_id)s)',
+                  {'volume_id': volume_id, 'attachment_id': attachment_id})
+        with self._get_locked(volume_id) as vol_attachment:
+            try:
+                vol_attachment.remove_attachment(attachment_id, host)
+            except KeyError:
+                LOG.warning(_LW("Request to remove attachment "
+                                "(%(volume_id)s, %(host)s) but we "
+                                "don't think it's in use."),
+                            {'volume_id': volume_id, 'host': host})
+
+            if not vol_attachment.in_use():
+                conn.disconnect_volume(connection_info, device)
+                del self.volumes[volume_id]
+            self.volume_api.attachment_delete(client, attachment_id)
+
+            LOG.debug('_AttachmentState.detach for volume %(volume_id)s '
+                      'and attachment_id=%(attachment_id)s completed '
+                      'successfully',
+                      {'volume_id': volume_id,
+                       'attachment_id': attachment_id})
+
+
+__manager__ = _AttachmentStateManager(HOST)
+
+
+def attach(client, volume_id, host, mode=None):
+    """A convenience wrapper around _AttachmentState.attach()"""
+
+    with __manager__.get_state() as attach_state:
+        attachment = attach_state.attach(client, volume_id, host, mode=mode)
+    return attachment
+
+
+def detach(client, attachment_id, volume_id, host, conn, connection_info,
+           device):
+    """A convenience wrapper around _AttachmentState.detach()"""
+
+    with __manager__.get_state() as attach_state:
+        attach_state.detach(client, attachment_id, volume_id, host, conn,
+                            connection_info, device)

glance_store/common/cinder_utils.py

@@ -47,6 +47,12 @@ def _retry_on_internal_server_error(e):
     return False
 
 
+def _retry_on_bad_request(e):
+    if isinstance(e, cinder_exception.BadRequest):
+        return True
+    return False
+
+
 class API(object):
     """API for interacting with the cinder."""
 
@@ -64,6 +70,8 @@ class API(object):
     def delete(self, client, volume_id):
         client.volumes.delete(volume_id)
 
+    @retrying.retry(stop_max_attempt_number=5,
+                    retry_on_exception=_retry_on_bad_request)
     @handle_exceptions
     def attachment_create(self, client, volume_id, connector=None,
                           mountpoint=None, mode=None):
@@ -95,11 +103,18 @@ class API(object):
             return attachment_ref
         except cinder_exception.ClientException as ex:
             with excutils.save_and_reraise_exception():
-                LOG.error(_LE('Create attachment failed for volume '
-                              '%(volume_id)s. Error: %(msg)s Code: %(code)s'),
-                          {'volume_id': volume_id,
-                           'msg': str(ex),
-                           'code': getattr(ex, 'code', None)})
+                # While handling simultaneous requests, the volume can be
+                # in different states and we retry on attachment_create
+                # until the volume reaches a valid state for attachment.
+                # Hence, it is better to not log 400 cases as no action
+                # from users is needed in this case
+                if getattr(ex, 'code', None) != 400:
+                    LOG.error(_LE('Create attachment failed for volume '
+                                  '%(volume_id)s. Error: %(msg)s '
+                                  'Code: %(code)s'),
+                              {'volume_id': volume_id,
+                               'msg': str(ex),
+                               'code': getattr(ex, 'code', None)})
 
     @handle_exceptions
     def attachment_get(self, client, attachment_id):

glance_store/tests/unit/common/test_attachment_state_manager.py

@@ -0,0 +1,108 @@
+# Copyright 2021 RedHat Inc.
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from unittest import mock
+
+from oslo_config import cfg
+from oslotest import base
+
+from glance_store.common import attachment_state_manager as attach_manager
+from glance_store.common import cinder_utils
+from glance_store import exceptions
+
+CONF = cfg.CONF
+
+
+class AttachmentStateManagerTestCase(base.BaseTestCase):
+
+    class FakeAttachmentState:
+        def __init__(self):
+            self.attachments = {mock.sentinel.attachments}
+
+    def setUp(self):
+        super(AttachmentStateManagerTestCase, self).setUp()
+        self.__manager__ = attach_manager.__manager__
+
+    def get_state(self):
+        with self.__manager__.get_state() as state:
+            return state
+
+    def test_get_state_host_not_initialized(self):
+        self.__manager__.state = None
+        self.assertRaises(exceptions.HostNotInitialized,
+                          self.get_state)
+
+    def test_get_state(self):
+        self.__manager__.state = self.FakeAttachmentState()
+        state = self.get_state()
+        self.assertEqual({mock.sentinel.attachments}, state.attachments)
+
+
+class AttachmentStateTestCase(base.BaseTestCase):
+
+    def setUp(self):
+        super(AttachmentStateTestCase, self).setUp()
+        self.attachments = set()
+        self.m = attach_manager._AttachmentState()
+        self.attach_call_1 = [mock.sentinel.client, mock.sentinel.volume_id]
+        self.attach_call_2 = {'mode': mock.sentinel.mode}
+        self.disconnect_vol_call = [mock.sentinel.connection_info,
+                                    mock.sentinel.device]
+        self.detach_call = [mock.sentinel.client, mock.sentinel.attachment_id]
+        self.attachment_dict = {'id': mock.sentinel.attachment_id}
+
+    def _sentinel_attach(self):
+        attachment_id = self.m.attach(
+            mock.sentinel.client, mock.sentinel.volume_id,
+            mock.sentinel.host, mode=mock.sentinel.mode)
+        return attachment_id
+
+    def _sentinel_detach(self, conn):
+        self.m.detach(mock.sentinel.client, mock.sentinel.attachment_id,
+                      mock.sentinel.volume_id, mock.sentinel.host,
+                      conn, mock.sentinel.connection_info,
+                      mock.sentinel.device)
+
+    @mock.patch.object(cinder_utils.API, 'attachment_create')
+    def test_attach(self, mock_attach_create):
+        mock_attach_create.return_value = self.attachment_dict
+        attachment = self._sentinel_attach()
+        mock_attach_create.assert_called_once_with(
+            *self.attach_call_1, **self.attach_call_2)
+        self.assertEqual(mock.sentinel.attachment_id, attachment['id'])
+
+    @mock.patch.object(cinder_utils.API, 'attachment_delete')
+    def test_detach_without_attach(self, mock_attach_delete):
+        ex = exceptions.BackendException
+        conn = mock.MagicMock()
+        mock_attach_delete.side_effect = ex()
+        self.assertRaises(ex, self._sentinel_detach, conn)
+        conn.disconnect_volume.assert_called_once_with(
+            *self.disconnect_vol_call)
+
+    @mock.patch.object(cinder_utils.API, 'attachment_create')
+    @mock.patch.object(cinder_utils.API, 'attachment_delete')
+    def test_detach_with_attach(self, mock_attach_delete, mock_attach_create):
+        conn = mock.MagicMock()
+        mock_attach_create.return_value = self.attachment_dict
+        attachment = self._sentinel_attach()
+        self._sentinel_detach(conn)
+        mock_attach_create.assert_called_once_with(
+            *self.attach_call_1, **self.attach_call_2)
+        self.assertEqual(mock.sentinel.attachment_id, attachment['id'])
+        conn.disconnect_volume.assert_called_once_with(
+            *self.disconnect_vol_call)
+        mock_attach_delete.assert_called_once_with(
+            *self.detach_call)

glance_store/tests/unit/common/test_cinder_utils.py

@@ -74,6 +74,27 @@ class CinderUtilsTestCase(base.BaseTestCase):
             self.volume_api.attachment_create,
             self.fake_client, self.fake_vol_id)
 
+    def test_attachment_create_retries(self):
+
+        fake_attach_id = 'fake-attach-id'
+        # Make create fail two times and succeed on the third attempt.
+        self.fake_client.attachments.create.side_effect = [
+            cinder_exception.BadRequest(400),
+            cinder_exception.BadRequest(400),
+            fake_attach_id]
+
+        # Make sure we get a clean result.
+        fake_attachment_id = self.volume_api.attachment_create(
+            self.fake_client, self.fake_vol_id)
+
+        self.assertEqual(fake_attach_id, fake_attachment_id)
+        # Assert that we called attachment create three times due to the retry
+        # decorator.
+        self.fake_client.attachments.create.assert_has_calls([
+            mock.call(self.fake_vol_id, None, mode=None),
+            mock.call(self.fake_vol_id, None, mode=None),
+            mock.call(self.fake_vol_id, None, mode=None)])
+
     def test_attachment_get(self):
         self.volume_api.attachment_get(self.fake_client, self.fake_attach_id)
         self.fake_client.attachments.show.assert_called_once_with(

glance_store/tests/unit/test_cinder_store.py

@@ -31,6 +31,7 @@ from oslo_concurrency import processutils
 from oslo_utils.secretutils import md5
 from oslo_utils import units
 
+from glance_store.common import attachment_state_manager
 from glance_store.common import cinder_utils
 from glance_store import exceptions
 from glance_store import location
@@ -152,9 +153,11 @@ class TestCinderStore(base.StoreBaseTest,
     def _test_open_cinder_volume(self, open_mode, attach_mode, error,
                                  multipath_supported=False,
                                  enforce_multipath=False,
-                                 encrypted_nfs=False, qcow2_vol=False):
+                                 encrypted_nfs=False, qcow2_vol=False,
+                                 multiattach=False):
         self.config(cinder_mount_point_base=None)
-        fake_volume = mock.MagicMock(id=str(uuid.uuid4()), status='available')
+        fake_volume = mock.MagicMock(id=str(uuid.uuid4()), status='available',
+                                     multiattach=multiattach)
         fake_volume.manager.get.return_value = fake_volume
         fake_volumes = FakeObject(get=lambda id: fake_volume)
         fake_attachment_id = str(uuid.uuid4())
@@ -178,10 +181,20 @@ class TestCinderStore(base.StoreBaseTest,
             yield
 
         def do_open():
-            with self.store._open_cinder_volume(
-                    fake_client, fake_volume, open_mode):
-                if error:
-                    raise error
+            if multiattach:
+                with mock.patch.object(
+                        attachment_state_manager._AttachmentStateManager,
+                        'get_state') as mock_get_state:
+                    mock_get_state.return_value.__enter__.return_value = (
+                        attachment_state_manager._AttachmentState())
+                    with self.store._open_cinder_volume(
+                            fake_client, fake_volume, open_mode):
+                        pass
+            else:
+                with self.store._open_cinder_volume(
+                        fake_client, fake_volume, open_mode):
+                    if error:
+                        raise error
 
         def fake_factory(protocol, root_helper, **kwargs):
             return fake_connector
@@ -298,6 +311,9 @@ class TestCinderStore(base.StoreBaseTest,
     def test_open_cinder_volume_nfs_qcow2_volume(self):
         self._test_open_cinder_volume('rb', 'ro', None, qcow2_vol=True)
 
+    def test_open_cinder_volume_multiattach_volume(self):
+        self._test_open_cinder_volume('rb', 'ro', None, multiattach=True)
+
     def test_cinder_configure_add(self):
         self.assertRaises(exceptions.BadStoreConfiguration,
                           self.store._check_context, None)

glance_store/tests/unit/test_multistore_cinder.py

@@ -33,6 +33,7 @@ from oslo_utils.secretutils import md5
 from oslo_utils import units
 
 import glance_store as store
+from glance_store.common import attachment_state_manager
 from glance_store.common import cinder_utils
 from glance_store import exceptions
 from glance_store import location
@@ -184,9 +185,11 @@ class TestMultiCinderStore(base.MultiStoreBaseTest,
     def _test_open_cinder_volume(self, open_mode, attach_mode, error,
                                  multipath_supported=False,
                                  enforce_multipath=False,
-                                 encrypted_nfs=False, qcow2_vol=False):
+                                 encrypted_nfs=False, qcow2_vol=False,
+                                 multiattach=False):
         self.config(cinder_mount_point_base=None, group='cinder1')
-        fake_volume = mock.MagicMock(id=str(uuid.uuid4()), status='available')
+        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}
@@ -210,10 +213,20 @@ class TestMultiCinderStore(base.MultiStoreBaseTest,
             yield
 
         def do_open():
-            with self.store._open_cinder_volume(
-                    fake_client, fake_volume, open_mode):
-                if error:
-                    raise error
+            if multiattach:
+                with mock.patch.object(
+                        attachment_state_manager._AttachmentStateManager,
+                        'get_state') as mock_get_state:
+                    mock_get_state.return_value.__enter__.return_value = (
+                        attachment_state_manager._AttachmentState())
+                    with self.store._open_cinder_volume(
+                            fake_client, fake_volume, open_mode):
+                        pass
+            else:
+                with self.store._open_cinder_volume(
+                        fake_client, fake_volume, open_mode):
+                    if error:
+                        raise error
 
         def fake_factory(protocol, root_helper, **kwargs):
             return fake_connector
@@ -332,6 +345,9 @@ class TestMultiCinderStore(base.MultiStoreBaseTest,
     def test_open_cinder_volume_nfs_qcow2_volume(self):
         self._test_open_cinder_volume('rb', 'ro', None, qcow2_vol=True)
 
+    def test_open_cinder_volume_multiattach_volume(self):
+        self._test_open_cinder_volume('rb', 'ro', None, multiattach=True)
+
     def test_cinder_check_context(self):
         self.assertRaises(exceptions.BadStoreConfiguration,
                           self.store._check_context, None)

releasenotes/notes/multiattach-volume-handling-1a8446a64463f2cf.yaml

@@ -0,0 +1,12 @@
+---
+prelude: >
+    This release adds support for handling cinder's multiattach volumes
+    in glance cinder store.
+features:
+  - |
+    Glance cinder store now supports handling of multiattach volumes.
+fixes:
+  - |
+    `Bug #1904546 <https://bugs.launchpad.net/cinder/+bug/1904546>`_:
+     Fixed creating multiple instances/volumes from image if multiattach
+     volumes are used.