LGTM, thanks.
On Tue, Dec 17, 2013 at 10:28 AM, Santi Raffa <[email protected]> wrote: > This will allow code reuse for Gluster through composition, rather > than inheritance. > > Signed-off-by: Santi Raffa <[email protected]> > --- > lib/storage/filestorage.py | 172 > ++++++++++++++++++------ > test/py/ganeti.storage.filestorage_unittest.py | 91 +++++++++++++ > 2 files changed, 222 insertions(+), 41 deletions(-) > > diff --git a/lib/storage/filestorage.py b/lib/storage/filestorage.py > index 9beb56b..8af559c 100644 > --- a/lib/storage/filestorage.py > +++ b/lib/storage/filestorage.py > @@ -32,9 +32,133 @@ from ganeti import constants > from ganeti import errors > from ganeti import pathutils > from ganeti import utils > +from ganeti.utils import io > from ganeti.storage import base > > > +class FileDeviceHelper(object): > + > + @classmethod > + def CreateFile(cls, path, size, create_folders=False, > + _file_path_acceptance_fn=None): > + """Create a new file and its file device helper. > + > + @param size: the size in MiBs the file should be truncated to. > + @param create_folders: create the directories for the path if > necessary > + (using L{ganeti.utils.io.Makedirs}) > + > + @rtype: FileDeviceHelper > + @return: The FileDeviceHelper object representing the object. > + @raise errors.FileStoragePathError: if the file path is disallowed by > policy > + > + """ > + > + if not _file_path_acceptance_fn: > + _file_path_acceptance_fn = CheckFileStoragePathAcceptance > + _file_path_acceptance_fn(path) > + > + if create_folders: > + folder = os.path.dirname(path) > + io.Makedirs(folder) > + > + try: > + fd = os.open(path, os.O_RDWR | os.O_CREAT | os.O_EXCL) > + f = os.fdopen(fd, "w") > + f.truncate(size * 1024 * 1024) > + f.close() > + except EnvironmentError as err: > + base.ThrowError("%s: can't create: %s", path, str(err)) > + > + return FileDeviceHelper(path, > + > _file_path_acceptance_fn=_file_path_acceptance_fn) > + > + def __init__(self, path, _file_path_acceptance_fn=None): > + """Create a new file device helper. > + > + @raise errors.FileStoragePathError: if the file path is disallowed by > policy > + > + """ > + if not _file_path_acceptance_fn: > + _file_path_acceptance_fn = CheckFileStoragePathAcceptance > + _file_path_acceptance_fn(path) > + > + self.path = path > + > + def Exists(self, assert_exists=None): > + """Check for the existence of the given file. > + > + @param assert_exists: creates an assertion on the result value: > + - if true, raise errors.BlockDeviceError if the file doesn't exist > + - if false, raise errors.BlockDeviceError if the file does exist > + @rtype: boolean > + @return: True if the file exists > + > + """ > + > + exists = os.path.isfile(self.path) > + > + if not exists and assert_exists is True: > + raise base.ThrowError("%s: No such file", self.path) > + if exists and assert_exists is False: > + raise base.ThrowError("%s: File exists", self.path) > + > + return exists > + > + def Remove(self): > + """Remove the file backing the block device. > + > + @rtype: boolean > + @return: True if the removal was successful > + > + """ > + try: > + os.remove(self.path) > + return True > + except OSError as err: > + if err.errno != errno.ENOENT: > + base.ThrowError("%s: can't remove: %s", self.path, err) > + return False > + > + def Size(self): > + """Return the actual disk size in bytes. > + > + @rtype: int > + @return: The file size in bytes. > + > + """ > + self.Exists(assert_exists=True) > + try: > + return os.stat(self.path).st_size > + except OSError as err: > + base.ThrowError("%s: can't stat: %s", self.path, err) > + > + def Grow(self, amount, dryrun, backingstore, _excl_stor): > + """Grow the file > + > + @param amount: the amount (in mebibytes) to grow by. > + > + """ > + # Check that the file exists > + self.Exists(assert_exists=True) > + > + if amount < 0: > + base.ThrowError("%s: can't grow by negative amount", self.path) > + > + if dryrun: > + return > + if not backingstore: > + return > + > + current_size = self.Size() > + new_size = current_size + amount * 1024 * 1024 > + try: > + f = open(self.path, "a+") > + f.truncate(new_size) > + f.close() > + except EnvironmentError, err: > + base.ThrowError("%s: can't grow: ", self.path, str(err)) > + > + > class FileStorage(base.BlockDev): > """File device. > > @@ -55,9 +179,7 @@ class FileStorage(base.BlockDev): > raise ValueError("Invalid configuration data %s" % str(unique_id)) > self.driver = unique_id[0] > self.dev_path = unique_id[1] > - > - CheckFileStoragePathAcceptance(self.dev_path) > - > + self.file = FileDeviceHelper(self.dev_path) > self.Attach() > > def Assemble(self): > @@ -66,8 +188,7 @@ class FileStorage(base.BlockDev): > Checks whether the file device exists, raises BlockDeviceError > otherwise. > > """ > - if not os.path.exists(self.dev_path): > - base.ThrowError("File device '%s' does not exist" % self.dev_path) > + self.file.Exists(assert_exists=True) > > def Shutdown(self): > """Shutdown the device. > @@ -101,11 +222,7 @@ class FileStorage(base.BlockDev): > @return: True if the removal was successful > > """ > - try: > - os.remove(self.dev_path) > - except OSError, err: > - if err.errno != errno.ENOENT: > - base.ThrowError("Can't remove file '%s': %s", self.dev_path, err) > + return self.file.Remove() > > def Rename(self, new_id): > """Renames the file. > @@ -122,20 +239,9 @@ class FileStorage(base.BlockDev): > """ > if not backingstore: > return > - # Check that the file exists > - self.Assemble() > - current_size = self.GetActualSize() > - new_size = current_size + amount * 1024 * 1024 > - assert new_size > current_size, "Cannot Grow with a negative amount" > - # We can't really simulate the growth > if dryrun: > return > - try: > - f = open(self.dev_path, "a+") > - f.truncate(new_size) > - f.close() > - except EnvironmentError, err: > - base.ThrowError("Error in file growth: %", str(err)) > + self.file.Grow(amount, dryrun, backingstore, excl_stor) > > def Attach(self): > """Attach to an existing file. > @@ -146,7 +252,7 @@ class FileStorage(base.BlockDev): > @return: True if file exists > > """ > - self.attached = os.path.exists(self.dev_path) > + self.attached = self.file.Exists() > return self.attached > > def GetActualSize(self): > @@ -155,12 +261,7 @@ class FileStorage(base.BlockDev): > @note: the device needs to be active when this is called > > """ > - assert self.attached, "BlockDevice not attached in GetActualSize()" > - try: > - st = os.stat(self.dev_path) > - return st.st_size > - except OSError, err: > - base.ThrowError("Can't stat %s: %s", self.dev_path, err) > + return self.file.Size() > > @classmethod > def Create(cls, unique_id, children, size, spindles, params, excl_stor, > @@ -182,18 +283,7 @@ class FileStorage(base.BlockDev): > > dev_path = unique_id[1] > > - CheckFileStoragePathAcceptance(dev_path) > - > - try: > - fd = os.open(dev_path, os.O_RDWR | os.O_CREAT | os.O_EXCL) > - f = os.fdopen(fd, "w") > - f.truncate(size * 1024 * 1024) > - f.close() > - except EnvironmentError, err: > - if err.errno == errno.EEXIST: > - base.ThrowError("File already existing: %s", dev_path) > - base.ThrowError("Error in file creation: %", str(err)) > - > + FileDeviceHelper.CreateFile(dev_path, size) > return FileStorage(unique_id, children, size, params, dyn_params) > > > diff --git a/test/py/ganeti.storage.filestorage_unittest.py b/test/py/ > ganeti.storage.filestorage_unittest.py > index 43b8de3..09b9885 100755 > --- a/test/py/ganeti.storage.filestorage_unittest.py > +++ b/test/py/ganeti.storage.filestorage_unittest.py > @@ -28,7 +28,9 @@ import unittest > > from ganeti import errors > from ganeti.storage import filestorage > +from ganeti.utils import io > from ganeti import utils > +from ganeti import constants > > import testutils > > @@ -219,5 +221,94 @@ class > TestCheckFileStoragePathExistance(testutils.GanetiTestCase): > "/usr/lib64/xyz", _filename=tmpfile) > > > +class TestFileDeviceHelper(testutils.GanetiTestCase): > + > + @staticmethod > + def _Make(path, create_with_size=None, create_folders=False): > + skip_checks = lambda path: None > + if create_with_size: > + return filestorage.FileDeviceHelper.Create( > + path, create_with_size, create_folders=create_folders, > + _file_path_acceptance_fn=skip_checks > + ) > + else: > + return filestorage.FileDeviceHelper(path, > + > _file_path_acceptance_fn=skip_checks) > + > + class TempEnvironment(object): > + > + def __init__(self, create_file=False, delete_file=True): > + self.create_file = create_file > + self.delete_file = delete_file > + > + def __enter__(self): > + self.directory = tempfile.mkdtemp() > + self.subdirectory = io.PathJoin(self.directory, "pinky") > + os.mkdir(self.subdirectory) > + self.path = io.PathJoin(self.subdirectory, "bunny") > + self.volume = TestFileDeviceHelper._Make(self.path) > + if self.create_file: > + open(self.path, mode="w").close() > + return self > + > + def __exit__(self, *args): > + if self.delete_file: > + os.unlink(self.path) > + os.rmdir(self.subdirectory) > + os.rmdir(self.directory) > + return False #don't swallow exceptions > + > + def testOperationsOnNonExistingFiles(self): > + path = "/e/no/ent" > + volume = TestFileDeviceHelper._Make(path) > + > + # These should fail horribly. > + volume.Exists(assert_exists=False) > + self.assertRaises(errors.BlockDeviceError, lambda: \ > + volume.Exists(assert_exists=True)) > + self.assertRaises(errors.BlockDeviceError, lambda: \ > + volume.Size()) > + self.assertRaises(errors.BlockDeviceError, lambda: \ > + volume.Grow(0.020, True, False, None)) > + > + # Removing however fails silently. > + volume.Remove() > + > + # Make sure we don't create all directories for you unless we ask for > it > + self.assertRaises(errors.BlockDeviceError, lambda: \ > + TestFileDeviceHelper._Make(path, create_with_size=1)) > + > + def testFileCreation(self): > + with TestFileDeviceHelper.TempEnvironment() as env: > + TestFileDeviceHelper._Make(env.path, create_with_size=1) > + > + self.assertTrue(env.volume.Exists()) > + env.volume.Exists(assert_exists=True) > + self.assertRaises(errors.BlockDeviceError, lambda: \ > + env.volume.Exists(assert_exists=False)) > + > + self.assertRaises(errors.BlockDeviceError, lambda: \ > + TestFileDeviceHelper._Make("/enoent", create_with_size=0.042)) > + > + def testFailSizeDirectory(self): > + # This should still fail. > + with TestFileDeviceHelper.TempEnvironment(delete_file=False) as env: > + self.assertRaises(errors.BlockDeviceError, lambda: \ > + TestFileDeviceHelper._Make(env.subdirectory).Size()) > + > + def testGrowFile(self): > + with TestFileDeviceHelper.TempEnvironment(create_file=True) as env: > + self.assertRaises(errors.BlockDeviceError, lambda: \ > + env.volume.Grow(-1, False, True, None)) > + > + env.volume.Grow(2, False, True, None) > + self.assertEqual(2.0, env.volume.Size() / 1024.0**2) > + > + def testRemoveFile(self): > + with TestFileDeviceHelper.TempEnvironment(create_file=True, > + delete_file=False) as env: > + env.volume.Remove() > + env.volume.Exists(assert_exists=False) > + > if __name__ == "__main__": > testutils.GanetiTestProgram() > -- > 1.7.10.4 > > -- Thomas Thrainer | Software Engineer | [email protected] | Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores
