Make Daemon thread-safe

This introduces `#run_thread` method.

The `#start_working` is executed in lock context,
the same as `#stop_working`. This allows safe
initialisation of resources before `#run_thread`
consumes them.

config/gitlab.yml.example

@@ -1024,6 +1024,12 @@ 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

config/initializers/1_settings.rb

@@ -668,6 +668,7 @@ 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
 
 #
 # Testing settings

lib/gitlab/metrics/exporter/base_exporter.rb

@@ -54,8 +54,11 @@ module Gitlab
           if server
             # we close sockets if thread is not longer running
             # this happens, when the process forks
-            server.listeners.each(&:close) unless thread.alive?
-            server.shutdown
+            if thread.alive?
+              server.shutdown
+            else
+              server.listeners.each(&:close)
+            end
           end
 
           @server = nil

lib/gitlab/metrics/exporter/web_exporter.rb

@@ -7,23 +7,60 @@ module Gitlab
   module Metrics
     module Exporter
       class WebExporter < BaseExporter
+        ExporterCheck = Struct.new(:exporter) do
+          def readiness
+            Gitlab::HealthChecks::Result.new(
+              'web_exporter', exporter.running)
+          end
+        end
+
+        attr_reader :running
+
         # This exporter is always run on master process
         def initialize
           super
 
           self.additional_checks = [
+            WebExporter::ExporterCheck.new(self),
             Gitlab::HealthChecks::PumaCheck,
             Gitlab::HealthChecks::UnicornCheck
           ]
         end
 
         def settings
-          Settings.monitoring.web_exporter
+          Gitlab.config.monitoring.web_exporter
         end
 
         def log_filename
           File.join(Rails.root, 'log', 'web_exporter.log')
         end
+
+        private
+
+        def start_working
+          @running = true
+          super
+        end
+
+        def stop_working
+          @running = false
+          wait_in_blackout_period if server && thread.alive?
+          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

lib/gitlab/metrics/samplers/puma_sampler.rb

 # frozen_string_literal: true
 
-require 'puma/state_file'
-
 module Gitlab
   module Metrics
     module Samplers

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

@@ -19,15 +19,11 @@ describe Gitlab::Metrics::Exporter::BaseExporter do
         BindAddress: anything,
         Logger: anything,
         AccessLog: anything
-      ).and_wrap_original do |m, *args|
-        m.call(DoNotListen: true, Logger: args.first[:Logger])
-      end
-
-      allow_any_instance_of(::WEBrick::HTTPServer).to receive(:start)
+      ).and_call_original
 
       allow(settings).to receive(:enabled).and_return(true)
-      allow(settings).to receive(:port).and_return(8082)
-      allow(settings).to receive(:address).and_return('localhost')
+      allow(settings).to receive(:port).and_return(0)
+      allow(settings).to receive(:address).and_return('127.0.0.1')
     end
 
     after do
@@ -61,6 +57,8 @@ describe Gitlab::Metrics::Exporter::BaseExporter do
               m.call(DoNotListen: true, Logger: args.first[:Logger])
             end
 
+            allow_any_instance_of(::WEBrick::HTTPServer).to receive(:start)
+
             exporter.start.join
           end
         end
@@ -89,14 +87,14 @@ describe Gitlab::Metrics::Exporter::BaseExporter do
 
     describe 'when exporter is running' do
       before do
-        exporter.start.join
+        exporter.start
       end
 
       describe '#start' do
         it "doesn't start running server" do
           expect_any_instance_of(::WEBrick::HTTPServer).not_to receive(:start)
 
-          expect { exporter.start.join }.not_to change { exporter.thread? }
+          expect { exporter.start }.not_to change { exporter.thread? }
         end
       end
 

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

+# frozen_string_literal: true
+
+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
+      stub_config(
+        monitoring: {
+          web_exporter: {
+            enabled: true,
+            port: 0,
+            address: '127.0.0.1',
+            blackout_seconds: blackout_seconds
+          }
+        }
+      )
+
+      exporter.start
+    end
+
+    after do
+      exporter.stop
+    end
+
+    context 'when running server' do
+      it 'readiness probe returns succesful status' do
+        expect(readiness_probe.http_status).to eq(200)
+        expect(readiness_probe.json).to include(status: 'ok')
+        expect(readiness_probe.json).to include('web_exporter' => [{ 'status': 'ok' }])
+      end
+    end
+
+    context 'when blackout seconds is 10s' do
+      let(:blackout_seconds) { 10 }
+
+      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
+          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