On 06/05/2012 02:36 AM, Guannan Ren wrote: > Bugzilla:https://bugzilla.redhat.com/show_bug.cgi?id=822839 > add two general virsh options to support keepalive message protocol > > -i | --keepalive_interval interval time value (default 5 seconds) > -n | --keepalive_count number of heartbeats (default 5 times) > > For non-p2p migration, start keepalive for remote driver to > determine the status of network connection, aborting migrating job > after defined amount of interval time. > --- > tools/virsh.c | 88 +++++++++++++++++++++++++++++++++++++++++++++----------- > 1 files changed, 70 insertions(+), 18 deletions(-)
Missing a counterpart documentation in virsh.pod.
> @@ -7213,6 +7219,12 @@ doMigrate (void *opaque)
> dconn = virConnectOpenAuth (desturi, virConnectAuthPtrDefault, 0);
> if (!dconn) goto out;
>
> + data->dconn = dconn;
> + if (virConnectSetKeepAlive(dconn,
> + ctl->keepalive_interval,
> + ctl->keepalive_count) < 0)
> + vshDebug(ctl, VSH_ERR_WARNING, "migrate: Failed to start
> keepalive\n");
This makes it impossible to migrate to an older server that lacks
virConnectSetKeepAlive() support. You need to avoid doing this if
keepalive_interval and/or keepalive_count is set to a value that avoids
keepalive. I think that also means you need to distinguish between a
normal default of 5 seconds with new servers but unspecified by the user
(where we just gracefully continue without keepalive), compared to an
explicit user request of 5 seconds (must fail if the keepalive request
cannot be honored), and compared to an explicit user request of no
keepalive.
> @@ -7329,6 +7342,13 @@ repoll:
> goto cleanup;
> }
>
> + if (data->dconn && virConnectIsAlive(data->dconn) <= 0) {
> + virDomainAbortJob(dom);
> + vshError(ctl, "%s",
> + _("Lost connection to destination host"));
> + goto cleanup;
> + }
Again, need to be careful of older servers that lack virConnectIsAlive()
support.
> @@ -18681,6 +18703,18 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd)
> if (enable_timing)
> GETTIMEOFDAY(&before);
>
> + /* start keepalive for remote driver */
> + driver = virConnectGetType(ctl->conn);
> + if (STREQ_NULLABLE(driver, "QEMU") ||
> + STREQ_NULLABLE(driver, "xenlight") ||
> + STREQ_NULLABLE(driver, "UML") ||
> + STREQ_NULLABLE(driver, "LXC") ||
> + STREQ_NULLABLE(driver, "remote"))
Why are you limiting this to particular hypervisors? That feels wrong.
Rather, you should blindly attempt it, and gracefully detect when it is
unsupported.
> @@ -19959,17 +19993,19 @@ vshUsage(void)
> fprintf(stdout, _("\n%s [options]... [<command_string>]"
> "\n%s [options]... <command> [args...]\n\n"
> " options:\n"
> - " -c | --connect=URI hypervisor connection
> URI\n"
> + " -c | --connect=URI hypervisor connection
> URI\n"
Rather than reindenting everything, I would just alter your new lines to
use multiple lines:
> + " -i | --keepalive_interval interval time value
> (default 5 seconds)\n"
> + " -n | --keepalive_count number of heartbeats
> (default 5 times)\n\n"
-c | --connect=URI hypervisor connection URI
-i | --keepalive_interval
interval time value (default 5 seconds)
>
> /* Standard (non-command) options. The leading + ensures that no
> * argument reordering takes place, so that command options are
> * not confused with top-level virsh options. */
> - while ((arg = getopt_long(argc, argv, "+d:hqtc:vVrl:e:", opt, NULL)) !=
> -1) {
> + while ((arg = getopt_long(argc, argv, "+d:hqtc:vVrl:e:i:n:", opt, NULL))
> != -1) {
Pre-existing, but I would welcome an independent patch that cleaned this
up into alphabetic ordering to make it easier to find code for a given
option now that we have more options to deal with.
Looking forward to v3.
--
Eric Blake [email protected] +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
