[PATCH 0/3] Acceptance Tests: support choosing specific distro and version

[PATCH 0/3] Acceptance Tests: support choosing specific distro and version

[Cleber Rosa]

Because Fedora 31 will not suit all tests that depend on a Linux
guest, this allows for the configuration of the guest distribution.
It came out of a suggestion from Eric Auger, and it was actually a
feature I planned to submit for a while.

This is based on the following series:

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

A GitLab CI pipeline can be seen here:

 https://gitlab.com/cleber.gnu/qemu/-/pipelines

Note: I'll address the line length caught in the check-patch job as
soon as I find what was the outcome of the line limits for Python
code discussion.

Based-On: <20210412044644.55083-1-crosa@redhat.com>

Cleber Rosa (3):
  Acceptance Tests: rename attribute holding the distro image checksum
  Acceptance Tests: move definition of distro checksums to the framework
  Acceptance Tests: support choosing specific distro and version

 docs/devel/testing.rst                    | 65 ++++++++++++++++++++++
 tests/acceptance/avocado_qemu/__init__.py | 67 +++++++++++++++++++++--
 tests/acceptance/boot_linux.py            |  8 ---
 3 files changed, 127 insertions(+), 13 deletions(-)

-- 
2.25.4

[PATCH 1/3] Acceptance Tests: rename attribute holding the distro image checksum

[Cleber Rosa]

This renames the attribute that holds the checksum for the image Linux
distribution image used.

The current name of the attribute is not very descriptive.  Also, in
preparation for making the distribution used configurable, which will
add distro related parameters, attributes and tags, let's make the
naming of those more uniform.

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

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index 1062a851b9..aae1e5bbc9 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -307,7 +307,7 @@ class LinuxTest(Test, LinuxSSHMixIn):
     """
 
     timeout = 900
-    chksum = None
+    distro_checksum = None
     username = 'root'
     password = 'password'
 
@@ -355,7 +355,7 @@ def download_boot(self):
         try:
             boot = vmimage.get(
                 'fedora', arch=image_arch, version='31',
-                checksum=self.chksum,
+                checksum=self.distro_checksum,
                 algorithm='sha256',
                 cache_dir=self.cache_dirs[0],
                 snapshot_dir=self.workdir)
diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
index 314370fd1f..c7bc3a589e 100644
--- a/tests/acceptance/boot_linux.py
+++ b/tests/acceptance/boot_linux.py
@@ -20,7 +20,7 @@ class BootLinuxX8664(LinuxTest):
     :avocado: tags=arch:x86_64
     """
 
-    chksum = 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'
+    distro_checksum = 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'
 
     def test_pc_i440fx_tcg(self):
         """
@@ -66,7 +66,7 @@ class BootLinuxAarch64(LinuxTest):
     :avocado: tags=machine:gic-version=2
     """
 
-    chksum = '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'
+    distro_checksum = '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'
 
     def add_common_args(self):
         self.vm.add_args('-bios',
@@ -119,7 +119,7 @@ class BootLinuxPPC64(LinuxTest):
     :avocado: tags=arch:ppc64
     """
 
-    chksum = '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'
+    distro_checksum = '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'
 
     def test_pseries_tcg(self):
         """
@@ -136,7 +136,7 @@ class BootLinuxS390X(LinuxTest):
     :avocado: tags=arch:s390x
     """
 
-    chksum = '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'
+    distro_checksum = '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'
 
     @skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
     def test_s390_ccw_virtio_tcg(self):
-- 
2.25.4

[PATCH 2/3] Acceptance Tests: move definition of distro checksums to the framework

[Cleber Rosa]

Instead of having, by default, the checksum in the tests, and the
definition of tests in the framework, let's keep them together.

A central definition for distributions is available, and it should
allow other known distros to be added more easily.

No behavior change is expected here, and tests can still define
a distro_checksum value if for some reason they want to override
the known distribution information.

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

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index aae1e5bbc9..97093614d9 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -299,6 +299,30 @@ def ssh_command(self, command):
         return stdout_lines, stderr_lines
 
 
+#: A collection of known distros and their respective image checksum
+KNOWN_DISTROS = {
+    'fedora': {
+        '31': {
+            'x86_64':
+            {'checksum': 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'},
+            'aarch64':
+            {'checksum': '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'},
+            'ppc64':
+            {'checksum': '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'},
+            's390x':
+            {'checksum': '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'},
+            }
+        }
+    }
+
+
+def get_known_distro_checksum(distro, distro_version, arch):
+    try:
+        return KNOWN_DISTROS.get(distro).get(distro_version).get(arch).get('checksum')
+    except AttributeError:
+        return None
+
+
 class LinuxTest(Test, LinuxSSHMixIn):
     """Facilitates having a cloud-image Linux based available.
 
@@ -348,14 +372,20 @@ def download_boot(self):
         vmimage.QEMU_IMG = qemu_img
 
         self.log.info('Downloading/preparing boot image')
+        distro = 'fedora'
+        distro_version = '31'
+        known_distro_checksum = get_known_distro_checksum(distro,
+                                                          distro_version,
+                                                          self.arch)
+        distro_checksum = self.distro_checksum or known_distro_checksum
         # 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.distro_checksum,
+                distro, arch=image_arch, version=distro_version,
+                checksum=distro_checksum,
                 algorithm='sha256',
                 cache_dir=self.cache_dirs[0],
                 snapshot_dir=self.workdir)
diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
index c7bc3a589e..9e618c6daa 100644
--- a/tests/acceptance/boot_linux.py
+++ b/tests/acceptance/boot_linux.py
@@ -20,8 +20,6 @@ class BootLinuxX8664(LinuxTest):
     :avocado: tags=arch:x86_64
     """
 
-    distro_checksum = 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'
-
     def test_pc_i440fx_tcg(self):
         """
         :avocado: tags=machine:pc
@@ -66,8 +64,6 @@ class BootLinuxAarch64(LinuxTest):
     :avocado: tags=machine:gic-version=2
     """
 
-    distro_checksum = '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'
-
     def add_common_args(self):
         self.vm.add_args('-bios',
                          os.path.join(BUILD_DIR, 'pc-bios',
@@ -119,8 +115,6 @@ class BootLinuxPPC64(LinuxTest):
     :avocado: tags=arch:ppc64
     """
 
