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] initial eucalyptus support
Date Wed, 02 Mar 2011 01:20:01 GMT
On Wed, 2011-02-23 at 22:56 -0800, sang-min.park@eucalyptus.com wrote:
> From: Sang-Min Park <spark@eucalyptus.com>
> 
> ---
>  server/config/drivers.yaml                         |    4 +
>  .../drivers/eucalyptus/eucalyptus_driver.rb        |  640 ++++++++++++++++++++
>  server/server.rb                                   |    4 +-
>  3 files changed, 646 insertions(+), 2 deletions(-)
>  create mode 100644 server/lib/deltacloud/drivers/eucalyptus/eucalyptus_driver.rb

This needs a lot more work - the driver is an almost verbatim copy of
the EC2 driver with changes in very specific places. With this approach
of cloning and then maintaining the code separately, I fear we'll have a
much higher maintenance burden going forward, especially as we add
features to the API.

The patch though very nicely outlines the changes that are needed
between the EC2 and Euca drivers.

Rather than review the submitted patch, here is a diff between
server/lib/deltacloud/drivers/ec2/ec2_driver.rb and
server/lib/deltacloud/drivers/eucalyptus/eucalyptus_driver.rb:

> eucalyptus_driver.rb |  198 +++++++++++++++++----------------------------------
>  1 file changed, 69 insertions(+), 129 deletions(-)
> --- ec2/ec2_driver.rb	2011-02-28 16:46:51.000000000 -0800
> +++ eucalyptus/eucalyptus_driver.rb	2011-02-28 16:47:11.000000000 -0800
> @@ -1,3 +1,5 @@
> +# Copyright (C) 2009-2011 Eucalyptus Systems, Inc.
> +#
>  # Copyright (C) 2009, 2010  Red Hat, Inc.

Only one of us can claim copyright; given the size of the change, I'd
still think it's (C) Red Hat. But I'd be just as happy dropping the
copyright notice altogether.

>  # Licensed to the Apache Software Foundation (ASF) under one or more
> @@ -31,11 +33,11 @@
>  
>  module Deltacloud
>    module Drivers
> -    module EC2
> -      class EC2Driver < Deltacloud::BaseDriver
> +    module Eucalyptus
> +      class EucalyptusDriver < Deltacloud::BaseDriver
>  
>          def supported_collections
> -          DEFAULT_COLLECTIONS + [ :keys, :buckets, :load_balancers ]
> +          DEFAULT_COLLECTIONS + [ :keys, :buckets ]

This will be a recurring theme: we need to find a way to have the Euca
driver reuse the EC2 driver with cutting and pasting code. There's a
number of ways to do that in Ruby, most notably subclassing and mixing
in modules.

I'd suggest we start with making EucalyptusDriver a subclass of
EC2Driver. With that, supported_collections for Euca can just override
the corresponding method for EC2.

> @@ -43,71 +45,43 @@
>          feature :instances, :security_group
>          feature :instances, :instance_count
>          feature :images, :owner_id
> -        feature :buckets, :bucket_location
> -        feature :instances, :register_to_load_balancer
> +        feature :buckets, :bucket_location	
> +        #feature :instances, :register_to_load_balancer  # not supported by Eucalyptus
2.0.3

We'll need a 'remove_feature' helper in the base driver to do the above.

>          DEFAULT_REGION = 'us-east-1'
>  
> -        define_hardware_profile('t1.micro') do
> -          cpu                1
> -          memory             0.63 * 1024
> -          storage            160
> -          architecture       'i386'
> -        end

For hardware profiles, I'd actually prefer if we listed them out
separately for EC2 and Euca. To make that work, we'd need a
'clear_hardware_profiles' helper that can be called before the first
'define_hardware_profile' in the Euca driver.

We could of course also read them in from YAML files, with a clever
naming convention to make sure each driver reads the right one. Seems
too complicated to me though.

> @@ -135,13 +109,13 @@
>              end
>              return img_arr
>            end
> -          owner_id = opts[:owner_id] || "amazon"
> +          owner_id = opts[:owner_id] || "self"

Factor into a 'default_owner' method.

>            safely do
> -            img_arr = ec2.describe_images_by_owner(owner_id, "machine").collect do |image|
> +            img_arr = ec2.describe_images_by_owner(owner_id).collect do |image|

Why doesn't AWS catch this difference (i.e. making the 2nd arg a dummy for Euca) ?

>                convert_image(image)
>              end
>            end
> -          img_arr = filter_on( img_arr, :architecture, opts )
> +          #img_arr = filter_on( img_arr, :architecture, opts )

Why ?
 
> @@ -162,18 +136,7 @@
>              inst_arr = ec2.describe_instances.collect do |instance| 
>                convert_instance(instance) if instance
>              end.flatten
> -            tags = ec2.describe_tags(
> -              'Filter.1.Name' => 'resource-type', 'Filter.1.Value' => 'instance'
> -            )
> -            inst_arr.each do |inst|
> -              name_tag = tags.select { |t| (t[:aws_resource_id] == inst.id) and t[:aws_key]
== 'name' }
> -              unless name_tag.empty?
> -                inst.name = name_tag.first[:aws_value]
> -              end
> -            end
> -            delete_unused_tags(credentials, inst_arr.collect {|inst| inst.id})
>            end
> -          inst_arr = filter_on( inst_arr, :id, opts )

Should be factored into a method in EC2Driver; Euca driver should override that and return
[]
 
> @@ -183,7 +146,7 @@
>            instance_options.merge!(:user_data => opts[:user_data]) if opts[:user_data]
>            instance_options.merge!(:key_name => opts[:keyname]) if opts[:keyname]
>            instance_options.merge!(:availability_zone => opts[:realm_id]) if opts[:realm_id]
> -          instance_options.merge!(:instance_type => opts[:hwp_id]) if opts[:hwp_id]
> +          instance_options.merge!(:instance_type => opts[:hwp_id]) if opts[:hwp_id]
&& opts[:hwp_id].length > 0

