From f74c3b232f5e93a2bd543a4aeb5d8303ac973f6e Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Tue, 15 Jul 2025 14:44:22 -0400 Subject: [PATCH] work around nfs glitch in ensuredir --- koji/__init__.py | 25 +++++++- tests/test_lib/test_file_ops.py | 110 ++++++++++++++++++++++++-------- 2 files changed, 105 insertions(+), 30 deletions(-) diff --git a/koji/__init__.py b/koji/__init__.py index c9a657eb..aab41a63 100644 --- a/koji/__init__.py +++ b/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 # diff --git a/tests/test_lib/test_file_ops.py b/tests/test_lib/test_file_ops.py index 23de1b35..2cfbc10d 100644 --- a/tests/test_lib/test_file_ops.py +++ b/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