deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michal Fojtik <mfoj...@redhat.com>
Subject Re: [PATCH core] Added Image creation from Instances (EC2, Mock, GoGrid, Rackspace)
Date Wed, 02 Mar 2011 10:06:40 GMT
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.

>
>> 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.

>> diff --git a/server/lib/deltacloud/drivers/mock/mock_driver.rb b/server/lib/deltacloud/drivers/mock/mock_driver.rb
>> index fa18d77..06de4da 100644
>> --- a/server/lib/deltacloud/drivers/mock/mock_driver.rb
>> +++ b/server/lib/deltacloud/drivers/mock/mock_driver.rb
>> @@ -135,6 +135,31 @@ class MockDriver < Deltacloud::BaseDriver
>>      images.sort_by{|e| [e.owner_id,e.description]}
>>    end
>>
>> +  def create_image(credentials, opts={})
>> +    check_credentials(credentials)
>> +    ids = Dir[ "#{@storage_root}/images/*.yml" ].collect{|e| File.basename( e, ".yml"
)}
>> +    count = 0
>> +    while true
>> +      next_id = "img#{count}"
>> +      break if not ids.include?(next_id)
>> +      count += 1
>> +    end
>> +    safely do
>> +      image = {
>> +	:name => opts[:name],
>> +	:owner_id => 'root',
>> +	:description => opts[:description],
>> +	:architecture => 'i386',
>> +	:state => 'UP'
>> +      }
>> +      File.open( "#{@storage_root}/images/#{next_id}.yml", 'w' ) do |f|
>> +	YAML.dump( image, f )
>> +      end
>> +      image[:id] = next_id
>> +      Image.new(image)
>> +    end
>> +  end
>
>It would be super-spiffy if the mock driver would report an error if the
>instance's create_image flag is false.

Good idea! Will integrate this before push.

>> @@ -171,11 +196,9 @@ class MockDriver < Deltacloud::BaseDriver
>>
>>      count = 0
>>      while true
>> -      next_id = "inst" + count.to_s
>> -      if not ids.include?(next_id)
>> -        break
>> -      end
>> -      count = count + 1
>> +      next_id = "inst#{count}"
>> +      break if not ids.include?(next_id)
>> +      count += 1
>
>That seems to be unrelated cleanup.

Sure, will commit this in extra patch ;-)

>
>> diff --git a/server/lib/deltacloud/models/instance.rb b/server/lib/deltacloud/models/instance.rb
>> index 6345590..b2a1a0d 100644
>> --- a/server/lib/deltacloud/models/instance.rb
>> +++ b/server/lib/deltacloud/models/instance.rb
>> @@ -32,11 +32,20 @@ class Instance < BaseModel
>>    attr_accessor :authn_error
>>    attr_accessor :username
>>    attr_accessor :password
>> +  attr_accessor :create_image
>> +
>> +  def can_create_image?
>> +    self.create_image
>> +  end
>>
>>    def to_s
>>      name
>>    end
>>
>> +  def ebs?
>> +    return true if 'ebs'.eql?(self.root_type)
>> +  end
>
>It's a little icky that we have a bit of driver-specific stuff in the
>instance object. Since EC2Driver already monkey-patches Instance, maybe
>we should just stick it there, too (not that I condone wide-spread
>monkey patching ;)

Yes and I think it's not used anywhere. I have this helper in views, but
since I add 'can_create_image?' helper it's not needed anymore.
Will remove this ebs? before push.

>
>> diff --git a/server/views/instances/show.html.haml b/server/views/instances/show.html.haml
>> index 76d77ba..9fc0916 100644
>> --- a/server/views/instances/show.html.haml
>> +++ b/server/views/instances/show.html.haml
>> @@ -50,3 +50,8 @@
>>      %dd
>>        -@instance.actions.each do |action|
>>          =link_to_action action, self.send(:"#{action}_instance_url", @instance.id),
instance_action_method(action)
>> +    %dt
>> +    %dd
>> +      - if @instance.can_create_image?
>> +        =link_to_action 'Create Image', url_for("/api/images/new?instance_id=#{@instance.id}"),
:get
>
>Shouldn't we also expose that in the XML ?

Definitely in actions. Good catch, thanks!

   -- Michal


-- 
--------------------------------------------------------
Michal Fojtik, mfojtik@redhat.com
Deltacloud API: http://deltacloud.org
--------------------------------------------------------

Mime
View raw message