From 8d75865a5ce034c41d41612d0d3c1dec80f1118f Mon Sep 17 00:00:00 2001
From: Rick Sherman <rick@shermdog.com>
Date: Tue, 10 May 2016 12:47:01 -0500
Subject: [PATCH 1/4] (RE-7014) Add support for statsd They way we were using
 graphite was incorrect for the type of data we were sending it.  statsd is
 the appropriate mechanism for our needs. statsd and graphite are mutually
 exclusive and configuring statsd will take precendence over Graphite. 
 Example of configuration in vmpooler.yaml.example

---
 Gemfile                            |  2 +
 lib/vmpooler.rb                    | 16 ++++++++
 lib/vmpooler/pool_manager.rb       | 15 ++++++--
 lib/vmpooler/statsd.rb             | 12 ++++++
 spec/spec_helper.rb                |  4 ++
 spec/vmpooler/pool_manager_spec.rb | 62 +++++++++++++++++++++++++++++-
 vmpooler                           |  8 +++-
 vmpooler.yaml.example              | 31 ++++++++++++++-
 8 files changed, 143 insertions(+), 7 deletions(-)
 create mode 100644 lib/vmpooler/statsd.rb

diff --git a/Gemfile b/Gemfile
index c71be35..c6cecf1 100644
--- a/Gemfile
+++ b/Gemfile
@@ -7,10 +7,12 @@ gem 'rbvmomi', '>= 1.8'
 gem 'redis', '>= 3.2'
 gem 'sinatra', '>= 1.4'
 gem 'net-ldap', '<= 0.12.1' # keep compatibility w/ jruby & mri-1.9.3
+gem 'statsd-ruby', '>= 1.3.0'
 
 # Test deps
 group :test do
   gem 'rack-test', '>= 0.6'
   gem 'rspec', '>= 3.2'
+  gem 'simplecov', '>= 0.11.2'
   gem 'yarjuf', '>= 2.0'
 end
diff --git a/lib/vmpooler.rb b/lib/vmpooler.rb
index 70bb819..0e3fffa 100644
--- a/lib/vmpooler.rb
+++ b/lib/vmpooler.rb
@@ -7,6 +7,7 @@ module Vmpooler
   require 'rbvmomi'
   require 'redis'
   require 'sinatra/base'
+  require "statsd-ruby"
   require 'time'
   require 'timeout'
   require 'yaml'
@@ -52,6 +53,13 @@ module Vmpooler
       parsed_config[:graphite]['prefix'] ||= 'vmpooler'
     end
 
+    # statsd is an addition and my not be present in YAML configuration
+    if parsed_config[:statsd]
+      if parsed_config[:statsd]['server']
+        parsed_config[:statsd]['prefix'] ||= 'vmpooler'
+      end
+    end
+
     if parsed_config[:tagfilter]
       parsed_config[:tagfilter].keys.each do |tag|
         parsed_config[:tagfilter][tag] = Regexp.new(parsed_config[:tagfilter][tag])
@@ -79,6 +87,14 @@ module Vmpooler
     end
   end
 
+  def self.new_statsd(server, port)
+    if server.nil? or server.empty? or server.length == 0
+      nil
+    else
+      Statsd.new server, port
+    end
+  end
+
   def self.pools(conf)
     conf[:pools]
   end
diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb
index 1a0454c..d309147 100644
--- a/lib/vmpooler/pool_manager.rb
+++ b/lib/vmpooler/pool_manager.rb
@@ -1,12 +1,17 @@
 module Vmpooler
   class PoolManager
-    def initialize(config, logger, redis, graphite=nil)
+    def initialize(config, logger, redis, graphite=nil, statsd=nil)
       $config = config
 
       # Load logger library
       $logger = logger
 
-      unless graphite.nil?
+      # statsd and graphite are mutex in the context of vmpooler
+      unless statsd.nil?
+        $statsd = statsd
+      end
+
+      unless graphite.nil? || !statsd.nil?
         $graphite = graphite
       end
 
@@ -258,6 +263,7 @@ module Vmpooler
         $redis.decr('vmpooler__tasks__clone')
 
         begin
+          $statsd.timing($config[:statsd]['prefix'] + ".clone.#{vm['template']}", finish) if defined? $statsd
           $graphite.log($config[:graphite]['prefix'] + ".clone.#{vm['template']}", finish) if defined? $graphite
         rescue
         end
@@ -565,7 +571,10 @@ module Vmpooler
       total = $redis.scard('vmpooler__pending__' + pool['name']) + ready
 
       begin
