[PATCH] virtiofs_submounts.py test: Note on vmlinuz param

[PATCH] virtiofs_submounts.py test: Note on vmlinuz param

[Max Reitz]

From the cancel message, it is not entirely clear why this parameter is
mandatory now, or that it will be optional in the future.  Add such a
more detailed explanation as a comment in the test source file.

Suggested-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
I’ve uploaded a build of Linux 5.10 here:
  https://xanclic.moe/linux-5.10

But I’ve decided against mentioning it in this new comment or the cancel
message, because, well, it’s my private server and I have limited
bandwidth.
---
 tests/acceptance/virtiofs_submounts.py | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
index 949ca87a83..9a69b6b17b 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -228,6 +228,18 @@ class VirtiofsSubmountsTest(BootLinux):
     def setUp(self):
         vmlinuz = self.params.get('vmlinuz')
         if vmlinuz is None:
+            """
+            The Linux kernel supports FUSE auto-submounts only as of 5.10.
+            boot_linux.py currently provides Fedora 31, whose kernel is too
+            old, so this test cannot pass with the on-image kernel (you are
+            welcome to try, hence the option to force such a test with
+            -p vmlinuz='').  Therefore, for now the user must provide a
+            sufficiently new custom kernel, or effectively explicitly
+            request failure with -p vmlinuz=''.
+            Once an image with a sufficiently new kernel is available
+            (probably Fedora 34), we can make -p vmlinuz='' the default, so
+            that this parameter no longer needs to be specified.
+            """
             self.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)')
-- 
2.29.2

Re: [PATCH] virtiofs_submounts.py test: Note on vmlinuz param

[Cleber Rosa]

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

On Fri, Feb 12, 2021 at 04:16:49PM +0100, Max Reitz wrote:
> From the cancel message, it is not entirely clear why this parameter is
> mandatory now, or that it will be optional in the future.  Add such a
> more detailed explanation as a comment in the test source file.
> 
> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> I’ve uploaded a build of Linux 5.10 here:
>   https://xanclic.moe/linux-5.10
> 
> But I’ve decided against mentioning it in this new comment or the cancel
> message, because, well, it’s my private server and I have limited
> bandwidth.
> ---
>  tests/acceptance/virtiofs_submounts.py | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
> index 949ca87a83..9a69b6b17b 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -228,6 +228,18 @@ class VirtiofsSubmountsTest(BootLinux):
>      def setUp(self):
>          vmlinuz = self.params.get('vmlinuz')
>          if vmlinuz is None:
> +            """
> +            The Linux kernel supports FUSE auto-submounts only as of 5.10.
> +            boot_linux.py currently provides Fedora 31, whose kernel is too
> +            old, so this test cannot pass with the on-image kernel (you are
> +            welcome to try, hence the option to force such a test with
> +            -p vmlinuz='').  Therefore, for now the user must provide a
> +            sufficiently new custom kernel, or effectively explicitly
> +            request failure with -p vmlinuz=''.
> +            Once an image with a sufficiently new kernel is available
> +            (probably Fedora 34), we can make -p vmlinuz='' the default, so
> +            that this parameter no longer needs to be specified.
> +            """
>              self.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)')
> -- 
> 2.29.2
>

Hi Max,

This looks good to me, and I've also tested your kernel build and
works like a charm.

As further work on top of this, it may be beneficial to have test
documentation in a predictable place.  The possibilities that come to
my mind:

 * docs/devel/testing.rst
 * tests/acceptance/$test_file.py/data/README

On a different topic, the "https://avocado-project.org/data/assets" has
enough bandwidth and can be used to hold this type asset.  Alternatively,
we can add a bit more automation to this test by letting people do something
like:

 $ avocado assets register virtiofs-auto-submounts-vmlinuz /path/to/vmlinuz

And on the test:

 vmlinuz = self.fetch_asset('virtiofs-auto-submounts-vmlinuz')

And the test should cancel if that asset has not been previously registered.

Anyway,

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

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

Re: [PATCH] virtiofs_submounts.py test: Note on vmlinuz param

