Re: [Qemu-devel] [PATCH 06/13] Add spent time for migration

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 11 Jul 2012 12:25:38 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 07/11/2012 12:08 PM, Luiz Capitulino wrote:
> > On Fri, 29 Jun 2012 18:43:57 +0200
> > Juan Quintela <quintela@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
> > 
> > 
> > 
>