From 6709bb8891e59572cb6a1877addf7f7a3f9e6d56 Mon Sep 17 00:00:00 2001 From: Matthew Oliver Date: Thu, 8 Jul 2021 17:22:07 +1000 Subject: sharder: If saving own_shard_range use no_default=True This is just a belts and braces patch that makes sure that we use own_shard_range(no_default=True) if we assume there will always be an own_shard_range and it might be written back to the broker. I'm not sure it's even possible for there not to be an own_shard_range when we get to `_cleave_shard_range` or `_complete_sharding`. But in any case we definitely shouldn't allow the possiblity of a default own_shard_range in any case. Change-Id: I68c3e7558d04e8dcd5874f80b4763ec0b820331d --- swift/container/sharder.py | 18 ++++++++++++-- test/unit/container/test_sharder.py | 49 ++++++++++++++++++++++++++++++++++++- 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/swift/container/sharder.py b/swift/container/sharder.py index 8b44524cb..6d10e7dd5 100644 --- a/swift/container/sharder.py +++ b/swift/container/sharder.py @@ -1578,7 +1578,14 @@ class ContainerSharder(ContainerSharderConf, ContainerReplicator): self._increment_stat('cleaved', 'failure', statsd=True) return CLEAVE_FAILED - own_shard_range = broker.get_own_shard_range() + own_shard_range = broker.get_own_shard_range(no_default=True) + if own_shard_range is None: + # A default should never be SHRINKING or SHRUNK but because we + # may write own_shard_range back to broker, let's make sure + # it can't be defaulted. + self.logger.warning('Failed to get own_shard_range for %s', + quote(broker.path)) + return CLEAVE_FAILED # only cleave from the retiring db - misplaced objects handler will # deal with any objects in the fresh db @@ -1796,7 +1803,14 @@ class ContainerSharder(ContainerSharderConf, ContainerReplicator): # Move all CLEAVED shards to ACTIVE state and if a shard then # delete own shard range; these changes will be simultaneously # reported in the next update to the root container. - own_shard_range = broker.get_own_shard_range() + own_shard_range = broker.get_own_shard_range(no_default=True) + if own_shard_range is None: + # This is more of a belts and braces, not sure we could even + # get this far with without an own_shard_range. But because + # we will be writing own_shard_range back, we need to make sure + self.logger.warning('Failed to get own_shard_range for %s', + quote(broker.path)) + return False own_shard_range.update_meta(0, 0) if own_shard_range.state in (ShardRange.SHRINKING, ShardRange.SHRUNK): diff --git a/test/unit/container/test_sharder.py b/test/unit/container/test_sharder.py index 8dd632250..64083a6ba 100644 --- a/test/unit/container/test_sharder.py +++ b/test/unit/container/test_sharder.py @@ -41,7 +41,8 @@ from swift.container.sharder import ContainerSharder, sharding_enabled, \ CleavingContext, DEFAULT_SHARDER_CONF, finalize_shrinking, \ find_shrinking_candidates, process_compactible_shard_sequences, \ find_compactible_shard_sequences, is_shrinking_candidate, \ - is_sharding_candidate, find_paths, rank_paths, ContainerSharderConf + is_sharding_candidate, find_paths, rank_paths, ContainerSharderConf, \ + CLEAVE_FAILED from swift.common.utils import ShardRange, Timestamp, hash_path, \ encode_timestamps, parse_db_filename, quorum_size, Everything, md5 from test import annotate_failure @@ -1665,6 +1666,27 @@ class TestSharder(BaseTestSharder): self.assertEqual(cleaving_context.ranges_todo, 2) self.assertFalse(cleaving_context.cleaving_done) + def test_cleave_shard_range_no_own_shard_range(self): + broker = self._make_sharding_broker() + obj = {'name': 'obj', 'created_at': next(self.ts_iter).internal, + 'size': 14, 'content_type': 'text/plain', 'etag': 'an etag', + 'deleted': 0} + broker.get_brokers()[0].merge_items([obj]) + self.assertEqual(2, len(broker.db_files)) # sanity check + context = CleavingContext.load(broker) + shard_range = broker.get_shard_ranges()[0] + + with self._mock_sharder() as sharder, mock.patch( + 'swift.container.backend.ContainerBroker.get_own_shard_range', + return_value=None): + self.assertEqual( + sharder._cleave_shard_range(broker, context, shard_range), + CLEAVE_FAILED) + self.assertEqual(SHARDING, broker.get_db_state()) + warning_lines = sharder.logger.get_lines_for_level('warning') + self.assertEqual(warning_lines[0], + 'Failed to get own_shard_range for a/c') + def test_cleave_shard(self): broker = self._make_broker(account='.shards_a', container='shard_c') own_shard_range = ShardRange( @@ -2875,6 +2897,31 @@ class TestSharder(BaseTestSharder): '.shards_', 'shard_c', (('l', 'mid'), ('mid', 'u'))) self.assertEqual(1, broker.get_own_shard_range().deleted) + def test_complete_sharding_missing_own_shard_range(self): + broker = self._make_sharding_broker() + obj = {'name': 'obj', 'created_at': next(self.ts_iter).internal, + 'size': 14, 'content_type': 'text/plain', 'etag': 'an etag', + 'deleted': 0} + broker.get_brokers()[0].merge_items([obj]) + self.assertEqual(2, len(broker.db_files)) # sanity check + + # Make cleaving context_done + context = CleavingContext.load(broker) + self.assertEqual(1, context.max_row) + context.cleave_to_row = 1 # pretend all rows have been cleaved + context.cleaving_done = True + context.misplaced_done = True + context.store(broker) + + with self._mock_sharder() as sharder, mock.patch( + 'swift.container.backend.ContainerBroker.get_own_shard_range', + return_value=None): + self.assertFalse(sharder._complete_sharding(broker)) + self.assertEqual(SHARDING, broker.get_db_state()) + warning_lines = sharder.logger.get_lines_for_level('warning') + self.assertEqual(warning_lines[0], + 'Failed to get own_shard_range for a/c') + def test_sharded_record_sharding_progress_missing_contexts(self): broker = self._check_complete_sharding( 'a', 'c', (('', 'mid'), ('mid', ''))) -- cgit v1.2.1