Re: [Qemu-devel] Failing iotests in CI (was: Add a gitlab-ci file for Continuous Integration testing on Gitlab)

[Daniel P . Berrangé]

On Tue, Feb 19, 2019 at 01:16:57PM +0100, Kevin Wolf wrote:
> Am 19.02.2019 um 13:01 hat Daniel P. Berrangé geschrieben:
> > On Tue, Feb 19, 2019 at 12:31:41PM +0100, Kevin Wolf wrote:
> > > Am 19.02.2019 um 12:06 hat Daniel P. Berrangé geschrieben:
> > > > On Tue, Feb 19, 2019 at 10:37:16AM +0100, Kevin Wolf wrote:
> > > > > Am 19.02.2019 um 10:04 hat Thomas Huth geschrieben:
> > > > > > 
> > > > > >  https://gitlab.com/huth/qemu/-/jobs/163680780
> > > > > > 
> > > > > > Some of them apparently need encryption to be enabled (as already
> > > > > > mentioned by Cleber in his patch) - thus should they really be in 
> > > > > > the
> > > > > > quick check, too? Or could they at least check whether QEMU has been
> > > > > > built with encryption?
> > > > > 
> > > > > The correct solution would be that they detect the situation
> > > > > automatically and skip the test by calling _notrun.
> > > > > 
> > > > > I'm not sure how to detect if a given QEMU binary supports encryption,
> > > > > but Dan might know.
> > > > 
> > > > It isn't easy & depends which encryption feature you're trying to use.
> > > > 
> > > > For TLS related features you can do something gross like
> > > > 
> > > >     qemu-img info --object tls-creds-anon,id=dummy README 2>&1
> > > >     test $? != 0 && exit 0
> > > > 
> > > > This relies on fact that 'tls-creds-anon' object type will report a
> > > > runtime error during initialization if gnutls isn't enabled.
> > > > 
> > > > For more general ciphers you pretty much have to just try the higher 
> > > > level
> > > > feature and see if it fails.
> > > 
> > > Actually, I think for test cases we should see 'qemu-img create' failing
> > > and could just skip the test if it returns a non-zero exit code.
> > > 
> > > But then I looked at Thomas' output again:
> > > 
> > >     --- /builds/huth/qemu/tests/qemu-iotests/188.out      2019-02-19 
> > > 08:23:54.000000000 +0000
> > >     +++ /builds/huth/qemu/tests/qemu-iotests/188.out.bad  2019-02-19 
> > > 08:34:54.000000000 +0000
> > >     @@ -1,4 +1,5 @@
> > >      QA output created by 188
> > >     +qemu-img: TEST_DIR/t.IMGFMT: No crypto library supporting PBKDF in 
> > > this build: Function not implemented
> > >      Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216 
> > > encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
> > > 
> > >      == reading whole image ==--- 
> > > /builds/huth/qemu/tests/qemu-iotests/188.out    2019-02-19 
> > > 08:23:54.000000000 +0000
> > > 
> > > What is it actually doing there? There's clearly an error message, but
> > > it almost looks like it's creating some kind of image anyway? The
> > > following I/O works fine (i.e. this created image can even be opened
> > > again with the luks driver), except that you can also access the image
> > > with the wrong password.
> > > 
> > > Is this a real bug in either qcow2 or luks?
> > 
> > It is an artifact of the way qcow2 image creation happens in multiple
> > phases. qcow2_co_create first creates a minimal qcow2 file, and then
> > opens it and updates it to add in the various extra features, including
> > luks encryption. We fail to create the luks encryption, but enough of
> > the qcow2 file has been created that it is able to still do plain text
> > I/O.
> 
> But why doesn't qcow2_update_options_prepare() fail then? If you pass
> encryption options like 188 does, but the image isn't encrypted, then
> the function at least attempts to error out. Where is this failing?

Oh, its because we check for "encrypt.format", but we made that field
optional when opening the image, allowing other encrypt.* fields to be
set and just pulling format from the header. I'v sent a patch for this.

> > Essentially the problem is that qcow2_co_create() doesn't unlink() the
> > partially created image when things fail. This is a generic problem
> > which can affect any part of qcow2_co_create that might fail, but it
> > is especially problematic with luks.
> > 
> > The complication in fixing this is that can't just do an unlink() as
> > we can't assume a local file. We need to have a bdrv_unlink() driver
> > callback we can use to delegate to the right block driver APIs for
> > deletion.
> 
> .bdrv_co_create can't unlink at all, because that would be undoing
> something that it didn't even do itself. If I created a file (local or
> on a remote server that is accessed over a network protocol) and my
> blockdev-create command to create the qcow2 layer fails, I certainly
> don't expect the resource that I manually created to go away.

Oh, I forgot that we unconditionally call bdrv_create_file even if
we might have a pre-existing file, expecting it to be a no-op.

> What .bdrv_co_create could and probably should do is make the header
> invalid so that it's still recognised as qcow2 (to avoid probing it as
> raw), but opening it fails.

Yes, good idea. I've sent a patch for this too

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|