[Philippe Mathieu-Daudé]

Hi Cleber,

On 2/12/21 7:58 PM, Cleber Rosa wrote:
> On Fri, Feb 12, 2021 at 04:16:49PM +0100, Max Reitz wrote:
>> From the cancel message, it is not entirely clear why this parameter is
>> mandatory now, or that it will be optional in the future.  Add such a
>> more detailed explanation as a comment in the test source file.
>>
>> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> I’ve uploaded a build of Linux 5.10 here:
>>   https://xanclic.moe/linux-5.10
>>
>> But I’ve decided against mentioning it in this new comment or the cancel
>> message, because, well, it’s my private server and I have limited
>> bandwidth.
>> ---
>>  tests/acceptance/virtiofs_submounts.py | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
>> index 949ca87a83..9a69b6b17b 100644
>> --- a/tests/acceptance/virtiofs_submounts.py
>> +++ b/tests/acceptance/virtiofs_submounts.py
>> @@ -228,6 +228,18 @@ class VirtiofsSubmountsTest(BootLinux):
>>      def setUp(self):
>>          vmlinuz = self.params.get('vmlinuz')
>>          if vmlinuz is None:
>> +            """
>> +            The Linux kernel supports FUSE auto-submounts only as of 5.10.
>> +            boot_linux.py currently provides Fedora 31, whose kernel is too
>> +            old, so this test cannot pass with the on-image kernel (you are
>> +            welcome to try, hence the option to force such a test with
>> +            -p vmlinuz='').  Therefore, for now the user must provide a
>> +            sufficiently new custom kernel, or effectively explicitly
>> +            request failure with -p vmlinuz=''.
>> +            Once an image with a sufficiently new kernel is available
>> +            (probably Fedora 34), we can make -p vmlinuz='' the default, so
>> +            that this parameter no longer needs to be specified.
>> +            """
>>              self.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)')
>> -- 
>> 2.29.2
>>
> 
> Hi Max,
> 
> This looks good to me, and I've also tested your kernel build and
> works like a charm.
> 
> As further work on top of this, it may be beneficial to have test
> documentation in a predictable place.  The possibilities that come to
> my mind:
> 
>  * docs/devel/testing.rst
>  * tests/acceptance/$test_file.py/data/README
> 
> On a different topic, the "https://avocado-project.org/data/assets" has
> enough bandwidth and can be used to hold this type asset.

Can you define "this type asset" please?

> Alternatively,
> we can add a bit more automation to this test by letting people do something
> like:
> 
>  $ avocado assets register virtiofs-auto-submounts-vmlinuz /path/to/vmlinuz
> 
> And on the test:
> 
>  vmlinuz = self.fetch_asset('virtiofs-auto-submounts-vmlinuz')
> 
> And the test should cancel if that asset has not been previously registered.

Yay, great news!

Re: [PATCH] virtiofs_submounts.py test: Note on vmlinuz param

[Cleber Rosa]

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

On Fri, Feb 12, 2021 at 09:42:24PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Cleber,
> > 
> > Hi Max,
> > 
> > This looks good to me, and I've also tested your kernel build and
> > works like a charm.
> > 
> > As further work on top of this, it may be beneficial to have test
> > documentation in a predictable place.  The possibilities that come to
> > my mind:
> > 
> >  * docs/devel/testing.rst
> >  * tests/acceptance/$test_file.py/data/README
> > 
> > On a different topic, the "https://avocado-project.org/data/assets" has
> > enough bandwidth and can be used to hold this type asset.
> 
> Can you define "this type asset" please?
>

Hi Phil,

Well, in this case I meant kernel/initrd builds, filesystem images, or
any other file that is used in such a test.  But, I guess anything
generated from purely free software, with instructions on how to
reproduce it would be OK.  The filesystem images there, for instance,
are generated by Avocado-VT unattended install tests.

Besides this server, there's another one that is being prepared,
provided by the fosshost that could be used for that sole purpose, and
be more closely attached to the QEMU project.

