Improve handling of SIGCONT after SIGTERM #4034

abetomo · 2023-01-31T14:11:41Z

Which issue(s) this PR fixes:

Ideas for #3932 fixes.

What this PR does / why we need it:

After SIGTERM, disable SIGCONT handler.
This will prevent sigdump files from being generated.

Docs Changes:

Release Note:

Signed-off-by: abetomo <[email protected]>

daipom

I am not familiar with SIGNAL but I think this fix looks nice!

It looks like this works well with ServerEngine.

daipom · 2023-02-02T06:49:35Z

lib/fluent/supervisor.rb

+      term_handler = trap :TERM do
+        # After SIGTERM, disable the sigdump file by ignoring SIGCONT.
+        trap(:CONT, nil)
+        term_handler.call


This term_handler.call() will call Server.stop() in ServerEngine.
I think this is a nice way.

https://github.com/treasure-data/serverengine/blob/v2.3.1/lib/serverengine/server.rb#L103

abetomo · 2023-02-02T07:35:19Z

Thanks for the review.
I will add any missing tests, etc.

daipom

At this point, the followings are what I have noticed.

daipom · 2023-02-02T06:52:10Z

lib/fluent/supervisor.rb

+      term_handler = trap :TERM do
+        # After SIGTERM, disable the sigdump file by ignoring SIGCONT.
+        trap(:CONT, nil)
+        term_handler.call


Is it safer to consider the possibility that term_hanler is nil?
(It seems to be usually impossible though. So I understand the idea of not needing a nil check here.)

Actually, term_handler may be a string.
I will add a check to see if it is a Proc.

I see! This can be String or Proc or nil according to the following document.

https://docs.ruby-lang.org/ja/latest/method/Signal/m/trap.html

I will add a check to see if it is a Proc.

So this looks good!

daipom · 2023-02-02T08:07:10Z

test/test_supervisor.rb

@@ -213,6 +214,52 @@ def test_main_process_signal_handlers
    $log.out.reset if $log && $log.out && $log.out.respond_to?(:reset)
  end

+  def test_cont_in_main_processsignal_handlers


Suggested change

def test_cont_in_main_processsignal_handlers

def test_cont_in_main_process_signal_handlers

daipom · 2023-02-02T08:15:19Z

test/test_supervisor.rb

+    File.delete(@sigdump_path) if File.exist?(@sigdump_path)
+  end
+
+  # TODO Sending SIGTERM terminates the test itself.


Hmm, it seems to depend on the order of the test execution.

I haven't been able to check it clearly, but it looks like the proc obj of term_handler is set to that of the previous test.
This may cause unexpected test results.

We may need some measures for this.

You are right. I was mistaken.

Signed-off-by: abetomo <[email protected]>

..._main_processsignal_handlers -> ..._main_process_signal_handlers Signed-off-by: abetomo <[email protected]>

kou · 2023-02-03T09:14:20Z

How about just disabling dumping after stop instead of changing signal handlers?

diff --git a/lib/fluent/supervisor.rb b/lib/fluent/supervisor.rb
index b372f922..7dd5fc80 100644
--- a/lib/fluent/supervisor.rb
+++ b/lib/fluent/supervisor.rb
@@ -355,6 +355,10 @@ module Fluent
       { conf: @fluentd_conf }
     end
 
+    def dump
+      super unless @stop
+    end
+
     private
 
     def reopen_log
@@ -413,6 +417,10 @@ module Fluent
     def after_start
       (config[:worker_pid] ||= {})[@worker_id] = @pm.pid
     end
+
+    def dump
+      super unless @stop
+    end
   end
 
   class Supervisor
@@ -754,12 +762,6 @@ module Fluent
     end
 
     def run_worker
-      begin
-        require 'sigdump/setup'
-      rescue Exception
-        # ignore LoadError and others (related with signals): it may raise these errors in Windows
-      end
-
       Process.setproctitle("worker:#{@system_config.process_name}") if @process_name
 
       if @standalone_worker && @system_config.workers != 1
@@ -940,6 +942,10 @@ module Fluent
         trap :USR2 do
           reload_config
         end
