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

[Michal Privoznik]

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