(POOLER-52) Add recovery to vSphere connections

The generic connection pooler is only responsible for managing the connection
objects, however the providers themselves are responsible for ensuring that the
connection is alive/healthy etc.  Previously, the older vSphere helper would
reconnect however this was lost when the connection pooler was introduced.  This
commit adds a method that checks the connection before use, and then reconnects
if the connection is in a bad state.

lib/vmpooler/providers/vsphere.rb

@@ -2,6 +2,9 @@ module Vmpooler
   class PoolManager
     class Provider
       class VSphere < Vmpooler::PoolManager::Provider::Base
+        # The connection_pool method is normally used only for testing
+        attr_reader :connection_pool
+
         def initialize(config, logger, metrics, name, options)
           super(config, logger, metrics, name, options)
 
@@ -23,9 +26,12 @@ module Vmpooler
             timeout: connpool_timeout
           ) do
             logger.log('d', "[#{name}] Connection Pool - Creating a connection object")
+            # Need to wrap the vSphere connection object in another object. The generic connection pooler will preserve
+            # the object reference for the connection, which means it cannot "reconnect" by creating an entirely new connection
+            # object.  Instead by wrapping it in a Hash, the Hash object reference itself never changes but the content of the
+            # Hash can change, and is preserved across invocations.
             new_conn = connect_to_vsphere
-
-            new_conn
+            { connection: new_conn }
           end
         end
 
@@ -35,7 +41,8 @@ module Vmpooler
 
         def vms_in_pool(pool_name)
           vms = []
-          @connection_pool.with_metrics do |connection|
+          @connection_pool.with_metrics do |pool_object|
+            connection = ensured_vsphere_connection(pool_object)
             foldername = pool_config(pool_name)['folder']
             folder_object = find_folder(foldername, connection)
 
@@ -51,7 +58,8 @@ module Vmpooler
         def get_vm_host(_pool_name, vm_name)
           host_name = nil
 
-          @connection_pool.with_metrics do |connection|
+          @connection_pool.with_metrics do |pool_object|
+            connection = ensured_vsphere_connection(pool_object)
             vm_object = find_vm(vm_name, connection)
             return host_name if vm_object.nil?
 
@@ -62,7 +70,8 @@ module Vmpooler
 
         def find_least_used_compatible_host(_pool_name, vm_name)
           hostname = nil
-          @connection_pool.with_metrics do |connection|
+          @connection_pool.with_metrics do |pool_object|
+            connection = ensured_vsphere_connection(pool_object)
             vm_object = find_vm(vm_name, connection)
 
             return hostname if vm_object.nil?
@@ -78,7 +87,8 @@ module Vmpooler
           pool = pool_config(pool_name)
           raise("Pool #{pool_name} does not exist for the provider #{name}") if pool.nil?
 
-          @connection_pool.with_metrics do |connection|
+          @connection_pool.with_metrics do |pool_object|
+            connection = ensured_vsphere_connection(pool_object)
             vm_object = find_vm(vm_name, connection)
             raise("VM #{vm_name} does not exist in Pool #{pool_name} for the provider #{name}") if vm_object.nil?
 
@@ -99,7 +109,8 @@ module Vmpooler
 
         def get_vm(_pool_name, vm_name)
           vm_hash = nil
-          @connection_pool.with_metrics do |connection|
+          @connection_pool.with_metrics do |pool_object|
+            connection = ensured_vsphere_connection(pool_object)
             vm_object = find_vm(vm_name, connection)
             return vm_hash if vm_object.nil?
 
@@ -123,7 +134,8 @@ module Vmpooler
           pool = pool_config(pool_name)
           raise("Pool #{pool_name} does not exist for the provider #{name}") if pool.nil?
           vm_hash = nil
-          @connection_pool.with_metrics do |connection|
+          @connection_pool.with_metrics do |pool_object|
+            connection = ensured_vsphere_connection(pool_object)
             # Assume all pool config is valid i.e. not missing
             template_path = pool['template']
             target_folder_path = pool['folder']
@@ -193,7 +205,8 @@ module Vmpooler
           datastore_name = pool['datastore']
           raise("Pool #{pool_name} does not have a datastore defined for the provider #{name}") if datastore_name.nil?
 
-          @connection_pool.with_metrics do |connection|
+          @connection_pool.with_metrics do |pool_object|
+            connection = ensured_vsphere_connection(pool_object)
             vm_object = find_vm(vm_name, connection)
             raise("VM #{vm_name} in pool #{pool_name} does not exist for the provider #{name}") if vm_object.nil?
 
@@ -203,7 +216,8 @@ module Vmpooler
         end
 
         def create_snapshot(pool_name, vm_name, new_snapshot_name)
-          @connection_pool.with_metrics do |connection|
+          @connection_pool.with_metrics do |pool_object|
+            connection = ensured_vsphere_connection(pool_object)
             vm_object = find_vm(vm_name, connection)
             raise("VM #{vm_name} in pool #{pool_name} does not exist for the provider #{name}") if vm_object.nil?
 
@@ -221,7 +235,8 @@ module Vmpooler
         end
 
         def revert_snapshot(pool_name, vm_name, snapshot_name)
-          @connection_pool.with_metrics do |connection|
+          @connection_pool.with_metrics do |pool_object|
+            connection = ensured_vsphere_connection(pool_object)
             vm_object = find_vm(vm_name, connection)
             raise("VM #{vm_name} in pool #{pool_name} does not exist for the provider #{name}") if vm_object.nil?
 
@@ -234,7 +249,8 @@ module Vmpooler
         end
 
         def destroy_vm(_pool_name, vm_name)
