From 498d3b0f1e8e3627de50ed44ab7793af595f4b0e Mon Sep 17 00:00:00 2001 From: "Erlon R. Cruz" Date: Wed, 18 Nov 2020 15:37:14 -0300 Subject: [PATCH] Fix swift port replication configuration Port replication configs are not being set into the ring files. When replication port configs (account|container|object)-server-port-rep, are changed in the swift-storage charm, swift-storage changes the related configs in the config files, but that does not update the rings. This patch adds a function that runs in every config-change triggered from the swift-storage nodes and make sure any replication config is written to the ring and distributed to all nodes. Partial-bug: #1903762 Change-Id: I87eb23de94e3f2f5b06d44df1f8bd9d2324456a0 --- lib/swift_utils.py | 103 +++++++++++++++++++++++++++++---- unit_tests/test_swift_utils.py | 81 +++++++++++++++++++++++--- 2 files changed, 166 insertions(+), 18 deletions(-) diff --git a/lib/swift_utils.py b/lib/swift_utils.py index 9c8e45b..b0a4106 100644 --- a/lib/swift_utils.py +++ b/lib/swift_utils.py @@ -216,6 +216,21 @@ class SwiftProxyCharmException(Exception): pass +def nodes_have_rep_data(nodes=None): + """ Checks if data received from remote nodes are compatible and have + replication data. + + """ + for node in nodes: + for field in ['object_port_rep', 'container_port_rep', + 'account_port_rep', 'ip_rep']: + try: + _ = node[field] + except KeyError: + return False + return True + + class SwiftProxyClusterRPC(object): """Provides cluster relation rpc dicts. @@ -582,6 +597,39 @@ def add_to_ring(ring_path, node): log(msg, level=INFO) +def update_ring_node_ports(ring_name, ring_path, node): + """ Updates ring port and replication for each device node + + :param ring_name: account, container or object + :type ring_name: str + :param ring_path: path to the ring + :type ring_path: str + :param node: device node + :type node: dict + :raises: SwiftProxyCharmException + """ + + log("Updating ports and replication ports for {} ring.".format(ring_name), + level=INFO) + + port_field_name = "{}_port".format(ring_name) + portrep_field_name = "{}_port_rep".format(ring_name) + cmd = ['swift-ring-builder', ring_path, 'set_info', + '--ip', node['ip'], + '--replication-ip', node['ip_rep'], + '--device', node['device'], + '--change-port', str(node[port_field_name]), + '--change-replication-port', str(node[portrep_field_name])] + try: + subprocess.check_call(cmd) + except subprocess.CalledProcessError as e: + raise SwiftProxyCharmException( + "Failed to update device (host={}, rep_host={}, " + "dev_name={}) on {} ring: {}".format( + node['ip'], node['ip_rep'], node['device'], + ring_name, e)) + + def remove_from_ring(ring_path, search_value): """ Removes the device(s) from the ring. @@ -907,7 +955,7 @@ def sync_builders_and_rings_if_changed(f): @functools.wraps(f) def _inner_sync_builders_and_rings_if_changed(*args, **kwargs): if not is_elected_leader(SWIFT_HA_RES): - log("Sync rings called by non-leader - skipping", level=WARNING) + log("Sync rings called by non-leader - skipping", level=INFO) return try: @@ -959,10 +1007,13 @@ def update_rings(nodes=None, min_part_hours=None, replicas=None): 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) + log("Update rings called by non-leader - skipping", level=INFO) return balance_required = False + rep_enabled = False + if nodes is not None: + rep_enabled = nodes_have_rep_data(nodes) if min_part_hours is not None: # NOTE: no need to stop the proxy since we are not changing the rings, @@ -970,11 +1021,11 @@ def update_rings(nodes=None, min_part_hours=None, replicas=None): # Only update if all exist if all(os.path.exists(p) for p in SWIFT_RINGS.values()): - for ring, path in SWIFT_RINGS.items(): + for ring_path, path in SWIFT_RINGS.items(): current_min_part_hours = get_min_part_hours(path) if min_part_hours != current_min_part_hours: log("Setting ring {} min_part_hours to {}" - .format(ring, min_part_hours), level=INFO) + .format(ring_path, min_part_hours), level=INFO) try: set_min_part_hours(path, min_part_hours) except SwiftProxyCharmException as exc: @@ -985,20 +1036,29 @@ def update_rings(nodes=None, min_part_hours=None, replicas=None): else: balance_required = True + log("Updading rings: nodes={}".format(nodes), level=DEBUG) if nodes is not None: for node in nodes: - for ring in SWIFT_RINGS.values(): - if not exists_in_ring(ring, node): - add_to_ring(ring, node) + for ring_path, path in SWIFT_RINGS.items(): + if not exists_in_ring(path, node): + add_to_ring(path, node) balance_required = True + if rep_enabled: + update_ring_node_ports(ring_path, path, node) if replicas is not None: - for ring, path in SWIFT_RINGS.items(): + for ring_path, path in SWIFT_RINGS.items(): current_replicas = get_current_replicas(path) if replicas != current_replicas: update_replicas(path, replicas) balance_required = True + if rep_enabled: + # Updates the ring with the builder contents even if re-balance is not + # needed. That makes sure that ports and replication ports changes are + # written into the ring. See Bug #1903762. + write_rings() + if balance_required: balance_rings() @@ -1053,11 +1113,32 @@ def update_replicas(path, replicas): "Failed to set replicas={} on {}".format(replicas, path)) +@sync_builders_and_rings_if_changed +def write_rings(): + """Write any change to builder files to the rings""" + if not is_elected_leader(SWIFT_HA_RES): + log("Balance rings called by non-leader - skipping", level=INFO) + return + + log("Writing rings from builder files", level=INFO) + for path in SWIFT_RINGS.values(): + cmd = ['swift-ring-builder', path, 'write_ring'] + try: + subprocess.check_output(cmd) + except subprocess.CalledProcessError as e: + # During swift-proxy install the ring does not yet have any nodes. + if "Unable to write empty ring" in str(e.output): + pass + else: + raise SwiftProxyCharmException( + "Failed to set write ring {}".format(path)) + + @sync_builders_and_rings_if_changed def balance_rings(): """Rebalance each ring and notify peers that new rings are available.""" if not is_elected_leader(SWIFT_HA_RES): - log("Balance rings called by non-leader - skipping", level=WARNING) + log("Balance rings called by non-leader - skipping", level=INFO) return if not should_balance([r for r in SWIFT_RINGS.values()]): @@ -1102,7 +1183,7 @@ def notify_peers_builders_available(broker_token, broker_timestamp, """ if not is_elected_leader(SWIFT_HA_RES): log("Ring availability peer broadcast requested by non-leader - " - "skipping", level=WARNING) + "skipping", level=INFO) return if not broker_token: @@ -1205,7 +1286,7 @@ def notify_storage_and_consumers_rings_available(broker_timestamp): """ if not is_elected_leader(SWIFT_HA_RES): log("Ring availability storage-relation broadcast requested by " - "non-leader - skipping", level=WARNING) + "non-leader - skipping", level=INFO) return hostname = get_hostaddr() diff --git a/unit_tests/test_swift_utils.py b/unit_tests/test_swift_utils.py index 1853d2a..7f7854b 100644 --- a/unit_tests/test_swift_utils.py +++ b/unit_tests/test_swift_utils.py @@ -72,18 +72,21 @@ class SwiftUtilsTestCase(unittest.TestCase): @mock.patch('lib.swift_utils.is_elected_leader') @mock.patch('lib.swift_utils.get_min_part_hours') @mock.patch('lib.swift_utils.set_min_part_hours') - 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, mock_update_www_rings, - mock_previously_synced): + @mock.patch('lib.swift_utils.write_rings') + @mock.patch('lib.swift_utils.nodes_have_rep_data') + def test_update_rings(self, + mock_nodes_have_rep_data, + mock_write_rings, 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, + mock_update_www_rings, mock_previously_synced): # 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 mock_previously_synced.return_value = True + mock_nodes_have_rep_data.return_value = True # Test blocker 1 mock_is_elected_leader.return_value = False @@ -116,13 +119,20 @@ class SwiftUtilsTestCase(unittest.TestCase): self.assertTrue(mock_get_min_hours.called) self.assertTrue(mock_set_min_hours.called) self.assertTrue(mock_balance_rings.called) + mock_write_rings.assert_not_called() @mock.patch('lib.swift_utils.previously_synced') @mock.patch('lib.swift_utils.balance_rings') @mock.patch('lib.swift_utils.add_to_ring') @mock.patch('lib.swift_utils.exists_in_ring') @mock.patch('lib.swift_utils.is_elected_leader') + @mock.patch('lib.swift_utils.update_ring_node_ports') + @mock.patch('lib.swift_utils.write_rings') + @mock.patch('lib.swift_utils.nodes_have_rep_data') def test_update_rings_multiple_devs(self, + mock_nodes_have_rep_data, + mock_write_rings, + mock_update_ring_node_ports, mock_is_leader_elected, mock_exists_in_ring, mock_add_to_ring, @@ -148,6 +158,7 @@ class SwiftUtilsTestCase(unittest.TestCase): mock_is_leader_elected.return_value = True mock_previously_synced.return_value = True mock_exists_in_ring.side_effect = lambda *args: False + mock_nodes_have_rep_data.return_value = True swift_utils.update_rings(nodes) calls = [mock.call(os.path.join(swift_utils.SWIFT_CONF_DIR, @@ -207,6 +218,8 @@ class SwiftUtilsTestCase(unittest.TestCase): mock_exists_in_ring.assert_has_calls(calls) mock_balance_rings.assert_called_once_with() mock_add_to_ring.assert_called() + mock_update_ring_node_ports.assert_called() + mock_write_rings.assert_called() # try re-adding, assert add_to_ring was not called mock_add_to_ring.reset_mock() @@ -283,6 +296,60 @@ class SwiftUtilsTestCase(unittest.TestCase): 'set_replicas', '3']) + @mock.patch('lib.swift_utils.is_elected_leader', lambda arg: True) + @mock.patch('lib.swift_utils.previously_synced') + @mock.patch.object(subprocess, 'check_output') + def test_write_rings(self, check_call, mock_previously_synced): + swift_utils.write_rings() + expected_calls = [] + for path in swift_utils.SWIFT_RINGS.values(): + expected_calls.append(mock.call(['swift-ring-builder', + path, 'write_ring'])) + check_call.assert_has_calls(expected_calls, any_order=True) + + @mock.patch('lib.swift_utils.is_elected_leader', lambda arg: True) + @mock.patch('lib.swift_utils.previously_synced') + @mock.patch.object(subprocess, 'check_output') + def test_write_rings_exception_pass(self, check_call, + mock_previously_synced): + my_execp = subprocess.CalledProcessError( + 2, "swift-ring-builder", output='Unable to write empty ring.') + check_call.side_effect = (my_execp, my_execp, my_execp) + swift_utils.write_rings() + + @mock.patch('lib.swift_utils.is_elected_leader', lambda arg: True) + @mock.patch('lib.swift_utils.previously_synced') + @mock.patch.object(subprocess, 'check_output') + def test_write_rings_exception_blocks(self, mock_check_call, + mock_previously_synced): + my_execp = subprocess.CalledProcessError(2, "swift-ring-builder", + output='') + mock_check_call.side_effect = my_execp + self.assertRaises(swift_utils.SwiftProxyCharmException, + swift_utils.write_rings) + + def test_nodes_have_rep_data_true(self): + nodes = [{ + 'ip_rep': '0.0.0.0', + 'ip_cls': '1.1.1.1', + 'region': '', + 'object_port_rep': 10, + 'container_port_rep': 20, + 'account_port_rep': 30}, { + 'ip_rep': '2.2.2.2', + 'ip_cls': '3.3.3.3', + 'region': '', + 'object_port_rep': 40, + 'container_port_rep': 50, + 'account_port_rep': 60}] + + self.assertTrue(swift_utils.nodes_have_rep_data(nodes)) + + def test_nodes_have_rep_data_false(self): + nodes = [{'ip_cls': '1.1.1.1', 'region': ''}, {'ip_cls': '3.3.3.3', }] + + self.assertFalse(swift_utils.nodes_have_rep_data(nodes)) + @mock.patch('lib.swift_utils.get_www_dir') def test_mark_www_rings_deleted(self, mock_get_www_dir): try: