From 47901ea4d7fe5ffbbb826d561ad4a02dc4ea8f4a Mon Sep 17 00:00:00 2001 From: Christian Schwede Date: Wed, 8 Jun 2016 09:20:26 +0000 Subject: [PATCH] Rebalance with min_part_seconds_left > 0 As described in bug #1558754 a rebalance after removing a device fails if min_part_seconds_left > 0, despite the note that a rebalance should remove partitions from removed devices on the next run. This patch skips the early exit, and lets the builder itself handling the rebalance. Partitions that shouldn't move now are still not moved (except for removed devices), and there is still a warning if no partition has been moved due to the fact that min_part_hours did not yet pass. A small test has been added to ensure rebalancing after removing a device works without using the --force option (tests fails on current master). Another test ensures that a rebalance after a recent change (for example increasing a device's weight) does not move partitions and still reports the former warning message. Closes-Bug: 1558754 Change-Id: I083022d066338cbe6234bab491c7a8e8e0a7b517 --- swift/cli/ringbuilder.py | 17 +++++----- test/unit/cli/test_ringbuilder.py | 56 +++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 9 deletions(-) diff --git a/swift/cli/ringbuilder.py b/swift/cli/ringbuilder.py index 5e812b9a39..3f1183a09e 100644 --- a/swift/cli/ringbuilder.py +++ b/swift/cli/ringbuilder.py @@ -851,15 +851,8 @@ swift-ring-builder rebalance [options] handler.setFormatter(formatter) logger.addHandler(handler) - if builder.min_part_seconds_left > 0 and not options.force: - print('No partitions could be reassigned.') - print('The time between rebalances must be at least ' - 'min_part_hours: %s hours (%s remaining)' % ( - builder.min_part_hours, - timedelta(seconds=builder.min_part_seconds_left))) - exit(EXIT_WARNING) - devs_changed = builder.devs_changed + min_part_seconds_left = builder.min_part_seconds_left try: last_balance = builder.get_balance() parts, balance, removed_devs = builder.rebalance(seed=get_seed(3)) @@ -874,7 +867,13 @@ swift-ring-builder rebalance [options] exit(EXIT_ERROR) if not (parts or options.force or removed_devs): print('No partitions could be reassigned.') - print('There is no need to do so at this time') + if min_part_seconds_left > 0: + print('The time between rebalances must be at least ' + 'min_part_hours: %s hours (%s remaining)' % ( + builder.min_part_hours, + timedelta(seconds=builder.min_part_seconds_left))) + else: + print('There is no need to do so at this time') exit(EXIT_WARNING) # If we set device's weight to zero, currently balance will be set # special value(MAX_BALANCE) until zero weighted device return all diff --git a/test/unit/cli/test_ringbuilder.py b/test/unit/cli/test_ringbuilder.py index b3e33acb54..ba8247ccfa 100644 --- a/test/unit/cli/test_ringbuilder.py +++ b/test/unit/cli/test_ringbuilder.py @@ -25,6 +25,7 @@ import unittest import uuid import shlex import shutil +import time from swift.cli import ringbuilder from swift.cli.ringbuilder import EXIT_SUCCESS, EXIT_WARNING, EXIT_ERROR @@ -1886,6 +1887,27 @@ class TestCommands(unittest.TestCase, RunSwiftRingBuilderMixin): ring = RingBuilder.load(self.tmpfile) self.assertEqual(ring.min_part_seconds_left, 3600) + def test_time_remaining(self): + self.create_sample_ring() + self.run_srb('rebalance') + now = time.time() + with mock.patch('swift.common.ring.builder.time', return_value=now): + out, err = self.run_srb('rebalance') + self.assertIn('No partitions could be reassigned', out) + self.assertIn('must be at least min_part_hours', out) + self.assertIn('1 hours (1:00:00 remaining)', out) + the_future = now + 3600 + with mock.patch('swift.common.ring.builder.time', + return_value=the_future): + out, err = self.run_srb('rebalance') + self.assertIn('No partitions could be reassigned', out) + self.assertIn('There is no need to do so at this time', out) + # or you can pretend_min_part_hours_passed + self.run_srb('pretend_min_part_hours_passed') + out, err = self.run_srb('rebalance') + self.assertIn('No partitions could be reassigned', out) + self.assertIn('There is no need to do so at this time', out) + def test_rebalance_failure_does_not_reset_last_moves_epoch(self): ring = RingBuilder(8, 3, 1) ring.add_dev({'id': 0, 'region': 0, 'zone': 0, 'weight': 1, @@ -1922,6 +1944,40 @@ class TestCommands(unittest.TestCase, RunSwiftRingBuilderMixin): argv = ["", self.tmpfile, "rebalance", "--seed", "2"] self.assertSystemExit(EXIT_SUCCESS, ringbuilder.main, argv) + def test_rebalance_removed_devices(self): + self.create_sample_ring() + argvs = [ + ["", self.tmpfile, "rebalance", "3"], + ["", self.tmpfile, "remove", "d0"], + ["", self.tmpfile, "rebalance", "3"]] + for argv in argvs: + self.assertSystemExit(EXIT_SUCCESS, ringbuilder.main, argv) + + def test_rebalance_min_part_hours_not_passed(self): + self.create_sample_ring() + argvs = [ + ["", self.tmpfile, "rebalance", "3"], + ["", self.tmpfile, "set_weight", "d0", "1000"]] + for argv in argvs: + self.assertSystemExit(EXIT_SUCCESS, ringbuilder.main, argv) + + ring = RingBuilder.load(self.tmpfile) + last_replica2part2dev = ring._replica2part2dev + + mock_stdout = six.StringIO() + argv = ["", self.tmpfile, "rebalance", "3"] + with mock.patch("sys.stdout", mock_stdout): + self.assertSystemExit(EXIT_WARNING, ringbuilder.main, argv) + expected = "No partitions could be reassigned.\n" + \ + "The time between rebalances must be " + \ + "at least min_part_hours: 1 hours" + self.assertTrue(expected in mock_stdout.getvalue()) + + # Messages can be faked, so let's assure that the partition assignment + # did not change at all, despite the warning + ring = RingBuilder.load(self.tmpfile) + self.assertEqual(last_replica2part2dev, ring._replica2part2dev) + def test_write_ring(self): self.create_sample_ring() argv = ["", self.tmpfile, "rebalance"]