[PATCH 00/22] Acceptance Test: introduce base class for Linux based tests

[PATCH 00/22] Acceptance Test: introduce base class for Linux based tests

[Cleber Rosa]

This introduces a base class for tests that need to interact with a
Linux guest.  It generalizes the "boot_linux.py" code, already been
used by the "virtiofs_submounts.py" and also SSH related code being
used by that and "linux_ssh_mips_malta.py".

While at it, a number of fixes on hopeful improvements to those tests
were added.

Cleber Rosa (22):
  tests/acceptance/boot_linux.py: fix typo on cloudinit error message
  tests/acceptance/boot_linux.py: rename misleading cloudinit method
  Acceptance Tests: remove unnecessary tag from documentation example
  tests/acceptance/virtiofs_submounts.py: use workdir property
  tests/acceptance/virtiofs_submounts.py: do not ask for ssh key
    password
  tests/acceptance/virtiofs_submounts.py: use a virtio-net device
    instead
  tests/acceptance/virtiofs_submounts.py: evaluate string not length
  tests/acceptance/virtiofs_submounts.py: standardize port as integer
  tests/acceptance/virtiofs_submounts.py: required space between IP and
    port
  Python: add utility function for retrieving port redirection
  tests/acceptance/linux_ssh_mips_malta.py: standardize port as integer
  Acceptance tests: clarify ssh connection failure reason
  tests/acceptance/virtiofs_submounts.py: add missing accel tag
  Acceptance Tests: introduce LinuxTest base class
  Acceptance Tests: move useful ssh methods to base class
  Acceptance Tests: introduce method for requiring an accelerator
  Acceptance Tests: fix population of public key in cloudinit image
  Acceptance Tests: set up existing ssh keys by default
  Acceptance Tests: add port redirection for ssh by default
  Acceptance Tests: add basic documentation on LinuxTest base class
  Acceptance Tests: introduce CPU hotplug test
  [NOTFORMERGE] Bump Avocado version to latest master

 docs/devel/testing.rst                    |  29 +++-
 python/qemu/utils.py                      |  35 +++++
 tests/acceptance/avocado_qemu/__init__.py | 176 ++++++++++++++++++++++
 tests/acceptance/boot_linux.py            | 128 ++--------------
 tests/acceptance/hotplug_cpu.py           |  38 +++++
 tests/acceptance/info_usernet.py          |  29 ++++
 tests/acceptance/linux_ssh_mips_malta.py  |  44 +-----
 tests/acceptance/virtiofs_submounts.py    |  73 +--------
 tests/requirements.txt                    |   2 +-
 tests/vm/basevm.py                        |   7 +-
 10 files changed, 334 insertions(+), 227 deletions(-)
 create mode 100644 python/qemu/utils.py
 create mode 100644 tests/acceptance/hotplug_cpu.py
 create mode 100644 tests/acceptance/info_usernet.py

-- 
2.25.4

[PATCH 01/22] tests/acceptance/boot_linux.py: fix typo on cloudinit error message

[Cleber Rosa]

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/boot_linux.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
index 1da4a53d6a..2ac3e57587 100644
--- a/tests/acceptance/boot_linux.py
+++ b/tests/acceptance/boot_linux.py
@@ -70,7 +70,7 @@ class BootLinuxBase(Test):
                           phone_home_port=self.phone_home_port,
                           authorized_key=ssh_pubkey)
         except Exception:
-            self.cancel('Failed to prepared cloudinit image')
+            self.cancel('Failed to prepare the cloudinit image')
         return cloudinit_iso
 
 class BootLinux(BootLinuxBase):
-- 
2.25.4

[PATCH 02/22] tests/acceptance/boot_linux.py: rename misleading cloudinit method

[Cleber Rosa]

There's no downloading happening on that method, so let's call it
"prepare" instead.  While at it, and because of it, the current
"prepare_boot" and "prepare_cloudinit" are also renamed.

The reasoning here is that "prepare_" methods will just work on the
images, while "set_up_" will make them effective to the VM that will
be launched.  Inspiration comes from the "virtiofs_submounts.py"
tests, which this expects to converge more into.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/boot_linux.py | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
index 2ac3e57587..bcd923bb4a 100644
--- a/tests/acceptance/boot_linux.py
+++ b/tests/acceptance/boot_linux.py
@@ -57,7 +57,7 @@ class BootLinuxBase(Test):
             self.cancel('Failed to download/prepare boot image')
         return boot.path
 
-    def download_cloudinit(self, ssh_pubkey=None):
+    def prepare_cloudinit(self, ssh_pubkey=None):
         self.log.info('Preparing cloudinit image')
         try:
             cloudinit_iso = os.path.join(self.workdir, 'cloudinit.iso')
@@ -85,15 +85,15 @@ class BootLinux(BootLinuxBase):
         super(BootLinux, self).setUp()
         self.vm.add_args('-smp', '2')
         self.vm.add_args('-m', '1024')
-        self.prepare_boot()
-        self.prepare_cloudinit(ssh_pubkey)
+        self.set_up_boot()
+        self.set_up_cloudinit(ssh_pubkey)
 
-    def prepare_boot(self):
+    def set_up_boot(self):
         path = self.download_boot()
         self.vm.add_args('-drive', 'file=%s' % path)
 
-    def prepare_cloudinit(self, ssh_pubkey=None):
-        cloudinit_iso = self.download_cloudinit(ssh_pubkey)
+    def set_up_cloudinit(self, ssh_pubkey=None):
+        cloudinit_iso = self.prepare_cloudinit(ssh_pubkey)
         self.vm.add_args('-drive', 'file=%s,format=raw' % cloudinit_iso)
 
     def launch_and_wait(self):
-- 
2.25.4

[PATCH 03/22] Acceptance Tests: remove unnecessary tag from documentation example

[Cleber Rosa]

The ":avocado: enable" is not necessary and was removed in 9531d26c,
so let's remove from the docs.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 docs/devel/testing.rst | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 809af69725..209f9d8172 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -765,9 +765,6 @@ and hypothetical example follows:
 
 
   class MultipleMachines(Test):
-      """
-      :avocado: enable
-      """
       def test_multiple_machines(self):
           first_machine = self.get_vm()
           second_machine = self.get_vm()
-- 
2.25.4

[PATCH 04/22] tests/acceptance/virtiofs_submounts.py: use workdir property

[Cleber Rosa]

For Avocado Instrumented based tests, it's a better idea to just use
the property.  The environment variable is a fall back for tests not
written using that Python API.

Reference: https://avocado-framework.readthedocs.io/en/84.0/api/test/avocado.html#avocado.Test.workdir
Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/virtiofs_submounts.py | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
index 361e5990b6..68d3cd6869 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -136,8 +136,7 @@ class VirtiofsSubmountsTest(BootLinux):
         return (stdout, stderr, ret)
 
     def set_up_shared_dir(self):
-        atwd = os.getenv('AVOCADO_TEST_WORKDIR')
-        self.shared_dir = os.path.join(atwd, 'virtiofs-shared')
+        self.shared_dir = os.path.join(self.workdir, 'virtiofs-shared')
 
         os.mkdir(self.shared_dir)
 
@@ -234,8 +233,7 @@ class VirtiofsSubmountsTest(BootLinux):
 
         self.seed = self.params.get('seed')
 
-        atwd = os.getenv('AVOCADO_TEST_WORKDIR')
-        self.ssh_key = os.path.join(atwd, 'id_ed25519')
+        self.ssh_key = os.path.join(self.workdir, 'id_ed25519')
 
         self.run(('ssh-keygen', '-t', 'ed25519', '-f', self.ssh_key))
 
-- 
2.25.4

[PATCH 05/22] tests/acceptance/virtiofs_submounts.py: do not ask for ssh key password

[Cleber Rosa]

Tests are supposed to be non-interactive, and ssh-keygen is asking for
a password when creating a key.  Let's set an empty key, which prevents
ssh-keygen for asking for a password.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/virtiofs_submounts.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
index 68d3cd6869..3b5a242385 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -235,7 +235,7 @@ class VirtiofsSubmountsTest(BootLinux):
 
         self.ssh_key = os.path.join(self.workdir, 'id_ed25519')
 
-        self.run(('ssh-keygen', '-t', 'ed25519', '-f', self.ssh_key))
+        self.run(('ssh-keygen', '-N', '', '-t', 'ed25519', '-f', self.ssh_key))
 
         pubkey = open(self.ssh_key + '.pub').read()
 
-- 
2.25.4

[PATCH 06/22] tests/acceptance/virtiofs_submounts.py: use a virtio-net device instead

[Cleber Rosa]

In a virtiofs based tests, it seems safe to assume that the guest will
be capable of a virtio-net device.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/virtiofs_submounts.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
index 3b5a242385..f1b49f03bb 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -247,7 +247,7 @@ class VirtiofsSubmountsTest(BootLinux):
 
         # Allow us to connect to SSH
         self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22',
-                         '-device', 'e1000,netdev=vnet')
+                         '-device', 'virtio-net,netdev=vnet')
 
         if not kvm_available(self.arch, self.qemu_bin):
             self.cancel(KVM_NOT_AVAILABLE)
-- 
2.25.4

[PATCH 07/22] tests/acceptance/virtiofs_submounts.py: evaluate string not length

[Cleber Rosa]

If the vmlinuz variable is set to anything that evaluates to True,
then the respective arguments should be set.  If the variable contains
an empty string, than it will evaluate to False, and the extra
arguments will not be set.

This keeps the same logic, but improves readability a bit.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/virtiofs_submounts.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
index f1b49f03bb..f25a386a19 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -241,7 +241,7 @@ class VirtiofsSubmountsTest(BootLinux):
 
         super(VirtiofsSubmountsTest, self).setUp(pubkey)
 
-        if len(vmlinuz) > 0:
+        if vmlinuz:
             self.vm.add_args('-kernel', vmlinuz,
                              '-append', 'console=ttyS0 root=/dev/sda1')
 
-- 
2.25.4

[PATCH 08/22] tests/acceptance/virtiofs_submounts.py: standardize port as integer

[Cleber Rosa]

Instead of having to cast it whenever it's going to be used, let's
standardize it as an integer, which is the data type that will be
used most often.

Given that the regex will only match digits, it's safe that we'll
end up getting a integer, but, it could as well be a zero.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/virtiofs_submounts.py | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
index f25a386a19..227a3cf1ee 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -86,17 +86,18 @@ class VirtiofsSubmountsTest(BootLinux):
                 re.search(r'TCP.HOST_FORWARD.*127\.0\.0\.1\s*(\d+)\s+10\.',
                           line)
             if match is not None:
-                port = match[1]
+                port = int(match[1])
                 break
 
         self.assertIsNotNone(port)
-        self.log.debug('sshd listening on port: ' + port)
+        self.assertGreater(port, 0)
+        self.log.debug('sshd listening on port: %d', port)
         return port
 
     def ssh_connect(self, username, keyfile):
         self.ssh_logger = logging.getLogger('ssh')
         port = self.get_portfwd()
-        self.ssh_session = ssh.Session('127.0.0.1', port=int(port),
+        self.ssh_session = ssh.Session('127.0.0.1', port=port,
                                        user=username, key=keyfile)
         for i in range(10):
             try:
-- 
2.25.4

[PATCH 09/22] tests/acceptance/virtiofs_submounts.py: required space between IP and port

[Cleber Rosa]

AFAICT, there should not be a situation where IP and port do not have
at least one whitespace character separating them.

This may be true for other '\s*' patterns in the same regex too.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/virtiofs_submounts.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
index 227a3cf1ee..0f9d7a5d9c 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -83,7 +83,7 @@ class VirtiofsSubmountsTest(BootLinux):
                               command_line='info usernet')
         for line in res.split('\r\n'):
             match = \
