object-storage: Initial unittest of DiskFile class

If we had this ahead of time, we could have avoided the errors that
were encountered leading to the fix-account-mapping fix (see
http://review.gluster.org/4222).

This represents 100% coverage of the DiskFile module, but the coverage
report says otherwise, unfortunately. That is because the put() method
invokes eventlets during the test run, and coverage is not able to
accurately track the coverage lines properly. If one comments out the
"tpool.execute()" line in DiskFile.put() the coverage report then
reports 100% for the DiskFile module.

Additionally, we changed DiskFile.put() in four ways that should not
change its behavior:

  1. Comments were added to explain various code paths and mark
     potential issues / fixes

  2. It no longer returns a boolean value, matching the behavior of
     swift.obj.server.DiskFile.put()

  3. It no longer logs a message when it detects a directory that
     already exists, instead is raises an exception

     We believe this is okay because we cannot find a code path that
     would lead to his condition. As a result, it makes it easier to
     test all the code paths in that routine.

  4. It no longer logs a message when create_dir_object() fails, since
     create_dir_object() raises an exception on failure only

     We also modified create_dir_object() to not return a boolean as a
     result of the above behavior.

Note that by implementing these tests up to this point we found three
code paths that would have failed if encountered due to missing
imports. We also made changes to the DiskFile module to make it a bit
easier to test, also eliminating an extra stat system call when
deleting directory objects.

Change-Id: I3286de83c1ec7c5e8d6cab9354e1c4397cee7497
BUG: 870589
Signed-off-by: Peter Portante <peter.portante@redhat.com>
Reviewed-on: http://review.gluster.org/4284
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Mohammed Junaid <junaid@redhat.com>

2 files changed, 52 insertions, 49 deletions

diff --git a/ufo/gluster/swift/common/DiskFile.py b/ufo/gluster/swift/common/DiskFile.py
index ddb53ed4201..e3f00a01ad7 100644
--- a/ufo/gluster/swift/common/DiskFile.py
+++ b/ufo/gluster/swift/common/DiskFile.py
@@ -14,10 +14,12 @@
 # limitations under the License.
 
 import os
+import errno
 from eventlet import tpool
 from tempfile import mkstemp
 from contextlib import contextmanager
 from swift.common.utils import normalize_timestamp, renamer
+from swift.common.exceptions import DiskFileNotExist
 from gluster.swift.common.utils import mkdirs, rmdirs, validate_object, \
      create_object_metadata,  do_open, do_close, do_unlink, do_chown, \
      do_stat, do_listdir, read_metadata, write_metadata
@@ -37,6 +39,10 @@ KEEP_CACHE_SIZE = (5 * 1024 * 1024)
 DISALLOWED_HEADERS = set('content-length content-type deleted etag'.split())
 
 
+class AlreadyExistsAsDir(Exception):
+    pass
+
+
 class Gluster_DiskFile(DiskFile):
     """
     Manage object files on disk.
@@ -142,7 +148,6 @@ class Gluster_DiskFile(DiskFile):
         mkdirs(dir_path)
         do_chown(dir_path, self.uid, self.gid)
         create_object_metadata(dir_path)
-        return True
 
     def put_metadata(self, metadata):
         obj_path = self.datadir + '/' + self.obj
@@ -162,7 +167,7 @@ class Gluster_DiskFile(DiskFile):
         """
         if extension == '.ts':
             # TombStone marker (deleted)
-            return True
+            return
 
         # Fix up the metadata to ensure it has a proper value for the
         # Content-Type metadata, as well as an X_TYPE and X_OBJECT_TYPE
@@ -170,6 +175,10 @@ class Gluster_DiskFile(DiskFile):
 
         content_type = metadata['Content-Type']
         if not content_type:
+            # FIXME: How can this be some object that evaluates to False?
+            #
+            # FIXME: If the file exists, we would already know it is a
+            # directory.
             metadata['Content-Type'] = FILE_TYPE
             x_object_type = FILE
         else:
@@ -178,23 +187,31 @@ class Gluster_DiskFile(DiskFile):
         metadata[X_OBJECT_TYPE] = x_object_type
 
         if extension == '.meta':
-            # Metadata recorded separately from the file
+            # Metadata recorded separately from the file, we just update the
+            # metadata for the file.
+            #
+            # FIXME: If the file does not exist, this call will fail.
             self.put_metadata(metadata)
-            return True
+            return
 
+        # Our caller will use '.data' here; we just ignore it since we map the
+        # URL directly to the file system.
         extension = ''
 
         if metadata[X_OBJECT_TYPE] == MARKER_DIR:
+            # FIXME: If we know it already exists, why call
+            # create_dir_object()?
             self.create_dir_object(os.path.join(self.datadir, self.obj))
             self.put_metadata(metadata)
             self.data_file = self.datadir + '/' + self.obj
-            return True
+            return
 
         # Check if directory already exists.
         if self.is_dir:
-            self.logger.error('Directory already exists %s/%s' % \
-                          (self.datadir , self.obj))
-            return False
+            # FIXME: How can we have a directory and it not be marked as a
+            # MARKER_DIR (see above)?
+            raise AlreadyExistsAsDir('File object already exists ' \
+                        'as a directory: %s/%s' % (self.datadir , self.obj))
 
         timestamp = normalize_timestamp(metadata[X_TIMESTAMP])
         write_metadata(tmppath, metadata)
@@ -203,18 +220,15 @@ class Gluster_DiskFile(DiskFile):
         tpool.execute(os.fsync, fd)
         if self.obj_path:
             dir_objs = self.obj_path.split('/')
+            assert len(dir_objs) >= 1
             tmp_path = ''
-            if len(dir_objs):
-                for dir_name in dir_objs:
-                    if tmp_path:
-                        tmp_path = tmp_path + '/' + dir_name
-                    else:
-                        tmp_path = dir_name
-                    if not self.create_dir_object(os.path.join(self.container_path,
-                            tmp_path)):
-                        self.logger.error("Failed in subdir %s",\
-                                        os.path.join(self.container_path,tmp_path))
-                        return False
+            for dir_name in dir_objs:
+                if tmp_path:
+                    tmp_path = tmp_path + '/' + dir_name
+                else:
+                    tmp_path = dir_name
+                self.create_dir_object(
+                    os.path.join(self.container_path, tmp_path))
 
         renamer(tmppath, os.path.join(self.datadir,
                                       self.obj + extension))
@@ -222,7 +236,7 @@ class Gluster_DiskFile(DiskFile):
               self.uid, self.gid)
         self.metadata = metadata
         self.data_file = self.datadir + '/' + self.obj + extension
