From bbd76bde4c34e347a92b8b72052cf9a8065ac4c4 Mon Sep 17 00:00:00 2001
From: John O'Connor <john.oconnor@puppet.com>
Date: Thu, 23 Apr 2020 16:37:29 +0100
Subject: [PATCH] (POOLER-160) Add Prometheus to pooler startup

This is a re-architect of the vmpooler initialisation code to:
1. Allow an API service for both manager and the api
2. Add the Prometheus endpoints to the web service.
   Needed to change the way the Rack Service is started as instantiating
   using ".New" leads to a failure to initialise the http Stats
   collection.
3. Selectively load the pooler api and/or Prometheus endpoints.
4. Rework API Spec tests for revised API loading. Needed to tidy up the
   initialisation and perform a reset! after each test to avoid "leaks"
   and dependencies between the tests.
---
 Gemfile                                     |  1 +
 bin/vmpooler                                | 10 ++-
 lib/vmpooler/api.rb                         | 77 +++++++++++----------
 lib/vmpooler/metrics/promstats.rb           |  2 +
 spec/integration/api/v1/config_spec.rb      | 12 +++-
 spec/integration/api/v1/ondemandvm_spec.rb  | 15 ++--
 spec/integration/api/v1/poolreset.rb        |  9 ++-
 spec/integration/api/v1/status_spec.rb      | 11 ++-
 spec/integration/api/v1/token_spec.rb       | 17 +++--
 spec/integration/api/v1/vm_hostname_spec.rb | 24 ++++---
 spec/integration/api/v1/vm_spec.rb          | 16 +++--
 spec/integration/api/v1/vm_template_spec.rb | 16 +++--
 spec/integration/dashboard_spec.rb          | 42 ++++++++---
 spec/unit/pool_manager_spec.rb              |  1 -
 vmpooler.gemspec                            |  1 +
 15 files changed, 166 insertions(+), 88 deletions(-)

diff --git a/Gemfile b/Gemfile
index e97d7cb..26eef16 100644
--- a/Gemfile
+++ b/Gemfile
@@ -8,6 +8,7 @@ gem 'rake', '~> 13.0'
 gem 'redis', '~> 4.1'
 gem 'rbvmomi', '~> 2.1'
 gem 'sinatra', '~> 2.0'
+gem 'prometheus-client', '~> 2.0'
 gem 'net-ldap', '~> 0.16'
 gem 'statsd-ruby', '~> 1.4.0', :require => 'statsd'
 gem 'connection_pool', '~> 2.2'
diff --git a/bin/vmpooler b/bin/vmpooler
index a54c99b..17578ac 100755
--- a/bin/vmpooler
+++ b/bin/vmpooler
@@ -25,12 +25,16 @@ end
 
 if torun.include? 'api'
   api = Thread.new do
-    thr = Vmpooler::API.new
     redis = Vmpooler.new_redis(redis_host, redis_port, redis_password)
-    thr.helpers.configure(config, redis, metrics)
-    thr.helpers.execute!
+    Vmpooler::API.execute(torun, config, redis, metrics)
   end
   torun_threads << api
+elsif metrics.respond_to?(:setup_prometheus_metrics)
+  # Run the cut down API - Prometheus Metrics only.
+  prometheus_only_api = Thread.new do
+    Vmpooler::API.execute(torun, config, nil, metrics)
+  end
+  torun_threads << prometheus_only_api
 end
 
 if torun.include? 'manager'
diff --git a/lib/vmpooler/api.rb b/lib/vmpooler/api.rb
index cf7e8ab..068f024 100644
--- a/lib/vmpooler/api.rb
+++ b/lib/vmpooler/api.rb
@@ -2,51 +2,52 @@
 
 module Vmpooler
   class API < Sinatra::Base
-    def initialize
-      super
-    end
-
-    not_found do
-      content_type :json
-
-      result = {
-        ok: false
-      }
-
-      JSON.pretty_generate(result)
-    end
-
-    # Load dashboard components
-    begin
-      require 'dashboard'
-    rescue LoadError
-      require File.expand_path(File.join(File.dirname(__FILE__), 'dashboard'))
-    end
-
-    use Vmpooler::Dashboard
-
     # Load API components
-    %w[helpers dashboard reroute v1].each do |lib|
-      begin
-        require "api/#{lib}"
-      rescue LoadError
-        require File.expand_path(File.join(File.dirname(__FILE__), 'api', lib))
-      end
+    %w[helpers dashboard reroute v1 request_logger].each do |lib|
+      require "vmpooler/api/#{lib}"
     end
