[Notes] [Git][BuildStream/buildstream][bschubert/fix-atomic-move-git-repo] Extract atomic move function to utils.py

[Benjamin Schubert <gitlab mg gitlab com>]

Title: GitLab

Benjamin Schubert pushed to branch bschubert/fix-atomic-move-git-repo at BuildStream / buildstream

Commits:

• d2cbc516
  by Benjamin Schubert at 2018-11-09T11:52:45Z

  Extract atomic move function to utils.py
  
  Moving atomically a file/directory can be tricky since different
  errors might be raised for the same underlying problem.
  
  Having a utility function to reduce this discrepancies will help
  in ensuring we have correct behavior
  

4 changed files:

• buildstream/plugins/sources/git.py
• buildstream/utils.py
• tests/sources/git.py
• + tests/utils/movedirectory.py

Changes:

buildstream/plugins/sources/git.py

---

...	...	@@ -86,7 +86,6 @@ This plugin also utilises the following configurable core plugin warnings:
86	86	"""
87	87
88	88	import os
89		-import errno
90	89	import re
91	90	import shutil
92	91	from collections.abc import Mapping
...	...	@@ -97,6 +96,7 @@ from configparser import RawConfigParser
97	96	from buildstream import Source, SourceError, Consistency, SourceFetcher
98	97	from buildstream import utils
99	98	from buildstream.plugin import CoreWarnings
	99	+from buildstream.utils import move_atomic, DirectoryExistsError
100	100
101	101	GIT_MODULES = '.gitmodules'
102	102
...	...	@@ -141,24 +141,16 @@ class GitMirror(SourceFetcher):
141	141	fail="Failed to clone git repository {}".format(url),
142	142	fail_temporarily=True)
143	143
144		- self._atomic_move_mirror(tmpdir, url)
145		-
146		- def _atomic_move_mirror(self, tmpdir, url):
147		- # Attempt atomic rename into destination, this will fail if
148		- # another process beat us to the punch
149		- try:
150		- os.rename(tmpdir, self.mirror)
151		- except OSError as e:
152		- # When renaming and the destination repo already exists, os.rename()
153		- # will fail with either ENOTEMPTY or EEXIST, depending on the underlying
154		- # implementation.
155		- # An empty directory would always be replaced.
156		- if e.errno in (errno.EEXIST, errno.ENOTEMPTY):
157		- self.source.status("{}: Discarding duplicate clone of {}"
158		- .format(self.source, url))
159		- else:
160		- raise SourceError("{}: Failed to move cloned git repository {} from '{}' to '{}': {}"
161		- .format(self.source, url, tmpdir, self.mirror, e)) from e
	144	+ try:
	145	+ move_atomic(tmpdir, self.mirror)
	146	+ except DirectoryExistsError:
	147	+ # Another process was quicker to download this repository.
	148	+ # Let's discard our own
	149	+ self.source.status("{}: Discarding duplicate clone of {}"
	150	+ .format(self.source, url))
	151	+ except OSError as e:
	152	+ raise SourceError("{}: Failed to move cloned git repository {} from '{}' to '{}': {}"
	153	+ .format(self.source, url, tmpdir, self.mirror, e)) from e
162	154
163	155	def _fetch(self, alias_override=None):
164	156	url = self.source.translate_url(self.url,

buildstream/utils.py

---

...	...	@@ -72,6 +72,11 @@ class ProgramNotFoundError(BstError):
72	72	super().__init__(message, domain=ErrorDomain.PROG_NOT_FOUND, reason=reason)
73	73
74	74
	75	+class DirectoryExistsError(OSError):
	76	+ """Raised when a `os.rename` is attempted but the destination is an existing directory.
	77	+ """
	78	+
	79	+
75	80	class FileListResult():
76	81	"""An object which stores the result of one of the operations
77	82	which run on a list of files.
...	...	@@ -500,6 +505,32 @@ def get_bst_version():
500	505	.format(__version__))
501	506
502	507
	508	+def move_atomic(source, destination, ensure_parents=True):
	509	+ """Move the source to the destination using atomic primitives.
	510	+
	511	+ This uses `os.rename` to move a file or directory to a new destination.
	512	+ It wraps some `OSError` thrown errors to ensure their handling is correct.
	513	+
	514	+ The main reason for this to exist is that rename can throw different errors
	515	+ for the same symptom (https://www.unix.com/man-page/POSIX/3posix/rename/).
	516	+
	517	+ We are especially interested here in the case when the destination already
	518	+ exists. In this case, either EEXIST or ENOTEMPTY are thrown.
	519	+
	520	+ In order to ensure consistent handling of these exceptions, this function
	521	+ should be used instead of `os.rename`
	522	+ """
	523	+ if ensure_parents:
	524	+ os.makedirs(os.path.dirname(destination), exist_ok=True)
	525	+
	526	+ try:
	527	+ os.rename(source, destination)
	528	+ except OSError as exc:
	529	+ if exc.errno in (errno.EEXIST, errno.ENOTEMPTY):
	530	+ raise DirectoryExistsError(*exc.args) from exc
	531	+ raise
	532	+
	533	+
