From a9f82feeb177c29aeba9f86d348f12d9f59b6bc8 Mon Sep 17 00:00:00 2001 From: Daniel Berger Date: Mon, 3 Dec 2018 10:45:12 -0700 Subject: [PATCH 1/6] Add more robust error handling to ConversionHost#get_conversion_state. --- app/models/conversion_host.rb | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/app/models/conversion_host.rb b/app/models/conversion_host.rb index e17e95ea734..5eb27970c70 100644 --- a/app/models/conversion_host.rb +++ b/app/models/conversion_host.rb @@ -145,17 +145,18 @@ def kill_process(pid, signal = 'TERM') false end - # Get the conversion state by reading from a remote json file at +path+ - # and return the parsed data. - #-- - # TODO: This should be more clear on failure, since it could fail because - # it's not found or because the contents were invalid. + # Retrieve the conversion state information from a remote file as a stream. + # Then parse and return the stream data as a hash using JSON.parse. # def get_conversion_state(path) json_state = connect_ssh { |ssu| ssu.get_file(path, nil) } JSON.parse(json_state) - rescue => e - raise "Could not get state file '#{path}' from '#{resource.name}' with [#{e.class}: #{e}" + rescue JSON::ParserError => err + raise "Could not parse conversion state data from file '#{path}': #{json_state}" + rescue Net::SSH::Exception => err + raise "Failed to connect and retrieve conversion state data from file '#{path}' with [#{err.class}: #{err}" + rescue => err + raise "Error retrieving and parsing conversion state file '#{path}' from '#{resource.name}' with [#{err.class}: #{err}" end # Get and return the contents of the remote conversion log at +path+. @@ -253,8 +254,7 @@ def miq_ssh_util_args # TODO: Move this to ManageIQ::Providers::Redhat::InfraManager::ConversionHost # def miq_ssh_util_args_manageiq_providers_redhat_inframanager_host - authentication = find_credentials - [hostname || ipaddress, authentication.userid, authentication.password, nil, nil] + [hostname || ipaddress, resource.authentication_userid, resource.authentication_password, nil, nil] end # For the OpenStack provider, use the first authentication containing an ssh keypair that has From 716702f2bfebb295787b5c046bad7c8fd9f8cbb3 Mon Sep 17 00:00:00 2001 From: Daniel Berger Date: Mon, 3 Dec 2018 10:50:06 -0700 Subject: [PATCH 2/6] Check for ssh exception first. --- app/models/conversion_host.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/conversion_host.rb b/app/models/conversion_host.rb index 5eb27970c70..abc1d1bec96 100644 --- a/app/models/conversion_host.rb +++ b/app/models/conversion_host.rb @@ -151,10 +151,10 @@ def kill_process(pid, signal = 'TERM') def get_conversion_state(path) json_state = connect_ssh { |ssu| ssu.get_file(path, nil) } JSON.parse(json_state) - rescue JSON::ParserError => err - raise "Could not parse conversion state data from file '#{path}': #{json_state}" rescue Net::SSH::Exception => err raise "Failed to connect and retrieve conversion state data from file '#{path}' with [#{err.class}: #{err}" + rescue JSON::ParserError => err + raise "Could not parse conversion state data from file '#{path}': #{json_state}" rescue => err raise "Error retrieving and parsing conversion state file '#{path}' from '#{resource.name}' with [#{err.class}: #{err}" end From c5b93e6e24968c7da6435b4adf19aa411d9bf194 Mon Sep 17 00:00:00 2001 From: Daniel Berger Date: Mon, 3 Dec 2018 12:23:02 -0700 Subject: [PATCH 3/6] Address a couple rubocop issues. --- app/models/conversion_host.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/conversion_host.rb b/app/models/conversion_host.rb index abc1d1bec96..5381372e18d 100644 --- a/app/models/conversion_host.rb +++ b/app/models/conversion_host.rb @@ -153,9 +153,9 @@ def get_conversion_state(path) JSON.parse(json_state) rescue Net::SSH::Exception => err raise "Failed to connect and retrieve conversion state data from file '#{path}' with [#{err.class}: #{err}" - rescue JSON::ParserError => err + rescue JSON::ParserError raise "Could not parse conversion state data from file '#{path}': #{json_state}" - rescue => err + rescue StandardError => err raise "Error retrieving and parsing conversion state file '#{path}' from '#{resource.name}' with [#{err.class}: #{err}" end From 453feb49251c52421dd082633b81a9e354809be7 Mon Sep 17 00:00:00 2001 From: Daniel Berger Date: Tue, 26 Mar 2019 14:58:25 -0400 Subject: [PATCH 4/6] Add specs, rescue specific connection errors. --- app/models/conversion_host.rb | 2 +- spec/models/conversion_host_spec.rb | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/app/models/conversion_host.rb b/app/models/conversion_host.rb index 5381372e18d..864f5eb827b 100644 --- a/app/models/conversion_host.rb +++ b/app/models/conversion_host.rb @@ -151,7 +151,7 @@ def kill_process(pid, signal = 'TERM') def get_conversion_state(path) json_state = connect_ssh { |ssu| ssu.get_file(path, nil) } JSON.parse(json_state) - rescue Net::SSH::Exception => err + rescue MiqException::MiqInvalidCredentialsError, MiqException::MiqSshUtilHostKeyMismatch => err raise "Failed to connect and retrieve conversion state data from file '#{path}' with [#{err.class}: #{err}" rescue JSON::ParserError raise "Could not parse conversion state data from file '#{path}': #{json_state}" diff --git a/spec/models/conversion_host_spec.rb b/spec/models/conversion_host_spec.rb index ac26debe5e8..5cd5670a36d 100644 --- a/spec/models/conversion_host_spec.rb +++ b/spec/models/conversion_host_spec.rb @@ -309,4 +309,33 @@ expect(conversion_host_vm.verify_credentials('v2v')).to be_truthy end end + + context "#get_conversion_state" do + let(:vm) { FactoryBot.create(:vm_openstack) } + let(:conversion_host) { FactoryBot.create(:conversion_host, :resource => vm) } + let(:path) { 'some_path' } + + it "works as expected if the connection is successful and the JSON is valid" do + allow(conversion_host).to receive(:connect_ssh).and_return({:alpha => {:beta => 'hello'}}.to_json) + expect(conversion_host.get_conversion_state(path)).to eql({'alpha' => {'beta' => 'hello'}}) + end + + it "works as expected if the connection is successful but the JSON is invalid" do + allow(conversion_host).to receive(:connect_ssh).and_return('bogus') + expected_message = "Could not parse conversion state data from file '#{path}': bogus" + expect { conversion_host.get_conversion_state(path) }.to raise_error(expected_message) + end + + it "works as expected if the connection is unsuccessful" do + allow(conversion_host).to receive(:connect_ssh).and_raise(MiqException::MiqInvalidCredentialsError) + expected_message = "Failed to connect and retrieve conversion state data from file '#{path}'" + expect { conversion_host.get_conversion_state(path) }.to raise_error(/#{expected_message}/) + end + + it "works as expected if an unknown error occurs" do + allow(conversion_host).to receive(:connect_ssh).and_raise(StandardError) + expected_message = "Error retrieving and parsing conversion state file '#{path}' from '#{vm.name}'" + expect { conversion_host.get_conversion_state(path) }.to raise_error(/#{expected_message}/) + end + end end From 8894db9e8186c2f9b129e011d4fac23910a5be17 Mon Sep 17 00:00:00 2001 From: Daniel Berger Date: Tue, 26 Mar 2019 17:13:15 -0400 Subject: [PATCH 5/6] Undo a forced merge mistake. --- app/models/conversion_host.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/conversion_host.rb b/app/models/conversion_host.rb index 864f5eb827b..45eb7737b18 100644 --- a/app/models/conversion_host.rb +++ b/app/models/conversion_host.rb @@ -254,7 +254,8 @@ def miq_ssh_util_args # TODO: Move this to ManageIQ::Providers::Redhat::InfraManager::ConversionHost # def miq_ssh_util_args_manageiq_providers_redhat_inframanager_host - [hostname || ipaddress, resource.authentication_userid, resource.authentication_password, nil, nil] + authentication = find_credentials + [hostname || ipaddress, authentication.userid, authentication.password, nil, nil] end # For the OpenStack provider, use the first authentication containing an ssh keypair that has From e7482f1795c7d8afa8313d268b744971cbec3cd1 Mon Sep 17 00:00:00 2001 From: Daniel Berger Date: Wed, 27 Mar 2019 07:53:49 -0400 Subject: [PATCH 6/6] Rubocop fix. --- spec/models/conversion_host_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/conversion_host_spec.rb b/spec/models/conversion_host_spec.rb index 5cd5670a36d..3c714b0470b 100644 --- a/spec/models/conversion_host_spec.rb +++ b/spec/models/conversion_host_spec.rb @@ -317,7 +317,7 @@ it "works as expected if the connection is successful and the JSON is valid" do allow(conversion_host).to receive(:connect_ssh).and_return({:alpha => {:beta => 'hello'}}.to_json) - expect(conversion_host.get_conversion_state(path)).to eql({'alpha' => {'beta' => 'hello'}}) + expect(conversion_host.get_conversion_state(path)).to eql('alpha' => {'beta' => 'hello'}) end it "works as expected if the connection is successful but the JSON is invalid" do