| Message ID | 20240726134438.14720-8-crosa@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Bump Avocado to 103.0 LTS and update tests for compatibility and new features | expand |
On Fri, Jul 26, 2024 at 09:44:32AM -0400, Cleber Rosa wrote: > Some of these tests actually require the root filesystem image, > obtained through Avocado's asset feature and kept in a common cache > location, to be writable. > > This makes a distinction between the tests that actually have this > requirement and those who don't. The goal is to be as safe as > possible, avoiding causing cache misses (because the assets get > modified and thus need to be dowloaded again) while avoid copying the > root filesystem backing file whenever possible. > > This also allow these tests to be run in parallel with newer Avocado > versions. > > Signed-off-by: Cleber Rosa <crosa@redhat.com> > --- > tests/avocado/kvm_xen_guest.py | 27 ++++++++++++++++++--------- > 1 file changed, 18 insertions(+), 9 deletions(-) > > diff --git a/tests/avocado/kvm_xen_guest.py b/tests/avocado/kvm_xen_guest.py > index 318fadebc3..d73fa888ef 100644 > --- a/tests/avocado/kvm_xen_guest.py > +++ b/tests/avocado/kvm_xen_guest.py > @@ -10,6 +10,7 @@ > # SPDX-License-Identifier: GPL-2.0-or-later > > import os > +import shutil > > from qemu.machine import machine > > @@ -43,7 +44,7 @@ def get_asset(self, name, sha1): > return self.fetch_asset(name=f"qemu-kvm-xen-guest-{name}", > locations=(url), asset_hash=sha1) > > - def common_vm_setup(self): > + def common_vm_setup(self, readwrite=False): > # We also catch lack of KVM_XEN support if we fail to launch > self.require_accelerator("kvm") > > @@ -56,11 +57,19 @@ def common_vm_setup(self): > "367962983d0d32109998a70b45dcee4672d0b045") > self.rootfs = self.get_asset("rootfs.ext4", > "f1478401ea4b3fa2ea196396be44315bab2bb5e4") > + if readwrite: > + dest = os.path.join(self.workdir, os.path.basename(self.rootfs)) > + shutil.copy(self.rootfs, dest) > + self.rootfs = dest This is a very expensive way of creating a writable disk. Better to avoid adding this 'readwrite' parameter at all, and instead create a throwaway qcow2 overlay for the image for all tests. That ensures writability for everything in a cheap manner. > > - def run_and_check(self): > + def run_and_check(self, readwrite=False): > + if readwrite: > + drive = f"file={self.rootfs},if=none,format=raw,id=drv0" > + else: > + drive = f"file={self.rootfs},if=none,readonly=on,format=raw,id=drv0" > self.vm.add_args('-kernel', self.kernel_path, > '-append', self.kernel_params, > - '-drive', f"file={self.rootfs},if=none,snapshot=on,format=raw,id=drv0", > + '-drive', drive, > '-device', 'xen-disk,drive=drv0,vdev=xvda', > '-device', 'virtio-net-pci,netdev=unet', > '-netdev', 'user,id=unet,hostfwd=:127.0.0.1:0-:22') > @@ -90,11 +99,11 @@ def test_kvm_xen_guest(self): > :avocado: tags=kvm_xen_guest > """ > > - self.common_vm_setup() > + self.common_vm_setup(True) > > self.kernel_params = (self.KERNEL_DEFAULT + > ' xen_emul_unplug=ide-disks') > - self.run_and_check() > + self.run_and_check(True) > self.ssh_command('grep xen-pirq.*msi /proc/interrupts') > > def test_kvm_xen_guest_nomsi(self): > @@ -102,11 +111,11 @@ def test_kvm_xen_guest_nomsi(self): > :avocado: tags=kvm_xen_guest_nomsi > """ > > - self.common_vm_setup() > + self.common_vm_setup(True) > > self.kernel_params = (self.KERNEL_DEFAULT + > ' xen_emul_unplug=ide-disks pci=nomsi') > - self.run_and_check() > + self.run_and_check(True) > self.ssh_command('grep xen-pirq.* /proc/interrupts') > > def test_kvm_xen_guest_noapic_nomsi(self): > @@ -114,11 +123,11 @@ def test_kvm_xen_guest_noapic_nomsi(self): > :avocado: tags=kvm_xen_guest_noapic_nomsi > """ > > - self.common_vm_setup() > + self.common_vm_setup(True) > > self.kernel_params = (self.KERNEL_DEFAULT + > ' xen_emul_unplug=ide-disks noapic pci=nomsi') > - self.run_and_check() > + self.run_and_check(True) > self.ssh_command('grep xen-pirq /proc/interrupts') > > def test_kvm_xen_guest_vapic(self): > -- > 2.45.2 > > With regards, Daniel
On Mon, 2024-07-29 at 11:58 +0100, Daniel P. Berrangé wrote: > On Fri, Jul 26, 2024 at 09:44:32AM -0400, Cleber Rosa wrote: > > Some of these tests actually require the root filesystem image, > > obtained through Avocado's asset feature and kept in a common cache > > location, to be writable. Hm, I'm not sure *why* they require a writable image. Mostly they're just testing the interrupt routing. What's the failure mode for a read- only image? > > @@ -56,11 +57,19 @@ def common_vm_setup(self): > > "367962983d0d32109998a70b45dcee4672d0b045") > > self.rootfs = self.get_asset("rootfs.ext4", > > "f1478401ea4b3fa2ea196396be44315bab2bb5e4") > > + if readwrite: > > + dest = os.path.join(self.workdir, os.path.basename(self.rootfs)) > > + shutil.copy(self.rootfs, dest) > > + self.rootfs = dest > > This is a very expensive way of creating a writable disk. Better to > avoid adding this 'readwrite' parameter at all, and instead create > a throwaway qcow2 overlay for the image for all tests. That ensures > writability for everything in a cheap manner. Or just use -snapshot?
David Woodhouse <dwmw2@infradead.org> writes: > On Mon, 2024-07-29 at 11:58 +0100, Daniel P. Berrangé wrote: >> On Fri, Jul 26, 2024 at 09:44:32AM -0400, Cleber Rosa wrote: >> > Some of these tests actually require the root filesystem image, >> > obtained through Avocado's asset feature and kept in a common cache >> > location, to be writable. > > Hm, I'm not sure *why* they require a writable image. Mostly they're > just testing the interrupt routing. What's the failure mode for a read- > only image? There are no real failures with readonly=on although you do see init complain a bit about not being able to mount /dev and open log files. snapshot allows that to happen but doesn't change the underlying storage. > >> > @@ -56,11 +57,19 @@ def common_vm_setup(self): >> > "367962983d0d32109998a70b45dcee4672d0b045") >> > self.rootfs = self.get_asset("rootfs.ext4", >> > "f1478401ea4b3fa2ea196396be44315bab2bb5e4") >> > + if readwrite: >> > + dest = os.path.join(self.workdir, os.path.basename(self.rootfs)) >> > + shutil.copy(self.rootfs, dest) >> > + self.rootfs = dest >> >> This is a very expensive way of creating a writable disk. Better to >> avoid adding this 'readwrite' parameter at all, and instead create >> a throwaway qcow2 overlay for the image for all tests. That ensures >> writability for everything in a cheap manner. > > Or just use -snapshot?
On Thu, Aug 1, 2024 at 10:26 AM Alex Bennée <alex.bennee@linaro.org> wrote: > There are no real failures with readonly=on although you do see init > complain a bit about not being able to mount /dev and open log files. > snapshot allows that to happen but doesn't change the underlying > storage. > I could swear I experienced failures when I developed this patch. But like your patch b9371a7b90 already fixes it. Dropping this one. Thanks! - Cleber.
diff --git a/tests/avocado/kvm_xen_guest.py b/tests/avocado/kvm_xen_guest.py index 318fadebc3..d73fa888ef 100644 --- a/tests/avocado/kvm_xen_guest.py +++ b/tests/avocado/kvm_xen_guest.py @@ -10,6 +10,7 @@ # SPDX-License-Identifier: GPL-2.0-or-later import os +import shutil from qemu.machine import machine @@ -43,7 +44,7 @@ def get_asset(self, name, sha1): return self.fetch_asset(name=f"qemu-kvm-xen-guest-{name}", locations=(url), asset_hash=sha1) - def common_vm_setup(self): + def common_vm_setup(self, readwrite=False): # We also catch lack of KVM_XEN support if we fail to launch self.require_accelerator("kvm") @@ -56,11 +57,19 @@ def common_vm_setup(self): "367962983d0d32109998a70b45dcee4672d0b045") self.rootfs = self.get_asset("rootfs.ext4", "f1478401ea4b3fa2ea196396be44315bab2bb5e4") + if readwrite: + dest = os.path.join(self.workdir, os.path.basename(self.rootfs)) + shutil.copy(self.rootfs, dest) + self.rootfs = dest - def run_and_check(self): + def run_and_check(self, readwrite=False): + if readwrite: + drive = f"file={self.rootfs},if=none,format=raw,id=drv0" + else: + drive = f"file={self.rootfs},if=none,readonly=on,format=raw,id=drv0" self.vm.add_args('-kernel', self.kernel_path, '-append', self.kernel_params, - '-drive', f"file={self.rootfs},if=none,snapshot=on,format=raw,id=drv0", + '-drive', drive, '-device', 'xen-disk,drive=drv0,vdev=xvda', '-device', 'virtio-net-pci,netdev=unet', '-netdev', 'user,id=unet,hostfwd=:127.0.0.1:0-:22') @@ -90,11 +99,11 @@ def test_kvm_xen_guest(self): :avocado: tags=kvm_xen_guest """ - self.common_vm_setup() + self.common_vm_setup(True) self.kernel_params = (self.KERNEL_DEFAULT + ' xen_emul_unplug=ide-disks') - self.run_and_check() + self.run_and_check(True) self.ssh_command('grep xen-pirq.*msi /proc/interrupts') def test_kvm_xen_guest_nomsi(self): @@ -102,11 +111,11 @@ def test_kvm_xen_guest_nomsi(self): :avocado: tags=kvm_xen_guest_nomsi """ - self.common_vm_setup() + self.common_vm_setup(True) self.kernel_params = (self.KERNEL_DEFAULT + ' xen_emul_unplug=ide-disks pci=nomsi') - self.run_and_check() + self.run_and_check(True) self.ssh_command('grep xen-pirq.* /proc/interrupts') def test_kvm_xen_guest_noapic_nomsi(self): @@ -114,11 +123,11 @@ def test_kvm_xen_guest_noapic_nomsi(self): :avocado: tags=kvm_xen_guest_noapic_nomsi """ - self.common_vm_setup() + self.common_vm_setup(True) self.kernel_params = (self.KERNEL_DEFAULT + ' xen_emul_unplug=ide-disks noapic pci=nomsi') - self.run_and_check() + self.run_and_check(True) self.ssh_command('grep xen-pirq /proc/interrupts') def test_kvm_xen_guest_vapic(self):
Some of these tests actually require the root filesystem image, obtained through Avocado's asset feature and kept in a common cache location, to be writable. This makes a distinction between the tests that actually have this requirement and those who don't. The goal is to be as safe as possible, avoiding causing cache misses (because the assets get modified and thus need to be dowloaded again) while avoid copying the root filesystem backing file whenever possible. This also allow these tests to be run in parallel with newer Avocado versions. Signed-off-by: Cleber Rosa <crosa@redhat.com> --- tests/avocado/kvm_xen_guest.py | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-)