From 9e3de04802d4ef49cef69eb5c78ae7a7c2edc65c Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Wed, 10 Dec 2014 10:37:43 +0000 Subject: [PATCH] applied post-review changes --- hooks/swift_hooks.py | 3 +- hooks/swift_utils.py | 76 +++++++++++++++------------------- unit_tests/test_swift_utils.py | 34 ++++++++------- 3 files changed, 53 insertions(+), 60 deletions(-) diff --git a/hooks/swift_hooks.py b/hooks/swift_hooks.py index 3b76ede..0617185 100755 --- a/hooks/swift_hooks.py +++ b/hooks/swift_hooks.py @@ -21,7 +21,6 @@ from swift_utils import ( balance_rings, fully_synced, sync_proxy_rings, - update_min_part_hours, broadcast_rings_available, mark_www_rings_deleted, SwiftProxyClusterRPC, @@ -126,7 +125,7 @@ def config_changed(): if openstack.openstack_upgrade_available('python-swift'): do_openstack_upgrade(CONFIGS) - update_min_part_hours() + update_rings(min_part_hours=config('min-hours')) if not config('disable-ring-balance') and is_elected_leader(SWIFT_HA_RES): # Try ring balance. If rings are balanced, no sync will occur. diff --git a/hooks/swift_utils.py b/hooks/swift_utils.py index d164204..bdf8646 100644 --- a/hooks/swift_utils.py +++ b/hooks/swift_utils.py @@ -455,7 +455,7 @@ def set_min_part_hours(path, value): def get_zone(assignment_policy): - """Determine the appropriate zone depending on configured assignment policy. + """Determine appropriate zone based on configured assignment policy. Manual assignment relies on each storage zone being deployed as a separate service unit with its desired zone set as a configuration @@ -717,19 +717,47 @@ def sync_builders_and_rings_if_changed(f): @sync_builders_and_rings_if_changed -def update_rings(node_settings=None): - """Update builder with node settings and balance rings if necessary.""" +def update_rings(node_settings=None, min_part_hours=None): + """Update builder with node settings and balance rings if necessary. + + Also update min_part_hours if provided. + """ if not is_elected_leader(SWIFT_HA_RES): log("Update rings called by non-leader - skipping", level=WARNING) return + balance_required = False + + if min_part_hours: + # NOTE: no need to stop the proxy since we are not changing the rings, + # only the builder. + + # Only update if all exist + if all([os.path.exists(p) for p in SWIFT_RINGS.itervalues()]): + for ring, path in SWIFT_RINGS.iteritems(): + current_min_part_hours = get_min_part_hours(path) + if min_part_hours != current_min_part_hours: + log("Setting ring %s min_part_hours to %s" % + (ring, min_part_hours), level=INFO) + try: + set_min_part_hours(path, min_part_hours) + except SwiftProxyCharmException as exc: + # TODO: ignore for now since this should not be + # critical but in the future we should support a + # rollback. + log(str(exc), level=WARNING) + else: + balance_required = True + if node_settings: if 'device' in node_settings: for ring in SWIFT_RINGS.itervalues(): if not exists_in_ring(ring, node_settings): add_to_ring(ring, node_settings) + balance_required = True - balance_rings() + if balance_required: + balance_rings() @sync_builders_and_rings_if_changed @@ -852,8 +880,8 @@ def cluster_sync_rings(peers_only=False, builders_only=False): def notify_storage_rings_available(): - """Notify peer swift-storage relations that they should synchronise ring and - builder files. + """Notify peer swift-storage relations that they should synchronise ring + and builder files. Note that this should only be called from the leader unit. """ @@ -905,39 +933,3 @@ def get_hostaddr(): return get_ipv6_addr(exc_list=[config('vip')])[0] return unit_get('private-address') - - -def update_min_part_hours(): - """Update the min_part_hours setting on swift rings. - - This should only be called by the leader unit. Once update has been - performed and if setting has changed, rings will be resynced across the - cluster. - """ - if not is_elected_leader(SWIFT_HA_RES): - # Only the leader can do this. - return - - # NOTE: no need to stop the proxy since we are not changing the rings, - # only the builder. - - new_min_part_hours = config('min-hours') - resync_builders = False - # Only update if all exist - if all([os.path.exists(p) for p in SWIFT_RINGS.itervalues()]): - for ring, path in SWIFT_RINGS.iteritems(): - min_part_hours = get_min_part_hours(path) - if min_part_hours != new_min_part_hours: - log("Setting ring %s min_part_hours to %s" % - (ring, new_min_part_hours), level=INFO) - try: - set_min_part_hours(path, new_min_part_hours) - except SwiftProxyCharmException as exc: - # TODO: ignore for now since this should not be critical - # but in the future we should support a rollback. - log(str(exc), level=WARNING) - else: - resync_builders = True - - if resync_builders: - balance_rings() diff --git a/unit_tests/test_swift_utils.py b/unit_tests/test_swift_utils.py index ac80523..740ae39 100644 --- a/unit_tests/test_swift_utils.py +++ b/unit_tests/test_swift_utils.py @@ -21,51 +21,53 @@ def init_ring_paths(tmpdir): class SwiftUtilsTestCase(unittest.TestCase): - @mock.patch('swift_utils.sync_builders_and_rings_if_changed') + @mock.patch('swift_utils.get_builders_checksum') + @mock.patch('swift_utils.get_rings_checksum') @mock.patch('swift_utils.balance_rings') @mock.patch('swift_utils.log') @mock.patch('swift_utils.os.path.exists') @mock.patch('swift_utils.is_elected_leader') - @mock.patch('swift_utils.config') @mock.patch('swift_utils.get_min_part_hours') @mock.patch('swift_utils.set_min_part_hours') - def test_update_min_part_hours(self, mock_set_min_hours, - mock_get_min_hours, mock_config, - mock_is_elected_leader, mock_path_exists, - mock_log, mock_balance_rings, - mock_sync_builders_and_rings_if_changed): + def test_update_rings(self, mock_set_min_hours, + mock_get_min_hours, + mock_is_elected_leader, mock_path_exists, + mock_log, mock_balance_rings, + mock_get_rings_checksum, + mock_get_builders_checksum): + + # Make sure same is returned for both so that we don't try to sync + mock_get_rings_checksum.return_value = None + mock_get_builders_checksum.return_value = None # Test blocker 1 mock_is_elected_leader.return_value = False - swift_utils.update_min_part_hours() - self.assertFalse(mock_config.called) + swift_utils.update_rings() self.assertFalse(mock_balance_rings.called) # Test blocker 2 mock_path_exists.return_value = False mock_is_elected_leader.return_value = True - swift_utils.update_min_part_hours() - self.assertTrue(mock_config.called) + swift_utils.update_rings() self.assertFalse(mock_get_min_hours.called) self.assertFalse(mock_balance_rings.called) # Test blocker 3 mock_path_exists.return_value = True mock_is_elected_leader.return_value = True - mock_config.return_value = 10 mock_get_min_hours.return_value = 10 - swift_utils.update_min_part_hours() + swift_utils.update_rings(min_part_hours=10) self.assertTrue(mock_get_min_hours.called) self.assertFalse(mock_set_min_hours.called) self.assertFalse(mock_balance_rings.called) + mock_get_min_hours.reset_mock() + # Test go through mock_path_exists.return_value = True mock_is_elected_leader.return_value = True - mock_config.return_value = 10 mock_get_min_hours.return_value = 0 - swift_utils.update_min_part_hours() - self.assertTrue(mock_config.called) + swift_utils.update_rings(min_part_hours=10) self.assertTrue(mock_get_min_hours.called) self.assertTrue(mock_set_min_hours.called) self.assertTrue(mock_balance_rings.called)