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