Merge branch '300661-fix-readiness-for-puma-single-2' into 'master'

Fix /-/readiness probe for Puma Single

See merge request gitlab-org/gitlab!53708

changelogs/unreleased/300661-fix-readiness-for-puma-single-2.yml

+---
+title: Fix /-/readiness probe for Puma Single
+merge_request: 53708
+author:
+type: other

config/initializers/validate_puma.rb

@@ -4,7 +4,7 @@ def max_puma_workers
   Puma.cli_config.options[:workers].to_i
 end
 
-if Gitlab::Runtime.puma? && max_puma_workers == 0
+if Gitlab::Runtime.puma? && !Gitlab::Runtime.puma_in_clustered_mode?
   raise 'Puma is only supported in Clustered mode (workers > 0)' if Gitlab.com?
 
   warn 'WARNING: Puma is running in Single mode (workers = 0). Some features may not work. Please refer to https://gitlab.com/groups/gitlab-org/-/epics/5303 for info.'

lib/gitlab/health_checks/base_abstract_check.rb

@@ -11,6 +11,10 @@ module Gitlab
         name.sub(/_check$/, '').capitalize
       end
 
+      def available?
+        true
+      end
+
       def readiness
         raise NotImplementedError
       end

lib/gitlab/health_checks/master_check.rb

@@ -8,7 +8,16 @@ module Gitlab
       extend SimpleAbstractCheck
 
       class << self
+        extend ::Gitlab::Utils::Override
+
+        override :available?
+        def available?
+          Gitlab::Runtime.puma_in_clustered_mode?
+        end
+
         def register_master
+          return unless available?
+
           # when we fork, we pass the read pipe to child
           # child can then react on whether the other end
           # of pipe is still available
@@ -16,11 +25,15 @@ module Gitlab
         end
 
         def finish_master
+          return unless available?
+
           close_read
           close_write
         end
 
         def register_worker
+          return unless available?
+
           # fork needs to close the pipe
           close_write
         end

lib/gitlab/health_checks/probes/collection.rb

@@ -48,6 +48,7 @@ module Gitlab
 
         def probe_readiness
           checks
+            .select(&:available?)
             .flat_map(&:readiness)
             .compact
             .group_by(&:name)

lib/gitlab/metrics/exporter/web_exporter.rb

@@ -12,6 +12,10 @@ module Gitlab
             Gitlab::HealthChecks::Result.new(
               'web_exporter', exporter.running)
           end
+
+          def available?
+            true
+          end
         end
 
         attr_reader :running

lib/gitlab/runtime.rb

@@ -81,6 +81,10 @@ module Gitlab
         puma? || sidekiq? || action_cable?
       end
 
+      def puma_in_clustered_mode?
+        puma? && Puma.cli_config.options[:workers].to_i > 0
+      end
+
       def max_threads
         threads = 1 # main thread
 

spec/lib/gitlab/health_checks/master_check_spec.rb

@@ -4,47 +4,67 @@ require 'spec_helper'
 require_relative './simple_check_shared'
 
 RSpec.describe Gitlab::HealthChecks::MasterCheck do
-  let(:result_class) { Gitlab::HealthChecks::Result }
-
   before do
     stub_const('SUCCESS_CODE', 100)
     stub_const('FAILURE_CODE', 101)
-    described_class.register_master
   end
 
-  after do
-    described_class.finish_master
-  end
+  context 'when Puma runs in Clustered mode' do
+    before do
+      allow(Gitlab::Runtime).to receive(:puma_in_clustered_mode?).and_return(true)
 
-  describe '#readiness' do
-    context 'when master is running' do
-      it 'worker does return success' do
-        _, child_status = run_worker
+      described_class.register_master
+    end
 
-        expect(child_status.exitstatus).to eq(SUCCESS_CODE)
-      end
+    after do
+      described_class.finish_master
     end
 
