Re: [PATCH v12 26/26] cmdline switches and config vars to control colo-proxy

From: Wei Liu <wei.liu2@citrix.com>
To: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
Cc: Lars Kurth <lars.kurth@citrix.com>,
	Li Zhijian <lizhijian@cn.fujitsu.com>,
	Wei Liu <wei.liu2@citrix.com>,
	Ian Campbell <ian.campbell@citrix.com>,
	Wen Congyang <wency@cn.fujitsu.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Jiang Yunhong <yunhong.jiang@intel.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>,
	xen devel <xen-devel@lists.xen.org>,
	Anthony Perard <anthony.perard@citrix.com>,
	Dong Eddie <eddie.dong@intel.com>,
	Gui Jianfeng <guijianfeng@cn.fujitsu.com>,
	Shriram Rajagopalan <rshriram@cs.ubc.ca>,
	Yang Hongyang <hongyang.yang@easystack.cn>
Subject: Re: [PATCH v12 26/26] cmdline switches and config vars to control colo-proxy
Date: Fri, 25 Mar 2016 12:29:33 +0000	[thread overview]
Message-ID: <20160325122933.GB20441@citrix.com> (raw)
In-Reply-To: <56F4D64F.2030108@cn.fujitsu.com>

On Fri, Mar 25, 2016 at 02:10:23PM +0800, Changlong Xie wrote:
> On 03/25/2016 12:12 AM, Ian Jackson wrote:
> >Changlong Xie writes ("[PATCH v12 26/26] cmdline switches and config vars to control colo-proxy"):
> >>From: Wen Congyang <wency@cn.fujitsu.com>
> >>
> >>Add cmdline switches to 'xl migrate-receive' command to specify
> >>a domain-specific hotplug script to setup COLO proxy.
> >
> >>+    if (nic->forwarddev) {
> >>+        flexarray_append(back, "forwarddev");
> >>+        flexarray_append(back, nic->forwarddev);
> >>+    }
> >
> >I'd prefer a name with `coloft' in it, throughout, even for the
> >xenstore node.  But again this is not critical if we are not trying to
> >make a stable API.
> >
> >>+    static struct option opts[] = {
> >>+        {"script", 1, 0, 0x100},
> >
> >This should surely be --coloft-script, or --colo-script, something.
> >
> >I think this script is (actually, in your setup) created by your
> >management software, and contains some actual functionality.  Am I
> >right ?  In which case my comments about including that functionality
> >in xen.git apply here too.  But again this is not a blocker for 4.7.
> 
> Also, I can't find your "comments about xxx".
> 

Ian's comment on 20/26:

    It is a shame that this management code is not also here.

    We would like to have enough management code in xen.git that we can
    introduce a COLO test in osstest.  That will ensure that your feature
    does not regress.

The above comment applies to the script in this patch, too. Ian is
asking you to upstream all relevant management code so that there is a
chance upstream tests COLO -- but that's not a blocker for 4.7.

As a side note, we should devise a plan to test COLO -- either do it in
OSStest or rely on third party testing. But that's a topic for another
day.

Wei.

> Thanks
> 	-Xie
> >
> >>-            xasprintf(&rune, "exec %s %s xl migrate-receive %s %s",
> >>-                      ssh_command, host,
> >>-                      libxl_defbool_val(r_info.colo) ? "-c" : "-r",
> >>-                      daemonize ? "" : " -e");
> >>+            if (!libxl_defbool_val(r_info.colo)) {
> >>+                xasprintf(&rune, "exec %s %s xl migrate-receive %s %s",
> >>+                          ssh_command, host,
> >>+                          "-r",
> >>+                          daemonize ? "" : " -e");
> >>+            } else {
> >>+                xasprintf(&rune, "exec %s %s xl migrate-receive %s %s %s %s",
> >>+                          ssh_command, host,
> >>+                          "-c",
> >>+                          r_info.netbufscript ? "--script" : "",
> >>+                          r_info.netbufscript ? r_info.netbufscript : "",
> >>+                          daemonize ? "" : " -e");
> >
> >I have just noticed here that you have introduced `-c' (in an earlier
> >patch) to mean to receive a COLO checkpointed stream.
> >
> >Can you please change this to `--colo' ?
> >
> >Sorry,
> >Ian.
> >
> >
> >.
> >
> 
> 

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