Re: [PATCH RFC 1/6] COLO-Proxy: Add remus command to open userspace proxy

From: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
To: Wei Liu <wei.liu2@citrix.com>
Cc: Li Zhijian <lizhijian@cn.fujitsu.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>,
	"eddie . dong" <eddie.dong@intel.com>,
	Yang Hongyang <imhy.yang@gmail.com>,
	Xen devel <xen-devel@lists.xenproject.org>,
	Bian Naimeng <biannm@cn.fujitsu.com>
Subject: Re: [PATCH RFC 1/6] COLO-Proxy: Add remus command to open userspace proxy
Date: Mon, 6 Feb 2017 14:01:30 +0800	[thread overview]
Message-ID: <f8c686e8-b9e3-b188-c4f4-aea4a1a4478c@cn.fujitsu.com> (raw)
In-Reply-To: <20170127170501.hptjghpvgcn4oa4c@citrix.com>

On 01/28/2017 01:05 AM, Wei Liu wrote:
> On Thu, Jan 26, 2017 at 02:36:04PM +0800, Zhang Chen wrote:
>> Add remus '-p' to enable userspace colo proxy(in qemu).
>>
>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>> ---
>>   docs/man/xl.pod.1.in          |  4 ++++
>>   tools/libxl/libxl_colo.h      |  5 +++++
>>   tools/libxl/libxl_colo_save.c |  2 ++
>>   tools/libxl/libxl_types.idl   | 17 +++++++++--------
>>   tools/libxl/xl_cmdimpl.c      | 13 ++++++++++++-
>>   tools/libxl/xl_cmdtable.c     |  3 ++-
>>   6 files changed, 34 insertions(+), 10 deletions(-)
>>
>> diff --git a/docs/man/xl.pod.1.in b/docs/man/xl.pod.1.in
>> index 09c1faa..b5fb7c1 100644
>> --- a/docs/man/xl.pod.1.in
>> +++ b/docs/man/xl.pod.1.in
>> @@ -553,6 +553,10 @@ Disable disk replication. Requires enabling unsafe mode.
>>   Enable COLO HA. This conflicts with B<-i> and B<-b>, and memory
>>   checkpoint compression must be disabled.
>>   
>> +=item B<-p>
>> +
>> +Enable userspace COLO Proxy. Must open with B<-c>.
>> +
> Use userspace COLO Proxy. This option must be used in conjunction with B<-c>.

OK.