-    distro_checksum = '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'
-
     def test_pseries_tcg(self):
         """
         :avocado: tags=machine:pseries
@@ -136,8 +130,6 @@ class BootLinuxS390X(LinuxTest):
     :avocado: tags=arch:s390x
     """
 
-    distro_checksum = '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'
-
     @skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
     def test_s390_ccw_virtio_tcg(self):
         """
-- 
2.25.4

[PATCH 3/3] Acceptance Tests: support choosing specific distro and version

[Cleber Rosa]

The tests based on the LinuxTest class give the test writer a ready to
use guest operating system, currently pinned to Fedora 31.

With this change, it's now possible to choose different distros and
versions, similar to how other tags and parameter can be set for the
target arch, accelerator, etc.

One of the reasons for this work, is that some development features
depend on updates on the guest side.  For instance the tests on
virtiofs_submounts.py, require newer kernels, and may benefit from
running, say on Fedora 34, without the need for a custom kernel.

Please notice that the pre-caching of the Fedora 31 images done during
the early stages of `make check-acceptance` (before the tests are
actually executed) are not expanded here to cover every new image
added.  But, the tests will download other needed images (and cache
them) during the first execution.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 docs/devel/testing.rst                    | 65 +++++++++++++++++++++++
 tests/acceptance/avocado_qemu/__init__.py | 47 ++++++++++++----
 2 files changed, 102 insertions(+), 10 deletions(-)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 4e42392810..19cbf532ae 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -922,6 +922,39 @@ The preserved value of the ``qemu_bin`` parameter or the result of the
 dynamic probe for a QEMU binary in the current working directory or
 source tree.
 
+LinuxTest
+~~~~~~~~~
+
+Besides the attributes present on the ``avocado_qemu.Test`` base
+class, the ``avocado_qemu.LinuxTest`` adds the following attributes:
+
+distro
+......
+
+The name of the Linux distribution used as the guest image for the
+test.  The name should match the **Provider** column on the list
+of images supported by the avocado.utils.vmimage library:
+
+https://avocado-framework.readthedocs.io/en/latest/guides/writer/libs/vmimage.html#supported-images
+
+distro_version
+..............
+
+The version of the Linux distribution as the guest image for the
+test.  The name should match the **Version** column on the list
+of images supported by the avocado.utils.vmimage library:
+
+https://avocado-framework.readthedocs.io/en/latest/guides/writer/libs/vmimage.html#supported-images
+
+distro_checksum
+...............
+
+The sha256 hash of the guest image file used for the test.
+
+If this value is not set in the code or by a test parameter (with the
+same name), no validation on the integrity of the image will be
+performed.
+
 Parameter reference
 -------------------
 
@@ -962,6 +995,38 @@ qemu_bin
 
 The exact QEMU binary to be used on QEMUMachine.
 
+LinuxTest
+~~~~~~~~~
+
+Besides the parameters present on the ``avocado_qemu.Test`` base
+class, the ``avocado_qemu.LinuxTest`` adds the following parameters:
+
+distro
+......
+
+The name of the Linux distribution used as the guest image for the
+test.  The name should match the **Provider** column on the list
+of images supported by the avocado.utils.vmimage library:
+
+https://avocado-framework.readthedocs.io/en/latest/guides/writer/libs/vmimage.html#supported-images
+
+distro_version
+..............
+
+The version of the Linux distribution as the guest image for the
+test.  The name should match the **Version** column on the list
+of images supported by the avocado.utils.vmimage library:
+
+https://avocado-framework.readthedocs.io/en/latest/guides/writer/libs/vmimage.html#supported-images
+
+distro_checksum
+...............
+
+The sha256 hash of the guest image file used for the test.
+
+If this value is not set in the code or by this parameter no
+validation on the integrity of the image will be performed.
+
 Skipping tests
 --------------
 The Avocado framework provides Python decorators which allow for easily skip
diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index 97093614d9..6fd0016917 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -335,8 +335,39 @@ class LinuxTest(Test, LinuxSSHMixIn):
     username = 'root'
     password = 'password'
 
+    def _set_distro(self):
+        distro = self.params.get(
+            'distro',
+            default=self._get_unique_tag_val('distro'))
+        if not distro:
+            distro = 'fedora'
+        self.distro = distro
+
+        distro_version = self.params.get(
+            'distro_version',
+            default=self._get_unique_tag_val('distro_version'))
+        if not distro_version:
+            distro_version = '31'
+        self.distro_version = distro_version
+
+        # The distro checksum behaves differently than distro name and
+        # version. First, it does not respect a tag with the same
+        # name, given that it's not expected to be used for filtering
+        # (distro name versions are the natural choice).  Second, the
+        # order of precedence is: parameter, attribute and then value
+        # from KNOWN_DISTROS.
+        distro_checksum = self.params.get('distro_checksum',
+                                          default=self.distro_checksum)
+        if not distro_checksum:
+            distro_checksum = get_known_distro_checksum(self.distro,
+                                                        self.distro_version,
+                                                        self.arch)
+        if distro_checksum:
+            self.distro_checksum = distro_checksum
+
     def setUp(self, ssh_pubkey=None, network_device_type='virtio-net'):
         super(LinuxTest, self).setUp()
+        self._set_distro()
         self.vm.add_args('-smp', '2')
         self.vm.add_args('-m', '1024')
         # The following network device allows for SSH connections
@@ -372,20 +403,16 @@ def download_boot(self):
         vmimage.QEMU_IMG = qemu_img
 
         self.log.info('Downloading/preparing boot image')
-        distro = 'fedora'
-        distro_version = '31'
-        known_distro_checksum = get_known_distro_checksum(distro,
-                                                          distro_version,
-                                                          self.arch)
-        distro_checksum = self.distro_checksum or known_distro_checksum
         # Fedora 31 only provides ppc64le images
         image_arch = self.arch
-        if image_arch == 'ppc64':
-            image_arch = 'ppc64le'
+        if self.distro == 'fedora':
+            if image_arch == 'ppc64':
+                image_arch = 'ppc64le'
+
         try:
             boot = vmimage.get(
-                distro, arch=image_arch, version=distro_version,
-                checksum=distro_checksum,
+                self.distro, arch=image_arch, version=self.distro_version,
+                checksum=self.distro_checksum,
                 algorithm='sha256',
                 cache_dir=self.cache_dirs[0],
                 snapshot_dir=self.workdir)
-- 
2.25.4

Re: [PATCH 0/3] Acceptance Tests: support choosing specific distro and version

[Philippe Mathieu-Daudé]

On 4/15/21 12:14 AM, Cleber Rosa wrote:
> Cleber Rosa (3):
>   Acceptance Tests: rename attribute holding the distro image checksum
>   Acceptance Tests: move definition of distro checksums to the framework
>   Acceptance Tests: support choosing specific distro and version
> 
>  docs/devel/testing.rst                    | 65 ++++++++++++++++++++++
>  tests/acceptance/avocado_qemu/__init__.py | 67 +++++++++++++++++++++--
>  tests/acceptance/boot_linux.py            |  8 ---
>  3 files changed, 127 insertions(+), 13 deletions(-)

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

Re: [PATCH 1/3] Acceptance Tests: rename attribute holding the distro image checksum

[Wainer dos Santos Moschetta]

On 4/14/21 7:14 PM, Cleber Rosa wrote:
> This renames the attribute that holds the checksum for the image Linux
> distribution image used.
>
> The current name of the attribute is not very descriptive.  Also, in
> preparation for making the distribution used configurable, which will
> add distro related parameters, attributes and tags, let's make the
> naming of those more uniform.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/acceptance/avocado_qemu/__init__.py | 4 ++--
>   tests/acceptance/boot_linux.py            | 8 ++++----
>   2 files changed, 6 insertions(+), 6 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 1062a851b9..aae1e5bbc9 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -307,7 +307,7 @@ class LinuxTest(Test, LinuxSSHMixIn):
>       """
>   
>       timeout = 900
> -    chksum = None
> +    distro_checksum = None
>       username = 'root'
>       password = 'password'
>   
> @@ -355,7 +355,7 @@ def download_boot(self):
>           try:
>               boot = vmimage.get(
>                   'fedora', arch=image_arch, version='31',
> -                checksum=self.chksum,
> +                checksum=self.distro_checksum,
>                   algorithm='sha256',
>                   cache_dir=self.cache_dirs[0],
>                   snapshot_dir=self.workdir)
> diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
> index 314370fd1f..c7bc3a589e 100644
> --- a/tests/acceptance/boot_linux.py
> +++ b/tests/acceptance/boot_linux.py
> @@ -20,7 +20,7 @@ class BootLinuxX8664(LinuxTest):
>       :avocado: tags=arch:x86_64
>       """
>   
> -    chksum = 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'
> +    distro_checksum = 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'
>   
>       def test_pc_i440fx_tcg(self):
>           """
> @@ -66,7 +66,7 @@ class BootLinuxAarch64(LinuxTest):
>       :avocado: tags=machine:gic-version=2
>       """
>   
> -    chksum = '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'
> +    distro_checksum = '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'
>   
>       def add_common_args(self):
>           self.vm.add_args('-bios',
> @@ -119,7 +119,7 @@ class BootLinuxPPC64(LinuxTest):
>       :avocado: tags=arch:ppc64
>       """
>   
> -    chksum = '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'
> +    distro_checksum = '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'
>   
>       def test_pseries_tcg(self):
>           """
> @@ -136,7 +136,7 @@ class BootLinuxS390X(LinuxTest):
>       :avocado: tags=arch:s390x
>       """
>   
> -    chksum = '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'
> +    distro_checksum = '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'
>   
>       @skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
>       def test_s390_ccw_virtio_tcg(self):

