[Notes] [Git][BuildStream/buildstream][willsalmon/580-backport] 5 commits: cascache: move tmp directory one level up

[Will Salmon <gitlab mg gitlab com>]

Title: GitLab

Will Salmon pushed to branch willsalmon/580-backport at BuildStream / buildstream

Commits:

• fb9df976
  by Tiago Gomes at 2018-08-09T09:20:35Z

  cascache: move tmp directory one level up
  
  The CAS uses a temp directory while manipulating the cache, temporary
  files can be added and removed while adding artifacts to the cache here.
  
  Since calculation of the cache size happens in parallel to artifact
  cache additions, this causes race conditions in the size calculation
  job, as we end up calling `stat` on temporary files which are being
  removed in parallel.
  
  Handle this by moving the temporary directory out of the way, and avoid
  considering the tmp directory when calculating the cache size
  

• 59886841
  by Tiago Gomes at 2018-08-09T10:27:19Z

  Merge branch 'tiagogomes/issue-520-bp' into 'bst-1.2'
  
  cascache: move tmp directory one level up
  
  See merge request BuildStream/buildstream!623

• 928c4d99
  by Valentin David at 2018-08-09T12:51:48Z

  Keep original flags for create in SafeHardlinks.
  
  When open(2) is used with flags O_CREAT|O_RDWR, the file descriptor
  must be readable. Unfortunately O_RDWR was not passed which made
  read fail with EBADF and mmap to signal SIGBUS.
  
  This issue happened with man-db for example.
  
  Fixes #143.
  

• f5683244
  by Valentin David at 2018-08-09T13:49:03Z

  Merge branch 'valentindavid/fuse-create-flags-1.2' into 'bst-1.2'
  
  Keep original flags for create in SafeHardlinks.
  
  See merge request BuildStream/buildstream!625

• b92a6789
  by William Salmon at 2018-08-09T16:15:37Z

  Add Error to git track if track and ref are not present
  
  This is to address https://gitlab.com/BuildStream/buildstream/issues/471 that
  documented unhelpfull behavour when tracking git sources.
  
  It is a back port of
  https://gitlab.com/BuildStream/buildstream/merge_requests/580 and
  https://gitlab.com/BuildStream/buildstream/merge_requests/628
  

8 changed files:

• buildstream/_artifactcache/artifactcache.py
• buildstream/_artifactcache/cascache.py
• buildstream/_artifactcache/casserver.py
• buildstream/_fuse/fuse.py
• buildstream/_fuse/hardlinks.py
• buildstream/_pipeline.py
• buildstream/plugins/sources/git.py
• tests/sources/git.py

Changes:

buildstream/_artifactcache/artifactcache.py

---

...	...	@@ -80,6 +80,8 @@ class ArtifactCache():
80	80	self.context = context
81	81	self.required_artifacts = set()
82	82	self.extractdir = os.path.join(context.artifactdir, 'extract')
	83	+ self.tmpdir = os.path.join(context.artifactdir, 'tmp')
	84	+
83	85	self.max_size = context.cache_quota
84	86	self.estimated_size = None
85	87
...	...	@@ -89,7 +91,8 @@ class ArtifactCache():
89	91	self._local = False
90	92	self.cache_size = None
91	93
92		- os.makedirs(context.artifactdir, exist_ok=True)
	94	+ os.makedirs(self.extractdir, exist_ok=True)
	95	+ os.makedirs(self.tmpdir, exist_ok=True)
93	96
94	97	################################################
95	98	# Methods implemented on the abstract class #

buildstream/_artifactcache/cascache.py

---

...	...	@@ -56,7 +56,8 @@ class CASCache(ArtifactCache):
56	56	super().__init__(context)
57	57
58	58	self.casdir = os.path.join(context.artifactdir, 'cas')
59		- os.makedirs(os.path.join(self.casdir, 'tmp'), exist_ok=True)
	59	+ os.makedirs(os.path.join(self.casdir, 'refs', 'heads'), exist_ok=True)
	60	+ os.makedirs(os.path.join(self.casdir, 'objects'), exist_ok=True)
