From 2659888c921d09bd1dd23cda6ee2f158187d80e6 Mon Sep 17 00:00:00 2001 From: Matthew Oliver Date: Fri, 13 Jun 2014 19:12:31 +1000 Subject: [PATCH] When a filesystem does't support xattr return a 507 Currently when the object server tries to write an object's metadata to a filesystem that doesn't support xattr, it errors with a stacktrace and returns a 500 error back to the user with no information. This patch catches the resulting IOError when attempting to read or write the xattr metadata, logs the error nicely and then returns a 507 error back to the user. Seeing as this change is sending back a 507, it also catches and logs the out of disk space errors (ENOSPC and EDQUOT). Change-Id: I31932b57582817a0b3b58dd315a996bd0bcbc99b Closes-Bug: #966671 --- swift/common/exceptions.py | 4 +++ swift/obj/diskfile.py | 52 ++++++++++++++++++++++++++++------ swift/obj/server.py | 20 +++++++++++-- test/unit/obj/test_diskfile.py | 47 +++++++++++++++++++++++++++++- test/unit/obj/test_server.py | 22 ++++++++++++++ 5 files changed, 133 insertions(+), 12 deletions(-) diff --git a/swift/common/exceptions.py b/swift/common/exceptions.py index e7999bab97..55469073e8 100644 --- a/swift/common/exceptions.py +++ b/swift/common/exceptions.py @@ -75,6 +75,10 @@ class DiskFileDeviceUnavailable(DiskFileError): pass +class DiskFileXattrNotSupported(DiskFileError): + pass + + class DeviceUnavailable(SwiftException): pass diff --git a/swift/obj/diskfile.py b/swift/obj/diskfile.py index be3cde7416..857d8ce7ff 100644 --- a/swift/obj/diskfile.py +++ b/swift/obj/diskfile.py @@ -38,13 +38,13 @@ import uuid import hashlib import logging import traceback +import xattr from os.path import basename, dirname, exists, getmtime, join from random import shuffle from tempfile import mkstemp from contextlib import contextmanager from collections import defaultdict -from xattr import getxattr, setxattr from eventlet import Timeout from swift import gettext_ as _ @@ -56,7 +56,7 @@ from swift.common.utils import mkdirs, Timestamp, \ from swift.common.exceptions import DiskFileQuarantined, DiskFileNotExist, \ DiskFileCollision, DiskFileNoSpace, DiskFileDeviceUnavailable, \ DiskFileDeleted, DiskFileError, DiskFileNotOpen, PathNotDir, \ - ReplicationLockTimeout, DiskFileExpired + ReplicationLockTimeout, DiskFileExpired, DiskFileXattrNotSupported from swift.common.swob import multi_range_iterator from swift.common.storage_policy import get_policy_string, POLICIES from functools import partial @@ -76,6 +76,22 @@ get_async_dir = partial(get_policy_string, ASYNCDIR_BASE) get_tmp_dir = partial(get_policy_string, TMP_BASE) +def _get_filename(fd): + """ + Helper function to get to file name from a file descriptor or filename. + + :param fd: file descriptor or filename. + + :returns: the filename. + """ + if hasattr(fd, 'name'): + # fd object + return fd.name + + # fd is a filename + return fd + + def read_metadata(fd): """ Helper function to read the pickled metadata from an object file. @@ -88,10 +104,16 @@ def read_metadata(fd): key = 0 try: while True: - metadata += getxattr(fd, '%s%s' % (METADATA_KEY, (key or ''))) + metadata += xattr.getxattr(fd, '%s%s' % (METADATA_KEY, + (key or ''))) key += 1 - except IOError: - pass + except IOError as e: + for err in 'ENOTSUP', 'EOPNOTSUPP': + if hasattr(errno, err) and e.errno == getattr(errno, err): + msg = "Filesystem at %s does not support xattr" % \ + _get_filename(fd) + logging.exception(msg) + raise DiskFileXattrNotSupported(e) return pickle.loads(metadata) @@ -105,9 +127,23 @@ def write_metadata(fd, metadata): metastr = pickle.dumps(metadata, PICKLE_PROTOCOL) key = 0 while metastr: - setxattr(fd, '%s%s' % (METADATA_KEY, key or ''), metastr[:254]) - metastr = metastr[254:] - key += 1 + try: + xattr.setxattr(fd, '%s%s' % (METADATA_KEY, key or ''), + metastr[:254]) + metastr = metastr[254:] + key += 1 + except IOError as e: + for err in 'ENOTSUP', 'EOPNOTSUPP': + if hasattr(errno, err) and e.errno == getattr(errno, err): + msg = "Filesystem at %s does not support xattr" % \ + _get_filename(fd) + logging.exception(msg) + raise DiskFileXattrNotSupported(e) + if e.errno in (errno.ENOSPC, errno.EDQUOT): + msg = "No space left on device for %s" % _get_filename(fd) + logging.exception(msg) + raise DiskFileNoSpace() + raise def extract_policy_index(obj_path): diff --git a/swift/obj/server.py b/swift/obj/server.py index 4a3b9926ff..51a4a5c107 100644 --- a/swift/obj/server.py +++ b/swift/obj/server.py @@ -35,7 +35,8 @@ from swift.common.constraints import check_object_creation, \ valid_timestamp, check_utf8 from swift.common.exceptions import ConnectionTimeout, DiskFileQuarantined, \ DiskFileNotExist, DiskFileCollision, DiskFileNoSpace, DiskFileDeleted, \ - DiskFileDeviceUnavailable, DiskFileExpired, ChunkReadTimeout + DiskFileDeviceUnavailable, DiskFileExpired, ChunkReadTimeout, \ + DiskFileXattrNotSupported from swift.obj import ssync_receiver from swift.common.http import is_success from swift.common.request_helpers import get_name_and_placement, is_user_meta @@ -338,6 +339,8 @@ class ObjectController(object): return HTTPInsufficientStorage(drive=device, request=request) try: orig_metadata = disk_file.read_metadata() + except DiskFileXattrNotSupported: + return HTTPInsufficientStorage(drive=device, request=request) except (DiskFileNotExist, DiskFileQuarantined): return HTTPNotFound(request=request) orig_timestamp = Timestamp(orig_metadata.get('X-Timestamp', 0)) @@ -359,7 +362,10 @@ class ObjectController(object): self.delete_at_update('DELETE', orig_delete_at, account, container, obj, request, device, policy_idx) - disk_file.write_metadata(metadata) + try: + disk_file.write_metadata(metadata) + except (DiskFileXattrNotSupported, DiskFileNoSpace): + return HTTPInsufficientStorage(drive=device, request=request) return HTTPAccepted(request=request) @public @@ -389,6 +395,8 @@ class ObjectController(object): return HTTPInsufficientStorage(drive=device, request=request) try: orig_metadata = disk_file.read_metadata() + except DiskFileXattrNotSupported: + return HTTPInsufficientStorage(drive=device, request=request) except (DiskFileNotExist, DiskFileQuarantined): orig_metadata = {} @@ -453,7 +461,7 @@ class ObjectController(object): header_caps = header_key.title() metadata[header_caps] = request.headers[header_key] writer.put(metadata) - except DiskFileNoSpace: + except (DiskFileXattrNotSupported, DiskFileNoSpace): return HTTPInsufficientStorage(drive=device, request=request) if orig_delete_at != new_delete_at: if new_delete_at: @@ -517,6 +525,8 @@ class ObjectController(object): response.headers['X-Timestamp'] = file_x_ts.normal response.headers['X-Backend-Timestamp'] = file_x_ts.internal resp = request.get_response(response) + except DiskFileXattrNotSupported: + return HTTPInsufficientStorage(drive=device, request=request) except (DiskFileNotExist, DiskFileQuarantined) as e: headers = {} if hasattr(e, 'timestamp'): @@ -539,6 +549,8 @@ class ObjectController(object): return HTTPInsufficientStorage(drive=device, request=request) try: metadata = disk_file.read_metadata() + except DiskFileXattrNotSupported: + return HTTPInsufficientStorage(drive=device, request=request) except (DiskFileNotExist, DiskFileQuarantined) as e: headers = {} if hasattr(e, 'timestamp'): @@ -580,6 +592,8 @@ class ObjectController(object): return HTTPInsufficientStorage(drive=device, request=request) try: orig_metadata = disk_file.read_metadata() + except DiskFileXattrNotSupported: + return HTTPInsufficientStorage(drive=device, request=request) except DiskFileExpired as e: orig_timestamp = e.timestamp orig_metadata = e.metadata diff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py index 43bd85d55b..237f57bb33 100644 --- a/test/unit/obj/test_diskfile.py +++ b/test/unit/obj/test_diskfile.py @@ -43,7 +43,7 @@ from swift.common import ring from swift.common.exceptions import DiskFileNotExist, DiskFileQuarantined, \ DiskFileDeviceUnavailable, DiskFileDeleted, DiskFileNotOpen, \ DiskFileError, ReplicationLockTimeout, PathNotDir, DiskFileCollision, \ - DiskFileExpired, SwiftException, DiskFileNoSpace + DiskFileExpired, SwiftException, DiskFileNoSpace, DiskFileXattrNotSupported from swift.common.storage_policy import POLICIES, get_policy_string from functools import partial @@ -1046,6 +1046,17 @@ class TestDiskFile(unittest.TestCase): md = df.read_metadata() self.assertEqual(md['X-Timestamp'], Timestamp(42).internal) + def test_read_metadata_no_xattr(self): + def mock_getxattr(*args, **kargs): + error_num = errno.ENOTSUP if hasattr(errno, 'ENOTSUP') else \ + errno.EOPNOTSUPP + raise IOError(error_num, "Operation not supported") + + with mock.patch('xattr.getxattr', mock_getxattr): + self.assertRaises( + DiskFileXattrNotSupported, + diskfile.read_metadata, 'n/a') + def test_get_metadata_not_opened(self): df = self._simple_get_diskfile() self.assertRaises(DiskFileNotOpen, df.get_metadata) @@ -1586,6 +1597,40 @@ class TestDiskFile(unittest.TestCase): exp_name = '%s.meta' % timestamp self.assertTrue(exp_name in set(dl)) + def test_write_metadata_no_xattr(self): + timestamp = Timestamp(time()).internal + metadata = {'X-Timestamp': timestamp, 'X-Object-Meta-test': 'data'} + + def mock_setxattr(*args, **kargs): + error_num = errno.ENOTSUP if hasattr(errno, 'ENOTSUP') else \ + errno.EOPNOTSUPP + raise IOError(error_num, "Operation not supported") + + with mock.patch('xattr.setxattr', mock_setxattr): + self.assertRaises( + DiskFileXattrNotSupported, + diskfile.write_metadata, 'n/a', metadata) + + def test_write_metadata_disk_full(self): + timestamp = Timestamp(time()).internal + metadata = {'X-Timestamp': timestamp, 'X-Object-Meta-test': 'data'} + + def mock_setxattr_ENOSPC(*args, **kargs): + raise IOError(errno.ENOSPC, "No space left on device") + + def mock_setxattr_EDQUOT(*args, **kargs): + raise IOError(errno.EDQUOT, "Exceeded quota") + + with mock.patch('xattr.setxattr', mock_setxattr_ENOSPC): + self.assertRaises( + DiskFileNoSpace, + diskfile.write_metadata, 'n/a', metadata) + + with mock.patch('xattr.setxattr', mock_setxattr_EDQUOT): + self.assertRaises( + DiskFileNoSpace, + diskfile.write_metadata, 'n/a', metadata) + def test_delete(self): df = self._get_open_disk_file() ts = time() diff --git a/test/unit/obj/test_server.py b/test/unit/obj/test_server.py index 67c24dd743..35ed17c1c9 100755 --- a/test/unit/obj/test_server.py +++ b/test/unit/obj/test_server.py @@ -18,6 +18,7 @@ import cPickle as pickle import datetime +import errno import operator import os import mock @@ -708,6 +709,27 @@ class TestObjectController(unittest.TestCase): 'X-Object-Meta-1': 'One', 'X-Object-Meta-Two': 'Two'}) + def test_PUT_user_metadata_no_xattr(self): + timestamp = normalize_timestamp(time()) + req = Request.blank( + '/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'}, + headers={'X-Timestamp': timestamp, + 'Content-Type': 'text/plain', + 'ETag': 'b114ab7b90d9ccac4bd5d99cc7ebb568', + 'X-Object-Meta-1': 'One', + 'X-Object-Meta-Two': 'Two'}) + req.body = 'VERIFY THREE' + + def mock_get_and_setxattr(*args, **kargs): + error_num = errno.ENOTSUP if hasattr(errno, 'ENOTSUP') else \ + errno.EOPNOTSUPP + raise IOError(error_num, 'Operation not supported') + + with mock.patch('xattr.getxattr', mock_get_and_setxattr): + with mock.patch('xattr.setxattr', mock_get_and_setxattr): + resp = req.get_response(self.object_controller) + self.assertEquals(resp.status_int, 507) + def test_PUT_client_timeout(self): class FakeTimeout(BaseException): def __enter__(self):