Re: [PATCH 1/3] Acceptance Tests: rename attribute holding the distro image checksum

[Auger Eric]

Hi Cleber,

On 4/15/21 12:14 AM, Cleber Rosa wrote:
> This renames the attribute that holds the checksum for the image Linux
> distribution image used.
> 
> The current name of the attribute is not very descriptive.  Also, in
> preparation for making the distribution used configurable, which will
user configurable
> add distro related parameters, attributes and tags, let's make the
> naming of those more uniform.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> ---
>  tests/acceptance/avocado_qemu/__init__.py | 4 ++--
>  tests/acceptance/boot_linux.py            | 8 ++++----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index 1062a851b9..aae1e5bbc9 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -307,7 +307,7 @@ class LinuxTest(Test, LinuxSSHMixIn):
>      """
>  
>      timeout = 900
> -    chksum = None
> +    distro_checksum = None
>      username = 'root'
>      password = 'password'
>  
> @@ -355,7 +355,7 @@ def download_boot(self):
>          try:
>              boot = vmimage.get(
>                  'fedora', arch=image_arch, version='31',
> -                checksum=self.chksum,
> +                checksum=self.distro_checksum,
>                  algorithm='sha256',
>                  cache_dir=self.cache_dirs[0],
>                  snapshot_dir=self.workdir)
> diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
> index 314370fd1f..c7bc3a589e 100644
> --- a/tests/acceptance/boot_linux.py
> +++ b/tests/acceptance/boot_linux.py
> @@ -20,7 +20,7 @@ class BootLinuxX8664(LinuxTest):
>      :avocado: tags=arch:x86_64
>      """
>  
> -    chksum = 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'
> +    distro_checksum = 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'
>  
>      def test_pc_i440fx_tcg(self):
>          """
> @@ -66,7 +66,7 @@ class BootLinuxAarch64(LinuxTest):
>      :avocado: tags=machine:gic-version=2
>      """
>  
> -    chksum = '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'
> +    distro_checksum = '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'
>  
>      def add_common_args(self):
>          self.vm.add_args('-bios',
> @@ -119,7 +119,7 @@ class BootLinuxPPC64(LinuxTest):
>      :avocado: tags=arch:ppc64
>      """
>  
> -    chksum = '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'
> +    distro_checksum = '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'
>  
>      def test_pseries_tcg(self):
>          """
> @@ -136,7 +136,7 @@ class BootLinuxS390X(LinuxTest):
>      :avocado: tags=arch:s390x
>      """
>  
> -    chksum = '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'
> +    distro_checksum = '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'
>  
>      @skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
>      def test_s390_ccw_virtio_tcg(self):
> 

Re: [PATCH 2/3] Acceptance Tests: move definition of distro checksums to the framework

[Wainer dos Santos Moschetta]

Hi,

On 4/14/21 7:14 PM, Cleber Rosa wrote:
> Instead of having, by default, the checksum in the tests, and the
> definition of tests in the framework, let's keep them together.
>
> A central definition for distributions is available, and it should
> allow other known distros to be added more easily.
>
> No behavior change is expected here, and tests can still define
> a distro_checksum value if for some reason they want to override
> the known distribution information.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/acceptance/avocado_qemu/__init__.py | 34 +++++++++++++++++++++--
>   tests/acceptance/boot_linux.py            |  8 ------
>   2 files changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index aae1e5bbc9..97093614d9 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -299,6 +299,30 @@ def ssh_command(self, command):
>           return stdout_lines, stderr_lines
>   
>   
> +#: A collection of known distros and their respective image checksum
> +KNOWN_DISTROS = {

Do you plan to expand that mapping to record values other than 
checksums? Otherwise it could be named KNOWN_DISTROS_CHECKSUMS.

> +    'fedora': {
> +        '31': {
> +            'x86_64':
> +            {'checksum': 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'},
> +            'aarch64':
> +            {'checksum': '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'},
> +            'ppc64':
> +            {'checksum': '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'},
> +            's390x':
> +            {'checksum': '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'},
> +            }
> +        }
> +    }
> +
> +
> +def get_known_distro_checksum(distro, distro_version, arch):
> +    try:
> +        return KNOWN_DISTROS.get(distro).get(distro_version).get(arch).get('checksum')
> +    except AttributeError:
> +        return None
> +
> +

Currently we have a few loose methods on avocado_qemu/__init__.py, and 
I'm about to send a series to wrap them in a mixin class. This series 
will introduce more loose code on the file; so would you consider moving 
KNOWN_DISTROS and get_known_distro_checksum() to the LinuxTest class, 
and possibly making the latest a class method?

>   class LinuxTest(Test, LinuxSSHMixIn):
>       """Facilitates having a cloud-image Linux based available.
>   
> @@ -348,14 +372,20 @@ def download_boot(self):
>           vmimage.QEMU_IMG = qemu_img
>   
>           self.log.info('Downloading/preparing boot image')
> +        distro = 'fedora'
> +        distro_version = '31'
> +        known_distro_checksum = get_known_distro_checksum(distro,
> +                                                          distro_version,
> +                                                          self.arch)
> +        distro_checksum = self.distro_checksum or known_distro_checksum


distro_checksum may be None. In this case vmimage.get() will silently 
skip the check? I suggest to log a warn message.


>           # 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.distro_checksum,
> +                distro, arch=image_arch, version=distro_version,
> +                checksum=distro_checksum,
>                   algorithm='sha256',
>                   cache_dir=self.cache_dirs[0],
>                   snapshot_dir=self.workdir)
> diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
> index c7bc3a589e..9e618c6daa 100644
> --- a/tests/acceptance/boot_linux.py
> +++ b/tests/acceptance/boot_linux.py
> @@ -20,8 +20,6 @@ class BootLinuxX8664(LinuxTest):
>       :avocado: tags=arch:x86_64
>       """
>   
> -    distro_checksum = 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'
> -
>       def test_pc_i440fx_tcg(self):
>           """
>           :avocado: tags=machine:pc
> @@ -66,8 +64,6 @@ class BootLinuxAarch64(LinuxTest):
>       :avocado: tags=machine:gic-version=2
>       """
>   
> -    distro_checksum = '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'
> -
>       def add_common_args(self):
>           self.vm.add_args('-bios',
>                            os.path.join(BUILD_DIR, 'pc-bios',
> @@ -119,8 +115,6 @@ class BootLinuxPPC64(LinuxTest):
>       :avocado: tags=arch:ppc64
>       """
>   
> -    distro_checksum = '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'
> -
>       def test_pseries_tcg(self):
>           """
>           :avocado: tags=machine:pseries
> @@ -136,8 +130,6 @@ class BootLinuxS390X(LinuxTest):
>       :avocado: tags=arch:s390x
>       """
>   
> -    distro_checksum = '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'
> -
>       @skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
>       def test_s390_ccw_virtio_tcg(self):
>           """

Re: [PATCH 3/3] Acceptance Tests: support choosing specific distro and version

[Willian Rampazzo]

On Wed, Apr 14, 2021 at 7:15 PM Cleber Rosa <crosa@redhat.com> wrote:
>
> The tests based on the LinuxTest class give the test writer a ready to
> use guest operating system, currently pinned to Fedora 31.
>
> With this change, it's now possible to choose different distros and
> versions, similar to how other tags and parameter can be set for the
> target arch, accelerator, etc.
>
> One of the reasons for this work, is that some development features
> depend on updates on the guest side.  For instance the tests on
> virtiofs_submounts.py, require newer kernels, and may benefit from
> running, say on Fedora 34, without the need for a custom kernel.
>
> Please notice that the pre-caching of the Fedora 31 images done during
> the early stages of `make check-acceptance` (before the tests are
> actually executed) are not expanded here to cover every new image
> added.  But, the tests will download other needed images (and cache
> them) during the first execution.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  docs/devel/testing.rst                    | 65 +++++++++++++++++++++++
>  tests/acceptance/avocado_qemu/__init__.py | 47 ++++++++++++----
>  2 files changed, 102 insertions(+), 10 deletions(-)
>

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

Re: [PATCH 1/3] Acceptance Tests: rename attribute holding the distro image checksum

[Willian Rampazzo]

On Wed, Apr 14, 2021 at 7:15 PM Cleber Rosa <crosa@redhat.com> wrote:
>
> This renames the attribute that holds the checksum for the image Linux
> distribution image used.
>
> The current name of the attribute is not very descriptive.  Also, in
> preparation for making the distribution used configurable, which will
> add distro related parameters, attributes and tags, let's make the
> naming of those more uniform.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/avocado_qemu/__init__.py | 4 ++--
>  tests/acceptance/boot_linux.py            | 8 ++++----
>  2 files changed, 6 insertions(+), 6 deletions(-)
>

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

Re: [PATCH 2/3] Acceptance Tests: move definition of distro checksums to the framework

[Cleber Rosa]

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

On Mon, Apr 19, 2021 at 12:25:44PM -0300, Wainer dos Santos Moschetta wrote:
> Hi,
> 
> On 4/14/21 7:14 PM, Cleber Rosa wrote:
> > Instead of having, by default, the checksum in the tests, and the
> > definition of tests in the framework, let's keep them together.
> > 
> > A central definition for distributions is available, and it should
> > allow other known distros to be added more easily.
> > 
> > No behavior change is expected here, and tests can still define
> > a distro_checksum value if for some reason they want to override
> > the known distribution information.
> > 
> > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > ---
> >   tests/acceptance/avocado_qemu/__init__.py | 34 +++++++++++++++++++++--
> >   tests/acceptance/boot_linux.py            |  8 ------
> >   2 files changed, 32 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> > index aae1e5bbc9..97093614d9 100644
> > --- a/tests/acceptance/avocado_qemu/__init__.py
> > +++ b/tests/acceptance/avocado_qemu/__init__.py
> > @@ -299,6 +299,30 @@ def ssh_command(self, command):
> >           return stdout_lines, stderr_lines
> > +#: A collection of known distros and their respective image checksum
> > +KNOWN_DISTROS = {
> 
> Do you plan to expand that mapping to record values other than checksums?
> Otherwise it could be named KNOWN_DISTROS_CHECKSUMS.
>

Let's just say I had an intuition about it being used for other
purposes.  Talking to Eric Auger earlier this morning, he will expand
this mapping with default kernel args distros, so that he can *add*
to the common args.

> > +    'fedora': {
> > +        '31': {
> > +            'x86_64':
> > +            {'checksum': 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'},
> > +            'aarch64':
> > +            {'checksum': '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'},
> > +            'ppc64':
> > +            {'checksum': '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'},
> > +            's390x':
> > +            {'checksum': '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'},
> > +            }
> > +        }
> > +    }
> > +
> > +
> > +def get_known_distro_checksum(distro, distro_version, arch):
> > +    try:
> > +        return KNOWN_DISTROS.get(distro).get(distro_version).get(arch).get('checksum')
> > +    except AttributeError:
> > +        return None
> > +
> > +
> 
> Currently we have a few loose methods on avocado_qemu/__init__.py, and I'm
> about to send a series to wrap them in a mixin class. This series will
> introduce more loose code on the file; so would you consider moving
> KNOWN_DISTROS and get_known_distro_checksum() to the LinuxTest class, and
> possibly making the latest a class method?
>

Some of our experience in "avocado.Test" revealed that users would:

  1) find it confusing to have so many methods in the class that are not useful
     to them

  2) would conflict with variables/attributes of their own

About #2, we end up turning a lot of variables atttributes into
properties so that errors would be explicit when users tried to
overwrite them unknowingly.

But, in the specific example of KNOWN_DISTROS and its expansion I
mentioned before, it may indeed make sense to have a Test or LinuxTest
method that test writers can use.  It'd probably need to be a bit more
generic and evolved than this current version though.

Maybe wait for Eric's input based on real world use case here?

> >   class LinuxTest(Test, LinuxSSHMixIn):
> >       """Facilitates having a cloud-image Linux based available.
> > @@ -348,14 +372,20 @@ def download_boot(self):
> >           vmimage.QEMU_IMG = qemu_img
> >           self.log.info('Downloading/preparing boot image')
> > +        distro = 'fedora'
> > +        distro_version = '31'
> > +        known_distro_checksum = get_known_distro_checksum(distro,
> > +                                                          distro_version,
> > +                                                          self.arch)
> > +        distro_checksum = self.distro_checksum or known_distro_checksum
> 
> 
> distro_checksum may be None. In this case vmimage.get() will silently skip
> the check? I suggest to log a warn message.
> 
> 

