diff --git a/glance_store/_drivers/cinder/store.py b/glance_store/_drivers/cinder/store.py index 6281e923..350a2f7e 100644 --- a/glance_store/_drivers/cinder/store.py +++ b/glance_store/_drivers/cinder/store.py @@ -34,7 +34,6 @@ from oslo_utils import units from glance_store._drivers.cinder import base 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 @@ -727,9 +726,73 @@ class Store(glance_store.driver.Store): except socket.gaierror: return socket.getaddrinfo(host, None, socket.AF_INET)[0][4][0] + def _disconnect_and_detach(self, client, volume_id, multiattach, host, + conn, device, attachment_id): + @utils.synchronized(volume_id, external=True) + def _disconnect_and_detach_with_lock(self, client, volume_id, + multiattach, host, conn, + device, attachment_id): + """This method disconnects and detaches a volume. + + The decision to disconnect a volume is based on the number of + attachments it has on a particular host. If there are > 1 + attachments for a specific volume on a given host, we should + not disconnect the volume. + """ + should_disconnect = False + # If the volume is not multiattach, we should always disconnect + if not multiattach: + # Since the lock is acquired on volume_id, it's non-blocking + # for disconnecting different non-multiattach volumes + should_disconnect = True + else: + # Counting number of attachments on this host + conn_count = 0 + volume = self.volume_api.get(client, volume_id) + attachments = volume.attachments + # When connections are <= 1, we should disconnect + if len(attachments) > 1: + for attachment in attachments: + if attachment['host_name'] == host: + conn_count += 1 + # If we have more than 1 attachment on the same host, + # we should not disconnect otherwise we can safely disconnect + if conn_count <= 1: + should_disconnect = True + + if should_disconnect: + if device: + LOG.debug("Disconnecting volume %s from current host", + volume_id) + # disconnect_volume has it's own lock so it doesn't + # require the additional lock here but to include the + # attachment_delete call, we have to do disconnect here + try: + conn.disconnect_volume(device) + except Exception: + LOG.exception(_LE('Failed to disconnect volume ' + '%(volume_id)s.'), + {'volume_id': volume_id}) + if attachment_id: + # Delete the attachment. + # Cinder volume driver handles unmapping based on attachments + # so we don't need to handle anything here. + # We need to do this inside the lock since we fetch + # attachments to make disconnect decision which could be + # influenced by this call. + self.volume_api.attachment_delete(client, attachment_id) + LOG.debug('Attachment %(attachment_id)s deleted successfully.', + {'attachment_id': attachment_id}) + + return _disconnect_and_detach_with_lock(self, client, volume_id, + multiattach, host, conn, + device, attachment_id) + @contextlib.contextmanager def _open_cinder_volume(self, client, volume, mode): attach_mode = 'rw' if mode == 'wb' else 'ro' + attachment_id = None + conn = None device = None root_helper = self.get_root_helper() priv_context.init(root_helper=shlex.split(root_helper)) @@ -738,17 +801,13 @@ class Store(glance_store.driver.Store): use_multipath = self.store_conf.cinder_use_multipath enforce_multipath = self.store_conf.cinder_enforce_multipath volume_id = volume.id + multiattach = volume.multiattach connector_prop = connector.get_connector_properties( root_helper, my_ip, use_multipath, enforce_multipath, host=host) - 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_create(client, volume_id, + mode=attach_mode) LOG.debug('Attachment %(attachment_id)s created successfully.', {'attachment_id': attachment['id']}) @@ -795,23 +854,10 @@ class Store(glance_store.driver.Store): '%(volume_id)s.'), {'volume_id': volume.id}) raise finally: - if device: - try: - if volume.multiattach: - attachment_state_manager.detach( - client, attachment_id, volume_id, host, conn, - 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.'), - {'volume_id': volume.id}) - - if not volume.multiattach: - self.volume_api.attachment_delete(client, attachment_id) + self._disconnect_and_detach(client, volume_id, multiattach, + host, conn, device, attachment_id) + if self.volume_connector_map.get(volume.id): + del self.volume_connector_map[volume.id] def _cinder_volume_data_iterator(self, client, volume, max_size, offset=0, chunk_size=None, partial_length=None): diff --git a/glance_store/common/attachment_state_manager.py b/glance_store/common/attachment_state_manager.py deleted file mode 100644 index 04cad12d..00000000 --- a/glance_store/common/attachment_state_manager.py +++ /dev/null @@ -1,261 +0,0 @@ -# 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 _AttachmentStateManager') - 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(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) diff --git a/glance_store/tests/unit/cinder/test_cinder_base.py b/glance_store/tests/unit/cinder/test_cinder_base.py index 59169284..fdd8fef0 100644 --- a/glance_store/tests/unit/cinder/test_cinder_base.py +++ b/glance_store/tests/unit/cinder/test_cinder_base.py @@ -31,8 +31,8 @@ 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.common import utils from glance_store import exceptions from glance_store import location @@ -208,11 +208,31 @@ class TestCinderStoreBase(object): enforce_multipath=False, encrypted_nfs=False, qcow2_vol=False, multiattach=False, - update_attachment_error=None): + update_attachment_error=None, + disconnect_multiattach=True): fake_volume = mock.MagicMock(id=str(uuid.uuid4()), status='available', multiattach=multiattach) fake_attachment_id = str(uuid.uuid4()) fake_attachment_create = {'id': fake_attachment_id} + if disconnect_multiattach: + fake_volume.attachments = [ + { + 'id': fake_attachment_id, + 'host_name': 'fake_host', + } + ] + else: + fake_attachment_id_2 = str(uuid.uuid4()) + fake_volume.attachments = [ + { + 'id': fake_attachment_id, + 'host_name': 'fake_host', + }, + { + 'id': fake_attachment_id_2, + 'host_name': 'fake_host', + }, + ] if encrypted_nfs or qcow2_vol: fake_attachment_update = mock.MagicMock( id=fake_attachment_id, @@ -235,24 +255,19 @@ class TestCinderStoreBase(object): yield def do_open(): - 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 + 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 + def fake_synchronized(*args, **kwargs): + def decorator(f): + return f + return decorator + root_helper = "sudo glance-rootwrap /etc/glance/rootwrap.conf" with mock.patch.object(cinder.Store, '_wait_volume_status', @@ -282,7 +297,9 @@ class TestCinderStoreBase(object): 'gethostname') as mock_get_host, \ mock.patch.object(socket, 'getaddrinfo') as mock_get_host_ip, \ - mock.patch.object(cinder.strutils, 'mask_dict_password'): + mock.patch.object(cinder.strutils, 'mask_dict_password'), \ + mock.patch.object(utils, 'synchronized', + side_effect=fake_synchronized): if update_attachment_error: attach_update.side_effect = update_attachment_error @@ -326,8 +343,13 @@ class TestCinderStoreBase(object): host=fake_host) fake_connector.connect_volume.assert_called_once_with( mock.ANY) - fake_connector.disconnect_volume.assert_called_once_with( - mock.ANY, fake_devinfo, force=True) + if not multiattach or ( + multiattach and disconnect_multiattach): + disconnect = fake_connector.disconnect_volume + disconnect.assert_called_once_with( + mock.ANY, fake_devinfo, force=True) + else: + 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) @@ -375,6 +397,10 @@ class TestCinderStoreBase(object): def test_open_cinder_volume_multiattach_volume(self): self._test_open_cinder_volume('rb', 'ro', None, multiattach=True) + def test_open_cinder_volume_multiattach_volume_not_disconnect(self): + self._test_open_cinder_volume('rb', 'ro', None, multiattach=True, + disconnect_multiattach=False) + def _fake_volume_type_check(self, name): if name != 'some_type': raise cinder.cinder_exception.NotFound(code=404) diff --git a/glance_store/tests/unit/common/test_attachment_state_manager.py b/glance_store/tests/unit/common/test_attachment_state_manager.py deleted file mode 100644 index 23f66d30..00000000 --- a/glance_store/tests/unit/common/test_attachment_state_manager.py +++ /dev/null @@ -1,116 +0,0 @@ -# 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 cinderclient import exceptions as cinder_exception -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.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) - - @mock.patch.object(cinder_utils.API, 'attachment_create') - def test_attach_fails(self, mock_attach_create): - mock_attach_create.side_effect = cinder_exception.BadRequest(code=400) - self.assertRaises( - cinder_exception.BadRequest, self.m.attach, - mock.sentinel.client, mock.sentinel.volume_id, - mock.sentinel.host, mode=mock.sentinel.mode) diff --git a/releasenotes/notes/fix-multiattach-multiprocessing-56120e9fa7c40cc8.yaml b/releasenotes/notes/fix-multiattach-multiprocessing-56120e9fa7c40cc8.yaml new file mode 100644 index 00000000..d29bf014 --- /dev/null +++ b/releasenotes/notes/fix-multiattach-multiprocessing-56120e9fa7c40cc8.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Cinder: Fixed locking mechanism to allow multiple workers + to use the same image safely.