From af8acd80ca4326d82266f5d8fa2eca0cca1e1fbc Mon Sep 17 00:00:00 2001 From: Sam Thursfield Date: Tue, 3 Jun 2014 14:40:47 +0000 Subject: Make all remote artifact cache get errors inherit from GetError This saves from having to catch three separate exceptions. --- morphlib/remoteartifactcache.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/morphlib/remoteartifactcache.py b/morphlib/remoteartifactcache.py index 9f6bf69e..3fe0f444 100644 --- a/morphlib/remoteartifactcache.py +++ b/morphlib/remoteartifactcache.py @@ -1,4 +1,4 @@ -# Copyright (C) 2012-2013 Codethink Limited +# Copyright (C) 2012-2014 Codethink Limited # # This program is free software; you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -36,7 +36,7 @@ class GetError(cliapp.AppException): (artifact, artifact.cache_key, cache)) -class GetArtifactMetadataError(cliapp.AppException): +class GetArtifactMetadataError(GetError): def __init__(self, cache, artifact, name): cliapp.AppException.__init__( @@ -44,7 +44,7 @@ class GetArtifactMetadataError(cliapp.AppException): 'from the artifact cache %s' % (name, artifact, cache)) -class GetSourceMetadataError(cliapp.AppException): +class GetSourceMetadataError(GetError): def __init__(self, cache, source, cache_key, name): cliapp.AppException.__init__( -- cgit v1.2.1 From d87d73d2121638cb12f3a17ca01562f2d3e33e48 Mon Sep 17 00:00:00 2001 From: Sam Thursfield Date: Tue, 3 Jun 2014 15:40:27 +0000 Subject: Tweak exception message of remote artifact cache GetError Before: ERROR: Failed to get metadata meta for the artifact file:///src/ws-baserock-hawk/baserock/ps/build-system/baserock/baserock/definitions|refs/heads/baserock/builds/778b1a370a1f43c497c1354a2a949de1/56c9ec89d09240fd80faa7d2226b7eda|core|core-devel from the artifact cache http://git.baserock.org:8080/ After: ERROR: Failed to get metadata meta for the artifact f896a081beacd4a99ded38d28b44fbf02970038fb53349265f85f8f3298ead9d.stratum.core-devel from the artifact cache http://git.baserock.org:8080/ When debugging artifact cache issues, the information that's most useful is the filename of the artfact. --- morphlib/remoteartifactcache.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/morphlib/remoteartifactcache.py b/morphlib/remoteartifactcache.py index 3fe0f444..0f8edce8 100644 --- a/morphlib/remoteartifactcache.py +++ b/morphlib/remoteartifactcache.py @@ -33,7 +33,7 @@ class GetError(cliapp.AppException): cliapp.AppException.__init__( self, 'Failed to get the artifact %s with cache key %s ' 'from the artifact cache %s' % - (artifact, artifact.cache_key, cache)) + (artifact.basename(), artifact.cache_key, cache)) class GetArtifactMetadataError(GetError): @@ -41,7 +41,8 @@ class GetArtifactMetadataError(GetError): def __init__(self, cache, artifact, name): cliapp.AppException.__init__( self, 'Failed to get metadata %s for the artifact %s ' - 'from the artifact cache %s' % (name, artifact, cache)) + 'from the artifact cache %s' % + (name, artifact.basename(), cache)) class GetSourceMetadataError(GetError): -- cgit v1.2.1 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 ++++++++++++++---------------- morphlib/plugins/cross-bootstrap_plugin.py | 14 ++---------- 2 files changed, 18 insertions(+), 30 deletions(-) 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) diff --git a/morphlib/plugins/cross-bootstrap_plugin.py b/morphlib/plugins/cross-bootstrap_plugin.py index ec0cfbcb..bfd0d047 100644 --- a/morphlib/plugins/cross-bootstrap_plugin.py +++ b/morphlib/plugins/cross-bootstrap_plugin.py @@ -1,4 +1,4 @@ -# Copyright (C) 2013 Codethink Limited +# Copyright (C) 2013-2014 Codethink Limited # # This program is free software; you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -299,18 +299,8 @@ class CrossBootstrapPlugin(cliapp.Plugin): 'Nothing to cross-compile. Only chunks built in \'bootstrap\' ' 'mode can be cross-compiled.') - # FIXME: merge with build-command's code for i, a in enumerate(cross_chunks): - if build_command.is_built(a): - self.app.status(msg='The %(kind)s %(name)s is already built', - kind=a.source.morphology['kind'], - name=a.name) - build_command.cache_artifacts_locally([a]) - else: - self.app.status(msg='Cross-building %(kind)s %(name)s', - kind=a.source.morphology['kind'], - name=a.name) - build_command.build_artifact(a, build_env) + build_command.cache_or_build_artifact(a, build_env) for i, a in enumerate(native_chunks): build_command.get_sources(a) -- 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(-) 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(-) 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