Merge lp:~rvb/maas/bug-1336709 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

MACAddress.claim_static_ip isn't meant to deal with the USER_RESERVED alloc_type.

Drive-by fix: fix lint in src/maasserver/models/tests/test_staticipaddress.py.

[Jeroen T. Vermeulen (jtv)]

I also hit the problem in maasserver.tests.test_api_node.TestStickyIP.test_claim_sticky_ip_address_rtns_error_if_clashing_type_exists

[Julian Edwards (julian-edwards)]

Conditionally approved with some changes, otherwise I'd have landed it already.

review: Approve

[MAAS Lander (maas-lander)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[Jeroen T. Vermeulen (jtv)]

Another affected test: maasserver.models.tests.test_node.NodeTest.test_delete_node_also_deletes_related_static_IPs

[Raphaël Badin (rvb)]

> I also hit the problem in maasserver.tests.test_api_node.TestStickyIP.test_cla
> im_sticky_ip_address_rtns_error_if_clashing_type_exists

Done.

[Raphaël Badin (rvb)]

> Another affected test: maasserver.models.tests.test_node.NodeTest.test_delete_
> node_also_deletes_related_static_IPs

Done.

[Julian Edwards (julian-edwards)]

Thanks for fixing my upper-decker!

On 02/07/14 20:41, Raphaël Badin wrote:
>
>
> Diff comments:
>
>> === modified file 'src/maasserver/models/macaddress.py'
>> --- src/maasserver/models/macaddress.py 2014-06-12 10:08:51 +0000
>> +++ src/maasserver/models/macaddress.py 2014-07-02 10:18:44 +0000
>> @@ -106,6 +106,9 @@
>> :raises: StaticIPAddressTypeClash if an IP already exists with a
>> different type.
>> """
>> + if alloc_type == IPADDRESS_TYPE.USER_RESERVED:
>> + raise AssertionError(
>> + "Can't claim a static IP for USER_RESERVED alloc_type")
>> # Avoid circular import.
>
>> Did you write and run the test before adding this code?
>
> Yes I did.
>
>> Because the test would have passed
>
> No it would not. That's precisely why I added a check for the exception message.

Well done on chopping out context from my original statement to make me
look stupid here. ;)

My actual words were:
"Because the test would have passed (with the exception of the exception
text)"

meaning, all this is doing is raising the same exception with different
text. As I said on IRC:

> Anyway, I agree that it's probably not worth bothering with adding a new assertion because it's a programming error and so the only really important information would be the stacktrace.

This is the more important thing here, the new assertion is not adding
any value.

=== modified file 'src/maasserver/models/macaddress.py'
--- src/maasserver/models/macaddress.py	2014-06-12 10:08:51 +0000
+++ src/maasserver/models/macaddress.py	2014-07-02 10:47:59 +0000
@@ -97,6 +97,7 @@
         the host boots it is too late.
 
         :param alloc_type: See :class:`StaticIPAddress`.alloc_type.
+            This parameter musn't be IPADDRESS_TYPE.USER_RESERVED.
         :return: A :class:`StaticIPAddress` object. Returns None if
             the cluster_interface is not yet known, or the
             static_ip_range_low/high values values are not set on the

=== modified file 'src/maasserver/models/staticipaddress.py'
--- src/maasserver/models/staticipaddress.py	2014-07-02 03:47:43 +0000
+++ src/maasserver/models/staticipaddress.py	2014-07-02 10:47:59 +0000
@@ -68,7 +68,7 @@
         """
         if alloc_type == IPADDRESS_TYPE.USER_RESERVED and user is None:
             raise AssertionError(
-                "Must provide user for USER_RESERVED alloc_type")
+                "Must provide user for USER_RESERVED alloc_type.")
         if user is not None and alloc_type != IPADDRESS_TYPE.USER_RESERVED:
             raise AssertionError(
                 "Must not provide user for USER_RESERVED alloc_type.")

