All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/8] iotests: make meson aware of individual I/O tests
@ 2023-03-03 16:07 Daniel P. Berrangé
  2023-03-03 16:07 ` [PATCH v2 1/8] iotests: explicitly pass source/build dir to 'check' command Daniel P. Berrangé
                   ` (10 more replies)
  0 siblings, 11 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2023-03-03 16:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Kevin Wolf, qemu-block, Thomas Huth, Hanna Reitz,
	Alex Bennée, Daniel P. Berrangé

To just repeat the patch 5 description...

Currently meson registers a single test that invokes an entire group of
I/O tests, hiding the test granularity from meson. There are various
downsides of doing this

 * You cannot ask 'meson test' to invoke a single I/O test
 * The meson test timeout can't be applied to the individual
   tests
 * Meson only gets a pass/fail for the overall I/O test group
   not individual tests
 * Meson can't show the time of individual I/O tests, so we
   can't see why 4-5 are consuming the bulk of the time
   and ripe for optimization
 * If a CI job gets killed by the GitLab timeout, we don't
   get visibility into how far through the I/O tests
   execution got.

This is not really specific to the I/O tests, the problem is common
to any case of us running a test which is in fact another test
harness which runs many tests. It would be nice to have meson have
the full view of all tests run. Adapting the I/O tests is as easy
win in this respect.

This switches meson to perform test discovery by invoking 'check' in
dry-run mode. It then registers one meson test case for each I/O
test. Parallel execution remains disabled since the I/O tests do not
use self contained execution environments and thus conflict with
each other.

Compare contrast output from a current job:

  https://gitlab.com/qemu-project/qemu/-/jobs/3863603546

[quote]
204/224 qemu:block / qemu-iotests qcow2   OK 329.94s   119 subtests passed
[/quote]

Vs what is seen with this series:

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

[quote]
204/350 qemu:block / qemu-iotests-qcow2-001   OK    2.16s   1 subtests passed
205/350 qemu:block / qemu-iotests-qcow2-002   OK    2.77s   1 subtests passed

...snip...

329/350 qemu:block / qemu-iotests-qcow2-qemu-img-close-errors       OK    6.19s   1 subtests passed
330/350 qemu:block / qemu-iotests-qcow2-qsd-jobs          OK    0.55s   1 subtests passed
[/quote]

A few tweaks were needed to the iotests runner because it had a few
assumptions about it always running in a tree that has already been
built, which is obviously not the case at the time meson does test
discovery.

In v2:

New example pipeline job

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

 * Set build/source dir defaults in CLI option parser
   instead of testenv.py (Alex)
 * Fix messed up termios settings with parallel execution
   by connecting stdin to /dev/null (Thomas)
 * Remove the obsolete check-block.sh script (Thomas)
 * Use a unique sub-directory per test to allow parallelization (Thomas)
 * Enable parallel execution by meson (Thomas)
 * Remove leftover debugging message (Thomas)
 * Use a shorter meson test name 'io-qcow2-012' instead of
   'qemu-iotests-qcow2-012'

Daniel P. Berrangé (8):
  iotests: explicitly pass source/build dir to 'check' command
  iotests: allow test discovery before building
  iotests: strip subdir path when listing tests
  iotests: print TAP protocol version when reporting tests
  iotests: connect stdin to /dev/null when running tests
  iotests: always use a unique sub-directory per test
  iotests: register each I/O test separately with meson
  iotests: remove the check-block.sh script

 tests/check-block.sh             | 43 --------------------------------
 tests/qemu-iotests/check         | 30 +++++++++++++++++++---
 tests/qemu-iotests/meson.build   | 35 +++++++++++++++++++++-----
 tests/qemu-iotests/testenv.py    | 20 +++++++--------
 tests/qemu-iotests/testrunner.py | 43 ++++++++++----------------------
 5 files changed, 78 insertions(+), 93 deletions(-)
 delete mode 100755 tests/check-block.sh

-- 
2.39.2



^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v2 1/8] iotests: explicitly pass source/build dir to 'check' command
  2023-03-03 16:07 [PATCH v2 0/8] iotests: make meson aware of individual I/O tests Daniel P. Berrangé
@ 2023-03-03 16:07 ` Daniel P. Berrangé
  2023-03-03 16:53   ` Thomas Huth
  2023-03-03 16:07 ` [PATCH v2 2/8] iotests: allow test discovery before building Daniel P. Berrangé
                   ` (9 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2023-03-03 16:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Kevin Wolf, qemu-block, Thomas Huth, Hanna Reitz,
	Alex Bennée, Daniel P. Berrangé

The 'check' script has some rather dubious logic whereby it assumes
that if invoked as a symlink, then it is running from a separate
source tree and build tree, otherwise it assumes the current working
directory is a combined source and build tree.

This doesn't work if you want to invoke the 'check' script using
its full source tree path while still using a split source and build
tree layout. This would be a typical situation with meson if you ask
it to find the 'check' script path using files('check').

Rather than trying to make the logic more magical, add support for
explicitly passing the dirs using --source-dir and --build-dir. If
either is omitted the current logic is maintained.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/check      | 25 +++++++++++++++++++++++--
 tests/qemu-iotests/testenv.py | 13 ++++---------
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 9bdda1394e..da7e8a87fe 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -26,9 +26,23 @@ from findtests import TestFinder
 from testenv import TestEnv
 from testrunner import TestRunner
 
+def get_default_path(follow_link=False):
+    """
+    Try to automagically figure out the path we are running from.
+    """
+    # called from the build tree?
+    if os.path.islink(sys.argv[0]):
+        if follow_link:
+            return os.path.dirname(os.readlink(sys.argv[0]))
+        else:
+            return os.path.dirname(os.path.abspath(sys.argv[0]))
+    else:  # or source tree?
+        return os.getcwd()
 
 def make_argparser() -> argparse.ArgumentParser:
-    p = argparse.ArgumentParser(description="Test run options")
+    p = argparse.ArgumentParser(
+        description="Test run options",
+        formatter_class=argparse.ArgumentDefaultsHelpFormatter)
 
     p.add_argument('-n', '--dry-run', action='store_true',
                    help='show me, do not run tests')
@@ -113,6 +127,11 @@ def make_argparser() -> argparse.ArgumentParser:
                        'middle of the process.')
     g_sel.add_argument('tests', metavar='TEST_FILES', nargs='*',
                        help='tests to run, or "--" followed by a command')
+    g_sel.add_argument('--build-dir', default=get_default_path(),
+                       help='Path to iotests build directory')
+    g_sel.add_argument('--source-dir',
+                       default=get_default_path(follow_link=True),
+                       help='Path to iotests build directory')
 
     return p
 
@@ -120,7 +139,9 @@ def make_argparser() -> argparse.ArgumentParser:
 if __name__ == '__main__':
     args = make_argparser().parse_args()
 
