From 0c7f5fcfe50d6f51896e76818330af156907fcd2 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Tue, 18 May 2021 18:26:21 +0200 Subject: [PATCH] NVMe-oF: Flush on disconnect The current NVMe-oF connector code in os-brick doesn't flush on disconnect, as it just calls nvme disconnect -n Which doesn't flush on the `nvme` code or the kernel code. This patch adds the normal block device ioctl flush call: blockdev --flushbufs /dev/nvme0n1 The patch also adds and extends disconnect unit tests that were removed when we added the new NVMe connector code (were not removed when we added old code back). Closes-Bug: #1903032 Change-Id: Ie6c8bdf1f3a629206da019405aee11c802a6a9bb (cherry picked from commit 85dbd0f5d72475f97ca6716cbe1b86ece05e3fbd) --- os_brick/initiator/connectors/nvmeof.py | 18 +-- .../tests/initiator/connectors/test_nvmeof.py | 111 ++++++++++++++++++ .../notes/nvme-flush-f31ab337224e5d3d.yaml | 6 + 3 files changed, 128 insertions(+), 7 deletions(-) create mode 100644 releasenotes/notes/nvme-flush-f31ab337224e5d3d.yaml diff --git a/os_brick/initiator/connectors/nvmeof.py b/os_brick/initiator/connectors/nvmeof.py index 5d3259b9e..844f80608 100644 --- a/os_brick/initiator/connectors/nvmeof.py +++ b/os_brick/initiator/connectors/nvmeof.py @@ -370,6 +370,10 @@ class NVMeOFConnector(base.BaseLinuxConnector): {'device_path': device_path, 'conn_nqn': conn_nqn}) return + exc = exception.ExceptionChainer() + with exc.context(force, 'Flushing %s failed', device_path): + self._linuxscsi.flush_device_io(device_path) + LOG.debug( "Trying to disconnect from device %(device_path)s with " "subnqn %(conn_nqn)s", @@ -379,19 +383,19 @@ class NVMeOFConnector(base.BaseLinuxConnector): 'disconnect', '-n', conn_nqn] - try: + with exc.context(force, "Failed to disconnect from NVMe nqn " + "%(conn_nqn)s with device_path %(device_path)s", + {'conn_nqn': conn_nqn, 'device_path': device_path}): self._execute( *cmd, root_helper=self._root_helper, run_as_root=True) - except putils.ProcessExecutionError: - LOG.error( - "Failed to disconnect from NVMe nqn " - "%(conn_nqn)s with device_path %(device_path)s", - {'conn_nqn': conn_nqn, 'device_path': device_path}) + if exc: + LOG.warning('There were errors removing %s, leftovers may remain ' + 'in the system', device_path) if not ignore_errors: - raise + raise exc @utils.trace @synchronized('extend_volume') diff --git a/os_brick/tests/initiator/connectors/test_nvmeof.py b/os_brick/tests/initiator/connectors/test_nvmeof.py index 573137563..e5de73f03 100644 --- a/os_brick/tests/initiator/connectors/test_nvmeof.py +++ b/os_brick/tests/initiator/connectors/test_nvmeof.py @@ -192,6 +192,117 @@ class NVMeOFConnectorTestCase(test_connector.ConnectorTestCase): self.assertRaises(exception.VolumeDeviceNotFound, self.connector.connect_volume, connection_properties) + @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_device_io', autospec=True) + @mock.patch.object(nvmeof.NVMeOFConnector, '_get_nvme_devices', + autospec=True) + @mock.patch.object(nvmeof.NVMeOFConnector, '_execute', autospec=True) + @mock.patch('os_brick.utils._time_sleep') + def test_disconnect_volume_nova(self, mock_sleep, mock_execute, + mock_devices, mock_flush): + device = '/dev/nvme0n1' + connection_properties = {'target_portal': 'portal', + 'target_port': 1, + 'nqn': 'nqn.volume_123', + 'device_path': device, + 'transport_type': 'rdma'} + mock_devices.return_value = [device] + + self.connector.disconnect_volume(connection_properties, None) + + mock_flush.assert_called_once_with(mock.ANY, device) + mock_execute.assert_called_once_with( + self.connector, + 'nvme', 'disconnect', '-n', 'nqn.volume_123', + root_helper=None, + run_as_root=True) + + @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_device_io', autospec=True) + @mock.patch.object(nvmeof.NVMeOFConnector, '_get_nvme_devices', + autospec=True) + @mock.patch.object(nvmeof.NVMeOFConnector, '_execute', autospec=True) + @mock.patch('os_brick.utils._time_sleep') + def test_disconnect_volume_cinder(self, mock_sleep, mock_execute, + mock_devices, mock_flush): + device = '/dev/nvme0n1' + connection_properties = {'target_portal': 'portal', + 'target_port': 1, + 'nqn': 'nqn.volume_123', + 'transport_type': 'rdma'} + device_info = {'path': device} + mock_devices.return_value = [device] + + self.connector.disconnect_volume(connection_properties, + device_info, + ignore_errors=True) + + mock_flush.assert_called_once_with(mock.ANY, device) + mock_execute.assert_called_once_with( + self.connector, + 'nvme', 'disconnect', '-n', 'nqn.volume_123', + root_helper=None, + run_as_root=True) + + @ddt.data({'force': False, 'expected': putils.ProcessExecutionError}, + {'force': True, 'expected': exception.ExceptionChainer}) + @ddt.unpack + @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_device_io', autospec=True) + @mock.patch.object(nvmeof.NVMeOFConnector, '_get_nvme_devices', + autospec=True) + @mock.patch.object(nvmeof.NVMeOFConnector, '_execute', autospec=True) + @mock.patch('os_brick.utils._time_sleep') + def test_disconnect_volume_raise(self, mock_sleep, mock_execute, + mock_devices, mock_flush, + force, expected): + device = '/dev/nvme0n1' + mock_execute.side_effect = putils.ProcessExecutionError + mock_devices.return_value = device + connection_properties = {'target_portal': 'portal', + 'target_port': 1, + 'nqn': 'nqn.volume_123', + 'device_path': device, + 'transport_type': 'rdma'} + + self.assertRaises(expected, + self.connector.disconnect_volume, + connection_properties, + None, force) + mock_flush.assert_called_once_with(mock.ANY, device) + mock_execute.assert_called_once_with( + self.connector, + 'nvme', 'disconnect', '-n', 'nqn.volume_123', + root_helper=None, + run_as_root=True) + + @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_device_io', autospec=True) + @mock.patch.object(nvmeof.NVMeOFConnector, '_get_nvme_devices', + autospec=True) + @mock.patch.object(nvmeof.NVMeOFConnector, '_execute', autospec=True) + @mock.patch('os_brick.utils._time_sleep') + def test_disconnect_volume_force_ignore_errors(self, mock_sleep, + mock_execute, mock_devices, + mock_flush): + device = '/dev/nvme0n1' + mock_flush.side_effect = putils.ProcessExecutionError + mock_execute.side_effect = putils.ProcessExecutionError + mock_devices.return_value = device + connection_properties = {'target_portal': 'portal', + 'target_port': 1, + 'nqn': 'nqn.volume_123', + 'device_path': device, + 'transport_type': 'rdma'} + + res = self.connector.disconnect_volume(connection_properties, + None, + force=True, + ignore_errors=True) + self.assertIsNone(res) + mock_flush.assert_called_once_with(mock.ANY, device) + mock_execute.assert_called_once_with( + self.connector, + 'nvme', 'disconnect', '-n', 'nqn.volume_123', + root_helper=None, + run_as_root=True) + @mock.patch.object(nvmeof.NVMeOFConnector, '_get_fs_type') def test_disconnect_unreplicated_volume_nova(self, mock_get_fs_type): connection_properties = { diff --git a/releasenotes/notes/nvme-flush-f31ab337224e5d3d.yaml b/releasenotes/notes/nvme-flush-f31ab337224e5d3d.yaml new file mode 100644 index 000000000..7f034f04d --- /dev/null +++ b/releasenotes/notes/nvme-flush-f31ab337224e5d3d.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + NVMe-oF connector `bug #1903032 + `_: Fixed not flushing + device on disconnect.