[PATCH] qemu-nbd: Document benefit of --pid-file

[PATCH] qemu-nbd: Document benefit of --pid-file

[Eric Blake]

One benefit of --pid-file is that it is easier to probe the file
system to see if a pid file has been created than it is to probe if a
socket is available for connection. Document that this is an
intentional feature.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qemu-nbd.texi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 7f55657722bd..d495bbe8a0ed 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -118,7 +118,8 @@ in list mode.
 @item --fork
 Fork off the server process and exit the parent once the server is running.
 @item --pid-file=PATH
-Store the server's process ID in the given file.
+Store the server's process ID in the given file.  The pid file is not
+created until after the server socket is open.
 @item --tls-authz=ID
 Specify the ID of a qauthz object previously created with the
 --object option. This will be used to authorize connecting users
-- 
2.21.0

Re: [PATCH] qemu-nbd: Document benefit of --pid-file

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1204 bytes --]

On 07.10.19 21:48, Eric Blake wrote:
> One benefit of --pid-file is that it is easier to probe the file
> system to see if a pid file has been created than it is to probe if a
> socket is available for connection. Document that this is an
> intentional feature.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  qemu-nbd.texi | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-nbd.texi b/qemu-nbd.texi
> index 7f55657722bd..d495bbe8a0ed 100644
> --- a/qemu-nbd.texi
> +++ b/qemu-nbd.texi
> @@ -118,7 +118,8 @@ in list mode.
>  @item --fork
>  Fork off the server process and exit the parent once the server is running.
>  @item --pid-file=PATH
> -Store the server's process ID in the given file.
> +Store the server's process ID in the given file.  The pid file is not
> +created until after the server socket is open.
>  @item --tls-authz=ID
>  Specify the ID of a qauthz object previously created with the
>  --object option. This will be used to authorize connecting users

Well, not wrong, but at least most iotests do this by --fork and seeing
when the parent exits.  But I suppose:

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] qemu-nbd: Document benefit of --pid-file

[Daniel P. Berrangé]

On Mon, Oct 07, 2019 at 02:48:40PM -0500, Eric Blake wrote:
> One benefit of --pid-file is that it is easier to probe the file
> system to see if a pid file has been created than it is to probe if a
> socket is available for connection. Document that this is an
> intentional feature.

I'm not seeing how checking the pid file is better than checking
the socket directly ? I think it is probably actually worse.

The main problem with the socket is that while we unlink on clean
shutdown, it may still exist in disk if the process has exitted
abnormally.

With the pidfile though we don't ever unlink it, even on clean
shutdown, as we don't use the pid files existance as a mutual
exclusion check. We instead acquire fcntl locks on it.

IOW the pidfile could exist already when qemu-nbd starts up and
will still exist when it quits.

> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  qemu-nbd.texi | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-nbd.texi b/qemu-nbd.texi
> index 7f55657722bd..d495bbe8a0ed 100644
> --- a/qemu-nbd.texi
> +++ b/qemu-nbd.texi
> @@ -118,7 +118,8 @@ in list mode.
>  @item --fork
>  Fork off the server process and exit the parent once the server is running.
>  @item --pid-file=PATH
> -Store the server's process ID in the given file.
> +Store the server's process ID in the given file.  The pid file is not
> +created until after the server socket is open.
>  @item --tls-authz=ID
>  Specify the ID of a qauthz object previously created with the
>  --object option. This will be used to authorize connecting users
> -- 
> 2.21.0
> 
> 

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: [PATCH] qemu-nbd: Document benefit of --pid-file

[Vladimir Sementsov-Ogievskiy]

08.10.2019 12:24, Daniel P. Berrangé wrote:
> On Mon, Oct 07, 2019 at 02:48:40PM -0500, Eric Blake wrote:
>> One benefit of --pid-file is that it is easier to probe the file
>> system to see if a pid file has been created than it is to probe if a
>> socket is available for connection. Document that this is an
>> intentional feature.
> 
> I'm not seeing how checking the pid file is better than checking
> the socket directly ? I think it is probably actually worse.
> 
> The main problem with the socket is that while we unlink on clean
> shutdown, it may still exist in disk if the process has exitted
> abnormally.
> 
> With the pidfile though we don't ever unlink it, even on clean
> shutdown, as we don't use the pid files existance as a mutual
> exclusion check. We instead acquire fcntl locks on it.
> 
> IOW the pidfile could exist already when qemu-nbd starts up and
> will still exist when it quits.

