[ovs-dev] rhel: Use PIDFile on forking systemd service files
diff mbox series

Message ID 85725e8f6f5009ef03c91d68152b9d2c0bf52d4b.1551374866.git.tredaelli@redhat.com
State New
Headers show
Series
  • [ovs-dev] rhel: Use PIDFile on forking systemd service files
Related show

Commit Message

Timothy Redaelli Feb. 28, 2019, 5:27 p.m. UTC
Currently, PIDFile is not used in systemd service files with
Type=forking. This means sometimes systemd fails to restart a daemon
that is killed (with SIGKILL) or that is crashed.

This commit adds PIDFile to all systemd service file with Type=forking
in order to always have the correct PID to monitor.

Reported-at: https://bugzilla.redhat.com/1653717
Reported-by: Candido Campos <ccamposr@redhat.com>

Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
---
 rhel/usr_lib_systemd_system_openvswitch-ipsec.service | 1 +
 rhel/usr_lib_systemd_system_ovn-controller.service    | 1 +
 rhel/usr_lib_systemd_system_ovs-vswitchd.service.in   | 1 +
 rhel/usr_lib_systemd_system_ovsdb-server.service      | 1 +
 4 files changed, 4 insertions(+)

Comments

Ben Pfaff Feb. 28, 2019, 6:07 p.m. UTC | #1
On Thu, Feb 28, 2019 at 06:27:46PM +0100, Timothy Redaelli wrote:
> Currently, PIDFile is not used in systemd service files with
> Type=forking. This means sometimes systemd fails to restart a daemon
> that is killed (with SIGKILL) or that is crashed.
> 
> This commit adds PIDFile to all systemd service file with Type=forking
> in order to always have the correct PID to monitor.
> 
> Reported-at: https://bugzilla.redhat.com/1653717
> Reported-by: Candido Campos <ccamposr@redhat.com>
> 
> Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>

Just as a note, I don't know systemd enough to review this, so someone
else will have to review it.
Candido Campos Rivas Feb. 28, 2019, 7:31 p.m. UTC | #2
hi,
 yes, if there are some expert in systemd, it'd be the best :)

I only can help passing you the  systemd doc cof that  recommends use this
option with the forking type :

https://www.freedesktop.org/software/systemd/man/systemd.service.html

.....

Type=

Configures the process start-up type for this service unit. One of simple,
exec, forking, oneshot, dbus, notify or idle:

   -

   If set to forking, it is expected that the process configured with
   ExecStart= will call fork() as part of its start-up. The parent process
   is expected to exit when start-up is complete and all communication
   channels are set up. The child continues to run as the main service
   process, and the service manager will consider the unit started when the
   parent process exits. This is the behavior of traditional UNIX services. If
   this setting is used, it is recommended to also use the PIDFile= option,
   so that systemd can reliably identify the main process of the service.
   systemd will proceed with starting follow-up units as soon as the parent
   process exits.


....

PIDFile=

Takes a path referring to the PID file of the service. Usage of this option
is recommended for services where Type= is set to forking. The path
specified typically points to a file below /run/. If a relative path is
specified it is hence prefixed with /run/. The service manager will read
the PID of the main process of the service from this file after start-up of
the service. The service manager will not write to the file configured
here, although it will remove the file after the service has shut down if
it still exists. The PID file does not need to be owned by a privileged
user, but if it is owned by an unprivileged user additional safety
restrictions are enforced: the file may not be a symlink to a file owned by
a different user (neither directly nor indirectly), and the PID file must
refer to a process already belonging to the service.

BR's

On Thu, Feb 28, 2019 at 7:07 PM Ben Pfaff <blp@ovn.org> wrote:

> On Thu, Feb 28, 2019 at 06:27:46PM +0100, Timothy Redaelli wrote:
> > Currently, PIDFile is not used in systemd service files with
> > Type=forking. This means sometimes systemd fails to restart a daemon
> > that is killed (with SIGKILL) or that is crashed.
> >
> > This commit adds PIDFile to all systemd service file with Type=forking
> > in order to always have the correct PID to monitor.
> >
> > Reported-at: https://bugzilla.redhat.com/1653717
> > Reported-by: Candido Campos <ccamposr@redhat.com>
> >
> > Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
>
> Just as a note, I don't know systemd enough to review this, so someone
> else will have to review it.
>
Flavio Leitner Feb. 28, 2019, 8:06 p.m. UTC | #3
On Thu, Feb 28, 2019 at 06:27:46PM +0100, Timothy Redaelli wrote:
> Currently, PIDFile is not used in systemd service files with
> Type=forking. This means sometimes systemd fails to restart a daemon
> that is killed (with SIGKILL) or that is crashed.
> 
> This commit adds PIDFile to all systemd service file with Type=forking
> in order to always have the correct PID to monitor.
> 
> Reported-at: https://bugzilla.redhat.com/1653717
> Reported-by: Candido Campos <ccamposr@redhat.com>
> 
> Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
> ---

