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

[Boeuf, Sebastien]

On Fri, 2020-02-14 at 14:21 +0000, Daniel P. Berrangé wrote:
> On Fri, Feb 14, 2020 at 03:00:34PM +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.
> > 
> > 
> > > > 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.
> 
> Further, this vhost-user.rst spec document is explicitly describing
> the contract between the vhost-user binaries and the management
> layer. So it doesn't make sense to say update this doc to describe
> desired semantics for usage /without/ a management layer.

Is it? What I call management layer is something like Kata Containers,
spawning both the backend and the VMM. IMO, the document is more about
describing the protocol and how the communication is handled between
the backend and the VMM.

> 
> So I agree, the default behaviour should be that there is one binary
> spawned at time the associated device is initialization, and that
> the lifetime of the binary is 1:1 associated with the lifetime of
> the VM, or until the device is unplugged. 

If you all agree on this, then at least we should make this clear in
the document. I'll update the patch.

Thanks,
Sebastien

> 
> Regards,
> Daniel
---------------------------------------------------------------------
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.