Good point.

I was just a bit confused, because pid file is something unrelated to
socket, and why use one thing to check the existence of another, if we
can directly try to connect.

> 
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>   qemu-nbd.texi | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/qemu-nbd.texi b/qemu-nbd.texi
>> index 7f55657722bd..d495bbe8a0ed 100644
>> --- a/qemu-nbd.texi
>> +++ b/qemu-nbd.texi
>> @@ -118,7 +118,8 @@ in list mode.
>>   @item --fork
>>   Fork off the server process and exit the parent once the server is running.
>>   @item --pid-file=PATH
>> -Store the server's process ID in the given file.
>> +Store the server's process ID in the given file.  The pid file is not
>> +created until after the server socket is open.
>>   @item --tls-authz=ID
>>   Specify the ID of a qauthz object previously created with the
>>   --object option. This will be used to authorize connecting users
>> -- 
>> 2.21.0
>>
>>
> 
> Regards,
> Daniel
> 


-- 
Best regards,
Vladimir

Re: [PATCH] qemu-nbd: Document benefit of --pid-file

[Eric Blake]

On 10/8/19 4:40 AM, Vladimir Sementsov-Ogievskiy wrote:
> 08.10.2019 12:24, Daniel P. Berrangé wrote:
>> On Mon, Oct 07, 2019 at 02:48:40PM -0500, Eric Blake wrote:
>>> One benefit of --pid-file is that it is easier to probe the file
>>> system to see if a pid file has been created than it is to probe if a
>>> socket is available for connection. Document that this is an
>>> intentional feature.
>>
>> I'm not seeing how checking the pid file is better than checking
>> the socket directly ? I think it is probably actually worse.
>>
>> The main problem with the socket is that while we unlink on clean
>> shutdown, it may still exist in disk if the process has exitted
>> abnormally.
>>
>> With the pidfile though we don't ever unlink it, even on clean
>> shutdown, as we don't use the pid files existance as a mutual
>> exclusion check. We instead acquire fcntl locks on it.
>>
>> IOW the pidfile could exist already when qemu-nbd starts up and
>> will still exist when it quits.
> 
> Good point.
> 
> I was just a bit confused, because pid file is something unrelated to
> socket, and why use one thing to check the existence of another, if we
> can directly try to connect.

Consider the case of writing a testsuite that involves an nbd client, 
where you want to fire up qemu-nbd as the server.  Checking for a pid 
file in shell is easy, and can be done prior to the point of spawning a 
client.  Checking for a successful connect is harder - the shell is not 
the point that would actually connect, so checking if connect works 
involves the shell actually spawning off the child process that attempts 
the connect.  If the client itself has a retry builtin, then you don't 
need to do anything in shell - just spawn the client with retry (at 
which point, the client retrying on the connection is smarter than the 
client retrying on the pid file).  But pid files are immensely useful 
when you have a client that does not have builtin retry, and when 
writing a testing framework where you use shell to learn whether it is 
safe to spawn the client: rather than having to modify the client or 
write a shell loop that respawns child attempts, you merely need a shell 
loop probing for the pid file to exist.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] qemu-nbd: Document benefit of --pid-file

[Daniel P. Berrangé]

On Tue, Oct 08, 2019 at 08:28:16AM -0500, Eric Blake wrote:
> On 10/8/19 4:40 AM, Vladimir Sementsov-Ogievskiy wrote:
> > 08.10.2019 12:24, Daniel P. Berrangé wrote:
> > > On Mon, Oct 07, 2019 at 02:48:40PM -0500, Eric Blake wrote:
> > > > One benefit of --pid-file is that it is easier to probe the file
> > > > system to see if a pid file has been created than it is to probe if a
> > > > socket is available for connection. Document that this is an
> > > > intentional feature.
> > > 
> > > I'm not seeing how checking the pid file is better than checking
> > > the socket directly ? I think it is probably actually worse.
> > > 
> > > The main problem with the socket is that while we unlink on clean
> > > shutdown, it may still exist in disk if the process has exitted
> > > abnormally.
> > > 
> > > With the pidfile though we don't ever unlink it, even on clean
> > > shutdown, as we don't use the pid files existance as a mutual
> > > exclusion check. We instead acquire fcntl locks on it.
> > > 
> > > IOW the pidfile could exist already when qemu-nbd starts up and
> > > will still exist when it quits.
> > 
> > Good point.
> > 
> > I was just a bit confused, because pid file is something unrelated to
> > socket, and why use one thing to check the existence of another, if we
> > can directly try to connect.
> 
> Consider the case of writing a testsuite that involves an nbd client, where
> you want to fire up qemu-nbd as the server.  Checking for a pid file in
> shell is easy, and can be done prior to the point of spawning a client.
> Checking for a successful connect is harder - the shell is not the point
> that would actually connect, so checking if connect works involves the shell
> actually spawning off the child process that attempts the connect.  If the
> client itself has a retry builtin, then you don't need to do anything in
> shell - just spawn the client with retry (at which point, the client
> retrying on the connection is smarter than the client retrying on the pid
> file).  But pid files are immensely useful when you have a client that does
> not have builtin retry, and when writing a testing framework where you use
> shell to learn whether it is safe to spawn the client: rather than having to
> modify the client or write a shell loop that respawns child attempts, you
> merely need a shell loop probing for the pid file to exist.

