Merge branch 'improve-health-to-look-at-state-of-master' into 'master'

Improve `/-/health` to look at state of master

See merge request gitlab-org/gitlab!19441

app/controllers/health_controller.rb

@@ -5,6 +5,11 @@ class HealthController < ActionController::Base
   include RequiresWhitelistedMonitoringClient
 
   CHECKS = [
+    Gitlab::HealthChecks::MasterCheck
+  ].freeze
+
+  ALL_CHECKS = [
+    *CHECKS,
     Gitlab::HealthChecks::DbCheck,
     Gitlab::HealthChecks::Redis::RedisCheck,
     Gitlab::HealthChecks::Redis::CacheCheck,
@@ -14,8 +19,9 @@ class HealthController < ActionController::Base
   ].freeze
 
   def readiness
-    # readiness check is a collection with all above application-level checks
-    render_checks(*CHECKS)
+    # readiness check is a collection of application-level checks
+    # and optionally all service checks
+    render_checks(params[:all] ? ALL_CHECKS : CHECKS)
   end
 
   def liveness
@@ -25,7 +31,7 @@ class HealthController < ActionController::Base
 
   private
 
-  def render_checks(*checks)
+  def render_checks(checks = [])
     result = Gitlab::HealthChecks::Probes::Collection
       .new(*checks)
       .execute

config/gitlab.yml.example

@@ -1032,12 +1032,6 @@ production: &base
     #  enabled: true
     #  address: localhost
     #  port: 8083
-    #  # blackout_seconds:
-    #  #   defines an interval to block healthcheck,
-    #  #   but continue accepting application requests
-    #  #   this allows Load Balancer to notice service
-    #  #   being shutdown and not interrupt any of the clients
-    #  blackout_seconds: 10
 
   ## Prometheus settings
   # Do not modify these settings here. They should be modified in /etc/gitlab/gitlab.rb
@@ -1049,6 +1043,14 @@ production: &base
     # enable: true
     # listen_address: 'localhost:9090'
 
+  shutdown:
+    #  # blackout_seconds:
+    #  #   defines an interval to block healthcheck,
+    #  #   but continue accepting application requests
+    #  #   this allows Load Balancer to notice service
+    #  #   being shutdown and not interrupt any of the clients
+    #  blackout_seconds: 10
+
   #
   # 5. Extra customization
   # ==========================

config/initializers/1_settings.rb

@@ -676,7 +676,12 @@ Settings.monitoring['web_exporter'] ||= Settingslogic.new({})
 Settings.monitoring.web_exporter['enabled'] ||= false
 Settings.monitoring.web_exporter['address'] ||= 'localhost'
 Settings.monitoring.web_exporter['port'] ||= 8083
-Settings.monitoring.web_exporter['blackout_seconds'] ||= 10
+
+#
+# Shutdown settings
+#
+Settings['shutdown'] ||= Settingslogic.new({})
+Settings.shutdown['blackout_seconds'] ||= 10
 
 #
 # Testing settings

config/initializers/7_prometheus_metrics.rb

@@ -70,6 +70,13 @@ if defined?(::Unicorn) || defined?(::Puma)
     Gitlab::Metrics::Exporter::WebExporter.instance.start
   end
 
+  # DEPRECATED: TO BE REMOVED
+  # This is needed to implement blackout period of `web_exporter`
+  # https://gitlab.com/gitlab-org/gitlab/issues/35343#note_238479057
+  Gitlab::Cluster::LifecycleEvents.on_before_blackout_period do
+    Gitlab::Metrics::Exporter::WebExporter.instance.mark_as_not_running!
+  end
+
   Gitlab::Cluster::LifecycleEvents.on_before_graceful_shutdown do
     # We need to ensure that before we re-exec or shutdown server
     # we do stop the exporter

config/initializers/health_check.rb

@@ -8,3 +8,15 @@ HealthCheck.setup do |config|
     end
   end
 end
+
+Gitlab::Cluster::LifecycleEvents.on_before_fork do
+  Gitlab::HealthChecks::MasterCheck.register_master
+end
+
+Gitlab::Cluster::LifecycleEvents.on_before_blackout_period do
+  Gitlab::HealthChecks::MasterCheck.finish_master
+end
+
+Gitlab::Cluster::LifecycleEvents.on_worker_start do
+  Gitlab::HealthChecks::MasterCheck.register_worker
+end

