openstack/glance_store
Code Issues Proposed changes

Cinder: Handle multiattach volumes in multi worker env

Browse Source 
Since glance completed the support for uWSGI[1], devstack[2]
along with other deployment tools have started to default
glance in uWSGI mode.
The problem we currently face is that the attachment state
manager (written with threading locks) is not capable of
handling multiple uWSGI processes accessing the same image-volume
with multiple attach/detach operations which are not coordinated.

This patch aims to address the problem with a different approach
keeping the following things in mind:

1. Attachment create doesn't require locking since we already handle
it with Cinder attachment states ('reserved', 'attaching', 'in-use')
and the retry mechanism while creating attachment[3]

2. While disconnecting, we need to only perform it if there are
no other attachments for same volume

3. Delete attachment should always be performed to remove
attachment records from Cinder DB to make decision for step 2.
NOTE that Cinder volume driver handles case for multiattach
volumes and avoids unmapping if LUN is attached > 1 times.

With the following in mind, the new approach is:

1. Stop persisting attachment state in memory and rely on
Cinder DB since that is more reliable and fault tolerant.

2. Removing locking from attachment create call

3. Use external/file locks for disconnect and detach operations

4. While disconnecting, acquire file lock and count number
of attachments for a specific volume and don't disconnect if
attachments > 1

5. For non-multiattach volumes, always disconnect

6. Delete the volume attachment inside file lock to ensure
the attachment counting is accurate.

[1] https://review.opendev.org/c/openstack/glance/+/742065
[2] https://review.opendev.org/c/openstack/devstack/+/932201
[3] https://review.opendev.org/c/openstack/glance_store/+/786410

Closes-Bug: #1965679
Closes-Bug: #2125840

Depends-On: https://review.opendev.org/c/openstack/cinder/+/962409

Signed-off-by: Rajat Dhasmana <rajatdhasmana@gmail.com>
Change-Id: I32fce5aeebe3319f167affa954c681598f81ad74
This commit is contained in:
Rajat Dhasmana
2025-09-26 21:28:25 +00:00
parent 3d55084cde
commit 915dffabc1
5 changed files with 121 additions and 421 deletions
Download Patch File Download Diff File Expand all files Collapse all files

96
glance_store/_drivers/cinder/store.py
View File

@@ -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):

261
glance_store/common/attachment_state_manager.py
View File

@@ -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)

64
glance_store/tests/unit/cinder/test_cinder_base.py
View File

@@ -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)

116
glance_store/tests/unit/common/test_attachment_state_manager.py
View File

@@ -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)

5
releasenotes/notes/fix-multiattach-multiprocessing-56120e9fa7c40cc8.yaml Normal file
View File

@@ -0,0 +1,5 @@
---
fixes:
- |
Cinder: Fixed locking mechanism to allow multiple workers
to use the same image safely.