[libvirt] [PATCH 1/3] qemu: migration: Improve p2p error if we can't open conn
By actually showing the Open() error to the user --- src/qemu/qemu_migration.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 19b1236..9ac9be4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3472,7 +3472,8 @@ static int doPeer2PeerMigrate(virQEMUDriverPtr driver, qemuDomainObjExitRemote(vm); if (dconn == NULL) { virReportError(VIR_ERR_OPERATION_FAILED, - _("Failed to connect to remote libvirt URI %s"), dconnuri); + _("Failed to connect to remote libvirt URI %s: %s"), + dconnuri, virGetLastErrorMessage()); virObjectUnref(cfg); return -1; } -- 1.8.2.1
Since as the code indicates it doesn't work yet, so let's be explicit about it. --- src/qemu/qemu_migration.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9ac9be4..ffc86a4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1900,12 +1900,13 @@ char *qemuMigrationBegin(virQEMUDriverPtr driver, if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) { /* TODO support NBD for TUNNELLED migration */ - if (flags & VIR_MIGRATE_TUNNELLED) - VIR_DEBUG("NBD in tunnelled migration is currently not supported"); - else { - cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; - priv->nbdPort = 0; + if (flags & VIR_MIGRATE_TUNNELLED) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("NBD in tunnelled migration is currently not supported")); + goto cleanup; } + cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; + priv->nbdPort = 0; } if (!(mig = qemuMigrationEatCookie(driver, vm, NULL, 0, 0))) @@ -2200,16 +2201,11 @@ done: if (mig->nbd && flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) { - /* TODO support NBD for TUNNELLED migration */ - if (flags & VIR_MIGRATE_TUNNELLED) - VIR_DEBUG("NBD in tunnelled migration is currently not supported"); - else { - if (qemuMigrationStartNBDServer(driver, vm, listenAddr) < 0) { - /* error already reported */ - goto endjob; - } - cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; + if (qemuMigrationStartNBDServer(driver, vm, listenAddr) < 0) { + /* error already reported */ + goto endjob; } + cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; } if (qemuMigrationBakeCookie(mig, driver, vm, cookieout, -- 1.8.2.1
On 05/28/2013 02:44 PM, Cole Robinson wrote:
Since as the code indicates it doesn't work yet, so let's be explicit about it. --- src/qemu/qemu_migration.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-)
ACK. An explicit error is always nicer than a nebulous todo that plows on in unsupported state. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 28.05.2013 22:44, Cole Robinson wrote:
Since as the code indicates it doesn't work yet, so let's be explicit about it. --- src/qemu/qemu_migration.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9ac9be4..ffc86a4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1900,12 +1900,13 @@ char *qemuMigrationBegin(virQEMUDriverPtr driver, if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) { /* TODO support NBD for TUNNELLED migration */ - if (flags & VIR_MIGRATE_TUNNELLED) - VIR_DEBUG("NBD in tunnelled migration is currently not supported"); - else { - cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; - priv->nbdPort = 0; + if (flags & VIR_MIGRATE_TUNNELLED) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("NBD in tunnelled migration is currently not supported")); + goto cleanup; } + cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; + priv->nbdPort = 0; }
if (!(mig = qemuMigrationEatCookie(driver, vm, NULL, 0, 0))) @@ -2200,16 +2201,11 @@ done: if (mig->nbd && flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) { - /* TODO support NBD for TUNNELLED migration */ - if (flags & VIR_MIGRATE_TUNNELLED) - VIR_DEBUG("NBD in tunnelled migration is currently not supported"); - else { - if (qemuMigrationStartNBDServer(driver, vm, listenAddr) < 0) { - /* error already reported */ - goto endjob; - } - cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; + if (qemuMigrationStartNBDServer(driver, vm, listenAddr) < 0) { + /* error already reported */ + goto endjob; } + cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; }
if (qemuMigrationBakeCookie(mig, driver, vm, cookieout,
I know you've already pushed this but I don't think this is desired. There are currently two ways to copy the storage during migration: 1) new one using nbd server 2) old one, using an 'copy_storage' attribute to 'migrate' command The first one is preferred and turned on whenever libvirt detects both sides support it. There's no way for a user to enforce one or another way of copying the storage. So if the old way works even for tunnelled migration, we should fall back to using the old way rather then erroring out. The debug message was really just a debug message :) Michal
On 05/29/2013 03:16 PM, Michal Privoznik wrote:
On 28.05.2013 22:44, Cole Robinson wrote:
Since as the code indicates it doesn't work yet, so let's be explicit about it. --- src/qemu/qemu_migration.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9ac9be4..ffc86a4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1900,12 +1900,13 @@ char *qemuMigrationBegin(virQEMUDriverPtr driver, if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) { /* TODO support NBD for TUNNELLED migration */ - if (flags & VIR_MIGRATE_TUNNELLED) - VIR_DEBUG("NBD in tunnelled migration is currently not supported"); - else { - cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; - priv->nbdPort = 0; + if (flags & VIR_MIGRATE_TUNNELLED) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("NBD in tunnelled migration is currently not supported")); + goto cleanup; } + cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; + priv->nbdPort = 0; }
if (!(mig = qemuMigrationEatCookie(driver, vm, NULL, 0, 0))) @@ -2200,16 +2201,11 @@ done: if (mig->nbd && flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) { - /* TODO support NBD for TUNNELLED migration */ - if (flags & VIR_MIGRATE_TUNNELLED) - VIR_DEBUG("NBD in tunnelled migration is currently not supported"); - else { - if (qemuMigrationStartNBDServer(driver, vm, listenAddr) < 0) { - /* error already reported */ - goto endjob; - } - cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; + if (qemuMigrationStartNBDServer(driver, vm, listenAddr) < 0) { + /* error already reported */ + goto endjob; } + cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; }
if (qemuMigrationBakeCookie(mig, driver, vm, cookieout,
I know you've already pushed this but I don't think this is desired. There are currently two ways to copy the storage during migration:
1) new one using nbd server 2) old one, using an 'copy_storage' attribute to 'migrate' command
The first one is preferred and turned on whenever libvirt detects both sides support it. There's no way for a user to enforce one or another way of copying the storage. So if the old way works even for tunnelled migration, we should fall back to using the old way rather then erroring out. The debug message was really just a debug message :)
Indeed I didn't consider that, sorry. However given that the old storage migration support is 1) considered not very useful and 2) more or less deprecated in upstream qemu, I still think my patch is an improvement. The fact that a common combination of cli options falls back to a kinda-working-but-not-really-supported version with virtually no indication to the user is pretty confusing. More motivation to actually implement it for tunnelled migration :) But I won't put up a fight, so if you'd prefer a revert I'll ACK that. - Cole
On 29.05.2013 21:37, Cole Robinson wrote:
On 05/29/2013 03:16 PM, Michal Privoznik wrote:
On 28.05.2013 22:44, Cole Robinson wrote:
Since as the code indicates it doesn't work yet, so let's be explicit about it. --- src/qemu/qemu_migration.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9ac9be4..ffc86a4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1900,12 +1900,13 @@ char *qemuMigrationBegin(virQEMUDriverPtr driver, if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) { /* TODO support NBD for TUNNELLED migration */ - if (flags & VIR_MIGRATE_TUNNELLED) - VIR_DEBUG("NBD in tunnelled migration is currently not supported"); - else { - cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; - priv->nbdPort = 0; + if (flags & VIR_MIGRATE_TUNNELLED) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("NBD in tunnelled migration is currently not supported")); + goto cleanup; } + cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; + priv->nbdPort = 0; }
if (!(mig = qemuMigrationEatCookie(driver, vm, NULL, 0, 0))) @@ -2200,16 +2201,11 @@ done: if (mig->nbd && flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) { - /* TODO support NBD for TUNNELLED migration */ - if (flags & VIR_MIGRATE_TUNNELLED) - VIR_DEBUG("NBD in tunnelled migration is currently not supported"); - else { - if (qemuMigrationStartNBDServer(driver, vm, listenAddr) < 0) { - /* error already reported */ - goto endjob; - } - cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; + if (qemuMigrationStartNBDServer(driver, vm, listenAddr) < 0) { + /* error already reported */ + goto endjob; } + cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; }
if (qemuMigrationBakeCookie(mig, driver, vm, cookieout,
I know you've already pushed this but I don't think this is desired. There are currently two ways to copy the storage during migration:
1) new one using nbd server 2) old one, using an 'copy_storage' attribute to 'migrate' command
The first one is preferred and turned on whenever libvirt detects both sides support it. There's no way for a user to enforce one or another way of copying the storage. So if the old way works even for tunnelled migration, we should fall back to using the old way rather then erroring out. The debug message was really just a debug message :)
Indeed I didn't consider that, sorry.
However given that the old storage migration support is 1) considered not very useful and 2) more or less deprecated in upstream qemu, I still think my patch is an improvement. The fact that a common combination of cli options falls back to a kinda-working-but-not-really-supported version with virtually no indication to the user is pretty confusing. More motivation to actually implement it for tunnelled migration :)
But I won't put up a fight, so if you'd prefer a revert I'll ACK that.
- Cole
Mmm. Okay, I think we both have point there. So I can live with the code as is. After all, it will enforce me to finish the NBD storage migration for tunelled migration :) Michal
Because it's a valid combination. p2p still uses a separate channel for qemu migration, so there's value in letting the user specify a manual migrate URI for overriding auto-port, or libvirt's FQDN lookup. What _isn't_ allowed is --migrateuri and TUNNELLED, since there is no separate migration channel. Disallow that instead --- tools/virsh-domain.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 4fdf4ba..eb8688d 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8306,15 +8306,15 @@ doMigrate(void *opaque) if ((flags & VIR_MIGRATE_PEER2PEER) || vshCommandOptBool(cmd, "direct")) { - /* For peer2peer migration or direct migration we only expect one URI - * a libvirt URI, or a hypervisor specific URI. */ - if (migrateuri != NULL) { + /* migrateuri doesn't make sense for tunnelled migration */ + if (flags & VIR_MIGRATE_TUNNELLED && migrateuri != NULL) { vshError(ctl, "%s", _("migrate: Unexpected migrateuri for peer2peer/direct migration")); goto out; } - if (virDomainMigrateToURI2(dom, desturi, NULL, xml, flags, dname, 0) == 0) + if (virDomainMigrateToURI2(dom, desturi, migrateuri, + xml, flags, dname, 0) == 0) ret = '0'; } else { /* For traditional live migration, connect to the destination host directly. */ -- 1.8.2.1
On 05/28/2013 02:44 PM, Cole Robinson wrote:
Because it's a valid combination. p2p still uses a separate channel for qemu migration, so there's value in letting the user specify a manual migrate URI for overriding auto-port, or libvirt's FQDN lookup.
What _isn't_ allowed is --migrateuri and TUNNELLED, since there is no separate migration channel. Disallow that instead --- tools/virsh-domain.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
ACK. This exposes more power of the underlying API (the fact that it only touches tools/virsh-domain.c is good).
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 4fdf4ba..eb8688d 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8306,15 +8306,15 @@ doMigrate(void *opaque)
if ((flags & VIR_MIGRATE_PEER2PEER) || vshCommandOptBool(cmd, "direct")) { - /* For peer2peer migration or direct migration we only expect one URI - * a libvirt URI, or a hypervisor specific URI. */
- if (migrateuri != NULL) { + /* migrateuri doesn't make sense for tunnelled migration */ + if (flags & VIR_MIGRATE_TUNNELLED && migrateuri != NULL) { vshError(ctl, "%s", _("migrate: Unexpected migrateuri for peer2peer/direct migration")); goto out; }
- if (virDomainMigrateToURI2(dom, desturi, NULL, xml, flags, dname, 0) == 0) + if (virDomainMigrateToURI2(dom, desturi, migrateuri, + xml, flags, dname, 0) == 0) ret = '0'; } else { /* For traditional live migration, connect to the destination host directly. */
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/28/2013 02:44 PM, Cole Robinson wrote:
By actually showing the Open() error to the user --- src/qemu/qemu_migration.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Does qemuDomainObjExitRemote(vm) risk clobbering the last error message? Since it potentially unrefs vm on the last reference, I'm wondering if we have to cache the last error at the right point in time instead of relying on current behavior. But obviously it worked in your testing as written.
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 19b1236..9ac9be4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3472,7 +3472,8 @@ static int doPeer2PeerMigrate(virQEMUDriverPtr driver, qemuDomainObjExitRemote(vm); if (dconn == NULL) { virReportError(VIR_ERR_OPERATION_FAILED, - _("Failed to connect to remote libvirt URI %s"), dconnuri); + _("Failed to connect to remote libvirt URI %s: %s"), + dconnuri, virGetLastErrorMessage()); virObjectUnref(cfg); return -1;
Another alternative is to just return -1 directly instead of doing a virReportError() that overwrites the original error, although I'm not sure if that loses some context that would help the user. Can you include a sample error message in your commit log that shows the improvement? If so, I'm inclined to ACK this. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/28/2013 05:04 PM, Eric Blake wrote:
On 05/28/2013 02:44 PM, Cole Robinson wrote:
By actually showing the Open() error to the user --- src/qemu/qemu_migration.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Does qemuDomainObjExitRemote(vm) risk clobbering the last error message? Since it potentially unrefs vm on the last reference, I'm wondering if we have to cache the last error at the right point in time instead of relying on current behavior. But obviously it worked in your testing as written.
Yeah seems to work, and doesn't look like anything in virobject.c touches the error APIs, so it think it's okay. Error objects are thread local, not tied to any VM reference.
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 19b1236..9ac9be4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3472,7 +3472,8 @@ static int doPeer2PeerMigrate(virQEMUDriverPtr driver, qemuDomainObjExitRemote(vm); if (dconn == NULL) { virReportError(VIR_ERR_OPERATION_FAILED, - _("Failed to connect to remote libvirt URI %s"), dconnuri); + _("Failed to connect to remote libvirt URI %s: %s"), + dconnuri, virGetLastErrorMessage()); virObjectUnref(cfg); return -1;
Another alternative is to just return -1 directly instead of doing a virReportError() that overwrites the original error, although I'm not sure if that loses some context that would help the user. Can you include a sample error message in your commit log that shows the improvement? If so, I'm inclined to ACK this.
I think the current way is preferred, with migration it could be ambiguous what connection the error is coming from. With the original code you see: error: operation failed: Failed to connect to remote libvirt URI qemu+ssh://root@192.168.123.142/system If we just raised the connection error you get: Cannot recv data: Host key verification failed.: Connection reset by peer With my fix you get: error: operation failed: Failed to connect to remote libvirt URI qemu+ssh://root@192.168.123.142/system: Cannot recv data: Host key verification failed.: Connection reset by peer Unfortunately I just pushed before adding this to the commit message, sorry. Thanks for the review. - Cole
participants (3)
-
Cole Robinson -
Eric Blake -
Michal Privoznik