From 163fb4d52a85e0467c8c6b616e2cd9faa1faa41b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20Kvasni=C4=8Dka?= Date: Wed, 19 Apr 2017 15:09:40 +0200 Subject: [PATCH] Always require device dir for containers For test purposes (e.g. saio probetests) even if mount_check is False, still require check_dir for account/container server storage when real mount points are not used. This behavior is consistent with the object-server's checks in diskfile. Co-Author: Clay Gerrard Related lp bug #1693005 Related-Change-Id: I344f9daaa038c6946be11e1cf8c4ef104a09e68b Depends-On: I52c4ecb70b1ae47e613ba243da5a4d94e5adedf2 Change-Id: I3362a6ebff423016bb367b4b6b322bb41ae08764 --- swift/account/server.py | 14 +-- swift/common/constraints.py | 30 +++++-- swift/common/middleware/recon.py | 4 +- swift/container/server.py | 14 +-- swift/obj/diskfile.py | 11 ++- test/unit/__init__.py | 57 ++++--------- test/unit/account/test_server.py | 98 ++++++++++----------- test/unit/common/test_constraints.py | 58 +++++++++---- test/unit/container/test_server.py | 122 +++++++++++++-------------- test/unit/obj/test_auditor.py | 4 +- test/unit/obj/test_diskfile.py | 26 +++--- test/unit/obj/test_reconstructor.py | 41 +++++---- test/unit/obj/test_server.py | 80 ++++++++++++++---- test/unit/obj/test_ssync_receiver.py | 23 +++-- 14 files changed, 319 insertions(+), 263 deletions(-) diff --git a/swift/account/server.py b/swift/account/server.py index 5334b97c1c..c67ac5d97d 100644 --- a/swift/account/server.py +++ b/swift/account/server.py @@ -29,7 +29,7 @@ from swift.common.request_helpers import get_param, get_listing_content_type, \ from swift.common.utils import get_logger, hash_path, public, \ Timestamp, storage_directory, config_true_value, \ json, timing_stats, replication, get_log_line -from swift.common.constraints import check_mount, valid_timestamp, check_utf8 +from swift.common.constraints import valid_timestamp, check_utf8, check_drive from swift.common import constraints from swift.common.db_replicator import ReplicatorRpc from swift.common.base_storage_server import BaseStorageServer @@ -87,7 +87,7 @@ class AccountController(BaseStorageServer): def DELETE(self, req): """Handle HTTP DELETE request.""" drive, part, account = split_and_validate_path(req, 3) - if self.mount_check and not check_mount(self.root, drive): + if not check_drive(self.root, drive, self.mount_check): return HTTPInsufficientStorage(drive=drive, request=req) req_timestamp = valid_timestamp(req) broker = self._get_account_broker(drive, part, account) @@ -101,7 +101,7 @@ class AccountController(BaseStorageServer): def PUT(self, req): """Handle HTTP PUT request.""" drive, part, account, container = split_and_validate_path(req, 3, 4) - if self.mount_check and not check_mount(self.root, drive): + if not check_drive(self.root, drive, self.mount_check): return HTTPInsufficientStorage(drive=drive, request=req) if container: # put account container if 'x-timestamp' not in req.headers: @@ -168,7 +168,7 @@ class AccountController(BaseStorageServer): """Handle HTTP HEAD request.""" drive, part, account = split_and_validate_path(req, 3) out_content_type = get_listing_content_type(req) - if self.mount_check and not check_mount(self.root, drive): + if not check_drive(self.root, drive, self.mount_check): return HTTPInsufficientStorage(drive=drive, request=req) broker = self._get_account_broker(drive, part, account, pending_timeout=0.1, @@ -203,7 +203,7 @@ class AccountController(BaseStorageServer): end_marker = get_param(req, 'end_marker') out_content_type = get_listing_content_type(req) - if self.mount_check and not check_mount(self.root, drive): + if not check_drive(self.root, drive, self.mount_check): return HTTPInsufficientStorage(drive=drive, request=req) broker = self._get_account_broker(drive, part, account, pending_timeout=0.1, @@ -224,7 +224,7 @@ class AccountController(BaseStorageServer): """ post_args = split_and_validate_path(req, 3) drive, partition, hash = post_args - if self.mount_check and not check_mount(self.root, drive): + if not check_drive(self.root, drive, self.mount_check): return HTTPInsufficientStorage(drive=drive, request=req) try: args = json.load(req.environ['wsgi.input']) @@ -240,7 +240,7 @@ class AccountController(BaseStorageServer): """Handle HTTP POST request.""" drive, part, account = split_and_validate_path(req, 3) req_timestamp = valid_timestamp(req) - if self.mount_check and not check_mount(self.root, drive): + if not check_drive(self.root, drive, self.mount_check): return HTTPInsufficientStorage(drive=drive, request=req) broker = self._get_account_broker(drive, part, account) if broker.is_deleted(): diff --git a/swift/common/constraints.py b/swift/common/constraints.py index 679926356a..e0a0851fae 100644 --- a/swift/common/constraints.py +++ b/swift/common/constraints.py @@ -15,6 +15,7 @@ import functools import os +from os.path import isdir # tighter scoped import for mocking import time import six @@ -234,9 +235,9 @@ def check_dir(root, drive): :param root: base path where the dir is :param drive: drive name to be checked - :returns: True if it is a valid directoy, False otherwise + :returns: full path to the device, or None if drive fails to validate """ - return os.path.isdir(os.path.join(root, drive)) + return check_drive(root, drive, False) def check_mount(root, drive): @@ -248,12 +249,31 @@ def check_mount(root, drive): :param root: base path where the devices are mounted :param drive: drive name to be checked - :returns: True if it is a valid mounted device, False otherwise + :returns: full path to the device, or None if drive fails to validate + """ + return check_drive(root, drive, True) + + +def check_drive(root, drive, mount_check): + """ + Validate the path given by root and drive is a valid existing directory. + + :param root: base path where the devices are mounted + :param drive: drive name to be checked + :param mount_check: additionally require path is mounted + + :returns: full path to the device, or None if drive fails to validate """ if not (urllib.parse.quote_plus(drive) == drive): - return False + return None path = os.path.join(root, drive) - return utils.ismount(path) + if mount_check: + if utils.ismount(path): + return path + else: + if isdir(path): + return path + return None def check_float(string): diff --git a/swift/common/middleware/recon.py b/swift/common/middleware/recon.py index c9c994fe72..1ad36244b2 100644 --- a/swift/common/middleware/recon.py +++ b/swift/common/middleware/recon.py @@ -209,7 +209,7 @@ class ReconMiddleware(object): continue try: - mounted = check_mount(self.devices, entry) + mounted = bool(check_mount(self.devices, entry)) except OSError as err: mounted = str(err) mpoint = {'device': entry, 'mounted': mounted} @@ -225,7 +225,7 @@ class ReconMiddleware(object): continue try: - mounted = check_mount(self.devices, entry) + mounted = bool(check_mount(self.devices, entry)) except OSError as err: devices.append({'device': entry, 'mounted': str(err), 'size': '', 'used': '', 'avail': ''}) diff --git a/swift/container/server.py b/swift/container/server.py index c71925f1b1..0c58089c10 100644 --- a/swift/container/server.py +++ b/swift/container/server.py @@ -35,7 +35,7 @@ from swift.common.utils import get_logger, hash_path, public, \ Timestamp, storage_directory, validate_sync_to, \ config_true_value, timing_stats, replication, \ override_bytes_from_content_type, get_log_line -from swift.common.constraints import check_mount, valid_timestamp, check_utf8 +from swift.common.constraints import valid_timestamp, check_utf8, check_drive from swift.common import constraints from swift.common.bufferedhttp import http_connect from swift.common.exceptions import ConnectionTimeout @@ -263,7 +263,7 @@ class ContainerController(BaseStorageServer): drive, part, account, container, obj = split_and_validate_path( req, 4, 5, True) req_timestamp = valid_timestamp(req) - if self.mount_check and not check_mount(self.root, drive): + if not check_drive(self.root, drive, self.mount_check): return HTTPInsufficientStorage(drive=drive, request=req) # policy index is only relevant for delete_obj (and transitively for # auto create accounts) @@ -351,7 +351,7 @@ class ContainerController(BaseStorageServer): self.realms_conf) if err: return HTTPBadRequest(err) - if self.mount_check and not check_mount(self.root, drive): + if not check_drive(self.root, drive, self.mount_check): return HTTPInsufficientStorage(drive=drive, request=req) requested_policy_index = self.get_and_validate_policy_index(req) broker = self._get_container_broker(drive, part, account, container) @@ -419,7 +419,7 @@ class ContainerController(BaseStorageServer): drive, part, account, container, obj = split_and_validate_path( req, 4, 5, True) out_content_type = get_listing_content_type(req) - if self.mount_check and not check_mount(self.root, drive): + if not check_drive(self.root, drive, self.mount_check): return HTTPInsufficientStorage(drive=drive, request=req) broker = self._get_container_broker(drive, part, account, container, pending_timeout=0.1, @@ -483,7 +483,7 @@ class ContainerController(BaseStorageServer): body='Maximum limit is %d' % constraints.CONTAINER_LISTING_LIMIT) out_content_type = get_listing_content_type(req) - if self.mount_check and not check_mount(self.root, drive): + if not check_drive(self.root, drive, self.mount_check): return HTTPInsufficientStorage(drive=drive, request=req) broker = self._get_container_broker(drive, part, account, container, pending_timeout=0.1, @@ -545,7 +545,7 @@ class ContainerController(BaseStorageServer): """ post_args = split_and_validate_path(req, 3) drive, partition, hash = post_args - if self.mount_check and not check_mount(self.root, drive): + if not check_drive(self.root, drive, self.mount_check): return HTTPInsufficientStorage(drive=drive, request=req) try: args = json.load(req.environ['wsgi.input']) @@ -567,7 +567,7 @@ class ContainerController(BaseStorageServer): self.realms_conf) if err: return HTTPBadRequest(err) - if self.mount_check and not check_mount(self.root, drive): + if not check_drive(self.root, drive, self.mount_check): return HTTPInsufficientStorage(drive=drive, request=req) broker = self._get_container_broker(drive, part, account, container) if broker.is_deleted(): diff --git a/swift/obj/diskfile.py b/swift/obj/diskfile.py index 362b3a3f34..42d5eacc47 100644 --- a/swift/obj/diskfile.py +++ b/swift/obj/diskfile.py @@ -57,7 +57,7 @@ from pyeclib.ec_iface import ECDriverError, ECInvalidFragmentMetadata, \ ECBadFragmentChecksum, ECInvalidParameter from swift import gettext_ as _ -from swift.common.constraints import check_mount, check_dir +from swift.common.constraints import check_drive from swift.common.request_helpers import is_sys_meta from swift.common.utils import mkdirs, Timestamp, \ storage_directory, hash_path, renamer, fallocate, fsync, fdatasync, \ @@ -1191,12 +1191,11 @@ class BaseDiskFileManager(object): # we'll do some kind of check unless explicitly forbidden if mount_check is not False: if mount_check or self.mount_check: - check = check_mount + mount_check = True else: - check = check_dir - if not check(self.devices, device): - return None - return os.path.join(self.devices, device) + mount_check = False + return check_drive(self.devices, device, mount_check) + return join(self.devices, device) @contextmanager def replication_lock(self, device): diff --git a/test/unit/__init__.py b/test/unit/__init__.py index d9750b7f8b..7f7726d25b 100644 --- a/test/unit/__init__.py +++ b/test/unit/__init__.py @@ -709,49 +709,28 @@ def quiet_eventlet_exceptions(): eventlet_debug.hub_exceptions(orig_state) -class MockTrue(object): +@contextmanager +def mock_check_drive(isdir=False, ismount=False): """ - Instances of MockTrue evaluate like True - Any attr accessed on an instance of MockTrue will return a MockTrue - instance. Any method called on an instance of MockTrue will return - a MockTrue instance. + All device/drive/mount checking should be done through the constraints + module if we keep the mocking consistly w/i that module we can keep our + test robust to further rework on that interface. - >>> thing = MockTrue() - >>> thing - True - >>> thing == True # True == True - True - >>> thing == False # True == False - False - >>> thing != True # True != True - False - >>> thing != False # True != False - True - >>> thing.attribute - True - >>> thing.method() - True - >>> thing.attribute.method() - True - >>> thing.method().attribute - True + Replace the constraint modules underlying os calls with mocks. + :param isdir: return value of constraints isdir calls, default False + :param ismount: return value of constraints ismount calls, default False + :returns: a dict of constraint module mocks """ - - def __getattribute__(self, *args, **kwargs): - return self - - def __call__(self, *args, **kwargs): - return self - - def __repr__(*args, **kwargs): - return repr(True) - - def __eq__(self, other): - return other is True - - def __ne__(self, other): - return other is not True + mock_base = 'swift.common.constraints.' + with mocklib.patch(mock_base + 'isdir') as mock_isdir, \ + mocklib.patch(mock_base + 'utils.ismount') as mock_ismount: + mock_isdir.return_value = isdir + mock_ismount.return_value = ismount + yield { + 'isdir': mock_isdir, + 'ismount': mock_ismount, + } @contextmanager diff --git a/test/unit/account/test_server.py b/test/unit/account/test_server.py index da1712c210..cef4efc345 100644 --- a/test/unit/account/test_server.py +++ b/test/unit/account/test_server.py @@ -36,7 +36,7 @@ from swift.account.server import AccountController from swift.common.utils import (normalize_timestamp, replication, public, mkdirs, storage_directory, Timestamp) from swift.common.request_helpers import get_sys_meta_prefix -from test.unit import patch_policies, debug_logger +from test.unit import patch_policies, debug_logger, mock_check_drive from swift.common.storage_policy import StoragePolicy, POLICIES @@ -47,6 +47,7 @@ class TestAccountController(unittest.TestCase): """Set up for testing swift.account.server.AccountController""" self.testdir_base = mkdtemp() self.testdir = os.path.join(self.testdir_base, 'account_server') + mkdirs(os.path.join(self.testdir, 'sda1')) self.controller = AccountController( {'devices': self.testdir, 'mount_check': 'false'}) @@ -71,6 +72,50 @@ class TestAccountController(unittest.TestCase): self.assertEqual(resp.headers['Server'], (server_handler.server_type + '/' + swift_version)) + def test_insufficient_storage_mount_check_true(self): + conf = {'devices': self.testdir, 'mount_check': 'true'} + account_controller = AccountController(conf) + self.assertTrue(account_controller.mount_check) + for method in account_controller.allowed_methods: + if method == 'OPTIONS': + continue + req = Request.blank('/sda1/p/a-or-suff', method=method, + headers={'x-timestamp': '1'}) + with mock_check_drive() as mocks: + try: + resp = req.get_response(account_controller) + self.assertEqual(resp.status_int, 507) + mocks['ismount'].return_value = True + resp = req.get_response(account_controller) + self.assertNotEqual(resp.status_int, 507) + # feel free to rip out this last assertion... + expected = 2 if method == 'PUT' else 4 + self.assertEqual(resp.status_int // 100, expected) + except AssertionError as e: + self.fail('%s for %s' % (e, method)) + + def test_insufficient_storage_mount_check_false(self): + conf = {'devices': self.testdir, 'mount_check': 'false'} + account_controller = AccountController(conf) + self.assertFalse(account_controller.mount_check) + for method in account_controller.allowed_methods: + if method == 'OPTIONS': + continue + req = Request.blank('/sda1/p/a-or-suff', method=method, + headers={'x-timestamp': '1'}) + with mock_check_drive() as mocks: + try: + resp = req.get_response(account_controller) + self.assertEqual(resp.status_int, 507) + mocks['isdir'].return_value = True + resp = req.get_response(account_controller) + self.assertNotEqual(resp.status_int, 507) + # feel free to rip out this last assertion... + expected = 2 if method == 'PUT' else 4 + self.assertEqual(resp.status_int // 100, expected) + except AssertionError as e: + self.fail('%s for %s' % (e, method)) + def test_DELETE_not_found(self): req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'DELETE', 'HTTP_X_TIMESTAMP': '0'}) @@ -147,29 +192,6 @@ class TestAccountController(unittest.TestCase): resp = req.get_response(self.controller) self.assertEqual(resp.status_int, 400) - def test_DELETE_insufficient_storage(self): - self.controller = AccountController({'devices': self.testdir}) - req = Request.blank( - '/sda-null/p/a', environ={'REQUEST_METHOD': 'DELETE', - 'HTTP_X_TIMESTAMP': '1'}) - resp = req.get_response(self.controller) - self.assertEqual(resp.status_int, 507) - - def test_REPLICATE_insufficient_storage(self): - conf = {'devices': self.testdir, 'mount_check': 'true'} - self.account_controller = AccountController(conf) - - def fake_check_mount(*args, **kwargs): - return False - - with mock.patch("swift.common.constraints.check_mount", - fake_check_mount): - req = Request.blank('/sda1/p/suff', - environ={'REQUEST_METHOD': 'REPLICATE'}, - headers={}) - resp = req.get_response(self.account_controller) - self.assertEqual(resp.status_int, 507) - def test_REPLICATE_rsync_then_merge_works(self): def fake_rsync_then_merge(self, drive, db_file, args): return HTTPNoContent() @@ -331,13 +353,6 @@ class TestAccountController(unittest.TestCase): resp = req.get_response(self.controller) self.assertEqual(resp.status_int, 406) - def test_HEAD_insufficient_storage(self): - self.controller = AccountController({'devices': self.testdir}) - req = Request.blank('/sda-null/p/a', environ={'REQUEST_METHOD': 'HEAD', - 'HTTP_X_TIMESTAMP': '1'}) - resp = req.get_response(self.controller) - self.assertEqual(resp.status_int, 507) - def test_HEAD_invalid_format(self): format = '%D1%BD%8A9' # invalid UTF-8; should be %E1%BD%8A9 (E -> D) req = Request.blank('/sda1/p/a?format=' + format, @@ -569,13 +584,6 @@ class TestAccountController(unittest.TestCase): resp = req.get_response(self.controller) self.assertEqual(resp.status_int, 400) - def test_PUT_insufficient_storage(self): - self.controller = AccountController({'devices': self.testdir}) - req = Request.blank('/sda-null/p/a', environ={'REQUEST_METHOD': 'PUT', - 'HTTP_X_TIMESTAMP': '1'}) - resp = req.get_response(self.controller) - self.assertEqual(resp.status_int, 507) - def test_POST_HEAD_metadata(self): req = Request.blank( '/sda1/p/a', environ={'REQUEST_METHOD': 'PUT'}, @@ -693,13 +701,6 @@ class TestAccountController(unittest.TestCase): resp = req.get_response(self.controller) self.assertEqual(resp.status_int, 400) - def test_POST_insufficient_storage(self): - self.controller = AccountController({'devices': self.testdir}) - req = Request.blank('/sda-null/p/a', environ={'REQUEST_METHOD': 'POST', - 'HTTP_X_TIMESTAMP': '1'}) - resp = req.get_response(self.controller) - self.assertEqual(resp.status_int, 507) - def test_POST_after_DELETE_not_found(self): req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'PUT', 'HTTP_X_TIMESTAMP': '0'}) @@ -1502,13 +1503,6 @@ class TestAccountController(unittest.TestCase): listing.append(node2.firstChild.nodeValue) self.assertEqual(listing, ['sub.1.0', 'sub.1.1', 'sub.1.2']) - def test_GET_insufficient_storage(self): - self.controller = AccountController({'devices': self.testdir}) - req = Request.blank('/sda-null/p/a', environ={'REQUEST_METHOD': 'GET', - 'HTTP_X_TIMESTAMP': '1'}) - resp = req.get_response(self.controller) - self.assertEqual(resp.status_int, 507) - def test_through_call(self): inbuf = BytesIO() errbuf = StringIO() diff --git a/test/unit/common/test_constraints.py b/test/unit/common/test_constraints.py index a15045313e..c975a135c3 100644 --- a/test/unit/common/test_constraints.py +++ b/test/unit/common/test_constraints.py @@ -20,7 +20,7 @@ import time from six.moves import range from test import safe_repr -from test.unit import MockTrue +from test.unit import mock_check_drive from swift.common.swob import Request, HTTPException from swift.common.http import HTTP_REQUEST_ENTITY_TOO_LARGE, \ @@ -372,21 +372,49 @@ class TestConstraints(unittest.TestCase): self.assertTrue('X-Delete-At' in req.headers) self.assertEqual(req.headers['X-Delete-At'], expected) - def test_check_dir(self): - self.assertFalse(constraints.check_dir('', '')) - with mock.patch("os.path.isdir", MockTrue()): - self.assertTrue(constraints.check_dir('/srv', 'foo/bar')) + def test_check_drive_invalid_path(self): + root = '/srv/' + with mock_check_drive() as mocks: + self.assertIsNone(constraints.check_dir(root, 'foo?bar')) + self.assertIsNone(constraints.check_mount(root, 'foo bar')) + self.assertIsNone(constraints.check_drive(root, 'foo/bar', True)) + self.assertIsNone(constraints.check_drive(root, 'foo%bar', False)) + self.assertEqual([], mocks['isdir'].call_args_list) + self.assertEqual([], mocks['ismount'].call_args_list) - def test_check_mount(self): - self.assertFalse(constraints.check_mount('', '')) - with mock.patch("swift.common.utils.ismount", MockTrue()): - self.assertTrue(constraints.check_mount('/srv', '1')) - self.assertTrue(constraints.check_mount('/srv', 'foo-bar')) - self.assertTrue(constraints.check_mount( - '/srv', '003ed03c-242a-4b2f-bee9-395f801d1699')) - self.assertFalse(constraints.check_mount('/srv', 'foo bar')) - self.assertFalse(constraints.check_mount('/srv', 'foo/bar')) - self.assertFalse(constraints.check_mount('/srv', 'foo?bar')) + def test_check_drive_ismount(self): + root = '/srv' + path = 'sdb1' + with mock_check_drive(ismount=True) as mocks: + self.assertIsNone(constraints.check_dir(root, path)) + self.assertIsNone(constraints.check_drive(root, path, False)) + self.assertEqual([mock.call('/srv/sdb1'), mock.call('/srv/sdb1')], + mocks['isdir'].call_args_list) + self.assertEqual([], mocks['ismount'].call_args_list) + with mock_check_drive(ismount=True) as mocks: + self.assertEqual('/srv/sdb1', constraints.check_mount(root, path)) + self.assertEqual('/srv/sdb1', constraints.check_drive( + root, path, True)) + self.assertEqual([], mocks['isdir'].call_args_list) + self.assertEqual([mock.call('/srv/sdb1'), mock.call('/srv/sdb1')], + mocks['ismount'].call_args_list) + + def test_check_drive_isdir(self): + root = '/srv' + path = 'sdb2' + with mock_check_drive(isdir=True) as mocks: + self.assertEqual('/srv/sdb2', constraints.check_dir(root, path)) + self.assertEqual('/srv/sdb2', constraints.check_drive( + root, path, False)) + self.assertEqual([mock.call('/srv/sdb2'), mock.call('/srv/sdb2')], + mocks['isdir'].call_args_list) + self.assertEqual([], mocks['ismount'].call_args_list) + with mock_check_drive(isdir=True) as mocks: + self.assertIsNone(constraints.check_mount(root, path)) + self.assertIsNone(constraints.check_drive(root, path, True)) + self.assertEqual([], mocks['isdir'].call_args_list) + self.assertEqual([mock.call('/srv/sdb2'), mock.call('/srv/sdb2')], + mocks['ismount'].call_args_list) def test_check_float(self): self.assertFalse(constraints.check_float('')) diff --git a/test/unit/container/test_server.py b/test/unit/container/test_server.py index 760b1c6557..f0339c7852 100644 --- a/test/unit/container/test_server.py +++ b/test/unit/container/test_server.py @@ -41,7 +41,7 @@ from swift.container import server as container_server from swift.common import constraints from swift.common.utils import (Timestamp, mkdirs, public, replication, storage_directory, lock_parent_directory) -from test.unit import fake_http_connect, debug_logger +from test.unit import fake_http_connect, debug_logger, mock_check_drive from swift.common.storage_policy import (POLICIES, StoragePolicy) from swift.common.request_helpers import get_sys_meta_prefix @@ -63,9 +63,8 @@ def save_globals(): class TestContainerController(unittest.TestCase): """Test swift.container.server.ContainerController""" def setUp(self): - """Set up for testing swift.object_server.ObjectController""" - self.testdir = os.path.join(mkdtemp(), - 'tmp_test_object_server_ObjectController') + self.testdir = os.path.join( + mkdtemp(), 'tmp_test_container_server_ContainerController') mkdirs(self.testdir) rmtree(self.testdir) mkdirs(os.path.join(self.testdir, 'sda1')) @@ -305,15 +304,6 @@ class TestContainerController(unittest.TestCase): resp = req.get_response(self.controller) self.assertEqual(resp.status_int, 400) - def test_HEAD_insufficient_storage(self): - self.controller = container_server.ContainerController( - {'devices': self.testdir}) - req = Request.blank( - '/sda-null/p/a/c', environ={'REQUEST_METHOD': 'HEAD', - 'HTTP_X_TIMESTAMP': '1'}) - resp = req.get_response(self.controller) - self.assertEqual(resp.status_int, 507) - def test_HEAD_invalid_content_type(self): req = Request.blank( '/sda1/p/a/c', environ={'REQUEST_METHOD': 'HEAD'}, @@ -343,6 +333,60 @@ class TestContainerController(unittest.TestCase): self.assertEqual(resp.headers['Server'], (self.controller.server_type + '/' + swift_version)) + def test_insufficient_storage_mount_check_true(self): + conf = {'devices': self.testdir, 'mount_check': 'true'} + container_controller = container_server.ContainerController(conf) + self.assertTrue(container_controller.mount_check) + for method in container_controller.allowed_methods: + if method == 'OPTIONS': + continue + path = '/sda1/p/' + if method == 'REPLICATE': + path += 'suff' + else: + path += 'a/c' + req = Request.blank(path, method=method, + headers={'x-timestamp': '1'}) + with mock_check_drive() as mocks: + try: + resp = req.get_response(container_controller) + self.assertEqual(resp.status_int, 507) + mocks['ismount'].return_value = True + resp = req.get_response(container_controller) + self.assertNotEqual(resp.status_int, 507) + # feel free to rip out this last assertion... + expected = 2 if method == 'PUT' else 4 + self.assertEqual(resp.status_int // 100, expected) + except AssertionError as e: + self.fail('%s for %s' % (e, method)) + + def test_insufficient_storage_mount_check_false(self): + conf = {'devices': self.testdir, 'mount_check': 'false'} + container_controller = container_server.ContainerController(conf) + self.assertFalse(container_controller.mount_check) + for method in container_controller.allowed_methods: + if method == 'OPTIONS': + continue + path = '/sda1/p/' + if method == 'REPLICATE': + path += 'suff' + else: + path += 'a/c' + req = Request.blank(path, method=method, + headers={'x-timestamp': '1'}) + with mock_check_drive() as mocks: + try: + resp = req.get_response(container_controller) + self.assertEqual(resp.status_int, 507) + mocks['isdir'].return_value = True + resp = req.get_response(container_controller) + self.assertNotEqual(resp.status_int, 507) + # feel free to rip out this last assertion... + expected = 2 if method == 'PUT' else 4 + self.assertEqual(resp.status_int // 100, expected) + except AssertionError as e: + self.fail('%s for %s' % (e, method)) + def test_PUT(self): req = Request.blank( '/sda1/p/a/c', @@ -813,15 +857,6 @@ class TestContainerController(unittest.TestCase): resp = req.get_response(self.controller) self.assertEqual(resp.status_int, 400) - def test_PUT_insufficient_storage(self): - self.controller = container_server.ContainerController( - {'devices': self.testdir}) - req = Request.blank( - '/sda-null/p/a/c', environ={'REQUEST_METHOD': 'PUT', - 'HTTP_X_TIMESTAMP': '1'}) - resp = req.get_response(self.controller) - self.assertEqual(resp.status_int, 507) - def test_POST_HEAD_metadata(self): req = Request.blank( '/sda1/p/a/c', environ={'REQUEST_METHOD': 'PUT'}, @@ -948,15 +983,6 @@ class TestContainerController(unittest.TestCase): resp = req.get_response(self.controller) self.assertEqual(resp.status_int, 400) - def test_POST_insufficient_storage(self): - self.controller = container_server.ContainerController( - {'devices': self.testdir}) - req = Request.blank( - '/sda-null/p/a/c', environ={'REQUEST_METHOD': 'POST', - 'HTTP_X_TIMESTAMP': '1'}) - resp = req.get_response(self.controller) - self.assertEqual(resp.status_int, 507) - def test_POST_invalid_container_sync_to(self): self.controller = container_server.ContainerController( {'devices': self.testdir}) @@ -1270,22 +1296,6 @@ class TestContainerController(unittest.TestCase): sync_containers = [c for c in sync_store.synced_containers_generator()] self.assertFalse(sync_containers) - def test_REPLICATE_insufficient_storage(self): - conf = {'devices': self.testdir, 'mount_check': 'true'} - self.container_controller = container_server.ContainerController( - conf) - - def fake_check_mount(*args, **kwargs): - return False - - with mock.patch("swift.common.constraints.check_mount", - fake_check_mount): - req = Request.blank('/sda1/p/suff', - environ={'REQUEST_METHOD': 'REPLICATE'}, - headers={}) - resp = req.get_response(self.container_controller) - self.assertEqual(resp.status_int, 507) - def test_REPLICATE_rsync_then_merge_works(self): def fake_rsync_then_merge(self, drive, db_file, args): return HTTPNoContent() @@ -2012,15 +2022,6 @@ class TestContainerController(unittest.TestCase): resp = req.get_response(self.controller) self.assertEqual(resp.status_int, 400) - def test_DELETE_insufficient_storage(self): - self.controller = container_server.ContainerController( - {'devices': self.testdir}) - req = Request.blank( - '/sda-null/p/a/c', environ={'REQUEST_METHOD': 'DELETE', - 'HTTP_X_TIMESTAMP': '1'}) - resp = req.get_response(self.controller) - self.assertEqual(resp.status_int, 507) - def test_GET_over_limit(self): req = Request.blank( '/sda1/p/a/c?limit=%d' % @@ -2603,15 +2604,6 @@ class TestContainerController(unittest.TestCase): "content_type": "text/plain", "last_modified": "1970-01-01T00:00:01.000000"}]) - def test_GET_insufficient_storage(self): - self.controller = container_server.ContainerController( - {'devices': self.testdir}) - req = Request.blank( - '/sda-null/p/a/c', environ={'REQUEST_METHOD': 'GET', - 'HTTP_X_TIMESTAMP': '1'}) - resp = req.get_response(self.controller) - self.assertEqual(resp.status_int, 507) - def test_through_call(self): inbuf = BytesIO() errbuf = StringIO() diff --git a/test/unit/obj/test_auditor.py b/test/unit/obj/test_auditor.py index 0947f2f870..95a7533ec2 100644 --- a/test/unit/obj/test_auditor.py +++ b/test/unit/obj/test_auditor.py @@ -906,7 +906,7 @@ class TestAuditor(unittest.TestCase): self.disk_file.delete(ts_tomb) # this get_hashes call will truncate the invalid hashes entry self.disk_file.manager.get_hashes( - self.devices + '/sda', '0', [], self.disk_file.policy) + 'sda', '0', [], self.disk_file.policy) suffix = basename(dirname(self.disk_file._datadir)) part_dir = dirname(dirname(self.disk_file._datadir)) # sanity checks... @@ -1011,7 +1011,7 @@ class TestAuditor(unittest.TestCase): # this get_hashes call will truncate the invalid hashes entry self.disk_file.manager.get_hashes( - self.devices + '/sda', '0', [], self.disk_file.policy) + 'sda', '0', [], self.disk_file.policy) with open(hash_invalid, 'rb') as fp: self.assertEqual('', fp.read().strip('\n')) # sanity check diff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py index 7038a571ac..2d39c5c9af 100644 --- a/test/unit/obj/test_diskfile.py +++ b/test/unit/obj/test_diskfile.py @@ -44,7 +44,8 @@ from swift.obj.diskfile import MD5_OF_EMPTY_STRING, update_auditor_status from test.unit import (FakeLogger, mock as unit_mock, temptree, patch_policies, debug_logger, EMPTY_ETAG, make_timestamp_iter, DEFAULT_TEST_EC_TYPE, - requires_o_tmpfile_support, encode_frag_archive_bodies) + requires_o_tmpfile_support, encode_frag_archive_bodies, + mock_check_drive) from nose import SkipTest from swift.obj import diskfile from swift.common import utils @@ -2944,32 +2945,26 @@ class DiskFileMixin(BaseDiskFileTestMixin): mount_check = None self.df_mgr.mount_check = True - with mock.patch('swift.obj.diskfile.check_mount', - mock.MagicMock(return_value=False)): + with mock_check_drive(ismount=False): self.assertEqual(self.df_mgr.get_dev_path(device, mount_check), None) - with mock.patch('swift.obj.diskfile.check_mount', - mock.MagicMock(return_value=True)): + with mock_check_drive(ismount=True): self.assertEqual(self.df_mgr.get_dev_path(device, mount_check), dev_path) self.df_mgr.mount_check = False - with mock.patch('swift.obj.diskfile.check_dir', - mock.MagicMock(return_value=False)): + with mock_check_drive(isdir=False): self.assertEqual(self.df_mgr.get_dev_path(device, mount_check), None) - with mock.patch('swift.obj.diskfile.check_dir', - mock.MagicMock(return_value=True)): + with mock_check_drive(isdir=True): self.assertEqual(self.df_mgr.get_dev_path(device, mount_check), dev_path) mount_check = True - with mock.patch('swift.obj.diskfile.check_mount', - mock.MagicMock(return_value=False)): + with mock_check_drive(ismount=False): self.assertEqual(self.df_mgr.get_dev_path(device, mount_check), None) - with mock.patch('swift.obj.diskfile.check_mount', - mock.MagicMock(return_value=True)): + with mock_check_drive(ismount=True): self.assertEqual(self.df_mgr.get_dev_path(device, mount_check), dev_path) @@ -5855,7 +5850,7 @@ class TestSuffixHashes(unittest.TestCase): suffix_dir = os.path.dirname(df._datadir) for i in itertools.count(): df2 = df._manager.get_diskfile( - df._device_path, + os.path.basename(df._device_path), df._datadir.split('/')[-3], df._account, df._container, @@ -7706,8 +7701,7 @@ class TestSuffixHashes(unittest.TestCase): for policy in self.iter_policies(): df_mgr = self.df_router[policy] df_mgr.mount_check = True - with mock.patch('swift.obj.diskfile.check_mount', - mock.MagicMock(side_effect=[False])): + with mock_check_drive(ismount=False): self.assertRaises( DiskFileDeviceUnavailable, df_mgr.get_hashes, self.existing_device, '0', ['123'], diff --git a/test/unit/obj/test_reconstructor.py b/test/unit/obj/test_reconstructor.py index 25ace7339a..176afdfca1 100644 --- a/test/unit/obj/test_reconstructor.py +++ b/test/unit/obj/test_reconstructor.py @@ -2762,46 +2762,51 @@ class TestObjectReconstructor(BaseTestObjectReconstructor): paths = [] - def fake_check_mount(devices, device): - paths.append(os.path.join(devices, device)) - return False + def fake_check_drive(devices, device, mount_check): + path = os.path.join(devices, device) + if (not mount_check) and os.path.isdir(path): + # while mount_check is false, the test still creates the dirs + paths.append(path) + return path + return None with mock.patch('swift.obj.reconstructor.whataremyips', return_value=[self.ip]), \ mock.patch.object(self.policy.object_ring, '_devs', new=stub_ring_devs), \ - mock.patch('swift.obj.diskfile.check_mount', - fake_check_mount): + mock.patch('swift.obj.diskfile.check_drive', + fake_check_drive): part_infos = list(self.reconstructor.collect_parts()) self.assertEqual(2, len(part_infos)) # sanity, same jobs self.assertEqual(set(int(p['partition']) for p in part_infos), set([0, 1])) - # ... because ismount was not called - self.assertEqual(paths, []) + # ... because fake_check_drive returned paths for both dirs + self.assertEqual(set(paths), set([ + os.path.join(self.devices, dev) for dev in local_devs])) # ... now with mount check self._configure_reconstructor(mount_check=True) self.assertTrue(self.reconstructor.mount_check) + paths = [] for policy in POLICIES: self.assertTrue(self.reconstructor._df_router[policy].mount_check) with mock.patch('swift.obj.reconstructor.whataremyips', return_value=[self.ip]), \ mock.patch.object(self.policy.object_ring, '_devs', new=stub_ring_devs), \ - mock.patch('swift.obj.diskfile.check_mount', - fake_check_mount): + mock.patch('swift.obj.diskfile.check_drive', + fake_check_drive): part_infos = list(self.reconstructor.collect_parts()) self.assertEqual([], part_infos) # sanity, no jobs - # ... because fake_ismount returned False for both paths - self.assertEqual(set(paths), set([ - os.path.join(self.devices, dev) for dev in local_devs])) + # ... because fake_check_drive returned False for both paths + self.assertFalse(paths) - def fake_check_mount(devices, device): - path = os.path.join(devices, device) - if path.endswith('sda'): - return True + def fake_check_drive(devices, device, mount_check): + self.assertTrue(mount_check) + if device == 'sda': + return os.path.join(devices, device) else: return False @@ -2809,8 +2814,8 @@ class TestObjectReconstructor(BaseTestObjectReconstructor): return_value=[self.ip]), \ mock.patch.object(self.policy.object_ring, '_devs', new=stub_ring_devs), \ - mock.patch('swift.obj.diskfile.check_mount', - fake_check_mount): + mock.patch('swift.obj.diskfile.check_drive', + fake_check_drive): part_infos = list(self.reconstructor.collect_parts()) self.assertEqual(1, len(part_infos)) # only sda picked up (part 0) self.assertEqual(part_infos[0]['partition'], 0) diff --git a/test/unit/obj/test_server.py b/test/unit/obj/test_server.py index 9ee2df1763..bc37d182a2 100644 --- a/test/unit/obj/test_server.py +++ b/test/unit/obj/test_server.py @@ -45,7 +45,7 @@ from swift import __version__ as swift_version from swift.common.http import is_success from test import listen_zero from test.unit import FakeLogger, debug_logger, mocked_http_conn, \ - make_timestamp_iter, DEFAULT_TEST_EC_TYPE + make_timestamp_iter, DEFAULT_TEST_EC_TYPE, mock_check_drive from test.unit import connect_tcp, readuntil2crlfs, patch_policies, \ encode_frag_archive_bodies from swift.obj import server as object_server @@ -2762,6 +2762,68 @@ class TestObjectController(unittest.TestCase): self.assertEqual(resp.headers['Server'], (server_handler.server_type + '/' + swift_version)) + def test_insufficient_storage_mount_check_true(self): + conf = {'devices': self.testdir, 'mount_check': 'true'} + object_controller = object_server.ObjectController(conf) + for policy in POLICIES: + mgr = object_controller._diskfile_router[policy] + self.assertTrue(mgr.mount_check) + for method in object_controller.allowed_methods: + if method in ('OPTIONS', 'SSYNC'): + continue + path = '/sda1/p/' + if method == 'REPLICATE': + path += 'suff' + else: + path += 'a/c/o' + req = Request.blank(path, method=method, + headers={'x-timestamp': '1', + 'content-type': 'app/test', + 'content-length': 0}) + with mock_check_drive() as mocks: + try: + resp = req.get_response(object_controller) + self.assertEqual(resp.status_int, 507) + mocks['ismount'].return_value = True + resp = req.get_response(object_controller) + self.assertNotEqual(resp.status_int, 507) + # feel free to rip out this last assertion... + expected = 2 if method in ('PUT', 'REPLICATE') else 4 + self.assertEqual(resp.status_int // 100, expected) + except AssertionError as e: + self.fail('%s for %s' % (e, method)) + + def test_insufficient_storage_mount_check_false(self): + conf = {'devices': self.testdir, 'mount_check': 'false'} + object_controller = object_server.ObjectController(conf) + for policy in POLICIES: + mgr = object_controller._diskfile_router[policy] + self.assertFalse(mgr.mount_check) + for method in object_controller.allowed_methods: + if method in ('OPTIONS', 'SSYNC'): + continue + path = '/sda1/p/' + if method == 'REPLICATE': + path += 'suff' + else: + path += 'a/c/o' + req = Request.blank(path, method=method, + headers={'x-timestamp': '1', + 'content-type': 'app/test', + 'content-length': 0}) + with mock_check_drive() as mocks: + try: + resp = req.get_response(object_controller) + self.assertEqual(resp.status_int, 507) + mocks['isdir'].return_value = True + resp = req.get_response(object_controller) + self.assertNotEqual(resp.status_int, 507) + # feel free to rip out this last assertion... + expected = 2 if method in ('PUT', 'REPLICATE') else 4 + self.assertEqual(resp.status_int // 100, expected) + except AssertionError as e: + self.fail('%s for %s' % (e, method)) + def test_GET(self): # Test swift.obj.server.ObjectController.GET req = Request.blank('/sda1/p/a/c', environ={'REQUEST_METHOD': 'GET'}) @@ -6225,22 +6287,6 @@ class TestObjectController(unittest.TestCase): tpool.execute = was_tpool_exe diskfile.DiskFileManager._get_hashes = was_get_hashes - def test_REPLICATE_insufficient_storage(self): - conf = {'devices': self.testdir, 'mount_check': 'true'} - self.object_controller = object_server.ObjectController( - conf, logger=debug_logger()) - self.object_controller.bytes_per_sync = 1 - - def fake_check_mount(*args, **kwargs): - return False - - with mock.patch("swift.obj.diskfile.check_mount", fake_check_mount): - req = Request.blank('/sda1/p/suff', - environ={'REQUEST_METHOD': 'REPLICATE'}, - headers={}) - resp = req.get_response(self.object_controller) - self.assertEqual(resp.status_int, 507) - def test_REPLICATE_reclaims_tombstones(self): conf = {'devices': self.testdir, 'mount_check': False, 'reclaim_age': 100} diff --git a/test/unit/obj/test_ssync_receiver.py b/test/unit/obj/test_ssync_receiver.py index 5821f6282e..739d28d38d 100644 --- a/test/unit/obj/test_ssync_receiver.py +++ b/test/unit/obj/test_ssync_receiver.py @@ -34,7 +34,8 @@ from swift.obj import ssync_receiver, ssync_sender from swift.obj.reconstructor import ObjectReconstructor from test import listen_zero, unit -from test.unit import debug_logger, patch_policies, make_timestamp_iter +from test.unit import (debug_logger, patch_policies, make_timestamp_iter, + mock_check_drive) from test.unit.obj.common import write_diskfile @@ -370,8 +371,7 @@ class TestReceiver(unittest.TestCase): mock.patch.object( self.controller._diskfile_router[POLICIES.legacy], 'mount_check', False), \ - mock.patch('swift.obj.diskfile.check_mount', - return_value=False) as mocked_check_mount: + mock_check_drive(isdir=True) as mocks: req = swob.Request.blank( '/device/partition', environ={'REQUEST_METHOD': 'SSYNC'}) resp = req.get_response(self.controller) @@ -379,14 +379,13 @@ class TestReceiver(unittest.TestCase): self.body_lines(resp.body), [':ERROR: 0 "Looking for :MISSING_CHECK: START got \'\'"']) self.assertEqual(resp.status_int, 200) - self.assertFalse(mocked_check_mount.called) + self.assertEqual([], mocks['ismount'].call_args_list) with mock.patch.object(self.controller, 'replication_semaphore'), \ mock.patch.object( self.controller._diskfile_router[POLICIES.legacy], 'mount_check', True), \ - mock.patch('swift.obj.diskfile.check_mount', - return_value=False) as mocked_check_mount: + mock_check_drive(ismount=False) as mocks: req = swob.Request.blank( '/device/partition', environ={'REQUEST_METHOD': 'SSYNC'}) resp = req.get_response(self.controller) @@ -396,12 +395,12 @@ class TestReceiver(unittest.TestCase): "was not enough space to save the resource. Drive: " "device

"]) self.assertEqual(resp.status_int, 507) - mocked_check_mount.assert_called_once_with( + self.assertEqual([mock.call(os.path.join( self.controller._diskfile_router[POLICIES.legacy].devices, - 'device') + 'device'))], mocks['ismount'].call_args_list) - mocked_check_mount.reset_mock() - mocked_check_mount.return_value = True + mocks['ismount'].reset_mock() + mocks['ismount'].return_value = True req = swob.Request.blank( '/device/partition', environ={'REQUEST_METHOD': 'SSYNC'}) resp = req.get_response(self.controller) @@ -409,9 +408,9 @@ class TestReceiver(unittest.TestCase): self.body_lines(resp.body), [':ERROR: 0 "Looking for :MISSING_CHECK: START got \'\'"']) self.assertEqual(resp.status_int, 200) - mocked_check_mount.assert_called_once_with( + self.assertEqual([mock.call(os.path.join( self.controller._diskfile_router[POLICIES.legacy].devices, - 'device') + 'device'))], mocks['ismount'].call_args_list) def test_SSYNC_Exception(self):