[Notes] [Git][BuildStream/buildstream][tristan/fix-artifact-config-crash] 6 commits: _artifactcache/artifactcache.py: Ensure no double-setup of remotes

[Tristan Van Berkom <gitlab mg gitlab com>]

Title: GitLab

Tristan Van Berkom pushed to branch tristan/fix-artifact-config-crash at BuildStream / buildstream

Commits:

• 7b117e40
  by Daniel Silverstone at 2018-09-18T08:41:32Z

  _artifactcache/artifactcache.py: Ensure no double-setup of remotes
  
  Since ArtifactCache.setup_remotes() can be expensive and should only
  happen once, this commit will assert() if it is called a second time
  on an artifact cache instance.
  
  Signed-off-by: Daniel Silverstone <daniel silverstone codethink co uk>
  

• 345f5f49
  by Daniel Silverstone at 2018-09-18T08:41:32Z

  tests/artifactcache/pull.py: Do not double-initialize remotes
  
  The initialization of remotes is done by ArtifactCache.setup_remotes()
  and as such it was wrong for these tests to be calling
  CASCache.initialize_remotes() a second time.
  
  Signed-off-by: Daniel Silverstone <daniel silverstone codethink co uk>
  

• e32221b6
  by Daniel Silverstone at 2018-09-18T08:45:50Z

  sandbox/_sandboxremote.py: Acquire artifact cache via Platform
  
  The SandboxRemote used to construct its own CASCache which was
  considered dangerous.  This patch replaces that with acquisition of
  the cache via the Platform singleton, hopefully eliminating issues
  from having more than one artifact cache object in a single process.
  
  Signed-off-by: Daniel Silverstone <daniel silverstone codethink co uk>
  

• b587579f
  by Tristan Van Berkom at 2018-09-18T09:53:26Z

  Merge branch 'danielsilverstone-ct/fix-654' into 'master'
  
  sandbox/_sandboxremote.py: Acquire cache via Platform
  
  See merge request BuildStream/buildstream!797

• 30b41959
  by Tristan Van Berkom at 2018-09-18T09:56:45Z

  _artifactcache/artifactcache.py: Error out gracefully when push remote is mal-specified
  
  When configuring a push remote and specifying either the client-cert
  or the client-key, then both must be specified. This ensures we
  get an informative error instead of a stack trace and BUG.
  
  Fixes issue #625
  

• 41e8dc81
  by Tristan Van Berkom at 2018-09-18T09:56:45Z

  tests/artifactcache/config.py: Added test for invalid push remote configuration
  
  Test that we get the expected error when configuring a client-cert
  without client-key, or the inverse.
  

7 changed files:

• buildstream/_artifactcache/artifactcache.py
• buildstream/sandbox/_sandboxremote.py
• tests/artifactcache/config.py
• + tests/artifactcache/missing-certs/certificates/client.crt
• + tests/artifactcache/missing-certs/certificates/client.key
• + tests/artifactcache/missing-certs/element.bst
• tests/artifactcache/pull.py

Changes:

buildstream/_artifactcache/artifactcache.py

---