-        if defined? $graphite
+        if defined? $statsd
+          $statsd.increment($config[:statsd]['prefix'] + '.ready.' + pool['name'], $redis.scard('vmpooler__ready__' + pool['name']))
+          $statsd.increment($config[:statsd]['prefix'] + '.running.' + pool['name'], $redis.scard('vmpooler__running__' + pool['name']))
+        elsif defined? $graphite
           $graphite.log($config[:graphite]['prefix'] + '.ready.' + pool['name'], $redis.scard('vmpooler__ready__' + pool['name']))
           $graphite.log($config[:graphite]['prefix'] + '.running.' + pool['name'], $redis.scard('vmpooler__running__' + pool['name']))
         end
diff --git a/lib/vmpooler/statsd.rb b/lib/vmpooler/statsd.rb
new file mode 100644
index 0000000..7995fb5
--- /dev/null
+++ b/lib/vmpooler/statsd.rb
@@ -0,0 +1,12 @@
+require 'rubygems' unless defined?(Gem)
+
+module Vmpooler
+  class Statsd
+    def initialize(
+      s = 'statsd',
+      port = 8125
+    )
+      @server = Statsd.new s, port
+    end
+  end
+end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 2186bdb..f4fbf55 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -1,3 +1,7 @@
+require 'simplecov'
+SimpleCov.start do
+  add_filter '/spec/'
+end
 require 'helpers'
 require 'rbvmomi'
 require 'rspec'
diff --git a/spec/vmpooler/pool_manager_spec.rb b/spec/vmpooler/pool_manager_spec.rb
index a402bf4..b314ec3 100644
--- a/spec/vmpooler/pool_manager_spec.rb
+++ b/spec/vmpooler/pool_manager_spec.rb
@@ -5,13 +5,12 @@ describe 'Pool Manager' do
   let(:logger) { double('logger') }
   let(:redis) { double('redis') }
   let(:config) { {} }
-  let(:graphite) { nil }
   let(:pool) { 'pool1' }
   let(:vm) { 'vm1' }
   let(:timeout) { 5 }
   let(:host) { double('host') }
 
-  subject { Vmpooler::PoolManager.new(config, logger, redis, graphite) }
+  subject { Vmpooler::PoolManager.new(config, logger, redis) }
 
   describe '#_check_pending_vm' do
     let(:pool_helper) { double('pool') }
@@ -252,6 +251,65 @@ describe 'Pool Manager' do
     end
   end
 
+  describe '#_stats_running_ready' do
+    let(:pool_helper) { double('pool') }
+    let(:vsphere) { {pool => pool_helper} }
+    let(:graphite) { double('graphite') }
+    let(:config) { {
+      config: { task_limit: 10 },
+      pools: [ {'name' => 'pool1', 'size' => 5} ],
+      graphite: { 'prefix' => 'vmpooler' }
+    } }
+
+    before do
+      expect(subject).not_to be_nil
+      $vsphere = vsphere
+      allow(logger).to receive(:log)
+      allow(pool_helper).to receive(:find_folder)
+      allow(redis).to receive(:smembers).and_return([])
+      allow(redis).to receive(:set)
+      allow(redis).to receive(:get).with('vmpooler__tasks__clone').and_return(0)
+      allow(redis).to receive(:get).with('vmpooler__empty__pool1').and_return(nil)
+    end
+
+    context 'graphite' do
+      let(:graphite) { double('graphite') }
+      subject { Vmpooler::PoolManager.new(config, logger, redis, graphite) }
+
+      it 'increments graphite when enabled and statsd disabled' do
+        allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(1)
+        allow(redis).to receive(:scard).with('vmpooler__cloning__pool1').and_return(0)
+        allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
+        allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(5)
+
+        expect(graphite).to receive(:log).with('vmpooler.ready.pool1', 1)
+        expect(graphite).to receive(:log).with('vmpooler.running.pool1', 5)
+        subject._check_pool(config[:pools][0])
+      end
+    end
+
+    context 'statsd' do
+      let(:statsd) { double('statsd') }
+      let(:config) { {
+        config: { task_limit: 10 },
+        pools: [ {'name' => 'pool1', 'size' => 5} ],
+        statsd: { 'prefix' => 'vmpooler' }
+      } }
+      subject { Vmpooler::PoolManager.new(config, logger, redis, graphite, statsd) }
+
+      it 'increments statsd when configured' do
+        allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(1)
+        allow(redis).to receive(:scard).with('vmpooler__cloning__pool1').and_return(0)
+        allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
+        allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(5)
+
+        expect(statsd).to receive(:increment).with('vmpooler.ready.pool1', 1)
+        expect(statsd).to receive(:increment).with('vmpooler.running.pool1', 5)
+        subject._check_pool(config[:pools][0])
+      end
+    end
+  end
+
   describe '#_create_vm_snapshot' do
     let(:snapshot_manager) { 'snapshot_manager' }
     let(:pool_helper) { double('snapshot_manager') }
