| Message ID | 20210706131729.30749-2-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | avocado-qemu: New SMMUv3 and intel IOMMU tests | expand |
On 7/6/21 9:17 AM, Eric Auger wrote: > From: Willian Rampazzo <willianr@redhat.com> > > As the KNOWN_DISTROS grows, more loosely methods will be created in > the avocado_qemu/__init__.py file. > > Let's refactor the code so that KNOWN_DISTROS and related methods are > packaged in a class > > Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com> > Signed-off-by: Eric Auger <eric.auger@redhat.com> > --- > tests/acceptance/avocado_qemu/__init__.py | 74 +++++++++++++---------- > 1 file changed, 42 insertions(+), 32 deletions(-) > > diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py > index 81ac90bebb..af93cd63ea 100644 > --- a/tests/acceptance/avocado_qemu/__init__.py > +++ b/tests/acceptance/avocado_qemu/__init__.py > @@ -299,29 +299,43 @@ def ssh_command(self, command): > f'Guest command failed: {command}') > return stdout_lines, stderr_lines > > +class LinuxDistro: > + """Represents a Linux distribution > I definitely like the idea. > -#: 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'}, > + Holds information of known distros. > + """ > + #: A collection of known distros and their respective image checksum > + KNOWN_DISTROS = { > + 'fedora': { > + '31': { > + 'x86_64': > + {'checksum': 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'}, > + 'ppc64': > + {'checksum': '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'}, > + 's390x': > + {'checksum': '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'}, > } > } > } > > + def __init__(self, name, version, arch): > + self.name = name > + self.version = version > + self.arch = arch This looks a lot like https://github.com/avocado-framework/avocado/blob/f0996dafefa412c77c221c2d1a6fafdcba1c97b7/avocado/utils/distro.py#L34 , although admittedly, their goals are very different. As a next step, in the future, I'd consider separating the data from the actual class and having it the LinuxDistro instances, helped by a registry. Something like: class LinuxDistroRegistry: def __init__(self): self.distros = set() def register(self, linux_distro): self.distros.add(linux_distro) def query(self, **kwargs): ... registry = LinuxDistroRegistry() registry.register(LinuxDistro('fedora', '31', 'x86_64', 'deadbeefdeadbeef')) registry.register(LinuxDistro('fedora', '31', 'aarch64', 'beefdeadbeefdead')) checksum = registry.query(name='fedora', version='31', arch='x86_64').checksum > + try: > + self._info = self.KNOWN_DISTROS.get(name).get(version).get(arch) The `AttributeError` that could be caught at the removed `get_known_distro_checksum()` function, could come from any of the `.get()`s returning `None`, which in turn would not have a `.get()` attribute. But now, if there's a "name", then a "version", but no "arch" entry, this line will set `self._info` to `None`. This is manifested if you try to run a test that tries to find an aarch64 distro, such as: ./tests/venv/bin/avocado run tests/acceptance/boot_linux.py:BootLinuxAarch64.test_virt_tcg_gicv2 It will result in: 20:38:18 ERROR| Reproduced traceback from: /var/lib/users/cleber/build/qemu/tests/venv/lib64/python3.9/site-packages/avocado/core/test.py:756 20:38:18 ERROR| Traceback (most recent call last): 20:38:18 ERROR| File "/var/lib/users/cleber/build/qemu/tests/acceptance/avocado_qemu/__init__.py", line 426, in download_boot 20:38:18 ERROR| checksum=self.distro.checksum, 20:38:18 ERROR| File "/var/lib/users/cleber/build/qemu/tests/acceptance/avocado_qemu/__init__.py", line 334, in checksum 20:38:18 ERROR| return self._info.get('checksum', None) 20:38:18 ERROR| AttributeError: 'NoneType' object has no attribute 'get' 20:38:18 ERROR| 20:38:18 ERROR| During handling of the above exception, another exception occurred: 20:38:18 ERROR| 20:38:18 ERROR| Traceback (most recent call last): 20:38:18 ERROR| File "/var/lib/users/cleber/build/qemu/tests/acceptance/avocado_qemu/__init__.py", line 387, in setUp 20:38:18 ERROR| self.set_up_boot() 20:38:18 ERROR| File "/var/lib/users/cleber/build/qemu/tests/acceptance/avocado_qemu/__init__.py", line 455, in set_up_boot 20:38:18 ERROR| path = self.download_boot() 20:38:18 ERROR| File "/var/lib/users/cleber/build/qemu/tests/acceptance/avocado_qemu/__init__.py", line 431, in download_boot 20:38:18 ERROR| self.cancel('Failed to download/prepare boot image') 20:38:18 ERROR| File "/var/lib/users/cleber/build/qemu/tests/venv/lib64/python3.9/site-packages/avocado/core/test.py", line 988, in cancel 20:38:18 ERROR| raise exceptions.TestCancel(message) 20:38:18 ERROR| avocado.core.exceptions.TestCancel: Failed to download/prepare boot image Cheers, - Cleber.
Hi Wainer, William, Cleber, On 7/8/21 3:17 AM, Cleber Rosa wrote: > > On 7/6/21 9:17 AM, Eric Auger wrote: >> From: Willian Rampazzo <willianr@redhat.com> >> >> As the KNOWN_DISTROS grows, more loosely methods will be created in >> the avocado_qemu/__init__.py file. >> >> Let's refactor the code so that KNOWN_DISTROS and related methods are >> packaged in a class >> >> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> --- >> tests/acceptance/avocado_qemu/__init__.py | 74 +++++++++++++---------- >> 1 file changed, 42 insertions(+), 32 deletions(-) >> >> diff --git a/tests/acceptance/avocado_qemu/__init__.py >> b/tests/acceptance/avocado_qemu/__init__.py >> index 81ac90bebb..af93cd63ea 100644 >> --- a/tests/acceptance/avocado_qemu/__init__.py >> +++ b/tests/acceptance/avocado_qemu/__init__.py >> @@ -299,29 +299,43 @@ def ssh_command(self, command): >> f'Guest command failed: {command}') >> return stdout_lines, stderr_lines >> +class LinuxDistro: >> + """Represents a Linux distribution >> > > > I definitely like the idea. > > >> -#: 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'}, >> + Holds information of known distros. >> + """ >> + #: A collection of known distros and their respective image >> checksum >> + KNOWN_DISTROS = { >> + 'fedora': { >> + '31': { >> + 'x86_64': >> + {'checksum': >> 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'}, >> + 'ppc64': >> + {'checksum': >> '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'}, >> + 's390x': >> + {'checksum': >> '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'}, >> } >> } >> } >> + def __init__(self, name, version, arch): >> + self.name = name >> + self.version = version >> + self.arch = arch > > > This looks a lot like > https://github.com/avocado-framework/avocado/blob/f0996dafefa412c77c221c2d1a6fafdcba1c97b7/avocado/utils/distro.py#L34 > , although admittedly, their goals are very different. > > > As a next step, in the future, I'd consider separating the data from > the actual class and having it the LinuxDistro instances, helped by a > registry. Something like: > > > class LinuxDistroRegistry: > > def __init__(self): > self.distros = set() > > def register(self, linux_distro): > > self.distros.add(linux_distro) > > def query(self, **kwargs): > > ... > > > registry = LinuxDistroRegistry() > > registry.register(LinuxDistro('fedora', '31', 'x86_64', > 'deadbeefdeadbeef')) > > registry.register(LinuxDistro('fedora', '31', 'aarch64', > 'beefdeadbeefdead')) > > checksum = registry.query(name='fedora', version='31', > arch='x86_64').checksum > > >> + try: >> + self._info = >> self.KNOWN_DISTROS.get(name).get(version).get(arch) > > > The `AttributeError` that could be caught at the removed > `get_known_distro_checksum()` function, could come from any of the > `.get()`s returning `None`, which in turn would not have a `.get()` > attribute. > > But now, if there's a "name", then a "version", but no "arch" entry, > this line will set `self._info` to `None`. This is manifested if you > try to run a test that tries to find an aarch64 distro, such as: > > ./tests/venv/bin/avocado run > tests/acceptance/boot_linux.py:BootLinuxAarch64.test_virt_tcg_gicv2 > > > It will result in: > > > 20:38:18 ERROR| Reproduced traceback from: > /var/lib/users/cleber/build/qemu/tests/venv/lib64/python3.9/site-packages/avocado/core/test.py:756 > 20:38:18 ERROR| Traceback (most recent call last): > 20:38:18 ERROR| File > "/var/lib/users/cleber/build/qemu/tests/acceptance/avocado_qemu/__init__.py", > line 426, in download_boot > 20:38:18 ERROR| checksum=self.distro.checksum, > 20:38:18 ERROR| File > "/var/lib/users/cleber/build/qemu/tests/acceptance/avocado_qemu/__init__.py", > line 334, in checksum > 20:38:18 ERROR| return self._info.get('checksum', None) > 20:38:18 ERROR| AttributeError: 'NoneType' object has no attribute 'get' > 20:38:18 ERROR| > 20:38:18 ERROR| During handling of the above exception, another > exception occurred: > 20:38:18 ERROR| > 20:38:18 ERROR| Traceback (most recent call last): > 20:38:18 ERROR| File > "/var/lib/users/cleber/build/qemu/tests/acceptance/avocado_qemu/__init__.py", > line 387, in setUp > 20:38:18 ERROR| self.set_up_boot() > 20:38:18 ERROR| File > "/var/lib/users/cleber/build/qemu/tests/acceptance/avocado_qemu/__init__.py", > line 455, in set_up_boot > 20:38:18 ERROR| path = self.download_boot() > 20:38:18 ERROR| File > "/var/lib/users/cleber/build/qemu/tests/acceptance/avocado_qemu/__init__.py", > line 431, in download_boot > 20:38:18 ERROR| self.cancel('Failed to download/prepare boot image') > 20:38:18 ERROR| File > "/var/lib/users/cleber/build/qemu/tests/venv/lib64/python3.9/site-packages/avocado/core/test.py", > line 988, in cancel > 20:38:18 ERROR| raise exceptions.TestCancel(message) > 20:38:18 ERROR| avocado.core.exceptions.TestCancel: Failed to > download/prepare boot image I am not sufficiently expert on the test infra and python to be really efficient fixing that. Can anyone help quickly to target the soft freeze? Otherwise, today I will drop that patch and restore the code I had in v4, just based on Cleber series. I think the refactoring can happen later... Thanks Eric > > > Cheers, > > - Cleber. >
On 7/8/21 4:56 AM, Eric Auger wrote: > > I am not sufficiently expert on the test infra and python to be really > efficient fixing that. Can anyone help quickly to target the soft > freeze? Otherwise, today I will drop that patch and restore the code I > had in v4, just based on Cleber series. I think the refactoring can > happen later... Hi Eric, The following diff works for me: diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py index af93cd63ea..b3bed00062 100644 --- a/tests/acceptance/avocado_qemu/__init__.py +++ b/tests/acceptance/avocado_qemu/__init__.py @@ -310,6 +310,8 @@ class LinuxDistro: '31': { 'x86_64': {'checksum': 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'}, + 'aarch64': + {'checksum': '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'}, 'ppc64': {'checksum': '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'}, 's390x': @@ -323,10 +325,11 @@ def __init__(self, name, version, arch): self.version = version self.arch = arch try: - self._info = self.KNOWN_DISTROS.get(name).get(version).get(arch) + info = self.KNOWN_DISTROS.get(name).get(version).get(arch) except AttributeError: # Unknown distro - self._info = {} + info = None + self._info = info or {} @property def checksum(self): I've tested it with both existing and the newly introduced tests. Cheers, - Cleber.
Hi Cleber, On 7/8/21 7:34 PM, Cleber Rosa wrote: > > On 7/8/21 4:56 AM, Eric Auger wrote: >> >> I am not sufficiently expert on the test infra and python to be really >> efficient fixing that. Can anyone help quickly to target the soft >> freeze? Otherwise, today I will drop that patch and restore the code I >> had in v4, just based on Cleber series. I think the refactoring can >> happen later... > > > Hi Eric, > > > The following diff works for me: > > > diff --git a/tests/acceptance/avocado_qemu/__init__.py > b/tests/acceptance/avocado_qemu/__init__.py > index af93cd63ea..b3bed00062 100644 > --- a/tests/acceptance/avocado_qemu/__init__.py > +++ b/tests/acceptance/avocado_qemu/__init__.py > @@ -310,6 +310,8 @@ class LinuxDistro: > '31': { > 'x86_64': > {'checksum': > 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'}, > + 'aarch64': > + {'checksum': > '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'}, > 'ppc64': > {'checksum': > '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'}, > 's390x': > @@ -323,10 +325,11 @@ def __init__(self, name, version, arch): > self.version = version > self.arch = arch > try: > - self._info = > self.KNOWN_DISTROS.get(name).get(version).get(arch) > + info = self.KNOWN_DISTROS.get(name).get(version).get(arch) > except AttributeError: > # Unknown distro > - self._info = {} > + info = None > + self._info = info or {} > > @property > def checksum(self): > > > I've tested it with both existing and the newly introduced tests. Thank you for the work! Do you plan to introduce it as a fixup or do I need to respin? Thanks Eric > > > Cheers, > > - Cleber. >
On 7/8/21 2:34 PM, Cleber Rosa wrote: > > On 7/8/21 4:56 AM, Eric Auger wrote: >> >> I am not sufficiently expert on the test infra and python to be really >> efficient fixing that. Can anyone help quickly to target the soft >> freeze? Otherwise, today I will drop that patch and restore the code I >> had in v4, just based on Cleber series. I think the refactoring can >> happen later... > > > Hi Eric, > > > The following diff works for me: > > > diff --git a/tests/acceptance/avocado_qemu/__init__.py > b/tests/acceptance/avocado_qemu/__init__.py > index af93cd63ea..b3bed00062 100644 > --- a/tests/acceptance/avocado_qemu/__init__.py > +++ b/tests/acceptance/avocado_qemu/__init__.py > @@ -310,6 +310,8 @@ class LinuxDistro: > '31': { > 'x86_64': > {'checksum': > 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'}, > + 'aarch64': > + {'checksum': > '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'}, > 'ppc64': > {'checksum': > '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'}, > 's390x': > @@ -323,10 +325,11 @@ def __init__(self, name, version, arch): > self.version = version > self.arch = arch > try: > - self._info = > self.KNOWN_DISTROS.get(name).get(version).get(arch) > + info = self.KNOWN_DISTROS.get(name).get(version).get(arch) > except AttributeError: > # Unknown distro > - self._info = {} > + info = None > + self._info = info or {} > > @property > def checksum(self): > > Thanks for that fix, Cleber. Acked-by: Wainer dos Santos Moschetta <wainersm@redhat.com> > I've tested it with both existing and the newly introduced tests. > > > Cheers, > > - Cleber. >
On 7/8/21 3:32 PM, Eric Auger wrote: > Hi Cleber, > > On 7/8/21 7:34 PM, Cleber Rosa wrote: >> On 7/8/21 4:56 AM, Eric Auger wrote: >>> I am not sufficiently expert on the test infra and python to be really >>> efficient fixing that. Can anyone help quickly to target the soft >>> freeze? Otherwise, today I will drop that patch and restore the code I >>> had in v4, just based on Cleber series. I think the refactoring can >>> happen later... >> >> Hi Eric, >> >> >> The following diff works for me: >> >> >> diff --git a/tests/acceptance/avocado_qemu/__init__.py >> b/tests/acceptance/avocado_qemu/__init__.py >> index af93cd63ea..b3bed00062 100644 >> --- a/tests/acceptance/avocado_qemu/__init__.py >> +++ b/tests/acceptance/avocado_qemu/__init__.py >> @@ -310,6 +310,8 @@ class LinuxDistro: >> '31': { >> 'x86_64': >> {'checksum': >> 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'}, >> + 'aarch64': >> + {'checksum': >> '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'}, >> 'ppc64': >> {'checksum': >> '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'}, >> 's390x': >> @@ -323,10 +325,11 @@ def __init__(self, name, version, arch): >> self.version = version >> self.arch = arch >> try: >> - self._info = >> self.KNOWN_DISTROS.get(name).get(version).get(arch) >> + info = self.KNOWN_DISTROS.get(name).get(version).get(arch) >> except AttributeError: >> # Unknown distro >> - self._info = {} >> + info = None >> + self._info = info or {} >> >> @property >> def checksum(self): >> >> >> I've tested it with both existing and the newly introduced tests. > Thank you for the work! Do you plan to introduce it as a fixup or do I > need to respin? Hi Eric, Yes, I can add it as a fixup. Thanks, - Cleber.
diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py index 81ac90bebb..af93cd63ea 100644 --- a/tests/acceptance/avocado_qemu/__init__.py +++ b/tests/acceptance/avocado_qemu/__init__.py @@ -299,29 +299,43 @@ def ssh_command(self, command): f'Guest command failed: {command}') return stdout_lines, stderr_lines +class LinuxDistro: + """Represents a Linux distribution -#: 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'}, + Holds information of known distros. + """ + #: A collection of known distros and their respective image checksum + KNOWN_DISTROS = { + 'fedora': { + '31': { + 'x86_64': + {'checksum': 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'}, + 'ppc64': + {'checksum': '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'}, + 's390x': + {'checksum': '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'}, } } } + def __init__(self, name, version, arch): + self.name = name + self.version = version + self.arch = arch + try: + self._info = self.KNOWN_DISTROS.get(name).get(version).get(arch) + except AttributeError: + # Unknown distro + self._info = {} -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 + @property + def checksum(self): + """Gets the cloud-image file checksum""" + return self._info.get('checksum', None) + + @checksum.setter + def checksum(self, value): + self._info['checksum'] = value class LinuxTest(Test, LinuxSSHMixIn): @@ -332,24 +346,24 @@ class LinuxTest(Test, LinuxSSHMixIn): """ timeout = 900 - distro_checksum = None + distro = None username = 'root' password = 'password' def _set_distro(self): - distro = self.params.get( + distro_name = self.params.get( 'distro', default=self._get_unique_tag_val('distro')) - if not distro: - distro = 'fedora' - self.distro = distro + if not distro_name: + distro_name = 'fedora' 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 + + self.distro = LinuxDistro(distro_name, distro_version, self.arch) # The distro checksum behaves differently than distro name and # version. First, it does not respect a tag with the same @@ -358,13 +372,9 @@ def _set_distro(self): # 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) + default=None) if distro_checksum: - self.distro_checksum = distro_checksum + self.distro.checksum = distro_checksum def setUp(self, ssh_pubkey=None, network_device_type='virtio-net'): super(LinuxTest, self).setUp() @@ -406,14 +416,14 @@ def download_boot(self): self.log.info('Downloading/preparing boot image') # Fedora 31 only provides ppc64le images image_arch = self.arch - if self.distro == 'fedora': + if self.distro.name == 'fedora': if image_arch == 'ppc64': image_arch = 'ppc64le' try: boot = vmimage.get( - self.distro, arch=image_arch, version=self.distro_version, - checksum=self.distro_checksum, + self.distro.name, arch=image_arch, version=self.distro.version, + checksum=self.distro.checksum, algorithm='sha256', cache_dir=self.cache_dirs[0], snapshot_dir=self.workdir)