[PATCH] remote: systemd: Remove unix sockets from filesystem when disabling a '.socket' unit

Peter Krempa posted 1 patch 2 years, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/a214f63bb09bf83344a5c610d7b9e35cce551519.1642516706.git.pkrempa@redhat.com
Test syntax-check failed
src/remote/libvirtd.socket.in | 1 +
1 file changed, 1 insertion(+)
[PATCH] remote: systemd: Remove unix sockets from filesystem when disabling a '.socket' unit
Posted by Peter Krempa 2 years, 3 months ago
The existence of the unix socket path is used by the remote driver to
determine whether modular daemons are in use, so if the socket file
stays behind and the user decided to switch from modular to monolithic
daemon which was socket activated, the remote driver will insist on
picking '/var/run/libvirt/virtqemud-sock', even when it's no longer in
use:

 # systemctl start libvirtd.service
 # virsh list
  Id   Name   State
 --------------------

 # systemctl stop libvirtd.service
 Warning: Stopping libvirtd.service, but it can still be activated by:
   libvirtd.socket
   libvirtd-ro.socket
   libvirtd-admin.socket
 # systemctl start virtqemud.socket
 # virsh list
  Id   Name   State
 --------------------

 # systemctl stop virtqemud.socket
 # systemctl start libvirtd.service
 # virsh list
 error: failed to connect to the hypervisor
 error: Failed to connect socket to '/var/run/libvirt/virtqemud-sock': Connection refused

 # virsh -c 'qemu:///system?socket=/var/run/libvirt/libvirt-sock' list
  Id   Name   State
 --------------------

Fix this by instructing systemd to delete the socket file when
deactivating the unit file for the socket.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/remote/libvirtd.socket.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/remote/libvirtd.socket.in b/src/remote/libvirtd.socket.in
index 85b4aa800a..0f349656f5 100644
--- a/src/remote/libvirtd.socket.in
+++ b/src/remote/libvirtd.socket.in
@@ -9,6 +9,7 @@ Before=@service@.service
 ListenStream=@runstatedir@/libvirt/@sockprefix@-sock
 Service=@service@.service
 SocketMode=@mode@
+RemoveOnStop=yes

 [Install]
 WantedBy=sockets.target
-- 
2.34.1

Re: [PATCH] remote: systemd: Remove unix sockets from filesystem when disabling a '.socket' unit
Posted by Erik Skultety 2 years, 3 months ago
On Tue, Jan 18, 2022 at 03:38:26PM +0100, Peter Krempa wrote:
> The existence of the unix socket path is used by the remote driver to
> determine whether modular daemons are in use, so if the socket file
> stays behind and the user decided to switch from modular to monolithic
> daemon which was socket activated, the remote driver will insist on
> picking '/var/run/libvirt/virtqemud-sock', even when it's no longer in
> use:
> 
>  # systemctl start libvirtd.service
>  # virsh list
>   Id   Name   State
>  --------------------
> 
>  # systemctl stop libvirtd.service
>  Warning: Stopping libvirtd.service, but it can still be activated by:
>    libvirtd.socket
>    libvirtd-ro.socket
>    libvirtd-admin.socket
>  # systemctl start virtqemud.socket
>  # virsh list
>   Id   Name   State
>  --------------------
> 
>  # systemctl stop virtqemud.socket
>  # systemctl start libvirtd.service
>  # virsh list
>  error: failed to connect to the hypervisor
>  error: Failed to connect socket to '/var/run/libvirt/virtqemud-sock': Connection refused
> 
>  # virsh -c 'qemu:///system?socket=/var/run/libvirt/libvirt-sock' list
>   Id   Name   State
>  --------------------
> 
> Fix this by instructing systemd to delete the socket file when
> deactivating the unit file for the socket.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---