diff --git a/vmpooler b/vmpooler
index 5d0dd51..96b46a1 100755
--- a/vmpooler
+++ b/vmpooler
@@ -9,6 +9,11 @@ config = Vmpooler.config
 redis_host = config[:redis]['server']
 logger_file = config[:config]['logfile']
 graphite = config[:graphite]['server'] ? config[:graphite]['server'] : nil
+# statsd is an addition and my not be present in YAML configuration
+if config[:statsd]
+  statsd = config[:statsd]['server'] ? config[:statsd]['server'] : nil
+  statsd_port = config[:statsd]['port'] ? config[:statsd]['port'] : 8125
+end
 
 api = Thread.new {
   thr = Vmpooler::API.new
@@ -21,7 +26,8 @@ manager = Thread.new {
     config,
     Vmpooler.new_logger(logger_file),
     Vmpooler.new_redis(redis_host),
-    Vmpooler.new_graphite(graphite)
+    Vmpooler.new_graphite(graphite),
+    Vmpooler.new_statsd(statsd, statsd_port)
   ).execute!
 }
 
diff --git a/vmpooler.yaml.example b/vmpooler.yaml.example
index 26f2c51..5476094 100644
--- a/vmpooler.yaml.example
+++ b/vmpooler.yaml.example
@@ -53,10 +53,39 @@
 :redis:
   server: 'redis.company.com'
 
+
+  # :statsd:
+  #
+  # This section contains the connection information required to store
+  # historical data via statsd.  This is mutually exclusive with graphite 
+  # and takes precedence.
+  #
+  # Available configuration parameters:
+  #
+  #   - server
+  #     The FQDN hostname of the statsd daemon.
+  #     (optional)
+  #
+  #   - prefix
+  #     The prefix to use while storing statsd data.
+  #     (optional; default: 'vmpooler')
+  #
+  #   - port
+  #     The UDP port to communicate with statsd daemon.
+  #     (optional; default: 8125)
+
+  # Example:
+
+  :statsd:
+    server: 'statsd.company.com'
+    prefix: 'vmpooler'
+    port: 8125
+
 # :graphite:
 #
 # This section contains the connection information required to store
-# historical data in an external Graphite database.
+# historical data in an external Graphite database.  This is mutually exclusive
+# with statsd.
 #
 # Available configuration parameters:
 #

From b9834720883283e4031e617b11be2ac5300839d2 Mon Sep 17 00:00:00 2001
From: Rick Sherman <rick@shermdog.com>
Date: Tue, 7 Jun 2016 23:01:03 -0500
Subject: [PATCH 2/4] (RE-7014) Add tracking of vm gets via statsd Add the
 tracking of successful, failed, invalid, and empty pool vm gets.  It is
 possible we may want to tweak this, but have validated with spec tests and
 pcaps.

```
vmpooler-tmp-dev.ready.debian-7-x86_64:1|c
vmpooler-tmp-dev.running.debian-7-x86_64:1|c

vmpooler-tmp-dev.checkout.invalid:1|c
vmpooler-tmp-dev.checkout.success.debian-7-x86_64:1|c
vmpooler-tmp-dev.checkout.empty:1|c

vmpooler-tmp-dev.running.debian-7-x86_64:1|c

vmpooler-tmp-dev.clone.debian-7-x86_64:12.10|ms

vmpooler-tmp-dev.ready.debian-7-x86_64:1|c
```
---
 lib/vmpooler/api.rb          |  3 ++-
 lib/vmpooler/api/v1.rb       | 47 +++++++++++++++++++++++++++------
 spec/vmpooler/api/v1_spec.rb | 51 +++++++++++++++++++++++++++++++-----
 vmpooler                     |  6 ++++-
 4 files changed, 90 insertions(+), 17 deletions(-)

diff --git a/lib/vmpooler/api.rb b/lib/vmpooler/api.rb
index ee51cbc..45a9bfa 100644
--- a/lib/vmpooler/api.rb
+++ b/lib/vmpooler/api.rb
@@ -42,9 +42,10 @@ module Vmpooler
     use Vmpooler::API::Reroute
     use Vmpooler::API::V1
 
