You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@deltacloud.apache.org by mf...@redhat.com on 2010/12/07 15:05:27 UTC
Various fixes in REST (status codes, etc)
Hi,
This patch includes a lot of REST-specific fixes to be more 'RESTish' ;-)
Some notes:
* Start action on stopped instance (EC2 especially)
This will lead to immediate Internal Server Error right now when this
instance is not EBS.
So rather than returning ISE, return 304 (Not Modified) and redirect
back to that instance (eg. instance status was not modified by this call)
* DELETE calls
Now we redirecting client to deleted resource, which from my point of
view has no sense ;-)
So instead of pointing client to deleted resource, reply with '410'
status code (Gone), and tell client that this resource was deleted
and it's 'marked' as deleted.
* CREATE calls
I fixed (hopefully) all create calls where code '200' was returned and
replaced this HTTP code with '201' (Created).
* Instance actions
Instead of returning '200' (OK), return '202' which means 'Accepted'.
Obviously, actions are not performed 'immediately' so it means client
can get wrong result and think that Instance is 'STOPPED' for example
where it's not (it's PENDING or SHUTTING_DOWN).
Code '202' is more like 'job queued' or 'task accepted'.
* Use PUT when pushing something to resource
For now this applies only for registering instance to created
LoadBalancer object. So instead of using 'POST' method for that,
use 'PUT' because we 'literally' 'putting' a new object to this
resource.
This patch also includes fix for typo in 'load balancer' view.
Let me know, what do you think about this.
-- Michal
Re: Various fixes in REST (status codes, etc)
Posted by Michal Fojtik <mf...@redhat.com>.
On 07/12/10 16:09 +0100, Michal Fojtik wrote:
>On 07/12/10 15:08 +0100, Ladislav Martincik wrote:
>>Nice one. Like it very much. :)
>
>Thanks,
>
>BTW. this will need for sure some client update, just forgot to check
>when I sent this patch. I'll sent another patch with client fixes, after
>some discussion about this stuff.
OK, I was wrong, our Ruby client is so inteligent that it can deal with
this changes without any modifications. (rake spec without errors)
-- Michal
>
> -- Michal
>
>>
>>-- Ladislav
>>
>>On Dec 7, 2010, at 3:05 PM, mfojtik@redhat.com wrote:
>>
>>>Hi,
>>>
>>>This patch includes a lot of REST-specific fixes to be more 'RESTish' ;-)
>>>
>>>Some notes:
>>>
>>>* Start action on stopped instance (EC2 especially)
>>> This will lead to immediate Internal Server Error right now when this
>>> instance is not EBS.
>>> So rather than returning ISE, return 304 (Not Modified) and redirect
>>> back to that instance (eg. instance status was not modified by this call)
>>>
>>>* DELETE calls
>>> Now we redirecting client to deleted resource, which from my point of
>>> view has no sense ;-)
>>> So instead of pointing client to deleted resource, reply with '410'
>>> status code (Gone), and tell client that this resource was deleted
>>> and it's 'marked' as deleted.
>>>
>>>* CREATE calls
>>> I fixed (hopefully) all create calls where code '200' was returned and
>>> replaced this HTTP code with '201' (Created).
>>>
>>>* Instance actions
>>> Instead of returning '200' (OK), return '202' which means 'Accepted'.
>>> Obviously, actions are not performed 'immediately' so it means client
>>> can get wrong result and think that Instance is 'STOPPED' for example
>>> where it's not (it's PENDING or SHUTTING_DOWN).
>>> Code '202' is more like 'job queued' or 'task accepted'.
>>>
>>>* Use PUT when pushing something to resource
>>> For now this applies only for registering instance to created
>>> LoadBalancer object. So instead of using 'POST' method for that,
>>> use 'PUT' because we 'literally' 'putting' a new object to this
>>> resource.
>>>
>>>This patch also includes fix for typo in 'load balancer' view.
>>>
>>>Let me know, what do you think about this.
>>>
>>> -- Michal
>>>
>>
>
>--
>--------------------------------------------------------
>Michal Fojtik, mfojtik@redhat.com
>Deltacloud API: http://deltacloud.org
>--------------------------------------------------------
--
--------------------------------------------------------
Michal Fojtik, mfojtik@redhat.com
Deltacloud API: http://deltacloud.org
--------------------------------------------------------
Re: Various fixes in REST (status codes, etc)
Posted by Michal Fojtik <mf...@redhat.com>.
On 07/12/10 15:08 +0100, Ladislav Martincik wrote:
>Nice one. Like it very much. :)
Thanks,
BTW. this will need for sure some client update, just forgot to check
when I sent this patch. I'll sent another patch with client fixes, after
some discussion about this stuff.
-- Michal
>
>-- Ladislav
>
>On Dec 7, 2010, at 3:05 PM, mfojtik@redhat.com wrote:
>
>> Hi,
>>
>> This patch includes a lot of REST-specific fixes to be more 'RESTish' ;-)
>>
>> Some notes:
>>
>> * Start action on stopped instance (EC2 especially)
>> This will lead to immediate Internal Server Error right now when this
>> instance is not EBS.
>> So rather than returning ISE, return 304 (Not Modified) and redirect
>> back to that instance (eg. instance status was not modified by this call)
>>
>> * DELETE calls
>> Now we redirecting client to deleted resource, which from my point of
>> view has no sense ;-)
>> So instead of pointing client to deleted resource, reply with '410'
>> status code (Gone), and tell client that this resource was deleted
>> and it's 'marked' as deleted.
>>
>> * CREATE calls
>> I fixed (hopefully) all create calls where code '200' was returned and
>> replaced this HTTP code with '201' (Created).
>>
>> * Instance actions
>> Instead of returning '200' (OK), return '202' which means 'Accepted'.
>> Obviously, actions are not performed 'immediately' so it means client
>> can get wrong result and think that Instance is 'STOPPED' for example
>> where it's not (it's PENDING or SHUTTING_DOWN).
>> Code '202' is more like 'job queued' or 'task accepted'.
>>
>> * Use PUT when pushing something to resource
>> For now this applies only for registering instance to created
>> LoadBalancer object. So instead of using 'POST' method for that,
>> use 'PUT' because we 'literally' 'putting' a new object to this
>> resource.
>>
>> This patch also includes fix for typo in 'load balancer' view.
>>
>> Let me know, what do you think about this.
>>
>> -- Michal
>>
>
--
--------------------------------------------------------
Michal Fojtik, mfojtik@redhat.com
Deltacloud API: http://deltacloud.org
--------------------------------------------------------
Re: Various fixes in REST (status codes, etc)
Posted by Ladislav Martincik <lm...@redhat.com>.
Nice one. Like it very much. :)
-- Ladislav
On Dec 7, 2010, at 3:05 PM, mfojtik@redhat.com wrote:
> Hi,
>
> This patch includes a lot of REST-specific fixes to be more 'RESTish' ;-)
>
> Some notes:
>
> * Start action on stopped instance (EC2 especially)
> This will lead to immediate Internal Server Error right now when this
> instance is not EBS.
> So rather than returning ISE, return 304 (Not Modified) and redirect
> back to that instance (eg. instance status was not modified by this call)
>
> * DELETE calls
> Now we redirecting client to deleted resource, which from my point of
> view has no sense ;-)
> So instead of pointing client to deleted resource, reply with '410'
> status code (Gone), and tell client that this resource was deleted
> and it's 'marked' as deleted.
>
> * CREATE calls
> I fixed (hopefully) all create calls where code '200' was returned and
> replaced this HTTP code with '201' (Created).
>
> * Instance actions
> Instead of returning '200' (OK), return '202' which means 'Accepted'.
> Obviously, actions are not performed 'immediately' so it means client
> can get wrong result and think that Instance is 'STOPPED' for example
> where it's not (it's PENDING or SHUTTING_DOWN).
> Code '202' is more like 'job queued' or 'task accepted'.
>
> * Use PUT when pushing something to resource
> For now this applies only for registering instance to created
> LoadBalancer object. So instead of using 'POST' method for that,
> use 'PUT' because we 'literally' 'putting' a new object to this
> resource.
>
> This patch also includes fix for typo in 'load balancer' view.
>
> Let me know, what do you think about this.
>
> -- Michal
>
Re: Various fixes in REST (status codes, etc)
Posted by "marios@redhat.com" <ma...@redhat.com>.
after chatting with Michal on irc, i will ack-ish this patch :)
Basically it needs to be reworked, comments below:
On 07/12/10 16:05, mfojtik@redhat.com wrote:
> Hi,
>
> This patch includes a lot of REST-specific fixes to be more 'RESTish' ;-)
>
> Some notes:
>
> * Start action on stopped instance (EC2 especially)
> This will lead to immediate Internal Server Error right now when this
> instance is not EBS.
> So rather than returning ISE, return 304 (Not Modified) and redirect
> back to that instance (eg. instance status was not modified by this call)
this is a good idea, but might be invalidated by our re-introduction of
the 'terminated' state for ec2 (and any other drivers that need it tbd).
Once that happens then you won't have the option of doing a 'start' when
you stop an s3 backed ec2 instance.
>
> * DELETE calls
> Now we redirecting client to deleted resource, which from my point of
> view has no sense ;-)
> So instead of pointing client to deleted resource, reply with '410'
> status code (Gone), and tell client that this resource was deleted
> and it's 'marked' as deleted.
I agree with the principle of returning a 410 for deleted resources and
NOT redirecting to that resource. However, this patch doesn't fix a
'redirect to deleted resource' problem - i mean in this patch the
delete actions are referring to blobs and when we delete blobs, we
redirect to the *bucket* not the blob. Also, we can't generalise the
whole 410 thing - if the client is xml/json then a 410 response is fine,
but if its a html client then we should redirect to the resource index
(e.g. delete a blob ==> redirect to bucket, delete an instance ==>
redirect to instances, etc).
marios
[PATCH core] Various fixes in REST and Load Balancer code.
Posted by mf...@redhat.com.
From: Michal Fojtik <mf...@redhat.com>
---
.../lib/deltacloud/helpers/application_helper.rb | 22 +++++++--
server/server.rb | 48 ++++++++++++--------
server/views/load_balancers/new.html.haml | 4 +-
server/views/load_balancers/show.html.haml | 3 +-
4 files changed, 51 insertions(+), 26 deletions(-)
diff --git a/server/lib/deltacloud/helpers/application_helper.rb b/server/lib/deltacloud/helpers/application_helper.rb
index f538714..b27e377 100644
--- a/server/lib/deltacloud/helpers/application_helper.rb
+++ b/server/lib/deltacloud/helpers/application_helper.rb
@@ -112,12 +112,26 @@ module ApplicationHelper
@instance = driver.send(:"#{name}_instance", credentials, params["id"])
- return redirect(instances_url) if name.eql?(:destroy) or @instance.class!=Instance
+ # Check if instance has image_id attribute, so for example when you execute
+ # action and there is no response from that action (eg. no Instance), it
+ # means that this action doesn't modify a state of given Instance
+ # eg. try a 'start' action on non-ELB instance which is in STOPPED state
+ unless @instance
+ status 304
+ response['Location'] = instance_url(params[:id])
+ halt
+ end
+
+ if name.eql?(:destroy) or @instance.class!=Instance
+ status 410 # Instance is Gone and clients should not link to this resouce
+ else
+ status 202 # Instance action was accepted and it will be executed on backend
+ end
respond_to do |format|
- format.xml { haml :"instances/show" }
- format.html { haml :"instances/show" }
- format.json {convert_to_json(:instance, @instance) }
+ format.xml { haml(:"instances/show") }
+ format.html { haml(:"instances/show") }
+ format.json { convert_to_json(:instance, @instance) }
end
end
diff --git a/server/server.rb b/server/server.rb
index 076859d..0e36fd5 100644
--- a/server/server.rb
+++ b/server/server.rb
@@ -229,19 +229,21 @@ collection :load_balancers do
control do
@load_balancer = driver.create_load_balancer(credentials, params)
respond_to do |format|
- format.xml { haml :"load_balancers/show" }
- format.html { haml :"load_balancers/show" }
+ format.xml { [ 201, haml(:"load_balancers/show") ] }
+ format.html { [ 201, haml(:"load_balancers/show") ] }
end
end
end
- operation :register, :method => :post, :member => true do
+ operation :register, :method => :put, :member => true do
description "Add instance to loadbalancer"
param :id, :string, :required
param :instance_id, :string, :required
control do
- driver.lb_register_instance(credentials, params)
- redirect(load_balancer_url(params[:id]))
+ if driver.lb_register_instance(credentials, params)
+ status 202 # Accepted
+ show( :load_balancer )
+ end
end
end
@@ -250,8 +252,10 @@ collection :load_balancers do
param :id, :string, :required
param :instance_id, :string, :required
control do
- driver.lb_unregister_instance(credentials, params)
- redirect(load_balancer_url(params[:id]))
+ if driver.lb_unregister_instance(credentials, params)
+ status 202
+ show( :load_balancer )
+ end
end
end
@@ -259,8 +263,9 @@ collection :load_balancers do
description "Destroy given load balancer"
param :id, :string, :required
control do
- driver.destroy_load_balancer(credentials, params[:id])
- redirect(load_balancers_url)
+ if driver.destroy_load_balancer(credentials, params[:id])
+ status 410
+ end
end
end
@@ -299,10 +304,10 @@ END
instance = driver.create_instance(credentials, @image.id, params)
respond_to do |format|
format.xml do
- response.status = 201 # Created
response['Location'] = instance_url(instance.id)
@instance = instance
- haml :"instances/show"
+ status 210
+ haml(:"instances/show")
end
format.html do
redirect instance_url(instance.id) if instance and instance.id
@@ -450,9 +455,11 @@ collection :keys do
param :name, :string, :required
control do
@key = driver.create_key(credentials, { :key_name => params[:name] })
+ response['Location'] = key_url(@key.id)
+ status 210
respond_to do |format|
- format.html { haml :"keys/show" }
- format.xml { haml :"keys/show", :ugly => true }
+ format.html { haml(:"keys/show") }
+ format.xml { haml(:"keys/show", :ugly => true) }
end
end
end
@@ -462,8 +469,9 @@ collection :keys do
with_capability :destroy_key
param :id, :string, :required
control do
- driver.destroy_key(credentials, { :key_name => params[:id]})
- redirect(keys_url)
+ if driver.destroy_key(credentials, { :key_name => params[:id]})
+ status 410 # Resource is 'Gone'
+ end
end
end
@@ -493,8 +501,9 @@ end
delete '/api/buckets/:bucket/:blob' do
bucket_id = params[:bucket]
blob_id = params[:blob]
- driver.delete_blob(credentials, bucket_id, blob_id)
- redirect(bucket_url(bucket_id))
+ if driver.delete_blob(credentials, bucket_id, blob_id)
+ status 410 # Resource is 'Gone'
+ end
end
#Get a particular blob's particulars (not actual blob data)
@@ -570,8 +579,9 @@ collection :buckets do
with_capability :delete_bucket
param :id, :string, :required
control do
- driver.delete_bucket(credentials, params[:id], params)
- redirect(buckets_url)
+ if driver.delete_bucket(credentials, params[:id], params)
+ status 410
+ end
end
end
diff --git a/server/views/load_balancers/new.html.haml b/server/views/load_balancers/new.html.haml
index 561caa4..58a89f7 100644
--- a/server/views/load_balancers/new.html.haml
+++ b/server/views/load_balancers/new.html.haml
@@ -28,11 +28,11 @@
%p
%label
Load balancer port:
- %input{ :name => "listener_lbr_port", :size => 30}
+ %input{ :name => "listener_balancer_port", :size => 30}
%p
%label
Instances port:
- %input{ :name => "listener_inst_port", :size => 30}
+ %input{ :name => "listener_instance_port", :size => 30}
%p
%input{ :type => :submit, :name => "commit", :value => "create" }/
diff --git a/server/views/load_balancers/show.html.haml b/server/views/load_balancers/show.html.haml
index 26e1766..6ef52b9 100644
--- a/server/views/load_balancers/show.html.haml
+++ b/server/views/load_balancers/show.html.haml
@@ -25,9 +25,10 @@
- @load_balancer.instances.each do |inst|
%dd
=inst.id
- =link_to_action, 'Delete', unregister_load_balancer_url(@load_balancer.id, :instance_id => inst.id), :post
+ =link_to_action 'Delete', unregister_load_balancer_url(@load_balancer.id, :instance_id => inst.id), :post
%form{:action => url_for("/api/load_balancers/#{@load_balancer.id}/register"), :method => :post}
+ %input{ :type => :hidden, :name => "_method", :value => 'put'}
%p
%strong Add instances to load balancer
%p
--
1.7.3.2