From 4ecd5dea5166c6743e1dbcac09c201afbbceef8f Mon Sep 17 00:00:00 2001 From: Samuel Date: Thu, 11 Jun 2020 12:39:34 -0500 Subject: [PATCH] (POOLER-174) Reduce duplicate of on demand code introduced in POOLER-158 (#383) * (POOLER-174) Reduce duplicate of on demand code introduced in POOLER-158 refactored every parsing of request of type 'pool_alias:pool:count' into a utility class, that is used by pool_manager and the api v1 class * add some metrics to the od request generation * fix rubocop offenses, we are now friends --- lib/vmpooler/api/v1.rb | 30 +++++++++++++++--------------- lib/vmpooler/pool_manager.rb | 9 +++------ lib/vmpooler/util/parsing.rb | 16 ++++++++++++++++ spec/unit/pool_manager_spec.rb | 8 ++++++++ 4 files changed, 42 insertions(+), 21 deletions(-) create mode 100644 lib/vmpooler/util/parsing.rb diff --git a/lib/vmpooler/api/v1.rb b/lib/vmpooler/api/v1.rb index 616841436..29406f848 100644 --- a/lib/vmpooler/api/v1.rb +++ b/lib/vmpooler/api/v1.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require 'vmpooler/util/parsing' + module Vmpooler class API class V1 < Sinatra::Base @@ -334,9 +336,10 @@ def sync_clone_targets end def too_many_requested?(payload) - payload&.each do |_poolname, count| + payload&.each do |poolname, count| next unless count.to_i > config['max_ondemand_instances_per_request'] + metrics.increment('ondemandrequest.toomanyrequests.' + poolname) return true end false @@ -360,6 +363,7 @@ def generate_ondemand_request(payload) if backend.exists("vmpooler__odrequest__#{request_id}") result['message'] = "request_id '#{request_id}' has already been created" status 409 + metrics.increment('ondemandrequest.generate.duplicaterequests') return result end @@ -383,6 +387,7 @@ def generate_ondemand_request(payload) result['domain'] = config['domain'] if config['domain'] result[:ok] = true + metrics.increment('ondemandrequest.generate.success') result end @@ -974,11 +979,9 @@ def check_ondemand_request(request_id) if request_hash['status'] == 'ready' result['ready'] = true - platform_parts = request_hash['requested'].split(',') - platform_parts.each do |platform| - pool_alias, pool, _count = platform.split(':') - instances = backend.smembers("vmpooler__#{request_id}__#{pool_alias}__#{pool}") - result[pool_alias] = { 'hostname': instances } + Parsing.get_platform_pool_count(request_hash['requested']) do |platform_alias, pool, _count| + instances = backend.smembers("vmpooler__#{request_id}__#{platform_alias}__#{pool}") + result[platform_alias] = { 'hostname': instances } end result['domain'] = config['domain'] if config['domain'] status 200 @@ -989,11 +992,9 @@ def check_ondemand_request(request_id) result['message'] = 'The request has been deleted' status 200 else - platform_parts = request_hash['requested'].split(',') - platform_parts.each do |platform| - pool_alias, pool, count = platform.split(':') - instance_count = backend.scard("vmpooler__#{request_id}__#{pool_alias}__#{pool}") - result[pool_alias] = { + Parsing.get_platform_pool_count(request_hash['requested']) do |platform_alias, pool, count| + instance_count = backend.scard("vmpooler__#{request_id}__#{platform_alias}__#{pool}") + result[platform_alias] = { 'ready': instance_count.to_s, 'pending': (count.to_i - instance_count.to_i).to_s } @@ -1017,12 +1018,11 @@ def delete_ondemand_request(request_id) else backend.hset("vmpooler__odrequest__#{request_id}", 'status', 'deleted') - platforms.split(',').each do |platform| - pool_alias, pool, _count = platform.split(':') - backend.smembers("vmpooler__#{request_id}__#{pool_alias}__#{pool}")&.each do |vm| + Parsing.get_platform_pool_count(platforms) do |platform_alias, pool, _count| + backend.smembers("vmpooler__#{request_id}__#{platform_alias}__#{pool}")&.each do |vm| backend.smove("vmpooler__running__#{pool}", "vmpooler__completed__#{pool}", vm) end - backend.del("vmpooler__#{request_id}__#{pool_alias}__#{pool}") + backend.del("vmpooler__#{request_id}__#{platform_alias}__#{pool}") end backend.expire("vmpooler__odrequest__#{request_id}", 129_600_0) end diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb index b694e5b58..bed6a4882 100644 --- a/lib/vmpooler/pool_manager.rb +++ b/lib/vmpooler/pool_manager.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'vmpooler/providers' +require 'vmpooler/util/parsing' require 'spicy-proton' require 'resolv' # ruby standard lib @@ -1500,9 +1501,7 @@ def process_ondemand_vms(redis) def vms_ready?(request_id, redis) catch :request_not_ready do request_hash = redis.hgetall("vmpooler__odrequest__#{request_id}") - requested_platforms = request_hash['requested'].split(',') - requested_platforms.each do |platform| - platform_alias, pool, count = platform.split(':') + Parsing.get_platform_pool_count(request_hash['requested']) do |platform_alias, pool, count| pools_filled = redis.scard("vmpooler__#{request_id}__#{platform_alias}__#{pool}") throw :request_not_ready unless pools_filled.to_i == count.to_i end @@ -1554,9 +1553,7 @@ def request_expired?(request_id, score, redis) def remove_vms_for_failed_request(request_id, expiration_ttl, redis) request_hash = redis.hgetall("vmpooler__odrequest__#{request_id}") - requested_platforms = request_hash['requested'].split(',') - requested_platforms.each do |platform| - platform_alias, pool, _count = platform.split(':') + Parsing.get_platform_pool_count(request_hash['requested']) do |platform_alias, pool, _count| pools_filled = redis.smembers("vmpooler__#{request_id}__#{platform_alias}__#{pool}") redis.pipelined do pools_filled&.each do |vm| diff --git a/lib/vmpooler/util/parsing.rb b/lib/vmpooler/util/parsing.rb new file mode 100644 index 000000000..8a8273fa9 --- /dev/null +++ b/lib/vmpooler/util/parsing.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +# utility class shared between apps +module Vmpooler + class Parsing + def self.get_platform_pool_count(requested, &_block) + requested_platforms = requested.split(',') + requested_platforms.each do |platform| + platform_alias, pool, count = platform.split(':') + raise ArgumentError if platform_alias.nil? || pool.nil? || count.nil? + + yield platform_alias, pool, count + end + end + end +end diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb index e886138be..8e75f454f 100644 --- a/spec/unit/pool_manager_spec.rb +++ b/spec/unit/pool_manager_spec.rb @@ -4872,6 +4872,14 @@ end end + it 'fails if the request is not valid' do + redis_connection_pool.with do |redis| + request_id = "#{request_id}-wrong" + create_ondemand_request_for_test(request_id, current_time.to_i, "#{pool}:5", redis) + expect{subject.vms_ready?(request_id, redis)}.to raise_error(ArgumentError) + end + end + it 'returns false when vms for request_id are not ready' do redis_connection_pool.with do |redis| result = subject.vms_ready?(request_id, redis)