You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@deltacloud.apache.org by jv...@redhat.com on 2012/12/11 23:27:11 UTC
DTACLOUD-379 using Marios's 409 solution.
DTACLOUD-379 using Marios's 409 solution.
Re: DTACLOUD-379 using Marios's 409 solution.
Posted by "marios@redhat.com" <ma...@redhat.com>.
On 12/12/12 16:24, jvlcek wrote:
> On 12/11/2012 07:05 PM, David Lutterkort wrote:
>> On Tue, 2012-12-11 at 17:27 -0500, jvlcek@redhat.com wrote:
>>> DTACLOUD-379 using Marios's 409 solution.
>>>
>
> Thank you David,
>
>> ACK to this; stylistically, the two patches should be squashed into one,
>> since after applying 1/2 triggering this error will cause a server error
>> because of a missing template.
>
> My intention was to clearly show credit to Marios for the work he had done.
> Collapsing the patches into one would have made it appear that the
> entire solution was only my doing. That said I fully agree with your
> point about the one patch alone leading to issues.
hey - thanks Joe - just for future reference, this really wouldn't have
been a problem :) - though I appreciate the notion all the same. Plus,
my 'solution' was basically what you had come up with in the first place,
cheers, marios
>
>>
>> Also, the commit message is way too terse; it should contain a sentence
>> on what the problem was (RHEV-M does not allow deletion of templates
>> that are in use) and what the fix was (propagating the error cleanly)
>
> OK will do in the future.
>
>>
>> Related to this: looking at the XML template for image, we generate
>> links for 'create_instance' and 'delete_image' unconditionally. That
>> probably needs to be changed so we only create links for actions that
>> can currently be performed.
>
> I'm working another Jira for exactly the same issue on an
> Openstack template. https://issues.apache.org/jira/browse/DTACLOUD-346
>
> I'll add this issue to that work.
>
>
> Thanks for the feedback!
>
> Joe
>
Re: DTACLOUD-379 using Marios's 409 solution.
Posted by David Lutterkort <lu...@redhat.com>.
On Wed, 2012-12-12 at 09:24 -0500, jvlcek wrote:
> On 12/11/2012 07:05 PM, David Lutterkort wrote:
> > On Tue, 2012-12-11 at 17:27 -0500, jvlcek@redhat.com wrote:
> >> DTACLOUD-379 using Marios's 409 solution.
> >>
>
> Thank you David,
>
> > ACK to this; stylistically, the two patches should be squashed into one,
> > since after applying 1/2 triggering this error will cause a server error
> > because of a missing template.
>
> My intention was to clearly show credit to Marios for the work he had done.
> Collapsing the patches into one would have made it appear that the
> entire solution was only my doing. That said I fully agree with your
> point about the one patch alone leading to issues.
Wanting to give credit is very good - but as a rule of thumb,
bisectability beats credit ;) IOW, we want to make sure that all tests
can run through successfully after each patch.
David
Re: DTACLOUD-379 using Marios's 409 solution.
Posted by jvlcek <jv...@redhat.com>.
On 12/12/2012 09:24 AM, jvlcek wrote:
> On 12/11/2012 07:05 PM, David Lutterkort wrote:
>> On Tue, 2012-12-11 at 17:27 -0500, jvlcek@redhat.com wrote:
>>> DTACLOUD-379 using Marios's 409 solution.
>>>
> Thank you David,
>
>> ACK to this; stylistically, the two patches should be squashed into one,
>> since after applying 1/2 triggering this error will cause a server error
>> because of a missing template.
> My intention was to clearly show credit to Marios for the work he had done.
> Collapsing the patches into one would have made it appear that the
> entire solution was only my doing. That said I fully agree with your
> point about the one patch alone leading to issues.
>
>> Also, the commit message is way too terse; it should contain a sentence
>> on what the problem was (RHEV-M does not allow deletion of templates
>> that are in use) and what the fix was (propagating the error cleanly)
> OK will do in the future.
>
>> Related to this: looking at the XML template for image, we generate
>> links for 'create_instance' and 'delete_image' unconditionally. That
>> probably needs to be changed so we only create links for actions that
>> can currently be performed.
> I'm working another Jira for exactly the same issue on an
> Openstack template. https://issues.apache.org/jira/browse/DTACLOUD-346
>
> I'll add this issue to that work.
>
>
> Thanks for the feedback!
>
> Joe
NACK-ing
Michal Fojtik has pointed out this patch breaks tests.
I will fix the tests and resubmit the patch.
Joe
Re: DTACLOUD-379 using Marios's 409 solution.
Posted by jvlcek <jv...@redhat.com>.
On 12/11/2012 07:05 PM, David Lutterkort wrote:
> On Tue, 2012-12-11 at 17:27 -0500, jvlcek@redhat.com wrote:
>> DTACLOUD-379 using Marios's 409 solution.
>>
Thank you David,
> ACK to this; stylistically, the two patches should be squashed into one,
> since after applying 1/2 triggering this error will cause a server error
> because of a missing template.
My intention was to clearly show credit to Marios for the work he had done.
Collapsing the patches into one would have made it appear that the
entire solution was only my doing. That said I fully agree with your
point about the one patch alone leading to issues.
>
> Also, the commit message is way too terse; it should contain a sentence
> on what the problem was (RHEV-M does not allow deletion of templates
> that are in use) and what the fix was (propagating the error cleanly)
OK will do in the future.
>
> Related to this: looking at the XML template for image, we generate
> links for 'create_instance' and 'delete_image' unconditionally. That
> probably needs to be changed so we only create links for actions that
> can currently be performed.
I'm working another Jira for exactly the same issue on an
Openstack template. https://issues.apache.org/jira/browse/DTACLOUD-346
I'll add this issue to that work.
Thanks for the feedback!
Joe
Re: DTACLOUD-379 using Marios's 409 solution.
Posted by jvlcek <jv...@redhat.com>.
On 12/11/2012 07:05 PM, David Lutterkort wrote:
> On Tue, 2012-12-11 at 17:27 -0500, jvlcek@redhat.com wrote:
>> DTACLOUD-379 using Marios's 409 solution.
>>
> ACK to this; stylistically, the two patches should be squashed into one,
> since after applying 1/2 triggering this error will cause a server error
> because of a missing template.
>
> Also, the commit message is way too terse; it should contain a sentence
> on what the problem was (RHEV-M does not allow deletion of templates
> that are in use) and what the fix was (propagating the error cleanly)
>
> Related to this: looking at the XML template for image, we generate
> links for 'create_instance' and 'delete_image' unconditionally. That
> probably needs to be changed so we only create links for actions that
> can currently be performed.
>
> David
>
>
David,
Regarding the last paragraph in the above email. It's not clear
to me what conditions Deltacloud should be checking prior to
creating the links for image 'create_instance' and 'delete_image'.
I think it's back end specific. I know EC2 allows the deletion of
an in use AMI but RHEVm doesn't.
Can you please explain what you have in mind?
Thanks,
Joe
Re: DTACLOUD-379 using Marios's 409 solution.
Posted by David Lutterkort <lu...@redhat.com>.
On Tue, 2012-12-11 at 17:27 -0500, jvlcek@redhat.com wrote:
> DTACLOUD-379 using Marios's 409 solution.
>
ACK to this; stylistically, the two patches should be squashed into one,
since after applying 1/2 triggering this error will cause a server error
because of a missing template.
Also, the commit message is way too terse; it should contain a sentence
on what the problem was (RHEV-M does not allow deletion of templates
that are in use) and what the fix was (propagating the error cleanly)
Related to this: looking at the XML template for image, we generate
links for 'create_instance' and 'delete_image' unconditionally. That
probably needs to be changed so we only create links for actions that
can currently be performed.
David
[PATCH 2/2] DTACLOUD-379 using Marios's 409 solution.
Posted by jv...@redhat.com.
From: Joe VLcek <jv...@redhat.com>
---
server/lib/deltacloud/collections/base.rb | 4 ++
server/lib/deltacloud/helpers/deltacloud_helper.rb | 1 +
server/views/errors/409.html.haml | 47 ++++++++++++++++++++++
server/views/errors/409.xml.haml | 11 +++++
4 files changed, 63 insertions(+)
create mode 100644 server/views/errors/409.html.haml
create mode 100644 server/views/errors/409.xml.haml
diff --git a/server/lib/deltacloud/collections/base.rb b/server/lib/deltacloud/collections/base.rb
index 21a7e98..bc01eff 100644
--- a/server/lib/deltacloud/collections/base.rb
+++ b/server/lib/deltacloud/collections/base.rb
@@ -43,6 +43,10 @@ module Deltacloud::Collections
report_error
end
+ error Deltacloud::Exceptions::Conflict do
+ report_error
+ end
+
error Deltacloud::Exceptions::ValidationFailure do
report_error
end
diff --git a/server/lib/deltacloud/helpers/deltacloud_helper.rb b/server/lib/deltacloud/helpers/deltacloud_helper.rb
index 34519c0..bc4cdfc 100644
--- a/server/lib/deltacloud/helpers/deltacloud_helper.rb
+++ b/server/lib/deltacloud/helpers/deltacloud_helper.rb
@@ -245,6 +245,7 @@ module Deltacloud::Helpers
when 404; { :message => "Not Found" }
when 405; { :message => "Method Not Allowed" }
when 406; { :message => "Not Acceptable" }
+ when 409; { :message => "Resource Conflict" }
when 500; { :message => "Internal Server Error" }
when 502; { :message => "Backend Server Error" }
when 504; { :message => "Gateway Timeout" }
diff --git a/server/views/errors/409.html.haml b/server/views/errors/409.html.haml
new file mode 100644
index 0000000..fb9daca
--- /dev/null
+++ b/server/views/errors/409.html.haml
@@ -0,0 +1,47 @@
+%div{ :'data-role' => :content, :'data-theme' => 'b'}
+ %ul{ :'data-role' => :listview , :'data-inset' => :true, :'data-divider-theme' => 'e'}
+ %li{ :'data-role' => 'list-divider'} Server message
+ %li
+ %h3= h [@error.class.name, @error.message].join(' - ')
+ %li{ :'data-role' => 'list-divider'} Original request URI
+ %li
+ %a{ :href => request.env['REQUEST_URI'], :'data-ajax' => 'false'}
+ %span=request.env['REQUEST_URI']
+ %span{ :class => 'ui-li-count'} Retry
+ %li{ :'data-role' => 'list-divider'} Error details
+ %li
+ - if @error.class.method_defined? :details
+ %p= h @error.details
+ - else
+ %em No details
+ %li{ :'data-role' => 'list-divider'} Backtrace
+ %li
+ %em No details
+
+ - if @error.backtrace
+ %div{ 'data-role' => :collapsible, 'data-collapsed' => "true"}
+ %h3 Backtrace
+ %ul{ :'data-role' => :listview , :'data-inset' => :true, :'data-divider-theme' => 'e'}
+ %li
+ %pre= h @error.backtrace.join("\n")
+
+ %div{ 'data-role' => :collapsible, 'data-collapsed' => "true"}
+ %h3 Parameters
+ %ul{ :'data-role' => :listview , :'data-inset' => :true, :'data-divider-theme' => 'e'}
+ - if params.keys.empty?
+ %li{ :'data-role' => 'list-divider'} No parameters
+ - params.each do |key, value|
+ - next if value.inspect.to_s == '#'
+ %li{ :'data-role' => 'list-divider'}=key
+ %li
+ %span{:style => 'font-weight:normal;'}=value.inspect
+
+
+ %div{ 'data-role' => :collapsible, 'data-collapsed' => "true"}
+ %h3 Request details
+ %ul{ :'data-role' => :listview , :'data-inset' => :true, :'data-divider-theme' => 'e'}
+ - request.env.each do |key, value|
+ - next if value.inspect.to_s == '#'
+ %li{ :'data-role' => 'list-divider'}=key
+ %li
+ %span{:style => 'font-weight:normal;'}= h value.inspect
diff --git a/server/views/errors/409.xml.haml b/server/views/errors/409.xml.haml
new file mode 100644
index 0000000..de05bf8
--- /dev/null
+++ b/server/views/errors/409.xml.haml
@@ -0,0 +1,11 @@
+- unless defined?(partial)
+ !!! XML
+%error{:url => "#{request.env['REQUEST_URI']}", :status => "#{response.status}"}
+ %kind backend_error
+ %backend{ :driver => driver_symbol, :provider => "#{Thread::current[:provider] || ENV['API_PROVIDER'] || 'default'}" }
+ %code=response.status
+ %message< #{cdata @error.message}
+ - if params
+ %request
+ - params.each do |k, v|
+ %param{ :name => k}=v
--
1.7.11.7
[PATCH 1/2] Proposed fix for DTACLOUD-379
Posted by jv...@redhat.com.
From: marios <ma...@redhat.com>
https://issues.apache.org/jira/browse/DTACLOUD-379
Signed-off-by: marios <ma...@redhat.com>
TrackedAt: http://tracker-mfojtik.rhcloud.com/patch/6e465c4a530f4715cc11a2db14b84a1c349e513b
---
server/lib/deltacloud/drivers/exceptions.rb | 8 ++++++++
server/lib/deltacloud/drivers/rhevm/rhevm_driver.rb | 8 +++++---
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/server/lib/deltacloud/drivers/exceptions.rb b/server/lib/deltacloud/drivers/exceptions.rb
index 7bbf510..5663dae 100644
--- a/server/lib/deltacloud/drivers/exceptions.rb
+++ b/server/lib/deltacloud/drivers/exceptions.rb
@@ -58,6 +58,13 @@ module Deltacloud
end
end
+ class Conflict < DeltacloudException
+ def initialize(e, message=nil)
+ message ||= e.message
+ super(409, e.class.name, message, e.backtrace)
+ end
+ end
+
class MethodNotAllowed < DeltacloudException
def initialize(e, message=nil)
message ||= e.message
@@ -157,6 +164,7 @@ module Deltacloud
when 406 then UnknownMediaTypeError.new(e, @message)
when 405 then MethodNotAllowed.new(e, @message)
when 400 then ValidationFailure.new(e, @message)
+ when 409 then Conflict.new(e, @message)
when 500 then BackendError.new(e, @message)
when 501 then NotImplemented.new(e, @message)
when 502 then ProviderError.new(e, @message)
diff --git a/server/lib/deltacloud/drivers/rhevm/rhevm_driver.rb b/server/lib/deltacloud/drivers/rhevm/rhevm_driver.rb
index 37e6622..7709be4 100644
--- a/server/lib/deltacloud/drivers/rhevm/rhevm_driver.rb
+++ b/server/lib/deltacloud/drivers/rhevm/rhevm_driver.rb
@@ -118,9 +118,7 @@ class RhevmDriver < Deltacloud::BaseDriver
def destroy_image(credentials, image_id)
client = new_client(credentials)
safely do
- unless client.destroy_template(image_id)
- raise "ERROR: Unable to remove image"
- end
+ client.destroy_template(image_id)
end
end
@@ -342,6 +340,10 @@ class RhevmDriver < Deltacloud::BaseDriver
status 404
end
+ on /(Cannot delete Template. Template is being used)/ do
+ status 409
+ end
+
on /(RestClient|RHEVM|OVIRT)/ do
status 500
end
--
1.7.11.7