I haven't tested this myself, but it makes sense and it most
probably will make the services management more reliable.

Acked-by: Flavio Leitner <fbl@sysclose.org>
Ben Pfaff Feb. 28, 2019, 10:29 p.m. UTC | #4
On Thu, Feb 28, 2019 at 05:06:40PM -0300, Flavio Leitner wrote:
> On Thu, Feb 28, 2019 at 06:27:46PM +0100, Timothy Redaelli wrote:
> > Currently, PIDFile is not used in systemd service files with
> > Type=forking. This means sometimes systemd fails to restart a daemon
> > that is killed (with SIGKILL) or that is crashed.
> > 
> > This commit adds PIDFile to all systemd service file with Type=forking
> > in order to always have the correct PID to monitor.
> > 
> > Reported-at: https://bugzilla.redhat.com/1653717
> > Reported-by: Candido Campos <ccamposr@redhat.com>
> > 
> > Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
> > ---
> 
> I haven't tested this myself, but it makes sense and it most
> probably will make the services management more reliable.
> 
> Acked-by: Flavio Leitner <fbl@sysclose.org>

Thanks Timothy (and Flavio).  I applied this to master.
Guru Shetty April 22, 2019, 9:02 p.m. UTC | #5
On Thu, 28 Feb 2019 at 09:37, Timothy Redaelli <tredaelli@redhat.com> wrote:

> Currently, PIDFile is not used in systemd service files with
> Type=forking. This means sometimes systemd fails to restart a daemon
> that is killed (with SIGKILL) or that is crashed.
>
> This commit adds PIDFile to all systemd service file with Type=forking
> in order to always have the correct PID to monitor.
>
> Reported-at: https://bugzilla.redhat.com/1653717
> Reported-by: Candido Campos <ccamposr@redhat.com>
>
> Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
>

This should be okay to backport to old branches, correct?



> ---
>  rhel/usr_lib_systemd_system_openvswitch-ipsec.service | 1 +
>  rhel/usr_lib_systemd_system_ovn-controller.service    | 1 +
>  rhel/usr_lib_systemd_system_ovs-vswitchd.service.in   | 1 +
>  rhel/usr_lib_systemd_system_ovsdb-server.service      | 1 +
>  4 files changed, 4 insertions(+)
>
> diff --git a/rhel/usr_lib_systemd_system_openvswitch-ipsec.service
> b/rhel/usr_lib_systemd_system_openvswitch-ipsec.service
> index 6e309aa57..d8f47af68 100644
> --- a/rhel/usr_lib_systemd_system_openvswitch-ipsec.service
> +++ b/rhel/usr_lib_systemd_system_openvswitch-ipsec.service
> @@ -5,6 +5,7 @@ After=openvswitch.service
>
>  [Service]
>  Type=forking
> +PIDFile=/var/run/openvswitch/ovs-monitor-ipsec.pid
>  ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
>                      --ike-daemon=libreswan start-ovs-ipsec
>  ExecStop=/usr/share/openvswitch/scripts/ovs-ctl stop-ovs-ipsec
> diff --git a/rhel/usr_lib_systemd_system_ovn-controller.service
> b/rhel/usr_lib_systemd_system_ovn-controller.service
> index 283e581df..cf65988fe 100644
> --- a/rhel/usr_lib_systemd_system_ovn-controller.service
> +++ b/rhel/usr_lib_systemd_system_ovn-controller.service
> @@ -21,6 +21,7 @@ After=openvswitch.service
>
>  [Service]
>  Type=forking
> +PIDFile=/var/run/openvswitch/ovn-controller.pid
>  Restart=on-failure
>  EnvironmentFile=-/etc/sysconfig/ovn-controller
>  ExecStart=/usr/share/openvswitch/scripts/ovn-ctl --no-monitor \
> diff --git a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in b/rhel/
> usr_lib_systemd_system_ovs-vswitchd.service.in
> index 525deae0b..82925133d 100644
> --- a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
> +++ b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
> @@ -9,6 +9,7 @@ PartOf=openvswitch.service
>
>  [Service]
>  Type=forking
> +PIDFile=/var/run/openvswitch/ovs-vswitchd.pid
>  Restart=on-failure
>  Environment=XDG_RUNTIME_DIR=/var/run/openvswitch
>  EnvironmentFile=/etc/openvswitch/default.conf
> diff --git a/rhel/usr_lib_systemd_system_ovsdb-server.service
> b/rhel/usr_lib_systemd_system_ovsdb-server.service
> index 70da1ec95..41ac2dded 100644
> --- a/rhel/usr_lib_systemd_system_ovsdb-server.service
> +++ b/rhel/usr_lib_systemd_system_ovsdb-server.service
> @@ -7,6 +7,7 @@ PartOf=openvswitch.service
>
>  [Service]
>  Type=forking
> +PIDFile=/var/run/openvswitch/ovsdb-server.pid
>  Restart=on-failure
>  EnvironmentFile=/etc/openvswitch/default.conf
>  EnvironmentFile=-/etc/sysconfig/openvswitch
> --
> 2.20.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Guru Shetty April 23, 2019, 5:57 p.m. UTC | #6
On Mon, 22 Apr 2019 at 14:02, Guru Shetty <guru@ovn.org> wrote:

