Update clone_vm so the redis connection is released during the VM clone.

lib/vmpooler/pool_manager.rb

@@ -323,7 +323,7 @@ module Vmpooler
       end
     end
 
-    def generate_and_check_hostname(redis)
+    def generate_and_check_hostname
       # Generate a randomized hostname. The total name must no longer than 15
       # character including the hyphen. The shortest adjective in the corpus is
       # three characters long. Therefore, we can technically select a noun up to 11
@@ -332,20 +332,22 @@ module Vmpooler
       # letter adjectives, we actually limit the noun to 10 letters to avoid
       # inviting more conflicts. We favor selecting a longer noun rather than a
       # longer adjective because longer adjectives tend to be less fun.
-      noun = @name_generator.noun(max: 10)
+      @redis.with do |redis|
-      adjective = @name_generator.adjective(max: 14 - noun.length)
+        noun = @name_generator.noun(max: 10)
-      random_name = [adjective, noun].join('-')
+        adjective = @name_generator.adjective(max: 14 - noun.length)
-      hostname = $config[:config]['prefix'] + random_name
+        random_name = [adjective, noun].join('-')
-      available = redis.hlen('vmpooler__vm__' + hostname) == 0
+        hostname = $config[:config]['prefix'] + random_name
+        available = redis.hlen('vmpooler__vm__' + hostname) == 0
 
-      [hostname, available]
+        [hostname, available]
+      end
     end
 
-    def find_unique_hostname(pool_name, redis)
+    def find_unique_hostname(pool_name)
       hostname_retries = 0
       max_hostname_retries = 3
       while hostname_retries < max_hostname_retries
-        hostname, available = generate_and_check_hostname(redis)
+        hostname, available = generate_and_check_hostname
         break if available
 
         hostname_retries += 1
@@ -359,10 +361,10 @@ module Vmpooler
     end
 
     def _clone_vm(pool_name, provider, request_id = nil, pool_alias = nil)
-      @redis.with_metrics do |redis|
+      new_vmname = find_unique_hostname(pool_name)
-        new_vmname = find_unique_hostname(pool_name, redis)
+      mutex = vm_mutex(new_vmname)
-        mutex = vm_mutex(new_vmname)
+      mutex.synchronize do
-        mutex.synchronize do
+        @redis.with_metrics do |redis|
           # Add VM to Redis inventory ('pending' pool)
           redis.multi
           redis.sadd('vmpooler__pending__' + pool_name, new_vmname)
@@ -372,31 +374,37 @@ module Vmpooler
           redis.hset('vmpooler__vm__' + new_vmname, 'request_id', request_id) if request_id
           redis.hset('vmpooler__vm__' + new_vmname, 'pool_alias', pool_alias) if pool_alias
           redis.exec
+        end
 
-          begin
+        begin
-            $logger.log('d', "[ ] [#{pool_name}] Starting to clone '#{new_vmname}'")
+          $logger.log('d', "[ ] [#{pool_name}] Starting to clone '#{new_vmname}'")
-            start = Time.now
+          start = Time.now
-            provider.create_vm(pool_name, new_vmname)
+          provider.create_vm(pool_name, new_vmname)
-            finish = format('%<time>.2f', time: Time.now - start)
+          finish = format('%<time>.2f', time: Time.now - start)
 
+          @redis.with_metrics do |redis|
             redis.pipelined do
               redis.hset('vmpooler__clone__' + Date.today.to_s, pool_name + ':' + new_vmname, finish)
               redis.hset('vmpooler__vm__' + new_vmname, 'clone_time', finish)
             end
-            $logger.log('s', "[+] [#{pool_name}] '#{new_vmname}' cloned in #{finish} seconds")
+          end
+          $logger.log('s', "[+] [#{pool_name}] '#{new_vmname}' cloned in #{finish} seconds")
 
-            $metrics.timing("clone.#{pool_name}", finish)
+          $metrics.timing("clone.#{pool_name}", finish)
-          rescue StandardError
+        rescue StandardError
+          @redis.with_metrics do |redis|
             redis.pipelined do
               redis.srem("vmpooler__pending__#{pool_name}", new_vmname)
               expiration_ttl = $config[:redis]['data_ttl'].to_i * 60 * 60
               redis.expire("vmpooler__vm__#{new_vmname}", expiration_ttl)
             end
