[libvirt] [PATCH v2 03/12] Introduce two public APIs for keepalive protocol

[Daniel P. Berrange]

On Fri, Sep 23, 2011 at 10:24:48AM +0200, Jiri Denemark wrote:
> This introduces virConnectAllowKeepAlive and virConnectStartKeepAlive
> public APIs which can be used by a client connecting to remote server to
> indicate support for keepalive protocol. Both APIs are handled directly
> by remote driver and not transmitted over the wire to the server.
> ---
>  include/libvirt/libvirt.h.in |    5 ++
>  src/driver.h                 |    9 ++++
>  src/libvirt.c                |  107 ++++++++++++++++++++++++++++++++++++++++++
>  src/libvirt_internal.h       |   10 +++-
>  src/libvirt_public.syms      |    6 ++
>  5 files changed, 135 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 39155a6..6f61cc0 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -3210,6 +3210,11 @@ typedef struct _virTypedParameter virMemoryParameter;
>   */
>  typedef virMemoryParameter *virMemoryParameterPtr;
>  
> +int virConnectAllowKeepAlive(virConnectPtr conn);
> +int virConnectStartKeepAlive(virConnectPtr conn,
> +                             int interval,
> +                             unsigned int count);

With this design, even if both the client and server support
keepalive at a protocol level, it will only ever be enabled if
the client application remembers to call virConnectAllowKeepAlive.
I think this puts too much responsibility on the client, at the
detriment of the server.

An administrator of libvirtd might want to mandate use of keep
alive for all clients (knowing this would exclude any libvirt
client <= 0.9.6 of course). With this design they cannot do this
since they are reliant on the client application programmer to
call virConnectAllowKeepAlive, which I believe 95% of people will
never bother todo.

IMHO we should change this so that

 - In remote_driver.c, doRemoteOpen(), after performing
   authentication, but before opening the connection
   we should send a

      supports_feature(KEEPALIVE)

 - Upon receiving supports_feature(KEEPALIVE) the server
   shall be free to start sending keep alives, if it is
   configured todo so.

 - Libvirtd should not send server initiated keepalives
   by default since this is needlessly short-circuiting
   the normal TCP client timeout process.

 - libvirt has a configure paramater to turn on keepalives
   taking values

      0 - disable                          (default)
      1 - enable if supported by client
      2 - enable, drop client if not supported

   and should error upon virConnectOpen if '2' and clients
   do not support it.

 - Client simply has a virConnectSetKeepAlive(conn, interval, count).
   This returns an error if the earlier feature check during doRemoteOpen
   failed. If 'interval' or 'count' is zero then this stops any
   previously started keepalive.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|