| Message ID | 85725e8f6f5009ef03c91d68152b9d2c0bf52d4b.1551374866.git.tredaelli@redhat.com |
|---|---|
| State | Accepted |
| Commit | f385abded52064364e13a188d1ddfcb02dce4df7 |
| Headers | show |
| Series | [ovs-dev] rhel: Use PIDFile on forking systemd service files | expand |
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.
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. >
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>
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.
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 >
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 >> >
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 >>> >>
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
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(+)