Merge lp:~jtran/nova/lp794651 into lp:~hudson-openstack/nova/trunk

[John Tran]

ec2 api method allocate_address ; raises exception.NoFloatingIpsDefined instead of UnknownError when there aren't any floating ips available.

[Vish Ishaya (vishvananda)]

Can we match the type of the inner exception as well? I don't know if we want to raise NoFloatingIpsDefined every time we get a remote error from network.

Vish

On Jun 8, 2011, at 5:22 PM, John Tran wrote:

> John Tran has proposed merging lp:~jtran/nova/lp794651 into lp:nova.
>
> Requested reviews:
> Nova Core (nova-core)
> Related bugs:
> Bug #794651 in OpenStack Compute (nova): "nova api allocate_address returns 'UnknownError' when no addresses available"
> https://bugs.launchpad.net/nova/+bug/794651
>
> For more details, see:
> https://code.launchpad.net/~jtran/nova/lp794651/+merge/63943
> --
> https://code.launchpad.net/~jtran/nova/lp794651/+merge/63943
> You are subscribed to branch lp:nova.
> === modified file 'nova/api/ec2/cloud.py'
> --- nova/api/ec2/cloud.py 2011-06-07 17:47:40 +0000
> +++ nova/api/ec2/cloud.py 2011-06-09 00:22:24 +0000
> @@ -39,6 +39,7 @@
> from nova import ipv6
> from nova import log as logging
> from nova import network
> +from nova import rpc
> from nova import utils
> from nova import volume
> from nova.api.ec2 import ec2utils
> @@ -872,8 +873,11 @@
>
> def allocate_address(self, context, **kwargs):
> LOG.audit(_("Allocate address"), context=context)
> - public_ip = self.network_api.allocate_floating_ip(context)
> - return {'publicIp': public_ip}
> + try:
> + public_ip = self.network_api.allocate_floating_ip(context)
> + return {'publicIp': public_ip}
> + except rpc.RemoteError:
> + raise exception.NoFloatingIpsDefined
>
> def release_address(self, context, public_ip, **kwargs):
> LOG.audit(_("Release address %s"), public_ip, context=context)
>
> === modified file 'nova/tests/test_cloud.py'
> --- nova/tests/test_cloud.py 2011-06-07 20:05:03 +0000
> +++ nova/tests/test_cloud.py 2011-06-09 00:22:24 +0000
> @@ -115,6 +115,17 @@
> public_ip=address)
> db.floating_ip_destroy(self.context, address)
>
> + def test_allocate_address(self):
> + address = "10.10.10.10"
> + allocate = self.cloud.allocate_address
> + db.floating_ip_create(self.context,
> + {'address': address,
> + 'host': self.network.host})
> + self.assertEqual(allocate(self.context)['publicIp'], address)
> + db.floating_ip_destroy(self.context, address)
> + self.assertRaises(exception.NoFloatingIpsDefined, allocate,
> + self.context)
> +
> def test_associate_disassociate_address(self):
> """Verifies associate runs cleanly without raising an exception"""
> address = "10.10.10.10"
>

[John Tran (jtran)]

I was thinking the same thing. I've updated.

> Can we match the type of the inner exception as well? I don't know if we want
> to raise NoFloatingIpsDefined every time we get a remote error from network.
>
> Vish

[Vish Ishaya (vishvananda)]

great lgtm

review: Approve

[Vish Ishaya (vishvananda)]

Although it does occur to me that NoFloatingIpDefined might be a little too descriptive, since you will get the same error if all of the floating ips are in use...

[termie (termie)]

Looks good, I'd make one small style change and make this either use 1 line or 3 lines:

45 + self.assertRaises(exception.NoFloatingIpsDefined, allocate,
46 + self.context)

Beyond that, looks good, I'll let vish (or other) hit the approve button pending thoughts on the no more ips vs no ips defined discussion

[Brian Waldon (bcwaldon)]

+1 for error handling! Just a couple of things:

23: You need to raise an object, not a class. Add '()' to the end of the line.

25: This statement actually triggers a TypeError. You cannot raise nothing. Please provide an instance of an exception to raise.

review: Needs Fixing

[Brian Waldon (bcwaldon)]

