[Qemu-devel] [PATCH 0/3] Acceptance tests: boot Linux with KVM test

[Qemu-devel] [PATCH 0/3] Acceptance tests: boot Linux with KVM test

[Wainer dos Santos Moschetta]

Hi all!

This series introduce a simple test case which boots Linux with
KVM enabled aiming to exercise qemu-kvm integration. No other
acceptance test does that as far as I know.

The patch 02 allows the test writter to mark (by means of Avocado tag)
the test require a given accelerator that if not present will end up
on skipping its run. It uses `qemu -accel help` to determine if accel
is enabled in binary and in some cases (e.g. kvm) it can check the host
support too. Because `qemu -accel help` needs to execute without the
qmp machinery, the patch 01 is needed. The patch 01 allows to boot qemu
with no qmp which can also be used on LinuxInitrd test for instance.

Depends on '[PATCH 1/2] Acceptance tests: exclude "flaky" tests'
which bumps Avocado version to 69.1.

Conflicts with '[RFC PATCH v2 1/3] python/qemu: split QEMUMachine out
from underneath __init__.py'. Upon merge of John's patch I can then
rebase this.

Git: https://github.com/wainersm/qemu
Branch: acceptance_kvm_test 
Travis: https://travis-ci.org/wainersm/qemu/jobs/551455876

Wainer dos Santos Moschetta (3):
  python/qemu: Allow to launch the VM without qmp
  tests/acceptance: Introduce the "accel" tag
  tests/acceptance: Add boot linux with kvm test

 python/qemu/__init__.py                   | 72 +++++++++++++++--------
 tests/acceptance/avocado_qemu/__init__.py |  5 ++
 tests/acceptance/avocado_qemu/accel.py    | 60 +++++++++++++++++++
 tests/acceptance/kvm.py                   | 58 ++++++++++++++++++
 4 files changed, 171 insertions(+), 24 deletions(-)
 create mode 100644 tests/acceptance/avocado_qemu/accel.py
 create mode 100644 tests/acceptance/kvm.py

-- 
2.21.0

[Qemu-devel] [PATCH 1/3] python/qemu: Allow to launch the VM without qmp

[Wainer dos Santos Moschetta]

QEMUMachine launches the VM with a monitor enabled, afterwards
a qmp connection is attempted on _post_launch(). In case
the QEMU process exits with an error, qmp.accept() reaches
timeout and raises an exception.

But sometimes you don't need that monitor. As an example,
when a test launches the VM expecting its immediate crash,
and only intend to check the process's return code. In this
case the fact that launch() tries to establish the qmp
connection (ending up in an exception) is troublesome.

So this patch adds the set_qmp_monitor() that allow to
launch the VM without creating the monitor machinery.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
---
 python/qemu/__init__.py | 72 +++++++++++++++++++++++++++--------------
 1 file changed, 48 insertions(+), 24 deletions(-)

diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
index dbaf8a5311..dd577e9446 100644
--- a/python/qemu/__init__.py
+++ b/python/qemu/__init__.py
@@ -113,6 +113,7 @@ class QEMUMachine(object):
         self._events = []
         self._iolog = None
         self._socket_scm_helper = socket_scm_helper
+        self._qmp_set = True   # Enable QMP monitor by default.
         self._qmp = None
         self._qemu_full_args = None
         self._test_dir = test_dir
@@ -227,15 +228,7 @@ class QEMUMachine(object):
                 self._iolog = iolog.read()
 
     def _base_args(self):
-        if isinstance(self._monitor_address, tuple):
-            moncdev = "socket,id=mon,host=%s,port=%s" % (
-                self._monitor_address[0],
-                self._monitor_address[1])
-        else:
-            moncdev = 'socket,id=mon,path=%s' % self._vm_monitor
-        args = ['-chardev', moncdev,
-                '-mon', 'chardev=mon,mode=control',
-                '-display', 'none', '-vga', 'none']
+        args = ['-display', 'none', '-vga', 'none']
         if self._machine is not None:
             args.extend(['-machine', self._machine])
         if self._console_set:
@@ -249,23 +242,33 @@ class QEMUMachine(object):
             else:
                 device = '%s,chardev=console' % self._console_device_type
                 args.extend(['-device', device])
