improved _build, update_node_pool to be more pythonic

tests/core/service_test.py

@@ -154,34 +154,22 @@ def test_update_node_pool_enabled(self):
         self.service.enabled = True
         node_pool = mock.Mock()
 
-        with mock.patch('tron.node.NodePoolRepository', autospec=True) as pool_patch:
-            get_mock = pool_patch.get_instance().get_by_name
-            get_mock.configure_mock(return_value=node_pool)
+        self.service.update_node_pool(node_pool)
 
-            self.service.update_node_pool()
-
-            assert_equal(pool_patch.get_instance.call_count, 2)
-            get_mock.assert_called_once_with(self.service.config.node)
-            self.service.instances.update_node_pool.assert_called_once_with(node_pool)
-            self.service.instances.clear_extra.assert_called_once_with()
-            self.service.repair.assert_called_once_with()
+        self.service.instances.update_node_pool.assert_called_once_with(node_pool)
+        self.service.instances.clear_extra.assert_called_once_with()
+        self.service.repair.assert_called_once_with()
 
     def test_update_node_pool_disabled(self):
         autospec_method(self.service.repair)
         self.service.enabled = False
         node_pool = mock.Mock()
 
-        with mock.patch('tron.node.NodePoolRepository', autospec=True) as pool_patch:
-            get_mock = pool_patch.get_instance().get_by_name
-            get_mock.configure_mock(return_value=node_pool)
-
-            self.service.update_node_pool()
+        self.service.update_node_pool(node_pool)
 
-            assert_equal(pool_patch.get_instance.call_count, 2)
-            get_mock.assert_called_once_with(self.service.config.node)
-            self.service.instances.update_node_pool.assert_called_once_with(node_pool)
-            self.service.instances.clear_extra.assert_called_once_with()
-            assert not self.service.repair.called
+        self.service.instances.update_node_pool.assert_called_once_with(node_pool)
+        self.service.instances.clear_extra.assert_called_once_with()
+        assert not self.service.repair.called
 
 
 class ServiceCollectionTestCase(TestCase):
@@ -218,7 +206,7 @@ def test_build_with_same_config(self):
         as get_patch:
             assert self.collection._build(new_service)
             get_patch.assert_called_once_with(config.name)
-            old_service.update_node_pool.assert_called_once_with()
+            old_service.update_node_pool.assert_called_once_with(new_service.instances.node_pool)
             assert_equal(old_service.instances.context, new_service.instances.context)
 
     def test_build_with_diff_count(self):
@@ -239,7 +227,7 @@ def test_build_with_diff_count(self):
         as get_patch:
             assert self.collection._build(new_service)
             get_patch.assert_called_once_with(new_service.config.name)
-            old_service.update_node_pool.assert_called_once_with()
+            old_service.update_node_pool.assert_called_once_with(new_service.instances.node_pool)
             assert_equal(old_service.instances.context, new_service.instances.context)
 
     @mock.patch('tron.core.service.Service', autospec=True)

tron/core/service.py

@@ -156,10 +156,7 @@ def restore_state(self, state_data):
         (self.enable if state_data.get('enabled') else self.disable)()
         self.event_recorder.info("restored")
 
-    def update_node_pool(self):
-        node_repo = node.NodePoolRepository.get_instance()
-        node_pool = node_repo.get_by_name(self.config.node)
-
+    def update_node_pool(self, node_pool):
         self.instances.update_node_pool(node_pool)
         self.instances.clear_extra()
         if self.enabled:
@@ -173,14 +170,30 @@ def __init__(self):
         self.services = collections.MappingCollection('services')
 
     def _build(self, new_service):
+        """A method to be used as an update function for MappingCollection.add.
+        This function attempts to load an old Service object, and if one
+        exists, see if we don't actually have to use an entirely new
+        Service object on reconfiguration.
+
+        To do this, we first check if the number of instances (config.count) is
+        different, as we have a method to fix this when updating the service's
+        node pool. Then, if the configs are now equal, we can simply update
+        the node pool of the old Service object and be done- no need for the
+        new Service object. Otherwise, we use the new object as normal.
+        """
         old_service = self.get_by_name(new_service.config.name)
-        if old_service and old_service.config.count != new_service.config.count:
+
+        if not old_service:
+            log.debug("Building new service %s", new_service.config.name)
+            return False
+
+        if old_service.config.count != new_service.config.count:
             old_service.config.count = new_service.config.count
 
-        if old_service and old_service.config == new_service.config:
+        if old_service.config == new_service.config:
             log.debug("Updating service %s\'s node pool" % new_service.config.name)
             old_service.instances.context = new_service.instances.context
-            old_service.update_node_pool()
+            old_service.update_node_pool(new_service.instances.node_pool)
             return True
         else:
             log.debug("Building new service %s", new_service.config.name)

tron/core/serviceinstance.py

@@ -464,18 +464,20 @@ def get_by_number(self, instance_number):
     def update_node_pool(self, node_pool):
         """Attempt to load a new node pool from the NodePoolRepository, and
         remove instances that no longer have their node in the NodePool."""
-        if node_pool != self.node_pool:
-            self.node_pool = node_pool
-            needs_new_node = []
+        if node_pool == self.node_pool:
+            return
+
+        self.node_pool = node_pool
 
+        def _trim_old_nodes():
             for instance in self.instances:
                 new_node = self.node_pool.get_by_name(instance.node.name)
                 if new_node != instance.node:
                     instance.stop()
-                    needs_new_node.append(instance)
+                else:
+                    yield instance
 
-            self.instances = [i for i in self.instances if
-                              i not in needs_new_node]
+        self.instances = list(_trim_old_nodes())
 
     def clear_extra(self):
         """Clear out instances if too many exist."""