-    def configure(config, redis, environment = :production)
+    def configure(config, redis, statsd, environment = :production)
       self.settings.set :config, config
       self.settings.set :redis, redis
+      self.settings.set :statsd, statsd
       self.settings.set :environment, environment
     end
 
diff --git a/lib/vmpooler/api/v1.rb b/lib/vmpooler/api/v1.rb
index 140bfb8..42f092c 100644
--- a/lib/vmpooler/api/v1.rb
+++ b/lib/vmpooler/api/v1.rb
@@ -12,6 +12,16 @@ module Vmpooler
       Vmpooler::API.settings.redis
     end
 
+    def statsd
+      Vmpooler::API.settings.statsd
+    end
+
+    def statsd_prefix
+      if Vmpooler::API.settings.statsd
+        Vmpooler::API.settings.config[:statsd]['prefix']? Vmpooler::API.settings.config[:statsd]['prefix'] : 'vmpooler'
+      end
+    end
+
     def config
       Vmpooler::API.settings.config[:config]
     end
@@ -32,13 +42,16 @@ module Vmpooler
       newhash = {}
 
       hash.each do |key, val|
+        if Vmpooler::API.settings.config[:alias][key]
+          key = Vmpooler::API.settings.config[:alias][key]
+        end
+
         if backend.exists('vmpooler__ready__' + key)
           newhash[key] = val
+        elsif backend.exists('vmpooler__empty__' + key)
+          newhash['empty'] = (newhash['empty'] || 0) + val.to_i
         else
-          if Vmpooler::API.settings.config[:alias][key]
-            newkey = Vmpooler::API.settings.config[:alias][key]
-            newhash[newkey] = val
-          end
+          newhash['invalid'] = (newhash['invalid'] || 0) + val.to_i
         end
       end
 
@@ -94,8 +107,10 @@ module Vmpooler
           vm = fetch_single_vm(template)
           if !vm
             failed = true
+            statsd.increment(statsd_prefix + '.checkout.fail.' + template, 1)
             break
           else
+            statsd.increment(statsd_prefix + '.checkout.success.' + template, 1)
             vms << [ template, vm ]
           end
         end
@@ -375,8 +390,16 @@ module Vmpooler
       content_type :json
       result = { 'ok' => false }
 
-      if jdata and !jdata.empty?
-        result = atomically_allocate_vms(jdata)
+      if jdata
+        empty = jdata.delete('empty')
+        invalid = jdata.delete('invalid')
+        statsd.increment(statsd_prefix + '.checkout.empty', empty) if !empty.nil?
+        statsd.increment(statsd_prefix + '.checkout.invalid', invalid) if !invalid.nil?
+        if !jdata.empty?
+          result = atomically_allocate_vms(jdata)
+        else
+          status 404
+        end
       else
         status 404
       end
@@ -400,8 +423,16 @@ module Vmpooler
       content_type :json
       result = { 'ok' => false }
 
-      if payload and !payload.empty?
-        result = atomically_allocate_vms(payload)
+      if payload
+        empty = payload.delete('empty')
+        invalid = payload.delete('invalid')
+        statsd.increment(statsd_prefix + '.checkout.empty', empty) if !empty.nil?
+        statsd.increment(statsd_prefix + '.checkout.invalid', invalid) if !invalid.nil?
+        if !payload.empty?
+          result = atomically_allocate_vms(payload)
+        else
+          status 404
+        end
       else
         status 404
       end
diff --git a/spec/vmpooler/api/v1_spec.rb b/spec/vmpooler/api/v1_spec.rb
index fac75de..a269f60 100644
--- a/spec/vmpooler/api/v1_spec.rb
+++ b/spec/vmpooler/api/v1_spec.rb
@@ -180,6 +180,7 @@ describe Vmpooler::API::V1 do
 
   describe '/vm' do
     let(:redis)  { double('redis') }
+    let(:statsd)  { double('stats') }
     let(:prefix) { '/api/v1' }
     let(:config) { {
       config: {
@@ -190,20 +191,27 @@ describe Vmpooler::API::V1 do
         {'name' => 'pool1', 'size' => 5},
         {'name' => 'pool2', 'size' => 10}
       ],
-      alias: { 'poolone' => 'pool1' }
+      alias: { 'poolone' => 'pool1' },
+      statsd: { 'prefix' => 'vmpooler' }
     } }
 
     before do
       app.settings.set :config, config
       app.settings.set :redis, redis
+      app.settings.set :statsd, statsd
 