+        if self._qmp_set:
+            if isinstance(self._monitor_address, tuple):
+                moncdev = "socket,id=mon,host=%s,port=%s" % (
+                    self._monitor_address[0],
+                    self._monitor_address[1])
+            else:
+                moncdev = 'socket,id=mon,path=%s' % self._vm_monitor
+            args.extend(['-chardev', moncdev, '-mon', 'chardev=mon,mode=control'])
+
         return args
 
     def _pre_launch(self):
         self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
-        if self._monitor_address is not None:
-            self._vm_monitor = self._monitor_address
-        else:
-            self._vm_monitor = os.path.join(self._temp_dir,
-                                            self._name + "-monitor.sock")
         self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
         self._qemu_log_file = open(self._qemu_log_path, 'wb')
 
-        self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor,
-                                            server=True)
-
+        if self._qmp_set:
+            if self._monitor_address is not None:
+                self._vm_monitor = self._monitor_address
+            else:
+                self._vm_monitor = os.path.join(self._temp_dir,
+                                            self._name + "-monitor.sock")
+            self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor,
+                                                    server=True)
     def _post_launch(self):
-        self._qmp.accept()
+        if self._qmp:
+            self._qmp.accept()
 
     def _post_shutdown(self):
         if self._qemu_log_file is not None:
@@ -328,7 +331,8 @@ class QEMUMachine(object):
         Wait for the VM to power off
         """
         self._popen.wait()
-        self._qmp.close()
+        if self._qmp:
+            self._qmp.close()
         self._load_io_log()
         self._post_shutdown()
 
@@ -337,11 +341,14 @@ class QEMUMachine(object):
         Terminate the VM and clean up
         """
         if self.is_running():
-            try:
-                self._qmp.cmd('quit')
-                self._qmp.close()
-            except:
-                self._popen.kill()
+            if self._qmp:
+                try:
+                    self._qmp.cmd('quit')
+                    self._qmp.close()
+                except:
+                    self._popen.kill()
+            else:
+                self._popen.terminate()
             self._popen.wait()
 
         self._load_io_log()
@@ -358,6 +365,23 @@ class QEMUMachine(object):
 
         self._launched = False
 