-            raise
+          end
-          ensure
+          raise
-            if request_id
+        ensure
-              @tasks['ondemand_clone_count'] -= 1
+          if request_id
-            else
+            @tasks['ondemand_clone_count'] -= 1
+          else
+            @redis.with_metrics do |redis|
               redis.decr('vmpooler__tasks__clone')
             end
           end
@@ -1169,7 +1177,7 @@ module Vmpooler
       end
     end
 
-    def check_pending_pool_vms(pool_name, provider, pool_check_response, inventory, pool_timeout = nil)
+    def check_pending_pool_vms(pool_name, provider, pool_check_response, inventory, pool_timeout)
       pool_timeout ||= $config[:config]['timeout'] || 15
       @redis.with_metrics do |redis|
         redis.smembers("vmpooler__pending__#{pool_name}").reverse.each do |vm|

spec/unit/pool_manager_spec.rb

@@ -740,7 +740,7 @@ EOT
         expect(provider).to receive(:create_vm).with(pool, String)
         allow(logger).to receive(:log)
         redis_connection_pool.with do |redis|
-          expect(subject).to receive(:find_unique_hostname).with(pool, redis).and_return(vm)
+          expect(subject).to receive(:find_unique_hostname).with(pool).and_return(vm)
         end
       end
 
@@ -786,7 +786,7 @@ EOT
         expect(provider).to receive(:create_vm).with(pool, String).and_raise('MockError')
         allow(logger).to receive(:log)
         redis_connection_pool.with do |redis|
-          expect(subject).to receive(:find_unique_hostname).with(pool, redis).and_return(vm)
+          expect(subject).to receive(:find_unique_hostname).with(pool).and_return(vm)
         end
       end
 
@@ -833,7 +833,7 @@ EOT
         expect(provider).to receive(:create_vm).with(pool, String)
         allow(logger).to receive(:log)
         redis_connection_pool.with do |redis|
-          expect(subject).to receive(:find_unique_hostname).with(pool, redis).and_return(vm)
+          expect(subject).to receive(:find_unique_hostname).with(pool).and_return(vm)
         end
       end
 
@@ -3793,6 +3793,7 @@ EOT
 
   describe '#check_pending_pool_vms' do
     let(:provider) { double('provider') }
+    let(:timeout) { 10 }
     let(:pool_check_response) {
       {:checked_pending_vms => 0}
     }
@@ -3814,7 +3815,7 @@ EOT
           expect(subject).to receive(:fail_pending_vm).with(vm,pool,Integer,redis,false)
         end
 
-        subject.check_pending_pool_vms(pool, provider, pool_check_response, inventory)
+        subject.check_pending_pool_vms(pool, provider, pool_check_response, inventory, timeout)
       end
     end
 
@@ -3832,7 +3833,7 @@ EOT
 
       it 'should return the number of checked pending VMs' do
         allow(subject).to receive(:check_pending_vm)
-        subject.check_pending_pool_vms(pool, provider, pool_check_response, inventory)
+        subject.check_pending_pool_vms(pool, provider, pool_check_response, inventory, timeout)
 
         expect(pool_check_response[:checked_pending_vms]).to be(1)
       end
@@ -3841,7 +3842,7 @@ EOT
         expect(subject).to receive(:check_pending_vm).and_raise(RuntimeError,'MockError')
         expect(logger).to receive(:log).with('d', "[!] [#{pool}] _check_pool failed with an error while evaluating pending VMs: MockError")
 
-        subject.check_pending_pool_vms(pool, provider, pool_check_response, inventory)
+        subject.check_pending_pool_vms(pool, provider, pool_check_response, inventory, timeout)
       end
 
       it 'should use the pool timeout if set' do
@@ -3859,13 +3860,13 @@ EOT
         config[:config]['timeout'] = big_lifetime
         expect(subject).to receive(:check_pending_vm).with(vm,pool,big_lifetime,provider)
 
-        subject.check_pending_pool_vms(pool, provider, pool_check_response, inventory)
+        subject.check_pending_pool_vms(pool, provider, pool_check_response, inventory, big_lifetime)
       end
 
       it 'should use a pool timeout of 15 if nothing is set' do
-        expect(subject).to receive(:check_pending_vm).with(vm,pool,15,provider)
+        expect(subject).to receive(:check_pending_vm).with(vm,pool,timeout,provider)
 
-        subject.check_pending_pool_vms(pool, provider, pool_check_response, inventory)
+        subject.check_pending_pool_vms(pool, provider, pool_check_response, inventory, timeout)
       end
     end
   end