-      allow(redis).to receive(:exists).and_return '1'
+      allow(redis).to receive(:exists).with('vmpooler__token__abcdefghijklmnopqrstuvwxyz012345').and_return '1'
+      allow(redis).to receive(:exists).with('vmpooler__ready__pool1').and_return '1'
+      allow(redis).to receive(:exists).with('vmpooler__ready__pool2').and_return '1'
       allow(redis).to receive(:hget).with('vmpooler__token__abcdefghijklmnopqrstuvwxyz012345', 'user').and_return 'jdoe'
       allow(redis).to receive(:hset).and_return '1'
       allow(redis).to receive(:sadd).and_return '1'
       allow(redis).to receive(:scard).and_return '5'
       allow(redis).to receive(:spop).with('vmpooler__ready__pool1').and_return 'abcdefghijklmnop'
       allow(redis).to receive(:spop).with('vmpooler__ready__pool2').and_return 'qrstuvwxyz012345'
+      allow(statsd).to receive(:increment).with('vmpooler.checkout.success.pool1', 1)
+      allow(statsd).to receive(:increment).with('vmpooler.checkout.success.pool2', 1)
+      allow(statsd).to receive(:increment).with('vmpooler.checkout.fail.pool2', 1)
     end
 
     describe 'POST /vm' do
@@ -223,7 +231,7 @@ describe Vmpooler::API::V1 do
       end
 
       it 'returns a single VM for an alias' do
-        expect(redis).to receive(:exists).with("vmpooler__ready__poolone").and_return(false)
+        expect(redis).to receive(:exists).with("vmpooler__ready__pool1").and_return(1)
 
         post "#{prefix}/vm", '{"poolone":"1"}'
 
@@ -241,12 +249,22 @@ describe Vmpooler::API::V1 do
 
       it 'fails on nonexistent pools' do
         expect(redis).to receive(:exists).with("vmpooler__ready__poolpoolpool").and_return(false)
-
+        expect(redis).to receive(:exists).with("vmpooler__empty__poolpoolpool").and_return(false)
+        expect(statsd).to receive(:increment).with('vmpooler.checkout.invalid', 1)
         post "#{prefix}/vm", '{"poolpoolpool":"1"}'
 
         expect_json(ok = false, http = 404)
       end
 
+      it 'fails on empty pools' do
+        expect(redis).to receive(:exists).with("vmpooler__ready__emptypool").and_return(false)
+        expect(redis).to receive(:exists).with("vmpooler__empty__emptypool").and_return(true)
+        expect(statsd).to receive(:increment).with('vmpooler.checkout.empty', 1)
+        post "#{prefix}/vm", '{"emptypool":"1"}'
+
+        expect_json(ok = false, http = 404)
+      end
+
       it 'returns multiple VMs' do
         post "#{prefix}/vm", '{"pool1":"1","pool2":"1"}'
 
@@ -446,6 +464,7 @@ describe Vmpooler::API::V1 do
 
   describe '/vm/:template' do
     let(:redis)  { double('redis') }
+    let(:statsd)  { double('stats') }
     let(:prefix) { '/api/v1' }
     let(:config) { {
       config: {
@@ -456,20 +475,27 @@ describe Vmpooler::API::V1 do
         {'name' => 'pool1', 'size' => 5},
         {'name' => 'pool2', 'size' => 10}
       ],
-      alias: { 'poolone' => 'pool1' }
+      alias: { 'poolone' => 'pool1' },
+      statsd: { 'prefix' => 'vmpooler' }
     } }
 
     before do
       app.settings.set :config, config
       app.settings.set :redis, redis
+      app.settings.set :statsd, statsd
 
-      allow(redis).to receive(:exists).and_return '1'
+      allow(redis).to receive(:exists).with('vmpooler__token__abcdefghijklmnopqrstuvwxyz012345').and_return '1'
+      allow(redis).to receive(:exists).with('vmpooler__ready__pool1').and_return '1'
+      allow(redis).to receive(:exists).with('vmpooler__ready__pool2').and_return '1'
       allow(redis).to receive(:hget).with('vmpooler__token__abcdefghijklmnopqrstuvwxyz012345', 'user').and_return 'jdoe'
       allow(redis).to receive(:hset).and_return '1'
       allow(redis).to receive(:sadd).and_return '1'
       allow(redis).to receive(:scard).and_return '5'
       allow(redis).to receive(:spop).with('vmpooler__ready__pool1').and_return 'abcdefghijklmnop'
       allow(redis).to receive(:spop).with('vmpooler__ready__pool2').and_return 'qrstuvwxyz012345'
