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

[Thomas Huth <thuth@redhat.com>]

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" ?

  Thomas