60	61
61	62	self._enable_push = enable_push
62	63
...	...	@@ -85,8 +86,6 @@ class CASCache(ArtifactCache):
85	86	# artifact has already been extracted
86	87	return dest
87	88
88		- os.makedirs(self.extractdir, exist_ok=True)
89		-
90	89	with tempfile.TemporaryDirectory(prefix='tmp', dir=self.extractdir) as tmpdir:
91	90	checkoutdir = os.path.join(tmpdir, ref)
92	91	self._checkout(checkoutdir, tree)
...	...	@@ -394,7 +393,7 @@ class CASCache(ArtifactCache):
394	393	try:
395	394	h = hashlib.sha256()
396	395	# Always write out new file to avoid corruption if input file is modified
397		- with tempfile.NamedTemporaryFile(dir=os.path.join(self.casdir, 'tmp')) as out:
	396	+ with tempfile.NamedTemporaryFile(dir=self.tmpdir) as out:
398	397	# Set mode bits to 0644
399	398	os.chmod(out.name, stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH)
400	399
...	...	@@ -764,7 +763,7 @@ class CASCache(ArtifactCache):
764	763	# already in local cache
765	764	return
766	765
767		- with tempfile.NamedTemporaryFile(dir=os.path.join(self.casdir, 'tmp')) as out:
	766	+ with tempfile.NamedTemporaryFile(dir=self.tmpdir) as out:
768	767	self._fetch_blob(remote, tree, out)
769	768
770	769	directory = remote_execution_pb2.Directory()
...	...	@@ -778,7 +777,7 @@ class CASCache(ArtifactCache):
778	777	# already in local cache
779	778	continue
780	779
781		- with tempfile.NamedTemporaryFile(dir=os.path.join(self.casdir, 'tmp')) as f:
	780	+ with tempfile.NamedTemporaryFile(dir=self.tmpdir) as f:
782	781	self._fetch_blob(remote, filenode.digest, f)
783	782
784	783	digest = self.add_object(path=f.name)

buildstream/_artifactcache/casserver.py

---

...	...	@@ -161,7 +161,7 @@ class _ByteStreamServicer(bytestream_pb2_grpc.ByteStreamServicer):
161	161	offset = 0
162	162	finished = False
163	163	resource_name = None
164		- with tempfile.NamedTemporaryFile(dir=os.path.join(self.cas.casdir, 'tmp')) as out:
	164	+ with tempfile.NamedTemporaryFile(dir=self.cas.tmpdir) as out:
165	165	for request in request_iterator:
166	166	assert not finished
167	167	assert request.write_offset == offset

buildstream/_fuse/fuse.py

---

...	...	@@ -757,7 +757,11 @@ class FUSE(object):
757	757	if self.raw_fi:
758	758	return self.operations('create', path, mode, fi)
759	759	else:
760		- fi.fh = self.operations('create', path, mode)
	760	+ # This line is different from upstream to fix issues
	761	+ # reading file opened with O_CREAT|O_RDWR.
	762	+ # See issue #143.
	763	+ fi.fh = self.operations('create', path, mode, fi.flags)
	764	+ # END OF MODIFICATION
761	765	return 0
762	766
763	767	def ftruncate(self, path, length, fip):

buildstream/_fuse/hardlinks.py

---

...	...	@@ -185,12 +185,12 @@ class SafeHardlinkOps(Operations):
185	185
186	186	return os.open(full_path, flags)
187	187
188		- def create(self, path, mode, fi=None):
	188	+ def create(self, path, mode, flags):
189	189	full_path = self._full_path(path)
190	190
191	191	# If it already exists, ensure it's a copy first
192	192	self._ensure_copy(full_path)
193		- return os.open(full_path, os.O_WRONLY | os.O_CREAT, mode)
	193	+ return os.open(full_path, flags, mode)
194	194
195	195	def read(self, path, length, offset, fh):
196	196	os.lseek(fh, offset, os.SEEK_SET)

buildstream/_pipeline.py

---

...	...	@@ -358,10 +358,15 @@ class Pipeline():
358	358	inconsistent.append(element)
359	359
360	360	if inconsistent:
361		- detail = "Exact versions are missing for the following elements\n" + \
362		- "Try tracking these elements first with `bst track`\n\n"
	361	+ detail = "Exact versions are missing for the following elements:\n\n"
	362	+
363	363	for element in inconsistent:
364		- detail += " " + element._get_full_name() + "\n"
	364	+ detail += " " + element._get_full_name()
	365	+ for source in element.sources():
	366	+ if not source._get_consistency() and not source.get_ref():
	367	+ detail += ": Source {} is missing ref\n".format(source)
	368	+ detail += "\nTry tracking these elements first with `bst track`\n"
	369	+
