Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(POOLER-129) Allow setting weights for backends #298

Merged
merged 4 commits into from
Sep 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
@@ -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'
Expand Down
5 changes: 3 additions & 2 deletions lib/vmpooler.rb
Original file line number Diff line number Diff line change
@@ -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}"
Expand Down
35 changes: 29 additions & 6 deletions lib/vmpooler/api/v1.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,38 @@ def need_token!
end

def fetch_single_vm(template)
vm = backend.spop('vmpooler__ready__' + template)
return [vm, template] if vm

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')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add this backend_weight to the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove and re-add it at the beginning of the array?

Copy link
Contributor

@sbeaulie sbeaulie Sep 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm following the logic now. Why not capture the selection here and in the else below. Then instead of doing template_backends.each on line 75 again, just backend.spop() the selection?

Copy link
Contributor Author

@mattkirby mattkirby Sep 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My goal is to order the array so the first VM we try to retrieve is based on the selection from pickup. There is an each statement because we need to try this for the next one in the list in case there are no ready VMs for the first selection. If no pool or matching alias can provide the VM only then will it return nil.

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

Expand Down
4 changes: 3 additions & 1 deletion lib/vmpooler/pool_manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,10 @@ 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?
return if hostname.empty?
return if hostname == vm
$redis.smove('vmpooler__ready__' + pool['name'], 'vmpooler__completed__' + pool['name'], vm)
Expand Down Expand Up @@ -870,7 +872,7 @@ def check_ready_pool_vms(pool_name, provider, pool_check_response, inventory, po
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
Expand Down
2 changes: 1 addition & 1 deletion lib/vmpooler/providers/vsphere.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like an unrelated change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I previously implemented the ability to specify a VM domain to use when opening a socket connection. Without this change that domain setting is never used. When interacting with a local instance of vmpooler in docker I found that I could not get VMs to reach the ready state without this addition. I could make this a separate commit if you think that makes more sense.

rescue => _err
return false
end
Expand Down
14 changes: 4 additions & 10 deletions spec/unit/pool_manager_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions spec/unit/providers/vsphere_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions vmpooler.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
11 changes: 11 additions & 0 deletions vmpooler.yaml.example
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -493,6 +501,9 @@
domain: 'example.com'
prefix: 'poolvm-'
experimental_features: true
backend_weight:
'backend1': 60
'backend2': 40

# :pools:
#
Expand Down