Merge lp:~oubiwann/txaws/415486-reservation-object into lp:~txawsteam/txaws/trunk

[Duncan McGreggor]

[Duncan McGreggor (oubiwann)]

This is ready for review.

[lp:~oubiwann/txaws/415486-reservation-object]

9. By Duncan McGreggor on 2009-08-18

Updated the gtk client to wrap the new API change.

[Thomas Herve (therve)]

Nice job, +1!

review: Approve

[Robert Collins (lifeless)]

As far as I could tell only one reservation is returned, not N?

If I'm wrong this is fine, if I'm correct, then it would be nicer to
return the reservation rather than an iterable of reservations.

-Rob

[Jamu Kakar (jkakar)]

[1]

+ def __init__(self, reservation_id, owner_id, groups=[], instances=[]):

The default empty list values for groups and instances will be
inadvertently shared by all Reservation instances that don't provide
explicit values during instantiation. I recommend you use None and
set the value in the body of the constructor as necessary.

[2]

+ def test_parse_reservation(self):
+ ec2 = client.EC2Client(creds='foo')
+ results = ec2._parse_reservation(sample_describe_instances_result)
+ self.check_parsed_reservations(results)

It would be nice to avoid accessing the private _parse_reservation
method by passing a query_factory to EC2Client that returns a
FakeQuery. FakeQuery.submit could return a succeeded deferred and
you could call ec2.describe_instances to get a Deferred with the
result you want to make assertions about.

+1 considering #1.

review: Approve

[Thomas Herve (therve)]

> As far as I could tell only one reservation is returned, not N?

No, you get one reservation per call to CreateInstances. If you're calling CreateInstances several times, you're getting several reservations.
The reservations are what is holding the information about the security groups used, for example.

[lp:~oubiwann/txaws/415486-reservation-object]

10. By Duncan McGreggor on 2009-08-18

Merged from trunk and resolved conflicts.

[Robert Collins (lifeless)]

On Tue, 2009-08-18 at 20:34 +0000, Thomas Herve wrote:
> > As far as I could tell only one reservation is returned, not N?
>
> No, you get one reservation per call to CreateInstances. If you're calling CreateInstances several times, you're getting several reservations.
> The reservations are what is holding the information about the security groups used, for example.

Thats 'yes' then - a call to CreateInstances returns one reservation.

Having our thunk to create instances return a _list of reservations_
makes the API harder to work with.

-Rob

[lp:~oubiwann/txaws/415486-reservation-object]

11. By Duncan McGreggor on 2009-08-18

Changed default values in signature so that groups and instances wouldn't be
shared across all Reservation objects.

[Thomas Herve (therve)]

> On Tue, 2009-08-18 at 20:34 +0000, Thomas Herve wrote:
> > > As far as I could tell only one reservation is returned, not N?
> >
> > No, you get one reservation per call to CreateInstances. If you're calling
> CreateInstances several times, you're getting several reservations.
> > The reservations are what is holding the information about the security
> groups used, for example.
>
> Thats 'yes' then - a call to CreateInstances returns one reservation.
>
> Having our thunk to create instances return a _list of reservations_
> makes the API harder to work with.

We're not *creating* instances, we're describing them. A call to CreateInstances may return a list of instances, but it's not what we're doing right now.

[lp:~oubiwann/txaws/415486-reservation-object]

12. By Duncan McGreggor on 2009-08-18

Finished the fix from the last commit (jkakar 1).

[Duncan McGreggor (oubiwann)]

> [1]
>
> + def __init__(self, reservation_id, owner_id, groups=[], instances=[]):
>
> The default empty list values for groups and instances will be
> inadvertently shared by all Reservation instances that don't provide
> explicit values during instantiation. I recommend you use None and
> set the value in the body of the constructor as necessary.

Nice catch! Fixed.

> [2]
>
> + def test_parse_reservation(self):
> + ec2 = client.EC2Client(creds='foo')
> + results = ec2._parse_reservation(sample_describe_instances_result)
> + self.check_parsed_reservations(results)
>
> It would be nice to avoid accessing the private _parse_reservation
> method by passing a query_factory to EC2Client that returns a
> FakeQuery. FakeQuery.submit could return a succeeded deferred and
> you could call ec2.describe_instances to get a Deferred with the
> result you want to make assertions about.

This is interesting. I'm not completely sold on it yet... but I'll create a ticket for this.

[Robert Collins (lifeless)]

We just chatted on IRC.

It would be nice to keep returning a list of instances. We can expose
the reservation as a reservation attribute on each instance.

-Rob

[Duncan McGreggor (oubiwann)]

> > On Tue, 2009-08-18 at 20:34 +0000, Thomas Herve wrote:
> > > > As far as I could tell only one reservation is returned, not N?
> > >
> > > No, you get one reservation per call to CreateInstances. If you're calling
> > CreateInstances several times, you're getting several reservations.
> > > The reservations are what is holding the information about the security
> > groups used, for example.
> >
> > Thats 'yes' then - a call to CreateInstances returns one reservation.
> >
> > Having our thunk to create instances return a _list of reservations_
> > makes the API harder to work with.
>
> We're not *creating* instances, we're describing them. A call to
> CreateInstances may return a list of instances, but it's not what we're doing
> right now.