+
+        trap :CONT do
+          dump_non_windows
+        end
       end
     end
 
@@ -969,7 +975,7 @@ module Fluent
             reload_config
           when "DUMP"
             $log.debug "fluentd main process get #{cmd} command"
-            dump
+            dump_windows
           else
             $log.warn "fluentd main process get unknown command [#{cmd}]"
           end
@@ -1020,7 +1026,15 @@ module Fluent
       end
     end
 
-    def dump
+    def dump_non_windows
+      begin
+        Sigdump.dump unless @finished
+      rescue => e
+        $log.error("failed to dump: #{e}")
+      end
+    end
+
+    def dump_windows
       Thread.new do
         begin
           FluentSigdump.dump_windows

Signed-off-by: abetomo <[email protected]>

abetomo · 2023-02-03T14:43:44Z

@kou Thanks. I will try it.

abetomo · 2023-02-05T13:00:47Z

@daipom What do you think about #4034 (comment)?
I like this idea.
After stopping, it is easy to understand that sigdump is disabled.

ashie · 2023-02-06T00:04:29Z

I prefer @kou's idea, too.

daipom · 2023-02-06T04:14:07Z

@daipom What do you think about #4034 (comment)? I like this idea. After stopping, it is easy to understand that sigdump is disabled.

I think so too!

kou · 2023-02-06T04:34:49Z

test/test_supervisor.rb

+    sv.send(:install_main_process_signal_handlers)
+
+    begin
+      Process.kill :CONT, $$


Process.pid is preferred than $$ for readability.

Suggested change

Process.kill :CONT, $$

Process.kill :CONT, Process.pid

kou · 2023-02-06T04:35:12Z

test/test_supervisor.rb

+    begin
+      Process.kill :CONT, $$
+    rescue
+    end


Can we remove this begin ... rescue ... end?

kou · 2023-02-06T04:38:14Z

test/test_supervisor.rb

+
+    sleep 1
+
+    assert_true File.exist?(@sigdump_path)


Power assert style is preferred for better failure message:

Suggested change

assert_true File.exist?(@sigdump_path)

assert do

File.exist?(@sigdump_path)

end

assert_true ... shows just File.exist?(@sigdump_path) is not true on failure.
But assert {...} shows not only File.exist?(@sigdump_path) value but also @sigdump_path and so on.

kou · 2023-02-06T04:39:00Z

test/test_supervisor.rb

+
+    sleep 1
+
+    debug_msg = '[debug]: fluentd main process get SIGTERM' + "\n"


Suggested change

debug_msg = '[debug]: fluentd main process get SIGTERM' + "\n"

debug_msg = "[debug]: fluentd main process get SIGTERM\n"

kou · 2023-02-06T04:39:31Z

test/test_supervisor.rb

+      Process.kill :TERM, $$
+      Process.kill :CONT, $$
+    rescue
+    end


Can we remove this begin ... rescue ... end?

kou · 2023-02-06T04:40:45Z

test/test_supervisor.rb

+
+    debug_msg = '[debug]: fluentd main process get SIGTERM' + "\n"
+    logs = $log.out.logs
+    assert{ logs.any?{|log| log.include?(debug_msg) } }


Suggested change

assert{ logs.any?{|log| log.include?(debug_msg) } }

assert{ logs.any?{|log| log.include?(debug_msg) } }

It seems to be the same code.
I would like to know if there is a better way.

Ah, sorry. This is garbage.

kou · 2023-02-06T04:41:02Z

test/test_supervisor.rb

+    logs = $log.out.logs
+    assert{ logs.any?{|log| log.include?(debug_msg) } }
+
+    assert_false File.exist?(@sigdump_path)


Suggested change

assert_false File.exist?(@sigdump_path)

assert do

not File.exist?(@sigdump_path)

end

kou · 2023-02-06T04:41:17Z

test/test_supervisor.rb

+
+    assert_false File.exist?(@sigdump_path)
+  ensure
+    $log.out.reset if $log && $log.out && $log.out.respond_to?(:reset)


Suggested change

