Refactor runner and move seperate webserver into own class

Author: svkrieger

lib/cloud_controller/metrics_webserver.rb

@@ -0,0 +1,98 @@
+require 'rack'
+require 'prometheus/middleware/exporter'
+
+module VCAP
+  module CloudController
+    class MetricsWebserver
+      attr_reader :app
+
+      def initialize
+        @app = build_app
+      end
+
+      def start(config)
+        server = Puma::Server.new(@app)
+
+        if config.get(:nginx, :metrics_socket).nil? || config.get(:nginx, :metrics_socket).empty?
+          server.add_tcp_listener('127.0.0.1', 9395)
+        else
+          server.add_unix_listener(config.get(:nginx, :metrics_socket))
+        end
+
+        server.run
+      end
+
+      private
+
+      def build_app
+        status_proc = method(:status)
+        Rack::Builder.new do
+          use Prometheus::Middleware::Exporter, path: '/internal/v4/metrics'
+
+          map '/internal/v4/status' do
+            run ->(_env) { status_proc.call }
+          end
+
+          map '/' do
+            run lambda { |_env|
+              # Return 404 for any other request
+              ['404', { 'Content-Type' => 'text/plain' }, ['Not Found']]
+            }
+          end
+        end
+      end
+
+      def status
+        stats = Puma.stats_hash
+        worker_statuses = stats[:worker_status]
+
+        all_busy = all_workers_busy?(worker_statuses)
+        current_requests_count_sum = worker_requests_count_sum(worker_statuses)
+
+        track_request_count_increase(current_requests_count_sum)
+
+        unhealthy = determine_unhealthy_state(all_busy)
+
+        build_status_response(all_busy, unhealthy)
+      rescue StandardError => e
+        [500, { 'Content-Type' => 'text/plain' }, ["Readiness check error: #{e}"]]
+      end
+
+      def track_request_count_increase(current_requests_count_sum)
+        now = Time.now
+        prev = @previous_requests_count_sum
+
+        @last_requests_count_increase_time = now if prev.nil? || current_requests_count_sum > prev
+        @previous_requests_count_sum = current_requests_count_sum
+      end
+
+      def determine_unhealthy_state(all_busy)
+        return false unless all_busy && @last_requests_count_increase_time
+
+        (Time.now - @last_requests_count_increase_time) > 60
+      end
+
+      def build_status_response(all_busy, unhealthy)
+        if all_busy && unhealthy
+          [503, { 'Content-Type' => 'text/plain' }, ['UNHEALTHY']]
+        elsif all_busy
+          [429, { 'Content-Type' => 'text/plain' }, ['BUSY']]
+        else
+          [200, { 'Content-Type' => 'text/plain' }, ['OK']]
+        end
+      end
+
+      def all_workers_busy?(worker_statuses)
+        worker_statuses.all? do |worker|
+          worker[:last_status][:busy_threads] == worker[:last_status][:running]
+        end
+      end
+
+      def worker_requests_count_sum(worker_statuses)
+        worker_statuses.sum do |worker|
+          worker[:last_status][:requests_count] || 0
+        end
+      end
+    end
+  end
+end

lib/cloud_controller/runner.rb

@@ -13,6 +13,7 @@
 require 'cloud_controller/secrets_fetcher'
 require 'cloud_controller/runners/thin_runner'
 require 'cloud_controller/runners/puma_runner'
+require 'cloud_controller/metrics_webserver'
 require 'prometheus/client/data_stores/direct_file_store'
 require 'prometheus/middleware/exporter'
 
@@ -133,84 +134,8 @@ def setup_metrics
 
       Prometheus::Client.config.data_store = Prometheus::Client::DataStores::DirectFileStore.new(dir: prometheus_dir)
 