+    # Load dashboard components
+    require 'vmpooler/dashboard'
 
-    use Vmpooler::API::Dashboard
-    use Vmpooler::API::Reroute
-    use Vmpooler::API::V1
-
-    def configure(config, redis, metrics)
+    def self.execute(torun, config, redis, metrics)
       self.settings.set :config, config
-      self.settings.set :redis, redis
+      self.settings.set :redis, redis unless redis.nil?
       self.settings.set :metrics, metrics
       self.settings.set :checkoutlock, Mutex.new
-    end
 
-    def execute!
-      self.settings.run!
+      # Deflating in all situations
+      # https://www.schneems.com/2017/11/08/80-smaller-rails-footprint-with-rack-deflate/
+      use Rack::Deflater
+
+      # not_found clause placed here to fix rspec test issue.
+      not_found do
+        content_type :json
+
+        result = {
+          ok: false
+        }
+
+        JSON.pretty_generate(result)
+      end
+
+      if metrics.respond_to?(:setup_prometheus_metrics)
+        # Prometheus metrics are only setup if actually specified
+        # in the config file.
+        metrics.setup_prometheus_metrics
+
+        use Prometheus::Middleware::Collector, metrics_prefix: "#{metrics.metrics_prefix}_http"
+        use Prometheus::Middleware::Exporter, path: metrics.endpoint
+      end
+
+      if torun.include? 'api'
+        use Vmpooler::Dashboard
+        use Vmpooler::API::Dashboard
+        use Vmpooler::API::Reroute
+        use Vmpooler::API::V1
+      end
+
+      # Get thee started O WebServer
+      self.run!
     end
   end
 end
diff --git a/lib/vmpooler/metrics/promstats.rb b/lib/vmpooler/metrics/promstats.rb
index 2b159b2..b5bd657 100644
--- a/lib/vmpooler/metrics/promstats.rb
+++ b/lib/vmpooler/metrics/promstats.rb
@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require 'prometheus/client'
+
 module Vmpooler
   class Promstats < Metrics
     attr_reader :prefix, :endpoint, :metrics_prefix
diff --git a/spec/integration/api/v1/config_spec.rb b/spec/integration/api/v1/config_spec.rb
index 91008a4..7462307 100644
--- a/spec/integration/api/v1/config_spec.rb
+++ b/spec/integration/api/v1/config_spec.rb
@@ -8,6 +8,13 @@ describe Vmpooler::API::V1 do
     Vmpooler::API
   end
 
