On Thu, Jul 18, 2013 at 05:29:14AM +0300, Itamar Heim wrote:
On 07/18/2013 01:24 AM, Doron Fediuck wrote:
>
>
>----- Original Message -----
>| From: "Michal Skrivanek" <mskrivan(a)redhat.com>
>| To: "Peter V. Saveliev" <peet(a)redhat.com>,
"vdsm-devel(a)lists.fedorahosted.org Development"
>| <vdsm-devel(a)lists.fedorahosted.org>
>| Sent: Monday, July 8, 2013 5:13:42 PM
>| Subject: Re: [vdsm] migration progress feature
>|
>|
>| On Jul 4, 2013, at 16:04 , Peter V. Saveliev <peet(a)redhat.com> wrote:
>|
>| > …
>| >
>| > Goal
>| > ====
>| >
>| > We have to implement a feature, migration progress bar in the UI. This
>| > migration bar should reflect not only the progress, but if the migration
>| > is stalled and so on.
>| >
>| > Tasks
>| > =====
>| >
>| > * Get the information from libvirt: it provides job progress in the same
>| > way for all migration-like jobs: migration, suspend, snapshot
>| > * Feed this information to the engine
>| > * Reflect it in the UI
>| >
>| > API status
>| > ==========
>| >
>| > Libvirt info is OK — it is available for any migration-like job, be it
>| > migration, suspend or snapshot.
>| >
>| > In VDSM, we have an API call, a separate verb to report the migration
>| > progress: migrateStatus()
>| >
>| > But also we have getVmList() call, polled by the engine every several
>| > seconds.
>| >
>| > Proposal
>| > ========
>| >
>| > We would propose to provide an optional field, `migrateStatus`, in the
>| > report sent by getVmList(). This approach should save a good amount of
>| > traffic and ease the engine side logic.
>| >
>| > Having the separate verb, this can sound weird, but I'm sure that the
>| > optimisation worth it.
>| >
>| > Please, read the patchset [1] and discuss it here.
>|
>| From my point of view this makes sense as it doesn't require additional
>| bandwidth and calls.
>| Also means no additional complexity in engine. I don't feel comfortable to
>| add another poll request during migration which makes the logic in engine
>| more complicated and means even higher traffic between hosts and engine. Not
>| that it's not doable, I just don't think it's the right way to go. I
agree
>| it's not ideal from purely API point of view...however to me it seems
>| logical to have it as part of statistics as it is a sort of statistic, and
>| it doesn't require an extra call. The extra call already exist, it's just
>| that I think we already have too many. We should start looking into
>| effective communication, not necessarily clean
>|
>| Thanks,
>| michal
>|
>
>+1
>such a feature will be very handy when migrating large guests (8GB+) over
>sub-optimal network. In such a case the last thing you want to do is additional
>calls. Re-using the existing calls by extending the statistics makes a lot
>of sense.
i'm also in favor - vm status is treated as an async update based on
the stats. vdsm can send this item only for vms which are migrating
for example.
I oppose the idea of overloading the getVMList(full=False) with
migration-related statistics just because Engine happens to be calling
this verb frequently. The full=False was born in sin and defies common
practices of client/server design.
getAllVmStats() was intended to supply all interesting statistics for
all running VMs. Adding migration statistics to it makes much more
sense. However, the size of its repsonse is quite large already, and I
heard that people are afread that calling it more frequestly would
degrade Engine's performence.
Introducing a new verb, getMigrationStatuses(), would allow Engine to
collect this piece of statistics in the exact frequency that it needs
it. Coupling this data with the frequently-polled state of VM begs for
future complaints that getVMList(full=False) is too heavy, and that we
must introduce getVMList(full=False, veryveryskinny=True).
Regrads,
Dan.