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

@@ -851,15 +851,8 @@ swift-ring-builder <builder_file> 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 <builder_file> 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

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
@@ -1885,6 +1886,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,
@@ -1921,6 +1943,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"]