-    env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto,
+    env = TestEnv(source_dir=args.source_dir,
+                  build_dir=args.build_dir,
+                  imgfmt=args.imgfmt, imgproto=args.imgproto,
                   aiomode=args.aiomode, cachemode=args.cachemode,
                   imgopts=args.imgopts, misalign=args.misalign,
                   debug=args.debug, valgrind=args.valgrind,
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index a864c74b12..aa9d735414 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -170,7 +170,8 @@ def root(*names: str) -> str:
             if not isxfile(b):
                 sys.exit('Not executable: ' + b)
 
-    def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
+    def __init__(self, source_dir: str, build_dir: str,
+                 imgfmt: str, imgproto: str, aiomode: str,
                  cachemode: Optional[str] = None,
                  imgopts: Optional[str] = None,
                  misalign: bool = False,
@@ -211,14 +212,8 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
         # which are needed to initialize some environment variables. They are
         # used by init_*() functions as well.
 
-        if os.path.islink(sys.argv[0]):
-            # called from the build tree
-            self.source_iotests = os.path.dirname(os.readlink(sys.argv[0]))
-            self.build_iotests = os.path.dirname(os.path.abspath(sys.argv[0]))
-        else:
-            # called from the source tree
-            self.source_iotests = os.getcwd()
-            self.build_iotests = self.source_iotests
+        self.source_iotests = source_dir
+        self.build_iotests = build_dir
 
         self.build_root = os.path.join(self.build_iotests, '..', '..')
 
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 2/8] iotests: allow test discovery before building
  2023-03-03 16:07 [PATCH v2 0/8] iotests: make meson aware of individual I/O tests Daniel P. Berrangé
  2023-03-03 16:07 ` [PATCH v2 1/8] iotests: explicitly pass source/build dir to 'check' command Daniel P. Berrangé
@ 2023-03-03 16:07 ` Daniel P. Berrangé
  2023-03-03 16:07 ` [PATCH v2 3/8] iotests: strip subdir path when listing tests Daniel P. Berrangé
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2023-03-03 16:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Kevin Wolf, qemu-block, Thomas Huth, Hanna Reitz,
	Alex Bennée, Daniel P. Berrangé

The 'check' script can be invoked in "dry run" mode, in which case it
merely does test discovery and prints out all their names. Despite only
doing test discovery it still validates that the various QEMU binaries
can be found. This makes it impossible todo test discovery prior to
building QEMU. This is a desirable feature to support, because it will
let meson discover tests.

Fortunately the code in the TestEnv constructor is ordered in a way
that makes this fairly trivial to achieve. We can just short circuit
the constructor after the basic directory paths have been set.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/check      | 3 ++-
 tests/qemu-iotests/testenv.py | 7 ++++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index da7e8a87fe..bb294ef556 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -145,7 +145,8 @@ if __name__ == '__main__':
                   aiomode=args.aiomode, cachemode=args.cachemode,
                   imgopts=args.imgopts, misalign=args.misalign,
                   debug=args.debug, valgrind=args.valgrind,
-                  gdb=args.gdb, qprint=args.print)
+                  gdb=args.gdb, qprint=args.print,
+                  dry_run=args.dry_run)
 
     if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--':
         if not args.tests:
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index aa9d735414..9a37ad9152 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -178,7 +178,8 @@ def __init__(self, source_dir: str, build_dir: str,
                  debug: bool = False,
                  valgrind: bool = False,
                  gdb: bool = False,
-                 qprint: bool = False) -> None:
+                 qprint: bool = False,
+                 dry_run: bool = False) -> None:
         self.imgfmt = imgfmt
         self.imgproto = imgproto
         self.aiomode = aiomode
@@ -218,6 +219,10 @@ def __init__(self, source_dir: str, build_dir: str,
         self.build_root = os.path.join(self.build_iotests, '..', '..')
 
         self.init_directories()
+
+        if dry_run:
+            return
+
         self.init_binaries()
 
         self.malloc_perturb_ = os.getenv('MALLOC_PERTURB_',
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 3/8] iotests: strip subdir path when listing tests
  2023-03-03 16:07 [PATCH v2 0/8] iotests: make meson aware of individual I/O tests Daniel P. Berrangé
  2023-03-03 16:07 ` [PATCH v2 1/8] iotests: explicitly pass source/build dir to 'check' command Daniel P. Berrangé
  2023-03-03 16:07 ` [PATCH v2 2/8] iotests: allow test discovery before building Daniel P. Berrangé
@ 2023-03-03 16:07 ` Daniel P. Berrangé
  2023-03-03 16:07 ` [PATCH v2 4/8] iotests: print TAP protocol version when reporting tests Daniel P. Berrangé
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2023-03-03 16:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Kevin Wolf, qemu-block, Thomas Huth, Hanna Reitz,
	Alex Bennée, Daniel P. Berrangé

When asking 'check' to list individual tests by invoking it in dry run
mode, it prints the paths to the tests relative to the base of the
I/O test directory.

When asking 'check' to run an individual test, however, it mandates that
only the unqualified test name is given, without any path prefix. This
inconsistency makes it harder to ask for a list of tests and then invoke
each one.

Thus the test listing code is change to flatten the test names, by
printing only the base name, which can be directly invoked.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/check | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index bb294ef556..f2e9d27dcf 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -184,7 +184,7 @@ if __name__ == '__main__':
         sys.exit(str(e))
 
     if args.dry_run:
-        print('\n'.join(tests))
+        print('\n'.join([os.path.basename(t) for t in tests]))
     else:
         with TestRunner(env, tap=args.tap,
                         color=args.color) as tr:
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 4/8] iotests: print TAP protocol version when reporting tests
  2023-03-03 16:07 [PATCH v2 0/8] iotests: make meson aware of individual I/O tests Daniel P. Berrangé
                   ` (2 preceding siblings ...)
  2023-03-03 16:07 ` [PATCH v2 3/8] iotests: strip subdir path when listing tests Daniel P. Berrangé
@ 2023-03-03 16:07 ` Daniel P. Berrangé
  2023-03-03 16:07 ` [PATCH v2 5/8] iotests: connect stdin to /dev/null when running tests Daniel P. Berrangé
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2023-03-03 16:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Kevin Wolf, qemu-block, Thomas Huth, Hanna Reitz,
	Alex Bennée, Daniel P. Berrangé

Recently meson started complaining that TAP test reports don't include
the TAP protocol version. While this warning is bogus and has since been
removed from Meson, it looks like good practice to include this header
going forward. The GLib library test harness has started unconditionally
printing the version, so this brings the I/O tests into line.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/testrunner.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 5a771da86e..e734800b3d 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -391,6 +391,7 @@ def run_tests(self, tests: List[str], jobs: int = 1) -> bool:
         casenotrun = []
 
         if self.tap:
+            print('TAP version 13')
             self.env.print_env('# ')
             print('1..%d' % len(tests))
         else:
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 5/8] iotests: connect stdin to /dev/null when running tests
  2023-03-03 16:07 [PATCH v2 0/8] iotests: make meson aware of individual I/O tests Daniel P. Berrangé
                   ` (3 preceding siblings ...)
  2023-03-03 16:07 ` [PATCH v2 4/8] iotests: print TAP protocol version when reporting tests Daniel P. Berrangé
@ 2023-03-03 16:07 ` Daniel P. Berrangé
  2023-03-03 16:37   ` Thomas Huth
  2023-03-03 16:07 ` [PATCH v2 6/8] iotests: always use a unique sub-directory per test Daniel P. Berrangé
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2023-03-03 16:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Kevin Wolf, qemu-block, Thomas Huth, Hanna Reitz,
	Alex Bennée, Daniel P. Berrangé

Currently the tests have their stdin inherited from the test harness,
meaning they are connected to a TTY. The QEMU processes spawned by
certain tests, however, modify TTY settings and if the test exits
abnormally the settings might not be restored.

The python test harness thus has some logic which will capture the
initial TTY settings and restore them once all tests are finished.

This does not, however, take into account the possibility of many
copies of the 'check' program running in parallel. With parallel
execution, a later invokation may save the TTY state that QEMU has
already modified, and thus restore bad state leaving the TTY
non-functional.

None of the I/O tests shnould actually be interactive requiring
user input and so they should not require a TTY at all. To avoid
this while TTY save/restore complexity we can connect the test
stdin to /dev/null instead.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/testrunner.py | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index e734800b3d..81519ed6e2 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -24,12 +24,10 @@
 import subprocess
 import contextlib
 import json
-import termios
 import shutil
 import sys
 from multiprocessing import Pool
-from contextlib import contextmanager
-from typing import List, Optional, Iterator, Any, Sequence, Dict, \
+from typing import List, Optional, Any, Sequence, Dict, \
         ContextManager
 
 from testenv import TestEnv
@@ -56,22 +54,6 @@ def file_diff(file1: str, file2: str) -> List[str]:
         return res
 
 
-# We want to save current tty settings during test run,
-# since an aborting qemu call may leave things screwed up.
-@contextmanager
-def savetty() -> Iterator[None]:
-    isterm = sys.stdin.isatty()
-    if isterm:
-        fd = sys.stdin.fileno()
-        attr = termios.tcgetattr(fd)
-
-    try:
-        yield
-    finally:
-        if isterm:
-            termios.tcsetattr(fd, termios.TCSADRAIN, attr)
-
-
 class LastElapsedTime(ContextManager['LastElapsedTime']):
     """ Cache for elapsed time for tests, to show it during new test run
 
@@ -169,7 +151,6 @@ def __enter__(self) -> 'TestRunner':
         self._stack = contextlib.ExitStack()
         self._stack.enter_context(self.env)
         self._stack.enter_context(self.last_elapsed)
-        self._stack.enter_context(savetty())
         return self
 
     def __exit__(self, exc_type: Any, exc_value: Any, traceback: Any) -> None:
@@ -294,6 +275,7 @@ def do_run_test(self, test: str, mp: bool) -> TestResult:
         t0 = time.time()
         with f_bad.open('w', encoding="utf-8") as f:
             with subprocess.Popen(args, cwd=str(f_test.parent), env=env,
+                                  stdin=subprocess.DEVNULL,
                                   stdout=f, stderr=subprocess.STDOUT) as proc:
                 try:
                     proc.wait()
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 6/8] iotests: always use a unique sub-directory per test
  2023-03-03 16:07 [PATCH v2 0/8] iotests: make meson aware of individual I/O tests Daniel P. Berrangé
                   ` (4 preceding siblings ...)
  2023-03-03 16:07 ` [PATCH v2 5/8] iotests: connect stdin to /dev/null when running tests Daniel P. Berrangé
@ 2023-03-03 16:07 ` Daniel P. Berrangé
  2023-03-03 16:39   ` Thomas Huth
  2023-05-19  2:20   ` Eric Blake
  2023-03-03 16:07 ` [PATCH v2 7/8] iotests: register each I/O test separately with meson Daniel P. Berrangé
                   ` (4 subsequent siblings)
  10 siblings, 2 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2023-03-03 16:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Kevin Wolf, qemu-block, Thomas Huth, Hanna Reitz,
	Alex Bennée, Daniel P. Berrangé

The current test runner is only safe against parallel execution within
a single instance of the 'check' process, and only if -j is given a
value greater than 2. This prevents running multiple copies of the
'check' process for different test scenarios.

This change switches the output / socket directories to always include
the test name, image format and image protocol. This should allow full
parallelism of all distinct test scenarios. eg running both qcow2 and
raw tests at the same time, or both file and nbd tests at the same
time.

It would be possible to allow for parallelism of the same test scenario
by including the pid, but that would potentially let many directories
accumulate over time on failures, so is not done.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/testrunner.py | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 81519ed6e2..7b322272e9 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -228,13 +228,11 @@ def find_reference(self, test: str) -> str:
 
         return f'{test}.out'
 
-    def do_run_test(self, test: str, mp: bool) -> TestResult:
+    def do_run_test(self, test: str) -> TestResult:
         """
         Run one test
 
         :param test: test file path
-        :param mp: if true, we are in a multiprocessing environment, use
-                   personal subdirectories for test run
 
         Note: this method may be called from subprocess, so it does not
         change ``self`` object in any way!
@@ -257,12 +255,14 @@ def do_run_test(self, test: str, mp: bool) -> TestResult:
 
         args = [str(f_test.resolve())]
         env = self.env.prepare_subprocess(args)
-        if mp:
-            # Split test directories, so that tests running in parallel don't
-            # break each other.
-            for d in ['TEST_DIR', 'SOCK_DIR']:
-                env[d] = os.path.join(env[d], f_test.name)
-                Path(env[d]).mkdir(parents=True, exist_ok=True)
+
+        # Split test directories, so that tests running in parallel don't
+        # break each other.
+        for d in ['TEST_DIR', 'SOCK_DIR']:
+            env[d] = os.path.join(
+                env[d],
+                f"{self.env.imgfmt}-{self.env.imgproto}-{f_test.name}")
+            Path(env[d]).mkdir(parents=True, exist_ok=True)
 
         test_dir = env['TEST_DIR']
         f_bad = Path(test_dir, f_test.name + '.out.bad')
@@ -347,7 +347,7 @@ def run_test(self, test: str,
             testname = os.path.basename(test)
             print(f'# running {self.env.imgfmt} {testname}')
 
-        res = self.do_run_test(test, mp)
+        res = self.do_run_test(test)
 
         end = datetime.datetime.now().strftime('%H:%M:%S')
         self.test_print_one_line(test=test,
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 7/8] iotests: register each I/O test separately with meson
  2023-03-03 16:07 [PATCH v2 0/8] iotests: make meson aware of individual I/O tests Daniel P. Berrangé
                   ` (5 preceding siblings ...)
  2023-03-03 16:07 ` [PATCH v2 6/8] iotests: always use a unique sub-directory per test Daniel P. Berrangé
@ 2023-03-03 16:07 ` Daniel P. Berrangé
  2023-03-03 16:45   ` Thomas Huth
  2023-03-29 10:47   ` Thomas Huth
  2023-03-03 16:07 ` [PATCH v2 8/8] iotests: remove the check-block.sh script Daniel P. Berrangé
                   ` (3 subsequent siblings)
  10 siblings, 2 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2023-03-03 16:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Kevin Wolf, qemu-block, Thomas Huth, Hanna Reitz,
	Alex Bennée, Daniel P. Berrangé

Currently meson registers a single test that invokes an entire group of
I/O tests, hiding the test granularity from meson. There are various
downsides of doing this

 * You cannot ask 'meson test' to invoke a single I/O test
 * The meson test timeout can't be applied to the individual
   tests
 * Meson only gets a pass/fail for the overall I/O test group
   not individual tests
 * If a CI job gets killed by the GitLab timeout, we don't
   get visibility into how far through the I/O tests
   execution got.

This switches meson to perform test discovery by invoking 'check' in
dry-run mode. It then registers one meson test case for each I/O
test. Parallel execution remains disabled since the I/O tests do not
use self contained execution environments and thus conflict with
each other.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/meson.build | 35 ++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
index 323a4acb6a..a162f683ef 100644
--- a/tests/qemu-iotests/meson.build
+++ b/tests/qemu-iotests/meson.build
@@ -32,16 +32,39 @@ foreach k, v : emulators
   endif
 endforeach
 
+qemu_iotests_check_cmd = files('check')
+
 foreach format, speed: qemu_iotests_formats
   if speed == 'quick'
     suites = 'block'
   else
     suites = ['block-' + speed, speed]
   endif
-  test('qemu-iotests ' + format, sh, args: [files('../check-block.sh'), format],
-       depends: qemu_iotests_binaries, env: qemu_iotests_env,
-       protocol: 'tap',
-       suite: suites,
-       timeout: 0,
-       is_parallel: false)
+
+  args = ['-tap', '-' + format]
+  if speed == 'quick'
+      args += ['-g', 'auto']
+  endif
+
+  rc = run_command(
+      [qemu_iotests_check_cmd] + args + ['-n'],
+      check: true,
+  )
+
+  foreach item: rc.stdout().strip().split()
+      args = ['-tap', '-' + format, item,
+              '--source-dir', meson.current_source_dir(),
+              '--build-dir', meson.current_build_dir()]
+      # Some individual tests take as long as 45 seconds
+      # Bump the timeout to 3 minutes for some headroom
+      # on slow machines to minimize spurious failures
+      test('io-' + format + '-' + item,
+           qemu_iotests_check_cmd,
+           args: args,
+           depends: qemu_iotests_binaries,
+           env: qemu_iotests_env,
+           protocol: 'tap',
+           timeout: 180,
+           suite: suites)
+  endforeach
 endforeach
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 8/8] iotests: remove the check-block.sh script
  2023-03-03 16:07 [PATCH v2 0/8] iotests: make meson aware of individual I/O tests Daniel P. Berrangé
                   ` (6 preceding siblings ...)
  2023-03-03 16:07 ` [PATCH v2 7/8] iotests: register each I/O test separately with meson Daniel P. Berrangé
@ 2023-03-03 16:07 ` Daniel P. Berrangé
  2023-03-03 16:50   ` Thomas Huth
  2023-03-03 17:10 ` [PATCH v2 0/8] iotests: make meson aware of individual I/O tests Thomas Huth
                   ` (2 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2023-03-03 16:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Kevin Wolf, qemu-block, Thomas Huth, Hanna Reitz,
	Alex Bennée, Daniel P. Berrangé

Now that meson directly invokes the individual I/O tests, the
check-block.sh wrapper script is no longer required.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/check-block.sh | 43 -------------------------------------------
 1 file changed, 43 deletions(-)
 delete mode 100755 tests/check-block.sh

diff --git a/tests/check-block.sh b/tests/check-block.sh
deleted file mode 100755
index 5de2c1ba0b..0000000000
--- a/tests/check-block.sh
+++ /dev/null
@@ -1,43 +0,0 @@
-#!/bin/sh
-
-if [ "$#" -eq 0 ]; then
-    echo "Usage: $0 fmt..." >&2
-    exit 99
-fi
-
-# Honor the SPEED environment variable, just like we do it for "meson test"
-format_list="$@"
-if [ "$SPEED" = "slow" ] || [ "$SPEED" = "thorough" ]; then
-    group=
-else
-    group="-g auto"
-fi
-
-skip() {
-    echo "1..0 #SKIP $*"
-    exit 0
-}
-
-if [ -z "$(find . -name 'qemu-system-*' -print)" ]; then
-    skip "No qemu-system binary available ==> Not running the qemu-iotests."
-fi
-
-cd tests/qemu-iotests
-
-# QEMU_CHECK_BLOCK_AUTO is used to disable some unstable sub-tests
-export QEMU_CHECK_BLOCK_AUTO=1
-export PYTHONUTF8=1
-# If make was called with -jN we want to call ./check with -j N. Extract the
-# flag from MAKEFLAGS, so that if it absent (or MAKEFLAGS is not defined), JOBS
-# would be an empty line otherwise JOBS is prepared string of flag with value:
-# "-j N"
-# Note, that the following works even if make was called with "-j N" or even
-# "--jobs N", as all these variants becomes simply "-jN" in MAKEFLAGS variable.
-JOBS=$(echo "$MAKEFLAGS" | sed -n 's/\(^\|.* \)-j\([0-9]\+\)\( .*\|$\)/-j \2/p')
-
-ret=0
-for fmt in $format_list ; do
-    ${PYTHON} ./check $JOBS -tap -$fmt $group || ret=1
-done
-
-exit $ret
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 5/8] iotests: connect stdin to /dev/null when running tests
  2023-03-03 16:07 ` [PATCH v2 5/8] iotests: connect stdin to /dev/null when running tests Daniel P. Berrangé
@ 2023-03-03 16:37   ` Thomas Huth
  0 siblings, 0 replies; 22+ messages in thread
From: Thomas Huth @ 2023-03-03 16:37 UTC (permalink / raw)
  To: Daniel P. Berrangé, qemu-devel
  Cc: Paolo Bonzini, Kevin Wolf, qemu-block, Hanna Reitz, Alex Bennée

On 03/03/2023 17.07, Daniel P. Berrangé wrote:
> Currently the tests have their stdin inherited from the test harness,
> meaning they are connected to a TTY. The QEMU processes spawned by
> certain tests, however, modify TTY settings and if the test exits
> abnormally the settings might not be restored.
> 
> The python test harness thus has some logic which will capture the
> initial TTY settings and restore them once all tests are finished.
> 
> This does not, however, take into account the possibility of many
> copies of the 'check' program running in parallel. With parallel
> execution, a later invokation may save the TTY state that QEMU has

s/invokation/invocation/

> already modified, and thus restore bad state leaving the TTY
> non-functional.
> 
> None of the I/O tests shnould actually be interactive requiring

s/shnould/should/

> user input and so they should not require a TTY at all. To avoid
> this while TTY save/restore complexity we can connect the test
> stdin to /dev/null instead.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qemu-iotests/testrunner.py | 22 ++--------------------
>   1 file changed, 2 insertions(+), 20 deletions(-)

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



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 6/8] iotests: always use a unique sub-directory per test
  2023-03-03 16:07 ` [PATCH v2 6/8] iotests: always use a unique sub-directory per test Daniel P. Berrangé
@ 2023-03-03 16:39   ` Thomas Huth
  2023-05-19  2:20   ` Eric Blake
  1 sibling, 0 replies; 22+ messages in thread
From: Thomas Huth @ 2023-03-03 16:39 UTC (permalink / raw)
  To: Daniel P. Berrangé, qemu-devel
  Cc: Paolo Bonzini, Kevin Wolf, qemu-block, Hanna Reitz, Alex Bennée

On 03/03/2023 17.07, Daniel P. Berrangé wrote:
> The current test runner is only safe against parallel execution within
> a single instance of the 'check' process, and only if -j is given a
> value greater than 2. This prevents running multiple copies of the
> 'check' process for different test scenarios.
> 
> This change switches the output / socket directories to always include
> the test name, image format and image protocol. This should allow full
> parallelism of all distinct test scenarios. eg running both qcow2 and
> raw tests at the same time, or both file and nbd tests at the same
> time.
> 
> It would be possible to allow for parallelism of the same test scenario
> by including the pid, but that would potentially let many directories
> accumulate over time on failures, so is not done.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qemu-iotests/testrunner.py | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)

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



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 7/8] iotests: register each I/O test separately with meson
  2023-03-03 16:07 ` [PATCH v2 7/8] iotests: register each I/O test separately with meson Daniel P. Berrangé
@ 2023-03-03 16:45   ` Thomas Huth
  2023-03-29 10:47   ` Thomas Huth
  1 sibling, 0 replies; 22+ messages in thread
From: Thomas Huth @ 2023-03-03 16:45 UTC (permalink / raw)
  To: Daniel P. Berrangé, qemu-devel
  Cc: Paolo Bonzini, Kevin Wolf, qemu-block, Hanna Reitz, Alex Bennée

On 03/03/2023 17.07, Daniel P. Berrangé wrote:
> Currently meson registers a single test that invokes an entire group of
> I/O tests, hiding the test granularity from meson. There are various
> downsides of doing this
> 
>   * You cannot ask 'meson test' to invoke a single I/O test
>   * The meson test timeout can't be applied to the individual
>     tests
>   * Meson only gets a pass/fail for the overall I/O test group
>     not individual tests
>   * If a CI job gets killed by the GitLab timeout, we don't
>     get visibility into how far through the I/O tests
>     execution got.
> 
> This switches meson to perform test discovery by invoking 'check' in
> dry-run mode. It then registers one meson test case for each I/O
> test. Parallel execution remains disabled since the I/O tests do not
> use self contained execution environments and thus conflict with
> each other.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qemu-iotests/meson.build | 35 ++++++++++++++++++++++++++++------
>   1 file changed, 29 insertions(+), 6 deletions(-)

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



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 8/8] iotests: remove the check-block.sh script
  2023-03-03 16:07 ` [PATCH v2 8/8] iotests: remove the check-block.sh script Daniel P. Berrangé
