Re: docs: Update vhost-user spec regarding backend program conventions

[Boeuf, Sebastien]

On Fri, 2020-02-14 at 15:00 +0100, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Feb 14, 2020 at 2:24 PM Boeuf, Sebastien
> <address@hidden> wrote:
> > Hi Marc-Andre,
> > 
> > On Tue, 2020-02-11 at 22:24 +0100, Marc-André Lureau wrote:
> > > Hi
> > > 
> > > On Tue, Feb 11, 2020 at 4:24 PM Boeuf, Sebastien
> > > <address@hidden> wrote:
> > > > From c073d528b8cd7082832fd1825dc33dd65b305aa2 Mon Sep 17
> > > > 00:00:00
> > > > 2001
> > > > From: Sebastien Boeuf <address@hidden>
> > > > Date: Tue, 11 Feb 2020 16:01:22 +0100
> > > > Subject: [PATCH] docs: Update vhost-user spec regarding backend
> > > > program
> > > >  conventions
> > > > 
> > > > The vhost-user specification is not clearly stating the
> > > > expected
> > > > behavior from a backend program whenever the client
> > > > disconnects.
> > > > 
> > > > This patch addresses the issue by defining the default behavior
> > > > and
> > > > proposing an alternative through a command line option.
> > > > 
> > > > By default, a backend program will have to keep listening even
> > > > if
> > > > the
> > > > client disconnects, unless told otherwise through the newly
> > > > introduced
> > > > option --exit-on-disconnect.
> > > > 
> > > > Signed-off-by: Sebastien Boeuf <address@hidden>
> > > > Signed-off-by: Stefan Hajnoczi <address@hidden>
> > > > ---
> > > >  docs/interop/vhost-user.rst | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-
> > > > user.rst
> > > > index 5f8b3a456b..da9a1ebc4c 100644
> > > > --- a/docs/interop/vhost-user.rst
> > > > +++ b/docs/interop/vhost-user.rst
> > > > @@ -1323,6 +1323,10 @@ The backend program must end (as quickly
> > > > and
> > > > cleanly as possible) when
> > > >  the SIGTERM signal is received. Eventually, it may receive
> > > > SIGKILL
> > > > by
> > > >  the management layer after a few seconds.
> > > > 
> > > > +By default, the backend program continues running after the
> > > > client
> > > > +disconnects. It accepts only 1 connection at a time on each
> > > > UNIX
> > > > domain
> > > > +socket.
> > > 
> > > I don't think that's the most common behaviour. libvhost-user
> > > will
> > > panic() on disconnect in general, unless the error/exit is
> > > handled
> > > gracefully by the backend.
> > 
> > It's not the default behavior from libvhost-user, but that's
> > exactly
> > something I'd like to see changing. This should be the normal case
> > if
> > you think about a standard client/server connection, where the
> > server
> > would simply listen again after the client disconnects.
> 
> I disagree, a "normal" lifecycle is a single connection & instance
> per device.
> 
> Having the backend handle multiple connections is needlessly more
> complicated. You need to correctly handle multiple states, flushed
> anything private between connections etc. It should be optional.

Ok so you're okay with explicitely stating the default behavior being a
single connection per device, which means the backend terminates after
the client disconnected. But also, that we can add an option to
explicitely ask the backend to continue listening after the client
disconnects. Does that sounds good?

> 
> 
> > > The most common case is to have 1-1 relation between device/qemu
> > > instance and backend.
> > 
> > Yes this part is fine, but that's not a reason why the backend
> > should
> > terminates.
> 
> It is simpler to ensure it is reset correctly.
> 
> > > Why not restart the backend for another instance?
> > 
> > Because you need some management tool to do so. And I think that by
> > default it could be interesting to have the least amount of extra
> > management involved.
> 
> The management layer should be involved if any side crashes or
> restart anyway.
> 
> > > > +
> > > >  The following command line options have an expected behaviour.
> > > > They
> > > >  are mandatory, unless explicitly said differently:
> > > > 
> > > > @@ -1337,6 +1341,12 @@ are mandatory, unless explicitly said
> > > > differently:
> > > >    vhost-user socket as file descriptor FDNUM. It is
> > > > incompatible
> > > > with
> > > >    --socket-path.
> > > > 
> > > > +--exit-on-disconnect
> > > > +
> > > > +  When this option is provided, the backend program must
> > > > terminate
> > > > when
> > > > +  the client disconnects. This can be used to keep the backend
> > > > program's
> > > > +  lifetime synchronized with its client process.
> > > 
> > > This section list options that are mandatory. It's probably a bit
> > > late
> > > to add more mandatory options (I regret already some of them)
> > 
> > The spec states "They are mandatory, unless explicitly said
> > differently", and in this case I'm explicitely saying "When this
> > option
> > is provided", which means if it's not provided it's fine and we can
> > ignore the fact it's not there.
> 
> Ah ok,  I think we can guard it with a capability too.

Could you elaborate, I'm not sure to understand how to use this
capability that you're referring to.

> 
> > > Do we need to specify the behaviour on client disconnect? Can't
> > > we
> > > leave that to the backend and management layer to decide?
> > 
> > My goal here is to make the spec a bit less loose. I know libvhost-
> > user
> > is the de-facto implementation but we cannot just assume everything
> > out
> > of the libvhost-user implementation, especially since there is a
> > dedicated spec. That's the reason why I thought it'd be nice to
> > have
> > the backend behavior well defined in the spec.
> 
> Sure, the goal of the spec is to have basic interoperability between
> implementations, not to follow whatever libvhost-user is currently
> doing.
> 
> > The point is, relying on the current definition, there's not enough
> > information to make sure a VMM will properly interface with a
> > vhost-
> > user backend.
> 
> I don't know why having the backend handle multiple connections would
> help with that.

By stating what should be the default, we would make the spec a bit
tighter.

> 
> Having undefined behaviour for things that should not happen in
> normal
> circumstances seems acceptable. Having QEMU (or the /master/) restart
> is currently undefined, because it's not considered a simple/normal
> situation: the vhost-user spec doesn't say much about it, does it?

I agree, the spec says nothing about this but this is an important use
case. Think about the case where a cloud provider gives his customers
some VMs based on a random VMM that implements the vhost-user spec but
doesn't have a clear behavior about reboot. Well that means the end
user cannot expect having the same behavior when rebooting his VM
between two different VMMs, while the vhost-user backend might be the
same.

> 
> I'd prefer to keep things simple and have 1-1 device-backend instance
> relationship by default.
> 

I understand, but at least this should be defined clearly in the spec.

Thanks,
Sebastien
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.