[PATCH 00/10] for-8.3 tests/avocado: prep for Avocado 103.0 LTS

[PATCH 00/10] for-8.3 tests/avocado: prep for Avocado 103.0 LTS

[Cleber Rosa]

This is a collection of improvements to a number of Avocado based
tests, but also fixes that will allow them to behave properly under
Avocado's upcoming new Long Term Stability release (LTS) version
103.0.

A pipeline with (pretty much) these changes can be seen at:
  - https://gitlab.com/cleber.gnu/qemu/-/pipelines/1096168899

While a pipeline with the Avocado version bump (using a preview of the
103.0 release) can be seen at:
  - https://gitlab.com/cleber.gnu/qemu/-/pipelines/1099488480

Once Avocado officially releases 103.0 LTS, which is expected to take
no longer than 2 weeks (after a huge development window), the actual
version bump will be posted, along with more profound changes to the
tests to leverage the new features.

Cleber Rosa (10):
  tests/avocado: mips: fallback to HTTP given certificate expiration
  tests/avocado: mips: add hint for fetchasset plugin
  tests/avocado/intel_iommu.py: increase timeout
  tests/avocado: machine aarch64: standardize location and RO/RW access
  tests/avocado: use more distinct names for assets
  tests/avocado/kvm_xen_guest.py: cope with asset RW requirements
  testa/avocado: test_arm_emcraft_sf2: handle RW requirements for asset
  tests/avocado/boot_xen.py: merge base classes
  tests/avocado/boot_xen.py: unify tags
  tests/avocado/boot_xen.py: use class attribute

 tests/avocado/boot_linux_console.py      | 27 +++++++++++++++----
 tests/avocado/boot_xen.py                | 34 +++++-------------------
 tests/avocado/intel_iommu.py             |  2 ++
 tests/avocado/kvm_xen_guest.py           | 30 ++++++++++++++-------
 tests/avocado/machine_aarch64_sbsaref.py |  9 +++++--
 tests/avocado/machine_aarch64_virt.py    | 14 +++++-----
 tests/avocado/netdev-ethtool.py          |  3 ++-
 7 files changed, 67 insertions(+), 52 deletions(-)

-- 
2.43.0

[PATCH 01/10] tests/avocado: mips: fallback to HTTP given certificate expiration

[Cleber Rosa]

The SSL certificate installed at mipsdistros.mips.com has expired:

 0 s:CN = mipsdistros.mips.com
 i:C = US, O = Amazon, OU = Server CA 1B, CN = Amazon
 a:PKEY: rsaEncryption, 2048 (bit); sigalg: RSA-SHA256
 v:NotBefore: Dec 23 00:00:00 2019 GMT; NotAfter: Jan 23 12:00:00 2021 GMT

Because this project has no control over that certificate and host,
this falls back to plain HTTP instead.  The integrity of the
downloaded files can be guaranteed by the existing hashes for those
files (which are not modified here).

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/avocado/boot_linux_console.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/avocado/boot_linux_console.py b/tests/avocado/boot_linux_console.py
index 3f0180e1f8..8066861c17 100644
--- a/tests/avocado/boot_linux_console.py
+++ b/tests/avocado/boot_linux_console.py
@@ -299,7 +299,7 @@ def test_mips_malta32el_nanomips_4k(self):
         :avocado: tags=endian:little
         :avocado: tags=cpu:I7200
         """
-        kernel_url = ('https://mipsdistros.mips.com/LinuxDistro/nanomips/'
+        kernel_url = ('http://mipsdistros.mips.com/LinuxDistro/nanomips/'
                       'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
                       'generic_nano32r6el_page4k.xz')
         kernel_hash = '477456aafd2a0f1ddc9482727f20fe9575565dd6'
@@ -312,7 +312,7 @@ def test_mips_malta32el_nanomips_16k_up(self):
         :avocado: tags=endian:little
         :avocado: tags=cpu:I7200
         """
