[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 4 of 5] tools: set migration constraints from cmdline



On Wed, 2013-03-06 at 16:48 +0000, Olaf Hering wrote:
> # HG changeset patch
> # User Olaf Hering <olaf@xxxxxxxxx>
> # Date 1362585914 -3600
> # Node ID 29c66a248f5bb153ae6ac157afcdd91c2390c6a9
> # Parent  1ea501d602649c58412f73e83e24115eb3d8fe44
> tools: set migration constraints from cmdline
> 
> Add new options to xm/xl migrate to control the process of migration.
> The intention is to optionally abort the migration if it takes too long
> to migrate a busy guest due to the high number of dirty pages. Currently
> the guest is suspended to transfer the remaining dirty pages. This
> transfer can take too long, which can confuse the guest if its suspended
> for too long.
> 
> -M <number>   Number of iterations before final suspend (default: 30)

Do the defaults here reflect the current behaviour or are they in
themselves a change?

> --max_iters <number>

I'm really not a fan of underscores in command line arguments, but I
suppose there is precedent in --tslice_ms in other places.

> 
> -m <factor>   Max amount of memory to transfer before final suspend (default: 
> 3*RAM)
> --max_factor <factor>
> 
> -A            Abort migration instead of doing final suspend.
                                                             ... if <what>

> --abort_if_busy
> 
> 
> 
> The changes to libxl change the API, handle LIBXL_API_VERSION == 0x040200.
> 
> TODO:
>  eventually add also --min_remaining (default value 50) in a seperate patch
> 
> v6:
>  - update the LIBXL_API_VERSION handling for libxl_domain_suspend
>    change it to an inline function if LIBXL_API_VERSION is defined to 4.2.0
>  - rename libxl_save_properties to libxl_domain_suspend_properties
>  - rename ->xlflags to ->flags within that struct
> 
> v5:
>  - adjust libxl_domain_suspend prototype, move flags, max_iters,
>    max_factor into a new, optional struct libxl_save_properties
>  - rename XCFLAGS_DOMSAVE_NOSUSPEND to XCFLAGS_DOMSAVE_ABORT_IF_BUSY
>  - rename LIBXL_SUSPEND_NO_FINAL_SUSPEND to LIBXL_SUSPEND_ABORT_IF_BUSY
>  - rename variables no_suspend to abort_if_busy
>  - rename option -N/--no_suspend to -A/--abort_if_busy
>  - update xl.1, extend description of -A option
> 
> v4:
>  - update default for no_suspend from None to 0 in XendCheckpoint.py:save
>  - update logoutput in setMigrateConstraints
>  - change xm migrate defaults from None to 0
>  - add new options to xl.1
>  - fix syntax error in XendDomain.py:domain_migrate_constraints_set
>  - fix xm migrate -N option name to match xl migrate
> 
> v3:
>  - move logic errors in libxl__domain_suspend and fixed help text in
>    cmd_table to separate patches
>  - fix syntax error in XendCheckpoint.py
>  - really pass max_iters and max_factor in libxl__xc_domain_save
>  - make libxl_domain_suspend_0x040200 declaration globally visible
>  - bump libxenlight.so SONAME from 2.0 to 2.1 due to changed
>    libxl_domain_suspend
> 
> v2:
>  - use LIBXL_API_VERSION and define libxl_domain_suspend_0x040200
>  - fix logic error in min_reached check in xc_domain_save
>  - add longopts
>  - update --help text
>  - correct description of migrate --help text
> 
> Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>
> 
> diff -r 1ea501d60264 -r 29c66a248f5b docs/man/xl.pod.1
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -391,6 +391,21 @@ Send <config> instead of config file fro
> 
>  Print huge (!) amount of debug during the migration process.
> 
> +=item B<-M> I<number>, B<--max_iters> I<number>
> +
> +Number of iterations before final suspend (default: 30)
> +
> +=item B<-m> I<factor>, B<--max_factor> I<factor>
> +
> +Max amount of memory to transfer before final suspend (default: 3*RAM)
> +
> +=item B<-A>, B<--abort_if_busy>
> +
> +Abort migration instead of doing final suspend/transfer/resume if the
> +guest has still dirty pages after the number of iterations and/or the
> +amount of RAM transfered. This avoids long periods of time were the

                 transferred                                  where

