[Libguestfs] [PATCH virt-v2v 1/2] v2v: -o rhv-upload: Split plugin functions from create/finalize
Richard W.M. Jones
rjones at redhat.com
Tue Aug 3 17:47:59 UTC 2021
On Tue, Aug 03, 2021 at 07:51:03PM +0300, Nir Soffer wrote:
> On Tue, Aug 3, 2021 at 3:10 PM Richard W.M. Jones <rjones at redhat.com> wrote:
> >
> > The existing plugin created a new disk in open() and tried to finalize
> > it in close(). A correct NBD client may make several connections
> > (eg. connecting just to query for multi-conn or other metadata). This
> > could result in multiple disks being created.
> >
> > Let's fix this by using Nir's suggestion to split out the create and
> > finalize functions. The nbdkit plugin becomes a much more
> > straightforward plugin - it no longer depends on the oVirt SDK at all
> > so could in theory be rewritten in another programming language. The
> > other functions have been moved into new scripts ("transfer",
> > "finalize", "cancel") which are run separately.
> >
> > Some subtler points about this commit:
> >
> > - We no longer use or need the @failing decorator. I have extended
> > the cleanup code so as well as deleting disks it also cancels the
> > transfers. Any failure detected by qemu-img convert will result in
> > virt-v2v exiting and cancelling the transfer, which is much cleaner
> > and more robust.
>
> Canceling the transfer on errors is fine but deleting a disk *used* by
> a transfer will fail since the transfer owns the disk. Once the transfer
> was created, there is no need to delete the disk, it will delete the disk
> on errors.
(Commented on below)
> > - We still require the HTTP pool. The reasons is a bit subtle: Since
> > we are using the parallel thread model, pread and pwrite (etc) can
> > be called at any time, even overlapping, so storing an HTTP handle
> > per NBD connection does not work as the handle gets reused (which
> > results in a fatal error).
>
> Do we need the parallel threading model now? If we create a python instance
> per connection, one http connection to image is best.
I did consider this, but I thought it would be better to keep the
parallel thread model and multiple the requests down to a fixed number
of HTTP connections (in the pool). In the eventual multi-conn case it
would look like:
+----------+ +---------+
-----| nbdkit p|--------| imageio |
-----| o|--------| |
-----| o| (8) | |
-----| l|--------| |
+----------+ +---------+
4 multi-conn HTTP
NBD connections
On the left you'd have up to 4 x 64 NBD requests coming in. In the
middle (assuming no HTTP pipelining is possible) the requests are
issued by multiple threads on to the 8 HTTP connections to imageio.
> > diff --git a/v2v/rhv-upload-deletedisks.py b/v2v/rhv-upload-cancel.py
> > similarity index 80%
> > rename from v2v/rhv-upload-deletedisks.py
> > rename to v2v/rhv-upload-cancel.py
> > index 8ed8cb88b..f08713b8c 100644
> > --- a/v2v/rhv-upload-deletedisks.py
> > +++ b/v2v/rhv-upload-cancel.py
> > @@ -1,6 +1,6 @@
> > # -*- python -*-
> > -# oVirt or RHV upload delete disks used by ‘virt-v2v -o rhv-upload’
> > -# Copyright (C) 2019 Red Hat Inc.
> > +# oVirt or RHV upload cancel used by ‘virt-v2v -o rhv-upload’
> > +# Copyright (C) 2019-2021 Red Hat Inc.
> > #
> > # This program is free software; you can redistribute it and/or modify
> > # it under the terms of the GNU General Public License as published by
> > @@ -57,13 +57,22 @@ connection = sdk.Connection(
> > )
>
> You should close the connection (helps engine manages its sessions).
> The best way is using:
>
> from contextlib import closing
>
> with closing(connection):
> code using the econnection here...
>
> I'm not sure if connection.close() does more than socket.close(), but it
> is better to be explicit.
OK I'll add this.
It's worth noting that in my local copy I'm now closing the HTTP
connections in the plugin on exit, which I think wasn't in the patch
posted. It does unfortunately require a small change to nbdkit to do
this (https://gitlab.com/nbdkit/nbdkit/-/commit/f2fe99e4b0f54467ab8028eaf2d039cf918b2961).
> > system_service = connection.system_service()
> > +image_transfers_service = system_service.image_transfers_service()
> > +
> > +# Try to cancel the transfers. This will probably delete the disk.
>
> This *will* delete the disk, not probably.
>
> > +for id in params['transfer_ids']:
> > + try:
> > + transfer_service = image_transfers_service.image_transfer_service(id)
> > + transfer_service.cancel()
> > + except sdk.NotFoundError:
> > + pass
>
> This can fail because of other reasons, and in this case we will fail
> on the first
> failure, instead of trying to cancel all transfers. Previously we always cancel
> all transfers on all errors.
>
> I think this should be:
>
> for id in params['transfer_ids']:
> try:
> cancel the transfer...
> except sdk.NotFoundError:
> log warning - we don't expect non existing transfer
> except Exception:
> log traceback with the transfer id, someone will have to do manual
> cleanup using this id.
OK I'll see if I can fix this.
> Note that cancel is async, so the transfer is still in the cleanup
> flow when we reach this line, so...
>
> > disks_service = system_service.disks_service()
> >
> > +# As a last resort, try to remove the disks.
> > for uuid in params['disk_uuids']:
> > - # Try to get and remove the disk, however do not fail
> > - # if it does not exist (maybe removed in the meanwhile).
> > try:
> > disk_service = disks_service.disk_service(uuid)
> > disk_service.remove()
> > - except sdk.NotFoundError:
> > + except (sdk.NotFoundError, sdk.Error):
> > pass
>
> this will likely fail since the transfer still holds a lock on the disk.
>
> We don't need to delete the disks, the transfers will delete their disk.
Right, but I believe there is a window between creating the disk and
adding it to the transfer, and therefore this is an attempt to have a
"backup plan" to delete the disk.
I did discover that the transfer deletes the disk, which was why I
changed the exception to ignore sdk.Error.
...
> > # Using version 2 supporting the buffer protocol for better performance.
> > API_VERSION = 2
> >
> > @@ -43,31 +34,47 @@ API_VERSION = 2
> > # client, this give best performance.
> > MAX_CONNECTIONS = 4
>
> This is not need now since we use mult_con. This can also casue
> a dealock since the underlying qemu-nbd per this transfer supports up
> to 8 clients (--shared 8). If we create 4 http connections per nbdkit
> connections, we will have 16 connections. 8 of these will be blocked
> in imageio on qemu-nbd, which can lead to deadlock or make some
> nbdkit connections ineffective.
>
> To simplify this huge refactoring, I would use MAX_CONNECITIONS = 1
> for now.
Since the pool is a global (there is only one plugin instance, but
open() is called 4 times), I believe this actually limits the pool to
4, unless max_readers/writers overrides it.
> > +def after_fork():
> > + global options, pool
>
> This works but hiding the globals here makes it hard to understand the code.
> The globalls should be initialized at the top of the module.
OK.
PS. The way "global" works in Python is dumb!
...
> > -def create_http_pool(url, host, options):
> > +# Connection pool.
> > +def create_http_pool(url, options):
> > pool = queue.Queue()
> >
> > count = min(options["max_readers"],
> > options["max_writers"],
> > MAX_CONNECTIONS)
>
> Note that this logic must move to virt-v2v now. If imageio does not support
> multiple writers, you should not use multi_con.
>
> Since we create the transfer with format="raw", we should always
> support 8 writers, but the correctness this must be enforce using
> max_writers option.
I don't understand this comment.
...
> > +# Parameters are passed in via a JSON doc from the OCaml code.
> > +# Because this Python code ships embedded inside virt-v2v there
> > +# is no formal API here.
> > +params = None
> > +
> > +if len(sys.argv) != 2:
> > + raise RuntimeError("incorrect number of parameters")
> > +
> > +# Parameters are passed in via a JSON document.
> > +with open(sys.argv[1], 'r') as fp:
> > + params = json.load(fp)
>
> This works, but why not read the json from stdin in the same way
> we write the output to stdout?
It's a bit complicated to have two pipes to an external process, and
the other programs pass a params file as a parameter, that's the only
reason.
> > +# Send the destination URL, transfer ID, and host flag back to OCaml code.
> > +results = {
> > + "transfer_id": transfer.id,
> > + "destination_url": destination_url,
> > + "is_host": host is not None,
>
> is_host is not clear, mayb "is_local"?
Yes, I agree. Or "is_ovirt_host" perhaps.
Thanks,
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
More information about the Libguestfs
mailing list