-        kernel_url = ('https://mipsdistros.mips.com/LinuxDistro/nanomips/'
+        kernel_url = ('http://mipsdistros.mips.com/LinuxDistro/nanomips/'
                       'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
                       'generic_nano32r6el_page16k_up.xz')
         kernel_hash = 'e882868f944c71c816e832e2303b7874d044a7bc'
@@ -325,7 +325,7 @@ def test_mips_malta32el_nanomips_64k_dbg(self):
         :avocado: tags=endian:little
         :avocado: tags=cpu:I7200
         """
-        kernel_url = ('https://mipsdistros.mips.com/LinuxDistro/nanomips/'
+        kernel_url = ('http://mipsdistros.mips.com/LinuxDistro/nanomips/'
                       'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
                       'generic_nano32r6el_page64k_dbg.xz')
         kernel_hash = '18d1c68f2e23429e266ca39ba5349ccd0aeb7180'
-- 
2.43.0

Re: [PATCH 01/10] tests/avocado: mips: fallback to HTTP given certificate expiration

[Alex Bennée]

Cleber Rosa <crosa@redhat.com> writes:

> The SSL certificate installed at mipsdistros.mips.com has expired:
>
>  0 s:CN = mipsdistros.mips.com
>  i:C = US, O = Amazon, OU = Server CA 1B, CN = Amazon
>  a:PKEY: rsaEncryption, 2048 (bit); sigalg: RSA-SHA256
>  v:NotBefore: Dec 23 00:00:00 2019 GMT; NotAfter: Jan 23 12:00:00 2021 GMT
>
> Because this project has no control over that certificate and host,
> this falls back to plain HTTP instead.  The integrity of the
> downloaded files can be guaranteed by the existing hashes for those
> files (which are not modified here).
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 02/10] tests/avocado: mips: add hint for fetchasset plugin

[Cleber Rosa]

Avocado's fetchasset plugin runs before the actual Avocado job (and
any test).  It analyses the test's code looking for occurrences of
"self.fetch_asset()" in the either the actual test or setUp() method.
It's not able to fully analyze all code, though.

The way these tests are written, make the fetchasset plugin blind to
the assets.  This adds redundant code, true, but one that doesn't hurt
the test and aids the fetchasset plugin to download or verify the
existence of these assets in advance.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/avocado/boot_linux_console.py | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tests/avocado/boot_linux_console.py b/tests/avocado/boot_linux_console.py
index 8066861c17..f5c5d647a4 100644
--- a/tests/avocado/boot_linux_console.py
+++ b/tests/avocado/boot_linux_console.py
@@ -303,6 +303,11 @@ def test_mips_malta32el_nanomips_4k(self):
                       'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
                       'generic_nano32r6el_page4k.xz')
         kernel_hash = '477456aafd2a0f1ddc9482727f20fe9575565dd6'
+
+        # The following line is a no-op that aids the avocado
+        # fetchasset plugin that runs before any portion of the test
+        self.fetch_asset(kernel_url, asset_hash=kernel_hash)
+
         self.do_test_mips_malta32el_nanomips(kernel_url, kernel_hash)
 
     def test_mips_malta32el_nanomips_16k_up(self):
@@ -316,6 +321,11 @@ def test_mips_malta32el_nanomips_16k_up(self):
                       'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
                       'generic_nano32r6el_page16k_up.xz')
         kernel_hash = 'e882868f944c71c816e832e2303b7874d044a7bc'
+
+        # The following line is a no-op that aids the avocado
+        # fetchasset plugin that runs before any portion of the test
+        self.fetch_asset(kernel_url, asset_hash=kernel_hash)
+
         self.do_test_mips_malta32el_nanomips(kernel_url, kernel_hash)
 
     def test_mips_malta32el_nanomips_64k_dbg(self):
@@ -329,6 +339,11 @@ def test_mips_malta32el_nanomips_64k_dbg(self):
                       'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
                       'generic_nano32r6el_page64k_dbg.xz')
         kernel_hash = '18d1c68f2e23429e266ca39ba5349ccd0aeb7180'
+
+        # The following line is a no-op that aids the avocado
+        # fetchasset plugin that runs before any portion of the test
+        self.fetch_asset(kernel_url, asset_hash=kernel_hash)
+
         self.do_test_mips_malta32el_nanomips(kernel_url, kernel_hash)
 
     def test_aarch64_xlnx_versal_virt(self):
-- 
2.43.0

Re: [PATCH 02/10] tests/avocado: mips: add hint for fetchasset plugin

[Akihiko Odaki]

On 2023/12/09 4:09, Cleber Rosa wrote:
> Avocado's fetchasset plugin runs before the actual Avocado job (and
> any test).  It analyses the test's code looking for occurrences of
> "self.fetch_asset()" in the either the actual test or setUp() method.
> It's not able to fully analyze all code, though.
> 
> The way these tests are written, make the fetchasset plugin blind to
> the assets.  This adds redundant code, true, but one that doesn't hurt
> the test and aids the fetchasset plugin to download or verify the
> existence of these assets in advance.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Why not delete fetch_asset() in do_test_mips_malta32el_nanomips()?

Re: [PATCH 02/10] tests/avocado: mips: add hint for fetchasset plugin

[Cleber Rosa]

Akihiko Odaki <akihiko.odaki@daynix.com> writes:

> On 2023/12/09 4:09, Cleber Rosa wrote:
>> Avocado's fetchasset plugin runs before the actual Avocado job (and
>> any test).  It analyses the test's code looking for occurrences of
>> "self.fetch_asset()" in the either the actual test or setUp() method.
>> It's not able to fully analyze all code, though.
>> 
>> The way these tests are written, make the fetchasset plugin blind to
>> the assets.  This adds redundant code, true, but one that doesn't hurt
>> the test and aids the fetchasset plugin to download or verify the
>> existence of these assets in advance.
>> 
>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>
> Why not delete fetch_asset() in do_test_mips_malta32el_nanomips()?

I was trying to preserve do_test_mips_malta32el_nanomips() in such a way
that with the eventual migration to the "dependency" system in newer
Avocado, the lines added here could simply be reversed.

But, that's not a strong enough reason to justify the duplication.  I'll
follow your suggestion on v2.

Thanks!
- Cleber.

[PATCH 03/10] tests/avocado/intel_iommu.py: increase timeout

[Cleber Rosa]

Based on many runs, the average run time for these 4 tests is around
250 seconds, with 320 seconds being the ceiling.  In any way, the
default 120 seconds timeout is inappropriate in my experience.

Let's increase the timeout so these tests get a chance to completion.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/avocado/intel_iommu.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/avocado/intel_iommu.py b/tests/avocado/intel_iommu.py
index f04ee1cf9d..24bfad0756 100644
--- a/tests/avocado/intel_iommu.py
+++ b/tests/avocado/intel_iommu.py
@@ -25,6 +25,8 @@ class IntelIOMMU(LinuxTest):
     :avocado: tags=flaky
     """
 
+    timeout = 360
+
     IOMMU_ADDON = ',iommu_platform=on,disable-modern=off,disable-legacy=on'
     kernel_path = None
     initrd_path = None
-- 
2.43.0

Re: [PATCH 03/10] tests/avocado/intel_iommu.py: increase timeout

[Alex Bennée]

Cleber Rosa <crosa@redhat.com> writes:

> Based on many runs, the average run time for these 4 tests is around
> 250 seconds, with 320 seconds being the ceiling.  In any way, the
> default 120 seconds timeout is inappropriate in my experience.

I would rather see these tests updated to fix:

 - Don't use such an old Fedora 31 image
 - Avoid updating image packages (when will RH stop serving them?)
 - The "test" is a fairly basic check of dmesg/sysfs output

I think building a buildroot image with the tools pre-installed (with
perhaps more testing) would be a better use of our limited test time.

FWIW the runtime on my machine is:

➜  env QEMU_TEST_FLAKY_TESTS=1 ./pyvenv/bin/avocado run ./tests/avocado/intel_iommu.py
JOB ID     : 5c582ccf274f3aee279c2208f969a7af8ceb9943
JOB LOG    : /home/alex/avocado/job-results/job-2023-12-11T16.53-5c582cc/job.log
 (1/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu: PASS (44.21 s)
 (2/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu_strict: PASS (78.60 s)
 (3/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu_strict_cm: PASS (65.57 s)
 (4/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu_pt: PASS (66.63 s)
RESULTS    : PASS 4 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 255.43 s

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 03/10] tests/avocado/intel_iommu.py: increase timeout

[Akihiko Odaki]

On 2023/12/12 2:01, Alex Bennée wrote:
> Cleber Rosa <crosa@redhat.com> writes:
> 
>> Based on many runs, the average run time for these 4 tests is around
>> 250 seconds, with 320 seconds being the ceiling.  In any way, the
>> default 120 seconds timeout is inappropriate in my experience.
> 
> I would rather see these tests updated to fix:
> 
>   - Don't use such an old Fedora 31 image
>   - Avoid updating image packages (when will RH stop serving them?)
>   - The "test" is a fairly basic check of dmesg/sysfs output
> 
> I think building a buildroot image with the tools pre-installed (with
> perhaps more testing) would be a better use of our limited test time.

That's what tests/avocado/netdev-ethtool.py does, but I don't like it 
much because building a buildroot image takes long and results in a 
somewhat big binary blob.

I rather prefer to have some script that runs mkosi[1] to make an image; 
it downloads packages from distributor so it will take much less than 
using buildroot. The CI system can run the script and cache the image.

[1] https://github.com/systemd/mkosi

Re: [PATCH 03/10] tests/avocado/intel_iommu.py: increase timeout

[Alex Bennée]

Akihiko Odaki <akihiko.odaki@daynix.com> writes:

> On 2023/12/12 2:01, Alex Bennée wrote:
>> Cleber Rosa <crosa@redhat.com> writes:
>> 
>>> Based on many runs, the average run time for these 4 tests is around
>>> 250 seconds, with 320 seconds being the ceiling.  In any way, the
>>> default 120 seconds timeout is inappropriate in my experience.
>> I would rather see these tests updated to fix:
>>   - Don't use such an old Fedora 31 image
>>   - Avoid updating image packages (when will RH stop serving them?)
>>   - The "test" is a fairly basic check of dmesg/sysfs output
>> I think building a buildroot image with the tools pre-installed
>> (with
>> perhaps more testing) would be a better use of our limited test time.
>
> That's what tests/avocado/netdev-ethtool.py does, but I don't like it
> much because building a buildroot image takes long and results in a
> somewhat big binary blob.
>
> I rather prefer to have some script that runs mkosi[1] to make an
> image; it downloads packages from distributor so it will take much
> less than using buildroot. The CI system can run the script and cache
> the image.

I'm all more smaller more directed test cases and I'm less worried about
exactly how things are built. I only use buildroot personally because
I'm familiar with it and it makes it easy to build testcases for
multiple architectures.

> [1] https://github.com/systemd/mkosi

If that works for you go for it ;-)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 03/10] tests/avocado/intel_iommu.py: increase timeout

[Cleber Rosa]

Alex Bennée <alex.bennee@linaro.org> writes:

> Cleber Rosa <crosa@redhat.com> writes:
>
>> Based on many runs, the average run time for these 4 tests is around
>> 250 seconds, with 320 seconds being the ceiling.  In any way, the
>> default 120 seconds timeout is inappropriate in my experience.
>
> I would rather see these tests updated to fix:
>
>  - Don't use such an old Fedora 31 image

I remember proposing a bump in Fedora version used by default in
avocado_qemu.LinuxTest (which would propagate to tests such as
boot_linux.py and others), but that was not well accepted.  I can
definitely work on such a version bump again.

>  - Avoid updating image packages (when will RH stop serving them?)

IIUC the only reason for updating the packages is to test the network
from the guest, and could/should be done another way.

Eric, could you confirm this?

>  - The "test" is a fairly basic check of dmesg/sysfs output

Maybe the network is also an implicit check here.  Let's see what Eric
has to say.

>
> I think building a buildroot image with the tools pre-installed (with
> perhaps more testing) would be a better use of our limited test time.
>
> FWIW the runtime on my machine is:
>
> ➜  env QEMU_TEST_FLAKY_TESTS=1 ./pyvenv/bin/avocado run ./tests/avocado/intel_iommu.py
> JOB ID     : 5c582ccf274f3aee279c2208f969a7af8ceb9943
> JOB LOG    : /home/alex/avocado/job-results/job-2023-12-11T16.53-5c582cc/job.log
>  (1/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu: PASS (44.21 s)
>  (2/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu_strict: PASS (78.60 s)
>  (3/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu_strict_cm: PASS (65.57 s)
>  (4/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu_pt: PASS (66.63 s)
> RESULTS    : PASS 4 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
> JOB TIME   : 255.43 s
>

Yes, I've also seen similar runtimes in other environments... so it
looks like it depends a lot on the "dnf -y install numactl-devel".  If
that can be removed, the tests would have much more predictable runtimes.

Re: [PATCH 03/10] tests/avocado/intel_iommu.py: increase timeout

[Eric Auger]

Hi Cleber,

On 12/13/23 21:08, Cleber Rosa wrote:
> Alex Bennée <alex.bennee@linaro.org> writes:
>
>> Cleber Rosa <crosa@redhat.com> writes:
>>
>>> Based on many runs, the average run time for these 4 tests is around
>>> 250 seconds, with 320 seconds being the ceiling.  In any way, the
>>> default 120 seconds timeout is inappropriate in my experience.
>> I would rather see these tests updated to fix:
>>
>>  - Don't use such an old Fedora 31 image
> I remember proposing a bump in Fedora version used by default in
> avocado_qemu.LinuxTest (which would propagate to tests such as
> boot_linux.py and others), but that was not well accepted.  I can
> definitely work on such a version bump again.
>
>>  - Avoid updating image packages (when will RH stop serving them?)
> IIUC the only reason for updating the packages is to test the network
> from the guest, and could/should be done another way.
>
> Eric, could you confirm this?
Sorry for the delay. Yes effectively I used the dnf install to stress
the viommu. In the past I was able to trigger viommu bugs that way
whereas getting an IP @ for the guest was just successful.
>
>>  - The "test" is a fairly basic check of dmesg/sysfs output
> Maybe the network is also an implicit check here.  Let's see what Eric
> has to say.

To be honest I do not remember how avocado does the check in itself; my
guess if that if the dnf install does not complete you get a timeout and
the test fails. But you may be more knowledged on this than me ;-)

Thanks

Eric
>
>> I think building a buildroot image with the tools pre-installed (with
>> perhaps more testing) would be a better use of our limited test time.
>>
>> FWIW the runtime on my machine is:
>>
>> ➜  env QEMU_TEST_FLAKY_TESTS=1 ./pyvenv/bin/avocado run ./tests/avocado/intel_iommu.py
>> JOB ID     : 5c582ccf274f3aee279c2208f969a7af8ceb9943
>> JOB LOG    : /home/alex/avocado/job-results/job-2023-12-11T16.53-5c582cc/job.log
>>  (1/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu: PASS (44.21 s)
>>  (2/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu_strict: PASS (78.60 s)
>>  (3/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu_strict_cm: PASS (65.57 s)
>>  (4/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu_pt: PASS (66.63 s)
>> RESULTS    : PASS 4 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
>> JOB TIME   : 255.43 s
>>
> Yes, I've also seen similar runtimes in other environments... so it
> looks like it depends a lot on the "dnf -y install numactl-devel".  If
> that can be removed, the tests would have much more predictable runtimes.
>

Re: [PATCH 03/10] tests/avocado/intel_iommu.py: increase timeout

[Alex Bennée]

Eric Auger <eric.auger@redhat.com> writes:

> Hi Cleber,
>
> On 12/13/23 21:08, Cleber Rosa wrote:
>> Alex Bennée <alex.bennee@linaro.org> writes:
>>
>>> Cleber Rosa <crosa@redhat.com> writes:
>>>
>>>> Based on many runs, the average run time for these 4 tests is around
>>>> 250 seconds, with 320 seconds being the ceiling.  In any way, the
>>>> default 120 seconds timeout is inappropriate in my experience.
>>> I would rather see these tests updated to fix:
>>>
>>>  - Don't use such an old Fedora 31 image
>> I remember proposing a bump in Fedora version used by default in
>> avocado_qemu.LinuxTest (which would propagate to tests such as
>> boot_linux.py and others), but that was not well accepted.  I can
>> definitely work on such a version bump again.
>>
>>>  - Avoid updating image packages (when will RH stop serving them?)
>> IIUC the only reason for updating the packages is to test the network
>> from the guest, and could/should be done another way.
>>
>> Eric, could you confirm this?
> Sorry for the delay. Yes effectively I used the dnf install to stress
> the viommu. In the past I was able to trigger viommu bugs that way
> whereas getting an IP @ for the guest was just successful.
>>
>>>  - The "test" is a fairly basic check of dmesg/sysfs output
>> Maybe the network is also an implicit check here.  Let's see what Eric
>> has to say.
>
> To be honest I do not remember how avocado does the check in itself; my
> guess if that if the dnf install does not complete you get a timeout and
> the test fails. But you may be more knowledged on this than me ;-)

I guess the problem is relying on external infrastructure can lead to
unpredictable results. However its a lot easier to configure user mode
networking just to pull something off the internet than have a local
netperf or some such setup to generate local traffic.

I guess there is no loopback like setup which would sufficiently
exercise the code?

>
> Thanks
>
> Eric
>>
>>> I think building a buildroot image with the tools pre-installed (with
>>> perhaps more testing) would be a better use of our limited test time.
>>>
>>> FWIW the runtime on my machine is:
>>>
>>> ➜  env QEMU_TEST_FLAKY_TESTS=1 ./pyvenv/bin/avocado run ./tests/avocado/intel_iommu.py
>>> JOB ID     : 5c582ccf274f3aee279c2208f969a7af8ceb9943
>>> JOB LOG    : /home/alex/avocado/job-results/job-2023-12-11T16.53-5c582cc/job.log
>>>  (1/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu: PASS (44.21 s)
>>>  (2/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu_strict: PASS (78.60 s)
>>>  (3/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu_strict_cm: PASS (65.57 s)
>>>  (4/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu_pt: PASS (66.63 s)
>>> RESULTS    : PASS 4 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
>>> JOB TIME   : 255.43 s
>>>
>> Yes, I've also seen similar runtimes in other environments... so it
>> looks like it depends a lot on the "dnf -y install numactl-devel".  If
>> that can be removed, the tests would have much more predictable runtimes.
>>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 03/10] tests/avocado/intel_iommu.py: increase timeout

[Eric Auger]

On 12/14/23 10:41, Alex Bennée wrote:
> Eric Auger <eric.auger@redhat.com> writes:
>
>> Hi Cleber,
>>
>> On 12/13/23 21:08, Cleber Rosa wrote:
>>> Alex Bennée <alex.bennee@linaro.org> writes:
>>>
>>>> Cleber Rosa <crosa@redhat.com> writes:
>>>>
>>>>> Based on many runs, the average run time for these 4 tests is around
>>>>> 250 seconds, with 320 seconds being the ceiling.  In any way, the
>>>>> default 120 seconds timeout is inappropriate in my experience.
>>>> I would rather see these tests updated to fix:
>>>>
>>>>  - Don't use such an old Fedora 31 image
>>> I remember proposing a bump in Fedora version used by default in
>>> avocado_qemu.LinuxTest (which would propagate to tests such as
>>> boot_linux.py and others), but that was not well accepted.  I can
>>> definitely work on such a version bump again.
>>>
>>>>  - Avoid updating image packages (when will RH stop serving them?)
>>> IIUC the only reason for updating the packages is to test the network
>>> from the guest, and could/should be done another way.
>>>
>>> Eric, could you confirm this?
>> Sorry for the delay. Yes effectively I used the dnf install to stress
>> the viommu. In the past I was able to trigger viommu bugs that way
>> whereas getting an IP @ for the guest was just successful.
>>>>  - The "test" is a fairly basic check of dmesg/sysfs output
>>> Maybe the network is also an implicit check here.  Let's see what Eric
>>> has to say.
>> To be honest I do not remember how avocado does the check in itself; my
>> guess if that if the dnf install does not complete you get a timeout and
>> the test fails. But you may be more knowledged on this than me ;-)
> I guess the problem is relying on external infrastructure can lead to
> unpredictable results. However its a lot easier to configure user mode
> networking just to pull something off the internet than have a local
> netperf or some such setup to generate local traffic.
>
> I guess there is no loopback like setup which would sufficiently
> exercise the code?

I don't think so. This test is a reproducer for a bug I encountered and
fixed in the past.
Besudes, I am totally fine moving the test out of the gating CI and just
keep it as a tier2 test, as suggested by Phil.

Thanks

Eric
>
>> Thanks
>>
>> Eric
>>>> I think building a buildroot image with the tools pre-installed (with
>>>> perhaps more testing) would be a better use of our limited test time.
>>>>
>>>> FWIW the runtime on my machine is:
>>>>
>>>> ➜  env QEMU_TEST_FLAKY_TESTS=1 ./pyvenv/bin/avocado run ./tests/avocado/intel_iommu.py
>>>> JOB ID     : 5c582ccf274f3aee279c2208f969a7af8ceb9943
>>>> JOB LOG    : /home/alex/avocado/job-results/job-2023-12-11T16.53-5c582cc/job.log
>>>>  (1/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu: PASS (44.21 s)
>>>>  (2/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu_strict: PASS (78.60 s)
>>>>  (3/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu_strict_cm: PASS (65.57 s)
>>>>  (4/4) ./tests/avocado/intel_iommu.py:IntelIOMMU.test_intel_iommu_pt: PASS (66.63 s)
>>>> RESULTS    : PASS 4 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
>>>> JOB TIME   : 255.43 s
>>>>
>>> Yes, I've also seen similar runtimes in other environments... so it
>>> looks like it depends a lot on the "dnf -y install numactl-devel".  If
>>> that can be removed, the tests would have much more predictable runtimes.
>>>

Re: [PATCH 03/10] tests/avocado/intel_iommu.py: increase timeout

[Philippe Mathieu-Daudé]

On 14/12/23 08:24, Eric Auger wrote:
> Hi Cleber,
> 
> On 12/13/23 21:08, Cleber Rosa wrote:
>> Alex Bennée <alex.bennee@linaro.org> writes:
>>
>>> Cleber Rosa <crosa@redhat.com> writes:
>>>
>>>> Based on many runs, the average run time for these 4 tests is around
>>>> 250 seconds, with 320 seconds being the ceiling.  In any way, the
>>>> default 120 seconds timeout is inappropriate in my experience.
>>> I would rather see these tests updated to fix:
>>>
>>>   - Don't use such an old Fedora 31 image
>> I remember proposing a bump in Fedora version used by default in
>> avocado_qemu.LinuxTest (which would propagate to tests such as
>> boot_linux.py and others), but that was not well accepted.  I can
>> definitely work on such a version bump again.
>>
>>>   - Avoid updating image packages (when will RH stop serving them?)
>> IIUC the only reason for updating the packages is to test the network
>> from the guest, and could/should be done another way.
>>
>> Eric, could you confirm this?
> Sorry for the delay. Yes effectively I used the dnf install to stress
> the viommu. In the past I was able to trigger viommu bugs that way
> whereas getting an IP @ for the guest was just successful.

Maybe this test is useful as what Daniel described as "Tier 2" [*],
that maintainers run locally but don't need to be gating CI? That
would save us some resources there.

[*] https://lore.kernel.org/qemu-devel/20200427152036.GI1244803@redhat.com/

>>
>>>   - The "test" is a fairly basic check of dmesg/sysfs output
>> Maybe the network is also an implicit check here.  Let's see what Eric
>> has to say.
> 
> To be honest I do not remember how avocado does the check in itself; my
> guess if that if the dnf install does not complete you get a timeout and
> the test fails. But you may be more knowledged on this than me ;-)
> 
> Thanks
> 
> Eric

[PATCH 04/10] tests/avocado: machine aarch64: standardize location and RO/RW access

[Cleber Rosa]

The tests under machine_aarch64_virt.py do not need read-write access
to the ISOs.  The ones under machine_aarch64_sbsaref.py, on the other
hand, will need read-write access, so let's give each test an unique
file.

And while at it, let's use a single code style and hash for the ISO
url.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/avocado/machine_aarch64_sbsaref.py |  9 +++++++--
 tests/avocado/machine_aarch64_virt.py    | 14 +++++++-------
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/tests/avocado/machine_aarch64_sbsaref.py b/tests/avocado/machine_aarch64_sbsaref.py
index 528c7d2934..6ae84d77ac 100644
--- a/tests/avocado/machine_aarch64_sbsaref.py
+++ b/tests/avocado/machine_aarch64_sbsaref.py
@@ -7,6 +7,7 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
 
 import os
+import shutil
 
 from avocado import skipUnless
 from avocado.utils import archive
@@ -123,13 +124,15 @@ def boot_alpine_linux(self, cpu):
 
         iso_hash = "5a36304ecf039292082d92b48152a9ec21009d3a62f459de623e19c4bd9dc027"
         iso_path = self.fetch_asset(iso_url, algorithm="sha256", asset_hash=iso_hash)
+        iso_path_rw = os.path.join(self.workdir, os.path.basename(iso_path))
+        shutil.copy(iso_path, iso_path_rw)
 
         self.vm.set_console()
         self.vm.add_args(
             "-cpu",
             cpu,
             "-drive",
-            f"file={iso_path},format=raw",
+            f"file={iso_path_rw},format=raw",
             "-device",
             "virtio-rng-pci,rng=rng0",
             "-object",
@@ -170,13 +173,15 @@ def boot_openbsd73(self, cpu):
 
         img_hash = "7fc2c75401d6f01fbfa25f4953f72ad7d7c18650056d30755c44b9c129b707e5"
         img_path = self.fetch_asset(img_url, algorithm="sha256", asset_hash=img_hash)
+        img_path_rw = os.path.join(self.workdir, os.path.basename(img_path))
+        shutil.copy(img_path, img_path_rw)
 
         self.vm.set_console()
         self.vm.add_args(
             "-cpu",
             cpu,
             "-drive",
-            f"file={img_path},format=raw",
+            f"file={img_path_rw},format=raw",
             "-device",
             "virtio-rng-pci,rng=rng0",
             "-object",
diff --git a/tests/avocado/machine_aarch64_virt.py b/tests/avocado/machine_aarch64_virt.py
index a90dc6ff4b..093d68f837 100644
--- a/tests/avocado/machine_aarch64_virt.py
+++ b/tests/avocado/machine_aarch64_virt.py
@@ -37,13 +37,13 @@ def test_alpine_virt_tcg_gic_max(self):
         :avocado: tags=machine:virt
         :avocado: tags=accel:tcg
         """
-        iso_url = ('https://dl-cdn.alpinelinux.org/'
-                   'alpine/v3.17/releases/aarch64/'
-                   'alpine-standard-3.17.2-aarch64.iso')
+        iso_url = (
+            "https://dl-cdn.alpinelinux.org/"
+            "alpine/v3.17/releases/aarch64/alpine-standard-3.17.2-aarch64.iso"
+        )
 
-        # Alpine use sha256 so I recalculated this myself
-        iso_sha1 = '76284fcd7b41fe899b0c2375ceb8470803eea839'
-        iso_path = self.fetch_asset(iso_url, asset_hash=iso_sha1)
+        iso_hash = "5a36304ecf039292082d92b48152a9ec21009d3a62f459de623e19c4bd9dc027"
+        iso_path = self.fetch_asset(iso_url, algorithm="sha256", asset_hash=iso_hash)
 
         self.vm.set_console()
         kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
@@ -60,7 +60,7 @@ def test_alpine_virt_tcg_gic_max(self):
         self.vm.add_args("-smp", "2", "-m", "1024")
         self.vm.add_args('-bios', os.path.join(BUILD_DIR, 'pc-bios',
                                                'edk2-aarch64-code.fd'))
-        self.vm.add_args("-drive", f"file={iso_path},format=raw")
+        self.vm.add_args("-drive", f"file={iso_path},readonly=on,format=raw")
         self.vm.add_args('-device', 'virtio-rng-pci,rng=rng0')
         self.vm.add_args('-object', 'rng-random,id=rng0,filename=/dev/urandom')
 
-- 
2.43.0

Re: [PATCH 04/10] tests/avocado: machine aarch64: standardize location and RO/RW access

[Marcin Juszkiewicz]

W dniu 8.12.2023 o 20:09, Cleber Rosa pisze:
> The tests under machine_aarch64_virt.py do not need read-write access
> to the ISOs.  The ones under machine_aarch64_sbsaref.py, on the other
> hand, will need read-write access, so let's give each test an unique
> file.
> 
> And while at it, let's use a single code style and hash for the ISO
> url.
> 
> Signed-off-by: Cleber Rosa<crosa@redhat.com>

It is ISO file, so sbsa-ref tests should be fine with readonly as well.

Nothing gets installed so nothing is written. We only test does boot works.

Re: [PATCH 04/10] tests/avocado: machine aarch64: standardize location and RO/RW access

[Cleber Rosa]

Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org> writes:

> W dniu 8.12.2023 o 20:09, Cleber Rosa pisze:
>> The tests under machine_aarch64_virt.py do not need read-write access
>> to the ISOs.  The ones under machine_aarch64_sbsaref.py, on the other
>> hand, will need read-write access, so let's give each test an unique
>> file.
>> 
>> And while at it, let's use a single code style and hash for the ISO
>> url.
>> 
>> Signed-off-by: Cleber Rosa<crosa@redhat.com>
>
> It is ISO file, so sbsa-ref tests should be fine with readonly as well.
>
> Nothing gets installed so nothing is written. We only test does boot works.

That was my original expectation too.  But, with nothing but the
following change:

diff --git a/tests/avocado/machine_aarch64_sbsaref.py b/tests/avocado/machine_aarch64_sbsaref.py
index 528c7d2934..436da4b156 100644
--- a/tests/avocado/machine_aarch64_sbsaref.py
+++ b/tests/avocado/machine_aarch64_sbsaref.py
@@ -129,7 +129,7 @@ def boot_alpine_linux(self, cpu):
             "-cpu",
             cpu,
             "-drive",
-            f"file={iso_path},format=raw",
+            f"file={iso_path},readonly=on,format=raw",
             "-device",
             "virtio-rng-pci,rng=rng0",
             "-object",

We get:

15:55:10 DEBUG| VM launch command: './qemu-system-aarch64 -display none -vga none -chardev socket,id=mon,fd=15 -mon chardev=mon,mode=control -machine sbsa-ref -
chardev socket,id=console,fd=20 -serial chardev:console -cpu cortex-a57 -drive if=pflash,file=/home/cleber/avocado/job-results/job-2023-12-13T15.55-28ef2b5/test
-results/tmp_dirx8p5xzt4/1-tests_avocado_machine_aarch64_sbsaref.py_Aarch64SbsarefMachine.test_sbsaref_alpine_linux_cortex_a57/SBSA_FLASH0.fd,format=raw -drive 
if=pflash,file=/home/cleber/avocado/job-results/job-2023-12-13T15.55-28ef2b5/test-results/tmp_dirx8p5xzt4/1-tests_avocado_machine_aarch64_sbsaref.py_Aarch64Sbsa
refMachine.test_sbsaref_alpine_linux_cortex_a57/SBSA_FLASH1.fd,format=raw -smp 1 -machine sbsa-ref -cpu cortex-a57 -drive file=/home/cleber/avocado/data/cache/b
y_location/0154b7cd3a4f5e135299060c8cabbeec10b70b6d/alpine-standard-3.17.2-aarch64.iso,readonly=on,format=raw -device virtio-rng-pci,rng=rng0 -object rng-random
,id=rng0,filename=/dev/urandom'

Followed by:

15:55:10 DEBUG| Failed to establish session:
  | Traceback (most recent call last):
  |   File "/home/cleber/src/qemu/python/qemu/qmp/protocol.py", line 425, in _session_guard
  |     await coro
  |   File "/home/cleber/src/qemu/python/qemu/qmp/qmp_client.py", line 253, in _establish_session
  |     await self._negotiate()
  |   File "/home/cleber/src/qemu/python/qemu/qmp/qmp_client.py", line 305, in _negotiate
  |     reply = await self._recv()
  |             ^^^^^^^^^^^^^^^^^^
  |   File "/home/cleber/src/qemu/python/qemu/qmp/protocol.py", line 1009, in _recv
  |     message = await self._do_recv()
  |               ^^^^^^^^^^^^^^^^^^^^^
  |   File "/home/cleber/src/qemu/python/qemu/qmp/qmp_client.py", line 402, in _do_recv
  |     msg_bytes = await self._readline()
  |                 ^^^^^^^^^^^^^^^^^^^^^^
  |   File "/home/cleber/src/qemu/python/qemu/qmp/protocol.py", line 977, in _readline
  |     raise EOFError
  | EOFError

With qemu-system-arch producing on stdout:

   qemu-system-aarch64: Block node is read-only

Any ideas on the reason or cause?

Thanks,
- Cleber.

Re: [PATCH 04/10] tests/avocado: machine aarch64: standardize location and RO/RW access

[Alex Bennée]

Cleber Rosa <crosa@redhat.com> writes:

> Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org> writes:
>
>> W dniu 8.12.2023 o 20:09, Cleber Rosa pisze:
>>> The tests under machine_aarch64_virt.py do not need read-write access
>>> to the ISOs.  The ones under machine_aarch64_sbsaref.py, on the other
>>> hand, will need read-write access, so let's give each test an unique
>>> file.
>>> 
>>> And while at it, let's use a single code style and hash for the ISO
>>> url.
>>> 
>>> Signed-off-by: Cleber Rosa<crosa@redhat.com>
>>
>> It is ISO file, so sbsa-ref tests should be fine with readonly as well.
>>
>> Nothing gets installed so nothing is written. We only test does boot works.
>
> That was my original expectation too.  But, with nothing but the
> following change:
>
> diff --git a/tests/avocado/machine_aarch64_sbsaref.py b/tests/avocado/machine_aarch64_sbsaref.py
> index 528c7d2934..436da4b156 100644
> --- a/tests/avocado/machine_aarch64_sbsaref.py
> +++ b/tests/avocado/machine_aarch64_sbsaref.py
> @@ -129,7 +129,7 @@ def boot_alpine_linux(self, cpu):
>              "-cpu",
>              cpu,
>              "-drive",
> -            f"file={iso_path},format=raw",
> +            f"file={iso_path},readonly=on,format=raw",

               f"file={iso_path},readonly=on,media=cdrom,format=raw",

works (although possible the readonly is redundant in this case).

>              "-device",
>              "virtio-rng-pci,rng=rng0",
>              "-object",
>
> We get:
>
> 15:55:10 DEBUG| VM launch command: './qemu-system-aarch64 -display none -vga none -chardev socket,id=mon,fd=15 -mon chardev=mon,mode=control -machine sbsa-ref -
> chardev socket,id=console,fd=20 -serial chardev:console -cpu cortex-a57 -drive if=pflash,file=/home/cleber/avocado/job-results/job-2023-12-13T15.55-28ef2b5/test
> -results/tmp_dirx8p5xzt4/1-tests_avocado_machine_aarch64_sbsaref.py_Aarch64SbsarefMachine.test_sbsaref_alpine_linux_cortex_a57/SBSA_FLASH0.fd,format=raw -drive 
> if=pflash,file=/home/cleber/avocado/job-results/job-2023-12-13T15.55-28ef2b5/test-results/tmp_dirx8p5xzt4/1-tests_avocado_machine_aarch64_sbsaref.py_Aarch64Sbsa
> refMachine.test_sbsaref_alpine_linux_cortex_a57/SBSA_FLASH1.fd,format=raw -smp 1 -machine sbsa-ref -cpu cortex-a57 -drive file=/home/cleber/avocado/data/cache/b
> y_location/0154b7cd3a4f5e135299060c8cabbeec10b70b6d/alpine-standard-3.17.2-aarch64.iso,readonly=on,format=raw -device virtio-rng-pci,rng=rng0 -object rng-random
> ,id=rng0,filename=/dev/urandom'
>
> Followed by:
>
> 15:55:10 DEBUG| Failed to establish session:
>   | Traceback (most recent call last):
>   |   File "/home/cleber/src/qemu/python/qemu/qmp/protocol.py", line 425, in _session_guard
>   |     await coro
>   |   File "/home/cleber/src/qemu/python/qemu/qmp/qmp_client.py", line 253, in _establish_session
>   |     await self._negotiate()
>   |   File "/home/cleber/src/qemu/python/qemu/qmp/qmp_client.py", line 305, in _negotiate
>   |     reply = await self._recv()
>   |             ^^^^^^^^^^^^^^^^^^
>   |   File "/home/cleber/src/qemu/python/qemu/qmp/protocol.py", line 1009, in _recv
>   |     message = await self._do_recv()
>   |               ^^^^^^^^^^^^^^^^^^^^^
>   |   File "/home/cleber/src/qemu/python/qemu/qmp/qmp_client.py", line 402, in _do_recv
>   |     msg_bytes = await self._readline()
>   |                 ^^^^^^^^^^^^^^^^^^^^^^
>   |   File "/home/cleber/src/qemu/python/qemu/qmp/protocol.py", line 977, in _readline
>   |     raise EOFError
>   | EOFError
>
> With qemu-system-arch producing on stdout:
>
>    qemu-system-aarch64: Block node is read-only
>
> Any ideas on the reason or cause?
>
> Thanks,
> - Cleber.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 04/10] tests/avocado: machine aarch64: standardize location and RO/RW access

[Alex Bennée]

Cleber Rosa <crosa@redhat.com> writes:

> The tests under machine_aarch64_virt.py do not need read-write access
> to the ISOs.  The ones under machine_aarch64_sbsaref.py, on the other
> hand, will need read-write access, so let's give each test an unique
> file.

I think we are making two separate changes here so probably best split
the patch.

> And while at it, let's use a single code style and hash for the ISO
> url.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/avocado/machine_aarch64_sbsaref.py |  9 +++++++--
>  tests/avocado/machine_aarch64_virt.py    | 14 +++++++-------
>  2 files changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/tests/avocado/machine_aarch64_sbsaref.py b/tests/avocado/machine_aarch64_sbsaref.py
> index 528c7d2934..6ae84d77ac 100644
> --- a/tests/avocado/machine_aarch64_sbsaref.py
> +++ b/tests/avocado/machine_aarch64_sbsaref.py
> @@ -7,6 +7,7 @@
>  # SPDX-License-Identifier: GPL-2.0-or-later
>  
>  import os
> +import shutil
>  
>  from avocado import skipUnless
>  from avocado.utils import archive
> @@ -123,13 +124,15 @@ def boot_alpine_linux(self, cpu):
>  
>          iso_hash = "5a36304ecf039292082d92b48152a9ec21009d3a62f459de623e19c4bd9dc027"
>          iso_path = self.fetch_asset(iso_url, algorithm="sha256", asset_hash=iso_hash)
> +        iso_path_rw = os.path.join(self.workdir, os.path.basename(iso_path))
> +        shutil.copy(iso_path, iso_path_rw)
>  
>          self.vm.set_console()
>          self.vm.add_args(
>              "-cpu",
>              cpu,
>              "-drive",
> -            f"file={iso_path},format=raw",
> +            f"file={iso_path_rw},format=raw",

Instead of copying why not add ",snapshot=on" to preserve the original
image. We don't want to persist data between tests.

>              "-device",
>              "virtio-rng-pci,rng=rng0",
>              "-object",
> @@ -170,13 +173,15 @@ def boot_openbsd73(self, cpu):
>  
>          img_hash = "7fc2c75401d6f01fbfa25f4953f72ad7d7c18650056d30755c44b9c129b707e5"
>          img_path = self.fetch_asset(img_url, algorithm="sha256", asset_hash=img_hash)
> +        img_path_rw = os.path.join(self.workdir, os.path.basename(img_path))
> +        shutil.copy(img_path, img_path_rw)
>  
>          self.vm.set_console()
>          self.vm.add_args(
>              "-cpu",
>              cpu,
>              "-drive",
> -            f"file={img_path},format=raw",
> +            f"file={img_path_rw},format=raw",

ditto.


>              "-device",
>              "virtio-rng-pci,rng=rng0",
>              "-object",
> diff --git a/tests/avocado/machine_aarch64_virt.py b/tests/avocado/machine_aarch64_virt.py
> index a90dc6ff4b..093d68f837 100644
> --- a/tests/avocado/machine_aarch64_virt.py
> +++ b/tests/avocado/machine_aarch64_virt.py
> @@ -37,13 +37,13 @@ def test_alpine_virt_tcg_gic_max(self):
>          :avocado: tags=machine:virt
>          :avocado: tags=accel:tcg
>          """
> -        iso_url = ('https://dl-cdn.alpinelinux.org/'
> -                   'alpine/v3.17/releases/aarch64/'
> -                   'alpine-standard-3.17.2-aarch64.iso')
> +        iso_url = (
> +            "https://dl-cdn.alpinelinux.org/"
> +            "alpine/v3.17/releases/aarch64/alpine-standard-3.17.2-aarch64.iso"
> +        )
>  
> -        # Alpine use sha256 so I recalculated this myself
> -        iso_sha1 = '76284fcd7b41fe899b0c2375ceb8470803eea839'
> -        iso_path = self.fetch_asset(iso_url, asset_hash=iso_sha1)
> +        iso_hash = "5a36304ecf039292082d92b48152a9ec21009d3a62f459de623e19c4bd9dc027"
> +        iso_path = self.fetch_asset(iso_url, algorithm="sha256", asset_hash=iso_hash)
>  
>          self.vm.set_console()
>          kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
> @@ -60,7 +60,7 @@ def test_alpine_virt_tcg_gic_max(self):
>          self.vm.add_args("-smp", "2", "-m", "1024")
>          self.vm.add_args('-bios', os.path.join(BUILD_DIR, 'pc-bios',
>                                                 'edk2-aarch64-code.fd'))
> -        self.vm.add_args("-drive", f"file={iso_path},format=raw")
> +        self.vm.add_args("-drive",
>          f"file={iso_path},readonly=on,format=raw")

Perhaps we can set ",media=cdrom" here.

>          self.vm.add_args('-device', 'virtio-rng-pci,rng=rng0')
>          self.vm.add_args('-object', 'rng-random,id=rng0,filename=/dev/urandom')

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 04/10] tests/avocado: machine aarch64: standardize location and RO/RW access

[Cleber Rosa]

Alex Bennée <alex.bennee@linaro.org> writes:

> Cleber Rosa <crosa@redhat.com> writes:
>
>> The tests under machine_aarch64_virt.py do not need read-write access
>> to the ISOs.  The ones under machine_aarch64_sbsaref.py, on the other
>> hand, will need read-write access, so let's give each test an unique
>> file.
>
> I think we are making two separate changes here so probably best split
> the patch.
>

Sure, but, do you mean separating the "readonly=on" and the "writable
file" changes?  Or separating those two from the ISO url code style
change?

>> And while at it, let's use a single code style and hash for the ISO
>> url.
>>
>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>> ---
>>  tests/avocado/machine_aarch64_sbsaref.py |  9 +++++++--
>>  tests/avocado/machine_aarch64_virt.py    | 14 +++++++-------
>>  2 files changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/tests/avocado/machine_aarch64_sbsaref.py b/tests/avocado/machine_aarch64_sbsaref.py
>> index 528c7d2934..6ae84d77ac 100644
>> --- a/tests/avocado/machine_aarch64_sbsaref.py
>> +++ b/tests/avocado/machine_aarch64_sbsaref.py
>> @@ -7,6 +7,7 @@
>>  # SPDX-License-Identifier: GPL-2.0-or-later
>>  
>>  import os
>> +import shutil
>>  
>>  from avocado import skipUnless
>>  from avocado.utils import archive
>> @@ -123,13 +124,15 @@ def boot_alpine_linux(self, cpu):
>>  
>>          iso_hash = "5a36304ecf039292082d92b48152a9ec21009d3a62f459de623e19c4bd9dc027"
>>          iso_path = self.fetch_asset(iso_url, algorithm="sha256", asset_hash=iso_hash)
>> +        iso_path_rw = os.path.join(self.workdir, os.path.basename(iso_path))
>> +        shutil.copy(iso_path, iso_path_rw)
>>  
>>          self.vm.set_console()
>>          self.vm.add_args(
>>              "-cpu",
>>              cpu,
>>              "-drive",
>> -            f"file={iso_path},format=raw",
>> +            f"file={iso_path_rw},format=raw",
>
> Instead of copying why not add ",snapshot=on" to preserve the original
> image. We don't want to persist data between tests.
>
>>              "-device",
>>              "virtio-rng-pci,rng=rng0",
>>              "-object",
>> @@ -170,13 +173,15 @@ def boot_openbsd73(self, cpu):
>>  
>>          img_hash = "7fc2c75401d6f01fbfa25f4953f72ad7d7c18650056d30755c44b9c129b707e5"
>>          img_path = self.fetch_asset(img_url, algorithm="sha256", asset_hash=img_hash)
>> +        img_path_rw = os.path.join(self.workdir, os.path.basename(img_path))
>> +        shutil.copy(img_path, img_path_rw)
>>  
>>          self.vm.set_console()
>>          self.vm.add_args(
>>              "-cpu",
>>              cpu,
>>              "-drive",
>> -            f"file={img_path},format=raw",
>> +            f"file={img_path_rw},format=raw",
>
> ditto.
>
>
>>              "-device",
>>              "virtio-rng-pci,rng=rng0",
>>              "-object",
>> diff --git a/tests/avocado/machine_aarch64_virt.py b/tests/avocado/machine_aarch64_virt.py
>> index a90dc6ff4b..093d68f837 100644
>> --- a/tests/avocado/machine_aarch64_virt.py
>> +++ b/tests/avocado/machine_aarch64_virt.py
>> @@ -37,13 +37,13 @@ def test_alpine_virt_tcg_gic_max(self):
>>          :avocado: tags=machine:virt
>>          :avocado: tags=accel:tcg
>>          """
>> -        iso_url = ('https://dl-cdn.alpinelinux.org/'
>> -                   'alpine/v3.17/releases/aarch64/'
>> -                   'alpine-standard-3.17.2-aarch64.iso')
>> +        iso_url = (
>> +            "https://dl-cdn.alpinelinux.org/"
>> +            "alpine/v3.17/releases/aarch64/alpine-standard-3.17.2-aarch64.iso"
>> +        )
>>  
>> -        # Alpine use sha256 so I recalculated this myself
>> -        iso_sha1 = '76284fcd7b41fe899b0c2375ceb8470803eea839'
>> -        iso_path = self.fetch_asset(iso_url, asset_hash=iso_sha1)
>> +        iso_hash = "5a36304ecf039292082d92b48152a9ec21009d3a62f459de623e19c4bd9dc027"
>> +        iso_path = self.fetch_asset(iso_url, algorithm="sha256", asset_hash=iso_hash)
>>  
>>          self.vm.set_console()
>>          kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
>> @@ -60,7 +60,7 @@ def test_alpine_virt_tcg_gic_max(self):
>>          self.vm.add_args("-smp", "2", "-m", "1024")
>>          self.vm.add_args('-bios', os.path.join(BUILD_DIR, 'pc-bios',
>>                                                 'edk2-aarch64-code.fd'))
>> -        self.vm.add_args("-drive", f"file={iso_path},format=raw")
>> +        self.vm.add_args("-drive",
>>          f"file={iso_path},readonly=on,format=raw")
>
> Perhaps we can set ",media=cdrom" here.
>

Yes, but more importantly, adding both "readonly=on" and "media=cdrom"
to the tests under machine_aarch64_sbsaref.py do the trick.  Now, the
behavior explained in my previous response still warrants investigation
IMO.

Thanks
- Cleber.

Re: [PATCH 04/10] tests/avocado: machine aarch64: standardize location and RO/RW access

[Alex Bennée]

Cleber Rosa <crosa@redhat.com> writes:

> Alex Bennée <alex.bennee@linaro.org> writes:
>
>> Cleber Rosa <crosa@redhat.com> writes:
>>
>>> The tests under machine_aarch64_virt.py do not need read-write access
>>> to the ISOs.  The ones under machine_aarch64_sbsaref.py, on the other
>>> hand, will need read-write access, so let's give each test an unique
>>> file.
>>
>> I think we are making two separate changes here so probably best split
>> the patch.
>>
>
> Sure, but, do you mean separating the "readonly=on" and the "writable
> file" changes?  Or separating those two from the ISO url code style
> change?

I was thinking about splitting the sbsaref and virt patches, but
actually they are fairly related as they all use the alpine image so
maybe no need.

>
>>> And while at it, let's use a single code style and hash for the ISO
>>> url.
>>>
>>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>>> ---
>>>  tests/avocado/machine_aarch64_sbsaref.py |  9 +++++++--
>>>  tests/avocado/machine_aarch64_virt.py    | 14 +++++++-------
>>>  2 files changed, 14 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/tests/avocado/machine_aarch64_sbsaref.py b/tests/avocado/machine_aarch64_sbsaref.py
>>> index 528c7d2934..6ae84d77ac 100644
>>> --- a/tests/avocado/machine_aarch64_sbsaref.py
>>> +++ b/tests/avocado/machine_aarch64_sbsaref.py
>>> @@ -7,6 +7,7 @@
>>>  # SPDX-License-Identifier: GPL-2.0-or-later
>>>  
>>>  import os
>>> +import shutil
>>>  
>>>  from avocado import skipUnless
>>>  from avocado.utils import archive
>>> @@ -123,13 +124,15 @@ def boot_alpine_linux(self, cpu):
>>>  
>>>          iso_hash = "5a36304ecf039292082d92b48152a9ec21009d3a62f459de623e19c4bd9dc027"
>>>          iso_path = self.fetch_asset(iso_url, algorithm="sha256", asset_hash=iso_hash)
>>> +        iso_path_rw = os.path.join(self.workdir, os.path.basename(iso_path))
>>> +        shutil.copy(iso_path, iso_path_rw)
>>>  
>>>          self.vm.set_console()
>>>          self.vm.add_args(
>>>              "-cpu",
>>>              cpu,
>>>              "-drive",
>>> -            f"file={iso_path},format=raw",
>>> +            f"file={iso_path_rw},format=raw",
>>
>> Instead of copying why not add ",snapshot=on" to preserve the original
>> image. We don't want to persist data between tests.

Ahh yes these are isos so snapshot isn't needed.

>>
>>>              "-device",
>>>              "virtio-rng-pci,rng=rng0",
>>>              "-object",
>>> @@ -170,13 +173,15 @@ def boot_openbsd73(self, cpu):
>>>  
>>>          img_hash = "7fc2c75401d6f01fbfa25f4953f72ad7d7c18650056d30755c44b9c129b707e5"
>>>          img_path = self.fetch_asset(img_url, algorithm="sha256", asset_hash=img_hash)
>>> +        img_path_rw = os.path.join(self.workdir, os.path.basename(img_path))
>>> +        shutil.copy(img_path, img_path_rw)
>>>  
>>>          self.vm.set_console()
>>>          self.vm.add_args(
>>>              "-cpu",
>>>              cpu,
>>>              "-drive",
>>> -            f"file={img_path},format=raw",
>>> +            f"file={img_path_rw},format=raw",
>>
>> ditto.
>>
>>
>>>              "-device",
>>>              "virtio-rng-pci,rng=rng0",
>>>              "-object",
>>> diff --git a/tests/avocado/machine_aarch64_virt.py b/tests/avocado/machine_aarch64_virt.py
>>> index a90dc6ff4b..093d68f837 100644
>>> --- a/tests/avocado/machine_aarch64_virt.py
>>> +++ b/tests/avocado/machine_aarch64_virt.py
>>> @@ -37,13 +37,13 @@ def test_alpine_virt_tcg_gic_max(self):
>>>          :avocado: tags=machine:virt
>>>          :avocado: tags=accel:tcg
>>>          """
>>> -        iso_url = ('https://dl-cdn.alpinelinux.org/'
>>> -                   'alpine/v3.17/releases/aarch64/'
>>> -                   'alpine-standard-3.17.2-aarch64.iso')
>>> +        iso_url = (
>>> +            "https://dl-cdn.alpinelinux.org/"
>>> +            "alpine/v3.17/releases/aarch64/alpine-standard-3.17.2-aarch64.iso"
>>> +        )
>>>  
>>> -        # Alpine use sha256 so I recalculated this myself
>>> -        iso_sha1 = '76284fcd7b41fe899b0c2375ceb8470803eea839'
>>> -        iso_path = self.fetch_asset(iso_url, asset_hash=iso_sha1)
>>> +        iso_hash = "5a36304ecf039292082d92b48152a9ec21009d3a62f459de623e19c4bd9dc027"
>>> +        iso_path = self.fetch_asset(iso_url, algorithm="sha256", asset_hash=iso_hash)
>>>  
>>>          self.vm.set_console()
>>>          kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
>>> @@ -60,7 +60,7 @@ def test_alpine_virt_tcg_gic_max(self):
>>>          self.vm.add_args("-smp", "2", "-m", "1024")
>>>          self.vm.add_args('-bios', os.path.join(BUILD_DIR, 'pc-bios',
>>>                                                 'edk2-aarch64-code.fd'))
>>> -        self.vm.add_args("-drive", f"file={iso_path},format=raw")
>>> +        self.vm.add_args("-drive",
>>>          f"file={iso_path},readonly=on,format=raw")
>>
>> Perhaps we can set ",media=cdrom" here.
>>
>
> Yes, but more importantly, adding both "readonly=on" and "media=cdrom"
> to the tests under machine_aarch64_sbsaref.py do the trick.  Now, the
> behavior explained in my previous response still warrants investigation
> IMO.
>
> Thanks
> - Cleber.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 05/10] tests/avocado: use more distinct names for assets

[Cleber Rosa]

Avocado's asset system will deposit files in a cache organized either
by their original location (the URI) or by their names.  Because the
cache (and the "by_name" sub directory) is common across tests, it's a
good idea to make these names as distinct as possible.

This avoid name clashes, which makes future Avocado runs to attempt to
redownload the assets with the same name, but from the different
locations they actually are from.  This causes cache misses, extra
downloads, and possibly canceled tests.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/avocado/kvm_xen_guest.py  | 3 ++-
 tests/avocado/netdev-ethtool.py | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/avocado/kvm_xen_guest.py b/tests/avocado/kvm_xen_guest.py
index 5391283113..ec4052a1fe 100644
--- a/tests/avocado/kvm_xen_guest.py
+++ b/tests/avocado/kvm_xen_guest.py
@@ -40,7 +40,8 @@ def get_asset(self, name, sha1):
         url = base_url + name
         # use explicit name rather than failing to neatly parse the
         # URL into a unique one
-        return self.fetch_asset(name=name, locations=(url), asset_hash=sha1)
+        return self.fetch_asset(name=f"qemu-kvm-xen-guest-{name}",
+                                locations=(url), asset_hash=sha1)
 
     def common_vm_setup(self):
         # We also catch lack of KVM_XEN support if we fail to launch
diff --git a/tests/avocado/netdev-ethtool.py b/tests/avocado/netdev-ethtool.py
index 5f33288f81..462cf8de7d 100644
--- a/tests/avocado/netdev-ethtool.py
+++ b/tests/avocado/netdev-ethtool.py
@@ -27,7 +27,8 @@ def get_asset(self, name, sha1):
         url = base_url + name
         # use explicit name rather than failing to neatly parse the
         # URL into a unique one
-        return self.fetch_asset(name=name, locations=(url), asset_hash=sha1)
+        return self.fetch_asset(name=f"qemu-netdev-ethtool-{name}",
+                                locations=(url), asset_hash=sha1)
 
     def common_test_code(self, netdev, extra_args=None):
 
-- 
2.43.0

Re: [PATCH 05/10] tests/avocado: use more distinct names for assets

[Philippe Mathieu-Daudé]

On 8/12/23 20:09, Cleber Rosa wrote:
> Avocado's asset system will deposit files in a cache organized either
> by their original location (the URI) or by their names.  Because the
> cache (and the "by_name" sub directory) is common across tests, it's a
> good idea to make these names as distinct as possible.
> 
> This avoid name clashes, which makes future Avocado runs to attempt to
> redownload the assets with the same name, but from the different
> locations they actually are from.  This causes cache misses, extra
> downloads, and possibly canceled tests.

Could it be clever to use the content hash for asset location?

> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/avocado/kvm_xen_guest.py  | 3 ++-
>   tests/avocado/netdev-ethtool.py | 3 ++-
>   2 files changed, 4 insertions(+), 2 deletions(-)

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

[Cleber Rosa]

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 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):
-- 
2.43.0

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

[Philippe Mathieu-Daudé]

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(-)

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

[Cleber Rosa]

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.

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

[Philippe Mathieu-Daudé]

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.
> 

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

[Cleber Rosa]

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.

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

[Philippe Mathieu-Daudé]

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!

[PATCH 07/10] testa/avocado: test_arm_emcraft_sf2: handle RW requirements for asset

[Cleber Rosa]

The asset used in the mentioned test gets truncated before it's used
in the test.  This means that the file gets modified, and thus the
asset's expected hash doesn't match anymore.  This causes cache misses
and re-downloads every time the test is re-run.

Let's make a copy of the asset so that the one in the cache is
preserved and the cache sees a hit on re-runs.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/avocado/boot_linux_console.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/avocado/boot_linux_console.py b/tests/avocado/boot_linux_console.py
index f5c5d647a4..e2e928e703 100644
--- a/tests/avocado/boot_linux_console.py
+++ b/tests/avocado/boot_linux_console.py
@@ -414,14 +414,16 @@ def test_arm_emcraft_sf2(self):
                    'fe371d32e50ca682391e1e70ab98c2942aeffb01/spi.bin')
         spi_hash = '65523a1835949b6f4553be96dec1b6a38fb05501'
         spi_path = self.fetch_asset(spi_url, asset_hash=spi_hash)
+        spi_path_rw = os.path.join(self.workdir, os.path.basename(spi_path))
+        shutil.copy(spi_path, spi_path_rw)
 
-        file_truncate(spi_path, 16 << 20) # Spansion S25FL128SDPBHICO is 16 MiB
+        file_truncate(spi_path_rw, 16 << 20) # Spansion S25FL128SDPBHICO is 16 MiB
 
         self.vm.set_console()
         kernel_command_line = self.KERNEL_COMMON_COMMAND_LINE
         self.vm.add_args('-kernel', uboot_path,
                          '-append', kernel_command_line,
-                         '-drive', 'file=' + spi_path + ',if=mtd,format=raw',
+                         '-drive', 'file=' + spi_path_rw + ',if=mtd,format=raw',
                          '-no-reboot')
         self.vm.launch()
         self.wait_for_console_pattern('Enter \'help\' for a list')
-- 
2.43.0

Re: [PATCH 07/10] testa/avocado: test_arm_emcraft_sf2: handle RW requirements for asset

[Philippe Mathieu-Daudé]

On 8/12/23 20:09, Cleber Rosa wrote:
> The asset used in the mentioned test gets truncated before it's used
> in the test.  This means that the file gets modified, and thus the
> asset's expected hash doesn't match anymore.  This causes cache misses
> and re-downloads every time the test is re-run.
> 
> Let's make a copy of the asset so that the one in the cache is
> preserved and the cache sees a hit on re-runs.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/avocado/boot_linux_console.py | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/avocado/boot_linux_console.py b/tests/avocado/boot_linux_console.py
> index f5c5d647a4..e2e928e703 100644
> --- a/tests/avocado/boot_linux_console.py
> +++ b/tests/avocado/boot_linux_console.py
> @@ -414,14 +414,16 @@ def test_arm_emcraft_sf2(self):
>                      'fe371d32e50ca682391e1e70ab98c2942aeffb01/spi.bin')
>           spi_hash = '65523a1835949b6f4553be96dec1b6a38fb05501'
>           spi_path = self.fetch_asset(spi_url, asset_hash=spi_hash)
> +        spi_path_rw = os.path.join(self.workdir, os.path.basename(spi_path))
> +        shutil.copy(spi_path, spi_path_rw)

This is an implementation detail. By default fetch_asset() should return
a path to a read-only artifact. We should extend it to optionally return
a writable file path, with the possibility to provide a dir/path.

Re: [PATCH 07/10] testa/avocado: test_arm_emcraft_sf2: handle RW requirements for asset

[Akihiko Odaki]

On 2023/12/12 1:36, Philippe Mathieu-Daudé wrote:
> On 8/12/23 20:09, Cleber Rosa wrote:
>> The asset used in the mentioned test gets truncated before it's used
>> in the test.  This means that the file gets modified, and thus the
>> asset's expected hash doesn't match anymore.  This causes cache misses
>> and re-downloads every time the test is re-run.
>>
>> Let's make a copy of the asset so that the one in the cache is
>> preserved and the cache sees a hit on re-runs.
>>
>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>> ---
>>   tests/avocado/boot_linux_console.py | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/avocado/boot_linux_console.py 
>> b/tests/avocado/boot_linux_console.py
>> index f5c5d647a4..e2e928e703 100644
>> --- a/tests/avocado/boot_linux_console.py
>> +++ b/tests/avocado/boot_linux_console.py
>> @@ -414,14 +414,16 @@ def test_arm_emcraft_sf2(self):
>>                      'fe371d32e50ca682391e1e70ab98c2942aeffb01/spi.bin')
>>           spi_hash = '65523a1835949b6f4553be96dec1b6a38fb05501'
>>           spi_path = self.fetch_asset(spi_url, asset_hash=spi_hash)
>> +        spi_path_rw = os.path.join(self.workdir, 
>> os.path.basename(spi_path))
>> +        shutil.copy(spi_path, spi_path_rw)
> 
> This is an implementation detail. By default fetch_asset() should return
> a path to a read-only artifact. We should extend it to optionally return
> a writable file path, with the possibility to provide a dir/path.

Apparently it is not OK to abstract fetch_asset() away; it confuses some 
static analysis according to:
"[PATCH 02/10] tests/avocado: mips: add hint for fetchasset plugin"

That said, it is still nice to have a method that does os.path.join() 
and shutil.copy().

[PATCH 08/10] tests/avocado/boot_xen.py: merge base classes

[Cleber Rosa]

While it's a good practice to have reusable base classes, in this
specific case there's no other user of the BootXenBase class.

By unifying the class used in this test, we can improve readability
and have the opportunity to add some future improvements in a clearer
fashion.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/avocado/boot_xen.py | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tests/avocado/boot_xen.py b/tests/avocado/boot_xen.py
index fc2faeedb5..f80cbcb8fb 100644
--- a/tests/avocado/boot_xen.py
+++ b/tests/avocado/boot_xen.py
@@ -17,7 +17,7 @@
 from boot_linux_console import LinuxKernelTest
 
 
-class BootXenBase(LinuxKernelTest):
+class BootXen(LinuxKernelTest):
     """
     Boots a Xen hypervisor with a Linux DomU kernel.
     """
@@ -59,9 +59,6 @@ def launch_xen(self, xen_path):
         console_pattern = 'VFS: Cannot open root device'
         wait_for_console_pattern(self, console_pattern, "Panic on CPU 0:")
 
-
-class BootXen(BootXenBase):
-
     def test_arm64_xen_411_and_dom0(self):
         """
         :avocado: tags=arch:aarch64
-- 
2.43.0

[PATCH 09/10] tests/avocado/boot_xen.py: unify tags

[Cleber Rosa]

Because all tests share the same tags, it's possible to have all of
them at the class level.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/avocado/boot_xen.py | 26 +++++---------------------
 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/tests/avocado/boot_xen.py b/tests/avocado/boot_xen.py
index f80cbcb8fb..f4b63c1ef2 100644
--- a/tests/avocado/boot_xen.py
+++ b/tests/avocado/boot_xen.py
@@ -20,6 +20,11 @@
 class BootXen(LinuxKernelTest):
     """
     Boots a Xen hypervisor with a Linux DomU kernel.
+
+    :avocado: tags=arch:aarch64
+    :avocado: tags=accel:tcg
+    :avocado: tags=cpu:cortex-a57
+    :avocado: tags=machine:virt
     """
 
     timeout = 90
@@ -60,13 +65,6 @@ def launch_xen(self, xen_path):
         wait_for_console_pattern(self, console_pattern, "Panic on CPU 0:")
 
     def test_arm64_xen_411_and_dom0(self):
-        """
-        :avocado: tags=arch:aarch64
-        :avocado: tags=accel:tcg
-        :avocado: tags=cpu:cortex-a57
-        :avocado: tags=machine:virt
-        """
-
         # archive of file from https://deb.debian.org/debian/pool/main/x/xen/
         xen_url = ('https://fileserver.linaro.org/s/JSsewXGZ6mqxPr5/'
                    'download?path=%2F&files='
@@ -78,13 +76,6 @@ def test_arm64_xen_411_and_dom0(self):
         self.launch_xen(xen_path)
 
     def test_arm64_xen_414_and_dom0(self):
-        """
-        :avocado: tags=arch:aarch64
-        :avocado: tags=accel:tcg
-        :avocado: tags=cpu:cortex-a57
-        :avocado: tags=machine:virt
-        """
-
         # archive of file from https://deb.debian.org/debian/pool/main/x/xen/
         xen_url = ('https://fileserver.linaro.org/s/JSsewXGZ6mqxPr5/'
                    'download?path=%2F&files='
@@ -96,13 +87,6 @@ def test_arm64_xen_414_and_dom0(self):
         self.launch_xen(xen_path)
 
     def test_arm64_xen_415_and_dom0(self):
-        """
-        :avocado: tags=arch:aarch64
-        :avocado: tags=accel:tcg
-        :avocado: tags=cpu:cortex-a57
-        :avocado: tags=machine:virt
-        """
-
         xen_url = ('https://fileserver.linaro.org/'
                    's/JSsewXGZ6mqxPr5/download'
                    '?path=%2F&files=xen-upstream-4.15-unstable.deb')
-- 
2.43.0

Re: [PATCH 09/10] tests/avocado/boot_xen.py: unify tags

[Philippe Mathieu-Daudé]

On 8/12/23 20:09, Cleber Rosa wrote:
> Because all tests share the same tags, it's possible to have all of
> them at the class level.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/avocado/boot_xen.py | 26 +++++---------------------
>   1 file changed, 5 insertions(+), 21 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 10/10] tests/avocado/boot_xen.py: use class attribute

[Cleber Rosa]

Rather than defining a single use variable, let's just use the class
attribute directly.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/avocado/boot_xen.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/avocado/boot_xen.py b/tests/avocado/boot_xen.py
index f4b63c1ef2..f29bc58b9e 100644
--- a/tests/avocado/boot_xen.py
+++ b/tests/avocado/boot_xen.py
@@ -50,11 +50,10 @@ def launch_xen(self, xen_path):
 
         self.vm.set_console()
 
-        xen_command_line = self.XEN_COMMON_COMMAND_LINE
         self.vm.add_args('-machine', 'virtualization=on',
                          '-m', '768',
                          '-kernel', xen_path,
-                         '-append', xen_command_line,
+                         '-append', self.XEN_COMMON_COMMAND_LINE,
                          '-device',
                          'guest-loader,addr=0x47000000,kernel=%s,bootargs=console=hvc0'
                          % (kernel_path))
-- 
2.43.0

Re: [PATCH 10/10] tests/avocado/boot_xen.py: use class attribute

[Philippe Mathieu-Daudé]

On 8/12/23 20:09, Cleber Rosa wrote:
> Rather than defining a single use variable, let's just use the class
> attribute directly.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/avocado/boot_xen.py | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>