config/initializers/rack_attack_new.rb

@@ -39,46 +39,63 @@ module Gitlab::Throttle
 end
 
 class Rack::Attack
+  # Order conditions by how expensive they are:
+  # 1. The most expensive is the `req.unauthenticated?` and
+  #    `req.authenticated_user_id` as it performs an expensive
+  #    DB/Redis query to validate the request
+  # 2. Slightly less expensive is the need to query DB/Redis
+  #    to unmarshal settings (`Gitlab::Throttle.settings`)
+  #
+  # We deliberately skip `/-/health|liveness|readiness`
+  # from Rack Attack as they need to always be accessible
+  # by Load Balancer and additional measure is implemented
+  # (token and whitelisting) to prevent abuse.
   throttle('throttle_unauthenticated', Gitlab::Throttle.unauthenticated_options) do |req|
+    if !req.should_be_skipped? &&
         Gitlab::Throttle.settings.throttle_unauthenticated_enabled &&
-      req.unauthenticated? &&
-      !req.should_be_skipped? &&
+        req.unauthenticated?
       req.ip
     end
+  end
 
   throttle('throttle_authenticated_api', Gitlab::Throttle.authenticated_api_options) do |req|
-    Gitlab::Throttle.settings.throttle_authenticated_api_enabled &&
-      req.api_request? &&
+    if req.api_request? &&
+        Gitlab::Throttle.settings.throttle_authenticated_api_enabled
       req.authenticated_user_id([:api])
     end
+  end
 
   throttle('throttle_authenticated_web', Gitlab::Throttle.authenticated_web_options) do |req|
-    Gitlab::Throttle.settings.throttle_authenticated_web_enabled &&
-      req.web_request? &&
+    if req.web_request? &&
+        Gitlab::Throttle.settings.throttle_authenticated_web_enabled
       req.authenticated_user_id([:api, :rss, :ics])
     end
+  end
 
   throttle('throttle_unauthenticated_protected_paths', Gitlab::Throttle.protected_paths_options) do |req|
-    Gitlab::Throttle.protected_paths_enabled? &&
-      req.unauthenticated? &&
-      !req.should_be_skipped? &&
+    if !req.should_be_skipped? &&
         req.protected_path? &&
+        Gitlab::Throttle.protected_paths_enabled? &&
+        req.unauthenticated?
       req.ip
     end
+  end
 
   throttle('throttle_authenticated_protected_paths_api', Gitlab::Throttle.protected_paths_options) do |req|
+    if req.api_request? &&
         Gitlab::Throttle.protected_paths_enabled? &&
-      req.api_request? &&
-      req.protected_path? &&
+        req.protected_path?
       req.authenticated_user_id([:api])
     end
+  end
 
   throttle('throttle_authenticated_protected_paths_web', Gitlab::Throttle.protected_paths_options) do |req|
+    if req.web_request? &&
         Gitlab::Throttle.protected_paths_enabled? &&
-      req.web_request? &&
-      req.protected_path? &&
+        req.protected_path?
       req.authenticated_user_id([:api, :rss, :ics])
     end
+  end
 
   class Request
     def unauthenticated?
@@ -97,12 +114,16 @@ class Rack::Attack
       path =~ %r{^/api/v\d+/internal/}
     end
 
+    def health_check_request?
+      path =~ %r{^/-/(health|liveness|readiness)}
+    end
+
     def should_be_skipped?
-      api_internal_request?
+      api_internal_request? || health_check_request?
     end
 
     def web_request?
-      !api_request?
+      !api_request? && !health_check_request?
     end
 
     def protected_path?

doc/user/admin_area/monitoring/health_check.md

@@ -39,7 +39,11 @@ GET http://localhost/-/liveness
 
 ## Health
 
-Checks whether the application server is running. It does not verify the database or other services are running.
+Checks whether the application server is running.
+It does not verify the database or other services
+are running. This endpoint circumvents Rails Controllers
+and is implemented as additional middleware `BasicHealthCheck`
+very early into the request processing lifecycle.
 
 ```text
 GET /-/health
@@ -59,10 +63,17 @@ GitLab OK
 
 ## Readiness
 
-The readiness probe checks whether the GitLab instance is ready to use. It checks the dependent services (Database, Redis, Gitaly etc.) and gives a status for each.
+The readiness probe checks whether the GitLab instance is ready
+to accept traffic via Rails Controllers. The check by default
+does validate only instance-checks.
+
+If the `all=1` parameter is specified, the check will also validate
+the dependent services (Database, Redis, Gitaly etc.)
+and gives a status for each.
 
 ```text
 GET /-/readiness
