On 01/03/11 17:29 -0800, David Lutterkort wrote:
On Fri, 2011-02-25 at 12:18 +0100, [email protected] wrote:
From: Michal Fojtik <[email protected]>
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
[email protected] 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, [email protected]
Deltacloud API: http://deltacloud.org
--------------------------------------------------------
