Disallow fractional replicas in EC policies

Change-Id: I873d7bf7de54e4b1dccdafc8a61f03c09a65dfbc
Closes-Bug: 1554391
Closes-Bug: 1677547

swift/common/ring/ring.py

@@ -35,6 +35,12 @@ from swift.common.utils import hash_path, validate_configuration
 from swift.common.ring.utils import tiers_for_dev
 
 
+def calc_replica_count(replica2part2dev_id):
+    base = len(replica2part2dev_id) - 1
+    extra = 1.0 * len(replica2part2dev_id[-1]) / len(replica2part2dev_id[0])
+    return base + extra
+
+
 class RingData(object):
     """Partitioned consistent hashing ring data (used for serialization)."""
 
@@ -49,6 +55,11 @@ class RingData(object):
             if dev is not None:
                 dev.setdefault("region", 1)
 
+    @property
+    def replica_count(self):
+        """Number of replicas (full or partial) used in the ring."""
+        return calc_replica_count(self._replica2part2dev_id)
+
     @classmethod
     def deserialize_v1(cls, gz_file, metadata_only=False):
         """
@@ -285,7 +296,7 @@ class Ring(object):
     @property
     def replica_count(self):
         """Number of replicas (full or partial) used in the ring."""
-        return len(self._replica2part2dev_id)
+        return calc_replica_count(self._replica2part2dev_id)
 
     @property
     def partition_count(self):

swift/common/storage_policy.py

@@ -617,13 +617,13 @@ class ECStoragePolicy(BaseStoragePolicy):
             considering the number of nodes in the primary list from the ring.
             """
 
