(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

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require 'vmpooler/util/parsing'
+
 module Vmpooler
   class API
   class V1 < Sinatra::Base
@@ -334,9 +336,10 @@ module Vmpooler
     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 @@ module Vmpooler
       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 @@ module Vmpooler
 
       result['domain'] = config['domain'] if config['domain']
       result[:ok] = true
+      metrics.increment('ondemandrequest.generate.success')
       result
     end
 
@@ -974,11 +979,9 @@ module Vmpooler
 
       if request_hash['status'] == 'ready'
         result['ready'] = true
-        platform_parts = request_hash['requested'].split(',')
+        Parsing.get_platform_pool_count(request_hash['requested']) do |platform_alias, pool, _count|
-        platform_parts.each do |platform|
+          instances = backend.smembers("vmpooler__#{request_id}__#{platform_alias}__#{pool}")
-          pool_alias, pool, _count = platform.split(':')
+          result[platform_alias] = { 'hostname': instances }
-          instances = backend.smembers("vmpooler__#{request_id}__#{pool_alias}__#{pool}")
-          result[pool_alias] = { 'hostname': instances }
         end
         result['domain'] = config['domain'] if config['domain']
         status 200
@@ -989,11 +992,9 @@ module Vmpooler
         result['message'] = 'The request has been deleted'
         status 200
       else
-        platform_parts = request_hash['requested'].split(',')
+        Parsing.get_platform_pool_count(request_hash['requested']) do |platform_alias, pool, count|
-        platform_parts.each do |platform|
+          instance_count = backend.scard("vmpooler__#{request_id}__#{platform_alias}__#{pool}")
-          pool_alias, pool, count = platform.split(':')
+          result[platform_alias] = {
-          instance_count = backend.scard("vmpooler__#{request_id}__#{pool_alias}__#{pool}")
-          result[pool_alias] = {
             'ready': instance_count.to_s,
             'pending': (count.to_i - instance_count.to_i).to_s
           }
@@ -1017,12 +1018,11 @@ module Vmpooler
       else
         backend.hset("vmpooler__odrequest__#{request_id}", 'status', 'deleted')
 
-        platforms.split(',').each do |platform|
+        Parsing.get_platform_pool_count(platforms) do |platform_alias, pool, _count|
-          pool_alias, pool, _count = platform.split(':')
+          backend.smembers("vmpooler__#{request_id}__#{platform_alias}__#{pool}")&.each do |vm|
-          backend.smembers("vmpooler__#{request_id}__#{pool_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

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 @@ module Vmpooler
     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(',')
+        Parsing.get_platform_pool_count(request_hash['requested']) do |platform_alias, pool, count|
-        requested_platforms.each do |platform|
-          platform_alias, pool, count = platform.split(':')
           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 @@ module Vmpooler
 
     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(',')
+      Parsing.get_platform_pool_count(request_hash['requested']) do |platform_alias, pool, _count|
-      requested_platforms.each do |platform|
-        platform_alias, pool, _count = platform.split(':')
         pools_filled = redis.smembers("vmpooler__#{request_id}__#{platform_alias}__#{pool}")
         redis.pipelined do
           pools_filled&.each do |vm|

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

spec/unit/pool_manager_spec.rb

@@ -4872,6 +4872,14 @@ EOT
       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)