On Tue, Aug 25, 2020 at 06:01:28PM +0200, Jiri Denemark wrote:
On Tue, Aug 25, 2020 at 16:28:12 +0100, Daniel P. Berrangé wrote:On Tue, Aug 25, 2020 at 07:47:12AM +0200, Martin Kletzander wrote: > Adds new typed param for migration and uses this as a UNIX socket path that > should be used for the NBD part of migration. And also adds virsh support. > > Partially resolves: https://bugzilla.redhat.com/1638889 > > Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx> > --- > docs/manpages/virsh.rst | 8 +++ > include/libvirt/libvirt-domain.h | 12 ++++ > src/qemu/qemu_domain.h | 1 + > src/qemu/qemu_driver.c | 33 ++++++++-- > src/qemu/qemu_migration.c | 110 ++++++++++++++++++++++--------- > src/qemu/qemu_migration.h | 3 + > src/qemu/qemu_migration_cookie.c | 22 +++++-- > src/qemu/qemu_migration_cookie.h | 1 + > tools/virsh-domain.c | 12 ++++ > 9 files changed, 160 insertions(+), 42 deletions(-) > > diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst > index 9187037a5615..75f475eea6ad 100644 > --- a/docs/manpages/virsh.rst > +++ b/docs/manpages/virsh.rst > @@ -3113,6 +3113,7 @@ migrate > [--postcopy-bandwidth bandwidth] > [--parallel [--parallel-connections connections]] > [--bandwidth bandwidth] [--tls-destination hostname] > + [--disks-socket socket-path] For the primary migration connection we have a proper URI, so we can support new schemes without adding new parameters for each one. For the NBD connetion we have the "disk port" parameter only, not a full URI. Adding "disks socket" makes this design mistake worse. IMHO we should add a "disks uri" accepting the exact same syntax as the migration URI.Oh yeah, thanks for mentioning this. I originally thought about generic disks-uri too, but forgot about it when getting back to the review after lunch :-/
I wanted to do it that way, but I did not want to be arguing for that. So here's what I thought others would use to argue against the URI: If we allow URI it can then be used for non-socket connections as well, but in that case we might need different IP address for source and destination, the same reason we have listenAddress for. I guess my counter-argument would be that we already have the listenAddress for that. That might have been countered by saying that someone might want to migrate the disk over different data path. At that point I might say to forward it yourself using a proxy. I have few more following arguments in my head. I'm very much fine with adding URI instead, but I am not going to be arguing for either.
Just beware the migration URI is basically passed through to QEMU, while we use nbd or nbd+unix URIs for disks when talking to QEMU and we should not use these in the API. Just tcp:// or unix:// and translate them to the correct QEMU nbd URI internally.
yeah, well, qemu should accept both nbd+unix and unix: I think. I'll see what I can do.
Jirka
Attachment:
signature.asc
Description: PGP signature