365	370	raise PipelineError("Inconsistent pipeline", detail=detail, reason="inconsistent-pipeline")
366	371
367	372	#############################################################

buildstream/plugins/sources/git.py

---

...	...	@@ -80,6 +80,7 @@ from configparser import RawConfigParser
80	80
81	81	from buildstream import Source, SourceError, Consistency, SourceFetcher
82	82	from buildstream import utils
	83	+from buildstream._exceptions import LoadError, LoadErrorReason
83	84
84	85	GIT_MODULES = '.gitmodules'
85	86
...	...	@@ -296,6 +297,12 @@ class GitSource(Source):
296	297	self.original_url = self.node_get_member(node, str, 'url')
297	298	self.mirror = GitMirror(self, '', self.original_url, ref)
298	299	self.tracking = self.node_get_member(node, str, 'track', None)
	300	+
	301	+ # At this point we now know if the source has a ref and/or a track.
	302	+ # If it is missing both then we will be unable to track or build.
	303	+ if self.mirror.ref is None and self.tracking is None:
	304	+ raise LoadError(LoadErrorReason.INVALID_DATA, "{}: Git sources require a ref and/or track".format(self))
	305	+
299	306	self.checkout_submodules = self.node_get_member(node, bool, 'checkout-submodules', True)
300	307	self.submodules = []
301	308
...	...	@@ -359,6 +366,12 @@ class GitSource(Source):
359	366
360	367	# If self.tracking is not specified it's not an error, just silently return
361	368	if not self.tracking:
	369	+ # Is there a better way to check if a ref is given.
	370	+ if self.mirror.ref is None:
	371	+ detail = 'Without a tracking branch ref can not be updated. Please ' + \
	372	+ 'provide a ref or a track.'
	373	+ raise SourceError("{}: No track or ref".format(self),
	374	+ detail=detail, reason="track-attempt-no-track")
362	375	return None
363	376
364	377	with self.timed_activity("Tracking {} from {}"

tests/sources/git.py

---

1	1	import os
2	2	import pytest
3	3
4		-from buildstream._exceptions import ErrorDomain
	4	+from buildstream._exceptions import ErrorDomain, LoadErrorReason
5	5	from buildstream import _yaml
6	6
7	7	from tests.testutils import cli, create_repo
...	...	@@ -359,3 +359,41 @@ def test_submodule_track_ignore_inconsistent(cli, tmpdir, datafiles):
359	359
360	360	# Assert that we are just fine without it, and emit a warning to the user.
361	361	assert "Ignoring inconsistent submodule" in result.stderr
	362	+
	363	+
	364	+@pytest.mark.skipif(HAVE_GIT is False, reason="git is not available")
	365	+@pytest.mark.datafiles(os.path.join(DATA_DIR, 'template'))
	366	+def test_submodule_track_no_ref_or_track(cli, tmpdir, datafiles):
	367	+ project = os.path.join(datafiles.dirname, datafiles.basename)
	368	+
	369	+ # Create the repo from 'repofiles' subdir
	370	+ repo = create_repo('git', str(tmpdir))
	371	+ ref = repo.create(os.path.join(project, 'repofiles'))
	372	+
	373	+ # Write out our test target
	374	+ gitsource = repo.source_config(ref=None)
	375	+ gitsource.pop('track')
	376	+ element = {
	377	+ 'kind': 'import',
	378	+ 'sources': [
	379	+ gitsource
	380	+ ]
	381	+ }
	382	+
	383	+ _yaml.dump(element, os.path.join(project, 'target.bst'))
	384	+
	385	+ # Track will encounter an inconsistent submodule without any ref
	386	+ result = cli.run(project=project, args=['track', 'target.bst'])
	387	+ result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA)
	388	+ result.assert_task_error(None, None)
	389	+
	390	+ # Assert that we are just fine without it, and emit a warning to the user.
	391	+ assert "Git sources require a ref and/or track" in result.stderr
	392	+
	393	+ # Track will encounter an inconsistent submodule without any ref
	394	+ result = cli.run(project=project, args=['build', 'target.bst'])
	395	+ result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA)
	396	+ result.assert_task_error(None, None)
	397	+
	398	+ # Assert that we are just fine without it, and emit a warning to the user.
	399	+ assert "Git sources require a ref and/or track" in result.stderr

—
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.