| Message ID | 1444198846-5383-8-git-send-email-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
* Denis V. Lunev (den@openvz.org) wrote: > From: Igor Redko <redkoi@virtuozzo.com> > > In this patch the ability to start a migration with test-only > capability was added. It allows to gather the guest VM’s memory > usage statistics avoiding time and memory overheads and real > data transmission. New MIGRATION_STATUS_TEST_COMPLETED was > added to distinguish between test migration and true migration > success states. Why isn't this just a new transport? i.e. I could do this just by doing a migrate to test: ? It seems simpler and avoids some of the special casing? Dave > Signed-off-by: Igor Redko <redkoi@virtuozzo.com> > Reviewed-by: Anna Melekhova <annam@virtuozzo.com> > Signed-off-by: Denis V. Lunev <den@openvz.org> > --- > migration/migration.c | 12 ++++++++++-- > qapi-schema.json | 4 +++- > 2 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 3182e15..3470d39 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -790,7 +790,9 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, > > s = migrate_init(¶ms); > > - if (strstart(uri, "tcp:", &p)) { > + if (migrate_is_test()) { > + test_start_migration(s, p, &local_err); > + } else if (strstart(uri, "tcp:", &p)) { > tcp_start_outgoing_migration(s, p, &local_err); > #ifdef CONFIG_RDMA > } else if (strstart(uri, "rdma:", &p)) { > @@ -1054,8 +1056,14 @@ static void *migration_thread(void *opaque) > } > > if (qemu_file_get_error(s->file)) { > - migrate_set_state(s, MIGRATION_STATUS_ACTIVE, > + /*FIXME replace magic number with smth legit*/ > + if (migrate_is_test() && qemu_file_get_error(s->file) == -42) { > + migrate_set_state(s, MIGRATION_STATUS_ACTIVE, > + MIGRATION_STATUS_TEST_COMPLETED); > + } else { > + migrate_set_state(s, MIGRATION_STATUS_ACTIVE, > MIGRATION_STATUS_FAILED); > + } > break; > } > > diff --git a/qapi-schema.json b/qapi-schema.json > index 38bf199..e022f9c 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -432,6 +432,8 @@ > # > # @completed: migration is finished. > # > +# @test-completed: migration time estimation finished. > +# > # @failed: some error occurred during migration process. > # > # Since: 2.3 > @@ -439,7 +441,7 @@ > ## > { 'enum': 'MigrationStatus', > 'data': [ 'none', 'setup', 'cancelling', 'cancelled', > - 'active', 'completed', 'failed' ] } > + 'active', 'completed', 'test-completed', 'failed' ] } > > ## > # @MigrationInfo > -- > 2.1.4 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 10/07/2015 07:56 AM, Dr. David Alan Gilbert wrote: > * Denis V. Lunev (den@openvz.org) wrote: >> From: Igor Redko <redkoi@virtuozzo.com> >> >> In this patch the ability to start a migration with test-only >> capability was added. It allows to gather the guest VM’s memory >> usage statistics avoiding time and memory overheads and real >> data transmission. New MIGRATION_STATUS_TEST_COMPLETED was >> added to distinguish between test migration and true migration >> success states. > > Why isn't this just a new transport? i.e. I could do this just by doing > a migrate to test: ? > > It seems simpler and avoids some of the special casing? Indeed. Even if the new state can only be triggered by using a new option, the fact that the new state exists in introspection means clients have to figure out what to do with it; while a new transport would not require any new states ("running" to completion means that nothing migrated, but the existing states can then be reused to see that the test: transport has finished computing its numbers). >> +++ b/qapi-schema.json >> @@ -432,6 +432,8 @@ >> # >> # @completed: migration is finished. >> # >> +# @test-completed: migration time estimation finished. >> +# If this gets added, in spite of our ideas to use a 'test:' transport instead of a new state, it would need a 'since 2.5' marker.
On 10/07/2015 06:08 PM, Eric Blake wrote: > On 10/07/2015 07:56 AM, Dr. David Alan Gilbert wrote: >> * Denis V. Lunev (den@openvz.org) wrote: >>> From: Igor Redko <redkoi@virtuozzo.com> >>> >>> In this patch the ability to start a migration with test-only >>> capability was added. It allows to gather the guest VM’s memory >>> usage statistics avoiding time and memory overheads and real >>> data transmission. New MIGRATION_STATUS_TEST_COMPLETED was >>> added to distinguish between test migration and true migration >>> success states. >> Why isn't this just a new transport? i.e. I could do this just by doing >> a migrate to test: ? >> >> It seems simpler and avoids some of the special casing? > Indeed. Even if the new state can only be triggered by using a new > option, the fact that the new state exists in introspection means > clients have to figure out what to do with it; while a new transport > would not require any new states ("running" to completion means that > nothing migrated, but the existing states can then be reused to see that > the test: transport has finished computing its numbers). > we need to expose calculated numbers to the caller somehow. This could be done in MIGRATION_STATUS_COMPLETED state even for ordinary migration. Will it be OK? Den >>> +++ b/qapi-schema.json >>> @@ -432,6 +432,8 @@ >>> # >>> # @completed: migration is finished. >>> # >>> +# @test-completed: migration time estimation finished. >>> +# > If this gets added, in spite of our ideas to use a 'test:' transport > instead of a new state, it would need a 'since 2.5' marker. >
On 10/07/2015 04:56 PM, Dr. David Alan Gilbert wrote: > * Denis V. Lunev (den@openvz.org) wrote: >> From: Igor Redko <redkoi@virtuozzo.com> >> >> In this patch the ability to start a migration with test-only >> capability was added. It allows to gather the guest VM’s memory >> usage statistics avoiding time and memory overheads and real >> data transmission. New MIGRATION_STATUS_TEST_COMPLETED was >> added to distinguish between test migration and true migration >> success states. > Why isn't this just a new transport? i.e. I could do this just by doing > a migrate to test: ? > > It seems simpler and avoids some of the special casing? > > Dave we would like to avoid VM pause in the migration_thread when the process is finished. Thus we should have a capability for this in the rest of the code. Though we can setup the capability here or in the suitable place and check that capability in the migration thread using transport here as a distinction. Will it be OK for you? Den >> Signed-off-by: Igor Redko <redkoi@virtuozzo.com> >> Reviewed-by: Anna Melekhova <annam@virtuozzo.com> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> --- >> migration/migration.c | 12 ++++++++++-- >> qapi-schema.json | 4 +++- >> 2 files changed, 13 insertions(+), 3 deletions(-) >> >> diff --git a/migration/migration.c b/migration/migration.c >> index 3182e15..3470d39 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -790,7 +790,9 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, >> >> s = migrate_init(¶ms); >> >> - if (strstart(uri, "tcp:", &p)) { >> + if (migrate_is_test()) { >> + test_start_migration(s, p, &local_err); >> + } else if (strstart(uri, "tcp:", &p)) { >> tcp_start_outgoing_migration(s, p, &local_err); >> #ifdef CONFIG_RDMA >> } else if (strstart(uri, "rdma:", &p)) { >> @@ -1054,8 +1056,14 @@ static void *migration_thread(void *opaque) >> } >> >> if (qemu_file_get_error(s->file)) { >> - migrate_set_state(s, MIGRATION_STATUS_ACTIVE, >> + /*FIXME replace magic number with smth legit*/ >> + if (migrate_is_test() && qemu_file_get_error(s->file) == -42) { >> + migrate_set_state(s, MIGRATION_STATUS_ACTIVE, >> + MIGRATION_STATUS_TEST_COMPLETED); >> + } else { >> + migrate_set_state(s, MIGRATION_STATUS_ACTIVE, >> MIGRATION_STATUS_FAILED); >> + } >> break; >> } >> >> diff --git a/qapi-schema.json b/qapi-schema.json >> index 38bf199..e022f9c 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -432,6 +432,8 @@ >> # >> # @completed: migration is finished. >> # >> +# @test-completed: migration time estimation finished. >> +# >> # @failed: some error occurred during migration process. >> # >> # Since: 2.3 >> @@ -439,7 +441,7 @@ >> ## >> { 'enum': 'MigrationStatus', >> 'data': [ 'none', 'setup', 'cancelling', 'cancelled', >> - 'active', 'completed', 'failed' ] } >> + 'active', 'completed', 'test-completed', 'failed' ] } >> >> ## >> # @MigrationInfo >> -- >> 2.1.4 >> >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Denis V. Lunev (den@openvz.org) wrote: > On 10/07/2015 06:08 PM, Eric Blake wrote: > >On 10/07/2015 07:56 AM, Dr. David Alan Gilbert wrote: > >>* Denis V. Lunev (den@openvz.org) wrote: > >>>From: Igor Redko <redkoi@virtuozzo.com> > >>> > >>>In this patch the ability to start a migration with test-only > >>>capability was added. It allows to gather the guest VM’s memory > >>>usage statistics avoiding time and memory overheads and real > >>>data transmission. New MIGRATION_STATUS_TEST_COMPLETED was > >>>added to distinguish between test migration and true migration > >>>success states. > >>Why isn't this just a new transport? i.e. I could do this just by doing > >>a migrate to test: ? > >> > >>It seems simpler and avoids some of the special casing? > >Indeed. Even if the new state can only be triggered by using a new > >option, the fact that the new state exists in introspection means > >clients have to figure out what to do with it; while a new transport > >would not require any new states ("running" to completion means that > >nothing migrated, but the existing states can then be reused to see that > >the test: transport has finished computing its numbers). > > > > we need to expose calculated numbers to the caller somehow. > This could be done in MIGRATION_STATUS_COMPLETED state > even for ordinary migration. Will it be OK? I think so; if you look at the way the information is displayed in info migrate, a lot of the data is only displayed 'sometimes', e.g. : if (blk_mig_active()) { info->has_disk = true; info->disk = g_malloc0(sizeof(*info->disk)); info->disk->transferred = blk_mig_bytes_transferred(); info->disk->remaining = blk_mig_bytes_remaining(); info->disk->total = blk_mig_bytes_total(); } It seems fair to do the same type of thing if you have an extra block of values. Dave > > Den > > >>>+++ b/qapi-schema.json > >>>@@ -432,6 +432,8 @@ > >>> # > >>> # @completed: migration is finished. > >>> # > >>>+# @test-completed: migration time estimation finished. > >>>+# > >If this gets added, in spite of our ideas to use a 'test:' transport > >instead of a new state, it would need a 'since 2.5' marker. > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Denis V. Lunev (den@openvz.org) wrote: > On 10/07/2015 04:56 PM, Dr. David Alan Gilbert wrote: > >* Denis V. Lunev (den@openvz.org) wrote: > >>From: Igor Redko <redkoi@virtuozzo.com> > >> > >>In this patch the ability to start a migration with test-only > >>capability was added. It allows to gather the guest VM’s memory > >>usage statistics avoiding time and memory overheads and real > >>data transmission. New MIGRATION_STATUS_TEST_COMPLETED was > >>added to distinguish between test migration and true migration > >>success states. > >Why isn't this just a new transport? i.e. I could do this just by doing > >a migrate to test: ? > > > >It seems simpler and avoids some of the special casing? > > > >Dave > we would like to avoid VM pause in the migration_thread when > the process is finished. Thus we should have a capability > for this in the rest of the code. Ah OK. > Though we can setup the capability here or in the suitable > place and check that capability in the migration thread > using transport here as a distinction. > > Will it be OK for you? Yes, that works for me I think. Dave > > Den > > >>Signed-off-by: Igor Redko <redkoi@virtuozzo.com> > >>Reviewed-by: Anna Melekhova <annam@virtuozzo.com> > >>Signed-off-by: Denis V. Lunev <den@openvz.org> > >>--- > >> migration/migration.c | 12 ++++++++++-- > >> qapi-schema.json | 4 +++- > >> 2 files changed, 13 insertions(+), 3 deletions(-) > >> > >>diff --git a/migration/migration.c b/migration/migration.c > >>index 3182e15..3470d39 100644 > >>--- a/migration/migration.c > >>+++ b/migration/migration.c > >>@@ -790,7 +790,9 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, > >> s = migrate_init(¶ms); > >>- if (strstart(uri, "tcp:", &p)) { > >>+ if (migrate_is_test()) { > >>+ test_start_migration(s, p, &local_err); > >>+ } else if (strstart(uri, "tcp:", &p)) { > >> tcp_start_outgoing_migration(s, p, &local_err); > >> #ifdef CONFIG_RDMA > >> } else if (strstart(uri, "rdma:", &p)) { > >>@@ -1054,8 +1056,14 @@ static void *migration_thread(void *opaque) > >> } > >> if (qemu_file_get_error(s->file)) { > >>- migrate_set_state(s, MIGRATION_STATUS_ACTIVE, > >>+ /*FIXME replace magic number with smth legit*/ > >>+ if (migrate_is_test() && qemu_file_get_error(s->file) == -42) { > >>+ migrate_set_state(s, MIGRATION_STATUS_ACTIVE, > >>+ MIGRATION_STATUS_TEST_COMPLETED); > >>+ } else { > >>+ migrate_set_state(s, MIGRATION_STATUS_ACTIVE, > >> MIGRATION_STATUS_FAILED); > >>+ } > >> break; > >> } > >>diff --git a/qapi-schema.json b/qapi-schema.json > >>index 38bf199..e022f9c 100644 > >>--- a/qapi-schema.json > >>+++ b/qapi-schema.json > >>@@ -432,6 +432,8 @@ > >> # > >> # @completed: migration is finished. > >> # > >>+# @test-completed: migration time estimation finished. > >>+# > >> # @failed: some error occurred during migration process. > >> # > >> # Since: 2.3 > >>@@ -439,7 +441,7 @@ > >> ## > >> { 'enum': 'MigrationStatus', > >> 'data': [ 'none', 'setup', 'cancelling', 'cancelled', > >>- 'active', 'completed', 'failed' ] } > >>+ 'active', 'completed', 'test-completed', 'failed' ] } > >> ## > >> # @MigrationInfo > >>-- > >>2.1.4 > >> > >> > >-- > >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/migration/migration.c b/migration/migration.c index 3182e15..3470d39 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -790,7 +790,9 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, s = migrate_init(¶ms); - if (strstart(uri, "tcp:", &p)) { + if (migrate_is_test()) { + test_start_migration(s, p, &local_err); + } else if (strstart(uri, "tcp:", &p)) { tcp_start_outgoing_migration(s, p, &local_err); #ifdef CONFIG_RDMA } else if (strstart(uri, "rdma:", &p)) { @@ -1054,8 +1056,14 @@ static void *migration_thread(void *opaque) } if (qemu_file_get_error(s->file)) { - migrate_set_state(s, MIGRATION_STATUS_ACTIVE, + /*FIXME replace magic number with smth legit*/ + if (migrate_is_test() && qemu_file_get_error(s->file) == -42) { + migrate_set_state(s, MIGRATION_STATUS_ACTIVE, + MIGRATION_STATUS_TEST_COMPLETED); + } else { + migrate_set_state(s, MIGRATION_STATUS_ACTIVE, MIGRATION_STATUS_FAILED); + } break; } diff --git a/qapi-schema.json b/qapi-schema.json index 38bf199..e022f9c 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -432,6 +432,8 @@ # # @completed: migration is finished. # +# @test-completed: migration time estimation finished. +# # @failed: some error occurred during migration process. # # Since: 2.3 @@ -439,7 +441,7 @@ ## { 'enum': 'MigrationStatus', 'data': [ 'none', 'setup', 'cancelling', 'cancelled', - 'active', 'completed', 'failed' ] } + 'active', 'completed', 'test-completed', 'failed' ] } ## # @MigrationInfo