Mailing-List: contact deltacloud-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: deltacloud-dev@incubator.apache.org
Received-SPF: pass (athena.apache.org: domain of tcrawley@redhat.com
 designates 209.132.183.28 as permitted sender)
Message-ID: <4CF3FABF.2050003@redhat.com>
Date: Mon, 29 Nov 2010 14:10:55 -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: deltacloud-dev@incubator.apache.org
CC: Michal Fojtik <mfojtik@redhat.com>
Subject: Re: EC2 driver rewrite (rev 3)
References: <1291036618-21348-1-git-send-email-mfojtik@redhat.com>
In-Reply-To: <1291036618-21348-1-git-send-email-mfojtik@redhat.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

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

* 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

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

I only tested instance and storage volume creation & management. I didn't test any of the blob storage features.