-    context 'when master finishes early' do
-      before do
-        described_class.send(:close_write)
+    describe '.available?' do
+      specify { expect(described_class.available?).to be true }
+    end
+
+    describe '.readiness' do
+      context 'when master is running' do
+        it 'worker does return success' do
+          _, child_status = run_worker
+
+          expect(child_status.exitstatus).to eq(SUCCESS_CODE)
+        end
       end
 
-      it 'worker does return failure' do
-        _, child_status = run_worker
+      context 'when master finishes early' do
+        before do
+          described_class.send(:close_write)
+        end
 
-        expect(child_status.exitstatus).to eq(FAILURE_CODE)
+        it 'worker does return failure' do
+          _, child_status = run_worker
+
+          expect(child_status.exitstatus).to eq(FAILURE_CODE)
+        end
       end
-    end
 
-    def run_worker
-      pid = fork do
-        described_class.register_worker
+      def run_worker
+        pid = fork do
+          described_class.register_worker
 
-        exit(described_class.readiness.success ? SUCCESS_CODE : FAILURE_CODE)
+          exit(described_class.readiness.success ? SUCCESS_CODE : FAILURE_CODE)
+        end
+
+        Process.wait2(pid)
       end
+    end
+  end
+
+  # '.readiness' check is not invoked if '.available?' returns false
+  context 'when Puma runs in Single mode' do
+    before do
+      allow(Gitlab::Runtime).to receive(:puma_in_clustered_mode?).and_return(false)
+    end
 
-      Process.wait2(pid)
+    describe '.available?' do
+      specify { expect(described_class.available?).to be false }
     end
   end
 end

spec/lib/gitlab/health_checks/probes/collection_spec.rb

@@ -61,6 +61,35 @@ RSpec.describe Gitlab::HealthChecks::Probes::Collection do
           expect(subject.json[:message]).to eq('Redis::CannotConnectError : Redis down')
         end
       end
+
+      context 'when some checks are not available' do
+        before do
+          allow(Gitlab::Runtime).to receive(:puma_in_clustered_mode?).and_return(false)
+        end
+
+        let(:checks) do
+          [
+            Gitlab::HealthChecks::MasterCheck
+          ]
+        end
+
+        it 'asks for check availability' do
+          expect(Gitlab::HealthChecks::MasterCheck).to receive(:available?)
+
+          subject
+        end
+
+        it 'does not call `readiness` on checks that are not available' do
+          expect(Gitlab::HealthChecks::MasterCheck).not_to receive(:readiness)
+
+          subject
+        end
+
+        it 'does not fail collection check' do
+          expect(subject.http_status).to eq(200)
+          expect(subject.json[:status]).to eq('ok')
+        end
+      end
     end
 
     context 'without checks' do

spec/lib/gitlab/runtime_spec.rb

@@ -44,10 +44,11 @@ RSpec.describe Gitlab::Runtime do
 
   context "puma" do
     let(:puma_type) { double('::Puma') }
+    let(:max_workers) { 2 }
 
     before do
       stub_const('::Puma', puma_type)
-      allow(puma_type).to receive_message_chain(:cli_config, :options).and_return(max_threads: 2)
+      allow(puma_type).to receive_message_chain(:cli_config, :options).and_return(max_threads: 2, workers: max_workers)
       stub_env('ACTION_CABLE_IN_APP', 'false')
     end
 
@@ -70,6 +71,20 @@ RSpec.describe Gitlab::Runtime do
 
       it_behaves_like "valid runtime", :puma, 11
     end
+
+    describe ".puma_in_clustered_mode?" do
+      context 'when Puma is set up with workers > 0' do
+        let(:max_workers) { 4 }
+
+        specify { expect(described_class.puma_in_clustered_mode?).to be true }
+      end
+
+      context 'when Puma is set up with workers = 0' do
+        let(:max_workers) { 0 }
+
+        specify { expect(described_class.puma_in_clustered_mode?).to be false }
+      end
+    end
   end
 
   context "unicorn" do

spec/requests/health_controller_spec.rb

@@ -77,91 +77,129 @@ RSpec.describe HealthController do
 
     shared_context 'endpoint responding with readiness data' do
       context 'when requesting instance-checks' do
