Re: [PATCH 06/13] tests/avocado: use more distinct names for assets

[Alex Bennée]

Cleber Rosa <crosa@redhat.com> writes:

> On Mon, Jul 29, 2024 at 6:49 AM Daniel P. Berrangé <berrange@redhat.com> 
> wrote:
>>
>> On Fri, Jul 26, 2024 at 09:44:31AM -0400, 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.
>> >
>> > 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 f8cb458d5d..318fadebc3 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)
>>
>> Why do we need to pass a name here at all ? I see the comment here
>> but it isn't very clear about what the problem is. It just feels
>> wrong to be creating ourselves uniqueness naming problems, when we
>> have a nicely unique URL, and that cached URL can be shared across
>> tests, where as the custom names added by this patch are forcing
>> no-caching of the same URL between tests.
>>
>
> Now with your comment, I do agree that this adds some unneeded
> maintenance burden indeed.  Also, this was part of my pre-avocado bump
> patches that would work around issues present in < 103.0.  But let me
> give the complete answer.
>
> Under 88.1 the "uniqueness" of the URL did not consider the query
> parameters in the URL.  So, under 88.1:
>
>    avocado.utils.asset.Asset(name='bzImage',
> locations=['https://fileserver.linaro.org/s/kE4nCFLdQcoBF9t/download?path=%2Fkvm-xen-guest&files=bzImage',
> ...)
>    avocado.utils.asset.Asset(name='bzImage',
> locations=['https://fileserver.linaro.org/s/kE4nCFLdQcoBF9t/download?path=%2Fnetdev-ethtool&files=bzImage',
> ...)

This is mostly a hack to avoid having to tell NextCloud to generate a
unique sharing URL for every file.

>
> Would save content to the same location:
> /tmp/cache_old/by_location/2a8ecd750eb952504ad96b89576207afe1be6a8f/download.
>
> This is no longer the case on 103.0 (actually since 92.0), the
> contents of those exact assets would be saved to
> '/by_location/415c998a0061347e5115da53d57ea92c908a2e7f/path=%2Fkvm-xen-guest&files=bzImage'
> and 
> /by_location/415c998a0061347e5115da53d57ea92c908a2e7f/path=%2Fnetdev-ethtool&files=bzImage'.
>
> I personally don't like having the files named, although uniquely,
> after the query parameters.  But, If this doesn't bother others more
> than the maintenance burden, and Avocado version bump is applied, this
> patch can be dropped.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro