[PATCH 0/3] tests/functional: improve handling flaky downloads

[PATCH 0/3] tests/functional: improve handling flaky downloads

[Daniel P. Berrangé]

This fixes the functional-system-debian job with the currently
awol archive.openwrt.org server.

  https://gitlab.com/berrange/qemu/-/jobs/11173491503

We can see teh download fail & retry, and then gracefully
skip pre-cache:

2025-08-29 08:02:16,510 - qemu-test - INFO - Downloading https://archive.openwrt.org/chaos_calmer/15.05.1/realview/generic/openwrt-15.05.1-realview-vmlinux-initramfs.elf to /builds/berrange/qemu/functional-cache/download/d3a01037f33e7512d46d50975588d5c3a0e0cbf25f37afab44775c2a2be523e6...
2025-08-29 08:04:28,104 - qemu-test - ERROR - Unable to download https://archive.openwrt.org/chaos_calmer/15.05.1/realview/generic/openwrt-15.05.1-realview-vmlinux-initramfs.elf: URL error <urlopen error [Errno 101] Network is unreachable>
2025-08-29 08:06:39,177 - qemu-test - ERROR - Unable to download https://archive.openwrt.org/chaos_calmer/15.05.1/realview/generic/openwrt-15.05.1-realview-vmlinux-initramfs.elf: URL error <urlopen error [Errno 101] Network is unreachable>
2025-08-29 08:08:50,248 - qemu-test - ERROR - Unable to download https://archive.openwrt.org/chaos_calmer/15.05.1/realview/generic/openwrt-15.05.1-realview-vmlinux-initramfs.elf: URL error <urlopen error [Errno 101] Network is unreachable>
2025-08-29 08:08:50,250 - qemu-test - ERROR - https://archive.openwrt.org/chaos_calmer/15.05.1/realview/generic/openwrt-15.05.1-realview-vmlinux-initramfs.elf: Download retries exceeded: skipping asset precache

And then later the test itself is skipped:

55/67 qemu:func-thorough+func-arm-thorough+thorough / func-arm-realview            SKIP              0.11s   0 subtests passed

Daniel P. Berrangé (3):
  tests/functional: enable force refresh of cached assets
  tests/functional: fix formatting of exception args
  tests/functional: handle URLError when fetching assets

 docs/devel/testing/functional.rst   |  3 +++
 tests/functional/qemu_test/asset.py | 20 ++++++++++++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)

-- 
2.50.1

[PATCH 1/3] tests/functional: enable force refresh of cached assets

[Daniel P. Berrangé]

If the 'QEMU_TEST_REFRESH_CACHE' environment variable is set, then
ignore any existing cached asset and download a fresh copy.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/devel/testing/functional.rst   | 3 +++
 tests/functional/qemu_test/asset.py | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/docs/devel/testing/functional.rst b/docs/devel/testing/functional.rst
index 3728bab6c0..fdeaebaadc 100644
--- a/docs/devel/testing/functional.rst
+++ b/docs/devel/testing/functional.rst
@@ -312,6 +312,9 @@ The cache is populated in the ``~/.cache/qemu/download`` directory by
 default, but the location can be changed by setting the
 ``QEMU_TEST_CACHE_DIR`` environment variable.
 
+To force the test suite to re-download the cache, even if still valid,
+set the ``QEMU_TEST_REFRESH_CACHE`` environment variable.
+
 Skipping tests
 --------------
 
diff --git a/tests/functional/qemu_test/asset.py b/tests/functional/qemu_test/asset.py
index 704b84d0ea..b5a6136d36 100644
--- a/tests/functional/qemu_test/asset.py
+++ b/tests/functional/qemu_test/asset.py
@@ -72,6 +72,10 @@ def _check(self, cache_file):
         return self.hash == hl.hexdigest()
 
     def valid(self):
+        if os.getenv("QEMU_TEST_REFRESH_CACHE", None) is not None:
+            self.log.info("Force refresh of asset %s", self.url)
+            return False
+
         return self.cache_file.exists() and self._check(self.cache_file)
 
     def fetchable(self):
-- 
2.50.1

Re: [PATCH 1/3] tests/functional: enable force refresh of cached assets

[Thomas Huth]