$log.out.reset if $log && $log.out && $log.out.respond_to?(:reset)

$log.out.reset if $log&.out&.respond_to?(:reset)

abetomo · 2023-02-07T13:14:45Z

Thank you for your comments and review.

I'll fix it with @kou's idea.
Close this PR and open another PR with a suitable title.

#4043

Improve handling of SIGCONT after SIGTERM

e1c39c8

Signed-off-by: abetomo <[email protected]>

abetomo mentioned this pull request Jan 31, 2023

Provide option to ignore SIGCONT #3932

Closed

daipom self-requested a review February 1, 2023 06:42

daipom reviewed Feb 2, 2023

View reviewed changes

abetomo added 2 commits February 3, 2023 08:44

Check that term_handler is a Proc

9f7fa18

Signed-off-by: abetomo <[email protected]>

Rename test methods (fix typo)

01eba25

..._main_processsignal_handlers -> ..._main_process_signal_handlers Signed-off-by: abetomo <[email protected]>

test: add test_term_cont_in_supervisor_signal_handler

88827b1

Signed-off-by: abetomo <[email protected]>

kou reviewed Feb 6, 2023

View reviewed changes

This was referenced Feb 6, 2023

Replace $$ with Process.pid #4040

Merged

Disable sigdump after stop #4043

Merged

abetomo closed this Feb 7, 2023

abetomo mentioned this pull request Feb 7, 2023

refactor: test/test_supervisor.rb #4044

Merged

ashie added this to the v1.16.0 milestone Feb 9, 2023

ashie mentioned this pull request Feb 13, 2023

buffer: backup broken file chunk #4025

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of SIGCONT after SIGTERM #4034

Improve handling of SIGCONT after SIGTERM #4034

abetomo commented Jan 31, 2023

daipom left a comment

daipom Feb 2, 2023

abetomo commented Feb 2, 2023

daipom left a comment

daipom Feb 2, 2023 •

edited

Loading

abetomo Feb 2, 2023

daipom Feb 2, 2023 •

edited

Loading

daipom Feb 2, 2023

daipom Feb 2, 2023

abetomo Feb 3, 2023

kou commented Feb 3, 2023

abetomo commented Feb 3, 2023

abetomo commented Feb 5, 2023

ashie commented Feb 6, 2023

daipom commented Feb 6, 2023

kou Feb 6, 2023

kou Feb 6, 2023

kou Feb 6, 2023

kou Feb 6, 2023

kou Feb 6, 2023

kou Feb 6, 2023

abetomo Feb 6, 2023

kou Feb 7, 2023

kou Feb 6, 2023

kou Feb 6, 2023

abetomo commented Feb 7, 2023

	def test_cont_in_main_processsignal_handlers
	def test_cont_in_main_process_signal_handlers


		sleep 1

		debug_msg = '[debug]: fluentd main process get SIGTERM' + "\n"

	debug_msg = '[debug]: fluentd main process get SIGTERM' + "\n"
	debug_msg = "[debug]: fluentd main process get SIGTERM\n"

	assert{ logs.any?{\|log\| log.include?(debug_msg) } }
	assert{ logs.any?{\|log\| log.include?(debug_msg) } }

-    assert_false File.exist?(@sigdump_path)
+    assert do
+      not File.exist?(@sigdump_path)
+    end

	$log.out.reset if $log && $log.out && $log.out.respond_to?(:reset)
	$log.out.reset if $log&.out&.respond_to?(:reset)

Improve handling of SIGCONT after SIGTERM #4034

Improve handling of SIGCONT after SIGTERM #4034

Conversation

abetomo commented Jan 31, 2023

daipom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abetomo commented Feb 2, 2023

daipom left a comment

Choose a reason for hiding this comment

daipom Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daipom Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kou commented Feb 3, 2023

abetomo commented Feb 3, 2023

abetomo commented Feb 5, 2023

ashie commented Feb 6, 2023

daipom commented Feb 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abetomo commented Feb 7, 2023

daipom Feb 2, 2023 •

edited

Loading

daipom Feb 2, 2023 •

edited

Loading