From 85b0f035aa2b1c524ab7bc9b970d6b97da0af5bf Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Fri, 12 May 2017 14:54:30 -0700 Subject: [PATCH] (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 | 52 ++++++++++++++---- spec/unit/generic_connection_pool_spec.rb | 41 ++++++++++++++ spec/unit/providers/vsphere_spec.rb | 67 +++++++++++++++++++++++ 3 files changed, 148 insertions(+), 12 deletions(-) diff --git a/lib/vmpooler/providers/vsphere.rb b/lib/vmpooler/providers/vsphere.rb index 3e1401a..ee6b3bc 100644 --- a/lib/vmpooler/providers/vsphere.rb +++ b/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 diff --git a/spec/unit/generic_connection_pool_spec.rb b/spec/unit/generic_connection_pool_spec.rb index fac7478..ff57472 100644 --- a/spec/unit/generic_connection_pool_spec.rb +++ b/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 diff --git a/spec/unit/providers/vsphere_spec.rb b/spec/unit/providers/vsphere_spec.rb index 30b1d09..74ccc66 100644 --- a/spec/unit/providers/vsphere_spec.rb +++ b/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)