From 6449f1aab1752cc8c901b0454e41895bbba8d76c Mon Sep 17 00:00:00 2001
From: "kirby@puppetlabs.com" <mk.kirby@gmail.com>
Date: Tue, 24 Jul 2018 10:41:46 -0700
Subject: [PATCH] Fix number of ends at end of file. Move update_pool_size out
 of repopulate_pool_vms. Remove instances of check_pool_vms within the test.

---
 lib/vmpooler/pool_manager.rb   |  55 +++--
 spec/unit/pool_manager_spec.rb | 381 ++++++++++++++++-----------------
 2 files changed, 209 insertions(+), 227 deletions(-)

diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb
index d580f40..e2da636 100644
--- a/lib/vmpooler/pool_manager.rb
+++ b/lib/vmpooler/pool_manager.rb
@@ -822,37 +822,31 @@ module Vmpooler
     end
 
     def repopulate_pool_vms(pool_name, provider, pool_check_response, pool_size)
-      unless pool_mutex(pool_name).locked?
-        ready = $redis.scard("vmpooler__ready__#{pool_name}")
-        total = $redis.scard("vmpooler__pending__#{pool_name}") + ready
+      return if pool_mutex(pool_name).locked?
+      ready = $redis.scard("vmpooler__ready__#{pool_name}")
+      total = $redis.scard("vmpooler__pending__#{pool_name}") + ready
 
-        $metrics.gauge("ready.#{pool_name}", $redis.scard("vmpooler__ready__#{pool_name}"))
-        $metrics.gauge("running.#{pool_name}", $redis.scard("vmpooler__running__#{pool_name}"))
+      $metrics.gauge("ready.#{pool_name}", $redis.scard("vmpooler__ready__#{pool_name}"))
+      $metrics.gauge("running.#{pool_name}", $redis.scard("vmpooler__running__#{pool_name}"))
 
-        if $redis.get("vmpooler__empty__#{pool_name}")
-          $redis.del("vmpooler__empty__#{pool_name}") unless ready.zero?
-        elsif ready.zero?
-          $redis.set("vmpooler__empty__#{pool_name}", 'true')
-          $logger.log('s', "[!] [#{pool_name}] is empty")
-        end
+      if $redis.get("vmpooler__empty__#{pool_name}")
+        $redis.del("vmpooler__empty__#{pool_name}") unless ready.zero?
+      elsif ready.zero?
+        $redis.set("vmpooler__empty__#{pool_name}", 'true')
+        $logger.log('s', "[!] [#{pool_name}] is empty")
+      end
 
-        # Check to see if a pool size change has been made via the configuration API
-        # Since check_pool runs in a loop it does not
-        # otherwise identify this change when running
-        update_pool_size(pool)
-
-        if total < pool_size
-          (1..(pool_size - total)).each do |_i|
-            if $redis.get('vmpooler__tasks__clone').to_i < $config[:config]['task_limit'].to_i
-              begin
-                $redis.incr('vmpooler__tasks__clone')
-                pool_check_response[:cloned_vms] += 1
-                clone_vm(pool, provider)
-              rescue => err
-                $logger.log('s', "[!] [#{pool_name}] clone failed during check_pool with an error: #{err}")
-                $redis.decr('vmpooler__tasks__clone')
-                raise
-              end
+      if total < pool_size
+        (1..(pool_size - total)).each do |_i|
+          if $redis.get('vmpooler__tasks__clone').to_i < $config[:config]['task_limit'].to_i
+            begin
+              $redis.incr('vmpooler__tasks__clone')
+              pool_check_response[:cloned_vms] += 1
+              clone_vm(pool, provider)
+            rescue => err
+              $logger.log('s', "[!] [#{pool_name}] clone failed during check_pool with an error: #{err}")
+              $redis.decr('vmpooler__tasks__clone')
+              raise
             end
           end
         end
@@ -894,6 +888,11 @@ module Vmpooler
       # Additionally, a pool will drain ready and pending instances
       evaluate_template(pool, provider)
 
+      # Check to see if a pool size change has been made via the configuration API
+      # Since check_pool runs in a loop it does not
+      # otherwise identify this change when running
+      update_pool_size(pool)
+
       repopulate_pool_vms(pool['name'], provider, pool_check_response, pool['size'])
 
       # Remove VMs in excess of the configured pool size
diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index ef69b25..b1d8dcd 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -3103,207 +3103,185 @@ EOT
       }
     }
 