-        it 'responds with readiness checks data' do
-          expect(Gitlab::HealthChecks::MasterCheck).to receive(:check) { true }
-
-          subject
-
-          expect(json_response).to include({ 'status' => 'ok' })
-          expect(json_response['master_check']).to contain_exactly({ 'status' => 'ok' })
-        end
+        context 'when Puma runs in Clustered mode' do
+          before do
+            allow(Gitlab::Runtime).to receive(:puma_in_clustered_mode?).and_return(true)
+          end
 
-        it 'responds with readiness checks data when a failure happens' do
-          expect(Gitlab::HealthChecks::MasterCheck).to receive(:check) { false }
+          it 'responds with readiness checks data' do
+            expect(Gitlab::HealthChecks::MasterCheck).to receive(:check) { true }
 
-          subject
+            subject
 
-          expect(json_response).to include({ 'status' => 'failed' })
-          expect(json_response['master_check']).to contain_exactly(
-            { 'status' => 'failed', 'message' => 'unexpected Master check result: false' })
+            expect(json_response).to include({ 'status' => 'ok' })
+            expect(json_response['master_check']).to contain_exactly({ 'status' => 'ok' })
+          end
 
-          expect(response).to have_gitlab_http_status(:service_unavailable)
-          expect(response.headers['X-GitLab-Custom-Error']).to eq(1)
-        end
-      end
+          it 'responds with readiness checks data when a failure happens' do
+            expect(Gitlab::HealthChecks::MasterCheck).to receive(:check) { false }
 
-      context 'when requesting all checks' do
-        before do
-          params.merge!(all: true)
-        end
+            subject
 
-        it 'responds with readiness checks data' do
-          subject
+            expect(json_response).to include({ 'status' => 'failed' })
+            expect(json_response['master_check']).to contain_exactly(
+              { 'status' => 'failed', 'message' => 'unexpected Master check result: false' })
 
-          expect(json_response['db_check']).to contain_exactly({ 'status' => 'ok' })
-          expect(json_response['cache_check']).to contain_exactly({ 'status' => 'ok' })
-          expect(json_response['queues_check']).to contain_exactly({ 'status' => 'ok' })
-          expect(json_response['shared_state_check']).to contain_exactly({ 'status' => 'ok' })
-          expect(json_response['gitaly_check']).to contain_exactly(
-            { 'status' => 'ok', 'labels' => { 'shard' => 'default' } })
+            expect(response).to have_gitlab_http_status(:service_unavailable)
+            expect(response.headers['X-GitLab-Custom-Error']).to eq(1)
+          end
         end
 
-        it 'responds with readiness checks data when a failure happens' do
-          allow(Gitlab::HealthChecks::Redis::RedisCheck).to receive(:readiness).and_return(
-            Gitlab::HealthChecks::Result.new('redis_check', false, "check error"))
+        context 'when Puma runs in Single mode' do
+          before do
+            allow(Gitlab::Runtime).to receive(:puma_in_clustered_mode?).and_return(false)
+          end
 
-          subject
+          it 'does not invoke MasterCheck, succeedes' do
+            expect(Gitlab::HealthChecks::MasterCheck).not_to receive(:check) { true }
 
-          expect(json_response['cache_check']).to contain_exactly({ 'status' => 'ok' })
-          expect(json_response['redis_check']).to contain_exactly(
-            { 'status' => 'failed', 'message' => 'check error' })
+            subject
 
-          expect(response).to have_gitlab_http_status(:service_unavailable)
-          expect(response.headers['X-GitLab-Custom-Error']).to eq(1)
+            expect(json_response).to eq('status' => 'ok')
+          end
         end
+      end
 
-        context 'when DB is not accessible and connection raises an exception' do
+      context 'when requesting all checks' do
+        shared_context 'endpoint responding with readiness data for all checks' do
           before do
