diff mbox series

[06/10] tests/avocado/kvm_xen_guest.py: cope with asset RW requirements

Message ID 20231208190911.102879-7-crosa@redhat.com (mailing list archive)
State New, archived
Headers show
Series for-8.3 tests/avocado: prep for Avocado 103.0 LTS | expand

Commit Message

Cleber Rosa Dec. 8, 2023, 7:09 p.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé Dec. 11, 2023, 4:32 p.m. UTC | #1
On 8/12/23 20:09, 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.

Having cache assets modified is a design issue. We should assume
the cache directory as read-only.

> 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(-)
Cleber Rosa Aug. 1, 2024, 3:30 a.m. UTC | #2
On Mon, Dec 11, 2023 at 11:32 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 8/12/23 20:09, 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.
>
> Having cache assets modified is a design issue. We should assume
> the cache directory as read-only.
>

I agree those files should not be modified, but I wonder if you
thought about any solution to this? Given that the same user writes
(downloads) those files, do you think setting file permissions between
the download and the use of the files should be done?

That can make the management of the cache (such as pruning it) either
require undoing the restriction or being done by a super user.

Anyway, just curious.

Regards,
- Cleber.
Philippe Mathieu-Daudé Aug. 1, 2024, 12:57 p.m. UTC | #3
On 1/8/24 05:30, Cleber Rosa wrote:
> On Mon, Dec 11, 2023 at 11:32 AM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>
>> On 8/12/23 20:09, 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.
>>
>> Having cache assets modified is a design issue. We should assume
>> the cache directory as read-only.
>>
> 
> I agree those files should not be modified, but I wonder if you
> thought about any solution to this? Given that the same user writes
> (downloads) those files, do you think setting file permissions between
> the download and the use of the files should be done?

We want to share a cachedir on development hosts with multiple
developers. OK to alter a downloaded file before adding it to
the cache; but then once a file is added/hashed it shouldn't be
modified IMO.

So far this directory is group=RW but we like the ability to track
a read-only directory (like owned by a particular user) and adding
missing assets to current user cachedir, to avoid duplication of
files and waste of network transfer.

> That can make the management of the cache (such as pruning it) either
> require undoing the restriction or being done by a super user.
> 
> Anyway, just curious.
> 
> Regards,
> - Cleber.
>
Cleber Rosa Aug. 1, 2024, 3:17 p.m. UTC | #4
On Thu, Aug 1, 2024 at 8:57 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > I agree those files should not be modified, but I wonder if you
> > thought about any solution to this? Given that the same user writes
> > (downloads) those files, do you think setting file permissions between
> > the download and the use of the files should be done?
>
> We want to share a cachedir on development hosts with multiple
> developers. OK to alter a downloaded file before adding it to
> the cache; but then once a file is added/hashed it shouldn't be
> modified IMO.
>

I was asking more in terms of what to do before/after the test.  When
it comes to this type of setup, Avocado's cache was designed to
support this use case.  You can provide multiple cache dirs in the
configuration, and some (the first ones, ideally) can be RO (life NFS
mounts).

But this is hardly something that can be configured without proper
user input, so this is not present in the generic "make
check-avocado".

> So far this directory is group=RW but we like the ability to track
> a read-only directory (like owned by a particular user) and adding
> missing assets to current user cachedir, to avoid duplication of
> files and waste of network transfer.
>

That can be done in avocado.conf, something like:

[datadir.paths]
cache_dirs = ['/path/that/is/ro/because/owned/by/someone/else',
'/home/cleber/avocado/data/cache']

The asset library will take care of trying to find assets in the RO
directories, while writing to the RW ones.

Hope this helps,
- Cleber.
Philippe Mathieu-Daudé Aug. 2, 2024, 1:14 p.m. UTC | #5
On 1/8/24 17:17, Cleber Rosa wrote:
> On Thu, Aug 1, 2024 at 8:57 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>> I agree those files should not be modified, but I wonder if you
>>> thought about any solution to this? Given that the same user writes
>>> (downloads) those files, do you think setting file permissions between
>>> the download and the use of the files should be done?
>>
>> We want to share a cachedir on development hosts with multiple
>> developers. OK to alter a downloaded file before adding it to
>> the cache; but then once a file is added/hashed it shouldn't be
>> modified IMO.
>>
> 
> I was asking more in terms of what to do before/after the test.  When
> it comes to this type of setup, Avocado's cache was designed to
> support this use case.  You can provide multiple cache dirs in the
> configuration, and some (the first ones, ideally) can be RO (life NFS
> mounts).
> 
> But this is hardly something that can be configured without proper
> user input, so this is not present in the generic "make
> check-avocado".
> 
>> So far this directory is group=RW but we like the ability to track
>> a read-only directory (like owned by a particular user) and adding
>> missing assets to current user cachedir, to avoid duplication of
>> files and waste of network transfer.
>>
> 
> That can be done in avocado.conf, something like:
> 
> [datadir.paths]
> cache_dirs = ['/path/that/is/ro/because/owned/by/someone/else',
> '/home/cleber/avocado/data/cache']
> 
> The asset library will take care of trying to find assets in the RO
> directories, while writing to the RW ones.

Great, thanks!
diff mbox series

Patch

diff --git a/tests/avocado/kvm_xen_guest.py b/tests/avocado/kvm_xen_guest.py
index ec4052a1fe..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,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):