From 7ffa3fb7d9d497fd3848ab0313ba8aa84136a06e Mon Sep 17 00:00:00 2001
From: Lee Yarwood <lyarwood@redhat.com>
Date: Thu, 21 Nov 2019 20:01:50 +0000
Subject: [PATCH] iscsi: Add _get_device_link retry when waiting for
 /dev/disk/by-id/ to populate

Bug #1820007 documents failures to find /dev/disk/by-id/ symlinks
associated with encrypted volumes both in real world and CI
environments. These failures appear to be due to udev on these slow or
overloaded hosts failing to populate the required /dev/disk/by-id/
symlinks in time after the iSCSI volume has been connected.

This change seeks to avoid such failures by simply decorating
_get_device_link with the @utils.retry to hopefully allow udev time to
create the required symlinks under /dev/disk/by-id/.

Closes-Bug: #1820007
Change-Id: Ib9c8ebae7a6051e18538920139fecd123682a474
(cherry picked from commit 331316827a60aa50ee26be08a06027525f5194ef)
(cherry picked from commit d8ed21e7d25f67c6d2bfe71515723f68ce15b887)
---
 os_brick/initiator/connectors/iscsi.py        |  1 +
 .../tests/initiator/connectors/test_iscsi.py  | 62 +++++++++++++++++--
 2 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/os_brick/initiator/connectors/iscsi.py b/os_brick/initiator/connectors/iscsi.py
index 238c88501..a15b9a9ea 100644
--- a/os_brick/initiator/connectors/iscsi.py
+++ b/os_brick/initiator/connectors/iscsi.py
@@ -517,6 +517,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
             with excutils.save_and_reraise_exception():
                 self._cleanup_connection(connection_properties, force=True)
 
+    @utils.retry(exceptions=(exception.VolumeDeviceNotFound))
     def _get_device_link(self, wwn, device, mpath):
         # These are the default symlinks that should always be there
         if mpath:
diff --git a/os_brick/tests/initiator/connectors/test_iscsi.py b/os_brick/tests/initiator/connectors/test_iscsi.py
index 83af738c4..dd0dae953 100644
--- a/os_brick/tests/initiator/connectors/test_iscsi.py
+++ b/os_brick/tests/initiator/connectors/test_iscsi.py
@@ -1562,17 +1562,67 @@ Setting up iSCSI targets: unused
             mock.call('/dev/disk/by-id/dm-...'),
             mock.call('/dev/disk/by-id/scsi-wwn')])
 
+    @mock.patch('time.sleep')
     @mock.patch('os.path.realpath', return_value='/dev/sdz')
     @mock.patch('os.listdir', return_value=['dm-...', 'scsi-...'])
-    def test__get_device_link_not_found(self, listdir_mock, realpath_mock):
+    def test__get_device_link_not_found(self, listdir_mock, realpath_mock,
+                                        mock_time):
         self.assertRaises(exception.VolumeDeviceNotFound,
                           self.connector._get_device_link,
                           'wwn', '/dev/sda', None)
-        listdir_mock.assert_called_once_with('/dev/disk/by-id/')
-        realpath_mock.assert_has_calls([
-            mock.call('/dev/disk/by-id/scsi-wwn'),
-            mock.call('/dev/disk/by-id/dm-...'),
-            mock.call('/dev/disk/by-id/scsi-...')])
+        listdir_mock.assert_has_calls(3 * [mock.call('/dev/disk/by-id/')])
+        self.assertEqual(3, listdir_mock.call_count)
+        realpath_mock.assert_has_calls(
+            3 * [mock.call('/dev/disk/by-id/scsi-wwn'),
+                 mock.call('/dev/disk/by-id/dm-...'),
+                 mock.call('/dev/disk/by-id/scsi-...')])
+        self.assertEqual(9, realpath_mock.call_count)
+
+    @mock.patch('time.sleep')
+    @mock.patch('os.path.realpath')
+    @mock.patch('os.listdir', return_value=['dm-...', 'scsi-...'])
+    def test__get_device_link_symlink_found_after_retry(self, mock_listdir,
+                                                        mock_realpath,
+                                                        mock_time):
+        # Return the expected realpath on the third retry
+        mock_realpath.side_effect = [
+            None, None, None, None, None, None, '/dev/sda']
+
+        # Assert that VolumeDeviceNotFound isn't raised
+        self.connector._get_device_link('wwn', '/dev/sda', None)
+
+        # Assert that listdir and realpath have been called correctly
+        mock_listdir.assert_has_calls(2 * [mock.call('/dev/disk/by-id/')])
+        self.assertEqual(2, mock_listdir.call_count)
+        mock_realpath.assert_has_calls(
+            2 * [mock.call('/dev/disk/by-id/scsi-wwn'),
+                 mock.call('/dev/disk/by-id/dm-...'),
+                 mock.call('/dev/disk/by-id/scsi-...')]
+            + [mock.call('/dev/disk/by-id/scsi-wwn')])
+        self.assertEqual(7, mock_realpath.call_count)
+
+    @mock.patch('time.sleep')
+    @mock.patch('os.path.realpath')
+    @mock.patch('os.listdir', return_value=['dm-...', 'scsi-...'])
+    def test__get_device_link_symlink_found_after_retry_by_listdir(
+            self, mock_listdir, mock_realpath, mock_time):
+
+        # Return the expected realpath on the second retry while looping over
+        # the devices returned by listdir
+        mock_realpath.side_effect = [
+            None, None, None, None, None, '/dev/sda']
+
+        # Assert that VolumeDeviceNotFound isn't raised
+        self.connector._get_device_link('wwn', '/dev/sda', None)
+
+        # Assert that listdir and realpath have been called correctly
+        mock_listdir.assert_has_calls(2 * [mock.call('/dev/disk/by-id/')])
+        self.assertEqual(2, mock_listdir.call_count)
+        mock_realpath.assert_has_calls(
+            2 * [mock.call('/dev/disk/by-id/scsi-wwn'),
+                 mock.call('/dev/disk/by-id/dm-...'),
+                 mock.call('/dev/disk/by-id/scsi-...')])
+        self.assertEqual(6, mock_realpath.call_count)
 
     @mock.patch.object(iscsi.ISCSIConnector, '_run_iscsiadm_bare')
     def test_get_node_startup_values(self, run_iscsiadm_bare_mock):