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

[Matthias Bolte]

2010/3/17 Daniel P. Berrange <berrange at redhat.com>:
> On Wed, Mar 17, 2010 at 11:05:35AM +0100, Matthias Bolte wrote:
>> 2010/3/17 Jiri Denemark <jdenemar at redhat.com>:
>> >> > @@ -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
>

True. I just had the API POV in mind and forgot about the rest.

So we add a new function to configure a per-migration option because
we cannot change the existing migrate functions to add a new
parameter.

Matthias