> 25: This statement actually triggers a TypeError. You cannot raise nothing.
> Please provide an instance of an exception to raise.

Scratch that. I learned something today :)

[Vish Ishaya (vishvananda)]

23: You can actually raise an exception class in python. It is functionally equivalent to the (). That said, i think it is a little prettier to use the () version. Settiing to work in progress for the style fixes above. Please set back to needs review when they are in.

[Brian Waldon (bcwaldon)]

> 23: You can actually raise an exception class in python. It is functionally
> equivalent to the (). That said, i think it is a little prettier to use the ()
> version. Settiing to work in progress for the style fixes above. Please set
> back to needs review when they are in.

I read the exceptions documentation earlier today and somehow convinced myself you couldn't raise a class (not sure how). I do agree with your style point, but I also think we need to raise all subclasses of NovaException as instances. In its current implementation, NovaException.__str__ does not appear to work without first calling its __init__ method. I'm not sure what magic (if any) raise does to instantiate exception classes. Maybe we should change NovaException so if there are no keyword arguments required (as with NoFloatingIpsDefined), it doesn't matter if you raise the subclass as a class or instance. Thoughts?

[termie (termie)]

I think it is generally good form to raise an instance rather than class and see little reason not to ask people to do it in reviews. I also am not sure exactly what happens if you don't raise an instance but it also seems like something pylint would catch.

[John Tran (jtran)]

I've made the style changes, and added a new exception 'NoMoreFloatingIps'

[Vish Ishaya (vishvananda)]

excellent. Looks good now. Hopefully Brian can come in and flip his review.

review: Approve

[Brian Waldon (bcwaldon)]

I was running tests earlier and got distracted. All's good.

review: Approve

=== modified file 'nova/api/ec2/cloud.py'
--- nova/api/ec2/cloud.py	2011-06-07 17:47:40 +0000
+++ nova/api/ec2/cloud.py	2011-06-10 18:12:24 +0000
@@ -39,6 +39,7 @@
 from nova import ipv6
 from nova import log as logging
 from nova import network
+from nova import rpc
 from nova import utils
 from nova import volume
 from nova.api.ec2 import ec2utils
@@ -872,8 +873,14 @@
 
     def allocate_address(self, context, **kwargs):
         LOG.audit(_("Allocate address"), context=context)
-        public_ip = self.network_api.allocate_floating_ip(context)
-        return {'publicIp': public_ip}
+        try:
+            public_ip = self.network_api.allocate_floating_ip(context)
+            return {'publicIp': public_ip}
+        except rpc.RemoteError as ex:
+            if ex.exc_type == 'NoMoreAddresses':
+                raise exception.NoMoreFloatingIps()
+            else:
+                raise
 
     def release_address(self, context, public_ip, **kwargs):
         LOG.audit(_("Release address %s"), public_ip, context=context)

=== modified file 'nova/exception.py'
--- nova/exception.py	2011-06-03 20:32:42 +0000
+++ nova/exception.py	2011-06-10 18:12:24 +0000
@@ -376,6 +376,10 @@
     message = _("Zero floating ips defined for instance %(instance_id)s.")
 
 
+class NoMoreFloatingIps(NotFound):
+    message = _("Zero floating ips available.")
+
+
 class KeypairNotFound(NotFound):
     message = _("Keypair %(keypair_name)s not found for user %(user_id)s")
 

=== modified file 'nova/tests/test_cloud.py'
--- nova/tests/test_cloud.py	2011-06-07 20:05:03 +0000
+++ nova/tests/test_cloud.py	2011-06-10 18:12:24 +0000
@@ -115,6 +115,18 @@
                                   public_ip=address)
         db.floating_ip_destroy(self.context, address)
 
+    def test_allocate_address(self):
+        address = "10.10.10.10"
+        allocate = self.cloud.allocate_address
+        db.floating_ip_create(self.context,
+                              {'address': address,
+                               'host': self.network.host})
+        self.assertEqual(allocate(self.context)['publicIp'], address)
+        db.floating_ip_destroy(self.context, address)
+        self.assertRaises(exception.NoMoreFloatingIps,
+                          allocate,
+                          self.context)
+
     def test_associate_disassociate_address(self):
         """Verifies associate runs cleanly without raising an exception"""
         address = "10.10.10.10"