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 Wed, 02 Mar 2011 01:29:36 GMT
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

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

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

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

> 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 ;)

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

David



Mime
View raw message