Mailing-List: contact deltacloud-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: deltacloud-dev@incubator.apache.org
Received-SPF: pass (nike.apache.org: domain of tcrawley@redhat.com designates
 209.132.183.28 as permitted sender)
Message-ID: <4CF56DB3.6060204@redhat.com>
Date: Tue, 30 Nov 2010 16:33:39 -0500
From: Toby Crawley <tcrawley@redhat.com>
Reply-To: tcrawley@redhat.com
Organization: Red Hat
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US;
 rv:1.9.2.12) Gecko/20101103 Fedora/1.0-0.33.b2pre.fc14 Thunderbird/3.1.6
MIME-Version: 1.0
To: Michal Fojtik <mfojtik@redhat.com>
CC: deltacloud-dev@incubator.apache.org
Subject: Re: EC2 driver rewrite (rev 3)
References: <1291036618-21348-1-git-send-email-mfojtik@redhat.com>
 <4CF3FABF.2050003@redhat.com> <20101130171822.GA29236@redhat.com>
In-Reply-To: <20101130171822.GA29236@redhat.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

On 11/30/2010 12:18 PM, Michal Fojtik wrote:
> On 29/11/10 14:10 -0500, Toby Crawley wrote:
>> On 11/29/2010 08:16 AM, mfojtik@redhat.com wrote:
>>> Hi guys,
>>>
>>> This is third edition of new EC2 driver. I removed / merged all
>>> patches which was not related to new EC2 driver.
>>>
>>> To get this patchset running, you need latest AWS gem which is
>>> now pending 2 pull commits (Tobi 1.8.6 compatibility patch and my
>>> S3 location patch). For now, you can install AWS from this location:
>>>
>>> https://github.com/mifo/aws/raw/master/pkg/aws-2.3.9999.gem
>>>
>>> This version has all necessary patches/fixed applied.
>>>
>>> --- Michal
>>>
>>
>> Michal:
>>
>> I'm going to NACK this patch as it is currently, for the following reasons:
>>
>> * the attaching a public ip feature did not work for me at instance launch time - it appears that AWS requires an instance to be in the
>> RUNNING state before it will attach an ip address (though I did not see references to this in the AWS docs). Here is Dc log output for the
>> request:
>>
>> 13:29:07,427 INFO [STDOUT] ##### Aws::Ec2 returned an error: 400 Bad Request
>> 13:29:07,427 INFO [STDOUT] <?xml version="1.0" encoding="UTF-8"?>
>> 13:29:07,427 INFO [STDOUT] <Response><Errors><Error><Code>InvalidInstanceId</Code><Message>The instance 'i-77e3ac1a' is not in the
>> 'running' state</Message></Error></Errors><RequestID>507d46ee-6968-4774-8c65-71932b64d971</RequestID></Response> #####
>> 13:29:07,427 INFO [STDOUT] ##### Aws::Ec2 request:
>> ec2.amazonaws.com:443/?AWSAccessKeyId=<redacted>&Action=AssociateAddress&InstanceId=i-77e3ac1a&PublicIp=50.16.242.45&SignatureMethod=HmacSHA256&SignatureVersion=2&Timestamp=2010-11-29T18%3A29%3A07.000Z&Version=2010-08-31&Signature=VAMkRQle2H0DhLm3SDRpI3bSSMifwum2e%2FnXY1rh6oI%3D
>> ####
>> 13:29:07,431 ERROR [STDERR] Deltacloud::BackendError - InvalidInstanceId: The instance 'i-77e3ac1a' is not in the 'running' state
>
> I'm sorry, I don't test this feature. We can move this 'feature' to be
> a separate action on instance (PUT /api/instances/i-12345/ip or something
> similar).
>> * the new aws gem returns the in use state for a storage_volume as 'IN_USE', whereas amazon-ec2 returned 'IN-USE'. The storage_volume
>> index still makes reference to IN-USE (server/views/storage_volumes/index.html.haml#26
>
> Sure, good catch. I'll fix that to be amazon-ec2 compatible.
>
>> * I had to make the following change to have the mounted instance information be reported correctly on a storage_volume:
>>
>> diff --git a/client/lib/deltacloud.rb b/client/lib/deltacloud.rb
>> index c632594..30e0a1d 100644
>> --- a/client/lib/deltacloud.rb
>> +++ b/client/lib/deltacloud.rb
>> @@ -167,7 +167,7 @@ module DeltaCloud
>> end
>>
>> if attribute.name == 'mount'
>> - obj.add_link!("instance", (attribute/"./instance/@id").first)
>> + obj.add_link!("instance", (attribute/"./instance/@id").first.value)
>> obj.add_text!("device", (attribute/"./device/@name").first.value)
>> next
>> end
>>
>> * I had to make the following two changes to the instance code to launch in the correct realm, and to properly return the public hostname:
>>
>> diff --git a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb b/server/lib/deltacloud/drivers/ec2/
>> index 59206d7..af7a1e6 100644
>> --- a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
>> +++ b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
>> @@ -170,7 +170,7 @@ module Deltacloud
>> instance_options = {}
>> instance_options.merge!(:user_data => opts[:user_data]) if opts[:user_data]
>> instance_options.merge!(:key_name => opts[:key_name]) if opts[:key_name]
>> - instance_options.merge!(:availability_zone => opts[:availability_zone]) if opts[:avail
>> + 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!(:group_ids => opts[:security_group]) if opts[:security_group]
>> safely do
>> @@ -451,7 +451,7 @@ module Deltacloud
>> :instance_profile => InstanceProfile.new(instance[:aws_instance_type]),
>> :realm_id => instance[:aws_availability_zone],
>> :private_addresses => instance[:private_dns_name],
>> - :public_addresses => instance[:public_addresses]
>> + :public_addresses => instance[:dns_name]
>> )
>> end
>>
>>
>> Both of the above patches are the same changes I sent last week, but these patches should apply cleanly to your current tree on top of the
>> patch set.
>
> Ah, yes I forgot to apply your patches, will apply them + fix
> storage_volume and move IP assignment elsewhere and resend this patchset
> again.
>> I only tested instance and storage volume creation & management. I didn't test any of the blob storage features.
>
> We already did some testing with Marios and seems like it works without
> problems. We just discover that 'aws' gem has problems with Asia and US
> west region, but it was fixed in AWS and I issued a pull request for that.
>
> However, if you find some other bugs/problems with this new gem, please
> report them so it can be fixed and applied as full-working driver.
>
>
> -- Michal
>

Michal:

I did find one other issue with the new ec2 driver: it is missing the valid_credentials? method. Here is a patch on top of this patchset to 
restore it. It is based off of the one from the old driver, with some small changes.

diff --git a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
index 59206d7..e47829d 100644
--- a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
+++ b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
@@ -121,7 +121,14 @@ module Deltacloud
            stopped.to( :finish )         .automatically
          end

+        def valid_credentials?(credentials)
+          # FIXME: We need to do this call to determine if
+          #        EC2 is working with given credentials. There is no
+          #        other way to check if given credentials are valid or not.
+          realms = realms(credentials) rescue false
+          return realms ? true : false
+        end
+
          def images(credentials, opts={})
            ec2 = new_client(credentials)
            img_arr = []