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 | 44 +++++++++++++++++++------------ lib/vmpooler/providers/vsphere.rb | 2 +- vmpooler.gemspec | 1 + 6 files changed, 69 insertions(+), 26 deletions(-) diff --git a/Gemfile b/Gemfile index 2b06870..1ce5e86 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 fe1480b..0b05599 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 9915a60..694a05e 100644 --- a/lib/vmpooler/api/v1.rb +++ b/lib/vmpooler/api/v1.rb @@ -36,16 +36,46 @@ module Vmpooler 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 d42bbb2..66ac8b6 100644 --- a/lib/vmpooler/pool_manager.rb +++ b/lib/vmpooler/pool_manager.rb @@ -149,25 +149,34 @@ module Vmpooler # 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']) + 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 - 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) + $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 + $logger.log('d', "[!] [#{pool['name']}] '#{vm}' reached end of TTL after #{ttl} minutes, removed from 'ready' queue") + return + end end + + stage = 'hostname mismatch' + return if has_mismatched_hostname?(vm, pool, provider) + + stage = 'still ready' + vm_still_ready?(pool['name'], vm, provider) end - - return if has_mismatched_hostname?(vm, pool, provider) - - vm_still_ready?(pool['name'], vm, provider) + rescue => err + $logger.log('s', "Failed at stage #{stage} for #{vm}") + raise end end @@ -190,6 +199,7 @@ module Vmpooler 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 @@ module Vmpooler 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 e139459..fd95ccf 100644 --- a/lib/vmpooler/providers/vsphere.rb +++ b/lib/vmpooler/providers/vsphere.rb @@ -428,7 +428,7 @@ module Vmpooler 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 4df9609..c8b0433 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 | 42 ++++++++++++----------------- spec/unit/pool_manager_spec.rb | 14 +++------- spec/unit/providers/vsphere_spec.rb | 5 ++-- 3 files changed, 24 insertions(+), 37 deletions(-) diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb index 66ac8b6..561d642 100644 --- a/lib/vmpooler/pool_manager.rb +++ b/lib/vmpooler/pool_manager.rb @@ -149,34 +149,25 @@ module Vmpooler # 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']) + 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 - 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) + $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 - end + $logger.log('d', "[!] [#{pool['name']}] '#{vm}' reached end of TTL after #{ttl} minutes, removed from 'ready' queue") + return end - - stage = 'hostname mismatch' - return if has_mismatched_hostname?(vm, pool, provider) - - stage = 'still ready' - vm_still_ready?(pool['name'], vm, provider) 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 @@ module Vmpooler # 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 @@ module Vmpooler 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 b8f144a..4e4ff96 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/spec/unit/pool_manager_spec.rb @@ -2974,9 +2974,7 @@ EOT # 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 @@ EOT 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 @@ EOT 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 d09f7f4..69134cd 100644 --- a/spec/unit/providers/vsphere_spec.rb +++ b/spec/unit/providers/vsphere_spec.rb @@ -925,9 +925,10 @@ EOT 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 @@ EOT # 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 8621612..7c847e3 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 694a05e..434e17e 100644 --- a/lib/vmpooler/api/v1.rb +++ b/lib/vmpooler/api/v1.rb @@ -36,13 +36,6 @@ module Vmpooler 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 2f4847f..c25e362 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: #