IMPALA-4737: Prevent SIGUSR1 from killing daemons when minidumps are disabled
If a user disabled minidumps before this change, we did not register the signal handler for SIGUSR1 at all. Sending SIGUSR1 to a daemon would subsequently kill it. This change registers the SIG_IGN handler to ignore the signal if minidumps are disabled. Change-Id: I13d866a2eec832500131954a7f693c33585ea51e Reviewed-on: http://gerrit.cloudera.org:8080/7631 Reviewed-by: Lars Volker <[email protected]> Tested-by: Impala Public Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/b8295705 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/b8295705 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/b8295705 Branch: refs/heads/master Commit: b82957055cfa56ef22bce36122928d6d26eb3558 Parents: f8c36b9 Author: Lars Volker <[email protected]> Authored: Wed Aug 9 17:54:24 2017 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Wed Aug 16 01:18:22 2017 +0000 ---------------------------------------------------------------------- be/src/util/minidump.cc | 21 ++++++++++++++------- tests/custom_cluster/test_breakpad.py | 21 +++++++++++++++++++-- 2 files changed, 33 insertions(+), 9 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/b8295705/be/src/util/minidump.cc ---------------------------------------------------------------------- diff --git a/be/src/util/minidump.cc b/be/src/util/minidump.cc index 78c48df..f722b20 100644 --- a/be/src/util/minidump.cc +++ b/be/src/util/minidump.cc @@ -101,13 +101,18 @@ static void HandleSignal(int signal) { minidump_exception_handler->WriteMinidump(FLAGS_minidump_path, DumpCallback, NULL); } -/// Register our signal handler to write minidumps on SIGUSR1. -static void SetupSignalHandler() { - DCHECK(minidump_exception_handler != NULL); +/// Register our signal handler to write minidumps on SIGUSR1. Will make us ignore the +/// signal if 'minidumps_enabled' is false. +static void SetupSigUSR1Handler(bool minidumps_enabled) { struct sigaction sig_action; memset(&sig_action, 0, sizeof(sig_action)); sigemptyset(&sig_action.sa_mask); - sig_action.sa_handler = &HandleSignal; + if (minidumps_enabled) { + DCHECK(minidump_exception_handler != NULL); + sig_action.sa_handler = &HandleSignal; + } else { + sig_action.sa_handler = SIG_IGN; + } sigaction(SIGUSR1, &sig_action, NULL); } @@ -194,8 +199,10 @@ Status RegisterMinidump(const char* cmd_line_path) { DCHECK(!registered); registered = true; - if (!FLAGS_enable_minidumps) return Status::OK(); - if (FLAGS_minidump_path.empty()) return Status::OK(); + if (!FLAGS_enable_minidumps || FLAGS_minidump_path.empty()) { + SetupSigUSR1Handler(false); + return Status::OK(); + } if (path(FLAGS_minidump_path).is_relative()) { path log_dir(FLAGS_log_dir); @@ -238,7 +245,7 @@ Status RegisterMinidump(const char* cmd_line_path) { desc, FilterCallback, DumpCallback, NULL, true, -1); // Setup signal handler for SIGUSR1. - SetupSignalHandler(); + SetupSigUSR1Handler(true); return Status::OK(); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/b8295705/tests/custom_cluster/test_breakpad.py ---------------------------------------------------------------------- diff --git a/tests/custom_cluster/test_breakpad.py b/tests/custom_cluster/test_breakpad.py index 8300634..79ba70c 100644 --- a/tests/custom_cluster/test_breakpad.py +++ b/tests/custom_cluster/test_breakpad.py @@ -155,7 +155,7 @@ class TestBreakpad(CustomClusterTestSuite): @pytest.mark.execute_serially def test_minidump_creation(self): - """Check that when a daemon crashes it writes a minidump file.""" + """Check that when a daemon crashes, it writes a minidump file.""" assert self.count_all_minidumps() == 0 self.start_cluster() assert self.count_all_minidumps() == 0 @@ -165,7 +165,7 @@ class TestBreakpad(CustomClusterTestSuite): @pytest.mark.execute_serially def test_sigusr1_writes_minidump(self): - """Check that when a daemon receives SIGUSR1 it writes a minidump file.""" + """Check that when a daemon receives SIGUSR1, it writes a minidump file.""" assert self.count_all_minidumps() == 0 self.start_cluster() assert self.count_all_minidumps() == 0 @@ -183,6 +183,23 @@ class TestBreakpad(CustomClusterTestSuite): self.assert_num_minidumps_for_all_daemons(cluster_size) @pytest.mark.execute_serially + def test_sigusr1_doesnt_kill(self): + """Check that when minidumps are disabled and a daemon receives SIGUSR1, it does not + die. + """ + assert self.count_all_minidumps() == 0 + self.start_cluster_with_args(enable_minidumps=False) + cluster_size = self.get_num_processes('impalad') + self.kill_cluster(SIGUSR1) + # Check that no minidumps have been written. + self.assert_num_logfile_entries(0) + assert self.count_all_minidumps() == 0 + # Check that all daemons are still alive. + assert self.get_num_processes('impalad') == cluster_size + assert self.get_num_processes('catalogd') == 1 + assert self.get_num_processes('statestored') == 1 + + @pytest.mark.execute_serially def test_minidump_relative_path(self): """Check that setting 'minidump_path' to a relative value results in minidump files written to 'log_dir'.