+      allow(statsd).to receive(:increment).with('vmpooler.checkout.success.pool1', 1)
+      allow(statsd).to receive(:increment).with('vmpooler.checkout.success.pool2', 1)
+      allow(statsd).to receive(:increment).with('vmpooler.checkout.fail.pool2', 1)
     end
 
     describe 'POST /vm/:template' do
@@ -489,7 +515,7 @@ describe Vmpooler::API::V1 do
       end
 
       it 'returns a single VM for an alias' do
-        expect(redis).to receive(:exists).with("vmpooler__ready__poolone").and_return(false)
+        expect(redis).to receive(:exists).with("vmpooler__ready__pool1").and_return(1)
 
         post "#{prefix}/vm/poolone", ''
 
@@ -507,12 +533,23 @@ describe Vmpooler::API::V1 do
 
       it 'fails on nonexistent pools' do
         expect(redis).to receive(:exists).with("vmpooler__ready__poolpoolpool").and_return(false)
+        expect(redis).to receive(:exists).with("vmpooler__empty__poolpoolpool").and_return(false)
+        expect(statsd).to receive(:increment).with('vmpooler.checkout.invalid', 1)
 
         post "#{prefix}/vm/poolpoolpool", ''
 
         expect_json(ok = false, http = 404)
       end
 
+      it 'fails on empty pools' do
+        expect(redis).to receive(:exists).with("vmpooler__ready__emptypool").and_return(false)
+        expect(redis).to receive(:exists).with("vmpooler__empty__emptypool").and_return(true)
+        expect(statsd).to receive(:increment).with('vmpooler.checkout.empty', 1)
+        post "#{prefix}/vm/emptypool", ''
+
+        expect_json(ok = false, http = 404)
+      end
+
       it 'returns multiple VMs' do
         post "#{prefix}/vm/pool1+pool2", ''
 
diff --git a/vmpooler b/vmpooler
index 96b46a1..44ef89c 100755
--- a/vmpooler
+++ b/vmpooler
@@ -17,7 +17,11 @@ end
 
 api = Thread.new {
   thr = Vmpooler::API.new
-  thr.helpers.configure(config, Vmpooler.new_redis(redis_host))
+  if statsd
+    thr.helpers.configure(config, Vmpooler.new_redis(redis_host), Vmpooler.new_statsd(statsd, statsd_port))
+  else
+    thr.helpers.configure(config, Vmpooler.new_redis(redis_host), statsd=nil)
+  end
   thr.helpers.execute!
 }
 

From c133bed9456fac0493b739ae87cfeae78d8083a9 Mon Sep 17 00:00:00 2001
From: Rick Sherman <rick@shermdog.com>
Date: Wed, 8 Jun 2016 11:03:22 -0500
Subject: [PATCH 3/4] (RE-7014) statsd nitpicks and additional rspec Cleaned up
 some code review nitpicks and added pool_manager_spec for empty pool.

---
 lib/vmpooler.rb                    |  2 +-
 lib/vmpooler/api/v1.rb             | 14 +++++++-------
 lib/vmpooler/pool_manager.rb       | 18 ++++++++----------
 lib/vmpooler/statsd.rb             |  7 ++-----
 spec/vmpooler/pool_manager_spec.rb | 21 +++++++++++++++++++++
 5 files changed, 39 insertions(+), 23 deletions(-)

diff --git a/lib/vmpooler.rb b/lib/vmpooler.rb
index 0e3fffa..02c8e9f 100644
--- a/lib/vmpooler.rb
+++ b/lib/vmpooler.rb
@@ -88,7 +88,7 @@ module Vmpooler
   end
 
   def self.new_statsd(server, port)
-    if server.nil? or server.empty? or server.length == 0
+    if server.nil? || server.empty?
       nil
     else
       Statsd.new server, port
diff --git a/lib/vmpooler/api/v1.rb b/lib/vmpooler/api/v1.rb
index 42f092c..b762151 100644
--- a/lib/vmpooler/api/v1.rb
+++ b/lib/vmpooler/api/v1.rb
@@ -18,7 +18,7 @@ module Vmpooler
 
     def statsd_prefix
       if Vmpooler::API.settings.statsd
-        Vmpooler::API.settings.config[:statsd]['prefix']? Vmpooler::API.settings.config[:statsd]['prefix'] : 'vmpooler'
+        Vmpooler::API.settings.config[:statsd]['prefix'] ? Vmpooler::API.settings.config[:statsd]['prefix'] : 'vmpooler'
       end
     end
 
@@ -393,9 +393,9 @@ module Vmpooler
       if jdata
         empty = jdata.delete('empty')
         invalid = jdata.delete('invalid')
