| Message ID | 1425883543-8852-5-git-send-email-zhang.zhanghailiang@huawei.com |
|---|---|
| State | New |
| Headers | show |
On 09/03/2015 07:45, zhanghailiang wrote: > 'cancelling' status was introduced by commit 51cf4c1a, mainly to avoid a > possible start of a new migration process while the previous one still exists. > But we didn't expose this status to user, instead we returned the 'active' state. > > Here, we expose it to the user (such as libvirt), 'cancelling' status only > occurs for a short window before the migration aborts, so for users, > if they cancel a migration process, it will observe 'cancelling' status > occasionally. > > Testing revealed that with older libvirt (anything 1.2.13 or less) will > print an odd error message if the state is seen, but that the migration > is still properly cancelled. Newer libvirt will be patched to recognize > the new state without the odd error message. > > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Cc: libvir-list@redhat.com Why is this necessary? Paolo > --- > migration/migration.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 035e005..a57928d 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -179,13 +179,11 @@ MigrationInfo *qmp_query_migrate(Error **errp) > break; > case MIGRATION_STATUS_SETUP: > info->has_status = true; > - info->status = MIGRATION_STATUS_SETUP; > info->has_total_time = false; > break; > case MIGRATION_STATUS_ACTIVE: > case MIGRATION_STATUS_CANCELLING: > info->has_status = true; > - info->status = MIGRATION_STATUS_ACTIVE; > info->has_total_time = true; > info->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) > - s->total_time; > @@ -221,7 +219,6 @@ MigrationInfo *qmp_query_migrate(Error **errp) > get_xbzrle_cache_stats(info); > > info->has_status = true; > - info->status = MIGRATION_STATUS_COMPLETED; > info->has_total_time = true; > info->total_time = s->total_time; > info->has_downtime = true; > @@ -243,13 +240,12 @@ MigrationInfo *qmp_query_migrate(Error **errp) > break; > case MIGRATION_STATUS_FAILED: > info->has_status = true; > - info->status = MIGRATION_STATUS_FAILED; > break; > case MIGRATION_STATUS_CANCELLED: > info->has_status = true; > - info->status = MIGRATION_STATUS_CANCELLED; > break; > } > + info->status = s->state; > > return info; > } >
On 03/13/2015 04:49 AM, Paolo Bonzini wrote: > > > On 09/03/2015 07:45, zhanghailiang wrote: >> 'cancelling' status was introduced by commit 51cf4c1a, mainly to avoid a >> possible start of a new migration process while the previous one still exists. >> But we didn't expose this status to user, instead we returned the 'active' state. >> >> Here, we expose it to the user (such as libvirt), 'cancelling' status only >> occurs for a short window before the migration aborts, so for users, >> if they cancel a migration process, it will observe 'cancelling' status >> occasionally. >> >> Testing revealed that with older libvirt (anything 1.2.13 or less) will >> print an odd error message if the state is seen, but that the migration >> is still properly cancelled. Newer libvirt will be patched to recognize >> the new state without the odd error message. >> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> Cc: libvir-list@redhat.com > > Why is this necessary? It simplifies qemu's job of reporting migration status information (qemu is no longer maintaining one set of states internally and a different set of states externally), and I already have the libvirt counterpart patch ready to go to gracefully accept the new state name.
* Eric Blake (eblake@redhat.com) wrote: > On 03/13/2015 04:49 AM, Paolo Bonzini wrote: > > > > > > On 09/03/2015 07:45, zhanghailiang wrote: > >> 'cancelling' status was introduced by commit 51cf4c1a, mainly to avoid a > >> possible start of a new migration process while the previous one still exists. > >> But we didn't expose this status to user, instead we returned the 'active' state. > >> > >> Here, we expose it to the user (such as libvirt), 'cancelling' status only > >> occurs for a short window before the migration aborts, so for users, > >> if they cancel a migration process, it will observe 'cancelling' status > >> occasionally. > >> > >> Testing revealed that with older libvirt (anything 1.2.13 or less) will > >> print an odd error message if the state is seen, but that the migration > >> is still properly cancelled. Newer libvirt will be patched to recognize > >> the new state without the odd error message. > >> > >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > >> Reviewed-by: Eric Blake <eblake@redhat.com> > >> Cc: libvir-list@redhat.com > > > > Why is this necessary? > > It simplifies qemu's job of reporting migration status information (qemu > is no longer maintaining one set of states internally and a different > set of states externally), and I already have the libvirt counterpart > patch ready to go to gracefully accept the new state name. Yes, it does make life simpler in the long run. (It does worry me a bit what happens to new qemu on old libvirt) Dave > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 03/13/2015 06:28 AM, Dr. David Alan Gilbert wrote: >> >> It simplifies qemu's job of reporting migration status information (qemu >> is no longer maintaining one set of states internally and a different >> set of states externally), and I already have the libvirt counterpart >> patch ready to go to gracefully accept the new state name. > > Yes, it does make life simpler in the long run. > (It does worry me a bit what happens to new qemu on old libvirt) In the past, we've already stated that it is okay for new qemu to require new libvirt. What is not okay is for new libvirt to require new qemu. This change is okay given those rules.
diff --git a/migration/migration.c b/migration/migration.c index 035e005..a57928d 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -179,13 +179,11 @@ MigrationInfo *qmp_query_migrate(Error **errp) break; case MIGRATION_STATUS_SETUP: info->has_status = true; - info->status = MIGRATION_STATUS_SETUP; info->has_total_time = false; break; case MIGRATION_STATUS_ACTIVE: case MIGRATION_STATUS_CANCELLING: info->has_status = true; - info->status = MIGRATION_STATUS_ACTIVE; info->has_total_time = true; info->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) - s->total_time; @@ -221,7 +219,6 @@ MigrationInfo *qmp_query_migrate(Error **errp) get_xbzrle_cache_stats(info); info->has_status = true; - info->status = MIGRATION_STATUS_COMPLETED; info->has_total_time = true; info->total_time = s->total_time; info->has_downtime = true; @@ -243,13 +240,12 @@ MigrationInfo *qmp_query_migrate(Error **errp) break; case MIGRATION_STATUS_FAILED: info->has_status = true; - info->status = MIGRATION_STATUS_FAILED; break; case MIGRATION_STATUS_CANCELLED: info->has_status = true; - info->status = MIGRATION_STATUS_CANCELLED; break; } + info->status = s->state; return info; }