openstack
/
charm-swift-proxy
Code Issues Proposed changes

Handle holes in swift rings 

The Swift rings may have "holes" for devices which no longer
exist, but the code does not handle the holes in general. Holes
appear in the Swift rings as None objects in the devices list.

This change adds checks to the places in the code which are loading
the devices from the Swift ring to account for None. Generally,
this is just checking the truthy value of a device.

However, this change also removes the next device id calculation
from the add_to_ring function, deferring the device id selection
to the Swift RingBuilder. Upon examining the Swift RingBuilder
code, it was seen that the RingBuilder will automatically calculate
a device id for a new device when no id is specified. The Swift
algorithm considers factors other than a hole in the ring (e.g.
out of order devices) which were missing from the charm's code.

Change-Id: Ibb0908338ac958ebf1adf17c12f9484f6963c695
Closes-Bug: #1708310
Co-Authored-By: Drew Freiberger <drew.freiberger@canonical.com>"
This commit is contained in:
Billy Olsen 2017-08-25 15:48:33 -07:00
parent 426f03c576
commit ac553419f4
2 changed files with 117 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
lib/swift_utils.py
View File

@ -500,10 +500,13 @@ def exists_in_ring(ring_path, node):
node['port'] = ring_port(ring_path, node)
for dev in ring['devs']:
# Devices in the ring can be None if there are holes from previously
# removed devices so skip any that are None.
if not dev:
continue
d = [(i, dev[i]) for i in dev if i in node and i != 'zone']
n = [(i, node[i]) for i in node if i in dev and i != 'zone']
if sorted(d) == sorted(n):
log('Node already exists in ring (%s).' % ring_path, level=INFO)
return True
@ -514,13 +517,12 @@ def add_to_ring(ring_path, node):
ring = _load_builder(ring_path)
port = ring_port(ring_path, node)
devs = ring.to_dict()['devs']
next_id = 0
if devs:
next_id = len([d['id'] for d in devs])
# Note: this code used to attempt to calculate new dev ids, but made
# various assumptions (e.g. in order devices, all devices in the ring
# being valid, etc). The RingBuilder from the Swift library will
# automatically calculate the new device's ID if no id is provided (at
# least since the Icehouse release).
new_dev = {
'id': next_id,
'zone': node['zone'],
'ip': node['ip'],
'port': port,
@ -1125,7 +1127,7 @@ def has_minimum_zones(rings):
return False
builder = _load_builder(ring).to_dict()
replicas = builder['replicas']
zones = [dev['zone'] for dev in builder['devs']]
zones = [dev['zone'] for dev in builder['devs'] if dev]
num_zones = len(set(zones))
if num_zones < replicas:
log("Not enough zones (%d) defined to satisfy minimum replicas "

128
unit_tests/test_swift_utils.py
View File

@ -33,6 +33,32 @@ def init_ring_paths(tmpdir):
fd.write("0\n")
def create_mock_load_builder_fn(mock_rings):
"""To avoid the need for swift.common.ring library, mock a basic rings
dictionary, keyed by path. Each ring has enough logic to hold a dictionary
with a single 'devs' key, which stores the list of passed dev(s) by
add_dev().
If swift (actual) ring representation diverges (see _load_builder),
this mock will need to be adapted.
:param mock_rings: a dict containing the dict form of the rings
"""
def mock_load_builder_fn(path):
class mock_ring(object):
def __init__(self, path):
self.path = path
def to_dict(self):
return mock_rings[self.path]
def add_dev(self, dev):
mock_rings[self.path]['devs'].append(dev)
return mock_ring(path)
return mock_load_builder_fn
class SwiftUtilsTestCase(unittest.TestCase):
@mock.patch.object(swift_utils, 'previously_synced')
@ -114,33 +140,13 @@ class SwiftUtilsTestCase(unittest.TestCase):
mock_initialize_ring,
mock_load_builder,
mock_previously_synced):
# To avoid the need for swift.common.ring library, mock a basic
# rings dictionary, keyed by path.
# Each ring has enough logic to hold a dictionary with a single 'devs'
# key, which stores the list of passed dev(s) by add_dev().
#
# If swift (actual) ring representation diverges (see _load_builder),
# this mock will need to be adapted.
mock_rings = {}
def mock_load_builder_fn(path):
class mock_ring(object):
def __init__(self, path):
self.path = path
def to_dict(self):
return mock_rings[self.path]
def add_dev(self, dev):
mock_rings[self.path]['devs'].append(dev)
return mock_ring(path)
def mock_initialize_ring_fn(path, *args):
mock_rings.setdefault(path, {'devs': []})
mock_is_elected_leader.return_value = True
mock_load_builder.side_effect = mock_load_builder_fn
mock_load_builder.side_effect = create_mock_load_builder_fn(mock_rings)
mock_initialize_ring.side_effect = mock_initialize_ring_fn
init_ring_paths(tempfile.mkdtemp())
@ -371,6 +377,86 @@ class SwiftUtilsTestCase(unittest.TestCase):
mock_rel_get.return_value = {'broker-timestamp': '1234'}
self.assertTrue(swift_utils.timestamps_available('proxy/2'))
@mock.patch.object(swift_utils, '_load_builder')
def test_exists_in_ring(self, mock_load_builder):
mock_rings = {}
mock_load_builder.side_effect = create_mock_load_builder_fn(mock_rings)
ring = 'account'
mock_rings[ring] = {
'devs': [
{'replication_port': 6000, 'zone': 1, 'weight': 100.0,
'ip': '172.16.0.2', 'region': 1, 'port': 6000,
'replication_ip': '172.16.0.2', 'parts': 2, 'meta': '',
'device': u'bcache10', 'parts_wanted': 0, 'id': 199},
None, # Ring can have holes, so add None to simulate
{'replication_port': 6000, 'zone': 1, 'weight': 100.0,
'ip': '172.16.0.2', 'region': 1, 'id': 198,
'replication_ip': '172.16.0.2', 'parts': 2, 'meta': '',
'device': u'bcache13', 'parts_wanted': 0, 'port': 6000},
]
}
node = {
'ip': '172.16.0.2',
'region': 1,
'account_port': 6000,
'zone': 1,
'replication_port': 6000,
'weight': 100.0,
'device': u'bcache10',
}
ret = swift_utils.exists_in_ring(ring, node)
self.assertTrue(ret)
node['region'] = 2
ret = swift_utils.exists_in_ring(ring, node)
self.assertFalse(ret)
@mock.patch.object(swift_utils, '_write_ring')
@mock.patch.object(swift_utils, '_load_builder')
def test_add_to_ring(self, mock_load_builder, mock_write_ring):
mock_rings = {}
mock_load_builder.side_effect = create_mock_load_builder_fn(mock_rings)
ring = 'account'
mock_rings[ring] = {
'devs': []
}
node = {
'ip': '172.16.0.2',
'region': 1,
'account_port': 6000,
'zone': 1,
'device': '/dev/sdb',
}
swift_utils.add_to_ring(ring, node)
mock_write_ring.assert_called_once()
self.assertTrue('id' not in mock_rings[ring]['devs'][0])
@mock.patch('os.path.isfile')
@mock.patch.object(swift_utils, '_load_builder')
def test_has_minimum_zones(self, mock_load_builder, mock_is_file):
mock_rings = {}
mock_load_builder.side_effect = create_mock_load_builder_fn(mock_rings)
for ring in swift_utils.SWIFT_RINGS:
mock_rings[ring] = {
'replicas': 3,
'devs': [{'zone': 1}, {'zone': 2}, None, {'zone': 3}],
}
ret = swift_utils.has_minimum_zones(swift_utils.SWIFT_RINGS)
self.assertTrue(ret)
# Increase the replicas to make sure that it returns false
for ring in swift_utils.SWIFT_RINGS:
mock_rings[ring]['replicas'] = 4
ret = swift_utils.has_minimum_zones(swift_utils.SWIFT_RINGS)
self.assertFalse(ret)
@mock.patch('lib.swift_utils.config')
@mock.patch('lib.swift_utils.set_os_workload_status')
@mock.patch('lib.swift_utils.SwiftRingContext.__call__')