From 568036f1b74e1057462401492ed323958d9d8ff5 Mon Sep 17 00:00:00 2001 From: Alistair Coles Date: Wed, 17 Mar 2021 15:25:14 +0000 Subject: [PATCH] manage-shard-ranges: add --dry-run option for compact and repair Change-Id: Ib566a2618abd78aa08a9b540000b2094e7f0e25e --- swift/cli/manage_shard_ranges.py | 69 +++++++++------ test/unit/cli/test_manage_shard_ranges.py | 101 ++++++++++++++-------- 2 files changed, 106 insertions(+), 64 deletions(-) diff --git a/swift/cli/manage_shard_ranges.py b/swift/cli/manage_shard_ranges.py index 995c23df35..6e48084e5b 100644 --- a/swift/cli/manage_shard_ranges.py +++ b/swift/cli/manage_shard_ranges.py @@ -205,6 +205,20 @@ class InvalidSolutionException(ManageShardRangesException): self.overlapping_donors = overlapping_donors +def _proceed(args): + if args.dry_run: + choice = 'no' + elif args.yes: + choice = 'yes' + else: + choice = input('Do you want to apply these changes to the container ' + 'DB? [yes/N]') + if choice != 'yes': + print('No changes applied') + + return choice == 'yes' + + def _print_shard_range(sr, level=0): indent = ' ' * level print(indent + '%r' % sr.name) @@ -501,22 +515,20 @@ def compact_shard_ranges(broker, args): file=sys.stderr) return EXIT_ERROR - if not args.yes: - for sequence in compactible: - acceptor = sequence[-1] - donors = sequence[:-1] - print('Donor shard range(s) with total of %d rows:' - % donors.row_count) - for donor in donors: - _print_shard_range(donor, level=1) - print('can be compacted into acceptor shard range:') - _print_shard_range(acceptor, level=1) - print('Once applied to the broker these changes will result in shard ' - 'range compaction the next time the sharder runs.') - choice = input('Do you want to apply these changes? [yes/N]') - if choice != 'yes': - print('No changes applied') - return EXIT_USER_QUIT + for sequence in compactible: + acceptor = sequence[-1] + donors = sequence[:-1] + print('Donor shard range(s) with total of %d rows:' + % donors.row_count) + for donor in donors: + _print_shard_range(donor, level=1) + print('can be compacted into acceptor shard range:') + _print_shard_range(acceptor, level=1) + print('Once applied to the broker these changes will result in shard ' + 'range compaction the next time the sharder runs.') + + if not _proceed(args): + return EXIT_USER_QUIT process_compactible_shard_sequences(broker, compactible) print('Updated %s shard sequences for compaction.' % len(compactible)) @@ -651,12 +663,8 @@ def repair_shard_ranges(broker, args): if not acceptor_path: return EXIT_SUCCESS - if not args.yes: - choice = input('Do you want to apply these changes to the container ' - 'DB? [yes/N]') - if choice != 'yes': - print('No changes applied') - return EXIT_USER_QUIT + if not _proceed(args): + return EXIT_USER_QUIT # merge changes to the broker... # note: acceptors do not need to be modified since they already span the @@ -717,10 +725,15 @@ def _add_enable_args(parser): help='DB timeout to use when enabling sharding.') -def _add_yes_arg(parser): +def _add_prompt_args(parser): parser.add_argument( '--yes', '-y', action='store_true', default=False, - help='Apply shard range changes to broker without prompting.') + help='Apply shard range changes to broker without prompting. ' + 'Cannot be used with --dry-run option.') + parser.add_argument( + '--dry-run', '-n', action='store_true', default=False, + help='Do not apply any shard range changes to broker. ' + 'Cannot be used with --yes option.') def _make_parser(): @@ -808,7 +821,7 @@ def _make_parser(): 'compact', help='Compact shard ranges with less than the shrink-threshold number ' 'of rows. This command only works on root containers.') - _add_yes_arg(compact_parser) + _add_prompt_args(compact_parser) compact_parser.add_argument('--shrink-threshold', nargs='?', type=_positive_int, default=None, @@ -849,7 +862,7 @@ def _make_parser(): 'repair', help='Repair overlapping shard ranges. No action will be taken ' 'without user confirmation unless the -y option is used.') - _add_yes_arg(repair_parser) + _add_prompt_args(repair_parser) repair_parser.set_defaults(func=repair_shard_ranges) # analyze @@ -875,6 +888,10 @@ def main(args=None): print('\nA sub-command is required.', file=sys.stderr) return EXIT_INVALID_ARGS + if getattr(args, 'yes', False) and getattr(args, 'dry_run', False): + print('--yes and --dry-run cannot both be set.', file=sys.stderr) + return EXIT_INVALID_ARGS + conf = {} rows_per_shard = DEFAULT_ROWS_PER_SHARD shrink_threshold = DEFAULT_SHRINK_THRESHOLD diff --git a/test/unit/cli/test_manage_shard_ranges.py b/test/unit/cli/test_manage_shard_ranges.py index de3e010532..d6a646e9ab 100644 --- a/test/unit/cli/test_manage_shard_ranges.py +++ b/test/unit/cli/test_manage_shard_ranges.py @@ -203,7 +203,8 @@ class TestManageShardRanges(unittest.TestCase): max_shrinking=1, shrink_threshold=100000, expansion_limit=500000, - yes=False) + yes=False, + dry_run=False) mocked.assert_called_once_with(mock.ANY, expected) # conf file @@ -221,7 +222,8 @@ class TestManageShardRanges(unittest.TestCase): max_shrinking=33, shrink_threshold=150, expansion_limit=650, - yes=False) + yes=False, + dry_run=False) mocked.assert_called_once_with(mock.ANY, expected) # conf file - small percentages resulting in zero absolute values @@ -253,7 +255,8 @@ class TestManageShardRanges(unittest.TestCase): max_shrinking=33, shrink_threshold=0, expansion_limit=0, - yes=False) + yes=False, + dry_run=False) mocked.assert_called_once_with(mock.ANY, expected) # cli options @@ -275,7 +278,8 @@ class TestManageShardRanges(unittest.TestCase): max_shrinking=22, shrink_threshold=1234, expansion_limit=3456, - yes=False) + yes=False, + dry_run=False) mocked.assert_called_once_with(mock.ANY, expected) # conf file - invalid value for shard_container_threshold @@ -894,7 +898,7 @@ class TestManageShardRanges(unittest.TestCase): 'shard range compaction the next time the sharder runs.', ] - def do_compact(user_input, exit_code): + def do_compact(user_input, options, exp_changes, exit_code): out = StringIO() err = StringIO() with mock.patch('sys.stdout', out),\ @@ -902,13 +906,13 @@ class TestManageShardRanges(unittest.TestCase): mock.patch('swift.cli.manage_shard_ranges.input', return_value=user_input): ret = main([broker.db_file, 'compact', - '--max-shrinking', '99']) + '--max-shrinking', '99'] + options) self.assertEqual(exit_code, ret) err_lines = err.getvalue().split('\n') self.assert_starts_with(err_lines[0], 'Loaded db broker for ') out_lines = out.getvalue().split('\n') expected = list(expected_base) - if user_input == 'yes': + if exp_changes: expected.extend([ 'Updated 2 shard sequences for compaction.', 'Run container-replicator to replicate the changes to ' @@ -924,13 +928,19 @@ class TestManageShardRanges(unittest.TestCase): self.assertEqual(expected, [l.split('/', 1)[0] for l in out_lines]) return broker.get_shard_ranges() - broker_ranges = do_compact('n', 3) + broker_ranges = do_compact('n', [], False, 3) # expect no changes to shard ranges self.assertEqual(shard_ranges, broker_ranges) for i, sr in enumerate(broker_ranges): self.assertEqual(ShardRange.ACTIVE, sr.state) - broker_ranges = do_compact('yes', 0) + broker_ranges = do_compact('yes', ['--dry-run'], False, 3) + # expect no changes to shard ranges + self.assertEqual(shard_ranges, broker_ranges) + for i, sr in enumerate(broker_ranges): + self.assertEqual(ShardRange.ACTIVE, sr.state) + + broker_ranges = do_compact('yes', [], True, 0) # expect updated shard ranges shard_ranges[5].lower = shard_ranges[3].lower shard_ranges[8].lower = shard_ranges[7].lower @@ -960,9 +970,7 @@ class TestManageShardRanges(unittest.TestCase): err_lines = err.getvalue().split('\n') self.assert_starts_with(err_lines[0], 'Loaded db broker for ') out_lines = out.getvalue().split('\n') - self.assertEqual( - ['Updated 2 shard sequences for compaction.'], - out_lines[:1]) + self.assertIn('Updated 2 shard sequences for compaction.', out_lines) updated_ranges = broker.get_shard_ranges() for i, sr in enumerate(updated_ranges): if i in small_ranges: @@ -1013,9 +1021,7 @@ class TestManageShardRanges(unittest.TestCase): err_lines = err.getvalue().split('\n') self.assert_starts_with(err_lines[0], 'Loaded db broker for ') out_lines = out.getvalue().split('\n') - self.assertEqual( - ['Updated 1 shard sequences for compaction.'], - out_lines[:1]) + self.assertIn('Updated 1 shard sequences for compaction.', out_lines) updated_ranges = broker.get_shard_ranges() self.assertEqual(shard_ranges, updated_ranges) self.assertEqual([ShardRange.SHRINKING] * 10, @@ -1052,9 +1058,7 @@ class TestManageShardRanges(unittest.TestCase): err_lines = err.getvalue().split('\n') self.assert_starts_with(err_lines[0], 'Loaded db broker for ') out_lines = out.getvalue().split('\n') - self.assertEqual( - ['Updated 1 shard sequences for compaction.'], - out_lines[:1]) + self.assertIn('Updated 1 shard sequences for compaction.', out_lines) updated_ranges = broker.get_shard_ranges() self.assertEqual(shard_ranges, updated_ranges) self.assertEqual([ShardRange.SHRINKING], @@ -1093,9 +1097,7 @@ class TestManageShardRanges(unittest.TestCase): err_lines = err.getvalue().split('\n') self.assert_starts_with(err_lines[0], 'Loaded db broker for ') out_lines = out.getvalue().split('\n') - self.assertEqual( - ['Updated 1 shard sequences for compaction.'], - out_lines[:1]) + self.assertIn('Updated 1 shard sequences for compaction.', out_lines) updated_ranges = broker.get_shard_ranges() shard_ranges[9].lower = shard_ranges[4].lower # expanded acceptor self.assertEqual(shard_ranges, updated_ranges) @@ -1126,9 +1128,7 @@ class TestManageShardRanges(unittest.TestCase): err_lines = err.getvalue().split('\n') self.assert_starts_with(err_lines[0], 'Loaded db broker for ') out_lines = out.getvalue().split('\n') - self.assertEqual( - ['Updated 2 shard sequences for compaction.'], - out_lines[:1]) + self.assertIn('Updated 2 shard sequences for compaction.', out_lines) updated_ranges = broker.get_shard_ranges() gapped_ranges[2].lower = gapped_ranges[0].lower gapped_ranges[8].lower = gapped_ranges[3].lower @@ -1155,7 +1155,7 @@ class TestManageShardRanges(unittest.TestCase): err_lines = err.getvalue().split('\n') self.assert_starts_with(err_lines[0], 'Loaded db broker for ') out_lines = out.getvalue().split('\n') - self.assertEqual([expect_msg], out_lines[:1]) + self.assertIn(expect_msg, out_lines) return broker.get_shard_ranges() updated_ranges = do_compact( @@ -1191,7 +1191,7 @@ class TestManageShardRanges(unittest.TestCase): err_lines = err.getvalue().split('\n') self.assert_starts_with(err_lines[0], 'Loaded db broker for ') out_lines = out.getvalue().split('\n') - self.assertEqual([expect_msg], out_lines[:1]) + self.assertIn(expect_msg, out_lines) return broker.get_shard_ranges() updated_ranges = do_compact( @@ -1231,7 +1231,7 @@ class TestManageShardRanges(unittest.TestCase): err_lines = err.getvalue().split('\n') self.assert_starts_with(err_lines[0], 'Loaded db broker for ') out_lines = out.getvalue().split('\n') - self.assertEqual([expect_msg], out_lines[:1]) + self.assertIn(expect_msg, out_lines) return broker.get_shard_ranges() updated_ranges = do_compact( @@ -1267,10 +1267,8 @@ class TestManageShardRanges(unittest.TestCase): self.assertEqual(0, ret, out.getvalue()) err_lines = err.getvalue().split('\n') self.assert_starts_with(err_lines[0], 'Loaded db broker for ') - out_lines = out.getvalue().split('\n') - self.assertEqual( - ['Updated 5 shard sequences for compaction.'], - out_lines[:1]) + out_lines = out.getvalue().rstrip('\n').split('\n') + self.assertIn('Updated 5 shard sequences for compaction.', out_lines) updated_ranges = broker.get_shard_ranges() shard_ranges[1].lower = shard_ranges[0].lower shard_ranges[3].lower = shard_ranges[2].lower @@ -1378,9 +1376,7 @@ class TestManageShardRanges(unittest.TestCase): err_lines = err.getvalue().split('\n') self.assert_starts_with(err_lines[0], 'Loaded db broker for ') out_lines = out.getvalue().split('\n') - self.assertEqual( - ['Updated 1 shard sequences for compaction.'], - out_lines[:1]) + self.assertIn('Updated 1 shard sequences for compaction.', out_lines) updated_ranges = broker.get_shard_ranges() shard_ranges[8].lower = shard_ranges[0].lower self.assertEqual(shard_ranges, updated_ranges) @@ -1565,7 +1561,8 @@ class TestManageShardRanges(unittest.TestCase): broker.merge_shard_ranges(shard_ranges + overlap_shard_ranges_2) self.assertTrue(broker.is_root_container()) - def do_repair(user_input, ts_now, exit_code): + def do_repair(user_input, ts_now, options, exit_code): + options = options if options else [] out = StringIO() err = StringIO() with mock.patch('sys.stdout', out), \ @@ -1573,7 +1570,7 @@ class TestManageShardRanges(unittest.TestCase): mock_timestamp_now(ts_now), \ mock.patch('swift.cli.manage_shard_ranges.input', return_value=user_input): - ret = main([broker.db_file, 'repair']) + ret = main([broker.db_file, 'repair'] + options) self.assertEqual(exit_code, ret) err_lines = err.getvalue().split('\n') self.assert_starts_with(err_lines[0], 'Loaded db broker for ') @@ -1584,7 +1581,25 @@ class TestManageShardRanges(unittest.TestCase): # user input 'n' ts_now = next(self.ts_iter) - do_repair('n', ts_now, 3) + do_repair('n', ts_now, [], 3) + updated_ranges = broker.get_shard_ranges() + expected = sorted( + shard_ranges + overlap_shard_ranges_2, + key=ShardRange.sort_key) + self.assert_shard_ranges_equal(expected, updated_ranges) + + # --dry-run + ts_now = next(self.ts_iter) + do_repair('y', ts_now, ['--dry-run'], 3) + updated_ranges = broker.get_shard_ranges() + expected = sorted( + shard_ranges + overlap_shard_ranges_2, + key=ShardRange.sort_key) + self.assert_shard_ranges_equal(expected, updated_ranges) + + # --n + ts_now = next(self.ts_iter) + do_repair('y', ts_now, ['-n'], 3) updated_ranges = broker.get_shard_ranges() expected = sorted( shard_ranges + overlap_shard_ranges_2, @@ -1593,7 +1608,7 @@ class TestManageShardRanges(unittest.TestCase): # user input 'yes' ts_now = next(self.ts_iter) - do_repair('yes', ts_now, 0) + do_repair('yes', ts_now, [], 0) updated_ranges = broker.get_shard_ranges() for sr in overlap_shard_ranges_2: sr.update_state(ShardRange.SHRINKING, ts_now) @@ -1755,3 +1770,13 @@ class TestManageShardRanges(unittest.TestCase): self.assertEqual(2, ret) err_lines = err.getvalue().split('\n') self.assertIn('A sub-command is required.', err_lines) + + def test_dry_run_and_yes_is_invalid(self): + out = StringIO() + err = StringIO() + with mock.patch('sys.stdout', out), \ + mock.patch('sys.stderr', err): + ret = main(['db file', 'repair', '--dry-run', '--yes']) + self.assertEqual(2, ret) + err_lines = err.getvalue().split('\n') + self.assertIn('--yes and --dry-run cannot both be set.', err_lines)