On 29/08/2025 10.31, Daniel P. Berrangé wrote:
> If the 'QEMU_TEST_REFRESH_CACHE' environment variable is set, then
> ignore any existing cached asset and download a fresh copy.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   docs/devel/testing/functional.rst   | 3 +++
>   tests/functional/qemu_test/asset.py | 4 ++++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/docs/devel/testing/functional.rst b/docs/devel/testing/functional.rst
> index 3728bab6c0..fdeaebaadc 100644
> --- a/docs/devel/testing/functional.rst
> +++ b/docs/devel/testing/functional.rst
> @@ -312,6 +312,9 @@ The cache is populated in the ``~/.cache/qemu/download`` directory by
>   default, but the location can be changed by setting the
>   ``QEMU_TEST_CACHE_DIR`` environment variable.
>   
> +To force the test suite to re-download the cache, even if still valid,
> +set the ``QEMU_TEST_REFRESH_CACHE`` environment variable.
> +
>   Skipping tests
>   --------------
>   
> diff --git a/tests/functional/qemu_test/asset.py b/tests/functional/qemu_test/asset.py
> index 704b84d0ea..b5a6136d36 100644
> --- a/tests/functional/qemu_test/asset.py
> +++ b/tests/functional/qemu_test/asset.py
> @@ -72,6 +72,10 @@ def _check(self, cache_file):
>           return self.hash == hl.hexdigest()
>   
>       def valid(self):
> +        if os.getenv("QEMU_TEST_REFRESH_CACHE", None) is not None:
> +            self.log.info("Force refresh of asset %s", self.url)
> +            return False
> +

Do we really need this? You can also simply "rm ~/.cache/qemu/downloads" 
instead, or click on the "Clear runner caches" in the gitlab CI.

  Thomas

Re: [PATCH 1/3] tests/functional: enable force refresh of cached assets

[Daniel P. Berrangé]

On Fri, Aug 29, 2025 at 10:50:41AM +0200, Thomas Huth wrote:
> On 29/08/2025 10.31, Daniel P. Berrangé wrote:
> > If the 'QEMU_TEST_REFRESH_CACHE' environment variable is set, then
> > ignore any existing cached asset and download a fresh copy.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   docs/devel/testing/functional.rst   | 3 +++
> >   tests/functional/qemu_test/asset.py | 4 ++++
> >   2 files changed, 7 insertions(+)
> > 
> > diff --git a/docs/devel/testing/functional.rst b/docs/devel/testing/functional.rst
> > index 3728bab6c0..fdeaebaadc 100644
> > --- a/docs/devel/testing/functional.rst
> > +++ b/docs/devel/testing/functional.rst
> > @@ -312,6 +312,9 @@ The cache is populated in the ``~/.cache/qemu/download`` directory by
> >   default, but the location can be changed by setting the
> >   ``QEMU_TEST_CACHE_DIR`` environment variable.
> > +To force the test suite to re-download the cache, even if still valid,
> > +set the ``QEMU_TEST_REFRESH_CACHE`` environment variable.
> > +
> >   Skipping tests
> >   --------------
> > diff --git a/tests/functional/qemu_test/asset.py b/tests/functional/qemu_test/asset.py
> > index 704b84d0ea..b5a6136d36 100644
> > --- a/tests/functional/qemu_test/asset.py
> > +++ b/tests/functional/qemu_test/asset.py
> > @@ -72,6 +72,10 @@ def _check(self, cache_file):
> >           return self.hash == hl.hexdigest()
> >       def valid(self):
> > +        if os.getenv("QEMU_TEST_REFRESH_CACHE", None) is not None:
> > +            self.log.info("Force refresh of asset %s", self.url)
> > +            return False
> > +
> 
> Do we really need this? You can also simply "rm ~/.cache/qemu/downloads"
> instead, or click on the "Clear runner caches" in the gitlab CI.

This can selectively refresh assets from a single test

  QEMU_TEST_REFRESH_CACHE=1 ./tests/functional/test_blah.py


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/3] tests/functional: enable force refresh of cached assets

[Thomas Huth]

On 29/08/2025 11.04, Daniel P. Berrangé wrote:
> On Fri, Aug 29, 2025 at 10:50:41AM +0200, Thomas Huth wrote:
>> On 29/08/2025 10.31, Daniel P. Berrangé wrote:
>>> If the 'QEMU_TEST_REFRESH_CACHE' environment variable is set, then
>>> ignore any existing cached asset and download a fresh copy.
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>    docs/devel/testing/functional.rst   | 3 +++
>>>    tests/functional/qemu_test/asset.py | 4 ++++
>>>    2 files changed, 7 insertions(+)
>>>
>>> diff --git a/docs/devel/testing/functional.rst b/docs/devel/testing/functional.rst
>>> index 3728bab6c0..fdeaebaadc 100644
>>> --- a/docs/devel/testing/functional.rst
>>> +++ b/docs/devel/testing/functional.rst
>>> @@ -312,6 +312,9 @@ The cache is populated in the ``~/.cache/qemu/download`` directory by
>>>    default, but the location can be changed by setting the
>>>    ``QEMU_TEST_CACHE_DIR`` environment variable.
>>> +To force the test suite to re-download the cache, even if still valid,
>>> +set the ``QEMU_TEST_REFRESH_CACHE`` environment variable.
>>> +
>>>    Skipping tests
>>>    --------------
>>> diff --git a/tests/functional/qemu_test/asset.py b/tests/functional/qemu_test/asset.py
>>> index 704b84d0ea..b5a6136d36 100644
>>> --- a/tests/functional/qemu_test/asset.py
>>> +++ b/tests/functional/qemu_test/asset.py
>>> @@ -72,6 +72,10 @@ def _check(self, cache_file):
>>>            return self.hash == hl.hexdigest()
>>>        def valid(self):
>>> +        if os.getenv("QEMU_TEST_REFRESH_CACHE", None) is not None:
>>> +            self.log.info("Force refresh of asset %s", self.url)
>>> +            return False
>>> +
>>
>> Do we really need this? You can also simply "rm ~/.cache/qemu/downloads"
>> instead, or click on the "Clear runner caches" in the gitlab CI.
> 
> This can selectively refresh assets from a single test
> 
>    QEMU_TEST_REFRESH_CACHE=1 ./tests/functional/test_blah.py