Anyway, just an idea...

Regards,
- Cleber.

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

Re: [PATCH] virtiofs_submounts.py test: Note on vmlinuz param

[Max Reitz]

On 12.02.21 19:58, Cleber Rosa wrote:
> On Fri, Feb 12, 2021 at 04:16:49PM +0100, Max Reitz wrote:
>>  From the cancel message, it is not entirely clear why this parameter is
>> mandatory now, or that it will be optional in the future.  Add such a
>> more detailed explanation as a comment in the test source file.
>>
>> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> I’ve uploaded a build of Linux 5.10 here:
>>    https://xanclic.moe/linux-5.10
>>
>> But I’ve decided against mentioning it in this new comment or the cancel
>> message, because, well, it’s my private server and I have limited
>> bandwidth.
>> ---
>>   tests/acceptance/virtiofs_submounts.py | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
>> index 949ca87a83..9a69b6b17b 100644
>> --- a/tests/acceptance/virtiofs_submounts.py
>> +++ b/tests/acceptance/virtiofs_submounts.py
>> @@ -228,6 +228,18 @@ class VirtiofsSubmountsTest(BootLinux):
>>       def setUp(self):
>>           vmlinuz = self.params.get('vmlinuz')
>>           if vmlinuz is None:
>> +            """
>> +            The Linux kernel supports FUSE auto-submounts only as of 5.10.
>> +            boot_linux.py currently provides Fedora 31, whose kernel is too
>> +            old, so this test cannot pass with the on-image kernel (you are
>> +            welcome to try, hence the option to force such a test with
>> +            -p vmlinuz='').  Therefore, for now the user must provide a
>> +            sufficiently new custom kernel, or effectively explicitly
>> +            request failure with -p vmlinuz=''.
>> +            Once an image with a sufficiently new kernel is available
>> +            (probably Fedora 34), we can make -p vmlinuz='' the default, so
>> +            that this parameter no longer needs to be specified.
>> +            """
>>               self.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)')
>> -- 
>> 2.29.2
>>
> 
> Hi Max,
> 
> This looks good to me, and I've also tested your kernel build and
> works like a charm.

Great :)

> As further work on top of this, it may be beneficial to have test
> documentation in a predictable place.  The possibilities that come to
> my mind:
> 
>   * docs/devel/testing.rst
>   * tests/acceptance/$test_file.py/data/README

Hm.  I think I’d prefer a dedicated file, i.e. the second one.  In any 
case, sounds good.

> On a different topic, the "https://avocado-project.org/data/assets" has
> enough bandwidth and can be used to hold this type asset.

I think it can’t hurt to put the kernel there, and then link to it in 
the cancel message.

> Alternatively,
> we can add a bit more automation to this test by letting people do something
> like:
> 
>   $ avocado assets register virtiofs-auto-submounts-vmlinuz /path/to/vmlinuz
> 
> And on the test:
> 
>   vmlinuz = self.fetch_asset('virtiofs-auto-submounts-vmlinuz')
> 
> And the test should cancel if that asset has not been previously registered.

Thanks, I wasn’t aware of this system.  Though I have no idea how 
“assets register” works, I can’t seem to find doc on it on
avocado-framework.readthedocs.io.  Is it global?  Is there a way to 
register an asset of a specific name only for a specific test?

Because I think this would make more sense if we tried to fetch the 
asset from e.g. https://avocado-project.org/data/assets, i.e. just put 
its name as linux-5.10.  Though perhaps it would also work with 
name='virtiofs-auto-submounts-vmlinuz', as you suggested.  But in any 
case, I’m a bit uneasy about a global namespace, which “assets register” 
seems to use.

Max

Re: [PATCH] virtiofs_submounts.py test: Note on vmlinuz param

[Willian Rampazzo]

