| Message ID | 20210203172357.1422425-8-crosa@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Acceptance Test: introduce base class for Linux based tests | expand |
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>
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:
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
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
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
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
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
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 ;-)
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.
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 --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')
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(-)