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:[email protected]] Sent: Tuesday, March 01, 2011 5:20 PM To: [email protected] Cc: [email protected]; Sang-Min Park Subject: Re: [PATCH] initial eucalyptus support On Wed, 2011-02-23 at 22:56 -0800, [email protected] wrote: > From: Sang-Min Park <[email protected]> > > --- > 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