+  # Added to ensure no leakage in rack state from previous tests.
+  # Removes all routes, filters, middleware and extension hooks from the current class
+  # https://rubydoc.info/gems/sinatra/Sinatra/Base#reset!-class_method 
+  before(:each) do
+    app.reset!
+  end
+
   let(:config) {
     {
       config: {
@@ -32,9 +39,8 @@ describe Vmpooler::API::V1 do
     let(:current_time) { Time.now }
 
     before(:each) do
-      app.settings.set :config, config
-      app.settings.set :redis, redis
-      app.settings.set :metrics, metrics
+      expect(app).to receive(:run!).once
+      app.execute(['api'], config, redis, metrics)
       app.settings.set :config, auth: false
       create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time)
     end
diff --git a/spec/integration/api/v1/ondemandvm_spec.rb b/spec/integration/api/v1/ondemandvm_spec.rb
index 0665973..f9b31b2 100644
--- a/spec/integration/api/v1/ondemandvm_spec.rb
+++ b/spec/integration/api/v1/ondemandvm_spec.rb
@@ -5,7 +5,14 @@ describe Vmpooler::API::V1 do
   include Rack::Test::Methods
 
   def app()
-    Vmpooler::API end
+    Vmpooler::API
+  end
+  # Added to ensure no leakage in rack state from previous tests.
+  # Removes all routes, filters, middleware and extension hooks from the current class
+  # https://rubydoc.info/gems/sinatra/Sinatra/Base#reset!-class_method 
+  before(:each) do
+    app.reset!
+  end
 
   describe '/ondemandvm' do
     let(:prefix) { '/api/v1' }
@@ -32,13 +39,11 @@ describe Vmpooler::API::V1 do
     let(:current_time) { Time.now }
     let(:vmname) { 'abcdefghijkl' }
     let(:checkoutlock) { Mutex.new }
-    let(:redis) { MockRedis.new }
     let(:uuid) { SecureRandom.uuid }
 
     before(:each) do
-      app.settings.set :config, config
-      app.settings.set :redis, redis
-      app.settings.set :metrics, metrics
+      expect(app).to receive(:run!).once
+      app.execute(['api'], config, redis, metrics)
       app.settings.set :config, auth: false
       app.settings.set :checkoutlock, checkoutlock
       create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time)
diff --git a/spec/integration/api/v1/poolreset.rb b/spec/integration/api/v1/poolreset.rb
index 9c87c6c..043989e 100644
--- a/spec/integration/api/v1/poolreset.rb
+++ b/spec/integration/api/v1/poolreset.rb
@@ -8,6 +8,10 @@ describe Vmpooler::API::V1 do
     Vmpooler::API
   end
 
+  after(:each) do
+    Vmpooler::API.reset!
+  end
+
   let(:config) {
     {
       config: {
@@ -32,9 +36,8 @@ describe Vmpooler::API::V1 do
     let(:current_time) { Time.now }
 
     before(:each) do
-      app.settings.set :config, config
-      app.settings.set :redis, redis
-      app.settings.set :metrics, metrics
+      expect(app).to receive(:run!).once
+      app.execute(['api'], config, redis, metrics)
       app.settings.set :config, auth: false
       create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time)
     end
diff --git a/spec/integration/api/v1/status_spec.rb b/spec/integration/api/v1/status_spec.rb
index 10f127f..486c263 100644
--- a/spec/integration/api/v1/status_spec.rb
+++ b/spec/integration/api/v1/status_spec.rb
@@ -12,6 +12,13 @@ describe Vmpooler::API::V1 do
     Vmpooler::API
   end
 
+  # Added to ensure no leakage in rack state from previous tests.
+  # Removes all routes, filters, middleware and extension hooks from the current class
+  # https://rubydoc.info/gems/sinatra/Sinatra/Base#reset!-class_method 
+  before(:each) do
+    app.reset!
+  end
+
   describe 'status and metrics endpoints' do
     let(:prefix) { '/api/v1' }
 
@@ -32,8 +39,8 @@ describe Vmpooler::API::V1 do
     let(:current_time) { Time.now }
 
     before(:each) do
-      app.settings.set :config, config
-      app.settings.set :redis, redis
+      expect(app).to receive(:run!).once
+      app.execute(['api'], config, redis, nil)
       app.settings.set :config, auth: false
       create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time)
     end
diff --git a/spec/integration/api/v1/token_spec.rb b/spec/integration/api/v1/token_spec.rb
index 983dac6..72e60bd 100644
--- a/spec/integration/api/v1/token_spec.rb
+++ b/spec/integration/api/v1/token_spec.rb
@@ -8,14 +8,21 @@ describe Vmpooler::API::V1 do
     Vmpooler::API
   end
 
+  # Added to ensure no leakage in rack state from previous tests.
+  # Removes all routes, filters, middleware and extension hooks from the current class
+  # https://rubydoc.info/gems/sinatra/Sinatra/Base#reset!-class_method 
+  before(:each) do
+    app.reset!
+  end
+
   describe '/token' do
     let(:prefix) { '/api/v1' }
     let(:current_time) { Time.now }
     let(:config) { { } }
 
-    before do
-      app.settings.set :config, config
-      app.settings.set :redis, redis
+    before(:each) do
+      expect(app).to receive(:run!).once
+      app.execute(['api'], config, redis, nil)
     end
 
     describe 'GET /token' do
@@ -97,7 +104,9 @@ describe Vmpooler::API::V1 do
     let(:prefix) { '/api/v1' }
     let(:current_time) { Time.now }
 
-    before do
+    before(:each) do
+      expect(app).to receive(:run!).once
+      app.execute(['api'], config, redis, nil)
       app.settings.set :config, config
       app.settings.set :redis, redis
     end
diff --git a/spec/integration/api/v1/vm_hostname_spec.rb b/spec/integration/api/v1/vm_hostname_spec.rb
index bca9866..18fcec3 100644
--- a/spec/integration/api/v1/vm_hostname_spec.rb
+++ b/spec/integration/api/v1/vm_hostname_spec.rb
@@ -12,8 +12,16 @@ describe Vmpooler::API::V1 do
     Vmpooler::API
   end
 
+  # Added to ensure no leakage in rack state from previous tests.
+  # Removes all routes, filters, middleware and extension hooks from the current class
+  # https://rubydoc.info/gems/sinatra/Sinatra/Base#reset!-class_method 
+  before(:each) do
+    app.reset!
+  end
+
   describe '/vm/:hostname' do
     let(:prefix) { '/api/v1' }
+    let(:metrics) { Vmpooler::DummyStatsd.new }
 
     let(:config) {
       {
@@ -33,11 +41,9 @@ describe Vmpooler::API::V1 do
 
     let(:current_time) { Time.now }
 
-    let(:redis) { MockRedis.new }
-
     before(:each) do
-      app.settings.set :config, config
-      app.settings.set :redis, redis
+      expect(app).to receive(:run!).once
+      app.execute(['api'], config, redis, metrics)
       create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time)
     end
 
@@ -81,9 +87,9 @@ describe Vmpooler::API::V1 do
       end
 
       context '(tagfilter configured)' do
-        let(:config) { {
-          tagfilter: { 'url' => '(.*)\/' },
-        } }
+        before(:each) do
+          app.settings.set :config, tagfilter: { 'url' => '(.*)\/' }
+        end
 
         it 'correctly filters tags' do
           create_vm('testhost', redis)
@@ -104,7 +110,9 @@ describe Vmpooler::API::V1 do
       end
 
       context '(auth not configured)' do
-        let(:config) { { auth: false } }
+        before(:each) do
+          app.settings.set :config, auth: false
+        end
 
         it 'allows VM lifetime to be modified without a token' do
           create_vm('testhost', redis)
diff --git a/spec/integration/api/v1/vm_spec.rb b/spec/integration/api/v1/vm_spec.rb
index 8e3799b..7f5c29a 100644
--- a/spec/integration/api/v1/vm_spec.rb
+++ b/spec/integration/api/v1/vm_spec.rb
@@ -8,6 +8,13 @@ describe Vmpooler::API::V1 do
     Vmpooler::API
   end
 
+  # Added to ensure no leakage in rack state from previous tests.
+  # Removes all routes, filters, middleware and extension hooks from the current class
+  # https://rubydoc.info/gems/sinatra/Sinatra/Base#reset!-class_method 
+  before(:each) do
+    app.reset!
+  end
+
   describe '/vm' do
     let(:prefix) { '/api/v1' }
     let(:metrics) { Vmpooler::DummyStatsd.new }
@@ -32,9 +39,8 @@ describe Vmpooler::API::V1 do
     let(:checkoutlock) { Mutex.new }
 
     before(:each) do
-      app.settings.set :config, config
-      app.settings.set :redis, redis
-      app.settings.set :metrics, metrics
+      expect(app).to receive(:run!).once
+      app.execute(['api'], config, redis, metrics)
       app.settings.set :config, auth: false
       app.settings.set :checkoutlock, checkoutlock
       create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time)
@@ -104,8 +110,8 @@ describe Vmpooler::API::V1 do
       end
 
       it 'returns 503 for empty pool when aliases are not defined' do
-        Vmpooler::API.settings.config.delete(:alias)
-        Vmpooler::API.settings.config[:pool_names] = ['pool1', 'pool2']
+        app.settings.config.delete(:alias)
+        app.settings.config[:pool_names] = ['pool1', 'pool2']
 
         create_ready_vm 'pool1', vmname, redis
         post "#{prefix}/vm/pool1"
diff --git a/spec/integration/api/v1/vm_template_spec.rb b/spec/integration/api/v1/vm_template_spec.rb
index 043c8e5..da83116 100644
--- a/spec/integration/api/v1/vm_template_spec.rb
+++ b/spec/integration/api/v1/vm_template_spec.rb
@@ -8,6 +8,13 @@ describe Vmpooler::API::V1 do
     Vmpooler::API
   end
 
+  # Added to ensure no leakage in rack state from previous tests.
+  # Removes all routes, filters, middleware and extension hooks from the current class
+  # https://rubydoc.info/gems/sinatra/Sinatra/Base#reset!-class_method 
+  before(:each) do
+    app.reset!
+  end
+
   describe '/vm/:template' do
     let(:prefix) { '/api/v1' }
     let(:metrics) { Vmpooler::DummyStatsd.new }
@@ -33,9 +40,8 @@ describe Vmpooler::API::V1 do
     let(:checkoutlock) { Mutex.new }
 
     before(:each) do
-      app.settings.set :config, config
-      app.settings.set :redis, redis
-      app.settings.set :metrics, metrics
+      expect(app).to receive(:run!).once
+      app.execute(['api'], config, redis, metrics)
       app.settings.set :config, auth: false
       app.settings.set :checkoutlock, checkoutlock
       create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time)
@@ -84,8 +90,8 @@ describe Vmpooler::API::V1 do
       end
 
       it 'returns 503 for empty pool when aliases are not defined' do
-        Vmpooler::API.settings.config.delete(:alias)
-        Vmpooler::API.settings.config[:pool_names] = ['pool1', 'pool2']
+        app.settings.config.delete(:alias)
+        app.settings.config[:pool_names] = ['pool1', 'pool2']
 
         create_ready_vm 'pool1', 'abcdefghijklmnop', redis
         post "#{prefix}/vm/pool1"
diff --git a/spec/integration/dashboard_spec.rb b/spec/integration/dashboard_spec.rb
index 9d5a64a..32929b7 100644
--- a/spec/integration/dashboard_spec.rb
+++ b/spec/integration/dashboard_spec.rb
@@ -5,13 +5,35 @@ describe Vmpooler::API do
   include Rack::Test::Methods
 
   def app()
-    described_class
+    Vmpooler::API
+  end
+
+  # Added to ensure no leakage in rack state from previous tests.
+  # Removes all routes, filters, middleware and extension hooks from the current class
+  # https://rubydoc.info/gems/sinatra/Sinatra/Base#reset!-class_method 
+  before(:each) do
+    app.reset!
   end
 
   describe 'Dashboard' do
 
+    let(:config) { {
+      pools: [
+          {'name' => 'pool1', 'size' => 5}
+      ],
+      graphite: {}
+    } }
+
+    before(:each) do
+      expect(app).to receive(:run!)
+      app.execute(['api'], config, redis, nil)
+      app.settings.set :config, auth: false
+    end
+
     context '/' do
-      before { get '/' }
+      before(:each) do
+        get '/'
+      end
 
       it { expect(last_response.status).to eq(302) }
       it { expect(last_response.location).to eq('http://example.org/dashboard/') }
@@ -21,7 +43,7 @@ describe Vmpooler::API do
       ENV['SITE_NAME'] = 'test pooler'
       ENV['VMPOOLER_CONFIG'] = 'thing'
 
-      before do
+      before(:each) do
         get '/dashboard/'
       end
 
@@ -31,10 +53,12 @@ describe Vmpooler::API do
     end
 
     context 'unknown route' do
-      before { get '/doesnotexist' }
+      before(:each) do
+        get '/doesnotexist'
+      end
 
-      it { expect(last_response.status).to eq(404) }
       it { expect(last_response.header['Content-Type']).to eq('application/json') }
+      it { expect(last_response.status).to eq(404) }
       it { expect(last_response.body).to eq(JSON.pretty_generate({ok: false})) }
     end
 
@@ -47,10 +71,8 @@ describe Vmpooler::API do
           graphite: {}
       } }
 