>
>>   =back
>>   
>>   =item B<pause> I<domain-id>
>> diff --git a/tools/libxl/libxl_colo.h b/tools/libxl/libxl_colo.h
>> index 682275c..4746d8c 100644
>> --- a/tools/libxl/libxl_colo.h
>> +++ b/tools/libxl/libxl_colo.h
>> @@ -64,6 +64,11 @@ struct libxl__colo_proxy_state {
>>   
>>       int sock_fd;
>>       int index;
>> +    /*
>> +     * Private, True means use userspace colo proxy
>> +     *          False means use kernel colo proxy.
>> +     */
>> +    bool is_userspace_proxy;
>>   };
>>   
>>   struct libxl__colo_save_state {
>> diff --git a/tools/libxl/libxl_colo_save.c b/tools/libxl/libxl_colo_save.c
>> index 6277770..eb8336c 100644
>> --- a/tools/libxl/libxl_colo_save.c
>> +++ b/tools/libxl/libxl_colo_save.c
>> @@ -101,6 +101,8 @@ void libxl__colo_save_setup(libxl__egc *egc, libxl__colo_save_state *css)
>>       css->qdisk_setuped = false;
>>       css->qdisk_used = false;
>>       libxl__ev_child_init(&css->child);
>> +    css->cps.is_userspace_proxy =
>> +        libxl_defbool_val(dss->remus->userspace_colo_proxy);
>>   
>>       if (dss->remus->netbufscript)
>>           css->colo_proxy_script = libxl__strdup(gc, dss->remus->netbufscript);
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index a612d1f..1bd2057 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -844,14 +844,15 @@ libxl_sched_credit2_params = Struct("sched_credit2_params", [
>>       ], dispose_fn=None)
>>   
>>   libxl_domain_remus_info = Struct("domain_remus_info",[
>> -    ("interval",     integer),
>> -    ("allow_unsafe", libxl_defbool),
>> -    ("blackhole",    libxl_defbool),
>> -    ("compression",  libxl_defbool),
>> -    ("netbuf",       libxl_defbool),
>> -    ("netbufscript", string),
>> -    ("diskbuf",      libxl_defbool),
>> -    ("colo",         libxl_defbool)
>> +    ("interval",             integer),
>> +    ("allow_unsafe",         libxl_defbool),
>> +    ("blackhole",            libxl_defbool),
>> +    ("compression",          libxl_defbool),
>> +    ("netbuf",               libxl_defbool),
>> +    ("netbufscript",         string),
>> +    ("diskbuf",              libxl_defbool),
>> +    ("colo",                 libxl_defbool),
>> +    ("userspace_colo_proxy", libxl_defbool)
> Please add a LIBXL_HAVE macro in libxl.h.

I will add #define LIBXL_HAVE_USERSPACE_COLO_PROXY 1 in libxl.h
In next version.

>
>>       ])
>>   
>>   libxl_event_type = Enumeration("event_type", [
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 7e8a8ae..905c5f6 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -8893,7 +8893,7 @@ int main_remus(int argc, char **argv)
>>   
>>       memset(&r_info, 0, sizeof(libxl_domain_remus_info));
>>   
>> -    SWITCH_FOREACH_OPT(opt, "Fbundi:s:N:ec", NULL, "remus", 2) {
>> +    SWITCH_FOREACH_OPT(opt, "Fbundi:s:N:ecp", NULL, "remus", 2) {
>>       case 'i':
>>           r_info.interval = atoi(optarg);
>>           break;
>> @@ -8923,6 +8923,9 @@ int main_remus(int argc, char **argv)
>>           break;
>>       case 'c':
>>           libxl_defbool_set(&r_info.colo, true);
>> +        break;
>> +    case 'p':
>> +        libxl_defbool_set(&r_info.userspace_colo_proxy, true);
>>       }
>>   
>>       domid = find_domain(argv[optind]);
>> @@ -8931,9 +8934,17 @@ int main_remus(int argc, char **argv)
>>       /* Defaults */
>>       libxl_defbool_setdefault(&r_info.blackhole, false);
>>       libxl_defbool_setdefault(&r_info.colo, false);
>> +    libxl_defbool_setdefault(&r_info.userspace_colo_proxy, false);
>> +
> Hmm... I think setting defaults should be pushed into libxl.
>
> But I think this is issue is orthogonal to this patch, and we can
> revisit this later.
>
>>       if (!libxl_defbool_val(r_info.colo) && !r_info.interval)
>>           r_info.interval = 200;
>>   
>> +    if (libxl_defbool_val(r_info.userspace_colo_proxy) &&
>> +        !libxl_defbool_val(r_info.colo)) {
>> +        perror("option -p must open with -c");
> "Option -p must be used in conjunction with -c".
>
> And please use fprintf(stderr,...) here because libxl_defbool_val
> doesn't touch errno.

OK, I will fix this in next version.

>
>> +        exit(-1);
>> +    }
>> +
>>       if (libxl_defbool_val(r_info.colo)) {
>>           if (r_info.interval || libxl_defbool_val(r_info.blackhole) ||
>>               !libxl_defbool_is_default(r_info.netbuf) ||
>> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
>> index 588d5d9..a124059 100644
>> --- a/tools/libxl/xl_cmdtable.c
>> +++ b/tools/libxl/xl_cmdtable.c
>> @@ -506,7 +506,8 @@ struct cmd_spec cmd_table[] = {
>>         "-n                      Disable network output buffering. Works only in unsafe mode.\n"
>>         "-d                      Disable disk replication. Works only in unsafe mode.\n"
>>         "-c                      Enable COLO HA. It is conflict with -i and -b, and memory\n"
>> -      "                        checkpoint must be disabled"
>> +      "                        checkpoint must be disabled.\n"
>> +      "-p                      Enable COLO userspace proxy."
> "Use".

OK.

Thanks
Zhang Chen

>
> Wei.
>
>>       },
>>   #endif
>>       { "devd",
>> -- 
>> 2.7.4
>>
>>
>>
>
> .
>

-- 
Thanks
Zhang Chen

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