[libvirt] [PATCH] pass correct uri to source host when we do p2p migration
When we migrate a guest from remote host to localhost by p2p, the libvirtd on remote host will be deadlock. This patch fixes a bug and we can avoid the deadlock with this patch. The steps to reproduce this bug: # virsh -c qemu+ssh://<remotehost IP>/system migrate --p2p <domain name> qemu+ssh:///system We connect dest host(qemu+ssh:///system) on source host, and the uri we pass to source host is qemu+ssh:///system. And then we connect a wrong dest host. Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> --- src/libvirt.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 51 insertions(+), 2 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index ee2495a..81a1bf8 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3533,22 +3533,71 @@ virDomainMigratePeer2Peer (virDomainPtr domain, const char *uri, unsigned long bandwidth) { + xmlURIPtr tempxmluri = NULL; + char * tempuri = NULL; + int ret = -1; + char * hostname = NULL; + if (!domain->conn->driver->domainMigratePerform) { virLibConnError (domain->conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); virDispatchError(domain->conn); return -1; } + tempxmluri = xmlParseURI(uri); + if (!tempxmluri) { + virLibConnError (domain->conn, VIR_ERR_INVALID_ARG, __FUNCTION__); + virDispatchError(domain->conn); + goto out; + } + if (tempxmluri->server) { + tempuri = uri; + } else { + /* + * The uri will be passed to source host. If user inputs qemu:///system, + * the dest host is localhost. But we connect the dest host on source + * host(a remote host). So we should pass qemu://<dest hostname>/system + * to the source host. + */ + hostname = virGetHostname(NULL); + if (!hostname) { + virDispatchError(domain->conn); + goto out; + } + + if (STRPREFIX(hostname, "localhost")) { + virLibConnError(domain->conn, VIR_ERR_INTERNAL_ERROR, + _("hostname on destination resolved to localhost, but migration requires an FQDN")); + virDispatchError(domain->conn); + goto out; + } + + tempxmluri->server = hostname; + tempuri = strdup((char *)xmlSaveUri(tempxmluri)); + if (!tempuri) { + virReportOOMError(); + virDispatchError(domain->conn); + goto out; + } + } + /* Perform the migration. The driver isn't supposed to return * until the migration is complete. */ - return domain->conn->driver->domainMigratePerform(domain, + ret = domain->conn->driver->domainMigratePerform(domain, NULL, /* cookie */ 0, /* cookielen */ - uri, + tempuri, flags, dname, bandwidth); + +out: + VIR_FREE(hostname); + if (tempuri && tempuri != uri) + VIR_FREE(tempuri); + + return ret; } -- 1.7.1
On Mon, Jan 10, 2011 at 11:20:31AM +0800, Wen Congyang wrote:
When we migrate a guest from remote host to localhost by p2p, the libvirtd on remote host will be deadlock. This patch fixes a bug and we can avoid the deadlock with this patch.
The steps to reproduce this bug: # virsh -c qemu+ssh://<remotehost IP>/system migrate --p2p <domain name> qemu+ssh:///system
We connect dest host(qemu+ssh:///system) on source host, and the uri we pass to source host is qemu+ssh:///system. And then we connect a wrong dest host.
IMHO, we should be reporting an error in this scenario, because the user is specifying a combination of parameters that don't make sense. Daniel
At 01/10/2011 07:02 PM, Daniel P. Berrange Write:
On Mon, Jan 10, 2011 at 11:20:31AM +0800, Wen Congyang wrote:
When we migrate a guest from remote host to localhost by p2p, the libvirtd on remote host will be deadlock. This patch fixes a bug and we can avoid the deadlock with this patch.
The steps to reproduce this bug: # virsh -c qemu+ssh://<remotehost IP>/system migrate --p2p <domain name> qemu+ssh:///system
We connect dest host(qemu+ssh:///system) on source host, and the uri we pass to source host is qemu+ssh:///system. And then we connect a wrong dest host.
IMHO, we should be reporting an error in this scenario, because the user is specifying a combination of parameters that don't make sense.
I don't think so. If we don't use --p2p, the migration can finish. If we reporting an error here, the behavior of this command will be strange(success without --p2p, but fail with --p2p).
Daniel
On Tue, Jan 11, 2011 at 09:37:46AM +0800, Wen Congyang wrote:
At 01/10/2011 07:02 PM, Daniel P. Berrange Write:
On Mon, Jan 10, 2011 at 11:20:31AM +0800, Wen Congyang wrote:
When we migrate a guest from remote host to localhost by p2p, the libvirtd on remote host will be deadlock. This patch fixes a bug and we can avoid the deadlock with this patch.
The steps to reproduce this bug: # virsh -c qemu+ssh://<remotehost IP>/system migrate --p2p <domain name> qemu+ssh:///system
We connect dest host(qemu+ssh:///system) on source host, and the uri we pass to source host is qemu+ssh:///system. And then we connect a wrong dest host.
IMHO, we should be reporting an error in this scenario, because the user is specifying a combination of parameters that don't make sense.
I don't think so. If we don't use --p2p, the migration can finish. If we reporting an error here, the behavior of this command will be strange(success without --p2p, but fail with --p2p).
If the user did a slight variation on your example, they'll hit exactly the same problem: virsh -c qemu+ssh://<remotehost IP>/system migrate --p2p <domain name> qemu+ssh://localhost/system In both cases the URI is erroneously pointing to the machine running virsh. The URI needs to be the public IP of the machine to migrate to, as known to the source machine. The fact that it works with traditional migration but fails with peer2peer migration is actually expected, because these have very different architectures & are not intended to behave equivalently for a given URI. The URI parameter for these two modes of migration has different semantics: * normal migration: the URI is an address of the target host as seen from the client machine. * peer2peer migration: the URI is an address of the target host as seen from the source machine. So in normal migration, your example requests migration between a <remote host> and the local machine running virsh. In the peer2peer migration your example requests a "localhost migration" on the <remote host>. While some hypervisors will support localhost migration, we don't support that with QEMU currently and so QEMU should refuse to attempt that (rather than deadlock). Putting in the code to libvirt.c which changes the URI to add in a full hostname of the virsh client, is in effect re-writing the semantics of the peer2peer migration URI argument, to apply the semantics of the normal migration URI argument. This is not what we want, because it will prevent use of "localhost migration" for hypervisors which do support this concept. Also, nothing here is actually fixing the deadlock, which could likely be triggered, just by giving 2 URIs explicitly pointing to the same machine which is again requesting a "localhost" migration: virsh -c qemu+ssh://<remotehost IP>/system migrate --p2p <domain name> qemu+ssh://<remotehost IP>/system Regards, Daniel
On 01/11/2011 04:00 AM, Daniel P. Berrange wrote:
* normal migration: the URI is an address of the target host as seen from the client machine.
* peer2peer migration: the URI is an address of the target host as seen from the source machine.
If nothing else, we need to improve the documentation of 'virsh migrate' to make this point more explicit, that using --p2p changes the interpretation of the --desturi argument. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
At 01/11/2011 07:00 PM, Daniel P. Berrange Write:
Also, nothing here is actually fixing the deadlock, which could likely be triggered, just by giving 2 URIs explicitly pointing to the same machine which is again requesting a "localhost" migration:
virsh -c qemu+ssh://<remotehost IP>/system migrate --p2p <domain name> qemu+ssh://<remotehost IP>/system
In this case, the deadlock does not happen, and we will receive the following error message: error: Requested operation is not valid: domain is already active as 'domain name' Thanks for such a detailed explanation about desturi.
Regards, Daniel
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Wen Congyang