There was some confusion about this which got sorted out on IRC. Robert then proposed an excellent idea that we all liked. Here's the IRC chat:

[18:14] <therve`> lifeless: the API we're calling is DescribeInstances
[18:14] <therve`> this one returns a list of reservations
[18:14] <lifeless> ok
[18:14] <lifeless> its still a little ugly to work with but I can see the rationale
[18:14] <therve`> yeah, the problem is that the reservation actually holds important information
[18:15] <therve`> which we can't get anywhere else
[18:15] <lifeless> we could fold that down to the instance
[18:15] <lifeless> if the info is only a field or two
[18:15] <obiwann> lifeless: yeah, that was one of my thoughts too... still not sure how I feel about it
[18:15] <lifeless> that is
[18:15] <lifeless> rather than reservations[N].instances[N]
[18:15] <lifeless> have instances[N].reservation
[18:16] <therve`> this is a valid idea
[18:16] <obiwann> yeah, I kinda like that
[18:16] <obiwann> instances[N].reservation is better than what I was thinking

[lp:~oubiwann/txaws/415486-reservation-object]

13. By Duncan McGreggor on 2009-08-18

Made the reservation for a given instance an attribute on that instance.

14. By Duncan McGreggor on 2009-08-18

Renamed the functions associated with parsing the AWS reponse, due to the
change in functionality.

15. By Duncan McGreggor on 2009-08-18

Rolled back the now unnecessary change to the gtk code.

[Duncan McGreggor (oubiwann)]

> We just chatted on IRC.
>
> It would be nice to keep returning a list of instances. We can expose
> the reservation as a reservation attribute on each instance.
>
> -Rob

Hey Rob, when you get a chance, I've updated the code (and pushed it) according to my understanding of your suggestions. Let me know if it's what you had in mind.

Thanks!

[Robert Collins (lifeless)]

Looks good to me - thanks.

-Rob

=== modified file 'txaws/ec2/client.py'
--- txaws/ec2/client.py	2009-04-28 11:52:57 +0000
+++ txaws/ec2/client.py	2009-08-18 19:08:31 +0000
@@ -14,22 +14,37 @@
 from txaws.util import iso8601time, XML
 
 
+class Reservation(object):
+    """An Amazon EC2 Reservation.
+
+    @attrib reservation_id: Unique ID of the reservation.
+    @attrib owner_id: AWS Access Key ID of the user who owns the reservation. 
+    @attrib groups: A list of security groups.
+    @attrib instances: A list of C{Instance}s.
+    """
+    def __init__(self, reservation_id, owner_id, groups=[], instances=[]):
+        self.reservation_id = reservation_id
+        self.owner_id = owner_id
+        self.groups = groups
+        self.instances = instances
+
+
 class Instance(object):
     """An Amazon EC2 Instance.
 
-    :attrib instanceId: The instance ID of this instance.
-    :attrib instanceState: The state of this instance.
+    @attrib instance_id: The instance ID of this instance.
+    @attrib instance_state: The state of this instance.
     """
 
-    def __init__(self, instanceId, instanceState):
-        self.instanceId = instanceId
-        self.instanceState = instanceState
+    def __init__(self, instance_id, instance_state):
+        self.instance_id = instance_id
+        self.instance_state = instance_state
 
 
 class EC2Client(object):
     """A client for EC2."""
 
-    NS = '{http://ec2.amazonaws.com/doc/2008-12-01/}'
+    name_space = '{http://ec2.amazonaws.com/doc/2008-12-01/}'
 
     def __init__(self, creds=None, query_factory=None):
         """Create an EC2Client.
@@ -50,19 +65,38 @@
         """Describe current instances."""
         q = self.query_factory('DescribeInstances', self.creds)
         d = q.submit()
-        return d.addCallback(self._parse_Reservation)
+        return d.addCallback(self._parse_reservation)
 
-    def _parse_Reservation(self, xml_bytes):
+    def _parse_reservation(self, xml_bytes):
         root = XML(xml_bytes)
-        result = []
+        results = []
         # May be a more elegant way to do this:
-        for reservation in root.find(self.NS + 'reservationSet'):
-            for instance in reservation.find(self.NS + 'instancesSet'):
-                instanceId = instance.findtext(self.NS + 'instanceId')
-                instanceState = instance.find(
-                    self.NS + 'instanceState').findtext(self.NS + 'name')
-                result.append(Instance(instanceId, instanceState))
-        return result
+        for reservation_data in root.find(self.name_space + 'reservationSet'):
+            # Get the security group information.
+            groups = []
+            for group_data in reservation_data.find(
+                self.name_space + 'groupSet'):
+                group_id = group_data.findtext(self.name_space + 'groupId')
+                groups.append(group_id)
+            # Get the list of instances.
+            instances = []
+            for instance_data in reservation_data.find(
+                self.name_space + 'instancesSet'):
+                instance_id = instance_data.findtext(
+                    self.name_space + 'instanceId')
+                instance_state = instance_data.find(
+                    self.name_space + 'instanceState').findtext(
+                        self.name_space + 'name')
+                instances.append(Instance(instance_id, instance_state))
+            # Create a reservation object with the parsed data.
+            reservation = Reservation(
+                reservation_id=reservation_data.findtext(
+                    self.name_space + 'reservationId'),
+                owner_id=reservation_data.findtext(
+                    self.name_space + 'ownerId'),
+                groups=groups, instances=instances)
+            results.append(reservation)
+        return results
 
     def terminate_instances(self, *instance_ids):
         """Terminate some instances.
@@ -82,12 +116,14 @@
         root = XML(xml_bytes)
         result = []
         # May be a more elegant way to do this:
-        for instance in root.find(self.NS + 'instancesSet'):
-            instanceId = instance.findtext(self.NS + 'instanceId')
+        for instance in root.find(self.name_space + 'instancesSet'):
+            instanceId = instance.findtext(self.name_space + 'instanceId')
             previousState = instance.find(
-                self.NS + 'previousState').findtext(self.NS + 'name')
+                self.name_space + 'previousState').findtext(
+                    self.name_space + 'name')
             shutdownState = instance.find(
-                self.NS + 'shutdownState').findtext(self.NS + 'name')
+                self.name_space + 'shutdownState').findtext(
+                    self.name_space + 'name')
             result.append((instanceId, previousState, shutdownState))
         return result
 

=== modified file 'txaws/ec2/tests/test_client.py'
--- txaws/ec2/tests/test_client.py	2009-04-28 11:52:57 +0000
+++ txaws/ec2/tests/test_client.py	2009-08-18 19:08:31 +0000
@@ -9,6 +9,7 @@
 from txaws.ec2 import client
 from txaws.tests import TXAWSTestCase
 
+
 sample_describe_instances_result = """<?xml version="1.0"?>
 <DescribeInstancesResponse xmlns="http://ec2.amazonaws.com/doc/2008-12-01/">
     <requestId>52b4c730-f29f-498d-94c1-91efb75994cc</requestId>
@@ -49,6 +50,7 @@
 </DescribeInstancesResponse>
 """
 
+
 sample_terminate_instances_result = """<?xml version="1.0"?>
 <TerminateInstancesResponse xmlns="http://ec2.amazonaws.com/doc/2008-12-01/">
   <instancesSet>
@@ -79,6 +81,20 @@
 """
 
 
+class ReservationTestCase(TXAWSTestCase):
+
+    def test_reservation_creation(self):
+        reservation = client.Reservation(
+            "id1", "owner", groups=["one", "two"],
+            instances=[client.Instance("id2", "state")])
+        self.assertEquals(reservation.reservation_id, "id1")
+        self.assertEquals(reservation.owner_id, "owner")
+        self.assertEquals(reservation.groups, ["one", "two"])
+        instance = reservation.instances[0]
+        self.assertEquals(instance.instance_id, "id2")
+        self.assertEquals(instance.instance_state, "state")
+
+
 class TestEC2Client(TXAWSTestCase):
     
     def test_init_no_creds(self):
@@ -95,6 +111,21 @@
         ec2 = client.EC2Client(creds=creds)
         self.assertEqual(creds, ec2.creds)
 
+    def check_parsed_reservations(self, results):
+        reservation = results[0]
+        self.assertEquals(reservation.reservation_id, "r-cf24b1a6")
+        self.assertEquals(reservation.owner_id, "123456789012")
+        instance = reservation.instances[0]
+        self.assertEquals(instance.instance_id, "i-abcdef01")
+        self.assertEquals(instance.instance_state, "running")
+        group = reservation.groups[0]
+        self.assertEquals(group, "default")
+
+    def test_parse_reservation(self):
+        ec2 = client.EC2Client(creds='foo')
+        results = ec2._parse_reservation(sample_describe_instances_result)
+        self.check_parsed_reservations(results)
+
     def test_describe_instances(self):
         class StubQuery(object):
             def __init__(stub, action, creds):
@@ -104,11 +135,7 @@
                 return succeed(sample_describe_instances_result)
         ec2 = client.EC2Client(creds='foo', query_factory=StubQuery)
         d = ec2.describe_instances()
-        def check_instances(reservation):
-            self.assertEqual(1, len(reservation))
-            self.assertEqual('i-abcdef01', reservation[0].instanceId)
-            self.assertEqual('running', reservation[0].instanceState)
-        d.addCallback(check_instances)
+        d.addCallback(self.check_parsed_reservations)
         return d
 
     def test_terminate_instances(self):