-      setup_metrics_webserver
-    end
-
-    # The webserver runs in the main process and serves only the metrics and status endpoint.
-    # This makes it possible to retrieve both even if all Puma workers of the main app are busy.
-    def setup_metrics_webserver
-      readiness_status_proc = method(:status)
-      metrics_app = Rack::Builder.new do
-        use Prometheus::Middleware::Exporter, path: '/internal/v4/metrics'
-
-        map '/internal/v4/status' do
-          run ->(_env) { readiness_status_proc.call }
-        end
-
-        map '/' do
-          run lambda { |_env|
-            # Return 404 for any other request
-            ['404', { 'Content-Type' => 'text/plain' }, ['Not Found']]
-          }
-        end
-      end
-
       Thread.new do
-        server = Puma::Server.new(metrics_app)
-
-        if config.get(:nginx, :metrics_socket).nil? || config.get(:nginx, :metrics_socket).empty?
-          server.add_tcp_listener('127.0.0.1', 9395)
-        else
-          server.add_unix_listener(@config.get(:nginx, :metrics_socket))
-        end
-
-        server.run
-      end
-    end
-
-    # Persist state for status endpoint
-    @previous_requests_count_sum = nil
-    @last_requests_count_increase_time = nil
-
-    def status
-      stats = Puma.stats_hash
-      worker_statuses = stats[:worker_status]
-      all_busy = all_workers_busy?(worker_statuses)
-      current_requests_count_sum = worker_requests_count_sum(worker_statuses)
-
-      now = Time.now
-      prev = @previous_requests_count_sum
-
-      # Track when requests_count_sum increases
-      @last_requests_count_increase_time = now if prev.nil? || current_requests_count_sum > prev
-      @previous_requests_count_sum = current_requests_count_sum
-
-      unhealthy = false
-      if all_busy && @last_requests_count_increase_time && (now - @last_requests_count_increase_time) > 60
-        # If requests_count_sum hasn't increased in 60 seconds, unhealthy
-        unhealthy = true
-      end
-
-      if all_busy && unhealthy
-        [503, { 'Content-Type' => 'text/plain' }, ['UNHEALTHY']]
-      elsif all_busy
-        [429, { 'Content-Type' => 'text/plain' }, ['BUSY']]
-      else
-        [200, { 'Content-Type' => 'text/plain' }, ['OK']]
-      end
-    rescue StandardError => e
-      [500, { 'Content-Type' => 'text/plain' }, ["Readiness check error: #{e}"]]
-    end
-
-    def all_workers_busy?(worker_statuses)
-      worker_statuses.all? do |worker|
-        worker[:last_status][:busy_threads] == worker[:last_status][:running]
-      end
-    end
-
-    def worker_requests_count_sum(worker_statuses)
-      worker_statuses.sum do |worker|
-        worker[:last_status][:requests_count] || 0
+        VCAP::CloudController::MetricsWebserver.new.start(@config)
       end
     end
 

spec/request/status_spec.rb

@@ -0,0 +1,69 @@
+require 'spec_helper'
+
+module VCAP::CloudController
+  RSpec.describe 'Status Endpoint' do
+    include Rack::Test::Methods
+
+    let(:metrics_webserver) { MetricsWebserver.new }
+
+    delegate :app, to: :metrics_webserver
+
+    describe 'GET /internal/v4/status' do
+      context 'when all workers are busy and unhealthy' do
+        before do
+          allow(Puma).to receive(:stats_hash).and_return(
+            worker_status: [
+              { last_status: { busy_threads: 2, running: 2, requests_count: 5 } },
+              { last_status: { busy_threads: 1, running: 1, requests_count: 3 } }
+            ]
+          )
+          allow(metrics_webserver).to receive(:determine_unhealthy_state).and_return(true)
+        end
+
+        it 'returns 503 UNHEALTHY' do
+          get '/internal/v4/status'
+
+          expect(last_response.status).to eq(503)
+          expect(last_response.body).to eq('UNHEALTHY')
+        end
+      end
+
+      context 'when all workers are busy but not unhealthy' do
+        before do
+          allow(Puma).to receive(:stats_hash).and_return(
+            worker_status: [
+              { last_status: { busy_threads: 2, running: 2, requests_count: 5 } },
+              { last_status: { busy_threads: 1, running: 1, requests_count: 3 } }
+            ]
+          )
+          allow(metrics_webserver).to receive(:determine_unhealthy_state).and_return(false)
+        end
+
+        it 'returns 429 BUSY' do
+          get '/internal/v4/status'
+
+          expect(last_response.status).to eq(429)
+          expect(last_response.body).to eq('BUSY')
+        end
+      end
+
+      context 'when not all workers are busy' do
+        before do
+          allow(Puma).to receive(:stats_hash).and_return(
+            worker_status: [
+              { last_status: { busy_threads: 1, running: 2, requests_count: 5 } },
+              { last_status: { busy_threads: 0, running: 1, requests_count: 3 } }
+            ]
+          )
+        end
+
+        it 'returns 200 OK' do
+          get '/internal/v4/status'
+
+          expect(last_response.status).to eq(200)
+          expect(last_response.body).to eq('OK')
+        end
+      end
+    end
+  end
+end