503	534	@contextmanager
504	535	def save_file_atomic(filename, mode='w', *, buffering=-1, encoding=None,
505	536	errors=None, newline=None, closefd=True, opener=None, tempdir=None):

tests/sources/git.py

---

...	...	@@ -21,14 +21,11 @@
21	21	#
22	22
23	23	import os
24		-from unittest import mock
25	24	import pytest
26		-import py.path
27	25
28	26	from buildstream._exceptions import ErrorDomain
29		-from buildstream import _yaml, SourceError
	27	+from buildstream import _yaml
30	28	from buildstream.plugin import CoreWarnings
31		-from buildstream.plugins.sources.git import GitMirror
32	29
33	30	from tests.testutils import cli, create_repo
34	31	from tests.testutils.site import HAVE_GIT
...	...	@@ -39,64 +36,6 @@ DATA_DIR = os.path.join(
39	36	)
40	37
41	38
42		-@pytest.mark.parametrize(
43		- ["type_", "setup"],
44		- [
45		- ("inexistent-dir", lambda path: None),
46		- ("empty-dir", lambda path: path.mkdir()),
47		- ("non-empty-dir", lambda path: path.join("bad").write("hi", ensure=True)),
48		- ("file", lambda path: path.write("no")),
49		- ],
50		-)
51		-def test_git_mirror_atomic_move(tmpdir, type_, setup):
52		- # Create required elements
53		- class DummySource:
54		- def get_mirror_directory(self):
55		- return str(tmpdir.join("source").mkdir())
56		-
57		- status = mock.MagicMock()
58		-
59		- url = "file://{}/url".format(tmpdir)
60		-
61		- # Create fake download directory
62		- download_dir = tmpdir.join("download_dir")
63		- download_dir.join("oracle").write("hello", ensure=True)
64		-
65		- # Initiate mirror
66		- mirror = GitMirror(DummySource(), None, url, None)
67		-
68		- # Setup destination directory
69		- setup(py.path.local(mirror.mirror))
70		-
71		- # Copy directory content
72		- if type_ == "file":
73		- with pytest.raises(SourceError):
74		- mirror._atomic_move_mirror(str(download_dir), mirror.url)
75		- else:
76		- mirror._atomic_move_mirror(str(download_dir), mirror.url)
77		-
78		- # Validate
79		- if type_ == "non-empty-dir":
80		- # With a non empty directory, we should not have overriden
81		- # and notified a status
82		- assert DummySource.status.called
83		-
84		- expected_oracle = os.path.join(mirror.mirror, "bad")
85		- expected_content = "hi"
86		- elif type_ == "file":
87		- expected_oracle = mirror.mirror
88		- expected_content = "no"
89		- else:
90		- # Otherwise we should have a new directory and the data
91		- expected_oracle = os.path.join(mirror.mirror, "oracle")
92		- expected_content = "hello"
93		-
94		- assert os.path.exists(expected_oracle)
95		-
96		- with open(expected_oracle) as fp:
97		- assert fp.read() == expected_content
98		-
99		-
100	39	@pytest.mark.skipif(HAVE_GIT is False, reason="git is not available")
101	40	@pytest.mark.datafiles(os.path.join(DATA_DIR, 'template'))
102	41	def test_fetch_bad_ref(cli, tmpdir, datafiles):

