From f209c2b8303025d89fc96f25bcd89caa9f6fcae1 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Fri, 1 Sep 2017 16:21:44 -0700 Subject: [PATCH 1/2] (GH-226) Respond quickly to VMs being consumed Previously in commit 9b0e55f9596a the looping period was changed from a static number to a dynamic one depending on load, however this meant that the operation to refill a pool was slowed down somewhat. While not a problem under normal loads, when a pool was quickly consumed, the pool manager may not respond quickly enough to refill the pool. This commit: - Changes the sleep method, to us a helper sleep method that will wakeup periodically and evaluate other wakeup events. This could be used later to exist sleep loops when pooler is shutting down to stop blocking threads - By default the wakeup_period is set to the minimum pool check loop time, thus emulating the behaviour prior to commit 9b0e55f9596a - Adds tests for the behaviour --- lib/vmpooler/pool_manager.rb | 41 ++++++++++++++- spec/unit/pool_manager_spec.rb | 92 +++++++++++++++++++++++++++++----- 2 files changed, 119 insertions(+), 14 deletions(-) diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb index 9c4d78a..533c78d 100644 --- a/lib/vmpooler/pool_manager.rb +++ b/lib/vmpooler/pool_manager.rb @@ -521,6 +521,45 @@ module Vmpooler finish end + # Helper method mainly used for unit testing + def time_passed?(_event, time) + Time.now > time + end + + # Possible wakeup events + # :pool_size_change + # - Fires when the number of ready VMs changes due to being consumed. + # - Additional options + # :poolname + # + def sleep_with_wakeup_events(loop_delay, wakeup_period = 5, options = {}) + exit_by = Time.now + loop_delay + wakeup_by = Time.now + wakeup_period + return if time_passed?(:exit_by, exit_by) + + if options[:pool_size_change] + initial_ready_size = $redis.scard("vmpooler__ready__#{options[:poolname]}") + end + + loop do + sleep(1) + break if time_passed?(:exit_by, exit_by) + + # Check for wakeup events + if time_passed?(:wakeup_by, wakeup_by) + wakeup_by = Time.now + wakeup_period + + # Wakeup if the number of ready VMs has changed + if options[:pool_size_change] + ready_size = $redis.scard("vmpooler__ready__#{options[:poolname]}") + break unless ready_size == initial_ready_size + end + end + + break if time_passed?(:exit_by, exit_by) + end + end + def check_pool(pool, maxloop = 0, loop_delay_min = CHECK_LOOP_DELAY_MIN_DEFAULT, @@ -551,7 +590,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(loop_delay) + sleep_with_wakeup_events(loop_delay, loop_delay_min, pool_size_change: true, poolname: pool['name']) unless maxloop.zero? break if loop_count >= maxloop diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb index 3cc6887..0546221 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/spec/unit/pool_manager_spec.rb @@ -2024,7 +2024,61 @@ EOT end end - describe "#check_pool" do + describe "#sleep_with_wakeup_events" do + let(:loop_delay) { 10 } + before(:each) do + allow(Kernel).to receive(:sleep).and_raise("sleep should not be called") + allow(subject).to receive(:time_passed?).with(:wakeup_by, Time).and_call_original + allow(subject).to receive(:time_passed?).with(:exit_by, Time).and_call_original + end + + it 'should not sleep if the loop delay is negative' do + expect(subject).to receive(:sleep).exactly(0).times + + subject.sleep_with_wakeup_events(-1) + end + + it 'should sleep until the loop delay is exceeded' do + # This test is a little brittle as it requires knowledge of the implementation + # Basically the number of sleep delays will dictate how often the time_passed? method is called + + expect(subject).to receive(:sleep).exactly(2).times + expect(subject).to receive(:time_passed?).with(:exit_by, Time).and_return(false, false, false, true) + + subject.sleep_with_wakeup_events(loop_delay) + end + + describe 'with the pool_size_change wakeup option' do + let(:wakeup_option) {{ + :pool_size_change => true, + :poolname => pool, + }} + let(:wakeup_period) { -1 } # A negative number forces the wakeup evaluation to always occur + + it 'should check the number of VMs ready in Redis' do + expect(subject).to receive(:time_passed?).with(:exit_by, Time).and_return(false, true) + expect(redis).to receive(:scard).with("vmpooler__ready__#{pool}").once + + subject.sleep_with_wakeup_events(loop_delay, wakeup_period, wakeup_option) + end + + it 'should sleep until the number of VMs ready in Redis increases' do + expect(subject).to receive(:sleep).exactly(3).times + expect(redis).to receive(:scard).with("vmpooler__ready__#{pool}").and_return(1,1,1,2) + + subject.sleep_with_wakeup_events(loop_delay, wakeup_period, wakeup_option) + end + + it 'should sleep until the number of VMs ready in Redis decreases' do + expect(subject).to receive(:sleep).exactly(3).times + expect(redis).to receive(:scard).with("vmpooler__ready__#{pool}").and_return(2,2,2,1) + + subject.sleep_with_wakeup_events(loop_delay, wakeup_period, wakeup_option) + end + end + end + + describe "#check_pool" do let(:threads) {{}} let(:provider_name) { 'mock_provider' } let(:config) { @@ -2093,7 +2147,19 @@ EOT end it 'when a non-default loop delay is specified' do - expect(subject).to receive(:sleep).with(loop_delay).exactly(maxloop).times + expect(subject).to receive(:sleep_with_wakeup_events).with(loop_delay, Numeric, Hash).exactly(maxloop).times + + subject.check_pool(pool_object,maxloop,loop_delay,loop_delay) + end + + it 'specifies a wakeup option for pool size change' do + expect(subject).to receive(:sleep_with_wakeup_events).with(loop_delay, Numeric, hash_including(:pool_size_change => true)).exactly(maxloop).times + + subject.check_pool(pool_object,maxloop,loop_delay,loop_delay) + end + + it 'specifies a wakeup option for poolname' do + expect(subject).to receive(:sleep_with_wakeup_events).with(loop_delay, Numeric, hash_including(:poolname => pool)).exactly(maxloop).times subject.check_pool(pool_object,maxloop,loop_delay,loop_delay) end @@ -2126,7 +2192,7 @@ EOT end it 'when a non-default loop delay is specified' do - expect(subject).to receive(:sleep).with(pool_loop_delay).exactly(maxloop).times + expect(subject).to receive(:sleep_with_wakeup_events).with(pool_loop_delay, pool_loop_delay, Hash).exactly(maxloop).times subject.check_pool(pool_object,maxloop,loop_delay) end @@ -2151,7 +2217,7 @@ EOT [:checked_pending_vms, :discovered_vms, :cloned_vms].each do |testcase| describe "when #{testcase} is greater than zero" do it "delays the minimum delay time" do - expect(subject).to receive(:sleep).with(loop_delay_min).exactly(maxloop).times + expect(subject).to receive(:sleep_with_wakeup_events).with(loop_delay_min, loop_delay_min, Hash).exactly(maxloop).times check_pool_response[testcase] = 1 subject.check_pool(pool_object,maxloop,loop_delay_min,loop_delay_max) @@ -2163,10 +2229,10 @@ EOT describe "when #{testcase} is greater than zero" do let(:loop_decay) { 3.0 } it "delays increases with a decay" do - expect(subject).to receive(:sleep).with(3).once - expect(subject).to receive(:sleep).with(9).once - expect(subject).to receive(:sleep).with(27).once - expect(subject).to receive(:sleep).with(60).twice + expect(subject).to receive(:sleep_with_wakeup_events).with(3, Numeric, Hash).once + expect(subject).to receive(:sleep_with_wakeup_events).with(9, Numeric, Hash).once + expect(subject).to receive(:sleep_with_wakeup_events).with(27, Numeric, Hash).once + expect(subject).to receive(:sleep_with_wakeup_events).with(60, Numeric, Hash).twice check_pool_response[testcase] = 1 subject.check_pool(pool_object,maxloop,loop_delay_min,loop_delay_max,loop_decay) @@ -2209,7 +2275,7 @@ EOT [:checked_pending_vms, :discovered_vms, :cloned_vms].each do |testcase| describe "when #{testcase} is greater than zero" do it "delays the minimum delay time" do - expect(subject).to receive(:sleep).with(pool_loop_delay_min).exactly(maxloop).times + expect(subject).to receive(:sleep_with_wakeup_events).with(pool_loop_delay_min, Numeric, Hash).exactly(maxloop).times check_pool_response[testcase] = 1 subject.check_pool(pool_object,maxloop,loop_delay_min,loop_delay_max,loop_decay) @@ -2220,10 +2286,10 @@ EOT [:checked_running_vms, :checked_ready_vms, :destroyed_vms, :migrated_vms].each do |testcase| describe "when #{testcase} is greater than zero" do it "delays increases with a decay" do - expect(subject).to receive(:sleep).with(7).once - expect(subject).to receive(:sleep).with(17).once - expect(subject).to receive(:sleep).with(42).once - expect(subject).to receive(:sleep).with(70).twice + expect(subject).to receive(:sleep_with_wakeup_events).with(7, Numeric, Hash).once + expect(subject).to receive(:sleep_with_wakeup_events).with(17, Numeric, Hash).once + expect(subject).to receive(:sleep_with_wakeup_events).with(42, Numeric, Hash).once + expect(subject).to receive(:sleep_with_wakeup_events).with(70, Numeric, Hash).twice check_pool_response[testcase] = 1 subject.check_pool(pool_object,maxloop,loop_delay_min,loop_delay_max,loop_decay) From 0840e11e71beff7b4bd2038b2cd7d986613cb902 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Tue, 19 Sep 2017 16:35:21 -0700 Subject: [PATCH 2/2] (maint) Pin nokogiri and redis due to old ruby versions Nokogiri and Redis gems have had recent releases which are not compatible with older ruby versions. This commit modifies the Gemfile to get the latest of each of these gems on modern ruby versions and pin to the older gem versions for older ruby engines. --- Gemfile | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/Gemfile b/Gemfile index aa427e5..580fde5 100644 --- a/Gemfile +++ b/Gemfile @@ -12,15 +12,34 @@ gem 'puma', '>= 3.6.0' gem 'rack', '~> 1.6' gem 'rake', '>= 10.4' gem 'rbvmomi', '>= 1.8' -if RUBY_VERSION =~ /^1\.9\./ - gem 'nokogiri', '< 1.7.0' -end -gem 'redis', '>= 3.2' gem 'sinatra', '>= 1.4' gem 'net-ldap', '<= 0.12.1' # keep compatibility w/ jruby & mri-1.9.3 gem 'statsd-ruby', '>= 1.3.0', :require => 'statsd' gem 'connection_pool', '>= 2.2.1' +# Pin gems against Ruby version +# Note we can't use platform restrictions easily so use +# lowest version range any platform +# ---- +# nokogiri +# redis +if RUBY_VERSION =~ /^1\.9\./ + gem 'nokogiri', '~> 1.6.0' + gem 'redis', '~> 3.0' +elsif RUBY_VERSION =~ /^2\.[0]/ + gem 'nokogiri', '~> 1.6.0' + gem 'redis', '~> 3.0' +elsif RUBY_VERSION =~ /^2\.[1]/ + gem 'nokogiri', '~> 1.7.0' + gem 'redis', '~> 3.0' +elsif RUBY_VERSION =~ /^2\.2\.[01]/ + gem 'nokogiri', "~> 1.7" + gem 'redis', '~> 3.0' +else + gem 'nokogiri', "~> 1.7" + gem 'redis', '>= 3.2' +end + # Test deps group :test do gem 'mock_redis', '>= 0.17.0'