-      it 'should not call clone_vm when number of VMs is equal to the pool size' do
-        expect(provider).to receive(:vms_in_pool).with(pool).and_return([])
+    it 'should not call clone_vm when number of VMs is equal to the pool size' do
+      expect(subject).to receive(:clone_vm).exactly(0).times
+
+      subject.repopulate_pool_vms(pool, provider, pool_check_response, pool_size)
+    end
+
+    it 'should not call clone_vm when number of VMs is greater than the pool size' do
+      create_ready_vm(pool,vm,token)
+      expect(subject).to receive(:clone_vm).exactly(0).times
+
+      subject._check_pool(pool_object,provider)
+    end
+
+    ['ready','pending'].each do |queue_name|
+      it "should use VMs in #{queue_name} queue to caculate pool size" do
         expect(subject).to receive(:clone_vm).exactly(0).times
-
-        subject.repopulate_pool_vms(pool ,provider, pool_check_response, pool_size)
-      end
-
-      it 'should not call clone_vm when number of VMs is greater than the pool size' do
-        expect(provider).to receive(:vms_in_pool).with(pool).and_return(vm_response)
-        create_ready_vm(pool,vm,token)
-        expect(subject).to receive(:clone_vm).exactly(0).times
-
+        # Modify the pool size to 1 and add a VM in the queue
+        redis.sadd("vmpooler__#{queue_name}__#{pool}",vm)
+        config[:pools][0]['size'] = 1
+  
         subject._check_pool(pool_object,provider)
       end
-
-      ['ready','pending'].each do |queue_name|
-        it "should use VMs in #{queue_name} queue to caculate pool size" do
-          expect(provider).to receive(:vms_in_pool).with(pool).and_return(vm_response)
-          expect(subject).to receive(:clone_vm).exactly(0).times
-          # Modify the pool size to 1 and add a VM in the queue
-          redis.sadd("vmpooler__#{queue_name}__#{pool}",vm)
-          config[:pools][0]['size'] = 1
-   
-          subject._check_pool(pool_object,provider)
-        end
-      end
-
-      ['running','completed','discovered','migrating'].each do |queue_name|
-        it "should not use VMs in #{queue_name} queue to caculate pool size" do
-          expect(provider).to receive(:vms_in_pool).with(pool).and_return(vm_response)
-          expect(subject).to receive(:clone_vm)
-          # Modify the pool size to 1 and add a VM in the queue
-          redis.sadd("vmpooler__#{queue_name}__#{pool}",vm)
-          config[:pools][0]['size'] = 1
-
-          subject._check_pool(pool_object,provider)
-        end
-      end
-
-      it 'should log a message the first time a pool is empty' do
-        expect(provider).to receive(:vms_in_pool).with(pool).and_return([])
-        expect(logger).to receive(:log).with('s', "[!] [#{pool}] is empty")
-
-        subject._check_pool(pool_object,provider)
-      end
-
-      context 'when pool is marked as empty' do
-        let(:vm_response) {
-          # Mock response from Base Provider for vms_in_pool
-          [{ 'name' => vm}]
-        }          
-
-        before(:each) do
-          redis.set("vmpooler__empty__#{pool}", 'true')
-        end
-
-        it 'should not log a message when the pool remains empty' do
-          expect(provider).to receive(:vms_in_pool).with(pool).and_return([])
-          expect(logger).to receive(:log).with('s', "[!] [#{pool}] is empty").exactly(0).times
-
-          subject._check_pool(pool_object,provider)
-        end
-
-        it 'should remove the empty pool mark if it is no longer empty' do
-          expect(provider).to receive(:vms_in_pool).with(pool).and_return(vm_response)
-          create_ready_vm(pool,vm,token)
-
-          expect(redis.get("vmpooler__empty__#{pool}")).to be_truthy
-          subject._check_pool(pool_object,provider)
-          expect(redis.get("vmpooler__empty__#{pool}")).to be_falsey
-        end
-      end
-
-      context 'when number of VMs is less than the pool size' do
-        before(:each) do
-          expect(provider).to receive(:vms_in_pool).with(pool).and_return([])
-        end
-
-        it 'should return the number of cloned VMs' do
-          pool_size = 5
-          config[:pools][0]['size'] = pool_size
-
-          result = subject._check_pool(pool_object,provider)
-
-          expect(result[:cloned_vms]).to be(pool_size)
-        end
-
-        it 'should call clone_vm to populate the pool' do
-          pool_size = 5
-          config[:pools][0]['size'] = pool_size
-
-          expect(subject).to receive(:clone_vm).exactly(pool_size).times
-          
-          subject._check_pool(pool_object,provider)
-        end
-
-        it 'should call clone_vm until task_limit is hit' do
-          task_limit = 2
-          pool_size = 5
-          config[:pools][0]['size'] = pool_size
-          config[:config]['task_limit'] = task_limit
-
-          expect(subject).to receive(:clone_vm).exactly(task_limit).times
-          
-          subject._check_pool(pool_object,provider)
-        end
-
-        it 'log a message if a cloning error occurs' do
-          pool_size = 1
-          config[:pools][0]['size'] = pool_size
-
-          expect(subject).to receive(:clone_vm).and_raise(RuntimeError,"MockError")
-          expect(logger).to receive(:log).with("s", "[!] [#{pool}] clone failed during check_pool with an error: MockError")
-          
-          expect{ subject._check_pool(pool_object,provider) }.to raise_error(RuntimeError,'MockError')
-        end
-      end
-
-      context 'when a pool size configuration change is detected' do
-        let(:poolsize) { 2 }
-        let(:newpoolsize) { 3 }
-        before(:each) do
-          config[:pools][0]['size'] = poolsize
-          redis.hset('vmpooler__config__poolsize', pool, newpoolsize)
-          expect(provider).to receive(:vms_in_pool).with(pool).and_return([])
-        end
-
-        it 'should change the pool size configuration' do
-          subject._check_pool(config[:pools][0],provider)
-
-          expect(config[:pools][0]['size']).to be(newpoolsize)
-        end
-      end
-
-      context 'when a pool template is updating' do
-        let(:poolsize) { 2 }
-        before(:each) do
-          redis.hset('vmpooler__config__updating', pool, 1)
-          expect(provider).to receive(:vms_in_pool).with(pool).and_return([])
-        end
-
-        it 'should not call clone_vm to populate the pool' do
-          expect(subject).to_not receive(:clone_vm)
-
-          subject._check_pool(config[:pools][0],provider)
-        end
-      end
-
-      context 'when an excess number of ready vms exist' do
-
-        before(:each) do
-          allow(redis).to receive(:scard)
-          expect(redis).to receive(:scard).with("vmpooler__ready__#{pool}").and_return(1)
-          expect(redis).to receive(:scard).with("vmpooler__pending__#{pool}").and_return(1)
-          expect(provider).to receive(:vms_in_pool).with(pool).and_return([])
-        end
-
-        it 'should call remove_excess_vms' do
-          expect(subject).to receive(:remove_excess_vms).with(config[:pools][0], provider, 1, 2)
-
-          subject._check_pool(config[:pools][0],provider)
-        end
-      end
-
-      context 'export metrics' do
-        it 'increments metrics for ready queue' do
-          create_ready_vm(pool,'vm1')
-          create_ready_vm(pool,'vm2')
-          create_ready_vm(pool,'vm3')
-          expect(provider).to receive(:vms_in_pool).with(pool).and_return(multi_vm_response)
-
-          expect(metrics).to receive(:gauge).with("ready.#{pool}", 3)
-          allow(metrics).to receive(:gauge)
-
-          subject._check_pool(pool_object,provider)
-        end
-
-        it 'increments metrics for running queue' do
-          create_running_vm(pool,'vm1',token)
-          create_running_vm(pool,'vm2',token)
-          create_running_vm(pool,'vm3',token)
-          expect(provider).to receive(:vms_in_pool).with(pool).and_return(multi_vm_response)
-
-          expect(metrics).to receive(:gauge).with("running.#{pool}", 3)
-          allow(metrics).to receive(:gauge)
-
-          subject._check_pool(pool_object,provider)
-        end
-
-        it 'increments metrics with 0 when pool empty' do
-        expect(provider).to receive(:vms_in_pool).with(pool).and_return([])
-
-          expect(metrics).to receive(:gauge).with("ready.#{pool}", 0)
-          expect(metrics).to receive(:gauge).with("running.#{pool}", 0)
-
-          subject._check_pool(pool_object,provider)
-        end
-      end
     end