-          @connection_pool.with_metrics do |connection|
+          @connection_pool.with_metrics do |pool_object|
+            connection = ensured_vsphere_connection(pool_object)
             vm_object = find_vm(vm_name, connection)
             # If a VM doesn't exist then it is effectively deleted
             return true if vm_object.nil?
@@ -288,6 +304,18 @@ module Vmpooler
         DISK_TYPE = 'thin'.freeze
         DISK_MODE = 'persistent'.freeze
 
+        def ensured_vsphere_connection(connection_pool_object)
+          connection_pool_object[:connection] = connect_to_vsphere unless vsphere_connection_ok?(connection_pool_object[:connection])
+          connection_pool_object[:connection]
+        end
+
+        def vsphere_connection_ok?(connection)
+          _result = connection.serviceInstance.CurrentTime
+          return true
+        rescue
+          return false
+        end
+
         def connect_to_vsphere
           max_tries = global_config[:config]['max_tries'] || 3
           retry_factor = global_config[:config]['retry_factor'] || 10

spec/unit/generic_connection_pool_spec.rb

@@ -16,6 +16,47 @@ describe 'GenericConnectionPool' do
             ) { connection_object }
   }
 
+  describe "When consuming a pool object" do
+    let(:pool_size) { 1 }
+    let(:pool_timeout) { 1 }
+    let(:connection_object) {{
+      connection: 'connection'
+    }}
+
+    it 'should return a connection object when grabbing one from the pool' do
+      subject.with_metrics do |conn_pool_object|
+        expect(conn_pool_object).to be(connection_object)
+      end
+    end
+
+    it 'should return the same connection object when calling the pool multiple times' do
+      subject.with_metrics do |conn_pool_object|
+        expect(conn_pool_object).to be(connection_object)
+      end
+      subject.with_metrics do |conn_pool_object|
+        expect(conn_pool_object).to be(connection_object)
+      end
+      subject.with_metrics do |conn_pool_object|
+        expect(conn_pool_object).to be(connection_object)
+      end
+    end
+
+    it 'should preserve connection state across mulitple pool calls' do
+      new_connection = 'new_connection'
+      # Ensure the connection is not modified
+      subject.with_metrics do |conn_pool_object|
+        expect(conn_pool_object).to be(connection_object)
+        expect(conn_pool_object[:connection]).to_not eq(new_connection)
+        # Change the connection
+        conn_pool_object[:connection] = new_connection
+      end
+      # Ensure the connection is modified
+      subject.with_metrics do |conn_pool_object|
+        expect(conn_pool_object).to be(connection_object)
+        expect(conn_pool_object[:connection]).to eq(new_connection)
+      end
+    end
+  end
 
   describe "#with_metrics" do
     before(:each) do

spec/unit/providers/vsphere_spec.rb

@@ -75,6 +75,10 @@ EOT
 
   subject { Vmpooler::PoolManager::Provider::VSphere.new(config, logger, metrics, 'vsphere', provider_options) }
 
+  before(:each) do
+    allow(subject).to receive(:vsphere_connection_ok?).and_return(true)
+  end
+
   describe '#name' do
     it 'should be vsphere' do
       expect(subject.name).to eq('vsphere')
@@ -878,6 +882,69 @@ EOT
   end
 
   # vSphere helper methods
+  describe '#ensured_vsphere_connection' do
+    let(:config) { YAML.load(<<-EOT
+---
+:config:
+:providers:
+  :vsphere:
+    # Drop the connection pool timeout way down for spec tests so they fail fast
+    connection_pool_timeout: 1
+    connection_pool_size: 1
+:pools:
+EOT
+      )
+    }
+    let(:connection1) { mock_RbVmomi_VIM_Connection(connection_options) }
+    let(:connection2) { mock_RbVmomi_VIM_Connection(connection_options) }
+
+    before(:each) do
+      allow(subject).to receive(:connect_to_vsphere).and_return(connection1)
+    end
+
+    # This is to ensure that the pool_size of 1 is in effect
+    it 'should return the same connection object when calling the pool multiple times' do
+      subject.connection_pool.with_metrics do |pool_object|
+        expect(pool_object[:connection]).to be(connection1)
+      end
+      subject.connection_pool.with_metrics do |pool_object|
+        expect(pool_object[:connection]).to be(connection1)
+      end
+      subject.connection_pool.with_metrics do |pool_object|
+        expect(pool_object[:connection]).to be(connection1)
+      end
+    end
+
+    context 'when the connection breaks' do
+      before(:each) do
+        # Emulate the connection state being good, then bad, then good again
+        expect(subject).to receive(:vsphere_connection_ok?).and_return(true, false, true)
+        expect(subject).to receive(:connect_to_vsphere).and_return(connection1, connection2)
+      end
+
+      it 'should restore the connection' do
+        subject.connection_pool.with_metrics do |pool_object|
+          # This line needs to be added to all instances of the connection_pool allocation
+          connection = subject.ensured_vsphere_connection(pool_object)
+
+          expect(connection).to be(connection1)
+        end
+
+        subject.connection_pool.with_metrics do |pool_object|
+          connection = subject.ensured_vsphere_connection(pool_object)
+          # The second connection would have failed.  This test ensures that a
+          # new connection object was created.
+          expect(connection).to be(connection2)
+        end
+
+        subject.connection_pool.with_metrics do |pool_object|
+          connection = subject.ensured_vsphere_connection(pool_object)
+          expect(connection).to be(connection2)
+        end
+      end
+    end
+  end
+
   describe '#connect_to_vsphere' do
     before(:each) do
       allow(RbVmomi::VIM).to receive(:connect).and_return(connection)