@ 2023-03-03 16:50   ` Thomas Huth
  0 siblings, 0 replies; 22+ messages in thread
From: Thomas Huth @ 2023-03-03 16:50 UTC (permalink / raw)
  To: Daniel P. Berrangé, qemu-devel
  Cc: Paolo Bonzini, Kevin Wolf, qemu-block, Hanna Reitz, Alex Bennée

On 03/03/2023 17.07, Daniel P. Berrangé wrote:
> Now that meson directly invokes the individual I/O tests, the
> check-block.sh wrapper script is no longer required.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/check-block.sh | 43 -------------------------------------------
>   1 file changed, 43 deletions(-)
>   delete mode 100755 tests/check-block.sh

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



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/8] iotests: explicitly pass source/build dir to 'check' command
  2023-03-03 16:07 ` [PATCH v2 1/8] iotests: explicitly pass source/build dir to 'check' command Daniel P. Berrangé
@ 2023-03-03 16:53   ` Thomas Huth
  0 siblings, 0 replies; 22+ messages in thread
From: Thomas Huth @ 2023-03-03 16:53 UTC (permalink / raw)
  To: Daniel P. Berrangé, qemu-devel
  Cc: Paolo Bonzini, Kevin Wolf, qemu-block, Hanna Reitz, Alex Bennée

On 03/03/2023 17.07, Daniel P. Berrangé wrote:
> The 'check' script has some rather dubious logic whereby it assumes
> that if invoked as a symlink, then it is running from a separate
> source tree and build tree, otherwise it assumes the current working
> directory is a combined source and build tree.
> 
> This doesn't work if you want to invoke the 'check' script using
> its full source tree path while still using a split source and build
> tree layout. This would be a typical situation with meson if you ask
> it to find the 'check' script path using files('check').
> 
> Rather than trying to make the logic more magical, add support for
> explicitly passing the dirs using --source-dir and --build-dir. If
> either is omitted the current logic is maintained.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qemu-iotests/check      | 25 +++++++++++++++++++++++--
>   tests/qemu-iotests/testenv.py | 13 ++++---------
>   2 files changed, 27 insertions(+), 11 deletions(-)

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




^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 0/8] iotests: make meson aware of individual I/O tests
  2023-03-03 16:07 [PATCH v2 0/8] iotests: make meson aware of individual I/O tests Daniel P. Berrangé
                   ` (7 preceding siblings ...)
  2023-03-03 16:07 ` [PATCH v2 8/8] iotests: remove the check-block.sh script Daniel P. Berrangé
@ 2023-03-03 17:10 ` Thomas Huth
  2023-03-10 16:17 ` Daniel P. Berrangé
  2023-03-14 13:54 ` Alex Bennée
  10 siblings, 0 replies; 22+ messages in thread
