From 0e798c391d498852008552a38acd231d1570d4df Mon Sep 17 00:00:00 2001 From: "kirby@puppetlabs.com" Date: Mon, 11 May 2020 13:13:03 -0700 Subject: [PATCH] Add a configurable maximum per pool request upper limit --- lib/vmpooler.rb | 3 +- lib/vmpooler/api/v1.rb | 42 +++++--- lib/vmpooler/pool_manager.rb | 3 + spec/integration/api/v1/ondemandvm_spec.rb | 119 ++++++++++++--------- spec/unit/pool_manager_spec.rb | 7 ++ 5 files changed, 108 insertions(+), 66 deletions(-) diff --git a/lib/vmpooler.rb b/lib/vmpooler.rb index 5679087..e37f3eb 100644 --- a/lib/vmpooler.rb +++ b/lib/vmpooler.rb @@ -60,10 +60,11 @@ module Vmpooler # Set some configuration defaults parsed_config[:config]['task_limit'] = string_to_int(ENV['TASK_LIMIT']) || parsed_config[:config]['task_limit'] || 10 parsed_config[:config]['ondemand_clone_limit'] = string_to_int(ENV['ONDEMAND_CLONE_LIMIT']) || parsed_config[:config]['ondemand_clone_limit'] || 10 + parsed_config[:config]['max_ondemand_instances_per_request'] = string_to_int(ENV['MAX_ONDEMAND_INSTANCES_PER_REQUEST']) || parsed_config[:config]['max_ondemand_instances_per_request'] || 10 parsed_config[:config]['migration_limit'] = string_to_int(ENV['MIGRATION_LIMIT']) if ENV['MIGRATION_LIMIT'] parsed_config[:config]['vm_checktime'] = string_to_int(ENV['VM_CHECKTIME']) || parsed_config[:config]['vm_checktime'] || 1 parsed_config[:config]['vm_lifetime'] = string_to_int(ENV['VM_LIFETIME']) || parsed_config[:config]['vm_lifetime'] || 24 - parsed_config[:config]['max_lifetime_upper_limit'] = string_to_int(ENV['MAX_LIFETIME_UPPER_LIMIT']) || string_to_int(parsed_config[:config]['max_lifetime_upper_limit']) + parsed_config[:config]['max_lifetime_upper_limit'] = string_to_int(ENV['MAX_LIFETIME_UPPER_LIMIT']) || parsed_config[:config]['max_lifetime_upper_limit'] parsed_config[:config]['ready_ttl'] = string_to_int(ENV['READY_TTL']) || parsed_config[:config]['ready_ttl'] || 5 parsed_config[:config]['ondemand_request_ttl'] = string_to_int(ENV['ONDEMAND_REQUEST_TTL']) || parsed_config[:config]['ondemand_request_ttl'] || 5 parsed_config[:config]['prefix'] = ENV['PREFIX'] || parsed_config[:config]['prefix'] || '' diff --git a/lib/vmpooler/api/v1.rb b/lib/vmpooler/api/v1.rb index 188a5aa..aee4e93 100644 --- a/lib/vmpooler/api/v1.rb +++ b/lib/vmpooler/api/v1.rb @@ -307,11 +307,9 @@ module Vmpooler pool_index = pool_index(pools) template_configs = backend.hgetall('vmpooler__config__template') template_configs&.each do |poolname, template| - if pool_index.include? poolname - unless pools[pool_index[poolname]]['template'] == template - pools[pool_index[poolname]]['template'] = template - end - end + next unless pool_index.include? poolname + + pools[pool_index[poolname]]['template'] = template unless pools[pool_index[poolname]]['template'] == template end end @@ -319,11 +317,9 @@ module Vmpooler pool_index = pool_index(pools) poolsize_configs = backend.hgetall('vmpooler__config__poolsize') poolsize_configs&.each do |poolname, size| - if pool_index.include? poolname - unless pools[pool_index[poolname]]['size'] == size.to_i - pools[pool_index[poolname]]['size'] == size.to_i - end - end + next unless pool_index.include? poolname + + pools[pool_index[poolname]]['size'] == size.to_i unless pools[pool_index[poolname]]['size'] == size.to_i end end @@ -331,21 +327,33 @@ module Vmpooler pool_index = pool_index(pools) clone_target_configs = backend.hgetall('vmpooler__config__clone_target') clone_target_configs&.each do |poolname, clone_target| - if pool_index.include? poolname - unless pools[pool_index[poolname]]['clone_target'] == clone_target - pools[pool_index[poolname]]['clone_target'] == clone_target - end - end + next unless pool_index.include? poolname + + pools[pool_index[poolname]]['clone_target'] == clone_target unless pools[pool_index[poolname]]['clone_target'] == clone_target end end + def too_many_requested?(payload) + payload&.each do |_poolname, count| + next unless count.to_i > config['max_ondemand_instances_per_request'] + + return true + end + false + end + def generate_ondemand_request(payload) result = { 'ok': false } + if too_many_requested?(payload.reject { |k, _v| k == 'request_id' }) + result['message'] = "requested amount of instances exceeds the maximum #{config['max_ondemand_instances_per_request']}" + status 403 + return result + end + + score = Time.now.to_i request_id = payload['request_id'] request_id ||= generate_request_id - score = Time.now.to_i - result['request_id'] = request_id if backend.exists("vmpooler__odrequest__#{request_id}") diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb index 0fc5cc1..b74d2b2 100644 --- a/lib/vmpooler/pool_manager.rb +++ b/lib/vmpooler/pool_manager.rb @@ -1487,6 +1487,8 @@ 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) @@ -1494,6 +1496,7 @@ module Vmpooler 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 end diff --git a/spec/integration/api/v1/ondemandvm_spec.rb b/spec/integration/api/v1/ondemandvm_spec.rb index 34f5f49..07d5ad2 100644 --- a/spec/integration/api/v1/ondemandvm_spec.rb +++ b/spec/integration/api/v1/ondemandvm_spec.rb @@ -15,6 +15,7 @@ describe Vmpooler::API::V1 do config: { 'site_name' => 'test pooler', 'vm_lifetime_auth' => 2, + 'max_ondemand_instances_per_request' => 50, 'backend_weight' => { 'compute1' => 5 } @@ -49,63 +50,85 @@ describe Vmpooler::API::V1 do describe 'POST /ondemandvm' do context 'with a configured pool' do - it 'generates a request_id when none is provided' do - expect(SecureRandom).to receive(:uuid).and_return(uuid) - post "#{prefix}/ondemandvm", '{"pool1":"1"}' - expect_json(true, 201) - expected = { - "ok": true, - "request_id": uuid - } - expect(last_response.body).to eq(JSON.pretty_generate(expected)) + context 'with no request_id provided in payload' do + before(:each) do + expect(SecureRandom).to receive(:uuid).and_return(uuid) + end + + it 'generates a request_id when none is provided' do + post "#{prefix}/ondemandvm", '{"pool1":"1"}' + expect_json(true, 201) + + expected = { + "ok": true, + "request_id": uuid + } + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end + + it 'uses a configured platform to fulfill a ondemand request' do + post "#{prefix}/ondemandvm", '{"poolone":"1"}' + expect_json(true, 201) + expected = { + "ok": true, + "request_id": uuid + } + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end + + it 'creates a provisioning request in redis' do + expect(redis).to receive(:zadd).with('vmpooler__provisioning__request', Integer, uuid).and_return(1) + post "#{prefix}/ondemandvm", '{"poolone":"1"}' + end + + it 'sets a platform string in redis for the request to indicate selected platforms' do + expect(redis).to receive(:hset).with("vmpooler__odrequest__#{uuid}", 'requested', 'poolone:pool1:1') + post "#{prefix}/ondemandvm", '{"poolone":"1"}' + end end - it 'uses the given request_id when provided' do - post "#{prefix}/ondemandvm", '{"pool1":"1","request_id":"1234"}' - expect_json(true, 201) + context 'with a resource request that exceeds the specified limit' do + let(:max_instances) { 50 } + before(:each) do + config[:config]['max_ondemand_instances_per_request'] = max_instances + end - expected = { - "ok": true, - "request_id": "1234" - } - expect(last_response.body).to eq(JSON.pretty_generate(expected)) + it 'should reject the request with a message' do + post "#{prefix}/ondemandvm", '{"pool1":"51"}' + expect_json(false, 403) + expected = { + "ok": false, + "message": "requested amount of instances exceeds the maximum #{max_instances}" + } + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end end - it 'returns 409 conflict error when the request_id has been used' do - post "#{prefix}/ondemandvm", '{"pool1":"1","request_id":"1234"}' - post "#{prefix}/ondemandvm", '{"pool1":"1","request_id":"1234"}' - expect_json(false, 409) + context 'with request_id provided in the payload' do + it 'uses the given request_id when provided' do + post "#{prefix}/ondemandvm", '{"pool1":"1","request_id":"1234"}' + expect_json(true, 201) - expected = { - "ok": false, - "request_id": "1234", - "message": "request_id '1234' has already been created" - } - expect(last_response.body).to eq(JSON.pretty_generate(expected)) - end + expected = { + "ok": true, + "request_id": "1234" + } + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end - it 'uses a configured platform to fulfill a ondemand request' do - expect(SecureRandom).to receive(:uuid).and_return(uuid) - post "#{prefix}/ondemandvm", '{"poolone":"1"}' - expect_json(true, 201) - expected = { - "ok": true, - "request_id": uuid - } - expect(last_response.body).to eq(JSON.pretty_generate(expected)) - end + it 'returns 409 conflict error when the request_id has been used' do + post "#{prefix}/ondemandvm", '{"pool1":"1","request_id":"1234"}' + post "#{prefix}/ondemandvm", '{"pool1":"1","request_id":"1234"}' + expect_json(false, 409) - it 'creates a provisioning request in redis' do - expect(SecureRandom).to receive(:uuid).and_return(uuid) - expect(redis).to receive(:zadd).with('vmpooler__provisioning__request', Integer, uuid).and_return(1) - post "#{prefix}/ondemandvm", '{"poolone":"1"}' - end - - it 'sets a platform string in redis for the request to indicate selected platforms' do - expect(SecureRandom).to receive(:uuid).and_return(uuid) - expect(redis).to receive(:hset).with("vmpooler__odrequest__#{uuid}", 'requested', 'poolone:pool1:1') - post "#{prefix}/ondemandvm", '{"poolone":"1"}' + expected = { + "ok": false, + "request_id": "1234", + "message": "request_id '1234' has already been created" + } + expect(last_response.body).to eq(JSON.pretty_generate(expected)) + end end context 'with auth configured' do diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb index 192ccc2..9839634 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/spec/unit/pool_manager_spec.rb @@ -4901,6 +4901,13 @@ EOT 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)