Re: [PATCH v5 1/4] avocado_qemu: Fix KNOWN_DISTROS map into the LinuxDistro class

[Eric Auger <eric.auger@redhat.com>]

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