On Wed, 11 Jul 2012 12:25:38 -0600
Eric Blake <ebl...@redhat.com> wrote:
> 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.
Yes, agreed. At least until we have libqmp...
> >> + 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.
Good to know.
>
> >> +++ 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.
Juan, if you agree with the change I can do it myself, or you could submit the
patch and I'll apply it to the QMP tree.
>
> >> #
> >> # @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
Thanks.
> >> +# 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)?
I don't think so :) Maybe we should accept both?
Would a UK grammar teacher mark 'canceled' as wrong (or vice-versa in the US)?
:)
>
> >> # 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
> >
> >
> >
>
