| Message ID | 20210212151649.252440-1-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtiofs_submounts.py test: Note on vmlinuz param | expand |
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>
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!
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>
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)')
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.
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.
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
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)')
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(+)