From bd75a97ad47fa28025d3a769f1eb7f05072dcd25 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Thu, 26 Jan 2017 02:03:28 +0000 Subject: [PATCH] Warn about using EC with isa_l_rs_vand and nparity >= 5 We *know* there are combinations that will prevent decoding, and we're going to be increasingly aggressive about getting it out of clusters. Partial-Bug: 1639691 Change-Id: I50159c9d19f2385d5f60112e9aaefa1a68098313 --- swift/common/storage_policy.py | 22 +++++ test/unit/common/test_storage_policy.py | 108 ++++++++++++++++++++++++ 2 files changed, 130 insertions(+) diff --git a/swift/common/storage_policy.py b/swift/common/storage_policy.py index 0714e51dab..7b55cfed9b 100644 --- a/swift/common/storage_policy.py +++ b/swift/common/storage_policy.py @@ -12,8 +12,10 @@ # limitations under the License. +import logging import os import string +import sys import textwrap import six from six.moves.configparser import ConfigParser @@ -453,6 +455,26 @@ class ECStoragePolicy(BaseStoragePolicy): raise PolicyError('Invalid ec_object_segment_size %r' % ec_segment_size, index=self.idx) + if self._ec_type == 'isa_l_rs_vand' and self._ec_nparity >= 5: + logger = logging.getLogger("swift.common.storage_policy") + if not logger.handlers: + # If nothing else, log to stderr + logger.addHandler(logging.StreamHandler(sys.__stderr__)) + logger.warning( + 'Storage policy %s uses an EC configuration known to harm ' + 'data durability. Any data in this policy should be migrated. ' + 'See https://bugs.launchpad.net/swift/+bug/1639691 for ' + 'more information.' % self.name) + if not is_deprecated: + # TODO: To fully close bug 1639691, uncomment the raise and + # removing the warning below. This will be in the Pike release + # at the earliest. + logger.warning( + 'In a future release, this will prevent services from ' + 'starting unless the policy is marked as deprecated.') + # raise PolicyError('Storage policy %s MUST be deprecated' % + # self.name) + # Initialize PyECLib EC backend try: self.pyeclib_driver = \ diff --git a/test/unit/common/test_storage_policy.py b/test/unit/common/test_storage_policy.py index 5b9c38b913..102123f0de 100755 --- a/test/unit/common/test_storage_policy.py +++ b/test/unit/common/test_storage_policy.py @@ -12,7 +12,9 @@ # limitations under the License. """ Tests for swift.common.storage_policies """ +import contextlib import six +import logging import unittest import os import mock @@ -20,6 +22,7 @@ from functools import partial from six.moves.configparser import ConfigParser from tempfile import NamedTemporaryFile from test.unit import patch_policies, FakeRing, temptree, DEFAULT_TEST_EC_TYPE +import swift.common.storage_policy from swift.common.storage_policy import ( StoragePolicyCollection, POLICIES, PolicyError, parse_storage_policies, reload_storage_policies, get_policy_string, split_policy_string, @@ -30,6 +33,26 @@ from swift.common.exceptions import RingLoadError from pyeclib.ec_iface import ECDriver +class CapturingHandler(logging.Handler): + def __init__(self): + super(CapturingHandler, self).__init__() + self._records = [] + + def emit(self, record): + self._records.append(record) + + +@contextlib.contextmanager +def capture_logging(log_name): + captured = CapturingHandler() + logger = logging.getLogger(log_name) + logger.addHandler(captured) + try: + yield captured._records + finally: + logger.removeHandler(captured) + + @BaseStoragePolicy.register('fake') class FakeStoragePolicy(BaseStoragePolicy): """ @@ -582,6 +605,91 @@ class TestStoragePolicies(unittest.TestCase): PolicyError, "must specify a storage policy section " "for policy index 0", parse_storage_policies, bad_conf) + @mock.patch.object(swift.common.storage_policy, 'VALID_EC_TYPES', + ['isa_l_rs_vand', 'isa_l_rs_cauchy']) + @mock.patch('swift.common.storage_policy.ECDriver') + def test_known_bad_ec_config(self, mock_driver): + good_conf = self._conf(""" + [storage-policy:0] + name = bad-policy + policy_type = erasure_coding + ec_type = isa_l_rs_cauchy + ec_num_data_fragments = 10 + ec_num_parity_fragments = 5 + """) + + with capture_logging('swift.common.storage_policy') as records: + parse_storage_policies(good_conf) + mock_driver.assert_called_once() + mock_driver.reset_mock() + self.assertFalse([(r.levelname, r.message) for r in records]) + + good_conf = self._conf(""" + [storage-policy:0] + name = bad-policy + policy_type = erasure_coding + ec_type = isa_l_rs_vand + ec_num_data_fragments = 10 + ec_num_parity_fragments = 4 + """) + + with capture_logging('swift.common.storage_policy') as records: + parse_storage_policies(good_conf) + mock_driver.assert_called_once() + mock_driver.reset_mock() + self.assertFalse([(r.levelname, r.message) for r in records]) + + bad_conf = self._conf(""" + [storage-policy:0] + name = bad-policy + policy_type = erasure_coding + ec_type = isa_l_rs_vand + ec_num_data_fragments = 10 + ec_num_parity_fragments = 5 + """) + + with capture_logging('swift.common.storage_policy') as records: + parse_storage_policies(bad_conf) + mock_driver.assert_called_once() + mock_driver.reset_mock() + self.assertEqual([r.levelname for r in records], + ['WARNING', 'WARNING']) + for msg in ('known to harm data durability', + 'Any data in this policy should be migrated', + 'https://bugs.launchpad.net/swift/+bug/1639691'): + self.assertIn(msg, records[0].message) + self.assertIn('In a future release, this will prevent services from ' + 'starting', records[1].message) + + slightly_less_bad_conf = self._conf(""" + [storage-policy:0] + name = bad-policy + policy_type = erasure_coding + ec_type = isa_l_rs_vand + ec_num_data_fragments = 10 + ec_num_parity_fragments = 5 + deprecated = true + + [storage-policy:1] + name = good-policy + policy_type = erasure_coding + ec_type = isa_l_rs_cauchy + ec_num_data_fragments = 10 + ec_num_parity_fragments = 5 + default = true + """) + + with capture_logging('swift.common.storage_policy') as records: + parse_storage_policies(slightly_less_bad_conf) + self.assertEqual(2, mock_driver.call_count) + mock_driver.reset_mock() + self.assertEqual([r.levelname for r in records], + ['WARNING']) + for msg in ('known to harm data durability', + 'Any data in this policy should be migrated', + 'https://bugs.launchpad.net/swift/+bug/1639691'): + self.assertIn(msg, records[0].message) + def test_no_default(self): orig_conf = self._conf(""" [storage-policy:0]