From ac553419f4757112344a302acbb010838e340abb Mon Sep 17 00:00:00 2001 From: Billy Olsen Date: Fri, 25 Aug 2017 15:48:33 -0700 Subject: [PATCH] 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 " --- lib/swift_utils.py | 18 ++--- unit_tests/test_swift_utils.py | 128 +++++++++++++++++++++++++++------ 2 files changed, 117 insertions(+), 29 deletions(-) diff --git a/lib/swift_utils.py b/lib/swift_utils.py index b2ba4a5..666b6a2 100644 --- a/lib/swift_utils.py +++ b/lib/swift_utils.py @@ -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 " diff --git a/unit_tests/test_swift_utils.py b/unit_tests/test_swift_utils.py index a6e8b6d..e224f5b 100644 --- a/unit_tests/test_swift_utils.py +++ b/unit_tests/test_swift_utils.py @@ -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__')