[libvirt] [PATCH] Fix migration with QEMU 1.6
QEMU 1.6.0 introduced new migration status: setup Libvirt does not expect such string in QMP and refuses to migrate with error "unexpected migration status in setup" This patch fixes it. Sorry for previous bad try. Unfortunately I have seen Serge Hallyn's patch too late. Now I think it's better to include following code into my patch: if (setting_up) { priv->job.info.fileTotal = -1; priv->job.info.fileRemaining = -1; priv->job.info.fileProcessed = 0; priv->job.info.memTotal = -1; priv->job.info.memRemaining = -1; priv->job.info.memProcessed = 0; priv->job.info.dataTotal = -1; priv->job.info.dataRemaining = -1; priv->job.info.dataProcessed = 0; } But I'm not sure if my patch is good enough. If it is then I will include aforementioned code in it. --- src/qemu/qemu_migration.c | 4 ++++ src/qemu/qemu_monitor.c | 2 +- src/qemu/qemu_monitor.h | 1 + 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a3d986f..4b5fdba 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1644,6 +1644,10 @@ qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver, _("%s: %s"), job, _("is not active")); break; + case QEMU_MONITOR_MIGRATION_STATUS_SETUP: + ret = 0; + break; + case QEMU_MONITOR_MIGRATION_STATUS_ACTIVE: priv->job.info.fileTotal = priv->job.status.disk_total; priv->job.info.fileRemaining = priv->job.status.disk_remaining; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 30315b3..0d4598e 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -114,7 +114,7 @@ VIR_ONCE_GLOBAL_INIT(qemuMonitor) VIR_ENUM_IMPL(qemuMonitorMigrationStatus, QEMU_MONITOR_MIGRATION_STATUS_LAST, - "inactive", "active", "completed", "failed", "cancelled") + "inactive", "active", "completed", "failed", "cancelled", "setup") VIR_ENUM_IMPL(qemuMonitorMigrationCaps, QEMU_MONITOR_MIGRATION_CAPS_LAST, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index f893b1f..eabf000 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -397,6 +397,7 @@ enum { QEMU_MONITOR_MIGRATION_STATUS_COMPLETED, QEMU_MONITOR_MIGRATION_STATUS_ERROR, QEMU_MONITOR_MIGRATION_STATUS_CANCELLED, + QEMU_MONITOR_MIGRATION_STATUS_SETUP, QEMU_MONITOR_MIGRATION_STATUS_LAST }; -- 1.7.10.4
On Fri, Nov 15, 2013 at 20:47:43 +0900, Michael Avdienko wrote:
QEMU 1.6.0 introduced new migration status: setup Libvirt does not expect such string in QMP and refuses to migrate with error "unexpected migration status in setup"
This patch fixes it.
Sorry for previous bad try. Unfortunately I have seen Serge Hallyn's patch too late. Now I think it's better to include following code into my patch: if (setting_up) { priv->job.info.fileTotal = -1; priv->job.info.fileRemaining = -1; priv->job.info.fileProcessed = 0;
priv->job.info.memTotal = -1; priv->job.info.memRemaining = -1; priv->job.info.memProcessed = 0;
priv->job.info.dataTotal = -1; priv->job.info.dataRemaining = -1; priv->job.info.dataProcessed = 0; } But I'm not sure if my patch is good enough. If it is then I will include aforementioned code in it.
I don't think this is needed at all, I'd just keep them all 0. This patch gets my ACK as is but I won't it push yet in case someone provides a good reasoning for initializing the counters to 2^64-1. Jirka
On Fri, Nov 15, 2013 at 01:18:08PM +0100, Jiri Denemark wrote:
On Fri, Nov 15, 2013 at 20:47:43 +0900, Michael Avdienko wrote:
QEMU 1.6.0 introduced new migration status: setup Libvirt does not expect such string in QMP and refuses to migrate with error "unexpected migration status in setup"
This patch fixes it.
Sorry for previous bad try. Unfortunately I have seen Serge Hallyn's patch too late. Now I think it's better to include following code into my patch: if (setting_up) { priv->job.info.fileTotal = -1; priv->job.info.fileRemaining = -1; priv->job.info.fileProcessed = 0;
priv->job.info.memTotal = -1; priv->job.info.memRemaining = -1; priv->job.info.memProcessed = 0;
priv->job.info.dataTotal = -1; priv->job.info.dataRemaining = -1; priv->job.info.dataProcessed = 0; } But I'm not sure if my patch is good enough. If it is then I will include aforementioned code in it.
I don't think this is needed at all, I'd just keep them all 0.
This patch gets my ACK as is but I won't it push yet in case someone provides a good reasoning for initializing the counters to 2^64-1.
I don't see a good reason to initialize it to -1. IMHO 0 is more appropriate Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 11/15/2013 05:18 AM, Jiri Denemark wrote:
On Fri, Nov 15, 2013 at 20:47:43 +0900, Michael Avdienko wrote:
QEMU 1.6.0 introduced new migration status: setup Libvirt does not expect such string in QMP and refuses to migrate with error "unexpected migration status in setup"
This patch fixes it.
Sorry for previous bad try. Unfortunately I have seen Serge Hallyn's patch too late. Now I think it's better to include following code into my patch: if (setting_up) { priv->job.info.fileTotal = -1; priv->job.info.fileRemaining = -1; priv->job.info.fileProcessed = 0;
I don't think this is needed at all, I'd just keep them all 0.
This patch gets my ACK as is but I won't it push yet in case someone provides a good reasoning for initializing the counters to 2^64-1.
At least for block-mirror jobs, we document that if total and processed are equal, the job is done with phase 1 and the user can finally perform a pivot. But this is migration, not block-mirror; and I don't know if we have any code that would be confused by a total of 0 and a remaining of 0. In other words, I don't know of any reason that all 0 wouldn't work. [Hmm, I just noticed that libvirt.h claims that job info timing is tracked in 'mill-seconds', whatever those are] -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, 15 Nov 2013 21:18:08 +0900, Jiri Denemark <jdenemar@redhat.com> wrote:
On Fri, Nov 15, 2013 at 20:47:43 +0900, Michael Avdienko wrote:
QEMU 1.6.0 introduced new migration status: setup Libvirt does not expect such string in QMP and refuses to migrate with error "unexpected migration status in setup"
This patch fixes it.
Sorry for previous bad try. Unfortunately I have seen Serge Hallyn's patch too late. Now I think it's better to include following code into my patch: if (setting_up) { priv->job.info.fileTotal = -1; priv->job.info.fileRemaining = -1; priv->job.info.fileProcessed = 0;
I don't think this is needed at all, I'd just keep them all 0.
This patch gets my ACK as is but I won't it push yet in case someone provides a good reasoning for initializing the counters to 2^64-1.
Jirka
But my patch does not initialize these fields at all. Are these fields initialized here? qemuDomainObjPrivatePtr priv = vm->privateData; If not, I'm almost sure that I should set them all to 0 explicitly when status is QEMU_MONITOR_MIGRATION_STATUS_SETUP
On Fri, Nov 15, 2013 at 21:38:35 +0900, WhiteWind wrote:
On Fri, 15 Nov 2013 21:18:08 +0900, Jiri Denemark <jdenemar@redhat.com> wrote:
On Fri, Nov 15, 2013 at 20:47:43 +0900, Michael Avdienko wrote:
QEMU 1.6.0 introduced new migration status: setup Libvirt does not expect such string in QMP and refuses to migrate with error "unexpected migration status in setup"
This patch fixes it.
Sorry for previous bad try. Unfortunately I have seen Serge Hallyn's patch too late. Now I think it's better to include following code into my patch: if (setting_up) { priv->job.info.fileTotal = -1; priv->job.info.fileRemaining = -1; priv->job.info.fileProcessed = 0;
I don't think this is needed at all, I'd just keep them all 0.
This patch gets my ACK as is but I won't it push yet in case someone provides a good reasoning for initializing the counters to 2^64-1.
Jirka
But my patch does not initialize these fields at all. Are these fields initialized here? qemuDomainObjPrivatePtr priv = vm->privateData;
If not, I'm almost sure that I should set them all to 0 explicitly when status is QEMU_MONITOR_MIGRATION_STATUS_SETUP
It's initialized at the very beginning of migration by qemuMigrationJobStart(), which results in the following sequence of function calls leading to the initialization: qemuDomainObjBeginAsyncJob, qemuDomainObjBeginJobInternal, qemuDomainObjResetAsyncJob, memset(&job->info, 0, sizeof(job->info)). I agree it's pretty hidden :-) Jirka
On Fri, Nov 15, 2013 at 13:18:08 +0100, Jiri Denemark wrote:
On Fri, Nov 15, 2013 at 20:47:43 +0900, Michael Avdienko wrote:
QEMU 1.6.0 introduced new migration status: setup Libvirt does not expect such string in QMP and refuses to migrate with error "unexpected migration status in setup"
This patch fixes it.
Sorry for previous bad try. Unfortunately I have seen Serge Hallyn's patch too late. Now I think it's better to include following code into my patch: if (setting_up) { priv->job.info.fileTotal = -1; priv->job.info.fileRemaining = -1; priv->job.info.fileProcessed = 0;
priv->job.info.memTotal = -1; priv->job.info.memRemaining = -1; priv->job.info.memProcessed = 0;
priv->job.info.dataTotal = -1; priv->job.info.dataRemaining = -1; priv->job.info.dataProcessed = 0; } But I'm not sure if my patch is good enough. If it is then I will include aforementioned code in it.
I don't think this is needed at all, I'd just keep them all 0.
This patch gets my ACK as is but I won't it push yet in case someone provides a good reasoning for initializing the counters to 2^64-1.
OK, given the responses from Daniel and Eric I pushed this patch. Jirka
On 11/15/2013 04:47 AM, Michael Avdienko wrote:
QEMU 1.6.0 introduced new migration status: setup Libvirt does not expect such string in QMP and refuses to migrate with error "unexpected migration status in setup"
This patch fixes it.
+++ b/src/qemu/qemu_monitor.c @@ -114,7 +114,7 @@ VIR_ONCE_GLOBAL_INIT(qemuMonitor)
VIR_ENUM_IMPL(qemuMonitorMigrationStatus, QEMU_MONITOR_MIGRATION_STATUS_LAST, - "inactive", "active", "completed", "failed", "cancelled") + "inactive", "active", "completed", "failed", "cancelled", "setup")
VIR_ENUM_IMPL(qemuMonitorMigrationCaps, QEMU_MONITOR_MIGRATION_CAPS_LAST, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index f893b1f..eabf000 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -397,6 +397,7 @@ enum { QEMU_MONITOR_MIGRATION_STATUS_COMPLETED, QEMU_MONITOR_MIGRATION_STATUS_ERROR, QEMU_MONITOR_MIGRATION_STATUS_CANCELLED, + QEMU_MONITOR_MIGRATION_STATUS_SETUP,
Thanks for your first patch! However... In https://bugzilla.redhat.com/show_bug.cgi?id=1015636, we call out: https://lists.gnu.org/archive/html/qemu-devel/2013-10/msg00908.html where the qemu developers mention that we should be tolerant of all future unknown values, rather than the current setup of choking on unknown strings. Would you like to prepare a followup along those lines? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Thanks for your first patch! However...
In https://bugzilla.redhat.com/show_bug.cgi?id=1015636, we call out: https://lists.gnu.org/archive/html/qemu-devel/2013-10/msg00908.html
where the qemu developers mention that we should be tolerant of all future unknown values, rather than the current setup of choking on unknown strings.
Would you like to prepare a followup along those lines?
Sorry but now I have no time for such big patches
participants (6)
-
Daniel P. Berrange -
Eric Blake -
Jiri Denemark -
Michael Avdienko -
WhiteWind
-
Михаил Авдиенко