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

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.