Yes, please, I've hit this a few times already in the past couple of days.
However - the service may still remain running when you stop the socket and as
the man page states, it should still be possible to communicate with the
service via the filesystem node, e.g. doing 'virsh list'.
But I agree this is very annoying and I don't particularly like wiping
/run/libvirt whenever I want to switch :( .

Erik

I'm still inclined to ACK the patch though...

Re: [PATCH] remote: systemd: Remove unix sockets from filesystem when disabling a '.socket' unit
Posted by Ani Sinha 2 years, 3 months ago

On Tue, 18 Jan 2022, Peter Krempa wrote:

> The existence of the unix socket path is used by the remote driver to
> determine whether modular daemons are in use, so if the socket file
> stays behind and the user decided to switch from modular to monolithic
> daemon which was socket activated, the remote driver will insist on
> picking '/var/run/libvirt/virtqemud-sock', even when it's no longer in
> use:
>
>  # systemctl start libvirtd.service
>  # virsh list
>   Id   Name   State
>  --------------------
>
>  # systemctl stop libvirtd.service
>  Warning: Stopping libvirtd.service, but it can still be activated by:
>    libvirtd.socket
>    libvirtd-ro.socket
>    libvirtd-admin.socket
>  # systemctl start virtqemud.socket
>  # virsh list
>   Id   Name   State
>  --------------------
>
>  # systemctl stop virtqemud.socket
>  # systemctl start libvirtd.service
>  # virsh list
>  error: failed to connect to the hypervisor
>  error: Failed to connect socket to '/var/run/libvirt/virtqemud-sock': Connection refused
>
>  # virsh -c 'qemu:///system?socket=/var/run/libvirt/libvirt-sock' list
>   Id   Name   State
>  --------------------
>
> Fix this by instructing systemd to delete the socket file when
> deactivating the unit file for the socket.
>
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>

Reviewed-by: Ani Sinha <ani@anisinha.ca>

> ---
>  src/remote/libvirtd.socket.in | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/remote/libvirtd.socket.in b/src/remote/libvirtd.socket.in
> index 85b4aa800a..0f349656f5 100644
> --- a/src/remote/libvirtd.socket.in
> +++ b/src/remote/libvirtd.socket.in
> @@ -9,6 +9,7 @@ Before=@service@.service
>  ListenStream=@runstatedir@/libvirt/@sockprefix@-sock
>  Service=@service@.service
>  SocketMode=@mode@
> +RemoveOnStop=yes
>
>  [Install]
>  WantedBy=sockets.target
> --
> 2.34.1
>
>

Re: [PATCH] remote: systemd: Remove unix sockets from filesystem when disabling a '.socket' unit
Posted by Michal Prívozník 2 years, 3 months ago
On 1/18/22 15:38, Peter Krempa wrote:
> The existence of the unix socket path is used by the remote driver to
> determine whether modular daemons are in use, so if the socket file
> stays behind and the user decided to switch from modular to monolithic
> daemon which was socket activated, the remote driver will insist on
> picking '/var/run/libvirt/virtqemud-sock', even when it's no longer in
> use:
> 
>  # systemctl start libvirtd.service
>  # virsh list
>   Id   Name   State
>  --------------------
> 
>  # systemctl stop libvirtd.service
>  Warning: Stopping libvirtd.service, but it can still be activated by:
>    libvirtd.socket
>    libvirtd-ro.socket
>    libvirtd-admin.socket
>  # systemctl start virtqemud.socket
>  # virsh list
>   Id   Name   State
>  --------------------
> 
>  # systemctl stop virtqemud.socket
>  # systemctl start libvirtd.service
>  # virsh list
>  error: failed to connect to the hypervisor
>  error: Failed to connect socket to '/var/run/libvirt/virtqemud-sock': Connection refused
> 
>  # virsh -c 'qemu:///system?socket=/var/run/libvirt/libvirt-sock' list
>   Id   Name   State
>  --------------------
> 
> Fix this by instructing systemd to delete the socket file when
> deactivating the unit file for the socket.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/remote/libvirtd.socket.in | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/remote/libvirtd.socket.in b/src/remote/libvirtd.socket.in
> index 85b4aa800a..0f349656f5 100644
> --- a/src/remote/libvirtd.socket.in
> +++ b/src/remote/libvirtd.socket.in
> @@ -9,6 +9,7 @@ Before=@service@.service
>  ListenStream=@runstatedir@/libvirt/@sockprefix@-sock
>  Service=@service@.service
>  SocketMode=@mode@
> +RemoveOnStop=yes

I beg your pardon? Systemd leaves a stale socket behind? Isn't this
something that systemd needs to fix?

Michal

Re: [PATCH] remote: systemd: Remove unix sockets from filesystem when disabling a '.socket' unit
Posted by Peter Krempa 2 years, 3 months ago
On Tue, Jan 18, 2022 at 15:51:21 +0100, Michal Prívozník wrote:
> On 1/18/22 15:38, Peter Krempa wrote:

[...]

> > diff --git a/src/remote/libvirtd.socket.in b/src/remote/libvirtd.socket.in
> > index 85b4aa800a..0f349656f5 100644
> > --- a/src/remote/libvirtd.socket.in
> > +++ b/src/remote/libvirtd.socket.in
> > @@ -9,6 +9,7 @@ Before=@service@.service
> >  ListenStream=@runstatedir@/libvirt/@sockprefix@-sock
> >  Service=@service@.service
> >  SocketMode=@mode@
> > +RemoveOnStop=yes
> 
> I beg your pardon? Systemd leaves a stale socket behind? Isn't this
> something that systemd needs to fix?

Well, for that exact reason, they provide a config setting in the unit
file that instruts systemd to delete the socket file when the socket
unit is stopped, and also the exact reason I'm enabling that setting.

Re: [PATCH] remote: systemd: Remove unix sockets from filesystem when disabling a '.socket' unit
Posted by Michal Prívozník 2 years, 3 months ago
On 1/18/22 15:56, Peter Krempa wrote:
> On Tue, Jan 18, 2022 at 15:51:21 +0100, Michal Prívozník wrote:
>> On 1/18/22 15:38, Peter Krempa wrote:
> 
> [...]
> 
>>> diff --git a/src/remote/libvirtd.socket.in b/src/remote/libvirtd.socket.in
>>> index 85b4aa800a..0f349656f5 100644
>>> --- a/src/remote/libvirtd.socket.in
>>> +++ b/src/remote/libvirtd.socket.in
>>> @@ -9,6 +9,7 @@ Before=@service@.service
>>>  ListenStream=@runstatedir@/libvirt/@sockprefix@-sock
>>>  Service=@service@.service
>>>  SocketMode=@mode@
>>> +RemoveOnStop=yes
>>
>> I beg your pardon? Systemd leaves a stale socket behind? Isn't this
>> something that systemd needs to fix?
> 
> Well, for that exact reason, they provide a config setting in the unit
> file that instruts systemd to delete the socket file when the socket
> unit is stopped, and also the exact reason I'm enabling that setting.
> 

That's one of the worst possible defaults I've sever seen. If anything,
units should have been given an option to opt out, not opt in. But
fixing bugs in their respective components instead of libvirt is what we
do (I'm looking at you glib).

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

Re: [PATCH] remote: systemd: Remove unix sockets from filesystem when disabling a '.socket' unit
Posted by Erik Skultety 2 years, 3 months ago
On Tue, Jan 18, 2022 at 03:51:21PM +0100, Michal Prívozník wrote:
> On 1/18/22 15:38, Peter Krempa wrote:
> > The existence of the unix socket path is used by the remote driver to
> > determine whether modular daemons are in use, so if the socket file
> > stays behind and the user decided to switch from modular to monolithic
> > daemon which was socket activated, the remote driver will insist on
> > picking '/var/run/libvirt/virtqemud-sock', even when it's no longer in
> > use:
> > 
> >  # systemctl start libvirtd.service
> >  # virsh list
> >   Id   Name   State
> >  --------------------
> > 
> >  # systemctl stop libvirtd.service
> >  Warning: Stopping libvirtd.service, but it can still be activated by:
> >    libvirtd.socket
> >    libvirtd-ro.socket
> >    libvirtd-admin.socket
> >  # systemctl start virtqemud.socket
> >  # virsh list
> >   Id   Name   State
> >  --------------------
> > 
> >  # systemctl stop virtqemud.socket
> >  # systemctl start libvirtd.service
> >  # virsh list
> >  error: failed to connect to the hypervisor
> >  error: Failed to connect socket to '/var/run/libvirt/virtqemud-sock': Connection refused
> > 
> >  # virsh -c 'qemu:///system?socket=/var/run/libvirt/libvirt-sock' list
> >   Id   Name   State
> >  --------------------
> > 
> > Fix this by instructing systemd to delete the socket file when
> > deactivating the unit file for the socket.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  src/remote/libvirtd.socket.in | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/src/remote/libvirtd.socket.in b/src/remote/libvirtd.socket.in
> > index 85b4aa800a..0f349656f5 100644
> > --- a/src/remote/libvirtd.socket.in
> > +++ b/src/remote/libvirtd.socket.in
> > @@ -9,6 +9,7 @@ Before=@service@.service
> >  ListenStream=@runstatedir@/libvirt/@sockprefix@-sock
> >  Service=@service@.service
> >  SocketMode=@mode@
> > +RemoveOnStop=yes
> 
> I beg your pardon? Systemd leaves a stale socket behind? Isn't this
> something that systemd needs to fix?
> 
> Michal
> 

Re: [PATCH] remote: systemd: Remove unix sockets from filesystem when disabling a '.socket' unit
Posted by Daniel P. Berrangé 2 years, 3 months ago
On Tue, Jan 18, 2022 at 03:51:21PM +0100, Michal Prívozník wrote:
> On 1/18/22 15:38, Peter Krempa wrote:
> > The existence of the unix socket path is used by the remote driver to
> > determine whether modular daemons are in use, so if the socket file
> > stays behind and the user decided to switch from modular to monolithic
> > daemon which was socket activated, the remote driver will insist on
> > picking '/var/run/libvirt/virtqemud-sock', even when it's no longer in
> > use:
> > 
> >  # systemctl start libvirtd.service
> >  # virsh list
> >   Id   Name   State
> >  --------------------
> > 
> >  # systemctl stop libvirtd.service
> >  Warning: Stopping libvirtd.service, but it can still be activated by:
> >    libvirtd.socket
> >    libvirtd-ro.socket
> >    libvirtd-admin.socket
> >  # systemctl start virtqemud.socket
> >  # virsh list
> >   Id   Name   State
> >  --------------------
> > 
> >  # systemctl stop virtqemud.socket
> >  # systemctl start libvirtd.service
> >  # virsh list
> >  error: failed to connect to the hypervisor
> >  error: Failed to connect socket to '/var/run/libvirt/virtqemud-sock': Connection refused
> > 
> >  # virsh -c 'qemu:///system?socket=/var/run/libvirt/libvirt-sock' list
> >   Id   Name   State
> >  --------------------
> > 
> > Fix this by instructing systemd to delete the socket file when
> > deactivating the unit file for the socket.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  src/remote/libvirtd.socket.in | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/src/remote/libvirtd.socket.in b/src/remote/libvirtd.socket.in
> > index 85b4aa800a..0f349656f5 100644
> > --- a/src/remote/libvirtd.socket.in
> > +++ b/src/remote/libvirtd.socket.in
> > @@ -9,6 +9,7 @@ Before=@service@.service
> >  ListenStream=@runstatedir@/libvirt/@sockprefix@-sock
> >  Service=@service@.service
> >  SocketMode=@mode@
> > +RemoveOnStop=yes
> 
> I beg your pardon? Systemd leaves a stale socket behind? Isn't this
> something that systemd needs to fix?

I was surprised but their justification is:

      "Normally, it should
       not be necessary to use this option, and is not recommended as
       services might continue to run after the socket unit has been
       terminated and it should still be possible to communicate with them
       via their file system node. Defaults to off."

I would have some agreement with that view if it were possible to
then start the .socket unit again, and still connect to the .service.
Starting the .socket, however, does an implicit delete.

So leaving the socket on disk is only useful if you want to stop the
.socket, but never start it again.

On balance I think libvirt's need to distinguish daemons is more
important, unless we change the probing logic to actually try a
connect() but I wasn't too enthusiastic about doing that.

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