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/07 20:09:52 UTC
DTACLOUD-379 catch and handle in haml
Fix for DTACLOUD-379 by handling in haml emitting a backtrace for error:
"Cannot delete Template. Template is being used by the following VMs:"
Re: [PATCH] DTACLOUD-379 catch and handle in haml
Posted by "marios@redhat.com" <ma...@redhat.com>.
Hi Dies:
On 11/12/12 08:20, Koper, Dies wrote:
> Hi Marios,
>
> I like your patch but the return code you picked raises questions (seems
> to introduce inconsistencies).
thanks for checking that out.
>
> We used (still do?) return a 405 from the Deltacloud API when trying to
> start an instance that is already started, stopping an instance that is
> already stopped, etc., according to:
>
> https://issues.apache.org/jira/browse/DTACLOUD-214
>
> I don't understand how this case is any different.
>
Perhaps we need to look at the status codes returned from there - I mean
it may be that 405 was not the right code to be returning in those cases
- though changing them now may not be so easy as it would endanger
backwards compatibility.
In '405 Method not allowed' - Method is referring to the HTTP verb [1]
... i.e. GET, POST etc. and not 'restart'/'reboot'.
> I don't know the background of returning 405 for the case above, and the
> cases the spec writers envisioned.
> I read the 409 description (also due to Wikipedia's interpretation) to
> mean a case where the action would put the resource in an inconsistent
> state, and the client can change their request and retry to prevent the
> inconsistency.
I looked at wikipedia - though the definition there is just a one line
interpretation of the definition in RFC2616. Looking at that [1] I still
think 409 is more appropriate here.
In '409' - the 'resource inconsistency' ('edit conflict') is given as an
example and reading [1] again, it does say "most likely to occur in
response to a PUT request ... For example, if versioning were being used
and the entity being PUT included changes to a resource which conflict
with those made by an earlier (third-party) request,".
> In our case the action is not possible and applicable at all to the
> resource at the moment, and it requires more than changing the request
> to reach the intended outcome.
API action ('restart/reboot') vs 'GET/POST/PUT' - from [1] I understand
that 'method' in 'method not allowed' in about the verb.
409 is not about getting the client to change the request but rather
"This code is only allowed in situations where it is expected that the
user might be able to resolve the conflict and resubmit the request".
>
> In cimi (as in DC), we return actions that are allowed on resources. It
> is my understanding (from a discussion I had with Doug last week) that
> actions that are not allowed because the user doesn't have privileges or
> because the resource is not in the right state, are not included.
> In that case, it makes sense to me to return "405 Method Not Allowed" to
> an action not included in that list.
again, API actions vs HTTP verb. Not sure how to proceed - do you still
think 405 is more appropriate here? I understand your concern that "this
is what we return for instance state actions" but *in my opinion* and IF
that was an error, we shouldn't repeat it here.
On a final note - this is awesome! Having this much
scrutiny/interest/discussion about the status code returned for a
particular action can only mean good things for this project moving forward!
all the best, marios
[1] http://www.w3.org/Protocols/rfc2616/rfc2616.txt
>
> So I find the introduction of 409 for this particular case look
> inconsistent with what we do in other similar cases.
>
> Regards,
> Dies Koper
>
>
>> -----Original Message-----
>> From: marios@redhat.com [mailto:mandreou@redhat.com]
>> Sent: Monday, 10 December 2012 11:51 PM
>> To: dev@deltacloud.apache.org
>> Cc: Koper, Dies; Joe Vlcek
>> Subject: Re: [PATCH] DTACLOUD-379 catch and handle in haml
>>
>> Hi Dies - as always thanks for your input here - more inline:
>>
>> On 08/12/12 05:49, Koper, Dies wrote:
>>> Nack.
>>> I don't think it's a good idea to start putting messages specific to
> one
>>> provider in the common haml files.
>>> If we start doing that with all messages from all providers, it's
> going
>>> to be a big mess.
>>>
>>
>> yes indeed - and I have to claim ownership of this great idea :/ -
> Joe's
>> initial solution was to raise a new 5XX error and then catch this so
> it
>> silences the error logging. I didn't want to 'mess around' too much
> with
>> the error handling code, hence my 'do it in haml' suggestion. Looking
> at
>> it, you are right, its not the way to go.
>>
>>> Maybe the driver can raise a particular class of (common) error for
>>> which the backtrace is not logged?
>>>
>>> Before that, I don't think the driver should be raising a 5xx for
> this
>>> error. 5xx's are for problems with the server. They're of the type
> that
>>> you would contact your system administrator for to fix the server.
>>> Trying to delete a template that is in use, sounds like a user
> error;
>>> "412 Precondition Failed" or "405 Method Not Allowed" seems more
>>> appropriate.
>>
>> Indeed. I spent some time looking at the status code definitions [1].
> I
>> think 405 or 412 could both be used without too much drama, but since
>> we're being specific about it... 405 says 'method not allowed' which
> imo
>> isn't accurate since it *is* allowed, but not for current resource
>> state. For 412, we don't user preconditions in the request headers
> here
>> (or at all afaik). I think perhaps 409 is a good fit:
>>
>> "The request could not be completed due to a conflict with the
> current
>> state of the resource. This code is only allowed in situations where
> it
>> is expected that the user might be able to resolve the conflict and
>> resubmit the request" ...
>>
>> I have a patch at http://tracker.deltacloud.org/set/193 - Joe will be
>> testing that and then update the JIRA ticket accordingly,
>>
>> thanks, marios
>>
>> [1] http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html
>>
>>
>>>
>>> Regards,
>>> Dies Koper
>>>
>>>> -----Original Message-----
>>>> From: jvlcek@redhat.com [mailto:jvlcek@redhat.com]
>>>> Sent: Saturday, 8 December 2012 4:10 AM
>>>> To: dev@deltacloud.apache.org
>>>> Subject: [PATCH] DTACLOUD-379 catch and handle in haml
>>>>
>>>> From: Joe VLcek <jv...@redhat.com>
>>>>
>>>> ---
>>>> server/views/errors/500.html.haml | 3 ++-
>>>> server/views/errors/500.xml.haml | 3 ++-
>>>> 2 files changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/server/views/errors/500.html.haml
>>>> b/server/views/errors/500.html.haml
>>>> index 8cd3c74..0fa4563 100644
>>>> --- a/server/views/errors/500.html.haml
>>>> +++ b/server/views/errors/500.html.haml
>>>> @@ -16,7 +16,8 @@
>>>> %em No details
>>>> %li{ :'data-role' => 'list-divider'} Backtrace
>>>> %li
>>>> - %pre= bt @error.backtrace
>>>> + - unless @error.message.downcase.gsub(/\s+/,"
>>> ").include?('cannot
>>>> delete template. template is being used')
>>>> + %pre= bt @error.backtrace
>>>>
>>>> - if @error.backtrace
>>>> %div{ 'data-role' => :collapsible, 'data-collapsed' => "true"}
>>>> diff --git a/server/views/errors/500.xml.haml
>>>> b/server/views/errors/500.xml.haml
>>>> index b3e71e2..11aa4e1 100644
>>>> --- a/server/views/errors/500.xml.haml
>>>> +++ b/server/views/errors/500.xml.haml
>>>> @@ -6,7 +6,8 @@
>>>> %code=response.status
>>>> %message< #{cdata @error.message}
>>>> - if @error.respond_to?(:backtrace) and !@error.backtrace.nil?
>>>> - %backtrace=cdata(@error.backtrace.join("\n"))
>>>> + - unless @error.message.downcase.gsub(/\s+/,"
> ").include?('cannot
>>>> delete template. template is being used')
>>>> + %backtrace=cdata(@error.backtrace.join("\n"))
>>>> - if params
>>>> %request
>>>> - params.each do |k, v|
>>>> --
>>>> 1.7.11.7
>>>>
>>>
>>>
>>
>
>
RE: [PATCH] DTACLOUD-379 catch and handle in haml
Posted by "Koper, Dies" <di...@fast.au.fujitsu.com>.
Hi Marios,
I like your patch but the return code you picked raises questions (seems
to introduce inconsistencies).
We used (still do?) return a 405 from the Deltacloud API when trying to
start an instance that is already started, stopping an instance that is
already stopped, etc., according to:
https://issues.apache.org/jira/browse/DTACLOUD-214
I don't understand how this case is any different.
I don't know the background of returning 405 for the case above, and the
cases the spec writers envisioned.
I read the 409 description (also due to Wikipedia's interpretation) to
mean a case where the action would put the resource in an inconsistent
state, and the client can change their request and retry to prevent the
inconsistency.
In our case the action is not possible and applicable at all to the
resource at the moment, and it requires more than changing the request
to reach the intended outcome.
In cimi (as in DC), we return actions that are allowed on resources. It
is my understanding (from a discussion I had with Doug last week) that
actions that are not allowed because the user doesn't have privileges or
because the resource is not in the right state, are not included.
In that case, it makes sense to me to return "405 Method Not Allowed" to
an action not included in that list.
So I find the introduction of 409 for this particular case look
inconsistent with what we do in other similar cases.
Regards,
Dies Koper
> -----Original Message-----
> From: marios@redhat.com [mailto:mandreou@redhat.com]
> Sent: Monday, 10 December 2012 11:51 PM
> To: dev@deltacloud.apache.org
> Cc: Koper, Dies; Joe Vlcek
> Subject: Re: [PATCH] DTACLOUD-379 catch and handle in haml
>
> Hi Dies - as always thanks for your input here - more inline:
>
> On 08/12/12 05:49, Koper, Dies wrote:
> > Nack.
> > I don't think it's a good idea to start putting messages specific to
one
> > provider in the common haml files.
> > If we start doing that with all messages from all providers, it's
going
> > to be a big mess.
> >
>
> yes indeed - and I have to claim ownership of this great idea :/ -
Joe's
> initial solution was to raise a new 5XX error and then catch this so
it
> silences the error logging. I didn't want to 'mess around' too much
with
> the error handling code, hence my 'do it in haml' suggestion. Looking
at
> it, you are right, its not the way to go.
>
> > Maybe the driver can raise a particular class of (common) error for
> > which the backtrace is not logged?
> >
> > Before that, I don't think the driver should be raising a 5xx for
this
> > error. 5xx's are for problems with the server. They're of the type
that
> > you would contact your system administrator for to fix the server.
> > Trying to delete a template that is in use, sounds like a user
error;
> > "412 Precondition Failed" or "405 Method Not Allowed" seems more
> > appropriate.
>
> Indeed. I spent some time looking at the status code definitions [1].
I
> think 405 or 412 could both be used without too much drama, but since
> we're being specific about it... 405 says 'method not allowed' which
imo
> isn't accurate since it *is* allowed, but not for current resource
> state. For 412, we don't user preconditions in the request headers
here
> (or at all afaik). I think perhaps 409 is a good fit:
>
> "The request could not be completed due to a conflict with the
current
> state of the resource. This code is only allowed in situations where
it
> is expected that the user might be able to resolve the conflict and
> resubmit the request" ...
>
> I have a patch at http://tracker.deltacloud.org/set/193 - Joe will be
> testing that and then update the JIRA ticket accordingly,
>
> thanks, marios
>
> [1] http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html
>
>
> >
> > Regards,
> > Dies Koper
> >
> >> -----Original Message-----
> >> From: jvlcek@redhat.com [mailto:jvlcek@redhat.com]
> >> Sent: Saturday, 8 December 2012 4:10 AM
> >> To: dev@deltacloud.apache.org
> >> Subject: [PATCH] DTACLOUD-379 catch and handle in haml
> >>
> >> From: Joe VLcek <jv...@redhat.com>
> >>
> >> ---
> >> server/views/errors/500.html.haml | 3 ++-
> >> server/views/errors/500.xml.haml | 3 ++-
> >> 2 files changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/server/views/errors/500.html.haml
> >> b/server/views/errors/500.html.haml
> >> index 8cd3c74..0fa4563 100644
> >> --- a/server/views/errors/500.html.haml
> >> +++ b/server/views/errors/500.html.haml
> >> @@ -16,7 +16,8 @@
> >> %em No details
> >> %li{ :'data-role' => 'list-divider'} Backtrace
> >> %li
> >> - %pre= bt @error.backtrace
> >> + - unless @error.message.downcase.gsub(/\s+/,"
> > ").include?('cannot
> >> delete template. template is being used')
> >> + %pre= bt @error.backtrace
> >>
> >> - if @error.backtrace
> >> %div{ 'data-role' => :collapsible, 'data-collapsed' => "true"}
> >> diff --git a/server/views/errors/500.xml.haml
> >> b/server/views/errors/500.xml.haml
> >> index b3e71e2..11aa4e1 100644
> >> --- a/server/views/errors/500.xml.haml
> >> +++ b/server/views/errors/500.xml.haml
> >> @@ -6,7 +6,8 @@
> >> %code=response.status
> >> %message< #{cdata @error.message}
> >> - if @error.respond_to?(:backtrace) and !@error.backtrace.nil?
> >> - %backtrace=cdata(@error.backtrace.join("\n"))
> >> + - unless @error.message.downcase.gsub(/\s+/,"
").include?('cannot
> >> delete template. template is being used')
> >> + %backtrace=cdata(@error.backtrace.join("\n"))
> >> - if params
> >> %request
> >> - params.each do |k, v|
> >> --
> >> 1.7.11.7
> >>
> >
> >
>
Re: [PATCH] DTACLOUD-379 catch and handle in haml
Posted by "marios@redhat.com" <ma...@redhat.com>.
Hi Dies - as always thanks for your input here - more inline:
On 08/12/12 05:49, Koper, Dies wrote:
> Nack.
> I don't think it's a good idea to start putting messages specific to one
> provider in the common haml files.
> If we start doing that with all messages from all providers, it's going
> to be a big mess.
>
yes indeed - and I have to claim ownership of this great idea :/ - Joe's
initial solution was to raise a new 5XX error and then catch this so it
silences the error logging. I didn't want to 'mess around' too much with
the error handling code, hence my 'do it in haml' suggestion. Looking at
it, you are right, its not the way to go.
> Maybe the driver can raise a particular class of (common) error for
> which the backtrace is not logged?
>
> Before that, I don't think the driver should be raising a 5xx for this
> error. 5xx's are for problems with the server. They're of the type that
> you would contact your system administrator for to fix the server.
> Trying to delete a template that is in use, sounds like a user error;
> "412 Precondition Failed" or "405 Method Not Allowed" seems more
> appropriate.
Indeed. I spent some time looking at the status code definitions [1]. I
think 405 or 412 could both be used without too much drama, but since
we're being specific about it... 405 says 'method not allowed' which imo
isn't accurate since it *is* allowed, but not for current resource
state. For 412, we don't user preconditions in the request headers here
(or at all afaik). I think perhaps 409 is a good fit:
"The request could not be completed due to a conflict with the current
state of the resource. This code is only allowed in situations where it
is expected that the user might be able to resolve the conflict and
resubmit the request" ...
I have a patch at http://tracker.deltacloud.org/set/193 - Joe will be
testing that and then update the JIRA ticket accordingly,
thanks, marios
[1] http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html
>
> Regards,
> Dies Koper
>
>> -----Original Message-----
>> From: jvlcek@redhat.com [mailto:jvlcek@redhat.com]
>> Sent: Saturday, 8 December 2012 4:10 AM
>> To: dev@deltacloud.apache.org
>> Subject: [PATCH] DTACLOUD-379 catch and handle in haml
>>
>> From: Joe VLcek <jv...@redhat.com>
>>
>> ---
>> server/views/errors/500.html.haml | 3 ++-
>> server/views/errors/500.xml.haml | 3 ++-
>> 2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/server/views/errors/500.html.haml
>> b/server/views/errors/500.html.haml
>> index 8cd3c74..0fa4563 100644
>> --- a/server/views/errors/500.html.haml
>> +++ b/server/views/errors/500.html.haml
>> @@ -16,7 +16,8 @@
>> %em No details
>> %li{ :'data-role' => 'list-divider'} Backtrace
>> %li
>> - %pre= bt @error.backtrace
>> + - unless @error.message.downcase.gsub(/\s+/,"
> ").include?('cannot
>> delete template. template is being used')
>> + %pre= bt @error.backtrace
>>
>> - if @error.backtrace
>> %div{ 'data-role' => :collapsible, 'data-collapsed' => "true"}
>> diff --git a/server/views/errors/500.xml.haml
>> b/server/views/errors/500.xml.haml
>> index b3e71e2..11aa4e1 100644
>> --- a/server/views/errors/500.xml.haml
>> +++ b/server/views/errors/500.xml.haml
>> @@ -6,7 +6,8 @@
>> %code=response.status
>> %message< #{cdata @error.message}
>> - if @error.respond_to?(:backtrace) and !@error.backtrace.nil?
>> - %backtrace=cdata(@error.backtrace.join("\n"))
>> + - unless @error.message.downcase.gsub(/\s+/," ").include?('cannot
>> delete template. template is being used')
>> + %backtrace=cdata(@error.backtrace.join("\n"))
>> - if params
>> %request
>> - params.each do |k, v|
>> --
>> 1.7.11.7
>>
>
>
RE: [PATCH] DTACLOUD-379 catch and handle in haml
Posted by "Koper, Dies" <di...@fast.au.fujitsu.com>.
Nack.
I don't think it's a good idea to start putting messages specific to one
provider in the common haml files.
If we start doing that with all messages from all providers, it's going
to be a big mess.
Maybe the driver can raise a particular class of (common) error for
which the backtrace is not logged?
Before that, I don't think the driver should be raising a 5xx for this
error. 5xx's are for problems with the server. They're of the type that
you would contact your system administrator for to fix the server.
Trying to delete a template that is in use, sounds like a user error;
"412 Precondition Failed" or "405 Method Not Allowed" seems more
appropriate.
Regards,
Dies Koper
> -----Original Message-----
> From: jvlcek@redhat.com [mailto:jvlcek@redhat.com]
> Sent: Saturday, 8 December 2012 4:10 AM
> To: dev@deltacloud.apache.org
> Subject: [PATCH] DTACLOUD-379 catch and handle in haml
>
> From: Joe VLcek <jv...@redhat.com>
>
> ---
> server/views/errors/500.html.haml | 3 ++-
> server/views/errors/500.xml.haml | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/server/views/errors/500.html.haml
> b/server/views/errors/500.html.haml
> index 8cd3c74..0fa4563 100644
> --- a/server/views/errors/500.html.haml
> +++ b/server/views/errors/500.html.haml
> @@ -16,7 +16,8 @@
> %em No details
> %li{ :'data-role' => 'list-divider'} Backtrace
> %li
> - %pre= bt @error.backtrace
> + - unless @error.message.downcase.gsub(/\s+/,"
").include?('cannot
> delete template. template is being used')
> + %pre= bt @error.backtrace
>
> - if @error.backtrace
> %div{ 'data-role' => :collapsible, 'data-collapsed' => "true"}
> diff --git a/server/views/errors/500.xml.haml
> b/server/views/errors/500.xml.haml
> index b3e71e2..11aa4e1 100644
> --- a/server/views/errors/500.xml.haml
> +++ b/server/views/errors/500.xml.haml
> @@ -6,7 +6,8 @@
> %code=response.status
> %message< #{cdata @error.message}
> - if @error.respond_to?(:backtrace) and !@error.backtrace.nil?
> - %backtrace=cdata(@error.backtrace.join("\n"))
> + - unless @error.message.downcase.gsub(/\s+/," ").include?('cannot
> delete template. template is being used')
> + %backtrace=cdata(@error.backtrace.join("\n"))
> - if params
> %request
> - params.each do |k, v|
> --
> 1.7.11.7
>
[PATCH] DTACLOUD-379 catch and handle in haml
Posted by jv...@redhat.com.
From: Joe VLcek <jv...@redhat.com>
---
server/views/errors/500.html.haml | 3 ++-
server/views/errors/500.xml.haml | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/server/views/errors/500.html.haml b/server/views/errors/500.html.haml
index 8cd3c74..0fa4563 100644
--- a/server/views/errors/500.html.haml
+++ b/server/views/errors/500.html.haml
@@ -16,7 +16,8 @@
%em No details
%li{ :'data-role' => 'list-divider'} Backtrace
%li
- %pre= bt @error.backtrace
+ - unless @error.message.downcase.gsub(/\s+/," ").include?('cannot delete template. template is being used')
+ %pre= bt @error.backtrace
- if @error.backtrace
%div{ 'data-role' => :collapsible, 'data-collapsed' => "true"}
diff --git a/server/views/errors/500.xml.haml b/server/views/errors/500.xml.haml
index b3e71e2..11aa4e1 100644
--- a/server/views/errors/500.xml.haml
+++ b/server/views/errors/500.xml.haml
@@ -6,7 +6,8 @@
%code=response.status
%message< #{cdata @error.message}
- if @error.respond_to?(:backtrace) and !@error.backtrace.nil?
- %backtrace=cdata(@error.backtrace.join("\n"))
+ - unless @error.message.downcase.gsub(/\s+/," ").include?('cannot delete template. template is being used')
+ %backtrace=cdata(@error.backtrace.join("\n"))
- if params
%request
- params.each do |k, v|
--
1.7.11.7