Re: [PATCH for-4.5 v20 08/12] xl/remus: cmdline switch to explicitly enable unsafe configurations

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Sep 25, 2014 at 02:16:20PM +0800, Yang Hongyang wrote:
> By default, network buffering and disk replication are enabled;
> checkpoints are replicated to another standby VM.
> 
> This patch allows the user to disable any of these features by
> explicitly specifying a 'run in unsafe mode' switch when invoking
> the 'xl remus' command.  While running Remus in an unsafe mode
> makes little sense under normal circumstances, it is useful to be
> able to disable one or more features mentioned above for
> testing/debugging/profiling purposes.
> 
> Unless this option is enabled, it will not be possible to
> replicate memory checkpoints to /dev/null (blackhole replication),
> disable network buffering or disk replication.
> 
> As a starter, the use of blackhole replication now requires that
> the unsafe mode be enabled. Subsequent patches will add support
> for disabling network buffering and disk replication in a similar
> manner.
> 
> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
> Signed-off-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  docs/man/xl.pod.1           | 15 ++++++++++-----
>  tools/libxl/libxl.c         |  7 +++++++
>  tools/libxl/libxl_types.idl |  1 +
>  tools/libxl/xl_cmdimpl.c    |  5 ++++-
>  tools/libxl/xl_cmdtable.c   |  7 +++++--
>  5 files changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index f9bc812..2ae3007 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -446,11 +446,6 @@ B<OPTIONS>
>  
>  Checkpoint domain memory every MS milliseconds (default 200ms).
>  
> -=item B<-b>
> -
> -Replicate memory checkpoints to /dev/null (blackhole).
> -Generally useful for debugging.
> -
>  =item B<-u>
>  
>  Disable memory checkpoint compression.
> @@ -465,6 +460,16 @@ If empty, run <host> instead of ssh <host> xl migrate-receive -r [-e].
>  On the new host, do not wait in the background (on <host>) for the death
>  of the domain. See the corresponding option of the I<create> subcommand.
>  
> +=item B<-F>
> +
> +Run Remus in unsafe mode. Use this option with caution as failover may
> +not work as intended.
> +
> +=item B<-b>
> +
> +Replicate memory checkpoints to /dev/null (blackhole).
> +Generally useful for debugging. Requires enabling unsafe mode.
> +
>  =back
>  
>  =item B<pause> I<domain-id>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 9e0a800..cc5c3ac 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -804,9 +804,16 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
>          goto out;
>      }
>  
> +    libxl_defbool_setdefault(&info->unsafe, false);
>      libxl_defbool_setdefault(&info->blackhole, false);
>      libxl_defbool_setdefault(&info->compression, true);
>  
> +    if (!libxl_defbool_val(info->unsafe) &&
> +        libxl_defbool_val(info->blackhole)) {
> +        LOG(ERROR, "Unsafe mode must be enabled to replicate to /dev/null");
> +        goto out;
> +    }
> +
>      GCNEW(dss);
>      dss->ao = ao;
>      dss->callback = remus_failover_cb;
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 16e374f..348f794 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -611,6 +611,7 @@ libxl_sched_credit_params = Struct("sched_credit_params", [
>  
>  libxl_domain_remus_info = Struct("domain_remus_info",[
>      ("interval",     integer),
> +    ("unsafe",       libxl_defbool),
>      ("blackhole",    libxl_defbool),
>      ("compression",  libxl_defbool),
>      ])
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index e9e8900..3463d45 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -7497,10 +7497,13 @@ int main_remus(int argc, char **argv)
>      r_info.interval = 200;
>      libxl_defbool_setdefault(&r_info.blackhole, false);
>  
> -    SWITCH_FOREACH_OPT(opt, "bui:s:e", NULL, "remus", 2) {
> +    SWITCH_FOREACH_OPT(opt, "Fbui:s:e", NULL, "remus", 2) {
>      case 'i':
>          r_info.interval = atoi(optarg);
>          break;
> +    case 'F':
> +        libxl_defbool_set(&r_info.unsafe, true);
> +        break;
>      case 'b':
>          libxl_defbool_set(&r_info.blackhole, true);
>          break;
> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index dd15947..08f3c90 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -495,13 +495,16 @@ struct cmd_spec cmd_table[] = {
>        "Enable Remus HA for domain",
>        "[options] <Domain> [<host>]",
>        "-i MS                   Checkpoint domain memory every MS milliseconds (def. 200ms).\n"
> -      "-b                      Replicate memory checkpoints to /dev/null (blackhole)\n"
>        "-u                      Disable memory checkpoint compression.\n"
>        "-s <sshcommand>         Use <sshcommand> instead of ssh.  String will be passed\n"
>        "                        to sh. If empty, run <host> instead of \n"
>        "                        ssh <host> xl migrate-receive -r [-e]\n"
>        "-e                      Do not wait in the background (on <host>) for the death\n"
> -      "                        of the domain."
> +      "                        of the domain.\n"
> +      "-F                      Enable unsafe configurations [-b flags]. Use this option\n"
> +      "                        with caution as failover may not work as intended.\n"
> +      "-b                      Replicate memory checkpoints to /dev/null (blackhole).\n"
> +      "                        Works only in unsafe mode."
>      },
>  #endif
>      { "devd",
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel