[libvirt] [PATCH v2 0/3] Add keepalive support in virsh
This series adds client keepalive support into virsh. This was previously almost done by Guannan around 2 years ago, but not finished. Martin Kletzander (3): 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 | 4 +-- tools/virsh-domain.c | 21 +++++++++---- tools/virsh.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++----- tools/virsh.h | 7 +++++ tools/virsh.pod | 12 ++++++++ 6 files changed, 118 insertions(+), 17 deletions(-) -- 1.9.0
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> --- Notes: v2: - Skip calling virConnectSetKeepAlive() when keepalive-inteval is set to 0 - Add keepalive-related options into virsh man page - Just disable keepalive in virsh-optparse instead of checking for the error. tests/virsh-optparse | 4 +-- tools/virsh-domain.c | 2 +- tools/virsh.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++----- tools/virsh.h | 5 ++++ tools/virsh.pod | 12 ++++++++ 5 files changed, 94 insertions(+), 10 deletions(-) diff --git a/tests/virsh-optparse b/tests/virsh-optparse index 214ae41..39e6cde 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 @@ -65,7 +65,7 @@ for args in \ '--count 2 test' \ '--count=2 test' \ ; do - virsh -d0 -c $test_url setvcpus $args >out 2>>err || fail=1 + virsh -k0 -d0 -c $test_url setvcpus $args >out 2>>err || fail=1 LC_ALL=C sort out | compare exp-out - || fail=1 done 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..02835d9 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -315,6 +315,46 @@ 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 (interval > 0 && + 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 +380,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 +456,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 +3151,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 +3344,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 +3352,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 +3365,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 +3400,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 +3552,11 @@ main(int argc, char **argv) ctl->log_fd = -1; /* Initialize log file descriptor */ ctl->debug = VSH_DEBUG_DEFAULT; ctl->escapeChar = "^]"; /* Same default as telnet */ + + /* In order to distinguish default from setting to 0 */ + ctl->keepalive_interval = -1; + ctl->keepalive_count = -1; + 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); diff --git a/tools/virsh.pod b/tools/virsh.pod index cefce1b..5c74bb3 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -74,6 +74,18 @@ alphabetic character, @, [, ], \, ^, _. Ignore all other arguments, and behave as if the B<help> command were given instead. +=item B<-k>, B<--keepalive-interval> I<INTERVAL> + +Set an I<INTERVAL> (in seconds) for sending keepalive messages to +check whether connection to the server is still alive. Setting the +interval to 0 disables client keepalive mechanism. + +=item B<-K>, B<--keepalive-count> I<COUNT> + +Set a number of times keepalive message can be sent without getting an +answer from the server without marking the connection dead. There is +no effect to this setting in case the I<INTERVAL> is set to 0. + =item B<-l>, B<--log> I<FILE> Output logging details to I<FILE>. -- 1.9.0
On 10.03.2014 12:26, 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> ---
Notes: v2: - Skip calling virConnectSetKeepAlive() when keepalive-inteval is set to 0 - Add keepalive-related options into virsh man page - Just disable keepalive in virsh-optparse instead of checking for the error.
tests/virsh-optparse | 4 +-- tools/virsh-domain.c | 2 +- tools/virsh.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++----- tools/virsh.h | 5 ++++ tools/virsh.pod | 12 ++++++++ 5 files changed, 94 insertions(+), 10 deletions(-)
ACK Michal
On Tue, Mar 18, 2014 at 08:05:54AM +0100, Michal Privoznik wrote:
On 10.03.2014 12:26, 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> ---
Notes: v2: - Skip calling virConnectSetKeepAlive() when keepalive-inteval is set to 0 - Add keepalive-related options into virsh man page - Just disable keepalive in virsh-optparse instead of checking for the error.
tests/virsh-optparse | 4 +-- tools/virsh-domain.c | 2 +- tools/virsh.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++----- tools/virsh.h | 5 ++++ tools/virsh.pod | 12 ++++++++ 5 files changed, 94 insertions(+), 10 deletions(-)
ACK
Michal
Pushed, thanks. Martin
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
On 10.03.2014 12:26, Martin Kletzander wrote:
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
ACK Michal
On Tue, Mar 18, 2014 at 08:05:51AM +0100, Michal Privoznik wrote:
On 10.03.2014 12:26, Martin Kletzander wrote:
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
ACK
Michal
Pushed, thanks. Martin
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. 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. 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 02835d9..9288a49 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1789,7 +1789,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