[libvirt] [PATCH v3 2/2] auto cold migration fallback at timeout

[Eric Blake]

On 12/17/2010 01:49 AM, Wen Congyang wrote:
> implement auto cold migration fallback at timeout

Maybe it's a language barrier issue thing, but I had to read this patch
several times before I understood what you were getting at.  Does this
wording work any better?

If a guest has not completed live migration before timeout, then
auto-suspend the guest, where the migration will complete offline.

> 
> Signed-off-by: Wen Congyang <wency at cn.fujitsu.com>
> 
> ---
>  tools/virsh.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 66 insertions(+), 0 deletions(-)

Missing a change to tools/virsh.pod to document the new option (that
wording should be more complete, in comparison to the one-line blurb for
the 'virsh help migrate' output).

> 
> diff --git a/tools/virsh.c b/tools/virsh.c
> index cbde085..b95c8fe 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -3379,9 +3379,32 @@ static const vshCmdOptDef opts_migrate[] = {
>      {"desturi", VSH_OT_DATA, VSH_OFLAG_REQ, N_("connection URI of the destination host")},
>      {"migrateuri", VSH_OT_DATA, 0, N_("migration URI, usually can be omitted")},
>      {"dname", VSH_OT_DATA, 0, N_("rename to new name during migration (if supported)")},
> +    {"timeout", VSH_OT_INT, 0, N_("auto cold migration fallback when live migration timeouts(in seconds)")},

Maybe: "force guest to suspend if live migration exceeds timeout (in
seconds)"

>      {NULL, 0, 0, NULL}
>  };
>  
> +static void migrateTimeoutHandler(void *data)
> +{

I'd float this helper function to live above the cmdMigrate definition;
placing it here interrupts the flow between what options cmdMigrate
takes and how it uses them.

> +    virDomainPtr dom = (virDomainPtr)data;

Cast is not necessary.

>  cmdMigrate (vshControl *ctl, const vshCmd *cmd)
>  {
> @@ -3389,6 +3412,8 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd)
>      const char *desturi;
>      const char *migrateuri;
>      const char *dname;
> +    long long timeout;
> +    virTimerPtr migratetimer = NULL;
>      int flags = 0, found, ret = FALSE;
>  
>      if (!vshConnectionUsability (ctl, ctl->conn))
> @@ -3426,6 +3451,28 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd)
>      if (vshCommandOptBool (cmd, "copy-storage-inc"))
>          flags |= VIR_MIGRATE_NON_SHARED_INC;
>  
> +    timeout = vshCommandOptLongLong(cmd, "timeout", &found);

Why long long?  No one is going to set a timeout bigger than 4 billion
seconds, so plain int would do just fine here.  In particular, since
virEventAddTimeout can only sleep up to int milliseconds, and you are
already multiplying the user's timeout value (in seconds) by 1000, you
already have to worry about overflow issues - if the user requests 5
million seconds (which overflows 4 billion milliseconds for the
underlying timer max frequency), you must reject the user's value.

> +
> +        migratetimer = virNewTimer(migrateTimeoutHandler, (void *)dom);
> +        if (!migratetimer)
> +            goto done;
> +
> +        if (virStartTimer() < 0) {

Hmm - given your usage of starting a timer thread as long as any
virTimerPtr object exists, maybe you're better off creating some sort of
global ref-counting state under the hood of timer.c that tracks how many
virTimer objects are currently set to have a time.  The user should only
have to call a single function to create their timer object, without
having to make extra calls to feed your underlying implementation.

> @@ -3455,6 +3516,11 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd)
>      }
>  
>   done:
> +    if (migratetimer) {
> +        virStopTimer();
> +        virDelTimer(migratetimer);

Again, if multiple threads are managing timers, then the user shouldn't
be able to kill the timer resources just because one thread no longer
needs a timer.  The user shouldn't have to call virStopTimer.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20101217/51ff6ef9/attachment-0001.sig>