From daa803b8ee1adcadc7d0e797bd04b287a7df5946 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Tue, 8 Mar 2022 21:37:21 +0100 Subject: [PATCH] LVM: terminate_connection fails if no initiator The LVM driver assumes that all connecting hosts will have the iSCSI initiator installed and configured. If they don't, then there won't be an "initiator" key in the connector properties dictionary and the call to terminate connection will always fail with a KeyError exception on the 'initiator' key. This is the case if we don't have iSCSI configured on the computes because we are only using NVMe-oF volumes with the nvmet target. This patch starts using the dictionary ``get`` method so there is no failure even when the keys don't exist, and it also differentiates by target type so they target the identifier they care about, which is the ``initiator`` for iSCSI and ``nqn`` for NVMe-oF. Closes-Bug: #1966513 Related-Bug: #1786327 Change-Id: Ie967a42188bd020178cb7af527e3dd3ab8975a3d --- .../unit/targets/test_base_iscsi_driver.py | 14 +++++++++++++ .../tests/unit/targets/test_nvmeof_driver.py | 11 ++++++++++ .../unit/volume/drivers/test_lvm_driver.py | 21 +++++++++++++++++++ cinder/volume/drivers/lvm.py | 12 +++++++---- cinder/volume/targets/driver.py | 14 +++++++++++++ cinder/volume/targets/iscsi.py | 5 +++++ cinder/volume/targets/nvmeof.py | 5 +++++ ...ature-terminate-conn-63e3cc1fd1832874.yaml | 7 +++++++ 8 files changed, 85 insertions(+), 4 deletions(-) create mode 100644 releasenotes/notes/nvmeof-premature-terminate-conn-63e3cc1fd1832874.yaml 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.