-        statsd.increment(statsd_prefix + '.checkout.empty', empty) if !empty.nil?
-        statsd.increment(statsd_prefix + '.checkout.invalid', invalid) if !invalid.nil?
-        if !jdata.empty?
+        statsd.increment(statsd_prefix + '.checkout.empty', empty) if empty
+        statsd.increment(statsd_prefix + '.checkout.invalid', invalid) if invalid
+        unless jdata.empty?
           result = atomically_allocate_vms(jdata)
         else
           status 404
@@ -426,9 +426,9 @@ module Vmpooler
       if payload
         empty = payload.delete('empty')
         invalid = payload.delete('invalid')
-        statsd.increment(statsd_prefix + '.checkout.empty', empty) if !empty.nil?
-        statsd.increment(statsd_prefix + '.checkout.invalid', invalid) if !invalid.nil?
-        if !payload.empty?
+        statsd.increment(statsd_prefix + '.checkout.empty', empty) if empty
+        statsd.increment(statsd_prefix + '.checkout.invalid', invalid) if invalid
+        unless payload.empty?
           result = atomically_allocate_vms(payload)
         else
           status 404
diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb
index d309147..6490a93 100644
--- a/lib/vmpooler/pool_manager.rb
+++ b/lib/vmpooler/pool_manager.rb
@@ -1,17 +1,15 @@
 module Vmpooler
   class PoolManager
-    def initialize(config, logger, redis, graphite=nil, statsd=nil)
+    def initialize(config, logger, redis, graphite = nil, statsd = nil)
       $config = config
 
       # Load logger library
       $logger = logger
 
       # statsd and graphite are mutex in the context of vmpooler
-      unless statsd.nil?
+      if statsd
         $statsd = statsd
-      end
-
-      unless graphite.nil? || !statsd.nil?
+      elsif graphite
         $graphite = graphite
       end
 
@@ -263,8 +261,8 @@ module Vmpooler
         $redis.decr('vmpooler__tasks__clone')
 
         begin
-          $statsd.timing($config[:statsd]['prefix'] + ".clone.#{vm['template']}", finish) if defined? $statsd
-          $graphite.log($config[:graphite]['prefix'] + ".clone.#{vm['template']}", finish) if defined? $graphite
+          $statsd.timing($config[:statsd]['prefix'] + ".clone.#{vm['template']}", finish) if $statsd
+          $graphite.log($config[:graphite]['prefix'] + ".clone.#{vm['template']}", finish) if $graphite
         rescue
         end
       end
@@ -300,7 +298,7 @@ module Vmpooler
 
           $logger.log('s', "[-] [#{pool}] '#{vm}' destroyed in #{finish} seconds")
 
-          $graphite.log($config[:graphite]['prefix'] + ".destroy.#{pool}", finish) if defined? $graphite
+          $graphite.log($config[:graphite]['prefix'] + ".destroy.#{pool}", finish) if $graphite
         end
       end
     end
@@ -571,10 +569,10 @@ module Vmpooler
       total = $redis.scard('vmpooler__pending__' + pool['name']) + ready
 
       begin
-        if defined? $statsd
+        if $statsd
           $statsd.increment($config[:statsd]['prefix'] + '.ready.' + pool['name'], $redis.scard('vmpooler__ready__' + pool['name']))
           $statsd.increment($config[:statsd]['prefix'] + '.running.' + pool['name'], $redis.scard('vmpooler__running__' + pool['name']))
-        elsif defined? $graphite
+        elsif $graphite
           $graphite.log($config[:graphite]['prefix'] + '.ready.' + pool['name'], $redis.scard('vmpooler__ready__' + pool['name']))
           $graphite.log($config[:graphite]['prefix'] + '.running.' + pool['name'], $redis.scard('vmpooler__running__' + pool['name']))
         end
diff --git a/lib/vmpooler/statsd.rb b/lib/vmpooler/statsd.rb
index 7995fb5..b449a59 100644
--- a/lib/vmpooler/statsd.rb
+++ b/lib/vmpooler/statsd.rb
@@ -2,11 +2,8 @@ require 'rubygems' unless defined?(Gem)
 
 module Vmpooler
   class Statsd
-    def initialize(
-      s = 'statsd',
-      port = 8125
-    )
-      @server = Statsd.new s, port
+    def initialize(server = 'statsd', port = 8125)
+      @server = Statsd.new(server, port)
     end
   end
 end
