From 62254e42c40c2a99f79816ce47ec26101780c1d0 Mon Sep 17 00:00:00 2001 From: Florian Hines Date: Wed, 18 Dec 2013 14:03:23 -0600 Subject: [PATCH] Fix checkmount error parsing in swift-recon - swift-recon now handles parsing instances where 'mounted' key (in unmounted and disk_usage) is an error message instead of a bool. - Add's checkmount exception handling to the recon umounted endpoint. - Updates existing unittest to have ismount throw an error. - Updates unittests to cover the corner cases Change-Id: Id51d14a8b98de69faaac84b2b34b7404b7df69e9 --- bin/swift-recon | 28 ++++++++++++++++------- swift/common/middleware/recon.py | 9 +++++--- test/unit/common/middleware/test_recon.py | 28 +++++++++++++++++++---- 3 files changed, 50 insertions(+), 15 deletions(-) diff --git a/bin/swift-recon b/bin/swift-recon index 362b8ecf26..496f96f956 100755 --- a/bin/swift-recon +++ b/bin/swift-recon @@ -242,20 +242,29 @@ class SwiftRecon(object): :param hosts: set of hosts to check. in the format of: set([('127.0.0.1', 6020), ('127.0.0.2', 6030)]) """ - stats = {} + unmounted = {} + errors = {} recon = Scout("unmounted", self.verbose, self.suppress_errors, self.timeout) print "[%s] Getting unmounted drives from %s hosts..." % \ (self._ptime(), len(hosts)) for url, response, status in self.pool.imap(recon.scout, hosts): if status == 200: - stats[url] = [] + unmounted[url] = [] + errors[url] = [] for i in response: - stats[url].append(i['device']) - for host in stats: + if not isinstance(i['mounted'], bool): + errors[url].append(i['device']) + else: + unmounted[url].append(i['device']) + for host in unmounted: node = urlparse(host).netloc - for entry in stats[host]: + for entry in unmounted[host]: print "Not mounted: %s on %s" % (entry, node) + for host in errors: + node = urlparse(host).netloc + for entry in errors[host]: + print "Device errors: %s on %s" % (entry, node) print "=" * 79 def expirer_check(self, hosts): @@ -655,7 +664,10 @@ class SwiftRecon(object): if status == 200: hostusage = [] for entry in response: - if entry['mounted']: + if not isinstance(entry['mounted'], bool): + print "-> %s/%s: Error: %s" % (url, entry['device'], + entry['mounted']) + elif entry['mounted']: used = float(entry['used']) / float(entry['size']) \ * 100.0 raw_total_used.append(entry['used']) @@ -672,7 +684,7 @@ class SwiftRecon(object): for url in stats: if len(stats[url]) > 0: - #get per host hi/los for another day + # get per host hi/los for another day low = min(stats[url]) high = max(stats[url]) highs.append(high) @@ -685,7 +697,7 @@ class SwiftRecon(object): if len(lows) > 0: low = min(lows) high = max(highs) - #dist graph shamelessly stolen from https://github.com/gholt/tcod + # dist graph shamelessly stolen from https://github.com/gholt/tcod print "Distribution Graph:" mul = 69.0 / max(percents.values()) for percent in sorted(percents): diff --git a/swift/common/middleware/recon.py b/swift/common/middleware/recon.py index ffd289f3d1..6212c9f1fd 100644 --- a/swift/common/middleware/recon.py +++ b/swift/common/middleware/recon.py @@ -191,9 +191,12 @@ class ReconMiddleware(object): """list unmounted (failed?) devices""" mountlist = [] for entry in os.listdir(self.devices): - mpoint = {'device': entry, - 'mounted': check_mount(self.devices, entry)} - if not mpoint['mounted']: + try: + mounted = check_mount(self.devices, entry) + except OSError as err: + mounted = str(err) + mpoint = {'device': entry, 'mounted': mounted} + if mpoint['mounted'] is not True: mountlist.append(mpoint) return mountlist diff --git a/test/unit/common/middleware/test_recon.py b/test/unit/common/middleware/test_recon.py index 64afe4e085..6d690ca6fb 100644 --- a/test/unit/common/middleware/test_recon.py +++ b/test/unit/common/middleware/test_recon.py @@ -101,7 +101,10 @@ class MockOS(object): def fake_ismount(self, *args, **kwargs): self.ismount_calls.append((args, kwargs)) - return self.ismount_output + if isinstance(self.ismount_output, Exception): + raise self.ismount_output + else: + return self.ismount_output def fake_statvfs(self, *args, **kwargs): self.statvfs_calls.append((args, kwargs)) @@ -560,7 +563,6 @@ class TestReconSuccess(TestCase): "quarantined": 0}}) def test_get_unmounted(self): - unmounted_resp = [{'device': 'fakeone', 'mounted': False}, {'device': 'faketwo', 'mounted': False}] self.mockos.ls_output = ['fakeone', 'faketwo'] @@ -569,6 +571,24 @@ class TestReconSuccess(TestCase): self.assertEquals(self.mockos.listdir_calls, [(('/srv/node/',), {})]) self.assertEquals(rv, unmounted_resp) + def test_get_unmounted_everything_normal(self): + unmounted_resp = [] + self.mockos.ls_output = ['fakeone', 'faketwo'] + self.mockos.ismount_output = True + rv = self.app.get_unmounted() + self.assertEquals(self.mockos.listdir_calls, [(('/srv/node/',), {})]) + self.assertEquals(rv, unmounted_resp) + + def test_get_unmounted_checkmount_fail(self): + unmounted_resp = [{'device': 'fakeone', 'mounted': 'brokendrive'}] + self.mockos.ls_output = ['fakeone'] + self.mockos.ismount_output = OSError('brokendrive') + rv = self.app.get_unmounted() + self.assertEquals(self.mockos.listdir_calls, [(('/srv/node/',), {})]) + self.assertEquals(self.mockos.ismount_calls, + [(('/srv/node/fakeone',), {})]) + self.assertEquals(rv, unmounted_resp) + def test_no_get_unmounted(self): def fake_checkmount_true(*args): @@ -601,9 +621,9 @@ class TestReconSuccess(TestCase): def test_get_diskusage_checkmount_fail(self): du_resp = [{'device': 'canhazdrive1', 'avail': '', - 'mounted': False, 'used': '', 'size': ''}] + 'mounted': 'brokendrive', 'used': '', 'size': ''}] self.mockos.ls_output = ['canhazdrive1'] - self.mockos.ismount_output = False + self.mockos.ismount_output = OSError('brokendrive') rv = self.app.get_diskusage() self.assertEquals(self.mockos.listdir_calls, [(('/srv/node/',), {})]) self.assertEquals(self.mockos.ismount_calls,