incubator-deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Adrian Cole <fernc...@gmail.com>
Subject Re: [PATCH] initial eucalyptus support
Date Wed, 02 Mar 2011 13:30:16 GMT
FYI, in jclouds, the way we manage this is have a base ec2 driver,
which euca and aws-ec2 extend.  The reason for this is that there are
many features that aws have that are offering specific (ex. billing
codes, spot market, cluster nodes etc), or are way ahead of the pack.
In other words, we took a LCD approach with the EC2 api and have the
aws-only features or offerings in the aws-ec2 extension.

-A


On Wed, Mar 2, 2011 at 3:07 AM, Sang-Min Park
<sang-min.park@eucalyptus.com> wrote:
> 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