Ok, fair point.

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 2/3] tests/functional: fix formatting of exception args

[Daniel P. Berrangé]

On HTTP 404 errors, we failed to include the URL in the exception
that was raised, while for the catch-all exception we failed to
include the URL and forgot the placeholder for the exception
details too.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/functional/qemu_test/asset.py | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/functional/qemu_test/asset.py b/tests/functional/qemu_test/asset.py
index b5a6136d36..ae2bec3ea5 100644
--- a/tests/functional/qemu_test/asset.py
+++ b/tests/functional/qemu_test/asset.py
@@ -168,12 +168,13 @@ def fetch(self):
                 # 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)
+                    raise AssetError(self, "Unable to download %s: "
+                                     "HTTP error %d" % (self.url, e.code))
                 continue
             except Exception as e:
                 tmp_cache_file.unlink()
-                raise AssetError(self, "Unable to download: " % e)
+                raise AssetError(self, "Unable to download %s: %s" % (
+                    self.url, e))
 
         if not os.path.exists(tmp_cache_file):
             raise AssetError(self, "Download retries exceeded", transient=True)
-- 
2.50.1

Re: [PATCH 2/3] tests/functional: fix formatting of exception args

[Thomas Huth]

On 29/08/2025 10.31, Daniel P. Berrangé wrote:
> On HTTP 404 errors, we failed to include the URL in the exception
> that was raised, while for the catch-all exception we failed to
> include the URL and forgot the placeholder for the exception
> details too.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/functional/qemu_test/asset.py | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/functional/qemu_test/asset.py b/tests/functional/qemu_test/asset.py
> index b5a6136d36..ae2bec3ea5 100644
> --- a/tests/functional/qemu_test/asset.py
> +++ b/tests/functional/qemu_test/asset.py
> @@ -168,12 +168,13 @@ def fetch(self):
>                   # 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)
> +                    raise AssetError(self, "Unable to download %s: "
> +                                     "HTTP error %d" % (self.url, e.code))
>                   continue
>               except Exception as e:
>                   tmp_cache_file.unlink()
> -                raise AssetError(self, "Unable to download: " % e)
> +                raise AssetError(self, "Unable to download %s: %s" % (
> +                    self.url, e))
>   
>           if not os.path.exists(tmp_cache_file):
>               raise AssetError(self, "Download retries exceeded", transient=True)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 3/3] tests/functional: handle URLError when fetching assets

[Daniel P. Berrangé]

We treat most HTTP errors as non-fatal when fetching assets,
but forgot to handle network level errors. This adds catching
of URLError so that we retry on failure, and will ultimately
trigger graceful skipping in the pre-cache task.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/functional/qemu_test/asset.py | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tests/functional/qemu_test/asset.py b/tests/functional/qemu_test/asset.py
index ae2bec3ea5..36f64fe2f6 100644
--- a/tests/functional/qemu_test/asset.py
+++ b/tests/functional/qemu_test/asset.py
@@ -15,7 +15,7 @@
 from time import sleep
 from pathlib import Path
 from shutil import copyfileobj
-from urllib.error import HTTPError
+from urllib.error import HTTPError, URLError
 
 class AssetError(Exception):
     def __init__(self, asset, msg, transient=False):
@@ -171,6 +171,13 @@ def fetch(self):
                     raise AssetError(self, "Unable to download %s: "
                                      "HTTP error %d" % (self.url, e.code))
                 continue
