diff --git a/swift/common/ring/builder.py b/swift/common/ring/builder.py index b8fe9c6bba..2244de65a8 100644 --- a/swift/common/ring/builder.py +++ b/swift/common/ring/builder.py @@ -156,6 +156,9 @@ class RingBuilder(object): return bool(self._part_moved_bitmap[byte] & (128 >> bit)) def _can_part_move(self, part): + # if min_part_hours is zero then checking _last_part_moves will not + # indicate if the part has already moved during the current rebalance, + # but _has_part_moved will. return (self._last_part_moves[part] >= self.min_part_hours and not self._has_part_moved(part)) @@ -457,6 +460,7 @@ class RingBuilder(object): below 1% or doesn't change by more than 1% (only happens with a ring that can't be balanced no matter what). + :param seed: a value for the random seed (optional) :returns: (number_of_partitions_altered, resulting_balance, number_of_removed_devices) """ @@ -484,7 +488,6 @@ class RingBuilder(object): if self._last_part_moves is None: self.logger.debug("New builder; performing initial balance") self._last_part_moves = array('B', itertools.repeat(0, self.parts)) - self._part_moved_bitmap = bytearray(max(2 ** (self.part_power - 3), 1)) self._update_last_part_moves() replica_plan = self._build_replica_plan() @@ -896,8 +899,9 @@ class RingBuilder(object): current time. The builder won't move a partition that has been moved more recently than min_part_hours. """ + self._part_moved_bitmap = bytearray(max(2 ** (self.part_power - 3), 1)) elapsed_hours = int(time() - self._last_part_moves_epoch) / 3600 - if elapsed_hours <= 0: + if elapsed_hours <= 0 or not self._last_part_moves: return for part in range(self.parts): # The "min(self._last_part_moves[part] + elapsed_hours, 0xff)" @@ -1651,7 +1655,7 @@ class RingBuilder(object): yield (part, replica) @classmethod - def load(cls, builder_file, open=open): + def load(cls, builder_file, open=open, **kwargs): """ Obtain RingBuilder instance of the provided builder file @@ -1680,7 +1684,7 @@ class RingBuilder(object): if not hasattr(builder, 'devs'): builder_dict = builder - builder = RingBuilder(1, 1, 1) + builder = cls(1, 1, 1, **kwargs) builder.copy_from(builder_dict) if not hasattr(builder, '_id'): diff --git a/swift/common/ring/composite_builder.py b/swift/common/ring/composite_builder.py index cdde7676a8..f8789763b8 100644 --- a/swift/common/ring/composite_builder.py +++ b/swift/common/ring/composite_builder.py @@ -93,6 +93,9 @@ import copy import json import os +from random import shuffle + +from swift.common.exceptions import RingBuilderError from swift.common.ring import RingBuilder from swift.common.ring import RingData from collections import defaultdict @@ -363,13 +366,16 @@ def check_builder_ids(builders): class CompositeRingBuilder(object): """ - Provides facility to create, persist, load and update composite rings, for - example:: + Provides facility to create, persist, load, rebalance and update composite + rings, for example:: # create a CompositeRingBuilder instance with a list of # component builder files crb = CompositeRingBuilder(["region1.builder", "region2.builder"]) + # perform a cooperative rebalance of the component builders + crb.rebalance() + # call compose which will make a new RingData instance ring_data = crb.compose() @@ -432,6 +438,7 @@ class CompositeRingBuilder(object): self.ring_data = None self._builder_files = None self._set_builder_files(builder_files or []) + self._builders = None # these are lazy loaded in _load_components def _set_builder_files(self, builder_files): self._builder_files = [os.path.abspath(bf) for bf in builder_files] @@ -500,10 +507,39 @@ class CompositeRingBuilder(object): metadata['serialization_version'] = 1 json.dump(metadata, fp) - def compose(self, builder_files=None, force=False): + def _load_components(self, builder_files=None, force=False, + require_modified=False): + if self._builders: + return self._builder_files, self._builders + + builder_files = builder_files or self._builder_files + if len(builder_files) < 2: + raise ValueError('Two or more component builders are required.') + + builders = [] + for builder_file in builder_files: + # each component builder gets a reference to this composite builder + # so that it can delegate part movement decisions to the composite + # builder during rebalance + builders.append(CooperativeRingBuilder.load(builder_file, + parent_builder=self)) + check_builder_ids(builders) + new_metadata = _make_composite_metadata(builders) + if self.components and self._builder_files and not force: + modified = check_against_existing(self.to_dict(), new_metadata) + if require_modified and not modified: + raise ValueError( + "None of the component builders has been modified" + " since the existing composite ring was built.") + self._set_builder_files(builder_files) + self._builders = builders + return self._builder_files, self._builders + + def load_components(self, builder_files=None, force=False, + require_modified=False): """ - Builds a composite ring using component ring builders loaded from a - list of builder files. + Loads component ring builders from builder files. Previously loaded + component ring builders will discarded and reloaded. If a list of component ring builder files is given then that will be used to load component ring builders. Otherwise, component ring @@ -515,6 +551,43 @@ class CompositeRingBuilder(object): with the existing composition of builders, unless the optional ``force`` flag if set True. + :param builder_files: Optional list of paths to ring builder + files that will be used to load the component ring builders. + Typically the list of component builder files will have been set + when the instance was constructed, for example when using the + load() class method. However, this parameter may be used if the + component builder file paths have moved, or, in conjunction with + the ``force`` parameter, if a new list of component builders is to + be used. + :param force: if True then do not verify given builders are + consistent with any existing composite ring (default is False). + :param require_modified: if True and ``force`` is False, then + verify that at least one of the given builders has been modified + since the composite ring was last built (default is False). + :return: A tuple of (builder files, loaded builders) + :raises: ValueError if the component ring builders are not suitable for + composing with each other, or are inconsistent with any existing + composite ring, or if require_modified is True and there has been + no change with respect to the existing ring. + """ + self._builders = None # force a reload of builders + return self._load_components( + builder_files, force, require_modified) + + def compose(self, builder_files=None, force=False, require_modified=False): + """ + Builds a composite ring using component ring builders loaded from a + list of builder files and updates composite ring metadata. + + If a list of component ring builder files is given then that will be + used to load component ring builders. Otherwise, component ring + builders will be loaded using the list of builder files that was set + when the instance was constructed. + + In either case, if metadata for an existing composite ring has been + loaded then the component ring builders are verified for consistency + with the existing composition of builders, unless the optional + ``force`` flag if set True. :param builder_files: Optional list of paths to ring builder files that will be used to load the component ring builders. @@ -524,27 +597,139 @@ class CompositeRingBuilder(object): component builder file paths have moved, or, in conjunction with the ``force`` parameter, if a new list of component builders is to be used. - :param force: if True then do not verify given builders are consistent - with any existing composite ring. + :param force: if True then do not verify given builders are + consistent with any existing composite ring (default is False). + :param require_modified: if True and ``force`` is False, then + verify that at least one of the given builders has been modified + since the composite ring was last built (default is False). :return: An instance of :class:`swift.common.ring.ring.RingData` :raises: ValueError if the component ring builders are not suitable for composing with each other, or are inconsistent with any existing - composite ring, or if there has been no change with respect to the - existing ring. + composite ring, or if require_modified is True and there has been + no change with respect to the existing ring. """ - builder_files = builder_files or self._builder_files - builders = [RingBuilder.load(f) for f in builder_files] - check_builder_ids(builders) - new_metadata = _make_composite_metadata(builders) - if self.components and self._builder_files and not force: - modified = check_against_existing(self.to_dict(), new_metadata) - if not modified: - raise ValueError( - "None of the component builders has been modified" - " since the existing composite ring was built.") - - self.ring_data = compose_rings(builders) + self.load_components(builder_files, force=force, + require_modified=require_modified) + self.ring_data = compose_rings(self._builders) self.version += 1 + new_metadata = _make_composite_metadata(self._builders) self.components = new_metadata['components'] - self._set_builder_files(builder_files) return self.ring_data + + def rebalance(self): + """ + Cooperatively rebalances all component ring builders. + + This method does not change the state of the composite ring; a + subsequent call to :meth:`compose` is required to generate updated + composite :class:`RingData`. + + :return: A list of dicts, one per component builder, each having the + following keys: + + * 'builder_file' maps to the component builder file; + * 'builder' maps to the corresponding instance of + :class:`swift.common.ring.builder.RingBuilder`; + * 'result' maps to the results of the rebalance of that component + i.e. a tuple of: `(number_of_partitions_altered, + resulting_balance, number_of_removed_devices)` + + The list has the same order as components in the composite ring. + :raises RingBuilderError: if there is an error while rebalancing any + component builder. + """ + self._load_components() + component_builders = zip(self._builder_files, self._builders) + # don't let the same builder go first each time + shuffle(component_builders) + results = {} + for builder_file, builder in component_builders: + try: + results[builder] = { + 'builder': builder, + 'builder_file': builder_file, + 'result': builder.rebalance() + } + builder.validate() + except RingBuilderError as err: + self._builders = None + raise RingBuilderError( + 'An error occurred while rebalancing component %s: %s' % + (builder_file, err)) + + for builder_file, builder in component_builders: + builder.save(builder_file) + # return results in component order + return [results[builder] for builder in self._builders] + + def can_part_move(self, part): + """ + Check with all component builders that it is ok to move a partition. + + :param part: The partition to check. + :return: True if all component builders agree that the partition can be + moved, False otherwise. + """ + # Called by component builders. + return all(b.can_part_move(part) for b in self._builders) + + def update_last_part_moves(self): + """ + Updates the record of how many hours ago each partition was moved in + all component builders. + """ + # Called by component builders. We need all component builders to be at + # same last_part_moves epoch before any builder starts moving parts; + # this will effectively be a no-op for builders that have already been + # updated in last hour + for b in self._builders: + b.update_last_part_moves() + + +class CooperativeRingBuilder(RingBuilder): + """ + A subclass of :class:`RingBuilder` that participates in cooperative + rebalance. + + During rebalance this subclass will consult with its `parent_builder` + before moving a partition. The `parent_builder` may in turn check with + co-builders (including this instance) to verify that none have moved that + partition in the last `min_part_hours`. + + :param part_power: number of partitions = 2**part_power. + :param replicas: number of replicas for each partition. + :param min_part_hours: minimum number of hours between partition changes. + :param parent_builder: an instance of :class:`CompositeRingBuilder`. + """ + def __init__(self, part_power, replicas, min_part_hours, parent_builder): + super(CooperativeRingBuilder, self).__init__( + part_power, replicas, min_part_hours) + self.parent_builder = parent_builder + + def _can_part_move(self, part): + # override superclass method to delegate to the parent builder + return self.parent_builder.can_part_move(part) + + def can_part_move(self, part): + """ + Check that in the context of this builder alone it is ok to move a + partition. + + :param part: The partition to check. + :return: True if the partition can be moved, False otherwise. + """ + # called by parent_builder - now forward to the superclass + return (not self._last_part_moves or + super(CooperativeRingBuilder, self)._can_part_move(part)) + + def _update_last_part_moves(self): + # overrides superclass method to delegate to parent builder + return self.parent_builder.update_last_part_moves() + + def update_last_part_moves(self): + """ + Updates the record of how many hours ago each partition was moved in + in this builder. + """ + # called by parent_builder - now forward to the superclass + return super(CooperativeRingBuilder, self)._update_last_part_moves() diff --git a/test/unit/common/ring/test_composite_builder.py b/test/unit/common/ring/test_composite_builder.py index 5ac6d15962..3dae9a6ef0 100644 --- a/test/unit/common/ring/test_composite_builder.py +++ b/test/unit/common/ring/test_composite_builder.py @@ -13,6 +13,8 @@ # See the License for the specific language governing permissions and # limitations under the License. import json +from contextlib import contextmanager + import mock import os import random @@ -21,9 +23,12 @@ import unittest import shutil import copy +from collections import defaultdict, Counter + +from swift.common.exceptions import RingBuilderError from swift.common.ring import RingBuilder, Ring from swift.common.ring.composite_builder import ( - compose_rings, CompositeRingBuilder) + compose_rings, CompositeRingBuilder, CooperativeRingBuilder) def make_device_iter(): @@ -81,7 +86,7 @@ class BaseTestCompositeBuilder(unittest.TestCase): builder_files.append(fname) return builder_files - def create_sample_ringbuilders(self, num_builders=2): + def create_sample_ringbuilders(self, num_builders=2, rebalance=True): """ Create sample rings with four devices @@ -101,19 +106,25 @@ class BaseTestCompositeBuilder(unittest.TestCase): new_dev = self.pop_region_device(region) new_dev['weight'] = 0 builder.add_dev(new_dev) - builder.rebalance() + if rebalance: + builder.rebalance() builder.save(fname) self.assertTrue(os.path.exists(fname)) builders.append(builder) return builders - def add_dev_and_rebalance(self, builder, weight=None): - dev = next(builder._iter_devs()) - new_dev = self.pop_region_device(dev['region']) + def add_dev(self, builder, weight=None, region=None): + if region is None: + dev = next(builder._iter_devs()) + region = dev['region'] + new_dev = self.pop_region_device(region) if weight is not None: new_dev['weight'] = weight builder.add_dev(new_dev) + + def add_dev_and_rebalance(self, builder, weight=None): + self.add_dev(builder, weight) builder.rebalance() def assertDevices(self, composite_ring, builders): @@ -190,6 +201,14 @@ class BaseTestCompositeBuilder(unittest.TestCase): } self.assertEqual(expected_metadata, actual) + def _make_composite_builder(self, builders): + # helper to compose a ring, save it and sanity check it + builder_files = self.save_builders(builders) + cb = CompositeRingBuilder(builder_files) + cb.compose().save(self.output_ring) + self.check_composite_ring(self.output_ring, builders) + return cb, builder_files + class TestCompositeBuilder(BaseTestCompositeBuilder): def test_compose_rings(self): @@ -308,7 +327,7 @@ class TestCompositeBuilder(BaseTestCompositeBuilder): def test_compose_rings_rebalance_needed(self): builders = self.create_sample_ringbuilders(2) - # add a new device to builider 1 but no rebalance + # add a new device to builder 1 but no rebalance dev = self.pop_region_device(1) builders[1].add_dev(dev) self.assertTrue(builders[1].devs_changed) # sanity check @@ -393,14 +412,6 @@ class TestCompositeRingBuilder(BaseTestCompositeBuilder): cb.compose().save(self.output_ring) self.check_composite_ring(self.output_ring, builders) - def _make_composite_builder(self, builders): - # helper to compose a ring, save it and sanity check it - builder_files = self.save_builders(builders) - cb = CompositeRingBuilder(builder_files) - cb.compose().save(self.output_ring) - self.check_composite_ring(self.output_ring, builders) - return cb, builder_files - def test_compose_ok(self): cb_file = os.path.join(self.tmpdir, 'test-composite-ring.json') builders = self.create_sample_ringbuilders(2) @@ -413,13 +424,13 @@ class TestCompositeRingBuilder(BaseTestCompositeBuilder): # and reloads ok cb = CompositeRingBuilder.load(cb_file) self.assertEqual(1, cb.version) - # composes after with no component builder changes will fail... + # compose detects if no component builder changes, if we ask it to... with self.assertRaises(ValueError) as cm: - cb.compose() + cb.compose(require_modified=True) self.assertIn('None of the component builders has been modified', cm.exception.message) self.assertEqual(1, cb.version) - # ...unless we force it + # ...but by default will compose again despite no changes to components cb.compose(force=True).save(self.output_ring) self.check_composite_ring(self.output_ring, builders) self.assertEqual(2, cb.version) @@ -468,9 +479,10 @@ class TestCompositeRingBuilder(BaseTestCompositeBuilder): # modify builders and save in different files self.add_dev_and_rebalance(builders[1]) with self.assertRaises(ValueError): - cb.compose(builder_files) # sanity check - originals are unchanged + # sanity check - originals are unchanged + cb.compose(builder_files, require_modified=True) other_files = self.save_builders(builders, prefix='other') - cb.compose(other_files).save(self.output_ring) + cb.compose(other_files, require_modified=True).save(self.output_ring) self.check_composite_ring(self.output_ring, builders) # check composite builder persists ok cb.save(cb_file) @@ -504,176 +516,6 @@ class TestCompositeRingBuilder(BaseTestCompositeBuilder): finally: os.chdir(cwd) - def test_compose_insufficient_builders(self): - def do_test(builder_files): - cb = CompositeRingBuilder(builder_files) - with self.assertRaises(ValueError) as cm: - cb.compose() - self.assertIn('Two or more component builders are required', - cm.exception.message) - - cb = CompositeRingBuilder() - with self.assertRaises(ValueError) as cm: - cb.compose(builder_files) - self.assertIn('Two or more component builders are required', - cm.exception.message) - - builders = self.create_sample_ringbuilders(3) - builder_files = self.save_builders(builders) - do_test([]) - do_test(builder_files[:1]) - - def test_compose_missing_builder_id(self): - def check_missing_id(cb, builders): - # not ok to compose with builder_files that have no id assigned - orig_version = cb.version - no_id = random.randint(0, len(builders) - 1) - # rewrite the builder files so that one has missing id - self.save_builders(builders, missing_ids=[no_id]) - with self.assertRaises(ValueError) as cm: - cb.compose() - error_lines = cm.exception.message.split('\n') - self.assertIn("Problem with builder at index %s" % no_id, - error_lines[0]) - self.assertIn("id attribute has not been initialised", - error_lines[0]) - self.assertFalse(error_lines[1:]) - self.assertEqual(orig_version, cb.version) - # check with compose not previously called, cb has no existing metadata - builders = self.create_sample_ringbuilders(3) - builder_files = self.save_builders(builders) - cb = CompositeRingBuilder(builder_files) - check_missing_id(cb, builders) - # now save good copies of builders and compose so this cb has - # existing component metadata - builder_files = self.save_builders(builders) - cb = CompositeRingBuilder(builder_files) - cb.compose() # cb now has component metadata - check_missing_id(cb, builders) - - def test_compose_duplicate_builder_ids(self): - builders = self.create_sample_ringbuilders(3) - builders[2]._id = builders[0]._id - cb = CompositeRingBuilder(self.save_builders(builders)) - with self.assertRaises(ValueError) as cm: - cb.compose() - error_lines = cm.exception.message.split('\n') - self.assertIn("Builder id %r used at indexes 0, 2" % builders[0].id, - error_lines[0]) - self.assertFalse(error_lines[1:]) - self.assertEqual(0, cb.version) - - def test_compose_ring_unchanged_builders(self): - def do_test(cb, builder_files): - with self.assertRaises(ValueError) as cm: - cb.compose(builder_files) - error_lines = cm.exception.message.split('\n') - self.assertIn("None of the component builders has been modified", - error_lines[0]) - self.assertFalse(error_lines[1:]) - self.assertEqual(1, cb.version) - - builders = self.create_sample_ringbuilders(2) - cb, builder_files = self._make_composite_builder(builders) - # not ok to compose again with same *unchanged* builders - do_test(cb, builder_files) - # even if we rewrite the files - builder_files = self.save_builders(builders) - do_test(cb, builder_files) - # even if we rename the files - builder_files = self.save_builders(builders, prefix='other') - do_test(cb, builder_files) - - def test_compose_older_builder(self): - # make first version of composite ring - builders = self.create_sample_ringbuilders(2) - cb, builder_files = self._make_composite_builder(builders) - old_builders = [copy.deepcopy(b) for b in builders] - for i, b in enumerate(builders): - self.add_dev_and_rebalance(b) - self.assertLess(old_builders[i].version, b.version) - self.save_builders(builders) - cb.compose() # newer version - self.assertEqual(2, cb.version) # sanity check - # not ok to use old versions of same builders - self.save_builders([old_builders[0], builders[1]]) - with self.assertRaises(ValueError) as cm: - cb.compose() - error_lines = cm.exception.message.split('\n') - self.assertIn("Invalid builder change at index 0", error_lines[0]) - self.assertIn("Older builder version", error_lines[0]) - self.assertFalse(error_lines[1:]) - self.assertEqual(2, cb.version) - # not even if one component ring has changed - self.add_dev_and_rebalance(builders[1]) - self.save_builders([old_builders[0], builders[1]]) - with self.assertRaises(ValueError) as cm: - cb.compose() - error_lines = cm.exception.message.split('\n') - self.assertIn("Invalid builder change at index 0", error_lines[0]) - self.assertIn("Older builder version", error_lines[0]) - self.assertFalse(error_lines[1:]) - self.assertEqual(2, cb.version) - - def test_compose_different_number_builders(self): - # not ok to use a different number of component rings - builders = self.create_sample_ringbuilders(3) - cb, builder_files = self._make_composite_builder(builders[:2]) - - def do_test(bad_builders): - with self.assertRaises(ValueError) as cm: - cb.compose(self.save_builders(bad_builders)) - error_lines = cm.exception.message.split('\n') - self.assertFalse(error_lines[1:]) - self.assertEqual(1, cb.version) - return error_lines - - error_lines = do_test(builders[:1]) # too few - self.assertIn("Missing builder at index 1", error_lines[0]) - error_lines = do_test(builders) # too many - self.assertIn("Unexpected extra builder at index 2", error_lines[0]) - - def test_compose_different_builders(self): - # not ok to change component rings - builders = self.create_sample_ringbuilders(3) - cb, builder_files = self._make_composite_builder(builders[:2]) - # ensure builder[0] is newer version so that's not the problem - self.add_dev_and_rebalance(builders[0]) - with self.assertRaises(ValueError) as cm: - cb.compose(self.save_builders([builders[0], builders[2]])) - error_lines = cm.exception.message.split('\n') - self.assertIn("Invalid builder change at index 1", error_lines[0]) - self.assertIn("Attribute mismatch for id", error_lines[0]) - self.assertFalse(error_lines[1:]) - self.assertEqual(1, cb.version) - - def test_compose_different_builder_order(self): - # not ok to change order of component rings - builders = self.create_sample_ringbuilders(4) - cb, builder_files = self._make_composite_builder(builders) - builder_files.reverse() - with self.assertRaises(ValueError) as cm: - cb.compose(builder_files) - error_lines = cm.exception.message.split('\n') - for i, line in enumerate(error_lines): - self.assertIn("Invalid builder change at index %s" % i, line) - self.assertIn("Attribute mismatch for id", line) - self.assertEqual(1, cb.version) - - def test_compose_replica_count_changed(self): - # not ok to change the number of replicas in a ring - builders = self.create_sample_ringbuilders(3) - cb, builder_files = self._make_composite_builder(builders) - builders[0].set_replicas(4) - self.save_builders(builders) - with self.assertRaises(ValueError) as cm: - cb.compose() - error_lines = cm.exception.message.split('\n') - for i, line in enumerate(error_lines): - self.assertIn("Invalid builder change at index 0", line) - self.assertIn("Attribute mismatch for replicas", line) - self.assertEqual(1, cb.version) - def test_load_errors(self): bad_file = os.path.join(self.tmpdir, 'bad_file.json') with self.assertRaises(IOError): @@ -719,6 +561,610 @@ class TestCompositeRingBuilder(BaseTestCompositeBuilder): do_test(CompositeRingBuilder([])) do_test(CompositeRingBuilder(['file1', 'file2'])) + def test_rebalance(self): + @contextmanager + def mock_rebalance(): + # captures component builder rebalance call results, yields a dict + # that maps builder -> results + calls = defaultdict(list) + orig_func = RingBuilder.rebalance + + def func(builder, **kwargs): + result = orig_func(builder, **kwargs) + calls[builder].append(result) + return result + + with mock.patch('swift.common.ring.RingBuilder.rebalance', func): + yield calls + + def check_results(): + self.assertEqual(2, len(rebalance_calls)) # 2 builders called + for calls in rebalance_calls.values(): + self.assertFalse(calls[1:]) # 1 call to each builder + + self.assertEqual(sorted(expected_ids), + sorted([b.id for b in rebalance_calls])) + self.assertEqual(sorted(expected_versions), + sorted([b.version for b in rebalance_calls])) + for b in rebalance_calls: + self.assertEqual(set(rebalance_calls.keys()), + set(b.parent_builder._builders)) + + # check the rebalanced builders were saved + written_builders = [RingBuilder.load(f) for f in builder_files] + self.assertEqual(expected_ids, + [b.id for b in written_builders]) + self.assertEqual(expected_versions, + [b.version for b in written_builders]) + + # check returned results, should be in component order + self.assertEqual(2, len(results)) + self.assertEqual(builder_files, + [r['builder_file'] for r in results]) + self.assertEqual(expected_versions, + [r['builder'].version for r in results]) + self.assertEqual(expected_ids, [r['builder'].id for r in results]) + self.assertEqual( + [rebalance_calls[r['builder']][0] for r in results], + [r['result'] for r in results]) + + # N.B. the sample builders have zero min_part_hours + builders = self.create_sample_ringbuilders(2) + expected_versions = [b.version + 1 for b in builders] + expected_ids = [b.id for b in builders] + + # test rebalance loads component builders + builder_files = self.save_builders(builders) + cb = CompositeRingBuilder(builder_files) + with mock_rebalance() as rebalance_calls: + results = cb.rebalance() + check_results() + + # test loading builder files via load_components + # revert builder files to original builder state + builder_files = self.save_builders(builders) + cb = CompositeRingBuilder() + cb.load_components(builder_files) + with mock_rebalance() as rebalance_calls: + results = cb.rebalance() + check_results() + + def test_rebalance_errors(self): + cb = CompositeRingBuilder() + with self.assertRaises(ValueError) as cm: + cb.rebalance() + self.assertIn('Two or more component builders are required', + cm.exception.message) + + builders = self.create_sample_ringbuilders(2) + cb, builder_files = self._make_composite_builder(builders) + with mock.patch('swift.common.ring.RingBuilder.rebalance', + side_effect=RingBuilderError('test')): + with mock.patch('swift.common.ring.composite_builder.shuffle', + lambda x: x): + with self.assertRaises(RingBuilderError) as cm: + cb.rebalance() + self.assertIn('An error occurred while rebalancing component %s' % + builder_files[0], str(cm.exception)) + self.assertIsNone(cb._builders) + + with mock.patch('swift.common.ring.RingBuilder.validate', + side_effect=RingBuilderError('test')): + with mock.patch('swift.common.ring.composite_builder.shuffle', + lambda x: x): + with self.assertRaises(RingBuilderError) as cm: + cb.rebalance() + self.assertIn('An error occurred while rebalancing component %s' % + builder_files[0], str(cm.exception)) + self.assertIsNone(cb._builders) + + def test_rebalance_with_unrebalanced_builders(self): + # create 2 non-rebalanced rings + builders = self.create_sample_ringbuilders(rebalance=False) + # save builders + builder_files = self.save_builders(builders) + cb = CompositeRingBuilder(builder_files) + # sanity, it is impossible to compose un-rebalanced component rings + with self.assertRaises(ValueError) as cm: + cb.compose() + self.assertIn("Builder needs rebalance", cm.exception.message) + # but ok to compose after rebalance + cb.rebalance() + rd = cb.compose() + rd.save(self.output_ring) + rebalanced_builders = [RingBuilder.load(f) for f in builder_files] + self.check_composite_ring(self.output_ring, rebalanced_builders) + + +class TestLoadComponents(BaseTestCompositeBuilder): + # Tests for the loading of component builders. + def _call_method_under_test(self, cb, *args, **kwargs): + # Component builder loading is triggered by the load_components method + # and the compose method. This method provides a hook for subclasses to + # configure a different method to repeat the component loading tests. + cb.load_components(*args, **kwargs) + + def test_load_components(self): + builders = self.create_sample_ringbuilders(2) + builder_files = self.save_builders(builders) + cb = CompositeRingBuilder(builder_files) + # check lazy loading + self.assertEqual(builder_files, cb._builder_files) + self.assertFalse(cb._builders) # none loaded yet + + # check loading configured files + self._call_method_under_test(cb) + self.assertEqual(builder_files, cb._builder_files) + for i, builder in enumerate(cb._builders): + self.assertEqual(builders[i].id, builder.id) + self.assertEqual(builders[i].devs, builder.devs) + + # modify builders and save in different files + self.add_dev_and_rebalance(builders[0]) + other_files = self.save_builders(builders, prefix='other') + # reload from other files + self._call_method_under_test(cb, other_files) + self.assertEqual(other_files, cb._builder_files) + for i, builder in enumerate(cb._builders): + self.assertEqual(builders[i].id, builder.id) + self.assertEqual(builders[i].devs, builder.devs) + + # modify builders again and save in same files + self.add_dev_and_rebalance(builders[1]) + self.save_builders(builders, prefix='other') + # reload from same files + self._call_method_under_test(cb) + self.assertEqual(other_files, cb._builder_files) + for i, builder in enumerate(cb._builders): + self.assertEqual(builders[i].id, builder.id) + self.assertEqual(builders[i].devs, builder.devs) + + def test_load_components_insufficient_builders(self): + def do_test(builder_files, force): + cb = CompositeRingBuilder(builder_files) + with self.assertRaises(ValueError) as cm: + self._call_method_under_test(cb, builder_files, + force=force) + self.assertIn('Two or more component builders are required', + cm.exception.message) + + cb = CompositeRingBuilder() + with self.assertRaises(ValueError) as cm: + self._call_method_under_test(cb, builder_files, + force=force) + self.assertIn('Two or more component builders are required', + cm.exception.message) + + builders = self.create_sample_ringbuilders(3) + builder_files = self.save_builders(builders) + do_test([], force=False) + do_test([], force=True) # this error is never ignored + do_test(builder_files[:1], force=False) + do_test(builder_files[:1], force=True) # this error is never ignored + + def test_load_components_missing_builder_id(self): + def check_missing_id(cb, builders): + # not ok to load builder_files that have no id assigned + orig_version = cb.version + no_id = random.randint(0, len(builders) - 1) + # rewrite the builder files so that one has missing id + builder_files = self.save_builders(builders, missing_ids=[no_id]) + + def do_check(force): + with self.assertRaises(ValueError) as cm: + self._call_method_under_test(cb, builder_files, + force=force) + error_lines = cm.exception.message.split('\n') + self.assertIn("Problem with builder at index %s" % no_id, + error_lines[0]) + self.assertIn("id attribute has not been initialised", + error_lines[0]) + self.assertFalse(error_lines[1:]) + self.assertEqual(orig_version, cb.version) + + do_check(False) + do_check(True) # we never ignore this error + + # check with compose not previously called, cb has no existing metadata + builders = self.create_sample_ringbuilders(3) + cb = CompositeRingBuilder() + check_missing_id(cb, builders) + # now save good copies of builders and compose so this cb has + # existing component metadata + builder_files = self.save_builders(builders) + cb = CompositeRingBuilder(builder_files) + cb.compose() # cb now has component metadata + check_missing_id(cb, builders) + + def test_load_components_duplicate_builder_ids(self): + builders = self.create_sample_ringbuilders(3) + builders[2]._id = builders[0]._id + cb = CompositeRingBuilder(self.save_builders(builders)) + + def do_check(force): + with self.assertRaises(ValueError) as cm: + self._call_method_under_test(cb, force=force) + error_lines = cm.exception.message.split('\n') + self.assertIn("Builder id %r used at indexes 0, 2" % + builders[0].id, error_lines[0]) + self.assertFalse(error_lines[1:]) + self.assertEqual(0, cb.version) + + do_check(False) + do_check(True) + + def test_load_components_unchanged_builders(self): + def do_test(cb, builder_files, **kwargs): + orig_version = cb.version + with self.assertRaises(ValueError) as cm: + self._call_method_under_test(cb, builder_files, **kwargs) + error_lines = cm.exception.message.split('\n') + self.assertIn("None of the component builders has been modified", + error_lines[0]) + self.assertFalse(error_lines[1:]) + self.assertEqual(orig_version, cb.version) + + builders = self.create_sample_ringbuilders(2) + cb, builder_files = self._make_composite_builder(builders) + # ok to load same *unchanged* builders + self._call_method_under_test(cb, builder_files) + # unless require_modified is set + do_test(cb, builder_files, require_modified=True) + # even if we rewrite the files + builder_files = self.save_builders(builders) + do_test(cb, builder_files, require_modified=True) + # even if we rename the files + builder_files = self.save_builders(builders, prefix='other') + do_test(cb, builder_files, require_modified=True) + # force trumps require_modified + self._call_method_under_test(cb, builder_files, force=True, + require_modified=True) + + def test_load_components_older_builder(self): + # make first version of composite ring + builders = self.create_sample_ringbuilders(2) + cb, builder_files = self._make_composite_builder(builders) + old_builders = [copy.deepcopy(b) for b in builders] + # update components and reload + for i, b in enumerate(builders): + self.add_dev_and_rebalance(b) + self.assertLess(old_builders[i].version, b.version) + self.save_builders(builders) + self._call_method_under_test(cb) + orig_version = cb.version + cb.compose() # compose with newer builder versions + self.assertEqual(orig_version + 1, cb.version) # sanity check + # not ok to use old versions of same builders + self.save_builders([old_builders[0], builders[1]]) + with self.assertRaises(ValueError) as cm: + self._call_method_under_test(cb) + error_lines = cm.exception.message.split('\n') + self.assertIn("Invalid builder change at index 0", error_lines[0]) + self.assertIn("Older builder version", error_lines[0]) + self.assertFalse(error_lines[1:]) + self.assertEqual(orig_version + 1, cb.version) + # not even if one component ring has changed + self.add_dev_and_rebalance(builders[1]) + self.save_builders([old_builders[0], builders[1]]) + with self.assertRaises(ValueError) as cm: + self._call_method_under_test(cb) + error_lines = cm.exception.message.split('\n') + self.assertIn("Invalid builder change at index 0", error_lines[0]) + self.assertIn("Older builder version", error_lines[0]) + self.assertFalse(error_lines[1:]) + self.assertEqual(orig_version + 1, cb.version) + # unless we ignore errors + self._call_method_under_test(cb, force=True) + self.assertEqual(old_builders[0].version, cb._builders[0].version) + + def test_load_components_different_number_builders(self): + # not ok to use a different number of component rings + builders = self.create_sample_ringbuilders(4) + + def do_test(bad_builders): + cb, builder_files = self._make_composite_builder(builders[:3]) + # expect an error + with self.assertRaises(ValueError) as cm: + self._call_method_under_test( + cb, self.save_builders(bad_builders)) + error_lines = cm.exception.message.split('\n') + self.assertFalse(error_lines[1:]) + self.assertEqual(1, cb.version) + # unless we ignore errors + self._call_method_under_test(cb, self.save_builders(bad_builders), + force=True) + self.assertEqual(len(bad_builders), len(cb._builders)) + return error_lines + + error_lines = do_test(builders[:2]) # too few + self.assertIn("Missing builder at index 2", error_lines[0]) + error_lines = do_test(builders) # too many + self.assertIn("Unexpected extra builder at index 3", error_lines[0]) + + def test_load_components_different_builders(self): + # not ok to change component rings + builders = self.create_sample_ringbuilders(3) + cb, builder_files = self._make_composite_builder(builders[:2]) + # ensure builder[0] is newer version so that's not the problem + self.add_dev_and_rebalance(builders[0]) + different_files = self.save_builders([builders[0], builders[2]]) + with self.assertRaises(ValueError) as cm: + self._call_method_under_test(cb, different_files) + error_lines = cm.exception.message.split('\n') + self.assertIn("Invalid builder change at index 1", error_lines[0]) + self.assertIn("Attribute mismatch for id", error_lines[0]) + self.assertFalse(error_lines[1:]) + self.assertEqual(1, cb.version) + # ok if we force + self._call_method_under_test(cb, different_files, force=True) + self.assertEqual(different_files, cb._builder_files) + + def test_load_component_different_builder_order(self): + # not ok to change order of component rings + builders = self.create_sample_ringbuilders(4) + cb, builder_files = self._make_composite_builder(builders) + builder_files.reverse() + with self.assertRaises(ValueError) as cm: + self._call_method_under_test(cb, builder_files) + error_lines = cm.exception.message.split('\n') + for i, line in enumerate(error_lines): + self.assertIn("Invalid builder change at index %s" % i, line) + self.assertIn("Attribute mismatch for id", line) + self.assertEqual(1, cb.version) + # ok if we force + self._call_method_under_test(cb, builder_files, force=True) + self.assertEqual(builder_files, cb._builder_files) + + def test_load_components_replica_count_changed(self): + # not ok to change the number of replicas in a ring + builders = self.create_sample_ringbuilders(3) + cb, builder_files = self._make_composite_builder(builders) + builders[0].set_replicas(4) + self.save_builders(builders) + with self.assertRaises(ValueError) as cm: + self._call_method_under_test(cb) + error_lines = cm.exception.message.split('\n') + for i, line in enumerate(error_lines): + self.assertIn("Invalid builder change at index 0", line) + self.assertIn("Attribute mismatch for replicas", line) + self.assertEqual(1, cb.version) + # ok if we force + self._call_method_under_test(cb, force=True) + + +class TestComposeLoadComponents(TestLoadComponents): + def _call_method_under_test(self, cb, *args, **kwargs): + cb.compose(*args, **kwargs) + + def test_load_components_replica_count_changed(self): + # For compose method this test differs from superclass when the force + # flag is used, because although the force flag causes load_components + # to skip checks, the actual ring composition fails. + # not ok to change the number of replicas in a ring + builders = self.create_sample_ringbuilders(3) + cb, builder_files = self._make_composite_builder(builders) + builders[0].set_replicas(4) + self.save_builders(builders) + with self.assertRaises(ValueError) as cm: + self._call_method_under_test(cb) + error_lines = cm.exception.message.split('\n') + for i, line in enumerate(error_lines): + self.assertIn("Invalid builder change at index 0", line) + self.assertIn("Attribute mismatch for replicas", line) + self.assertEqual(1, cb.version) + # if we force, then load_components succeeds but the compose pre + # validate will fail because the builder needs rebalancing + with self.assertRaises(ValueError) as cm: + self._call_method_under_test(cb, force=True) + error_lines = cm.exception.message.split('\n') + self.assertIn("Problem with builders", error_lines[0]) + self.assertIn("Builder needs rebalance", error_lines[1]) + self.assertFalse(error_lines[2:]) + self.assertEqual(1, cb.version) + + +class TestCooperativeRingBuilder(BaseTestCompositeBuilder): + def _make_coop_builder(self, region, composite_builder, rebalance=False): + rb = CooperativeRingBuilder(8, 3, 1, composite_builder) + if composite_builder._builders is None: + composite_builder._builders = [rb] + for i in range(3): + self.add_dev(rb, region=region) + if rebalance: + rb.rebalance() + self.assertEqual(self._partition_counts(rb), + {0: 256, 1: 256, 2: 256}) # sanity check + return rb + + def _partition_counts(self, builder, key='id'): + """ + Returns a dictionary mapping the given device key to (number of + partitions assigned to that key). + """ + return Counter(builder.devs[dev_id][key] + for part2dev_id in builder._replica2part2dev + for dev_id in part2dev_id) + + @mock.patch('swift.common.ring.builder.time') + def test_rebalance_respects_cobuilder_part_moves(self, mock_time): + def do_rebalance(builder): + old_part_devs = [builder._devs_for_part(part) + for part in range(builder.parts)] + num_moved, _, _ = builder.rebalance() + moved_parts = { + p for p in range(builder.parts) + if old_part_devs[p] != builder._devs_for_part(p)} + self.assertEqual(len(moved_parts), num_moved) # sanity check + return num_moved, moved_parts + + def num_parts_can_move(builder): + # note that can_part_move() gives consideration to the + # _part_moved_bitmap which is only reset when a rebalance starts + return len( + [p for p in range(builder.parts) + if super(CooperativeRingBuilder, builder)._can_part_move(p)]) + + mock_time.return_value = 0 + cb = CompositeRingBuilder() + rb1 = self._make_coop_builder(1, cb) + rb2 = self._make_coop_builder(2, cb) + rb3 = self._make_coop_builder(3, cb) + cb._builders = [rb1, rb2, rb3] + + # all cobuilders can perform initial rebalance + for rb in (rb1, rb2, rb3): + rb.rebalance() + actual = self._partition_counts(rb) + exp = {0: 256, 1: 256, 2: 256} + self.assertEqual(exp, actual, + 'Expected %s but got %s for region %s' % + (exp, actual, next(rb._iter_devs())['region'])) + + # jump forwards min_part_hours, both builders can move all parts + mock_time.return_value = 3600 + self.add_dev(rb1) + # sanity checks: rb1 and rb2 are both ready for rebalance + self.assertEqual(0, rb2.min_part_seconds_left) + self.assertEqual(0, rb1.min_part_seconds_left) + # ... but last_part_moves not yet updated to current epoch + self.assertEqual(0, num_parts_can_move(rb1)) + self.assertEqual(0, num_parts_can_move(rb2)) + # rebalancing rb1 will update epoch for both builders' last_part_moves + num_moved, rb1_parts_moved = do_rebalance(rb1) + self.assertEqual(192, num_moved) + self.assertEqual(self._partition_counts(rb1), + {0: 192, 1: 192, 2: 192, 3: 192}) + self.assertEqual(256, num_parts_can_move(rb2)) + self.assertEqual(64, num_parts_can_move(rb1)) + + # rebalancing rb2 - rb2 in isolation could potentially move all parts + # so would move 192 parts to new device, but it is constrained by rb1 + # only having 64 parts that can move + self.add_dev(rb2) + num_moved, rb2_parts_moved = do_rebalance(rb2) + self.assertEqual(64, num_moved) + counts = self._partition_counts(rb2) + self.assertEqual(counts[3], 64) + self.assertEqual([234, 235, 235], sorted(counts.values()[:3])) + self.assertFalse(rb2_parts_moved.intersection(rb1_parts_moved)) + self.assertEqual(192, num_parts_can_move(rb2)) + self.assertEqual(64, num_parts_can_move(rb1)) + + # rb3 can't rebalance - all parts moved while rebalancing rb1 and rb2 + self.add_dev(rb3) + num_moved, rb3_parts_moved = do_rebalance(rb3) + self.assertEqual(0, num_moved) + + # jump forwards min_part_hours, both builders can move all parts again, + # so now rb2 should be able to further rebalance + mock_time.return_value = 7200 + do_rebalance(rb2) + self.assertGreater(self._partition_counts(rb2)[3], 64) + self.assertLess(num_parts_can_move(rb2), 256) + self.assertEqual(256, num_parts_can_move(rb1)) # sanity check + + # but cobuilders will not prevent a rb rebalancing for first time + rb4 = self._make_coop_builder(4, cb, rebalance=False) + cb._builders.append(rb4) + num_moved, _, _ = rb4.rebalance() + self.assertEqual(3 * 256, num_moved) + + def test_rebalance_cobuilders(self): + # verify that co-builder methods are called during one builder's + # rebalance + @contextmanager + def mock_update_last_part_moves(): + # intercept calls to RingBuilder._update_last_part_moves (yes, the + # superclass method) and populate a dict mapping builder instance + # to a list of that builder's parent builder when method was called + calls = [] + orig_func = RingBuilder._update_last_part_moves + + def fake_update(builder): + calls.append(builder) + return orig_func(builder) + + with mock.patch( + 'swift.common.ring.RingBuilder._update_last_part_moves', + fake_update): + yield calls + + @contextmanager + def mock_can_part_move(): + # intercept calls to RingBuilder._can_part_move (yes, the + # superclass method) and populate a dict mapping builder instance + # to a list of that builder's parent builder when method was called + calls = defaultdict(list) + orig_func = RingBuilder._can_part_move + + def fake_can_part_move(builder, part): + calls[builder].append(part) + return orig_func(builder, part) + with mock.patch('swift.common.ring.RingBuilder._can_part_move', + fake_can_part_move): + yield calls + + # single component builder in parent builder + cb = CompositeRingBuilder() + rb1 = self._make_coop_builder(1, cb) + with mock_update_last_part_moves() as update_calls: + with mock_can_part_move() as can_part_move_calls: + rb1.rebalance() + self.assertEqual([rb1], update_calls) + self.assertEqual([rb1], can_part_move_calls.keys()) + self.assertEqual(512, len(can_part_move_calls[rb1])) + + # two component builders with same parent builder + cb = CompositeRingBuilder() + rb1 = self._make_coop_builder(1, cb) + rb2 = self._make_coop_builder(2, cb) + cb._builders = [rb1, rb2] + with mock_update_last_part_moves() as update_calls: + with mock_can_part_move() as can_part_move_calls: + rb2.rebalance() + # both builders get updated + self.assertEqual(sorted([rb1, rb2]), sorted(update_calls)) + # rb1 has never been rebalanced so no calls propagate from its + # can_part_move method to to its superclass _can_part_move method + self.assertEqual([rb2], can_part_move_calls.keys()) + + with mock_update_last_part_moves() as update_calls: + with mock_can_part_move() as can_part_move_calls: + rb1.rebalance() + # both builders get updated + self.assertEqual(sorted([rb1, rb2]), sorted(update_calls)) + + # rb1 is being rebalanced so gets checked, and rb2 also gets checked + self.assertEqual(sorted([rb1, rb2]), sorted(can_part_move_calls)) + self.assertEqual(512, len(can_part_move_calls[rb1])) + self.assertEqual(512, len(can_part_move_calls[rb2])) + + def test_save_then_load(self): + cb = CompositeRingBuilder() + coop_rb = self._make_coop_builder(1, cb, rebalance=True) + builder_file = os.path.join(self.tmpdir, 'test.builder') + coop_rb.save(builder_file) + cb = CompositeRingBuilder() + loaded_coop_rb = CooperativeRingBuilder.load(builder_file, + parent_builder=cb) + self.assertIs(cb, loaded_coop_rb.parent_builder) + self.assertEqual(coop_rb.to_dict(), loaded_coop_rb.to_dict()) + + # check can be loaded as superclass + loaded_rb = RingBuilder.load(builder_file) + self.assertEqual(coop_rb.to_dict(), loaded_rb.to_dict()) + + # check can load a saved superclass + rb = RingBuilder(6, 3, 0) + for _ in range(3): + self.add_dev(rb, region=1) + rb.save(builder_file) + cb = CompositeRingBuilder() + loaded_coop_rb = CooperativeRingBuilder.load(builder_file, + parent_builder=cb) + self.assertIs(cb, loaded_coop_rb.parent_builder) + self.assertEqual(rb.to_dict(), loaded_coop_rb.to_dict()) + if __name__ == '__main__': unittest.main()