| Message ID | 20191007194840.29518-1-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-nbd: Document benefit of --pid-file | expand |
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>
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
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 >
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.
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
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 >
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.
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.
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
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(-)