-end
 
+    ['running','completed','discovered','migrating'].each do |queue_name|
+      it "should not use VMs in #{queue_name} queue to caculate pool size" do
+        expect(subject).to receive(:clone_vm)
+        # Modify the pool size to 1 and add a VM in the queue
+        redis.sadd("vmpooler__#{queue_name}__#{pool}",vm)
+        config[:pools][0]['size'] = 1
+
+        subject._check_pool(pool_object,provider)
+      end
+    end
+
+    it 'should log a message the first time a pool is empty' do
+      expect(logger).to receive(:log).with('s', "[!] [#{pool}] is empty")
+
+      subject._check_pool(pool_object,provider)
+    end
+
+    context 'when pool is marked as empty' do
+
+      before(:each) do
+        redis.set("vmpooler__empty__#{pool}", 'true')
+      end
+
+      it 'should not log a message when the pool remains empty' do
+        expect(logger).to receive(:log).with('s', "[!] [#{pool}] is empty").exactly(0).times
+
+        subject._check_pool(pool_object,provider)
+      end
+
+      it 'should remove the empty pool mark if it is no longer empty' do
+        create_ready_vm(pool,vm,token)
+
+        expect(redis.get("vmpooler__empty__#{pool}")).to be_truthy
+        subject._check_pool(pool_object,provider)
+        expect(redis.get("vmpooler__empty__#{pool}")).to be_falsey
+      end
+    end
+
+    context 'when number of VMs is less than the pool size' do
+
+      it 'should return the number of cloned VMs' do
+        pool_size = 5
+        config[:pools][0]['size'] = pool_size
+
+        result = subject._check_pool(pool_object,provider)
+
+        expect(result[:cloned_vms]).to be(pool_size)
+      end
+
+      it 'should call clone_vm to populate the pool' do
+        pool_size = 5
+        config[:pools][0]['size'] = pool_size
+
+        expect(subject).to receive(:clone_vm).exactly(pool_size).times
+        
+        subject._check_pool(pool_object,provider)
+      end
+
+      it 'should call clone_vm until task_limit is hit' do
+        task_limit = 2
+        pool_size = 5
+        config[:pools][0]['size'] = pool_size
+        config[:config]['task_limit'] = task_limit
+
+        expect(subject).to receive(:clone_vm).exactly(task_limit).times
+        
+        subject._check_pool(pool_object,provider)
+      end
+
+      it 'log a message if a cloning error occurs' do
+        pool_size = 1
+        config[:pools][0]['size'] = pool_size
+
+        expect(subject).to receive(:clone_vm).and_raise(RuntimeError,"MockError")
+        expect(logger).to receive(:log).with("s", "[!] [#{pool}] clone failed during check_pool with an error: MockError")
+        
+        expect{ subject._check_pool(pool_object,provider) }.to raise_error(RuntimeError,'MockError')
+      end
+    end
+
+    context 'when a pool size configuration change is detected' do
+      let(:poolsize) { 2 }
+      let(:newpoolsize) { 3 }
+      before(:each) do
+        config[:pools][0]['size'] = poolsize
+        redis.hset('vmpooler__config__poolsize', pool, newpoolsize)
+      end
+
+      it 'should change the pool size configuration' do
+        subject._check_pool(config[:pools][0],provider)
+
+        expect(config[:pools][0]['size']).to be(newpoolsize)
+      end
+    end
+
+    context 'when a pool template is updating' do
+      let(:poolsize) { 2 }
+      before(:each) do
+        redis.hset('vmpooler__config__updating', pool, 1)
+      end
+
+      it 'should not call clone_vm to populate the pool' do
+        expect(subject).to_not receive(:clone_vm)
+
+        subject._check_pool(config[:pools][0],provider)
+      end
+    end
+
+    context 'when an excess number of ready vms exist' do
+
+      before(:each) do
+        allow(redis).to receive(:scard)
+        expect(redis).to receive(:scard).with("vmpooler__ready__#{pool}").and_return(1)
+        expect(redis).to receive(:scard).with("vmpooler__pending__#{pool}").and_return(1)
+      end
+
+      it 'should call remove_excess_vms' do
+        expect(subject).to receive(:remove_excess_vms).with(config[:pools][0], provider, 1, 2)
+
+        subject._check_pool(config[:pools][0],provider)
+      end
+    end
+
+    context 'export metrics' do
+      it 'increments metrics for ready queue' do
+        create_ready_vm(pool,'vm1')
+        create_ready_vm(pool,'vm2')
+        create_ready_vm(pool,'vm3')
+
+        expect(metrics).to receive(:gauge).with("ready.#{pool}", 3)
+        allow(metrics).to receive(:gauge)
+
+        subject._check_pool(pool_object,provider)
+      end
+
+      it 'increments metrics for running queue' do
+        create_running_vm(pool,'vm1',token)
+        create_running_vm(pool,'vm2',token)
+        create_running_vm(pool,'vm3',token)
+
+        expect(metrics).to receive(:gauge).with("running.#{pool}", 3)
+        allow(metrics).to receive(:gauge)
+
+        subject._check_pool(pool_object,provider)
+      end
+
+      it 'increments metrics with 0 when pool empty' do
+
+        expect(metrics).to receive(:gauge).with("ready.#{pool}", 0)
+        expect(metrics).to receive(:gauge).with("running.#{pool}", 0)
+
+        subject._check_pool(pool_object,provider)
+      end
+    end
+  end
 
   describe '#_check_pool' do
     let(:new_vm_response) {
@@ -3568,9 +3546,14 @@ EOT
         subject._check_pool(pool_object,provider)
       end
     end
+    #REPOPULATE
+    context 'when checking if pools need to be repopulated' do
+      it 'should call #repopulate_pool_vms' do
+        expect(subject).to receive(:repopulate_pool_vms).with(pool, provider, pool_check_response, config[:pools][0]['size'])
 
+        subject._check_pool(pool_object, provider)
+      end
+    end
 
-
-    # REPOPULATE
-    context 'Repopulate a pool' 
+  end
 end