docs: Update vhost-user spec regarding backend program conventions

docs: Update vhost-user spec regarding backend program conventions

[Boeuf, Sebastien]

From c073d528b8cd7082832fd1825dc33dd65b305aa2 Mon Sep 17 00:00:00 2001
From: Sebastien Boeuf <sebastien.boeuf@intel.com>
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 <sebastien.boeuf@intel.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 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.
+
 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.
+
 --print-capabilities
 
   Output to stdout the backend capabilities in JSON format, and then
-- 
2.20.1

---------------------------------------------------------------------
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.

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

[Marc-André Lureau]

Hi

On Tue, Feb 11, 2020 at 4:24 PM Boeuf, Sebastien
<sebastien.boeuf@intel.com> wrote:
>
> From c073d528b8cd7082832fd1825dc33dd65b305aa2 Mon Sep 17 00:00:00 2001
> From: Sebastien Boeuf <sebastien.boeuf@intel.com>
> 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 <sebastien.boeuf@intel.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  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.

The most common case is to have 1-1 relation between device/qemu
instance and backend.

Why not restart the backend for another instance?

> +
>  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)

Do we need to specify the behaviour on client disconnect? Can't we
leave that to the backend and management layer to decide?


> +
>  --print-capabilities
>
>    Output to stdout the backend capabilities in JSON format, and then
> --
> 2.20.1
>
> ---------------------------------------------------------------------
> 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.

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

[Boeuf, Sebastien]

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
> <sebastien.boeuf@intel.com> wrote:
> > From c073d528b8cd7082832fd1825dc33dd65b305aa2 Mon Sep 17 00:00:00
> > 2001
> > From: Sebastien Boeuf <sebastien.boeuf@intel.com>
> > 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 <sebastien.boeuf@intel.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  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.

> 
> 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.

> 
> 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 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.

> 
> 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.
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.

Thanks,
Sebastien

> 
> 
> > +
> >  --print-capabilities
> > 
> >    Output to stdout the backend capabilities in JSON format, and
> > then
> > --
> > 2.20.1
> > 
> > -----------------------------------------------------------------
> > ----
> > 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.
---------------------------------------------------------------------
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.

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

[Marc-André Lureau]

Hi

On Fri, Feb 14, 2020 at 2:24 PM Boeuf, Sebastien
<sebastien.boeuf@intel.com> 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
> > <sebastien.boeuf@intel.com> wrote:
> > > From c073d528b8cd7082832fd1825dc33dd65b305aa2 Mon Sep 17 00:00:00
> > > 2001
> > > From: Sebastien Boeuf <sebastien.boeuf@intel.com>
> > > 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 <sebastien.boeuf@intel.com>
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >  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.

>
> >
> > > +
> > >  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.

>
> >
> > 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.

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'd prefer to keep things simple and have 1-1 device-backend instance
relationship by default.

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

[Daniel P. Berrangé]

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
> <sebastien.boeuf@intel.com> 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
> > > <sebastien.boeuf@intel.com> wrote:
> > > > From c073d528b8cd7082832fd1825dc33dd65b305aa2 Mon Sep 17 00:00:00
> > > > 2001
> > > > From: Sebastien Boeuf <sebastien.boeuf@intel.com>
> > > > 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 <sebastien.boeuf@intel.com>
> > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > ---
> > > >  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.

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. 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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
> > <sebastien.boeuf@intel.com> 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
> > > > <sebastien.boeuf@intel.com> wrote:
> > > > > From c073d528b8cd7082832fd1825dc33dd65b305aa2 Mon Sep 17
> > > > > 00:00:00
> > > > > 2001
> > > > > From: Sebastien Boeuf <sebastien.boeuf@intel.com>
> > > > > 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 <sebastien.boeuf@intel.com>
> > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > > ---
> > > > >  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.

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
> <sebastien.boeuf@intel.com> 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
> > > <sebastien.boeuf@intel.com> wrote:
> > > > From c073d528b8cd7082832fd1825dc33dd65b305aa2 Mon Sep 17
> > > > 00:00:00
> > > > 2001
> > > > From: Sebastien Boeuf <sebastien.boeuf@intel.com>
> > > > 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 <sebastien.boeuf@intel.com>
> > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > ---
> > > >  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.