> +guest is suspended.
> +
>  =back
> 
>  =item B<remus> [I<OPTIONS>] I<domain-id> I<host>
> diff -r 1ea501d60264 -r 29c66a248f5b tools/libxc/xc_domain_save.c
> --- a/tools/libxc/xc_domain_save.c
> +++ b/tools/libxc/xc_domain_save.c
> @@ -804,6 +804,7 @@ int xc_domain_save(xc_interface *xch, in
>      int rc = 1, frc, i, j, last_iter = 0, iter = 0;
>      int live  = (flags & XCFLAGS_LIVE);
>      int debug = (flags & XCFLAGS_DEBUG);
> +    int abort_if_busy = (flags & XCFLAGS_DOMSAVE_ABORT_IF_BUSY);
>      int superpages = !!hvm;
>      int race = 0, sent_last_iter, skip_this_iter = 0;
>      unsigned int sent_this_iter = 0;
> @@ -1527,10 +1528,20 @@ int xc_domain_save(xc_interface *xch, in
> 
>          if ( live )
>          {
> +            int min_reached = sent_this_iter + skip_this_iter < 50;
>              if ( (iter >= max_iters) ||
> -                 (sent_this_iter+skip_this_iter < 50) ||
> +                 min_reached ||
>                   (total_sent > dinfo->p2m_size*max_factor) )
>              {
> +                if ( !min_reached && abort_if_busy )
> +                {
> +                    ERROR("Live migration aborted, as requested. (guest too 
> busy?)"
> +                     " total_sent %lu iter %d, max_iters %u max_factor %u",
> +                      total_sent, iter, max_iters, max_factor);
> +                    rc = 1;
> +                    goto out;
> +                }
> +
>                  DPRINTF("Start last iteration\n");
>                  last_iter = 1;
> 
> diff -r 1ea501d60264 -r 29c66a248f5b tools/libxc/xenguest.h
> --- a/tools/libxc/xenguest.h
> +++ b/tools/libxc/xenguest.h
> @@ -28,6 +28,7 @@
>  #define XCFLAGS_HVM       (1 << 2)
>  #define XCFLAGS_STDVGA    (1 << 3)
>  #define XCFLAGS_CHECKPOINT_COMPRESS    (1 << 4)
> +#define XCFLAGS_DOMSAVE_ABORT_IF_BUSY  (1 << 5)
> 
>  #define X86_64_B_SIZE   64
>  #define X86_32_B_SIZE   32
> diff -r 1ea501d60264 -r 29c66a248f5b tools/libxl/Makefile
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -5,7 +5,7 @@
>  XEN_ROOT = $(CURDIR)/../..
>  include $(XEN_ROOT)/tools/Rules.mk
> 
> -MAJOR = 2.0
> +MAJOR = 2.1
>  MINOR = 0
> 
>  XLUMAJOR = 1.0
> diff -r 1ea501d60264 -r 29c66a248f5b tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -756,7 +756,8 @@ static void domain_suspend_cb(libxl__egc
> 
>  }
> 
> -int libxl_domain_suspend(libxl_ctx *ctx, uint32_t domid, int fd, int flags,
> +int libxl_domain_suspend(libxl_ctx *ctx, uint32_t domid, int fd,
> +                         const libxl_domain_suspend_properties *props,
>                           const libxl_asyncop_how *ao_how)
>  {
>      AO_CREATE(ctx, domid, ao_how);
> @@ -777,8 +778,13 @@ int libxl_domain_suspend(libxl_ctx *ctx,
>      dss->domid = domid;
>      dss->fd = fd;
>      dss->type = type;
> -    dss->live = flags & LIBXL_SUSPEND_LIVE;
> -    dss->debug = flags & LIBXL_SUSPEND_DEBUG;
> +    if (props) {
> +        dss->live = props->flags & LIBXL_SUSPEND_LIVE;
> +        dss->debug = props->flags & LIBXL_SUSPEND_DEBUG;
> +        dss->max_iters = props->max_iters;
> +        dss->max_factor = props->max_factor;
> +        dss->xlflags = props->flags;
> +    }

Do these things all get sane defaults if !props? Or is !props
disallowed? (in which case error return required)

> 
>      libxl__domain_suspend(egc, dss);
>      return AO_INPROGRESS;
> @@ -3769,13 +3778,17 @@ int main_migrate(int argc, char **argv)
>      char *rune = NULL;
>      char *host;
>      int opt, daemonize = 1, monitor = 1, debug = 0;
> +    int max_iters = 0, max_factor = 0, abort_if_busy = 0;
>      static struct option opts[] = {
>          {"debug", 0, 0, 0x100},
> +        {"max_iters", 1, 0, 'M'},
> +        {"max_factor", 1, 0, 'm'},

Wouldn't I or i and F or f be more descriptive than M and m (without
looking, tell me which is which? ;-) )

I wouldn't especially object if these were long only options, I expect
using them will be something only a tiny minority of users even think
of, unless some higher level entity does it for them (in which case the
length of the option is irrelevant). 


> +        {"abort_if_busy", 0, 0, 'A'},
>          COMMON_LONG_OPTS,
>          {0, 0, 0, 0}
>      };
[...]
> diff -r 1ea501d60264 -r 29c66a248f5b tools/libxl/xl_cmdtable.c
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -147,14 +147,19 @@ struct cmd_spec cmd_table[] = {
>        &main_migrate, 0, 1,
>        "Migrate a domain to another host",
>        "[options] <Domain> <host>",
> -      "-h              Print this help.\n"
> -      "-C <config>     Send <config> instead of config file from creation.\n"
> -      "-s <sshcommand> Use <sshcommand> instead of ssh.  String will be 
> passed\n"
> -      "                to sh. If empty, run <host> instead of ssh <host> 
> xl\n"
> -      "                migrate-receive [-d -e]\n"
> -      "-e              Do not wait in the background (on <host>) for the 
> death\n"
> -      "                of the domain.\n"
> -      "--debug         Print huge (!) amount of debug during the migration 
> process."
> +      "-h                   Print this help.\n"
> +      "-C <config>          Send <config> instead of config file from 
> creation.\n"
> +      "-s <sshcommand>      Use <sshcommand> instead of ssh.  String will be 
> passed\n"
> +      "                     to sh. If empty, run <host> instead of ssh 
> <host> xl\n"
> +      "                     migrate-receive [-d -e]\n"
> +      "-e                   Do not wait in the background (on <host>) for 
> the death\n"
> +      "                     of the domain.\n"
> +      "--debug              Print huge (!) amount of debug during the 
> migration process.\n"
> +      "-M <number>          Number of iterations before final suspend 
> (default: 30)\n"
> +      "--max_iters <number>\n"
> +      "-m <factor>          Max amount of memory to transfer before final 
> suspend (default: 3*RAM).\n"
> +      "--max_factor <factor>\n"
> +      "-A, --abort_if_busy  Abort migration instead of doing final suspend."

As with other similar locations the text of -A needs to say under what
conditions it aborts, I think.

>      },
>      { "dump-core",
>        &main_dump_core, 0, 1,
> diff -r 1ea501d60264 -r 29c66a248f5b tools/python/xen/xend/XendCheckpoint.py
> --- a/tools/python/xen/xend/XendCheckpoint.py
> +++ b/tools/python/xen/xend/XendCheckpoint.py

...



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.