From cd73f53561acbdfd3883186fe8b603775a0d8673 Mon Sep 17 00:00:00 2001 From: "kirby@puppetlabs.com" Date: Tue, 11 Sep 2018 11:14:51 -0700 Subject: [PATCH 1/4] (POOLER-129) Allow setting weights for backends This commit updates get_vm in the vmpooler API to allow for setting weights for backends. Additionally, when an alias for a pool exists, and the backend configured is not weighted, then the selection of the pool based on alias will be randomly sampled. Without this change any pool with the title of the alias is exhausted before an alternate pool with the configured alias is used, which results in an uneven distribution of VMs. When all backends involved are configured with weighted values the VM selection will be based on probability using those weights. A bug is fixed when setting the default ttl for check_ready_vm. Pickup is added to handle weighted VM selection. A dockerfile is added that allows for building and installing vmpooler from the current HEAD in docker to make for easy testing. --- Gemfile | 1 + lib/vmpooler.rb | 5 ++-- lib/vmpooler/api/v1.rb | 42 ++++++++++++++++++++++++---- lib/vmpooler/pool_manager.rb | 46 +++++++++++++++++++------------ lib/vmpooler/providers/vsphere.rb | 2 +- vmpooler.gemspec | 1 + 6 files changed, 70 insertions(+), 27 deletions(-) diff --git a/Gemfile b/Gemfile index 2b06870fd..1ce5e865d 100644 --- a/Gemfile +++ b/Gemfile @@ -1,6 +1,7 @@ source ENV['GEM_SOURCE'] || 'https://rubygems.org' gem 'json', '>= 1.8' +gem 'pickup', '~> 0.0.11' gem 'puma', '~> 3.11' gem 'rack', '~> 2.0' gem 'rake', '~> 12.3' diff --git a/lib/vmpooler.rb b/lib/vmpooler.rb index fe1480bdd..0b05599a1 100644 --- a/lib/vmpooler.rb +++ b/lib/vmpooler.rb @@ -1,15 +1,16 @@ module Vmpooler require 'date' require 'json' - require 'open-uri' require 'net/ldap' + require 'open-uri' + require 'pickup' require 'rbvmomi' require 'redis' + require 'set' require 'sinatra/base' require 'time' require 'timeout' require 'yaml' - require 'set' %w[api graphite logger pool_manager statsd dummy_statsd generic_connection_pool].each do |lib| require "vmpooler/#{lib}" diff --git a/lib/vmpooler/api/v1.rb b/lib/vmpooler/api/v1.rb index 9915a603d..694a05e70 100644 --- a/lib/vmpooler/api/v1.rb +++ b/lib/vmpooler/api/v1.rb @@ -36,16 +36,46 @@ def need_token! validate_token(backend) end - def fetch_single_vm(template) - vm = backend.spop('vmpooler__ready__' + template) - return [vm, template] if vm + def pool_backend(pool) + pool_index = pool_index(pools) + pool_config = pools[pool_index[pool]] + backend = pool_config['clone_target'] || config['clone_target'] || 'default' + return backend + end + def fetch_single_vm(template) + template_backends = [template] aliases = Vmpooler::API.settings.config[:alias] - if aliases && aliased_template = aliases[template] - vm = backend.spop('vmpooler__ready__' + aliased_template) - return [vm, aliased_template] if vm + if aliases + template_backends << aliases[template] if aliases[template] + + pool_index = pool_index(pools) + weighted_pools = {} + template_backends.each do |t| + next unless pool_index.key? t + index = pool_index[t] + clone_target = pools[index]['clone_target'] || config['clone_target'] + next unless config.key?('backend_weight') + weight = config['backend_weight'][clone_target] + if weight + weighted_pools[t] = weight + end + end + + if weighted_pools.count == template_backends.count + pickup = Pickup.new(weighted_pools) + selection = pickup.pick + template_backends.delete(selection) + template_backends.unshift(selection) + else + template_backends = template_backends.sample(template_backends.count) + end end + template_backends.each do |t| + vm = backend.spop('vmpooler__ready__' + t) + return [vm, t] if vm + end [nil, nil] end diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb index d42bbb27a..66ac8b656 100644 --- a/lib/vmpooler/pool_manager.rb +++ b/lib/vmpooler/pool_manager.rb @@ -149,25 +149,34 @@ def _check_ready_vm(vm, pool, ttl, provider) # Periodically check that the VM is available 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']) - - $redis.hset('vmpooler__vm__' + vm, 'check', Time.now) - # 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['name'], 'vmpooler__completed__' + pool['name'], vm) - - $logger.log('d', "[!] [#{pool['name']}] '#{vm}' reached end of TTL after #{ttl} minutes, removed from 'ready' queue") - return + begin + mutex.synchronize do + stage = 'stamp check' + check_stamp = $redis.hget('vmpooler__vm__' + vm, 'check') + return if check_stamp && (((Time.now - Time.parse(check_stamp)) / 60) <= $config[:config]['vm_checktime']) + + $redis.hset('vmpooler__vm__' + vm, 'check', Time.now) + # Check if the hosts TTL has expired + stage = 'ttl' + 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['name'], 'vmpooler__completed__' + pool['name'], vm) + + $logger.log('d', "[!] [#{pool['name']}] '#{vm}' reached end of TTL after #{ttl} minutes, removed from 'ready' queue") + return + end end - end - return if has_mismatched_hostname?(vm, pool, provider) + stage = 'hostname mismatch' + return if has_mismatched_hostname?(vm, pool, provider) - vm_still_ready?(pool['name'], vm, provider) + stage = 'still ready' + vm_still_ready?(pool['name'], vm, provider) + end + rescue => err + $logger.log('s', "Failed at stage #{stage} for #{vm}") + raise end end @@ -190,6 +199,7 @@ def has_mismatched_hostname?(vm, pool, provider) vm_hash = provider.get_vm(pool['name'], vm) hostname = vm_hash['hostname'] + return if hostname.nil? return if hostname.empty? return if hostname == vm $redis.smove('vmpooler__ready__' + pool['name'], 'vmpooler__completed__' + pool['name'], vm) @@ -865,12 +875,12 @@ def check_running_pool_vms(pool_name, provider, pool_check_response, inventory) end end - def check_ready_pool_vms(pool_name, provider, pool_check_response, inventory, pool_ttl = 0) + def check_ready_pool_vms(pool_name, provider, pool_check_response, inventory, pool_ttl) $redis.smembers("vmpooler__ready__#{pool_name}").each do |vm| if inventory[vm] begin pool_check_response[:checked_ready_vms] += 1 - check_ready_vm(vm, pool_name, pool_ttl, provider) + check_ready_vm(vm, pool_name, pool_ttl || 0, provider) rescue => err $logger.log('d', "[!] [#{pool_name}] _check_pool failed with an error while evaluating ready VMs: #{err}") end diff --git a/lib/vmpooler/providers/vsphere.rb b/lib/vmpooler/providers/vsphere.rb index e139459d7..fd95ccf87 100644 --- a/lib/vmpooler/providers/vsphere.rb +++ b/lib/vmpooler/providers/vsphere.rb @@ -428,7 +428,7 @@ def destroy_vm(pool_name, vm_name) def vm_ready?(_pool_name, vm_name) begin - open_socket(vm_name) + open_socket(vm_name, global_config[:config]['domain']) rescue => _err return false end diff --git a/vmpooler.gemspec b/vmpooler.gemspec index 4df9609ca..c8b04335a 100644 --- a/vmpooler.gemspec +++ b/vmpooler.gemspec @@ -17,6 +17,7 @@ Gem::Specification.new do |s| s.bindir = 'bin' s.executables = 'vmpooler' s.require_paths = ["lib"] + s.add_dependency 'pickup', '~> 0.0.11' s.add_dependency 'puma', '~> 3.11' s.add_dependency 'rack', '~> 2.0' s.add_dependency 'rake', '~> 12.3' From 830acd705d7616343789204b8ceecf57b1d4b591 Mon Sep 17 00:00:00 2001 From: "kirby@puppetlabs.com" Date: Wed, 12 Sep 2018 12:28:12 -0700 Subject: [PATCH 2/4] Remove debug statements. Return when get_vm returns nil --- lib/vmpooler/pool_manager.rb | 44 ++++++++++++----------------- spec/unit/pool_manager_spec.rb | 14 +++------ spec/unit/providers/vsphere_spec.rb | 5 ++-- 3 files changed, 25 insertions(+), 38 deletions(-) diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb index 66ac8b656..561d6427c 100644 --- a/lib/vmpooler/pool_manager.rb +++ b/lib/vmpooler/pool_manager.rb @@ -149,34 +149,25 @@ def _check_ready_vm(vm, pool, ttl, provider) # Periodically check that the VM is available mutex = vm_mutex(vm) return if mutex.locked? - begin - mutex.synchronize do - stage = 'stamp check' - check_stamp = $redis.hget('vmpooler__vm__' + vm, 'check') - return if check_stamp && (((Time.now - Time.parse(check_stamp)) / 60) <= $config[:config]['vm_checktime']) - - $redis.hset('vmpooler__vm__' + vm, 'check', Time.now) - # Check if the hosts TTL has expired - stage = 'ttl' - 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['name'], 'vmpooler__completed__' + pool['name'], vm) - - $logger.log('d', "[!] [#{pool['name']}] '#{vm}' reached end of TTL after #{ttl} minutes, removed from 'ready' queue") - return - end - end + 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']) - stage = 'hostname mismatch' - return if has_mismatched_hostname?(vm, pool, provider) + $redis.hset('vmpooler__vm__' + vm, 'check', Time.now) + # 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['name'], 'vmpooler__completed__' + pool['name'], vm) - stage = 'still ready' - vm_still_ready?(pool['name'], vm, provider) + $logger.log('d', "[!] [#{pool['name']}] '#{vm}' reached end of TTL after #{ttl} minutes, removed from 'ready' queue") + return + end end - rescue => err - $logger.log('s', "Failed at stage #{stage} for #{vm}") - raise + + return if has_mismatched_hostname?(vm, pool, provider) + + vm_still_ready?(pool['name'], vm, provider) end end @@ -197,6 +188,7 @@ def has_mismatched_hostname?(vm, pool, provider) # Check if the hostname has magically changed from underneath Pooler vm_hash = provider.get_vm(pool['name'], vm) + return unless vm_hash.is_a? Hash hostname = vm_hash['hostname'] return if hostname.nil? @@ -875,7 +867,7 @@ def check_running_pool_vms(pool_name, provider, pool_check_response, inventory) end end - def check_ready_pool_vms(pool_name, provider, pool_check_response, inventory, pool_ttl) + def check_ready_pool_vms(pool_name, provider, pool_check_response, inventory, pool_ttl = 0) $redis.smembers("vmpooler__ready__#{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 b8f144a0e..4e4ff96bc 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/spec/unit/pool_manager_spec.rb @@ -2974,9 +2974,7 @@ # mock response from create_inventory {vm => 1} } - let(:big_lifetime) { - 2000 - } + let(:big_lifetime) { 2000 } before(:each) do allow(subject).to receive(:check_ready_vm) create_ready_vm(pool,vm,token) @@ -2992,7 +2990,7 @@ expect(subject).to receive(:check_ready_vm).and_raise(RuntimeError,'MockError') expect(logger).to receive(:log).with('d', "[!] [#{pool}] _check_pool failed with an error while evaluating ready VMs: MockError") - subject.check_ready_pool_vms(pool, provider, pool_check_response, inventory) + subject.check_ready_pool_vms(pool, provider, pool_check_response, inventory, big_lifetime) end it 'should use the pool TTL if set' do @@ -3511,12 +3509,8 @@ end it 'captures #create_inventory errors correctly' do - allow(subject).to receive(:create_inventory).and_raise( - RuntimeError,'Mock Error' - ) - expect { - subject._check_pool(pool_object, provider) - }.to_not raise_error(RuntimeError, /Mock Error/) + allow(subject).to receive(:create_inventory).and_raise(RuntimeError,'Mock Error') + subject._check_pool(pool_object, provider) end it 'should return early if an error occurs' do diff --git a/spec/unit/providers/vsphere_spec.rb b/spec/unit/providers/vsphere_spec.rb index d09f7f46b..69134cd46 100644 --- a/spec/unit/providers/vsphere_spec.rb +++ b/spec/unit/providers/vsphere_spec.rb @@ -925,9 +925,10 @@ end describe '#vm_ready?' do + let(:domain) { nil } context 'When a VM is ready' do before(:each) do - expect(subject).to receive(:open_socket).with(vmname) + expect(subject).to receive(:open_socket).with(vmname, domain) end it 'should return true' do @@ -939,7 +940,7 @@ # TODO not sure how to handle a VM that is passed in but # not located in the pool. Is that ready or not? before(:each) do - expect(subject).to receive(:open_socket).with(vmname) + expect(subject).to receive(:open_socket).with(vmname, domain) end it 'should return true' do From f5c3c86632d24f0a82593109d064a2edf9c3043f Mon Sep 17 00:00:00 2001 From: "kirby@puppetlabs.com" Date: Thu, 13 Sep 2018 09:24:17 -0700 Subject: [PATCH 3/4] Remove pool_backend method --- CHANGELOG.md | 1 + lib/vmpooler/api/v1.rb | 7 ------- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 86216124e..7c847e3d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ git logs & PR history. ### Added - Re-write check\_pool in pool\_manager to improve readability - Add a docker-compose file for testing vmpooler +- Add capability to weight backends when an alias spans multiple backends (POOLER-129) # [0.2.0](/~https://github.com/puppetlabs/vmpooler/compare/0.1.0...0.2.0) diff --git a/lib/vmpooler/api/v1.rb b/lib/vmpooler/api/v1.rb index 694a05e70..434e17e75 100644 --- a/lib/vmpooler/api/v1.rb +++ b/lib/vmpooler/api/v1.rb @@ -36,13 +36,6 @@ def need_token! validate_token(backend) end - def pool_backend(pool) - pool_index = pool_index(pools) - pool_config = pools[pool_index[pool]] - backend = pool_config['clone_target'] || config['clone_target'] || 'default' - return backend - end - def fetch_single_vm(template) template_backends = [template] aliases = Vmpooler::API.settings.config[:alias] From b1586393c74c85cce567f43c90f823e604e40be3 Mon Sep 17 00:00:00 2001 From: "kirby@puppetlabs.com" Date: Thu, 13 Sep 2018 09:32:20 -0700 Subject: [PATCH 4/4] Document backend_weight setting --- vmpooler.yaml.example | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/vmpooler.yaml.example b/vmpooler.yaml.example index 2f4847f7e..c25e3623b 100644 --- a/vmpooler.yaml.example +++ b/vmpooler.yaml.example @@ -477,6 +477,14 @@ # Expects a boolean value # (optional; default: false) # +# - backend_weight +# A hash of clone_target values with weights assigned to allow selecting VMs by alias with selection probability +# This setting is only used when there is a pool title and matching alias that both have values set in backend weight. +# When both conditions are met then the next VM is selected by probability using backend weight. When weight is not set +# in this configuration then distribution of load is random. +# Expects a hash value +# (optional) +# # Example: :config: @@ -493,6 +501,9 @@ domain: 'example.com' prefix: 'poolvm-' experimental_features: true + backend_weight: + 'backend1': 60 + 'backend2': 40 # :pools: #