-            expect(Gitlab::HealthChecks::DbCheck)
-              .to receive(:readiness)
-              .and_raise(PG::ConnectionBad, 'could not connect to server')
+            params.merge!(all: true)
           end
 
-          it 'responds with 500 including the exception info' do
+          it 'responds with readiness checks data' do
             subject
 
-            expect(response).to have_gitlab_http_status(:internal_server_error)
+            expect(json_response['db_check']).to contain_exactly({ 'status' => 'ok' })
+            expect(json_response['cache_check']).to contain_exactly({ 'status' => 'ok' })
+            expect(json_response['queues_check']).to contain_exactly({ 'status' => 'ok' })
+            expect(json_response['shared_state_check']).to contain_exactly({ 'status' => 'ok' })
+            expect(json_response['gitaly_check']).to contain_exactly(
+              { 'status' => 'ok', 'labels' => { 'shard' => 'default' } })
+          end
+
+          it 'responds with readiness checks data when a failure happens' do
+            allow(Gitlab::HealthChecks::Redis::RedisCheck).to receive(:readiness).and_return(
+              Gitlab::HealthChecks::Result.new('redis_check', false, "check error"))
+
+            subject
+
+            expect(json_response['cache_check']).to contain_exactly({ 'status' => 'ok' })
+            expect(json_response['redis_check']).to contain_exactly(
+              { 'status' => 'failed', 'message' => 'check error' })
+
+            expect(response).to have_gitlab_http_status(:service_unavailable)
             expect(response.headers['X-GitLab-Custom-Error']).to eq(1)
-            expect(json_response).to eq(
-              { 'status' => 'failed', 'message' => 'PG::ConnectionBad : could not connect to server' })
+          end
+
+          context 'when DB is not accessible and connection raises an exception' do
+            before do
+              expect(Gitlab::HealthChecks::DbCheck)
+                .to receive(:readiness)
+                .and_raise(PG::ConnectionBad, 'could not connect to server')
+            end
+
+            it 'responds with 500 including the exception info' do
+              subject
+
+              expect(response).to have_gitlab_http_status(:internal_server_error)
+              expect(response.headers['X-GitLab-Custom-Error']).to eq(1)
+              expect(json_response).to eq(
+                { 'status' => 'failed', 'message' => 'PG::ConnectionBad : could not connect to server' })
+            end
+          end
+
+          context 'when any exception happens during the probing' do
+            before do
+              expect(Gitlab::HealthChecks::Redis::RedisCheck)
+                .to receive(:readiness)
+                .and_raise(::Redis::CannotConnectError, 'Redis down')
+            end
+
+            it 'responds with 500 including the exception info' do
+              subject
+
+              expect(response).to have_gitlab_http_status(:internal_server_error)
+              expect(response.headers['X-GitLab-Custom-Error']).to eq(1)
+              expect(json_response).to eq(
+                { 'status' => 'failed', 'message' => 'Redis::CannotConnectError : Redis down' })
+            end
           end
         end
 
-        context 'when any exception happens during the probing' do
+        context 'when Puma runs in Clustered mode' do
           before do
-            expect(Gitlab::HealthChecks::Redis::RedisCheck)
-              .to receive(:readiness)
-              .and_raise(::Redis::CannotConnectError, 'Redis down')
+            allow(Gitlab::Runtime).to receive(:puma_in_clustered_mode?).and_return(true)
           end
 
-          it 'responds with 500 including the exception info' do
-            subject
+          it_behaves_like 'endpoint responding with readiness data for all checks'
+        end
 
-            expect(response).to have_gitlab_http_status(:internal_server_error)
-            expect(response.headers['X-GitLab-Custom-Error']).to eq(1)
-            expect(json_response).to eq(
-              { 'status' => 'failed', 'message' => 'Redis::CannotConnectError : Redis down' })
+        context 'when Puma runs in Single mode' do
+          before do
+            allow(Gitlab::Runtime).to receive(:puma_in_clustered_mode?).and_return(false)
           end
+
+          it_behaves_like 'endpoint responding with readiness data for all checks'
         end
       end
     end