[PATCH 0/3 conductor] BZ 782574 - update error handling when iwhd/account provider is unavailable

[Tzu-Mainn Chen]

These patches change the behavior when iwhd/account providers are unavailable; instead of returning nil (or in some cases doing nothing), it raises a specific exception that contains the error, as well as a note to contact the site administrator.

Mainn

[PATCH 1/3] BZ 782574 add error messages when unable to connect to provider account/iwhd

[Tzu-Mainn Chen]

---
 src/config/locales/en.yml |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
index b5dfc05..24e2e12 100644
--- a/src/config/locales/en.yml
+++ b/src/config/locales/en.yml
@@ -788,6 +788,8 @@ en:
       back: Back
       save_template: Save Template
     not_on_provider: The requested image was not found on the provider.
+    errors:
+      could_not_connect: "Could not connect to Image Warehouse: %{message}.  Please contact an administrator."
   template_xml:
     errors:
       xml_parse_error: Failed to parse XML.
@@ -1097,6 +1099,7 @@ en:
     errors:
       populate_hardware_profiles_failed: "Failed to populate hardware_profiles: %{message}"
       populate_realms_failed: "Failed to populate realms: %{message}"
+      could_not_connect: "Could not connect to provider account: %{message}.  Please contact an administrator."
   config_servers:
     certificate_help: Provide a certificate to enable https support
     section_header:

[PATCH 2/3] BZ 782574 raise exception when unable to connect to iwhd/provider account

[Tzu-Mainn Chen]

---
 src/app/models/deployable.rb       |    2 ++
 src/app/models/instance.rb         |    4 ++++
 src/app/models/provider_account.rb |    2 +-
 3 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/src/app/models/deployable.rb b/src/app/models/deployable.rb
index c9df4c5..eba7e01 100644
--- a/src/app/models/deployable.rb
+++ b/src/app/models/deployable.rb
@@ -69,6 +69,8 @@ class Deployable < ActiveRecord::Base
   def fetch_images
     uuids = fetch_image_uuids || []
     uuids.map { |uuid| Aeolus::Image::Warehouse::Image.find(uuid) }
+  rescue Exception => e
+    raise I18n.t("images.errors.could_not_connect", :message => e.message)
   end

def fetch_image_uuids
diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
index 03bc119..cfa614a 100644
--- a/src/app/models/instance.rb
+++ b/src/app/models/instance.rb
@@ -179,10 +179,14 @@ class Instance < ActiveRecord::Base

def image
     Aeolus::Image::Warehouse::Image.find(image_uuid) if image_uuid
+  rescue Exception => e
+    raise I18n.t("images.errors.could_not_connect", :message => e.message)
   end

def image_build
     Aeolus::Image::Warehouse::ImageBuild.find(image_build_uuid) if image_build_uuid
+  rescue Exception => e
+    raise I18n.t("images.errors.could_not_connect", :message => e.message)
   end

def build
diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb
index 89009aa..e29a637 100644
--- a/src/app/models/provider_account.rb
+++ b/src/app/models/provider_account.rb
@@ -140,7 +140,7 @@ class ProviderAccount < ActiveRecord::Base
     rescue Exception => e
       logger.error("Error connecting to framework: #{e.message}")
       logger.error("Backtrace: #{e.backtrace.join("\n")}")
-      return nil
+      raise I18n.t("provider_accounts.errors.could_not_connect", :message => e.message)
     end
   end

[PATCH 2/3] BZ 782574 raise exception when unable to connect to iwhd/provider account

[Jan Provazník]

On 02/03/2012 08:48 PM, Tzu-Mainn Chen wrote:
> src/app/models/deployable.rb       |    2 ++
>   src/app/models/instance.rb         |    4 ++++
>   src/app/models/provider_account.rb |    2 +-
>   3 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/src/app/models/deployable.rb b/src/app/models/deployable.rb
> index c9df4c5..eba7e01 100644
> --- a/src/app/models/deployable.rb
> +++ b/src/app/models/deployable.rb
> @@ -69,6 +69,8 @@ class Deployable<  ActiveRecord::Base
>     def fetch_images
>       uuids = fetch_image_uuids || []
>       uuids.map { |uuid| Aeolus::Image::Warehouse::Image.find(uuid) }
>
> rescue Exception =>  e
>
> raise I18n.t("images.errors.could_not_connect", :message =>  e.message)
> end
>
> def fetch_image_uuids
>
> diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
> index 03bc119..cfa614a 100644
> --- a/src/app/models/instance.rb
> +++ b/src/app/models/instance.rb
> @@ -179,10 +179,14 @@ class Instance<  ActiveRecord::Base
> def image
>   Aeolus::Image::Warehouse::Image.find(image_uuid) if image_uuid
>
> rescue Exception =>  e
>
> raise I18n.t("images.errors.could_not_connect", :message =>  e.message)
> end
>
> def image_build
>  Aeolus::Image::Warehouse::ImageBuild.find(image_build_uuid) if image_build_uuid
>
> rescue Exception =>  e
>
> raise I18n.t("images.errors.could_not_connect", :message =>  e.message)
> end

Warehouse lib is now used on 40 places in conductor - so there are still 
many other not covered situations.

All warehouse connect requests are done by 'do_request' method in
aeolus-image-rubygem/lib/aeolus_image/model/warehouse/warehouse_client.rb and 
now it raises Errno::ECONNREFUSED exception.

The method could be updated to raise something like 
Warehouse::ConnectionRefused and then you can define "rescue_from 
Warehouse::ConnectionRefused" in application_controller.rb -> this could 
cover all warehouse connection problems.
> def build
>
> diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb
> index 89009aa..e29a637 100644
> --- a/src/app/models/provider_account.rb
> +++ b/src/app/models/provider_account.rb
> @@ -140,7 +140,7 @@ class ProviderAccount<  ActiveRecord::Base
>       rescue Exception =>  e
>         logger.error("Error connecting to framework: #{e.message}")
>         logger.error("Backtrace: #{e.backtrace.join("\n")}")
>  return nil
>  raise I18n.t("provider_accounts.errors.could_not_connect", :message =>  e.message)

Ideally a more specific exception type could be defined/raised (e.g. 
DC::ConnectionRefused).

There are some places in conductor where nil value is used to check if 
connect was successful. If this method newly raises an exception, all 
these places should be fixed too.

Through ProviderAccount is done only part of all deltacloud connections, 
there is also Provider#connect method which doeas same/similar thing - 
errors for provider connects should be handled in the same way.
>   end
> end

Jan

[PATCH 3/3] BZ 782574 update test so that a fail provider account connection results in an exception

[Tzu-Mainn Chen]

---
 src/spec/models/provider_account_spec.rb |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/spec/models/provider_account_spec.rb b/src/spec/models/provider_account_spec.rb
index 04b4b1e..83fb318 100644
--- a/src/spec/models/provider_account_spec.rb
+++ b/src/spec/models/provider_account_spec.rb
@@ -76,10 +76,10 @@ describe ProviderAccount do
     lambda {provider_account.save!}.should raise_exception
   end

-  it "when calling connect and it fails with exception it will return nil" do
+  it "when calling connect and it fails with exception it will raise an exception" do
     DeltaCloud.should_receive(:new).and_raise(Exception.new)

-    @provider_account.connect.should be_nil
+    lambda {@provider_account.connect}.should raise_exception
   end

it "should generate xml for a provider account with credentials" do