The instance_action method in server.rb is clearly expecting the individual drivers to return an "Instance" object when the request is successfully fulfilled. Unfortunately, the return value from the individual drivers is woefully inconsistent here; some of them return a new Instance object, and some do not.
Further, as per https://fedorahosted.org/pipermail/deltacloud-devel/2010-May/001130.html
it seems like we should be doing a better job handling cases where we can create a full Instance object vs. a truncated one.
To help fix both of these, the following patch:
1) Changes the haml file to only show the fields of the instance that are actually available. This means that if an individual driver cannot reasonably return good information about an instance after an action has been performed, it can at least return the href and id of the instance; it would then be up to the client to do another GET with the instance HREF to get additional information.
2) Fixes up the EC2 driver to return the expected instance object. In particular, we now throw an exception if the action fails, but if the follow-up request for instance information fails, we just return a truncated Instance object.
In going over the drivers, it looks like:
Mock - returns Instance object - good EC2 - (after this patch) returns Instance object - good GoGrid - does not return Instance object - bad OpenNebula - returns Instance object - good RackSpace - does not return Instance object - bad RHEV-M - returns Instance object - good Rimu - does not return Instance object - bad
So if it is agreed this is the direction to go, I will fix up the other broken drivers as well.
Signed-off-by: Chris Lalancette clalance@redhat.com --- server/lib/deltacloud/drivers/ec2/ec2_driver.rb | 54 +++++++++++++++++++-- server/views/instances/show.xml.haml | 57 +++++++++++++---------- 2 files changed, 81 insertions(+), 30 deletions(-)
diff --git a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb index 69356e2..492a059 100644 --- a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb +++ b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb @@ -168,22 +168,64 @@ class EC2Driver < Deltacloud::BaseDriver
def reboot_instance(credentials, id) ec2 = new_client(credentials) - safely do - ec2.reboot_instances( :instance_id => id ) + backup = ec2.reboot_instances( :instance_id => id ) + + begin + this_instance = ec2.describe_instances( :instance_id => id).reservationSet.item.first.instancesSet.item.first + convert_instance(this_instance, this_instance.ownerId) + rescue Exception => e + puts "ERROR: #{e.message}" + # at this point, the reboot has succeeded but our follow-up + # "describe_instances" failed for some reason. Create a simple Instance + # object with only the ID and new state in place + state = backup.instancesSet.item.first.currentState.name + Instance.new( { + :id => id, + :state => state, + :actions => instance_actions_for( state ), + } ) end end
def stop_instance(credentials, id) ec2 = new_client(credentials) - safely do - ec2.terminate_instances( :instance_id => id ) + backup = ec2.terminate_instances( :instance_id => id ) + + begin + this_instance = ec2.describe_instances( :instance_id => id ).reservationSet.item.first.instancesSet.item.first + convert_instance(this_instance, this_instance.ownerId) + rescue Exception => e + puts "ERROR: #{e.message}" + # at this point, the stop has succeeded but our follow-up + # "describe_instances" failed for some reason. Create a simple Instance + # object with only the ID and new state in place + state = backup.instancesSet.item.first.currentState.name + Instance.new( { + :id => id, + :state => state, + :actions => instance_actions_for( state ), + } ) end end
def destroy_instance(credentials, id) ec2 = new_client(credentials) - safely do - ec2.terminate_instances( :instance_id => id ) + backup = ec2.terminate_instances( :instance_id => id ) + + begin + this_instance = ec2.describe_instances( :instance_id => id).reservationSet.item.first.instancesSet.item.first + convert_instance(this_instance, this_instance.ownerId) + rescue Exception => e + puts "ERROR: #{e.message}" + # at this point, the destroy has succeeded but our follow-up + # "describe_instances" failed for some reason. Create a simple Instance + # object with only the ID and new state in place + state = backup.instancesSet.item.first.currentState.name + Instance.new( { + :id => id, + :state => state, + :actions => instance_actions_for( state ), + } ) end end
diff --git a/server/views/instances/show.xml.haml b/server/views/instances/show.xml.haml index a4859b0..9ed1b89 100644 --- a/server/views/instances/show.xml.haml +++ b/server/views/instances/show.xml.haml @@ -2,27 +2,36 @@ %instance{:href => instance_url(@instance.id)} %id< =@instance.id - %name< - =@instance.name - %owner_id< - =@instance.owner_id - %image{:href => image_url(@instance.image_id)} - %realm{:href => realm_url(@instance.realm_id)} - %state< - =@instance.state - - haml_tag :"hardware-profile", {:href => hardware_profile_url(@instance.instance_profile.id)} do - %id< - =@instance.instance_profile.id - - @instance.instance_profile.overrides.each do |p, v| - %property{:kind => 'fixed', :name => p, :value => v, :unit => Deltacloud::HardwareProfile::unit(p)} - %actions - - @instance.actions.compact.each do |instance_action| - %link{:rel => instance_action, :method => instance_action_method(instance_action), :href => self.send("#{instance_action}_instance_url", @instance.id)} - %public-addresses - - @instance.public_addresses.each do |address| - %address< - =address - %private-addresses - - @instance.private_addresses.each do |address| - %address< - =address + - if @instance.name + %name< + =@instance.name + - if @instance.owner_id + %owner_id< + =@instance.owner_id + - if @instance.image_id + %image{:href => image_url(@instance.image_id)} + - if @instance.realm_id + %realm{:href => realm_url(@instance.realm_id)} + - if @instance.state + %state< + =@instance.state + - if @instance.instance_profile + - haml_tag :"hardware-profile", {:href => hardware_profile_url(@instance.instance_profile.id)} do + %id< + =@instance.instance_profile.id + - @instance.instance_profile.overrides.each do |p, v| + %property{:kind => 'fixed', :name => p, :value => v, :unit => Deltacloud::HardwareProfile::unit(p)} + - if @instance.actions + %actions + - @instance.actions.compact.each do |instance_action| + %link{:rel => instance_action, :method => instance_action_method(instance_action), :href => self.send("#{instance_action}_instance_url", @instance.id)} + - if @instance.public_addresses + %public-addresses + - @instance.public_addresses.each do |address| + %address< + =address + - if @instance.private_addresses + %private-addresses + - @instance.private_addresses.each do |address| + %address< + =address
On Wed, 2010-05-12 at 14:01 -0400, Chris Lalancette wrote:
The instance_action method in server.rb is clearly expecting the individual drivers to return an "Instance" object when the request is successfully fulfilled. Unfortunately, the return value from the individual drivers is woefully inconsistent here; some of them return a new Instance object, and some do not.
Further, as per https://fedorahosted.org/pipermail/deltacloud-devel/2010-May/001130.html
it seems like we should be doing a better job handling cases where we can create a full Instance object vs. a truncated one.
To help fix both of these, the following patch:
- Changes the haml file to only show the fields of
the instance that are actually available. This means that if an individual driver cannot reasonably return good information about an instance after an action has been performed, it can at least return the href and id of the instance; it would then be up to the client to do another GET with the instance HREF to get additional information.
- Fixes up the EC2 driver to return the expected
instance object. In particular, we now throw an exception if the action fails, but if the follow-up request for instance information fails, we just return a truncated Instance object.
In going over the drivers, it looks like:
Mock - returns Instance object - good EC2 - (after this patch) returns Instance object - good GoGrid - does not return Instance object - bad OpenNebula - returns Instance object - good RackSpace - does not return Instance object - bad RHEV-M - returns Instance object - good Rimu - does not return Instance object - bad
So if it is agreed this is the direction to go, I will fix up the other broken drivers as well.
I definitely like this approach. Two small nits about the patch:
Signed-off-by: Chris Lalancette clalance@redhat.com
server/lib/deltacloud/drivers/ec2/ec2_driver.rb | 54 +++++++++++++++++++-- server/views/instances/show.xml.haml | 57 +++++++++++++---------- 2 files changed, 81 insertions(+), 30 deletions(-)
diff --git a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb index 69356e2..492a059 100644 --- a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb +++ b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb @@ -168,22 +168,64 @@ class EC2Driver < Deltacloud::BaseDriver
def reboot_instance(credentials, id) ec2 = new_client(credentials)
- safely do
ec2.reboot_instances( :instance_id => id )
- backup = ec2.reboot_instances( :instance_id => id )
- begin
this_instance = ec2.describe_instances( :instance_id => id).reservationSet.item.first.instancesSet.item.first
convert_instance(this_instance, this_instance.ownerId)
- rescue Exception => e
puts "ERROR: #{e.message}"
# at this point, the reboot has succeeded but our follow-up
# "describe_instances" failed for some reason. Create a simple Instance
# object with only the ID and new state in place
state = backup.instancesSet.item.first.currentState.name
Instance.new( {
:id => id,
:state => state,
:actions => instance_actions_for( state ),
end end} )
(1) Please factor the 'try to get full instance, otherwise use abridged version' code into a private method.
(2) If you print something about the exception, it should be 'puts "WARNING: ignored error during instance refresh: #{e.message}" or some such, so that people can grep for error in the log and only get actual errors.
David
deltacloud-devel@lists.fedorahosted.org