Re: [PATCH 2/2] util/qemu-sockets: make keep-alive enabled by default

[Vladimir Sementsov-Ogievskiy]

09.07.2020 11:29, Daniel P. Berrangé wrote:
> On Wed, Jul 08, 2020 at 10:15:39PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > Keep-alive won't hurt, let's try to enable it even if not requested by
> > user.
>
> Keep-alive intentionally breaks TCP connections earlier than normal
> in face of transient networking problems.
>
> The question is more about which type of pain is more desirable. A
> stall in the network connection (for a potentially very long time),
> or an intentionally broken socket.
>
> I'm not at all convinced it is a good idea to intentionally break
> /all/ QEMU sockets in the face of transient problems, even if the
> problems last for 2 hours or more.
>
> I could see keep-alives being ok on some QEMU socket. For example
> VNC/SPICE clients, as there is no downside to proactively culling
> them as they can trivially reconnect. Migration too is quite
> reasonable to use keep alives, as you generally want migration to
> run to completion in a short amount of time, and aborting migration
> needs to be safe no matter what.
>
> Breaking chardevs or block devices or network devices that use
> QEMU sockets though will be disruptive. The only solution once
> those backends have a dead socket is going to be to kill QEMU
> and cold-boot the VM again.

Reasonable, thanks for explanation.

We are mostly interested in keep-alive for migration and NBD connections.
(NBD driver has ability to reconnect). What do you think about setting
keep-alive (with some KEEPIDLE smaller than 2 hours) by default for
migration and NBD (at least when NBD reconnect is enabled), would it be
valid?

> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   util/qemu-sockets.c | 15 ++++++++++-----
> >   1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > index b961963472..f6851376f5 100644
> > --- a/util/qemu-sockets.c
> > +++ b/util/qemu-sockets.c
> > @@ -438,7 +438,8 @@ static struct addrinfo 
> > *inet_parse_connect_saddr(InetSocketAddress *saddr,
> >    *
> >    * Handle keep_alive settings. If user specified settings explicitly, fail if
> >    * can't set the settings. If user just enabled keep-alive, not specifying the
> > - * settings, try to set defaults but ignore failures.
> > + * settings, try to set defaults but ignore failures. If keep-alive option is
> > + * not specified, try to set it but ignore failures.
> >    */
> >   static int inet_set_keepalive(int sock, bool has_keep_alive,
> >                                 KeepAliveField *keep_alive, Error **errp)
> > @@ -447,8 +448,8 @@ static int inet_set_keepalive(int sock, bool has_keep_alive,
> >       int val;
> >       bool has_settings = has_keep_alive &&  keep_alive->type == QTYPE_QDICT;
> >   
> > -    if (!has_keep_alive || (keep_alive->type == QTYPE_QBOOL &&
> > -                            !keep_alive->u.enabled))
> > +    if (has_keep_alive &&
> > +        keep_alive->type == QTYPE_QBOOL && !keep_alive->u.enabled)
> >       {
> >           return 0;
> >       }
> > @@ -456,8 +457,12 @@ static int inet_set_keepalive(int sock, bool 
> > has_keep_alive,
> >       val = 1;
> >       ret = qemu_setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE, &val, sizeof(val));
> >       if (ret < 0) {
> > -        error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
> > -        return -1;
> > +        if (has_keep_alive) {
> > +            error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
> > +            return -1;
> > +        } else {
> > +            return 0;
> > +        }
> >       }
> >   
> >       val = has_settings ? keep_alive->u.settings.idle : 30;
> > --
> > 2.21.0
>
> Regards,
> Daniel


--
Best regards,
Vladimir