openstack
/
charm-swift-proxy
Code Issues Proposed changes

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
This commit is contained in:
Erlon R. Cruz 2020-11-18 15:37:14 -03:00
parent dee0aa7c2e
commit 498d3b0f1e
2 changed files with 166 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

103
lib/swift_utils.py
View File

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

81
unit_tests/test_swift_utils.py
View File

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