launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/restrict-refresh into lp:maas.

Commit message:
Don't refresh workers unless they're accepted.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/restrict-refresh/+merge/133531

This was discussed to the smallest meaningful extent: I mentioned that we probably ought to do this, and I believe it was Raphaël who responded that we probably did.

One crucial point is that a starting cluster controller does not request a worker refresh until after it knows it's accepted, so the added restriction won't lead to the request coming in too early and skipping the worker that particularly needs it.  The ordering is covered by existing tests for start_cluster_controller.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/restrict-refresh/+merge/133531
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/restrict-refresh into lp:maas.

=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py	2012-11-08 17:30:47 +0000
+++ src/maasserver/models/nodegroup.py	2012-11-08 17:47:23 +0000
@@ -110,7 +110,7 @@
 
     def refresh_workers(self):
         """Send refresh tasks to all node-group workers."""
-        for nodegroup in self.all():
+        for nodegroup in self.filter(status=NODEGROUP_STATUS.ACCEPTED):
             refresh_worker(nodegroup)
 
     def _mass_change_status(self, old_status, new_status):

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-11-08 12:58:48 +0000
+++ src/maasserver/tests/test_api.py	2012-11-08 17:47:23 +0000
@@ -3588,7 +3588,7 @@
         master.save()
 
     def test_refresh_calls_refresh_worker(self):
-        nodegroup = factory.make_node_group()
+        nodegroup = factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED)
         response = self.client.post(
             reverse('nodegroups_handler'), {'op': 'refresh_workers'})
         self.assertEqual(httplib.OK, response.status_code)
@@ -4027,7 +4027,7 @@
         # In bug 1041158, the worker's upload_leases task tried to call
         # the update_leases API at the wrong URL path.  It has the right
         # path now.
-        nodegroup = factory.make_node_group()
+        nodegroup = factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED)
         refresh_worker(nodegroup)
         self.patch(MAASClient, 'post', Mock())
         leases = factory.make_random_leases()

=== modified file 'src/maasserver/tests/test_nodegroup.py'
--- src/maasserver/tests/test_nodegroup.py	2012-11-08 17:30:47 +0000
+++ src/maasserver/tests/test_nodegroup.py	2012-11-08 17:47:23 +0000
@@ -28,6 +28,7 @@
 from maasserver.testing import reload_object
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
+from maasserver.utils import map_enum
 from maasserver.utils.orm import get_one
 from maasserver.worker_user import get_worker_user
 from maastesting.celery import CeleryFixture
@@ -264,6 +265,20 @@
             ]
         self.assertItemsEqual(calls, recorder.apply_async.call_args_list)
 
+    def test_refresh_workers_refreshes_accepted_cluster_controllers(self):
+        self.patch(nodegroup_module, 'refresh_worker')
+        nodegroup = factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED)
+        NodeGroup.objects.refresh_workers()
+        nodegroup_module.refresh_worker.assert_called_once_with(nodegroup)
+
+    def test_refresh_workers_skips_unaccepted_cluster_controllers(self):
+        self.patch(nodegroup_module, 'refresh_worker')
+        for status in map_enum(NODEGROUP_STATUS).values():
+            if status != NODEGROUP_STATUS.ACCEPTED:
+                factory.make_node_group(status=status)
+        NodeGroup.objects.refresh_workers()
+        self.assertEqual(0, nodegroup_module.refresh_worker.call_count)
+
 
 def make_archive_url(name):
     """Create a fake archive URL."""