=== modified file 'src/maasserver/models/tests/test_macaddress.py'
--- src/maasserver/models/tests/test_macaddress.py	2014-06-17 01:14:36 +0000
+++ src/maasserver/models/tests/test_macaddress.py	2014-07-02 10:47:59 +0000
@@ -115,7 +115,8 @@
     def test_claim_static_ip_raises_if_clashing_type(self):
         node = factory.make_node_with_mac_attached_to_nodegroupinterface()
         mac = node.get_primary_mac()
-        iptype = factory.getRandomEnum(IPADDRESS_TYPE)
+        iptype = factory.getRandomEnum(
+            IPADDRESS_TYPE, but_not=[IPADDRESS_TYPE.USER_RESERVED])
         iptype2 = factory.getRandomEnum(IPADDRESS_TYPE, but_not=[iptype])
         mac.claim_static_ip(alloc_type=iptype)
         self.assertRaises(
@@ -125,7 +126,8 @@
     def test_claim_static_ip_returns_existing_if_claiming_same_type(self):
         node = factory.make_node_with_mac_attached_to_nodegroupinterface()
         mac = node.get_primary_mac()
-        iptype = factory.getRandomEnum(IPADDRESS_TYPE)
+        iptype = factory.getRandomEnum(
+            IPADDRESS_TYPE, but_not=[IPADDRESS_TYPE.USER_RESERVED])
         ip = mac.claim_static_ip(alloc_type=iptype)
         self.assertEqual(
             ip, mac.claim_static_ip(alloc_type=iptype))

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2014-07-02 07:05:24 +0000
+++ src/maasserver/models/tests/test_node.py	2014-07-02 10:47:59 +0000
@@ -339,7 +339,8 @@
         self.patch(Omshell, 'remove')
         node = factory.make_node_with_mac_attached_to_nodegroupinterface()
         primary_mac = node.get_primary_mac()
-        random_alloc_type = factory.getRandomEnum(IPADDRESS_TYPE)
+        random_alloc_type = factory.getRandomEnum(
+            IPADDRESS_TYPE, but_not=[IPADDRESS_TYPE.USER_RESERVED])
         primary_mac.claim_static_ip(alloc_type=random_alloc_type)
         node.delete()
         self.assertItemsEqual([], StaticIPAddress.objects.all())

=== modified file 'src/maasserver/models/tests/test_staticipaddress.py'
--- src/maasserver/models/tests/test_staticipaddress.py	2014-07-02 03:42:42 +0000
+++ src/maasserver/models/tests/test_staticipaddress.py	2014-07-02 10:47:59 +0000
@@ -61,8 +61,8 @@
             user=user, alloc_type=alloc_type)
 
     def test_allocate_new_with_reserved_type_requires_a_user(self):
-        low, high = factory.make_ip_range()                 
-        self.assertRaises(                                  
+        low, high = factory.make_ip_range()
+        self.assertRaises(
             AssertionError, StaticIPAddress.objects.allocate_new, low, high,
             alloc_type=IPADDRESS_TYPE.USER_RESERVED)
 

=== modified file 'src/maasserver/tests/test_api_node.py'
--- src/maasserver/tests/test_api_node.py	2014-06-27 03:48:14 +0000
+++ src/maasserver/tests/test_api_node.py	2014-07-02 10:47:59 +0000
@@ -972,7 +972,8 @@
         self.become_admin()
         node = factory.make_node_with_mac_attached_to_nodegroupinterface()
         random_alloc_type = factory.getRandomEnum(
-            IPADDRESS_TYPE, but_not=[IPADDRESS_TYPE.STICKY])
+            IPADDRESS_TYPE,
+            but_not=[IPADDRESS_TYPE.STICKY, IPADDRESS_TYPE.USER_RESERVED])
         node.get_primary_mac().claim_static_ip(alloc_type=random_alloc_type)
         response = self.client.post(
             self.get_node_uri(node), {'op': 'claim_sticky_ip_address'})