Improve error handling in buildcache downloads (#35568)

The checksum exception was not detailed enough and not reraised when using cache only, resulting in useless error messages.

Now it dumps the file path, expected
hash, computed hash, and the downloaded file summary.
This commit is contained in:
Harmen Stoppels 2023-02-18 19:22:48 +01:00 committed by GitHub
parent c42a4ec1ec
commit 86320eb569
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 67 additions and 44 deletions

View File

@ -2630,3 +2630,28 @@ def temporary_dir(
yield tmp_dir yield tmp_dir
finally: finally:
remove_directory_contents(tmp_dir) remove_directory_contents(tmp_dir)
def filesummary(path, print_bytes=16) -> Tuple[int, bytes]:
"""Create a small summary of the given file. Does not error
when file does not exist.
Args:
print_bytes (int): Number of bytes to print from start/end of file
Returns:
Tuple of size and byte string containing first n .. last n bytes.
Size is 0 if file cannot be read."""
try:
n = print_bytes
with open(path, "rb") as f:
size = os.fstat(f.fileno()).st_size
if size <= 2 * n:
short_contents = f.read(2 * n)
else:
short_contents = f.read(n)
f.seek(-n, 2)
short_contents += b"..." + f.read(n)
return size, short_contents
except OSError:
return 0, b""

View File

@ -41,6 +41,7 @@
import spack.repo import spack.repo
import spack.store import spack.store
import spack.traverse as traverse import spack.traverse as traverse
import spack.util.crypto
import spack.util.file_cache as file_cache import spack.util.file_cache as file_cache
import spack.util.gpg import spack.util.gpg
import spack.util.spack_json as sjson import spack.util.spack_json as sjson
@ -553,7 +554,12 @@ class NoChecksumException(spack.error.SpackError):
Raised if file fails checksum verification. Raised if file fails checksum verification.
""" """
pass def __init__(self, path, size, contents, algorithm, expected, computed):
super(NoChecksumException, self).__init__(
f"{algorithm} checksum failed for {path}",
f"Expected {expected} but got {computed}. "
f"File size = {size} bytes. Contents = {contents!r}",
)
class NewLayoutException(spack.error.SpackError): class NewLayoutException(spack.error.SpackError):
@ -1763,14 +1769,15 @@ def _extract_inner_tarball(spec, filename, extract_to, unsigned, remote_checksum
raise UnsignedPackageException( raise UnsignedPackageException(
"To install unsigned packages, use the --no-check-signature option." "To install unsigned packages, use the --no-check-signature option."
) )
# get the sha256 checksum of the tarball
# compute the sha256 checksum of the tarball
local_checksum = checksum_tarball(tarfile_path) local_checksum = checksum_tarball(tarfile_path)
expected = remote_checksum["hash"]
# if the checksums don't match don't install # if the checksums don't match don't install
if local_checksum != remote_checksum["hash"]: if local_checksum != expected:
raise NoChecksumException( size, contents = fsys.filesummary(tarfile_path)
"Package tarball failed checksum verification.\n" "It cannot be installed." raise NoChecksumException(tarfile_path, size, contents, "sha256", expected, local_checksum)
)
return tarfile_path return tarfile_path
@ -1828,12 +1835,14 @@ def extract_tarball(spec, download_result, allow_root=False, unsigned=False, for
# compute the sha256 checksum of the tarball # compute the sha256 checksum of the tarball
local_checksum = checksum_tarball(tarfile_path) local_checksum = checksum_tarball(tarfile_path)
expected = bchecksum["hash"]
# if the checksums don't match don't install # if the checksums don't match don't install
if local_checksum != bchecksum["hash"]: if local_checksum != expected:
size, contents = fsys.filesummary(tarfile_path)
_delete_staged_downloads(download_result) _delete_staged_downloads(download_result)
raise NoChecksumException( raise NoChecksumException(
"Package tarball failed checksum verification.\n" "It cannot be installed." tarfile_path, size, contents, "sha256", expected, local_checksum
) )
new_relative_prefix = str(os.path.relpath(spec.prefix, spack.store.layout.root)) new_relative_prefix = str(os.path.relpath(spec.prefix, spack.store.layout.root))
@ -1924,8 +1933,11 @@ def install_root_node(spec, allow_root, unsigned=False, force=False, sha256=None
tarball_path = download_result["tarball_stage"].save_filename tarball_path = download_result["tarball_stage"].save_filename
msg = msg.format(tarball_path, sha256) msg = msg.format(tarball_path, sha256)
if not checker.check(tarball_path): if not checker.check(tarball_path):
size, contents = fsys.filesummary(tarball_path)
_delete_staged_downloads(download_result) _delete_staged_downloads(download_result)
raise spack.binary_distribution.NoChecksumException(msg) raise NoChecksumException(
tarball_path, size, contents, checker.hash_name, sha256, checker.sum
)
tty.debug("Verified SHA256 checksum of the build cache") tty.debug("Verified SHA256 checksum of the build cache")
# don't print long padded paths while extracting/relocating binaries # don't print long padded paths while extracting/relocating binaries

View File

@ -89,22 +89,6 @@ def _ensure_one_stage_entry(stage_path):
return os.path.join(stage_path, stage_entries[0]) return os.path.join(stage_path, stage_entries[0])
def _filesummary(path, print_bytes=16):
try:
n = print_bytes
with open(path, "rb") as f:
size = os.fstat(f.fileno()).st_size
if size <= 2 * n:
short_contents = f.read(2 * n)
else:
short_contents = f.read(n)
f.seek(-n, 2)
short_contents += b"..." + f.read(n)
return size, short_contents
except OSError:
return 0, b""
def fetcher(cls): def fetcher(cls):
"""Decorator used to register fetch strategies.""" """Decorator used to register fetch strategies."""
all_strategies.append(cls) all_strategies.append(cls)
@ -513,7 +497,7 @@ def check(self):
# On failure, provide some information about the file size and # On failure, provide some information about the file size and
# contents, so that we can quickly see what the issue is (redirect # contents, so that we can quickly see what the issue is (redirect
# was not followed, empty file, text instead of binary, ...) # was not followed, empty file, text instead of binary, ...)
size, contents = _filesummary(self.archive_file) size, contents = fs.filesummary(self.archive_file)
raise ChecksumError( raise ChecksumError(
f"{checker.hash_name} checksum failed for {self.archive_file}", f"{checker.hash_name} checksum failed for {self.archive_file}",
f"Expected {self.digest} but got {checker.sum}. " f"Expected {self.digest} but got {checker.sum}. "

View File

@ -1755,14 +1755,16 @@ def install(self):
raise raise
except binary_distribution.NoChecksumException as exc: except binary_distribution.NoChecksumException as exc:
if not task.cache_only: if task.cache_only:
# Checking hash on downloaded binary failed. raise
err = "Failed to install {0} from binary cache due to {1}:"
err += " Requeueing to install from source." # Checking hash on downloaded binary failed.
tty.error(err.format(pkg.name, str(exc))) err = "Failed to install {0} from binary cache due to {1}:"
task.use_cache = False err += " Requeueing to install from source."
self._requeue_task(task) tty.error(err.format(pkg.name, str(exc)))
continue task.use_cache = False
self._requeue_task(task)
continue
except (Exception, SystemExit) as exc: except (Exception, SystemExit) as exc:
self._update_failed(task, True, exc) self._update_failed(task, True, exc)

View File

@ -14,13 +14,3 @@ def test_fetchstrategy_bad_url_scheme():
with pytest.raises(ValueError): with pytest.raises(ValueError):
fetcher = fetch_strategy.from_url_scheme("bogus-scheme://example.com/a/b/c") # noqa: F841 fetcher = fetch_strategy.from_url_scheme("bogus-scheme://example.com/a/b/c") # noqa: F841
def test_filesummary(tmpdir):
p = str(tmpdir.join("xyz"))
with open(p, "wb") as f:
f.write(b"abcdefghijklmnopqrstuvwxyz")
assert fetch_strategy._filesummary(p, print_bytes=8) == (26, b"abcdefgh...stuvwxyz")
assert fetch_strategy._filesummary(p, print_bytes=13) == (26, b"abcdefghijklmnopqrstuvwxyz")
assert fetch_strategy._filesummary(p, print_bytes=100) == (26, b"abcdefghijklmnopqrstuvwxyz")

View File

@ -861,3 +861,13 @@ def test_remove_linked_tree_doesnt_change_file_permission(tmpdir, initial_mode):
fs.remove_linked_tree(str(file_instead_of_dir)) fs.remove_linked_tree(str(file_instead_of_dir))
final_stat = os.stat(str(file_instead_of_dir)) final_stat = os.stat(str(file_instead_of_dir))
assert final_stat == initial_stat assert final_stat == initial_stat
def test_filesummary(tmpdir):
p = str(tmpdir.join("xyz"))
with open(p, "wb") as f:
f.write(b"abcdefghijklmnopqrstuvwxyz")
assert fs.filesummary(p, print_bytes=8) == (26, b"abcdefgh...stuvwxyz")
assert fs.filesummary(p, print_bytes=13) == (26, b"abcdefghijklmnopqrstuvwxyz")
assert fs.filesummary(p, print_bytes=100) == (26, b"abcdefghijklmnopqrstuvwxyz")