incubator-deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Lutterkort <lut...@redhat.com>
Subject Re: [PATCH core] Added Image creation from Instances (EC2, Mock, GoGrid, Rackspace)
Date Fri, 04 Mar 2011 01:16:40 GMT
On Wed, 2011-03-02 at 11:06 +0100, Michal Fojtik wrote:
> On 01/03/11 17:29 -0800, David Lutterkort wrote:
> >On Fri, 2011-02-25 at 12:18 +0100, mfojtik@redhat.com wrote:
> >> From: Michal Fojtik <mfojtik@redhat.com>
> >
> >Looks good, a few minor things:
> >
> >git am complains about lotsa trailing whitespace.
> >
> >> diff --git a/server/lib/deltacloud/base_driver/features.rb b/server/lib/deltacloud/base_driver/features.rb
> >> index a276314..79e859d 100644
> >> --- a/server/lib/deltacloud/base_driver/features.rb
> >> +++ b/server/lib/deltacloud/base_driver/features.rb
> >> @@ -205,7 +205,7 @@ module Deltacloud
> >>      end
> >>
> >>      declare_feature :instances, :sandboxing do
> >> -      description "Allow lanuching sandbox images"
> >> +      description "Allow launching sandbox images"
> >
> >That seems to be spurious
> 
> Yes, it's a typo I found when I did some testing ;-)
> I can commit this in extra patch.

Haha .. I had stared at that diff for quite some time, and didn't see
the typo in 'launch' until just now.

> >> diff --git a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> >> index d657429..e300d56 100644
> >> --- a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> >> +++ b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> >> @@ -155,6 +155,20 @@ module Deltacloud
> >>            end
> >>          end
> >>
> >> +        def create_image(credentials, opts={})
> >> +          ec2 = new_client(credentials)
> >> +          instance = instance(credentials, :id => opts[:id])
> >> +          safely do
> >> +           raise BackendFeatureUnsupported::new(415, 'InstanceNotEBS', 'Instance
must be EBS format to create image') unless instance.ebs?
> >> +            if not instance.state == 'RUNNING'
> >> +              raise BackendFeatureUnsupported::new(415, 'InstanceNotRunning',
'Instance must be in running state to create image')
> >> +            else
> >> +              new_image_id = ec2.create_image(instance.id, opts[:name], opts[:description])
> >> +              image(credentials, :id => new_image_id)
> >> +            end
> >> +          end
> >
> >Should we really replicate this business logic here ? Shouldn't we leave
> >it up to EC2 to fail the operation and then report that failure back ?
> 
> Surely we can. But I through it could be better to get some 'readable'
> reason to client (mean status code) instead of throwing 500 and message
> from Amazon.

It's a tradeoff between clearer error messages and the extra lookup we
need to do for error checking. How undecipherable are the errors that
come back from EC2 ?

David



Mime
View raw message