+    def set_qmp_monitor(self, disabled=False, monitor_address=None):
+        """
+        Set the QMP monitor.
+
+        @param disabled: if True, qmp monitor options will be removed from the
+                         base arguments of the resulting QEMU command line.
+        @param monitor_address: address for the QMP monitor.
+        @note: call this function before launch().
+        """
+        if disabled:
+            self._qmp_set = False
+            self._qmp = None
+        else:
+            self._qmp_set = True
+            if monitor_address:
+                self._monitor_address = monitor_address
+
     def qmp(self, cmd, conv_keys=True, **args):
         """
         Invoke a QMP command and return the response dict
-- 
2.21.0

[Qemu-devel] [PATCH 2/3] tests/acceptance: Introduce the "accel" tag

[Wainer dos Santos Moschetta]

Some test cases may boot a VM with accelerator that isn't actually
enabled on the QEMU binary or present in the host. In this case
the test case is gonna fail miserably, unless it can be skipped.

This change introduces the "accel" tag, used to mark the test
case requires a given accelerator(s). It was implemented a
mechanism to check the given accelerator is available, and if not
then the test case is skipped.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
---
 tests/acceptance/avocado_qemu/__init__.py |  5 ++
 tests/acceptance/avocado_qemu/accel.py    | 60 +++++++++++++++++++++++
 2 files changed, 65 insertions(+)
 create mode 100644 tests/acceptance/avocado_qemu/accel.py

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index 2b236a1cf0..f823f7525b 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -18,6 +18,7 @@ SRC_ROOT_DIR = os.path.join(os.path.dirname(__file__), '..', '..', '..')
 sys.path.append(os.path.join(SRC_ROOT_DIR, 'python'))
 
 from qemu import QEMUMachine
+from .accel import is_accel_available
 
 def is_readable_executable_file(path):
     return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
@@ -65,6 +66,10 @@ class Test(avocado.Test):
         if self.qemu_bin is None:
             self.cancel("No QEMU binary defined or found in the source tree")
 
+        for accel in self.tags.get('accel', []):
+            if not is_accel_available(accel, self.qemu_bin):
+                self.cancel("Accelerator %s not available" % accel)
+
     def _new_vm(self, *args):
         vm = QEMUMachine(self.qemu_bin)
         if args:
diff --git a/tests/acceptance/avocado_qemu/accel.py b/tests/acceptance/avocado_qemu/accel.py
new file mode 100644
index 0000000000..21f7240d56
--- /dev/null
+++ b/tests/acceptance/avocado_qemu/accel.py
@@ -0,0 +1,60 @@
+# Utilities for using QEMU accelerators on tests.
+#
+# Copyright (c) 2019 Red Hat, Inc.
+#
+# Author:
+#  Wainer dos Santos Moschetta <wainersm@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later.  See the COPYING file in the top-level directory.
+
+from qemu import QEMUMachine
+from qemu import kvm_available
+
+def list_accel(qemu_bin):
+    """
+    List accelerators enabled in the binary.
+
+    :param qemu_bin: path to the QEMU binary.
+    :type qemu_bin: str
+    :returns: list of accelerator names.
+    :rtype: list
+    """
+    vm = QEMUMachine(qemu_bin)
+    vm.set_qmp_monitor(disabled=True)
+    vm.add_args('-accel', 'help')
+    vm.launch()
+    vm.wait()
+    if vm.exitcode() != 0:
+        raise Exception("Failed to get the accelerators in %s" % qemu_bin)
+    lines = vm.get_log().splitlines()
+    # skip first line which is the output header.
+    return [l for l in lines[1:] if l]
+
+def _tcg_avail_checker(qemu_bin):
+    # checks TCG is enabled in the binary only.
+    return 'tcg' in list_accel(qemu_bin)
+
+def _kvm_avail_checker(qemu_bin):
+    # checks KVM is present in the host as well as enabled in the binary.
+    return kvm_available() and "kvm" in list_accel(qemu_bin)
+
+_CHECKERS = {"tcg": _tcg_avail_checker, "kvm": _kvm_avail_checker}
+
+def is_accel_available(accel, qemu_bin):
+    """
+    Check the accelerator is available (enabled in the binary as well as
+    present on host).
+
+    :param accel:  accelerator's name.
+    :type accel: str
+    :param qemu_bin: path to the QEMU binary.
+    :type qemu_bin: str
+    :returns: True if accelerator is available, False otherwise.
+    :rtype: boolean
+    """
+    checker = _CHECKERS.get(accel, None)
+    if checker:
+        return checker(qemu_bin)
+    raise Exception("Availability checker not implemented for %s accelerator." %
+                    accel)
-- 
2.21.0

[Qemu-devel] [PATCH 3/3] tests/acceptance: Add boot linux with kvm test

[Wainer dos Santos Moschetta]

Until now the suite of acceptance tests doesn't exercise
QEMU with kvm enabled. So this introduces a simple test
that boots the Linux kernel and checks it boots on the
accelerator correctly.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
---
 tests/acceptance/kvm.py | 58 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 tests/acceptance/kvm.py

diff --git a/tests/acceptance/kvm.py b/tests/acceptance/kvm.py
new file mode 100644
index 0000000000..aafb865cdb
--- /dev/null
+++ b/tests/acceptance/kvm.py
@@ -0,0 +1,58 @@
+# KVM acceptance tests.
+#
+# Copyright (c) 2019 Red Hat, Inc.
+#
+# Author:
+#  Wainer dos Santos Moschetta <wainersm@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later.  See the COPYING file in the top-level directory.
+
+import logging
+
+from avocado_qemu import Test
+
+
+class Kvm(Test):
+    """
+    Suite of acceptance tests to check QEMU and KVM integration.
+    """
+
+    def test_boot_linux(self):
+        """
+        Simple Linux boot test with kvm enabled.
+
+        :avocado: tags=arch:x86_64
+        :avocado: tags=accel:kvm
+        """
+        self.vm.add_args('-enable-kvm')
+        kernel_url = ('https://download.fedoraproject.org/pub/fedora/linux/'
+                      'releases/29/Everything/x86_64/os/images/pxeboot/vmlinuz')
+        kernel_hash = '23bebd2680757891cf7adedb033532163a792495'
+        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
+
+        self.vm.set_machine('pc')
+        self.vm.set_console()
+        self.vm.add_args('-kernel', kernel_path,
+                         '-append', 'printk.time=0 console=ttyS0')
+        self.vm.launch()
+
+        query = self.vm.command('query-kvm')
+        self.assertTrue(query['enabled'])
+        self.assertTrue(query['present'])
+
+        console = self.vm.console_socket.makefile()
+        console_logger = logging.getLogger('console')
+        failure_message = 'Kernel panic - not syncing'
+        success_message = 'Booting paravirtualized kernel on KVM'
+
+        while True:
+            msg = console.readline().strip()
+            if not msg:
+                continue
+            console_logger.debug(msg)
+            if success_message in msg:
+                break
+            if failure_message in msg:
+                fail = 'Failure message found in console: %s' % failure_message
+                self.fail(fail)
-- 
2.21.0

Re: [Qemu-devel] [PATCH 3/3] tests/acceptance: Add boot linux with kvm test

[Eduardo Habkost]

On Fri, Jun 28, 2019 at 11:02:17AM -0400, Wainer dos Santos Moschetta wrote:
> Until now the suite of acceptance tests doesn't exercise
> QEMU with kvm enabled. So this introduces a simple test
> that boots the Linux kernel and checks it boots on the
> accelerator correctly.
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

Why not just change the existing test_x86_64_pc() test case to
use KVM by default?  We can use "accel=kvm:tcg" to allow it to
fall back to TCG if KVM is not available.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/3] tests/acceptance: Add boot linux with kvm test

[Cleber Rosa]

On Fri, Jun 28, 2019 at 05:18:46PM -0300, Eduardo Habkost wrote:
> On Fri, Jun 28, 2019 at 11:02:17AM -0400, Wainer dos Santos Moschetta wrote:
> > Until now the suite of acceptance tests doesn't exercise
> > QEMU with kvm enabled. So this introduces a simple test
> > that boots the Linux kernel and checks it boots on the
> > accelerator correctly.
> > 
> > Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> 
> Why not just change the existing test_x86_64_pc() test case to
> use KVM by default?  We can use "accel=kvm:tcg" to allow it to
> fall back to TCG if KVM is not available.
> 
> -- 
> Eduardo

I though of something similar, but not exactly the same.  An example
can be seen here:

  https://travis-ci.org/clebergnu/qemu/jobs/551437429#L3350

IMO, it's a good practice to be able to briefly describe what a test
does, given its name.  It's also very important for the test to
attempt to exercise the same behavior across executions.

I'm saying that because I don't think we should fallback to TCG if KVM
is not available, but instead, have two different tests that do each a
simpler and more predictable set of checks.  This would make it
simpler to find KVM issues when a given test fails but the TCG
continues to pass.  The tags (and other mechanisms) can be used to
select the tests that a given job should run though.

Regards!
- Cleber.

Re: [Qemu-devel] [PATCH 3/3] tests/acceptance: Add boot linux with kvm test

[Eduardo Habkost]

On Sun, Jun 30, 2019 at 01:39:33PM -0400, Cleber Rosa wrote:
> On Fri, Jun 28, 2019 at 05:18:46PM -0300, Eduardo Habkost wrote:
> > On Fri, Jun 28, 2019 at 11:02:17AM -0400, Wainer dos Santos Moschetta wrote:
> > > Until now the suite of acceptance tests doesn't exercise
> > > QEMU with kvm enabled. So this introduces a simple test
> > > that boots the Linux kernel and checks it boots on the
> > > accelerator correctly.
> > > 
> > > Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> > 
> > Why not just change the existing test_x86_64_pc() test case to
> > use KVM by default?  We can use "accel=kvm:tcg" to allow it to
> > fall back to TCG if KVM is not available.
> > 
> > -- 
> > Eduardo
> 
> I though of something similar, but not exactly the same.  An example
> can be seen here:
> 
>   https://travis-ci.org/clebergnu/qemu/jobs/551437429#L3350
> 
> IMO, it's a good practice to be able to briefly describe what a test
> does, given its name.  It's also very important for the test to
> attempt to exercise the same behavior across executions.
> 
> I'm saying that because I don't think we should fallback to TCG if KVM
> is not available, but instead, have two different tests that do each a
> simpler and more predictable set of checks.  This would make it
> simpler to find KVM issues when a given test fails but the TCG
> continues to pass.  The tags (and other mechanisms) can be used to
> select the tests that a given job should run though.

Agreed that kvm:tcg fallback I suggested isn't a good idea.
However, do we really want to require a separate test method to
be written just because we want to use a different accelerator or
other QEMU option?

This patch may be the simplest solution short term, but can we
have something that doesn't require so much code duplication and
boilerplate code in the future?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/3] tests/acceptance: Add boot linux with kvm test

[Cleber Rosa]

On Mon, Jul 01, 2019 at 03:34:51PM -0300, Eduardo Habkost wrote:
> 
> Agreed that kvm:tcg fallback I suggested isn't a good idea.
> However, do we really want to require a separate test method to
> be written just because we want to use a different accelerator or
> other QEMU option?
>

No, in the short term we want to have tests that can respond to a
number of well known parameters, such as "accel".  But to actually
have tests (names) that are meaningful enough, we need to:

 1) Add a varianter implementation (or usage)
 2) Drop the duplicate tests

#1 is needed because:

 a) it doesn't feel right to name tests based on simple command
    line parameters (the ones given with -p, say, "-p accel=kvm"
    will add to the test name "accel_kvm".

 b) a variant *name* is added to the test ID, which then can be
    kept consistent.

Then we can proceed to #2, and drop the duplicate tests, say:

 - test_x86_64_pc, test_x86_64_pc_kvm => test_x86_64_pc

On a further iteration, it may even make sense to consolidate:

 - test_x86_64_pc, test_x86_64_q35 => test_x86_64

Time will tell.

> This patch may be the simplest solution short term, but can we
> have something that doesn't require so much code duplication and
> boilerplate code in the future?

Yes, the new implementation of the Varianter CIT is now generally
available on Avocado 70.0, so I'm working on a file that hopefully
will suite the acceptance tests.

> 
> -- 
> Eduardo

Best,
- Cleber.

Re: [Qemu-devel] [PATCH 3/3] tests/acceptance: Add boot linux with kvm test

[Wainer dos Santos Moschetta]

On 07/01/2019 05:29 PM, Cleber Rosa wrote:
> On Mon, Jul 01, 2019 at 03:34:51PM -0300, Eduardo Habkost wrote:
>> Agreed that kvm:tcg fallback I suggested isn't a good idea.
>> However, do we really want to require a separate test method to
>> be written just because we want to use a different accelerator or
>> other QEMU option?
>>
> No, in the short term we want to have tests that can respond to a
> number of well known parameters, such as "accel".  But to actually
> have tests (names) that are meaningful enough, we need to:
>
>   1) Add a varianter implementation (or usage)
>   2) Drop the duplicate tests
>
> #1 is needed because:
>
>   a) it doesn't feel right to name tests based on simple command
>      line parameters (the ones given with -p, say, "-p accel=kvm"
>      will add to the test name "accel_kvm".
>
>   b) a variant *name* is added to the test ID, which then can be
>      kept consistent.
>
> Then we can proceed to #2, and drop the duplicate tests, say:
>
>   - test_x86_64_pc, test_x86_64_pc_kvm => test_x86_64_pc
>
> On a further iteration, it may even make sense to consolidate:
>
>   - test_x86_64_pc, test_x86_64_q35 => test_x86_64
>
> Time will tell.
>
>> This patch may be the simplest solution short term, but can we
>> have something that doesn't require so much code duplication and
>> boilerplate code in the future?
> Yes, the new implementation of the Varianter CIT is now generally
> available on Avocado 70.0, so I'm working on a file that hopefully
> will suite the acceptance tests.

Cleber, Eduardo, that's a good discussion. I was expecting we would 
eventually evolve the acceptance tests to use Avocado varianter feature.

Now I think what to do with this series.

I can see the usefulness of patch 01 when you want to, for example, 
start qemu expecting a crash or just want to gather information from 
command-line (qemu -cpu help, qemu -accel help...etc).

The implementation on patch 02 to check the availability of accelerators 
seems desirable even on this (near, maybe) future where tests can be 
automatically derived.

Thus, can we merge patches 01 and 02 only? Of course, if they pass the 
reviews.

Thanks!

- Wainer

>
>> -- 
>> Eduardo
> Best,
> - Cleber.