-                re.search(r'TCP.HOST_FORWARD.*127\.0\.0\.1\s*(\d+)\s+10\.',
+                re.search(r'TCP.HOST_FORWARD.*127\.0\.0\.1\s+(\d+)\s+10\.',
                           line)
             if match is not None:
                 port = int(match[1])
-- 
2.25.4

[PATCH 10/22] Python: add utility function for retrieving port redirection

[Cleber Rosa]

Slightly different versions for the same utility code are currently
present on different locations.  This unifies them all, giving
preference to the version from virtiofs_submounts.py, because of the
last tweaks added to it.

While at it, this adds a "qemu.util" module to host the utility
function and a test.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 python/qemu/utils.py                     | 35 ++++++++++++++++++++++++
 tests/acceptance/info_usernet.py         | 29 ++++++++++++++++++++
 tests/acceptance/linux_ssh_mips_malta.py | 16 +++++------
 tests/acceptance/virtiofs_submounts.py   | 20 +++-----------
 tests/vm/basevm.py                       |  7 ++---
 5 files changed, 77 insertions(+), 30 deletions(-)
 create mode 100644 python/qemu/utils.py
 create mode 100644 tests/acceptance/info_usernet.py

diff --git a/python/qemu/utils.py b/python/qemu/utils.py
new file mode 100644
index 0000000000..89a246ab30
--- /dev/null
+++ b/python/qemu/utils.py
@@ -0,0 +1,35 @@
+"""
+QEMU utility library
+
+This offers miscellaneous utility functions, which may not be easily
+distinguishable or numerous to be in their own module.
+"""
+
+# Copyright (C) 2021 Red Hat Inc.
+#
+# Authors:
+#  Cleber Rosa <crosa@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2.  See
+# the COPYING file in the top-level directory.
+#
+
+import re
+from typing import Optional
+
+
+def get_info_usernet_hostfwd_port(info_usernet_output: str) -> Optional[int]:
+    """
+    Returns the port given to the hostfwd parameter via info usernet
+
+    :param info_usernet_output: output generated by hmp command "info usernet"
+    :param info_usernet_output: str
+    :return: the port number allocated by the hostfwd option
+    :rtype: int
+    """
+    for line in info_usernet_output.split('\r\n'):
+        regex = r'TCP.HOST_FORWARD.*127\.0\.0\.1\s+(\d+)\s+10\.'
+        match = re.search(regex, line)
+        if match is not None:
+            return int(match[1])
+    return None
diff --git a/tests/acceptance/info_usernet.py b/tests/acceptance/info_usernet.py
new file mode 100644
index 0000000000..9c1fd903a0
--- /dev/null
+++ b/tests/acceptance/info_usernet.py
@@ -0,0 +1,29 @@
+# Test for the hmp command "info usernet"
+#
+# Copyright (c) 2021 Red Hat, Inc.
+#
+# Author:
+#  Cleber Rosa <crosa@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 avocado_qemu import Test
+
+from qemu.utils import get_info_usernet_hostfwd_port
+
+
+class InfoUsernet(Test):
+
+    def test_hostfwd(self):
+        self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22')
+        self.vm.launch()
+        res = self.vm.command('human-monitor-command',
+                              command_line='info usernet')
+        port = get_info_usernet_hostfwd_port(res)
+        self.assertIsNotNone(port,
+                             ('"info usernet" output content does not seem to '
+                              'contain the redirected port'))
+        self.assertGreater(port, 0,
+                           ('Found a redirected port that is not greater than'
+                            ' zero'))
diff --git a/tests/acceptance/linux_ssh_mips_malta.py b/tests/acceptance/linux_ssh_mips_malta.py
index 25c5c5f741..275659c785 100644
--- a/tests/acceptance/linux_ssh_mips_malta.py
+++ b/tests/acceptance/linux_ssh_mips_malta.py
@@ -18,6 +18,8 @@ from avocado.utils import process
 from avocado.utils import archive
 from avocado.utils import ssh
 
+from qemu.utils import get_info_usernet_hostfwd_port
+
 
 class LinuxSSH(Test):
 
@@ -70,18 +72,14 @@ class LinuxSSH(Test):
     def setUp(self):
         super(LinuxSSH, self).setUp()
 
-    def get_portfwd(self):
+    def ssh_connect(self, username, password):
+        self.ssh_logger = logging.getLogger('ssh')
         res = self.vm.command('human-monitor-command',
                               command_line='info usernet')
-        line = res.split('\r\n')[2]
-        port = re.split(r'.*TCP.HOST_FORWARD.*127\.0\.0\.1 (\d+)\s+10\..*',
-                        line)[1]
+        port = get_info_usernet_hostfwd_port(res)
+        if not port:
+            self.cancel("Failed to retrieve SSH port")
         self.log.debug("sshd listening on port:" + port)
-        return port
-
-    def ssh_connect(self, username, password):
-        self.ssh_logger = logging.getLogger('ssh')
-        port = self.get_portfwd()
         self.ssh_session = ssh.Session(self.VM_IP, port=int(port),
                                        user=username, password=password)
         for i in range(10):
diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
index 0f9d7a5d9c..bf99164fcb 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -10,6 +10,7 @@ from avocado_qemu import wait_for_console_pattern
 from avocado.utils import ssh
 
 from qemu.accel import kvm_available
+from qemu.utils import get_info_usernet_hostfwd_port
 
 from boot_linux import BootLinux
 
@@ -76,27 +77,14 @@ class VirtiofsSubmountsTest(BootLinux):
     :avocado: tags=arch:x86_64
     """
 
-    def get_portfwd(self):
-        port = None
-
+    def ssh_connect(self, username, keyfile):
+        self.ssh_logger = logging.getLogger('ssh')
         res = self.vm.command('human-monitor-command',
                               command_line='info usernet')
-        for line in res.split('\r\n'):
-            match = \
-                re.search(r'TCP.HOST_FORWARD.*127\.0\.0\.1\s+(\d+)\s+10\.',
-                          line)
-            if match is not None:
-                port = int(match[1])
-                break
-
+        port = get_info_usernet_hostfwd_port(res)
         self.assertIsNotNone(port)
         self.assertGreater(port, 0)
         self.log.debug('sshd listening on port: %d', port)
-        return port
-
-    def ssh_connect(self, username, keyfile):
-        self.ssh_logger = logging.getLogger('ssh')
-        port = self.get_portfwd()
         self.ssh_session = ssh.Session('127.0.0.1', port=port,
                                        user=username, key=keyfile)
         for i in range(10):
diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index 00f1d5ca8d..75ce07df36 100644
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -21,6 +21,7 @@ import datetime
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu.accel import kvm_available
 from qemu.machine import QEMUMachine
+from qemu.utils import get_info_usernet_hostfwd_port
 import subprocess
 import hashlib
 import argparse
@@ -306,11 +307,7 @@ class BaseVM(object):
         self.console_init()
         usernet_info = guest.qmp("human-monitor-command",
                                  command_line="info usernet")
-        self.ssh_port = None
-        for l in usernet_info["return"].splitlines():
-            fields = l.split()
-            if "TCP[HOST_FORWARD]" in fields and "22" in fields:
-                self.ssh_port = l.split()[3]
+        self.ssh_port = get_info_usernet_hostfwd_port(usernet_info)
         if not self.ssh_port:
             raise Exception("Cannot find ssh port from 'info usernet':\n%s" % \
                             usernet_info)
-- 
2.25.4

[PATCH 11/22] tests/acceptance/linux_ssh_mips_malta.py: standardize port as integer

[Cleber Rosa]

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/linux_ssh_mips_malta.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/acceptance/linux_ssh_mips_malta.py b/tests/acceptance/linux_ssh_mips_malta.py
index 275659c785..ab6cb94aef 100644
--- a/tests/acceptance/linux_ssh_mips_malta.py
+++ b/tests/acceptance/linux_ssh_mips_malta.py
@@ -79,8 +79,8 @@ class LinuxSSH(Test):
         port = get_info_usernet_hostfwd_port(res)
         if not port:
             self.cancel("Failed to retrieve SSH port")
-        self.log.debug("sshd listening on port:" + port)
-        self.ssh_session = ssh.Session(self.VM_IP, port=int(port),
+        self.log.debug("sshd listening on port: %d", port)
+        self.ssh_session = ssh.Session(self.VM_IP, port=port,
                                        user=username, password=password)
         for i in range(10):
             try:
-- 
2.25.4

[PATCH 12/22] Acceptance tests: clarify ssh connection failure reason

[Cleber Rosa]

If the connection to the ssh server fails, it may indeed be a "sshd"
issue, but it may also not be that.  Let's state what we know: the
establishment of the connection from the client side was not possible.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/linux_ssh_mips_malta.py | 2 +-
 tests/acceptance/virtiofs_submounts.py   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/acceptance/linux_ssh_mips_malta.py b/tests/acceptance/linux_ssh_mips_malta.py
index ab6cb94aef..1742235758 100644
--- a/tests/acceptance/linux_ssh_mips_malta.py
+++ b/tests/acceptance/linux_ssh_mips_malta.py
@@ -89,7 +89,7 @@ class LinuxSSH(Test):
             except:
                 time.sleep(4)
                 pass
-        self.fail("sshd timeout")
+        self.fail("ssh connection timeout")
 
     def ssh_disconnect_vm(self):
         self.ssh_session.quit()
diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
index bf99164fcb..c998b297ee 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -94,7 +94,7 @@ class VirtiofsSubmountsTest(BootLinux):
             except:
                 time.sleep(4)
                 pass
-        self.fail('sshd timeout')
+        self.fail('ssh connection timeout')
 
     def ssh_command(self, command):
         self.ssh_logger.info(command)
-- 
2.25.4

[PATCH 13/22] tests/acceptance/virtiofs_submounts.py: add missing accel tag

[Cleber Rosa]

Which is useful to select tests that depend/use a particular feature.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/virtiofs_submounts.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
index c998b297ee..1e745f15a2 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -75,6 +75,7 @@ def has_cmds(*cmds):
 class VirtiofsSubmountsTest(BootLinux):
     """
     :avocado: tags=arch:x86_64
+    :avocado: tags=accel:kvm
     """
 
     def ssh_connect(self, username, keyfile):
-- 
2.25.4

[PATCH 14/22] Acceptance Tests: introduce LinuxTest base class

[Cleber Rosa]

This is basically the infrastructure around "boot_linux.py" tests, but
now made into a base class for general use.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/avocado_qemu/__init__.py | 87 +++++++++++++++++++++
 tests/acceptance/boot_linux.py            | 94 ++---------------------
 tests/acceptance/virtiofs_submounts.py    |  6 +-
 3 files changed, 94 insertions(+), 93 deletions(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index bf54e419da..b06692a59d 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -16,6 +16,13 @@ import tempfile
 
 import avocado
 
+from avocado.utils import cloudinit
+from avocado.utils import datadrainer
+from avocado.utils import network
+from avocado.utils import vmimage
+from avocado.utils.path import find_command
+
+
 #: The QEMU build root directory.  It may also be the source directory
 #: if building from the source dir, but it's safer to use BUILD_DIR for
 #: that purpose.  Be aware that if this code is moved outside of a source
@@ -206,3 +213,83 @@ class Test(avocado.Test):
                         expire=expire,
                         find_only=find_only,
                         cancel_on_missing=cancel_on_missing)
+
+
+class LinuxTest(Test):
+    """Facilitates having a cloud-image Linux based available.
+
+    For tests that indend to interact with guests, this is a better choice
+    to start with than the more vanilla `Test` class.
+    """
+
+    timeout = 900
+    chksum = None
+
+    def setUp(self, ssh_pubkey=None):
+        super(LinuxTest, self).setUp()
+        self.vm.add_args('-smp', '2')
+        self.vm.add_args('-m', '1024')
+        self.set_up_boot()
+        self.set_up_cloudinit(ssh_pubkey)
+
+    def download_boot(self):
+        self.log.debug('Looking for and selecting a qemu-img binary to be '
+                       'used to create the bootable snapshot image')
+        # If qemu-img has been built, use it, otherwise the system wide one
+        # will be used.  If none is available, the test will cancel.
+        qemu_img = os.path.join(BUILD_DIR, 'qemu-img')
+        if not os.path.exists(qemu_img):
+            qemu_img = find_command('qemu-img', False)
+        if qemu_img is False:
+            self.cancel('Could not find "qemu-img", which is required to '
+                        'create the bootable image')
+        vmimage.QEMU_IMG = qemu_img
+
+        self.log.info('Downloading/preparing boot image')
+        # Fedora 31 only provides ppc64le images
+        image_arch = self.arch
+        if image_arch == 'ppc64':
+            image_arch = 'ppc64le'
+        try:
+            boot = vmimage.get(
+                'fedora', arch=image_arch, version='31',
+                checksum=self.chksum,
+                algorithm='sha256',
+                cache_dir=self.cache_dirs[0],
+                snapshot_dir=self.workdir)
+        except:
+            self.cancel('Failed to download/prepare boot image')
+        return boot.path
+
+    def prepare_cloudinit(self, ssh_pubkey=None):
+        self.log.info('Preparing cloudinit image')
+        try:
+            cloudinit_iso = os.path.join(self.workdir, 'cloudinit.iso')
+            self.phone_home_port = network.find_free_port()
+            cloudinit.iso(cloudinit_iso, self.name,
+                          username='root',
+                          password='password',
+                          # QEMU's hard coded usermode router address
+                          phone_home_host='10.0.2.2',
+                          phone_home_port=self.phone_home_port,
+                          authorized_key=ssh_pubkey)
+        except Exception:
+            self.cancel('Failed to prepare the cloudinit image')
+        return cloudinit_iso
+
+    def set_up_boot(self):
+        path = self.download_boot()
+        self.vm.add_args('-drive', 'file=%s' % path)
+
+    def set_up_cloudinit(self, ssh_pubkey=None):
+        cloudinit_iso = self.prepare_cloudinit(ssh_pubkey)
+        self.vm.add_args('-drive', 'file=%s,format=raw' % cloudinit_iso)
+
+    def launch_and_wait(self):
+        self.vm.set_console()
+        self.vm.launch()
+        console_drainer = datadrainer.LineLogger(self.vm.console_socket.fileno(),
+                                                 logger=self.log.getChild('console'))
+        console_drainer.start()
+        self.log.info('VM launched, waiting for boot confirmation from guest')
+        cloudinit.wait_for_phone_home(('0.0.0.0', self.phone_home_port), self.name)
diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
index bcd923bb4a..14e89d020d 100644
--- a/tests/acceptance/boot_linux.py
+++ b/tests/acceptance/boot_linux.py
@@ -10,16 +10,11 @@
 
 import os
 
-from avocado_qemu import Test, BUILD_DIR
+from avocado_qemu import LinuxTest, BUILD_DIR
 
 from qemu.accel import kvm_available
 from qemu.accel import tcg_available
 
-from avocado.utils import cloudinit
-from avocado.utils import network
-from avocado.utils import vmimage
-from avocado.utils import datadrainer
-from avocado.utils.path import find_command
 from avocado import skipIf
 
 ACCEL_NOT_AVAILABLE_FMT = "%s accelerator does not seem to be available"
@@ -27,86 +22,7 @@ KVM_NOT_AVAILABLE = ACCEL_NOT_AVAILABLE_FMT % "KVM"
 TCG_NOT_AVAILABLE = ACCEL_NOT_AVAILABLE_FMT % "TCG"
 
 
-class BootLinuxBase(Test):
-    def download_boot(self):
-        self.log.debug('Looking for and selecting a qemu-img binary to be '
-                       'used to create the bootable snapshot image')
-        # If qemu-img has been built, use it, otherwise the system wide one
-        # will be used.  If none is available, the test will cancel.
-        qemu_img = os.path.join(BUILD_DIR, 'qemu-img')
-        if not os.path.exists(qemu_img):
-            qemu_img = find_command('qemu-img', False)
-        if qemu_img is False:
-            self.cancel('Could not find "qemu-img", which is required to '
-                        'create the bootable image')
-        vmimage.QEMU_IMG = qemu_img
-
-        self.log.info('Downloading/preparing boot image')
-        # Fedora 31 only provides ppc64le images
-        image_arch = self.arch
-        if image_arch == 'ppc64':
-            image_arch = 'ppc64le'
-        try:
-            boot = vmimage.get(
-                'fedora', arch=image_arch, version='31',
-                checksum=self.chksum,
-                algorithm='sha256',
-                cache_dir=self.cache_dirs[0],
-                snapshot_dir=self.workdir)
-        except:
-            self.cancel('Failed to download/prepare boot image')
-        return boot.path
-
-    def prepare_cloudinit(self, ssh_pubkey=None):
-        self.log.info('Preparing cloudinit image')
-        try:
-            cloudinit_iso = os.path.join(self.workdir, 'cloudinit.iso')
-            self.phone_home_port = network.find_free_port()
-            cloudinit.iso(cloudinit_iso, self.name,
-                          username='root',
-                          password='password',
-                          # QEMU's hard coded usermode router address
-                          phone_home_host='10.0.2.2',
-                          phone_home_port=self.phone_home_port,
-                          authorized_key=ssh_pubkey)
-        except Exception:
-            self.cancel('Failed to prepare the cloudinit image')
-        return cloudinit_iso
-
-class BootLinux(BootLinuxBase):
-    """
-    Boots a Linux system, checking for a successful initialization
-    """
-
-    timeout = 900
-    chksum = None
-
-    def setUp(self, ssh_pubkey=None):
-        super(BootLinux, self).setUp()
-        self.vm.add_args('-smp', '2')
-        self.vm.add_args('-m', '1024')
-        self.set_up_boot()
-        self.set_up_cloudinit(ssh_pubkey)
-
-    def set_up_boot(self):
-        path = self.download_boot()
-        self.vm.add_args('-drive', 'file=%s' % path)
-
-    def set_up_cloudinit(self, ssh_pubkey=None):
-        cloudinit_iso = self.prepare_cloudinit(ssh_pubkey)
-        self.vm.add_args('-drive', 'file=%s,format=raw' % cloudinit_iso)
-
-    def launch_and_wait(self):
-        self.vm.set_console()
-        self.vm.launch()
-        console_drainer = datadrainer.LineLogger(self.vm.console_socket.fileno(),
-                                                 logger=self.log.getChild('console'))
-        console_drainer.start()
-        self.log.info('VM launched, waiting for boot confirmation from guest')
-        cloudinit.wait_for_phone_home(('0.0.0.0', self.phone_home_port), self.name)
-
-
-class BootLinuxX8664(BootLinux):
+class BootLinuxX8664(LinuxTest):
     """
     :avocado: tags=arch:x86_64
     """
@@ -154,7 +70,7 @@ class BootLinuxX8664(BootLinux):
         self.launch_and_wait()
 
 
-class BootLinuxAarch64(BootLinux):
+class BootLinuxAarch64(LinuxTest):
     """
     :avocado: tags=arch:aarch64
     :avocado: tags=machine:virt
@@ -212,7 +128,7 @@ class BootLinuxAarch64(BootLinux):
         self.launch_and_wait()
 
 
-class BootLinuxPPC64(BootLinux):
+class BootLinuxPPC64(LinuxTest):
     """
     :avocado: tags=arch:ppc64
     """
@@ -230,7 +146,7 @@ class BootLinuxPPC64(BootLinux):
         self.launch_and_wait()
 
 
-class BootLinuxS390X(BootLinux):
+class BootLinuxS390X(LinuxTest):
     """
     :avocado: tags=arch:s390x
     """
diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
index 1e745f15a2..25ea54b6ff 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -5,15 +5,13 @@ import subprocess
 import time
 
 from avocado import skipUnless
-from avocado_qemu import Test, BUILD_DIR
+from avocado_qemu import LinuxTest, BUILD_DIR
 from avocado_qemu import wait_for_console_pattern
 from avocado.utils import ssh
 
 from qemu.accel import kvm_available
 from qemu.utils import get_info_usernet_hostfwd_port
 
-from boot_linux import BootLinux
-
 
 def run_cmd(args):
     subp = subprocess.Popen(args,
@@ -72,7 +70,7 @@ def has_cmds(*cmds):
     return (True, '')
 
 
-class VirtiofsSubmountsTest(BootLinux):
+class VirtiofsSubmountsTest(LinuxTest):
     """
     :avocado: tags=arch:x86_64
     :avocado: tags=accel:kvm
-- 
2.25.4

[PATCH 15/22] Acceptance Tests: move useful ssh methods to base class

[Cleber Rosa]

Both the virtiofs submounts and the linux ssh mips malta tests
contains useful methods related to ssh that deserve to be made
available to other tests.  Let's move them to the base LinuxTest
class.

The method that helps with setting up an ssh connection will now
support both key and password based authentication, defaulting to key
based.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/avocado_qemu/__init__.py | 49 ++++++++++++++++++++++-
 tests/acceptance/linux_ssh_mips_malta.py  | 38 ++----------------
 tests/acceptance/virtiofs_submounts.py    | 36 -----------------
 3 files changed, 51 insertions(+), 72 deletions(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index b06692a59d..f0649e5011 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -10,6 +10,7 @@
 
 import logging
 import os
+import re
 import sys
 import uuid
 import tempfile
@@ -19,6 +20,7 @@ import avocado
 from avocado.utils import cloudinit
 from avocado.utils import datadrainer
 from avocado.utils import network
+from avocado.utils import ssh
 from avocado.utils import vmimage
 from avocado.utils.path import find_command
 
@@ -40,6 +42,8 @@ else:
 sys.path.append(os.path.join(SOURCE_DIR, 'python'))
 
 from qemu.machine import QEMUMachine
+from qemu.utils import get_info_usernet_hostfwd_port
+
 
 def is_readable_executable_file(path):
     return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
@@ -215,7 +219,50 @@ class Test(avocado.Test):
                         cancel_on_missing=cancel_on_missing)
 
 
-class LinuxTest(Test):
+class LinuxSSHMixIn:
+    """Contains utility methods for interacting with a guest via SSH."""
+
+    def ssh_connect(self, username, credential, credential_is_key=True):
+        self.ssh_logger = logging.getLogger('ssh')
+        res = self.vm.command('human-monitor-command',
+                              command_line='info usernet')
+        port = get_info_usernet_hostfwd_port(res)
+        self.assertIsNotNone(port)
+        self.assertGreater(port, 0)
+        self.log.debug('sshd listening on port: %d', port)
+        if credential_is_key:
+            self.ssh_session = ssh.Session('127.0.0.1', port=port,
+                                           user=username, key=credential)
+        else:
+            self.ssh_session = ssh.Session('127.0.0.1', port=port,
+                                           user=username, password=credential)
+        for i in range(10):
+            try:
+                self.ssh_session.connect()
+                return
+            except:
+                time.sleep(4)
+                pass
+        self.fail('ssh connection timeout')
+
+    def ssh_command(self, command):
+        self.ssh_logger.info(command)
+        result = self.ssh_session.cmd(command)
+        stdout_lines = [line.rstrip() for line
+                        in result.stdout_text.splitlines()]
+        for line in stdout_lines:
+            self.ssh_logger.info(line)
+        stderr_lines = [line.rstrip() for line
+                        in result.stderr_text.splitlines()]
+        for line in stderr_lines:
+            self.ssh_logger.warning(line)
+
+        self.assertEqual(result.exit_status, 0,
+                         f'Guest command failed: {command}')
+        return stdout_lines, stderr_lines
+
+
+class LinuxTest(Test, LinuxSSHMixIn):
     """Facilitates having a cloud-image Linux based available.
 
     For tests that indend to interact with guests, this is a better choice
diff --git a/tests/acceptance/linux_ssh_mips_malta.py b/tests/acceptance/linux_ssh_mips_malta.py
index 1742235758..3f590a081f 100644
--- a/tests/acceptance/linux_ssh_mips_malta.py
+++ b/tests/acceptance/linux_ssh_mips_malta.py
@@ -12,7 +12,7 @@ import logging
 import time
 
 from avocado import skipUnless
-from avocado_qemu import Test
+from avocado_qemu import Test, LinuxSSHMixIn
 from avocado_qemu import wait_for_console_pattern
 from avocado.utils import process
 from avocado.utils import archive
@@ -21,7 +21,7 @@ from avocado.utils import ssh
 from qemu.utils import get_info_usernet_hostfwd_port
 
 
-class LinuxSSH(Test):
+class LinuxSSH(Test, LinuxSSHMixIn):
 
     timeout = 150 # Not for 'configure --enable-debug --enable-debug-tcg'
 
@@ -72,41 +72,9 @@ class LinuxSSH(Test):
     def setUp(self):
         super(LinuxSSH, self).setUp()
 
-    def ssh_connect(self, username, password):
-        self.ssh_logger = logging.getLogger('ssh')
-        res = self.vm.command('human-monitor-command',
-                              command_line='info usernet')
-        port = get_info_usernet_hostfwd_port(res)
-        if not port:
-            self.cancel("Failed to retrieve SSH port")
-        self.log.debug("sshd listening on port: %d", port)
-        self.ssh_session = ssh.Session(self.VM_IP, port=port,
-                                       user=username, password=password)
-        for i in range(10):
-            try:
-                self.ssh_session.connect()
-                return
-            except:
-                time.sleep(4)
-                pass
-        self.fail("ssh connection timeout")
-
     def ssh_disconnect_vm(self):
         self.ssh_session.quit()
 
-    def ssh_command(self, command, is_root=True):
-        self.ssh_logger.info(command)
-        result = self.ssh_session.cmd(command)
-        stdout_lines = [line.rstrip() for line
-                        in result.stdout_text.splitlines()]
-        for line in stdout_lines:
-            self.ssh_logger.info(line)
-        stderr_lines = [line.rstrip() for line
-                        in result.stderr_text.splitlines()]
-        for line in stderr_lines:
-            self.ssh_logger.warning(line)
-        return stdout_lines, stderr_lines
-
     def boot_debian_wheezy_image_and_ssh_login(self, endianess, kernel_path):
         image_url, image_hash = self.get_image_info(endianess)
         image_path = self.fetch_asset(image_url, asset_hash=image_hash)
@@ -127,7 +95,7 @@ class LinuxSSH(Test):
         wait_for_console_pattern(self, console_pattern, 'Oops')
         self.log.info('sshd ready')
 
-        self.ssh_connect('root', 'root')
+        self.ssh_connect('root', 'root', False)
 
     def shutdown_via_ssh(self):
         self.ssh_command('poweroff')
diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
index 25ea54b6ff..d0fc103f72 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -10,7 +10,6 @@ from avocado_qemu import wait_for_console_pattern
 from avocado.utils import ssh
 
 from qemu.accel import kvm_available
-from qemu.utils import get_info_usernet_hostfwd_port
 
 
 def run_cmd(args):
@@ -76,41 +75,6 @@ class VirtiofsSubmountsTest(LinuxTest):
     :avocado: tags=accel:kvm
     """
 
-    def ssh_connect(self, username, keyfile):
-        self.ssh_logger = logging.getLogger('ssh')
-        res = self.vm.command('human-monitor-command',
-                              command_line='info usernet')
-        port = get_info_usernet_hostfwd_port(res)
-        self.assertIsNotNone(port)
-        self.assertGreater(port, 0)
-        self.log.debug('sshd listening on port: %d', port)
-        self.ssh_session = ssh.Session('127.0.0.1', port=port,
-                                       user=username, key=keyfile)
-        for i in range(10):
-            try:
-                self.ssh_session.connect()
-                return
-            except:
-                time.sleep(4)
-                pass
-        self.fail('ssh connection timeout')
-
-    def ssh_command(self, command):
-        self.ssh_logger.info(command)
-        result = self.ssh_session.cmd(command)
-        stdout_lines = [line.rstrip() for line
-                        in result.stdout_text.splitlines()]
-        for line in stdout_lines:
-            self.ssh_logger.info(line)
-        stderr_lines = [line.rstrip() for line
-                        in result.stderr_text.splitlines()]
-        for line in stderr_lines:
-            self.ssh_logger.warning(line)
-
-        self.assertEqual(result.exit_status, 0,
-                         f'Guest command failed: {command}')
-        return stdout_lines, stderr_lines
-
     def run(self, args, ignore_error=False):
         stdout, stderr, ret = run_cmd(args)
 
-- 
2.25.4

[PATCH 16/22] Acceptance Tests: introduce method for requiring an accelerator

[Cleber Rosa]

Some tests explicitly require a QEMU accelerator to be available.
Given that this depends on some runtime aspects not known before
the test is started, such as the currently set QEMU binary, it's
left to be checked also at runtime.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/avocado_qemu/__init__.py | 24 ++++++++++++++++
 tests/acceptance/boot_linux.py            | 34 ++++++-----------------
 tests/acceptance/virtiofs_submounts.py    |  5 +---
 3 files changed, 34 insertions(+), 29 deletions(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index f0649e5011..472088ae7d 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -41,6 +41,8 @@ else:
 
 sys.path.append(os.path.join(SOURCE_DIR, 'python'))
 
+from qemu.accel import kvm_available
+from qemu.accel import tcg_available
 from qemu.machine import QEMUMachine
 from qemu.utils import get_info_usernet_hostfwd_port
 
@@ -166,6 +168,28 @@ class Test(avocado.Test):
             return vals.pop()
         return None
 
+    def require_accelerator(self, accelerator):
+        """
+        Requires an accelerator to be available for the test to continue
+
+        It takes into account the currently set qemu binary.
+
+        If the check fails, the test is canceled.  If the check itself
+        for the given accelerator is not available, the test is also
+        canceled.
+
+        :param accelerator: name of the accelerator, such as "kvm" or "tcg"
+        :type accelerator: str
+        """
+        checker = {'tcg': tcg_available,
+                   'kvm': kvm_available}.get(accelerator)
+        if checker is None:
+            self.cancel("Don't know how to check for the presence "
+                        "of accelerator %s" % accelerator)
+        if not checker(qemu_bin=self.qemu_bin):
+            self.cancel("%s accelerator does not seem to be "
+                        "available" % accelerator)
+
     def setUp(self):
         self._vms = {}
 
diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
index 14e89d020d..0d178038a0 100644
--- a/tests/acceptance/boot_linux.py
+++ b/tests/acceptance/boot_linux.py
@@ -12,15 +12,8 @@ import os
 
 from avocado_qemu import LinuxTest, BUILD_DIR
 
-from qemu.accel import kvm_available
-from qemu.accel import tcg_available
-
 from avocado import skipIf
 
-ACCEL_NOT_AVAILABLE_FMT = "%s accelerator does not seem to be available"
-KVM_NOT_AVAILABLE = ACCEL_NOT_AVAILABLE_FMT % "KVM"
-TCG_NOT_AVAILABLE = ACCEL_NOT_AVAILABLE_FMT % "TCG"
-
 
 class BootLinuxX8664(LinuxTest):
     """
@@ -34,8 +27,7 @@ class BootLinuxX8664(LinuxTest):
         :avocado: tags=machine:pc
         :avocado: tags=accel:tcg
         """
-        if not tcg_available(self.qemu_bin):
-            self.cancel(TCG_NOT_AVAILABLE)
+        self.require_accelerator("tcg")
         self.vm.add_args("-accel", "tcg")
         self.launch_and_wait()
 
@@ -44,8 +36,7 @@ class BootLinuxX8664(LinuxTest):
         :avocado: tags=machine:pc
         :avocado: tags=accel:kvm
         """
-        if not kvm_available(self.arch, self.qemu_bin):
-            self.cancel(KVM_NOT_AVAILABLE)
+        self.require_accelerator("kvm")
         self.vm.add_args("-accel", "kvm")
         self.launch_and_wait()
 
@@ -54,8 +45,7 @@ class BootLinuxX8664(LinuxTest):
         :avocado: tags=machine:q35
         :avocado: tags=accel:tcg
         """
-        if not tcg_available(self.qemu_bin):
-            self.cancel(TCG_NOT_AVAILABLE)
+        self.require_accelerator("tcg")
         self.vm.add_args("-accel", "tcg")
         self.launch_and_wait()
 
@@ -64,8 +54,7 @@ class BootLinuxX8664(LinuxTest):
         :avocado: tags=machine:q35
         :avocado: tags=accel:kvm
         """
-        if not kvm_available(self.arch, self.qemu_bin):
-            self.cancel(KVM_NOT_AVAILABLE)
+        self.require_accelerator("kvm")
         self.vm.add_args("-accel", "kvm")
         self.launch_and_wait()
 
@@ -91,8 +80,7 @@ class BootLinuxAarch64(LinuxTest):
         :avocado: tags=accel:tcg
         :avocado: tags=cpu:max
         """
-        if not tcg_available(self.qemu_bin):
-            self.cancel(TCG_NOT_AVAILABLE)
+        self.require_accelerator("tcg")
         self.vm.add_args("-accel", "tcg")
         self.vm.add_args("-cpu", "max")
         self.vm.add_args("-machine", "virt,gic-version=2")
@@ -105,8 +93,7 @@ class BootLinuxAarch64(LinuxTest):
         :avocado: tags=cpu:host
         :avocado: tags=device:gicv2
         """
-        if not kvm_available(self.arch, self.qemu_bin):
-            self.cancel(KVM_NOT_AVAILABLE)
+        self.require_accelerator("kvm")
         self.vm.add_args("-accel", "kvm")
         self.vm.add_args("-cpu", "host")
         self.vm.add_args("-machine", "virt,gic-version=2")
@@ -119,8 +106,7 @@ class BootLinuxAarch64(LinuxTest):
         :avocado: tags=cpu:host
         :avocado: tags=device:gicv3
         """
-        if not kvm_available(self.arch, self.qemu_bin):
-            self.cancel(KVM_NOT_AVAILABLE)
+        self.require_accelerator("kvm")
         self.vm.add_args("-accel", "kvm")
         self.vm.add_args("-cpu", "host")
         self.vm.add_args("-machine", "virt,gic-version=3")
@@ -140,8 +126,7 @@ class BootLinuxPPC64(LinuxTest):
         :avocado: tags=machine:pseries
         :avocado: tags=accel:tcg
         """
-        if not tcg_available(self.qemu_bin):
-            self.cancel(TCG_NOT_AVAILABLE)
+        self.require_accelerator("tcg")
         self.vm.add_args("-accel", "tcg")
         self.launch_and_wait()
 
@@ -159,7 +144,6 @@ class BootLinuxS390X(LinuxTest):
         :avocado: tags=machine:s390-ccw-virtio
         :avocado: tags=accel:tcg
         """
-        if not tcg_available(self.qemu_bin):
-            self.cancel(TCG_NOT_AVAILABLE)
+        self.require_accelerator("tcg")
         self.vm.add_args("-accel", "tcg")
         self.launch_and_wait()
diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
index d0fc103f72..0298807e5c 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -9,8 +9,6 @@ from avocado_qemu import LinuxTest, BUILD_DIR
 from avocado_qemu import wait_for_console_pattern
 from avocado.utils import ssh
 
-from qemu.accel import kvm_available
-
 
 def run_cmd(args):
     subp = subprocess.Popen(args,
@@ -201,8 +199,7 @@ class VirtiofsSubmountsTest(LinuxTest):
         self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22',
                          '-device', 'virtio-net,netdev=vnet')
 
-        if not kvm_available(self.arch, self.qemu_bin):
-            self.cancel(KVM_NOT_AVAILABLE)
+        self.require_accelerator("kvm")
         self.vm.add_args('-accel', 'kvm')
 
     def tearDown(self):
-- 
2.25.4

[PATCH 17/22] Acceptance Tests: fix population of public key in cloudinit image

[Cleber Rosa]

Currently the path of the ssh public key is being set, but its
content is obviously what's needed.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/avocado_qemu/__init__.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index 472088ae7d..8156224625 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -337,13 +337,15 @@ class LinuxTest(Test, LinuxSSHMixIn):
         try:
             cloudinit_iso = os.path.join(self.workdir, 'cloudinit.iso')
             self.phone_home_port = network.find_free_port()
+            with open(ssh_pubkey) as pubkey:
+                pubkey_content = pubkey.read()
             cloudinit.iso(cloudinit_iso, self.name,
                           username='root',
                           password='password',
                           # QEMU's hard coded usermode router address
                           phone_home_host='10.0.2.2',
                           phone_home_port=self.phone_home_port,
-                          authorized_key=ssh_pubkey)
+                          authorized_key=pubkey_content)
         except Exception:
             self.cancel('Failed to prepare the cloudinit image')
         return cloudinit_iso
-- 
2.25.4

[PATCH 18/22] Acceptance Tests: set up existing ssh keys by default

[Cleber Rosa]

It's questionable wether it's necessary to create one brand new pair
for each test.  It's not questionable that it takes less time and
resources to just use the keys available at "tests/keys" that exist
for that exact reason.

If a location for the public key is not given explicitly, the
LinuxTest will now set up the existing pair of keys as the default.
This removes the need for a lot of boiler plate code.

To avoid the ssh client from erroring on permission issues, a
directory with restricive permissions is created for the private key.
This should still be a lot cheaper than creating a new key.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/avocado_qemu/__init__.py | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index 8156224625..5f4dd6b9ec 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -11,6 +11,7 @@
 import logging
 import os
 import re
+import shutil
 import sys
 import uuid
 import tempfile
@@ -301,8 +302,21 @@ class LinuxTest(Test, LinuxSSHMixIn):
         self.vm.add_args('-smp', '2')
         self.vm.add_args('-m', '1024')
         self.set_up_boot()
+        if ssh_pubkey is None:
+            ssh_pubkey, self.ssh_key = self.set_up_existing_ssh_keys()
         self.set_up_cloudinit(ssh_pubkey)
 
+    def set_up_existing_ssh_keys(self):
+        ssh_public_key = os.path.join(SOURCE_DIR, 'tests', 'keys', 'id_rsa.pub')
+        source_private_key = os.path.join(SOURCE_DIR, 'tests', 'keys', 'id_rsa')
+        ssh_dir = os.path.join(self.workdir, '.ssh')
+        os.mkdir(ssh_dir, mode=0o700)
+        ssh_private_key = os.path.join(ssh_dir,
+                                       os.path.basename(source_private_key))
+        shutil.copyfile(source_private_key, ssh_private_key)
+        os.chmod(ssh_private_key, 0o600)
+        return (ssh_public_key, ssh_private_key)
+
     def download_boot(self):
         self.log.debug('Looking for and selecting a qemu-img binary to be '
                        'used to create the bootable snapshot image')
-- 
2.25.4

[PATCH 19/22] Acceptance Tests: add port redirection for ssh by default

[Cleber Rosa]

For users of the LinuxTest class, let's set up the VM with the port
redirection for SSH, instead of requiring each test to set the same
arguments.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/avocado_qemu/__init__.py | 2 ++
 tests/acceptance/virtiofs_submounts.py    | 4 ----
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index 5f4dd6b9ec..89669cc604 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -301,6 +301,8 @@ class LinuxTest(Test, LinuxSSHMixIn):
         super(LinuxTest, self).setUp()
         self.vm.add_args('-smp', '2')
         self.vm.add_args('-m', '1024')
+        self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22',
+                         '-device', 'virtio-net,netdev=vnet')
         self.set_up_boot()
         if ssh_pubkey is None:
             ssh_pubkey, self.ssh_key = self.set_up_existing_ssh_keys()
diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
index 0298807e5c..94b59edd6d 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -195,10 +195,6 @@ class VirtiofsSubmountsTest(LinuxTest):
             self.vm.add_args('-kernel', vmlinuz,
                              '-append', 'console=ttyS0 root=/dev/sda1')
 
-        # Allow us to connect to SSH
-        self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22',
-                         '-device', 'virtio-net,netdev=vnet')
-
         self.require_accelerator("kvm")
         self.vm.add_args('-accel', 'kvm')
 
-- 
2.25.4

[PATCH 20/22] Acceptance Tests: add basic documentation on LinuxTest base class

[Cleber Rosa]

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 docs/devel/testing.rst | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 209f9d8172..fe0112b21c 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -790,6 +790,32 @@ and hypothetical example follows:
 At test "tear down", ``avocado_qemu.Test`` handles all the QEMUMachines
 shutdown.
 
+The ``avocado_qemu.LinuxTest`` base test class
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The ``avocado_qemu.LinuxTest`` is further specialization of the
+``avocado_qemu.Test`` class, so it contains all the characteristics of
+the later plus some extra features.
+
+First of all, this base class is intended for tests that need to
+interact with a fully booted and operational Linux guest.  The most
+basic example looks like this:
+
+.. code::
+
+  from avocado_qemu import LinuxTest
+
+
+  class SomeTest(LinuxTest):
+
+      def test(self):
+          self.launch_and_wait()
+          self.ssh_connect('root', self.ssh_key)
+          self.ssh_command('some_command_to_be_run_in_the_guest')
+
+Please refer to tests that use ``avocado_qemu.LinuxTest`` under
+``tests/acceptance`` for more examples.
+
 QEMUMachine
 ~~~~~~~~~~~
 
-- 
2.25.4

[PATCH 21/22] Acceptance Tests: introduce CPU hotplug test

[Cleber Rosa]

Even though there are qtest based tests for hotplugging CPUs (from
which this test took some inspiration from), this one adds checks
from a Linux guest point of view.

It should also serve as an example for tests that follow a similar
pattern and need to interact with QEMU (via qmp) and with the Linux
guest via SSH.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/hotplug_cpu.py | 38 +++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 tests/acceptance/hotplug_cpu.py

diff --git a/tests/acceptance/hotplug_cpu.py b/tests/acceptance/hotplug_cpu.py
new file mode 100644
index 0000000000..a22b264b4b
--- /dev/null
+++ b/tests/acceptance/hotplug_cpu.py
@@ -0,0 +1,38 @@
+# Functional test that hotplugs a CPU and checks it on a Linux guest
+#
+# Copyright (c) 2021 Red Hat, Inc.
+#
+# Author:
+#  Cleber Rosa <crosa@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 avocado_qemu import LinuxTest
+
+
+class HotPlugCPU(LinuxTest):
+
+    def test(self):
+        """
+        :avocado: tags=arch:x86_64
+        :avocado: tags=machine:q35
+        :avocado: tags=accel:kvm
+        """
+        self.require_accelerator('kvm')
+        self.vm.add_args('-accel', 'kvm')
+        self.vm.add_args('-cpu', 'Haswell')
+        self.vm.add_args('-smp', '1,sockets=1,cores=2,threads=1,maxcpus=2')
+        self.launch_and_wait()
+
+        self.ssh_connect('root', self.ssh_key)
+        self.ssh_command('test -e /sys/devices/system/cpu/cpu0')
+        with self.assertRaises(AssertionError):
+            self.ssh_command('test -e /sys/devices/system/cpu/cpu1')
+
+        self.vm.command('device_add',
+                        driver='Haswell-x86_64-cpu',
+                        socket_id=0,
+                        core_id=1,
+                        thread_id=0)
+        self.ssh_command('test -e /sys/devices/system/cpu/cpu1')
-- 
2.25.4

[PATCH 22/22] [NOTFORMERGE] Bump Avocado version to latest master

[Cleber Rosa]

This is intende to be replaced by a bump to Avocado 85.0, to be
released by Feb 8 2021.

Latest master contains an improvement to "avocado.utils.vmimage" that
let's it download older Fedora images from the archive locations.
That allows the currently set Fedora 31 images to continue being used.

Reference: https://github.com/avocado-framework/avocado/milestone/11
Reference: https://github.com/avocado-framework/avocado/commit/e4d67e96ee9563436dcf7dd83902723576657d29
Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/requirements.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/requirements.txt b/tests/requirements.txt
index 62e8ffd28c..8ddf0d8c86 100644
--- a/tests/requirements.txt
+++ b/tests/requirements.txt
@@ -1,5 +1,5 @@
 # Add Python module requirements, one per line, to be installed
 # in the tests/venv Python virtual environment. For more info,
 # refer to: https://pip.pypa.io/en/stable/user_guide/#id1
-avocado-framework==83.0
+git+https://github.com/avocado-framework/avocado@9b372adeec6dceaba276ca4729f2e5a2fcccaa4c#egg=avocado_framework
 pycdlib==1.11.0
-- 
2.25.4

Re: [PATCH 01/22] tests/acceptance/boot_linux.py: fix typo on cloudinit error message

[Philippe Mathieu-Daudé]

On 2/3/21 6:23 PM, Cleber Rosa wrote:
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/boot_linux.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 03/22] Acceptance Tests: remove unnecessary tag from documentation example

[Philippe Mathieu-Daudé]

On 2/3/21 6:23 PM, Cleber Rosa wrote:
> The ":avocado: enable" is not necessary and was removed in 9531d26c,
> so let's remove from the docs.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  docs/devel/testing.rst | 3 ---
>  1 file changed, 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 12/22] Acceptance tests: clarify ssh connection failure reason

[Philippe Mathieu-Daudé]

On 2/3/21 6:23 PM, Cleber Rosa wrote:
> If the connection to the ssh server fails, it may indeed be a "sshd"
> issue, but it may also not be that.  Let's state what we know: the
> establishment of the connection from the client side was not possible.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/linux_ssh_mips_malta.py | 2 +-
>  tests/acceptance/virtiofs_submounts.py   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 19/22] Acceptance Tests: add port redirection for ssh by default

[Philippe Mathieu-Daudé]

On 2/3/21 6:23 PM, Cleber Rosa wrote:
> For users of the LinuxTest class, let's set up the VM with the port
> redirection for SSH, instead of requiring each test to set the same
> arguments.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/avocado_qemu/__init__.py | 2 ++
>  tests/acceptance/virtiofs_submounts.py    | 4 ----
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index 5f4dd6b9ec..89669cc604 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -301,6 +301,8 @@ class LinuxTest(Test, LinuxSSHMixIn):
>          super(LinuxTest, self).setUp()
>          self.vm.add_args('-smp', '2')
>          self.vm.add_args('-m', '1024')
> +        self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22',
> +                         '-device', 'virtio-net,netdev=vnet')

You need a machine with a virtio-bus to be able to plug a virtio device,
not all provide one.

Re: [PATCH 19/22] Acceptance Tests: add port redirection for ssh by default

[Philippe Mathieu-Daudé]

On 2/3/21 6:46 PM, Philippe Mathieu-Daudé wrote:
> On 2/3/21 6:23 PM, Cleber Rosa wrote:
>> For users of the LinuxTest class, let's set up the VM with the port
>> redirection for SSH, instead of requiring each test to set the same
>> arguments.
>>
>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>> ---
>>  tests/acceptance/avocado_qemu/__init__.py | 2 ++
>>  tests/acceptance/virtiofs_submounts.py    | 4 ----
>>  2 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
>> index 5f4dd6b9ec..89669cc604 100644
>> --- a/tests/acceptance/avocado_qemu/__init__.py
>> +++ b/tests/acceptance/avocado_qemu/__init__.py
>> @@ -301,6 +301,8 @@ class LinuxTest(Test, LinuxSSHMixIn):
>>          super(LinuxTest, self).setUp()
>>          self.vm.add_args('-smp', '2')
>>          self.vm.add_args('-m', '1024')
>> +        self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22',
>> +                         '-device', 'virtio-net,netdev=vnet')
> 
> You need a machine with a virtio-bus to be able to plug a virtio device,
> not all provide one.

Alternatively you could provide a network_device_type argument to
setUp() which has to be explicitly set (since the tests would be
pointless without network access).

Re: [PATCH 02/22] tests/acceptance/boot_linux.py: rename misleading cloudinit method

[Thomas Huth]

On 03/02/2021 18.23, Cleber Rosa wrote:
> There's no downloading happening on that method, so let's call it
> "prepare" instead.  While at it, and because of it, the current
> "prepare_boot" and "prepare_cloudinit" are also renamed.
> 
> The reasoning here is that "prepare_" methods will just work on the
> images, while "set_up_" will make them effective to the VM that will
> be launched.  Inspiration comes from the "virtiofs_submounts.py"
> tests, which this expects to converge more into.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/acceptance/boot_linux.py | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)

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

Re: [PATCH 01/22] tests/acceptance/boot_linux.py: fix typo on cloudinit error message

[Alex Bennée]

Cleber Rosa <crosa@redhat.com> writes:

> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH 01/22] tests/acceptance/boot_linux.py: fix typo on cloudinit error message

[Beraldo Leal]

On Wed, Feb 03, 2021 at 12:23:36PM -0500, Cleber Rosa wrote:
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/boot_linux.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Beraldo Leal <bleal@redhat.com>

Re: [PATCH 02/22] tests/acceptance/boot_linux.py: rename misleading cloudinit method

[Alex Bennée]

Cleber Rosa <crosa@redhat.com> writes:

> There's no downloading happening on that method, so let's call it
> "prepare" instead.  While at it, and because of it, the current
> "prepare_boot" and "prepare_cloudinit" are also renamed.
>
> The reasoning here is that "prepare_" methods will just work on the
> images, while "set_up_" will make them effective to the VM that will
> be launched.  Inspiration comes from the "virtiofs_submounts.py"
> tests, which this expects to converge more into.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH 03/22] Acceptance Tests: remove unnecessary tag from documentation example

[Alex Bennée]

Cleber Rosa <crosa@redhat.com> writes:

> The ":avocado: enable" is not necessary and was removed in 9531d26c,
> so let's remove from the docs.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH 02/22] tests/acceptance/boot_linux.py: rename misleading cloudinit method

[Beraldo Leal]

On Wed, Feb 03, 2021 at 12:23:37PM -0500, Cleber Rosa wrote:
> There's no downloading happening on that method, so let's call it
> "prepare" instead.  While at it, and because of it, the current
> "prepare_boot" and "prepare_cloudinit" are also renamed.
> 
> The reasoning here is that "prepare_" methods will just work on the
> images, while "set_up_" will make them effective to the VM that will
> be launched.  Inspiration comes from the "virtiofs_submounts.py"
> tests, which this expects to converge more into.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/boot_linux.py | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
> index 2ac3e57587..bcd923bb4a 100644
> --- a/tests/acceptance/boot_linux.py
> +++ b/tests/acceptance/boot_linux.py
> @@ -57,7 +57,7 @@ class BootLinuxBase(Test):
>              self.cancel('Failed to download/prepare boot image')
>          return boot.path
>  
> -    def download_cloudinit(self, ssh_pubkey=None):
> +    def prepare_cloudinit(self, ssh_pubkey=None):
>          self.log.info('Preparing cloudinit image')
>          try:
>              cloudinit_iso = os.path.join(self.workdir, 'cloudinit.iso')
> @@ -85,15 +85,15 @@ class BootLinux(BootLinuxBase):
>          super(BootLinux, self).setUp()
>          self.vm.add_args('-smp', '2')
>          self.vm.add_args('-m', '1024')
> -        self.prepare_boot()
> -        self.prepare_cloudinit(ssh_pubkey)
> +        self.set_up_boot()
> +        self.set_up_cloudinit(ssh_pubkey)
>  
> -    def prepare_boot(self):
> +    def set_up_boot(self):
>          path = self.download_boot()
>          self.vm.add_args('-drive', 'file=%s' % path)
>  
> -    def prepare_cloudinit(self, ssh_pubkey=None):
> -        cloudinit_iso = self.download_cloudinit(ssh_pubkey)
> +    def set_up_cloudinit(self, ssh_pubkey=None):
> +        cloudinit_iso = self.prepare_cloudinit(ssh_pubkey)
>          self.vm.add_args('-drive', 'file=%s,format=raw' % cloudinit_iso)
>  
>      def launch_and_wait(self):
> -- 
> 2.25.4
>

Reviewed-by: Beraldo Leal <bleal@redhat.com>

Re: [PATCH 04/22] tests/acceptance/virtiofs_submounts.py: use workdir property

[Alex Bennée]

Cleber Rosa <crosa@redhat.com> writes:

> For Avocado Instrumented based tests, it's a better idea to just use
> the property.  The environment variable is a fall back for tests not
> written using that Python API.
>
> Reference: https://avocado-framework.readthedocs.io/en/84.0/api/test/avocado.html#avocado.Test.workdir
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH 05/22] tests/acceptance/virtiofs_submounts.py: do not ask for ssh key password

[Alex Bennée]

Cleber Rosa <crosa@redhat.com> writes:

> Tests are supposed to be non-interactive, and ssh-keygen is asking for
> a password when creating a key.  Let's set an empty key, which prevents
> ssh-keygen for asking for a password.

We are not generating an empty key, just one that isn't protected by a
passphrase. So:

  Tests are supposed to be non-interactive, and ssh-keygen is asking for
  a passphrase when creating a key.  Let's set an empty passphrase to
  avoid the prompt.

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH 04/22] tests/acceptance/virtiofs_submounts.py: use workdir property

[Beraldo Leal]

On Wed, Feb 03, 2021 at 12:23:39PM -0500, Cleber Rosa wrote:
> For Avocado Instrumented based tests, it's a better idea to just use
> the property.  The environment variable is a fall back for tests not
> written using that Python API.
> 
> Reference: https://avocado-framework.readthedocs.io/en/84.0/api/test/avocado.html#avocado.Test.workdir
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/virtiofs_submounts.py | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
> index 361e5990b6..68d3cd6869 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -136,8 +136,7 @@ class VirtiofsSubmountsTest(BootLinux):
>          return (stdout, stderr, ret)
>  
>      def set_up_shared_dir(self):
> -        atwd = os.getenv('AVOCADO_TEST_WORKDIR')
> -        self.shared_dir = os.path.join(atwd, 'virtiofs-shared')
> +        self.shared_dir = os.path.join(self.workdir, 'virtiofs-shared')
>  
>          os.mkdir(self.shared_dir)
>  
> @@ -234,8 +233,7 @@ class VirtiofsSubmountsTest(BootLinux):
>  
>          self.seed = self.params.get('seed')
>  
> -        atwd = os.getenv('AVOCADO_TEST_WORKDIR')
> -        self.ssh_key = os.path.join(atwd, 'id_ed25519')
> +        self.ssh_key = os.path.join(self.workdir, 'id_ed25519')
>  
>          self.run(('ssh-keygen', '-t', 'ed25519', '-f', self.ssh_key))
>  
> -- 
> 2.25.4
>

Reviewed-by: Beraldo Leal <bleal@redhat.com>

Re: [PATCH 05/22] tests/acceptance/virtiofs_submounts.py: do not ask for ssh key password

[Beraldo Leal]

On Wed, Feb 03, 2021 at 12:23:40PM -0500, Cleber Rosa wrote:
> Tests are supposed to be non-interactive, and ssh-keygen is asking for
> a password when creating a key.  Let's set an empty key, which prevents
> ssh-keygen for asking for a password.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/virtiofs_submounts.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
> index 68d3cd6869..3b5a242385 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -235,7 +235,7 @@ class VirtiofsSubmountsTest(BootLinux):
>  
>          self.ssh_key = os.path.join(self.workdir, 'id_ed25519')
>  
> -        self.run(('ssh-keygen', '-t', 'ed25519', '-f', self.ssh_key))
> +        self.run(('ssh-keygen', '-N', '', '-t', 'ed25519', '-f', self.ssh_key))
>  
>          pubkey = open(self.ssh_key + '.pub').read()
>  
> -- 
> 2.25.4
> 

Reviewed-by: Beraldo Leal <bleal@redhat.com>

Re: [PATCH 07/22] tests/acceptance/virtiofs_submounts.py: evaluate string not length

[Beraldo Leal]

On Wed, Feb 03, 2021 at 12:23:42PM -0500, Cleber Rosa wrote:
> If the vmlinuz variable is set to anything that evaluates to True,
> then the respective arguments should be set.  If the variable contains
> an empty string, than it will evaluate to False, and the extra
> arguments will not be set.
> 
> This keeps the same logic, but improves readability a bit.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/virtiofs_submounts.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
> index f1b49f03bb..f25a386a19 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -241,7 +241,7 @@ class VirtiofsSubmountsTest(BootLinux):
>  
>          super(VirtiofsSubmountsTest, self).setUp(pubkey)
>  
> -        if len(vmlinuz) > 0:
> +        if vmlinuz:
>              self.vm.add_args('-kernel', vmlinuz,
>                               '-append', 'console=ttyS0 root=/dev/sda1')
>  
> -- 
> 2.25.4
>

Reviewed-by: Beraldo Leal <bleal@redhat.com>

Re: [PATCH 08/22] tests/acceptance/virtiofs_submounts.py: standardize port as integer

[Beraldo Leal]

On Wed, Feb 03, 2021 at 12:23:43PM -0500, Cleber Rosa wrote:
> Instead of having to cast it whenever it's going to be used, let's
> standardize it as an integer, which is the data type that will be
> used most often.
> 
> Given that the regex will only match digits, it's safe that we'll
> end up getting a integer, but, it could as well be a zero.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/virtiofs_submounts.py | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
> index f25a386a19..227a3cf1ee 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -86,17 +86,18 @@ class VirtiofsSubmountsTest(BootLinux):
>                  re.search(r'TCP.HOST_FORWARD.*127\.0\.0\.1\s*(\d+)\s+10\.',
>                            line)
>              if match is not None:
> -                port = match[1]
> +                port = int(match[1])
>                  break
>  
>          self.assertIsNotNone(port)
> -        self.log.debug('sshd listening on port: ' + port)
> +        self.assertGreater(port, 0)
> +        self.log.debug('sshd listening on port: %d', port)
>          return port
>  
>      def ssh_connect(self, username, keyfile):
>          self.ssh_logger = logging.getLogger('ssh')
>          port = self.get_portfwd()
> -        self.ssh_session = ssh.Session('127.0.0.1', port=int(port),
> +        self.ssh_session = ssh.Session('127.0.0.1', port=port,
>                                         user=username, key=keyfile)
>          for i in range(10):
>              try:
> -- 
> 2.25.4
>

Reviewed-by: Beraldo Leal <bleal@redhat.com>

Re: [PATCH 16/22] Acceptance Tests: introduce method for requiring an accelerator

[Beraldo Leal]

On Wed, Feb 03, 2021 at 12:23:51PM -0500, Cleber Rosa wrote:
> Some tests explicitly require a QEMU accelerator to be available.
> Given that this depends on some runtime aspects not known before
> the test is started, such as the currently set QEMU binary, it's
> left to be checked also at runtime.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/avocado_qemu/__init__.py | 24 ++++++++++++++++
>  tests/acceptance/boot_linux.py            | 34 ++++++-----------------
>  tests/acceptance/virtiofs_submounts.py    |  5 +---
>  3 files changed, 34 insertions(+), 29 deletions(-)

Reviewed-by: Beraldo Leal <bleal@redhat.com>

Re: [PATCH 06/22] tests/acceptance/virtiofs_submounts.py: use a virtio-net device instead

[Alex Bennée]

Cleber Rosa <crosa@redhat.com> writes:

> In a virtiofs based tests, it seems safe to assume that the guest will
> be capable of a virtio-net device.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH 07/22] tests/acceptance/virtiofs_submounts.py: evaluate string not length

[Alex Bennée]

Cleber Rosa <crosa@redhat.com> writes:

> If the vmlinuz variable is set to anything that evaluates to True,
> then the respective arguments should be set.  If the variable contains
> an empty string, than it will evaluate to False, and the extra
> arguments will not be set.
>
> This keeps the same logic, but improves readability a bit.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/virtiofs_submounts.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
> index f1b49f03bb..f25a386a19 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -241,7 +241,7 @@ class VirtiofsSubmountsTest(BootLinux):
>  
>          super(VirtiofsSubmountsTest, self).setUp(pubkey)
>  
> -        if len(vmlinuz) > 0:
> +        if vmlinuz:
>              self.vm.add_args('-kernel', vmlinuz,
>                               '-append', 'console=ttyS0 root=/dev/sda1')

And this is were I gave up because I can't see how to run the test:

  ./tests/venv/bin/avocado run ./tests/acceptance/virtiofs_submounts.py
  JOB ID     : b3d9bfcfcd603189a471bec7d770fca3b50a81ee
  JOB LOG    : /home/alex/avocado/job-results/job-2021-02-04T13.21-b3d9bfc/job.log
   (1/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_pre_virtiofsd_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
   (2/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_pre_launch_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
   (3/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_post_launch_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary
  to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
   (4/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_post_mount_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
   (5/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_two_runs: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
  RESULTS    : PASS 0 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 5
  JOB TIME   : 0.56 s

Given the test seems to make assumptions about an environment being
setup for it I think we need some documentation somewhere about what
those pre-requisites are. FWIW I also had the following locally applied
to workaround the fact the losetup and mkfs.xfs binaries aren't visible
to normal users.

modified   tests/acceptance/virtiofs_submounts.py
@@ -173,7 +173,10 @@ class VirtiofsSubmountsTest(LinuxTest):
         self.run(('bash', self.get_data('cleanup.sh'), scratch_dir))
 
     @skipUnless(*has_cmds(('sudo -n', ('sudo', '-n', 'true')),
-                          'ssh-keygen', 'bash', 'losetup', 'mkfs.xfs', 'mount'))
+                          'ssh-keygen', 'bash',
+                          ('losetup', ('sudo', '-n', 'losetup')),
+                          ('mkfs.xfs', ('sudo', '-n', 'which', 'mkfs.xfs')),
+                          'mount'))
     def setUp(self):
         vmlinuz = self.params.get('vmlinuz')
         if vmlinuz is None:

-- 
Alex Bennée

Re: [PATCH 10/22] Python: add utility function for retrieving port redirection

[John Snow]

On 2/3/21 12:23 PM, Cleber Rosa wrote:
> Slightly different versions for the same utility code are currently
> present on different locations.  This unifies them all, giving
> preference to the version from virtiofs_submounts.py, because of the
> last tweaks added to it.
> 
> While at it, this adds a "qemu.util" module to host the utility
> function and a test.
> 

qemu.utils, with the s!

And, OK, this makes me feel less weird about creating a qemu.utils 
package, actually.

ACK (but not reviewed, tested or linted) for the new code heading into 
python/qemu/.

--js

> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   python/qemu/utils.py                     | 35 ++++++++++++++++++++++++
>   tests/acceptance/info_usernet.py         | 29 ++++++++++++++++++++
>   tests/acceptance/linux_ssh_mips_malta.py | 16 +++++------
>   tests/acceptance/virtiofs_submounts.py   | 20 +++-----------
>   tests/vm/basevm.py                       |  7 ++---
>   5 files changed, 77 insertions(+), 30 deletions(-)
>   create mode 100644 python/qemu/utils.py
>   create mode 100644 tests/acceptance/info_usernet.py
> 
> diff --git a/python/qemu/utils.py b/python/qemu/utils.py
> new file mode 100644
> index 0000000000..89a246ab30
> --- /dev/null
> +++ b/python/qemu/utils.py
> @@ -0,0 +1,35 @@
> +"""
> +QEMU utility library
> +
> +This offers miscellaneous utility functions, which may not be easily
> +distinguishable or numerous to be in their own module.
> +"""
> +
> +# Copyright (C) 2021 Red Hat Inc.
> +#
> +# Authors:
> +#  Cleber Rosa <crosa@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2.  See
> +# the COPYING file in the top-level directory.
> +#
> +
> +import re
> +from typing import Optional
> +
> +
> +def get_info_usernet_hostfwd_port(info_usernet_output: str) -> Optional[int]:
> +    """
> +    Returns the port given to the hostfwd parameter via info usernet
> +
> +    :param info_usernet_output: output generated by hmp command "info usernet"
> +    :param info_usernet_output: str
> +    :return: the port number allocated by the hostfwd option
> +    :rtype: int

We can probably delete the :rtype: field, can't we? Annotations should 
handle this information now.

(Unless some tool needs it for compatibility reasons, but we don't use 
this field anywhere else in ./python/ yet.)

Re: [PATCH 09/22] tests/acceptance/virtiofs_submounts.py: required space between IP and port

[Philippe Mathieu-Daudé]

On 2/3/21 6:23 PM, Cleber Rosa wrote:
> AFAICT, there should not be a situation where IP and port do not have
> at least one whitespace character separating them.
> 
> This may be true for other '\s*' patterns in the same regex too.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/virtiofs_submounts.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 11/22] tests/acceptance/linux_ssh_mips_malta.py: standardize port as integer

[Philippe Mathieu-Daudé]

On 2/3/21 6:23 PM, Cleber Rosa wrote:
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/linux_ssh_mips_malta.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 13/22] tests/acceptance/virtiofs_submounts.py: add missing accel tag

