[libvirt] [PATCH v2 8/8] virsh: add postcopy-after option to migrate command

[Cristian Klein]

On 07 Oct 2014, at 23:48 , Jiri Denemark <jdenemar at redhat.com> wrote:

> On Tue, Sep 30, 2014 at 16:39:29 +0200, Cristian Klein wrote:
>> Signed-off-by: Cristian Klein <cristian.klein at cs.umu.se>
>> ---
>> tools/virsh-domain.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>> tools/virsh.pod      |  5 ++++
>> 2 files changed, 75 insertions(+), 2 deletions(-)
>> 
>> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>> index 36a6d52..395c73c 100644
>> --- a/tools/virsh-domain.c
>> +++ b/tools/virsh-domain.c
>> @@ -9251,6 +9251,10 @@ static const vshCmdOptDef opts_migrate[] = {
>>      .type = VSH_OT_INT,
>>      .help = N_("force guest to suspend if live migration exceeds timeout (in seconds)")
>>     },
>> +    {.name = "postcopy-after",
>> +     .type = VSH_OT_INT,
>> +     .help = N_("switch to post-copy migration if live migration exceeds timeout (in seconds)")
>> +    },
>>     {.name = "xml",
>>      .type = VSH_OT_STRING,
>>      .help = N_("filename containing updated XML for the target")
> 
> We also want to add --postcopy option to enable just
> VIR_MIGRATE_POSTCOPY without an automatic switch to post-copy after a
> timeout. The --postcopy-after option may turn it on automatically,
> though, so that users don't have to use --postcopy --postcopy-after. And
> we also want to add a new migrate-startpostcopy command as a direct
> wrapper to virDomainMigrateStartPostCopy.

I’m still not convinced this is a good idea. To use —postcopy and migrate-startpostcopy, the user would have to launch a second instance of virsh, since the first one is busy monitoring the migration. I feel that this is a bit too low level for a tool like virsh: It would be as if we offered the user a dedicated migrate-cancel command, instead of pressing CTRL+C.

>> @@ -9332,6 +9336,8 @@ doMigrate(void *opaque)
>>         VIR_FREE(xml);
>>     }
>> 
>> +    if (vshCommandOptBool(cmd, "postcopy-after")) /* actually an int */
>> +        flags |= VIR_MIGRATE_POSTCOPY;
>>     if (vshCommandOptBool(cmd, "live"))
>>         flags |= VIR_MIGRATE_LIVE;
>>     if (vshCommandOptBool(cmd, "p2p"))
>> @@ -9423,6 +9429,50 @@ vshMigrationTimeout(vshControl *ctl,
>>     virDomainSuspend(dom);
>> }
>> 
>> +static void
>> +vshMigrationPostCopyAfter(vshControl *ctl,
>> +                    virDomainPtr dom,
>> +                    void *opaque ATTRIBUTE_UNUSED)
>> +{
>> +    vshDebug(ctl, VSH_ERR_DEBUG, "starting post-copy\n");
>> +    int rv = virDomainMigrateStartPostCopy(dom, 0);
>> +    if (rv < 0) {
>> +        vshError(ctl, "%s", _("start post-copy command failed"));
>> +    } else {
>> +        vshDebug(ctl, VSH_ERR_INFO, "switched to post-copy\n");
>> +    }
>> +}
>> +
>> +/* Parse the --postcopy-after parameter in seconds, but store its
>> + * value in milliseconds. Return -1 on error, 0 if
>> + * no timeout was requested, and 1 if timeout was set.
>> + * Copy-paste-adapted from vshCommandOptTimeoutToMs.
>> + */
>> +static int
>> +vshCommandOptPostCopyAfterToMs(vshControl *ctl, const vshCmd *cmd, int *postCopyAfter)
>> +{
>> +    int rv = vshCommandOptInt(cmd, "postcopy-after", postCopyAfter);
>> +
>> +    if (rv < 0 || (rv > 0 && *postCopyAfter < 0)) {
>> +        vshError(ctl, "%s", _("invalid postcopy-after parameter"));
>> +        return -1;
>> +    }
>> +    if (rv > 0) {
>> +        /* Ensure that we can multiply by 1000 without overflowing. */
>> +        if (*postCopyAfter > INT_MAX / 1000) {
>> +            vshError(ctl, "%s", _("post-copy after parameter is too big"));
>> +            return -1;
>> +        }
>> +        *postCopyAfter *= 1000;
>> +        /* 0 is a special value inside virsh, which means no timeout, so
>> +         * use 1ms instead for "start post-copy immediately"
>> +         */
>> +        if (*postCopyAfter == 0)
>> +            *postCopyAfter = 1;
>> +    }
>> +    return rv;
>> +}
>> +
>> static bool
>> cmdMigrate(vshControl *ctl, const vshCmd *cmd)
>> {
>> @@ -9432,6 +9482,7 @@ cmdMigrate(vshControl *ctl, const vshCmd *cmd)
>>     bool verbose = false;
>>     bool functionReturn = false;
>>     int timeout = 0;
>> +    int postCopyAfter = 0;
>>     bool live_flag = false;
>>     vshCtrlData data = { .dconn = NULL };
>> 
>> @@ -9451,6 +9502,18 @@ cmdMigrate(vshControl *ctl, const vshCmd *cmd)
>>         goto cleanup;
>>     }
>> 
>> +    if (vshCommandOptPostCopyAfterToMs(ctl, cmd, &postCopyAfter) < 0) {
>> +        goto cleanup;
>> +    } else if (postCopyAfter > 0 && !live_flag) {
>> +        vshError(ctl, "%s",
>> +                 _("migrate: Unexpected postcopy-after for offline migration"));
> 
> Offline migration is a migration of a domain which is not running at
> all. We don't have a good name for migration of a paused domain, some
> refer to it as coma migration. Anyway, this is checked for in the daemon
> so checking here is redundant. Especially when this check seems to be
> somewhat artificial. Sure, it doesn't make a lot of sense but post-copy
> migration of a paused domain should be doable. Moreover, I believe you
> can call virDomainSuspend during a post-copy migration, which is very
> close to running migration without —live.
> 
>> +        goto cleanup;
>> +    } else if (postCopyAfter > 0 && timeout > 0) {
>> +        vshError(ctl, "%s",
>> +                 _("migrate: --postcopy-after is incompatible with --timeout"));
>> +        goto cleanup;
>> +    }
> 
> Anyway, handling --postcopy-after seems quite complicated since we have
> to repeat the logic around -1, 0, 1 return values in two places. Can't
> we just directly fetch and process the value in cmdMigrate?
> 
>> +
>>     if (pipe(p) < 0)
>>         goto cleanup;
>> 

I’ll fix the other issues you have raised.

Cristian