That seems a safe thing to do for both drivers - under what
circumstances is opts[:hwp_id] the empty string ?

> @@ -193,15 +156,15 @@
>              new_instance = convert_instance(ec2.launch_instances(image_id, instance_options).first)
>              # TODO: Rework this to use client_id for name instead of tag
>              #       Tags should be keept as an optional feature
> -            if opts[:name]
> -              tag_instance(credentials, new_instance, opts[:name])
> -            end
> +            #if opts[:name]
> +            #  tag_instance(credentials, new_instance, opts[:name])
> +            #end

We should pull the 'if opts[:name]' check into tag_instance and have the
Euca driver replace it with a dummy method.

>              # Register Instance to Load Balancer if load_balancer_id is set.
>              # This parameter is a feature parameter
> -            if opts[:load_balancer_id] and opts[:load_balancer_id].length>0
> -              elb = new_client(credentials, :elb)
> -              elb.register_instances_with_load_balancer(opts[:load_balancer_id], [new_instance.id])
> -            end
> +            #if opts[:load_balancer_id] and opts[:load_balancer_id].length>0
> +            #  elb = new_client(credentials, :elb)
> +            #  elb.register_instances_with_load_balancer(opts[:load_balancer_id], [new_instance.id])
> +            #end

Should go into a method similar to tag_instance.

> @@ -233,7 +196,7 @@
>            ec2 = new_client(credentials)
>            instance_id = instance_id
>            if ec2.terminate_instances([instance_id])
> -            untag_instance(credentials, instance_id)
> +            #untag_instance(credentials, instance_id)

Again, override untag_instance with a dummy.

> @@ -270,73 +233,41 @@
>          end
>  
>          def load_balancer(credentials, opts={})
> -          load_balancers(credentials, {
> -            :names => [opts[:id]]
> -          }).first
> +	    raise Deltacloud::BackendError.new(500, "Loadbalancer", "Loadbalancer not supported
yet", "")
>          end

This will work nicely in the subclass - similar for the other changes in this hunk.

> +
>          def buckets(credentials, opts={})
>            buckets = []
>            safely do
>              s3_client = new_client(credentials, :s3)
> -            unless (opts[:id].nil?)
> -              bucket = s3_client.bucket(opts[:id])
> -              buckets << convert_bucket(bucket)
> -            else
> -              bucket_list = s3_client.buckets
> -              bucket_list.each do |current|
> -                buckets << Bucket.new({:name => current.name, :id => current.name})
> -              end #bucket_list.each
> -            end #if
> -          end #safely
> -          filter_on(buckets, :id, opts)
> +            bucket_list = s3_client.buckets
> +		
> +            bucket_list.each do |current|
> +              buckets << convert_bucket(current)
> +            end
> +          end
> +	  buckets

Doesn't this mean that if I ask for a specific bucket, I'd still get all
of them back ? Is this needed because Euca doesn't support retrieving an
individual bucket ?

Either way, the filter_on needs to stay.

> @@ -436,11 +367,12 @@
>          def create_storage_volume(credentials, opts=nil)
>            ec2 = new_client(credentials)
>            opts ||= {}
> -          opts[:snapshot_id] ||= ""
> +          opts[:snapshot_id] ||= ""# -> this causes signature validation error with
Euca backend

Any idea why ?

>            opts[:capacity] ||= "1"
>            opts[:realm_id] ||= realms(credentials).first.id
>            safely do
> -            convert_volume(ec2.create_volume(opts[:snapshot_id], opts[:capacity], opts[:realm_id]))
> +           	convert_volume(ec2.create_volume(opts[:snapshot_id], opts[:capacity], opts[:realm_id]))
> +

Only whitespace change AFAICT
 
> @@ -488,7 +420,7 @@
>          def destroy_storage_snapshot(credentials, opts={})
>            ec2 = new_client(credentials)
>            safely do
> -            unless convert_snapshot(opts[:id])
> +            unless ec2.delete_snapshot(opts[:id])

This seems to be a bug in the EC2 driver, and should be broken out into
a separate patch.

> @@ -512,16 +444,21 @@
>                      when :ec2 then Aws::Ec2
>                      when :s3 then Aws::S3
>                    end
> -          klass.new(credentials.user, credentials.password, {:server => endpoint_for_service(type),
:connection_mode => :per_thread})
> +          klass.new(credentials.user, credentials.password, eucalyptus_endpoint)
> +#:server => endpoint_for_service(type))
>          end

Just override new_client wholesale.
 
> +	# shouldn't be called now; eucalyptus doens't support tagging yet
>          def tag_instance(credentials, instance, name)
>            ec2 = new_client(credentials)
>            safely do

As mentioned above, should become a dummy method.

> @@ -529,6 +466,7 @@
>            end
>          end
>  
> +        # shouldn't be called now; eucalyptus doens't support tagging yet
>          def untag_instance(credentials, instance_id)

As mentioned above, should become a dummy method.

> @@ -536,6 +474,7 @@
>            end
>          end
>  
> +        # shouldn't be called now; eucalyptus doens't support tagging yet
>          def delete_unused_tags(credentials, inst_ids)

As mentioned above, should become a dummy method.

> @@ -556,11 +495,12 @@
> 
>            #can use AWS::S3::Owner.current.display_name or current.id
>            Bucket.new(
>              :id => s3_bucket.name,
>              :name => s3_bucket.name,
> -            :size => blob_list.length,
> +            :size => s3_bucket.keys.length,
>              :blob_list => blob_list

Why ?

David



Mime
View raw message