>
>
> On Thu, 28 Feb 2019 at 09:37, Timothy Redaelli <tredaelli@redhat.com>
> wrote:
>
>> Currently, PIDFile is not used in systemd service files with
>> Type=forking. This means sometimes systemd fails to restart a daemon
>> that is killed (with SIGKILL) or that is crashed.
>>
>> This commit adds PIDFile to all systemd service file with Type=forking
>> in order to always have the correct PID to monitor.
>>
>> Reported-at: https://bugzilla.redhat.com/1653717
>> Reported-by: Candido Campos <ccamposr@redhat.com>
>>
>> Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
>>
>
> This should be okay to backport to old branches, correct?
>
I took the liberty to backport this to 2.11, 2.10 and 2.9 as it fixes a
couple of genuine issues.



>
>
>
>> ---
>>  rhel/usr_lib_systemd_system_openvswitch-ipsec.service | 1 +
>>  rhel/usr_lib_systemd_system_ovn-controller.service    | 1 +
>>  rhel/usr_lib_systemd_system_ovs-vswitchd.service.in   | 1 +
>>  rhel/usr_lib_systemd_system_ovsdb-server.service      | 1 +
>>  4 files changed, 4 insertions(+)
>>
>> diff --git a/rhel/usr_lib_systemd_system_openvswitch-ipsec.service
>> b/rhel/usr_lib_systemd_system_openvswitch-ipsec.service
>> index 6e309aa57..d8f47af68 100644
>> --- a/rhel/usr_lib_systemd_system_openvswitch-ipsec.service
>> +++ b/rhel/usr_lib_systemd_system_openvswitch-ipsec.service
>> @@ -5,6 +5,7 @@ After=openvswitch.service
>>
>>  [Service]
>>  Type=forking
>> +PIDFile=/var/run/openvswitch/ovs-monitor-ipsec.pid
>>  ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
>>                      --ike-daemon=libreswan start-ovs-ipsec
>>  ExecStop=/usr/share/openvswitch/scripts/ovs-ctl stop-ovs-ipsec
>> diff --git a/rhel/usr_lib_systemd_system_ovn-controller.service
>> b/rhel/usr_lib_systemd_system_ovn-controller.service
>> index 283e581df..cf65988fe 100644
>> --- a/rhel/usr_lib_systemd_system_ovn-controller.service
>> +++ b/rhel/usr_lib_systemd_system_ovn-controller.service
>> @@ -21,6 +21,7 @@ After=openvswitch.service
>>
>>  [Service]
>>  Type=forking
>> +PIDFile=/var/run/openvswitch/ovn-controller.pid
>>  Restart=on-failure
>>  EnvironmentFile=-/etc/sysconfig/ovn-controller
>>  ExecStart=/usr/share/openvswitch/scripts/ovn-ctl --no-monitor \
>> diff --git a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in b/rhel/
>> usr_lib_systemd_system_ovs-vswitchd.service.in
>> index 525deae0b..82925133d 100644
>> --- a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
>> +++ b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
>> @@ -9,6 +9,7 @@ PartOf=openvswitch.service
>>
>>  [Service]
>>  Type=forking
>> +PIDFile=/var/run/openvswitch/ovs-vswitchd.pid
>>  Restart=on-failure
>>  Environment=XDG_RUNTIME_DIR=/var/run/openvswitch
>>  EnvironmentFile=/etc/openvswitch/default.conf
>> diff --git a/rhel/usr_lib_systemd_system_ovsdb-server.service
>> b/rhel/usr_lib_systemd_system_ovsdb-server.service
>> index 70da1ec95..41ac2dded 100644
>> --- a/rhel/usr_lib_systemd_system_ovsdb-server.service
>> +++ b/rhel/usr_lib_systemd_system_ovsdb-server.service
>> @@ -7,6 +7,7 @@ PartOf=openvswitch.service
>>
>>  [Service]
>>  Type=forking
>> +PIDFile=/var/run/openvswitch/ovsdb-server.pid
>>  Restart=on-failure
>>  EnvironmentFile=/etc/openvswitch/default.conf
>>  EnvironmentFile=-/etc/sysconfig/openvswitch
>> --
>> 2.20.1
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
Candido Campos Rivas April 24, 2019, 5:45 a.m. UTC | #7
I think that it s a good configuration change whithout risks :)

