incubator-deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sang-Min Park <sang-min.p...@eucalyptus.com>
Subject RE: [PATCH] initial eucalyptus support
Date Wed, 02 Mar 2011 08:07:14 GMT
Thanks David for your careful review.
Yes, I agree that subclassing Euca driver from EC2 is less costly way to
maintain both drivers. I'll work on it and resubmit the patch soon.

I suspect EC2 driver has been modified since I forked it, because some
changes in the diff isn't what I intended for.
Below, I tried to answer some of your questions/comments in line.

Sang-min

-----Original Message-----
From: David Lutterkort [mailto:lutter@redhat.com]
Sent: Tuesday, March 01, 2011 5:20 PM
To: deltacloud-dev@incubator.apache.org
Cc: sang-min.park@eucalyptus.com; Sang-Min Park
Subject: Re: [PATCH] initial eucalyptus support

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.

---> Yes, I wholeheartedly agree with you.

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.


--> Yes, I'd agree that we leave only Red Hat copyright.


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

---> YAML way of doing this might be necessary for Euca, because we allow
administrators change the instance types (hardware profiles). The hardware
profile in this patch matches the default instance types after fresh
installation.

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

--> the second arg is image_type, which if presented to AWS, triggers
tagging by image type (and Euca doesn't support tagging yet).

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

Why ?

--> I think it's a bug which I'll fix.

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

--> the issue is that the line "(:instance_type => opts[:hwp_id]) if
opts[:hwp_id]" results in 'InstanceType=''' appended into EC2's
RunInstance request.
It works for EC2, but causes message signature validation error with Euca.
I think the patch is safely applied to EC2 as well.

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

--> I suspect EC2 driver has been modified since I forked it. I'll leave
filter_on in the next patch.

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

--> So this is fixed by patching AWS. Similarly to InstanceType in
RunInstance request, it leaves 'SnapshotId='' ' in the EC2 CreateVolume
request, which generates signature error with Euca.

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

--> again, I guess EC2 driver has been modified since my fork of it.
There's no need to do this.

David

Mime
View raw message