diff --git a/ironic/common/hash_ring.py b/ironic/common/hash_ring.py index db3081fcad..40f421424b 100644 --- a/ironic/common/hash_ring.py +++ b/ironic/common/hash_ring.py @@ -114,10 +114,23 @@ class HashRing(object): class HashRingManager(object): + _hash_rings = None + _lock = threading.Lock() + def __init__(self): - self._lock = threading.Lock() self.dbapi = dbapi.get_instance() - self.hash_rings = None + + @property + def ring(self): + # Hot path, no lock + if self._hash_rings is not None: + return self._hash_rings + + with self._lock: + if self._hash_rings is None: + rings = self._load_hash_rings() + self.__class__._hash_rings = rings + return self._hash_rings def _load_hash_rings(self): rings = {} @@ -127,21 +140,14 @@ class HashRingManager(object): rings[driver_name] = HashRing(hosts) return rings - def _ensure_rings_fresh(self): - # Hot path, no lock - # TODO(russell_h): Consider adding time-based invalidation of rings - if self.hash_rings is not None: - return - + @classmethod + def reset(self): with self._lock: - if self.hash_rings is None: - self.hash_rings = self._load_hash_rings() - - def get_hash_ring(self, driver_name): - self._ensure_rings_fresh() + self._hash_rings = None + def __getitem__(self, driver_name): try: - return self.hash_rings[driver_name] + return self.ring[driver_name] except KeyError: - raise exception.DriverNotFound(_("The driver '%s' is unknown.") % - driver_name) + raise exception.DriverNotFound( + _("The driver '%s' is unknown.") % driver_name) diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 9bc50f7368..e9439fb4b0 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -865,7 +865,7 @@ class ConductorManager(periodic_task.PeriodicTasks): take out a lock. """ try: - ring = self.ring_manager.get_hash_ring(driver) + ring = self.ring_manager[driver] except exception.DriverNotFound: return False diff --git a/ironic/conductor/rpcapi.py b/ironic/conductor/rpcapi.py index a2b19e1953..c1f4226f28 100644 --- a/ironic/conductor/rpcapi.py +++ b/ironic/conductor/rpcapi.py @@ -23,7 +23,7 @@ import random from oslo import messaging from ironic.common import exception -from ironic.common import hash_ring as hash +from ironic.common import hash_ring from ironic.common.i18n import _ from ironic.common import rpc from ironic.conductor import manager @@ -75,7 +75,8 @@ class ConductorAPI(object): self.client = rpc.get_client(target, version_cap=self.RPC_API_VERSION, serializer=serializer) - self.ring_manager = hash.HashRingManager() + # NOTE(deva): this is going to be buggy + self.ring_manager = hash_ring.HashRingManager() def get_topic_for(self, node): """Get the RPC topic for the conductor service which the node @@ -87,7 +88,7 @@ class ConductorAPI(object): """ try: - ring = self.ring_manager.get_hash_ring(node.driver) + ring = self.ring_manager[node.driver] dest = ring.get_hosts(node.uuid) return self.topic + "." + dest[0] except exception.DriverNotFound: @@ -105,7 +106,7 @@ class ConductorAPI(object): :raises: DriverNotFound """ - hash_ring = self.ring_manager.get_hash_ring(driver_name) + hash_ring = self.ring_manager[driver_name] host = random.choice(hash_ring.hosts) return self.topic + "." + host diff --git a/ironic/tests/base.py b/ironic/tests/base.py index 49c3d53acf..d3c4d8e936 100644 --- a/ironic/tests/base.py +++ b/ironic/tests/base.py @@ -37,6 +37,7 @@ from oslo.config import cfg from ironic.db.sqlalchemy import migration from ironic.db.sqlalchemy import models +from ironic.common import hash_ring from ironic.common import paths from ironic.db.sqlalchemy import api as sqla_api from ironic.objects import base as objects_base @@ -178,6 +179,7 @@ class TestCase(testtools.TestCase): self.addCleanup(self._restore_obj_registry) self.addCleanup(self._clear_attrs) + self.addCleanup(hash_ring.HashRingManager().reset) self.useFixture(fixtures.EnvironmentVariable('http_proxy')) self.policy = self.useFixture(policy_fixture.PolicyFixture()) CONF.set_override('fatal_exception_format_errors', True) diff --git a/ironic/tests/test_hash_ring.py b/ironic/tests/test_hash_ring.py index 402d211131..53da4a9e46 100644 --- a/ironic/tests/test_hash_ring.py +++ b/ironic/tests/test_hash_ring.py @@ -16,7 +16,7 @@ from oslo.config import cfg from ironic.common import exception -from ironic.common import hash_ring as hash +from ironic.common import hash_ring from ironic.db import api as dbapi from ironic.tests import base from ironic.tests.db import base as db_base @@ -36,45 +36,45 @@ class HashRingTestCase(base.TestCase): def test_create_ring(self): hosts = ['foo', 'bar'] replicas = 2 - ring = hash.HashRing(hosts, replicas=replicas) + ring = hash_ring.HashRing(hosts, replicas=replicas) self.assertEqual(hosts, ring.hosts) self.assertEqual(replicas, ring.replicas) def test_create_with_different_partition_counts(self): hosts = ['foo', 'bar'] CONF.set_override('hash_partition_exponent', 2) - ring = hash.HashRing(hosts) + ring = hash_ring.HashRing(hosts) self.assertEqual(2 ** 2, len(ring.part2host)) CONF.set_override('hash_partition_exponent', 8) - ring = hash.HashRing(hosts) + ring = hash_ring.HashRing(hosts) self.assertEqual(2 ** 8, len(ring.part2host)) CONF.set_override('hash_partition_exponent', 16) - ring = hash.HashRing(hosts) + ring = hash_ring.HashRing(hosts) self.assertEqual(2 ** 16, len(ring.part2host)) def test_distribution_one_replica(self): hosts = ['foo', 'bar', 'baz'] - ring = hash.HashRing(hosts, replicas=1) + ring = hash_ring.HashRing(hosts, replicas=1) self.assertEqual(['foo'], ring.get_hosts('fake')) self.assertEqual(['bar'], ring.get_hosts('fake-again')) def test_distribution_two_replicas(self): hosts = ['foo', 'bar', 'baz'] - ring = hash.HashRing(hosts, replicas=2) + ring = hash_ring.HashRing(hosts, replicas=2) self.assertEqual(['foo', 'bar'], ring.get_hosts('fake')) self.assertEqual(['bar', 'baz'], ring.get_hosts('fake-again')) def test_distribution_three_replicas(self): hosts = ['foo', 'bar', 'baz'] - ring = hash.HashRing(hosts, replicas=3) + ring = hash_ring.HashRing(hosts, replicas=3) self.assertEqual(['foo', 'bar', 'baz'], ring.get_hosts('fake')) self.assertEqual(['bar', 'baz', 'foo'], ring.get_hosts('fake-again')) def test_ignore_hosts(self): hosts = ['foo', 'bar', 'baz'] - ring = hash.HashRing(hosts, replicas=1) + ring = hash_ring.HashRing(hosts, replicas=1) self.assertEqual(['bar'], ring.get_hosts('fake', ignore_hosts=['foo'])) self.assertEqual(['baz'], ring.get_hosts('fake', @@ -84,7 +84,7 @@ class HashRingTestCase(base.TestCase): def test_ignore_hosts_with_replicas(self): hosts = ['foo', 'bar', 'baz'] - ring = hash.HashRing(hosts, replicas=2) + ring = hash_ring.HashRing(hosts, replicas=2) self.assertEqual(['bar', 'baz'], ring.get_hosts('fake', ignore_hosts=['foo'])) self.assertEqual(['baz'], ring.get_hosts('fake', @@ -98,24 +98,24 @@ class HashRingTestCase(base.TestCase): def test_more_replicas_than_hosts(self): hosts = ['foo', 'bar'] - ring = hash.HashRing(hosts, replicas=10) + ring = hash_ring.HashRing(hosts, replicas=10) self.assertEqual(hosts, ring.get_hosts('fake')) def test_ignore_non_existent_host(self): hosts = ['foo', 'bar'] - ring = hash.HashRing(hosts, replicas=1) + ring = hash_ring.HashRing(hosts, replicas=1) self.assertEqual(['foo'], ring.get_hosts('fake', ignore_hosts=['baz'])) def test_create_ring_invalid_data(self): hosts = None self.assertRaises(exception.Invalid, - hash.HashRing, + hash_ring.HashRing, hosts) def test_get_hosts_invalid_data(self): hosts = ['foo', 'bar'] - ring = hash.HashRing(hosts) + ring = hash_ring.HashRing(hosts) self.assertRaises(exception.Invalid, ring.get_hosts, None) @@ -125,7 +125,7 @@ class HashRingManagerTestCase(db_base.DbTestCase): def setUp(self): super(HashRingManagerTestCase, self).setUp() - self.ring_manager = hash.HashRingManager() + self.ring_manager = hash_ring.HashRingManager() self.dbapi = dbapi.get_instance() def register_conductors(self): @@ -140,13 +140,13 @@ class HashRingManagerTestCase(db_base.DbTestCase): def test_hash_ring_manager_get_ring_success(self): self.register_conductors() - ring = self.ring_manager.get_hash_ring('driver1') + ring = self.ring_manager['driver1'] self.assertEqual(sorted(['host1', 'host2']), sorted(ring.hosts)) def test_hash_ring_manager_driver_not_found(self): self.register_conductors() self.assertRaises(exception.DriverNotFound, - self.ring_manager.get_hash_ring, + self.ring_manager.__getitem__, 'driver3') def test_hash_ring_manager_no_refresh(self): @@ -154,9 +154,9 @@ class HashRingManagerTestCase(db_base.DbTestCase): # initialized, it won't be seen. Long term this is probably # undesirable, but today is the intended behavior. self.assertRaises(exception.DriverNotFound, - self.ring_manager.get_hash_ring, + self.ring_manager.__getitem__, 'driver1') self.register_conductors() self.assertRaises(exception.DriverNotFound, - self.ring_manager.get_hash_ring, + self.ring_manager.__getitem__, 'driver1')