diff --git a/spec/vmpooler/pool_manager_spec.rb b/spec/vmpooler/pool_manager_spec.rb
index b314ec3..694fc4f 100644
--- a/spec/vmpooler/pool_manager_spec.rb
+++ b/spec/vmpooler/pool_manager_spec.rb
@@ -286,6 +286,17 @@ describe 'Pool Manager' do
         expect(graphite).to receive(:log).with('vmpooler.running.pool1', 5)
         subject._check_pool(config[:pools][0])
       end
+
+      it 'increments graphite when ready with 0 when pool empty and statsd disabled' do
+        allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(0)
+        allow(redis).to receive(:scard).with('vmpooler__cloning__pool1').and_return(0)
+        allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
+        allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(5)
+
+        expect(graphite).to receive(:log).with('vmpooler.ready.pool1', 0)
+        expect(graphite).to receive(:log).with('vmpooler.running.pool1', 5)
+        subject._check_pool(config[:pools][0])
+      end
     end
 
     context 'statsd' do
@@ -307,6 +318,16 @@ describe 'Pool Manager' do
         expect(statsd).to receive(:increment).with('vmpooler.running.pool1', 5)
         subject._check_pool(config[:pools][0])
       end
+
+      it 'increments statsd ready with 0 when pool empty' do
+        allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(1)
+        allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(0)
+        allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
+        allow(statsd).to receive(:increment).with('vmpooler.running.pool1', 1)
+
+        expect(statsd).to receive(:increment).with('vmpooler.ready.pool1', 0)
+        subject._check_pool(config[:pools][0])
+      end
     end
   end
 

From b06de4bb8ef3278657551b7b89645e9f2a4ca873 Mon Sep 17 00:00:00 2001
From: Rick Sherman <rick@shermdog.com>
Date: Wed, 8 Jun 2016 13:06:31 -0500
Subject: [PATCH 4/4] (RE-7014) update statsd to use gauge for running/ready
 Previously was using increment which was incorrect for that particular
 application.

---
 lib/vmpooler/pool_manager.rb       | 4 ++--
 spec/vmpooler/pool_manager_spec.rb | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb
index 6490a93..5f54169 100644
--- a/lib/vmpooler/pool_manager.rb
+++ b/lib/vmpooler/pool_manager.rb
@@ -570,8 +570,8 @@ module Vmpooler
 
       begin
         if $statsd
-          $statsd.increment($config[:statsd]['prefix'] + '.ready.' + pool['name'], $redis.scard('vmpooler__ready__' + pool['name']))
-          $statsd.increment($config[:statsd]['prefix'] + '.running.' + pool['name'], $redis.scard('vmpooler__running__' + pool['name']))
+          $statsd.gauge($config[:statsd]['prefix'] + '.ready.' + pool['name'], $redis.scard('vmpooler__ready__' + pool['name']))
+          $statsd.gauge($config[:statsd]['prefix'] + '.running.' + pool['name'], $redis.scard('vmpooler__running__' + pool['name']))
         elsif $graphite
           $graphite.log($config[:graphite]['prefix'] + '.ready.' + pool['name'], $redis.scard('vmpooler__ready__' + pool['name']))
           $graphite.log($config[:graphite]['prefix'] + '.running.' + pool['name'], $redis.scard('vmpooler__running__' + pool['name']))
diff --git a/spec/vmpooler/pool_manager_spec.rb b/spec/vmpooler/pool_manager_spec.rb
index 694fc4f..9adeab9 100644
--- a/spec/vmpooler/pool_manager_spec.rb
+++ b/spec/vmpooler/pool_manager_spec.rb
@@ -314,8 +314,8 @@ describe 'Pool Manager' do
         allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
         allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(5)
 
-        expect(statsd).to receive(:increment).with('vmpooler.ready.pool1', 1)
-        expect(statsd).to receive(:increment).with('vmpooler.running.pool1', 5)
+        expect(statsd).to receive(:gauge).with('vmpooler.ready.pool1', 1)
+        expect(statsd).to receive(:gauge).with('vmpooler.running.pool1', 5)
         subject._check_pool(config[:pools][0])
       end
 
@@ -323,9 +323,9 @@ describe 'Pool Manager' do
         allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(1)
         allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(0)
         allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
-        allow(statsd).to receive(:increment).with('vmpooler.running.pool1', 1)
+        allow(statsd).to receive(:gauge).with('vmpooler.running.pool1', 1)
 
-        expect(statsd).to receive(:increment).with('vmpooler.ready.pool1', 0)
+        expect(statsd).to receive(:gauge).with('vmpooler.ready.pool1', 0)
         subject._check_pool(config[:pools][0])
       end
     end