Don't call driver.terminate_connection if there is no connector
It's possible to attach a volume to a shelved offloaded instance, which is not on a compute host - which means no host connector, and then detach that volume while the instance is unshelved. There is a test in Tempest just for this scenario. When nova calls attachment_delete on detach, the cinder volume manager is calling the driver.terminate_connection method regardless of there being a host connector, and some drivers blow up on that. This used to "work" with the pre-3.27 attach flow in nova because in the detach case where there was no connector, nova would simply not call os-terminate_connection, but with the new 3.27+ flow for attach, nova only calls attachment_delete regardless of there being a connector. This change simply checks to see if the attachment has a connector and if not, it avoids the call to the driver to terminate a connection which does not exist. Change-Id: I496e45608798a6a5d9606f9594feeb8b60855d1a Closes-Bug: #1738254
This commit is contained in:
Matt Riedemann
@@ -197,3 +197,54 @@ class AttachmentManagerTestCase(test.TestCase):
|
||||
mock_db_meta_delete.called_once_with(self.context, vref.id,
|
||||
'attached_mode')
|
||||
_test()
|
||||
|
||||
def test_connection_terminate_no_connector_force_false(self):
|
||||
# Tests that calling _connection_terminate with an attachment that
|
||||
# does not have a connector will not call the driver and return None
|
||||
# if the force flag is False.
|
||||
attachment = mock.MagicMock(connector={})
|
||||
with mock.patch.object(self.manager.driver, '_initialized',
|
||||
create=True, new=True):
|
||||
with mock.patch.object(self.manager.driver,
|
||||
'terminate_connection') as term_conn:
|
||||
has_shared_connection = self.manager._connection_terminate(
|
||||
self.context, mock.sentinel.volume, attachment)
|
||||
self.assertIsNone(has_shared_connection)
|
||||
term_conn.assert_not_called()
|
||||
|
||||
def test_connection_terminate_no_connector_force_true(self):
|
||||
# Tests that calling _connection_terminate with an attachment that
|
||||
# does not have a connector will call the driver when force is True.
|
||||
volume = mock.MagicMock()
|
||||
attachment = mock.MagicMock(connector={})
|
||||
with mock.patch.object(self.manager.driver, '_initialized',
|
||||
create=True, new=True):
|
||||
with mock.patch.object(self.manager.driver,
|
||||
'terminate_connection') as term_conn:
|
||||
has_shared_connection = self.manager._connection_terminate(
|
||||
self.context, volume, attachment, force=True)
|
||||
self.assertFalse(has_shared_connection)
|
||||
term_conn.assert_called_once_with(volume, {}, force=True)
|
||||
|
||||
@mock.patch('cinder.volume.manager.VolumeManager.'
|
||||
'_notify_about_volume_usage')
|
||||
@mock.patch('cinder.volume.manager.VolumeManager._connection_terminate',
|
||||
return_value=None)
|
||||
@mock.patch('cinder.db.volume_detached')
|
||||
@mock.patch('cinder.db.volume_admin_metadata_delete')
|
||||
def test_do_attachment_delete_none_shared_connection(self, mock_meta_del,
|
||||
mock_vol_detached,
|
||||
mock_conn_term,
|
||||
mock_notify):
|
||||
# Tests that _do_attachment_delete does not call remove_export
|
||||
# if _connection_terminate returns None indicating there is nothing
|
||||
# to consider for the export.
|
||||
volume = mock.MagicMock()
|
||||
attachment = mock.MagicMock()
|
||||
with mock.patch.object(self.manager.driver, '_initialized',
|
||||
create=True, new=True):
|
||||
with mock.patch.object(self.manager.driver,
|
||||
'remove_export') as remove_export:
|
||||
self.manager._do_attachment_delete(
|
||||
self.context, volume, attachment)
|
||||
remove_export.assert_not_called()
|
||||
|
||||
@@ -4416,9 +4416,24 @@ class VolumeManager(manager.CleanableManager,
|
||||
|
||||
def _connection_terminate(self, context, volume,
|
||||
attachment, force=False):
|
||||
"""Remove a volume connection, but leave attachment."""
|
||||
"""Remove a volume connection, but leave attachment.
|
||||
|
||||
Exits early if the attachment does not have a connector and returns
|
||||
None to indicate shared connections are irrelevant.
|
||||
"""
|
||||
utils.require_driver_initialized(self.driver)
|
||||
connector = attachment.connector
|
||||
if not connector and not force:
|
||||
# It's possible to attach a volume to a shelved offloaded server
|
||||
# in nova, and a shelved offloaded server is not on a compute host,
|
||||
# which means the attachment was made without a host connector,
|
||||
# so if we don't have a connector we can't terminate a connection
|
||||
# that was never actually made to the storage backend, so just
|
||||
# log a message and exit.
|
||||
LOG.debug('No connector for attachment %s; skipping storage '
|
||||
'backend terminate_connection call.', attachment.id)
|
||||
# None indicates we don't know and don't care.
|
||||
return None
|
||||
try:
|
||||
shared_connections = self.driver.terminate_connection(volume,
|
||||
connector,
|
||||
@@ -4471,7 +4486,7 @@ class VolumeManager(manager.CleanableManager,
|
||||
{'attachment_id': attachment.id},
|
||||
resource=vref)
|
||||
self.driver.detach_volume(context, vref, attachment)
|
||||
if not has_shared_connection:
|
||||
if has_shared_connection is not None and not has_shared_connection:
|
||||
self.driver.remove_export(context.elevated(), vref)
|
||||
except Exception:
|
||||
# FIXME(jdg): Obviously our volume object is going to need some
|
||||
|
||||
Reference in New Issue
Block a user