Re: [PATCH v4 15/35] tests/functional: enable pre-emptive caching of assets

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Aug 27, 2024 at 04:24:59PM +0200, Thomas Huth wrote:
> On 27/08/2024 15.16, Thomas Huth wrote:
> > On 23/08/2024 09.28, Philippe Mathieu-Daudé wrote:
> > > Hi,
> > > 
> > > On 21/8/24 10:27, Thomas Huth wrote:
> > > > From: Daniel P. Berrangé <berrange@redhat.com>
> > > > 
> > > > Many tests need to access assets stored on remote sites. We don't want
> > > > to download these during test execution when run by meson, since this
> > > > risks hitting test timeouts when data transfers are slow.
> > > > 
> > > > Add support for pre-emptive caching of assets by setting the env var
> > > > QEMU_TEST_PRECACHE to point to a timestamp file. When this is set,
> > > > instead of running the test, the assets will be downloaded and saved
> > > > to the cache, then the timestamp file created.
> > > > 
> > > > A meson custom target is created as a dependency of each test suite
> > > > to trigger the pre-emptive caching logic before the test runs.
> > > > 
> > > > When run in caching mode, it will locate assets by looking for class
> > > > level variables with a name prefix "ASSET_", and type "Asset".
> > > > 
> > > > At the ninja level
> > > > 
> > > >     ninja test --suite functional
> > > > 
> > > > will speculatively download any assets that are not already cached,
> > > > so it is advisable to set a timeout multiplier.
> > > > 
> > > >     QEMU_TEST_NO_DOWNLOAD=1 ninja test --suite functional
> > > > 
> > > > will fail the test if a required asset is not already cached
> > > > 
> > > >     ninja precache-functional
> > > > 
> > > > will download and cache all assets required by the functional
> > > > tests
> > > > 
> > > > At the make level, precaching is always done by
> > > > 
> > > >     make check-functional
> > > > 
> > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > Tested-by: Richard Henderson <richard.henderson@linaro.org>
> > > > [thuth: Remove the duplicated "path = os.path.basename(...)" line]
> > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > > ---
> > > >   tests/Makefile.include                 |  3 ++-
> > > >   tests/functional/meson.build           | 33 +++++++++++++++++++++++--
> > > >   tests/functional/qemu_test/asset.py    | 34 ++++++++++++++++++++++++++
> > > >   tests/functional/qemu_test/testcase.py |  7 ++++++
> > > >   4 files changed, 74 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/tests/Makefile.include b/tests/Makefile.include
> > > > index 66c8cc3123..010369bd3a 100644
> > > > --- a/tests/Makefile.include
> > > > +++ b/tests/Makefile.include
> > > > @@ -161,7 +161,8 @@ $(FUNCTIONAL_TARGETS):
> > > >   .PHONY: check-functional
> > > >   check-functional:
> > > > -    @$(MAKE) SPEED=thorough check-func check-func-quick
> > > > +    @$(NINJA) precache-functional
> > > > +    @QEMU_TEST_NO_DOWNLOAD=1 $(MAKE) SPEED=thorough check-func
> > > > check-func-quick
> > > >   # Consolidated targets
> > > > diff --git a/tests/functional/meson.build b/tests/functional/meson.build
> > > > index 8a8fa0ab99..cef74b82a9 100644
> > > > --- a/tests/functional/meson.build
> > > > +++ b/tests/functional/meson.build
> > > > @@ -33,6 +33,7 @@ tests_x86_64_quick = [
> > > >   tests_x86_64_thorough = [
> > > >   ]
> > > > +precache_all = []
> > > >   foreach speed : ['quick', 'thorough']
> > > >     foreach dir : target_dirs
> > > >       if not dir.endswith('-softmmu')
> > > > @@ -63,11 +64,35 @@ foreach speed : ['quick', 'thorough']
> > > >                                  meson.current_source_dir())
> > > >       foreach test : target_tests
> > > > -      test('func-@0@/@1@'.format(target_base, test),
> > > > +      testname = '@0@-@1@'.format(target_base, test)
> > > > +      testfile = 'test_' + test + '.py'
> > > > +      testpath = meson.current_source_dir() / testfile
> > > > +      teststamp = testname + '.tstamp'
> > > > +      test_precache_env = environment()
> > > > +      test_precache_env.set('QEMU_TEST_PRECACHE',
> > > > meson.current_build_dir() / teststamp)
> > > > +      test_precache_env.set('PYTHONPATH',
> > > > meson.project_source_root() / 'python:' +
> > > > +                                          meson.current_source_dir())
> > > > +      precache = custom_target('func-precache-' + testname,
> > > > +                               output: teststamp,
> > > > +                               command: [python, testpath],
> > > > +                               depend_files: files(testpath),
> > > > +                               build_by_default: false,
> > > > +                               env: test_precache_env)
> > > > +      precache_all += precache
> > > > +
> > > > +      # Ideally we would add 'precache' to 'depends' here, such that
> > > > +      # 'build_by_default: false' lets the pre-caching automatically
> > > > +      # run immediately before the test runs. In practice this is
> > > > +      # broken in meson, with it running the pre-caching in the normal
> > > > +      # compile phase https://github.com/mesonbuild/meson/issues/2518
> > > > +      # If the above bug ever gets fixed, when QEMU changes the min
> > > > +      # meson version, add the 'depends' and remove the custom
> > > > +      # 'run_target' logic below & in Makefile.include
> > > > +      test('func-' + testname,
> > > >              python,
> > > >              depends: [test_deps, test_emulator, emulator_modules],
> > > >              env: test_env,
> > > > -           args: [meson.current_source_dir() / 'test_' + test + '.py'],
> > > > +           args: [testpath],
> > > >              protocol: 'tap',
> > > >              timeout: test_timeouts.get(test, 60),
> > > >              priority: test_timeouts.get(test, 60),
> > > > @@ -75,3 +100,7 @@ foreach speed : ['quick', 'thorough']
> > > >       endforeach
> > > >     endforeach
> > > >   endforeach
> > > > +
> > > > +run_target('precache-functional',
> > > > +           depends: precache_all,
> > > > +           command: ['true'])
> > > > diff --git a/tests/functional/qemu_test/asset.py
> > > > b/tests/functional/qemu_test/asset.py
> > > > index cbeb6278af..9250ff2b06 100644
> > > > --- a/tests/functional/qemu_test/asset.py
> > > > +++ b/tests/functional/qemu_test/asset.py
> > > > @@ -9,6 +9,8 @@
> > > >   import logging
> > > >   import os
> > > >   import subprocess
> > > > +import sys
> > > > +import unittest
> > > >   import urllib.request
> > > >   from pathlib import Path
> > > >   from shutil import copyfileobj
> > > > @@ -62,6 +64,9 @@ def fetch(self):
> > > >                              self.cache_file, self.url)
> > > >               return str(self.cache_file)
> > > > +        if os.environ.get("QEMU_TEST_NO_DOWNLOAD", False):
> > > > +            raise Exception("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")
> > > > @@ -95,3 +100,32 @@ def fetch(self):
> > > >           self.log.info("Cached %s at %s" % (self.url, self.cache_file))
> > > >           return str(self.cache_file)
> > > > +
> > > > +    def precache_test(test):
> > > > +        log = logging.getLogger('qemu-test')
> > > > +        log.setLevel(logging.DEBUG)
> > > > +        handler = logging.StreamHandler(sys.stdout)
> > > > +        handler.setLevel(logging.DEBUG)
> > > > +        formatter = logging.Formatter(
> > > > +            '%(asctime)s - %(name)s - %(levelname)s - %(message)s')
> > > > +        handler.setFormatter(formatter)
> > > > +        log.addHandler(handler)
> > > > +        for name, asset in vars(test.__class__).items():
> > > > +            if name.startswith("ASSET_") and type(asset) == Asset:
> > > > +                log.info("Attempting to cache '%s'" % asset)
> > > > +                asset.fetch()
> > > > +        log.removeHandler(handler)
> > > > +
> > > > +    def precache_suite(suite):
> > > > +        for test in suite:
> > > > +            if isinstance(test, unittest.TestSuite):
> > > > +                Asset.precache_suite(test)
> > > > +            elif isinstance(test, unittest.TestCase):
> > > > +                Asset.precache_test(test)
> > > > +
> > > > +    def precache_suites(path, cacheTstamp):
> > > > +        loader = unittest.loader.defaultTestLoader
> > > > +        tests = loader.loadTestsFromNames([path], None)
> > > > +
> > > > +        with open(cacheTstamp, "w") as fh:
> > > > +            Asset.precache_suite(tests)
> > > 
> > > 
> > > When using multiple jobs (-jN) I'm observing some hangs,
> > > apparently multiple threads trying to download the same file.
> > > The files are eventually downloaded successfully but it takes
> > > longer. Should we acquire some exclusive lock somewhere?
> > 
> > I haven't seen that yet ... what did you exactly run? "make
> > check-functional -jN" ? Or "make check-functional-<target> -jN" ?
> 
> After applying some of your patches, I think I've run now into this problem,
> too: It's because test_aarch64_sbsaref.py and test_aarch64_virt.py try to
> download the same asset in parallel (alpine-standard-3.17.2-aarch64.iso).
> 
> Daniel, any ideas how to fix this in the Asset code?

So when downloading we open a file with a ".download" suffix, write to
that, and then rename it to the final filename.

If we have concurrent usage, both will open the same file and try to
write to it. Assuming both are downloading the same content we would
probably "get lucky" and have a consistent file at the end, but clearly
it is bad to rely on luck.

The lame option is to use NamedTemporaryFile for the teporary file.
This ensures both processes will write to different temp files, and
the final rename is atomic. This guarantees safety, but still has
the double download penalty.

The serious option is to use fcntl.lockf(..., fcntl.LOCK_EX) on the
temp file. If we can't acquire the lock then just immediately close
the temp file (don't delete it) and assume another thread is going to
finish its download.

On windows  we'll need msvcrt.locking(..., msvcrt.LK_WLCK, ...)
instead of fcntl.

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 :|