-        return True
+        return
 
     def unlinkold(self, timestamp):
         """
@@ -231,33 +245,19 @@ class Gluster_DiskFile(DiskFile):
 
         :param timestamp: timestamp to compare with each file
         """
-        if self.metadata and self.metadata['X-Timestamp'] != timestamp:
-            self.unlink()
+        if not self.metadata or self.metadata['X-Timestamp'] >= timestamp:
+            return
 
-    def unlink(self):
-        """
-        Remove the file.
-        """
-        #Marker dir.
         if self.is_dir:
-            rmdirs(os.path.join(self.datadir, self.obj))
-            if not os.path.isdir(os.path.join(self.datadir, self.obj)):
-                self.metadata = {}
-                self.data_file = None
-            else:
+            # Marker directory object
+            if not rmdirs(os.path.join(self.datadir, self.obj)):
                 logging.error('Unable to delete dir %s' % os.path.join(self.datadir, self.obj))
-            return
-
-        for fname in do_listdir(self.datadir):
-            if fname == self.obj:
-                try:
+                return
+        else:
+            # File object
+            for fname in do_listdir(self.datadir):
+                if fname == self.obj:
                     do_unlink(os.path.join(self.datadir, fname))
-                except OSError, err:
-                    if err.errno != errno.ENOENT:
-                        raise
-
-        #Remove entire path for object.
-        #remove_dir_path(self.obj_path, self.container_path)
 
         self.metadata = {}
         self.data_file = None
@@ -286,7 +286,7 @@ class Gluster_DiskFile(DiskFile):
                         self.update_object(self.metadata)
 
                 return file_size
-        except OSError, err:
+        except OSError as err:
             if err.errno != errno.ENOENT:
                 raise
         raise DiskFileNotExist('Data File does not exist.')
diff --git a/ufo/gluster/swift/common/fs_utils.py b/ufo/gluster/swift/common/fs_utils.py
index 7f5292c2bf1..88368c78c9e 100644
--- a/ufo/gluster/swift/common/fs_utils.py
+++ b/ufo/gluster/swift/common/fs_utils.py
@@ -101,7 +101,10 @@ def do_rmdir(path):
         logging.exception("Rmdir failed on %s err: %s", path, str(err))
         if err.errno != errno.ENOENT:
             raise
-    return True
+        res = False
+    else:
+        res = True
+    return res
 
 def do_rename(old_path, new_path):
     try:
@@ -149,8 +152,8 @@ def dir_empty(path):
             return True
 
 def rmdirs(path):
-    if os.path.isdir(path) and dir_empty(path):
-        do_rmdir(path)
-    else:
-        logging.error("rmdirs failed dir may not be empty or not valid dir")
+    if not os.path.isdir(path) or not dir_empty(path):
+        logging.error("rmdirs failed: %s may not be empty or not valid dir", path)
         return False
+
+    return do_rmdir(path)