Merge lp:~rvb/maas/check-commissioning-bug-1084292 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Fix the check_commissioning API call: make it available to the logged-in users only and return the changed nodes (which is in accordance with what the docs already say).

[Benji York (benji)]

This branch looks fine.

I found the assert of two constructed tuples on line 70, 82, and 97 odd.
Wouldn't two simple assertions be clearer and simpler?

The one on line 97 would especially benefit from unrolling.

review: Approve (code)

[Raphaël Badin (rvb)]

> This branch looks fine.

Thanks for the review!

> I found the assert of two constructed tuples on line 70, 82, and 97 odd.
> Wouldn't two simple assertions be clearer and simpler?

Well, we've try to do it this way in MAAS in order to get the maximum information out of a failure. This way, when something goes wrong (i.e. a test does not pass), we have all the failures revealed in one go as opposed to the tedious process of: having a failing assertion, fixing the problem, running the test again, fixing the next failing assertion, etc.

> The one on line 97 would especially benefit from unrolling.

I've fixed the style for that one a bit.

[Benji York (benji)]

On Mon, Dec 3, 2012 at 4:09 PM, Raphaël Badin
<email address hidden> wrote:
>> This branch looks fine.
>
> Thanks for the review!
>
>> I found the assert of two constructed tuples on line 70, 82, and 97 odd.
>> Wouldn't two simple assertions be clearer and simpler?
>
> Well, we've try to do it this way in MAAS in order to get the maximum
> information out of a failure. This way, when something goes wrong
> (i.e. a test does not pass), we have all the failures revealed in one
> go as opposed to the tedious process of: having a failing assertion,
> fixing the problem, running the test again, fixing the next failing
> assertion, etc.

Interesting. To me the cure seems worse than the disease, but that is
certainly open to debate. Ether way, it strikes me that stopping the
test on the first failure may be a design flaw in unittest (arising
form the use of exceptions to report failures).

Thinking out loud: doctest does not stop on failures and (by default)
reports them all, which is the effect you get with this pattern. Of
course, any positive that comes from that is normally marred by most
doctests lacking inter-test isolation.
--
Benji York

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-11-29 11:30:37 +0000
+++ src/maasserver/api.py	2012-12-03 16:08:24 +0000
@@ -458,21 +458,6 @@
         """Accept a node's enlistment: not allowed to anonymous users."""
         raise Unauthorized("You must be logged in to accept nodes.")
 
-    @operation(idempotent=False)
-    def check_commissioning(self, request):
-        """Check all commissioning nodes to see if they are taking too long.
-
-        Anything that has been commissioning for longer than
-        settings.COMMISSIONING_TIMEOUT is moved into the FAILED_TESTS status.
-        """
-        interval = timedelta(minutes=settings.COMMISSIONING_TIMEOUT)
-        cutoff = datetime.now() - interval
-        query = Node.objects.filter(
-            status=NODE_STATUS.COMMISSIONING, updated__lte=cutoff)
-        query.update(status=NODE_STATUS.FAILED_TESTS)
-        # Note that Django doesn't call save() on updated nodes here,
-        # but I don't think anything requires its effects anyway.
-
     @classmethod
     def resource_uri(cls, *args, **kwargs):
         return ('nodes_handler', [])
@@ -591,6 +576,23 @@
         return filter(None, nodes)
 
     @operation(idempotent=False)
+    def check_commissioning(self, request):
+        """Check all commissioning nodes to see if they are taking too long.
+
+        Anything that has been commissioning for longer than
+        settings.COMMISSIONING_TIMEOUT is moved into the FAILED_TESTS status.
+        """
+        interval = timedelta(minutes=settings.COMMISSIONING_TIMEOUT)
+        cutoff = datetime.now() - interval
+        query = Node.objects.filter(
+            status=NODE_STATUS.COMMISSIONING, updated__lte=cutoff)
+        results = list(query)
+        query.update(status=NODE_STATUS.FAILED_TESTS)
+        # Note that Django doesn't call save() on updated nodes here,
+        # but I don't think anything requires its effects anyway.
+        return results
+
+    @operation(idempotent=False)
     def release(self, request):
         """Release multiple nodes.
 

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-11-28 15:37:34 +0000
+++ src/maasserver/tests/test_api.py	2012-12-03 16:08:24 +0000
@@ -3284,24 +3284,28 @@
             (response.status_code, response.content))
 
 
-class TestAnonymousCommissioningTimeout(APIv10TestMixin, TestCase):
+class TestCommissioningTimeout(APIv10TestMixin, LoggedInTestCase):
     """Testing of commissioning timeout API."""
 
     def test_check_with_no_action(self):
         node = factory.make_node(status=NODE_STATUS.READY)
-        self.client.post(
+        response = self.client.post(
             self.get_uri('nodes/'), {'op': 'check_commissioning'})
         # Anything that's not commissioning should be ignored.
         node = reload_object(node)
-        self.assertEqual(NODE_STATUS.READY, node.status)
+        self.assertEqual(
+            (httplib.OK, NODE_STATUS.READY),
+            (response.status_code, node.status))
 
     def test_check_with_commissioning_but_not_expired_node(self):
         node = factory.make_node(
             status=NODE_STATUS.COMMISSIONING)
-        self.client.post(
+        response = self.client.post(
             self.get_uri('nodes/'), {'op': 'check_commissioning'})
         node = reload_object(node)
-        self.assertEqual(NODE_STATUS.COMMISSIONING, node.status)
+        self.assertEqual(
+            (httplib.OK, NODE_STATUS.COMMISSIONING),
+            (response.status_code, node.status))
 
     def test_check_with_commissioning_and_expired_node(self):
         # Have an interval 1 second longer than the timeout.
@@ -3311,10 +3315,19 @@
             status=NODE_STATUS.COMMISSIONING, created=datetime.now(),
             updated=updated_at)
 
-        self.client.post(
+        response = self.client.post(
             self.get_uri('nodes/'), {'op': 'check_commissioning'})
-        node = reload_object(node)
-        self.assertEqual(NODE_STATUS.FAILED_TESTS, node.status)
+        self.assertEqual(
+            (
+                httplib.OK,
+                NODE_STATUS.FAILED_TESTS,
+                [node.system_id]
+            ),
+            (
+                response.status_code,
+                reload_object(node).status,
+                [node['system_id'] for node in json.loads(response.content)],
+            ))
 
 
 class TestPXEConfigAPI(AnonAPITestCase):