Merge remote-tracking branch 'origin/master' into 11-9-stable

11 files changed, 159 insertions, 7 deletions

diff --git a/app/assets/javascripts/vue_shared/components/ci_icon.vue b/app/assets/javascripts/vue_shared/components/ci_icon.vue
index 2f498c4fa2a..e6f0a1c69cd 100644
--- a/app/assets/javascripts/vue_shared/components/ci_icon.vue
+++ b/app/assets/javascripts/vue_shared/components/ci_icon.vue
@@ -21,6 +21,7 @@ import Icon from '../../vue_shared/components/icon.vue';
  * - Jobs table
  * - Jobs show view header
  * - Jobs show view sidebar
+ * - Linked pipelines
  */
 const validSizes = [8, 12, 16, 18, 24, 32, 48, 72];
 
diff --git a/changelogs/unreleased/40396-sidekiq-in-process-group.yml b/changelogs/unreleased/40396-sidekiq-in-process-group.yml
new file mode 100644
index 00000000000..e41557e20d0
--- /dev/null
+++ b/changelogs/unreleased/40396-sidekiq-in-process-group.yml
@@ -0,0 +1,5 @@
+---
+title: 'sidekiq: terminate child processes at shutdown'
+merge_request: 25669
+author:
+type: added
diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb
index 4a8d72e37a5..2e4aa9c1053 100644
--- a/config/initializers/sidekiq.rb
+++ b/config/initializers/sidekiq.rb
@@ -77,6 +77,9 @@ Sidekiq.configure_server do |config|
   # Avoid autoload issue such as 'Mail::Parsers::AddressStruct'
   # https://github.com/mikel/mail/issues/912#issuecomment-214850355
   Mail.eager_autoload!
+
+  # Ensure the whole process group is terminated if possible
+  Gitlab::SidekiqSignals.install!(Sidekiq::CLI::SIGNAL_HANDLERS)
 end
 
 Sidekiq.configure_client do |config|
diff --git a/doc/administration/operations/sidekiq_memory_killer.md b/doc/administration/operations/sidekiq_memory_killer.md
index cbffd883774..8eac42f2fe2 100644
--- a/doc/administration/operations/sidekiq_memory_killer.md
+++ b/doc/administration/operations/sidekiq_memory_killer.md
@@ -17,6 +17,11 @@ With the default settings, the MemoryKiller will cause a Sidekiq restart no
 more often than once every 15 minutes, with the restart causing about one
 minute of delay for incoming background jobs.
 
+Some background jobs rely on long-running external processes. To ensure these
+are cleanly terminated when Sidekiq is restarted, each Sidekiq process should be
+run as a process group leader (e.g., using `chpst -P`). If using Omnibus or the
+`bin/background_jobs` script with `runit` installed, this is handled for you.
+
 ## Configuring the MemoryKiller
 
 The MemoryKiller is controlled using environment variables.
diff --git a/lib/gitlab/sidekiq_middleware/memory_killer.rb b/lib/gitlab/sidekiq_middleware/memory_killer.rb
index 47333d257eb..ed2c7ee9a2d 100644
--- a/lib/gitlab/sidekiq_middleware/memory_killer.rb
+++ b/lib/gitlab/sidekiq_middleware/memory_killer.rb
@@ -36,11 +36,13 @@ module Gitlab
           # Wait `SHUTDOWN_WAIT` to give already fetched jobs time to finish.
           # Then, tell Sidekiq to gracefully shut down by giving jobs a few more
           # moments to finish, killing and requeuing them if they didn't, and
-          # then terminating itself.
+          # then terminating itself. Sidekiq will replicate the TERM to all its
+          # children if it can.
           wait_and_signal(SHUTDOWN_WAIT, 'SIGTERM', 'gracefully shut down')
 
           # Wait for Sidekiq to shutdown gracefully, and kill it if it didn't.
-          wait_and_signal(Sidekiq.options[:timeout] + 2, 'SIGKILL', 'die')
+          # Kill the whole pgroup, so we can be sure no children are left behind
+          wait_and_signal_pgroup(Sidekiq.options[:timeout] + 2, 'SIGKILL', 'die')
         end
       end
 
@@ -53,6 +55,17 @@ module Gitlab
         output.to_i
       end
 
+      # If this sidekiq process is pgroup leader, signal to the whole pgroup
+      def wait_and_signal_pgroup(time, signal, explanation)
+        return wait_and_signal(time, signal, explanation) unless Process.getpgrp == pid
+
+        Sidekiq.logger.warn "waiting #{time} seconds before sending Sidekiq worker PGRP-#{pid} #{signal} (#{explanation})"
+        sleep(time)
+
+        Sidekiq.logger.warn "sending Sidekiq worker PGRP-#{pid} #{signal} (#{explanation})"
+        Process.kill(signal, "-#{pid}")
+      end
+
       def wait_and_signal(time, signal, explanation)
         Sidekiq.logger.warn "waiting #{time} seconds before sending Sidekiq worker PID-#{pid} #{signal} (#{explanation})"
         sleep(time)
