12:47 p.m.
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> 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