Yes, good point.  But, I think adding that warning to Avocado's
vmimage.get() method itself is even better.  What do you think?

Thanks for the review!
- Cleber.

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

Re: [PATCH 2/3] Acceptance Tests: move definition of distro checksums to the framework

[Wainer dos Santos Moschetta]

Hi,

On 4/19/21 3:35 PM, Cleber Rosa wrote:
> On Mon, Apr 19, 2021 at 12:25:44PM -0300, Wainer dos Santos Moschetta wrote:
>> Hi,
>>
>> On 4/14/21 7:14 PM, Cleber Rosa wrote:
>>> Instead of having, by default, the checksum in the tests, and the
>>> definition of tests in the framework, let's keep them together.
>>>
>>> A central definition for distributions is available, and it should
>>> allow other known distros to be added more easily.
>>>
>>> No behavior change is expected here, and tests can still define
>>> a distro_checksum value if for some reason they want to override
>>> the known distribution information.
>>>
>>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>>> ---
>>>    tests/acceptance/avocado_qemu/__init__.py | 34 +++++++++++++++++++++--
>>>    tests/acceptance/boot_linux.py            |  8 ------
>>>    2 files changed, 32 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
>>> index aae1e5bbc9..97093614d9 100644
>>> --- a/tests/acceptance/avocado_qemu/__init__.py
>>> +++ b/tests/acceptance/avocado_qemu/__init__.py
>>> @@ -299,6 +299,30 @@ def ssh_command(self, command):
>>>            return stdout_lines, stderr_lines
>>> +#: A collection of known distros and their respective image checksum
>>> +KNOWN_DISTROS = {
>> Do you plan to expand that mapping to record values other than checksums?
>> Otherwise it could be named KNOWN_DISTROS_CHECKSUMS.
>>
> Let's just say I had an intuition about it being used for other
> purposes.  Talking to Eric Auger earlier this morning, he will expand
> this mapping with default kernel args distros, so that he can *add*
> to the common args.
Ok
>
>>> +    'fedora': {
>>> +        '31': {
>>> +            'x86_64':
>>> +            {'checksum': 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'},
>>> +            'aarch64':
>>> +            {'checksum': '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'},
>>> +            'ppc64':
>>> +            {'checksum': '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'},
>>> +            's390x':
>>> +            {'checksum': '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'},
>>> +            }
>>> +        }
>>> +    }
>>> +
>>> +
>>> +def get_known_distro_checksum(distro, distro_version, arch):
>>> +    try:
>>> +        return KNOWN_DISTROS.get(distro).get(distro_version).get(arch).get('checksum')
>>> +    except AttributeError:
>>> +        return None
>>> +
>>> +
>> Currently we have a few loose methods on avocado_qemu/__init__.py, and I'm
>> about to send a series to wrap them in a mixin class. This series will
>> introduce more loose code on the file; so would you consider moving
>> KNOWN_DISTROS and get_known_distro_checksum() to the LinuxTest class, and
>> possibly making the latest a class method?
>>
> Some of our experience in "avocado.Test" revealed that users would:
>
>    1) find it confusing to have so many methods in the class that are not useful
>       to them
>
>    2) would conflict with variables/attributes of their own
>
> About #2, we end up turning a lot of variables atttributes into
> properties so that errors would be explicit when users tried to
> overwrite them unknowingly.
>
> But, in the specific example of KNOWN_DISTROS and its expansion I
> mentioned before, it may indeed make sense to have a Test or LinuxTest
> method that test writers can use.  It'd probably need to be a bit more
> generic and evolved than this current version though.
>
> Maybe wait for Eric's input based on real world use case here?
Sure, we can wait on Eric's input.
>
>>>    class LinuxTest(Test, LinuxSSHMixIn):
>>>        """Facilitates having a cloud-image Linux based available.
>>> @@ -348,14 +372,20 @@ def download_boot(self):
>>>            vmimage.QEMU_IMG = qemu_img
>>>            self.log.info('Downloading/preparing boot image')
>>> +        distro = 'fedora'
>>> +        distro_version = '31'
>>> +        known_distro_checksum = get_known_distro_checksum(distro,
>>> +                                                          distro_version,
>>> +                                                          self.arch)
>>> +        distro_checksum = self.distro_checksum or known_distro_checksum
>>
>> distro_checksum may be None. In this case vmimage.get() will silently skip
>> the check? I suggest to log a warn message.
>>
>>
> Yes, good point.  But, I think adding that warning to Avocado's
> vmimage.get() method itself is even better.  What do you think?

I like the idea. IMHO avocado_qemu should be a thin layer upon Avocado, 
so such as kind of code should live on avocado's side.

I just opened the PR: https://github.com/avocado-framework/avocado/pull/4539

- Wainer

>
> Thanks for the review!
> - Cleber.

Re: [PATCH 2/3] Acceptance Tests: move definition of distro checksums to the framework

[Auger Eric]

Hi Cleber,

On 4/15/21 12:14 AM, Cleber Rosa wrote:
> Instead of having, by default, the checksum in the tests, and the
> definition of tests in the framework, let's keep them together.
> 
> A central definition for distributions is available, and it should
> allow other known distros to be added more easily.
> 
> No behavior change is expected here, and tests can still define
> a distro_checksum value if for some reason they want to override
> the known distribution information.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/avocado_qemu/__init__.py | 34 +++++++++++++++++++++--
>  tests/acceptance/boot_linux.py            |  8 ------
>  2 files changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index aae1e5bbc9..97093614d9 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -299,6 +299,30 @@ def ssh_command(self, command):
>          return stdout_lines, stderr_lines
>  
>  
> +#: A collection of known distros and their respective image checksum
> +KNOWN_DISTROS = {
> +    'fedora': {
> +        '31': {
> +            'x86_64':
> +            {'checksum': 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'},
> +            'aarch64':
> +            {'checksum': '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'},
> +            'ppc64':
> +            {'checksum': '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'},
> +            's390x':
> +            {'checksum': '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'},
> +            }
> +        }
> +    }
assuming we may put other data like kernel params and maybe pxeboot URL,
this may grow rapidly, Maybe we should put that in a different file?
> +
> +
> +def get_known_distro_checksum(distro, distro_version, arch):
> +    try:
> +        return KNOWN_DISTROS.get(distro).get(distro_version).get(arch).get('checksum')
> +    except AttributeError:
> +        return None
> +
> +
>  class LinuxTest(Test, LinuxSSHMixIn):
>      """Facilitates having a cloud-image Linux based available.
>  
> @@ -348,14 +372,20 @@ def download_boot(self):
>          vmimage.QEMU_IMG = qemu_img
>  
>          self.log.info('Downloading/preparing boot image')
> +        distro = 'fedora'
> +        distro_version = '31'
> +        known_distro_checksum = get_known_distro_checksum(distro,
> +                                                          distro_version,
> +                                                          self.arch)
> +        distro_checksum = self.distro_checksum or known_distro_checksum
>          # 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.distro_checksum,
> +                distro, arch=image_arch, version=distro_version,
> +                checksum=distro_checksum,
>                  algorithm='sha256',
>                  cache_dir=self.cache_dirs[0],
>                  snapshot_dir=self.workdir)
> diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
> index c7bc3a589e..9e618c6daa 100644
> --- a/tests/acceptance/boot_linux.py
> +++ b/tests/acceptance/boot_linux.py
> @@ -20,8 +20,6 @@ class BootLinuxX8664(LinuxTest):
>      :avocado: tags=arch:x86_64
>      """
>  
> -    distro_checksum = 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'
> -
>      def test_pc_i440fx_tcg(self):
>          """
>          :avocado: tags=machine:pc
> @@ -66,8 +64,6 @@ class BootLinuxAarch64(LinuxTest):
>      :avocado: tags=machine:gic-version=2
>      """
>  
> -    distro_checksum = '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'
> -
>      def add_common_args(self):
>          self.vm.add_args('-bios',
>                           os.path.join(BUILD_DIR, 'pc-bios',
> @@ -119,8 +115,6 @@ class BootLinuxPPC64(LinuxTest):
>      :avocado: tags=arch:ppc64
>      """
>  
> -    distro_checksum = '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'
> -
>      def test_pseries_tcg(self):
>          """
>          :avocado: tags=machine:pseries
> @@ -136,8 +130,6 @@ class BootLinuxS390X(LinuxTest):
>      :avocado: tags=arch:s390x
>      """
>  
> -    distro_checksum = '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'
> -
>      @skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
>      def test_s390_ccw_virtio_tcg(self):
>          """
> 
Thanks

Eric

Re: [PATCH 0/3] Acceptance Tests: support choosing specific distro and version

[Eric Auger]

Hi CLeber,

On 4/15/21 12:14 AM, Cleber Rosa wrote:
> Because Fedora 31 will not suit all tests that depend on a Linux
> guest, this allows for the configuration of the guest distribution.
> It came out of a suggestion from Eric Auger, and it was actually a
> feature I planned to submit for a while.
>
> This is based on the following series:
>
>  [PATCH v3 00/11] Acceptance Test: introduce base class for Linux based tests

What is the state of this series, do you plan to respin. My SMMU
avocado-qemu tests depend on it. Also I have added some intel iommu
tests on top of it.

Thanks

Eric
>
> A GitLab CI pipeline can be seen here:
>
>  https://gitlab.com/cleber.gnu/qemu/-/pipelines
>
> Note: I'll address the line length caught in the check-patch job as
> soon as I find what was the outcome of the line limits for Python
> code discussion.
>
> Based-On: <20210412044644.55083-1-crosa@redhat.com>
>
> Cleber Rosa (3):
>   Acceptance Tests: rename attribute holding the distro image checksum
>   Acceptance Tests: move definition of distro checksums to the framework
>   Acceptance Tests: support choosing specific distro and version
>
>  docs/devel/testing.rst                    | 65 ++++++++++++++++++++++
>  tests/acceptance/avocado_qemu/__init__.py | 67 +++++++++++++++++++++--
>  tests/acceptance/boot_linux.py            |  8 ---
>  3 files changed, 127 insertions(+), 13 deletions(-)
>

Re: [PATCH 2/3] Acceptance Tests: move definition of distro checksums to the framework

[Cleber Rosa]

On 4/22/21 3:56 AM, Auger Eric wrote:
> Hi Cleber,
>
> On 4/15/21 12:14 AM, Cleber Rosa wrote:
>> Instead of having, by default, the checksum in the tests, and the
>> definition of tests in the framework, let's keep them together.
>>
>> A central definition for distributions is available, and it should
>> allow other known distros to be added more easily.
>>
>> No behavior change is expected here, and tests can still define
>> a distro_checksum value if for some reason they want to override
>> the known distribution information.
>>
>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>> ---
>>   tests/acceptance/avocado_qemu/__init__.py | 34 +++++++++++++++++++++--
>>   tests/acceptance/boot_linux.py            |  8 ------
>>   2 files changed, 32 insertions(+), 10 deletions(-)
>>
>> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
>> index aae1e5bbc9..97093614d9 100644
>> --- a/tests/acceptance/avocado_qemu/__init__.py
>> +++ b/tests/acceptance/avocado_qemu/__init__.py
>> @@ -299,6 +299,30 @@ def ssh_command(self, command):
>>           return stdout_lines, stderr_lines
>>   
>>   
>> +#: A collection of known distros and their respective image checksum
>> +KNOWN_DISTROS = {
>> +    'fedora': {
>> +        '31': {
>> +            'x86_64':
>> +            {'checksum': 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'},
>> +            'aarch64':
>> +            {'checksum': '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'},
>> +            'ppc64':
>> +            {'checksum': '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'},
>> +            's390x':
>> +            {'checksum': '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'},
>> +            }
>> +        }
>> +    }
> assuming we may put other data like kernel params and maybe pxeboot URL,
> this may grow rapidly, Maybe we should put that in a different file?


Hi Eric,


Sorry for the monstrous delay here.  You and I are thinking alike, but 
I'm planning to handle this on the Avocado side.  I'm writing a 
BluePrint[1] for what would be a revision of the Avocado asset API.  In 
that new proposal, data files (some builtin, some external) would be 
used to contain information about known assets.


So, for now, I think it's better to keep this AS IS.  Let me know if 
this works for you.


Thanks,

- Cleber.


[1] - https://github.com/avocado-framework/avocado/issues/4458

Re: [PATCH 0/3] Acceptance Tests: support choosing specific distro and version

[Cleber Rosa]

On 6/9/21 8:11 AM, Eric Auger wrote:
> Hi CLeber,
>
> On 4/15/21 12:14 AM, Cleber Rosa wrote:
>> Because Fedora 31 will not suit all tests that depend on a Linux
>> guest, this allows for the configuration of the guest distribution.
>> It came out of a suggestion from Eric Auger, and it was actually a
>> feature I planned to submit for a while.
>>
>> This is based on the following series:
>>
>>   [PATCH v3 00/11] Acceptance Test: introduce base class for Linux based tests
> What is the state of this series, do you plan to respin. My SMMU
> avocado-qemu tests depend on it. Also I have added some intel iommu
> tests on top of it.


Hi Eric,


Again, sorry for dropping the ball here.  This series has enough 
reviews, and I can queue it on my upcoming pre-freeze PR.  I'll just 
wait for your ACK on patch 2/3 about keeping the KNOWN_DISTROS info, for 
now, on the same file.


Thanks,

- Cleber.

Re: [PATCH 2/3] Acceptance Tests: move definition of distro checksums to the framework

[Eric Auger]

Hi Cleber,

On 4/15/21 12:14 AM, Cleber Rosa wrote:
> Instead of having, by default, the checksum in the tests, and the
> definition of tests in the framework, let's keep them together.
>
> A central definition for distributions is available, and it should
> allow other known distros to be added more easily.
>
> No behavior change is expected here, and tests can still define
> a distro_checksum value if for some reason they want to override
> the known distribution information.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
Acked-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  tests/acceptance/avocado_qemu/__init__.py | 34 +++++++++++++++++++++--
>  tests/acceptance/boot_linux.py            |  8 ------
>  2 files changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index aae1e5bbc9..97093614d9 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -299,6 +299,30 @@ def ssh_command(self, command):
>          return stdout_lines, stderr_lines
>  
>  
> +#: A collection of known distros and their respective image checksum
> +KNOWN_DISTROS = {
> +    'fedora': {
> +        '31': {
> +            'x86_64':
> +            {'checksum': 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'},
> +            'aarch64':
> +            {'checksum': '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'},
> +            'ppc64':
> +            {'checksum': '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'},
> +            's390x':
> +            {'checksum': '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'},
> +            }
> +        }
> +    }
> +
> +
> +def get_known_distro_checksum(distro, distro_version, arch):
> +    try:
> +        return KNOWN_DISTROS.get(distro).get(distro_version).get(arch).get('checksum')
> +    except AttributeError:
> +        return None
> +
> +
>  class LinuxTest(Test, LinuxSSHMixIn):
>      """Facilitates having a cloud-image Linux based available.
>  
> @@ -348,14 +372,20 @@ def download_boot(self):
>          vmimage.QEMU_IMG = qemu_img
>  
>          self.log.info('Downloading/preparing boot image')
> +        distro = 'fedora'
> +        distro_version = '31'
> +        known_distro_checksum = get_known_distro_checksum(distro,
> +                                                          distro_version,
> +                                                          self.arch)
> +        distro_checksum = self.distro_checksum or known_distro_checksum
>          # 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.distro_checksum,
> +                distro, arch=image_arch, version=distro_version,
> +                checksum=distro_checksum,
>                  algorithm='sha256',
>                  cache_dir=self.cache_dirs[0],
>                  snapshot_dir=self.workdir)
> diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
> index c7bc3a589e..9e618c6daa 100644
> --- a/tests/acceptance/boot_linux.py
> +++ b/tests/acceptance/boot_linux.py
> @@ -20,8 +20,6 @@ class BootLinuxX8664(LinuxTest):
>      :avocado: tags=arch:x86_64
>      """
>  
> -    distro_checksum = 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'
> -
>      def test_pc_i440fx_tcg(self):
>          """
>          :avocado: tags=machine:pc
> @@ -66,8 +64,6 @@ class BootLinuxAarch64(LinuxTest):
>      :avocado: tags=machine:gic-version=2
>      """
>  
> -    distro_checksum = '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'
> -
>      def add_common_args(self):
>          self.vm.add_args('-bios',
>                           os.path.join(BUILD_DIR, 'pc-bios',
> @@ -119,8 +115,6 @@ class BootLinuxPPC64(LinuxTest):
>      :avocado: tags=arch:ppc64
>      """
>  
> -    distro_checksum = '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'
> -
>      def test_pseries_tcg(self):
>          """
>          :avocado: tags=machine:pseries
> @@ -136,8 +130,6 @@ class BootLinuxS390X(LinuxTest):
>      :avocado: tags=arch:s390x
>      """
>  
> -    distro_checksum = '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'
> -
>      @skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
>      def test_s390_ccw_virtio_tcg(self):
>          """

Re: [PATCH 0/3] Acceptance Tests: support choosing specific distro and version

[Eric Auger]

Hi Cleber,

On 7/8/21 4:51 PM, Cleber Rosa wrote:
>
> On 6/9/21 8:11 AM, Eric Auger wrote:
>> Hi CLeber,
>>
>> On 4/15/21 12:14 AM, Cleber Rosa wrote:
>>> Because Fedora 31 will not suit all tests that depend on a Linux
>>> guest, this allows for the configuration of the guest distribution.
>>> It came out of a suggestion from Eric Auger, and it was actually a
>>> feature I planned to submit for a while.
>>>
>>> This is based on the following series:
>>>
>>>   [PATCH v3 00/11] Acceptance Test: introduce base class for Linux
>>> based tests
>> What is the state of this series, do you plan to respin. My SMMU
>> avocado-qemu tests depend on it. Also I have added some intel iommu
>> tests on top of it.
>
>
> Hi Eric,
>
>
> Again, sorry for dropping the ball here.  This series has enough
> reviews, and I can queue it on my upcoming pre-freeze PR.  I'll just
> wait for your ACK on patch 2/3 about keeping the KNOWN_DISTROS info,
> for now, on the same file.

No worries. I just sent the ACK for 2/3 of your series.

Thanks

Eric
>
>
> Thanks,
>
> - Cleber.
>

Re: [PATCH 2/3] Acceptance Tests: move definition of distro checksums to the framework

[Eric Auger]

Hi Cleber,

On 7/8/21 4:47 PM, Cleber Rosa wrote:
>
> On 4/22/21 3:56 AM, Auger Eric wrote:
>> Hi Cleber,
>>
>> On 4/15/21 12:14 AM, Cleber Rosa wrote:
>>> Instead of having, by default, the checksum in the tests, and the
>>> definition of tests in the framework, let's keep them together.
>>>
>>> A central definition for distributions is available, and it should
>>> allow other known distros to be added more easily.
>>>
>>> No behavior change is expected here, and tests can still define
>>> a distro_checksum value if for some reason they want to override
>>> the known distribution information.
>>>
>>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>>> ---
>>>   tests/acceptance/avocado_qemu/__init__.py | 34
>>> +++++++++++++++++++++--
>>>   tests/acceptance/boot_linux.py            |  8 ------
>>>   2 files changed, 32 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/tests/acceptance/avocado_qemu/__init__.py
>>> b/tests/acceptance/avocado_qemu/__init__.py
>>> index aae1e5bbc9..97093614d9 100644
>>> --- a/tests/acceptance/avocado_qemu/__init__.py
>>> +++ b/tests/acceptance/avocado_qemu/__init__.py
>>> @@ -299,6 +299,30 @@ def ssh_command(self, command):
>>>           return stdout_lines, stderr_lines
>>>     +#: A collection of known distros and their respective image
>>> checksum
>>> +KNOWN_DISTROS = {
>>> +    'fedora': {
>>> +        '31': {
>>> +            'x86_64':
>>> +            {'checksum':
>>> 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'},
>>> +            'aarch64':
>>> +            {'checksum':
>>> '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'},
>>> +            'ppc64':
>>> +            {'checksum':
>>> '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'},
>>> +            's390x':
>>> +            {'checksum':
>>> '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'},
>>> +            }
>>> +        }
>>> +    }
>> assuming we may put other data like kernel params and maybe pxeboot URL,
>> this may grow rapidly, Maybe we should put that in a different file?
>
>
> Hi Eric,
>
>
> Sorry for the monstrous delay here.  You and I are thinking alike, but
> I'm planning to handle this on the Avocado side.  I'm writing a
> BluePrint[1] for what would be a revision of the Avocado asset API. 
> In that new proposal, data files (some builtin, some external) would
> be used to contain information about known assets.
>
>
> So, for now, I think it's better to keep this AS IS.  Let me know if
> this works for you.

OK for me. This was just a suggestion anyway and the functionality is
more important atm.

Thanks

Eric
>
>
> Thanks,
>
> - Cleber.
>
>
> [1] - https://github.com/avocado-framework/avocado/issues/4458
>
>