On Tue, Apr 23, 2019 at 7:57 PM Guru Shetty <guru@ovn.org> wrote:

>
>
> On Mon, 22 Apr 2019 at 14:02, Guru Shetty <guru@ovn.org> wrote:
>
>>
>>
>> On Thu, 28 Feb 2019 at 09:37, Timothy Redaelli <tredaelli@redhat.com>
>> wrote:
>>
>>> Currently, PIDFile is not used in systemd service files with
>>> Type=forking. This means sometimes systemd fails to restart a daemon
>>> that is killed (with SIGKILL) or that is crashed.
>>>
>>> This commit adds PIDFile to all systemd service file with Type=forking
>>> in order to always have the correct PID to monitor.
>>>
>>> Reported-at: https://bugzilla.redhat.com/1653717
>>> Reported-by: Candido Campos <ccamposr@redhat.com>
>>>
>>> Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
>>>
>>
>> This should be okay to backport to old branches, correct?
>>
> I took the liberty to backport this to 2.11, 2.10 and 2.9 as it fixes a
> couple of genuine issues.
>
>
>
>>
>>
>>
>>> ---
>>>  rhel/usr_lib_systemd_system_openvswitch-ipsec.service | 1 +
>>>  rhel/usr_lib_systemd_system_ovn-controller.service    | 1 +
>>>  rhel/usr_lib_systemd_system_ovs-vswitchd.service.in   | 1 +
>>>  rhel/usr_lib_systemd_system_ovsdb-server.service      | 1 +
>>>  4 files changed, 4 insertions(+)
>>>
>>> diff --git a/rhel/usr_lib_systemd_system_openvswitch-ipsec.service
>>> b/rhel/usr_lib_systemd_system_openvswitch-ipsec.service
>>> index 6e309aa57..d8f47af68 100644
>>> --- a/rhel/usr_lib_systemd_system_openvswitch-ipsec.service
>>> +++ b/rhel/usr_lib_systemd_system_openvswitch-ipsec.service
>>> @@ -5,6 +5,7 @@ After=openvswitch.service
>>>
>>>  [Service]
>>>  Type=forking
>>> +PIDFile=/var/run/openvswitch/ovs-monitor-ipsec.pid
>>>  ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
>>>                      --ike-daemon=libreswan start-ovs-ipsec
>>>  ExecStop=/usr/share/openvswitch/scripts/ovs-ctl stop-ovs-ipsec
>>> diff --git a/rhel/usr_lib_systemd_system_ovn-controller.service
>>> b/rhel/usr_lib_systemd_system_ovn-controller.service
>>> index 283e581df..cf65988fe 100644
>>> --- a/rhel/usr_lib_systemd_system_ovn-controller.service
>>> +++ b/rhel/usr_lib_systemd_system_ovn-controller.service
>>> @@ -21,6 +21,7 @@ After=openvswitch.service
>>>
>>>  [Service]
>>>  Type=forking
>>> +PIDFile=/var/run/openvswitch/ovn-controller.pid
>>>  Restart=on-failure
>>>  EnvironmentFile=-/etc/sysconfig/ovn-controller
>>>  ExecStart=/usr/share/openvswitch/scripts/ovn-ctl --no-monitor \
>>> diff --git a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in b/rhel/
>>> usr_lib_systemd_system_ovs-vswitchd.service.in
>>> index 525deae0b..82925133d 100644
>>> --- a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
>>> +++ b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
>>> @@ -9,6 +9,7 @@ PartOf=openvswitch.service
>>>
>>>  [Service]
>>>  Type=forking
>>> +PIDFile=/var/run/openvswitch/ovs-vswitchd.pid
>>>  Restart=on-failure
>>>  Environment=XDG_RUNTIME_DIR=/var/run/openvswitch
>>>  EnvironmentFile=/etc/openvswitch/default.conf
>>> diff --git a/rhel/usr_lib_systemd_system_ovsdb-server.service
>>> b/rhel/usr_lib_systemd_system_ovsdb-server.service
>>> index 70da1ec95..41ac2dded 100644
>>> --- a/rhel/usr_lib_systemd_system_ovsdb-server.service
>>> +++ b/rhel/usr_lib_systemd_system_ovsdb-server.service
>>> @@ -7,6 +7,7 @@ PartOf=openvswitch.service
>>>
>>>  [Service]
>>>  Type=forking
>>> +PIDFile=/var/run/openvswitch/ovsdb-server.pid
>>>  Restart=on-failure
>>>  EnvironmentFile=/etc/openvswitch/default.conf
>>>  EnvironmentFile=-/etc/sysconfig/openvswitch
>>> --
>>> 2.20.1
>>>
>>> _______________________________________________
>>> dev mailing list
>>> dev@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>

