| Message ID | 20190621153806.13489-1-wainersm@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] tests/acceptance: Handle machine type for ARM target | expand |
On Fri, Jun 21, 2019 at 11:38:06AM -0400, Wainer dos Santos Moschetta wrote: > Hi all, > > I'm still unsure this is the best solution. I tend to think that > any arch-independent test case (i.e. not tagged 'arch') should > be skipped on all arches except for x86_64. Opening up for > discussion though. > I'm confused... if you're calling a test case "arch-independent", why should it be skipped on all but one arch? Anyway, I don't think we should define such a broad policy... This line of thought is very x86_64 centric, and quite honestly, doesn't map to QEMU's goals. I agree that we're being a bit "disonest" by not assuring that tests we send will work on all targets... but at least we're having that discussion. The next step would be to start triaging and discussing wether it's worth running those against other targets, considering the cost and benefits. > Note: It was decided that ARM targets should not default to any > machine type: https://www.mail-archive.com/qemu-devel@nongnu.org/msg625999.html > > -- 8< -- > Some tests are meant arch-independent and as such they don't set > the machine type (i.e. relying to defaults) on launched VMs. The arm > targets, however, don't provide any default machine so tests fail. > > This patch adds a logic on the base Test class so that machine type > is set to 'virt' when: > a) The test case doesn't have arch:aarch64 or arch:arm tag. Here > I assume that if the test was tagged for a specific arch then > the writer took care of setting a machine type. > b) The target binary arch is any of aarch64 or arm. Note: > self.target_arch can end up None if qemu_bin is passed by > Avocado parameter and the filename doesn't match expected > format. In this case the test will fail. > > Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com> > --- > tests/acceptance/avocado_qemu/__init__.py | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py > index 2b236a1cf0..fb3e0dc2bc 100644 > --- a/tests/acceptance/avocado_qemu/__init__.py > +++ b/tests/acceptance/avocado_qemu/__init__.py > @@ -9,6 +9,7 @@ > # later. See the COPYING file in the top-level directory. > > import os > +import re > import sys > import uuid > > @@ -65,10 +66,21 @@ class Test(avocado.Test): > if self.qemu_bin is None: > self.cancel("No QEMU binary defined or found in the source tree") > > + m = re.match('qemu-system-(.*)', self.qemu_bin.split('/').pop()) > + if m: > + self.target_arch = m.group(1) > + else: > + self.target_arch = None > + The "arch" tag and parameter are actually related to the target that should be used. I don't see the need for a "target_arch" based on that. > def _new_vm(self, *args): > vm = QEMUMachine(self.qemu_bin) > if args: > vm.add_args(*args) > + # Handle lack of default machine type on some targets. > + # Assume that arch tagged tests have machine type set properly. > + if self.tags.get('arch') is None and \ > + self.target_arch in ('aarch64', 'arm'): > + vm.set_machine('virt') This (considering it deals with "arch" instead of "target_arch") is one of the very important points to be determined. How much wrapping around different QEMU behavior on different targets/machines/devices should we do? This will possibly be case-by-case discussions with different outcomes, but hopefully we can come up with a general direction. Thanks, - Cleber. > return vm > > @property > -- > 2.18.1 >
On 06/21/2019 07:07 PM, Cleber Rosa wrote: > On Fri, Jun 21, 2019 at 11:38:06AM -0400, Wainer dos Santos Moschetta wrote: >> Hi all, >> >> I'm still unsure this is the best solution. I tend to think that >> any arch-independent test case (i.e. not tagged 'arch') should >> be skipped on all arches except for x86_64. Opening up for >> discussion though. >> > I'm confused... if you're calling a test case "arch-independent", why > should it be skipped on all but one arch? Anyway, I don't think we > should define such a broad policy... This line of thought is very > x86_64 centric, and quite honestly, doesn't map to QEMU's goals. What I meant for "arch-independent" is a test which does not have "arch" tag. You call it "generic test". We can agree on use "generic test" for better communication. The absence of "arch" tag can means either 1) "run on all built targets" or 2) "pick one target and run it". I suggested x86_64 thinking on case 2) because it provides a default machine type that I supposed (I might be indeed wrong) just works on most cases. Your follow up patch [1] addresses the scenario 1) but allows the user to pick that one target of 2). All in all, we should agree on the semantic of "arch" when it is not present. Should it be "any" or "all"? Configurable or not? Let's discuss this on [1]. So NACK this patch. [1] [RFC PATCH] Acceptance tests: run generic tests on all built targets > > I agree that we're being a bit "disonest" by not assuring that tests > we send will work on all targets... but at least we're having that > discussion. The next step would be to start triaging and discussing > wether it's worth running those against other targets, considering > the cost and benefits. > >> Note: It was decided that ARM targets should not default to any >> machine type: https://www.mail-archive.com/qemu-devel@nongnu.org/msg625999.html >> >> -- 8< -- >> Some tests are meant arch-independent and as such they don't set >> the machine type (i.e. relying to defaults) on launched VMs. The arm >> targets, however, don't provide any default machine so tests fail. >> >> This patch adds a logic on the base Test class so that machine type >> is set to 'virt' when: >> a) The test case doesn't have arch:aarch64 or arch:arm tag. Here >> I assume that if the test was tagged for a specific arch then >> the writer took care of setting a machine type. >> b) The target binary arch is any of aarch64 or arm. Note: >> self.target_arch can end up None if qemu_bin is passed by >> Avocado parameter and the filename doesn't match expected >> format. In this case the test will fail. >> >> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com> >> --- >> tests/acceptance/avocado_qemu/__init__.py | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py >> index 2b236a1cf0..fb3e0dc2bc 100644 >> --- a/tests/acceptance/avocado_qemu/__init__.py >> +++ b/tests/acceptance/avocado_qemu/__init__.py >> @@ -9,6 +9,7 @@ >> # later. See the COPYING file in the top-level directory. >> >> import os >> +import re >> import sys >> import uuid >> >> @@ -65,10 +66,21 @@ class Test(avocado.Test): >> if self.qemu_bin is None: >> self.cancel("No QEMU binary defined or found in the source tree") >> >> + m = re.match('qemu-system-(.*)', self.qemu_bin.split('/').pop()) >> + if m: >> + self.target_arch = m.group(1) >> + else: >> + self.target_arch = None >> + > The "arch" tag and parameter are actually related to the target that > should be used. I don't see the need for a "target_arch" based on > that. I could have used self.arch indeed. But notice that self.arch is None when "arch" tag does not exist. So I still need to guess the architecture from the binary. > >> def _new_vm(self, *args): >> vm = QEMUMachine(self.qemu_bin) >> if args: >> vm.add_args(*args) >> + # Handle lack of default machine type on some targets. >> + # Assume that arch tagged tests have machine type set properly. >> + if self.tags.get('arch') is None and \ >> + self.target_arch in ('aarch64', 'arm'): >> + vm.set_machine('virt') > This (considering it deals with "arch" instead of "target_arch") is > one of the very important points to be determined. How much wrapping > around different QEMU behavior on different targets/machines/devices > should we do? This will possibly be case-by-case discussions with > different outcomes, but hopefully we can come up with a general > direction. Yep. Let's continue that discussion on your patch. Thanks for the comments! - Wainer > > Thanks, > - Cleber. > >> return vm >> >> @property >> -- >> 2.18.1 >>
diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py index 2b236a1cf0..fb3e0dc2bc 100644 --- a/tests/acceptance/avocado_qemu/__init__.py +++ b/tests/acceptance/avocado_qemu/__init__.py @@ -9,6 +9,7 @@ # later. See the COPYING file in the top-level directory. import os +import re import sys import uuid @@ -65,10 +66,21 @@ class Test(avocado.Test): if self.qemu_bin is None: self.cancel("No QEMU binary defined or found in the source tree") + m = re.match('qemu-system-(.*)', self.qemu_bin.split('/').pop()) + if m: + self.target_arch = m.group(1) + else: + self.target_arch = None + def _new_vm(self, *args): vm = QEMUMachine(self.qemu_bin) if args: vm.add_args(*args) + # Handle lack of default machine type on some targets. + # Assume that arch tagged tests have machine type set properly. + if self.tags.get('arch') is None and \ + self.target_arch in ('aarch64', 'arm'): + vm.set_machine('virt') return vm @property
Hi all, I'm still unsure this is the best solution. I tend to think that any arch-independent test case (i.e. not tagged 'arch') should be skipped on all arches except for x86_64. Opening up for discussion though. Note: It was decided that ARM targets should not default to any machine type: https://www.mail-archive.com/qemu-devel@nongnu.org/msg625999.html -- 8< -- Some tests are meant arch-independent and as such they don't set the machine type (i.e. relying to defaults) on launched VMs. The arm targets, however, don't provide any default machine so tests fail. This patch adds a logic on the base Test class so that machine type is set to 'virt' when: a) The test case doesn't have arch:aarch64 or arch:arm tag. Here I assume that if the test was tagged for a specific arch then the writer took care of setting a machine type. b) The target binary arch is any of aarch64 or arm. Note: self.target_arch can end up None if qemu_bin is passed by Avocado parameter and the filename doesn't match expected format. In this case the test will fail. Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com> --- tests/acceptance/avocado_qemu/__init__.py | 12 ++++++++++++ 1 file changed, 12 insertions(+)