Merge "Always require device dir for containers"

openstack · Sep 15, 2017 · 30fd4ea · 30fd4ea
2 parents b78f3a7 + 163fb4d
commit 30fd4ea
Show file tree

Hide file tree

Showing 14 changed files with 321 additions and 265 deletions.
diff --git a/swift/account/server.py b/swift/account/server.py
@@ -29,7 +29,7 @@
 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 @@ def _deleted_response(self, broker, req, resp, body=''):
     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 @@ def DELETE(self, req):
     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 @@ def HEAD(self, req):
         """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 @@ def GET(self, req):
         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 @@ def REPLICATE(self, req):
         """
         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 @@ def POST(self, req):
         """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
@@ -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
@@ -209,7 +209,7 @@ def get_unmounted(self):
                 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 @@ def get_diskusage(self):
                 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
@@ -35,7 +35,7 @@
     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 @@ def DELETE(self, req):
         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 @@ def PUT(self, req):
                 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 @@ def HEAD(self, req):
         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 @@ def GET(self, req):
                     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 @@ def REPLICATE(self, req):
         """
         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 @@ def POST(self, req):
                 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
@@ -57,7 +57,7 @@
     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 @@ def get_dev_path(self, device, mount_check=None):
         # 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
@@ -709,49 +709,28 @@ def quiet_eventlet_exceptions():
         eventlet_debug.hub_exceptions(orig_state)
 
 
-class MockTrue(object):
-    """
-    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.
-
-    >>> 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
-
+@contextmanager
+def mock_check_drive(isdir=False, ismount=False):
     """
+    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.
 
-    def __getattribute__(self, *args, **kwargs):
-        return self
-
-    def __call__(self, *args, **kwargs):
-        return self
+    Replace the constraint modules underlying os calls with mocks.
 
-    def __repr__(*args, **kwargs):
-        return repr(True)
-
-    def __eq__(self, other):
-        return other is True
-
-    def __ne__(self, other):
-        return other is not True
+    :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
+    """
+    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