From b831faf5c05c46c3459dfef52b8241e0d0879d86 Mon Sep 17 00:00:00 2001 From: "kirby@puppetlabs.com" Date: Thu, 21 May 2020 14:16:48 -0700 Subject: [PATCH] Check each ondemand request to see if it is ready when moving an instance to ready This commit updates how ondemand requests are checked to include a check to see if the request is ready when an instance is added to the ready instances set. Without this change the delay between a check for the instances being ready and the last instance becoming availalbe could cause the API to show all instances are ready, but the request not be considered ready yet. --- lib/vmpooler/pool_manager.rb | 29 +++++++---- spec/unit/pool_manager_spec.rb | 88 ++++++++++++++++++++++------------ 2 files changed, 77 insertions(+), 40 deletions(-) diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb index f386b1f..fca49c3 100644 --- a/lib/vmpooler/pool_manager.rb +++ b/lib/vmpooler/pool_manager.rb @@ -153,6 +153,7 @@ module Vmpooler redis.sadd("vmpooler__#{request_id}__#{pool_alias}__#{pool}", vm) end move_vm_queue(pool, vm, 'pending', 'running', redis) + check_ondemand_request_ready(request_id, redis) else redis.smove('vmpooler__pending__' + pool, 'vmpooler__ready__' + pool, vm) end @@ -1486,22 +1487,30 @@ module Vmpooler end def check_ondemand_requests_ready(redis) - # default expiration is one month to ensure the data does not stay in redis forever - default_expiration = 259_200_0 in_progress_requests = redis.zrange('vmpooler__provisioning__processing', 0, -1, with_scores: true) in_progress_requests&.each do |request_id, score| - next if request_expired?(request_id, score, redis) - next unless vms_ready?(request_id, redis) - - redis.multi - redis.hset("vmpooler__odrequest__#{request_id}", 'status', 'ready') - redis.expire("vmpooler__odrequest__#{request_id}", default_expiration) - redis.zrem('vmpooler__provisioning__processing', request_id) - redis.exec + check_ondemand_request_ready(request_id, redis, score) end in_progress_requests.length end + def check_ondemand_request_ready(request_id, redis, score = nil) + # default expiration is one month to ensure the data does not stay in redis forever + default_expiration = 259_200_0 + processing_key = 'vmpooler__provisioning__processing' + ondemand_hash_key = "vmpooler__odrequest__#{request_id}" + score ||= redis.zscore(processing_key, request_id) + return if request_expired?(request_id, score, redis) + + return unless vms_ready?(request_id, redis) + + redis.multi + redis.hset(ondemand_hash_key, 'status', 'ready') + redis.expire(ondemand_hash_key, default_expiration) + redis.zrem(processing_key, request_id) + redis.exec + end + def request_expired?(request_id, score, redis) delta = Time.now.to_i - score.to_i ondemand_request_ttl = $config[:config]['ondemand_request_ttl'] diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb index 03d4f76..ef667cb 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/spec/unit/pool_manager_spec.rb @@ -315,6 +315,11 @@ EOT context 'with request_id' do context 'with a pending request' do + before(:each) do + allow(subject).to receive(:check_ondemand_request_ready) + config[:config]['ondemand_request_ttl'] = 20 + end + it 'sets the vm as active' do redis_connection_pool.with do |redis| expect(Time).to receive(:now).and_return(current_time).at_least(:once) @@ -386,6 +391,8 @@ EOT let(:platforms_string) { "#{platform_alias}:#{pool}:1" } let(:score) { current_time.to_i } before(:each) do + config[:config]['ondemand_request_ttl'] = 20 + allow(subject).to receive(:check_ondemand_request_ready) redis_connection_pool.with do |redis| create_ondemand_request_for_test(request_id, score, platforms_string, redis, user, token) end @@ -4888,7 +4895,6 @@ EOT end describe '#check_ondemand_requests_ready' do - before(:each) do config[:config]['ondemand_request_ttl'] = 5 end @@ -4914,40 +4920,62 @@ EOT expect(result).to eq(1) end end + end + end - context 'when the request is ready' do - before(:each) do - expect(subject).to receive(:vms_ready?).and_return(true) - end + describe '#check_ondemand_request_ready' do + let(:score) { current_time.to_f } + before(:each) do + config[:config]['ondemand_request_ttl'] = 5 + end - it 'sets the request as ready' do - redis_connection_pool.with do |redis| - expect(redis).to receive(:hset).with("vmpooler__odrequest__#{request_id}", 'status', 'ready') - subject.check_ondemand_requests_ready(redis) - end - end - - it 'marks the ondemand request hash key for expiration in one month' do - redis_connection_pool.with do |redis| - expect(redis).to receive(:expire).with("vmpooler__odrequest__#{request_id}", 2592000) - subject.check_ondemand_requests_ready(redis) - end - end - - it 'removes the request from processing' do - redis_connection_pool.with do |redis| - expect(redis).to receive(:zrem).with('vmpooler__provisioning__processing', request_id) - subject.check_ondemand_requests_ready(redis) - end + context 'when the request is ready' do + before(:each) do + expect(subject).to receive(:vms_ready?).and_return(true) + redis_connection_pool.with do |redis| + expect(redis).to receive(:zscore).and_return(score) end end - context 'when a request has taken too long to be filled' do - it 'should return true for request_expired?' do - redis_connection_pool.with do |redis| - expect(subject).to receive(:request_expired?).with(request_id, Float, redis).and_return(true) - subject.check_ondemand_requests_ready(redis) - end + it 'sets the request as ready' do + redis_connection_pool.with do |redis| + expect(redis).to receive(:hset).with("vmpooler__odrequest__#{request_id}", 'status', 'ready') + subject.check_ondemand_request_ready(request_id, redis) + end + end + + it 'marks the ondemand request hash key for expiration in one month' do + redis_connection_pool.with do |redis| + expect(redis).to receive(:expire).with("vmpooler__odrequest__#{request_id}", 2592000) + subject.check_ondemand_request_ready(request_id, redis) + end + end + + it 'removes the request from processing' do + redis_connection_pool.with do |redis| + expect(redis).to receive(:zrem).with('vmpooler__provisioning__processing', request_id) + subject.check_ondemand_request_ready(request_id, redis) + end + end + end + + context 'with the score provided' do + it 'should not request the score' do + redis_connection_pool.with do |redis| + expect(redis).to_not receive(:zscore) + expect(subject).to receive(:vms_ready?).and_return(true) + expect(redis).to receive(:zrem).with('vmpooler__provisioning__processing', request_id) + subject.check_ondemand_request_ready(request_id, redis, score) + end + end + end + + context 'when a request has taken too long to be filled' do + it 'should return true for request_expired?' do + redis_connection_pool.with do |redis| + expect(redis).to receive(:zscore).and_return(score) + expect(subject).to receive(:request_expired?).with(request_id, Float, redis).and_return(true) + subject.check_ondemand_request_ready(request_id, redis) end end end