From: Thomas Huth @ 2023-03-03 17:10 UTC (permalink / raw)
  To: Daniel P. Berrangé, qemu-devel
  Cc: Paolo Bonzini, Kevin Wolf, qemu-block, Hanna Reitz, Alex Bennée

On 03/03/2023 17.07, Daniel P. Berrangé wrote:
> To just repeat the patch 5 description...
> 
> Currently meson registers a single test that invokes an entire group of
> I/O tests, hiding the test granularity from meson. There are various
> downsides of doing this
> 
>   * You cannot ask 'meson test' to invoke a single I/O test
>   * The meson test timeout can't be applied to the individual
>     tests
>   * Meson only gets a pass/fail for the overall I/O test group
>     not individual tests
>   * Meson can't show the time of individual I/O tests, so we
>     can't see why 4-5 are consuming the bulk of the time
>     and ripe for optimization
>   * If a CI job gets killed by the GitLab timeout, we don't
>     get visibility into how far through the I/O tests
>     execution got.
> 
> This is not really specific to the I/O tests, the problem is common
> to any case of us running a test which is in fact another test
> harness which runs many tests. It would be nice to have meson have
> the full view of all tests run. Adapting the I/O tests is as easy
> win in this respect.
> 
> This switches meson to perform test discovery by invoking 'check' in
> dry-run mode. It then registers one meson test case for each I/O
> test. Parallel execution remains disabled since the I/O tests do not
> use self contained execution environments and thus conflict with
> each other.

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



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 0/8] iotests: make meson aware of individual I/O tests
  2023-03-03 16:07 [PATCH v2 0/8] iotests: make meson aware of individual I/O tests Daniel P. Berrangé
                   ` (8 preceding siblings ...)
  2023-03-03 17:10 ` [PATCH v2 0/8] iotests: make meson aware of individual I/O tests Thomas Huth
