From d1ae85c8af3df2ad397f929b2fed40bea3a974f4 Mon Sep 17 00:00:00 2001 From: "kirby@puppetlabs.com" Date: Tue, 29 May 2018 14:53:09 -0700 Subject: [PATCH 01/10] Remove propertyCollector from add_disk This commit updates add_disk to remove propertyCollector, which was used to back the find_vmdks method to locate the disk file on datastore and then use its length to name the new disk. Instead, the number of disks on the VM is used to ensure a unique disk resource title. Without this change add_disk can take 10-50x longer due to the propertyCollector method. Additionally, without this change propertyCollector is used in a non threadsafe manner, which may cause stability issues for vsphere provider backends. --- lib/vmpooler/providers/vsphere.rb | 30 +++++------------ spec/unit/providers/vsphere_spec.rb | 51 ----------------------------- 2 files changed, 8 insertions(+), 73 deletions(-) diff --git a/lib/vmpooler/providers/vsphere.rb b/lib/vmpooler/providers/vsphere.rb index cfd087c..d6a429f 100644 --- a/lib/vmpooler/providers/vsphere.rb +++ b/lib/vmpooler/providers/vsphere.rb @@ -465,9 +465,12 @@ module Vmpooler vmdk_datastore = find_datastore(datastore, connection, datacentername) raise("Datastore '#{datastore}' does not exist in datacenter '#{datacentername}'") if vmdk_datastore.nil? - vmdk_file_name = "#{vm['name']}/#{vm['name']}_#{find_vmdks(vm['name'], datastore, connection, datacentername).length + 1}.vmdk" + datacenter = connection.serviceInstance.find_datacenter(datacentername) controller = find_disk_controller(vm) + disk_unit_number = find_disk_unit_number(vm, controller) + disk_count = vm.config.hardware.device.grep(RbVmomi::VIM::VirtualDisk).count + vmdk_file_name = "#{vm['name']}/#{vm['name']}_#{disk_count}.vmdk" vmdk_spec = RbVmomi::VIM::FileBackedVirtualDiskSpec( capacityKb: size.to_i * 1024 * 1024, @@ -478,7 +481,7 @@ module Vmpooler vmdk_backing = RbVmomi::VIM::VirtualDiskFlatVer2BackingInfo( datastore: vmdk_datastore, diskMode: DISK_MODE, - fileName: "[#{vmdk_datastore.name}] #{vmdk_file_name}" + fileName: "[#{datastore}] #{vmdk_file_name}" ) device = RbVmomi::VIM::VirtualDisk( @@ -486,7 +489,7 @@ module Vmpooler capacityInKB: size.to_i * 1024 * 1024, controllerKey: controller.key, key: -1, - unitNumber: find_disk_unit_number(vm, controller) + unitNumber: disk_unit_number ) device_config_spec = RbVmomi::VIM::VirtualDeviceConfigSpec( @@ -499,8 +502,8 @@ module Vmpooler ) connection.serviceContent.virtualDiskManager.CreateVirtualDisk_Task( - datacenter: connection.serviceInstance.find_datacenter(datacentername), - name: "[#{vmdk_datastore.name}] #{vmdk_file_name}", + datacenter: datacenter, + name: "[#{datastore}] #{vmdk_file_name}", spec: vmdk_spec ).wait_for_completion @@ -840,23 +843,6 @@ module Vmpooler vms end - def find_vmdks(vmname, datastore, connection, datacentername) - disks = [] - - vmdk_datastore = find_datastore(datastore, connection, datacentername) - - vm_files = connection.serviceContent.propertyCollector.collectMultiple vmdk_datastore.vm, 'layoutEx.file' - vm_files.keys.each do |f| - vm_files[f]['layoutEx.file'].each do |l| - if l.name =~ /^\[#{vmdk_datastore.name}\] #{vmname}\/#{vmname}_([0-9]+).vmdk/ - disks.push(l) - end - end - end - - disks - end - def get_base_vm_container_from(connection) view_manager = connection.serviceContent.viewManager view_manager.CreateContainerView( diff --git a/spec/unit/providers/vsphere_spec.rb b/spec/unit/providers/vsphere_spec.rb index 39c891e..0cf3035 100644 --- a/spec/unit/providers/vsphere_spec.rb +++ b/spec/unit/providers/vsphere_spec.rb @@ -3435,57 +3435,6 @@ EOT end end - describe '#find_vmdks' do - let(:datastorename) { 'datastore' } - let(:connection_options) {{ - :serviceContent => { - :datacenters => [ - { :name => datacenter_name, :datastores => [datastorename] } - ] - } - }} - - let(:collectMultiple_response) { {} } - - before(:each) do - allow(connection.serviceContent.propertyCollector).to receive(:collectMultiple).and_return(collectMultiple_response) - end - - context 'Searching all files for all VMs on a Datastore' do - # This is fairly fragile mocking - let(:collectMultiple_response) { { - 'FakeVMObject1' => { 'layoutEx.file' => - [ - mock_RbVmomi_VIM_VirtualMachineFileLayoutExFileInfo({ :key => 101, :name => "[#{datastorename}] mock1/mock1_0.vmdk"}) - ]}, - vmname => { 'layoutEx.file' => - [ - # VMDKs which should match - mock_RbVmomi_VIM_VirtualMachineFileLayoutExFileInfo({ :key => 1, :name => "[#{datastorename}] #{vmname}/#{vmname}_0.vmdk"}), - mock_RbVmomi_VIM_VirtualMachineFileLayoutExFileInfo({ :key => 2, :name => "[#{datastorename}] #{vmname}/#{vmname}_1.vmdk"}), - # VMDKs which should not match - mock_RbVmomi_VIM_VirtualMachineFileLayoutExFileInfo({ :key => 102, :name => "[otherdatastore] #{vmname}/#{vmname}_0.vmdk"}), - mock_RbVmomi_VIM_VirtualMachineFileLayoutExFileInfo({ :key => 103, :name => "[otherdatastore] #{vmname}/#{vmname}.vmdk"}), - mock_RbVmomi_VIM_VirtualMachineFileLayoutExFileInfo({ :key => 104, :name => "[otherdatastore] #{vmname}/#{vmname}_abc.vmdk"}), - ]}, - } } - - it 'should return empty array if no VMDKs match the VM name' do - expect(subject.find_vmdks('missing_vm_name',datastorename,connection,datacenter_name)).to eq([]) - end - - it 'should return matching VMDKs for the VM' do - result = subject.find_vmdks(vmname,datastorename,connection,datacenter_name) - expect(result).to_not be_nil - expect(result.count).to eq(2) - # The keys for each VMDK should be less that 100 as per the mocks - result.each do |fileinfo| - expect(fileinfo.key).to be < 100 - end - end - end - end - describe '#get_base_vm_container_from' do it 'should return a recursive view of type VirtualMachine' do result = subject.get_base_vm_container_from(connection) From af8b73b6c946c6028072fb2e409e0b6a4062a9af Mon Sep 17 00:00:00 2001 From: "kirby@puppetlabs.com" Date: Fri, 8 Jun 2018 12:40:03 -0700 Subject: [PATCH 02/10] Change default vsphere connection behavior This commit changes the vsphere connection behavior to set insecure false. Without this change insecure is always set to true when making a connection regardless of the setting provided with the provider configuration. --- lib/vmpooler/providers/vsphere.rb | 2 +- vmpooler.yaml.example | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/vmpooler/providers/vsphere.rb b/lib/vmpooler/providers/vsphere.rb index cfd087c..aad3358 100644 --- a/lib/vmpooler/providers/vsphere.rb +++ b/lib/vmpooler/providers/vsphere.rb @@ -410,7 +410,7 @@ module Vmpooler connection = RbVmomi::VIM.connect host: provider_config['server'], user: provider_config['username'], password: provider_config['password'], - insecure: provider_config['insecure'] || true + insecure: provider_config['insecure'] || false metrics.increment('connect.open') return connection rescue => err diff --git a/vmpooler.yaml.example b/vmpooler.yaml.example index ad7eb61..206a814 100644 --- a/vmpooler.yaml.example +++ b/vmpooler.yaml.example @@ -52,7 +52,7 @@ # # - insecure # Whether to ignore any HTTPS negotiation errors (e.g. untrusted self-signed certificates) -# (optional: default true) +# (optional: default false) # # - datacenter # The datacenter within vCenter to manage VMs. This can be overridden in the pool configuration From 9bb4df7d8eb71f926f1494c70650fa907cdb55ea Mon Sep 17 00:00:00 2001 From: "kirby@puppetlabs.com" Date: Fri, 11 May 2018 13:42:21 -0700 Subject: [PATCH 03/10] (POOLER-107) Add configuration API endpoint This commit adds a configuration endpoint to the vmpooler API. Pool size, and pool template, can be adjusted for pools that are configured at vmpooler application start time. Pool template changes trigger a pool refresh, and the new template has delta disks created automatically by vmpooler. Additionally, the capability to create template delta disks is added to the vsphere provider, and this is implemented to ensure that templates have delta disks created at application start time. The mechanism used to find template VM objects is simplified to make the flow of logic easier to understand. As an additional benefit, performance of this lookup is improved by using FindByInventoryPath. A table of contents is added to API.md to ease navigation. Without this change API.md has no table of contents and is difficult to navigate. Add mutex object for managing pool configuration updates This commit adds a mutex object for ensuring that pool configuration changes are synchronized across multiple running threads, removing the possibility of two threads attempting to update something at once, without relying on redis data. Without this change this is managed crudely by specifying in redis that a configuration update is taking place. This redis data is left so the REPOPULATE section of _check_pool can still identify when a configuration change is in progress, and prevent a pool from repopulating at that time. Add wake up event for pool template changes This commit adds a wake up event to detect pool template changes. Additionally, GET /config has a template_ready section added to the output for each pool, which makes clear when a pool is ready to populate itself. --- docs/API.md | 115 +++++- lib/vmpooler/api/helpers.rb | 23 ++ lib/vmpooler/api/v1.rb | 183 ++++++++++ lib/vmpooler/pool_manager.rb | 182 ++++++++-- lib/vmpooler/providers/base.rb | 8 + lib/vmpooler/providers/vsphere.rb | 44 ++- spec/integration/api/v1/config_spec.rb | 250 +++++++++++++ spec/unit/api/helpers_spec.rb | 70 ++++ spec/unit/pool_manager_spec.rb | 469 ++++++++++++++++++++++++- spec/unit/providers/vsphere_spec.rb | 89 ++++- vmpooler.yaml.example | 6 + 11 files changed, 1393 insertions(+), 46 deletions(-) create mode 100644 spec/integration/api/v1/config_spec.rb diff --git a/docs/API.md b/docs/API.md index bbab501..5d1d1c1 100644 --- a/docs/API.md +++ b/docs/API.md @@ -1,8 +1,17 @@ -### API +# Table of contents +1. [API](#API) +2. [Token operations](#token) +3. [VM operations](#vmops) +4. [Add disks](#adddisks) +5. [VM snapshots](#vmsnapshots) +6. [Status and metrics](#statusmetrics) +7. [Pool configuration](#poolconfig) + +### API vmpooler provides a REST API for VM management. The following examples use `curl` for communication. -#### Token operations +#### Token operations Token-based authentication can be used when requesting or modifying VMs. The `/token` route can be used to create, query, or delete tokens. See the provided YAML configuration example, [vmpooler.yaml.example](vmpooler.yaml.example), for information on configuring an authentication store to use when performing token operations. @@ -76,7 +85,7 @@ Enter host password for user 'jdoe': } ``` -#### VM operations +#### VM operations ##### GET /vm @@ -230,7 +239,7 @@ $ curl -X DELETE --url vmpooler.company.com/api/v1/vm/fq6qlpjlsskycq6 } ``` -#### Adding additional disk(s) +#### Adding additional disk(s) ##### POST /vm/<hostname>/disk/<size> @@ -270,7 +279,7 @@ $ curl --url vmpooler.company.com/api/v1/vm/fq6qlpjlsskycq6 ```` -#### VM snapshots +#### VM snapshots ##### POST /vm/<hostname>/snapshot @@ -322,7 +331,7 @@ $ curl X POST -H X-AUTH-TOKEN:a9znth9dn01t416hrguu56ze37t790bl --url vmpooler.co } ```` -#### Status and metrics +#### Status and metrics ##### GET /status @@ -540,3 +549,97 @@ $ curl -G -d 'from=2015-03-10' -d 'to=2015-03-11' --url vmpooler.company.com/api ] } ``` + +#### Managing pool configuration via API + +##### GET /config + +Returns the running pool configuration + +Responses: +* 200 - OK +* 404 - No configuration found +``` +$ curl https://vmpooler.company.com/api/v1/config +``` +```json +{ + "pool_configuration": [ + { + "name": "redhat-7-x86_64", + "template": "templates/redhat-7.2-x86_64-0.0.3", + "folder": "vmpooler/redhat-7-x86_64", + "datastore": "stor1", + "size": 1, + "datacenter": "dc1", + "provider": "vsphere", + "capacity": 1, + "major": "redhat", + "template_ready": true + } + ], + "status": { + "ok": true + } +} +``` + +Note: to enable poolsize and pooltemplate config endpoints it is necessary to set 'experimental_features: true' in your vmpooler configuration. A 405 is returned when you attempt to interact with these endpoints when this configuration option is not set. + +##### POST /config/poolsize + +Change pool size without having to restart the service. + +All pool template changes requested must be for pools that exist in the vmpooler configuration running, or a 404 code will be returned + +When a pool size is changed due to the configuration posted a 201 status will be returned. When the pool configuration is valid, but will not result in any changes, 200 is returned. + +Pool size configuration changes persist through application restarts, and take precedence over a pool size value configured in the pool configuration provided when the application starts. This persistence is dependent on redis. So, if the redis data is lost then the configuration updates revert to those provided at startup at the next application start. + +An authentication token is required in order to change pool configuration when authentication is configured. +Responses: +* 200 - No changes required +* 201 - Changes made on at least one pool with changes requested +* 400 - An invalid configuration was provided causing requested changes to fail +* 404 - An unknown error occurred +* 405 - The endpoint is disabled because experimental features are disabled +``` +$ curl -X POST -H "Content-Type: application/json" -d '{"debian-7-i386":"2","debian-7-x86_64":"1"}' --url https://vmpooler.company.com/api/v1/config/poolsize +``` +```json +{ + "ok": true +} +``` + +##### POST /config/pooltemplate + +Change the template configured for a pool, and replenish the pool with instances built from the new template. + +All pool template changes requested must be for pools that exist in the vmpooler configuration running, or a 404 code will be returned + +When a pool template is changed due to the configuration posted a 201 status will be returned. When the pool configuration is valid, but will not result in any changes, 200 is returned. + +A pool template being updated will cause the following actions, which are logged in vmpooler.log: +* Destroy all instances for the pool template being updated that are in the ready and pending state +* Halt repopulating the pool while creating template deltas for the newly configured template +* Unblock pool population and let the pool replenish with instances based on the newly configured template + +Pool template changes persist through application restarts, and take precedence over a pool template configured in the pool configuration provided when the application starts. This persistence is dependent on redis. As a result, if the redis data is lost then the configuration values revert to those provided at startup at the next application start. + +An authentication token is required in order to change pool configuration when authentication is configured. + +Responses: +* 200 - No changes required +* 201 - Changes made on at least one pool with changes requested +* 400 - An invalid configuration was provided causing requested changes to fail +* 404 - An unknown error occurred +* 405 - The endpoint is disabled because experimental features are disabled +``` +$ curl -X POST -H "Content-Type: application/json" -d '{"debian-7-i386":"templates/debian-7-i386"}' --url https://vmpooler.company.com/api/v1/config/pooltemplate +``` +```json +{ + "ok": true +} +``` diff --git a/lib/vmpooler/api/helpers.rb b/lib/vmpooler/api/helpers.rb index 0bd6275..f76cbce 100644 --- a/lib/vmpooler/api/helpers.rb +++ b/lib/vmpooler/api/helpers.rb @@ -378,6 +378,29 @@ module Vmpooler result end + def pool_index(pools) + pools_hash = {} + index = 0 + for pool in pools + pools_hash[pool['name']] = index + index += 1 + end + pools_hash + end + + def template_ready?(pool, backend) + prepared_template = backend.hget('vmpooler__template__prepared', pool['name']) + return false if prepared_template.nil? + return true if pool['template'] == prepared_template + return false + end + + def is_integer?(x) + Integer(x) + true + rescue + false + end end end end diff --git a/lib/vmpooler/api/v1.rb b/lib/vmpooler/api/v1.rb index 1738558..29d52a6 100644 --- a/lib/vmpooler/api/v1.rb +++ b/lib/vmpooler/api/v1.rb @@ -120,6 +120,74 @@ module Vmpooler result end + def update_pool_size(payload) + result = { 'ok' => false } + + pool_index = pool_index(pools) + pools_updated = 0 + sync_pool_sizes + + payload.each do |poolname, size| + unless pools[pool_index[poolname]]['size'] == size.to_i + pools[pool_index[poolname]]['size'] = size.to_i + backend.hset('vmpooler__config__poolsize', poolname, size) + pools_updated += 1 + status 201 + end + end + status 200 unless pools_updated > 0 + result['ok'] = true + result + end + + def update_pool_template(payload) + result = { 'ok' => false } + + pool_index = pool_index(pools) + pools_updated = 0 + sync_pool_templates + + payload.each do |poolname, template| + unless pools[pool_index[poolname]]['template'] == template + pools[pool_index[poolname]]['template'] = template + backend.hset('vmpooler__config__template', poolname, template) + pools_updated += 1 + status 201 + end + end + status 200 unless pools_updated > 0 + result['ok'] = true + result + end + + def sync_pool_templates + pool_index = pool_index(pools) + template_configs = backend.hgetall('vmpooler__config__template') + unless template_configs.nil? + template_configs.each do |poolname, template| + if pool_index.include? poolname + unless pools[pool_index[poolname]]['template'] == template + pools[pool_index[poolname]]['template'] = template + end + end + end + end + end + + def sync_pool_sizes + pool_index = pool_index(pools) + poolsize_configs = backend.hgetall('vmpooler__config__poolsize') + unless poolsize_configs.nil? + poolsize_configs.each do |poolname, size| + if pool_index.include? poolname + unless pools[pool_index[poolname]]['size'] == size.to_i + pools[pool_index[poolname]]['size'] == size.to_i + end + end + end + end + end + # Provide run-time statistics # # Example: @@ -196,6 +264,8 @@ module Vmpooler } } + sync_pool_sizes + result[:capacity] = get_capacity_metrics(pools, backend) unless views and not views.include?("capacity") result[:queue] = get_queue_metrics(pools, backend) unless views and not views.include?("queue") result[:clone] = get_task_metrics(backend, 'clone', Date.today.to_s) unless views and not views.include?("clone") @@ -502,6 +572,30 @@ module Vmpooler invalid end + def invalid_template_or_size(payload) + invalid = [] + payload.each do |pool, size| + invalid << pool unless pool_exists?(pool) + unless is_integer?(size) + invalid << pool + next + end + invalid << pool unless Integer(size) >= 0 + end + invalid + end + + def invalid_template_or_path(payload) + invalid = [] + payload.each do |pool, template| + invalid << pool unless pool_exists?(pool) + invalid << pool unless template.include? '/' + invalid << pool if template[0] == '/' + invalid << pool if template[-1] == '/' + end + invalid + end + post "#{api_prefix}/vm/:template/?" do content_type :json result = { 'ok' => false } @@ -747,6 +841,95 @@ module Vmpooler JSON.pretty_generate(result) end + + post "#{api_prefix}/config/poolsize/?" do + content_type :json + result = { 'ok' => false } + + if config['experimental_features'] + need_token! if Vmpooler::API.settings.config[:auth] + + payload = JSON.parse(request.body.read) + + if payload + invalid = invalid_template_or_size(payload) + if invalid.empty? + result = update_pool_size(payload) + else + invalid.each do |bad_template| + metrics.increment("config.invalid.#{bad_template}") + end + result[:bad_templates] = invalid + status 400 + end + else + metrics.increment('config.invalid.unknown') + status 404 + end + else + status 405 + end + + JSON.pretty_generate(result) + end + + post "#{api_prefix}/config/pooltemplate/?" do + content_type :json + result = { 'ok' => false } + + if config['experimental_features'] + need_token! if Vmpooler::API.settings.config[:auth] + + payload = JSON.parse(request.body.read) + + if payload + invalid = invalid_template_or_path(payload) + if invalid.empty? + result = update_pool_template(payload) + else + invalid.each do |bad_template| + metrics.increment("config.invalid.#{bad_template}") + end + result[:bad_templates] = invalid + status 400 + end + else + metrics.increment('config.invalid.unknown') + status 404 + end + else + status 405 + end + + JSON.pretty_generate(result) + end + + get "#{api_prefix}/config/?" do + content_type :json + result = { 'ok' => false } + status 404 + + if pools + sync_pool_sizes + sync_pool_templates + + pool_configuration = [] + pools.each do |pool| + pool['template_ready'] = template_ready?(pool, backend) + pool_configuration << pool + end + + result = { + pool_configuration: pool_configuration, + status: { + ok: true + } + } + + status 200 + end + JSON.pretty_generate(result) + end end end end diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb index d9fe40d..866cf36 100644 --- a/lib/vmpooler/pool_manager.rb +++ b/lib/vmpooler/pool_manager.rb @@ -21,6 +21,9 @@ module Vmpooler # Our thread-tracker object $threads = {} + + # Pool mutex + @reconfigure_pool = {} end def config @@ -187,9 +190,9 @@ module Vmpooler end end - def move_vm_queue(pool, vm, queue_from, queue_to, msg) + def move_vm_queue(pool, vm, queue_from, queue_to, msg = nil) $redis.smove("vmpooler__#{queue_from}__#{pool}", "vmpooler__#{queue_to}__#{pool}", vm) - $logger.log('d', "[!] [#{pool}] '#{vm}' #{msg}") + $logger.log('d', "[!] [#{pool}] '#{vm}' #{msg}") if msg end # Clone a VM @@ -482,6 +485,10 @@ module Vmpooler # - Fires when the number of ready VMs changes due to being consumed. # - Additional options # :poolname + # :pool_template_change + # - Fires when a template configuration update is requested + # - Additional options + # :poolname # def sleep_with_wakeup_events(loop_delay, wakeup_period = 5, options = {}) exit_by = Time.now + loop_delay @@ -492,6 +499,10 @@ module Vmpooler initial_ready_size = $redis.scard("vmpooler__ready__#{options[:poolname]}") end + if options[:pool_template_change] + initial_template = $redis.hget('vmpooler__template__prepared', options[:poolname]) + end + loop do sleep(1) break if time_passed?(:exit_by, exit_by) @@ -505,6 +516,14 @@ module Vmpooler ready_size = $redis.scard("vmpooler__ready__#{options[:poolname]}") break unless ready_size == initial_ready_size end + + if options[:pool_template_change] + configured_template = $redis.hget('vmpooler__config__template', options[:poolname]) + if configured_template + break unless initial_template == configured_template + end + end + end break if time_passed?(:exit_by, exit_by) @@ -532,6 +551,7 @@ module Vmpooler loop_delay = loop_delay_min provider = get_provider_for_pool(pool['name']) raise("Could not find provider '#{pool['provider']}") if provider.nil? + sync_pool_template(pool) loop do result = _check_pool(pool, provider) @@ -541,7 +561,7 @@ module Vmpooler loop_delay = (loop_delay * loop_delay_decay).to_i loop_delay = loop_delay_max if loop_delay > loop_delay_max end - sleep_with_wakeup_events(loop_delay, loop_delay_min, pool_size_change: true, poolname: pool['name']) + sleep_with_wakeup_events(loop_delay, loop_delay_min, pool_size_change: true, poolname: pool['name'], pool_template_change: true) unless maxloop.zero? break if loop_count >= maxloop @@ -555,6 +575,101 @@ module Vmpooler end end + def pool_mutex(poolname) + @reconfigure_pool[poolname] || @reconfigure_pool[poolname] = Mutex.new + end + + def sync_pool_template(pool) + pool_template = $redis.hget('vmpooler__config__template', pool['name']) + if pool_template + unless pool['template'] == pool_template + pool['template'] = pool_template + end + end + end + + def prepare_template(pool, provider) + provider.create_template_delta_disks(pool) if $config[:config]['create_template_delta_disks'] + $redis.hset('vmpooler__template__prepared', pool['name'], pool['template']) + end + + def evaluate_template(pool, provider) + mutex = pool_mutex(pool['name']) + prepared_template = $redis.hget('vmpooler__template__prepared', pool['name']) + configured_template = $redis.hget('vmpooler__config__template', pool['name']) + return if mutex.locked? + if prepared_template.nil? + mutex.synchronize do + prepare_template(pool, provider) + prepared_template = $redis.hget('vmpooler__template__prepared', pool['name']) + end + end + return if configured_template.nil? + return if configured_template == prepared_template + mutex.synchronize do + update_pool_template(pool, provider, configured_template, prepared_template) + end + end + + def drain_pool(poolname) + # Clear a pool of ready and pending instances + if $redis.scard("vmpooler__ready__#{poolname}") > 0 + $logger.log('s', "[*] [#{poolname}] removing ready instances") + $redis.smembers("vmpooler__ready__#{poolname}").each do |vm| + move_vm_queue(poolname, vm, 'ready', 'completed') + end + end + if $redis.scard("vmpooler__pending__#{poolname}") > 0 + $logger.log('s', "[*] [#{poolname}] removing pending instances") + $redis.smembers("vmpooler__pending__#{poolname}").each do |vm| + move_vm_queue(poolname, vm, 'pending', 'completed') + end + end + end + + def update_pool_template(pool, provider, configured_template, prepared_template) + pool['template'] = configured_template + $logger.log('s', "[*] [#{pool['name']}] template updated from #{prepared_template} to #{configured_template}") + # Remove all ready and pending VMs so new instances are created from the new template + drain_pool(pool['name']) + # Prepare template for deployment + $logger.log('s', "[*] [#{pool['name']}] preparing pool template for deployment") + prepare_template(pool, provider) + $logger.log('s', "[*] [#{pool['name']}] is ready for use") + end + + def remove_excess_vms(pool, provider, ready, total) + return if total.nil? + return if total == 0 + mutex = pool_mutex(pool['name']) + return if mutex.locked? + return unless ready > pool['size'] + mutex.synchronize do + difference = ready - pool['size'] + difference.times do + next_vm = $redis.spop("vmpooler__ready__#{pool['name']}") + move_vm_queue(pool['name'], next_vm, 'ready', 'completed') + end + if total > ready + $redis.smembers("vmpooler__pending__#{pool['name']}").each do |vm| + move_vm_queue(pool['name'], vm, 'pending', 'completed') + end + end + end + end + + def update_pool_size(pool) + mutex = pool_mutex(pool['name']) + return if mutex.locked? + poolsize = $redis.hget('vmpooler__config__poolsize', pool['name']) + return if poolsize.nil? + poolsize = Integer(poolsize) + return if poolsize == pool['size'] + mutex.synchronize do + pool['size'] = poolsize + end + end + def _check_pool(pool, provider) pool_check_response = { discovered_vms: 0, @@ -683,36 +798,53 @@ module Vmpooler end end + # UPDATE TEMPLATE + # Evaluates a pool template to ensure templates are prepared adequately for the configured provider + # If a pool template configuration change is detected then template preparation is repeated for the new template + # Additionally, a pool will drain ready and pending instances + evaluate_template(pool, provider) + # REPOPULATE - ready = $redis.scard("vmpooler__ready__#{pool['name']}") - total = $redis.scard("vmpooler__pending__#{pool['name']}") + ready + # Do not attempt to repopulate a pool while a template is updating + unless pool_mutex(pool['name']).locked? + ready = $redis.scard("vmpooler__ready__#{pool['name']}") + total = $redis.scard("vmpooler__pending__#{pool['name']}") + ready - $metrics.gauge("ready.#{pool['name']}", $redis.scard("vmpooler__ready__#{pool['name']}")) - $metrics.gauge("running.#{pool['name']}", $redis.scard("vmpooler__running__#{pool['name']}")) + $metrics.gauge("ready.#{pool['name']}", $redis.scard("vmpooler__ready__#{pool['name']}")) + $metrics.gauge("running.#{pool['name']}", $redis.scard("vmpooler__running__#{pool['name']}")) - if $redis.get("vmpooler__empty__#{pool['name']}") - $redis.del("vmpooler__empty__#{pool['name']}") unless ready.zero? - elsif ready.zero? - $redis.set("vmpooler__empty__#{pool['name']}", 'true') - $logger.log('s', "[!] [#{pool['name']}] is empty") - end + if $redis.get("vmpooler__empty__#{pool['name']}") + $redis.del("vmpooler__empty__#{pool['name']}") unless ready.zero? + elsif ready.zero? + $redis.set("vmpooler__empty__#{pool['name']}", 'true') + $logger.log('s', "[!] [#{pool['name']}] is empty") + end - if total < pool['size'] - (1..(pool['size'] - total)).each do |_i| - if $redis.get('vmpooler__tasks__clone').to_i < $config[:config]['task_limit'].to_i - begin - $redis.incr('vmpooler__tasks__clone') - pool_check_response[:cloned_vms] += 1 - clone_vm(pool, provider) - rescue => err - $logger.log('s', "[!] [#{pool['name']}] clone failed during check_pool with an error: #{err}") - $redis.decr('vmpooler__tasks__clone') - raise + # Check to see if a pool size change has been made via the configuration API + # Since check_pool runs in a loop it does not + # otherwise identify this change when running + update_pool_size(pool) + + if total < pool['size'] + (1..(pool['size'] - total)).each do |_i| + if $redis.get('vmpooler__tasks__clone').to_i < $config[:config]['task_limit'].to_i + begin + $redis.incr('vmpooler__tasks__clone') + pool_check_response[:cloned_vms] += 1 + clone_vm(pool, provider) + rescue => err + $logger.log('s', "[!] [#{pool['name']}] clone failed during check_pool with an error: #{err}") + $redis.decr('vmpooler__tasks__clone') + raise + end end end end end + # Remove VMs in excess of the configured pool size + remove_excess_vms(pool, provider, ready, total) + pool_check_response end @@ -739,6 +871,8 @@ module Vmpooler $redis.set('vmpooler__tasks__clone', 0) # Clear out vmpooler__migrations since stale entries may be left after a restart $redis.del('vmpooler__migration') + # Ensure template deltas are created on each startup + $redis.del('vmpooler__template__prepared') # Copy vSphere settings to correct location. This happens with older configuration files if !$config[:vsphere].nil? && ($config[:providers].nil? || $config[:providers][:vsphere].nil?) diff --git a/lib/vmpooler/providers/base.rb b/lib/vmpooler/providers/base.rb index a33e07d..71b281d 100644 --- a/lib/vmpooler/providers/base.rb +++ b/lib/vmpooler/providers/base.rb @@ -217,6 +217,14 @@ module Vmpooler def vm_exists?(pool_name, vm_name) !get_vm(pool_name, vm_name).nil? end + + # inputs + # [Hash] pool : Configuration for the pool + # returns + # nil when successful. Raises error when encountered + def create_template_delta_disks(pool) + raise("#{self.class.name} does not implement create_template_delta_disks") + end end end end diff --git a/lib/vmpooler/providers/vsphere.rb b/lib/vmpooler/providers/vsphere.rb index 6e7721a..acb38c5 100644 --- a/lib/vmpooler/providers/vsphere.rb +++ b/lib/vmpooler/providers/vsphere.rb @@ -197,17 +197,10 @@ module Vmpooler target_cluster_name = get_target_cluster_from_config(pool_name) target_datacenter_name = get_target_datacenter_from_config(pool_name) - # Extract the template VM name from the full path - raise("Pool #{pool_name} did not specify a full path for the template for the provider #{name}") unless template_path =~ /\// - templatefolders = template_path.split('/') - template_name = templatefolders.pop + # Get the template VM object + raise("Pool #{pool_name} did not specify a full path for the template for the provider #{name}") unless valid_template_path? template_path - # Get the actual objects from vSphere - template_folder_object = find_folder(templatefolders.join('/'), connection, target_datacenter_name) - raise("Pool #{pool_name} specifies a template folder of #{templatefolders.join('/')} which does not exist for the provider #{name}") if template_folder_object.nil? - - 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? + template_vm_object = find_template_vm(pool, connection) # Annotate with creation time, origin template, etc. # Add extraconfig options that can be queried by vmtools @@ -933,6 +926,37 @@ module Vmpooler raise("Cannot create folder #{new_folder}") if folder_object.nil? folder_object end + + def find_template_vm(pool, connection) + datacenter = get_target_datacenter_from_config(pool['name']) + raise('cannot find datacenter') if datacenter.nil? + + propSpecs = { + :entity => self, + :inventoryPath => "#{datacenter}/vm/#{pool['template']}" + } + + template_vm_object = connection.searchIndex.FindByInventoryPath(propSpecs) + raise("Pool #{pool['name']} specifies a template VM of #{pool['template']} which does not exist for the provider #{name}") if template_vm_object.nil? + + template_vm_object + end + + def create_template_delta_disks(pool) + @connection_pool.with_metrics do |pool_object| + connection = ensured_vsphere_connection(pool_object) + template_vm_object = find_template_vm(pool, connection) + + template_vm_object.add_delta_disk_layer_on_all_disks + end + end + + def valid_template_path?(template) + return false unless template.include?('/') + return false if template[0] == '/' + return false if template[-1] == '/' + return true + end end end end diff --git a/spec/integration/api/v1/config_spec.rb b/spec/integration/api/v1/config_spec.rb new file mode 100644 index 0000000..abcfdb1 --- /dev/null +++ b/spec/integration/api/v1/config_spec.rb @@ -0,0 +1,250 @@ +require 'spec_helper' +require 'rack/test' + +module Vmpooler + class API + module Helpers + def authenticate(auth, username_str, password_str) + username_str == 'admin' and password_str == 's3cr3t' + end + end + end +end + +describe Vmpooler::API::V1 do + include Rack::Test::Methods + + def app() + Vmpooler::API + end + + let(:config) { + { + config: { + 'site_name' => 'test pooler', + 'vm_lifetime_auth' => 2, + 'experimental_features' => true + }, + pools: [ + {'name' => 'pool1', 'size' => 5, 'template' => 'templates/pool1'}, + {'name' => 'pool2', 'size' => 10} + ], + statsd: { 'prefix' => 'stats_prefix'}, + alias: { 'poolone' => 'pool1' }, + pool_names: [ 'pool1', 'pool2', 'poolone' ] + } + } + + describe '/config/pooltemplate' do + let(:prefix) { '/api/v1' } + let(:metrics) { Vmpooler::DummyStatsd.new } + + let(:current_time) { Time.now } + + before(:each) do + redis.flushdb + + app.settings.set :config, config + app.settings.set :redis, redis + app.settings.set :metrics, metrics + app.settings.set :config, auth: false + create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time) + end + + describe 'POST /config/pooltemplate' do + it 'updates a pool template' do + post "#{prefix}/config/pooltemplate", '{"pool1":"templates/new_template"}' + expect_json(ok = true, http = 201) + + expected = { ok: true } + + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end + + it 'fails on nonexistent pools' do + post "#{prefix}/config/pooltemplate", '{"poolpoolpool":"templates/newtemplate"}' + expect_json(ok = false, http = 400) + end + + it 'updates multiple pools' do + post "#{prefix}/config/pooltemplate", '{"pool1":"templates/new_template","pool2":"templates/new_template2"}' + expect_json(ok = true, http = 201) + + expected = { ok: true } + + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end + + it 'fails when not all pools exist' do + post "#{prefix}/config/pooltemplate", '{"pool1":"templates/new_template","pool3":"templates/new_template2"}' + expect_json(ok = false, http = 400) + + expected = { + ok: false, + bad_templates: ['pool3'] + } + + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end + + it 'returns no changes when the template does not change' do + post "#{prefix}/config/pooltemplate", '{"pool1":"templates/pool1"}' + expect_json(ok = true, http = 200) + + expected = { ok: true } + + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end + + it 'fails when a invalid template parameter is provided' do + post "#{prefix}/config/pooltemplate", '{"pool1":"template1"}' + expect_json(ok = false, http = 400) + + expected = { + ok: false, + bad_templates: ['pool1'] + } + + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end + + it 'fails when a template starts with /' do + post "#{prefix}/config/pooltemplate", '{"pool1":"/template1"}' + expect_json(ok = false, http = 400) + + expected = { + ok: false, + bad_templates: ['pool1'] + } + + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end + + it 'fails when a template ends with /' do + post "#{prefix}/config/pooltemplate", '{"pool1":"template1/"}' + expect_json(ok = false, http = 400) + + expected = { + ok: false, + bad_templates: ['pool1'] + } + + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end + + context 'with experimental features disabled' do + before(:each) do + config[:config]['experimental_features'] = false + end + + it 'should return 405' do + post "#{prefix}/config/pooltemplate", '{"pool1":"template/template1"}' + expect_json(ok = false, http = 405) + + expected = { ok: false } + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end + end + + end + + describe 'POST /config/poolsize' do + it 'changes a pool size' do + post "#{prefix}/config/poolsize", '{"pool1":"2"}' + expect_json(ok = true, http = 201) + + expected = { ok: true } + + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end + + it 'changes a pool size for multiple pools' do + post "#{prefix}/config/poolsize", '{"pool1":"2","pool2":"2"}' + expect_json(ok = true, http = 201) + + expected = { ok: true } + + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end + + it 'fails when a specified pool does not exist' do + post "#{prefix}/config/poolsize", '{"pool10":"2"}' + expect_json(ok = false, http = 400) + expected = { + ok: false, + bad_templates: ['pool10'] + } + + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end + + it 'succeeds with 200 when no change is required' do + post "#{prefix}/config/poolsize", '{"pool1":"5"}' + expect_json(ok = true, http = 200) + + expected = { ok: true } + + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end + + it 'succeeds with 201 when at least one pool changes' do + post "#{prefix}/config/poolsize", '{"pool1":"5","pool2":"5"}' + expect_json(ok = true, http = 201) + + expected = { ok: true } + + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end + + it 'fails when a non-integer value is provided for size' do + post "#{prefix}/config/poolsize", '{"pool1":"four"}' + expect_json(ok = false, http = 400) + + expected = { + ok: false, + bad_templates: ['pool1'] + } + + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end + + it 'fails when a negative value is provided for size' do + post "#{prefix}/config/poolsize", '{"pool1":"-1"}' + expect_json(ok = false, http = 400) + + expected = { + ok: false, + bad_templates: ['pool1'] + } + + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end + + context 'with experimental features disabled' do + before(:each) do + config[:config]['experimental_features'] = false + end + + it 'should return 405' do + post "#{prefix}/config/poolsize", '{"pool1":"1"}' + expect_json(ok = false, http = 405) + + expected = { ok: false } + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end + end + end + + describe 'GET /config' do + let(:prefix) { '/api/v1' } + + it 'returns pool configuration when set' do + get "#{prefix}/config" + + expect(last_response.header['Content-Type']).to eq('application/json') + result = JSON.parse(last_response.body) + expect(result['pool_configuration']).to eq(config[:pools]) + end + end + end +end diff --git a/spec/unit/api/helpers_spec.rb b/spec/unit/api/helpers_spec.rb index a24dfae..7e75045 100644 --- a/spec/unit/api/helpers_spec.rb +++ b/spec/unit/api/helpers_spec.rb @@ -168,4 +168,74 @@ describe Vmpooler::API::Helpers do end end + describe '#pool_index' do + let(:pools) { + [ + { + 'name' => 'pool1' + }, + { + 'name' => 'pool2' + } + ] + } + + it 'should return a hash' do + pools_hash = subject.pool_index(pools) + + expect(pools_hash).to be_a(Hash) + end + + it 'should return the correct index for each pool' do + pools_hash = subject.pool_index(pools) + + expect(pools[pools_hash['pool1']]['name']).to eq('pool1') + expect(pools[pools_hash['pool2']]['name']).to eq('pool2') + end + end + + describe '#template_ready?' do + let(:redis) { double('redis') } + let(:template) { 'template/test1' } + let(:poolname) { 'pool1' } + let(:pool) { + { + 'name' => poolname, + 'template' => template + } + } + + it 'returns false when there is no prepared template' do + expect(redis).to receive(:hget).with('vmpooler__template__prepared', poolname).and_return(nil) + + expect(subject.template_ready?(pool, redis)).to be false + end + + it 'returns true when configured and prepared templates match' do + expect(redis).to receive(:hget).with('vmpooler__template__prepared', poolname).and_return(template) + + expect(subject.template_ready?(pool, redis)).to be true + end + + it 'returns false when configured and prepared templates do not match' do + expect(redis).to receive(:hget).with('vmpooler__template__prepared', poolname).and_return('template3') + + expect(subject.template_ready?(pool, redis)).to be false + end + end + + describe '#is_integer?' do + it 'returns true when input is an integer' do + expect(subject.is_integer? 4).to be true + end + + it 'returns true when input is a string containing an integer' do + expect(subject.is_integer? '4').to be true + end + + it 'returns false when input is a string containing word characters' do + expect(subject.is_integer? 'four').to be false + end + end + end diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb index 1073e0e..19f6c3f 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/spec/unit/pool_manager_spec.rb @@ -1503,6 +1503,396 @@ EOT end end + describe 'sync_pool_template' do + let(:old_template) { 'templates/old-template' } + let(:new_template) { 'templates/new-template' } + let(:config) { YAML.load(<<-EOT +--- +:pools: + - name: '#{pool}' + size: 1 + template: old_template +EOT + ) + } + + it 'returns when a template is not set in redis' do + expect(subject.sync_pool_template(config[:pools][0])).to be_nil + end + + it 'returns when a template is set and matches the configured template' do + redis.hset('vmpooler__config__template', pool, old_template) + + subject.sync_pool_template(config[:pools][0]) + + expect(config[:pools][0]['template']).to eq(old_template) + end + + it 'updates a pool template when the redis provided value is different' do + redis.hset('vmpooler__config__template', pool, new_template) + + subject.sync_pool_template(config[:pools][0]) + + expect(config[:pools][0]['template']).to eq(new_template) + end + end + + describe 'pool_mutex' do + it 'should return a mutex' do + expect(subject.pool_mutex(pool)).to be_a(Mutex) + end + + it 'should return the same mutex when called twice' do + first = subject.pool_mutex(pool) + second = subject.pool_mutex(pool) + expect(first).to be(second) + end + end + + describe 'update_pool_template' do + let(:current_template) { 'templates/pool_template' } + let(:new_template) { 'templates/new_pool_template' } + let(:config) { + YAML.load(<<-EOT +--- +:config: {} +:pools: + - name: #{pool} + template: "#{current_template}" +EOT + ) + } + let(:poolconfig) { config[:pools][0] } + + before(:each) do + allow(logger).to receive(:log) + end + + it 'should set the pool template to match the configured template' do + subject.update_pool_template(poolconfig, provider, new_template, current_template) + + expect(poolconfig['template']).to eq(new_template) + end + + it 'should log that the template is updated' do + expect(logger).to receive(:log).with('s', "[*] [#{pool}] template updated from #{current_template} to #{new_template}") + + subject.update_pool_template(poolconfig, provider, new_template, current_template) + end + + it 'should run drain_pool' do + expect(subject).to receive(:drain_pool).with(pool) + + subject.update_pool_template(poolconfig, provider, new_template, current_template) + end + + it 'should log that a template is being prepared' do + expect(logger).to receive(:log).with('s', "[*] [#{pool}] preparing pool template for deployment") + + subject.update_pool_template(poolconfig, provider, new_template, current_template) + end + + it 'should run prepare_template' do + expect(subject).to receive(:prepare_template).with(poolconfig, provider) + + subject.update_pool_template(poolconfig, provider, new_template, current_template) + end + + it 'should log that the pool is ready for use' do + expect(logger).to receive(:log).with('s', "[*] [#{pool}] is ready for use") + + subject.update_pool_template(poolconfig, provider, new_template, current_template) + end + end + + describe 'remove_excess_vms' do + let(:config) { + YAML.load(<<-EOT +--- +:pools: + - name: #{pool} + size: 2 +EOT + ) + } + + before(:each) do + expect(subject).not_to be_nil + end + + context 'with a 0 total value' do + let(:ready) { 0 } + let(:total) { 0 } + it 'should return nil' do + expect(subject.remove_excess_vms(config[:pools][0], provider, ready, total)).to be_nil + end + end + + context 'when the mutex is locked' do + let(:mutex) { Mutex.new } + let(:ready) { 2 } + let(:total) { 3 } + before(:each) do + mutex.lock + expect(subject).to receive(:pool_mutex).with(pool).and_return(mutex) + end + + it 'should return nil' do + expect(subject.remove_excess_vms(config[:pools][0], provider, ready, total)).to be_nil + end + end + + context 'with a total size less than the pool size' do + let(:ready) { 1 } + let(:total) { 2 } + it 'should return nil' do + expect(subject.remove_excess_vms(config[:pools][0], provider, ready, total)).to be_nil + end + end + + context 'with a total size greater than the pool size' do + let(:ready) { 4 } + let(:total) { 4 } + it 'should remove excess ready vms' do + expect(subject).to receive(:move_vm_queue).exactly(2).times + + subject.remove_excess_vms(config[:pools][0], provider, ready, total) + end + + it 'should remove excess pending vms' do + create_pending_vm(pool,'vm1') + create_pending_vm(pool,'vm2') + create_ready_vm(pool, 'vm3') + create_ready_vm(pool, 'vm4') + create_ready_vm(pool, 'vm5') + expect(subject).to receive(:move_vm_queue).exactly(3).times + + subject.remove_excess_vms(config[:pools][0], provider, 3, 5) + end + end + end + + describe 'prepare_template' do + let(:config) { YAML.load(<<-EOT +--- +:config: + create_template_delta_disks: true +:providers: + :mock: +:pools: + - name: '#{pool}' + size: 1 + template: 'templates/pool1' +EOT + ) + } + + context 'when creating the template delta disks' do + before(:each) do + allow(redis).to receive(:hset) + allow(provider).to receive(:create_template_delta_disks) + end + + it 'should run create template delta disks' do + expect(provider).to receive(:create_template_delta_disks).with(config[:pools][0]) + + subject.prepare_template(config[:pools][0], provider) + end + + it 'should mark the template as prepared' do + expect(redis).to receive(:hset).with('vmpooler__template__prepared', pool, config[:pools][0]['template']) + + subject.prepare_template(config[:pools][0], provider) + end + end + end + + describe 'evaluate_template' do + let(:mutex) { Mutex.new } + let(:current_template) { 'templates/template1' } + let(:new_template) { 'templates/template2' } + let(:config) { YAML.load(<<-EOT +--- +:config: + task_limit: 5 +:providers: + :mock: +:pools: + - name: '#{pool}' + size: 1 + template: '#{current_template}' +EOT + ) + } + + before(:each) do + allow(redis).to receive(:hget) + expect(subject).to receive(:pool_mutex).with(pool).and_return(mutex) + end + + it 'should retreive the prepared template' do + expect(redis).to receive(:hget).with('vmpooler__template__prepared', pool).and_return(current_template) + + subject.evaluate_template(config[:pools][0], provider) + end + + it 'should retrieve the redis configured template' do + expect(redis).to receive(:hget).with('vmpooler__config__template', pool).and_return(new_template) + + subject.evaluate_template(config[:pools][0], provider) + end + + context 'when the mutex is locked' do + before(:each) do + mutex.lock + end + + it 'should return' do + expect(subject.evaluate_template(config[:pools][0], provider)).to be_nil + end + end + + context 'when prepared template is nil' do + before(:each) do + expect(redis).to receive(:hget).with('vmpooler__template__prepared', pool).and_return(nil) + end + + it 'should prepare the template' do + expect(subject).to receive(:prepare_template).with(config[:pools][0], provider) + + subject.evaluate_template(config[:pools][0], provider) + end + end + + context 'when a new template is requested' do + before(:each) do + expect(redis).to receive(:hget).with('vmpooler__template__prepared', pool).and_return(current_template) + expect(redis).to receive(:hget).with('vmpooler__config__template', pool).and_return(new_template) + end + + it 'should update the template' do + expect(subject).to receive(:update_pool_template).with(config[:pools][0], provider, new_template, current_template) + + subject.evaluate_template(config[:pools][0], provider) + end + end + end + + describe 'drain_pool' do + before(:each) do + allow(logger).to receive(:log) + end + + context 'with no vms' do + it 'should return nil' do + expect(subject.drain_pool(pool)).to be_nil + end + + it 'should not log any messages' do + expect(logger).to_not receive(:log) + + subject.drain_pool(pool) + end + + it 'should not try to move any vms' do + expect(subject).to_not receive(:move_vm_queue) + + subject.drain_pool(pool) + end + end + + context 'with ready vms' do + before(:each) do + create_ready_vm(pool, 'vm1') + create_ready_vm(pool, 'vm2') + end + + it 'removes the ready instances' do + expect(subject).to receive(:move_vm_queue).twice + + subject.drain_pool(pool) + end + + it 'logs that ready instances are being removed' do + expect(logger).to receive(:log).with('s', "[*] [#{pool}] removing ready instances") + + subject.drain_pool(pool) + end + end + + context 'with pending instances' do + before(:each) do + create_pending_vm(pool, 'vm1') + create_pending_vm(pool, 'vm2') + end + + it 'removes the pending instances' do + expect(subject).to receive(:move_vm_queue).twice + + subject.drain_pool(pool) + end + + it 'logs that pending instances are being removed' do + expect(logger).to receive(:log).with('s', "[*] [#{pool}] removing pending instances") + + subject.drain_pool(pool) + end + end + end + + describe 'update_pool_size' do + let(:newsize) { '3' } + let(:config) { + YAML.load(<<-EOT +--- +:pools: + - name: #{pool} + size: 2 +EOT + ) + } + let(:poolconfig) { config[:pools][0] } + + context 'with a locked mutex' do + + let(:mutex) { Mutex.new } + before(:each) do + mutex.lock + expect(subject).to receive(:pool_mutex).with(pool).and_return(mutex) + end + + it 'should return nil' do + expect(subject.update_pool_size(poolconfig)).to be_nil + end + end + + it 'should get the pool size configuration from redis' do + expect(redis).to receive(:hget).with('vmpooler__config__poolsize', pool) + + subject.update_pool_size(poolconfig) + end + + it 'should return when poolsize is not set in redis' do + expect(redis).to receive(:hget).with('vmpooler__config__poolsize', pool).and_return(nil) + + expect(subject.update_pool_size(poolconfig)).to be_nil + end + + it 'should return when no change in configuration is required' do + expect(redis).to receive(:hget).with('vmpooler__config__poolsize', pool).and_return('2') + + expect(subject.update_pool_size(poolconfig)).to be_nil + end + + it 'should update the pool size' do + expect(redis).to receive(:hget).with('vmpooler__config__poolsize', pool).and_return(newsize) + + subject.update_pool_size(poolconfig) + + expect(poolconfig['size']).to eq(Integer(newsize)) + end + end + describe "#execute!" do let(:config) { YAML.load(<<-EOT @@ -1824,6 +2214,7 @@ EOT it 'should run startup tasks only once' do expect(redis).to receive(:set).with('vmpooler__tasks__clone', 0).once expect(redis).to receive(:del).with('vmpooler__migration').once + expect(redis).to receive(:del).with('vmpooler__template__prepared').once subject.execute!(maxloop,0) end @@ -1902,8 +2293,38 @@ EOT subject.sleep_with_wakeup_events(loop_delay, wakeup_period, wakeup_option) end end + + describe 'with the pool_template_change wakeup option' do + let(:wakeup_option) {{ + :pool_template_change => true, + :poolname => pool + }} + let(:new_template) { 'templates/newtemplate' } + let(:wakeup_period) { -1 } # A negative number forces the wakeup evaluation to always occur + + context 'with a template configured' do + before(:each) do + redis.hset('vmpooler__config__template', pool, new_template) + allow(redis).to receive(:hget) + end + + it 'should check if a template is configured in redis' do + expect(subject).to receive(:time_passed?).with(:exit_by, Time).and_return(false, true) + expect(redis).to receive(:hget).with('vmpooler__template__prepared', pool).once + + subject.sleep_with_wakeup_events(loop_delay, wakeup_period, wakeup_option) + end + + it 'should sleep until a template change is detected' do + expect(subject).to receive(:sleep).exactly(3).times + expect(redis).to receive(:hget).with('vmpooler__config__template', pool).and_return(nil,nil,new_template) + + subject.sleep_with_wakeup_events(loop_delay, wakeup_period, wakeup_option) + end + end + end end - + describe "#check_pool" do let(:threads) {{}} let(:provider_name) { 'mock_provider' } @@ -2785,6 +3206,52 @@ EOT end end + context 'when a pool size configuration change is detected' do + let(:poolsize) { 2 } + let(:newpoolsize) { 3 } + before(:each) do + config[:pools][0]['size'] = poolsize + redis.hset('vmpooler__config__poolsize', pool, newpoolsize) + expect(provider).to receive(:vms_in_pool).with(pool).and_return([]) + end + + it 'should change the pool size configuration' do + subject._check_pool(config[:pools][0],provider) + + expect(config[:pools][0]['size']).to be(newpoolsize) + end + end + + context 'when a pool template is updating' do + let(:poolsize) { 2 } + before(:each) do + redis.hset('vmpooler__config__updating', pool, 1) + expect(provider).to receive(:vms_in_pool).with(pool).and_return([]) + end + + it 'should not call clone_vm to populate the pool' do + expect(subject).to_not receive(:clone_vm) + + subject._check_pool(config[:pools][0],provider) + end + end + + context 'when an excess number of ready vms exist' do + + before(:each) do + allow(redis).to receive(:scard) + expect(redis).to receive(:scard).with("vmpooler__ready__#{pool}").and_return(1) + expect(redis).to receive(:scard).with("vmpooler__pending__#{pool}").and_return(1) + expect(provider).to receive(:vms_in_pool).with(pool).and_return([]) + end + + it 'should call remove_excess_vms' do + expect(subject).to receive(:remove_excess_vms).with(config[:pools][0], provider, 1, 2) + + subject._check_pool(config[:pools][0],provider) + end + end + context 'export metrics' do it 'increments metrics for ready queue' do create_ready_vm(pool,'vm1') diff --git a/spec/unit/providers/vsphere_spec.rb b/spec/unit/providers/vsphere_spec.rb index 1757da9..948a7d4 100644 --- a/spec/unit/providers/vsphere_spec.rb +++ b/spec/unit/providers/vsphere_spec.rb @@ -283,6 +283,7 @@ EOT let(:clone_vm_task) { mock_RbVmomi_VIM_Task() } let(:new_vm_object) { mock_RbVmomi_VIM_VirtualMachine({ :name => vmname }) } + let(:new_template_object) { mock_RbVmomi_VIM_VirtualMachine({ :name => vmname }) } before(:each) do allow(subject).to receive(:connect_to_vsphere).and_return(connection) @@ -305,19 +306,30 @@ EOT end end - context 'Given a template path that does not exist' do + context 'Given a template that starts with /' do before(:each) do - config[:pools][0]['template'] = 'missing_Templates/pool1' + config[:pools][0]['template'] = '/bad_template' end it 'should raise an error' do - expect{ subject.create_vm(poolname, vmname) }.to raise_error(/specifies a template folder of .+ which does not exist/) + expect{ subject.create_vm(poolname, vmname) }.to raise_error(/did not specify a full path for the template/) + end + end + + context 'Given a template that ends with /' do + before(:each) do + config[:pools][0]['template'] = 'bad_template/' + end + + it 'should raise an error' do + expect{ subject.create_vm(poolname, vmname) }.to raise_error(/did not specify a full path for the template/) end end context 'Given a template VM that does not exist' do before(:each) do config[:pools][0]['template'] = 'Templates/missing_template' + expect(subject).to receive(:find_template_vm).and_raise("specifies a template VM of #{vmname} which does not exist") end it 'should raise an error' do @@ -327,7 +339,8 @@ EOT context 'Given a successful creation' do before(:each) do - template_vm = subject.find_folder('Templates',connection,datacenter_name).find('pool1') + template_vm = new_template_object + allow(subject).to receive(:find_template_vm).and_return(new_template_object) allow(template_vm).to receive(:CloneVM_Task).and_return(clone_vm_task) allow(clone_vm_task).to receive(:wait_for_completion).and_return(new_vm_object) end @@ -339,7 +352,7 @@ EOT end it 'should use the appropriate Create_VM spec' do - template_vm = subject.find_folder('Templates',connection,datacenter_name).find('pool1') + template_vm = new_template_object expect(template_vm).to receive(:CloneVM_Task) .with(create_vm_spec(vmname,'pool1','datastore0')) .and_return(clone_vm_task) @@ -3461,5 +3474,71 @@ EOT end end + describe 'find_template_vm' do + let(:vm_object) { mock_RbVmomi_VIM_VirtualMachine() } + before(:each) do + allow(connection.searchIndex).to receive(:FindByInventoryPath) + end + it 'should raise an error when the datacenter cannot be found' do + config[:providers][:vsphere]['datacenter'] = nil + + expect{ subject.find_template_vm(config[:pools][0],connection) }.to raise_error('cannot find datacenter') + end + + it 'should raise an error when the template specified cannot be found' do + expect(connection.searchIndex).to receive(:FindByInventoryPath).and_return(nil) + + expect{ subject.find_template_vm(config[:pools][0],connection) }.to raise_error("Pool #{poolname} specifies a template VM of #{config[:pools][0]['template']} which does not exist for the provider vsphere") + end + + it 'should return the vm object' do + expect(connection.searchIndex).to receive(:FindByInventoryPath).and_return(vm_object) + + subject.find_template_vm(config[:pools][0],connection) + end + end + + describe 'valid_template_path?' do + + it 'should return true with a valid template path' do + expect(subject.valid_template_path?('test/template')).to eq(true) + end + + it 'should return false when no / is found' do + expect(subject.valid_template_path?('testtemplate')).to eq(false) + end + + it 'should return false when template path begins with /' do + expect(subject.valid_template_path?('/testtemplate')).to eq(false) + end + + it 'should return false when template path ends with /' do + expect(subject.valid_template_path?('testtemplate/')).to eq(false) + end + end + + describe 'create_template_delta_disks' do + let(:template_object) { mock_RbVmomi_VIM_VirtualMachine({ + :name => vmname, + }) + } + + before(:each) do + allow(subject).to receive(:connect_to_vsphere).and_return(connection) + end + + context 'with a template VM found' do + + before(:each) do + expect(subject).to receive(:find_template_vm).and_return(template_object) + end + + it 'should reconfigure the VM creating delta disks' do + expect(template_object).to receive(:add_delta_disk_layer_on_all_disks) + + subject.create_template_delta_disks(config[:pools][0]) + end + end + end end diff --git a/vmpooler.yaml.example b/vmpooler.yaml.example index ad7eb61..5901c4b 100644 --- a/vmpooler.yaml.example +++ b/vmpooler.yaml.example @@ -443,6 +443,11 @@ # The value represents a percentage and applies to both memory and CPU # (optional; default: 90) # +# - experimental_features (Only affects API config endpoints) +# Enable experimental API capabilities such as changing pool template and size without application restart +# Expects a boolean value +# (optional; default: false) +# # Example: :config: @@ -458,6 +463,7 @@ - 'project' domain: 'company.com' prefix: 'poolvm-' + experimental_features: true # :pools: # From 3a0f0880e741009ad2775fdde49d3ecc68653e08 Mon Sep 17 00:00:00 2001 From: "kirby@puppetlabs.com" Date: Fri, 25 May 2018 11:54:46 -0700 Subject: [PATCH 04/10] (POOLER-112) Ensure a VM is only destroyed once This commit implements a vm_mutex hash to allow synchronizing VM operations that should only happen once across threads. Without this change pool_manager will try to evaluate or destroy a VM multiple times, which results in an error being thrown by one of the destroy attempts as only one can succeed and a duplication of resources unnecessarily when there are no errors. --- lib/vmpooler/pool_manager.rb | 163 +++++++++++++++++++-------------- spec/unit/pool_manager_spec.rb | 79 +++++++++++++++- 2 files changed, 172 insertions(+), 70 deletions(-) diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb index 866cf36..a00acb4 100644 --- a/lib/vmpooler/pool_manager.rb +++ b/lib/vmpooler/pool_manager.rb @@ -24,6 +24,8 @@ module Vmpooler # Pool mutex @reconfigure_pool = {} + + @vm_mutex = {} end def config @@ -44,15 +46,19 @@ module Vmpooler end def _check_pending_vm(vm, pool, timeout, provider) - host = provider.get_vm(pool, vm) - unless host - fail_pending_vm(vm, pool, timeout, false) - return - end - if provider.vm_ready?(pool, vm) - move_pending_vm_to_ready(vm, pool, host) - else - fail_pending_vm(vm, pool, timeout) + mutex = vm_mutex(vm) + return if mutex.locked? + mutex.synchronize do + host = provider.get_vm(pool, vm) + unless host + fail_pending_vm(vm, pool, timeout, false) + return + end + if provider.vm_ready?(pool, vm) + move_pending_vm_to_ready(vm, pool, host) + else + fail_pending_vm(vm, pool, timeout) + end end end @@ -114,51 +120,55 @@ module Vmpooler def _check_ready_vm(vm, pool, ttl, provider) # Periodically check that the VM is available - check_stamp = $redis.hget('vmpooler__vm__' + vm, 'check') - return if check_stamp && (((Time.now - Time.parse(check_stamp)) / 60) <= $config[:config]['vm_checktime']) + mutex = vm_mutex(vm) + return if mutex.locked? + mutex.synchronize do + check_stamp = $redis.hget('vmpooler__vm__' + vm, 'check') + return if check_stamp && (((Time.now - Time.parse(check_stamp)) / 60) <= $config[:config]['vm_checktime']) - host = provider.get_vm(pool, vm) - # Check if the host even exists - unless host - $redis.srem('vmpooler__ready__' + pool, vm) - $logger.log('s', "[!] [#{pool}] '#{vm}' not found in inventory, removed from 'ready' queue") - return - end - - $redis.hset('vmpooler__vm__' + vm, 'check', Time.now) - # Check if the VM is not powered on, before checking TTL - unless host['powerstate'].casecmp('poweredon').zero? - $redis.smove('vmpooler__ready__' + pool, 'vmpooler__completed__' + pool, vm) - $logger.log('d', "[!] [#{pool}] '#{vm}' appears to be powered off, removed from 'ready' queue") - return - end - - # Check if the hosts TTL has expired - if ttl > 0 - # host['boottime'] may be nil if host is not powered on - if ((Time.now - host['boottime']) / 60).to_s[/^\d+\.\d{1}/].to_f > ttl - $redis.smove('vmpooler__ready__' + pool, 'vmpooler__completed__' + pool, vm) - - $logger.log('d', "[!] [#{pool}] '#{vm}' reached end of TTL after #{ttl} minutes, removed from 'ready' queue") + host = provider.get_vm(pool, vm) + # Check if the host even exists + unless host + $redis.srem('vmpooler__ready__' + pool, vm) + $logger.log('s', "[!] [#{pool}] '#{vm}' not found in inventory, removed from 'ready' queue") return end - end - # Check if the hostname has magically changed from underneath Pooler - if host['hostname'] != vm - $redis.smove('vmpooler__ready__' + pool, 'vmpooler__completed__' + pool, vm) - $logger.log('d', "[!] [#{pool}] '#{vm}' has mismatched hostname, removed from 'ready' queue") - return - end + $redis.hset('vmpooler__vm__' + vm, 'check', Time.now) + # Check if the VM is not powered on, before checking TTL + unless host['powerstate'].casecmp('poweredon').zero? + $redis.smove('vmpooler__ready__' + pool, 'vmpooler__completed__' + pool, vm) + $logger.log('d', "[!] [#{pool}] '#{vm}' appears to be powered off, removed from 'ready' queue") + return + end - # Check if the VM is still ready/available - begin - raise("VM #{vm} is not ready") unless provider.vm_ready?(pool, vm) - rescue - if $redis.smove('vmpooler__ready__' + pool, 'vmpooler__completed__' + pool, vm) - $logger.log('d', "[!] [#{pool}] '#{vm}' is unreachable, removed from 'ready' queue") - else - $logger.log('d', "[!] [#{pool}] '#{vm}' is unreachable, and failed to remove from 'ready' queue") + # Check if the hosts TTL has expired + if ttl > 0 + # host['boottime'] may be nil if host is not powered on + if ((Time.now - host['boottime']) / 60).to_s[/^\d+\.\d{1}/].to_f > ttl + $redis.smove('vmpooler__ready__' + pool, 'vmpooler__completed__' + pool, vm) + + $logger.log('d', "[!] [#{pool}] '#{vm}' reached end of TTL after #{ttl} minutes, removed from 'ready' queue") + return + end + end + + # Check if the hostname has magically changed from underneath Pooler + if host['hostname'] != vm + $redis.smove('vmpooler__ready__' + pool, 'vmpooler__completed__' + pool, vm) + $logger.log('d', "[!] [#{pool}] '#{vm}' has mismatched hostname, removed from 'ready' queue") + return + end + + # Check if the VM is still ready/available + begin + raise("VM #{vm} is not ready") unless provider.vm_ready?(pool, vm) + rescue + if $redis.smove('vmpooler__ready__' + pool, 'vmpooler__completed__' + pool, vm) + $logger.log('d', "[!] [#{pool}] '#{vm}' is unreachable, removed from 'ready' queue") + else + $logger.log('d', "[!] [#{pool}] '#{vm}' is unreachable, and failed to remove from 'ready' queue") + end end end end @@ -175,16 +185,20 @@ module Vmpooler end def _check_running_vm(vm, pool, ttl, provider) - host = provider.get_vm(pool, vm) + mutex = vm_mutex(vm) + return if mutex.locked? + mutex.synchronize do + host = provider.get_vm(pool, vm) - if host - # Check that VM is within defined lifetime - checkouttime = $redis.hget('vmpooler__active__' + pool, vm) - if checkouttime - running = (Time.now - Time.parse(checkouttime)) / 60 / 60 + if host + # Check that VM is within defined lifetime + checkouttime = $redis.hget('vmpooler__active__' + pool, vm) + if checkouttime + running = (Time.now - Time.parse(checkouttime)) / 60 / 60 - if (ttl.to_i > 0) && (running.to_i >= ttl.to_i) - move_vm_queue(pool, vm, 'running', 'completed', "reached end of TTL after #{ttl} hours") + if (ttl.to_i > 0) && (running.to_i >= ttl.to_i) + move_vm_queue(pool, vm, 'running', 'completed', "reached end of TTL after #{ttl} hours") + end end end end @@ -251,20 +265,24 @@ module Vmpooler end def _destroy_vm(vm, pool, provider) - $redis.srem('vmpooler__completed__' + pool, vm) - $redis.hdel('vmpooler__active__' + pool, vm) - $redis.hset('vmpooler__vm__' + vm, 'destroy', Time.now) + mutex = vm_mutex(vm) + return if mutex.locked? + mutex.synchronize do + $redis.srem('vmpooler__completed__' + pool, vm) + $redis.hdel('vmpooler__active__' + pool, vm) + $redis.hset('vmpooler__vm__' + vm, 'destroy', Time.now) - # Auto-expire metadata key - $redis.expire('vmpooler__vm__' + vm, ($config[:redis]['data_ttl'].to_i * 60 * 60)) + # Auto-expire metadata key + $redis.expire('vmpooler__vm__' + vm, ($config[:redis]['data_ttl'].to_i * 60 * 60)) - start = Time.now + start = Time.now - provider.destroy_vm(pool, vm) + provider.destroy_vm(pool, vm) - finish = format('%.2f', Time.now - start) - $logger.log('s', "[-] [#{pool}] '#{vm}' destroyed in #{finish} seconds") - $metrics.timing("destroy.#{pool}", finish) + finish = format('%.2f', Time.now - start) + $logger.log('s', "[-] [#{pool}] '#{vm}' destroyed in #{finish} seconds") + $metrics.timing("destroy.#{pool}", finish) + end end def create_vm_disk(pool_name, vm, disk_size, provider) @@ -467,8 +485,11 @@ module Vmpooler def migrate_vm(vm_name, pool_name, provider) Thread.new do begin - $redis.srem("vmpooler__migrating__#{pool_name}", vm_name) - provider.migrate_vm(pool_name, vm_name) + mutex = vm_mutex(vm_name) + mutex.synchronize do + $redis.srem("vmpooler__migrating__#{pool_name}", vm_name) + provider.migrate_vm(pool_name, vm_name) + end rescue => err $logger.log('s', "[x] [#{pool_name}] '#{vm_name}' migration failed with an error: #{err}") end @@ -579,6 +600,10 @@ module Vmpooler @reconfigure_pool[poolname] || @reconfigure_pool[poolname] = Mutex.new end + def vm_mutex(vmname) + @vm_mutex[vmname] || @vm_mutex[vmname] = Mutex.new + end + def sync_pool_template(pool) pool_template = $redis.hget('vmpooler__config__template', pool['name']) if pool_template diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb index 19f6c3f..be176d6 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/spec/unit/pool_manager_spec.rb @@ -92,6 +92,19 @@ EOT subject._check_pending_vm(vm, pool, timeout, provider) end end + + context 'with a locked vm mutex' do + let(:mutex) { Mutex.new } + before(:each) do + mutex.lock + end + + it 'should return' do + expect(subject).to receive(:vm_mutex).and_return(mutex) + + expect(subject._check_pending_vm(vm, pool, timeout, provider)).to be_nil + end + end end describe '#remove_nonexistent_vm' do @@ -404,6 +417,19 @@ EOT end end end + + context 'with a locked vm mutex' do + let(:mutex) { Mutex.new } + before(:each) do + mutex.lock + end + + it 'should return' do + expect(subject).to receive(:vm_mutex).and_return(mutex) + + expect(subject._check_ready_vm(vm, pool, ttl, provider)).to be_nil + end + end end describe '#check_running_vm' do @@ -479,6 +505,19 @@ EOT expect(redis.sismember("vmpooler__completed__#{pool}", vm)).to be(true) end end + + context 'with a locked vm mutex' do + let(:mutex) { Mutex.new } + before(:each) do + mutex.lock + end + + it 'should return' do + expect(subject).to receive(:vm_mutex).and_return(mutex) + + expect(subject._check_running_vm(vm, pool, timeout, provider)).to be_nil + end + end end describe '#move_vm_queue' do @@ -681,7 +720,7 @@ EOT before(:each) do config[:redis] = nil end - + it 'should raise an error' do expect{ subject._destroy_vm(vm,pool,provider) }.to raise_error(NoMethodError) end @@ -732,6 +771,19 @@ EOT expect{ subject._destroy_vm(vm,pool,provider) }.to raise_error(/MockError/) end end + + context 'when the VM mutex is locked' do + let(:mutex) { Mutex.new } + before(:each) do + mutex.lock + end + + it 'should return' do + expect(subject).to receive(:vm_mutex).with(vm).and_return(mutex) + + expect(subject._destroy_vm(vm,pool,provider)).to eq(nil) + end + end end describe '#create_vm_disk' do @@ -1501,6 +1553,31 @@ EOT subject.migrate_vm(vm, pool, provider) end end + + context 'with a locked vm mutex' do + let(:mutex) { Mutex.new } + before(:each) do + mutex.lock + end + + it 'should return' do + expect(subject).to receive(:vm_mutex).and_return(mutex) + + expect(subject.migrate_vm(vm, pool, provider)).to be_nil + end + end + end + + describe '#vm_mutex' do + it 'should return a mutex' do + expect(subject.vm_mutex(vm)).to be_a(Mutex) + end + + it 'should return the same mutex when called twice' do + first = subject.vm_mutex(vm) + second = subject.vm_mutex(vm) + expect(first).to be(second) + end end describe 'sync_pool_template' do From 9fa27af8e5fc3cb6fa463ba02e3e2dead48e9734 Mon Sep 17 00:00:00 2001 From: "kirby@puppetlabs.com" Date: Mon, 25 Jun 2018 13:56:55 -0700 Subject: [PATCH 05/10] (POOLER-113) Add support for multiple LDAP search bases This commit updates vmpooler to support setting an array of search bases in addition to a single base provided as a string. Without this change it is not possible to specify multiple search bases to use with the LDAP authentication provider. Additionally, test coverage is added to the authentication helper method. --- lib/vmpooler/api/helpers.rb | 70 +++++--- spec/integration/api/v1/config_spec.rb | 10 -- spec/integration/api/v1/status_spec.rb | 10 -- spec/integration/api/v1/vm_hostname_spec.rb | 10 -- spec/integration/api/v1/vm_spec.rb | 10 -- spec/integration/api/v1/vm_template_spec.rb | 10 -- spec/unit/api/helpers_spec.rb | 169 ++++++++++++++++++++ 7 files changed, 218 insertions(+), 71 deletions(-) diff --git a/lib/vmpooler/api/helpers.rb b/lib/vmpooler/api/helpers.rb index f76cbce..7575b6d 100644 --- a/lib/vmpooler/api/helpers.rb +++ b/lib/vmpooler/api/helpers.rb @@ -54,35 +54,63 @@ module Vmpooler return false end + def authenticate_ldap(port, host, user_object, base, username_str, password_str) + require 'rubygems' + require 'net/ldap' + + ldap = Net::LDAP.new( + :host => host, + :port => port, + :encryption => { + :method => :start_tls, + :tls_options => { :ssl_version => 'TLSv1' } + }, + :base => base, + :auth => { + :method => :simple, + :username => "#{user_object}=#{username_str},#{base}", + :password => password_str + } + ) + + return true if ldap.bind + return false + end + def authenticate(auth, username_str, password_str) case auth['provider'] when 'dummy' return (username_str != password_str) when 'ldap' - require 'rubygems' - require 'net/ldap' + ldap_base = auth[:ldap]['base'] + ldap_port = auth[:ldap]['port'] || 389 - ldap = Net::LDAP.new( - :host => auth[:ldap]['host'], - :port => auth[:ldap]['port'] || 389, - :encryption => { - :method => :start_tls, - :tls_options => { :ssl_version => 'TLSv1' } - }, - :base => auth[:ldap]['base'], - :auth => { - :method => :simple, - :username => "#{auth[:ldap]['user_object']}=#{username_str},#{auth[:ldap]['base']}", - :password => password_str - } - ) - - if ldap.bind - return true + if ldap_base.is_a? Array + ldap_base.each do |search_base| + result = authenticate_ldap( + ldap_port, + auth[:ldap]['host'], + auth[:ldap]['user_object'], + search_base, + username_str, + password_str, + ) + return true if result == true + end + else + result = authenticate_ldap( + ldap_port, + auth[:ldap]['host'], + auth[:ldap]['user_object'], + ldap_base, + username_str, + password_str, + ) + return result end - end - return false + return false + end end def export_tags(backend, hostname, tags) diff --git a/spec/integration/api/v1/config_spec.rb b/spec/integration/api/v1/config_spec.rb index abcfdb1..e5136d7 100644 --- a/spec/integration/api/v1/config_spec.rb +++ b/spec/integration/api/v1/config_spec.rb @@ -1,16 +1,6 @@ require 'spec_helper' require 'rack/test' -module Vmpooler - class API - module Helpers - def authenticate(auth, username_str, password_str) - username_str == 'admin' and password_str == 's3cr3t' - end - end - end -end - describe Vmpooler::API::V1 do include Rack::Test::Methods diff --git a/spec/integration/api/v1/status_spec.rb b/spec/integration/api/v1/status_spec.rb index 2f2f1ba..3ec6cfc 100644 --- a/spec/integration/api/v1/status_spec.rb +++ b/spec/integration/api/v1/status_spec.rb @@ -1,16 +1,6 @@ require 'spec_helper' require 'rack/test' -module Vmpooler - class API - module Helpers - def authenticate(auth, username_str, password_str) - username_str == 'admin' and password_str == 's3cr3t' - end - end - end -end - def has_set_tag?(vm, tag, value) value == redis.hget("vmpooler__vm__#{vm}", "tag:#{tag}") end diff --git a/spec/integration/api/v1/vm_hostname_spec.rb b/spec/integration/api/v1/vm_hostname_spec.rb index f5dce5b..f8ade07 100644 --- a/spec/integration/api/v1/vm_hostname_spec.rb +++ b/spec/integration/api/v1/vm_hostname_spec.rb @@ -1,16 +1,6 @@ require 'spec_helper' require 'rack/test' -module Vmpooler - class API - module Helpers - def authenticate(auth, username_str, password_str) - username_str == 'admin' and password_str == 's3cr3t' - end - end - end -end - def has_set_tag?(vm, tag, value) value == redis.hget("vmpooler__vm__#{vm}", "tag:#{tag}") end diff --git a/spec/integration/api/v1/vm_spec.rb b/spec/integration/api/v1/vm_spec.rb index 0c14561..387ecd0 100644 --- a/spec/integration/api/v1/vm_spec.rb +++ b/spec/integration/api/v1/vm_spec.rb @@ -1,16 +1,6 @@ require 'spec_helper' require 'rack/test' -module Vmpooler - class API - module Helpers - def authenticate(auth, username_str, password_str) - username_str == 'admin' and password_str == 's3cr3t' - end - end - end -end - describe Vmpooler::API::V1 do include Rack::Test::Methods diff --git a/spec/integration/api/v1/vm_template_spec.rb b/spec/integration/api/v1/vm_template_spec.rb index aaabf4a..72fef36 100644 --- a/spec/integration/api/v1/vm_template_spec.rb +++ b/spec/integration/api/v1/vm_template_spec.rb @@ -1,16 +1,6 @@ require 'spec_helper' require 'rack/test' -module Vmpooler - class API - module Helpers - def authenticate(auth, username_str, password_str) - username_str == 'admin' and password_str == 's3cr3t' - end - end - end -end - describe Vmpooler::API::V1 do include Rack::Test::Methods diff --git a/spec/unit/api/helpers_spec.rb b/spec/unit/api/helpers_spec.rb index 7e75045..4b36542 100644 --- a/spec/unit/api/helpers_spec.rb +++ b/spec/unit/api/helpers_spec.rb @@ -1,4 +1,5 @@ require 'spec_helper' +require 'net/ldap' # A class for testing purposes that includes the Helpers. # this is impersonating V1's `helpers do include Helpers end` @@ -238,4 +239,172 @@ describe Vmpooler::API::Helpers do end end + describe '#authenticate' do + let(:username_str) { 'admin' } + let(:password_str) { 's3cr3t' } + + context 'with dummy provider' do + let(:auth) { + { + 'provider': 'dummy' + } + } + it 'should return true' do + expect(subject).to receive(:authenticate).with(auth, username_str, password_str).and_return(true) + + subject.authenticate(auth, username_str, password_str) + end + end + + context 'with ldap provider' do + let(:host) { 'ldap.example.com' } + let(:base) { 'ou=user,dc=test,dc=com' } + let(:user_object) { 'uid' } + let(:auth) { + { + 'provider' => 'ldap', + ldap: { + 'host' => host, + 'base' => base, + 'user_object' => user_object + } + } + } + let(:default_port) { 389 } + it 'should attempt ldap authentication' do + expect(subject).to receive(:authenticate_ldap).with(default_port, host, user_object, base, username_str, password_str) + + subject.authenticate(auth, username_str, password_str) + end + + it 'should return true when authentication is successful' do + expect(subject).to receive(:authenticate_ldap).with(default_port, host, user_object, base, username_str, password_str).and_return(true) + + expect(subject.authenticate(auth, username_str, password_str)).to be true + end + + it 'should return false when authentication fails' do + expect(subject).to receive(:authenticate_ldap).with(default_port, host, user_object, base, username_str, password_str).and_return(false) + + expect(subject.authenticate(auth, username_str, password_str)).to be false + end + + context 'with an alternate port' do + let(:alternate_port) { 636 } + before(:each) do + auth[:ldap]['port'] = alternate_port + end + + it 'should specify the alternate port when authenticating' do + expect(subject).to receive(:authenticate_ldap).with(alternate_port, host, user_object, base, username_str, password_str) + + subject.authenticate(auth, username_str, password_str) + end + end + + context 'with multiple search bases' do + let(:base) { + [ + 'ou=user,dc=test,dc=com', + 'ou=service,ou=user,dc=test,dc=com' + ] + } + before(:each) do + auth[:ldap]['base'] = base + end + + it 'should attempt to bind with each base' do + expect(subject).to receive(:authenticate_ldap).with(default_port, host, user_object, base[0], username_str, password_str) + expect(subject).to receive(:authenticate_ldap).with(default_port, host, user_object, base[1], username_str, password_str) + + subject.authenticate(auth, username_str, password_str) + end + + it 'should not search the second base when the first binds' do + expect(subject).to receive(:authenticate_ldap).with(default_port, host, user_object, base[0], username_str, password_str).and_return(true) + expect(subject).to_not receive(:authenticate_ldap).with(default_port, host, user_object, base[1], username_str, password_str) + + subject.authenticate(auth, username_str, password_str) + end + + it 'should search the second base when the first bind fails' do + expect(subject).to receive(:authenticate_ldap).with(default_port, host, user_object, base[0], username_str, password_str).and_return(false) + expect(subject).to receive(:authenticate_ldap).with(default_port, host, user_object, base[1], username_str, password_str) + + subject.authenticate(auth, username_str, password_str) + end + + it 'should return true when any bind succeeds' do + expect(subject).to receive(:authenticate_ldap).with(default_port, host, user_object, base[0], username_str, password_str).and_return(false) + expect(subject).to receive(:authenticate_ldap).with(default_port, host, user_object, base[1], username_str, password_str).and_return(true) + + expect(subject.authenticate(auth, username_str, password_str)).to be true + end + + it 'should return false when all bind attempts fail' do + expect(subject).to receive(:authenticate_ldap).with(default_port, host, user_object, base[0], username_str, password_str).and_return(false) + expect(subject).to receive(:authenticate_ldap).with(default_port, host, user_object, base[1], username_str, password_str).and_return(false) + + expect(subject.authenticate(auth, username_str, password_str)).to be false + end + end + + end + + context 'with unknown provider' do + let(:auth) { + { + 'provider': 'mystery' + } + } + it 'should return false' do + expect(subject).to receive(:authenticate).with(auth, username_str, password_str).and_return(false) + subject.authenticate(auth, username_str, password_str) + end + end + end + + describe '#authenticate_ldap' do + let(:port) { 389 } + let(:host) { 'ldap.example.com' } + let(:user_object) { 'uid' } + let(:base) { 'ou=users,dc=example,dc=com' } + let(:username_str) { 'admin' } + let(:password_str) { 's3cr3t' } + let(:ldap) { double('ldap') } + it 'should create a new ldap connection' do + allow(ldap).to receive(:bind) + expect(Net::LDAP).to receive(:new).with( + :host => host, + :port => port, + :encryption => { + :method => :start_tls, + :tls_options => { :ssl_version => 'TLSv1' } + }, + :base => base, + :auth => { + :method => :simple, + :username => "#{user_object}=#{username_str},#{base}", + :password => password_str + } + ).and_return(ldap) + + subject.authenticate_ldap(port, host, user_object, base, username_str, password_str) + end + + it 'should return true when a bind is successful' do + expect(Net::LDAP).to receive(:new).and_return(ldap) + expect(ldap).to receive(:bind).and_return(true) + + expect(subject.authenticate_ldap(port, host, user_object, base, username_str, password_str)).to be true + end + + it 'should return false when a bind fails' do + expect(Net::LDAP).to receive(:new).and_return(ldap) + expect(ldap).to receive(:bind).and_return(false) + + expect(subject.authenticate_ldap(port, host, user_object, base, username_str, password_str)).to be false + end + end + end From cbd4567454b9f04efb51cc3adac3f8650a8b26d9 Mon Sep 17 00:00:00 2001 From: "kirby@puppetlabs.com" Date: Wed, 27 Jun 2018 14:41:27 -0700 Subject: [PATCH 06/10] Provide valid configuration for auth This commit removes a additional authenticate method that is defined in the token_spec tests. Instead, authenticate is used from api/helpers. To support this change the config provided is updated to specify a dummy provider. Without this change authenticate cannot be tested along with token_spec because token_spec redefines authenticate. --- spec/integration/api/v1/token_spec.rb | 36 +++++++++++++++++---------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/spec/integration/api/v1/token_spec.rb b/spec/integration/api/v1/token_spec.rb index d386457..983dac6 100644 --- a/spec/integration/api/v1/token_spec.rb +++ b/spec/integration/api/v1/token_spec.rb @@ -1,16 +1,6 @@ require 'spec_helper' require 'rack/test' -module Vmpooler - class API - module Helpers - def authenticate(auth, username_str, password_str) - username_str == 'admin' and password_str == 's3cr3t' - end - end - end -end - describe Vmpooler::API::V1 do include Rack::Test::Methods @@ -39,7 +29,15 @@ describe Vmpooler::API::V1 do end context '(auth configured)' do - let(:config) { { auth: true } } + let(:config) { + { + auth: { + 'provider' => 'dummy' + } + } + } + let(:username_str) { 'admin' } + let(:password_str) { 's3cr3t' } it 'returns a 401 if not authed' do get "#{prefix}/token" @@ -69,7 +67,13 @@ describe Vmpooler::API::V1 do end context '(auth configured)' do - let(:config) { { auth: true } } + let(:config) { + { + auth: { + 'provider' => 'dummy' + } + } + } it 'returns a 401 if not authed' do post "#{prefix}/token" @@ -146,7 +150,13 @@ describe Vmpooler::API::V1 do end context '(auth configured)' do - let(:config) { { auth: true } } + let(:config) { + { + auth: { + 'provider' => 'dummy' + } + } + } it 'returns a 401 if not authed' do delete "#{prefix}/token/this" From 4fa54c8008def8e5ac44700d3040a7d671b99bd5 Mon Sep 17 00:00:00 2001 From: "kirby@puppetlabs.com" Date: Wed, 27 Jun 2018 15:21:15 -0700 Subject: [PATCH 07/10] Move net/ldap require into vmpooler.rb This commit moves net/ldap require from authenticate_ldap in api/helpers to vmpooler.rb. Without this change net/ldap and rubygems are required again every time authenticate_ldap is run. --- lib/vmpooler.rb | 1 + lib/vmpooler/api/helpers.rb | 3 --- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/vmpooler.rb b/lib/vmpooler.rb index a1c757b..73042d9 100644 --- a/lib/vmpooler.rb +++ b/lib/vmpooler.rb @@ -4,6 +4,7 @@ module Vmpooler require 'date' require 'json' require 'open-uri' + require 'net/ldap' require 'rbvmomi' require 'redis' require 'sinatra/base' diff --git a/lib/vmpooler/api/helpers.rb b/lib/vmpooler/api/helpers.rb index 7575b6d..34bf5b6 100644 --- a/lib/vmpooler/api/helpers.rb +++ b/lib/vmpooler/api/helpers.rb @@ -55,9 +55,6 @@ module Vmpooler end def authenticate_ldap(port, host, user_object, base, username_str, password_str) - require 'rubygems' - require 'net/ldap' - ldap = Net::LDAP.new( :host => host, :port => port, From 678290f77919cfda49eefeba26ad876830967cd8 Mon Sep 17 00:00:00 2001 From: Samuel Beaulieu Date: Tue, 19 Jun 2018 11:51:29 -0500 Subject: [PATCH 08/10] Adding a docker compose for local dev Changed the dummy example file to match the expected LOG location --- docker-compose.yml | 15 +++++++++++++++ vmpooler.yaml.dummy-example | 2 +- 2 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 docker-compose.yml diff --git a/docker-compose.yml b/docker-compose.yml new file mode 100644 index 0000000..062f915 --- /dev/null +++ b/docker-compose.yml @@ -0,0 +1,15 @@ +# For local development run with a dummy provider +version: '3' +services: + aio-local: + build: + context: . + dockerfile: Dockerfile-aio + volumes: + - ".:/var/lib/vmpooler" + ports: + - "80:4567" + environment: + - VMPOOLER_DEBUG=true # for use of dummy auth + - VMPOOLER_CONFIG_FILE=vmpooler.yaml.dummy-example + image: vmpooler-aio-local \ No newline at end of file diff --git a/vmpooler.yaml.dummy-example b/vmpooler.yaml.dummy-example index ffb2985..df55664 100644 --- a/vmpooler.yaml.dummy-example +++ b/vmpooler.yaml.dummy-example @@ -14,7 +14,7 @@ :config: site_name: 'vmpooler' - logfile: '/var/log/vmpooler/vmpooler.log' + logfile: '/var/log/vmpooler.log' task_limit: 10 timeout: 15 vm_checktime: 15 From ec07f04d9516615feb71f4a28cf2ea42047890e6 Mon Sep 17 00:00:00 2001 From: kevpl Date: Tue, 19 Jun 2018 11:00:16 -0700 Subject: [PATCH 09/10] (MOB) refactor create_inventory into own method Takes the #INVENTORY section from the _check_pool method & splits it into its own method --- lib/vmpooler/pool_manager.rb | 30 +++++++++++++++++------------- spec/unit/pool_manager_spec.rb | 26 ++++++++++++++++++++++---- 2 files changed, 39 insertions(+), 17 deletions(-) diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb index a00acb4..927404f 100644 --- a/lib/vmpooler/pool_manager.rb +++ b/lib/vmpooler/pool_manager.rb @@ -695,17 +695,7 @@ module Vmpooler end end - def _check_pool(pool, provider) - pool_check_response = { - discovered_vms: 0, - checked_running_vms: 0, - checked_ready_vms: 0, - checked_pending_vms: 0, - destroyed_vms: 0, - migrated_vms: 0, - cloned_vms: 0 - } - # INVENTORY + def create_inventory(pool, provider) inventory = {} begin provider.vms_in_pool(pool['name']).each do |vm| @@ -725,9 +715,23 @@ module Vmpooler inventory[vm['name']] = 1 end rescue => err - $logger.log('s', "[!] [#{pool['name']}] _check_pool failed with an error while inspecting inventory: #{err}") - return pool_check_response + $logger.log('s', "[!] [#{pool['name']}] _check_pool failed with an error while running create_inventory: #{err}") + raise(err) end + inventory + end + + def _check_pool(pool, provider) + pool_check_response = { + discovered_vms: 0, + checked_running_vms: 0, + checked_ready_vms: 0, + checked_pending_vms: 0, + destroyed_vms: 0, + migrated_vms: 0, + cloned_vms: 0 + } + inventory = create_inventory(pool, provider) # RUNNING $redis.smembers("vmpooler__running__#{pool['name']}").each do |vm| diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb index be176d6..47d1138 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/spec/unit/pool_manager_spec.rb @@ -2651,6 +2651,17 @@ EOT end end + describe '#create_inventory' do + + it 'should log an error if one occurs' # do +# expect(provider).to receive(:vms_in_pool).and_raise(RuntimeError,'Mock Error') +# expect(logger).to receive(:log).with('s', "[!] [#{pool}] _check_pool failed with an error while running create_inventory: Mock Error") +# +# subject._check_pool(pool_object,provider) +# end + + end + describe '#_check_pool' do let(:new_vm_response) { # Mock response from Base Provider for vms_in_pool @@ -2703,11 +2714,18 @@ EOT allow(provider).to receive(:vms_in_pool).with(pool).and_return(new_vm_response) end - it 'should log an error if one occurs' do - expect(provider).to receive(:vms_in_pool).and_raise(RuntimeError,'Mock Error') - expect(logger).to receive(:log).with('s', "[!] [#{pool}] _check_pool failed with an error while inspecting inventory: Mock Error") + it 'calls inventory correctly' do + expect(subject).to receive(:create_inventory) + subject._check_pool(pool_object, provider) + end - subject._check_pool(pool_object,provider) + it 'passes #create_inventory errors correctly' do + allow(subject).to receive(:create_inventory).and_raise( + RuntimeError,'Mock Error' + ) + expect { + subject._check_pool(pool_object, provider) + }.to raise_error(RuntimeError, /Mock Error/) end it 'should not perform any other actions if an error occurs' do From fa988470a702fa1f8c977a42a9299cc6e1a82f22 Mon Sep 17 00:00:00 2001 From: "kirby@puppetlabs.com" Date: Tue, 19 Jun 2018 11:27:47 -0700 Subject: [PATCH 10/10] Pass pool_check_response to create_inventory Ensure that pool_check_response is passed in to create_inventory. Start base for running pool vms. --- lib/vmpooler/pool_manager.rb | 19 +++++++++++++++++-- spec/unit/pool_manager_spec.rb | 28 +++++++--------------------- 2 files changed, 24 insertions(+), 23 deletions(-) diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb index 927404f..6d0bdfd 100644 --- a/lib/vmpooler/pool_manager.rb +++ b/lib/vmpooler/pool_manager.rb @@ -695,7 +695,7 @@ module Vmpooler end end - def create_inventory(pool, provider) + def create_inventory(pool, provider, pool_check_response) inventory = {} begin provider.vms_in_pool(pool['name']).each do |vm| @@ -721,6 +721,10 @@ module Vmpooler inventory end + def check_running_pool_vms(pool, provider, pool_check_response) + # do stuff here + end + def _check_pool(pool, provider) pool_check_response = { discovered_vms: 0, @@ -731,9 +735,20 @@ module Vmpooler migrated_vms: 0, cloned_vms: 0 } - inventory = create_inventory(pool, provider) + + begin + inventory = create_inventory(pool, provider, pool_check_response) + rescue => err + return(pool_check_response) + end # RUNNING + begin + check_running_pool_vms(pool, provider, pool_check_response) + rescue => err + return(pool_check_response) + end + $redis.smembers("vmpooler__running__#{pool['name']}").each do |vm| if inventory[vm] begin diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb index 47d1138..74a2446 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/spec/unit/pool_manager_spec.rb @@ -2729,29 +2729,15 @@ EOT end it 'should not perform any other actions if an error occurs' do - # Add VMs into redis - create_running_vm(pool_name, 'vm1') - create_ready_vm(pool_name, 'vm2') - create_completed_vm('vm3', pool_name) - create_discovered_vm('vm4', pool_name) - create_migrating_vm('vm5', pool_name) + allow(subject).to receive(:create_inventory).and_raise( + RuntimeError,'Mock Error' + ) - expect(subject).to receive(:move_vm_queue).exactly(0).times - expect(subject).to receive(:check_running_vm).exactly(0).times - expect(subject).to receive(:check_pending_vm).exactly(0).times - expect(subject).to receive(:destroy_vm).exactly(0).times - expect(redis).to receive(:srem).exactly(0).times - expect(redis).to receive(:del).exactly(0).times - expect(redis).to receive(:hdel).exactly(0).times - expect(redis).to receive(:smove).exactly(0).times - expect(subject).to receive(:migrate_vm).exactly(0).times - expect(redis).to receive(:set).exactly(0).times - expect(redis).to receive(:incr).exactly(0).times - expect(subject).to receive(:clone_vm).exactly(0).times - expect(redis).to receive(:decr).exactly(0).times + expect { + subject._check_pool(pool_object, provider) + }.to raise_error(RuntimeError, /Mock Error/) - expect(provider).to receive(:vms_in_pool).and_raise(RuntimeError,'Mock Error') - subject._check_pool(pool_object,provider) + expect(subject).to_not receive(:check_running_pool_vms) end it 'should return that no actions were taken' do