+            except URLError as e:
+                # This is typically a network/service level error
+                # eg urlopen error [Errno 110] Connection timed out>
+                tmp_cache_file.unlink()
+                self.log.error("Unable to download %s: URL error %s",
+                               self.url, e)
+                continue
             except Exception as e:
                 tmp_cache_file.unlink()
                 raise AssetError(self, "Unable to download %s: %s" % (
-- 
2.50.1

Re: [PATCH 3/3] tests/functional: handle URLError when fetching assets

[Thomas Huth]

On 29/08/2025 10.31, Daniel P. Berrangé wrote:
> We treat most HTTP errors as non-fatal when fetching assets,
> but forgot to handle network level errors. This adds catching
> of URLError so that we retry on failure, and will ultimately
> trigger graceful skipping in the pre-cache task.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/functional/qemu_test/asset.py | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/functional/qemu_test/asset.py b/tests/functional/qemu_test/asset.py
> index ae2bec3ea5..36f64fe2f6 100644
> --- a/tests/functional/qemu_test/asset.py
> +++ b/tests/functional/qemu_test/asset.py
> @@ -15,7 +15,7 @@
>   from time import sleep
>   from pathlib import Path
>   from shutil import copyfileobj
> -from urllib.error import HTTPError
> +from urllib.error import HTTPError, URLError
>   
>   class AssetError(Exception):
>       def __init__(self, asset, msg, transient=False):
> @@ -171,6 +171,13 @@ def fetch(self):
>                       raise AssetError(self, "Unable to download %s: "
>                                        "HTTP error %d" % (self.url, e.code))
>                   continue
> +            except URLError as e:
> +                # This is typically a network/service level error
> +                # eg urlopen error [Errno 110] Connection timed out>
> +                tmp_cache_file.unlink()
> +                self.log.error("Unable to download %s: URL error %s",
> +                               self.url, e)
> +                continue

Hmm, I don't think we should retry on each and every URLError. For example 
if you have a typo in the server name, you get a "Name or service not known" 
URLError, and it does not make sense to retry in that case.

Also, in case of the server being down, it takes a minute or two 'til the 
urllib gives up, so if you retry multiple times in that case, you can easily 
extend the test time by > 5 minutes (as you can see by the timestamps in the 
example in your cover letter), which is unfortunate, too (considering that 
there is also a timeout setting for the gitlab CI jobs).

I think I'd maybe rather do something like this instead, without retrying 
the download, just marking certain errors as transient:

        except URLError as e:
            tmp_cache_file.unlink()
            msg = str(e.reason)
            self.log.error("Unable to download %s: URL error %s",
                           self.url, msg)
            raise AssetError(self, msg,
                             transient=("Network is unreachable" in msg))

WDYT?

  Thomas

Re: [PATCH 3/3] tests/functional: handle URLError when fetching assets

[Daniel P. Berrangé]

On Fri, Aug 29, 2025 at 11:00:12AM +0200, Thomas Huth wrote:
> On 29/08/2025 10.31, Daniel P. Berrangé wrote:
> > We treat most HTTP errors as non-fatal when fetching assets,
> > but forgot to handle network level errors. This adds catching
> > of URLError so that we retry on failure, and will ultimately
> > trigger graceful skipping in the pre-cache task.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   tests/functional/qemu_test/asset.py | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/functional/qemu_test/asset.py b/tests/functional/qemu_test/asset.py
> > index ae2bec3ea5..36f64fe2f6 100644
> > --- a/tests/functional/qemu_test/asset.py
> > +++ b/tests/functional/qemu_test/asset.py
> > @@ -15,7 +15,7 @@
> >   from time import sleep
> >   from pathlib import Path
> >   from shutil import copyfileobj
> > -from urllib.error import HTTPError
> > +from urllib.error import HTTPError, URLError
> >   class AssetError(Exception):
> >       def __init__(self, asset, msg, transient=False):
> > @@ -171,6 +171,13 @@ def fetch(self):
> >                       raise AssetError(self, "Unable to download %s: "
> >                                        "HTTP error %d" % (self.url, e.code))
> >                   continue
> > +            except URLError as e:
> > +                # This is typically a network/service level error
> > +                # eg urlopen error [Errno 110] Connection timed out>
> > +                tmp_cache_file.unlink()
> > +                self.log.error("Unable to download %s: URL error %s",
> > +                               self.url, e)
> > +                continue
> 
> Hmm, I don't think we should retry on each and every URLError. For example
> if you have a typo in the server name, you get a "Name or service not known"
> URLError, and it does not make sense to retry in that case.
> 
> Also, in case of the server being down, it takes a minute or two 'til the
> urllib gives up, so if you retry multiple times in that case, you can easily
> extend the test time by > 5 minutes (as you can see by the timestamps in the
> example in your cover letter), which is unfortunate, too (considering that
> there is also a timeout setting for the gitlab CI jobs).
> 
> I think I'd maybe rather do something like this instead, without retrying
> the download, just marking certain errors as transient:
> 
>        except URLError as e:
>            tmp_cache_file.unlink()
>            msg = str(e.reason)
>            self.log.error("Unable to download %s: URL error %s",
>                           self.url, msg)
>            raise AssetError(self, msg,
>                             transient=("Network is unreachable" in msg))
> 
> WDYT?

Ok, yes, we can do that instead.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|