@ 2023-03-10 16:17 ` Daniel P. Berrangé
  2023-03-10 17:05   ` Hanna Czenczek
  2023-03-14 13:54 ` Alex Bennée
  10 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2023-03-10 16:17 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Kevin Wolf, qemu-block, Thomas Huth, Hanna Reitz,
	Alex Bennée

Kevin / Hanna.....  do you have any comments you want to
make on this, since it is notionally under the maintainership
of the block team ? If not Alex has volunteered to queue this
via his testing tree.

On Fri, Mar 03, 2023 at 04:07:19PM +0000, Daniel P. Berrangé wrote:
> To just repeat the patch 5 description...
> 
> Currently meson registers a single test that invokes an entire group of
> I/O tests, hiding the test granularity from meson. There are various
> downsides of doing this
> 
>  * You cannot ask 'meson test' to invoke a single I/O test
>  * The meson test timeout can't be applied to the individual
>    tests
>  * Meson only gets a pass/fail for the overall I/O test group
>    not individual tests
>  * Meson can't show the time of individual I/O tests, so we
>    can't see why 4-5 are consuming the bulk of the time
>    and ripe for optimization
>  * If a CI job gets killed by the GitLab timeout, we don't
>    get visibility into how far through the I/O tests
>    execution got.
> 
> This is not really specific to the I/O tests, the problem is common
> to any case of us running a test which is in fact another test
> harness which runs many tests. It would be nice to have meson have
> the full view of all tests run. Adapting the I/O tests is as easy
> win in this respect.
> 
> This switches meson to perform test discovery by invoking 'check' in
> dry-run mode. It then registers one meson test case for each I/O
> test. Parallel execution remains disabled since the I/O tests do not
> use self contained execution environments and thus conflict with
> each other.
> 
> Compare contrast output from a current job:
> 
>   https://gitlab.com/qemu-project/qemu/-/jobs/3863603546
> 
> [quote]
> 204/224 qemu:block / qemu-iotests qcow2   OK 329.94s   119 subtests passed
> [/quote]
> 
> Vs what is seen with this series:
> 
>   https://gitlab.com/berrange/qemu/-/jobs/3865975463
> 
> [quote]
> 204/350 qemu:block / qemu-iotests-qcow2-001   OK    2.16s   1 subtests passed
> 205/350 qemu:block / qemu-iotests-qcow2-002   OK    2.77s   1 subtests passed
> 
> ...snip...
> 
> 329/350 qemu:block / qemu-iotests-qcow2-qemu-img-close-errors       OK    6.19s   1 subtests passed
> 330/350 qemu:block / qemu-iotests-qcow2-qsd-jobs          OK    0.55s   1 subtests passed
> [/quote]
> 
> A few tweaks were needed to the iotests runner because it had a few
> assumptions about it always running in a tree that has already been
> built, which is obviously not the case at the time meson does test
> discovery.
> 
> In v2:
> 
> New example pipeline job
> 
>    https://gitlab.com/berrange/qemu/-/jobs/3871446106
> 
>  * Set build/source dir defaults in CLI option parser
>    instead of testenv.py (Alex)
>  * Fix messed up termios settings with parallel execution
>    by connecting stdin to /dev/null (Thomas)
>  * Remove the obsolete check-block.sh script (Thomas)
>  * Use a unique sub-directory per test to allow parallelization (Thomas)
>  * Enable parallel execution by meson (Thomas)
>  * Remove leftover debugging message (Thomas)
>  * Use a shorter meson test name 'io-qcow2-012' instead of
>    'qemu-iotests-qcow2-012'
> 
> Daniel P. Berrangé (8):
>   iotests: explicitly pass source/build dir to 'check' command
>   iotests: allow test discovery before building
>   iotests: strip subdir path when listing tests
>   iotests: print TAP protocol version when reporting tests
>   iotests: connect stdin to /dev/null when running tests
>   iotests: always use a unique sub-directory per test
>   iotests: register each I/O test separately with meson
>   iotests: remove the check-block.sh script
> 
>  tests/check-block.sh             | 43 --------------------------------
>  tests/qemu-iotests/check         | 30 +++++++++++++++++++---
>  tests/qemu-iotests/meson.build   | 35 +++++++++++++++++++++-----
>  tests/qemu-iotests/testenv.py    | 20 +++++++--------
>  tests/qemu-iotests/testrunner.py | 43 ++++++++++----------------------
>  5 files changed, 78 insertions(+), 93 deletions(-)
>  delete mode 100755 tests/check-block.sh
> 
> -- 
> 2.39.2
> 

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



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 0/8] iotests: make meson aware of individual I/O tests
  2023-03-10 16:17 ` Daniel P. Berrangé
