On 07/11/2012 12:08 PM, Luiz Capitulino wrote: > 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.
I see a difference between extending output (such as query command
return values), where clients can gracefully deal with the absence of a
parameter from an older qemu, and extending input (where it is hard to
tell up-front whether a parameter will be rejected by an older qemu).
This is a case of extending output, so I am okay with it; in fact, I'd
rather extend output of existing commands than add a new command,
because then libvirt has to probe which command to call, instead of
calling one command and then parsing everything available.
>> + 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?
Libvirt forces the use of QMP for qemu 0.15 and newer. So while there
may be other users that care, libvirt could care less whether HMP output
changes between qemu 1.1 and 1.2.
>> +++ 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 so, now's the time to change it before it becomes locked in stone
with the qemu 1.2 release. Libvirt has not yet been taught to use the
new information.
>> #
>> # @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
Actually, milliseconds
>
>> +# 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' } }
Also, s/total_time/total-time/ - QMP favors '-' over '_'
>>
>> ##
>> # @MigrationInfo
>> @@ -275,8 +280,9 @@
>> # 'cancelled'. If this field is not returned, no migration process
Did we ever decide whether to favor US (canceled) or UK (cancelled)?
>> # 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)
s/comppleted/completed/
>> #
>> # @disk: #optional @MigrationStats containing detailed disk migration
>> # status, only returned if status is 'active' and it is a block
>
>
>
--
Eric Blake ebl...@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
