[libvirt] [PATCH 0/4] Add keepalive support in virsh
*** BLURB *** Martin Kletzander (4): virsh: Sort options alphabetically virsh: Add keepalive in new vshConnect function virsh: Prohibit virConnectOpen* functions in virsh [DO_NOT_APPLY_UPSTREAM] virsh: add connection monitoring into vshWatchJob cfg.mk | 8 ++- tests/virsh-optparse | 3 +- tools/virsh-domain.c | 21 +++++--- tools/virsh.c | 141 +++++++++++++++++++++++++++++++++++++-------------- tools/virsh.h | 7 +++ tools/virsh.pod | 38 +++++++------- 6 files changed, 152 insertions(+), 66 deletions(-) -- 1.9.0
Man page, help output and also parsing is sorted in order to find options smoothly. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/virsh.c | 64 ++++++++++++++++++++++++++++----------------------------- tools/virsh.pod | 38 +++++++++++++++++----------------- 2 files changed, 51 insertions(+), 51 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 2d4aaff..9b8429f 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3110,16 +3110,16 @@ vshUsage(void) "\n%s [options]... <command> [args...]\n\n" " options:\n" " -c | --connect=URI hypervisor connection URI\n" - " -r | --readonly connect readonly\n" " -d | --debug=NUM debug level [0-4]\n" + " -e | --escape <char> set escape sequence for console\n" " -h | --help this help\n" + " -l | --log=FILE output logging to file\n" " -q | --quiet quiet mode\n" + " -r | --readonly connect readonly\n" " -t | --timing print timing information\n" - " -l | --log=FILE output logging to file\n" " -v short version\n" " -V long version\n" " --version[=TYPE] version, TYPE is short or long (default short)\n" - " -e | --escape <char> set escape sequence for console\n\n" " commands (non interactive mode):\n\n"), progname, progname); for (grp = cmdGroups; grp->name; grp++) { @@ -3306,23 +3306,27 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) size_t i; int longindex = -1; struct option opt[] = { + {"connect", required_argument, NULL, 'c'}, {"debug", required_argument, NULL, 'd'}, + {"escape", required_argument, NULL, 'e'}, {"help", no_argument, NULL, 'h'}, + {"log", required_argument, NULL, 'l'}, {"quiet", no_argument, NULL, 'q'}, + {"readonly", no_argument, NULL, 'r'}, {"timing", no_argument, NULL, 't'}, {"version", optional_argument, NULL, 'v'}, - {"connect", required_argument, NULL, 'c'}, - {"readonly", no_argument, NULL, 'r'}, - {"log", required_argument, NULL, 'l'}, - {"escape", required_argument, NULL, 'e'}, {NULL, 0, NULL, 0} }; /* 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, &longindex)) != -1) { + while ((arg = getopt_long(argc, argv, "+:c:d:e:hl:qrtvV", opt, &longindex)) != -1) { switch (arg) { + case 'c': + VIR_FREE(ctl->name); + ctl->name = vshStrdup(ctl, optarg); + break; case 'd': if (virStrToLong_i(optarg, NULL, 10, &debug) < 0) { vshError(ctl, _("option %s takes a numeric argument"), @@ -3335,19 +3339,36 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) else ctl->debug = debug; break; + case 'e': + len = strlen(optarg); + + if ((len == 2 && *optarg == '^' && + vshAllowedEscapeChar(optarg[1])) || + (len == 1 && *optarg != '^')) { + ctl->escapeChar = optarg; + } else { + vshError(ctl, _("Invalid string '%s' for escape sequence"), + optarg); + exit(EXIT_FAILURE); + } + break; case 'h': vshUsage(); exit(EXIT_SUCCESS); break; + case 'l': + vshCloseLogFile(ctl); + ctl->logfile = vshStrdup(ctl, optarg); + vshOpenLogFile(ctl); + break; case 'q': ctl->quiet = true; break; case 't': ctl->timing = true; break; - case 'c': - VIR_FREE(ctl->name); - ctl->name = vshStrdup(ctl, optarg); + case 'r': + ctl->readonly = true; break; case 'v': if (STRNEQ_NULLABLE(optarg, "long")) { @@ -3358,27 +3379,6 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) case 'V': vshShowVersion(ctl); exit(EXIT_SUCCESS); - case 'r': - ctl->readonly = true; - break; - case 'l': - vshCloseLogFile(ctl); - ctl->logfile = vshStrdup(ctl, optarg); - vshOpenLogFile(ctl); - break; - case 'e': - len = strlen(optarg); - - if ((len == 2 && *optarg == '^' && - vshAllowedEscapeChar(optarg[1])) || - (len == 1 && *optarg != '^')) { - ctl->escapeChar = optarg; - } else { - vshError(ctl, _("Invalid string '%s' for escape sequence"), - optarg); - exit(EXIT_FAILURE); - } - break; case ':': for (i = 0; opt[i].name != NULL; i++) { if (opt[i].val == optopt) diff --git a/tools/virsh.pod b/tools/virsh.pod index cafbb9a..cefce1b 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -52,21 +52,6 @@ The B<virsh> program understands the following I<OPTIONS>. =over 4 -=item B<-h>, B<--help> - -Ignore all other arguments, and behave as if the B<help> command were -given instead. - -=item B<-v>, B<--version[=short]> - -Ignore all other arguments, and prints the version of the libvirt library -virsh is coming from - -=item B<-V>, B<--version=long> - -Ignore all other arguments, and prints the version of the libvirt library -virsh is coming from and which options and driver are compiled in. - =item B<-c>, B<--connect> I<URI> Connect to the specified I<URI>, as if by the B<connect> command, @@ -78,6 +63,17 @@ Enable debug messages at integer I<LEVEL> and above. I<LEVEL> can range from 0 to 4 (default). See the documentation of B<VIRSH_DEBUG> environment variable below for the description of each I<LEVEL>. +=item B<-e>, B<--escape> I<string> + +Set alternative escape sequence for I<console> command. By default, +telnet's B<^]> is used. Allowed characters when using hat notation are: +alphabetic character, @, [, ], \, ^, _. + +=item B<-h>, B<--help> + +Ignore all other arguments, and behave as if the B<help> command were +given instead. + =item B<-l>, B<--log> I<FILE> Output logging details to I<FILE>. @@ -95,11 +91,15 @@ option of the B<connect> command. Output elapsed time information for each command. -=item B<-e>, B<--escape> I<string> +=item B<-v>, B<--version[=short]> -Set alternative escape sequence for I<console> command. By default, -telnet's B<^]> is used. Allowed characters when using hat notation are: -alphabetic character, @, [, ], \, ^, _. +Ignore all other arguments, and prints the version of the libvirt library +virsh is coming from + +=item B<-V>, B<--version=long> + +Ignore all other arguments, and prints the version of the libvirt library +virsh is coming from and which options and driver are compiled in. =back -- 1.9.0
On 07.03.2014 11:49, Martin Kletzander wrote:
Man page, help output and also parsing is sorted in order to find options smoothly.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/virsh.c | 64 ++++++++++++++++++++++++++++----------------------------- tools/virsh.pod | 38 +++++++++++++++++----------------- 2 files changed, 51 insertions(+), 51 deletions(-)
ACK Michal
On Fri, Mar 07, 2014 at 04:50:55PM +0100, Michal Privoznik wrote:
On 07.03.2014 11:49, Martin Kletzander wrote:
Man page, help output and also parsing is sorted in order to find options smoothly.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/virsh.c | 64 ++++++++++++++++++++++++++++----------------------------- tools/virsh.pod | 38 +++++++++++++++++----------------- 2 files changed, 51 insertions(+), 51 deletions(-)
ACK
Michal
Thanks, pushed. Martin
Introducing keepalive similarly to Guannan around 2 years ago. Since we want to introduce keepalive for every connection, it makes sense to wrap the connecting function into new virsh one that can deal keepalive as well. Function vshConnect() is now used for connecting and keepalive added in that function (if possible) helps preventing long waits e.g. while nework goes down during migration. This patch also adds the options for keepalive tuning into virsh and fails connecting only when keepalives are explicitly requested and cannot be set (whether it is due to missing support in connected driver or remote server). If not explicitely requested, a debug message is printed (hence the addition to virsh-optparse test). Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1073506 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=822839 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/virsh-optparse | 3 +- tools/virsh-domain.c | 2 +- tools/virsh.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++----- tools/virsh.h | 5 ++++ 4 files changed, 78 insertions(+), 9 deletions(-) diff --git a/tests/virsh-optparse b/tests/virsh-optparse index 214ae41..7d4c699 100755 --- a/tests/virsh-optparse +++ b/tests/virsh-optparse @@ -1,7 +1,7 @@ #!/bin/sh # Ensure that virsh option parsing doesn't regress -# Copyright (C) 2011-2012 Red Hat, Inc. +# Copyright (C) 2011-2012, 2014 Red Hat, Inc. # This program is free software: you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -38,6 +38,7 @@ fi cat <<\EOF > exp-out || framework_failure +Failed to setup keepalive on connection setvcpus: <domain> trying as domain NAME setvcpus: count(optdata): 2 setvcpus: domain(optdata): test diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1d3c5f0..3e989ee 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8781,7 +8781,7 @@ doMigrate(void *opaque) virConnectPtr dconn = NULL; virDomainPtr ddom = NULL; - dconn = virConnectOpenAuth(desturi, virConnectAuthPtrDefault, 0); + dconn = vshConnect(ctl, desturi, false); if (!dconn) goto out; diff --git a/tools/virsh.c b/tools/virsh.c index 9b8429f..73c58a5 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -315,6 +315,45 @@ vshCatchDisconnect(virConnectPtr conn ATTRIBUTE_UNUSED, disconnected++; } +/* Main Function which should be used for connecting. + * This function properly handles keepalive settings. */ +virConnectPtr +vshConnect(vshControl *ctl, const char *uri, bool readonly) +{ + virConnectPtr c = NULL; + int interval = 5; /* Default */ + int count = 6; /* Default */ + bool keepalive_forced = false; + + if (ctl->keepalive_interval >= 0) { + interval = ctl->keepalive_interval; + keepalive_forced = true; + } + if (ctl->keepalive_count > 0) { + count = ctl->keepalive_count; + keepalive_forced = true; + } + + c = virConnectOpenAuth(uri, virConnectAuthPtrDefault, + readonly ? VIR_CONNECT_RO : 0); + if (!c) + return NULL; + + if (virConnectSetKeepAlive(c, interval, count) != 0) { + if (keepalive_forced) { + vshError(ctl, "%s", + _("Cannot setup keepalive on connection " + "as requested, disconnecting")); + virConnectClose(c); + return NULL; + } + vshDebug(ctl, VSH_ERR_INFO, "%s", + _("Failed to setup keepalive on connection\n")); + } + + return c; +} + /* * vshReconnect: * @@ -340,9 +379,8 @@ vshReconnect(vshControl *ctl) "disconnect from the hypervisor")); } - ctl->conn = virConnectOpenAuth(ctl->name, - virConnectAuthPtrDefault, - ctl->readonly ? VIR_CONNECT_RO : 0); + ctl->conn = vshConnect(ctl, ctl->name, ctl->readonly); + if (!ctl->conn) { if (disconnected) vshError(ctl, "%s", _("Failed to reconnect to the hypervisor")); @@ -417,8 +455,7 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd) ctl->useSnapshotOld = false; ctl->readonly = ro; - ctl->conn = virConnectOpenAuth(ctl->name, virConnectAuthPtrDefault, - ctl->readonly ? VIR_CONNECT_RO : 0); + ctl->conn = vshConnect(ctl, ctl->name, ctl->readonly); if (!ctl->conn) { vshError(ctl, "%s", _("Failed to connect to the hypervisor")); @@ -3113,6 +3150,10 @@ vshUsage(void) " -d | --debug=NUM debug level [0-4]\n" " -e | --escape <char> set escape sequence for console\n" " -h | --help this help\n" + " -k | --keepalive-interval=NUM\n" + " keepalive interval in seconds, 0 for disable\n" + " -K | --keepalive-count=NUM\n" + " number of possible missed keepalive messages\n" " -l | --log=FILE output logging to file\n" " -q | --quiet quiet mode\n" " -r | --readonly connect readonly\n" @@ -3302,7 +3343,7 @@ vshAllowedEscapeChar(char c) static bool vshParseArgv(vshControl *ctl, int argc, char **argv) { - int arg, len, debug; + int arg, len, debug, keepalive; size_t i; int longindex = -1; struct option opt[] = { @@ -3310,6 +3351,8 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) {"debug", required_argument, NULL, 'd'}, {"escape", required_argument, NULL, 'e'}, {"help", no_argument, NULL, 'h'}, + {"keepalive-interval", required_argument, NULL, 'k'}, + {"keepalive-count", required_argument, NULL, 'K'}, {"log", required_argument, NULL, 'l'}, {"quiet", no_argument, NULL, 'q'}, {"readonly", no_argument, NULL, 'r'}, @@ -3321,7 +3364,7 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) /* 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, "+:c:d:e:hl:qrtvV", opt, &longindex)) != -1) { + while ((arg = getopt_long(argc, argv, "+:c:d:e:hk:K:l:qrtvV", opt, &longindex)) != -1) { switch (arg) { case 'c': VIR_FREE(ctl->name); @@ -3356,6 +3399,24 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) vshUsage(); exit(EXIT_SUCCESS); break; + case 'k': + if (virStrToLong_i(optarg, NULL, 0, &keepalive) < 0 || + keepalive < 0) { + vshError(ctl, _("option -%s requires a positive numeric argument"), + longindex == -1 ? "-k" : "--keepalive-interval"); + exit(EXIT_FAILURE); + } + ctl->keepalive_interval = keepalive; + break; + case 'K': + if (virStrToLong_i(optarg, NULL, 0, &keepalive) < 0 || + keepalive < 0) { + vshError(ctl, _("option -%s requires a positive numeric argument"), + longindex == -1 ? "-K" : "--keepalive-count"); + exit(EXIT_FAILURE); + } + ctl->keepalive_count = keepalive; + break; case 'l': vshCloseLogFile(ctl); ctl->logfile = vshStrdup(ctl, optarg); @@ -3490,6 +3551,8 @@ main(int argc, char **argv) ctl->log_fd = -1; /* Initialize log file descriptor */ ctl->debug = VSH_DEBUG_DEFAULT; ctl->escapeChar = "^]"; /* Same default as telnet */ + ctl->keepalive_interval = -1; /* In order to distinguish default + * from setting to 0 */ ctl->eventPipe[0] = -1; ctl->eventPipe[1] = -1; ctl->eventTimerId = -1; diff --git a/tools/virsh.h b/tools/virsh.h index 62a1eed..3e0251b 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -249,6 +249,9 @@ struct _vshControl { const char *escapeChar; /* String representation of console escape character */ + int keepalive_interval; /* Client keepalive interval */ + int keepalive_count; /* Client keepalive count */ + # ifndef WIN32 struct termios termattr; /* settings of the tty terminal */ # endif @@ -269,6 +272,8 @@ void vshOutputLogFile(vshControl *ctl, int log_level, const char *format, ATTRIBUTE_FMT_PRINTF(3, 0); void vshCloseLogFile(vshControl *ctl); +virConnectPtr vshConnect(vshControl *ctl, const char *uri, bool readonly); + const char *vshCmddefGetInfo(const vshCmdDef *cmd, const char *info); const vshCmdDef *vshCmddefSearch(const char *cmdname); bool vshCmddefHelp(vshControl *ctl, const char *name); -- 1.9.0
On 07.03.2014 11:49, Martin Kletzander wrote:
Introducing keepalive similarly to Guannan around 2 years ago. Since we want to introduce keepalive for every connection, it makes sense to wrap the connecting function into new virsh one that can deal keepalive as well.
Function vshConnect() is now used for connecting and keepalive added in that function (if possible) helps preventing long waits e.g. while nework goes down during migration.
This patch also adds the options for keepalive tuning into virsh and fails connecting only when keepalives are explicitly requested and cannot be set (whether it is due to missing support in connected driver or remote server). If not explicitely requested, a debug message is printed (hence the addition to virsh-optparse test).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1073506 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=822839
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/virsh-optparse | 3 +- tools/virsh-domain.c | 2 +- tools/virsh.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++----- tools/virsh.h | 5 ++++ 4 files changed, 78 insertions(+), 9 deletions(-)
Missing virsh.pod adjustment while you're adding new command line options.
diff --git a/tests/virsh-optparse b/tests/virsh-optparse index 214ae41..7d4c699 100755 --- a/tests/virsh-optparse +++ b/tests/virsh-optparse @@ -1,7 +1,7 @@ #!/bin/sh # Ensure that virsh option parsing doesn't regress
-# Copyright (C) 2011-2012 Red Hat, Inc. +# Copyright (C) 2011-2012, 2014 Red Hat, Inc.
# This program is free software: you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -38,6 +38,7 @@ fi
cat <<\EOF > exp-out || framework_failure
+Failed to setup keepalive on connection setvcpus: <domain> trying as domain NAME setvcpus: count(optdata): 2 setvcpus: domain(optdata): test diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1d3c5f0..3e989ee 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8781,7 +8781,7 @@ doMigrate(void *opaque) virConnectPtr dconn = NULL; virDomainPtr ddom = NULL;
- dconn = virConnectOpenAuth(desturi, virConnectAuthPtrDefault, 0); + dconn = vshConnect(ctl, desturi, false); if (!dconn) goto out;
diff --git a/tools/virsh.c b/tools/virsh.c index 9b8429f..73c58a5 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -315,6 +315,45 @@ vshCatchDisconnect(virConnectPtr conn ATTRIBUTE_UNUSED, disconnected++; }
+/* Main Function which should be used for connecting. + * This function properly handles keepalive settings. */ +virConnectPtr +vshConnect(vshControl *ctl, const char *uri, bool readonly) +{ + virConnectPtr c = NULL; + int interval = 5; /* Default */ + int count = 6; /* Default */ + bool keepalive_forced = false; + + if (ctl->keepalive_interval >= 0) { + interval = ctl->keepalive_interval; + keepalive_forced = true; + } + if (ctl->keepalive_count > 0) { + count = ctl->keepalive_count; + keepalive_forced = true; + }
Both interval and count are allowed to be zero. However, setting interval to zero disables keep alive. If count is set to zero, connection is closed automatically after @interval seconds of inactivity. You are allowing the first case (which doesn't make much sense to me - I mean, user requested keepalive, so why allowing zero value?), and disabling the second case - which could make more sense. Moreover, 'virsh --keepalive-count 0' will not use user specified count but the default value of 6.
+ + c = virConnectOpenAuth(uri, virConnectAuthPtrDefault, + readonly ? VIR_CONNECT_RO : 0); + if (!c) + return NULL; + + if (virConnectSetKeepAlive(c, interval, count) != 0) { + if (keepalive_forced) { + vshError(ctl, "%s", + _("Cannot setup keepalive on connection " + "as requested, disconnecting")); + virConnectClose(c); + return NULL; + } + vshDebug(ctl, VSH_ERR_INFO, "%s", + _("Failed to setup keepalive on connection\n")); + } + + return c; +} +
Michal
On Fri, Mar 07, 2014 at 04:50:51PM +0100, Michal Privoznik wrote:
On 07.03.2014 11:49, Martin Kletzander wrote:
Introducing keepalive similarly to Guannan around 2 years ago. Since we want to introduce keepalive for every connection, it makes sense to wrap the connecting function into new virsh one that can deal keepalive as well.
Function vshConnect() is now used for connecting and keepalive added in that function (if possible) helps preventing long waits e.g. while nework goes down during migration.
This patch also adds the options for keepalive tuning into virsh and fails connecting only when keepalives are explicitly requested and cannot be set (whether it is due to missing support in connected driver or remote server). If not explicitely requested, a debug message is printed (hence the addition to virsh-optparse test).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1073506 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=822839
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/virsh-optparse | 3 +- tools/virsh-domain.c | 2 +- tools/virsh.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++----- tools/virsh.h | 5 ++++ 4 files changed, 78 insertions(+), 9 deletions(-)
Missing virsh.pod adjustment while you're adding new command line options.
Oh, I removed it in favor of rebasing the previous commit properly and then haven't added it here. Thanks for noticing that.
diff --git a/tests/virsh-optparse b/tests/virsh-optparse index 214ae41..7d4c699 100755 --- a/tests/virsh-optparse +++ b/tests/virsh-optparse @@ -1,7 +1,7 @@ #!/bin/sh # Ensure that virsh option parsing doesn't regress
-# Copyright (C) 2011-2012 Red Hat, Inc. +# Copyright (C) 2011-2012, 2014 Red Hat, Inc.
# This program is free software: you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -38,6 +38,7 @@ fi
cat <<\EOF > exp-out || framework_failure
+Failed to setup keepalive on connection setvcpus: <domain> trying as domain NAME setvcpus: count(optdata): 2 setvcpus: domain(optdata): test diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1d3c5f0..3e989ee 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8781,7 +8781,7 @@ doMigrate(void *opaque) virConnectPtr dconn = NULL; virDomainPtr ddom = NULL;
- dconn = virConnectOpenAuth(desturi, virConnectAuthPtrDefault, 0); + dconn = vshConnect(ctl, desturi, false); if (!dconn) goto out;
diff --git a/tools/virsh.c b/tools/virsh.c index 9b8429f..73c58a5 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -315,6 +315,45 @@ vshCatchDisconnect(virConnectPtr conn ATTRIBUTE_UNUSED, disconnected++; }
+/* Main Function which should be used for connecting. + * This function properly handles keepalive settings. */ +virConnectPtr +vshConnect(vshControl *ctl, const char *uri, bool readonly) +{ + virConnectPtr c = NULL; + int interval = 5; /* Default */ + int count = 6; /* Default */ + bool keepalive_forced = false; + + if (ctl->keepalive_interval >= 0) { + interval = ctl->keepalive_interval; + keepalive_forced = true; + } + if (ctl->keepalive_count > 0) { + count = ctl->keepalive_count; + keepalive_forced = true; + }
Both interval and count are allowed to be zero. However, setting interval to zero disables keep alive. If count is set to zero, connection is closed automatically after @interval seconds of inactivity. You are allowing the first case (which doesn't make much sense to me - I mean, user requested keepalive, so why allowing zero value?), and disabling the second case - which could make more sense.
Moreover, 'virsh --keepalive-count 0' will not use user specified count but the default value of 6.
I had a different use-case in mind and I tried mimicking the virConnectSetKeepAlive() function which allows such parameters and haven't thought that through. But you're right... What would you say to the following change? Since we don't have commands to work with keepalive (and I don't think there is a need for them) "--keepalive-interval 0" can be treated as "don't call virConnectSetKeepAlive() at all". That will not start keepalive instead of forcibly stopping it. keepalive-count can be >= 0 and will be basically treated as keepalive-interval in this v1. Thanks for the review, Martin
On 07.03.2014 17:09, Martin Kletzander wrote:
On Fri, Mar 07, 2014 at 04:50:51PM +0100, Michal Privoznik wrote:
On 07.03.2014 11:49, Martin Kletzander wrote:
Introducing keepalive similarly to Guannan around 2 years ago. Since we want to introduce keepalive for every connection, it makes sense to wrap the connecting function into new virsh one that can deal keepalive as well.
Function vshConnect() is now used for connecting and keepalive added in that function (if possible) helps preventing long waits e.g. while nework goes down during migration.
This patch also adds the options for keepalive tuning into virsh and fails connecting only when keepalives are explicitly requested and cannot be set (whether it is due to missing support in connected driver or remote server). If not explicitely requested, a debug message is printed (hence the addition to virsh-optparse test).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1073506 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=822839
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/virsh-optparse | 3 +- tools/virsh-domain.c | 2 +- tools/virsh.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++----- tools/virsh.h | 5 ++++ 4 files changed, 78 insertions(+), 9 deletions(-)
Missing virsh.pod adjustment while you're adding new command line options.
Oh, I removed it in favor of rebasing the previous commit properly and then haven't added it here. Thanks for noticing that.
diff --git a/tests/virsh-optparse b/tests/virsh-optparse index 214ae41..7d4c699 100755 --- a/tests/virsh-optparse +++ b/tests/virsh-optparse @@ -1,7 +1,7 @@ #!/bin/sh # Ensure that virsh option parsing doesn't regress
-# Copyright (C) 2011-2012 Red Hat, Inc. +# Copyright (C) 2011-2012, 2014 Red Hat, Inc.
# This program is free software: you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -38,6 +38,7 @@ fi
cat <<\EOF > exp-out || framework_failure
+Failed to setup keepalive on connection setvcpus: <domain> trying as domain NAME setvcpus: count(optdata): 2 setvcpus: domain(optdata): test diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1d3c5f0..3e989ee 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8781,7 +8781,7 @@ doMigrate(void *opaque) virConnectPtr dconn = NULL; virDomainPtr ddom = NULL;
- dconn = virConnectOpenAuth(desturi, virConnectAuthPtrDefault, 0); + dconn = vshConnect(ctl, desturi, false); if (!dconn) goto out;
diff --git a/tools/virsh.c b/tools/virsh.c index 9b8429f..73c58a5 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -315,6 +315,45 @@ vshCatchDisconnect(virConnectPtr conn ATTRIBUTE_UNUSED, disconnected++; }
+/* Main Function which should be used for connecting. + * This function properly handles keepalive settings. */ +virConnectPtr +vshConnect(vshControl *ctl, const char *uri, bool readonly) +{ + virConnectPtr c = NULL; + int interval = 5; /* Default */ + int count = 6; /* Default */ + bool keepalive_forced = false; + + if (ctl->keepalive_interval >= 0) { + interval = ctl->keepalive_interval; + keepalive_forced = true; + } + if (ctl->keepalive_count > 0) { + count = ctl->keepalive_count; + keepalive_forced = true; + }
Both interval and count are allowed to be zero. However, setting interval to zero disables keep alive. If count is set to zero, connection is closed automatically after @interval seconds of inactivity. You are allowing the first case (which doesn't make much sense to me - I mean, user requested keepalive, so why allowing zero value?), and disabling the second case - which could make more sense.
Moreover, 'virsh --keepalive-count 0' will not use user specified count but the default value of 6.
I had a different use-case in mind and I tried mimicking the virConnectSetKeepAlive() function which allows such parameters and haven't thought that through. But you're right...
What would you say to the following change?
Since we don't have commands to work with keepalive (and I don't think there is a need for them) "--keepalive-interval 0" can be treated as "don't call virConnectSetKeepAlive() at all". That will not start keepalive instead of forcibly stopping it. keepalive-count can be >= 0 and will be basically treated as keepalive-interval in this v1.
Yes, I think this design is actually correct. Michal
Addition of vshConnect() makes virConnectOpen() functions obsolete in virsh. Thus all virsh-*.[ch] files should be left only with vshConnect() in the case of need. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- cfg.mk | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/cfg.mk b/cfg.mk index 2a8957a..25446ac 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1,5 +1,5 @@ # Customize Makefile.maint. -*- makefile -*- -# Copyright (C) 2008-2013 Red Hat, Inc. +# Copyright (C) 2008-2014 Red Hat, Inc. # Copyright (C) 2003-2008 Free Software Foundation, Inc. # This program is free software: you can redistribute it and/or modify @@ -863,6 +863,12 @@ sc_prohibit_atoi: halt='Use virStrToLong* instead of atoi, atol, atof, atoq, atoll' \ $(_sc_search_regexp) +sc_prohibit_virConnectOpen_in_virsh: + @prohibit='\bvirConnectOpen[a-zA-Z]* *\(' \ + in_vc_files='^tools/virsh-.*\.[ch]$$' \ + halt='Use vshConnect() in virsh instead of virConnectOpen*' \ + $(_sc_search_regexp) + # We don't use this feature of maint.mk. prev_version_file = /dev/null -- 1.9.0
Similarly to Guannan's patches in 2012, this one does add the connection monitoring into vshWatchJob function. *But Beware*, while testing this I discovered it adds no new functionality. Probably since we're catching disconnects by a callback, there is no way to get to the code introduced. Although it might sound weird, this patch just demonstrates that there is no need for it (now). Just for the purpose of nobody asking for such addition. On a side note: I'd love for this to be useful; changing the current output from "error received on socket" to simple "Lost connection to destination", but it would require much more wiring and overhead, I guess. Unless anyone has any idea what I might have done wrong and helps me turn into wrong direction, of course. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tools/virsh-domain.c | 19 ++++++++++++++----- tools/virsh.c | 2 +- tools/virsh.h | 2 ++ 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 3e989ee..5933ce8 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3569,7 +3569,7 @@ typedef void (*jobWatchTimeoutFunc)(vshControl *ctl, virDomainPtr dom, void *opaque); static bool -vshWatchJob(vshControl *ctl, +vshWatchJob(vshCtrlData *data, virDomainPtr dom, bool verbose, int pipe_fd, @@ -3590,6 +3590,7 @@ vshWatchJob(vshControl *ctl, sigset_t sigmask, oldsigmask; bool jobStarted = false; nfds_t npollfd = 2; + vshControl *ctl = data->ctl; sigemptyset(&sigmask); sigaddset(&sigmask, SIGINT); @@ -3642,6 +3643,14 @@ vshWatchJob(vshControl *ctl, goto cleanup; } + if (data->dconn && + !vshConnectionUsability(ctl, data->dconn)) { + vshError(ctl, "%s", _("Lost connection to destrination server, " + "aborting job")); + virDomainAbortJob(dom); + goto cleanup; + } + GETTIMEOFDAY(&curr); if (timeout_ms && (((int)(curr.tv_sec - start.tv_sec) * 1000 + (int)(curr.tv_usec - start.tv_usec) / 1000) > @@ -3716,7 +3725,7 @@ cmdSave(vshControl *ctl, const vshCmd *cmd) &data) < 0) goto cleanup; - ret = vshWatchJob(ctl, dom, verbose, p[0], 0, NULL, NULL, _("Save")); + ret = vshWatchJob(&data, dom, verbose, p[0], 0, NULL, NULL, _("Save")); virThreadJoin(&workerThread); @@ -4037,7 +4046,7 @@ cmdManagedSave(vshControl *ctl, const vshCmd *cmd) &data) < 0) goto cleanup; - ret = vshWatchJob(ctl, dom, verbose, p[0], 0, + ret = vshWatchJob(&data, dom, verbose, p[0], 0, NULL, NULL, _("Managedsave")); virThreadJoin(&workerThread); @@ -4572,7 +4581,7 @@ cmdDump(vshControl *ctl, const vshCmd *cmd) &data) < 0) goto cleanup; - ret = vshWatchJob(ctl, dom, verbose, p[0], 0, NULL, NULL, _("Dump")); + ret = vshWatchJob(&data, dom, verbose, p[0], 0, NULL, NULL, _("Dump")); virThreadJoin(&workerThread); @@ -8856,7 +8865,7 @@ cmdMigrate(vshControl *ctl, const vshCmd *cmd) doMigrate, &data) < 0) goto cleanup; - functionReturn = vshWatchJob(ctl, dom, verbose, p[0], timeout, + functionReturn = vshWatchJob(&data, dom, verbose, p[0], timeout, vshMigrationTimeout, NULL, _("Migration")); virThreadJoin(&workerThread); diff --git a/tools/virsh.c b/tools/virsh.c index 73c58a5..0150c66 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1788,7 +1788,7 @@ vshCommandOptTimeoutToMs(vshControl *ctl, const vshCmd *cmd, int *timeout) } -static bool +bool vshConnectionUsability(vshControl *ctl, virConnectPtr conn) { if (!conn || diff --git a/tools/virsh.h b/tools/virsh.h index 3e0251b..efad272 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -273,6 +273,7 @@ void vshOutputLogFile(vshControl *ctl, int log_level, const char *format, void vshCloseLogFile(vshControl *ctl); virConnectPtr vshConnect(vshControl *ctl, const char *uri, bool readonly); +bool vshConnectionUsability(vshControl *ctl, virConnectPtr conn); const char *vshCmddefGetInfo(const vshCmdDef *cmd, const char *info); const vshCmdDef *vshCmddefSearch(const char *cmdname); @@ -362,6 +363,7 @@ struct _vshCtrlData { vshControl *ctl; const vshCmd *cmd; int writefd; + virConnectPtr dconn; }; /* error handling */ -- 1.9.0
participants (2)
-
Martin Kletzander -
Michal Privoznik