From a5e8299d1a119b9d757ae28a57612f633894d2f6 Mon Sep 17 00:00:00 2001
From: Nicholas Piggin <npiggin@gmail.com>
Date: Wed, 12 Mar 2025 23:00:00 +1000
Subject: tests/functional/asset: Fail assert fetch when retries are exceeded
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Currently the fetch code does not fail gracefully when retry limit is
exceeded, it just falls through the loop with no file, which ends up
hitting other errors.
Add a check for non-existing file, which indicates the retry limit was
exceeded.
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Message-ID: <20250312130002.945508-2-npiggin@gmail.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
tests/functional/qemu_test/asset.py | 3 +++
1 file changed, 3 insertions(+)
(limited to 'tests/functional/qemu_test/asset.py')
diff --git a/tests/functional/qemu_test/asset.py b/tests/functional/qemu_test/asset.py
index f073069..27dd839 100644
--- a/tests/functional/qemu_test/asset.py
+++ b/tests/functional/qemu_test/asset.py
@@ -138,6 +138,9 @@ class Asset:
tmp_cache_file.unlink()
raise
+ if not os.path.exists(tmp_cache_file):
+ raise Exception("Retries exceeded downloading %s", self.url)
+
try:
# Set these just for informational purposes
os.setxattr(str(tmp_cache_file), "user.qemu-asset-url",
--
cgit v1.1
From 7524e1b33679dc1356e8bb4efdd18e83fc50f5cc Mon Sep 17 00:00:00 2001
From: Nicholas Piggin <npiggin@gmail.com>
Date: Wed, 12 Mar 2025 23:00:01 +1000
Subject: tests/functional/asset: Verify downloaded size
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
If the server provides a Content-Length header, use that to verify the
size of the downloaded file. This catches cases where the connection
terminates early, and gives the opportunity to retry. Without this, the
checksum will likely mismatch and fail without retry.
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Message-ID: <20250312130002.945508-3-npiggin@gmail.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
tests/functional/qemu_test/asset.py | 14 ++++++++++++++
1 file changed, 14 insertions(+)
(limited to 'tests/functional/qemu_test/asset.py')
diff --git a/tests/functional/qemu_test/asset.py b/tests/functional/qemu_test/asset.py
index 27dd839..6bbfb9e 100644
--- a/tests/functional/qemu_test/asset.py
+++ b/tests/functional/qemu_test/asset.py
@@ -121,6 +121,20 @@ class Asset:
with tmp_cache_file.open("xb") as dst:
with urllib.request.urlopen(self.url) as resp:
copyfileobj(resp, dst)
+ length_hdr = resp.getheader("Content-Length")
+
+ # Verify downloaded file size against length metadata, if
+ # available.
+ if length_hdr is not None:
+ length = int(length_hdr)
+ fsize = tmp_cache_file.stat().st_size
+ if fsize != length:
+ self.log.error("Unable to download %s: "
+ "connection closed before "
+ "transfer complete (%d/%d)",
+ self.url, fsize, length)
+ tmp_cache_file.unlink()
+ continue
break
except FileExistsError:
self.log.debug("%s already exists, "
--
cgit v1.1
From 28adad0a4d9f1f64f9f04748c6348a64ba7ad990 Mon Sep 17 00:00:00 2001
From: Nicholas Piggin <npiggin@gmail.com>
Date: Wed, 12 Mar 2025 23:00:02 +1000
Subject: tests/functional/asset: Add AssetError exception class
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Assets are uniquely identified by human-readable-ish url, so make an
AssetError exception class that prints url with error message.
A property 'transient' is used to capture whether the client may retry
or try again later, or if it is a serious and likely permanent error.
This is used to retain the existing behaviour of treating HTTP errors
other than 404 as 'transient' and not causing precache step to fail.
Additionally, partial-downloads and stale asset caches that fail to
resolve after the retry limit are now treated as transient and do not
cause precache step to fail.
For background: The NetBSD archive is, at the time of writing, failing
with short transfer. Retrying the fetch at that position (as wget does)
results in a "503 backend unavailable" error. We would like to get that
error code directly, but I have not found a way to do that with urllib,
so treating the short-copy as a transient failure covers that case (and
seems like a reasonable way to handle it in general).
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Message-ID: <20250312130002.945508-4-npiggin@gmail.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
tests/functional/qemu_test/asset.py | 43 ++++++++++++++++++++++++-------------
1 file changed, 28 insertions(+), 15 deletions(-)
(limited to 'tests/functional/qemu_test/asset.py')
diff --git a/tests/functional/qemu_test/asset.py b/tests/functional/qemu_test/asset.py
index 6bbfb9e..704b84d 100644
--- a/tests/functional/qemu_test/asset.py
+++ b/tests/functional/qemu_test/asset.py
@@ -17,6 +17,14 @@ from pathlib import Path
from shutil import copyfileobj
from urllib.error import HTTPError
+class AssetError(Exception):
+ def __init__(self, asset, msg, transient=False):
+ self.url = asset.url
+ self.msg = msg
+ self.transient = transient
+
+ def __str__(self):
+ return "%s: %s" % (self.url, self.msg)
# Instances of this class must be declared as class level variables
# starting with a name "ASSET_". This enables the pre-caching logic
@@ -51,7 +59,7 @@ class Asset:
elif len(self.hash) == 128:
hl = hashlib.sha512()
else:
- raise Exception("unknown hash type")
+ raise AssetError(self, "unknown hash type")
# Calculate the hash of the file:
with open(cache_file, 'rb') as file:
@@ -111,7 +119,8 @@ class Asset:
return str(self.cache_file)
if not self.fetchable():
- raise Exception("Asset cache is invalid and downloads disabled")
+ raise AssetError(self,
+ "Asset cache is invalid and downloads disabled")
self.log.info("Downloading %s to %s...", self.url, self.cache_file)
tmp_cache_file = self.cache_file.with_suffix(".download")
@@ -147,13 +156,23 @@ class Asset:
tmp_cache_file)
tmp_cache_file.unlink()
continue
+ except HTTPError as e:
+ tmp_cache_file.unlink()
+ self.log.error("Unable to download %s: HTTP error %d",
+ self.url, e.code)
+ # Treat 404 as fatal, since it is highly likely to
+ # indicate a broken test rather than a transient
+ # server or networking problem
+ if e.code == 404:
+ raise AssetError(self, "Unable to download: "
+ "HTTP error %d" % e.code)
+ continue
except Exception as e:
- self.log.error("Unable to download %s: %s", self.url, e)
tmp_cache_file.unlink()
- raise
+ raise AssetError(self, "Unable to download: " % e)
if not os.path.exists(tmp_cache_file):
- raise Exception("Retries exceeded downloading %s", self.url)
+ raise AssetError(self, "Download retries exceeded", transient=True)
try:
# Set these just for informational purposes
@@ -167,8 +186,7 @@ class Asset:
if not self._check(tmp_cache_file):
tmp_cache_file.unlink()
- raise Exception("Hash of %s does not match %s" %
- (self.url, self.hash))
+ raise AssetError(self, "Hash does not match %s" % self.hash)
tmp_cache_file.replace(self.cache_file)
# Remove write perms to stop tests accidentally modifying them
os.chmod(self.cache_file, stat.S_IRUSR | stat.S_IRGRP)
@@ -190,15 +208,10 @@ class Asset:
log.info("Attempting to cache '%s'" % asset)
try:
asset.fetch()
- except HTTPError as e:
- # Treat 404 as fatal, since it is highly likely to
- # indicate a broken test rather than a transient
- # server or networking problem
- if e.code == 404:
+ except AssetError as e:
+ if not e.transient:
raise
-
- log.debug(f"HTTP error {e.code} from {asset.url} " +
- "skipping asset precache")
+ log.error("%s: skipping asset precache" % e)
log.removeHandler(handler)
--
cgit v1.1