Re: [libvirt] [PATCH] Add --downtime option to virsh migrate command

On Wed, Mar 17, 2010 at 11:05:35AM +0100, Matthias Bolte wrote: > 2010/3/17 Jiri Denemark <jdenemar(a)redhat.com&gt;: > >> > @@ -2794,6 +2799,19 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) > >> >     if (vshCommandOptBool (cmd, "suspend")) > >> >         flags |= VIR_MIGRATE_PAUSED; > >> > > >> > +    downtime = vshCommandOptFloat(cmd, "downtime", &found); > >> > +    if (found) { > >> > +        unsigned long long nanoseconds = downtime * 1e9; > >> > + > >> > +        if (nanoseconds <= 0) { > >> > +            vshError(ctl, "%s", _("migrate: Invalid downtime")); > >> > +            goto done; > >> > +        } > >> > + > >> > +        if (virDomainMigrateSetDowntime(dom, nanoseconds)) > >> > >> Is this persistent, or only valid for the next migration? For example, > >> I do a migration with --downtime and afterwards I do another migration > >> with the same domain, but this time I don't specify a downtime. Would > >> the first downtime still apply to the second migration? > > > > Actually, this is a very good question since it reveals the API wasn't > > designed well enough. Current implementation doesn't do much with the value > > passed to virDomainMigrateSetDowntime, it just sends it to the appropriate > > hypervisor (if it supports such functionality). In other words, the behavior > > may differ for different hypervisors. Which doesn't seem to be a good thing > > for a libvirt's public API. So we should decide whether the effect of calling > > this API should be one-time or persistent and emulate that behavior for > > hypervisors which don't support it. We also need flags parameter so that we > > can change that behavior in the future. > > > > Opinions? > > > > Jirka > > > > Your patch series haven't been reviewed in detail yet, so some general > comments about it. > > First of all, as with all new public API functions, > virDomainMigrateSetDowntime should have an 'unsigned int flags' > parameter. Even if it's currently unused it could come in handy in the > future. > > Maybe the function should be called > virDomainMigrateSetTolerableDowntime or > virDomainMigrateSetMaxDowntime, to emphasis that this downtime is an > upper limit. > > There is no getter for the max downtime value, I think there should be one. > > Regarding the persistence, if the max downtime is implemented in the > way of a setter/(getter), it should be persistent. For a per-migration > option it should be a parameter to the migrate function like > bandwidth, but we can change the migrate function anymore. Therefore, > the max downtime should be a persistent option per domain, like the > autostart flag. Conceptually this doesn't make sense. The max downtime parameter is a live tunable that a management app will set based on the conditions at the time of migration. Suitable value is dependant on the workload of the guest (how quickly it is dirtying RAM) and network bandwidth between hosts (how quickly RAM is transferred). You would only increase the max downtime if the VM was not able to complete migration promptly enough. So it shouldn't be persistent, and I don't think there's any compelling need for a getter either. Regards, Daniel