Respect device weights when adding replicas

Previously, any new (partition, replica) pairs created by adding
replicas were spread evenly across the whole ring. Now it respects
device weights, so more (partition, replica) pairs are placed on
higher-weight devices.

Note that this only affects partition assignment *when the replica
count is increased*. Normal adding of disks without changing the
replica count is, and was, just fine.

The cause was that the devices' parts_wanted values weren't being
updated when the replica count went up. Since adding replicas makes
more things to assign, each device's desired share should have gone
up, but it didn't. This appears to be a simple oversight on the part
of the original author (me).

Change-Id: Idab14be90fab243c1077a584396a9981a4bd8638

2 files changed, 61 insertions, 1 deletions

diff --git a/swift/common/ring/builder.py b/swift/common/ring/builder.py
index 978e6c51f..141ae7440 100644
--- a/swift/common/ring/builder.py
+++ b/swift/common/ring/builder.py
@@ -345,6 +345,8 @@ class RingBuilder(object):
         last_balance = 0
         new_parts, removed_part_count = self._adjust_replica2part2dev_size()
         retval += removed_part_count
+        if new_parts or removed_part_count:
+            self._set_parts_wanted()
         self._reassign_parts(new_parts)
         retval += len(new_parts)
         while True:
@@ -584,7 +586,7 @@ class RingBuilder(object):
                 self._replica2part2dev.append(
                     array('H', (0 for _junk in xrange(desired_length))))
 
-        return (list(to_assign.iteritems()), removed_replicas)
+        return (to_assign.items(), removed_replicas)
 
     def _initial_balance(self):
         """
diff --git a/test/unit/common/ring/test_builder.py b/test/unit/common/ring/test_builder.py
index 164753099..c42e33891 100644
--- a/test/unit/common/ring/test_builder.py
+++ b/test/unit/common/ring/test_builder.py
@@ -698,6 +698,64 @@ class TestRingBuilder(unittest.TestCase):
         self.assertEqual([len(p2d) for p2d in rb._replica2part2dev],
                          [256, 256, 128])
 
+    def test_add_replicas_then_rebalance_respects_weight(self):
+        rb = ring.RingBuilder(8, 3, 1)
+        rb.add_dev({'id': 0, 'region': 0, 'region': 0, 'zone': 0, 'weight': 3,
+                    'ip': '127.0.0.1', 'port': 10000, 'device': 'sda'})
+        rb.add_dev({'id': 1, 'region': 0, 'region': 0, 'zone': 0, 'weight': 3,
+                    'ip': '127.0.0.1', 'port': 10000, 'device': 'sdb'})
+        rb.add_dev({'id': 2, 'region': 0, 'region': 0, 'zone': 0, 'weight': 1,
+                    'ip': '127.0.0.1', 'port': 10000, 'device': 'sdc'})
+        rb.add_dev({'id': 3, 'region': 0, 'region': 0, 'zone': 0, 'weight': 1,
+                    'ip': '127.0.0.1', 'port': 10000, 'device': 'sdd'})
+
+        rb.add_dev({'id': 4, 'region': 0, 'region': 0, 'zone': 1, 'weight': 3,
+                    'ip': '127.0.0.1', 'port': 10000, 'device': 'sde'})
+        rb.add_dev({'id': 5, 'region': 0, 'region': 0, 'zone': 1, 'weight': 3,
+                    'ip': '127.0.0.1', 'port': 10000, 'device': 'sdf'})
+        rb.add_dev({'id': 6, 'region': 0, 'region': 0, 'zone': 1, 'weight': 1,
+                    'ip': '127.0.0.1', 'port': 10000, 'device': 'sdg'})
+        rb.add_dev({'id': 7, 'region': 0, 'region': 0, 'zone': 1, 'weight': 1,
+                    'ip': '127.0.0.1', 'port': 10000, 'device': 'sdh'})
+
+        rb.add_dev({'id': 8, 'region': 0, 'region': 0, 'zone': 2, 'weight': 3,
+                    'ip': '127.0.0.1', 'port': 10000, 'device': 'sdi'})
+        rb.add_dev({'id': 9, 'region': 0, 'region': 0, 'zone': 2, 'weight': 3,
+                    'ip': '127.0.0.1', 'port': 10000, 'device': 'sdj'})
+        rb.add_dev({'id': 10, 'region': 0, 'region': 0, 'zone': 2, 'weight': 1,
+                    'ip': '127.0.0.1', 'port': 10000, 'device': 'sdk'})
+        rb.add_dev({'id': 11, 'region': 0, 'region': 0, 'zone': 2, 'weight': 1,
+                    'ip': '127.0.0.1', 'port': 10000, 'device': 'sdl'})
+
+        rb.rebalance(seed=1)
+
+        r = rb.get_ring()
+        counts = {}
+        for part2dev_id in r._replica2part2dev_id:
+            for dev_id in part2dev_id:
+                counts[dev_id] = counts.get(dev_id, 0) + 1
+        self.assertEquals(counts, {0: 96, 1: 96,
+                                   2: 32, 3: 32,
+                                   4: 96, 5: 96,
+                                   6: 32, 7: 32,
+                                   8: 96, 9: 96,
+                                   10: 32, 11: 32})
+
+        rb.replicas *= 2
+        rb.rebalance(seed=1)
+
+        r = rb.get_ring()
+        counts = {}
+        for part2dev_id in r._replica2part2dev_id:
+            for dev_id in part2dev_id:
+                counts[dev_id] = counts.get(dev_id, 0) + 1
+        self.assertEquals(counts, {0: 192, 1: 192,
+                                   2: 64, 3: 64,
+                                   4: 192, 5: 192,
+                                   6: 64, 7: 64,
+                                   8: 192, 9: 192,
+                                   10: 64, 11: 64})
+
     def test_load(self):
         rb = ring.RingBuilder(8, 3, 1)
         devs = [{'id': 0, 'region': 0, 'zone': 0, 'weight': 1,