From f25d2c2d7f721cc12e04c63e1a180eeda2d64a7b Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Thu, 6 Dec 2018 14:08:53 +0000 Subject: [PATCH] Cleanup ring manager storage relation settings If local unit is no longer leader, clear rings_url on storage relations to avoid storage units getting rings from wrong proxy unit. Also send broker-timestamp to storage units when providing rings_url so that we have a means of knowing which is the most recent. Broker timestamp is the same for peer and storage sync so this enables identifying most recent. Change-Id: I2c7e9028f345791bad0a736cb89979284b144e33 Closes-Bug: #1765203 --- hooks/swift_hooks.py | 3 +++ lib/swift_utils.py | 42 ++++++++++++++++++++++++----- unit_tests/test_swift_hooks.py | 5 +++- unit_tests/test_swift_utils.py | 48 +++++++++++++++++++++++++++++++--- 4 files changed, 87 insertions(+), 11 deletions(-) diff --git a/hooks/swift_hooks.py b/hooks/swift_hooks.py index 80dc9a8..c4723c9 100755 --- a/hooks/swift_hooks.py +++ b/hooks/swift_hooks.py @@ -64,6 +64,7 @@ from lib.swift_utils import ( timestamps_available, assess_status, try_initialize_swauth, + clear_storage_rings_available, ) import charmhelpers.contrib.openstack.utils as openstack @@ -251,6 +252,8 @@ def storage_joined(rid=None): # possibility of storage nodes getting out-of-date rings by deprecating # any existing ones from the www dir. mark_www_rings_deleted() + clear_storage_rings_available() + try_initialize_swauth() # NOTE(jamespage): Ensure private-address is set to any network-space diff --git a/lib/swift_utils.py b/lib/swift_utils.py index c6d0073..a256b99 100644 --- a/lib/swift_utils.py +++ b/lib/swift_utils.py @@ -261,12 +261,16 @@ class SwiftProxyClusterRPC(object): rq['peers-only'] = echo_peers_only return rq - def sync_rings_request(self, broker_token, builders_only=False): + def sync_rings_request(self, broker_token, broker_timestamp, + builders_only=False): """Request for peers to sync rings from leader. NOTE: this action must only be performed by the cluster leader. :param broker_token: token to identify sync request. + @param broker_timestamp: timestamp for peer and storage sync - this + MUST bethe same as the one used for storage + sync. :param builders_only: if False, tell peers to sync builders only (not rings). """ @@ -281,7 +285,7 @@ class SwiftProxyClusterRPC(object): rq['sync-only-builders'] = 1 rq['broker-token'] = broker_token - rq['broker-timestamp'] = "{:f}".format(time.time()) + rq['broker-timestamp'] = broker_timestamp rq['builder-broker'] = self._hostname return rq @@ -893,11 +897,15 @@ def mark_www_rings_deleted(): os.rename(path, "{}.deleted".format(path)) -def notify_peers_builders_available(broker_token, builders_only=False): +def notify_peers_builders_available(broker_token, broker_timestamp, + builders_only=False): """Notify peer swift-proxy units that they should synchronise ring and builder files. Note that this should only be called from the leader unit. + + @param broker_timestamp: timestamp for peer and storage sync - this MUST be + the same as the one used for storage sync. """ if not is_elected_leader(SWIFT_HA_RES): log("Ring availability peer broadcast requested by non-leader - " @@ -922,6 +930,7 @@ def notify_peers_builders_available(broker_token, builders_only=False): log("Notifying peer(s) that {} are ready for sync." .format(_type), level=INFO) rq = SwiftProxyClusterRPC().sync_rings_request(broker_token, + broker_timestamp, builders_only=builders_only) for rid in cluster_rids: log("Notifying rid={} ({})".format(rid, rq), level=DEBUG) @@ -935,16 +944,21 @@ def broadcast_rings_available(storage=True, builders_only=False, We can opt to only notify peer or storage relations if needs be. """ + + # NOTE: this MUST be the same for peer and storage sync + broker_timestamp = "{:f}".format(time.time()) + if storage: # TODO: get ack from storage units that they are synced before # syncing proxies. - notify_storage_rings_available() + notify_storage_rings_available(broker_timestamp) else: log("Skipping notify storage relations", level=DEBUG) # Always set peer info even if not clustered so that info is present when # units join notify_peers_builders_available(broker_token, + broker_timestamp, builders_only=builders_only) @@ -987,11 +1001,14 @@ def cluster_sync_rings(peers_only=False, builders_only=False, token=None): relation_set(relation_id=rid, relation_settings=rq) -def notify_storage_rings_available(): +def notify_storage_rings_available(broker_timestamp): """Notify peer swift-storage relations that they should synchronise ring and builder files. Note that this should only be called from the leader unit. + + @param broker_timestamp: timestamp for peer and storage sync - this MUST be + the same as the one used for peer sync. """ if not is_elected_leader(SWIFT_HA_RES): log("Ring availability storage-relation broadcast requested by " @@ -1002,13 +1019,26 @@ def notify_storage_rings_available(): hostname = format_ipv6_addr(hostname) or hostname path = os.path.basename(get_www_dir()) rings_url = 'http://{}/{}'.format(hostname, path) + + # TODO(hopem): consider getting rid of this trigger since the timestamp + # should do the job. trigger = uuid.uuid4() + # Notify storage nodes that there is a new ring to fetch. log("Notifying storage nodes that new rings are ready for sync.", level=INFO) for relid in relation_ids('swift-storage'): relation_set(relation_id=relid, swift_hash=get_swift_hash(), - rings_url=rings_url, trigger=trigger) + rings_url=rings_url, broker_timestamp=broker_timestamp, + trigger=trigger) + + +def clear_storage_rings_available(): + """Clear the rings_url setting so that storage units don't accidentally try + to sync from this unit (which is presumably no longer the leader). + """ + for relid in relation_ids('swift-storage'): + relation_set(relation_id=relid, rings_url=None) def fully_synced(): diff --git a/unit_tests/test_swift_hooks.py b/unit_tests/test_swift_hooks.py index a2d06f9..064fc86 100644 --- a/unit_tests/test_swift_hooks.py +++ b/unit_tests/test_swift_hooks.py @@ -239,6 +239,7 @@ class SwiftHooksTestCase(unittest.TestCase): relation_set.assert_called_once_with( relation_id='rid:23', rel_data='data') + @patch.object(swift_hooks, 'clear_storage_rings_available') @patch.object(swift_hooks, 'is_elected_leader') @patch.object(swift_hooks, 'get_relation_ip') @patch.object(swift_hooks, 'try_initialize_swauth') @@ -249,7 +250,8 @@ class SwiftHooksTestCase(unittest.TestCase): mark_www_rings_deleted, try_initialize_swauth, get_relation_ip, - is_elected_leader): + is_elected_leader, + mock_clear_storage_rings_available): is_elected_leader.return_value = False get_relation_ip.return_value = '10.10.20.243' swift_hooks.storage_joined(rid='swift-storage:23') @@ -259,3 +261,4 @@ class SwiftHooksTestCase(unittest.TestCase): relation_settings={'private-address': '10.10.20.243'} ) try_initialize_swauth.assert_called_once() + mock_clear_storage_rings_available.assert_called_once() diff --git a/unit_tests/test_swift_utils.py b/unit_tests/test_swift_utils.py index 5579890..a715737 100644 --- a/unit_tests/test_swift_utils.py +++ b/unit_tests/test_swift_utils.py @@ -311,13 +311,11 @@ class SwiftUtilsTestCase(unittest.TestCase): @mock.patch('lib.swift_utils.is_elected_leader', lambda arg: True) @mock.patch.object(swift_utils, 'get_hostaddr', lambda *args: '1.2.3.4') - @mock.patch.object(swift_utils, 'time') @mock.patch('lib.swift_utils.uuid') - def test_cluster_rpc_sync_request(self, mock_uuid, mock_time): - mock_time.time = mock.Mock(return_value=float(1.234)) + def test_cluster_rpc_sync_request(self, mock_uuid): mock_uuid.uuid4.return_value = 'token2' rpc = swift_utils.SwiftProxyClusterRPC() - rq = rpc.sync_rings_request('token1') + rq = rpc.sync_rings_request('token1', '1.234000') self.assertEqual({'trigger': 'token2', 'broker-token': 'token1', 'broker-timestamp': '1.234000', @@ -512,3 +510,45 @@ class SwiftUtilsTestCase(unittest.TestCase): 'http://localhost:8080/auth', '-K', 'Test']) + + @mock.patch.object(swift_utils.uuid, 'uuid4') + @mock.patch.object(swift_utils, 'relation_set') + @mock.patch.object(swift_utils, 'get_swift_hash') + @mock.patch.object(swift_utils, 'log') + @mock.patch.object(swift_utils, 'relation_ids') + @mock.patch.object(swift_utils, 'get_www_dir') + @mock.patch.object(swift_utils, 'format_ipv6_addr') + @mock.patch.object(swift_utils, 'get_hostaddr') + @mock.patch.object(swift_utils, 'is_elected_leader') + def test_notify_storage_rings_available(self, mock_is_leader, + mock_get_hostaddr, + mock_format_ipv6_addr, + mock_get_www_dir, + mock_relation_ids, + mock_log, + mock_get_swift_hash, + mock_relation_set, + mock_uuid): + mock_is_leader.return_value = True + mock_get_hostaddr.return_value = '10.0.0.1' + mock_format_ipv6_addr.return_value = None + mock_get_www_dir.return_value = 'some/dir' + mock_relation_ids.return_value = ['storage:0'] + mock_get_swift_hash.return_value = 'greathash' + mock_uuid.return_value = 'uuid-1234' + swift_utils.notify_storage_rings_available('1.234') + mock_relation_set.assert_called_once_with( + broker_timestamp='1.234', + relation_id='storage:0', + rings_url='http://10.0.0.1/dir', + swift_hash='greathash', + trigger='uuid-1234') + + @mock.patch.object(swift_utils, 'relation_set') + @mock.patch.object(swift_utils, 'relation_ids') + def test_clear_notify_storage_rings_available(self, mock_relation_ids, + mock_relation_set): + mock_relation_ids.return_value = ['storage:0'] + swift_utils.clear_storage_rings_available() + mock_relation_set.assert_called_once_with( + relation_id='storage:0', rings_url=None)