openstack
/
cinder
Code Issues Proposed changes

Remove unused code path in attachment_delete 

In our manager's attachment_delete method we have a part of the code
that is not only used anywhere in Cinder, but it is also broken, so this
patch removes it.

Removed code was meant to allow deleting all attachments for a volume,
but the condition it's checking is on `attachment_ref` being `None`,
which can never happen since the `get_by_id` method that is assigned to
that variable will raise an exception if the entity is not found.

To fix this the condition would have to check that `attachhment_id` is
`None` instead, but since we are not using that feature we should just
remove the code.

The patch also merges `_do_attachment_delete` into `attachment_delete`
since that's the only place where it is called from.

Change-Id: I6467dc8e40d46d4158b35d40e2ef9ec273cc7e8c
This commit is contained in:
Gorka Eguileor 2021-03-03 11:59:04 +01:00
parent 7c4b626c01
commit b9f525b7dd
2 changed files with 23 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

34
cinder/tests/unit/attachments/test_attachments_manager.py
View File

@ -17,6 +17,7 @@ from oslo_utils import importutils
from cinder import context
from cinder import db
from cinder.db.sqlalchemy import api as sqla_db
from cinder import exception
from cinder.objects import fields
from cinder.objects import volume_attachment
@ -153,14 +154,16 @@ class AttachmentManagerTestCase(test.TestCase):
attachment1.id = fake.UUID1
attachment2.id = fake.UUID2
@mock.patch.object(self.manager.db, 'volume_admin_metadata_delete')
@mock.patch.object(self.manager.db, 'volume_detached')
@mock.patch('cinder.objects.VolumeAttachment.get_by_id',
side_effect=[attachment1, attachment2])
@mock.patch.object(sqla_db, 'volume_admin_metadata_delete')
@mock.patch.object(sqla_db, 'volume_detached')
@mock.patch.object(self.context, 'elevated')
@mock.patch.object(self.manager, '_connection_terminate')
@mock.patch.object(self.manager.driver, 'remove_export')
@mock.patch.object(self.manager.driver, 'detach_volume')
def _test(mock_detach, mock_rm_export, mock_con_term,
mock_elevated, mock_db_detached, mock_db_meta_delete):
def _test(mock_detach, mock_rm_export, mock_con_term, mock_elevated,
mock_db_detached, mock_db_meta_delete, mock_get_attachment):
mock_elevated.return_value = self.context
mock_con_term.return_value = False
@ -168,7 +171,7 @@ class AttachmentManagerTestCase(test.TestCase):
# detach and remove_export
vref.volume_attachment.objects.append(attachment1)
self.manager._do_attachment_delete(self.context, vref, attachment1)
self.manager.attachment_delete(self.context, attachment1.id, vref)
mock_detach.assert_called_once_with(self.context, vref,
attachment1)
@ -188,7 +191,7 @@ class AttachmentManagerTestCase(test.TestCase):
mock_db_detached.reset_mock()
mock_db_meta_delete.reset_mock()
self.manager._do_attachment_delete(self.context, vref, attachment2)
self.manager.attachment_delete(self.context, attachment2.id, vref)
mock_rm_export.assert_not_called()
mock_db_detached.called_once_with(self.context, vref,
@ -225,25 +228,28 @@ class AttachmentManagerTestCase(test.TestCase):
self.assertFalse(has_shared_connection)
term_conn.assert_called_once_with(volume, {}, force=True)
@mock.patch('cinder.objects.VolumeAttachment.get_by_id')
@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
def test_attachment_delete_none_shared_connection(self, mock_meta_del,
mock_vol_detached,
mock_conn_term,
mock_notify,
mock_get_attachment):
# Tests that 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)
self.manager.attachment_delete(
self.context, mock.sentinel.attachment_id, volume)
mock_get_attachment.assert_called_once_with(
self.context, mock.sentinel.attachment_id)
remove_export.assert_not_called()

19
cinder/volume/manager.py
View File

@ -4793,25 +4793,12 @@ class VolumeManager(manager.CleanableManager,
Notifies the backend device that we're detaching the specified
attachment instance.
param: attachment_id: Attachment id to remove
param: vref: Volume object associated with the attachment
param: attachment: Attachment reference object to remove
NOTE if the attachment reference is None, we remove all existing
attachments for the specified volume object.
"""
attachment_ref = objects.VolumeAttachment.get_by_id(context,
attachment_id)
if not attachment_ref:
for attachment in VA_LIST.get_all_by_volume_id(context, vref.id):
self._do_attachment_delete(context, vref, attachment)
else:
self._do_attachment_delete(context, vref, attachment_ref)
def _do_attachment_delete(self,
context: context.RequestContext,
vref,
attachment: objects.VolumeAttachment) -> None:
utils.require_driver_initialized(self.driver)
attachment = objects.VolumeAttachment.get_by_id(context, attachment_id)
self._notify_about_volume_usage(context, vref, "detach.start")
has_shared_connection = self._connection_terminate(context,
vref,