Merge lp:~bcwaldon/nova/lp713144 into lp:~hudson-openstack/nova/trunk

[Dan Prince]

Update the Openstack API so that it returns 'addresses'.

This branch should resolve nova bug #713144 (https://bugs.launchpad.net/nova/+bug/713144).

[Devin Carlen (devcamcar)]

lgtm

review: Approve

[lp:~bcwaldon/nova/lp713144]

657. By Brian Waldon on 2011-02-10

adding myself to Authors file

658. By Brian Waldon on 2011-02-15

merging trunk back in; updating Authors conflict

[Matt Dietz (cerberus)]

Hi Brian!

The passes on lines 28 and 36 seem superfluous.

The list comprehension on 32-33 is pretty odd and a bit unreadable IMO. Perhaps even ruby-ish ;-) Additionally, you're effectively building a list of stuff that you're immediately throwing away, in addition to the list you're actually appending to. I'd like to see this refactored to something a bit cleaner.

Also datetime should be alphabetically listed before json in test_servers.py

review: Needs Fixing

[lp:~bcwaldon/nova/lp713144]

659. By Brian Waldon on 2011-02-17

removing superfluous pass statements; replacing list comprehension with for loop; alphabetizing imports

[Brian Waldon (bcwaldon)]

Thanks for the feedback. Here's what I did:

1) I removed the pass statements. I guess I had them there before I added the log lines, and didn't think about it when I committed. Does it make sense to log on KeyError in the private ip section of my code? I know an instance won't have a private ip immediately when the api returns successfully, and I'm just not sure what the state of that dict will be coming up from the db api code.

2) The list comprehension has been replaced with a for loop. The code is essentially just reversed, but now it doesn't build an unnecessary list.

3) I reordered the datetime import. I hadn't realized they were supposed to be in alphabetical order.

Brian Waldon

-----Original Message-----
From: "Matthew Dietz" <email address hidden>
Sent: Wednesday, February 16, 2011 3:08pm
To: "Dan Prince" <email address hidden>
Subject: Re: [Merge] lp:~bcwaldon/nova/lp713144 into lp:nova

Review: Needs Fixing
Hi Brian!

The passes on lines 28 and 36 seem superfluous.

The list comprehension on 32-33 is pretty odd and a bit unreadable IMO. Perhaps even ruby-ish ;-) Additionally, you're effectively building a list of stuff that you're immediately throwing away, in addition to the list you're actually appending to. I'd like to see this refactored to something a bit cleaner.

Also datetime should be alphabetically listed before json in test_servers.py

--
https://code.launchpad.net/~bcwaldon/nova/lp713144/+merge/49135
You are the owner of lp:~bcwaldon/nova/lp713144.

[Matt Dietz (cerberus)]

Thanks for making those changes!

1) If you know it will *never* have an IP it's probably not worth logging. If it might, but not having one breaks something, then we definitely want to log it.

[Matt Dietz (cerberus)]

^^

review: Approve

=== modified file 'Authors'
--- Authors	2011-02-16 21:29:52 +0000
+++ Authors	2011-02-17 14:52:19 +0000
@@ -3,6 +3,7 @@
 Anthony Young <[e-mail]>
 Antony Messerli <[e-mail]>
 Armando Migliaccio <[e-mail]>
+Brian Waldon <[e-mail]>
 Bilal Akhtar <[e-mail]>
 Brian Schott <[e-mail]> <[e-mail]>
 Chiradeep Vittal <[e-mail]>

=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2011-01-26 16:16:16 +0000
+++ nova/api/openstack/servers.py	2011-02-17 14:52:19 +0000
@@ -64,6 +64,22 @@
 
     inst_dict['status'] = power_mapping[inst_dict['status']]
     inst_dict['addresses'] = dict(public=[], private=[])
+
+    # grab single private fixed ip
+    try:
+        private_ip = inst['fixed_ip']['address']
+        if private_ip:
+            inst_dict['addresses']['private'].append(private_ip)
+    except KeyError:
+        LOG.debug(_("Failed to read private ip"))
+
+    # grab all public floating ips
+    try:
+        for floating in inst['fixed_ip']['floating_ips']:
+            inst_dict['addresses']['public'].append(floating['address'])
+    except KeyError:
+        LOG.debug(_("Failed to read public ip(s)"))
+
     inst_dict['metadata'] = {}
     inst_dict['hostId'] = ''
 

=== modified file 'nova/tests/api/openstack/test_servers.py'
--- nova/tests/api/openstack/test_servers.py	2011-01-27 18:48:00 +0000
+++ nova/tests/api/openstack/test_servers.py	2011-02-17 14:52:19 +0000
@@ -15,6 +15,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import datetime
 import json
 import unittest
 
@@ -39,6 +40,13 @@
     return stub_instance(id)
 
 
+def return_server_with_addresses(private, public):
+    def _return_server(context, id):
+        return stub_instance(id, private_address=private,
+                             public_addresses=public)
+    return _return_server
+
+
 def return_servers(context, user_id=1):
     return [stub_instance(i, user_id) for i in xrange(5)]
 
@@ -55,9 +63,45 @@
     return None
 
 
-def stub_instance(id, user_id=1):
-    return Instance(id=id, state=0, image_id=10, user_id=user_id,
-                    display_name='server%s' % id)
+def stub_instance(id, user_id=1, private_address=None, public_addresses=None):
+    if public_addresses == None:
+        public_addresses = list()
+
+    instance = {
+        "id": id,
+        "admin_pass": "",
+        "user_id": user_id,
+        "project_id": "",
+        "image_id": 10,
+        "kernel_id": "",
+        "ramdisk_id": "",
+        "launch_index": 0,
+        "key_name": "",
+        "key_data": "",
+        "state": 0,
+        "state_description": "",
+        "memory_mb": 0,
+        "vcpus": 0,
+        "local_gb": 0,
+        "hostname": "",
+        "host": "",
+        "instance_type": "",
+        "user_data": "",
+        "reservation_id": "",
+        "mac_address": "",
+        "scheduled_at": datetime.datetime.now(),
+        "launched_at": datetime.datetime.now(),
+        "terminated_at": datetime.datetime.now(),
+        "availability_zone": "",
+        "display_name": "server%s" % id,
+        "display_description": "",
+        "locked": False}
+
+    instance["fixed_ip"] = {
+        "address": private_address,
+        "floating_ips": [{"address":ip} for ip in public_addresses]}
+
+    return instance
 
 
 def fake_compute_api(cls, req, id):
@@ -105,6 +149,22 @@
         self.assertEqual(res_dict['server']['id'], '1')
         self.assertEqual(res_dict['server']['name'], 'server1')
 
+    def test_get_server_by_id_with_addresses(self):
+        private = "192.168.0.3"
+        public = ["1.2.3.4"]
+        new_return_server = return_server_with_addresses(private, public)
+        self.stubs.Set(nova.db.api, 'instance_get', new_return_server)
+        req = webob.Request.blank('/v1.0/servers/1')
+        res = req.get_response(fakes.wsgi_app())
+        res_dict = json.loads(res.body)
+        self.assertEqual(res_dict['server']['id'], '1')
+        self.assertEqual(res_dict['server']['name'], 'server1')
+        addresses = res_dict['server']['addresses']
+        self.assertEqual(len(addresses["public"]), len(public))
+        self.assertEqual(addresses["public"][0], public[0])
+        self.assertEqual(len(addresses["private"]), 1)
+        self.assertEqual(addresses["private"][0], private)
+
     def test_get_server_list(self):
         req = webob.Request.blank('/v1.0/servers')
         res = req.get_response(fakes.wsgi_app())