diff --git a/cinder/tests/unit/targets/test_base_iscsi_driver.py b/cinder/tests/unit/targets/test_base_iscsi_driver.py index 4536b302840..4d5e9544a6d 100644 --- a/cinder/tests/unit/targets/test_base_iscsi_driver.py +++ b/cinder/tests/unit/targets/test_base_iscsi_driver.py @@ -12,6 +12,7 @@ from unittest import mock +import ddt from oslo_config import cfg from cinder import context @@ -28,6 +29,7 @@ class FakeIncompleteDriver(iscsi.ISCSITarget): pass +@ddt.ddt class TestBaseISCSITargetDriver(tf.TargetDriverFixture): def setUp(self): @@ -183,3 +185,15 @@ class TestBaseISCSITargetDriver(tf.TargetDriverFixture): self.testvol)) self.target.db.volume_get.assert_called_once_with( ctxt, self.testvol['id']) + + def test_are_same_connector(self): + res = self.target.are_same_connector({'initiator': 'iqn'}, + {'initiator': 'iqn'}) + self.assertTrue(res) + + @ddt.data(({}, {}), ({}, {'initiator': 'iqn'}), ({'initiator': 'iqn'}, {}), + ({'initiator': 'iqn1'}, {'initiator': 'iqn2'})) + @ddt.unpack + def test_are_same_connector_different(self, a_conn_props, b_conn_props): + res = self.target.are_same_connector(a_conn_props, b_conn_props) + self.assertFalse(bool(res)) diff --git a/cinder/tests/unit/targets/test_nvmeof_driver.py b/cinder/tests/unit/targets/test_nvmeof_driver.py index 277b75c3cd8..ae48f9b7af5 100644 --- a/cinder/tests/unit/targets/test_nvmeof_driver.py +++ b/cinder/tests/unit/targets/test_nvmeof_driver.py @@ -162,3 +162,14 @@ class TestNVMeOFDriver(tf.TargetDriverFixture): FakeNVMeOFDriver, root_helper=utils.get_root_helper(), configuration=self.configuration) + + def test_are_same_connector(self): + res = self.target.are_same_connector({'nqn': 'nvme'}, {'nqn': 'nvme'}) + self.assertTrue(res) + + @ddt.data(({}, {}), ({}, {'nqn': 'nvmE'}), ({'nqn': 'nvmeE'}, {}), + ({'nqn': 'nvme1'}, {'nqn': 'nvme2'})) + @ddt.unpack + def test_are_same_connector_different(self, a_conn_props, b_conn_props): + res = self.target.are_same_connector(a_conn_props, b_conn_props) + self.assertFalse(bool(res)) diff --git a/cinder/tests/unit/volume/drivers/test_lvm_driver.py b/cinder/tests/unit/volume/drivers/test_lvm_driver.py index 12bfa00d4ab..e9d986b9d70 100644 --- a/cinder/tests/unit/volume/drivers/test_lvm_driver.py +++ b/cinder/tests/unit/volume/drivers/test_lvm_driver.py @@ -1029,6 +1029,10 @@ class LVMISCSITestCase(test_driver.BaseDriverTestCase): lvm_driver = lvm.LVMVolumeDriver( configuration=self.configuration, vg_obj=vg_obj) + mock_same = self.mock_object( + lvm_driver.target_driver, 'are_same_connector', + side_effect=lvm_driver.target_driver.are_same_connector) + with mock.patch.object(lvm_driver.target_driver, 'terminate_connection') as mock_term_conn: @@ -1037,14 +1041,31 @@ class LVMISCSITestCase(test_driver.BaseDriverTestCase): self.assertTrue(lvm_driver.terminate_connection(vol, host1_connector)) mock_term_conn.assert_not_called() + self.assertEqual(3, mock_same.call_count) + mock_same.assert_has_calls(( + mock.call(host1_connector, host1_connector), + mock.call(host1_connector, host1_connector), + mock.call(host2_connector, host1_connector))) + mock_same.reset_mock() # Verify that terminate_connection is called against either host # when only one active attachment per host is present. vol.volume_attachment.objects.remove(host1_attachment1) self.assertTrue(lvm_driver.terminate_connection(vol, host1_connector)) + self.assertEqual(2, mock_same.call_count) + mock_same.assert_has_calls(( + mock.call(host1_connector, host1_connector), + mock.call(host2_connector, host1_connector))) + mock_same.reset_mock() + self.assertTrue(lvm_driver.terminate_connection(vol, host2_connector)) + self.assertEqual(2, mock_same.call_count) + mock_same.assert_has_calls(( + mock.call(host1_connector, host2_connector), + mock.call(host2_connector, host2_connector))) + mock_same.reset_mock() mock_term_conn.assert_has_calls([mock.call(vol, host1_connector), mock.call(vol, host2_connector)]) diff --git a/cinder/volume/drivers/lvm.py b/cinder/volume/drivers/lvm.py index 83ca05e8314..eb59360fa6c 100644 --- a/cinder/volume/drivers/lvm.py +++ b/cinder/volume/drivers/lvm.py @@ -858,11 +858,15 @@ class LVMVolumeDriver(driver.VolumeDriver): # one attachment left for the host specified by the connector to # remove, otherwise the ACL will be removed prematurely while other # attachments on the same host are still accessing the volume. + def same_connector(attach): + return (attach.connector + and self.target_driver.are_same_connector(attach.connector, + connector)) + attachments = volume.volume_attachment - if volume.multiattach: - if sum(1 for a in attachments if a.connector and - a.connector['initiator'] == connector['initiator']) > 1: - return True + if (volume.multiattach + and sum(1 for a in filter(same_connector, attachments)) > 1): + return True self.target_driver.terminate_connection(volume, connector, **kwargs) return len(attachments) > 1 diff --git a/cinder/volume/targets/driver.py b/cinder/volume/targets/driver.py index 4ec2070f678..c67bdad647c 100644 --- a/cinder/volume/targets/driver.py +++ b/cinder/volume/targets/driver.py @@ -68,3 +68,17 @@ class Target(object, metaclass=abc.ABCMeta): def terminate_connection(self, volume, connector, **kwargs): """Disallow connection from connector.""" pass + + @staticmethod + def are_same_connector(A, B): + """Whether 2 connectors belong to the same host or not. + + This is used for multi attach volumes, to be able to know when there + are no more attachments on a given host. + + This is the generic implementation, but specific targets may overwrite + it. For example iSCSI would check the the "initiator" key instead, and + NVMe-oF would check the "nqn" key. + """ + a_host = A.get('host') + return a_host and (a_host == B.get('host')) diff --git a/cinder/volume/targets/iscsi.py b/cinder/volume/targets/iscsi.py index 1e89a06c3ef..46bea8ef346 100644 --- a/cinder/volume/targets/iscsi.py +++ b/cinder/volume/targets/iscsi.py @@ -355,6 +355,11 @@ class ISCSITarget(driver.Target): def _do_tgt_update(self, name, force=False): pass + @staticmethod + def are_same_connector(A, B): + a_initiator = A.get('initiator') + return a_initiator and (a_initiator == B.get('initiator')) + class SanISCSITarget(ISCSITarget): """iSCSI target for san devices. diff --git a/cinder/volume/targets/nvmeof.py b/cinder/volume/targets/nvmeof.py index 9513fa8b488..2de59b8b8f3 100644 --- a/cinder/volume/targets/nvmeof.py +++ b/cinder/volume/targets/nvmeof.py @@ -163,6 +163,11 @@ class NVMeOF(driver.Target): def terminate_connection(self, volume, connector, **kwargs): pass + @staticmethod + def are_same_connector(A, B): + a_nqn = A.get('nqn') + return a_nqn and (a_nqn == B.get('nqn')) + def create_export(self, context, volume, volume_path): """Creates export data for a logical volume.""" diff --git a/releasenotes/notes/nvmeof-premature-terminate-conn-63e3cc1fd1832874.yaml b/releasenotes/notes/nvmeof-premature-terminate-conn-63e3cc1fd1832874.yaml new file mode 100644 index 00000000000..0cb120fc1db --- /dev/null +++ b/releasenotes/notes/nvmeof-premature-terminate-conn-63e3cc1fd1832874.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + nvmeof target `bug #1966513 + `_: Fixed + LVM failing on terminate_connection if the connecting host doesn't have an + iSCSI initiator name setup, for example if LVM is using the nvmet target.