Mailing-List: contact deltacloud-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: deltacloud-dev@incubator.apache.org
Received-SPF: pass (athena.apache.org: domain of lutter@redhat.com designates
 209.132.183.28 as permitted sender)
Subject: Re: [PATCH core] Added Image creation from Instances (EC2, Mock,
 GoGrid, Rackspace)
From: David Lutterkort <lutter@redhat.com>
To: deltacloud-dev@incubator.apache.org
In-Reply-To: <20110302100639.GB22963@redhat.com>
References: <1298632683-25749-1-git-send-email-mfojtik@redhat.com>
	 <1298632683-25749-2-git-send-email-mfojtik@redhat.com>
	 <1299029376.32128.51.camel@avon.watzmann.net>
	 <20110302100639.GB22963@redhat.com>
Content-Type: text/plain; charset="ISO-8859-15"
Organization: Red Hat Inc
Date: Thu, 03 Mar 2011 17:16:40 -0800
Message-ID: <1299201400.13816.59.camel@avon.watzmann.net>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit

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