spec/unit/lib/cloud_controller/metrics_webserver_spec.rb

@@ -0,0 +1,41 @@
+require 'spec_helper'
+
+module VCAP::CloudController
+  RSpec.describe MetricsWebserver do
+    let(:metrics_webserver) { described_class.new }
+    let(:config) { double('config', get: nil) }
+
+    describe '#start' do
+      it 'configures and starts a Puma server' do
+        allow(Puma::Server).to receive(:new).and_call_original
+        expect_any_instance_of(Puma::Server).to receive(:run)
+
+        metrics_webserver.start(config)
+      end
+
+      context 'when no socket is specified' do
+        before do
+          allow(config).to receive(:get).with(:nginx, :metrics_socket).and_return(nil)
+        end
+
+        it 'uses a TCP listener' do
+          expect_any_instance_of(Puma::Server).to receive(:add_tcp_listener).with('127.0.0.1', 9395)
+
+          metrics_webserver.start(config)
+        end
+      end
+
+      context 'when a socket is specified' do
+        before do
+          allow(config).to receive(:get).with(:nginx, :metrics_socket).and_return('/tmp/metrics.sock')
+        end
+
+        it 'uses a Unix socket listener' do
+          expect_any_instance_of(Puma::Server).to receive(:add_unix_listener).with('/tmp/metrics.sock')
+
+          metrics_webserver.start(config)
+        end
+      end
+    end
+  end
+end

spec/unit/lib/cloud_controller/runner_spec.rb

@@ -290,36 +290,21 @@ module VCAP::CloudController
         context 'when the webserver is puma' do
           let(:test_config_overrides) { super().merge(webserver: 'puma') }
 
-          it 'sets up a separate webserver for metrics' do
-            subject
-
-            expect(Puma::Server).to have_received(:new).once
-          end
+          it 'starts the MetricsWebserver' do
+            expect(MetricsWebserver).to receive(:new).and_call_original
+            expect_any_instance_of(MetricsWebserver).to receive(:start)
 
-          it 'listens on the specified metrics port' do
             subject
-
-            expect(puma_server_double).to have_received(:add_tcp_listener).with('127.0.0.1', 9395)
-          end
-
-          context 'metrics_socket is configured' do
-            let(:test_config_overrides) { super().merge(nginx: { metrics_socket: '/tmp/metrics_socket.sock' }) }
-
-            it 'listens on the specified metrics socket' do
-              subject
-
-              expect(puma_server_double).to have_received(:add_unix_listener).with('/tmp/metrics_socket.sock')
-            end
           end
         end
 
         context 'when the webserver is not puma' do
           let(:test_config_overrides) { super().merge(webserver: 'thin') }
 
-          it 'does not set up the webserver' do
-            subject
+          it 'does not start the MetricsWebserver' do
+            expect(MetricsWebserver).not_to receive(:new)
 
-            expect(Puma::Server).not_to have_received(:new)
+            subject
           end
         end
       end