-      before do
-        $config = config
+      before(:each) do
         app.settings.set :config, config
-        app.settings.set :redis, redis
         app.settings.set :environment, :test
       end
 
@@ -120,10 +142,8 @@ describe Vmpooler::API do
           graphite: {}
       } }
 
-      before do
-        $config = config
+      before(:each) do
         app.settings.set :config, config
-        app.settings.set :redis, redis
         app.settings.set :environment, :test
       end
 
diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 8e75f45..88688d6 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -27,7 +27,6 @@ describe 'Pool Manager' do
     timeout: 5
   ) { MockRedis.new }
   }
-  let(:redis) { MockRedis.new }
 
   let(:provider) { Vmpooler::PoolManager::Provider::Base.new(config, logger, metrics, redis_connection_pool, 'mock_provider', provider_options) }
 
diff --git a/vmpooler.gemspec b/vmpooler.gemspec
index b0c5ff2..f10e18f 100644
--- a/vmpooler.gemspec
+++ b/vmpooler.gemspec
@@ -24,6 +24,7 @@ Gem::Specification.new do |s|
   s.add_dependency 'redis', '~> 4.1'
   s.add_dependency 'rbvmomi', '~> 2.1'
   s.add_dependency 'sinatra', '~> 2.0'
+  s.add_dependency 'prometheus-client', '~> 2.0'
   s.add_dependency 'net-ldap', '~> 0.16'
   s.add_dependency 'statsd-ruby', '~> 1.4'
   s.add_dependency 'connection_pool', '~> 2.2'