Skip to content

Fix Sidekiq process group killing

Nick Thomas requested to merge (removed):fix-sidekiq-process-group-killing into master

What does this MR do?

While killing gdk locally, I spotted this error:

12:30:53 system                  | sending SIGTERM to all processes
12:30:53 rails-background-jobs.1 | no implicit conversion of String into Integer
12:30:53 rails-background-jobs.1 | /home/lupine/dev/gitlab.com/gitlab-org/gdk-ce/gitlab/lib/gitlab/sidekiq_signals.rb:38:in `kill'
12:30:53 rails-background-jobs.1 | /home/lupine/dev/gitlab.com/gitlab-org/gdk-ce/gitlab/lib/gitlab/sidekiq_signals.rb:38:in `blindly_signal_pgroup!'
12:30:53 rails-background-jobs.1 | /home/lupine/dev/gitlab.com/gitlab-org/gdk-ce/gitlab/lib/gitlab/sidekiq_signals.rb:25:in `block (2 levels) in install!'
12:30:53 rails-background-jobs.1 | /home/lupine/.gem/ruby/2.5.3/gems/sidekiq-5.2.5/lib/sidekiq/cli.rb:176:in `handle_signal'

This sucks a lot, and means that https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25669 introduced a regression.

The documentation isn't explicit that an Integer must be used, and I do recall it working in the past. Oh well.

Passing 0 is documented as equivalent to passing "-#{Process.getpgrp}".to_i, but slightly neater, so I went that way: http://ruby-doc.org/core-2.5.3/Process.html#method-c-kill

If pid is zero signal is sent to all processes whose group ID is equal to the group ID of the process

The manpage for the kill syscall is clearer:

If pid equals 0, then sig is sent to every process in the process group of the calling process.

This is the only place we need to change this.

What are the relevant issue numbers?

Does this MR meet the acceptance criteria?

Merge request reports