@ 2023-03-10 17:05   ` Hanna Czenczek
  0 siblings, 0 replies; 22+ messages in thread
From: Hanna Czenczek @ 2023-03-10 17:05 UTC (permalink / raw)
  To: Daniel P. Berrangé, qemu-devel
  Cc: Paolo Bonzini, Kevin Wolf, qemu-block, Thomas Huth, Alex Bennée

On 10.03.23 17:17, Daniel P. Berrangé wrote:
> Kevin / Hanna.....  do you have any comments you want to
> make on this, since it is notionally under the maintainership
> of the block team ? If not Alex has volunteered to queue this
> via his testing tree.

Looks good to me!

So, if it helps:

Acked-by: Hanna Czenczek <hreitz@redhat.com>



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 0/8] iotests: make meson aware of individual I/O tests
  2023-03-03 16:07 [PATCH v2 0/8] iotests: make meson aware of individual I/O tests Daniel P. Berrangé
                   ` (9 preceding siblings ...)
  2023-03-10 16:17 ` Daniel P. Berrangé
@ 2023-03-14 13:54 ` Alex Bennée
  10 siblings, 0 replies; 22+ messages in thread
From: Alex Bennée @ 2023-03-14 13:54 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: qemu-devel, Paolo Bonzini, Kevin Wolf, qemu-block, Thomas Huth,
	Hanna Reitz


Daniel P. Berrangé <berrange@redhat.com> writes:

> To just repeat the patch 5 description...
>
<snip>

Queued to for-8.0/tweaks-and-fixes, thanks.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 7/8] iotests: register each I/O test separately with meson
  2023-03-03 16:07 ` [PATCH v2 7/8] iotests: register each I/O test separately with meson Daniel P. Berrangé
  2023-03-03 16:45   ` Thomas Huth
@ 2023-03-29 10:47   ` Thomas Huth
  2023-03-29 11:18     ` Daniel P. Berrangé
  1 sibling, 1 reply; 22+ messages in thread
From: Thomas Huth @ 2023-03-29 10:47 UTC (permalink / raw)
  To: Daniel P. Berrangé, qemu-devel
  Cc: Paolo Bonzini, Kevin Wolf, qemu-block, Hanna Reitz, Alex Bennée