diff --git a/lib/gitlab/sidekiq_signals.rb b/lib/gitlab/sidekiq_signals.rb
new file mode 100644
index 00000000000..b704ee9a0a9
--- /dev/null
+++ b/lib/gitlab/sidekiq_signals.rb
@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+module Gitlab
+  # As a process group leader, we can ensure that children of sidekiq are killed
+  # at the same time as sidekiq itself, to stop long-lived children from being
+  # reparented to init and "escaping". To do this, we override the default
+  # handlers used by sidekiq for INT and TERM signals
+  module SidekiqSignals
+    REPLACE_SIGNALS = %w[INT TERM].freeze
+
+    SIDEKIQ_CHANGED_MESSAGE =
+      "Intercepting signal handlers: #{REPLACE_SIGNALS.join(", ")} failed. " \
+      "Sidekiq should have registered them, but appears not to have done so."
+
+    def self.install!(sidekiq_handlers)
+      # This only works if we're process group leader
+      return unless Process.getpgrp == Process.pid
+
+      raise SIDEKIQ_CHANGED_MESSAGE unless
+        REPLACE_SIGNALS == sidekiq_handlers.keys & REPLACE_SIGNALS
+
+      REPLACE_SIGNALS.each do |signal|
+        old_handler = sidekiq_handlers[signal]
+        sidekiq_handlers[signal] = ->(cli) do
+          blindly_signal_pgroup!(signal)
+          old_handler.call(cli)
+        end
+      end
+    end
+
+    # The process group leader can forward INT and TERM signals to the whole
+    # group. However, the forwarded signal is *also* received by the leader,
+    # which could lead to an infinite loop. We can avoid this by temporarily
+    # ignoring the forwarded signal. This may cause us to miss some repeated
+    # signals from outside the process group, but that isn't fatal.
+    def self.blindly_signal_pgroup!(signal)
+      old_trap = trap(signal, 'IGNORE')
+      Process.kill(signal, "-#{Process.getpgrp}")
+      trap(signal, old_trap)
+    end
+  end
+end
diff --git a/qa/qa/specs/features/browser_ui/1_manage/project/view_project_activity_spec.rb b/qa/qa/specs/features/browser_ui/1_manage/project/view_project_activity_spec.rb
index e172206eb88..d4cedc9362d 100644
--- a/qa/qa/specs/features/browser_ui/1_manage/project/view_project_activity_spec.rb
+++ b/qa/qa/specs/features/browser_ui/1_manage/project/view_project_activity_spec.rb
@@ -1,8 +1,7 @@
 # frozen_string_literal: true
 
 module QA
-  # Failure issue: https://gitlab.com/gitlab-org/quality/staging/issues/21
-  context 'Manage', :quarantine do
+  context 'Manage' do
     describe 'Project activity' do
       it 'user creates an event in the activity page upon Git push' do
         Runtime::Browser.visit(:gitlab, Page::Main::Login)
diff --git a/qa/qa/specs/features/browser_ui/3_create/repository/protocol_v2_push_http_spec.rb b/qa/qa/specs/features/browser_ui/3_create/repository/protocol_v2_push_http_spec.rb
index d1535d6519d..01a367fceac 100644
--- a/qa/qa/specs/features/browser_ui/3_create/repository/protocol_v2_push_http_spec.rb
+++ b/qa/qa/specs/features/browser_ui/3_create/repository/protocol_v2_push_http_spec.rb
@@ -1,7 +1,9 @@
 # frozen_string_literal: true
 
 module QA
-  context 'Create' do
+  # Git protocol v2 is temporarily disabled
+  # https://gitlab.com/gitlab-org/gitlab-ce/issues/55769 (confidential)
+  context 'Create', :quarantine do
     describe 'Push over HTTP using Git protocol version 2', :requires_git_protocol_v2 do
       it 'user pushes to the repository' do
         Runtime::Browser.visit(:gitlab, Page::Main::Login)
diff --git a/qa/qa/specs/features/browser_ui/3_create/repository/protocol_v2_push_ssh_spec.rb b/qa/qa/specs/features/browser_ui/3_create/repository/protocol_v2_push_ssh_spec.rb
index 48800cc81e5..eb59b54e9ab 100644
--- a/qa/qa/specs/features/browser_ui/3_create/repository/protocol_v2_push_ssh_spec.rb
+++ b/qa/qa/specs/features/browser_ui/3_create/repository/protocol_v2_push_ssh_spec.rb
@@ -1,7 +1,9 @@
 # frozen_string_literal: true
 
 module QA