On Fri, Feb 12, 2021 at 12:20 PM Max Reitz <mreitz@redhat.com> wrote:
>
> From the cancel message, it is not entirely clear why this parameter is
> mandatory now, or that it will be optional in the future.  Add such a
> more detailed explanation as a comment in the test source file.
>
> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> I’ve uploaded a build of Linux 5.10 here:
>   https://xanclic.moe/linux-5.10
>
> But I’ve decided against mentioning it in this new comment or the cancel
> message, because, well, it’s my private server and I have limited
> bandwidth.
> ---
>  tests/acceptance/virtiofs_submounts.py | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

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

Re: [PATCH] virtiofs_submounts.py test: Note on vmlinuz param

[Alex Bennée]

Max Reitz <mreitz@redhat.com> writes:

> From the cancel message, it is not entirely clear why this parameter is
> mandatory now, or that it will be optional in the future.  Add such a
> more detailed explanation as a comment in the test source file.
>
> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>


Thanks!

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

> ---
> I’ve uploaded a build of Linux 5.10 here:
>   https://xanclic.moe/linux-5.10
>
> But I’ve decided against mentioning it in this new comment or the cancel
> message, because, well, it’s my private server and I have limited
> bandwidth.
> ---
>  tests/acceptance/virtiofs_submounts.py | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
> index 949ca87a83..9a69b6b17b 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -228,6 +228,18 @@ class VirtiofsSubmountsTest(BootLinux):
>      def setUp(self):
>          vmlinuz = self.params.get('vmlinuz')
>          if vmlinuz is None:
> +            """
> +            The Linux kernel supports FUSE auto-submounts only as of 5.10.
> +            boot_linux.py currently provides Fedora 31, whose kernel is too
> +            old, so this test cannot pass with the on-image kernel (you are
> +            welcome to try, hence the option to force such a test with
> +            -p vmlinuz='').  Therefore, for now the user must provide a
> +            sufficiently new custom kernel, or effectively explicitly
> +            request failure with -p vmlinuz=''.
> +            Once an image with a sufficiently new kernel is available
> +            (probably Fedora 34), we can make -p vmlinuz='' the default, so
> +            that this parameter no longer needs to be specified.
> +            """
>              self.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)')


-- 
Alex Bennée

Re: [PATCH] virtiofs_submounts.py test: Note on vmlinuz param

[Cleber Rosa]

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

On Fri, Feb 12, 2021 at 04:16:49PM +0100, Max Reitz wrote:
> From the cancel message, it is not entirely clear why this parameter is
> mandatory now, or that it will be optional in the future.  Add such a
> more detailed explanation as a comment in the test source file.
> 
> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> I’ve uploaded a build of Linux 5.10 here:
>   https://xanclic.moe/linux-5.10
> 
> But I’ve decided against mentioning it in this new comment or the cancel
> message, because, well, it’s my private server and I have limited
> bandwidth.
> ---
>  tests/acceptance/virtiofs_submounts.py | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/tests/acceptance/virtiofs_submounts.py b/tests/acceptance/virtiofs_submounts.py
> index 949ca87a83..9a69b6b17b 100644
> --- a/tests/acceptance/virtiofs_submounts.py
> +++ b/tests/acceptance/virtiofs_submounts.py
> @@ -228,6 +228,18 @@ class VirtiofsSubmountsTest(BootLinux):
>      def setUp(self):
>          vmlinuz = self.params.get('vmlinuz')
>          if vmlinuz is None:
> +            """
> +            The Linux kernel supports FUSE auto-submounts only as of 5.10.
> +            boot_linux.py currently provides Fedora 31, whose kernel is too
> +            old, so this test cannot pass with the on-image kernel (you are
> +            welcome to try, hence the option to force such a test with
> +            -p vmlinuz='').  Therefore, for now the user must provide a
> +            sufficiently new custom kernel, or effectively explicitly
> +            request failure with -p vmlinuz=''.
> +            Once an image with a sufficiently new kernel is available
> +            (probably Fedora 34), we can make -p vmlinuz='' the default, so
> +            that this parameter no longer needs to be specified.
> +            """
>              self.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)')
> -- 
> 2.29.2
> 

Hi Max,

FIY I'm queueing this patch.

Thanks,
- Cleber.

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