On 03/03/2023 17.07, Daniel P. Berrangé wrote:
> Currently meson registers a single test that invokes an entire group of
> I/O tests, hiding the test granularity from meson. There are various
> downsides of doing this
> 
>   * You cannot ask 'meson test' to invoke a single I/O test
>   * The meson test timeout can't be applied to the individual
>     tests
>   * Meson only gets a pass/fail for the overall I/O test group
>     not individual tests
>   * If a CI job gets killed by the GitLab timeout, we don't
>     get visibility into how far through the I/O tests
>     execution got.
> 
> This switches meson to perform test discovery by invoking 'check' in
> dry-run mode. It then registers one meson test case for each I/O
> test. Parallel execution remains disabled since the I/O tests do not
> use self contained execution environments and thus conflict with
> each other.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qemu-iotests/meson.build | 35 ++++++++++++++++++++++++++++------
>   1 file changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
> index 323a4acb6a..a162f683ef 100644
> --- a/tests/qemu-iotests/meson.build
> +++ b/tests/qemu-iotests/meson.build
> @@ -32,16 +32,39 @@ foreach k, v : emulators
>     endif
>   endforeach
>   
> +qemu_iotests_check_cmd = files('check')
> +
>   foreach format, speed: qemu_iotests_formats
>     if speed == 'quick'
>       suites = 'block'
>     else
>       suites = ['block-' + speed, speed]
>     endif
> -  test('qemu-iotests ' + format, sh, args: [files('../check-block.sh'), format],
> -       depends: qemu_iotests_binaries, env: qemu_iotests_env,
> -       protocol: 'tap',
> -       suite: suites,
> -       timeout: 0,
> -       is_parallel: false)
> +
> +  args = ['-tap', '-' + format]
> +  if speed == 'quick'
> +      args += ['-g', 'auto']
> +  endif
> +
> +  rc = run_command(
> +      [qemu_iotests_check_cmd] + args + ['-n'],
> +      check: true,
> +  )
> +
> +  foreach item: rc.stdout().strip().split()
> +      args = ['-tap', '-' + format, item,
> +              '--source-dir', meson.current_source_dir(),
> +              '--build-dir', meson.current_build_dir()]
> +      # Some individual tests take as long as 45 seconds
> +      # Bump the timeout to 3 minutes for some headroom
> +      # on slow machines to minimize spurious failures
> +      test('io-' + format + '-' + item,
> +           qemu_iotests_check_cmd,
> +           args: args,
> +           depends: qemu_iotests_binaries,
> +           env: qemu_iotests_env,
> +           protocol: 'tap',
> +           timeout: 180,
> +           suite: suites)
> +  endforeach
>   endforeach

Seems like this somehow broke compilation on NetBSD:

https://gitlab.com/thuth/qemu/-/jobs/4021584713#L2980

?

  Thomas



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 7/8] iotests: register each I/O test separately with meson
  2023-03-29 10:47   ` Thomas Huth
@ 2023-03-29 11:18     ` Daniel P. Berrangé
  2023-03-29 11:23       ` Thomas Huth
  0 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2023-03-29 11:18 UTC (permalink / raw)
  To: Thomas Huth
  Cc: qemu-devel, Paolo Bonzini, Kevin Wolf, qemu-block, Hanna Reitz,
	Alex Bennée

On Wed, Mar 29, 2023 at 12:47:51PM +0200, Thomas Huth wrote:
> On 03/03/2023 17.07, Daniel P. Berrangé wrote:
> > Currently meson registers a single test that invokes an entire group of
> > I/O tests, hiding the test granularity from meson. There are various
> > downsides of doing this
> > 
> >   * You cannot ask 'meson test' to invoke a single I/O test
> >   * The meson test timeout can't be applied to the individual
> >     tests
> >   * Meson only gets a pass/fail for the overall I/O test group
> >     not individual tests
> >   * If a CI job gets killed by the GitLab timeout, we don't
> >     get visibility into how far through the I/O tests
> >     execution got.
> > 
> > This switches meson to perform test discovery by invoking 'check' in
> > dry-run mode. It then registers one meson test case for each I/O
> > test. Parallel execution remains disabled since the I/O tests do not
> > use self contained execution environments and thus conflict with
> > each other.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   tests/qemu-iotests/meson.build | 35 ++++++++++++++++++++++++++++------
> >   1 file changed, 29 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
> > index 323a4acb6a..a162f683ef 100644
> > --- a/tests/qemu-iotests/meson.build
> > +++ b/tests/qemu-iotests/meson.build
> > @@ -32,16 +32,39 @@ foreach k, v : emulators
> >     endif
> >   endforeach
> > +qemu_iotests_check_cmd = files('check')
> > +
> >   foreach format, speed: qemu_iotests_formats
> >     if speed == 'quick'
> >       suites = 'block'
> >     else
> >       suites = ['block-' + speed, speed]
> >     endif
> > -  test('qemu-iotests ' + format, sh, args: [files('../check-block.sh'), format],
> > -       depends: qemu_iotests_binaries, env: qemu_iotests_env,
> > -       protocol: 'tap',
> > -       suite: suites,
> > -       timeout: 0,
> > -       is_parallel: false)
> > +
> > +  args = ['-tap', '-' + format]
> > +  if speed == 'quick'
> > +      args += ['-g', 'auto']
> > +  endif
> > +
> > +  rc = run_command(
> > +      [qemu_iotests_check_cmd] + args + ['-n'],
> > +      check: true,
> > +  )
> > +
> > +  foreach item: rc.stdout().strip().split()
> > +      args = ['-tap', '-' + format, item,
> > +              '--source-dir', meson.current_source_dir(),
> > +              '--build-dir', meson.current_build_dir()]
> > +      # Some individual tests take as long as 45 seconds
> > +      # Bump the timeout to 3 minutes for some headroom
> > +      # on slow machines to minimize spurious failures
> > +      test('io-' + format + '-' + item,
> > +           qemu_iotests_check_cmd,
> > +           args: args,
> > +           depends: qemu_iotests_binaries,
> > +           env: qemu_iotests_env,
> > +           protocol: 'tap',
> > +           timeout: 180,
> > +           suite: suites)
> > +  endforeach
> >   endforeach
> 
> Seems like this somehow broke compilation on NetBSD:
> 
> https://gitlab.com/thuth/qemu/-/jobs/4021584713#L2980

I ran it locally and got the meson-log.txt file which reports

  env: python3: No such file or directory

and indeed there is no python3 binary present in our netbsd
VM.

our tests/vm/netbsd script works around this by passing an
explicit --python=python3.7 arg to configure, but the way
we invoke the 'check' script means it is just using the
"#!/usr/bin/env python3"  logic 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 :|



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 7/8] iotests: register each I/O test separately with meson
  2023-03-29 11:18     ` Daniel P. Berrangé