-  context 'Create' do
+  # Git protocol v2 is temporarily disabled
+  # https://gitlab.com/gitlab-org/gitlab-ce/issues/55769 (confidential)
+  context 'Create', :quarantine do
     describe 'Push over SSH using Git protocol version 2', :requires_git_protocol_v2 do
       # Note: If you run this test against GDK make sure you've enabled sshd and
       # enabled setting the Git protocol by adding `AcceptEnv GIT_PROTOCOL` to
diff --git a/spec/lib/gitlab/sidekiq_middleware/memory_killer_spec.rb b/spec/lib/gitlab/sidekiq_middleware/memory_killer_spec.rb
index 5df56178df2..ff8c0825ee4 100644
--- a/spec/lib/gitlab/sidekiq_middleware/memory_killer_spec.rb
+++ b/spec/lib/gitlab/sidekiq_middleware/memory_killer_spec.rb
@@ -35,7 +35,7 @@ describe Gitlab::SidekiqMiddleware::MemoryKiller do
       stub_const("#{described_class}::MAX_RSS", 5.kilobytes)
     end
 
-    it 'sends the STP, TERM and KILL signals at expected times' do
+    it 'sends the TSTP, TERM and KILL signals at expected times' do
       expect(subject).to receive(:sleep).with(15 * 60).ordered
       expect(Process).to receive(:kill).with('SIGTSTP', pid).ordered
 
@@ -47,6 +47,17 @@ describe Gitlab::SidekiqMiddleware::MemoryKiller do
 
       run
     end
+
+    it 'sends TSTP and TERM to the pid, but KILL to the pgroup, when running as process leader' do
+      allow(Process).to receive(:getpgrp) { pid }
+      allow(subject).to receive(:sleep)
+
+      expect(Process).to receive(:kill).with('SIGTSTP', pid).ordered
+      expect(Process).to receive(:kill).with('SIGTERM', pid).ordered
+      expect(Process).to receive(:kill).with('SIGKILL', "-#{pid}").ordered
+
+      run
+    end
   end
 
   context 'when MAX_RSS is not exceeded' do
diff --git a/spec/lib/gitlab/sidekiq_signals_spec.rb b/spec/lib/gitlab/sidekiq_signals_spec.rb
new file mode 100644
index 00000000000..4483224f49d
--- /dev/null
+++ b/spec/lib/gitlab/sidekiq_signals_spec.rb
@@ -0,0 +1,69 @@
+require 'spec_helper'
+
+describe Gitlab::SidekiqSignals do
+  describe '.install' do
+    let(:result) { Hash.new { |h, k| h[k] = 0 } }
+    let(:int_handler) { -> (_) { result['INT'] += 1 } }
+    let(:term_handler) { -> (_) { result['TERM'] += 1 } }
+    let(:other_handler) { -> (_) { result['OTHER'] += 1 } }
+    let(:signals) { { 'INT' => int_handler, 'TERM' => term_handler, 'OTHER' => other_handler } }
+
+    context 'not a process group leader' do
+      before do
+        allow(Process).to receive(:getpgrp) { Process.pid * 2 }
+      end
+
+      it 'does nothing' do
+        expect { described_class.install!(signals) }
+          .not_to change { signals }
+      end
+    end
+
+    context 'as a process group leader' do
+      before do
+        allow(Process).to receive(:getpgrp) { Process.pid }
+      end
+
+      it 'installs its own signal handlers for TERM and INT only' do
+        described_class.install!(signals)
+
+        expect(signals['INT']).not_to eq(int_handler)
+        expect(signals['TERM']).not_to eq(term_handler)
+        expect(signals['OTHER']).to eq(other_handler)
+      end
+
+      %w[INT TERM].each do |signal|
+        it "installs a forwarding signal handler for #{signal}" do
+          described_class.install!(signals)
+
+          expect(described_class)
+            .to receive(:trap)
+            .with(signal, 'IGNORE')
+            .and_return(:original_trap)
+            .ordered
+
+          expect(Process)
+            .to receive(:kill)
+            .with(signal, "-#{Process.pid}")
+            .ordered
+
+          expect(described_class)
+            .to receive(:trap)
+            .with(signal, :original_trap)
+            .ordered
+
+          signals[signal].call(:cli)
+
+          expect(result[signal]).to eq(1)
+        end
+
+        it "raises if sidekiq no longer traps SIG#{signal}" do
+          signals.delete(signal)
+
+          expect { described_class.install!(signals) }
+            .to raise_error(/Sidekiq should have registered/)
+        end
+      end
+    end
+  end
+end