From df783f0ed061c7f0b2acff4f4350e0aee6a6ebad Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Tue, 18 Apr 2017 16:21:34 -0700 Subject: [PATCH] (POOLER-52) Use a Connection Pooler for vSphere connections Previously the vSphere Provider would share a single vSphere connection for all pools under management. This would cause issues in large environments as this would cause errors to be thrown or operations to slow down. This commit modifies the vSphere Provider to use a connection pool when communicating with the vSphere API - Uses the GenericConnectionPool object to manage the connection pool - Uses a default connection pool size of: Whatever is biggest from: - How many pools this provider services - Maximum number of cloning tasks allowed - Need at least 2 connections so that a pool can have inventory functions performed while cloning etc. - A large connection_pool_timeout is used as a connection object is consumed during a VM clone, which can take up to 2 minutes - Removes the `get_connection` method as that is now obsolete due to the connection pool - Removes the `close` method as it is now obsolete - Modified the spec tests slightly, to stop mocking get_connection as it no longer exists, and set a super low pool timeout so that if a test fails, it will fail quickly instead of taking the default time of 60+ seconds --- lib/vmpooler/providers/vsphere.rb | 317 ++++++++++++++-------------- spec/unit/providers/vsphere_spec.rb | 117 ++-------- 2 files changed, 185 insertions(+), 249 deletions(-) diff --git a/lib/vmpooler/providers/vsphere.rb b/lib/vmpooler/providers/vsphere.rb index 6dc9819..3e1401a 100644 --- a/lib/vmpooler/providers/vsphere.rb +++ b/lib/vmpooler/providers/vsphere.rb @@ -2,159 +2,188 @@ module Vmpooler class PoolManager class Provider class VSphere < Vmpooler::PoolManager::Provider::Base + def initialize(config, logger, metrics, name, options) + super(config, logger, metrics, name, options) + + task_limit = global_config[:config].nil? || global_config[:config]['task_limit'].nil? ? 10 : global_config[:config]['task_limit'].to_i + # The default connection pool size is: + # Whatever is biggest from: + # - How many pools this provider services + # - Maximum number of cloning tasks allowed + # - Need at least 2 connections so that a pool can have inventory functions performed while cloning etc. + default_connpool_size = [provided_pools.count, task_limit, 2].max + connpool_size = provider_config['connection_pool_size'].nil? ? default_connpool_size : provider_config['connection_pool_size'].to_i + # The default connection pool timeout should be quite large - 60 seconds + connpool_timeout = provider_config['connection_pool_timeout'].nil? ? 60 : provider_config['connection_pool_timeout'].to_i + logger.log('d', "[#{name}] ConnPool - Creating a connection pool of size #{connpool_size} with timeout #{connpool_timeout}") + @connection_pool = Vmpooler::PoolManager::GenericConnectionPool.new( + metrics: metrics, + metric_prefix: "#{name}_provider_connection_pool", + size: connpool_size, + timeout: connpool_timeout + ) do + logger.log('d', "[#{name}] Connection Pool - Creating a connection object") + new_conn = connect_to_vsphere + + new_conn + end + end + def name 'vsphere' end def vms_in_pool(pool_name) - connection = get_connection - - foldername = pool_config(pool_name)['folder'] - folder_object = find_folder(foldername, connection) - vms = [] + @connection_pool.with_metrics do |connection| + foldername = pool_config(pool_name)['folder'] + folder_object = find_folder(foldername, connection) - return vms if folder_object.nil? + return vms if folder_object.nil? - folder_object.childEntity.each do |vm| - vms << { 'name' => vm.name } + folder_object.childEntity.each do |vm| + vms << { 'name' => vm.name } + end end - vms end def get_vm_host(_pool_name, vm_name) - connection = get_connection - - vm_object = find_vm(vm_name, connection) - return nil if vm_object.nil? - host_name = nil - host_name = vm_object.summary.runtime.host.name if vm_object.summary && vm_object.summary.runtime && vm_object.summary.runtime.host + @connection_pool.with_metrics do |connection| + vm_object = find_vm(vm_name, connection) + return host_name if vm_object.nil? + + host_name = vm_object.summary.runtime.host.name if vm_object.summary && vm_object.summary.runtime && vm_object.summary.runtime.host + end host_name end def find_least_used_compatible_host(_pool_name, vm_name) - connection = get_connection + hostname = nil + @connection_pool.with_metrics do |connection| + vm_object = find_vm(vm_name, connection) - vm_object = find_vm(vm_name, connection) + return hostname if vm_object.nil? + host_object = find_least_used_vpshere_compatible_host(vm_object) - return nil if vm_object.nil? - host_object = find_least_used_vpshere_compatible_host(vm_object) - - return nil if host_object.nil? - host_object[0].name + return hostname if host_object.nil? + hostname = host_object[0].name + end + hostname end def migrate_vm_to_host(pool_name, vm_name, dest_host_name) pool = pool_config(pool_name) raise("Pool #{pool_name} does not exist for the provider #{name}") if pool.nil? - connection = get_connection + @connection_pool.with_metrics do |connection| + 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? - 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? + target_cluster_name = get_target_cluster_from_config(pool_name) + cluster = find_cluster(target_cluster_name, connection) + raise("Pool #{pool_name} specifies cluster #{target_cluster_name} which does not exist for the provider #{name}") if cluster.nil? - target_cluster_name = get_target_cluster_from_config(pool_name) - cluster = find_cluster(target_cluster_name, connection) - raise("Pool #{pool_name} specifies cluster #{target_cluster_name} which does not exist for the provider #{name}") if cluster.nil? - - # Go through each host and initiate a migration when the correct host name is found - cluster.host.each do |host| - if host.name == dest_host_name - migrate_vm_host(vm_object, host) - return true + # Go through each host and initiate a migration when the correct host name is found + cluster.host.each do |host| + if host.name == dest_host_name + migrate_vm_host(vm_object, host) + return true + end end end - false end def get_vm(_pool_name, vm_name) - connection = get_connection + vm_hash = nil + @connection_pool.with_metrics do |connection| + vm_object = find_vm(vm_name, connection) + return vm_hash if vm_object.nil? - vm_object = find_vm(vm_name, connection) - return nil if vm_object.nil? - - vm_folder_path = get_vm_folder_path(vm_object) - # Find the pool name based on the folder path - pool_name = nil - template_name = nil - global_config[:pools].each do |pool| - if pool['folder'] == vm_folder_path - pool_name = pool['name'] - template_name = pool['template'] + vm_folder_path = get_vm_folder_path(vm_object) + # Find the pool name based on the folder path + pool_name = nil + template_name = nil + global_config[:pools].each do |pool| + if pool['folder'] == vm_folder_path + pool_name = pool['name'] + template_name = pool['template'] + end end - end - generate_vm_hash(vm_object, template_name, pool_name) + vm_hash = generate_vm_hash(vm_object, template_name, pool_name) + end + vm_hash end def create_vm(pool_name, new_vmname) 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| + # Assume all pool config is valid i.e. not missing + template_path = pool['template'] + target_folder_path = pool['folder'] + target_datastore = pool['datastore'] + target_cluster_name = get_target_cluster_from_config(pool_name) - connection = get_connection + # Extract the template VM name from the full path + raise("Pool #{pool_name} did specify a full path for the template for the provider #{name}") unless template_path =~ /\// + templatefolders = template_path.split('/') + template_name = templatefolders.pop - # Assume all pool config is valid i.e. not missing - template_path = pool['template'] - target_folder_path = pool['folder'] - target_datastore = pool['datastore'] - target_cluster_name = get_target_cluster_from_config(pool_name) + # Get the actual objects from vSphere + template_folder_object = find_folder(templatefolders.join('/'), connection) + raise("Pool #{pool_name} specifies a template folder of #{templatefolders.join('/')} which does not exist for the provider #{name}") if template_folder_object.nil? - # Extract the template VM name from the full path - raise("Pool #{pool_name} did specify a full path for the template for the provider #{name}") unless template_path =~ /\// - templatefolders = template_path.split('/') - template_name = templatefolders.pop + template_vm_object = template_folder_object.find(template_name) + raise("Pool #{pool_name} specifies a template VM of #{template_name} which does not exist for the provider #{name}") if template_vm_object.nil? - # Get the actual objects from vSphere - template_folder_object = find_folder(templatefolders.join('/'), connection) - raise("Pool #{pool_name} specifies a template folder of #{templatefolders.join('/')} which does not exist for the provider #{name}") if template_folder_object.nil? + # Annotate with creation time, origin template, etc. + # Add extraconfig options that can be queried by vmtools + config_spec = RbVmomi::VIM.VirtualMachineConfigSpec( + annotation: JSON.pretty_generate( + name: new_vmname, + created_by: provider_config['username'], + base_template: template_path, + creation_timestamp: Time.now.utc + ), + extraConfig: [ + { key: 'guestinfo.hostname', value: new_vmname } + ] + ) - template_vm_object = template_folder_object.find(template_name) - raise("Pool #{pool_name} specifies a template VM of #{template_name} which does not exist for the provider #{name}") if template_vm_object.nil? + # Choose a cluster/host to place the new VM on + target_host_object = find_least_used_host(target_cluster_name, connection) - # Annotate with creation time, origin template, etc. - # Add extraconfig options that can be queried by vmtools - config_spec = RbVmomi::VIM.VirtualMachineConfigSpec( - annotation: JSON.pretty_generate( + # Put the VM in the specified folder and resource pool + relocate_spec = RbVmomi::VIM.VirtualMachineRelocateSpec( + datastore: find_datastore(target_datastore, connection), + host: target_host_object, + diskMoveType: :moveChildMostDiskBacking + ) + + # Create a clone spec + clone_spec = RbVmomi::VIM.VirtualMachineCloneSpec( + location: relocate_spec, + config: config_spec, + powerOn: true, + template: false + ) + + # Create the new VM + new_vm_object = template_vm_object.CloneVM_Task( + folder: find_folder(target_folder_path, connection), name: new_vmname, - created_by: provider_config['username'], - base_template: template_path, - creation_timestamp: Time.now.utc - ), - extraConfig: [ - { key: 'guestinfo.hostname', value: new_vmname } - ] - ) + spec: clone_spec + ).wait_for_completion - # Choose a cluster/host to place the new VM on - target_host_object = find_least_used_host(target_cluster_name, connection) - - # Put the VM in the specified folder and resource pool - relocate_spec = RbVmomi::VIM.VirtualMachineRelocateSpec( - datastore: find_datastore(target_datastore, connection), - host: target_host_object, - diskMoveType: :moveChildMostDiskBacking - ) - - # Create a clone spec - clone_spec = RbVmomi::VIM.VirtualMachineCloneSpec( - location: relocate_spec, - config: config_spec, - powerOn: true, - template: false - ) - - # Create the new VM - new_vm_object = template_vm_object.CloneVM_Task( - folder: find_folder(target_folder_path, connection), - name: new_vmname, - spec: clone_spec - ).wait_for_completion - - generate_vm_hash(new_vm_object, template_path, pool_name) + vm_hash = generate_vm_hash(new_vm_object, template_path, pool_name) + end + vm_hash end def create_disk(pool_name, vm_name, disk_size) @@ -164,62 +193,58 @@ 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 = get_connection - - 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? - - add_disk(vm_object, disk_size, datastore_name, connection) + @connection_pool.with_metrics do |connection| + 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? + add_disk(vm_object, disk_size, datastore_name, connection) + end true end def create_snapshot(pool_name, vm_name, new_snapshot_name) - connection = get_connection + @connection_pool.with_metrics do |connection| + 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? - 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? - - old_snap = find_snapshot(vm_object, new_snapshot_name) - raise("Snapshot #{new_snapshot_name} for VM #{vm_name} in pool #{pool_name} already exists for the provider #{name}") unless old_snap.nil? - - vm_object.CreateSnapshot_Task( - name: new_snapshot_name, - description: 'vmpooler', - memory: true, - quiesce: true - ).wait_for_completion + old_snap = find_snapshot(vm_object, new_snapshot_name) + raise("Snapshot #{new_snapshot_name} for VM #{vm_name} in pool #{pool_name} already exists for the provider #{name}") unless old_snap.nil? + vm_object.CreateSnapshot_Task( + name: new_snapshot_name, + description: 'vmpooler', + memory: true, + quiesce: true + ).wait_for_completion + end true end def revert_snapshot(pool_name, vm_name, snapshot_name) - connection = get_connection + @connection_pool.with_metrics do |connection| + 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? - 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? - - snapshot_object = find_snapshot(vm_object, snapshot_name) - raise("Snapshot #{snapshot_name} for VM #{vm_name} in pool #{pool_name} does not exist for the provider #{name}") if snapshot_object.nil? - - snapshot_object.RevertToSnapshot_Task.wait_for_completion + snapshot_object = find_snapshot(vm_object, snapshot_name) + raise("Snapshot #{snapshot_name} for VM #{vm_name} in pool #{pool_name} does not exist for the provider #{name}") if snapshot_object.nil? + snapshot_object.RevertToSnapshot_Task.wait_for_completion + end true end def destroy_vm(_pool_name, vm_name) - connection = get_connection + @connection_pool.with_metrics do |connection| + vm_object = find_vm(vm_name, connection) + # If a VM doesn't exist then it is effectively deleted + return true if vm_object.nil? - vm_object = find_vm(vm_name, connection) - # If a VM doesn't exist then it is effectively deleted - return true if vm_object.nil? - - # Poweroff the VM if it's running - vm_object.PowerOffVM_Task.wait_for_completion if vm_object.runtime && vm_object.runtime.powerState && vm_object.runtime.powerState == 'poweredOn' - - # Kill it with fire - vm_object.Destroy_Task.wait_for_completion + # Poweroff the VM if it's running + vm_object.PowerOffVM_Task.wait_for_completion if vm_object.runtime && vm_object.runtime.powerState && vm_object.runtime.powerState == 'poweredOn' + # Kill it with fire + vm_object.Destroy_Task.wait_for_completion + end true end @@ -263,16 +288,6 @@ module Vmpooler DISK_TYPE = 'thin'.freeze DISK_MODE = 'persistent'.freeze - def get_connection - begin - @connection.serviceInstance.CurrentTime - rescue - @connection = connect_to_vsphere - end - - @connection - end - def connect_to_vsphere max_tries = global_config[:config]['max_tries'] || 3 retry_factor = global_config[:config]['retry_factor'] || 10 @@ -667,10 +682,6 @@ module Vmpooler relospec = RbVmomi::VIM.VirtualMachineRelocateSpec(host: host) vm.RelocateVM_Task(spec: relospec).wait_for_completion end - - def close - @connection.close - end end end end diff --git a/spec/unit/providers/vsphere_spec.rb b/spec/unit/providers/vsphere_spec.rb index 948e6da..30b1d09 100644 --- a/spec/unit/providers/vsphere_spec.rb +++ b/spec/unit/providers/vsphere_spec.rb @@ -53,6 +53,8 @@ describe 'Vmpooler::PoolManager::Provider::VSphere' do username: "vcenter_user" password: "vcenter_password" insecure: true + # Drop the connection pool timeout way down for spec tests so they fail fast + connection_pool_timeout: 1 :pools: - name: '#{poolname}' alias: [ 'mockpool' ] @@ -84,7 +86,7 @@ EOT let(:pool_config) { config[:pools][0] } before(:each) do - allow(subject).to receive(:get_connection).and_return(connection) + allow(subject).to receive(:connect_to_vsphere).and_return(connection) end context 'Given a pool folder that is missing' do @@ -93,7 +95,7 @@ EOT end it 'should get a connection' do - expect(subject).to receive(:get_connection).and_return(connection) + expect(subject).to receive(:connect_to_vsphere).and_return(connection) subject.vms_in_pool(poolname) end @@ -111,7 +113,7 @@ EOT end it 'should get a connection' do - expect(subject).to receive(:get_connection).and_return(connection) + expect(subject).to receive(:connect_to_vsphere).and_return(connection) subject.vms_in_pool(poolname) end @@ -140,7 +142,7 @@ EOT end it 'should get a connection' do - expect(subject).to receive(:get_connection).and_return(connection) + expect(subject).to receive(:connect_to_vsphere).and_return(connection) subject.vms_in_pool(poolname) end @@ -155,7 +157,7 @@ EOT describe '#get_vm_host' do before(:each) do - allow(subject).to receive(:get_connection).and_return(connection) + allow(subject).to receive(:connect_to_vsphere).and_return(connection) expect(subject).to receive(:find_vm).with(vmname,connection).and_return(vm_object) end @@ -163,7 +165,7 @@ EOT let(:vm_object) { nil } it 'should get a connection' do - expect(subject).to receive(:get_connection).and_return(connection) + expect(subject).to receive(:connect_to_vsphere).and_return(connection) subject.get_vm_host(poolname,vmname) end @@ -185,7 +187,7 @@ EOT end it 'should get a connection' do - expect(subject).to receive(:get_connection).and_return(connection) + expect(subject).to receive(:connect_to_vsphere).and_return(connection) subject.get_vm_host(poolname,vmname) end @@ -208,7 +210,7 @@ EOT end it 'should get a connection' do - expect(subject).to receive(:get_connection).and_return(connection) + expect(subject).to receive(:connect_to_vsphere).and_return(connection) subject.get_vm_host(poolname,vmname) end @@ -223,7 +225,7 @@ EOT let(:vm_object) { nil } before(:each) do - allow(subject).to receive(:get_connection).and_return(connection) + allow(subject).to receive(:connect_to_vsphere).and_return(connection) expect(subject).to receive(:find_vm).with(vmname,connection).and_return(vm_object) end @@ -231,7 +233,7 @@ EOT let(:vm_object) { nil } it 'should get a connection' do - expect(subject).to receive(:get_connection).and_return(connection) + expect(subject).to receive(:connect_to_vsphere).and_return(connection) subject.find_least_used_compatible_host(poolname,vmname) end @@ -250,7 +252,7 @@ EOT end it 'should get a connection' do - expect(subject).to receive(:get_connection).and_return(connection) + expect(subject).to receive(:connect_to_vsphere).and_return(connection) subject.find_least_used_compatible_host(poolname,vmname) end @@ -272,7 +274,7 @@ EOT end it 'should get a connection' do - expect(subject).to receive(:get_connection).and_return(connection) + expect(subject).to receive(:connect_to_vsphere).and_return(connection) subject.find_least_used_compatible_host(poolname,vmname) end @@ -293,7 +295,7 @@ EOT before(:each) do config[:pools][0]['clone_target'] = cluster_name - allow(subject).to receive(:get_connection).and_return(connection) + allow(subject).to receive(:connect_to_vsphere).and_return(connection) allow(subject).to receive(:find_vm).and_return(vm_object) end @@ -389,7 +391,7 @@ EOT describe '#get_vm' do let(:vm_object) { nil } before(:each) do - allow(subject).to receive(:get_connection).and_return(connection) + allow(subject).to receive(:connect_to_vsphere).and_return(connection) expect(subject).to receive(:find_vm).with(vmname,connection).and_return(vm_object) end @@ -510,7 +512,7 @@ EOT let(:new_vm_object) { mock_RbVmomi_VIM_VirtualMachine({ :name => vmname }) } before(:each) do - allow(subject).to receive(:get_connection).and_return(connection) + allow(subject).to receive(:connect_to_vsphere).and_return(connection) allow(connection.serviceInstance).to receive(:find_datacenter).and_return(datacenter_object) end @@ -585,7 +587,7 @@ EOT let(:datastorename) { 'datastore0' } let(:disk_size) { 10 } before(:each) do - allow(subject).to receive(:get_connection).and_return(connection) + allow(subject).to receive(:connect_to_vsphere).and_return(connection) allow(subject).to receive(:find_vm).with(vmname, connection).and_return(vm_object) end @@ -643,7 +645,7 @@ EOT let(:vm_object) { mock_RbVmomi_VIM_VirtualMachine({ :name => vmname, :snapshot_tree => snapshot_tree }) } before(:each) do - allow(subject).to receive(:get_connection).and_return(connection) + allow(subject).to receive(:connect_to_vsphere).and_return(connection) allow(subject).to receive(:find_vm).with(vmname,connection).and_return(vm_object) end @@ -698,7 +700,7 @@ EOT let(:vm_object) { mock_RbVmomi_VIM_VirtualMachine({ :name => vmname, :snapshot_tree => snapshot_tree }) } before(:each) do - allow(subject).to receive(:get_connection).and_return(connection) + allow(subject).to receive(:connect_to_vsphere).and_return(connection) allow(subject).to receive(:find_vm).with(vmname,connection).and_return(vm_object) end @@ -747,7 +749,7 @@ EOT let(:destroy_task) { mock_RbVmomi_VIM_Task() } before(:each) do - allow(subject).to receive(:get_connection).and_return(connection) + allow(subject).to receive(:connect_to_vsphere).and_return(connection) end context 'Given a missing VM name' do @@ -876,57 +878,6 @@ EOT end # vSphere helper methods - describe '#get_connection' do - before(:each) do - # NOTE - Using instance_variable_set is a code smell of code that is not testable - subject.instance_variable_set("@connection",connection) - end - - context 'when connection is ok' do - it 'should not attempt to reconnect' do - expect(subject).to receive(:connect_to_vsphere).exactly(0).times - - subject.get_connection() - end - - it 'should return a connection' do - result = subject.get_connection() - - expect(result).to be(connection) - end - end - - context 'when connection has broken' do - before(:each) do - expect(connection.serviceInstance).to receive(:CurrentTime).and_raise(RuntimeError,'MockConnectionError') - end - - it 'should not increment the connect.open metric' do - # https://github.com/puppetlabs/vmpooler/issues/195 - expect(metrics).to receive(:increment).with('connect.open').exactly(0).times - allow(subject).to receive(:connect_to_vsphere) - - subject.get_connection() - end - - it 'should call connect_to_vsphere to reconnect' do - allow(metrics).to receive(:increment) - expect(subject).to receive(:connect_to_vsphere).with(no_args) - - subject.get_connection() - end - - it 'should return a new connection' do - new_connection = mock_RbVmomi_VIM_Connection(connection_options) - expect(subject).to receive(:connect_to_vsphere).with(no_args).and_return(new_connection) - - result = subject.get_connection() - - expect(result).to be(new_connection) - end - end - end - describe '#connect_to_vsphere' do before(:each) do allow(RbVmomi::VIM).to receive(:connect).and_return(connection) @@ -2828,30 +2779,4 @@ EOT expect(subject.migrate_vm_host(vm_object,host_object)).to eq('RELOCATE_RESULT') end end - - describe '#close' do - context 'no connection has been made' do - before(:each) do - # NOTE - Using instance_variable_set is a code smell of code that is not testable - subject.instance_variable_set("@connection",nil) - end - - it 'should not error' do - pending('https://github.com/puppetlabs/vmpooler/issues/211') - subject.close - end - end - - context 'on an open connection' do - before(:each) do - # NOTE - Using instance_variable_set is a code smell of code that is not testable - subject.instance_variable_set("@connection",connection) - end - - it 'should close the underlying connection object' do - expect(connection).to receive(:close) - subject.close - end - end - end end