From c133bed9456fac0493b739ae87cfeae78d8083a9 Mon Sep 17 00:00:00 2001 From: Rick Sherman Date: Wed, 8 Jun 2016 11:03:22 -0500 Subject: [PATCH] (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