openstack
/
neutron
Code Issues Proposed changes

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
This commit is contained in:
Assaf Muller 2015-02-23 17:07:30 -05:00
parent 194afde3a6
commit 911f8b57f8
6 changed files with 25 additions and 54 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
neutron/agent/l3/agent.py
View File

@ -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.

9
neutron/agent/l3/event_observers.py
View File

@ -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.

22
neutron/services/advanced_service.py
View File

@ -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

6
neutron/tests/common/agents/l3_agent.py
View File

@ -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)

19
neutron/tests/unit/agent/test_l3_event_observers.py
View File

@ -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(

19
neutron/tests/unit/services/test_advanced_service.py
View File

@ -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)