Merge "LVM: terminate_connection fails if no initiator"

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

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.