[Philippe Mathieu-Daudé]

On 2/3/21 6:23 PM, Cleber Rosa wrote:
> Which is useful to select tests that depend/use a particular feature.

Is that a question?

Why keep this last in your series?

> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/virtiofs_submounts.py | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
> index c998b297ee..1e745f15a2 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -75,6 +75,7 @@ def has_cmds(*cmds):
>  class VirtiofsSubmountsTest(BootLinux):
>      """
>      :avocado: tags=arch:x86_64
> +    :avocado: tags=accel:kvm
>      """
>  
>      def ssh_connect(self, username, keyfile):
> 

Re: [PATCH 00/22] Acceptance Test: introduce base class for Linux based tests

[Philippe Mathieu-Daudé]

On 2/3/21 6:23 PM, Cleber Rosa wrote:
> This introduces a base class for tests that need to interact with a
> Linux guest.  It generalizes the "boot_linux.py" code, already been
> used by the "virtiofs_submounts.py" and also SSH related code being
> used by that and "linux_ssh_mips_malta.py".
> 
> While at it, a number of fixes on hopeful improvements to those tests
> were added.
> 
> Cleber Rosa (22):
>   tests/acceptance/boot_linux.py: fix typo on cloudinit error message
>   tests/acceptance/boot_linux.py: rename misleading cloudinit method
>   Acceptance Tests: remove unnecessary tag from documentation example
>   tests/acceptance/virtiofs_submounts.py: use workdir property
>   tests/acceptance/virtiofs_submounts.py: do not ask for ssh key
>     password
>   tests/acceptance/virtiofs_submounts.py: use a virtio-net device
>     instead
>   tests/acceptance/virtiofs_submounts.py: evaluate string not length
>   tests/acceptance/virtiofs_submounts.py: standardize port as integer
>   tests/acceptance/virtiofs_submounts.py: required space between IP and
>     port
>   Python: add utility function for retrieving port redirection
>   tests/acceptance/linux_ssh_mips_malta.py: standardize port as integer
>   Acceptance tests: clarify ssh connection failure reason
>   tests/acceptance/virtiofs_submounts.py: add missing accel tag
>   Acceptance Tests: introduce LinuxTest base class
>   Acceptance Tests: move useful ssh methods to base class
>   Acceptance Tests: introduce method for requiring an accelerator
>   Acceptance Tests: fix population of public key in cloudinit image
>   Acceptance Tests: set up existing ssh keys by default
>   Acceptance Tests: add port redirection for ssh by default
>   Acceptance Tests: add basic documentation on LinuxTest base class
>   Acceptance Tests: introduce CPU hotplug test
>   [NOTFORMERGE] Bump Avocado version to latest master
> 
>  docs/devel/testing.rst                    |  29 +++-
>  python/qemu/utils.py                      |  35 +++++
>  tests/acceptance/avocado_qemu/__init__.py | 176 ++++++++++++++++++++++
>  tests/acceptance/boot_linux.py            | 128 ++--------------
>  tests/acceptance/hotplug_cpu.py           |  38 +++++
>  tests/acceptance/info_usernet.py          |  29 ++++
>  tests/acceptance/linux_ssh_mips_malta.py  |  44 +-----
>  tests/acceptance/virtiofs_submounts.py    |  73 +--------
>  tests/requirements.txt                    |   2 +-
>  tests/vm/basevm.py                        |   7 +-
>  10 files changed, 334 insertions(+), 227 deletions(-)
>  create mode 100644 python/qemu/utils.py
>  create mode 100644 tests/acceptance/hotplug_cpu.py
>  create mode 100644 tests/acceptance/info_usernet.py

