diff mbox series

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

Message ID 20210203172357.1422425-8-crosa@redhat.com
State New
Headers show
Series Acceptance Test: introduce base class for Linux based tests | expand

Commit Message

Cleber Rosa Feb. 3, 2021, 5:23 p.m. UTC
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(-)

Comments

Beraldo Leal Feb. 4, 2021, 11:07 a.m. UTC | #1
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>
Alex Bennée Feb. 4, 2021, 1:23 p.m. UTC | #2
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:
Max Reitz Feb. 9, 2021, 10:25 a.m. UTC | #3
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
Alex Bennée Feb. 9, 2021, 11:24 a.m. UTC | #4
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
Max Reitz Feb. 9, 2021, 12:03 p.m. UTC | #5
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
Alex Bennée Feb. 9, 2021, 12:52 p.m. UTC | #6
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
Max Reitz Feb. 9, 2021, 1:35 p.m. UTC | #7
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
Alex Bennée Feb. 9, 2021, 4:15 p.m. UTC | #8
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 ;-)
Philippe Mathieu-Daudé Feb. 9, 2021, 5:15 p.m. UTC | #9
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.
Cleber Rosa Feb. 15, 2021, 5:56 p.m. UTC | #10
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.
diff mbox series

Patch

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')