-            configured_fragment_count = len(ring_data._replica2part2dev_id)
+            configured_fragment_count = ring_data.replica_count
             required_fragment_count = \
                 (self.ec_n_unique_fragments) * self.ec_duplication_factor
             if configured_fragment_count != required_fragment_count:
                 raise RingLoadError(
                     'EC ring for policy %s needs to be configured with '
-                    'exactly %d replicas. Got %d.' % (
+                    'exactly %d replicas. Got %s.' % (
                         self.name, required_fragment_count,
                         configured_fragment_count))
 

test/unit/cli/test_ringbuilder.py

@@ -133,7 +133,8 @@ class TestCommands(unittest.TestCase, RunSwiftRingBuilderMixin):
                     msg += '%3d: %s\n' % (i, line)
                 self.fail(msg)
 
-    def create_sample_ring(self, part_power=6, overload=None, empty=False):
+    def create_sample_ring(self, part_power=6, replicas=3, overload=None,
+                           empty=False):
         """
         Create a sample ring with four devices
 
@@ -149,7 +150,7 @@ class TestCommands(unittest.TestCase, RunSwiftRingBuilderMixin):
         except OSError:
             pass
 
-        ring = RingBuilder(part_power, 3, 1)
+        ring = RingBuilder(part_power, replicas, 1)
         if overload is not None:
             ring.set_overload(overload)
         if not empty:
@@ -2205,6 +2206,25 @@ class TestCommands(unittest.TestCase, RunSwiftRingBuilderMixin):
         exp_results = {'valid_exit_codes': [2]}
         self.run_srb(*argv, exp_results=exp_results)
 
+    def test_write_builder_fractional_replicas(self):
+        # Test builder file already exists
+        self.create_sample_ring(replicas=1.2)
+        argv = ["", self.tmpfile, "rebalance"]
+        self.assertSystemExit(EXIT_SUCCESS, ringbuilder.main, argv)
+
+        ring_file = os.path.join(os.path.dirname(self.tmpfile),
+                                 os.path.basename(self.tmpfile) + ".ring.gz")
+        os.remove(self.tmpfile)  # loses file...
+
+        argv = ["", ring_file, "write_builder", "24"]
+        self.assertIsNone(ringbuilder.main(argv))
+
+        # Note that we've picked up an extension
+        builder = RingBuilder.load(self.tmpfile + '.builder')
+        # Note that this is different from the original! But it more-closely
+        # reflects the reality that we have an extra replica for 12 of 64 parts
+        self.assertEqual(builder.replicas, 1.1875)
+
     def test_write_builder_after_device_removal(self):
         # Test regenerating builder file after having removed a device
         # and lost the builder file

test/unit/common/ring/test_ring.py

@@ -163,6 +163,21 @@ class TestRingData(unittest.TestCase):
         self.assertEqual(oct(stat.S_IMODE(os.stat(ring_fname).st_mode)),
                          '0644')
 
+    def test_replica_count(self):
+        rd = ring.RingData(
+            [[0, 1, 0, 1], [0, 1, 0, 1]],
+            [{'id': 0, 'zone': 0, 'ip': '10.1.1.0', 'port': 7000},
+             {'id': 1, 'zone': 1, 'ip': '10.1.1.1', 'port': 7000}],
+            30)
+        self.assertEqual(rd.replica_count, 2)
+
+        rd = ring.RingData(
+            [[0, 1, 0, 1], [0, 1, 0]],
+            [{'id': 0, 'zone': 0, 'ip': '10.1.1.0', 'port': 7000},
+             {'id': 1, 'zone': 1, 'ip': '10.1.1.1', 'port': 7000}],
+            30)
+        self.assertEqual(rd.replica_count, 1.75)
+
 
 class TestRing(TestRingBase):
 
@@ -217,6 +232,11 @@ class TestRing(TestRingBase):
                 mock.patch.object(utils, 'SWIFT_CONF_FILE', ''):
             self.assertRaises(SystemExit, ring.Ring, self.testdir, 'whatever')
 
+    def test_replica_count(self):
+        self.assertEqual(self.ring.replica_count, 3)
+        self.ring._replica2part2dev_id.append([0])
+        self.assertEqual(self.ring.replica_count, 3.25)
+
     def test_has_changed(self):
         self.assertFalse(self.ring.has_changed())
         os.utime(self.testgz, (time() + 60, time() + 60))

test/unit/common/test_storage_policy.py

@@ -1299,25 +1299,31 @@ class TestStoragePolicies(unittest.TestCase):
                             ec_ndata=4, ec_nparity=2,
                             ec_duplication_factor=2)
         ]
-        actual_load_ring_replicas = [8, 10, 7, 11]
         policies = StoragePolicyCollection(test_policies)
 
         class MockRingData(object):
             def __init__(self, num_replica):
-                self._replica2part2dev_id = [0] * num_replica
+                self.replica_count = num_replica
 
-        for policy, ring_replicas in zip(policies, actual_load_ring_replicas):
+        def do_test(actual_load_ring_replicas):
-            with mock.patch('swift.common.ring.ring.RingData.load',
+            for policy, ring_replicas in zip(policies,
-                            return_value=MockRingData(ring_replicas)):
+                                             actual_load_ring_replicas):
-                necessary_replica_num = \
+                with mock.patch('swift.common.ring.ring.RingData.load',
-                    policy.ec_n_unique_fragments * policy.ec_duplication_factor
+                                return_value=MockRingData(ring_replicas)):
-                with mock.patch(
+                    necessary_replica_num = (policy.ec_n_unique_fragments *
-                        'swift.common.ring.ring.validate_configuration'):
+                                             policy.ec_duplication_factor)
-                    msg = 'EC ring for policy %s needs to be configured with ' \
+                    with mock.patch(
-                          'exactly %d replicas.' % \
+                            'swift.common.ring.ring.validate_configuration'):
-                          (policy.name, necessary_replica_num)
+                        msg = 'EC ring for policy %s needs to be configured ' \
-                    self.assertRaisesWithMessage(RingLoadError, msg,
+                              'with exactly %d replicas.' % \
-                                                 policy.load_ring, 'mock')
+                              (policy.name, necessary_replica_num)
+                        self.assertRaisesWithMessage(RingLoadError, msg,
+                                                     policy.load_ring, 'mock')
+
+        # first, do somethign completely different
+        do_test([8, 10, 7, 11])
+        # then again, closer to true, but fractional
+        do_test([9.9, 14.1, 5.99999, 12.000000001])
 
     def test_storage_policy_get_info(self):
         test_policies = [