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

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

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

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

cinder/volume/drivers/lvm.py

@@ -858,10 +858,14 @@ 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:
+        if (volume.multiattach
+                and sum(1 for a in filter(same_connector, attachments)) > 1):
             return True
 
         self.target_driver.terminate_connection(volume, connector, **kwargs)

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

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.

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."""
 

releasenotes/notes/nvmeof-premature-terminate-conn-63e3cc1fd1832874.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    nvmeof target `bug #1966513
+    <https://bugs.launchpad.net/cinder/+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.