+GET /-/readiness?all=1
 ```
 
 Example request:
@@ -75,37 +86,30 @@ Example response:
 
 ```json
 {
-   "db_check":{
+   "master_check":[{
       "status":"failed",
-      "message": "unexpected Db check result: 0"
-   },
-   "redis_check":{
-      "status":"ok"
-   },
-   "cache_check":{
-      "status":"ok"
-   },
-   "queues_check":{
-      "status":"ok"
-   },
-   "shared_state_check":{
-      "status":"ok"
-   },
-   "gitaly_check":{
-      "status":"ok",
-      "labels":{
-         "shard":"default"
-         }
-      }
-   }
+      "message": "unexpected Master check result: false"
+   }],
+   ...
+}
 ```
 
+On failure, the endpoint will return a `503` HTTP status code.
+
+This check does hit the database and Redis if authenticated via `token`.
+
+This check is being exempt from Rack Attack.
+
 ## Liveness
 
 DANGER: **Warning:**
-In Gitlab [12.4](https://about.gitlab.com/upcoming-releases/) the response body of the Liveness check will change to match the example below.
+In Gitlab [12.4](https://about.gitlab.com/upcoming-releases/)
+the response body of the Liveness check was changed
+to match the example below.
 
-The liveness probe checks whether the application server is alive. Unlike the [`health`](#health) check, this check hits the database.
+Checks whether the application server is running.
+This probe is used to know if Rails Controllers
+are not deadlocked due to a multi-threading.
 
 ```text
 GET /-/liveness
