From bfcae5cb0369c837b9acd8005a2f45496a18a81d Mon Sep 17 00:00:00 2001 From: Sam Thursfield Date: Tue, 3 Jun 2014 14:23:17 +0000 Subject: Improve robustness when fetching artifacts from remote artifact cache Previously Morph would check if an artifact is present in the remote artifact cache, then fetch the necessary files. If an error occured during fetching, it would raise an error and abort. Instead, we should just try and fetch the files and if anything fails, move on to building locally. This avoids the situation where an error in the remote cache makes local building impossible, which we experienced recently. --- morphlib/buildcommand.py | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) (limited to 'morphlib/buildcommand.py') diff --git a/morphlib/buildcommand.py b/morphlib/buildcommand.py index 7ad7909d..5e080a41 100644 --- a/morphlib/buildcommand.py +++ b/morphlib/buildcommand.py @@ -274,31 +274,25 @@ class BuildCommand(object): 'name': a.name, }) - self.app.status(msg='Checking if %(kind)s needs ' - 'building %(sha1)s', - kind=a.source.morphology['kind'], - sha1=a.source.sha1[:7]) - - if self.is_built(a): - self.cache_artifacts_locally([a]) - self.app.status( - msg='The %(kind)s is cached at %(cache)s', - kind=a.source.morphology['kind'], - cache=os.path.basename(self.lac.artifact_filename(a))[:7]) - else: - self.app.status(msg='Building %(kind)s %(name)s', - name=a.name, kind=a.source.morphology['kind']) - self.build_artifact(a, build_env) + self.cache_or_build_artifact(a, build_env) self.app.status(msg='%(kind)s %(name)s is cached at %(cachepath)s', kind=a.source.morphology['kind'], name=a.name, cachepath=self.lac.artifact_filename(a), chatty=(a.source.morphology['kind'] != "system")) + self.app.status_prefix = old_prefix - def is_built(self, artifact): - '''Does either cache already have the artifact?''' - return self.lac.has(artifact) or (self.rac and self.rac.has(artifact)) + def cache_or_build_artifact(self, artifact, build_env): + if self.rac is not None: + try: + self.cache_artifacts_locally([artifact]) + except morphlib.remoteartifactcache.GetError: + # Error is logged by the RemoteArtifactCache object. + pass + + if not self.lac.has(artifact): + self.build_artifact(artifact, build_env) def build_artifact(self, artifact, build_env): '''Build one artifact. @@ -307,6 +301,10 @@ class BuildCommand(object): in either the local or remote cache already. ''' + self.app.status(msg='Building %(kind)s %(name)s', + name=artifact.name, + kind=artifact.source.morphology['kind']) + self.get_sources(artifact) deps = self.get_recursive_deps(artifact) self.cache_artifacts_locally(deps) -- cgit v1.2.1 From 92547c0203a5b676020dd1f6816022454607c7b4 Mon Sep 17 00:00:00 2001 From: Sam Thursfield Date: Tue, 3 Jun 2014 16:11:42 +0000 Subject: Ensure that transferring an artifact from the remote cache is atomic Artifacts can have multiple parts; while this may not be an ideal design, changing the format of artifacts has implications for backwards compatibility. We should transfer all parts at once and delete them all if we encounter any errors, to reduce the change of getting the local artifact cache into an inconsistent state. --- morphlib/buildcommand.py | 44 +++++++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 15 deletions(-) (limited to 'morphlib/buildcommand.py') diff --git a/morphlib/buildcommand.py b/morphlib/buildcommand.py index 5e080a41..222b7127 100644 --- a/morphlib/buildcommand.py +++ b/morphlib/buildcommand.py @@ -387,27 +387,41 @@ class BuildCommand(object): def cache_artifacts_locally(self, artifacts): '''Get artifacts missing from local cache from remote cache.''' - def copy(remote, local): - shutil.copyfileobj(remote, local) - remote.close() - local.close() + def fetch_files(to_fetch): + '''Fetch a set of files atomically. + + If an error occurs during the transfer of any files, all downloaded + data is deleted, to ensure integrity of the local cache. + + ''' + try: + for remote, local in to_fetch: + shutil.copyfileobj(remote, local) + for remote, local in to_fetch: + remote.close() + local.close() + except BaseException: + for remote, local in to_fetch: + local.abort() + raise for artifact in artifacts: + to_fetch = [] if not self.lac.has(artifact): - self.app.status(msg='Fetching to local cache: ' - 'artifact %(name)s', - name=artifact.name) - rac_file = self.rac.get(artifact) - lac_file = self.lac.put(artifact) - copy(rac_file, lac_file) + to_fetch.append((self.rac.get(artifact), + self.lac.put(artifact))) if artifact.source.morphology.needs_artifact_metadata_cached: if not self.lac.has_artifact_metadata(artifact, 'meta'): - self.app.status(msg='Fetching to local cache: ' - 'artifact metadata %(name)s', - name=artifact.name) - copy(self.rac.get_artifact_metadata(artifact, 'meta'), - self.lac.put_artifact_metadata(artifact, 'meta')) + to_fetch.append(( + self.rac.get_artifact_metadata(artifact, 'meta'), + self.lac.put_artifact_metadata(artifact, 'meta'))) + + if len(to_fetch) > 0: + self.app.status( + msg='Fetching to local cache: artifact %(name)s', + name=artifact.name) + fetch_files(to_fetch) def create_staging_area(self, build_env, use_chroot=True, extra_env={}, extra_path=[]): -- cgit v1.2.1 From 29c836174baff2b6fa02d8ce7109ff656c4d4834 Mon Sep 17 00:00:00 2001 From: Sam Thursfield Date: Tue, 10 Jun 2014 12:18:13 +0100 Subject: Fix up sam/remote-artifact-cache-failure branch after review --- morphlib/buildcommand.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) (limited to 'morphlib/buildcommand.py') diff --git a/morphlib/buildcommand.py b/morphlib/buildcommand.py index 222b7127..5abef54c 100644 --- a/morphlib/buildcommand.py +++ b/morphlib/buildcommand.py @@ -284,6 +284,12 @@ class BuildCommand(object): self.app.status_prefix = old_prefix def cache_or_build_artifact(self, artifact, build_env): + '''Make the built artifact available in the local cache. + + This can be done by retrieving from a remote artifact cache, or if + that doesn't work for some reason, by building the artifact locally. + + ''' if self.rac is not None: try: self.cache_artifacts_locally([artifact]) @@ -397,13 +403,14 @@ class BuildCommand(object): try: for remote, local in to_fetch: shutil.copyfileobj(remote, local) - for remote, local in to_fetch: - remote.close() - local.close() except BaseException: for remote, local in to_fetch: local.abort() raise + else: + for remote, local in to_fetch: + remote.close() + local.close() for artifact in artifacts: to_fetch = [] -- cgit v1.2.1