On Fri, 29 Jun 2012 18:43:57 +0200
Juan Quintela <quint...@redhat.com> wrote:
> We add time spent for migration to the output of "info migrate"
> command. 'total_time' means time since the start fo migration if
> migration is 'active', and total time of migration if migration is
> completed. As we are also interested in transferred ram when
> migration completes, adding all ram statistics
I see this has already been merged and am sorry for being late with my
review, but it turns out that there are a few issues to be addressed in
this patch, comments inlined below.
Another point is that this patch extends the query-migrate command. We've
decided not to extend QMP commands, however I think that we should relax
that restriction for query commands, because the client doesn't need to know
the new fields in advance.
>
> Signed-off-by: Juan Quintela <quint...@redhat.com>
> ---
> hmp.c | 2 ++
> migration.c | 11 +++++++++++
> migration.h | 1 +
> qapi-schema.json | 12 +++++++++---
> 4 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index b9cec1d..4c6d4ae 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -145,6 +145,8 @@ void hmp_info_migrate(Monitor *mon)
> info->ram->remaining >> 10);
> monitor_printf(mon, "total ram: %" PRIu64 " kbytes\n",
> info->ram->total >> 10);
> + monitor_printf(mon, "total time: %" PRIu64 " milliseconds\n",
> + info->ram->total_time);
This adds a new line to the HMP output between the end of the ram stats and
the disk stats. Iirc libvirt parses this output when in non-json mode, although
I don't think it ever does it for disk migration.
Eric, does libvirt do that?
Btw, we'll need to change where 'total_time' is printed, see below.
> }
>
> if (info->has_disk) {
> diff --git a/migration.c b/migration.c
> index 810727f..8db1b43 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -131,6 +131,8 @@ MigrationInfo *qmp_query_migrate(Error **errp)
> info->ram->transferred = ram_bytes_transferred();
> info->ram->remaining = ram_bytes_remaining();
> info->ram->total = ram_bytes_total();
> + info->ram->total_time = qemu_get_clock_ms(rt_clock)
> + - s->total_time;
>
I really don't think that 'total_time' pertains to the ram stats info, I think
it should be in the MigrationInfo dict.
> if (blk_mig_active()) {
> info->has_disk = true;
> @@ -143,6 +145,13 @@ MigrationInfo *qmp_query_migrate(Error **errp)
> case MIG_STATE_COMPLETED:
> info->has_status = true;
> info->status = g_strdup("completed");
> +
> + info->has_ram = true;
> + info->ram = g_malloc0(sizeof(*info->ram));
> + info->ram->transferred = ram_bytes_transferred();
> + info->ram->remaining = 0;
> + info->ram->total = ram_bytes_total();
> + info->ram->total_time = s->total_time;
Having the 'total_time' in the MigrationInfo dict would avoid this change.
> break;
> case MIG_STATE_ERROR:
> info->has_status = true;
> @@ -260,6 +269,7 @@ static void migrate_fd_put_ready(void *opaque)
> } else {
> migrate_fd_completed(s);
> }
> + s->total_time = qemu_get_clock_ms(rt_clock) - s->total_time;
> if (s->state != MIG_STATE_COMPLETED) {
> if (old_vm_running) {
> vm_start();
> @@ -372,6 +382,7 @@ static MigrationState *migrate_init(const MigrationParams
> *params)
>
> s->bandwidth_limit = bandwidth_limit;
> s->state = MIG_STATE_SETUP;
> + s->total_time = qemu_get_clock_ms(rt_clock);
>
> return s;
> }
> diff --git a/migration.h b/migration.h
> index 35207bd..de13004 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -37,6 +37,7 @@ struct MigrationState
> int (*write)(MigrationState *s, const void *buff, size_t size);
> void *opaque;
> MigrationParams params;
> + int64_t total_time;
> };
>
> void process_incoming_migration(QEMUFile *f);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3b6e346..1ab5dbd 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -260,10 +260,15 @@
> #
> # @total: total amount of bytes involved in the migration process
> #
> +# @total_time: tota0l amount of ms since migration started. If
s/total0l/total
s/ms/miliseconds
> +# migration has ended, it returns the total migration
> +# time. (since 1.2)
> +#
> # Since: 0.14.0.
> ##
> { 'type': 'MigrationStats',
> - 'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' } }
> + 'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
> + 'total_time': 'int' } }
>
> ##
> # @MigrationInfo
> @@ -275,8 +280,9 @@
> # 'cancelled'. If this field is not returned, no migration process
> # has been initiated
> #
> -# @ram: #optional @MigrationStats containing detailed migration status,
> -# only returned if status is 'active'
> +# @ram: #optional @MigrationStats containing detailed migration
> +# status, only returned if status is 'active' or
> +# 'completed'. 'comppleted' (since 1.2)
> #
> # @disk: #optional @MigrationStats containing detailed disk migration
> # status, only returned if status is 'active' and it is a block
