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

Martin Kletzander mkletzan at redhat.com
Fri Mar 7 16:09:46 UTC 2014


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.

Thanks for the review,
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140307/81ba7bba/attachment-0001.sig>


More information about the libvir-list mailing list