| Message ID | 1458720400-4699-27-git-send-email-xiecl.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
> - 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.
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 prefer "--coloft-script" > 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 Yes > in xen.git apply here too. But again this is not a blocker for 4.7. > >> - 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' ? > Will replace "-c" with "--colo" Thanks -Xie > Sorry, > Ian. > > > . >
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". 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. > > > . >
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. > > > > > >. > > > >
On 03/25/2016 08:29 PM, Wei Liu wrote: > 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 Yes, we can talk about it later Thanks -Xie > 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. >>> >>> >>> . >>> >> >> > > > . >
diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5 index 8ae19bb..8f7fd28 100644 --- a/docs/man/xl.conf.pod.5 +++ b/docs/man/xl.conf.pod.5 @@ -111,6 +111,12 @@ Configures the default script used by Remus to setup network buffering. Default: C</etc/xen/scripts/remus-netbuf-setup> +=item B<colo.default.proxyscript="PATH"> + +Configures the default script used by COLO to setup colo-proxy. + +Default: C</etc/xen/scripts/colo-proxy-setup> + =item B<output_format="json|sxp"> Configures the default output format used by xl when printing "machine diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1 index 16ebfae..3977d4b 100644 --- a/docs/man/xl.pod.1 +++ b/docs/man/xl.pod.1 @@ -458,8 +458,6 @@ Remus support in xl is still in experimental (proof-of-concept) phase. Disk replication support is limited to DRBD disks. COLO support in xl is still in experimental (proof-of-concept) phase. -There is no support for network, so the guest will confuse its network -peers at the moment. =back @@ -483,6 +481,11 @@ and it's used by secondary. =item B<hidden-disk> :Primary's modified contents will be buffered in this disk, and it's used by secondary. +(b) An example for COLO network configuration: vif =[ '...,forwarddev=xxx,...'] + +=item B<forwarddev> :Forward devices for primary and secondary, there are +directly connected. + =back B<OPTIONS> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 5482219..b70d8b0 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -3379,6 +3379,11 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid, flexarray_append(back, nic->ifname); } + if (nic->forwarddev) { + flexarray_append(back, "forwarddev"); + flexarray_append(back, nic->forwarddev); + } + flexarray_append(back, "mac"); flexarray_append(back,GCSPRINTF(LIBXL_MAC_FMT, LIBXL_MAC_BYTES(nic->mac))); if (nic->ip) { @@ -3501,6 +3506,7 @@ static int libxl__device_nic_from_xs_be(libxl__gc *gc, nic->ip = READ_BACKEND(NOGC, "ip"); nic->bridge = READ_BACKEND(NOGC, "bridge"); nic->script = READ_BACKEND(NOGC, "script"); + nic->forwarddev = READ_BACKEND(NOGC, "forwarddev"); /* vif_ioemu nics use the same xenstore entries as vif interfaces */ tmp = READ_BACKEND(gc, "type"); diff --git a/tools/libxl/libxl_colo_restore.c b/tools/libxl/libxl_colo_restore.c index c8ad796..3483f39 100644 --- a/tools/libxl/libxl_colo_restore.c +++ b/tools/libxl/libxl_colo_restore.c @@ -233,6 +233,11 @@ void libxl__colo_restore_setup(libxl__egc *egc, crcs->crs = crs; crs->qdisk_setuped = false; crs->qdisk_used = false; + if (dcs->colo_proxy_script) + crs->colo_proxy_script = libxl__strdup(gc, dcs->colo_proxy_script); + else + crs->colo_proxy_script = GCSPRINTF("%s/colo-proxy-setup", + libxl__xen_script_dir_path()); /* setup dsps */ crcs->dsps.ao = ao; diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 6a96a7d..df83849 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -1621,6 +1621,7 @@ static void domain_create_cb(libxl__egc *egc, static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config, uint32_t *domid, int restore_fd, int send_back_fd, const libxl_domain_restore_params *params, + const char *colo_proxy_script, const libxl_asyncop_how *ao_how, const libxl_asyncprogress_how *aop_console_how) { @@ -1644,6 +1645,7 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config, } cdcs->dcs.callback = domain_create_cb; cdcs->dcs.domid_soft_reset = INVALID_DOMID; + cdcs->dcs.colo_proxy_script = colo_proxy_script; libxl__ao_progress_gethow(&cdcs->dcs.aop_console_how, aop_console_how); cdcs->domid_out = domid; @@ -1831,7 +1833,7 @@ int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config, const libxl_asyncprogress_how *aop_console_how) { unset_disk_colo_restore(d_config); - return do_domain_create(ctx, d_config, domid, -1, -1, NULL, + return do_domain_create(ctx, d_config, domid, -1, -1, NULL, NULL, ao_how, aop_console_how); } @@ -1842,14 +1844,17 @@ int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config, const libxl_asyncop_how *ao_how, const libxl_asyncprogress_how *aop_console_how) { + char *colo_proxy_script = NULL; + if (params->checkpointed_stream == LIBXL_CHECKPOINTED_STREAM_COLO) { + colo_proxy_script = params->colo_proxy_script; set_disk_colo_restore(d_config); } else { unset_disk_colo_restore(d_config); } return do_domain_create(ctx, d_config, domid, restore_fd, send_back_fd, - params, ao_how, aop_console_how); + params, colo_proxy_script, ao_how, aop_console_how); } int libxl_domain_soft_reset(libxl_ctx *ctx, diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index a8be078..bdbf0bc 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -385,6 +385,7 @@ libxl_domain_create_info = Struct("domain_create_info",[ libxl_domain_restore_params = Struct("domain_restore_params", [ ("checkpointed_stream", integer), ("stream_version", uint32, {'init_val': '1'}), + ("colo_proxy_script", string), ]) libxl_domain_sched_params = Struct("domain_sched_params",[ diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c index dfae84a..a272258 100644 --- a/tools/libxl/xl.c +++ b/tools/libxl/xl.c @@ -45,6 +45,7 @@ char *default_bridge = NULL; char *default_gatewaydev = NULL; char *default_vifbackend = NULL; char *default_remus_netbufscript = NULL; +char *default_colo_proxy_script = NULL; enum output_format default_output_format = OUTPUT_FORMAT_JSON; int claim_mode = 1; bool progress_use_cr = 0; @@ -179,6 +180,8 @@ static void parse_global_config(const char *configfile, xlu_cfg_replace_string (config, "remus.default.netbufscript", &default_remus_netbufscript, 0); + xlu_cfg_replace_string (config, "colo.default.proxyscript", + &default_colo_proxy_script, 0); xlu_cfg_destroy(config); } diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h index bdab125..709c3fd 100644 --- a/tools/libxl/xl.h +++ b/tools/libxl/xl.h @@ -189,6 +189,7 @@ extern char *default_bridge; extern char *default_gatewaydev; extern char *default_vifbackend; extern char *default_remus_netbufscript; +extern char *default_colo_proxy_script; extern char *blkdev_start; enum output_format { diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index 30d26bc..3fa5cd9 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -158,6 +158,7 @@ struct domain_create { const char *config_file; char *extra_config; /* extra config string */ const char *restore_file; + char *colo_proxy_script; int migrate_fd; /* -1 means none */ int send_back_fd; /* -1 means none */ char **migration_domname_r; /* from malloc */ @@ -1053,6 +1054,8 @@ static int parse_nic_config(libxl_device_nic *nic, XLU_Config **config, char *to replace_string(&nic->model, oparg); } else if (MATCH_OPTION("rate", token, oparg)) { parse_vif_rate(config, oparg, nic); + } else if (MATCH_OPTION("forwarddev", token, oparg)) { + replace_string(&nic->forwarddev, oparg); } else if (MATCH_OPTION("accel", token, oparg)) { fprintf(stderr, "the accel parameter for vifs is currently not supported\n"); } else { @@ -2896,6 +2899,7 @@ start: params.checkpointed_stream = dom_info->checkpointed_stream; params.stream_version = (hdr.mandatory_flags & XL_MANDATORY_FLAG_STREAMv2) ? 2 : 1; + params.colo_proxy_script = dom_info->colo_proxy_script; ret = libxl_domain_create_restore(ctx, &d_config, &domid, restore_fd, @@ -4438,7 +4442,8 @@ static void migrate_domain(uint32_t domid, const char *rune, int debug, static void migrate_receive(int debug, int daemonize, int monitor, int send_fd, int recv_fd, - libxl_checkpointed_stream checkpointed) + libxl_checkpointed_stream checkpointed, + char *colo_proxy_script) { uint32_t domid; int rc, rc2; @@ -4467,6 +4472,7 @@ static void migrate_receive(int debug, int daemonize, int monitor, dom_info.send_back_fd = send_fd; dom_info.migration_domname_r = &migration_domname; dom_info.checkpointed_stream = checkpointed; + dom_info.colo_proxy_script = colo_proxy_script; rc = create_domain(&dom_info); if (rc < 0) { @@ -4660,8 +4666,13 @@ int main_migrate_receive(int argc, char **argv) int debug = 0, daemonize = 1, monitor = 1; libxl_checkpointed_stream checkpointed = LIBXL_CHECKPOINTED_STREAM_NONE; int opt; + static struct option opts[] = { + {"script", 1, 0, 0x100}, + COMMON_LONG_OPTS + }; + char *script = NULL; - SWITCH_FOREACH_OPT(opt, "Fedrc", NULL, "migrate-receive", 0) { + SWITCH_FOREACH_OPT(opt, "Fedrc", opts, "migrate-receive", 0) { case 'F': daemonize = 0; break; @@ -4678,6 +4689,9 @@ int main_migrate_receive(int argc, char **argv) case 'c': checkpointed = LIBXL_CHECKPOINTED_STREAM_COLO; break; + case 0x100: + script = optarg; + break; } if (argc-optind != 0) { @@ -4686,7 +4700,7 @@ int main_migrate_receive(int argc, char **argv) } migrate_receive(debug, daemonize, monitor, STDOUT_FILENO, STDIN_FILENO, - checkpointed); + checkpointed, script); return 0; } @@ -8096,8 +8110,10 @@ int main_remus(int argc, char **argv) r_info.interval = 200; if (libxl_defbool_val(r_info.colo)) { - if (r_info.interval || libxl_defbool_val(r_info.blackhole)) { - perror("Option -c conflicts with -i or -b"); + if (r_info.interval || libxl_defbool_val(r_info.blackhole) || + !libxl_defbool_is_default(r_info.netbuf) || + !libxl_defbool_is_default(r_info.diskbuf)) { + perror("option -c is conflict with -i, -d, -n or -b"); exit(-1); } @@ -8108,8 +8124,12 @@ int main_remus(int argc, char **argv) } } - if (!r_info.netbufscript) - r_info.netbufscript = default_remus_netbufscript; + if (!r_info.netbufscript) { + if (libxl_defbool_val(r_info.colo)) + r_info.netbufscript = default_colo_proxy_script; + else + r_info.netbufscript = default_remus_netbufscript; + } if (libxl_defbool_val(r_info.blackhole)) { send_fd = open("/dev/null", O_RDWR, 0644); @@ -8122,10 +8142,19 @@ int main_remus(int argc, char **argv) if (!ssh_command[0]) { rune = host; } else { - 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"); + } } save_domain_core_begin(domid, NULL, &config_data, &config_len);