...	...	@@ -51,7 +51,7 @@ class ArtifactCacheSpec(namedtuple('ArtifactCacheSpec', 'url push server_cert cl
51	51	url = _yaml.node_get(spec_node, str, 'url')
52	52	push = _yaml.node_get(spec_node, bool, 'push', default_value=False)
53	53	if not url:
54		- provenance = _yaml.node_get_provenance(spec_node)
	54	+ provenance = _yaml.node_get_provenance(spec_node, 'url')
55	55	raise LoadError(LoadErrorReason.INVALID_DATA,
56	56	"{}: empty artifact cache URL".format(provenance))
57	57
...	...	@@ -67,6 +67,16 @@ class ArtifactCacheSpec(namedtuple('ArtifactCacheSpec', 'url push server_cert cl
67	67	if client_cert and basedir:
68	68	client_cert = os.path.join(basedir, client_cert)
69	69
	70	+ if client_key and not client_cert:
	71	+ provenance = _yaml.node_get_provenance(spec_node, 'client-key')
	72	+ raise LoadError(LoadErrorReason.INVALID_DATA,
	73	+ "{}: 'client-key' was specified without 'client-cert'".format(provenance))
	74	+
	75	+ if client_cert and not client_key:
	76	+ provenance = _yaml.node_get_provenance(spec_node, 'client-cert')
	77	+ raise LoadError(LoadErrorReason.INVALID_DATA,
	78	+ "{}: 'client-cert' was specified without 'client-key'".format(provenance))
	79	+
70	80	return ArtifactCacheSpec(url, push, server_cert, client_key, client_cert)
71	81
72	82
...	...	@@ -91,6 +101,7 @@ class ArtifactCache():
91	101	self._cache_size = None # The current cache size, sometimes it's an estimate
92	102	self._cache_quota = None # The cache quota
93	103	self._cache_lower_threshold = None # The target cache size for a cleanup
	104	+ self._remotes_setup = False # Check to prevent double-setup of remotes
94	105
95	106	os.makedirs(self.extractdir, exist_ok=True)
96	107	os.makedirs(self.tmpdir, exist_ok=True)
...	...	@@ -143,6 +154,10 @@ class ArtifactCache():
143	154	#
144	155	def setup_remotes(self, *, use_config=False, remote_url=None):
145	156
	157	+ # Ensure we do not double-initialise since this can be expensive
	158	+ assert(not self._remotes_setup)
	159	+ self._remotes_setup = True
	160	+
146	161	# Initialize remote artifact caches. We allow the commandline to override
147	162	# the user config in some cases (for example `bst push --remote=...`).
148	163	has_remote_caches = False

buildstream/sandbox/_sandboxremote.py

---

...	...	@@ -27,7 +27,7 @@ from . import Sandbox
27	27	from ..storage._filebaseddirectory import FileBasedDirectory
28	28	from ..storage._casbaseddirectory import CasBasedDirectory
29	29	from .._protos.build.bazel.remote.execution.v2 import remote_execution_pb2, remote_execution_pb2_grpc
30		-from .._artifactcache.cascache import CASCache
	30	+from .._platform import Platform
31	31
32	32
33	33	class SandboxError(Exception):
...	...	@@ -43,7 +43,6 @@ class SandboxRemote(Sandbox):
43	43
44	44	def __init__(self, *args, **kwargs):
45	45	super().__init__(*args, **kwargs)
46		- self.cascache = None
47	46
48	47	url = urlparse(kwargs['server_url'])
49	48	if not url.scheme or not url.hostname or not url.port:
...	...	@@ -56,12 +55,6 @@ class SandboxRemote(Sandbox):
56	55
57	56	self.server_url = '{}:{}'.format(url.hostname, url.port)
58	57
59		- def _get_cascache(self):
60		- if self.cascache is None:
61		- self.cascache = CASCache(self._get_context())
62		- self.cascache.setup_remotes(use_config=True)
63		- return self.cascache
64		-
65	58	def run_remote_command(self, command, input_root_digest, working_directory, environment):
66	59	# Sends an execution request to the remote execution server.
67	60	#
...	...	@@ -78,8 +71,8 @@ class SandboxRemote(Sandbox):
78	71	output_files=[],
79	72	output_directories=[self._output_directory],
80	73	platform=None)
81		-
82		- cascache = self._get_cascache()
	74	+ platform = Platform.get_platform()
	75	+ cascache = platform.artifactcache
83	76	# Upload the Command message to the remote CAS server
84	77	command_digest = cascache.push_message(self._get_project(), remote_command)
85	78	if not command_digest or not cascache.verify_digest_pushed(self._get_project(), command_digest):
...	...	@@ -141,7 +134,8 @@ class SandboxRemote(Sandbox):
141	134	if tree_digest is None or not tree_digest.hash:
142	135	raise SandboxError("Output directory structure had no digest attached.")
143	136
144		- cascache = self._get_cascache()
	137	+ platform = Platform.get_platform()
	138	+ cascache = platform.artifactcache
145	139	# Now do a pull to ensure we have the necessary parts.
146	140	dir_digest = cascache.pull_tree(self._get_project(), tree_digest)
147	141	if dir_digest is None or not dir_digest.hash or not dir_digest.size_bytes:
...	...	@@ -176,7 +170,8 @@ class SandboxRemote(Sandbox):
176	170
177	171	upload_vdir.recalculate_hash()
178	172
179		- cascache = self._get_cascache()
	173	+ platform = Platform.get_platform()
	174	+ cascache = platform.artifactcache
180	175	# Now, push that key (without necessarily needing a ref) to the remote.
181	176	vdir_digest = cascache.push_directory(self._get_project(), upload_vdir)
182	177	if not vdir_digest or not cascache.verify_digest_pushed(self._get_project(), vdir_digest):

tests/artifactcache/config.py

---

...	...	@@ -9,8 +9,12 @@ from buildstream._context import Context
9	9	from buildstream._project import Project
10	10	from buildstream.utils import _deduplicate
11	11	from buildstream import _yaml
	12	+from buildstream._exceptions import ErrorDomain, LoadErrorReason
12	13
	14	+from tests.testutils.runcli import cli
13	15
	16	+
	17	+DATA_DIR = os.path.dirname(os.path.realpath(__file__))
14	18	cache1 = ArtifactCacheSpec(url='https://example.com/cache1', push=True)
15	19	cache2 = ArtifactCacheSpec(url='https://example.com/cache2', push=False)
16	20	cache3 = ArtifactCacheSpec(url='https://example.com/cache3', push=False)
...	...	@@ -106,3 +110,33 @@ def test_artifact_cache_precedence(tmpdir, override_caches, project_caches, user
106	110	# Verify that it was correctly read.
107	111	expected_cache_specs = list(_deduplicate(itertools.chain(override_caches, project_caches, user_caches)))
108	112	assert parsed_cache_specs == expected_cache_specs
	113	+
	114	+
	115	+# Assert that if either the client key or client cert is specified
	116	+# without specifying it's counterpart, we get a comprehensive LoadError
	117	+# instead of an unhandled exception.
	118	+@pytest.mark.datafiles(DATA_DIR)
	119	+@pytest.mark.parametrize('config_key, config_value', [
	120	+ ('client-cert', 'client.crt'),
	121	+ ('client-key', 'client.key')
	122	+])
	123	+def test_missing_certs(cli, datafiles, config_key, config_value):
	124	+ project = os.path.join(datafiles.dirname, datafiles.basename, 'missing-certs')
	125	+
	126	+ project_conf = {
	127	+ 'name': 'test',
	128	+
	129	+ 'artifacts': {
	130	+ 'url': 'https://cache.example.com:12345',
	131	+ 'push': 'true',
	132	+ config_key: config_value
	133	+ }
	134	+ }
	135	+ project_conf_file = os.path.join(project, 'project.conf')
	136	+ _yaml.dump(project_conf, project_conf_file)
	137	+
	138	+ # Use `pull` here to ensure we try to initialize the remotes, triggering the error
	139	+ #
	140	+ # This does not happen for a simple `bst show`.
	141	+ result = cli.run(project=project, args=['pull', 'element.bst'])
	142	+ result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA)

tests/artifactcache/missing-certs/certificates/client.crt

---

tests/artifactcache/missing-certs/certificates/client.key

---

tests/artifactcache/missing-certs/element.bst

---

1

+kind: autotools

tests/artifactcache/pull.py

---

...	...	@@ -137,7 +137,6 @@ def _test_pull(user_config_file, project_dir, artifact_dir,
137	137
138	138	# Manually setup the CAS remote
139	139	cas.setup_remotes(use_config=True)
140		- cas.initialize_remotes()
141	140
142	141	if cas.has_push_remotes(element=element):
143	142	# Push the element's artifact
...	...	@@ -274,7 +273,6 @@ def _test_push_tree(user_config_file, project_dir, artifact_dir, artifact_digest
274	273
275	274	# Manually setup the CAS remote
276	275	cas.setup_remotes(use_config=True)
277		- cas.initialize_remotes()
278	276
279	277	if cas.has_push_remotes():
280	278	directory = remote_execution_pb2.Directory()
...	...	@@ -310,7 +308,6 @@ def _test_pull_tree(user_config_file, project_dir, artifact_dir, artifact_digest
310	308
311	309	# Manually setup the CAS remote
312	310	cas.setup_remotes(use_config=True)
313		- cas.initialize_remotes()
314	311
315	312	if cas.has_push_remotes():
316	313	# Pull the artifact using the Tree object

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