From ec22c32de6820184d7737c5af70e573c0634cd38 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Thu, 14 Sep 2023 12:19:26 +0200 Subject: [PATCH] NVMe-oF: Fix attach when reconnecting When an nvme subsystem has all portals in connecting state and we try to attach a new volume to that same subsystem it will fail. We can reproduce it with LVM+nvmet if we configure it to share targets and then: - Create instance - Attach 2 volumes - Delete instance (this leaves the subsystem in connecting state [1]) - Create instance - Attach volume <== FAILS The problem comes from the '_connect_target' method that ignores subsystems in 'connecting' state, so if they are all in that state it considers it equivalent to all portals being inaccessible. This patch changes this behavior and if we cannot connect to a target but we have portals in 'connecting' state we wait for the next retry of the nvme linux driver. Specifically we wait 10 more seconds that the interval between retries. [1]: https://bugs.launchpad.net/nova/+bug/2035375 Closes-Bug: #2035695 Change-Id: Ife710f52c339d67f2dcb160c20ad0d75480a1f48 --- os_brick/initiator/connectors/nvmeof.py | 41 ++++- .../tests/initiator/connectors/test_nvmeof.py | 152 +++++++++++++++++- .../nvmeof-connecting-788f77a42fe7dd3b.yaml | 7 + 3 files changed, 194 insertions(+), 6 deletions(-) create mode 100644 releasenotes/notes/nvmeof-connecting-788f77a42fe7dd3b.yaml diff --git a/os_brick/initiator/connectors/nvmeof.py b/os_brick/initiator/connectors/nvmeof.py index 7cb46df63..03639bf5c 100644 --- a/os_brick/initiator/connectors/nvmeof.py +++ b/os_brick/initiator/connectors/nvmeof.py @@ -133,6 +133,9 @@ class Portal(object): """Representation of an NVMe-oF Portal with some related operations.""" LIVE = 'live' MISSING = None # Unkown or not present in the system + CONNECTING = 'connecting' + # Default value of reconnect_delay in sysfs + DEFAULT_RECONNECT_DELAY = 10 controller: Optional[str] = None # Don't know controller name on start def __str__(self) -> str: @@ -176,6 +179,15 @@ class Portal(object): return ctrl_property('state', self.controller) return None + @property + def reconnect_delay(self) -> int: + # 10 seconds is the default value of reconnect_delay + if self.controller: + res = ctrl_property('reconnect_delay', self.controller) + if res is not None: + return int(res) + return self.DEFAULT_RECONNECT_DELAY + def get_device(self) -> Optional[str]: """Get a device path using available volume identification markers. @@ -683,6 +695,8 @@ class NVMeOFConnector(base.BaseLinuxConnector): # Use a class attribute since a restart is needed to change it on the host native_multipath_supported = None + # Time we think is more than reasonable to establish an NVMe-oF connection + TIME_TO_CONNECT = 10 def __init__(self, root_helper: str, @@ -861,6 +875,7 @@ class NVMeOFConnector(base.BaseLinuxConnector): """ connected = False missing_portals = [] + reconnecting_portals = [] for portal in target.portals: state = portal.state # store it so we read only once from sysfs @@ -871,6 +886,9 @@ class NVMeOFConnector(base.BaseLinuxConnector): # Remember portals that are not present in the system elif state == portal.MISSING: missing_portals.append(portal) + elif state == portal.CONNECTING: + LOG.debug('%s is reconnecting', portal) + reconnecting_portals.append(portal) # Ignore reconnecting/dead portals else: LOG.debug('%s exists but is %s', portal, state) @@ -905,11 +923,32 @@ class NVMeOFConnector(base.BaseLinuxConnector): LOG.warning('Race condition with some other application ' 'when connecting to %s, please check your ' 'system configuration.', portal) - connected = True + state = portal.state + if state == portal.LIVE: + connected = True + elif state == portal.CONNECTING: + reconnecting_portals.append(portal) + else: + LOG.error('Ignoring %s due to unknown state (%s)', + portal, state) if not do_multipath: break # We are connected + if not connected and reconnecting_portals: + delay = self.TIME_TO_CONNECT + max(p.reconnect_delay + for p in reconnecting_portals) + LOG.debug('Waiting %s seconds for some nvme controllers to ' + 'reconnect', delay) + timeout = time.time() + delay + while time.time() < timeout: + time.sleep(1) + if any(p.is_live for p in reconnecting_portals): + LOG.debug('Reconnected') + connected = True + break + LOG.debug('No controller reconnected') + if not connected: raise exception.VolumeDeviceNotFound(device=target.nqn) diff --git a/os_brick/tests/initiator/connectors/test_nvmeof.py b/os_brick/tests/initiator/connectors/test_nvmeof.py index 5d7e5fb9b..e1a745052 100644 --- a/os_brick/tests/initiator/connectors/test_nvmeof.py +++ b/os_brick/tests/initiator/connectors/test_nvmeof.py @@ -160,6 +160,13 @@ class PortalTestCase(test_base.TestCase): self.assertIs(expected, self.portal.is_live) mock_state.assert_called_once_with() + @mock.patch.object(nvmeof, 'ctrl_property', return_value='10') + def test_reconnect_delay(self, mock_property): + """Reconnect delay returns an int.""" + self.portal.controller = 'nvme0' + self.assertIs(10, self.portal.reconnect_delay) + mock_property.assert_called_once_with('reconnect_delay', 'nvme0') + @mock.patch.object(nvmeof, 'ctrl_property') def test_state(self, mock_property): """State uses sysfs to check the value.""" @@ -1360,6 +1367,11 @@ class NVMeOFConnectorTestCase(test_connector.ConnectorTestCase): else: mock_cli.assert_not_called() + @mock.patch('time.time', side_effect=[0, 1, 20] * 3) + @mock.patch.object(nvmeof.Portal, 'reconnect_delay', + new_callable=mock.PropertyMock, return_value=10) + @mock.patch.object(nvmeof.Portal, 'is_live', + new_callable=mock.PropertyMock, return_value=False) @mock.patch.object(nvmeof.Target, 'find_device') @mock.patch.object(nvmeof.Target, 'set_portals_controllers') @mock.patch.object(nvmeof.NVMeOFConnector, 'run_nvme_cli') @@ -1367,7 +1379,7 @@ class NVMeOFConnectorTestCase(test_connector.ConnectorTestCase): @mock.patch.object(nvmeof.Portal, 'state', new_callable=mock.PropertyMock) def test__connect_target_portals_down( self, mock_state, mock_rescan, mock_cli, mock_set_ctrls, - mock_find_dev): + mock_find_dev, mock_is_live, mock_delay, mock_time): """Test connect target has all portal connections down.""" retries = 3 mock_state.side_effect = retries * 3 * ['connecting'] @@ -1377,12 +1389,19 @@ class NVMeOFConnectorTestCase(test_connector.ConnectorTestCase): self.conn_props.targets[0]) self.assertEqual(retries * 3, mock_state.call_count) + self.assertEqual(retries * 3, mock_is_live.call_count) + self.assertEqual(retries * 3, mock_delay.call_count) mock_state.assert_has_calls(retries * 3 * [mock.call()]) mock_rescan.assert_not_called() mock_set_ctrls.assert_not_called() mock_find_dev.assert_not_called() mock_cli.assert_not_called() + @mock.patch('time.time', side_effect=[0, 1, 20] * 3) + @mock.patch.object(nvmeof.Portal, 'reconnect_delay', + new_callable=mock.PropertyMock, return_value=10) + @mock.patch.object(nvmeof.Portal, 'is_live', + new_callable=mock.PropertyMock, return_value=False) @mock.patch.object(nvmeof.LOG, 'error') @mock.patch.object(nvmeof.Target, 'find_device') @mock.patch.object(nvmeof.Target, 'set_portals_controllers') @@ -1391,7 +1410,7 @@ class NVMeOFConnectorTestCase(test_connector.ConnectorTestCase): @mock.patch.object(nvmeof.Portal, 'state', new_callable=mock.PropertyMock) def test__connect_target_no_portals_connect( self, mock_state, mock_rescan, mock_cli, mock_set_ctrls, - mock_find_dev, mock_log): + mock_find_dev, mock_log, mock_is_live, mock_delay, mock_time): """Test connect target when fails to connect to any portal.""" retries = 3 mock_state.side_effect = retries * ['connecting', 'connecting', None] @@ -1414,6 +1433,9 @@ class NVMeOFConnectorTestCase(test_connector.ConnectorTestCase): retries * [mock.call(['connect', '-a', portal.address, '-s', portal.port, '-t', portal.transport, '-n', target.nqn, '-Q', '128', '-l', '-1'])]) + # There are 2 in connecting state + self.assertEqual(retries * 2, mock_is_live.call_count) + self.assertEqual(retries * 2, mock_delay.call_count) @mock.patch.object(nvmeof.Target, 'find_device') @mock.patch.object(nvmeof.Target, 'set_portals_controllers') @@ -1490,7 +1512,7 @@ class NVMeOFConnectorTestCase(test_connector.ConnectorTestCase): self, exit_code, mock_state, mock_rescan, mock_cli, mock_set_ctrls, mock_find_dev, mock_log): """Treat race condition with sysadmin as success.""" - mock_state.side_effect = ['connecting', 'connecting', None] + mock_state.side_effect = ['connecting', 'connecting', None, 'live'] dev_path = '/dev/nvme0n1' mock_find_dev.return_value = dev_path mock_cli.side_effect = putils.ProcessExecutionError( @@ -1501,8 +1523,8 @@ class NVMeOFConnectorTestCase(test_connector.ConnectorTestCase): res = self.connector._connect_target(target) self.assertEqual(dev_path, res) - self.assertEqual(3, mock_state.call_count) - mock_state.assert_has_calls(3 * [mock.call()]) + self.assertEqual(4, mock_state.call_count) + mock_state.assert_has_calls(4 * [mock.call()]) mock_rescan.assert_not_called() mock_set_ctrls.assert_called_once_with() mock_find_dev.assert_called_once_with() @@ -1513,6 +1535,126 @@ class NVMeOFConnectorTestCase(test_connector.ConnectorTestCase): portal.transport, '-n', target.nqn, '-Q', '128', '-l', '-1']) self.assertEqual(1, mock_log.call_count) + @ddt.data(70, errno.EALREADY) + @mock.patch.object(nvmeof.LOG, 'warning') + @mock.patch('time.sleep') + @mock.patch('time.time', side_effect=[0, 0.1, 0.6]) + @mock.patch.object(nvmeof.Portal, 'reconnect_delay', + new_callable=mock.PropertyMock, return_value=10) + @mock.patch.object(nvmeof.Portal, 'is_live', + new_callable=mock.PropertyMock) + @mock.patch.object(nvmeof.Target, 'find_device') + @mock.patch.object(nvmeof.Target, 'set_portals_controllers') + @mock.patch.object(nvmeof.NVMeOFConnector, 'run_nvme_cli') + @mock.patch.object(nvmeof.NVMeOFConnector, 'rescan') + @mock.patch.object(nvmeof.Portal, 'state', new_callable=mock.PropertyMock) + def test__connect_target_race_connecting( + self, exit_code, mock_state, mock_rescan, mock_cli, mock_set_ctrls, + mock_find_dev, mock_is_live, mock_delay, mock_time, mock_sleep, + mock_log): + """Test connect target when portal is reconnecting after race.""" + mock_cli.side_effect = putils.ProcessExecutionError( + exit_code=exit_code) + mock_state.side_effect = ['connecting', 'connecting', None, + 'connecting'] + mock_is_live.side_effect = [False, False, False, False, True] + + target = self.conn_props.targets[0] + res = self.connector._connect_target(target) + + self.assertEqual(mock_find_dev.return_value, res) + self.assertEqual(4, mock_state.call_count) + self.assertEqual(5, mock_is_live.call_count) + self.assertEqual(3, mock_delay.call_count) + self.assertEqual(2, mock_sleep.call_count) + mock_sleep.assert_has_calls(2 * [mock.call(1)]) + mock_rescan.assert_not_called() + mock_set_ctrls.assert_called_once() + mock_find_dev.assert_called_once() + portal = target.portals[-1] + mock_cli.assert_called_once_with([ + 'connect', '-a', portal.address, '-s', portal.port, '-t', + portal.transport, '-n', target.nqn, '-Q', '128', '-l', '-1']) + self.assertEqual(1, mock_log.call_count) + + @ddt.data(70, errno.EALREADY) + @mock.patch.object(nvmeof.LOG, 'warning') + @mock.patch.object(nvmeof.LOG, 'error') + @mock.patch('time.sleep') + @mock.patch('time.time', side_effect=[0, 0.1, 0.6]) + @mock.patch.object(nvmeof.Portal, 'reconnect_delay', + new_callable=mock.PropertyMock, return_value=10) + @mock.patch.object(nvmeof.Portal, 'is_live', + new_callable=mock.PropertyMock) + @mock.patch.object(nvmeof.Target, 'find_device') + @mock.patch.object(nvmeof.Target, 'set_portals_controllers') + @mock.patch.object(nvmeof.NVMeOFConnector, 'run_nvme_cli') + @mock.patch.object(nvmeof.NVMeOFConnector, 'rescan') + @mock.patch.object(nvmeof.Portal, 'state', new_callable=mock.PropertyMock) + def test__connect_target_race_unknown( + self, exit_code, mock_state, mock_rescan, mock_cli, mock_set_ctrls, + mock_find_dev, mock_is_live, mock_delay, mock_time, mock_sleep, + mock_log_err, mock_log_warn): + """Test connect target when portal is unknown after race.""" + mock_cli.side_effect = putils.ProcessExecutionError( + exit_code=exit_code) + mock_state.side_effect = ['connecting', 'connecting', None, + 'unknown'] + mock_is_live.side_effect = [False, False, False, True] + + target = self.conn_props.targets[0] + res = self.connector._connect_target(target) + + self.assertEqual(mock_find_dev.return_value, res) + self.assertEqual(4, mock_state.call_count) + self.assertEqual(4, mock_is_live.call_count) + self.assertEqual(2, mock_delay.call_count) + self.assertEqual(2, mock_sleep.call_count) + mock_sleep.assert_has_calls(2 * [mock.call(1)]) + mock_rescan.assert_not_called() + mock_set_ctrls.assert_called_once() + mock_find_dev.assert_called_once() + portal = target.portals[-1] + mock_cli.assert_called_once_with([ + 'connect', '-a', portal.address, '-s', portal.port, '-t', + portal.transport, '-n', target.nqn, '-Q', '128', '-l', '-1']) + self.assertEqual(1, mock_log_err.call_count) + self.assertEqual(1, mock_log_warn.call_count) + + @mock.patch('time.sleep') + @mock.patch('time.time', side_effect=[0, 0.1, 0.6]) + @mock.patch.object(nvmeof.Portal, 'reconnect_delay', + new_callable=mock.PropertyMock, return_value=10) + @mock.patch.object(nvmeof.Portal, 'is_live', + new_callable=mock.PropertyMock) + @mock.patch.object(nvmeof.Target, 'find_device') + @mock.patch.object(nvmeof.Target, 'set_portals_controllers') + @mock.patch.object(nvmeof.NVMeOFConnector, 'run_nvme_cli') + @mock.patch.object(nvmeof.NVMeOFConnector, 'rescan') + @mock.patch.object(nvmeof.Portal, 'state', new_callable=mock.PropertyMock, + return_value='connecting') + def test__connect_target_portals_connecting( + self, mock_state, mock_rescan, mock_cli, mock_set_ctrls, + mock_find_dev, mock_is_live, mock_delay, mock_time, mock_sleep): + """Test connect target when portals reconnect.""" + # First pass everything connecting, second pass the second portal is up + mock_is_live.side_effect = [False, False, False, False, True] + + # Connecting state changing after 2 sleeps + target = self.conn_props.targets[0] + res = self.connector._connect_target(target) + + self.assertEqual(mock_find_dev.return_value, res) + self.assertEqual(3, mock_state.call_count) + self.assertEqual(5, mock_is_live.call_count) + self.assertEqual(3, mock_delay.call_count) + self.assertEqual(2, mock_sleep.call_count) + mock_sleep.assert_has_calls(2 * [mock.call(1)]) + mock_rescan.assert_not_called() + mock_set_ctrls.assert_called_once() + mock_find_dev.assert_called_once() + mock_cli.assert_not_called() + @mock.patch.object(nvmeof.NVMeOFConnector, 'stop_and_assemble_raid') @mock.patch.object(nvmeof.NVMeOFConnector, '_is_device_in_raid') def test_handle_replicated_volume_existing( diff --git a/releasenotes/notes/nvmeof-connecting-788f77a42fe7dd3b.yaml b/releasenotes/notes/nvmeof-connecting-788f77a42fe7dd3b.yaml new file mode 100644 index 000000000..bb0fd61e3 --- /dev/null +++ b/releasenotes/notes/nvmeof-connecting-788f77a42fe7dd3b.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + NVMe-oF connector `bug #2035695 + `_: Fixed + attaching volumes when all portals of a subsystem are reconnecting at the + time of the request.