Re: [systemd-devel] [PATCH] service: don't create extra cgroup for control process when reloading SysV service
On Thu, 13.03.14 09:40, Lukáš Nykrýn (lnyk...@redhat.com) wrote: Exactly. Systemd exec /etc/init.d/foo reload in control subgroup. Than the initscript kills the original deamon, starts a new one and quits. Systemd sees that the reload process finished and kills remaining processes in the control group, thus kills the daemon. This patch works quite fine when the initscripts is using pid files, systemd correctly updates the information about main pid. I see the problem now. But it's really not that simple as the patch.. We need to restart the PID logic if this shall be supported, and rerrange things so that we don't use the control cgroup anymore.. But I must say, I really don't like a change like this. Kay, any opinion? Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: don't create extra cgroup for control process when reloading SysV service
St 12. březen 2014, 18:34:11 CET, Uoti Urpala napsal: On Wed, 2014-03-12 at 16:51 +0100, Lennart Poettering wrote: On Mon, 10.03.14 15:25, Lukas Nykryn (lnyk...@redhat.com) wrote: Unfortunately common practice in initscripts is to have reload as an alias for restart (https://fedoraproject.org/wiki/Packaging:SysVInitScript). In that case the newly started process will be killed immediately after the reload process ends and its cgroup is destroyed. I am not sure I grok why this all would be a problem at all, given that on Fedora/RHEL we redirect those verbs to systemctl anyway, and systemctl handles reload/restart on its own anyway... What am I missing? But systemctl supports using the reload functionality in init scripts, so that doesn't really make a difference. As I understood the problem description, this is what happens: someone runs systemctl reload foo.service for a broken sysv script, systemd sees that the script seems to support a reload argument and runs /etc/init.d/foo reload in a temporary cgroup, but the broken script stops the running service and starts a new one in the temporary cgroup. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel Exactly. Systemd exec /etc/init.d/foo reload in control subgroup. Than the initscript kills the original deamon, starts a new one and quits. Systemd sees that the reload process finished and kills remaining processes in the control group, thus kills the daemon. This patch works quite fine when the initscripts is using pid files, systemd correctly updates the information about main pid. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: don't create extra cgroup for control process when reloading SysV service
On Mon, 10.03.14 15:25, Lukas Nykryn (lnyk...@redhat.com) wrote:
Unfortunately common practice in initscripts is to have reload as an
alias for restart (https://fedoraproject.org/wiki/Packaging:SysVInitScript).
In that case the newly started process will be killed immediately after
the reload process ends and its cgroup is destroyed.
I am pretty convinced that this is one of those cases where we should
accept breakage. Daemons should only be started with start, not
otherwise...
This patch is also quite incorrect, since it will confuse tracking of
the main PID from systemd.
I am not sure I grok why this all would be a problem at all, given that
on Fedora/RHEL we redirect those verbs to systemctl anyway, and
systemctl handles reload/restart on its own anyway... What am I missing?
---
src/core/service.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/core/service.c b/src/core/service.c
index 121ddec..16d7ae0 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -2272,7 +2272,15 @@ static void service_enter_reload(Service *s) {
!s-root_directory_start_only,
false,
false,
+#ifdef HAVE_SYSV_COMPAT
+ /* Don't create extra cgroup for SysV
services.
+ * Unfortunately common practice was to
have reload as an alias
+ * for restart and we are killing the new
main process, when destroying
+ * cgroup for the control process*/
+ !s-is_sysv,
+#else
true,
+#endif
s-control_pid);
if (r 0)
goto fail;
@@ -3082,7 +3090,10 @@ static void service_sigchld_event(Unit *u, pid_t pid,
int code, int status) {
/* Immediately get rid of the cgroup, so that the
* kernel doesn't delay the cgroup empty messages for
* the service cgroup any longer than necessary */
-service_kill_control_processes(s);
+#ifdef HAVE_SYSV_COMPAT
+if (!s-is_sysv)
+#endif
+service_kill_control_processes(s);
if (s-control_command
s-control_command-command_next
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] service: don't create extra cgroup for control process when reloading SysV service
On 03/11/2014 09:05 AM, Michael Biebl wrote: 2014-03-10 15:25 GMT+01:00 Lukas Nykryn lnyk...@redhat.com: Unfortunately common practice in initscripts is to have reload as an alias for restart (https://fedoraproject.org/wiki/Packaging:SysVInitScript). In that case the newly started process will be killed immediately after the reload process ends and its cgroup is destroyed. Interesting. The Debian/Ubuntu packaging policies are quite contrary in that regard. LSB/SysV init scripts only have the reload option, if the daemon actually supports it. There is a force-reload action though, which is mapped to reload, if the daemon supports reload and to restart, if not. Given that, I wonder if this should be maintained as a Fedora specific patch? Debian is getting it right in this regard since LSB defines the behavior of reload to be [1] *reload* cause the configuration of the service to be reloaded without actually stopping and restarting the service In anycase systemd should follow LSB in this regard leaving any downstream distribution having to patch their own common practices in initscripts by themselves. JBG 1. http://refspecs.linuxbase.org/LSB_4.1.0/LSB-Core-generic/LSB-Core-generic/iniscrptact.html ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] service: don't create extra cgroup for control process when reloading SysV service
Unfortunately common practice in initscripts is to have reload as an
alias for restart (https://fedoraproject.org/wiki/Packaging:SysVInitScript).
In that case the newly started process will be killed immediately after
the reload process ends and its cgroup is destroyed.
---
src/core/service.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/core/service.c b/src/core/service.c
index 121ddec..16d7ae0 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -2272,7 +2272,15 @@ static void service_enter_reload(Service *s) {
!s-root_directory_start_only,
false,
false,
+#ifdef HAVE_SYSV_COMPAT
+ /* Don't create extra cgroup for SysV
services.
+ * Unfortunately common practice was to have
reload as an alias
+ * for restart and we are killing the new
main process, when destroying
+ * cgroup for the control process*/
+ !s-is_sysv,
+#else
true,
+#endif
s-control_pid);
if (r 0)
goto fail;
@@ -3082,7 +3090,10 @@ static void service_sigchld_event(Unit *u, pid_t pid,
int code, int status) {
/* Immediately get rid of the cgroup, so that the
* kernel doesn't delay the cgroup empty messages for
* the service cgroup any longer than necessary */
-service_kill_control_processes(s);
+#ifdef HAVE_SYSV_COMPAT
+if (!s-is_sysv)
+#endif
+service_kill_control_processes(s);
if (s-control_command
s-control_command-command_next
--
1.8.5.3
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
