Re: [PATCH 3/5] tests/vm: use 'cp' instead of 'ln' for temporary vm images

[John Snow]

On Tue, Jun 14, 2022 at 12:40 AM Thomas Huth <thuth@redhat.com> wrote:
>
> On 14/06/2022 03.50, John Snow wrote:
> > If the initial setup fails, you've permanently altered the state of the
> > downloaded image in an unknowable way. Use 'cp' like our other test
> > setup scripts do.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/vm/centos | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tests/vm/centos b/tests/vm/centos
> > index 5c7bc1c1a9a..be4f6ff2f14 100755
> > --- a/tests/vm/centos
> > +++ b/tests/vm/centos
> > @@ -34,7 +34,7 @@ class CentosVM(basevm.BaseVM):
> >       def build_image(self, img):
> >           cimg = 
> > self._download_with_cache("https://cloud.centos.org/centos/8/x86_64/images/CentOS-8-GenericCloud-8.3.2011-20201204.2.x86_64.qcow2";)
> >           img_tmp = img + ".tmp"
> > -        subprocess.check_call(["ln", "-f", cimg, img_tmp])
> > +        subprocess.check_call(['cp', '-f', cimg, img_tmp])
>
> I wonder whether it would make sense to use "qemu-img create -b" instead to
> save some disk space?
>
> Anyway, your patch is certainly already an improvement, so:
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

I wondered the same, but decided to keep a smaller series this time
around. VM tests already use a lot of space, so I doubt this is adding
new constraints that didn't exist before. A more rigorous overhaul may
be in order, but not right now. (It looks like the config file stuff
to override defaults is not necessarily rigorously respected by the
different installer recipes.)

I think the caching of the fully set-up image needs work, too. In
practice we leave the image sitting around, but we seem to always
rebuild it no matter what, so it's not that useful. There's a few
things that can be done here to drastically speed up some things,
but... later.

--js