daemons: write pid file even when told not to daemonize
PATCH 1/1 daemons: write pid file even when told not to daemonize
systemd wants to run daemons in foreground, but daemons wouldn't write out the pid file with -f. Fixed.
--- src/ceph_mon.cc | 3 +-- src/common/config.cc | 2 -- src/global/global_init.cc | 10 +++++++++- 3 files changed, 10 insertions(+), 5 deletions(-)
src/ceph_mon.cc +1 -2
@@ -406,8 +406,7 @@ int main(int argc, const char **argv) // screwing us over Preforker prefork; if (!(flags & CINIT_FLAG_NO_DAEMON_ACTIONS)) { - if (g_conf->daemonize) { - global_init_prefork(g_ceph_context, 0); + if (global_init_prefork(g_ceph_context, 0) >= 0) { prefork.prefork(); if (prefork.is_parent()) { return prefork.parent_wait();
src/common/config.cc +0 -2
@@ -389,12 +389,10 @@ int md_config_t::parse_argv(std::vector<const char*>& args) } else if (ceph_argparse_flag(args, i, "--foreground", "-f", (char*)NULL)) { set_val_or_die("daemonize", "false"); - set_val_or_die("pid_file", ""); } else if (ceph_argparse_flag(args, i, "-d", (char*)NULL)) { set_val_or_die("daemonize", "false"); set_val_or_die("log_file", ""); - set_val_or_die("pid_file", ""); set_val_or_die("log_to_stderr", "true"); set_val_or_die("err_to_stderr", "true"); set_val_or_die("log_to_syslog", "false");
src/global/global_init.cc +9 -1
@@ -166,8 +166,16 @@ int global_init_prefork(CephContext *cct, int flags) if (g_code_env != CODE_ENVIRONMENT_DAEMON) return -1; const md_config_t *conf = cct->_conf; - if (!conf->daemonize) + if (!conf->daemonize) { + if (atexit(pidfile_remove_void)) { + derr << "global_init_daemonize: failed to set pidfile_remove function " + << "to run at exit." << dendl; + } + + pidfile_write(g_conf); + return -1; + } // stop log thread g_ceph_context->_log->flush();
Signature · 4 lines
Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/ You must be the change you wish to see in the world. -- Gandhi Be Free! -- http://FSFLA.org/ FSF Latin America board member Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Hi Alexandre,
With this patch ceph-osd -f will try to create the default pid file : this is a non backward compatible change. Maybe there is a way for systemd to capture the pid of the process and store it instead of requiring the deamon to create the pid file ?
Cheers
Quoted 64 lines · Alexandre Oliva
systemd wants to run daemons in foreground, but daemons wouldn't write out the pid file with -f. Fixed.
Signed-off-by Alexandre Oliva <[email protected]>--- src/ceph_mon.cc | 3 +-- src/common/config.cc | 2 -- src/global/global_init.cc | 10 +++++++++- 3 files changed, 10 insertions(+), 5 deletions(-)src/ceph_mon.cc +1
-2@@ -406,8 +406,7 @@ int main(int argc, const char **argv) // screwing us over Preforker prefork; if (!(flags & CINIT_FLAG_NO_DAEMON_ACTIONS)) { - if (g_conf->daemonize) { - global_init_prefork(g_ceph_context, 0); + if (global_init_prefork(g_ceph_context, 0) >= 0) { prefork.prefork(); if (prefork.is_parent()) { return prefork.parent_wait();src/common/config.cc +0
-2@@ -389,12 +389,10 @@ int md_config_t::parse_argv(std::vector<const char*>& args) } else if (ceph_argparse_flag(args, i, "--foreground", "-f", (char*)NULL)) { set_val_or_die("daemonize", "false"); - set_val_or_die("pid_file", ""); } else if (ceph_argparse_flag(args, i, "-d", (char*)NULL)) { set_val_or_die("daemonize", "false"); set_val_or_die("log_file", ""); - set_val_or_die("pid_file", ""); set_val_or_die("log_to_stderr", "true"); set_val_or_die("err_to_stderr", "true"); set_val_or_die("log_to_syslog", "false");src/global/global_init.cc +9
-1@@ -166,8 +166,16 @@ int global_init_prefork(CephContext *cct, int flags) if (g_code_env != CODE_ENVIRONMENT_DAEMON) return -1; const md_config_t *conf = cct->_conf; - if (!conf->daemonize) + if (!conf->daemonize) { + if (atexit(pidfile_remove_void)) { + derr << "global_init_daemonize: failed to set pidfile_remove function " + << "to run at exit." << dendl; + } + + pidfile_write(g_conf); + return -1; + } // stop log thread g_ceph_context->_log->flush();
Signature · 1 line
Loïc Dachary, Artisan Logiciel Libre
Quoted 6 lines · Loic Dachary
Hi Alexandre,
With this patch ceph-osd -f will try to create the default pid file :
this is a non backward compatible change. Maybe there is a way for systemd to capture the pid of the process and store it instead of requiring the deamon to create the pid file ?
Do we need the pid file at all when we aren't using sysinit?
sage
Quoted 73 lines
Cheers
Quoted 64 lines · Alexandre Oliva
systemd wants to run daemons in foreground, but daemons wouldn't write out the pid file with -f. Fixed.
Signed-off-by Alexandre Oliva <[email protected]>--- src/ceph_mon.cc | 3 +-- src/common/config.cc | 2 -- src/global/global_init.cc | 10 +++++++++- 3 files changed, 10 insertions(+), 5 deletions(-)src/ceph_mon.cc +1
-2@@ -406,8 +406,7 @@ int main(int argc, const char **argv) // screwing us over Preforker prefork; if (!(flags & CINIT_FLAG_NO_DAEMON_ACTIONS)) { - if (g_conf->daemonize) { - global_init_prefork(g_ceph_context, 0); + if (global_init_prefork(g_ceph_context, 0) >= 0) { prefork.prefork(); if (prefork.is_parent()) { return prefork.parent_wait();src/common/config.cc +0
-2@@ -389,12 +389,10 @@ int md_config_t::parse_argv(std::vector<const char*>& args) } else if (ceph_argparse_flag(args, i, "--foreground", "-f", (char*)NULL)) { set_val_or_die("daemonize", "false"); - set_val_or_die("pid_file", ""); } else if (ceph_argparse_flag(args, i, "-d", (char*)NULL)) { set_val_or_die("daemonize", "false"); set_val_or_die("log_file", ""); - set_val_or_die("pid_file", ""); set_val_or_die("log_to_stderr", "true"); set_val_or_die("err_to_stderr", "true"); set_val_or_die("log_to_syslog", "false");src/global/global_init.cc +9
-1@@ -166,8 +166,16 @@ int global_init_prefork(CephContext *cct, int flags) if (g_code_env != CODE_ENVIRONMENT_DAEMON) return -1; const md_config_t *conf = cct->_conf; - if (!conf->daemonize) + if (!conf->daemonize) { + if (atexit(pidfile_remove_void)) { + derr << "global_init_daemonize: failed to set pidfile_remove function " + << "to run at exit." << dendl; + } + + pidfile_write(g_conf); + return -1; + } // stop log thread g_ceph_context->_log->flush();Signature · 1 line
Lo?c Dachary, Artisan Logiciel Libre
Quoted 9 lines · Sage Weil
Quoted 6 lines · Loic Dachary
Hi Alexandre,
With this patch ceph-osd -f will try to create the default pid file :
this is a non backward compatible change. Maybe there is a way for systemd to capture the pid of the process and store it instead of requiring the deamon to create the pid file ?Do we need the pid file at all when we aren't using sysinit?
My own monitoring scripts use it, and ceph stop use it as well. I was surprised we were not creating them in spite of an explicit command line option to do so.
Signature · 4 lines
Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/ You must be the change you wish to see in the world. -- Gandhi Be Free! -- http://FSFLA.org/ FSF Latin America board member Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Hi Alexandre,
Changing this behavior is not backward compatible but it is indeed more intuitive. Has it been a significant inconvenience so far ?
Cheers
Quoted 16 lines · Alexandre Oliva
Quoted 9 lines · Sage Weil
Quoted 6 lines · Loic Dachary
Hi Alexandre,
With this patch ceph-osd -f will try to create the default pid file :
this is a non backward compatible change. Maybe there is a way for systemd to capture the pid of the process and store it instead of requiring the deamon to create the pid file ?Do we need the pid file at all when we aren't using sysinit?
My own monitoring scripts use it, and ceph stop use it as well. I was surprised we were not creating them in spite of an explicit command line option to do so.
Signature · 1 line
Loïc Dachary, Artisan Logiciel Libre
Quoted 1 line · Loic Dachary
Changing this behavior is not backward compatible but it is indeed more intuitive. Has it been a significant inconvenience so far ?
Before I wrote the patch, it was very inconvenient: in order to stop ceph services, I had to dig up the PIDs from ps output and then kill the processes manually, and I had to run ceph without my monitor that automatically restarted services that died (checking that the pid file was absent, or that the PID it named was not a running process).
After I applied the patch in my local build, I could just forget about the earlier problems, and none surfaced because of the creation of the PID file.
Is this what you were asking?
Quoted 16 lines · Alexandre Oliva
Quoted 15 lines · Sage Weil
Quoted 10 lines · Loic Dachary
Quoted 6 lines
Hi Alexandre,
With this patch ceph-osd -f will try to create the default pid file :
this is a non backward compatible change. Maybe there is a way for systemd to capture the pid of the process and store it instead of requiring the deamon to create the pid file ?Do we need the pid file at all when we aren't using sysinit?
My own monitoring scripts use it, and ceph stop use it as well. I was surprised we were not creating them in spite of an explicit command line option to do so.
Signature · 4 lines
Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/ You must be the change you wish to see in the world. -- Gandhi Be Free! -- http://FSFLA.org/ FSF Latin America board member Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Quoted 10 lines · Alexandre Oliva
Quoted 1 line · Loic Dachary
Changing this behavior is not backward compatible but it is indeed more intuitive. Has it been a significant inconvenience so far ?
Before I wrote the patch, it was very inconvenient: in order to stop ceph services, I had to dig up the PIDs from ps output and then kill the processes manually, and I had to run ceph without my monitor that automatically restarted services that died (checking that the pid file was absent, or that the PID it named was not a running process).
Quoted 5 lines
After I applied the patch in my local build, I could just forget about the earlier problems, and none surfaced because of the creation of the PID file.
Is this what you were asking?
Hi Alexandre,
After a quick glance at the systemd man page http://www.freedesktop.org/software/systemd/man/systemd.service.html#Options it seems possible to do something like
https://github.com/dachary/ceph/compare/wip-systemd?expand=1
Alternatively, shouldn't systemctl kill [email protected] be the canonical way of killing a process spawned by systemd ?
If both are impractical or frowned upon for some reason, it looks like your patch is the only way to go, indeed.
Cheers
Quoted 18 lines
Quoted 17 lines · Alexandre Oliva
Quoted 15 lines · Sage Weil
Quoted 10 lines · Loic Dachary
Quoted 6 lines
Hi Alexandre,
With this patch ceph-osd -f will try to create the default pid file :
this is a non backward compatible change. Maybe there is a way for systemd to capture the pid of the process and store it instead of requiring the deamon to create the pid file ?Do we need the pid file at all when we aren't using sysinit?
My own monitoring scripts use it, and ceph stop use it as well. I was surprised we were not creating them in spite of an explicit command line option to do so.
Signature · 1 line
Loïc Dachary, Artisan Logiciel Libre
Quoted 3 lines · Loic Dachary
After a quick glance at the systemd man page http://www.freedesktop.org/software/systemd/man/systemd.service.html#Options it seems possible to do something like
https://github.com/dachary/ceph/compare/wip-systemd?expand=1
Yeah, it sure is possible in systemd to start and track daemons that don't have a “run in foreground” option, but it's not as efficient.
Quoted 1 line
Alternatively, shouldn't systemctl kill [email protected] be the canonical way of killing a process spawned by systemd ?
If you know what number to use after @, I guess it would. I don't think we're talking about the same version of ceph, when it comes to systemd config files. Your patch doesn't modify any file present on my system (still running 0.80.5)
Quoted 2 lines
If both are impractical or frowned upon for some reason, it looks like your patch is the only way to go, indeed.
It's certainly not the only way to go, but silently dropping a command line option sounds like a bug to me. Of course the patch I proposed isn't the only way to go about that...
Signature · 8 lines
Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/ You must be the change you wish to see in the world. -- Gandhi Be Free! -- http://FSFLA.org/ FSF Latin America board member Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
I'm inlined to just apply this original patch. Is there any reason *not* to create a pid file when -f is used? It's a change from previous behavior, but is there any possible harm that can come of it?
sage
Quoted 75 lines · Alexandre Oliva
systemd wants to run daemons in foreground, but daemons wouldn't write out the pid file with -f. Fixed.
Signed-off-by Alexandre Oliva <[email protected]>--- src/ceph_mon.cc | 3 +-- src/common/config.cc | 2 -- src/global/global_init.cc | 10 +++++++++- 3 files changed, 10 insertions(+), 5 deletions(-)src/ceph_mon.cc +1
-2@@ -406,8 +406,7 @@ int main(int argc, const char **argv) // screwing us over Preforker prefork; if (!(flags & CINIT_FLAG_NO_DAEMON_ACTIONS)) { - if (g_conf->daemonize) { - global_init_prefork(g_ceph_context, 0); + if (global_init_prefork(g_ceph_context, 0) >= 0) { prefork.prefork(); if (prefork.is_parent()) { return prefork.parent_wait();src/common/config.cc +0
-2@@ -389,12 +389,10 @@ int md_config_t::parse_argv(std::vector<const char*>& args) } else if (ceph_argparse_flag(args, i, "--foreground", "-f", (char*)NULL)) { set_val_or_die("daemonize", "false"); - set_val_or_die("pid_file", ""); } else if (ceph_argparse_flag(args, i, "-d", (char*)NULL)) { set_val_or_die("daemonize", "false"); set_val_or_die("log_file", ""); - set_val_or_die("pid_file", ""); set_val_or_die("log_to_stderr", "true"); set_val_or_die("err_to_stderr", "true"); set_val_or_die("log_to_syslog", "false");src/global/global_init.cc +9
-1@@ -166,8 +166,16 @@ int global_init_prefork(CephContext *cct, int flags) if (g_code_env != CODE_ENVIRONMENT_DAEMON) return -1; const md_config_t *conf = cct->_conf; - if (!conf->daemonize) + if (!conf->daemonize) { + if (atexit(pidfile_remove_void)) { + derr << "global_init_daemonize: failed to set pidfile_remove function " + << "to run at exit." << dendl; + } + + pidfile_write(g_conf); + return -1; + } // stop log thread g_ceph_context->_log->flush();Signature · 9 lines
-- Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/ You must be the change you wish to see in the world. -- Gandhi Be Free! -- http://FSFLA.org/ FSF Latin America board member Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
The only reason I can think of is that it requires write permission to a directory that was previously not used. I think it will break a few unit tests but nothing we can't fix.
Quoted 89 lines · Sage Weil
I'm inlined to just apply this original patch. Is there any reason *not* to create a pid file when -f is used? It's a change from previous behavior, but is there any possible harm that can come of it?
sage
Quoted 75 lines · Alexandre Oliva
systemd wants to run daemons in foreground, but daemons wouldn't write out the pid file with -f. Fixed.
Signed-off-by Alexandre Oliva <[email protected]>--- src/ceph_mon.cc | 3 +-- src/common/config.cc | 2 -- src/global/global_init.cc | 10 +++++++++- 3 files changed, 10 insertions(+), 5 deletions(-)src/ceph_mon.cc +1
-2@@ -406,8 +406,7 @@ int main(int argc, const char **argv) // screwing us over Preforker prefork; if (!(flags & CINIT_FLAG_NO_DAEMON_ACTIONS)) { - if (g_conf->daemonize) { - global_init_prefork(g_ceph_context, 0); + if (global_init_prefork(g_ceph_context, 0) >= 0) { prefork.prefork(); if (prefork.is_parent()) { return prefork.parent_wait();src/common/config.cc +0
-2@@ -389,12 +389,10 @@ int md_config_t::parse_argv(std::vector<const char*>& args) } else if (ceph_argparse_flag(args, i, "--foreground", "-f", (char*)NULL)) { set_val_or_die("daemonize", "false"); - set_val_or_die("pid_file", ""); } else if (ceph_argparse_flag(args, i, "-d", (char*)NULL)) { set_val_or_die("daemonize", "false"); set_val_or_die("log_file", ""); - set_val_or_die("pid_file", ""); set_val_or_die("log_to_stderr", "true"); set_val_or_die("err_to_stderr", "true"); set_val_or_die("log_to_syslog", "false");src/global/global_init.cc +9
-1@@ -166,8 +166,16 @@ int global_init_prefork(CephContext *cct, int flags) if (g_code_env != CODE_ENVIRONMENT_DAEMON) return -1; const md_config_t *conf = cct->_conf; - if (!conf->daemonize) + if (!conf->daemonize) { + if (atexit(pidfile_remove_void)) { + derr << "global_init_daemonize: failed to set pidfile_remove function " + << "to run at exit." << dendl; + } + + pidfile_write(g_conf); + return -1; + } // stop log thread g_ceph_context->_log->flush();Signature · 9 lines
-- Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/ You must be the change you wish to see in the world. -- Gandhi Be Free! -- http://FSFLA.org/ FSF Latin America board member Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 1 line
Loïc Dachary, Artisan Logiciel Libre
Quoted 3 lines · Loic Dachary
The only reason I can think of is that it requires write permission to a directory that was previously not used. I think it will break a few unit tests but nothing we can't fix.
Let's go for it then and clean up whatever breaks. We may want to change the default value for pid_file for some contexts, but I agree with Alexandre that that will be a better situation that the funny -f/-d side-effect we have now!
sage
Quoted 96 lines · Sage Weil
Quoted 89 lines
I'm inlined to just apply this original patch. Is there any reason *not* to create a pid file when -f is used? It's a change from previous behavior, but is there any possible harm that can come of it?
sage
Quoted 75 lines · Alexandre Oliva
systemd wants to run daemons in foreground, but daemons wouldn't write out the pid file with -f. Fixed.
Signed-off-by Alexandre Oliva <[email protected]>--- src/ceph_mon.cc | 3 +-- src/common/config.cc | 2 -- src/global/global_init.cc | 10 +++++++++- 3 files changed, 10 insertions(+), 5 deletions(-)src/ceph_mon.cc +1
-2@@ -406,8 +406,7 @@ int main(int argc, const char **argv) // screwing us over Preforker prefork; if (!(flags & CINIT_FLAG_NO_DAEMON_ACTIONS)) { - if (g_conf->daemonize) { - global_init_prefork(g_ceph_context, 0); + if (global_init_prefork(g_ceph_context, 0) >= 0) { prefork.prefork(); if (prefork.is_parent()) { return prefork.parent_wait();src/common/config.cc +0
-2@@ -389,12 +389,10 @@ int md_config_t::parse_argv(std::vector<const char*>& args) } else if (ceph_argparse_flag(args, i, "--foreground", "-f", (char*)NULL)) { set_val_or_die("daemonize", "false"); - set_val_or_die("pid_file", ""); } else if (ceph_argparse_flag(args, i, "-d", (char*)NULL)) { set_val_or_die("daemonize", "false"); set_val_or_die("log_file", ""); - set_val_or_die("pid_file", ""); set_val_or_die("log_to_stderr", "true"); set_val_or_die("err_to_stderr", "true"); set_val_or_die("log_to_syslog", "false");src/global/global_init.cc +9
-1@@ -166,8 +166,16 @@ int global_init_prefork(CephContext *cct, int flags) if (g_code_env != CODE_ENVIRONMENT_DAEMON) return -1; const md_config_t *conf = cct->_conf; - if (!conf->daemonize) + if (!conf->daemonize) { + if (atexit(pidfile_remove_void)) { + derr << "global_init_daemonize: failed to set pidfile_remove function " + << "to run at exit." << dendl; + } + + pidfile_write(g_conf); + return -1; + } // stop log thread g_ceph_context->_log->flush();Signature · 9 lines
-- Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/ You must be the change you wish to see in the world. -- Gandhi Be Free! -- http://FSFLA.org/ FSF Latin America board member Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.htmlSignature · 1 line
Lo?c Dachary, Artisan Logiciel Libre
Hi Alexandre,
I applied the patch locally and running make check.
Cheers
Quoted 64 lines · Alexandre Oliva
systemd wants to run daemons in foreground, but daemons wouldn't write out the pid file with -f. Fixed.
Signed-off-by Alexandre Oliva <[email protected]>--- src/ceph_mon.cc | 3 +-- src/common/config.cc | 2 -- src/global/global_init.cc | 10 +++++++++- 3 files changed, 10 insertions(+), 5 deletions(-)src/ceph_mon.cc +1
-2@@ -406,8 +406,7 @@ int main(int argc, const char **argv) // screwing us over Preforker prefork; if (!(flags & CINIT_FLAG_NO_DAEMON_ACTIONS)) { - if (g_conf->daemonize) { - global_init_prefork(g_ceph_context, 0); + if (global_init_prefork(g_ceph_context, 0) >= 0) { prefork.prefork(); if (prefork.is_parent()) { return prefork.parent_wait();src/common/config.cc +0
-2@@ -389,12 +389,10 @@ int md_config_t::parse_argv(std::vector<const char*>& args) } else if (ceph_argparse_flag(args, i, "--foreground", "-f", (char*)NULL)) { set_val_or_die("daemonize", "false"); - set_val_or_die("pid_file", ""); } else if (ceph_argparse_flag(args, i, "-d", (char*)NULL)) { set_val_or_die("daemonize", "false"); set_val_or_die("log_file", ""); - set_val_or_die("pid_file", ""); set_val_or_die("log_to_stderr", "true"); set_val_or_die("err_to_stderr", "true"); set_val_or_die("log_to_syslog", "false");src/global/global_init.cc +9
-1@@ -166,8 +166,16 @@ int global_init_prefork(CephContext *cct, int flags) if (g_code_env != CODE_ENVIRONMENT_DAEMON) return -1; const md_config_t *conf = cct->_conf; - if (!conf->daemonize) + if (!conf->daemonize) { + if (atexit(pidfile_remove_void)) { + derr << "global_init_daemonize: failed to set pidfile_remove function " + << "to run at exit." << dendl; + } + + pidfile_write(g_conf); + return -1; + } // stop log thread g_ceph_context->_log->flush();
Signature · 1 line
Loïc Dachary, Artisan Logiciel Libre