Re: [Libguestfs] [PATCH virt-v2v 1/2] v2v: -o rhv-upload: Split plugin functions from create/finalize

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(a)redhat.com&gt; 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/f2fe99e4b0f54467ab8028eaf2d039c...). > > 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