@ 2023-03-29 11:23       ` Thomas Huth
  0 siblings, 0 replies; 22+ messages in thread
From: Thomas Huth @ 2023-03-29 11:23 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: qemu-devel, Paolo Bonzini, Kevin Wolf, qemu-block, Hanna Reitz,
	Alex Bennée

On 29/03/2023 13.18, Daniel P. Berrangé wrote:
> On Wed, Mar 29, 2023 at 12:47:51PM +0200, Thomas Huth wrote:
>> On 03/03/2023 17.07, Daniel P. Berrangé wrote:
>>> Currently meson registers a single test that invokes an entire group of
>>> I/O tests, hiding the test granularity from meson. There are various
>>> downsides of doing this
>>>
>>>    * You cannot ask 'meson test' to invoke a single I/O test
>>>    * The meson test timeout can't be applied to the individual
>>>      tests
>>>    * Meson only gets a pass/fail for the overall I/O test group
>>>      not individual tests
>>>    * If a CI job gets killed by the GitLab timeout, we don't
>>>      get visibility into how far through the I/O tests
>>>      execution got.
>>>
>>> This switches meson to perform test discovery by invoking 'check' in
>>> dry-run mode. It then registers one meson test case for each I/O
>>> test. Parallel execution remains disabled since the I/O tests do not
>>> use self contained execution environments and thus conflict with
>>> each other.
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>    tests/qemu-iotests/meson.build | 35 ++++++++++++++++++++++++++++------
>>>    1 file changed, 29 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
>>> index 323a4acb6a..a162f683ef 100644
>>> --- a/tests/qemu-iotests/meson.build
>>> +++ b/tests/qemu-iotests/meson.build
>>> @@ -32,16 +32,39 @@ foreach k, v : emulators
>>>      endif
>>>    endforeach
>>> +qemu_iotests_check_cmd = files('check')
>>> +
>>>    foreach format, speed: qemu_iotests_formats
>>>      if speed == 'quick'
>>>        suites = 'block'
>>>      else
>>>        suites = ['block-' + speed, speed]
>>>      endif
>>> -  test('qemu-iotests ' + format, sh, args: [files('../check-block.sh'), format],
>>> -       depends: qemu_iotests_binaries, env: qemu_iotests_env,
>>> -       protocol: 'tap',
>>> -       suite: suites,
>>> -       timeout: 0,
>>> -       is_parallel: false)
>>> +
>>> +  args = ['-tap', '-' + format]
>>> +  if speed == 'quick'
>>> +      args += ['-g', 'auto']
>>> +  endif
>>> +
>>> +  rc = run_command(
>>> +      [qemu_iotests_check_cmd] + args + ['-n'],
>>> +      check: true,
>>> +  )
>>> +
>>> +  foreach item: rc.stdout().strip().split()
>>> +      args = ['-tap', '-' + format, item,
>>> +              '--source-dir', meson.current_source_dir(),
>>> +              '--build-dir', meson.current_build_dir()]
>>> +      # Some individual tests take as long as 45 seconds
>>> +      # Bump the timeout to 3 minutes for some headroom
>>> +      # on slow machines to minimize spurious failures
>>> +      test('io-' + format + '-' + item,
>>> +           qemu_iotests_check_cmd,
>>> +           args: args,
>>> +           depends: qemu_iotests_binaries,
>>> +           env: qemu_iotests_env,
>>> +           protocol: 'tap',
>>> +           timeout: 180,
>>> +           suite: suites)
>>> +  endforeach
>>>    endforeach
>>
>> Seems like this somehow broke compilation on NetBSD:
>>
>> https://gitlab.com/thuth/qemu/-/jobs/4021584713#L2980
> 
> I ran it locally and got the meson-log.txt file which reports
> 
>    env: python3: No such file or directory
> 
> and indeed there is no python3 binary present in our netbsd
> VM.
> 
> our tests/vm/netbsd script works around this by passing an
> explicit --python=python3.7 arg to configure, but the way
> we invoke the 'check' script means it is just using the
> "#!/usr/bin/env python3"  logic instead.

Ah, that rings a bell - python scripts must not have the executable flags, 
otherwise meson tries to run them directly instead of using the interpreter 
that has been specified with the --python option.

  Thomas



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 6/8] iotests: always use a unique sub-directory per test
  2023-03-03 16:07 ` [PATCH v2 6/8] iotests: always use a unique sub-directory per test Daniel P. Berrangé
  2023-03-03 16:39   ` Thomas Huth
@ 2023-05-19  2:20   ` Eric Blake
  1 sibling, 0 replies; 22+ messages in thread
From: Eric Blake @ 2023-05-19  2:20 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: qemu-devel, Paolo Bonzini, Kevin Wolf, qemu-block, Thomas Huth,
	Hanna Reitz, Alex Bennée

On Fri, Mar 03, 2023 at 04:07:25PM +0000, Daniel P. Berrangé wrote:
> The current test runner is only safe against parallel execution within
> a single instance of the 'check' process, and only if -j is given a
> value greater than 2. This prevents running multiple copies of the
> 'check' process for different test scenarios.
> 
> This change switches the output / socket directories to always include
> the test name, image format and image protocol. This should allow full
> parallelism of all distinct test scenarios. eg running both qcow2 and
> raw tests at the same time, or both file and nbd tests at the same
> time.
> 
> It would be possible to allow for parallelism of the same test scenario
> by including the pid, but that would potentially let many directories
> accumulate over time on failures, so is not done.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  tests/qemu-iotests/testrunner.py | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)

git bisect points to this commit as being the reason behind the
following regression in iotests:

$ ./check -nbd 104
...
--- /home/eblake/qemu/tests/qemu-iotests/104.out
+++ /home/eblake/qemu/build/tests/qemu-iotests/scratch/raw-nbd-104/104.out.bad
@@ -2,11 +2,11 @@
 === Check qemu-img info output ===

 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1024
-image: TEST_DIR/t.IMGFMT
+image: nbd+unix://?socket=/tmp/tmpqathrkpn/IMGFMT-nbd-104/nbd
 file format: IMGFMT
 virtual size: 1 KiB (1024 bytes)
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1234
-image: TEST_DIR/t.IMGFMT
+image: nbd+unix://?socket=/tmp/tmpqathrkpn/IMGFMT-nbd-104/nbd
 file format: IMGFMT
 virtual size: 1.5 KiB (1536 bytes)
 *** done
Failures: 104

Back in 2015, commit a231cb2726, we added a hack that turned
nbd://... into TEST_DIR/t.IMGFMT to satisfy the output matching in the
various iotests, but with our new per-test directories, that filter is
no longer firing.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2023-05-19  2:21 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-03 16:07 [PATCH v2 0/8] iotests: make meson aware of individual I/O tests Daniel P. Berrangé
2023-03-03 16:07 ` [PATCH v2 1/8] iotests: explicitly pass source/build dir to 'check' command Daniel P. Berrangé
2023-03-03 16:53   ` Thomas Huth
2023-03-03 16:07 ` [PATCH v2 2/8] iotests: allow test discovery before building Daniel P. Berrangé
2023-03-03 16:07 ` [PATCH v2 3/8] iotests: strip subdir path when listing tests Daniel P. Berrangé
2023-03-03 16:07 ` [PATCH v2 4/8] iotests: print TAP protocol version when reporting tests Daniel P. Berrangé
2023-03-03 16:07 ` [PATCH v2 5/8] iotests: connect stdin to /dev/null when running tests Daniel P. Berrangé
2023-03-03 16:37   ` Thomas Huth
2023-03-03 16:07 ` [PATCH v2 6/8] iotests: always use a unique sub-directory per test Daniel P. Berrangé
2023-03-03 16:39   ` Thomas Huth
2023-05-19  2:20   ` Eric Blake
2023-03-03 16:07 ` [PATCH v2 7/8] iotests: register each I/O test separately with meson Daniel P. Berrangé
2023-03-03 16:45   ` Thomas Huth
2023-03-29 10:47   ` Thomas Huth
2023-03-29 11:18     ` Daniel P. Berrangé
2023-03-29 11:23       ` Thomas Huth
2023-03-03 16:07 ` [PATCH v2 8/8] iotests: remove the check-block.sh script Daniel P. Berrangé
2023-03-03 16:50   ` Thomas Huth
2023-03-03 17:10 ` [PATCH v2 0/8] iotests: make meson aware of individual I/O tests Thomas Huth
2023-03-10 16:17 ` Daniel P. Berrangé
2023-03-10 17:05   ` Hanna Czenczek
2023-03-14 13:54 ` Alex Bennée

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.