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: > > 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.
I definitely like this approach. Two small nits about the patch: > Signed-off-by: Chris Lalancette <[email protected]> > --- > 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 mailing list [email protected] https://fedorahosted.org/mailman/listinfo/deltacloud-devel
