PR#4427: work around nfs glitch in ensuredir

Merges #4427
https://pagure.io/koji/pull-request/4427

Fixes: #4417
https://pagure.io/koji/issue/4417
Task can fail with FileExistsError exception if session expires while uploads are happening

koji/__init__.py

@@ -565,16 +565,35 @@ def ensuredir(directory):
             raise OSError("root directory missing? %s" % directory)
         if head:
             ensuredir(head)
-        # note: if head is blank, then we've reached the top of a relative path
+            parent = head
+        else:
+            # if head is blank, then we've reached the top of a relative path
+            parent = '.'
         try:
             os.mkdir(directory)
-        except OSError:
-            # do not thrown when dir already exists (could happen in a race)
+        except OSError as e:
+            if e.errno != errno.EEXIST:
+                raise
+            # Work around an nfs glitch. Reading the parent dir gets the os to
+            # notice the new dir in a race
+            # See https://pagure.io/koji/issue/4417
+            _listdir(parent)
+
             if not os.path.isdir(directory):
                 # something else must have gone wrong
                 raise
+
     return directory
 
+
+def _listdir(path):
+    # os.listdir, but returns None if dir does not exist
+    try:
+        return os.listdir(path)
+    except FileNotFoundError:
+        return None
+
+
 # END kojikamid dup #
 
 

tests/test_lib/test_file_ops.py

@@ -1,5 +1,7 @@
 from __future__ import absolute_import
 
+import shutil
+import tempfile
 import unittest
 
 try:
@@ -8,50 +10,104 @@ except ImportError:
     import mock
 import errno
 
-from koji import ensuredir
+from koji import ensuredir, _listdir
 
 
 class TestEnsureDir(unittest.TestCase):
-    @mock.patch('os.mkdir')
-    @mock.patch('os.path.exists')
-    @mock.patch('os.path.isdir')
-    def test_ensuredir_errors(self, mock_isdir, mock_exists, mock_mkdir):
-        mock_exists.return_value = False
+
+    def setUp(self):
+        self.mkdir = mock.patch('os.mkdir').start()
+        self.exists = mock.patch('os.path.exists').start()
+        self.isdir = mock.patch('os.path.isdir').start()
+        self._listdir = mock.patch('koji._listdir').start()
+
+    def tearDown(self):
+        mock.patch.stopall()
+
+    def test_ensuredir_errors(self):
+        self.exists.return_value = False
         with self.assertRaises(OSError) as cm:
             ensuredir('/')
         self.assertEqual(cm.exception.args[0], 'root directory missing? /')
-        mock_mkdir.assert_not_called()
+        self.mkdir.assert_not_called()
 
-        mock_exists.return_value = True
-        mock_isdir.return_value = False
+        self.exists.return_value = True
+        self.isdir.return_value = False
         with self.assertRaises(OSError) as cm:
             ensuredir('/path/foo/bar')
         self.assertEqual(cm.exception.args[0],
                          'Not a directory: /path/foo/bar')
-        mock_mkdir.assert_not_called()
+        self.mkdir.assert_not_called()
 
-        mock_exists.return_value = False
-        mock_isdir.return_value = False
-        mock_mkdir.side_effect = OSError(errno.EEXIST, 'error msg')
+        self.exists.return_value = False
+        self.isdir.return_value = False
+        self.mkdir.side_effect = OSError(errno.EEXIST, 'error msg')
         with self.assertRaises(OSError) as cm:
             ensuredir('path')
         self.assertEqual(cm.exception.args[0], errno.EEXIST)
-        mock_mkdir.assert_called_once_with('path')
+        self.mkdir.assert_called_once_with('path')
 
-        mock_mkdir.reset_mock()
-        mock_mkdir.side_effect = OSError(errno.EEXIST, 'error msg')
-        mock_isdir.return_value = True
+        self.mkdir.reset_mock()
+        self.mkdir.side_effect = OSError(errno.EEXIST, 'error msg')
+        self.isdir.return_value = True
         ensuredir('path')
-        mock_mkdir.assert_called_once_with('path')
+        self.mkdir.assert_called_once_with('path')
 
-    @mock.patch('os.mkdir')
-    @mock.patch('os.path.exists')
-    @mock.patch('os.path.isdir')
-    def test_ensuredir(self, mock_isdir, mock_exists, mock_mkdir):
-        mock_exists.side_effect = [False, False, True]
-        mock_isdir.return_value = True
+    def test_ensuredir(self):
+        self.exists.side_effect = [False, False, True]
+        self.isdir.return_value = True
         ensuredir('/path/foo/bar/')
-        self.assertEqual(mock_exists.call_count, 3)
-        self.assertEqual(mock_isdir.call_count, 1)
-        mock_mkdir.assert_has_calls([mock.call('/path/foo'),
+        self.assertEqual(self.exists.call_count, 3)
+        self.assertEqual(self.isdir.call_count, 1)
+        self.mkdir.assert_has_calls([mock.call('/path/foo'),
                                      mock.call('/path/foo/bar')])
+
+    def test_ensuredir_error_is_dir(self):
+        # mkdir errors with EEXIST, and directory does exist
+        self.exists.return_value = False
+        self.isdir.return_value = True
+        self.mkdir.side_effect = OSError(errno.EEXIST, 'error msg')
+        ensuredir('path')
+        self._listdir.assert_called_once()
+        self.isdir.assert_called_once()
+
+    def test_ensuredir_wrong_errno(self):
+        self.exists.return_value = False
+        self.isdir.return_value = True
+        # if mkdir fails with a different error, this should not be trapped
+        self.mkdir.side_effect = OSError(errno.ENOENT, 'error msg')
+        with self.assertRaises(OSError) as cm:
+            ensuredir('path')
+        self.assertEqual(cm.exception.args[0], errno.ENOENT)
+        self._listdir.assert_not_called()
+        self.isdir.assert_not_called()
+
+
+class TestListDir(unittest.TestCase):
+
+    def setUp(self):
+        self.tempdir = tempfile.mkdtemp()
+
+    def tearDown(self):
+        mock.patch.stopall()
+        shutil.rmtree(self.tempdir)
+
+    def test_listdir_success(self):
+        # make some files in tempdir
+        names = ['abc', 'def', 'ghi']
+        for fn in names:
+            path = '%s/%s' % (self.tempdir, fn)
+            with open(path, 'wt') as fp:
+                fp.write('hello world')
+
+        contents = _listdir(self.tempdir)
+        contents.sort()
+        self.assertEqual(contents, names)
+
+    def test_listdir_missing(self):
+        missing = '%s/no-such-file' % self.tempdir
+        contents = _listdir(missing)
+        self.assertIsNone(contents)
+
+
+# the end