From 8326dc9f2a34581b1e5ab0d27b32d3073827296f Mon Sep 17 00:00:00 2001 From: Paul Luse Date: Mon, 14 Apr 2014 14:04:37 -0700 Subject: [PATCH] Add Storage Policy Support to Recon Middleware Recon middleware returns object ring file MD5 sums; this patch updates it to include other object files that may be present because of Storage Policies. Also adds unit test coverage for the MD5 reporting function which previously had none. The recon script will now check all rings the server responds with match the on-disk md5's regardless of server-type; including any storage policy object rings. Note the small change to the ring save method, needed to stimulate the right code paths in 2.6 and 2.7 versions of gzip to enable testing of ring MD5 sums. DocImpact Implements: blueprint storage-policies Change-Id: I01efd2999d6d9c57ee8693ac3a6236ace17c5566 --- swift/cli/recon.py | 61 +++++++--- swift/common/middleware/recon.py | 8 +- swift/common/ring/ring.py | 6 +- test/unit/cli/test_recon.py | 43 +++++++ .../common/middleware/test_list_endpoints.py | 2 +- test/unit/common/middleware/test_recon.py | 108 +++++++++++++++++- 6 files changed, 202 insertions(+), 26 deletions(-) diff --git a/swift/cli/recon.py b/swift/cli/recon.py index c9390deb37..1394eafaa4 100755 --- a/swift/cli/recon.py +++ b/swift/cli/recon.py @@ -192,35 +192,61 @@ class SwiftRecon(object): ips = set((n['ip'], n['port']) for n in ring_data.devs if n) return ips - def get_ringmd5(self, hosts, ringfile): + def get_ringmd5(self, hosts, swift_dir): """ Compare ring md5sum's with those on remote host :param hosts: set of hosts to check. in the format of: set([('127.0.0.1', 6020), ('127.0.0.2', 6030)]) - :param ringfile: The local ring file to compare the md5sum with. + :param swift_dir: The local directory with the ring files. """ matches = 0 errors = 0 - ring_sum = self._md5_file(ringfile) + ring_names = set() + for server_type in ('account', 'container'): + ring_name = '%s.ring.gz' % server_type + ring_names.add(ring_name) + # include any other object ring files + for ring_name in os.listdir(swift_dir): + if ring_name.startswith('object') and \ + ring_name.endswith('ring.gz'): + ring_names.add(ring_name) + rings = {} + for ring_name in ring_names: + md5sum = md5() + with open(os.path.join(swift_dir, ring_name), 'rb') as f: + block = f.read(4096) + while block: + md5sum.update(block) + block = f.read(4096) + ring_sum = md5sum.hexdigest() + rings[ring_name] = ring_sum recon = Scout("ringmd5", self.verbose, self.suppress_errors, self.timeout) print("[%s] Checking ring md5sums" % self._ptime()) if self.verbose: - print("-> On disk %s md5sum: %s" % (ringfile, ring_sum)) + for ring_file, ring_sum in rings.items(): + print("-> On disk %s md5sum: %s" % (ring_file, ring_sum)) for url, response, status in self.pool.imap(recon.scout, hosts): - if status == 200: - if response[ringfile] != ring_sum: - print("!! %s (%s) doesn't match on disk md5sum" % - (url, response[ringfile])) - else: - matches = matches + 1 - if self.verbose: - print("-> %s matches." % url) - else: + if status != 200: errors = errors + 1 - print("%s/%s hosts matched, %s error[s] while checking hosts." - % (matches, len(hosts), errors)) + continue + success = True + for remote_ring_file, remote_ring_sum in response.items(): + remote_ring_name = os.path.basename(remote_ring_file) + ring_sum = rings.get(remote_ring_name, None) + if remote_ring_sum != ring_sum: + success = False + print("!! %s (%s => %s) doesn't match on disk md5sum" % ( + url, remote_ring_name, remote_ring_sum)) + if not success: + errors += 1 + continue + matches += 1 + if self.verbose: + print("-> %s matches." % url) + print("%s/%s hosts matched, %s error[s] while checking hosts." % ( + matches, len(hosts), errors)) print("=" * 79) def get_swiftconfmd5(self, hosts, printfn=print): @@ -867,7 +893,6 @@ class SwiftRecon(object): self.server_type = 'object' swift_dir = options.swiftdir - ring_file = os.path.join(swift_dir, '%s.ring.gz' % self.server_type) self.verbose = options.verbose self.suppress_errors = options.suppress self.timeout = options.timeout @@ -897,7 +922,7 @@ class SwiftRecon(object): self.umount_check(hosts) self.load_check(hosts) self.disk_usage(hosts) - self.get_ringmd5(hosts, ring_file) + self.get_ringmd5(hosts, swift_dir) self.quarantine_check(hosts) self.socket_usage(hosts) else: @@ -933,7 +958,7 @@ class SwiftRecon(object): if options.diskusage: self.disk_usage(hosts, options.top, options.human_readable) if options.md5: - self.get_ringmd5(hosts, ring_file) + self.get_ringmd5(hosts, swift_dir) self.get_swiftconfmd5(hosts) if options.quarantined: self.quarantine_check(hosts) diff --git a/swift/common/middleware/recon.py b/swift/common/middleware/recon.py index 745a18752f..9defe6bc6d 100644 --- a/swift/common/middleware/recon.py +++ b/swift/common/middleware/recon.py @@ -55,9 +55,11 @@ class ReconMiddleware(object): 'account.recon') self.account_ring_path = os.path.join(swift_dir, 'account.ring.gz') self.container_ring_path = os.path.join(swift_dir, 'container.ring.gz') - self.object_ring_path = os.path.join(swift_dir, 'object.ring.gz') - self.rings = [self.account_ring_path, self.container_ring_path, - self.object_ring_path] + self.rings = [self.account_ring_path, self.container_ring_path] + # include all object ring files (for all policies) + for f in os.listdir(swift_dir): + if f.startswith('object') and f.endswith('ring.gz'): + self.rings.append(os.path.join(swift_dir, f)) self.mount_check = config_true_value(conf.get('mount_check', 'true')) def _from_recon_cache(self, cache_keys, cache_file, openr=open): diff --git a/swift/common/ring/ring.py b/swift/common/ring/ring.py index 8caa5b8b6b..e7c03e835d 100644 --- a/swift/common/ring/ring.py +++ b/swift/common/ring/ring.py @@ -97,11 +97,13 @@ class RingData(object): for part2dev_id in ring['replica2part2dev_id']: file_obj.write(part2dev_id.tostring()) - def save(self, filename): + def save(self, filename, mtime=1300507380.0): """ Serialize this RingData instance to disk. :param filename: File into which this instance should be serialized. + :param mtime: time used to override mtime for gzip, default or None + if the caller wants to include time """ # Override the timestamp so that the same ring data creates # the same bytes on disk. This makes a checksum comparison a @@ -112,7 +114,7 @@ class RingData(object): tempf = NamedTemporaryFile(dir=".", prefix=filename, delete=False) try: gz_file = GzipFile(filename, mode='wb', fileobj=tempf, - mtime=1300507380.0) + mtime=mtime) except TypeError: gz_file = GzipFile(filename, mode='wb', fileobj=tempf) self.serialize_v1(gz_file) diff --git a/test/unit/cli/test_recon.py b/test/unit/cli/test_recon.py index 5190480b80..40051022a2 100644 --- a/test/unit/cli/test_recon.py +++ b/test/unit/cli/test_recon.py @@ -13,11 +13,13 @@ # See the License for the specific language governing permissions and # limitations under the License. +from contextlib import nested import json import mock import os import random import string +from StringIO import StringIO import tempfile import time import unittest @@ -148,6 +150,47 @@ class TestRecon(unittest.TestCase): 1, self.swift_dir, self.ring_name) self.assertEqual(set([('127.0.0.1', 10001)]), ips) + def test_get_ringmd5(self): + for server_type in ('account', 'container', 'object', 'object-1'): + ring_name = '%s.ring.gz' % server_type + ring_file = os.path.join(self.swift_dir, ring_name) + open(ring_file, 'w') + + empty_file_hash = 'd41d8cd98f00b204e9800998ecf8427e' + hosts = [("127.0.0.1", "8080")] + with mock.patch('swift.cli.recon.Scout') as mock_scout: + scout_instance = mock.MagicMock() + url = 'http://%s:%s/recon/ringmd5' % hosts[0] + response = { + '/etc/swift/account.ring.gz': empty_file_hash, + '/etc/swift/container.ring.gz': empty_file_hash, + '/etc/swift/object.ring.gz': empty_file_hash, + '/etc/swift/object-1.ring.gz': empty_file_hash, + } + status = 200 + scout_instance.scout.return_value = (url, response, status) + mock_scout.return_value = scout_instance + stdout = StringIO() + mock_hash = mock.MagicMock() + patches = [ + mock.patch('sys.stdout', new=stdout), + mock.patch('swift.cli.recon.md5', new=mock_hash), + ] + with nested(*patches): + mock_hash.return_value.hexdigest.return_value = \ + empty_file_hash + self.recon_instance.get_ringmd5(hosts, self.swift_dir) + output = stdout.getvalue() + expected = '1/1 hosts matched' + for line in output.splitlines(): + if '!!' in line: + self.fail('Unexpected Error in output: %r' % line) + if expected in line: + break + else: + self.fail('Did not find expected substring %r ' + 'in output:\n%s' % (expected, output)) + class TestReconCommands(unittest.TestCase): def setUp(self): diff --git a/test/unit/common/middleware/test_list_endpoints.py b/test/unit/common/middleware/test_list_endpoints.py index 52c3dc60ca..bb491ad81b 100644 --- a/test/unit/common/middleware/test_list_endpoints.py +++ b/test/unit/common/middleware/test_list_endpoints.py @@ -232,7 +232,7 @@ class TestListEndpoints(unittest.TestCase): self.assertEquals(resp.content_type, 'application/json') self.assertEquals(json.loads(resp.body), expected[pol.idx]) - # test ustom path without trailing slash + # test custom path without trailing slash custom_path_le = list_endpoints.filter_factory({ 'swift_dir': self.testdir, 'list_endpoints_path': '/some/another/path' diff --git a/test/unit/common/middleware/test_recon.py b/test/unit/common/middleware/test_recon.py index fb9269d601..799cdd6a29 100644 --- a/test/unit/common/middleware/test_recon.py +++ b/test/unit/common/middleware/test_recon.py @@ -17,13 +17,15 @@ import unittest from unittest import TestCase from contextlib import contextmanager from posix import stat_result, statvfs_result +import array +from swift.common import ring, utils +from shutil import rmtree import os import mock from swift import __version__ as swiftver from swift.common.swob import Request from swift.common.middleware import recon -from swift.common import utils def fake_check_mount(a, b): @@ -186,7 +188,13 @@ class FakeRecon(object): class TestReconSuccess(TestCase): def setUp(self): - self.app = recon.ReconMiddleware(FakeApp(), {}) + # can't use mkdtemp here as 2.6 gzip puts the filename in the header + # which will cause ring md5 checks to fail + self.tempdir = '/tmp/swift_recon_md5_test' + utils.mkdirs(self.tempdir) + self._create_rings() + self.app = recon.ReconMiddleware(FakeApp(), + {'swift_dir': self.tempdir}) self.mockos = MockOS() self.fakecache = FakeFromCache() self.real_listdir = os.listdir @@ -206,6 +214,96 @@ class TestReconSuccess(TestCase): del self.mockos self.app._from_recon_cache = self.real_from_cache del self.fakecache + rmtree(self.tempdir) + + def _create_rings(self): + + def fake_time(): + return 0 + + def fake_base(fname): + # least common denominator with gzip versions is to + # not use the .gz extension in the gzip header + return fname[:-3] + + accountgz = os.path.join(self.tempdir, 'account.ring.gz') + containergz = os.path.join(self.tempdir, 'container.ring.gz') + objectgz = os.path.join(self.tempdir, 'object.ring.gz') + objectgz_1 = os.path.join(self.tempdir, 'object-1.ring.gz') + objectgz_2 = os.path.join(self.tempdir, 'object-2.ring.gz') + + # make the rings unique so they have different md5 sums + intended_replica2part2dev_id_a = [ + array.array('H', [3, 1, 3, 1]), + array.array('H', [0, 3, 1, 4]), + array.array('H', [1, 4, 0, 3])] + intended_replica2part2dev_id_c = [ + array.array('H', [4, 3, 0, 1]), + array.array('H', [0, 1, 3, 4]), + array.array('H', [3, 4, 0, 1])] + intended_replica2part2dev_id_o = [ + array.array('H', [0, 1, 0, 1]), + array.array('H', [0, 1, 0, 1]), + array.array('H', [3, 4, 3, 4])] + intended_replica2part2dev_id_o_1 = [ + array.array('H', [1, 0, 1, 0]), + array.array('H', [1, 0, 1, 0]), + array.array('H', [4, 3, 4, 3])] + intended_replica2part2dev_id_o_2 = [ + array.array('H', [1, 1, 1, 0]), + array.array('H', [1, 0, 1, 3]), + array.array('H', [4, 2, 4, 3])] + + intended_devs = [{'id': 0, 'zone': 0, 'weight': 1.0, + 'ip': '10.1.1.1', 'port': 6000, + 'device': 'sda1'}, + {'id': 1, 'zone': 0, 'weight': 1.0, + 'ip': '10.1.1.1', 'port': 6000, + 'device': 'sdb1'}, + None, + {'id': 3, 'zone': 2, 'weight': 1.0, + 'ip': '10.1.2.1', 'port': 6000, + 'device': 'sdc1'}, + {'id': 4, 'zone': 2, 'weight': 1.0, + 'ip': '10.1.2.2', 'port': 6000, + 'device': 'sdd1'}] + + # eliminate time from the equation as gzip 2.6 includes + # it in the header resulting in md5 file mismatch, also + # have to mock basename as one version uses it, one doesn't + with mock.patch("time.time", fake_time): + with mock.patch("os.path.basename", fake_base): + ring.RingData(intended_replica2part2dev_id_a, + intended_devs, 5).save(accountgz, mtime=None) + ring.RingData(intended_replica2part2dev_id_c, + intended_devs, 5).save(containergz, mtime=None) + ring.RingData(intended_replica2part2dev_id_o, + intended_devs, 5).save(objectgz, mtime=None) + ring.RingData(intended_replica2part2dev_id_o_1, + intended_devs, 5).save(objectgz_1, mtime=None) + ring.RingData(intended_replica2part2dev_id_o_2, + intended_devs, 5).save(objectgz_2, mtime=None) + + def test_get_ring_md5(self): + def fake_open(self, f): + raise IOError + + expt_out = {'%s/account.ring.gz' % self.tempdir: + 'd288bdf39610e90d4f0b67fa00eeec4f', + '%s/container.ring.gz' % self.tempdir: + '9a5a05a8a4fbbc61123de792dbe4592d', + '%s/object-1.ring.gz' % self.tempdir: + '3f1899b27abf5f2efcc67d6fae1e1c64', + '%s/object-2.ring.gz' % self.tempdir: + '8f0e57079b3c245d9b3d5a428e9312ee', + '%s/object.ring.gz' % self.tempdir: + 'da02bfbd0bf1e7d56faea15b6fe5ab1e'} + + self.assertEquals(sorted(self.app.get_ring_md5().items()), + sorted(expt_out.items())) + + # cover error path + self.app.get_ring_md5(openr=fake_open) def test_from_recon_cache(self): oart = OpenAndReadTester(['{"notneeded": 5, "testkey1": "canhazio"}']) @@ -712,9 +810,15 @@ class TestReconSuccess(TestCase): class TestReconMiddleware(unittest.TestCase): + def fake_list(self, path): + return ['a', 'b'] + def setUp(self): self.frecon = FakeRecon() + self.real_listdir = os.listdir + os.listdir = self.fake_list self.app = recon.ReconMiddleware(FakeApp(), {'object_recon': "true"}) + os.listdir = self.real_listdir #self.app.object_recon = True self.app.get_mem = self.frecon.fake_mem self.app.get_load = self.frecon.fake_load