From 5cd804f742be40b7c4af6e050322a72b3ba29a2d Mon Sep 17 00:00:00 2001 From: Daniel Berger Date: Fri, 14 Jun 2019 19:01:02 -0400 Subject: [PATCH 1/5] Use authentication_check and skip ssh connection if last valid within 15 minutes. Update specs to reflect changes in eligible? method. Add v2v tag to InfraConversionThrottler specs. Update host and vm in specs to be provider-specific, change custom task name to avoid deprecation warnings. Stub the authentication_check method. Add more specs for verify_credentials to validate that ssh calls are not made if already valid within last 15 minutes. --- app/models/conversion_host.rb | 8 ++++++-- lib/infra_conversion_throttler.rb | 12 +++-------- spec/lib/infra_conversion_throttler_spec.rb | 14 +++++++------ spec/models/conversion_host_spec.rb | 22 +++++++++++++++++---- 4 files changed, 35 insertions(+), 21 deletions(-) diff --git a/app/models/conversion_host.rb b/app/models/conversion_host.rb index e39afc2bda6..047fca2b76f 100644 --- a/app/models/conversion_host.rb +++ b/app/models/conversion_host.rb @@ -46,7 +46,7 @@ class ConversionHost < ApplicationRecord # TODO: Use the verify_credentials_ssh method in host.rb? Move that to the # AuthenticationMixin? # - def verify_credentials(auth_type = nil, options = {}) + def verify_credentials(auth_type = 'v2v', options = {}) if authentications.empty? check_ssh_connection else @@ -68,6 +68,10 @@ def verify_credentials(auth_type = nil, options = {}) raise MiqException::MiqInvalidCredentialsError, _("Unknown auth type: #{auth.authtype}") end + # Don't connect again if the authentication was valid within the last 15 + # minutes. This helps reduce unnecessary ssh connections. + return true if auth.last_valid_on && auth.last_valid_on > 15.minutes.ago + # Options from STI subclasses will override the defaults we've set above. ssh_options.merge!(options) @@ -91,7 +95,7 @@ def verify_credentials(auth_type = nil, options = {}) # - The number of concurrent tasks has not reached the limit. # def eligible? - source_transport_method.present? && verify_credentials && check_concurrent_tasks + source_transport_method.present? && authentication_check('v2v').first && check_concurrent_tasks end # Returns a boolean indicating whether or not the current number of active tasks diff --git a/lib/infra_conversion_throttler.rb b/lib/infra_conversion_throttler.rb index fda03effb28..49fb388b2cf 100644 --- a/lib/infra_conversion_throttler.rb +++ b/lib/infra_conversion_throttler.rb @@ -7,15 +7,9 @@ def self.start_conversions _log.debug("- EMS: #{ems.name}") _log.debug("-- Number of pending jobs: #{jobs.size}") running = ems.conversion_hosts.inject(0) { |sum, ch| sum + ch.active_tasks.count } - _log.debug("-- Currently running jobs in EMS: #{running}") - slots = (ems.miq_custom_get('Max Transformation Runners') || Settings.transformation.limits.max_concurrent_tasks_per_ems).to_i - running - - if slots <= 0 - _log.debug("-- No available slot in EMS. Stopping.") - next - end - _log.debug("-- Available slots in EMS: #{slots}") - + $log&.debug("There are currently #{running} conversion hosts running.") + slots = (ems.miq_custom_get('MaxTransformationRunners') || Settings.transformation.limits.max_concurrent_tasks_per_ems).to_i - running + $log&.debug("The maximum number of concurrent tasks for the EMS is: #{slots}.") jobs.each do |job| vm_name = job.migration_task.source.name _log.debug("- Looking for a conversion host for task for #{vm_name}") diff --git a/spec/lib/infra_conversion_throttler_spec.rb b/spec/lib/infra_conversion_throttler_spec.rb index dacefcfee61..ca45aff164f 100644 --- a/spec/lib/infra_conversion_throttler_spec.rb +++ b/spec/lib/infra_conversion_throttler_spec.rb @@ -1,7 +1,7 @@ -describe InfraConversionThrottler do - let(:ems) { FactoryBot.create(:ext_management_system, :zone => FactoryBot.create(:zone)) } - let(:host) { FactoryBot.create(:host, :ext_management_system => ems) } - let(:vm) { FactoryBot.create(:vm_or_template, :ext_management_system => ems) } +RSpec.describe InfraConversionThrottler, :v2v do + let(:ems) { FactoryBot.create(:ext_management_system, :zone => FactoryBot.create(:zone), :api_version => '4.2.4') } + let(:host) { FactoryBot.create(:host_redhat, :ext_management_system => ems) } + let(:vm) { FactoryBot.create(:vm_openstack, :ext_management_system => ems) } let(:task_waiting) { FactoryBot.create(:service_template_transformation_plan_task, :source => vm) } let(:task_running_1) { FactoryBot.create(:service_template_transformation_plan_task, :source => vm) } let(:task_running_2) { FactoryBot.create(:service_template_transformation_plan_task, :source => vm) } @@ -25,10 +25,12 @@ allow(ems).to receive(:conversion_hosts).and_return([conversion_host1, conversion_host2]) allow(conversion_host1).to receive(:check_ssh_connection).and_return(true) allow(conversion_host2).to receive(:check_ssh_connection).and_return(true) + allow(conversion_host1).to receive(:authentication_check).and_return([true, 'passed']) + allow(conversion_host2).to receive(:authentication_check).and_return([true, 'passed']) end it 'will not start a job when ems limit hit' do - ems.miq_custom_set('Max Transformation Runners', 2) + ems.miq_custom_set('MaxTransformationRunners', 2) allow(conversion_host1).to receive(:active_tasks).and_return([1]) allow(conversion_host2).to receive(:active_tasks).and_return([1]) expect(job_waiting).not_to receive(:queue_signal) @@ -36,7 +38,7 @@ end it 'will not start a job when conversion_host limit hit' do - ems.miq_custom_set('Max Transformation Runners', 100) + ems.miq_custom_set('MaxTransformationRunners', 100) allow(conversion_host1).to receive(:active_tasks).and_return([1, 2]) allow(conversion_host2).to receive(:active_tasks).and_return([1, 2]) expect(job_waiting).not_to receive(:queue_signal) diff --git a/spec/models/conversion_host_spec.rb b/spec/models/conversion_host_spec.rb index 4857f56adc1..033b36f44d6 100644 --- a/spec/models/conversion_host_spec.rb +++ b/spec/models/conversion_host_spec.rb @@ -25,28 +25,28 @@ context "#eligible?" do it "fails when no source transport method is enabled" do allow(conversion_host_1).to receive(:source_transport_method).and_return(nil) - allow(conversion_host_1).to receive(:verify_credentials).and_return(true) + allow(conversion_host_1).to receive(:authentication_check).and_return([true, 'worked']) allow(conversion_host_1).to receive(:check_concurrent_tasks).and_return(true) expect(conversion_host_1.eligible?).to eq(false) end it "fails when no source transport method is enabled" do allow(conversion_host_1).to receive(:source_transport_method).and_return('vddk') - allow(conversion_host_1).to receive(:verify_credentials).and_return(false) + allow(conversion_host_1).to receive(:authentication_check).and_return([false, 'failed']) allow(conversion_host_1).to receive(:check_concurrent_tasks).and_return(true) expect(conversion_host_1.eligible?).to eq(false) end it "fails when no source transport method is enabled" do allow(conversion_host_1).to receive(:source_transport_method).and_return('vddk') - allow(conversion_host_1).to receive(:verify_credentials).and_return(true) + allow(conversion_host_1).to receive(:authentication_check).and_return([true, 'worked']) allow(conversion_host_1).to receive(:check_concurrent_tasks).and_return(false) expect(conversion_host_1.eligible?).to eq(false) end it "succeeds when all criteria are met" do allow(conversion_host_1).to receive(:source_transport_method).and_return('vddk') - allow(conversion_host_1).to receive(:verify_credentials).and_return(true) + allow(conversion_host_1).to receive(:authentication_check).and_return([true, 'worked']) allow(conversion_host_1).to receive(:check_concurrent_tasks).and_return(true) expect(conversion_host_1.eligible?).to eq(true) end @@ -437,6 +437,20 @@ expect(conversion_host_vm.verify_credentials).to be_truthy end + it "makes an ssh call if the authentication was not valid within the last 15 minutes" do + authentication = FactoryBot.create(:authentication_ssh_keypair, :last_valid_on => Time.now.utc - 20.minutes) + conversion_host_vm.authentications << authentication + expect(Net::SSH).to receive(:start) + conversion_host_vm.verify_credentials + end + + it "does not make an ssh call if the authentication was valid within the last 15 minutes" do + authentication = FactoryBot.create(:authentication_ssh_keypair, :last_valid_on => Time.now.utc - 5.minutes) + conversion_host_vm.authentications << authentication + expect(Net::SSH).not_to receive(:start) + conversion_host_vm.verify_credentials + end + it "works as expected if there is an associated validation that is invalid" do authentication = FactoryBot.create(:authentication_ssh_keypair) conversion_host_vm.authentications << authentication From e74d09b9ac7fed4169cdcda3e260236dd28ada59 Mon Sep 17 00:00:00 2001 From: Daniel Berger Date: Thu, 20 Jun 2019 11:25:27 -0400 Subject: [PATCH 2/5] Reduce validation check from 15 minutes to 10 minutes, and continue with validation if invalid within last 10 minutes. --- app/models/conversion_host.rb | 8 +++++--- spec/models/conversion_host_spec.rb | 12 ++++++++++-- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/app/models/conversion_host.rb b/app/models/conversion_host.rb index 047fca2b76f..8ab546e75d5 100644 --- a/app/models/conversion_host.rb +++ b/app/models/conversion_host.rb @@ -68,9 +68,11 @@ def verify_credentials(auth_type = 'v2v', options = {}) raise MiqException::MiqInvalidCredentialsError, _("Unknown auth type: #{auth.authtype}") end - # Don't connect again if the authentication was valid within the last 15 - # minutes. This helps reduce unnecessary ssh connections. - return true if auth.last_valid_on && auth.last_valid_on > 15.minutes.ago + # Don't connect again if the authentication was valid (and also not invalid) + # within the last 10 minutes. This helps reduce unnecessary ssh connections. + if auth.last_valid_on && auth.last_valid_on > 10.minutes.ago + return true unless auth.last_invalid_on && auth.last_invalid_on > 10.minutes.ago + end # Options from STI subclasses will override the defaults we've set above. ssh_options.merge!(options) diff --git a/spec/models/conversion_host_spec.rb b/spec/models/conversion_host_spec.rb index 033b36f44d6..774e3a1c4c0 100644 --- a/spec/models/conversion_host_spec.rb +++ b/spec/models/conversion_host_spec.rb @@ -437,20 +437,28 @@ expect(conversion_host_vm.verify_credentials).to be_truthy end - it "makes an ssh call if the authentication was not valid within the last 15 minutes" do + it "makes an ssh call if the authentication was not valid within the last 10 minutes" do authentication = FactoryBot.create(:authentication_ssh_keypair, :last_valid_on => Time.now.utc - 20.minutes) conversion_host_vm.authentications << authentication expect(Net::SSH).to receive(:start) conversion_host_vm.verify_credentials end - it "does not make an ssh call if the authentication was valid within the last 15 minutes" do + it "does not make an ssh call if the authentication was valid within the last 10 minutes" do authentication = FactoryBot.create(:authentication_ssh_keypair, :last_valid_on => Time.now.utc - 5.minutes) conversion_host_vm.authentications << authentication expect(Net::SSH).not_to receive(:start) conversion_host_vm.verify_credentials end + it "does make an ssh call if the authentication was both valid and invalid within the last 10 minutes" do + authentication = FactoryBot.create(:authentication_ssh_keypair, :last_valid_on => Time.now.utc - 5.minutes) + authentication = FactoryBot.create(:authentication_ssh_keypair, :last_invalid_on => Time.now.utc - 7.minutes) + conversion_host_vm.authentications << authentication + expect(Net::SSH).to receive(:start) + conversion_host_vm.verify_credentials + end + it "works as expected if there is an associated validation that is invalid" do authentication = FactoryBot.create(:authentication_ssh_keypair) conversion_host_vm.authentications << authentication From 4385e1af2656d90ee4c4408a1f37fa5ea0c6734d Mon Sep 17 00:00:00 2001 From: Daniel Berger Date: Thu, 20 Jun 2019 11:31:01 -0400 Subject: [PATCH 3/5] Fix factory. --- spec/models/conversion_host_spec.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/models/conversion_host_spec.rb b/spec/models/conversion_host_spec.rb index 774e3a1c4c0..e4d06ce9e1c 100644 --- a/spec/models/conversion_host_spec.rb +++ b/spec/models/conversion_host_spec.rb @@ -452,8 +452,9 @@ end it "does make an ssh call if the authentication was both valid and invalid within the last 10 minutes" do - authentication = FactoryBot.create(:authentication_ssh_keypair, :last_valid_on => Time.now.utc - 5.minutes) - authentication = FactoryBot.create(:authentication_ssh_keypair, :last_invalid_on => Time.now.utc - 7.minutes) + authentication = FactoryBot.create(:authentication_ssh_keypair, + :last_valid_on => Time.now.utc - 5.minutes, + :last_invalid_on => Time.now.utc - 7.minutes) conversion_host_vm.authentications << authentication expect(Net::SSH).to receive(:start) conversion_host_vm.verify_credentials From b2de8484abb7ed382704a53e4e4545fcc571826e Mon Sep 17 00:00:00 2001 From: Daniel Berger Date: Mon, 24 Jun 2019 16:59:59 -0400 Subject: [PATCH 4/5] Use the authentication_status_ok? mixin method instead of rolling our own solution. --- app/models/conversion_host.rb | 7 ++----- spec/models/conversion_host_spec.rb | 14 ++++++-------- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/app/models/conversion_host.rb b/app/models/conversion_host.rb index 8ab546e75d5..454a6ae7620 100644 --- a/app/models/conversion_host.rb +++ b/app/models/conversion_host.rb @@ -68,11 +68,8 @@ def verify_credentials(auth_type = 'v2v', options = {}) raise MiqException::MiqInvalidCredentialsError, _("Unknown auth type: #{auth.authtype}") end - # Don't connect again if the authentication was valid (and also not invalid) - # within the last 10 minutes. This helps reduce unnecessary ssh connections. - if auth.last_valid_on && auth.last_valid_on > 10.minutes.ago - return true unless auth.last_invalid_on && auth.last_invalid_on > 10.minutes.ago - end + # Don't connect again if the authentication is still valid + return true if authentication_status_ok?(auth_type) # Options from STI subclasses will override the defaults we've set above. ssh_options.merge!(options) diff --git a/spec/models/conversion_host_spec.rb b/spec/models/conversion_host_spec.rb index e4d06ce9e1c..9d017523656 100644 --- a/spec/models/conversion_host_spec.rb +++ b/spec/models/conversion_host_spec.rb @@ -437,24 +437,22 @@ expect(conversion_host_vm.verify_credentials).to be_truthy end - it "makes an ssh call if the authentication was not valid within the last 10 minutes" do - authentication = FactoryBot.create(:authentication_ssh_keypair, :last_valid_on => Time.now.utc - 20.minutes) + it "makes an ssh call if the authentication is not valid" do + authentication = FactoryBot.create(:authentication_ssh_keypair, :status => 'Error', :authtype => 'v2v') conversion_host_vm.authentications << authentication expect(Net::SSH).to receive(:start) conversion_host_vm.verify_credentials end - it "does not make an ssh call if the authentication was valid within the last 10 minutes" do - authentication = FactoryBot.create(:authentication_ssh_keypair, :last_valid_on => Time.now.utc - 5.minutes) + it "does not make an ssh call if the authentication is valid" do + authentication = FactoryBot.create(:authentication_ssh_keypair, :status => 'Valid', :authtype => 'v2v') conversion_host_vm.authentications << authentication expect(Net::SSH).not_to receive(:start) conversion_host_vm.verify_credentials end - it "does make an ssh call if the authentication was both valid and invalid within the last 10 minutes" do - authentication = FactoryBot.create(:authentication_ssh_keypair, - :last_valid_on => Time.now.utc - 5.minutes, - :last_invalid_on => Time.now.utc - 7.minutes) + it "makes an ssh call if the authentication status is not set" do + authentication = FactoryBot.create(:authentication_ssh_keypair, :status => nil, :authtype => 'v2v') conversion_host_vm.authentications << authentication expect(Net::SSH).to receive(:start) conversion_host_vm.verify_credentials From 51bca563276aed4824113d500d2dd8d61fc03a1a Mon Sep 17 00:00:00 2001 From: Daniel Berger Date: Mon, 8 Jul 2019 14:17:05 -0400 Subject: [PATCH 5/5] Get this thing back in sync with master. --- lib/infra_conversion_throttler.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/infra_conversion_throttler.rb b/lib/infra_conversion_throttler.rb index 49fb388b2cf..f36fd33717a 100644 --- a/lib/infra_conversion_throttler.rb +++ b/lib/infra_conversion_throttler.rb @@ -7,9 +7,15 @@ def self.start_conversions _log.debug("- EMS: #{ems.name}") _log.debug("-- Number of pending jobs: #{jobs.size}") running = ems.conversion_hosts.inject(0) { |sum, ch| sum + ch.active_tasks.count } - $log&.debug("There are currently #{running} conversion hosts running.") + _log.debug("-- Currently running jobs in EMS: #{running}") slots = (ems.miq_custom_get('MaxTransformationRunners') || Settings.transformation.limits.max_concurrent_tasks_per_ems).to_i - running - $log&.debug("The maximum number of concurrent tasks for the EMS is: #{slots}.") + + if slots <= 0 + _log.debug("-- No available slot in EMS. Stopping.") + next + end + _log.debug("-- Available slots in EMS: #{slots}") + jobs.each do |job| vm_name = job.migration_task.source.name _log.debug("- Looking for a conversion host for task for #{vm_name}")