(RE-7014) statsd nitpicks and additional rspec

Cleaned up some code review nitpicks and added pool_manager_spec for empty pool.

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

lib/vmpooler/api/v1.rb

@@ -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.empty', empty) if empty
-        statsd.increment(statsd_prefix + '.checkout.invalid', invalid) if !invalid.nil?
+        statsd.increment(statsd_prefix + '.checkout.invalid', invalid) if invalid
-        if !jdata.empty?
+        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.empty', empty) if empty
-        statsd.increment(statsd_prefix + '.checkout.invalid', invalid) if !invalid.nil?
+        statsd.increment(statsd_prefix + '.checkout.invalid', invalid) if invalid
-        if !payload.empty?
+        unless payload.empty?
           result = atomically_allocate_vms(payload)
         else
           status 404

lib/vmpooler/pool_manager.rb

@@ -7,11 +7,9 @@ module Vmpooler
       $logger = logger
 
       # statsd and graphite are mutex in the context of vmpooler
-      unless statsd.nil?
+      if statsd
         $statsd = statsd
-      end
+      elsif graphite
-
-      unless graphite.nil? || !statsd.nil?
         $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
+          $statsd.timing($config[:statsd]['prefix'] + ".clone.#{vm['template']}", finish) if $statsd
-          $graphite.log($config[:graphite]['prefix'] + ".clone.#{vm['template']}", finish) if defined? $graphite
+          $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

lib/vmpooler/statsd.rb

@@ -2,11 +2,8 @@ require 'rubygems' unless defined?(Gem)
 
 module Vmpooler
   class Statsd
-    def initialize(
+    def initialize(server = 'statsd', port = 8125)
-      s = 'statsd',
+      @server = Statsd.new(server, port)
-      port = 8125
-    )
-      @server = Statsd.new s, port
     end
   end
 end

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