From a991b80f93f95e71c76e2d98aa25b210b7bb7ba9 Mon Sep 17 00:00:00 2001 From: James McGuinness Date: Tue, 6 Aug 2013 17:13:47 -0700 Subject: [PATCH] improved _build, update_node_pool to be more pythonic --- tests/core/service_test.py | 32 ++++++++++---------------------- tron/core/service.py | 27 ++++++++++++++++++++------- tron/core/serviceinstance.py | 14 ++++++++------ 3 files changed, 38 insertions(+), 35 deletions(-) diff --git a/tests/core/service_test.py b/tests/core/service_test.py index 8a39bdbbb..01f12371e 100644 --- a/tests/core/service_test.py +++ b/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) diff --git a/tron/core/service.py b/tron/core/service.py index 1d7086e4a..1034a4c2f 100644 --- a/tron/core/service.py +++ b/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) diff --git a/tron/core/serviceinstance.py b/tron/core/serviceinstance.py index 5e063a397..abc34b8e6 100644 --- a/tron/core/serviceinstance.py +++ b/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."""