@@ -127,7 +131,9 @@ On success, the endpoint will return a `200` HTTP status code, and a response li
 }
 ```
 
-On failure, the endpoint will return a `500` HTTP status code.
+On failure, the endpoint will return a `503` HTTP status code.
+
+This check is being exempt from Rack Attack.
 
 ## Access token (Deprecated)
 

ee/lib/ee/gitlab/rack/attack.rb

@@ -2,9 +2,10 @@
 
 module EE::Gitlab::Rack::Attack
   Rack::Attack.throttle('throttle_incident_management_notification_web', EE::Gitlab::Throttle.incident_management_options) do |req|
-    EE::Gitlab::Throttle.settings.throttle_incident_management_notification_enabled &&
-      req.web_request? &&
+    if req.web_request? &&
         req.path.include?('alerts/notify') &&
+        EE::Gitlab::Throttle.settings.throttle_incident_management_notification_enabled
       req.path
     end
+  end
 end

lib/gitlab/cluster/lifecycle_events.rb

@@ -10,38 +10,39 @@ module Gitlab
     #
     # We have the following lifecycle events.
     #
-    # - on_master_start:
+    # - on_before_fork (on master process):
     #
     #     Unicorn/Puma Cluster: This will be called exactly once,
     #       on startup, before the workers are forked. This is
     #       called in the PARENT/MASTER process.
     #
-    #     Sidekiq/Puma Single: This is called immediately.
+    #     Sidekiq/Puma Single: This is not called.
     #
-    # - on_before_fork:
+    # - on_master_start (on master process):
     #
     #     Unicorn/Puma Cluster: This will be called exactly once,
     #       on startup, before the workers are forked. This is
     #       called in the PARENT/MASTER process.
     #
-    #     Sidekiq/Puma Single: This is not called.
+    #     Sidekiq/Puma Single: This is called immediately.
     #
-    # - on_worker_start:
+    # - on_before_blackout_period (on master process):
     #
-    #     Unicorn/Puma Cluster: This is called in the worker process
-    #       exactly once before processing requests.
+    #     Unicorn/Puma Cluster: This will be called before a blackout
+    #       period when performing graceful shutdown of master.
+    #       This is called on `master` process.
     #
-    #     Sidekiq/Puma Single: This is called immediately.
+    #     Sidekiq/Puma Single: This is not called.
     #
-    # - on_before_graceful_shutdown:
+    # - on_before_graceful_shutdown (on master process):
     #
     #     Unicorn/Puma Cluster: This will be called before a graceful
-    #       shutdown of workers starts happening.
+    #       shutdown  of workers starts happening, but after blackout period.
     #       This is called on `master` process.
     #
     #     Sidekiq/Puma Single: This is not called.
     #
-    # - on_before_master_restart:
+    # - on_before_master_restart (on master process):
     #
     #     Unicorn: This will be called before a new master is spun up.
     #       This is called on forked master before `execve` to become
@@ -53,6 +54,13 @@ module Gitlab
     #
     #     Sidekiq/Puma Single: This is not called.
     #
+    # - on_worker_start (on worker process):
+    #
+    #     Unicorn/Puma Cluster: This is called in the worker process
+    #       exactly once before processing requests.
+    #
+    #     Sidekiq/Puma Single: This is called immediately.
+    #
     # Blocks will be executed in the order in which they are registered.
     #
     class LifecycleEvents
@@ -74,6 +82,12 @@ module Gitlab
           (@before_fork_hooks ||= []) << block
         end
 
+        # Read the config/initializers/cluster_events_before_phased_restart.rb
+        def on_before_blackout_period(&block)
+          # Defer block execution
+          (@master_blackout_period ||= []) << block
+        end
+
         # Read the config/initializers/cluster_events_before_phased_restart.rb
         def on_before_graceful_shutdown(&block)
           # Defer block execution
@@ -97,27 +111,24 @@ module Gitlab
         # Lifecycle integration methods (called from unicorn.rb, puma.rb, etc.)
         #
         def do_worker_start
-          @worker_start_hooks&.each do |block|
-            block.call
-          end
+          call(@worker_start_hooks)
         end
 
         def do_before_fork
-          @before_fork_hooks&.each do |block|
-            block.call
-          end
+          call(@before_fork_hooks)
         end
 
         def do_before_graceful_shutdown
-          @master_graceful_shutdown&.each do |block|
-            block.call
-          end
+          call(@master_blackout_period)
+
+          blackout_seconds = ::Settings.shutdown.blackout_seconds.to_i
+          sleep(blackout_seconds) if blackout_seconds > 0
+
+          call(@master_graceful_shutdown)
         end
 
         def do_before_master_restart
-          @master_restart_hooks&.each do |block|
-            block.call
-          end
+          call(@master_restart_hooks)
         end
 
         # DEPRECATED
@@ -132,6 +143,10 @@ module Gitlab
 
         private
 
+        def call(hooks)
+          hooks&.each(&:call)
+        end
+
         def in_clustered_environment?
           # Sidekiq doesn't fork
           return false if Sidekiq.server?

lib/gitlab/health_checks/master_check.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module HealthChecks
+    # This check is registered on master,
+    # and validated by worker
+    class MasterCheck
+      extend SimpleAbstractCheck
+
+      class << self
+        def register_master
+          # when we fork, we pass the read pipe to child
+          # child can then react on whether the other end
+          # of pipe is still available
+          @pipe_read, @pipe_write = IO.pipe
+        end
+
+        def finish_master
+          close_read
+          close_write
+        end
+
+        def register_worker
+          # fork needs to close the pipe
+          close_write
+        end
+
+        private
+
+        def close_read
+          @pipe_read&.close
+          @pipe_read = nil
+        end
+
+        def close_write
+          @pipe_write&.close
+          @pipe_write = nil
+        end
+
+        def metric_prefix
+          'master_check'
+        end
+
+        def successful?(result)
+          result
+        end
+
+        def check
+          # the lack of pipe is a legitimate failure of check
+          return false unless @pipe_read
+
+          @pipe_read.read_nonblock(1)
+
+          true
+        rescue IO::EAGAINWaitReadable
+          # if it is blocked, it means that the pipe is still open
+          # and there's no data waiting on it
+          true
+        rescue EOFError
+          # the pipe is closed
+          false
+        end
+      end
+    end
+  end
+end

lib/gitlab/metrics/exporter/web_exporter.rb

@@ -20,6 +20,10 @@ module Gitlab
         def initialize
           super
 
+          # DEPRECATED:
+          # these `readiness_checks` are deprecated
+          # as presenting no value in a way how we run
+          # application: https://gitlab.com/gitlab-org/gitlab/issues/35343
           self.readiness_checks = [
             WebExporter::ExporterCheck.new(self),
             Gitlab::HealthChecks::PumaCheck,
@@ -35,6 +39,10 @@ module Gitlab
           File.join(Rails.root, 'log', 'web_exporter.log')
         end
 
+        def mark_as_not_running!
+          @running = false
+        end
+
         private
 
         def start_working
@@ -43,24 +51,9 @@ module Gitlab
         end
 
         def stop_working
-          @running = false
-          wait_in_blackout_period if server && thread.alive?
+          mark_as_not_running!
           super
         end
-
-        def wait_in_blackout_period
-          return unless blackout_seconds > 0
-
-          @server.logger.info(
-            message: 'starting blackout...',
-            duration_s: blackout_seconds)
-
-          sleep(blackout_seconds)
-        end
-
-        def blackout_seconds
-          settings['blackout_seconds'].to_i
-        end
       end
     end
   end

spec/controllers/health_controller_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe HealthController do
-  include StubENV
-
-  let(:token) { Gitlab::CurrentSettings.health_check_access_token }
-  let(:whitelisted_ip) { '127.0.0.1' }
-  let(:not_whitelisted_ip) { '127.0.0.2' }
-
-  before do
-    allow(Settings.monitoring).to receive(:ip_whitelist).and_return([whitelisted_ip])
-    stub_storage_settings({}) # Hide the broken storage
-    stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
-  end
-
-  describe '#readiness' do
-    shared_context 'endpoint responding with readiness data' do
-      let(:request_params) { {} }
-
-      subject { get :readiness, params: request_params }
-
-      it 'responds with readiness checks data' do
-        subject
-
-        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.status).to eq(503)
-        expect(response.headers['X-GitLab-Custom-Error']).to eq(1)
-      end
-    end
-
-    context 'accessed from whitelisted ip' do
-      before do
-        allow(Gitlab::RequestContext).to receive(:client_ip).and_return(whitelisted_ip)
-      end
-
-      it_behaves_like 'endpoint responding with readiness data'
-    end
-
-    context 'accessed from not whitelisted ip' do
-      before do
-        allow(Gitlab::RequestContext).to receive(:client_ip).and_return(not_whitelisted_ip)
-      end
-
-      it 'responds with resource not found' do
-        get :readiness
-
-        expect(response.status).to eq(404)
-      end
-
-      context 'accessed with valid token' do
-        context 'token passed in request header' do
-          before do
-            request.headers['TOKEN'] = token
-          end
-
-          it_behaves_like 'endpoint responding with readiness data'
-        end
-      end
-
-      context 'token passed as URL param' do
-        it_behaves_like 'endpoint responding with readiness data' do
-          let(:request_params) { { token: token } }
-        end
-      end
-    end
-  end
-
-  describe '#liveness' do
-    shared_context 'endpoint responding with liveness data' do
-      subject { get :liveness }
-
-      it 'responds with liveness checks data' do
-        subject
-
-        expect(json_response).to eq('status' => 'ok')
-      end
-    end
-
-    context 'accessed from whitelisted ip' do
-      before do
-        allow(Gitlab::RequestContext).to receive(:client_ip).and_return(whitelisted_ip)
-      end
-
-      it_behaves_like 'endpoint responding with liveness data'
-    end
-
-    context 'accessed from not whitelisted ip' do
-      before do
-        allow(Gitlab::RequestContext).to receive(:client_ip).and_return(not_whitelisted_ip)
-      end
-
-      it 'responds with resource not found' do
-        get :liveness
-
-        expect(response.status).to eq(404)
-      end
-
-      context 'accessed with valid token' do
-        context 'token passed in request header' do
-          before do
-            request.headers['TOKEN'] = token
-          end
-
-          it_behaves_like 'endpoint responding with liveness data'
-        end
-
-        context 'token passed as URL param' do
-          it_behaves_like 'endpoint responding with liveness data' do
-            subject { get :liveness, params: { token: token } }
-          end
-        end
-      end
-    end
-  end
-end

spec/lib/gitlab/cluster/mixins/puma_cluster_spec.rb

@@ -77,7 +77,7 @@ describe Gitlab::Cluster::Mixins::PumaCluster do
 
         mutex = Mutex.new
 
-        Gitlab::Cluster::LifecycleEvents.on_before_graceful_shutdown do
+        Gitlab::Cluster::LifecycleEvents.on_before_blackout_period do
           mutex.synchronize do
             exit(140)
           end

spec/lib/gitlab/cluster/mixins/unicorn_http_server_spec.rb

@@ -75,7 +75,7 @@ describe Gitlab::Cluster::Mixins::UnicornHttpServer do
 
         mutex = Mutex.new
 
-        Gitlab::Cluster::LifecycleEvents.on_before_graceful_shutdown do
+        Gitlab::Cluster::LifecycleEvents.on_before_blackout_period do
           mutex.synchronize do
             exit(140)
           end

spec/lib/gitlab/health_checks/master_check_spec.rb

+require 'spec_helper'
+require_relative './simple_check_shared'
+
+describe Gitlab::HealthChecks::MasterCheck do
+  let(:result_class) { Gitlab::HealthChecks::Result }
+
+  SUCCESS_CODE = 100
+  FAILURE_CODE = 101
+
+  before do
+    described_class.register_master
+  end
+
+  after do
+    described_class.finish_master
+  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
+
+    context 'when master finishes early' do
+      before do
+        described_class.send(:close_write)
+      end
+
+      it 'worker does return failure' do
+        _, child_status = run_worker
+
+        expect(child_status.exitstatus).to eq(FAILURE_CODE)
+      end
+    end
+
+    def run_worker
+      pid = fork do
+        described_class.register_worker
+
+        exit(described_class.readiness.success ? SUCCESS_CODE : FAILURE_CODE)
+      end
+
+      Process.wait2(pid)
+    end
+  end
+end

spec/lib/gitlab/metrics/exporter/web_exporter_spec.rb

@@ -4,9 +4,6 @@ require 'spec_helper'
 
 describe Gitlab::Metrics::Exporter::WebExporter do
   let(:exporter) { described_class.new }
-
-  context 'when blackout seconds is used' do
-    let(:blackout_seconds) { 0 }
   let(:readiness_probe) { exporter.send(:readiness_probe).execute }
 
   before do
@@ -15,8 +12,7 @@ describe Gitlab::Metrics::Exporter::WebExporter do
         web_exporter: {
           enabled: true,
           port: 0,
-            address: '127.0.0.1',
-            blackout_seconds: blackout_seconds
+          address: '127.0.0.1'
         }
       }
     )
@@ -36,29 +32,13 @@ describe Gitlab::Metrics::Exporter::WebExporter do
     end
   end
 
-    context 'when blackout seconds is 10s' do
-      let(:blackout_seconds) { 10 }
-
+  describe '#mark_as_not_running!' do
     it 'readiness probe returns a failure status' do
-        # during sleep we check the status of readiness probe
-        expect(exporter).to receive(:sleep).with(10) do
+      exporter.mark_as_not_running!
+
       expect(readiness_probe.http_status).to eq(503)
       expect(readiness_probe.json).to include(status: 'failed')
       expect(readiness_probe.json).to include('web_exporter' => [{ 'status': 'failed' }])
     end
-
-        exporter.stop
-      end
-    end
-
-    context 'when blackout is disabled' do
-      let(:blackout_seconds) { 0 }
-
-      it 'readiness probe returns a failure status' do
-        expect(exporter).not_to receive(:sleep)
-
-        exporter.stop
-      end
-    end
   end
 end

spec/requests/health_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe HealthController do
+  include StubENV
+
+  let(:token) { Gitlab::CurrentSettings.health_check_access_token }
+  let(:whitelisted_ip) { '1.1.1.1' }
+  let(:not_whitelisted_ip) { '2.2.2.2' }
+  let(:params) { {} }
+  let(:headers) { {} }
+
+  before do
+    allow(Settings.monitoring).to receive(:ip_whitelist).and_return([whitelisted_ip])
+    stub_storage_settings({}) # Hide the broken storage
+    stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
+  end
+
+  shared_context 'endpoint querying database' do
+    it 'does query database' do
+      control_count = ActiveRecord::QueryRecorder.new { subject }.count
+
+      expect(control_count).not_to be_zero
+    end
+  end
+
+  shared_context 'endpoint not querying database' do
+    it 'does not query database' do
+      control_count = ActiveRecord::QueryRecorder.new { subject }.count
+
+      expect(control_count).to be_zero
+    end
+  end
+
+  shared_context 'endpoint not found' do
+    it 'responds with resource not found' do
+      subject
+
+      expect(response.status).to eq(404)
+    end
+  end
+
+  describe 'GET /-/health' do
+    subject { get '/-/health', params: params, headers: headers }
+
+    shared_context 'endpoint responding with health data' do
+      it 'responds with health checks data' do
+        subject
+
+        expect(response.status).to eq(200)
+        expect(response.body).to eq('GitLab OK')
+      end
+    end
+
+    context 'accessed from whitelisted ip' do
+      before do
+        stub_remote_addr(whitelisted_ip)
+      end
+
+      it_behaves_like 'endpoint responding with health data'
+      it_behaves_like 'endpoint not querying database'
+    end
+
+    context 'accessed from not whitelisted ip' do
+      before do
+        stub_remote_addr(not_whitelisted_ip)
+      end
+
+      it_behaves_like 'endpoint not querying database'
+      it_behaves_like 'endpoint not found'
+    end
+  end
+
+  describe 'GET /-/readiness' do
+    subject { get '/-/readiness', params: params, headers: headers }
+
+    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
+
+        it 'responds with readiness checks data when a failure happens' do
+          expect(Gitlab::HealthChecks::MasterCheck).to receive(:check) { false }
+
+          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(response.status).to eq(503)
+          expect(response.headers['X-GitLab-Custom-Error']).to eq(1)
+        end
+      end
+
+      context 'when requesting all checks' do
+        before do
+          params.merge!(all: true)
+        end
+
+        it 'responds with readiness checks data' do
+          subject
+
+          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.status).to eq(503)
+          expect(response.headers['X-GitLab-Custom-Error']).to eq(1)
+        end
+      end
+    end
+
+    context 'accessed from whitelisted ip' do
+      before do
+        stub_remote_addr(whitelisted_ip)
+      end
+
+      it_behaves_like 'endpoint not querying database'
+      it_behaves_like 'endpoint responding with readiness data'
+
+      context 'when requesting all checks' do
+        before do
+          params.merge!(all: true)
+        end
+
+        it_behaves_like 'endpoint querying database'
+      end
+    end
+
+    context 'accessed from not whitelisted ip' do
+      before do
+        stub_remote_addr(not_whitelisted_ip)
+      end
+
+      it_behaves_like 'endpoint not querying database'
+      it_behaves_like 'endpoint not found'
+    end
+
+    context 'accessed with valid token' do
+      context 'token passed in request header' do
+        let(:headers) { { TOKEN: token } }
+
+        it_behaves_like 'endpoint responding with readiness data'
+        it_behaves_like 'endpoint querying database'
+      end
+
+      context 'token passed as URL param' do
+        let(:params) { { token: token } }
+
+        it_behaves_like 'endpoint responding with readiness data'
+        it_behaves_like 'endpoint querying database'
+      end
+    end
+  end
+
+  describe 'GET /-/liveness' do
+    subject { get '/-/liveness', params: params, headers: headers }
+
+    shared_context 'endpoint responding with liveness data' do
+      it 'responds with liveness checks data' do
+        subject
+
+        expect(json_response).to eq('status' => 'ok')
+      end
+    end
+
+    context 'accessed from whitelisted ip' do
+      before do
+        stub_remote_addr(whitelisted_ip)
+      end
+
+      it_behaves_like 'endpoint not querying database'
+      it_behaves_like 'endpoint responding with liveness data'
+    end
+
+    context 'accessed from not whitelisted ip' do
+      before do
+        stub_remote_addr(not_whitelisted_ip)
+      end
+
+      it_behaves_like 'endpoint not querying database'
+      it_behaves_like 'endpoint not found'
+
+      context 'accessed with valid token' do
+        context 'token passed in request header' do
+          let(:headers) { { TOKEN: token } }
+
+          it_behaves_like 'endpoint responding with liveness data'
+          it_behaves_like 'endpoint querying database'
+        end
+
+        context 'token passed as URL param' do
+          let(:params) { { token: token } }
+
+          it_behaves_like 'endpoint responding with liveness data'
+          it_behaves_like 'endpoint querying database'
+        end
+      end
+    end
+  end
+
+  def stub_remote_addr(ip)
+    headers.merge!(REMOTE_ADDR: ip)
+  end
+end