We shouldn't need todo any of those tricks IIUC.  The --fork argument is
supposed to only let the parent process exit once the server is running.

IOW, if you run qemu-nbd --fork, in the foreground, then when execution
continues the sockets should be present & accepting connections. No need
to check for existance of any files or check connecting, etc.


Except that AFAICT, --fork isn't actually implemented with this semantics
in qemu-nbd. It looks like we only listen on the sockets after the parent
has already exited :-( Can we fix that to synchronize wrt socket listeners ?

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: [PATCH] qemu-nbd: Document benefit of --pid-file

[Vladimir Sementsov-Ogievskiy]

08.10.2019 16:38, Daniel P. Berrangé wrote:
> On Tue, Oct 08, 2019 at 08:28:16AM -0500, Eric Blake wrote:
>> On 10/8/19 4:40 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 08.10.2019 12:24, Daniel P. Berrangé wrote:
>>>> On Mon, Oct 07, 2019 at 02:48:40PM -0500, Eric Blake wrote:
>>>>> One benefit of --pid-file is that it is easier to probe the file
>>>>> system to see if a pid file has been created than it is to probe if a
>>>>> socket is available for connection. Document that this is an
>>>>> intentional feature.
>>>>
>>>> I'm not seeing how checking the pid file is better than checking
>>>> the socket directly ? I think it is probably actually worse.
>>>>
>>>> The main problem with the socket is that while we unlink on clean
>>>> shutdown, it may still exist in disk if the process has exitted
>>>> abnormally.
>>>>
>>>> With the pidfile though we don't ever unlink it, even on clean
>>>> shutdown, as we don't use the pid files existance as a mutual
>>>> exclusion check. We instead acquire fcntl locks on it.
>>>>
>>>> IOW the pidfile could exist already when qemu-nbd starts up and
>>>> will still exist when it quits.
>>>
>>> Good point.
>>>
>>> I was just a bit confused, because pid file is something unrelated to
>>> socket, and why use one thing to check the existence of another, if we
>>> can directly try to connect.
>>
>> Consider the case of writing a testsuite that involves an nbd client, where
>> you want to fire up qemu-nbd as the server.  Checking for a pid file in
>> shell is easy, and can be done prior to the point of spawning a client.
>> Checking for a successful connect is harder - the shell is not the point
>> that would actually connect, so checking if connect works involves the shell
>> actually spawning off the child process that attempts the connect.  If the
>> client itself has a retry builtin, then you don't need to do anything in
>> shell - just spawn the client with retry (at which point, the client
>> retrying on the connection is smarter than the client retrying on the pid
>> file).  But pid files are immensely useful when you have a client that does
>> not have builtin retry, and when writing a testing framework where you use
>> shell to learn whether it is safe to spawn the client: rather than having to
>> modify the client or write a shell loop that respawns child attempts, you
>> merely need a shell loop probing for the pid file to exist.

I've already implemented loop of attempting to connect in my series (patch 4/3).
It's a bit more difficult to implement, but it's done. And it's a bit better,
as it test exactly what we want to test. Can we proceed with it?

> 
> We shouldn't need todo any of those tricks IIUC.  The --fork argument is
> supposed to only let the parent process exit once the server is running.
> 
> IOW, if you run qemu-nbd --fork, in the foreground, then when execution
> continues the sockets should be present & accepting connections. No need
> to check for existance of any files or check connecting, etc.
> 
> 
> Except that AFAICT, --fork isn't actually implemented with this semantics
> in qemu-nbd. It looks like we only listen on the sockets after the parent
> has already exited :-( Can we fix that to synchronize wrt socket listeners ?
> 
> Regards,
> Daniel
> 


-- 
Best regards,
Vladimir

Re: [PATCH] qemu-nbd: Document benefit of --pid-file

[Eric Blake]

On 10/8/19 8:53 AM, Vladimir Sementsov-Ogievskiy wrote:

> 
> I've already implemented loop of attempting to connect in my series (patch 4/3).
> It's a bit more difficult to implement, but it's done. And it's a bit better,
> as it test exactly what we want to test. Can we proceed with it?
> 

For test purposes, yes, that's fine (a test doesn't have to be clean, 
just work).

>>
>> We shouldn't need todo any of those tricks IIUC.  The --fork argument is
>> supposed to only let the parent process exit once the server is running.
>>
>> IOW, if you run qemu-nbd --fork, in the foreground, then when execution
>> continues the sockets should be present & accepting connections. No need
>> to check for existance of any files or check connecting, etc.
>>
>>
>> Except that AFAICT, --fork isn't actually implemented with this semantics
>> in qemu-nbd. It looks like we only listen on the sockets after the parent
>> has already exited :-( Can we fix that to synchronize wrt socket listeners ?

Yes, sounds like something good to have.  I'll take a look at doing that.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] qemu-nbd: Document benefit of --pid-file

[Eric Blake]

On 10/8/19 8:38 AM, Daniel P. Berrangé wrote:
> On Tue, Oct 08, 2019 at 08:28:16AM -0500, Eric Blake wrote:
>> On 10/8/19 4:40 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 08.10.2019 12:24, Daniel P. Berrangé wrote:
>>>> On Mon, Oct 07, 2019 at 02:48:40PM -0500, Eric Blake wrote:
>>>>> One benefit of --pid-file is that it is easier to probe the file
>>>>> system to see if a pid file has been created than it is to probe if a
>>>>> socket is available for connection. Document that this is an
>>>>> intentional feature.

Revisiting an older thread,


>>> I was just a bit confused, because pid file is something unrelated to
>>> socket, and why use one thing to check the existence of another, if we
>>> can directly try to connect.
>>
>> Consider the case of writing a testsuite that involves an nbd client, where
>> you want to fire up qemu-nbd as the server.  Checking for a pid file in
>> shell is easy, and can be done prior to the point of spawning a client.
>> Checking for a successful connect is harder - the shell is not the point
>> that would actually connect, so checking if connect works involves the shell
>> actually spawning off the child process that attempts the connect.  If the
>> client itself has a retry builtin, then you don't need to do anything in
>> shell - just spawn the client with retry (at which point, the client
>> retrying on the connection is smarter than the client retrying on the pid
>> file).  But pid files are immensely useful when you have a client that does
>> not have builtin retry, and when writing a testing framework where you use
>> shell to learn whether it is safe to spawn the client: rather than having to
>> modify the client or write a shell loop that respawns child attempts, you
>> merely need a shell loop probing for the pid file to exist.
> 
> We shouldn't need todo any of those tricks IIUC.  The --fork argument is
> supposed to only let the parent process exit once the server is running.
> 
> IOW, if you run qemu-nbd --fork, in the foreground, then when execution
> continues the sockets should be present & accepting connections. No need
> to check for existance of any files or check connecting, etc.
> 
> 
> Except that AFAICT, --fork isn't actually implemented with this semantics
> in qemu-nbd. It looks like we only listen on the sockets after the parent
> has already exited :-( Can we fix that to synchronize wrt socket listeners ?

Actually, after re-reading the code, it IS implemented this way. The 
parent process when --fork is used does not exit until after the child 
process closes its temporary stderr pipe to the parent, which is after 
the socket has been set up and only right before beginning the main 
event loop.  So when --fork is in use, waiting for the process to exit 
is sufficient.

But when --fork is not in use (because you want to keep the qemu-nbd 
process in the foreground for whatever reason), then --pid-file can 
indeed serve as a witness when the single process has progressed far 
enough to have the socket open.

At any rate, I'm less certain if it is worth trying to revive this 
patch.  A doc-only change is still okay for 4.2-rc2, except that we have 
to agree on what such a doc change should be and if it adds anything 
useful to the reader.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org