tests/utils/movedirectory.py

---

	1	+import pytest
	2	+
	3	+from buildstream.utils import move_atomic, DirectoryExistsError
	4	+
	5	+
	6	+@pytest.fixture
	7	+def src(tmp_path):
	8	+ src = tmp_path.joinpath("src")
	9	+ src.mkdir()
	10	+
	11	+ with src.joinpath("test").open("w") as fp:
	12	+ fp.write("test")
	13	+
	14	+ return src
	15	+
	16	+
	17	+def test_move_to_empty_dir(src, tmp_path):
	18	+ dst = tmp_path.joinpath("dst")
	19	+
	20	+ move_atomic(src, dst)
	21	+
	22	+ assert dst.joinpath("test").exists()
	23	+
	24	+
	25	+def test_move_to_empty_dir_create_parents(src, tmp_path):
	26	+ dst = tmp_path.joinpath("nested/dst")
	27	+
	28	+ move_atomic(src, dst)
	29	+ assert dst.joinpath("test").exists()
	30	+
	31	+
	32	+def test_move_to_empty_dir_no_create_parents(src, tmp_path):
	33	+ dst = tmp_path.joinpath("nested/dst")
	34	+
	35	+ with pytest.raises(FileNotFoundError):
	36	+ move_atomic(src, dst, ensure_parents=False)
	37	+
	38	+
	39	+def test_move_non_existing_dir(tmp_path):
	40	+ dst = tmp_path.joinpath("dst")
	41	+ src = tmp_path.joinpath("src")
	42	+
	43	+ with pytest.raises(FileNotFoundError):
	44	+ move_atomic(src, dst)
	45	+
	46	+
	47	+def test_move_to_existing_empty_dir(src, tmp_path):
	48	+ dst = tmp_path.joinpath("dst")
	49	+ dst.mkdir()
	50	+
	51	+ move_atomic(src, dst)
	52	+ assert dst.joinpath("test").exists()
	53	+
	54	+
	55	+def test_move_to_existing_file(src, tmp_path):
	56	+ dst = tmp_path.joinpath("dst")
	57	+
	58	+ with dst.open("w") as fp:
	59	+ fp.write("error")
	60	+
	61	+ with pytest.raises(NotADirectoryError):
	62	+ move_atomic(src, dst)
	63	+
	64	+
	65	+def test_move_file_to_existing_file(tmp_path):
	66	+ dst = tmp_path.joinpath("dst")
	67	+ src = tmp_path.joinpath("src")
	68	+
	69	+ with src.open("w") as fp:
	70	+ fp.write("src")
	71	+
	72	+ with dst.open("w") as fp:
	73	+ fp.write("dst")
	74	+
	75	+ move_atomic(src, dst)
	76	+ with dst.open() as fp:
	77	+ assert fp.read() == "src"
	78	+
	79	+
	80	+def test_move_to_existing_non_empty_dir(src, tmp_path):
	81	+ dst = tmp_path.joinpath("dst")
	82	+ dst.mkdir()
	83	+
	84	+ with dst.joinpath("existing").open("w") as fp:
	85	+ fp.write("already there")
	86	+
	87	+ with pytest.raises(DirectoryExistsError):
	88	+ move_atomic(src, dst)

—
View it on GitLab.
You're receiving this email because of your account on gitlab.com. If you'd like to receive fewer emails, you can adjust your notification settings.