From: Assaf Muller Date: Mon, 23 Feb 2015 22:07:30 +0000 (-0500) Subject: Change L3 agent AdvancedService class to be non-singleton X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=911f8b57f80798ec8fe3c82282fb4c812cc9472c;p=openstack-build%2Fneutron-build.git Change L3 agent AdvancedService class to be non-singleton The idea behind the AdvancedServices (Metadata, *aaS) services to be singletons was to (1) offer a way to get an instance of such a service in a convinient manner, and to (2) ensure that only one service registered per service type. (1) Since the AdvancedService.instance method required an instance of a L3 agent, this point was kind of missed. If you have a L3 agent instance in your hand, just use it to access the service you're looking for. (2) This is now fulfilled by asserting that only one driver is registered (per type) in the .add method. The motivation to make them non-singletons is that: a) They don't need to be b) The code is simplified this way c) I've been facing crazy issues during functional testing where we're constantly instantiating L3 agents, and the (singleton) metadata service was referencing the wrong configuration object. The service was re-initialized during a test, screwing up the configuration of other mid-run tests. Closes-Bug: #1425759 Depends-On: I155aece9b7028f1bdd3d9709facef392b38aec27 Depends-On: I199f6a967c64e921624b397fa5b7dfc56572d63a Change-Id: I192e4d12380346c53c8ae0246371ccc41a836c4c --- diff --git a/neutron/agent/l3/agent.py b/neutron/agent/l3/agent.py index 4935614bc..2efbf4b94 100644 --- a/neutron/agent/l3/agent.py +++ b/neutron/agent/l3/agent.py @@ -207,8 +207,8 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, self.use_ipv6 = ipv6_utils.is_enabled() if self.conf.enable_metadata_proxy: - driver = metadata_driver.MetadataDriver.instance(self) - self.event_observers.add(driver) + self.metadata_driver = metadata_driver.MetadataDriver(self) + self.event_observers.add(self.metadata_driver) def _check_config_params(self): """Check items in configuration files. diff --git a/neutron/agent/l3/event_observers.py b/neutron/agent/l3/event_observers.py index fff0a5c1c..ab7c3693b 100644 --- a/neutron/agent/l3/event_observers.py +++ b/neutron/agent/l3/event_observers.py @@ -21,9 +21,14 @@ class L3EventObservers(object): def __init__(self): self.observers = set() - def add(self, observer): + def add(self, new_observer): """Add a listener for L3 agent notifications.""" - self.observers.add(observer) + for observer in self.observers: + if type(observer) == type(new_observer): + raise ValueError('Only a single instance of AdvancedService ' + 'may be registered, per type of service.') + + self.observers.add(new_observer) def notify(self, l3_event_action, *args, **kwargs): """Give interested parties a chance to act on event. diff --git a/neutron/services/advanced_service.py b/neutron/services/advanced_service.py index 4a080799f..fbca6cc0f 100644 --- a/neutron/services/advanced_service.py +++ b/neutron/services/advanced_service.py @@ -13,8 +13,6 @@ # License for the specific language governing permissions and limitations # under the License. -from oslo_concurrency import lockutils - from neutron.openstack.common import log as logging LOG = logging.getLogger(__name__) @@ -27,15 +25,12 @@ class AdvancedService(object): Instead, a child class is defined for each service type and instantiated by the corresponding service agent. The instances will have a back reference to the L3 agent, and will register as an observer of events. - A singleton is used to create only one service object per service type. This base class provides a definition for all of the L3 event handlers that a service could "observe". A child class for a service type will implement handlers, for events of interest. """ - _instance = None - def __init__(self, l3_agent): """Base class for an advanced service. @@ -48,23 +43,6 @@ class AdvancedService(object): # TODO(pcm): Address this in future refactorings. self.conf = l3_agent.conf - @classmethod - def instance(cls, l3_agent): - """Creates instance (singleton) of service. - - Do not directly call this for the base class. Instead, it should be - called by a child class, that represents a specific service type. - - This ensures that only one instance is created for all agents of a - specific service type. - """ - if not cls._instance: - with lockutils.lock('instance'): - if not cls._instance: - cls._instance = cls(l3_agent) - - return cls._instance - # NOTE: Handler definitions for events generated by the L3 agent. # Subclasses of AdvancedService can override these to perform service # specific actions. Unique methods are defined for add/update, as diff --git a/neutron/tests/common/agents/l3_agent.py b/neutron/tests/common/agents/l3_agent.py index fb72c6855..529ecefe0 100644 --- a/neutron/tests/common/agents/l3_agent.py +++ b/neutron/tests/common/agents/l3_agent.py @@ -19,12 +19,6 @@ from neutron.agent.l3 import agent class TestL3NATAgent(agent.L3NATAgentWithStateReport): NESTED_NAMESPACE_SEPARATOR = '@' - def __init__(self, host, conf=None): - super(TestL3NATAgent, self).__init__(host, conf) - self.event_observers.observers = set( - observer.__class__(self) for observer in - self.event_observers.observers) - def get_ns_name(self, router_id): ns_name = super(TestL3NATAgent, self).get_ns_name(router_id) return "%s%s%s" % (ns_name, self.NESTED_NAMESPACE_SEPARATOR, self.host) diff --git a/neutron/tests/unit/agent/test_l3_event_observers.py b/neutron/tests/unit/agent/test_l3_event_observers.py index eb9a18349..eb43ec85b 100644 --- a/neutron/tests/unit/agent/test_l3_event_observers.py +++ b/neutron/tests/unit/agent/test_l3_event_observers.py @@ -14,6 +14,7 @@ # under the License. import mock +import testtools from neutron.agent.l3 import event_observers from neutron.services import advanced_service as adv_svc @@ -45,21 +46,23 @@ class TestL3EventObservers(base.BaseTestCase): self.event_observers.add(observer) self.assertIn(observer, self.event_observers.observers) - def test_add_duplicate_observer_is_ignored(self): - observer = object() + def test_add_duplicate_observer_type_raises(self): + agent = mock.Mock() + observer = DummyService1(agent) self.event_observers.add(observer) - try: - self.event_observers.add(observer) - except Exception: - self.fail('Duplicate additions of observers should be ignored') + + observer2 = DummyService1(agent) + with testtools.ExpectedException(ValueError): + self.event_observers.add(observer2) + self.assertEqual(1, len(self.event_observers.observers)) def test_observers_in_service_notified(self): """Test that correct handlers for multiple services are called.""" l3_agent = mock.Mock() router_info = mock.Mock() - observer1 = DummyService1.instance(l3_agent) - observer2 = DummyService2.instance(l3_agent) + observer1 = DummyService1(l3_agent) + observer2 = DummyService2(l3_agent) observer1_before_add = mock.patch.object( DummyService1, 'before_router_added').start() observer2_before_add = mock.patch.object( diff --git a/neutron/tests/unit/services/test_advanced_service.py b/neutron/tests/unit/services/test_advanced_service.py index 6f282c50e..f063113a0 100644 --- a/neutron/tests/unit/services/test_advanced_service.py +++ b/neutron/tests/unit/services/test_advanced_service.py @@ -34,36 +34,27 @@ class TestAdvancedService(base.BaseTestCase): super(TestAdvancedService, self).setUp() self.agent = mock.Mock() self.test_observers = event_observers.L3EventObservers() - # Ensure no instances for each test - FakeServiceA._instance = None - FakeServiceB._instance = None def test_create_service(self): """Test agent saved and service added to observer list.""" - my_service = FakeServiceA.instance(self.agent) + my_service = FakeServiceA(self.agent) self.test_observers.add(my_service) self.assertIn(my_service, self.test_observers.observers) self.assertEqual(self.agent, my_service.l3_agent) - def test_service_is_singleton(self): - """Test that two services of same time use same instance.""" - a1 = FakeServiceA.instance(self.agent) - a2 = FakeServiceA.instance(self.agent) - self.assertIs(a1, a2) - def test_shared_observers_for_different_services(self): """Test different service type instances created. The services are unique instances, with different agents, but sharing the same observer list. """ - a = FakeServiceA.instance(self.agent) + a = FakeServiceA(self.agent) self.test_observers.add(a) self.assertEqual(self.agent, a.l3_agent) self.assertIn(a, self.test_observers.observers) another_agent = mock.Mock() - b = FakeServiceB.instance(another_agent) + b = FakeServiceB(another_agent) self.test_observers.add(b) self.assertNotEqual(a, b) self.assertEqual(another_agent, b.l3_agent) @@ -76,10 +67,10 @@ class TestAdvancedService(base.BaseTestCase): The services are unique instances, shared the same agent, but are using different observer lists. """ - a = FakeServiceA.instance(self.agent) + a = FakeServiceA(self.agent) self.test_observers.add(a) other_observers = event_observers.L3EventObservers() - b = FakeServiceB.instance(self.agent) + b = FakeServiceB(self.agent) other_observers.add(b) self.assertNotEqual(a, b)