Patches 1-6, 8-9 & 12 queued.

Re: [PATCH 07/22] tests/acceptance/virtiofs_submounts.py: evaluate string not length

[Max Reitz]

On 04.02.21 14:23, Alex Bennée wrote:
> 
> Cleber Rosa <crosa@redhat.com> writes:
> 
>> If the vmlinuz variable is set to anything that evaluates to True,
>> then the respective arguments should be set.  If the variable contains
>> an empty string, than it will evaluate to False, and the extra
>> arguments will not be set.
>>
>> This keeps the same logic, but improves readability a bit.
>>
>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>> ---
>>   tests/acceptance/virtiofs_submounts.py | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
>> index f1b49f03bb..f25a386a19 100644
>> --- a/tests/acceptance/virtiofs_submounts.py
>> +++ b/tests/acceptance/virtiofs_submounts.py
>> @@ -241,7 +241,7 @@ class VirtiofsSubmountsTest(BootLinux):
>>   
>>           super(VirtiofsSubmountsTest, self).setUp(pubkey)
>>   
>> -        if len(vmlinuz) > 0:
>> +        if vmlinuz:
>>               self.vm.add_args('-kernel', vmlinuz,
>>                                '-append', 'console=ttyS0 root=/dev/sda1')
> 
> And this is were I gave up because I can't see how to run the test:
> 
>    ./tests/venv/bin/avocado run ./tests/acceptance/virtiofs_submounts.py
>    JOB ID     : b3d9bfcfcd603189a471bec7d770fca3b50a81ee
>    JOB LOG    : /home/alex/avocado/job-results/job-2021-02-04T13.21-b3d9bfc/job.log
>     (1/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_pre_virtiofsd_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>     (2/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_pre_launch_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>     (3/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_post_launch_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary
>    to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>     (4/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_post_mount_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>     (5/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_two_runs: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>    RESULTS    : PASS 0 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 5
>    JOB TIME   : 0.56 s
> 
> Given the test seems to make assumptions about an environment being
> setup for it I think we need some documentation somewhere about what
> those pre-requisites are.

I find the cancel message pretty clear, but then again it was me who 
wrote it...

Either you point the vmlinuz parameter to some Linux kernel you built 
yourself, or you set it to the empty string to just use the kernel 
that’s part of the image.  Setting Avocado parameters is done via -p, 
i.e. “-p key=value”.  So in this case
“-p vmlinuz=/path/to/linux/build/arch/x86/boot/bzImage”, or
“-p vmlinuz=''”.

Ideally, vmlinuz='' would be the default, but there’s a problem with 
that: I submitted this test along with the patches that added the 
required feature to the Linux kernel, so at that point that feature was 
not part of Linux.  That’s why you generally have to point it to a Linux 
kernel binary you built yourself that has this feature (5.10 does).

Using vmlinuz='' you can test it with the kernel that is part of the 
cloud image.  Once that kernel is sufficiently new (i.e., >=5.10), we 
can make that the default.

Max

Re: [PATCH 07/22] tests/acceptance/virtiofs_submounts.py: evaluate string not length

[Alex Bennée]

Max Reitz <mreitz@redhat.com> writes:

> On 04.02.21 14:23, Alex Bennée wrote:
>> 
>> Cleber Rosa <crosa@redhat.com> writes:
>> 
>>> If the vmlinuz variable is set to anything that evaluates to True,
>>> then the respective arguments should be set.  If the variable contains
>>> an empty string, than it will evaluate to False, and the extra
>>> arguments will not be set.
>>>
>>> This keeps the same logic, but improves readability a bit.
>>>
>>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>>> ---
>>>   tests/acceptance/virtiofs_submounts.py | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
>>> index f1b49f03bb..f25a386a19 100644
>>> --- a/tests/acceptance/virtiofs_submounts.py
>>> +++ b/tests/acceptance/virtiofs_submounts.py
>>> @@ -241,7 +241,7 @@ class VirtiofsSubmountsTest(BootLinux):
>>>   
>>>           super(VirtiofsSubmountsTest, self).setUp(pubkey)
>>>   
>>> -        if len(vmlinuz) > 0:
>>> +        if vmlinuz:
>>>               self.vm.add_args('-kernel', vmlinuz,
>>>                                '-append', 'console=ttyS0 root=/dev/sda1')
>> 
>> And this is were I gave up because I can't see how to run the test:
>> 
>>    ./tests/venv/bin/avocado run ./tests/acceptance/virtiofs_submounts.py
>>    JOB ID     : b3d9bfcfcd603189a471bec7d770fca3b50a81ee
>>    JOB LOG    : /home/alex/avocado/job-results/job-2021-02-04T13.21-b3d9bfc/job.log
>>     (1/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_pre_virtiofsd_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>     (2/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_pre_launch_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>     (3/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_post_launch_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary
>>    to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>     (4/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_post_mount_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>     (5/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_two_runs: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>    RESULTS    : PASS 0 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 5
>>    JOB TIME   : 0.56 s
>> 
>> Given the test seems to make assumptions about an environment being
>> setup for it I think we need some documentation somewhere about what
>> those pre-requisites are.
>
> I find the cancel message pretty clear, but then again it was me who 
> wrote it...
>
> Either you point the vmlinuz parameter to some Linux kernel you built 
> yourself, or you set it to the empty string to just use the kernel 
> that’s part of the image.  Setting Avocado parameters is done via -p, 
> i.e. “-p key=value”.  So in this case
> “-p vmlinuz=/path/to/linux/build/arch/x86/boot/bzImage”, or
> “-p vmlinuz=''”.
>
> Ideally, vmlinuz='' would be the default, but there’s a problem with 
> that: I submitted this test along with the patches that added the 
> required feature to the Linux kernel, so at that point that feature was 
> not part of Linux.  That’s why you generally have to point it to a Linux 
> kernel binary you built yourself that has this feature (5.10 does).

This is where it deviates from the rest of the check-acceptance tests.
Generally I don't have to worry about finding the right image myself. At
the least it would be worth pointing to a part of our docs on how to
build such an image.

> Using vmlinuz='' you can test it with the kernel that is part of the 
> cloud image.  Once that kernel is sufficiently new (i.e., >=5.10), we 
> can make that the default.

Good.

>
> Max


-- 
Alex Bennée

Re: [PATCH 07/22] tests/acceptance/virtiofs_submounts.py: evaluate string not length

[Max Reitz]

On 09.02.21 12:24, Alex Bennée wrote:
> 
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 04.02.21 14:23, Alex Bennée wrote:
>>>
>>> Cleber Rosa <crosa@redhat.com> writes:
>>>
>>>> If the vmlinuz variable is set to anything that evaluates to True,
>>>> then the respective arguments should be set.  If the variable contains
>>>> an empty string, than it will evaluate to False, and the extra
>>>> arguments will not be set.
>>>>
>>>> This keeps the same logic, but improves readability a bit.
>>>>
>>>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>>>> ---
>>>>    tests/acceptance/virtiofs_submounts.py | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
>>>> index f1b49f03bb..f25a386a19 100644
>>>> --- a/tests/acceptance/virtiofs_submounts.py
>>>> +++ b/tests/acceptance/virtiofs_submounts.py
>>>> @@ -241,7 +241,7 @@ class VirtiofsSubmountsTest(BootLinux):
>>>>    
>>>>            super(VirtiofsSubmountsTest, self).setUp(pubkey)
>>>>    
>>>> -        if len(vmlinuz) > 0:
>>>> +        if vmlinuz:
>>>>                self.vm.add_args('-kernel', vmlinuz,
>>>>                                 '-append', 'console=ttyS0 root=/dev/sda1')
>>>
>>> And this is were I gave up because I can't see how to run the test:
>>>
>>>     ./tests/venv/bin/avocado run ./tests/acceptance/virtiofs_submounts.py
>>>     JOB ID     : b3d9bfcfcd603189a471bec7d770fca3b50a81ee
>>>     JOB LOG    : /home/alex/avocado/job-results/job-2021-02-04T13.21-b3d9bfc/job.log
>>>      (1/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_pre_virtiofsd_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>      (2/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_pre_launch_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>      (3/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_post_launch_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary
>>>     to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>      (4/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_post_mount_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>      (5/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_two_runs: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>     RESULTS    : PASS 0 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 5
>>>     JOB TIME   : 0.56 s
>>>
>>> Given the test seems to make assumptions about an environment being
>>> setup for it I think we need some documentation somewhere about what
>>> those pre-requisites are.
>>
>> I find the cancel message pretty clear, but then again it was me who
>> wrote it...
>>
>> Either you point the vmlinuz parameter to some Linux kernel you built
>> yourself, or you set it to the empty string to just use the kernel
>> that’s part of the image.  Setting Avocado parameters is done via -p,
>> i.e. “-p key=value”.  So in this case
>> “-p vmlinuz=/path/to/linux/build/arch/x86/boot/bzImage”, or
>> “-p vmlinuz=''”.
>>
>> Ideally, vmlinuz='' would be the default, but there’s a problem with
>> that: I submitted this test along with the patches that added the
>> required feature to the Linux kernel, so at that point that feature was
>> not part of Linux.  That’s why you generally have to point it to a Linux
>> kernel binary you built yourself that has this feature (5.10 does).
> 
> This is where it deviates from the rest of the check-acceptance tests.
> Generally I don't have to worry about finding the right image myself.

Yes, but there’s nothing I can do apart from just not having the test as 
part of qemu, which I don’t find better than to just cancel it when not 
run with the necessary parameters.

Or, well, I could let the test download and compile the Linux sources, 
but I don’t think that’s a viable alternative.

> At the least it would be worth pointing to a part of our docs on how
> to build such an image.

Well, I could perhaps come up with a comprehensive kernel configuration 
that works, but I really don’t think that’s valuable for people who just 
want to run the acceptance tests and don’t care about this test in 
particular.  I just don’t think they’re actually going to build a Linux 
kernel just for it.

(Alternatively, I could just build a Linux kernel here on my machine, 
upload the binary and point to it somewhere, e.g. in the cancel message. 
  That would be OK for me, and perhaps something I could imagine someone 
might actually use.)

I’d rather just wait until the test image contains a kernel that’s 
sufficiently new (should be the case once Fedora 34 is out), so we can 
make -p vmlinuz='' the default.  Until then I personally find it 
completely reasonable to have this test just be cancelled.

Max

Re: [PATCH 07/22] tests/acceptance/virtiofs_submounts.py: evaluate string not length

[Alex Bennée]

Max Reitz <mreitz@redhat.com> writes:

> On 09.02.21 12:24, Alex Bennée wrote:
>> 
>> Max Reitz <mreitz@redhat.com> writes:
>> 
>>> On 04.02.21 14:23, Alex Bennée wrote:
>>>>
>>>> Cleber Rosa <crosa@redhat.com> writes:
>>>>
>>>>> If the vmlinuz variable is set to anything that evaluates to True,
>>>>> then the respective arguments should be set.  If the variable contains
>>>>> an empty string, than it will evaluate to False, and the extra
>>>>> arguments will not be set.
>>>>>
>>>>> This keeps the same logic, but improves readability a bit.
>>>>>
>>>>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>>>>> ---
>>>>>    tests/acceptance/virtiofs_submounts.py | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
>>>>> index f1b49f03bb..f25a386a19 100644
>>>>> --- a/tests/acceptance/virtiofs_submounts.py
>>>>> +++ b/tests/acceptance/virtiofs_submounts.py
>>>>> @@ -241,7 +241,7 @@ class VirtiofsSubmountsTest(BootLinux):
>>>>>    
>>>>>            super(VirtiofsSubmountsTest, self).setUp(pubkey)
>>>>>    
>>>>> -        if len(vmlinuz) > 0:
>>>>> +        if vmlinuz:
>>>>>                self.vm.add_args('-kernel', vmlinuz,
>>>>>                                 '-append', 'console=ttyS0 root=/dev/sda1')
>>>>
>>>> And this is were I gave up because I can't see how to run the test:
>>>>
>>>>     ./tests/venv/bin/avocado run ./tests/acceptance/virtiofs_submounts.py
>>>>     JOB ID     : b3d9bfcfcd603189a471bec7d770fca3b50a81ee
>>>>     JOB LOG    : /home/alex/avocado/job-results/job-2021-02-04T13.21-b3d9bfc/job.log
>>>>      (1/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_pre_virtiofsd_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>>      (2/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_pre_launch_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>>      (3/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_post_launch_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary
>>>>     to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>>      (4/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_post_mount_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>>      (5/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_two_runs: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>>     RESULTS    : PASS 0 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 5
>>>>     JOB TIME   : 0.56 s
>>>>
>>>> Given the test seems to make assumptions about an environment being
>>>> setup for it I think we need some documentation somewhere about what
>>>> those pre-requisites are.
>>>
>>> I find the cancel message pretty clear, but then again it was me who
>>> wrote it...
>>>
>>> Either you point the vmlinuz parameter to some Linux kernel you built
>>> yourself, or you set it to the empty string to just use the kernel
>>> that’s part of the image.  Setting Avocado parameters is done via -p,
>>> i.e. “-p key=value”.  So in this case
>>> “-p vmlinuz=/path/to/linux/build/arch/x86/boot/bzImage”, or
>>> “-p vmlinuz=''”.
>>>
>>> Ideally, vmlinuz='' would be the default, but there’s a problem with
>>> that: I submitted this test along with the patches that added the
>>> required feature to the Linux kernel, so at that point that feature was
>>> not part of Linux.  That’s why you generally have to point it to a Linux
>>> kernel binary you built yourself that has this feature (5.10 does).
>> 
>> This is where it deviates from the rest of the check-acceptance tests.
>> Generally I don't have to worry about finding the right image myself.
>
> Yes, but there’s nothing I can do apart from just not having the test as 
> part of qemu, which I don’t find better than to just cancel it when not 
> run with the necessary parameters.

No I agree having the tests is useful.

>
> Or, well, I could let the test download and compile the Linux sources, 
> but I don’t think that’s a viable alternative.

There has been discussion before but I agree it's not viable given the
compile times for such things.

>> At the least it would be worth pointing to a part of our docs on how
>> to build such an image.
>
> Well, I could perhaps come up with a comprehensive kernel configuration 
> that works, but I really don’t think that’s valuable for people who just 
> want to run the acceptance tests and don’t care about this test in 
> particular.  I just don’t think they’re actually going to build a Linux 
> kernel just for it.

Sure - but I was trying to review the series and as part of my review I
generally like to run the things I review. Hence why I stopped as I
couldn't get things running.

> (Alternatively, I could just build a Linux kernel here on my machine, 
> upload the binary and point to it somewhere, e.g. in the cancel message. 
>   That would be OK for me, and perhaps something I could imagine someone 
> might actually use.)

I've actually done this with some Xen patches I'm working on at the
moment. I'll probably decorate the test with:

  @skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted code')

with a comment explaining what's waiting to be upstreamed. Once there
are upstream binaries I plan on transitioning the test to those.

> I’d rather just wait until the test image contains a kernel that’s 
> sufficiently new (should be the case once Fedora 34 is out), so we can 
> make -p vmlinuz='' the default.  Until then I personally find it 
> completely reasonable to have this test just be cancelled.
>
> Max


-- 
Alex Bennée

Re: [PATCH 07/22] tests/acceptance/virtiofs_submounts.py: evaluate string not length

[Max Reitz]

On 09.02.21 13:52, Alex Bennée wrote:
> 
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 09.02.21 12:24, Alex Bennée wrote:
>>>
>>> Max Reitz <mreitz@redhat.com> writes:
>>>
>>>> On 04.02.21 14:23, Alex Bennée wrote:
>>>>>
>>>>> Cleber Rosa <crosa@redhat.com> writes:
>>>>>
>>>>>> If the vmlinuz variable is set to anything that evaluates to True,
>>>>>> then the respective arguments should be set.  If the variable contains
>>>>>> an empty string, than it will evaluate to False, and the extra
>>>>>> arguments will not be set.
>>>>>>
>>>>>> This keeps the same logic, but improves readability a bit.
>>>>>>
>>>>>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>>>>>> ---
>>>>>>     tests/acceptance/virtiofs_submounts.py | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
>>>>>> index f1b49f03bb..f25a386a19 100644
>>>>>> --- a/tests/acceptance/virtiofs_submounts.py
>>>>>> +++ b/tests/acceptance/virtiofs_submounts.py
>>>>>> @@ -241,7 +241,7 @@ class VirtiofsSubmountsTest(BootLinux):
>>>>>>     
>>>>>>             super(VirtiofsSubmountsTest, self).setUp(pubkey)
>>>>>>     
>>>>>> -        if len(vmlinuz) > 0:
>>>>>> +        if vmlinuz:
>>>>>>                 self.vm.add_args('-kernel', vmlinuz,
>>>>>>                                  '-append', 'console=ttyS0 root=/dev/sda1')
>>>>>
>>>>> And this is were I gave up because I can't see how to run the test:
>>>>>
>>>>>      ./tests/venv/bin/avocado run ./tests/acceptance/virtiofs_submounts.py
>>>>>      JOB ID     : b3d9bfcfcd603189a471bec7d770fca3b50a81ee
>>>>>      JOB LOG    : /home/alex/avocado/job-results/job-2021-02-04T13.21-b3d9bfc/job.log
>>>>>       (1/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_pre_virtiofsd_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>>>       (2/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_pre_launch_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>>>       (3/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_post_launch_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary
>>>>>      to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>>>       (4/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_post_mount_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>>>       (5/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_two_runs: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>>>      RESULTS    : PASS 0 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 5
>>>>>      JOB TIME   : 0.56 s
>>>>>
>>>>> Given the test seems to make assumptions about an environment being
>>>>> setup for it I think we need some documentation somewhere about what
>>>>> those pre-requisites are.
>>>>
>>>> I find the cancel message pretty clear, but then again it was me who
>>>> wrote it...
>>>>
>>>> Either you point the vmlinuz parameter to some Linux kernel you built
>>>> yourself, or you set it to the empty string to just use the kernel
>>>> that’s part of the image.  Setting Avocado parameters is done via -p,
>>>> i.e. “-p key=value”.  So in this case
>>>> “-p vmlinuz=/path/to/linux/build/arch/x86/boot/bzImage”, or
>>>> “-p vmlinuz=''”.
>>>>
>>>> Ideally, vmlinuz='' would be the default, but there’s a problem with
>>>> that: I submitted this test along with the patches that added the
>>>> required feature to the Linux kernel, so at that point that feature was
>>>> not part of Linux.  That’s why you generally have to point it to a Linux
>>>> kernel binary you built yourself that has this feature (5.10 does).
>>>
>>> This is where it deviates from the rest of the check-acceptance tests.
>>> Generally I don't have to worry about finding the right image myself.
>>
>> Yes, but there’s nothing I can do apart from just not having the test as
>> part of qemu, which I don’t find better than to just cancel it when not
>> run with the necessary parameters.
> 
> No I agree having the tests is useful.
> 
>>
>> Or, well, I could let the test download and compile the Linux sources,
>> but I don’t think that’s a viable alternative.
> 
> There has been discussion before but I agree it's not viable given the
> compile times for such things.
> 
>>> At the least it would be worth pointing to a part of our docs on how
>>> to build such an image.
>>
>> Well, I could perhaps come up with a comprehensive kernel configuration
>> that works, but I really don’t think that’s valuable for people who just
>> want to run the acceptance tests and don’t care about this test in
>> particular.  I just don’t think they’re actually going to build a Linux
>> kernel just for it.
> 
> Sure - but I was trying to review the series and as part of my review I
> generally like to run the things I review. Hence why I stopped as I
> couldn't get things running.
> 
>> (Alternatively, I could just build a Linux kernel here on my machine,
>> upload the binary and point to it somewhere, e.g. in the cancel message.
>>    That would be OK for me, and perhaps something I could imagine someone
>> might actually use.)
> 
> I've actually done this with some Xen patches I'm working on at the
> moment. I'll probably decorate the test with:
> 
>    @skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted code')
> 
> with a comment explaining what's waiting to be upstreamed. Once there
> are upstream binaries I plan on transitioning the test to those.

Oh, so you’d be fine with an in-code comment that explains why the 
parameter is required right now, but will be optional in the future?  If 
so, sounds good to me.

Max

Re: [PATCH 10/22] Python: add utility function for retrieving port redirection

[Wainer dos Santos Moschetta]

Hi,

On 2/3/21 2:23 PM, Cleber Rosa wrote:
> Slightly different versions for the same utility code are currently
> present on different locations.  This unifies them all, giving
> preference to the version from virtiofs_submounts.py, because of the
> last tweaks added to it.
>
> While at it, this adds a "qemu.util" module to host the utility
> function and a test.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   python/qemu/utils.py                     | 35 ++++++++++++++++++++++++
>   tests/acceptance/info_usernet.py         | 29 ++++++++++++++++++++
>   tests/acceptance/linux_ssh_mips_malta.py | 16 +++++------
>   tests/acceptance/virtiofs_submounts.py   | 20 +++-----------
>   tests/vm/basevm.py                       |  7 ++---
>   5 files changed, 77 insertions(+), 30 deletions(-)
>   create mode 100644 python/qemu/utils.py
>   create mode 100644 tests/acceptance/info_usernet.py

I've a few suggestions:

- IMHO "utils" is too broad, people may start throwing random stuffs 
there. Maybe call it "net". I am sure there will be more net-related 
code to be clustered on that module in the future.

- Rename the method to "get_hostfwd_port()" because the parameter's name 
already implies it is expected an "info usernet" output.

- Drop the InfoUsernet Test. It is too simple, and the same 
functionality is tested indirectly by other tests.

>
> diff --git a/python/qemu/utils.py b/python/qemu/utils.py
> new file mode 100644
> index 0000000000..89a246ab30
> --- /dev/null
> +++ b/python/qemu/utils.py
> @@ -0,0 +1,35 @@
> +"""
> +QEMU utility library
> +
> +This offers miscellaneous utility functions, which may not be easily
> +distinguishable or numerous to be in their own module.
> +"""
> +
> +# Copyright (C) 2021 Red Hat Inc.
> +#
> +# Authors:
> +#  Cleber Rosa <crosa@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2.  See
> +# the COPYING file in the top-level directory.
> +#
> +
> +import re
> +from typing import Optional
> +
> +
> +def get_info_usernet_hostfwd_port(info_usernet_output: str) -> Optional[int]:
> +    """
> +    Returns the port given to the hostfwd parameter via info usernet
> +
> +    :param info_usernet_output: output generated by hmp command "info usernet"
> +    :param info_usernet_output: str
> +    :return: the port number allocated by the hostfwd option
> +    :rtype: int
> +    """
> +    for line in info_usernet_output.split('\r\n'):
> +        regex = r'TCP.HOST_FORWARD.*127\.0\.0\.1\s+(\d+)\s+10\.'
> +        match = re.search(regex, line)
> +        if match is not None:
> +            return int(match[1])
> +    return None
> diff --git a/tests/acceptance/info_usernet.py b/tests/acceptance/info_usernet.py
> new file mode 100644
> index 0000000000..9c1fd903a0
> --- /dev/null
> +++ b/tests/acceptance/info_usernet.py
> @@ -0,0 +1,29 @@
> +# Test for the hmp command "info usernet"
> +#
> +# Copyright (c) 2021 Red Hat, Inc.
> +#
> +# Author:
> +#  Cleber Rosa <crosa@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 avocado_qemu import Test
> +
> +from qemu.utils import get_info_usernet_hostfwd_port
> +
> +
> +class InfoUsernet(Test):
> +
> +    def test_hostfwd(self):
> +        self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22')
> +        self.vm.launch()
> +        res = self.vm.command('human-monitor-command',
> +                              command_line='info usernet')
> +        port = get_info_usernet_hostfwd_port(res)
> +        self.assertIsNotNone(port,
> +                             ('"info usernet" output content does not seem to '
> +                              'contain the redirected port'))
> +        self.assertGreater(port, 0,
> +                           ('Found a redirected port that is not greater than'
> +                            ' zero'))
> diff --git a/tests/acceptance/linux_ssh_mips_malta.py b/tests/acceptance/linux_ssh_mips_malta.py
> index 25c5c5f741..275659c785 100644
> --- a/tests/acceptance/linux_ssh_mips_malta.py
> +++ b/tests/acceptance/linux_ssh_mips_malta.py
> @@ -18,6 +18,8 @@ from avocado.utils import process
>   from avocado.utils import archive
>   from avocado.utils import ssh
>   
> +from qemu.utils import get_info_usernet_hostfwd_port
> +
>   
>   class LinuxSSH(Test):
>   
> @@ -70,18 +72,14 @@ class LinuxSSH(Test):
>       def setUp(self):
>           super(LinuxSSH, self).setUp()
>   
> -    def get_portfwd(self):
> +    def ssh_connect(self, username, password):
> +        self.ssh_logger = logging.getLogger('ssh')
>           res = self.vm.command('human-monitor-command',
>                                 command_line='info usernet')
> -        line = res.split('\r\n')[2]
> -        port = re.split(r'.*TCP.HOST_FORWARD.*127\.0\.0\.1 (\d+)\s+10\..*',
> -                        line)[1]
> +        port = get_info_usernet_hostfwd_port(res)
> +        if not port:
> +            self.cancel("Failed to retrieve SSH port")

Here I think it should assert not none, otherwise there is a bug somewhere.

- Wainer

>           self.log.debug("sshd listening on port:" + port)
> -        return port
> -
> -    def ssh_connect(self, username, password):
> -        self.ssh_logger = logging.getLogger('ssh')
> -        port = self.get_portfwd()
>           self.ssh_session = ssh.Session(self.VM_IP, port=int(port),
>                                          user=username, password=password)
>           for i in range(10):
> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
> index 0f9d7a5d9c..bf99164fcb 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -10,6 +10,7 @@ from avocado_qemu import wait_for_console_pattern
>   from avocado.utils import ssh
>   
>   from qemu.accel import kvm_available
> +from qemu.utils import get_info_usernet_hostfwd_port
>   
>   from boot_linux import BootLinux
>   
> @@ -76,27 +77,14 @@ class VirtiofsSubmountsTest(BootLinux):
>       :avocado: tags=arch:x86_64
>       """
>   
> -    def get_portfwd(self):
> -        port = None
> -
> +    def ssh_connect(self, username, keyfile):
> +        self.ssh_logger = logging.getLogger('ssh')
>           res = self.vm.command('human-monitor-command',
>                                 command_line='info usernet')
> -        for line in res.split('\r\n'):
> -            match = \
> -                re.search(r'TCP.HOST_FORWARD.*127\.0\.0\.1\s+(\d+)\s+10\.',
> -                          line)
> -            if match is not None:
> -                port = int(match[1])
> -                break
> -
> +        port = get_info_usernet_hostfwd_port(res)
>           self.assertIsNotNone(port)
>           self.assertGreater(port, 0)
>           self.log.debug('sshd listening on port: %d', port)
> -        return port
> -
> -    def ssh_connect(self, username, keyfile):
> -        self.ssh_logger = logging.getLogger('ssh')
> -        port = self.get_portfwd()
>           self.ssh_session = ssh.Session('127.0.0.1', port=port,
>                                          user=username, key=keyfile)
>           for i in range(10):
> diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
> index 00f1d5ca8d..75ce07df36 100644
> --- a/tests/vm/basevm.py
> +++ b/tests/vm/basevm.py
> @@ -21,6 +21,7 @@ import datetime
>   sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
>   from qemu.accel import kvm_available
>   from qemu.machine import QEMUMachine
> +from qemu.utils import get_info_usernet_hostfwd_port
>   import subprocess
>   import hashlib
>   import argparse
> @@ -306,11 +307,7 @@ class BaseVM(object):
>           self.console_init()
>           usernet_info = guest.qmp("human-monitor-command",
>                                    command_line="info usernet")
> -        self.ssh_port = None
> -        for l in usernet_info["return"].splitlines():
> -            fields = l.split()
> -            if "TCP[HOST_FORWARD]" in fields and "22" in fields:
> -                self.ssh_port = l.split()[3]
> +        self.ssh_port = get_info_usernet_hostfwd_port(usernet_info)
>           if not self.ssh_port:
>               raise Exception("Cannot find ssh port from 'info usernet':\n%s" % \
>                               usernet_info)

Re: [PATCH 13/22] tests/acceptance/virtiofs_submounts.py: add missing accel tag

[Wainer dos Santos Moschetta]

On 2/3/21 2:23 PM, Cleber Rosa wrote:
> Which is useful to select tests that depend/use a particular feature.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/acceptance/virtiofs_submounts.py | 1 +
>   1 file changed, 1 insertion(+)


Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


>
> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
> index c998b297ee..1e745f15a2 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -75,6 +75,7 @@ def has_cmds(*cmds):
>   class VirtiofsSubmountsTest(BootLinux):
>       """
>       :avocado: tags=arch:x86_64
> +    :avocado: tags=accel:kvm
>       """
>   
>       def ssh_connect(self, username, keyfile):

Re: [PATCH 07/22] tests/acceptance/virtiofs_submounts.py: evaluate string not length

[Alex Bennée]

Max Reitz <mreitz@redhat.com> writes:

> On 09.02.21 13:52, Alex Bennée wrote:
>> 
>> Max Reitz <mreitz@redhat.com> writes:
>> 
>>> On 09.02.21 12:24, Alex Bennée wrote:
>>>>
>>>> Max Reitz <mreitz@redhat.com> writes:
>>>>
>>>>> On 04.02.21 14:23, Alex Bennée wrote:
>>>>>>
>>>>>> Cleber Rosa <crosa@redhat.com> writes:
>>>>>>
>>>>>>> If the vmlinuz variable is set to anything that evaluates to True,
>>>>>>> then the respective arguments should be set.  If the variable contains
>>>>>>> an empty string, than it will evaluate to False, and the extra
>>>>>>> arguments will not be set.
>>>>>>>
>>>>>>> This keeps the same logic, but improves readability a bit.
>>>>>>>
>>>>>>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>>>>>>> ---
>>>>>>>     tests/acceptance/virtiofs_submounts.py | 2 +-
>>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
>>>>>>> index f1b49f03bb..f25a386a19 100644
>>>>>>> --- a/tests/acceptance/virtiofs_submounts.py
>>>>>>> +++ b/tests/acceptance/virtiofs_submounts.py
>>>>>>> @@ -241,7 +241,7 @@ class VirtiofsSubmountsTest(BootLinux):
>>>>>>>     
>>>>>>>             super(VirtiofsSubmountsTest, self).setUp(pubkey)
>>>>>>>     
>>>>>>> -        if len(vmlinuz) > 0:
>>>>>>> +        if vmlinuz:
>>>>>>>                 self.vm.add_args('-kernel', vmlinuz,
>>>>>>>                                  '-append', 'console=ttyS0 root=/dev/sda1')
>>>>>>
>>>>>> And this is were I gave up because I can't see how to run the test:
>>>>>>
>>>>>>      ./tests/venv/bin/avocado run ./tests/acceptance/virtiofs_submounts.py
>>>>>>      JOB ID     : b3d9bfcfcd603189a471bec7d770fca3b50a81ee
>>>>>>      JOB LOG    : /home/alex/avocado/job-results/job-2021-02-04T13.21-b3d9bfc/job.log
>>>>>>       (1/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_pre_virtiofsd_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>>>>       (2/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_pre_launch_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>>>>       (3/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_post_launch_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary
>>>>>>      to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>>>>       (4/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_post_mount_set_up: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>>>>       (5/5) ./tests/acceptance/virtiofs_submounts.py:VirtiofsSubmountsTest.test_two_runs: CANCEL: vmlinuz parameter not set; you must point it to a Linux kernel binary to test (to run this test with the on-image kernel, set it to an empty string) (0.00 s)
>>>>>>      RESULTS    : PASS 0 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 5
>>>>>>      JOB TIME   : 0.56 s
>>>>>>
>>>>>> Given the test seems to make assumptions about an environment being
>>>>>> setup for it I think we need some documentation somewhere about what
>>>>>> those pre-requisites are.
>>>>>
>>>>> I find the cancel message pretty clear, but then again it was me who
>>>>> wrote it...
>>>>>
>>>>> Either you point the vmlinuz parameter to some Linux kernel you built
>>>>> yourself, or you set it to the empty string to just use the kernel
>>>>> that’s part of the image.  Setting Avocado parameters is done via -p,
>>>>> i.e. “-p key=value”.  So in this case
>>>>> “-p vmlinuz=/path/to/linux/build/arch/x86/boot/bzImage”, or
>>>>> “-p vmlinuz=''”.
>>>>>
>>>>> Ideally, vmlinuz='' would be the default, but there’s a problem with
>>>>> that: I submitted this test along with the patches that added the
>>>>> required feature to the Linux kernel, so at that point that feature was
>>>>> not part of Linux.  That’s why you generally have to point it to a Linux
>>>>> kernel binary you built yourself that has this feature (5.10 does).
>>>>
>>>> This is where it deviates from the rest of the check-acceptance tests.
>>>> Generally I don't have to worry about finding the right image myself.
>>>
>>> Yes, but there’s nothing I can do apart from just not having the test as
>>> part of qemu, which I don’t find better than to just cancel it when not
>>> run with the necessary parameters.
>> 
>> No I agree having the tests is useful.
>> 
>>>
>>> Or, well, I could let the test download and compile the Linux sources,
>>> but I don’t think that’s a viable alternative.
>> 
>> There has been discussion before but I agree it's not viable given the
>> compile times for such things.
>> 
>>>> At the least it would be worth pointing to a part of our docs on how
>>>> to build such an image.
>>>
>>> Well, I could perhaps come up with a comprehensive kernel configuration
>>> that works, but I really don’t think that’s valuable for people who just
>>> want to run the acceptance tests and don’t care about this test in
>>> particular.  I just don’t think they’re actually going to build a Linux
>>> kernel just for it.
>> 
>> Sure - but I was trying to review the series and as part of my review I
>> generally like to run the things I review. Hence why I stopped as I
>> couldn't get things running.
>> 
>>> (Alternatively, I could just build a Linux kernel here on my machine,
>>> upload the binary and point to it somewhere, e.g. in the cancel message.
>>>    That would be OK for me, and perhaps something I could imagine someone
>>> might actually use.)
>> 
>> I've actually done this with some Xen patches I'm working on at the
>> moment. I'll probably decorate the test with:
>> 
>>    @skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted code')
>> 
>> with a comment explaining what's waiting to be upstreamed. Once there
>> are upstream binaries I plan on transitioning the test to those.
>
> Oh, so you’d be fine with an in-code comment that explains why the 
> parameter is required right now, but will be optional in the future?  If 
> so, sounds good to me.

Yes that would be great ;-)

-- 
Alex Bennée

Re: [PATCH 07/22] tests/acceptance/virtiofs_submounts.py: evaluate string not length

[Philippe Mathieu-Daudé]

On 2/9/21 1:52 PM, Alex Bennée wrote:
> Max Reitz <mreitz@redhat.com> writes:
>> On 09.02.21 12:24, Alex Bennée wrote:
>>> Max Reitz <mreitz@redhat.com> writes:
>>>> On 04.02.21 14:23, Alex Bennée wrote:
>>>>> Cleber Rosa <crosa@redhat.com> writes:
>>>>>
...
>>>> Ideally, vmlinuz='' would be the default, but there’s a problem with
>>>> that: I submitted this test along with the patches that added the
>>>> required feature to the Linux kernel, so at that point that feature was
>>>> not part of Linux.  That’s why you generally have to point it to a Linux
>>>> kernel binary you built yourself that has this feature (5.10 does).
>>>
>>> This is where it deviates from the rest of the check-acceptance tests.
>>> Generally I don't have to worry about finding the right image myself.
>>
>> Yes, but there’s nothing I can do apart from just not having the test as 
>> part of qemu, which I don’t find better than to just cancel it when not 
>> run with the necessary parameters.
> 
> No I agree having the tests is useful.
> 
>>
>> Or, well, I could let the test download and compile the Linux sources, 
>> but I don’t think that’s a viable alternative.
> 
> There has been discussion before but I agree it's not viable given the
> compile times for such things.
> 
>>> At the least it would be worth pointing to a part of our docs on how
>>> to build such an image.
>>
>> Well, I could perhaps come up with a comprehensive kernel configuration 
>> that works, but I really don’t think that’s valuable for people who just 
>> want to run the acceptance tests and don’t care about this test in 
>> particular.  I just don’t think they’re actually going to build a Linux 
>> kernel just for it.
> 
> Sure - but I was trying to review the series and as part of my review I
> generally like to run the things I review. Hence why I stopped as I
> couldn't get things running.
> 
>> (Alternatively, I could just build a Linux kernel here on my machine, 
>> upload the binary and point to it somewhere, e.g. in the cancel message. 
>>   That would be OK for me, and perhaps something I could imagine someone 
>> might actually use.)
> 
> I've actually done this with some Xen patches I'm working on at the
> moment. I'll probably decorate the test with:
> 
>   @skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted code')
> 
> with a comment explaining what's waiting to be upstreamed. Once there
> are upstream binaries I plan on transitioning the test to those.

Instead of a binary AVOCADO_ALLOW_UNTRUSTED_CODE variable, we could
have a list allowed domains/namespaces, that can be increased on the
tester discretion.

For example these are assumed trusted:

. archives.fedoraproject.org
. archive.debian.org
. cdn.netbsd.org
. github.com/torvalds
. people.debian.org/~aurel32
. snapshot.debian.org
. storage.kernelci.org
. www.qemu-advent-calendar.org

Then personally interested in testing ARM boards I'd amend:

. apt.armbian.com
. github.com/philmd
. github.com/groeck
. github.com/hskinnemoen
. github.com/pbatard

and Max's repo since I'm interested in testing virtiofs_submounts.

Re: [PATCH 14/22] Acceptance Tests: introduce LinuxTest base class

[Wainer dos Santos Moschetta]

On 2/3/21 2:23 PM, Cleber Rosa wrote:
> This is basically the infrastructure around "boot_linux.py" tests, but
> now made into a base class for general use.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/acceptance/avocado_qemu/__init__.py | 87 +++++++++++++++++++++
>   tests/acceptance/boot_linux.py            | 94 ++---------------------
>   tests/acceptance/virtiofs_submounts.py    |  6 +-
>   3 files changed, 94 insertions(+), 93 deletions(-)


Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index bf54e419da..b06692a59d 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -16,6 +16,13 @@ import tempfile
>   
>   import avocado
>   
> +from avocado.utils import cloudinit
> +from avocado.utils import datadrainer
> +from avocado.utils import network
> +from avocado.utils import vmimage
> +from avocado.utils.path import find_command
> +
> +
>   #: The QEMU build root directory.  It may also be the source directory
>   #: if building from the source dir, but it's safer to use BUILD_DIR for
>   #: that purpose.  Be aware that if this code is moved outside of a source
> @@ -206,3 +213,83 @@ class Test(avocado.Test):
>                           expire=expire,
>                           find_only=find_only,
>                           cancel_on_missing=cancel_on_missing)
> +
> +
> +class LinuxTest(Test):
> +    """Facilitates having a cloud-image Linux based available.
> +
> +    For tests that indend to interact with guests, this is a better choice
> +    to start with than the more vanilla `Test` class.
> +    """
> +
> +    timeout = 900
> +    chksum = None
> +
> +    def setUp(self, ssh_pubkey=None):
> +        super(LinuxTest, self).setUp()
> +        self.vm.add_args('-smp', '2')
> +        self.vm.add_args('-m', '1024')
> +        self.set_up_boot()
> +        self.set_up_cloudinit(ssh_pubkey)
> +
> +    def download_boot(self):
> +        self.log.debug('Looking for and selecting a qemu-img binary to be '
> +                       'used to create the bootable snapshot image')
> +        # If qemu-img has been built, use it, otherwise the system wide one
> +        # will be used.  If none is available, the test will cancel.
> +        qemu_img = os.path.join(BUILD_DIR, 'qemu-img')
> +        if not os.path.exists(qemu_img):
> +            qemu_img = find_command('qemu-img', False)
> +        if qemu_img is False:
> +            self.cancel('Could not find "qemu-img", which is required to '
> +                        'create the bootable image')
> +        vmimage.QEMU_IMG = qemu_img
> +
> +        self.log.info('Downloading/preparing boot image')
> +        # Fedora 31 only provides ppc64le images
> +        image_arch = self.arch
> +        if image_arch == 'ppc64':
> +            image_arch = 'ppc64le'
> +        try:
> +            boot = vmimage.get(
> +                'fedora', arch=image_arch, version='31',
> +                checksum=self.chksum,
> +                algorithm='sha256',
> +                cache_dir=self.cache_dirs[0],
> +                snapshot_dir=self.workdir)
> +        except:
> +            self.cancel('Failed to download/prepare boot image')
> +        return boot.path
> +
> +    def prepare_cloudinit(self, ssh_pubkey=None):
> +        self.log.info('Preparing cloudinit image')
> +        try:
> +            cloudinit_iso = os.path.join(self.workdir, 'cloudinit.iso')
> +            self.phone_home_port = network.find_free_port()
> +            cloudinit.iso(cloudinit_iso, self.name,
> +                          username='root',
> +                          password='password',
> +                          # QEMU's hard coded usermode router address
> +                          phone_home_host='10.0.2.2',
> +                          phone_home_port=self.phone_home_port,
> +                          authorized_key=ssh_pubkey)
> +        except Exception:
> +            self.cancel('Failed to prepare the cloudinit image')
> +        return cloudinit_iso
> +
> +    def set_up_boot(self):
> +        path = self.download_boot()
> +        self.vm.add_args('-drive', 'file=%s' % path)
> +
> +    def set_up_cloudinit(self, ssh_pubkey=None):
> +        cloudinit_iso = self.prepare_cloudinit(ssh_pubkey)
> +        self.vm.add_args('-drive', 'file=%s,format=raw' % cloudinit_iso)
> +
> +    def launch_and_wait(self):
> +        self.vm.set_console()
> +        self.vm.launch()
> +        console_drainer = datadrainer.LineLogger(self.vm.console_socket.fileno(),
> +                                                 logger=self.log.getChild('console'))
> +        console_drainer.start()
> +        self.log.info('VM launched, waiting for boot confirmation from guest')
> +        cloudinit.wait_for_phone_home(('0.0.0.0', self.phone_home_port), self.name)
> diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
> index bcd923bb4a..14e89d020d 100644
> --- a/tests/acceptance/boot_linux.py
> +++ b/tests/acceptance/boot_linux.py
> @@ -10,16 +10,11 @@
>   
>   import os
>   
> -from avocado_qemu import Test, BUILD_DIR
> +from avocado_qemu import LinuxTest, BUILD_DIR
>   
>   from qemu.accel import kvm_available
>   from qemu.accel import tcg_available
>   
> -from avocado.utils import cloudinit
> -from avocado.utils import network
> -from avocado.utils import vmimage
> -from avocado.utils import datadrainer
> -from avocado.utils.path import find_command
>   from avocado import skipIf
>   
>   ACCEL_NOT_AVAILABLE_FMT = "%s accelerator does not seem to be available"
> @@ -27,86 +22,7 @@ KVM_NOT_AVAILABLE = ACCEL_NOT_AVAILABLE_FMT % "KVM"
>   TCG_NOT_AVAILABLE = ACCEL_NOT_AVAILABLE_FMT % "TCG"
>   
>   
> -class BootLinuxBase(Test):
> -    def download_boot(self):
> -        self.log.debug('Looking for and selecting a qemu-img binary to be '
> -                       'used to create the bootable snapshot image')
> -        # If qemu-img has been built, use it, otherwise the system wide one
> -        # will be used.  If none is available, the test will cancel.
> -        qemu_img = os.path.join(BUILD_DIR, 'qemu-img')
> -        if not os.path.exists(qemu_img):
> -            qemu_img = find_command('qemu-img', False)
> -        if qemu_img is False:
> -            self.cancel('Could not find "qemu-img", which is required to '
> -                        'create the bootable image')
> -        vmimage.QEMU_IMG = qemu_img
> -
> -        self.log.info('Downloading/preparing boot image')
> -        # Fedora 31 only provides ppc64le images
> -        image_arch = self.arch
> -        if image_arch == 'ppc64':
> -            image_arch = 'ppc64le'
> -        try:
> -            boot = vmimage.get(
> -                'fedora', arch=image_arch, version='31',
> -                checksum=self.chksum,
> -                algorithm='sha256',
> -                cache_dir=self.cache_dirs[0],
> -                snapshot_dir=self.workdir)
> -        except:
> -            self.cancel('Failed to download/prepare boot image')
> -        return boot.path
> -
> -    def prepare_cloudinit(self, ssh_pubkey=None):
> -        self.log.info('Preparing cloudinit image')
> -        try:
> -            cloudinit_iso = os.path.join(self.workdir, 'cloudinit.iso')
> -            self.phone_home_port = network.find_free_port()
> -            cloudinit.iso(cloudinit_iso, self.name,
> -                          username='root',
> -                          password='password',
> -                          # QEMU's hard coded usermode router address
> -                          phone_home_host='10.0.2.2',
> -                          phone_home_port=self.phone_home_port,
> -                          authorized_key=ssh_pubkey)
> -        except Exception:
> -            self.cancel('Failed to prepare the cloudinit image')
> -        return cloudinit_iso
> -
> -class BootLinux(BootLinuxBase):
> -    """
> -    Boots a Linux system, checking for a successful initialization
> -    """
> -
> -    timeout = 900
> -    chksum = None
> -
> -    def setUp(self, ssh_pubkey=None):
> -        super(BootLinux, self).setUp()
> -        self.vm.add_args('-smp', '2')
> -        self.vm.add_args('-m', '1024')
> -        self.set_up_boot()
> -        self.set_up_cloudinit(ssh_pubkey)
> -
> -    def set_up_boot(self):
> -        path = self.download_boot()
> -        self.vm.add_args('-drive', 'file=%s' % path)
> -
> -    def set_up_cloudinit(self, ssh_pubkey=None):
> -        cloudinit_iso = self.prepare_cloudinit(ssh_pubkey)
> -        self.vm.add_args('-drive', 'file=%s,format=raw' % cloudinit_iso)
> -
> -    def launch_and_wait(self):
> -        self.vm.set_console()
> -        self.vm.launch()
> -        console_drainer = datadrainer.LineLogger(self.vm.console_socket.fileno(),
> -                                                 logger=self.log.getChild('console'))
> -        console_drainer.start()
> -        self.log.info('VM launched, waiting for boot confirmation from guest')
> -        cloudinit.wait_for_phone_home(('0.0.0.0', self.phone_home_port), self.name)
> -
> -
> -class BootLinuxX8664(BootLinux):
> +class BootLinuxX8664(LinuxTest):
>       """
>       :avocado: tags=arch:x86_64
>       """
> @@ -154,7 +70,7 @@ class BootLinuxX8664(BootLinux):
>           self.launch_and_wait()
>   
>   
> -class BootLinuxAarch64(BootLinux):
> +class BootLinuxAarch64(LinuxTest):
>       """
>       :avocado: tags=arch:aarch64
>       :avocado: tags=machine:virt
> @@ -212,7 +128,7 @@ class BootLinuxAarch64(BootLinux):
>           self.launch_and_wait()
>   
>   
> -class BootLinuxPPC64(BootLinux):
> +class BootLinuxPPC64(LinuxTest):
>       """
>       :avocado: tags=arch:ppc64
>       """
> @@ -230,7 +146,7 @@ class BootLinuxPPC64(BootLinux):
>           self.launch_and_wait()
>   
>   
> -class BootLinuxS390X(BootLinux):
> +class BootLinuxS390X(LinuxTest):
>       """
>       :avocado: tags=arch:s390x
>       """
> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
> index 1e745f15a2..25ea54b6ff 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -5,15 +5,13 @@ import subprocess
>   import time
>   
>   from avocado import skipUnless
> -from avocado_qemu import Test, BUILD_DIR
> +from avocado_qemu import LinuxTest, BUILD_DIR
>   from avocado_qemu import wait_for_console_pattern
>   from avocado.utils import ssh
>   
>   from qemu.accel import kvm_available
>   from qemu.utils import get_info_usernet_hostfwd_port
>   
> -from boot_linux import BootLinux
> -
>   
>   def run_cmd(args):
>       subp = subprocess.Popen(args,
> @@ -72,7 +70,7 @@ def has_cmds(*cmds):
>       return (True, '')
>   
>   
> -class VirtiofsSubmountsTest(BootLinux):
> +class VirtiofsSubmountsTest(LinuxTest):
>       """
>       :avocado: tags=arch:x86_64
>       :avocado: tags=accel:kvm

Re: [PATCH 15/22] Acceptance Tests: move useful ssh methods to base class

[Wainer dos Santos Moschetta]

On 2/3/21 2:23 PM, Cleber Rosa wrote:
> Both the virtiofs submounts and the linux ssh mips malta tests
> contains useful methods related to ssh that deserve to be made
> available to other tests.  Let's move them to the base LinuxTest
> class.
>
> The method that helps with setting up an ssh connection will now
> support both key and password based authentication, defaulting to key
> based.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/acceptance/avocado_qemu/__init__.py | 49 ++++++++++++++++++++++-
>   tests/acceptance/linux_ssh_mips_malta.py  | 38 ++----------------
>   tests/acceptance/virtiofs_submounts.py    | 36 -----------------
>   3 files changed, 51 insertions(+), 72 deletions(-)


Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index b06692a59d..f0649e5011 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -10,6 +10,7 @@
>   
>   import logging
>   import os
> +import re
>   import sys
>   import uuid
>   import tempfile
> @@ -19,6 +20,7 @@ import avocado
>   from avocado.utils import cloudinit
>   from avocado.utils import datadrainer
>   from avocado.utils import network
> +from avocado.utils import ssh
>   from avocado.utils import vmimage
>   from avocado.utils.path import find_command
>   
> @@ -40,6 +42,8 @@ else:
>   sys.path.append(os.path.join(SOURCE_DIR, 'python'))
>   
>   from qemu.machine import QEMUMachine
> +from qemu.utils import get_info_usernet_hostfwd_port
> +
>   
>   def is_readable_executable_file(path):
>       return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
> @@ -215,7 +219,50 @@ class Test(avocado.Test):
>                           cancel_on_missing=cancel_on_missing)
>   
>   
> -class LinuxTest(Test):
> +class LinuxSSHMixIn:
> +    """Contains utility methods for interacting with a guest via SSH."""
> +
> +    def ssh_connect(self, username, credential, credential_is_key=True):
> +        self.ssh_logger = logging.getLogger('ssh')
> +        res = self.vm.command('human-monitor-command',
> +                              command_line='info usernet')
> +        port = get_info_usernet_hostfwd_port(res)
> +        self.assertIsNotNone(port)
> +        self.assertGreater(port, 0)
> +        self.log.debug('sshd listening on port: %d', port)
> +        if credential_is_key:
> +            self.ssh_session = ssh.Session('127.0.0.1', port=port,
> +                                           user=username, key=credential)
> +        else:
> +            self.ssh_session = ssh.Session('127.0.0.1', port=port,
> +                                           user=username, password=credential)
> +        for i in range(10):
> +            try:
> +                self.ssh_session.connect()
> +                return
> +            except:
> +                time.sleep(4)
> +                pass
> +        self.fail('ssh connection timeout')
> +
> +    def ssh_command(self, command):
> +        self.ssh_logger.info(command)
> +        result = self.ssh_session.cmd(command)
> +        stdout_lines = [line.rstrip() for line
> +                        in result.stdout_text.splitlines()]
> +        for line in stdout_lines:
> +            self.ssh_logger.info(line)
> +        stderr_lines = [line.rstrip() for line
> +                        in result.stderr_text.splitlines()]
> +        for line in stderr_lines:
> +            self.ssh_logger.warning(line)
> +
> +        self.assertEqual(result.exit_status, 0,
> +                         f'Guest command failed: {command}')
> +        return stdout_lines, stderr_lines
> +
> +
> +class LinuxTest(Test, LinuxSSHMixIn):
>       """Facilitates having a cloud-image Linux based available.
>   
>       For tests that indend to interact with guests, this is a better choice
> diff --git a/tests/acceptance/linux_ssh_mips_malta.py b/tests/acceptance/linux_ssh_mips_malta.py
> index 1742235758..3f590a081f 100644
> --- a/tests/acceptance/linux_ssh_mips_malta.py
> +++ b/tests/acceptance/linux_ssh_mips_malta.py
> @@ -12,7 +12,7 @@ import logging
>   import time
>   
>   from avocado import skipUnless
> -from avocado_qemu import Test
> +from avocado_qemu import Test, LinuxSSHMixIn
>   from avocado_qemu import wait_for_console_pattern
>   from avocado.utils import process
>   from avocado.utils import archive
> @@ -21,7 +21,7 @@ from avocado.utils import ssh
>   from qemu.utils import get_info_usernet_hostfwd_port
>   
>   
> -class LinuxSSH(Test):
> +class LinuxSSH(Test, LinuxSSHMixIn):
>   
>       timeout = 150 # Not for 'configure --enable-debug --enable-debug-tcg'
>   
> @@ -72,41 +72,9 @@ class LinuxSSH(Test):
>       def setUp(self):
>           super(LinuxSSH, self).setUp()
>   
> -    def ssh_connect(self, username, password):
> -        self.ssh_logger = logging.getLogger('ssh')
> -        res = self.vm.command('human-monitor-command',
> -                              command_line='info usernet')
> -        port = get_info_usernet_hostfwd_port(res)
> -        if not port:
> -            self.cancel("Failed to retrieve SSH port")
> -        self.log.debug("sshd listening on port: %d", port)
> -        self.ssh_session = ssh.Session(self.VM_IP, port=port,
> -                                       user=username, password=password)
> -        for i in range(10):
> -            try:
> -                self.ssh_session.connect()
> -                return
> -            except:
> -                time.sleep(4)
> -                pass
> -        self.fail("ssh connection timeout")
> -
>       def ssh_disconnect_vm(self):
>           self.ssh_session.quit()
>   
> -    def ssh_command(self, command, is_root=True):
> -        self.ssh_logger.info(command)
> -        result = self.ssh_session.cmd(command)
> -        stdout_lines = [line.rstrip() for line
> -                        in result.stdout_text.splitlines()]
> -        for line in stdout_lines:
> -            self.ssh_logger.info(line)
> -        stderr_lines = [line.rstrip() for line
> -                        in result.stderr_text.splitlines()]
> -        for line in stderr_lines:
> -            self.ssh_logger.warning(line)
> -        return stdout_lines, stderr_lines
> -
>       def boot_debian_wheezy_image_and_ssh_login(self, endianess, kernel_path):
>           image_url, image_hash = self.get_image_info(endianess)
>           image_path = self.fetch_asset(image_url, asset_hash=image_hash)
> @@ -127,7 +95,7 @@ class LinuxSSH(Test):
>           wait_for_console_pattern(self, console_pattern, 'Oops')
>           self.log.info('sshd ready')
>   
> -        self.ssh_connect('root', 'root')
> +        self.ssh_connect('root', 'root', False)
>   
>       def shutdown_via_ssh(self):
>           self.ssh_command('poweroff')
> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
> index 25ea54b6ff..d0fc103f72 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -10,7 +10,6 @@ from avocado_qemu import wait_for_console_pattern
>   from avocado.utils import ssh
>   
>   from qemu.accel import kvm_available
> -from qemu.utils import get_info_usernet_hostfwd_port
>   
>   
>   def run_cmd(args):
> @@ -76,41 +75,6 @@ class VirtiofsSubmountsTest(LinuxTest):
>       :avocado: tags=accel:kvm
>       """
>   
> -    def ssh_connect(self, username, keyfile):
> -        self.ssh_logger = logging.getLogger('ssh')
> -        res = self.vm.command('human-monitor-command',
> -                              command_line='info usernet')
> -        port = get_info_usernet_hostfwd_port(res)
> -        self.assertIsNotNone(port)
> -        self.assertGreater(port, 0)
> -        self.log.debug('sshd listening on port: %d', port)
> -        self.ssh_session = ssh.Session('127.0.0.1', port=port,
> -                                       user=username, key=keyfile)
> -        for i in range(10):
> -            try:
> -                self.ssh_session.connect()
> -                return
> -            except:
> -                time.sleep(4)
> -                pass
> -        self.fail('ssh connection timeout')
> -
> -    def ssh_command(self, command):
> -        self.ssh_logger.info(command)
> -        result = self.ssh_session.cmd(command)
> -        stdout_lines = [line.rstrip() for line
> -                        in result.stdout_text.splitlines()]
> -        for line in stdout_lines:
> -            self.ssh_logger.info(line)
> -        stderr_lines = [line.rstrip() for line
> -                        in result.stderr_text.splitlines()]
> -        for line in stderr_lines:
> -            self.ssh_logger.warning(line)
> -
> -        self.assertEqual(result.exit_status, 0,
> -                         f'Guest command failed: {command}')
> -        return stdout_lines, stderr_lines
> -
>       def run(self, args, ignore_error=False):
>           stdout, stderr, ret = run_cmd(args)
>   

Re: [PATCH 17/22] Acceptance Tests: fix population of public key in cloudinit image

[Marc-André Lureau]

On Wed, Feb 3, 2021 at 9:40 PM Cleber Rosa <crosa@redhat.com> wrote:
>
> Currently the path of the ssh public key is being set, but its
> content is obviously what's needed.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  tests/acceptance/avocado_qemu/__init__.py | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index 472088ae7d..8156224625 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -337,13 +337,15 @@ class LinuxTest(Test, LinuxSSHMixIn):
>          try:
>              cloudinit_iso = os.path.join(self.workdir, 'cloudinit.iso')
>              self.phone_home_port = network.find_free_port()
> +            with open(ssh_pubkey) as pubkey:
> +                pubkey_content = pubkey.read()
>              cloudinit.iso(cloudinit_iso, self.name,
>                            username='root',
>                            password='password',
>                            # QEMU's hard coded usermode router address
>                            phone_home_host='10.0.2.2',
>                            phone_home_port=self.phone_home_port,
> -                          authorized_key=ssh_pubkey)
> +                          authorized_key=pubkey_content)
>          except Exception:
>              self.cancel('Failed to prepare the cloudinit image')
>          return cloudinit_iso
> --
> 2.25.4
>
>


-- 
Marc-André Lureau

Re: [PATCH 18/22] Acceptance Tests: set up existing ssh keys by default

[Marc-André Lureau]

Hi

On Wed, Feb 3, 2021 at 10:07 PM Cleber Rosa <crosa@redhat.com> wrote:
>
> It's questionable wether it's necessary to create one brand new pair

whether

> for each test.  It's not questionable that it takes less time and
> resources to just use the keys available at "tests/keys" that exist
> for that exact reason.
>
> If a location for the public key is not given explicitly, the
> LinuxTest will now set up the existing pair of keys as the default.
> This removes the need for a lot of boiler plate code.

boilerplate

>
> To avoid the ssh client from erroring on permission issues, a
> directory with restricive permissions is created for the private key.

restrictive

> This should still be a lot cheaper than creating a new key.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  tests/acceptance/avocado_qemu/__init__.py | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index 8156224625..5f4dd6b9ec 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -11,6 +11,7 @@
>  import logging
>  import os
>  import re
> +import shutil
>  import sys
>  import uuid
>  import tempfile
> @@ -301,8 +302,21 @@ class LinuxTest(Test, LinuxSSHMixIn):
>          self.vm.add_args('-smp', '2')
>          self.vm.add_args('-m', '1024')
>          self.set_up_boot()
> +        if ssh_pubkey is None:
> +            ssh_pubkey, self.ssh_key = self.set_up_existing_ssh_keys()
>          self.set_up_cloudinit(ssh_pubkey)
>
> +    def set_up_existing_ssh_keys(self):
> +        ssh_public_key = os.path.join(SOURCE_DIR, 'tests', 'keys', 'id_rsa.pub')
> +        source_private_key = os.path.join(SOURCE_DIR, 'tests', 'keys', 'id_rsa')
> +        ssh_dir = os.path.join(self.workdir, '.ssh')
> +        os.mkdir(ssh_dir, mode=0o700)
> +        ssh_private_key = os.path.join(ssh_dir,
> +                                       os.path.basename(source_private_key))
> +        shutil.copyfile(source_private_key, ssh_private_key)
> +        os.chmod(ssh_private_key, 0o600)
> +        return (ssh_public_key, ssh_private_key)
> +
>      def download_boot(self):
>          self.log.debug('Looking for and selecting a qemu-img binary to be '
>                         'used to create the bootable snapshot image')
> --
> 2.25.4
>
>


-- 
Marc-André Lureau

Re: [PATCH 20/22] Acceptance Tests: add basic documentation on LinuxTest base class

[Marc-André Lureau]

On Wed, Feb 3, 2021 at 9:47 PM Cleber Rosa <crosa@redhat.com> wrote:
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  docs/devel/testing.rst | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> index 209f9d8172..fe0112b21c 100644
> --- a/docs/devel/testing.rst
> +++ b/docs/devel/testing.rst
> @@ -790,6 +790,32 @@ and hypothetical example follows:
>  At test "tear down", ``avocado_qemu.Test`` handles all the QEMUMachines
>  shutdown.
>
> +The ``avocado_qemu.LinuxTest`` base test class
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +The ``avocado_qemu.LinuxTest`` is further specialization of the
> +``avocado_qemu.Test`` class, so it contains all the characteristics of
> +the later plus some extra features.
> +
> +First of all, this base class is intended for tests that need to
> +interact with a fully booted and operational Linux guest.  The most
> +basic example looks like this:
> +
> +.. code::
> +
> +  from avocado_qemu import LinuxTest
> +
> +
> +  class SomeTest(LinuxTest):
> +
> +      def test(self):
> +          self.launch_and_wait()
> +          self.ssh_connect('root', self.ssh_key)
> +          self.ssh_command('some_command_to_be_run_in_the_guest')
> +
> +Please refer to tests that use ``avocado_qemu.LinuxTest`` under
> +``tests/acceptance`` for more examples.
> +
>  QEMUMachine
>  ~~~~~~~~~~~
>
> --
> 2.25.4
>
>


-- 
Marc-André Lureau

Re: [PATCH 21/22] Acceptance Tests: introduce CPU hotplug test

[Marc-André Lureau]

On Wed, Feb 3, 2021 at 9:51 PM Cleber Rosa <crosa@redhat.com> wrote:
>
> Even though there are qtest based tests for hotplugging CPUs (from
> which this test took some inspiration from), this one adds checks
> from a Linux guest point of view.
>
> It should also serve as an example for tests that follow a similar
> pattern and need to interact with QEMU (via qmp) and with the Linux
> guest via SSH.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  tests/acceptance/hotplug_cpu.py | 38 +++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 tests/acceptance/hotplug_cpu.py
>
> diff --git a/tests/acceptance/hotplug_cpu.py b/tests/acceptance/hotplug_cpu.py
> new file mode 100644
> index 0000000000..a22b264b4b
> --- /dev/null
> +++ b/tests/acceptance/hotplug_cpu.py
> @@ -0,0 +1,38 @@
> +# Functional test that hotplugs a CPU and checks it on a Linux guest
> +#
> +# Copyright (c) 2021 Red Hat, Inc.
> +#
> +# Author:
> +#  Cleber Rosa <crosa@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 avocado_qemu import LinuxTest
> +
> +
> +class HotPlugCPU(LinuxTest):
> +
> +    def test(self):
> +        """
> +        :avocado: tags=arch:x86_64
> +        :avocado: tags=machine:q35
> +        :avocado: tags=accel:kvm
> +        """
> +        self.require_accelerator('kvm')
> +        self.vm.add_args('-accel', 'kvm')
> +        self.vm.add_args('-cpu', 'Haswell')
> +        self.vm.add_args('-smp', '1,sockets=1,cores=2,threads=1,maxcpus=2')
> +        self.launch_and_wait()
> +
> +        self.ssh_connect('root', self.ssh_key)
> +        self.ssh_command('test -e /sys/devices/system/cpu/cpu0')
> +        with self.assertRaises(AssertionError):
> +            self.ssh_command('test -e /sys/devices/system/cpu/cpu1')
> +
> +        self.vm.command('device_add',
> +                        driver='Haswell-x86_64-cpu',
> +                        socket_id=0,
> +                        core_id=1,
> +                        thread_id=0)
> +        self.ssh_command('test -e /sys/devices/system/cpu/cpu1')
> --
> 2.25.4
>
>


-- 
Marc-André Lureau

Re: [PATCH 22/22] [NOTFORMERGE] Bump Avocado version to latest master

[Marc-André Lureau]

Hi

On Wed, Feb 3, 2021 at 9:41 PM Cleber Rosa <crosa@redhat.com> wrote:
>
> This is intende to be replaced by a bump to Avocado 85.0, to be
> released by Feb 8 2021.
>
> Latest master contains an improvement to "avocado.utils.vmimage" that
> let's it download older Fedora images from the archive locations.
> That allows the currently set Fedora 31 images to continue being used.
>
> Reference: https://github.com/avocado-framework/avocado/milestone/11
> Reference: https://github.com/avocado-framework/avocado/commit/e4d67e96ee9563436dcf7dd83902723576657d29
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Although it's not a panacea to use its API, you may consider using
osinfo in the future. I gave it a 5min try to have:

import gi
gi.require_version('Libosinfo', '1.0')
from gi.repository import Libosinfo

l = Libosinfo.Loader.new()
l.process_system_path()
db = l.get_db()
os = db.get_os_list().get_elements()
for o in os:
    for l in o.get_image_list().get_elements():
        print(l.get_url())

> ---
>  tests/requirements.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/requirements.txt b/tests/requirements.txt
> index 62e8ffd28c..8ddf0d8c86 100644
> --- a/tests/requirements.txt
> +++ b/tests/requirements.txt
> @@ -1,5 +1,5 @@
>  # Add Python module requirements, one per line, to be installed
>  # in the tests/venv Python virtual environment. For more info,
>  # refer to: https://pip.pypa.io/en/stable/user_guide/#id1
> -avocado-framework==83.0
> +git+https://github.com/avocado-framework/avocado@9b372adeec6dceaba276ca4729f2e5a2fcccaa4c#egg=avocado_framework
>  pycdlib==1.11.0
> --
> 2.25.4
>
>


-- 
Marc-André Lureau

Re: [PATCH 20/22] Acceptance Tests: add basic documentation on LinuxTest base class

[Willian Rampazzo]

On Wed, Feb 3, 2021 at 2:25 PM Cleber Rosa <crosa@redhat.com> wrote:
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  docs/devel/testing.rst | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

Re: [PATCH 17/22] Acceptance Tests: fix population of public key in cloudinit image

[Wainer dos Santos Moschetta]

On 2/3/21 2:23 PM, Cleber Rosa wrote:
> Currently the path of the ssh public key is being set, but its
> content is obviously what's needed.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/acceptance/avocado_qemu/__init__.py | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)


Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index 472088ae7d..8156224625 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -337,13 +337,15 @@ class LinuxTest(Test, LinuxSSHMixIn):
>           try:
>               cloudinit_iso = os.path.join(self.workdir, 'cloudinit.iso')
>               self.phone_home_port = network.find_free_port()
> +            with open(ssh_pubkey) as pubkey:
> +                pubkey_content = pubkey.read()
>               cloudinit.iso(cloudinit_iso, self.name,
>                             username='root',
>                             password='password',
>                             # QEMU's hard coded usermode router address
>                             phone_home_host='10.0.2.2',
>                             phone_home_port=self.phone_home_port,
> -                          authorized_key=ssh_pubkey)
> +                          authorized_key=pubkey_content)
>           except Exception:
>               self.cancel('Failed to prepare the cloudinit image')
>           return cloudinit_iso

Re: [PATCH 00/22] Acceptance Test: introduce base class for Linux based tests

[Wainer dos Santos Moschetta]

Hi,

On 2/8/21 8:35 AM, Philippe Mathieu-Daudé wrote:
> On 2/3/21 6:23 PM, Cleber Rosa wrote:
>> This introduces a base class for tests that need to interact with a
>> Linux guest.  It generalizes the "boot_linux.py" code, already been
>> used by the "virtiofs_submounts.py" and also SSH related code being
>> used by that and "linux_ssh_mips_malta.py".
>>
>> While at it, a number of fixes on hopeful improvements to those tests
>> were added.
>>
>> Cleber Rosa (22):
>>    tests/acceptance/boot_linux.py: fix typo on cloudinit error message
>>    tests/acceptance/boot_linux.py: rename misleading cloudinit method
>>    Acceptance Tests: remove unnecessary tag from documentation example
>>    tests/acceptance/virtiofs_submounts.py: use workdir property
>>    tests/acceptance/virtiofs_submounts.py: do not ask for ssh key
>>      password
>>    tests/acceptance/virtiofs_submounts.py: use a virtio-net device
>>      instead
>>    tests/acceptance/virtiofs_submounts.py: evaluate string not length
>>    tests/acceptance/virtiofs_submounts.py: standardize port as integer
>>    tests/acceptance/virtiofs_submounts.py: required space between IP and
>>      port
>>    Python: add utility function for retrieving port redirection
>>    tests/acceptance/linux_ssh_mips_malta.py: standardize port as integer
>>    Acceptance tests: clarify ssh connection failure reason
>>    tests/acceptance/virtiofs_submounts.py: add missing accel tag
>>    Acceptance Tests: introduce LinuxTest base class
>>    Acceptance Tests: move useful ssh methods to base class
>>    Acceptance Tests: introduce method for requiring an accelerator
>>    Acceptance Tests: fix population of public key in cloudinit image
>>    Acceptance Tests: set up existing ssh keys by default
>>    Acceptance Tests: add port redirection for ssh by default
>>    Acceptance Tests: add basic documentation on LinuxTest base class
>>    Acceptance Tests: introduce CPU hotplug test
>>    [NOTFORMERGE] Bump Avocado version to latest master
>>
>>   docs/devel/testing.rst                    |  29 +++-
>>   python/qemu/utils.py                      |  35 +++++
>>   tests/acceptance/avocado_qemu/__init__.py | 176 ++++++++++++++++++++++
>>   tests/acceptance/boot_linux.py            | 128 ++--------------
>>   tests/acceptance/hotplug_cpu.py           |  38 +++++
>>   tests/acceptance/info_usernet.py          |  29 ++++
>>   tests/acceptance/linux_ssh_mips_malta.py  |  44 +-----
>>   tests/acceptance/virtiofs_submounts.py    |  73 +--------
>>   tests/requirements.txt                    |   2 +-
>>   tests/vm/basevm.py                        |   7 +-
>>   10 files changed, 334 insertions(+), 227 deletions(-)
>>   create mode 100644 python/qemu/utils.py
>>   create mode 100644 tests/acceptance/hotplug_cpu.py
>>   create mode 100644 tests/acceptance/info_usernet.py
> Patches 1-6, 8-9 & 12 queued.


Those are merged. Most of the remaining patches got at least one review, 
so could you (Cleber or Philippe) open a pull request for them as well? 
Telling it because there are many series in flight for the acceptance 
tests, and to avoid conflicts with future series...

Thanks!

- Wainer


>
>

Re: [PATCH 00/22] Acceptance Test: introduce base class for Linux based tests

[Philippe Mathieu-Daudé]

On 2/15/21 4:49 PM, Wainer dos Santos Moschetta wrote:
> Hi,
> 
> On 2/8/21 8:35 AM, Philippe Mathieu-Daudé wrote:
>> On 2/3/21 6:23 PM, Cleber Rosa wrote:
>>> This introduces a base class for tests that need to interact with a
>>> Linux guest.  It generalizes the "boot_linux.py" code, already been
>>> used by the "virtiofs_submounts.py" and also SSH related code being
>>> used by that and "linux_ssh_mips_malta.py".
>>>
>>> While at it, a number of fixes on hopeful improvements to those tests
>>> were added.
>>>
>>> Cleber Rosa (22):
>>>    tests/acceptance/boot_linux.py: fix typo on cloudinit error message
>>>    tests/acceptance/boot_linux.py: rename misleading cloudinit method
>>>    Acceptance Tests: remove unnecessary tag from documentation example
>>>    tests/acceptance/virtiofs_submounts.py: use workdir property
>>>    tests/acceptance/virtiofs_submounts.py: do not ask for ssh key
>>>      password
>>>    tests/acceptance/virtiofs_submounts.py: use a virtio-net device
>>>      instead
>>>    tests/acceptance/virtiofs_submounts.py: evaluate string not length
>>>    tests/acceptance/virtiofs_submounts.py: standardize port as integer
>>>    tests/acceptance/virtiofs_submounts.py: required space between IP and
>>>      port
>>>    Python: add utility function for retrieving port redirection
>>>    tests/acceptance/linux_ssh_mips_malta.py: standardize port as integer
>>>    Acceptance tests: clarify ssh connection failure reason
>>>    tests/acceptance/virtiofs_submounts.py: add missing accel tag
>>>    Acceptance Tests: introduce LinuxTest base class
>>>    Acceptance Tests: move useful ssh methods to base class
>>>    Acceptance Tests: introduce method for requiring an accelerator
>>>    Acceptance Tests: fix population of public key in cloudinit image
>>>    Acceptance Tests: set up existing ssh keys by default
>>>    Acceptance Tests: add port redirection for ssh by default
>>>    Acceptance Tests: add basic documentation on LinuxTest base class
>>>    Acceptance Tests: introduce CPU hotplug test
>>>    [NOTFORMERGE] Bump Avocado version to latest master
>>>
>>>   docs/devel/testing.rst                    |  29 +++-
>>>   python/qemu/utils.py                      |  35 +++++
>>>   tests/acceptance/avocado_qemu/__init__.py | 176 ++++++++++++++++++++++
>>>   tests/acceptance/boot_linux.py            | 128 ++--------------
>>>   tests/acceptance/hotplug_cpu.py           |  38 +++++
>>>   tests/acceptance/info_usernet.py          |  29 ++++
>>>   tests/acceptance/linux_ssh_mips_malta.py  |  44 +-----
>>>   tests/acceptance/virtiofs_submounts.py    |  73 +--------
>>>   tests/requirements.txt                    |   2 +-
>>>   tests/vm/basevm.py                        |   7 +-
>>>   10 files changed, 334 insertions(+), 227 deletions(-)
>>>   create mode 100644 python/qemu/utils.py
>>>   create mode 100644 tests/acceptance/hotplug_cpu.py
>>>   create mode 100644 tests/acceptance/info_usernet.py
>> Patches 1-6, 8-9 & 12 queued.
> 
> 
> Those are merged. Most of the remaining patches got at least one review,
> so could you (Cleber or Philippe) open a pull request for them as well?
> Telling it because there are many series in flight for the acceptance
> tests, and to avoid conflicts with future series...

I asked a question to Cleber in patch 13 and am waiting what he meant
before queuing the series (fixing the typo Marc-André noticed).

Regards,

Phil.

Re: [PATCH 13/22] tests/acceptance/virtiofs_submounts.py: add missing accel tag

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

On Mon, Feb 08, 2021 at 12:28:22PM +0100, Philippe Mathieu-Daudé wrote:
> On 2/3/21 6:23 PM, Cleber Rosa wrote:
> > Which is useful to select tests that depend/use a particular feature.
> 
> Is that a question?
>

No, it's not a question.

I meant: "add a tag which is useful to select tests...".  I guess this
statement followed a similar previous broken approach of mine, reusing
the commit title to give it meaning.

I'll try to keep avoid that in the future, thanks for asking about it.

> Why keep this last in your series?
>

No reason... it doesn't impact any pre or post patches in the series
AFAICT.

Cheers,
- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 07/22] tests/acceptance/virtiofs_submounts.py: evaluate string not length

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 2195 bytes --]

On Tue, Feb 09, 2021 at 06:15:26PM +0100, Philippe Mathieu-Daudé wrote:
> > 
> > I've actually done this with some Xen patches I'm working on at the
> > moment. I'll probably decorate the test with:
> > 
> >   @skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted code')
> > 
> > with a comment explaining what's waiting to be upstreamed. Once there
> > are upstream binaries I plan on transitioning the test to those.
> 
> Instead of a binary AVOCADO_ALLOW_UNTRUSTED_CODE variable, we could
> have a list allowed domains/namespaces, that can be increased on the
> tester discretion.
> 
> For example these are assumed trusted:
> 
> . archives.fedoraproject.org
> . archive.debian.org
> . cdn.netbsd.org
> . github.com/torvalds
> . people.debian.org/~aurel32
> . snapshot.debian.org
> . storage.kernelci.org
> . www.qemu-advent-calendar.org
> 
> Then personally interested in testing ARM boards I'd amend:
> 
> . apt.armbian.com
> . github.com/philmd
> . github.com/groeck
> . github.com/hskinnemoen
> . github.com/pbatard
> 
> and Max's repo since I'm interested in testing virtiofs_submounts.
> 

Hi Phil,

I think I follow your idea, but I see two issues here:

 1) Functional area (subsystem / architecture / machine type, etc)
 2) Trustfulness of the code

WRT 1, the domains do not contain meaning onto themselves, so a
secondary mapping of subsystem/architecture/machine to the domain
would be needed.  Also, wouldn't it be common to end up needing a N:N
mapping between domains and subsystem/architecture/machine?

WRT 2, while limiting download from a number of domains can add some
protection, the ultimate trust is achieved by setting a hash to the
exact code we will download/run.

If those points seem valid, then I believe it's better to continue
thinking of subsystem/architecture/machine because of the usability
aspects, and possibly improve the perceived level of trust/stability
of the assets by adding a "tier" classification.  That one, one could
pick, say:

 * board|machine_type == "foo" AND
 * tier == 1

And exclude what is considered inferior tiers.  How does that sound?

Regards,
- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 10/22] Python: add utility function for retrieving port redirection

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 7921 bytes --]

On Tue, Feb 09, 2021 at 11:50:51AM -0300, Wainer dos Santos Moschetta wrote:
> Hi,
> 
> On 2/3/21 2:23 PM, Cleber Rosa wrote:
> > Slightly different versions for the same utility code are currently
> > present on different locations.  This unifies them all, giving
> > preference to the version from virtiofs_submounts.py, because of the
> > last tweaks added to it.
> > 
> > While at it, this adds a "qemu.util" module to host the utility
> > function and a test.
> > 
> > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > ---
> >   python/qemu/utils.py                     | 35 ++++++++++++++++++++++++
> >   tests/acceptance/info_usernet.py         | 29 ++++++++++++++++++++
> >   tests/acceptance/linux_ssh_mips_malta.py | 16 +++++------
> >   tests/acceptance/virtiofs_submounts.py   | 20 +++-----------
> >   tests/vm/basevm.py                       |  7 ++---
> >   5 files changed, 77 insertions(+), 30 deletions(-)
> >   create mode 100644 python/qemu/utils.py
> >   create mode 100644 tests/acceptance/info_usernet.py
> 
> I've a few suggestions:
> 
> - IMHO "utils" is too broad, people may start throwing random stuffs there.
> Maybe call it "net". I am sure there will be more net-related code to be
> clustered on that module in the future.
>

It's hard to find the right balance here.  If you take a look at what John
is proposing wrt the packaging the "qemu" Python libs, I believe one module
is a good compromise at this point.  I really to expect that it will grow
and that more modules will be created.

> - Rename the method to "get_hostfwd_port()" because the parameter's name
> already implies it is expected an "info usernet" output.
>

I thought about the method name, and decided to keep the more verbose name
because this method is *not* about retrieving the "hostfwd port" from a
running QEMU, but rather from the output that should be produced by a "info
usernet" command.  It may become the foundation of a method on the QEMUMachine
class that is called "get_hostfwd_port()" as you suggested, that includes
getting the data (that is, issuing a "info usernet" command).

Anyway, I tend to favor "explicit is better than implicit" approach, but
I recognize that I can be too verbose at times.  Let's see if other people
chip in with naming suggestions.

> - Drop the InfoUsernet Test. It is too simple, and the same functionality is
> tested indirectly by other tests.
>

I find "too simple" is a typical case of "famous last words" :D.
Now, as a functional test to cover the utility function, it's indeed
a bit of overkill.  It'd probably be less necessary if there were unittests
for those (and there will hopefully be some soon).

Now, testing *directly* was indeed the intention here. I see a few
reasons for that, including saving a good amount of debugging time:
such a test failing would provide *direct* indication of where the
regression is.  These simpler tests can also be run with targets other
than the ones really connecting into guests at this moment (while it's
debatable wether such a regression would appear only on such targets).

> > 
> > diff --git a/python/qemu/utils.py b/python/qemu/utils.py
> > new file mode 100644
> > index 0000000000..89a246ab30
> > --- /dev/null
> > +++ b/python/qemu/utils.py
> > @@ -0,0 +1,35 @@
> > +"""
> > +QEMU utility library
> > +
> > +This offers miscellaneous utility functions, which may not be easily
> > +distinguishable or numerous to be in their own module.
> > +"""
> > +
> > +# Copyright (C) 2021 Red Hat Inc.
> > +#
> > +# Authors:
> > +#  Cleber Rosa <crosa@redhat.com>
> > +#
> > +# This work is licensed under the terms of the GNU GPL, version 2.  See
> > +# the COPYING file in the top-level directory.
> > +#
> > +
> > +import re
> > +from typing import Optional
> > +
> > +
> > +def get_info_usernet_hostfwd_port(info_usernet_output: str) -> Optional[int]:
> > +    """
> > +    Returns the port given to the hostfwd parameter via info usernet
> > +
> > +    :param info_usernet_output: output generated by hmp command "info usernet"
> > +    :param info_usernet_output: str
> > +    :return: the port number allocated by the hostfwd option
> > +    :rtype: int
> > +    """
> > +    for line in info_usernet_output.split('\r\n'):
> > +        regex = r'TCP.HOST_FORWARD.*127\.0\.0\.1\s+(\d+)\s+10\.'
> > +        match = re.search(regex, line)
> > +        if match is not None:
> > +            return int(match[1])
> > +    return None
> > diff --git a/tests/acceptance/info_usernet.py b/tests/acceptance/info_usernet.py
> > new file mode 100644
> > index 0000000000..9c1fd903a0
> > --- /dev/null
> > +++ b/tests/acceptance/info_usernet.py
> > @@ -0,0 +1,29 @@
> > +# Test for the hmp command "info usernet"
> > +#
> > +# Copyright (c) 2021 Red Hat, Inc.
> > +#
> > +# Author:
> > +#  Cleber Rosa <crosa@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 avocado_qemu import Test
> > +
> > +from qemu.utils import get_info_usernet_hostfwd_port
> > +
> > +
> > +class InfoUsernet(Test):
> > +
> > +    def test_hostfwd(self):
> > +        self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22')
> > +        self.vm.launch()
> > +        res = self.vm.command('human-monitor-command',
> > +                              command_line='info usernet')
> > +        port = get_info_usernet_hostfwd_port(res)
> > +        self.assertIsNotNone(port,
> > +                             ('"info usernet" output content does not seem to '
> > +                              'contain the redirected port'))
> > +        self.assertGreater(port, 0,
> > +                           ('Found a redirected port that is not greater than'
> > +                            ' zero'))
> > diff --git a/tests/acceptance/linux_ssh_mips_malta.py b/tests/acceptance/linux_ssh_mips_malta.py
> > index 25c5c5f741..275659c785 100644
> > --- a/tests/acceptance/linux_ssh_mips_malta.py
> > +++ b/tests/acceptance/linux_ssh_mips_malta.py
> > @@ -18,6 +18,8 @@ from avocado.utils import process
> >   from avocado.utils import archive
> >   from avocado.utils import ssh
> > +from qemu.utils import get_info_usernet_hostfwd_port
> > +
> >   class LinuxSSH(Test):
> > @@ -70,18 +72,14 @@ class LinuxSSH(Test):
> >       def setUp(self):
> >           super(LinuxSSH, self).setUp()
> > -    def get_portfwd(self):
> > +    def ssh_connect(self, username, password):
> > +        self.ssh_logger = logging.getLogger('ssh')
> >           res = self.vm.command('human-monitor-command',
> >                                 command_line='info usernet')
> > -        line = res.split('\r\n')[2]
> > -        port = re.split(r'.*TCP.HOST_FORWARD.*127\.0\.0\.1 (\d+)\s+10\..*',
> > -                        line)[1]
> > +        port = get_info_usernet_hostfwd_port(res)
> > +        if not port:
> > +            self.cancel("Failed to retrieve SSH port")
> 
> Here I think it should assert not none, otherwise there is a bug somewhere.
>
> - Wainer

I'm trying to be careful and conservative with adding assertions,
because IMO those belong to test writers.  IMO the expectation of a
user writing a test using "ssh_connect()" as a framework utility,
would be more aligned with the framework bailing out, than blatantly
setting a test failure.

It's similar to what happens if a "vmimage" snapshot can not be
created... there's an issue somewhere, but it'd be a bit unfair, and
confusing, to set a assertion error (and thus test failure).  But, I
think this is the kind of design decision that will evolve with (more)
time here.

Let me know if my explanations make sense and change your mind
any bit :).

Thanks for the review!
- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 11/22] tests/acceptance/linux_ssh_mips_malta.py: standardize port as integer

[Willian Rampazzo]

On Wed, Feb 3, 2021 at 2:24 PM Cleber Rosa <crosa@redhat.com> wrote:
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/linux_ssh_mips_malta.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

Re: [PATCH 14/22] Acceptance Tests: introduce LinuxTest base class

[Willian Rampazzo]

On Wed, Feb 3, 2021 at 2:24 PM Cleber Rosa <crosa@redhat.com> wrote:
>
> This is basically the infrastructure around "boot_linux.py" tests, but
> now made into a base class for general use.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/avocado_qemu/__init__.py | 87 +++++++++++++++++++++
>  tests/acceptance/boot_linux.py            | 94 ++---------------------
>  tests/acceptance/virtiofs_submounts.py    |  6 +-
>  3 files changed, 94 insertions(+), 93 deletions(-)
>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index bf54e419da..b06692a59d 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py

I found it not so intuitive to have the base class defined here. I see
the number of base classes for the tests growing in the future. A
common place for base classes for tests would be better, just like the
`qemu.utils` you are defining somewhere else. Anyway, this is a design
decision that can be changed later, so

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

Re: [PATCH 15/22] Acceptance Tests: move useful ssh methods to base class

[Willian Rampazzo]

On Wed, Feb 3, 2021 at 2:24 PM Cleber Rosa <crosa@redhat.com> wrote:
>
> Both the virtiofs submounts and the linux ssh mips malta tests
> contains useful methods related to ssh that deserve to be made
> available to other tests.  Let's move them to the base LinuxTest
> class.
>
> The method that helps with setting up an ssh connection will now
> support both key and password based authentication, defaulting to key
> based.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/avocado_qemu/__init__.py | 49 ++++++++++++++++++++++-
>  tests/acceptance/linux_ssh_mips_malta.py  | 38 ++----------------
>  tests/acceptance/virtiofs_submounts.py    | 36 -----------------
>  3 files changed, 51 insertions(+), 72 deletions(-)
>

Here I bring back my comment from patch 14, having a common place for
test specific classes would bring a better organization to the code.
Anyway,

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

Re: [PATCH 16/22] Acceptance Tests: introduce method for requiring an accelerator

[Willian Rampazzo]

On Wed, Feb 3, 2021 at 2:25 PM Cleber Rosa <crosa@redhat.com> wrote:
>
> Some tests explicitly require a QEMU accelerator to be available.
> Given that this depends on some runtime aspects not known before
> the test is started, such as the currently set QEMU binary, it's
> left to be checked also at runtime.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/avocado_qemu/__init__.py | 24 ++++++++++++++++
>  tests/acceptance/boot_linux.py            | 34 ++++++-----------------
>  tests/acceptance/virtiofs_submounts.py    |  5 +---
>  3 files changed, 34 insertions(+), 29 deletions(-)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

Re: [PATCH 17/22] Acceptance Tests: fix population of public key in cloudinit image

[Willian Rampazzo]

On Wed, Feb 3, 2021 at 2:25 PM Cleber Rosa <crosa@redhat.com> wrote:
>
> Currently the path of the ssh public key is being set, but its
> content is obviously what's needed.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/avocado_qemu/__init__.py | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

Re: [PATCH 18/22] Acceptance Tests: set up existing ssh keys by default

[Willian Rampazzo]

On Wed, Feb 3, 2021 at 2:25 PM Cleber Rosa <crosa@redhat.com> wrote:
>
> It's questionable wether it's necessary to create one brand new pair
> for each test.  It's not questionable that it takes less time and
> resources to just use the keys available at "tests/keys" that exist
> for that exact reason.
>
> If a location for the public key is not given explicitly, the
> LinuxTest will now set up the existing pair of keys as the default.
> This removes the need for a lot of boiler plate code.
>
> To avoid the ssh client from erroring on permission issues, a
> directory with restricive permissions is created for the private key.
> This should still be a lot cheaper than creating a new key.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/avocado_qemu/__init__.py | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

Re: [PATCH 10/22] Python: add utility function for retrieving port redirection

[John Snow]

On 2/15/21 1:27 PM, Cleber Rosa wrote:
> It's hard to find the right balance here.  If you take a look at what John
> is proposing wrt the packaging the "qemu" Python libs, I believe one module
> is a good compromise at this point.  I really to expect that it will grow
> and that more modules will be created.
> 

Yeah. We have a "qmp" package and a "machine" package, and these seem 
very well-defined.

Then we have everything else which is mostly a few random bits and 
pieces (at the moment: just accel.py). Over time those bits and pieces 
might take shape as something more important/meaningful, but for now 
it's pretty nebulous.

An emerging pattern I see is that these functions are "environment 
analysis" helpers; things designed to help interrogate the local 
environment to choose appropriate QEMU flags, or otherwise "QEMU output 
analysis", things designed to make better sense of the output received 
from QEMU.

accel.py is the former, this patch targets the latter.

I suspect accel.py will want to belong to "smart" tools for booting up 
an arbitrary VM (interrogate, decide on config, launch) whereas this 
patch fits into a class of API-esque tools designed for making sense of 
I/O information.

It's meant to digest HMP, though -- is this evidence that we need a 
better QMP command? We shouldn't be using HMP for machine driven 
testing. (I know we have to sometimes, but we should be working towards 
eliminating it.)

-js

Re: [PATCH 21/22] Acceptance Tests: introduce CPU hotplug test

[Willian Rampazzo]

On Wed, Feb 3, 2021 at 2:25 PM Cleber Rosa <crosa@redhat.com> wrote:
>
> Even though there are qtest based tests for hotplugging CPUs (from
> which this test took some inspiration from), this one adds checks
> from a Linux guest point of view.
>
> It should also serve as an example for tests that follow a similar
> pattern and need to interact with QEMU (via qmp) and with the Linux
> guest via SSH.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/hotplug_cpu.py | 38 +++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 tests/acceptance/hotplug_cpu.py
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

Re: [PATCH 13/22] tests/acceptance/virtiofs_submounts.py: add missing accel tag

[Willian Rampazzo]

On Wed, Feb 3, 2021 at 2:24 PM Cleber Rosa <crosa@redhat.com> wrote:
>
> Which is useful to select tests that depend/use a particular feature.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/virtiofs_submounts.py | 1 +
>  1 file changed, 1 insertion(+)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

Re: [PATCH 10/22] Python: add utility function for retrieving port redirection

[Wainer dos Santos Moschetta]

On 2/15/21 3:27 PM, Cleber Rosa wrote:
> On Tue, Feb 09, 2021 at 11:50:51AM -0300, Wainer dos Santos Moschetta wrote:
>> Hi,
>>
>> On 2/3/21 2:23 PM, Cleber Rosa wrote:
>>> Slightly different versions for the same utility code are currently
>>> present on different locations.  This unifies them all, giving
>>> preference to the version from virtiofs_submounts.py, because of the
>>> last tweaks added to it.
>>>
>>> While at it, this adds a "qemu.util" module to host the utility
>>> function and a test.
>>>
>>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>>> ---
>>>    python/qemu/utils.py                     | 35 ++++++++++++++++++++++++
>>>    tests/acceptance/info_usernet.py         | 29 ++++++++++++++++++++
>>>    tests/acceptance/linux_ssh_mips_malta.py | 16 +++++------
>>>    tests/acceptance/virtiofs_submounts.py   | 20 +++-----------
>>>    tests/vm/basevm.py                       |  7 ++---
>>>    5 files changed, 77 insertions(+), 30 deletions(-)
>>>    create mode 100644 python/qemu/utils.py
>>>    create mode 100644 tests/acceptance/info_usernet.py
>> I've a few suggestions:
>>
>> - IMHO "utils" is too broad, people may start throwing random stuffs there.
>> Maybe call it "net". I am sure there will be more net-related code to be
>> clustered on that module in the future.
>>
> It's hard to find the right balance here.  If you take a look at what John
> is proposing wrt the packaging the "qemu" Python libs, I believe one module
> is a good compromise at this point.  I really to expect that it will grow
> and that more modules will be created.
>
>> - Rename the method to "get_hostfwd_port()" because the parameter's name
>> already implies it is expected an "info usernet" output.
>>
> I thought about the method name, and decided to keep the more verbose name
> because this method is *not* about retrieving the "hostfwd port" from a
> running QEMU, but rather from the output that should be produced by a "info
> usernet" command.  It may become the foundation of a method on the QEMUMachine
> class that is called "get_hostfwd_port()" as you suggested, that includes
> getting the data (that is, issuing a "info usernet" command).
>
> Anyway, I tend to favor "explicit is better than implicit" approach, but
> I recognize that I can be too verbose at times.  Let's see if other people
> chip in with naming suggestions.
>
>> - Drop the InfoUsernet Test. It is too simple, and the same functionality is
>> tested indirectly by other tests.
>>
> I find "too simple" is a typical case of "famous last words" :D.
> Now, as a functional test to cover the utility function, it's indeed
> a bit of overkill.  It'd probably be less necessary if there were unittests
> for those (and there will hopefully be some soon).
>
> Now, testing *directly* was indeed the intention here. I see a few
> reasons for that, including saving a good amount of debugging time:
> such a test failing would provide *direct* indication of where the
> regression is.  These simpler tests can also be run with targets other
> than the ones really connecting into guests at this moment (while it's
> debatable wether such a regression would appear only on such targets).

I confess I was thinking on the impact of many small/simple tests on CI 
if that become a trend. Time to run a job should be manageable. In this 
case, having unittests for the utilities is all we need and would save 
some minutes on CI.

On the absence of unittests, I'm not opposed to merge this test.

>
>>> diff --git a/python/qemu/utils.py b/python/qemu/utils.py
>>> new file mode 100644
>>> index 0000000000..89a246ab30
>>> --- /dev/null
>>> +++ b/python/qemu/utils.py
>>> @@ -0,0 +1,35 @@
>>> +"""
>>> +QEMU utility library
>>> +
>>> +This offers miscellaneous utility functions, which may not be easily
>>> +distinguishable or numerous to be in their own module.
>>> +"""
>>> +
>>> +# Copyright (C) 2021 Red Hat Inc.
>>> +#
>>> +# Authors:
>>> +#  Cleber Rosa <crosa@redhat.com>
>>> +#
>>> +# This work is licensed under the terms of the GNU GPL, version 2.  See
>>> +# the COPYING file in the top-level directory.
>>> +#
>>> +
>>> +import re
>>> +from typing import Optional
>>> +
>>> +
>>> +def get_info_usernet_hostfwd_port(info_usernet_output: str) -> Optional[int]:
>>> +    """
>>> +    Returns the port given to the hostfwd parameter via info usernet
>>> +
>>> +    :param info_usernet_output: output generated by hmp command "info usernet"
>>> +    :param info_usernet_output: str
>>> +    :return: the port number allocated by the hostfwd option
>>> +    :rtype: int
>>> +    """
>>> +    for line in info_usernet_output.split('\r\n'):
>>> +        regex = r'TCP.HOST_FORWARD.*127\.0\.0\.1\s+(\d+)\s+10\.'
>>> +        match = re.search(regex, line)
>>> +        if match is not None:
>>> +            return int(match[1])
>>> +    return None
>>> diff --git a/tests/acceptance/info_usernet.py b/tests/acceptance/info_usernet.py
>>> new file mode 100644
>>> index 0000000000..9c1fd903a0
>>> --- /dev/null
>>> +++ b/tests/acceptance/info_usernet.py
>>> @@ -0,0 +1,29 @@
>>> +# Test for the hmp command "info usernet"
>>> +#
>>> +# Copyright (c) 2021 Red Hat, Inc.
>>> +#
>>> +# Author:
>>> +#  Cleber Rosa <crosa@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 avocado_qemu import Test
>>> +
>>> +from qemu.utils import get_info_usernet_hostfwd_port
>>> +
>>> +
>>> +class InfoUsernet(Test):
>>> +
>>> +    def test_hostfwd(self):
>>> +        self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22')
>>> +        self.vm.launch()
>>> +        res = self.vm.command('human-monitor-command',
>>> +                              command_line='info usernet')
>>> +        port = get_info_usernet_hostfwd_port(res)
>>> +        self.assertIsNotNone(port,
>>> +                             ('"info usernet" output content does not seem to '
>>> +                              'contain the redirected port'))
>>> +        self.assertGreater(port, 0,
>>> +                           ('Found a redirected port that is not greater than'
>>> +                            ' zero'))
>>> diff --git a/tests/acceptance/linux_ssh_mips_malta.py b/tests/acceptance/linux_ssh_mips_malta.py
>>> index 25c5c5f741..275659c785 100644
>>> --- a/tests/acceptance/linux_ssh_mips_malta.py
>>> +++ b/tests/acceptance/linux_ssh_mips_malta.py
>>> @@ -18,6 +18,8 @@ from avocado.utils import process
>>>    from avocado.utils import archive
>>>    from avocado.utils import ssh
>>> +from qemu.utils import get_info_usernet_hostfwd_port
>>> +
>>>    class LinuxSSH(Test):
>>> @@ -70,18 +72,14 @@ class LinuxSSH(Test):
>>>        def setUp(self):
>>>            super(LinuxSSH, self).setUp()
>>> -    def get_portfwd(self):
>>> +    def ssh_connect(self, username, password):
>>> +        self.ssh_logger = logging.getLogger('ssh')
>>>            res = self.vm.command('human-monitor-command',
>>>                                  command_line='info usernet')
>>> -        line = res.split('\r\n')[2]
>>> -        port = re.split(r'.*TCP.HOST_FORWARD.*127\.0\.0\.1 (\d+)\s+10\..*',
>>> -                        line)[1]
>>> +        port = get_info_usernet_hostfwd_port(res)
>>> +        if not port:
>>> +            self.cancel("Failed to retrieve SSH port")
>> Here I think it should assert not none, otherwise there is a bug somewhere.
>>
>> - Wainer
> I'm trying to be careful and conservative with adding assertions,
> because IMO those belong to test writers.  IMO the expectation of a
> user writing a test using "ssh_connect()" as a framework utility,
> would be more aligned with the framework bailing out, than blatantly
> setting a test failure.
>
> It's similar to what happens if a "vmimage" snapshot can not be
> created... there's an issue somewhere, but it'd be a bit unfair, and
> confusing, to set a assertion error (and thus test failure).  But, I
> think this is the kind of design decision that will evolve with (more)
> time here.
I see. I agree with that design, I will keep it in mind when reviewing 
patches and submitting changes.
>
> Let me know if my explanations make sense and change your mind
> any bit :).

Sure, thanks for the explanations. As I mentioned in my first reply, 
they were just suggestions. So,

Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


>
> Thanks for the review!
> - Cleber.

Re: [PATCH 14/22] Acceptance Tests: introduce LinuxTest base class

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 1666 bytes --]

On Mon, Feb 15, 2021 at 04:06:45PM -0300, Willian Rampazzo wrote:
> On Wed, Feb 3, 2021 at 2:24 PM Cleber Rosa <crosa@redhat.com> wrote:
> >
> > This is basically the infrastructure around "boot_linux.py" tests, but
> > now made into a base class for general use.
> >
> > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > ---
> >  tests/acceptance/avocado_qemu/__init__.py | 87 +++++++++++++++++++++
> >  tests/acceptance/boot_linux.py            | 94 ++---------------------
> >  tests/acceptance/virtiofs_submounts.py    |  6 +-
> >  3 files changed, 94 insertions(+), 93 deletions(-)
> >
> > diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> > index bf54e419da..b06692a59d 100644
> > --- a/tests/acceptance/avocado_qemu/__init__.py
> > +++ b/tests/acceptance/avocado_qemu/__init__.py
> 
> I found it not so intuitive to have the base class defined here. I see
> the number of base classes for the tests growing in the future. A
> common place for base classes for tests would be better, just like the
> `qemu.utils` you are defining somewhere else. Anyway, this is a design
> decision that can be changed later, so
>

Hi Willian,

I tend to agree, and my medium/long term vision is similar.  What I
expect to be able to do soon (this is connected to John's work) is to
have "avocado_qemu" as something like "qemu.testing.functional" which
describe its Avocado (and other) dependencies.

A "qemu.test.other" could describe its own dependencies (which may
not include Avocado).

> Reviewed-by: Willian Rampazzo <willianr@redhat.com>
> 
> 

Thanks for the review,
- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 18/22] Acceptance Tests: set up existing ssh keys by default

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 1100 bytes --]

On Thu, Feb 11, 2021 at 02:15:32PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Feb 3, 2021 at 10:07 PM Cleber Rosa <crosa@redhat.com> wrote:
> >
> > It's questionable wether it's necessary to create one brand new pair
> 
> whether
>

Yep, thanks!

> > for each test.  It's not questionable that it takes less time and
> > resources to just use the keys available at "tests/keys" that exist
> > for that exact reason.
> >
> > If a location for the public key is not given explicitly, the
> > LinuxTest will now set up the existing pair of keys as the default.
> > This removes the need for a lot of boiler plate code.
> 
> boilerplate
>

You're right again.

> >
> > To avoid the ssh client from erroring on permission issues, a
> > directory with restricive permissions is created for the private key.
> 
> restrictive
>

OK.



> > This should still be a lot cheaper than creating a new key.
> >
> > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>

Thanks for the review,
- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 00/22] Acceptance Test: introduce base class for Linux based tests

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 4069 bytes --]

On Mon, Feb 15, 2021 at 06:03:33PM +0100, Philippe Mathieu-Daudé wrote:
> On 2/15/21 4:49 PM, Wainer dos Santos Moschetta wrote:
> > Hi,
> > 
> > On 2/8/21 8:35 AM, Philippe Mathieu-Daudé wrote:
> >> On 2/3/21 6:23 PM, Cleber Rosa wrote:
> >>> This introduces a base class for tests that need to interact with a
> >>> Linux guest.  It generalizes the "boot_linux.py" code, already been
> >>> used by the "virtiofs_submounts.py" and also SSH related code being
> >>> used by that and "linux_ssh_mips_malta.py".
> >>>
> >>> While at it, a number of fixes on hopeful improvements to those tests
> >>> were added.
> >>>
> >>> Cleber Rosa (22):
> >>>    tests/acceptance/boot_linux.py: fix typo on cloudinit error message
> >>>    tests/acceptance/boot_linux.py: rename misleading cloudinit method
> >>>    Acceptance Tests: remove unnecessary tag from documentation example
> >>>    tests/acceptance/virtiofs_submounts.py: use workdir property
> >>>    tests/acceptance/virtiofs_submounts.py: do not ask for ssh key
> >>>      password
> >>>    tests/acceptance/virtiofs_submounts.py: use a virtio-net device
> >>>      instead
> >>>    tests/acceptance/virtiofs_submounts.py: evaluate string not length
> >>>    tests/acceptance/virtiofs_submounts.py: standardize port as integer
> >>>    tests/acceptance/virtiofs_submounts.py: required space between IP and
> >>>      port
> >>>    Python: add utility function for retrieving port redirection
> >>>    tests/acceptance/linux_ssh_mips_malta.py: standardize port as integer
> >>>    Acceptance tests: clarify ssh connection failure reason
> >>>    tests/acceptance/virtiofs_submounts.py: add missing accel tag
> >>>    Acceptance Tests: introduce LinuxTest base class
> >>>    Acceptance Tests: move useful ssh methods to base class
> >>>    Acceptance Tests: introduce method for requiring an accelerator
> >>>    Acceptance Tests: fix population of public key in cloudinit image
> >>>    Acceptance Tests: set up existing ssh keys by default
> >>>    Acceptance Tests: add port redirection for ssh by default
> >>>    Acceptance Tests: add basic documentation on LinuxTest base class
> >>>    Acceptance Tests: introduce CPU hotplug test
> >>>    [NOTFORMERGE] Bump Avocado version to latest master
> >>>
> >>>   docs/devel/testing.rst                    |  29 +++-
> >>>   python/qemu/utils.py                      |  35 +++++
> >>>   tests/acceptance/avocado_qemu/__init__.py | 176 ++++++++++++++++++++++
> >>>   tests/acceptance/boot_linux.py            | 128 ++--------------
> >>>   tests/acceptance/hotplug_cpu.py           |  38 +++++
> >>>   tests/acceptance/info_usernet.py          |  29 ++++
> >>>   tests/acceptance/linux_ssh_mips_malta.py  |  44 +-----
> >>>   tests/acceptance/virtiofs_submounts.py    |  73 +--------
> >>>   tests/requirements.txt                    |   2 +-
> >>>   tests/vm/basevm.py                        |   7 +-
> >>>   10 files changed, 334 insertions(+), 227 deletions(-)
> >>>   create mode 100644 python/qemu/utils.py
> >>>   create mode 100644 tests/acceptance/hotplug_cpu.py
> >>>   create mode 100644 tests/acceptance/info_usernet.py
> >> Patches 1-6, 8-9 & 12 queued.
> > 
> > 
> > Those are merged. Most of the remaining patches got at least one review,
> > so could you (Cleber or Philippe) open a pull request for them as well?
> > Telling it because there are many series in flight for the acceptance
> > tests, and to avoid conflicts with future series...
> 
> I asked a question to Cleber in patch 13 and am waiting what he meant
> before queuing the series (fixing the typo Marc-André noticed).
> 
> Regards,
> 
> Phil.
> 

Hi Phil,

Thanks for taking the previous patches, and thanks for the willingness
to take (and tweak) some more.  I only saw this comment now, and I
have a bunch of these ready to fire a PR.

I guess I also provided an answer to the question you're referring to.

If my tree passes CI, then I should be sending a PR within the next
few hours.

Best,
- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 19/22] Acceptance Tests: add port redirection for ssh by default

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 1582 bytes --]

On Wed, Feb 03, 2021 at 06:51:42PM +0100, Philippe Mathieu-Daudé wrote:
> On 2/3/21 6:46 PM, Philippe Mathieu-Daudé wrote:
> > On 2/3/21 6:23 PM, Cleber Rosa wrote:
> >> For users of the LinuxTest class, let's set up the VM with the port
> >> redirection for SSH, instead of requiring each test to set the same
> >> arguments.
> >>
> >> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> >> ---
> >>  tests/acceptance/avocado_qemu/__init__.py | 2 ++
> >>  tests/acceptance/virtiofs_submounts.py    | 4 ----
> >>  2 files changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> >> index 5f4dd6b9ec..89669cc604 100644
> >> --- a/tests/acceptance/avocado_qemu/__init__.py
> >> +++ b/tests/acceptance/avocado_qemu/__init__.py
> >> @@ -301,6 +301,8 @@ class LinuxTest(Test, LinuxSSHMixIn):
> >>          super(LinuxTest, self).setUp()
> >>          self.vm.add_args('-smp', '2')
> >>          self.vm.add_args('-m', '1024')
> >> +        self.vm.add_args('-netdev', 'user,id=vnet,hostfwd=:127.0.0.1:0-:22',
> >> +                         '-device', 'virtio-net,netdev=vnet')
> > 
> > You need a machine with a virtio-bus to be able to plug a virtio device,
> > not all provide one.
>

Very true.

> Alternatively you could provide a network_device_type argument to
> setUp() which has to be explicitly set (since the tests would be
> pointless without network access).
>

Sure, I'll add the capability to configure the device type on v2.

Thanks!
- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 10/22] Python: add utility function for retrieving port redirection

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 524 bytes --]

On Thu, Feb 04, 2021 at 07:25:52PM -0500, John Snow wrote:
> On 2/3/21 12:23 PM, Cleber Rosa wrote:
> > Slightly different versions for the same utility code are currently
> > present on different locations.  This unifies them all, giving
> > preference to the version from virtiofs_submounts.py, because of the
> > last tweaks added to it.
> > 
> > While at it, this adds a "qemu.util" module to host the utility
> > function and a test.
> > 
> 
> qemu.utils, with the s!
>

Nice catch, thanks!

- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]