[libvirt] [PATCH 2/4] virsh: Add keepalive in new vshConnect function

[Michal Privoznik]

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 at 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