Patch
diff mbox series

diff --git a/rhel/usr_lib_systemd_system_openvswitch-ipsec.service b/rhel/usr_lib_systemd_system_openvswitch-ipsec.service
index 6e309aa57..d8f47af68 100644
--- a/rhel/usr_lib_systemd_system_openvswitch-ipsec.service
+++ b/rhel/usr_lib_systemd_system_openvswitch-ipsec.service
@@ -5,6 +5,7 @@  After=openvswitch.service
 
 [Service]
 Type=forking
+PIDFile=/var/run/openvswitch/ovs-monitor-ipsec.pid
 ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
                     --ike-daemon=libreswan start-ovs-ipsec
 ExecStop=/usr/share/openvswitch/scripts/ovs-ctl stop-ovs-ipsec
diff --git a/rhel/usr_lib_systemd_system_ovn-controller.service b/rhel/usr_lib_systemd_system_ovn-controller.service
index 283e581df..cf65988fe 100644
--- a/rhel/usr_lib_systemd_system_ovn-controller.service
+++ b/rhel/usr_lib_systemd_system_ovn-controller.service
@@ -21,6 +21,7 @@  After=openvswitch.service
 
 [Service]
 Type=forking
+PIDFile=/var/run/openvswitch/ovn-controller.pid
 Restart=on-failure
 EnvironmentFile=-/etc/sysconfig/ovn-controller
 ExecStart=/usr/share/openvswitch/scripts/ovn-ctl --no-monitor \
diff --git a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
index 525deae0b..82925133d 100644
--- a/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
+++ b/rhel/usr_lib_systemd_system_ovs-vswitchd.service.in
@@ -9,6 +9,7 @@  PartOf=openvswitch.service
 
 [Service]
 Type=forking
+PIDFile=/var/run/openvswitch/ovs-vswitchd.pid
 Restart=on-failure
 Environment=XDG_RUNTIME_DIR=/var/run/openvswitch
 EnvironmentFile=/etc/openvswitch/default.conf
diff --git a/rhel/usr_lib_systemd_system_ovsdb-server.service b/rhel/usr_lib_systemd_system_ovsdb-server.service
index 70da1ec95..41ac2dded 100644
--- a/rhel/usr_lib_systemd_system_ovsdb-server.service
+++ b/rhel/usr_lib_systemd_system_ovsdb-server.service
@@ -7,6 +7,7 @@  PartOf=openvswitch.service
 
 [Service]
 Type=forking
+PIDFile=/var/run/openvswitch/ovsdb-server.pid
 Restart=on-failure
 EnvironmentFile=/etc/openvswitch/default.conf
 EnvironmentFile=-/etc/sysconfig/openvswitch