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@redhat.com | To: "Peter V. Saveliev" peet@redhat.com, "vdsm-devel@lists.fedorahosted.org Development" | vdsm-devel@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@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.