Change in vdsm[master]: storage: refactor nfsSD.py for posix storage

Greg Padgett has uploaded a new change for review.

Change subject: storage: refactor nfsSD.py for posix storage
......................................................................

storage: refactor nfsSD.py for posix storage

NFS and PosixFs domains are no longer handled the same way, so logic
that differs between them should be appropriately separated into
classes for each connection type.  This removes the need to have a
storageType parameter to NfsStorageDomain:_preCreateValidation().

For now, there's no reason to have a PosixFsStorageDomain class for
Posix storage (it would be empty), so just use MountableStorageDomain
until the need for a subclass arises.

Change-Id: Ic5788925f9c11b417aba713c652fc2a92f178830
Signed-off-by: Greg Padgett <gpadgett(a)redhat.com&gt;
---
M Makefile.am
M vdsm.spec.in
M vdsm/storage/Makefile.am
M vdsm/storage/hsm.py
A vdsm/storage/mountableSD.py
M vdsm/storage/nfsSD.py
M vdsm/storage/sdc.py
7 files changed, 148 insertions(+), 114 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/88/9088/1

diff --git a/Makefile.am b/Makefile.am
index bd262af..a96d3f7 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -83,6 +83,7 @@
 	vdsm/storage/lvm.py \
 	vdsm/storage/misc.py \
 	vdsm/storage/mount.py \
+	vdsm/storage/mountableSD.py \
 	vdsm/storage/multipath.py \
 	vdsm/storage/nfsSD.py \
 	vdsm/storage/outOfProcess.py \
diff --git a/vdsm.spec.in b/vdsm.spec.in
index c117ee5..d76495c 100644
--- a/vdsm.spec.in
+++ b/vdsm.spec.in
@@ -661,6 +661,7 @@
 %{_datadir}/%{vdsm_name}/storage/lvm.py*
 %{_datadir}/%{vdsm_name}/storage/misc.py*
 %{_datadir}/%{vdsm_name}/storage/mount.py*
+%{_datadir}/%{vdsm_name}/storage/mountableSD.py*
 %{_datadir}/%{vdsm_name}/storage/multipath.py*
 %{_datadir}/%{vdsm_name}/storage/nfsSD.py*
 %{_datadir}/%{vdsm_name}/storage/outOfProcess.py*
diff --git a/vdsm/storage/Makefile.am b/vdsm/storage/Makefile.am
index cff09be..cf8f768 100644
--- a/vdsm/storage/Makefile.am
+++ b/vdsm/storage/Makefile.am
@@ -41,6 +41,7 @@
 	lvm.py \
 	misc.py \
 	mount.py \
+	mountableSD.py \
 	multipath.py \
 	nfsSD.py \
 	outOfProcess.py \
diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index 259676b..f56c8b0 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -41,8 +41,9 @@
 import sp
 import sd
 import blockSD
-import nfsSD
 import localFsSD
+import mountableSD
+import nfsSD
 import lvm
 import fileUtils
 import multipath
@@ -2251,16 +2252,18 @@
 
         #getSharedLock(connectionsResource...)
         #getExclusiveLock(sdUUID...)
-        if storageType in sd.BLOCK_DOMAIN_TYPES:
-            newSD = blockSD.BlockStorageDomain.create(sdUUID, domainName,
-                    domClass, typeSpecificArg, storageType, domVersion)
-        elif storageType in (sd.NFS_DOMAIN, sd.POSIXFS_DOMAIN):
-            newSD = nfsSD.NfsStorageDomain.create(sdUUID, domainName, domClass,
-                    typeSpecificArg, storageType, domVersion)
-        elif storageType == sd.LOCALFS_DOMAIN:
-            newSD = localFsSD.LocalFsStorageDomain.create(sdUUID, domainName,
-                    domClass, typeSpecificArg, storageType, domVersion)
-        else:
+        sdConstructorMap = {
+            sd.NFS_DOMAIN : nfsSD.NfsStorageDomain,
+            sd.POSIXFS_DOMAIN : mountableSD.MountableStorageDomain,
+            sd.LOCALFS_DOMAIN : localFsSD.LocalFsStorageDomain,
+        }
+        for domType in sd.BLOCK_DOMAIN_TYPES:
+            sdConstructorMap[domType] = blockSD.BlockStorageDomain
+
+        try:
+            newSD = sdConstructorMap[storageType].create(sdUUID, domainName,
+                        domClass, typeSpecificArg, storageType, domVersion)
+        except KeyError:
             raise se.StorageDomainTypeError(storageType)
         sdCache.manuallyAddDomain(newSD)
 
diff --git a/vdsm/storage/mountableSD.py b/vdsm/storage/mountableSD.py
new file mode 100644
index 0000000..05284ab
--- /dev/null
+++ b/vdsm/storage/mountableSD.py
@@ -0,0 +1,122 @@
+#
+# Copyright 2009-2012 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301 USA
+#
+# Refer to the README and COPYING files for full details of the license
+#
+
+import os
+
+import sd
+import fileSD
+import fileUtils
+import storage_exception as se
+import outOfProcess as oop
+import mount
+import misc
+
+
+class MountableStorageDomain(fileSD.FileStorageDomain):
+
+    @classmethod
+    def _preCreateValidation(cls, sdUUID, domPath, typeSpecificArg, version):
+        # Some trivial resource validation
+        sd.validateDomainVersion(version)
+
+        # Make sure the underlying file system is mounted
+        if not mount.isMounted(domPath):
+            raise se.StorageDomainFSNotMounted(domPath)
+
+        fileSD.validateDirAccess(domPath)
+
+        # Make sure there are no remnants of other domain
+        mdpat = os.path.join(domPath, "*", sd.DOMAIN_META_DATA)
+        if len(oop.getProcessPool(sdUUID).glob.glob(mdpat)) > 0:
+            raise se.StorageDomainNotEmpty(typeSpecificArg)
+
+    @classmethod
+    def create(cls, sdUUID, domainName, domClass, remotePath, storageType,
+               version):
+        """
+        Create new storage domain.
+            'sdUUID' - Storage Domain UUID
+            'domainName' - storage domain name ("iso" or "data
domain name")
+            'domClass' - Data/Iso
+            'remotePath' - server:/export_path
+            'storageType' - NFS_DOMAIN, LOCALFS_DOMAIN, &etc.
+            'version' - DOMAIN_VERSIONS
+        """
+        cls.log.info("sdUUID=%s domainName=%s remotePath=%s "
+            "domClass=%s", sdUUID, domainName, remotePath, domClass)
+
+        if not misc.isAscii(domainName) and not sd.supportsUnicode(version):
+            raise se.UnicodeArgumentException()
+
+        # Create local path
+        mntPath = fileUtils.transformPath(remotePath)
+
+        mntPoint = os.path.join(cls.storage_repository,
+            sd.DOMAIN_MNT_POINT, mntPath)
+
+        cls._preCreateValidation(sdUUID, mntPoint, remotePath, version)
+
+        domainDir = os.path.join(mntPoint, sdUUID)
+        cls._prepareMetadata(domainDir, sdUUID, domainName, domClass,
+                            remotePath, storageType, version)
+
+        # create domain images folder
+        imagesDir = os.path.join(domainDir, sd.DOMAIN_IMAGES)
+        oop.getProcessPool(sdUUID).fileUtils.createdir(imagesDir)
+
+        # create special imageUUID for ISO/Floppy volumes
+        if domClass is sd.ISO_DOMAIN:
+            isoDir = os.path.join(imagesDir, sd.ISO_IMAGE_UUID)
+            oop.getProcessPool(sdUUID).fileUtils.createdir(isoDir)
+
+        fsd = cls(os.path.join(mntPoint, sdUUID))
+        fsd.initSPMlease()
+
+        return fsd
+
+    def selftest(self):
+        """
+        Run internal self test
+        """
+        if not mount.isMounted(self.mountpoint):
+            raise se.StorageDomainFSNotMounted(self.mountpoint)
+
+        # Run general part of selftest
+        fileSD.FileStorageDomain.selftest(self)
+
+    @staticmethod
+    def findDomainPath(sdUUID):
+        for tmpSdUUID, domainPath in fileSD.scanDomains("*"):
+            if tmpSdUUID == sdUUID and mount.isMounted(
+                                             os.path.join(domainPath, "..")):
+                return domainPath
+
+        raise se.StorageDomainDoesNotExist(sdUUID)
+
+    def getRealPath(self):
+        try:
+            return mount.getMountFromTarget(self.mountpoint).fs_spec
+        except mount.MountError:
+            return ""
+
+
+def findDomain(sdUUID):
+    return MountableStorageDomain(MountableStorageDomain
+                                  .findDomainPath(sdUUID))
diff --git a/vdsm/storage/nfsSD.py b/vdsm/storage/nfsSD.py
index 3a1c90e..15dd137 100644
--- a/vdsm/storage/nfsSD.py
+++ b/vdsm/storage/nfsSD.py
@@ -1,5 +1,5 @@
 #
-# Copyright 2009-2011 Red Hat, Inc.
+# Copyright 2009-2012 Red Hat, Inc.
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -18,110 +18,16 @@
 # Refer to the README and COPYING files for full details of the license
 #
 
-import os
-
-import sd
-import fileSD
-import fileUtils
+import mountableSD
 import storage_exception as se
-import outOfProcess as oop
-import mount
-import misc
 
 
-class NfsStorageDomain(fileSD.FileStorageDomain):
+class NfsStorageDomain(mountableSD.MountableStorageDomain):
 
     @classmethod
-    def _preCreateValidation(cls, sdUUID, domPath, typeSpecificArg,
-                             storageType, version):
-        # Some trivial resource validation
-        # TODO Checking storageType==nfs in the nfs class is not clean
-        if storageType == sd.NFS_DOMAIN and ":" not in typeSpecificArg:
+    def _preCreateValidation(cls, sdUUID, domPath, typeSpecificArg, version):
+        if ":" not in typeSpecificArg:
             raise se.StorageDomainIllegalRemotePath(typeSpecificArg)
 
-        sd.validateDomainVersion(version)
-
-        # Make sure the underlying file system is mounted
-        if not mount.isMounted(domPath):
-            raise se.StorageDomainFSNotMounted(domPath)
-
-        fileSD.validateDirAccess(domPath)
-
-        # Make sure there are no remnants of other domain
-        mdpat = os.path.join(domPath, "*", sd.DOMAIN_META_DATA)
-        if len(oop.getProcessPool(sdUUID).glob.glob(mdpat)) > 0:
-            raise se.StorageDomainNotEmpty(typeSpecificArg)
-
-    @classmethod
-    def create(cls, sdUUID, domainName, domClass, remotePath, storageType,
-               version):
-        """
-        Create new storage domain.
-            'sdUUID' - Storage Domain UUID
-            'domainName' - storage domain name ("iso" or "data
domain name")
-            'domClass' - Data/Iso
-            'remotePath' - server:/export_path
-            'storageType' - NFS_DOMAIN, LOCALFS_DOMAIN, &etc.
-            'version' - DOMAIN_VERSIONS
-        """
-        cls.log.info("sdUUID=%s domainName=%s remotePath=%s "
-            "domClass=%s", sdUUID, domainName, remotePath, domClass)
-
-        if not misc.isAscii(domainName) and not sd.supportsUnicode(version):
-            raise se.UnicodeArgumentException()
-
-        # Create local path
-        mntPath = fileUtils.transformPath(remotePath)
-
-        mntPoint = os.path.join(cls.storage_repository,
-            sd.DOMAIN_MNT_POINT, mntPath)
-
-        cls._preCreateValidation(sdUUID, mntPoint, remotePath, storageType,
-                                 version)
-
-        domainDir = os.path.join(mntPoint, sdUUID)
-        cls._prepareMetadata(domainDir, sdUUID, domainName, domClass,
-                            remotePath, storageType, version)
-
-        # create domain images folder
-        imagesDir = os.path.join(domainDir, sd.DOMAIN_IMAGES)
-        oop.getProcessPool(sdUUID).fileUtils.createdir(imagesDir)
-
-        # create special imageUUID for ISO/Floppy volumes
-        if domClass is sd.ISO_DOMAIN:
-            isoDir = os.path.join(imagesDir, sd.ISO_IMAGE_UUID)
-            oop.getProcessPool(sdUUID).fileUtils.createdir(isoDir)
-
-        fsd = cls(os.path.join(mntPoint, sdUUID))
-        fsd.initSPMlease()
-
-        return fsd
-
-    def selftest(self):
-        """
-        Run internal self test
-        """
-        if not mount.isMounted(self.mountpoint):
-            raise se.StorageDomainFSNotMounted(self.mountpoint)
-
-        # Run general part of selftest
-        fileSD.FileStorageDomain.selftest(self)
-
-    @staticmethod
-    def findDomainPath(sdUUID):
-        for tmpSdUUID, domainPath in fileSD.scanDomains("*"):
-            if tmpSdUUID == sdUUID and mount.isMounted(
-                                             os.path.join(domainPath, "..")):
-                return domainPath
-
-        raise se.StorageDomainDoesNotExist(sdUUID)
-
-    def getRealPath(self):
-        try:
-            return mount.getMountFromTarget(self.mountpoint).fs_spec
-        except mount.MountError:
-            return ""
-
-
-def findDomain(sdUUID):
-    return NfsStorageDomain(NfsStorageDomain.findDomainPath(sdUUID))
+        mountableSD.MountableStorageDomain._preCreateValidation(sdUUID,
+                domPath, typeSpecificArg, version)
diff --git a/vdsm/storage/sdc.py b/vdsm/storage/sdc.py
index f2f4534..1064b5e 100644
--- a/vdsm/storage/sdc.py
+++ b/vdsm/storage/sdc.py
@@ -132,7 +132,7 @@
     def _findDomain(self, sdUUID):
         import blockSD
         import localFsSD
-        import nfsSD
+        import mountableSD
 
         # The order is somewhat important, it's ordered
         # by how quickly get can find the domain. For instance
@@ -140,7 +140,7 @@
         # until it times out, this should affect fetching
         # of block\local domains. If for any case in the future
         # this changes, please update the order.
-        for mod in (blockSD, localFsSD, nfsSD):
+        for mod in (blockSD, localFsSD, mountableSD):
             try:
                 return mod.findDomain(sdUUID)
             except se.StorageDomainDoesNotExist:


--
To view, visit http://gerrit.ovirt.org/9088
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic5788925f9c11b417aba713c652fc2a92f178830
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett <gpadgett(a)redhat.com&gt;

Greg Padgett has posted comments on this change.

Change subject: storage: refactor nfsSD.py for posix storage
......................................................................


Patch Set 1:

Here is the nfs/posixfs cleanup as promised in http://gerrit.ovirt.org/8525

--
To view, visit http://gerrit.ovirt.org/9088
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5788925f9c11b417aba713c652fc2a92f178830
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett <gpadgett(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Greg Padgett <gpadgett(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Itamar Heim has posted comments on this change.

Change subject: storage: refactor nfsSD.py for posix storage
......................................................................


Patch Set 1:

ping

-- 
To view, visit http://gerrit.ovirt.org/9088
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5788925f9c11b417aba713c652fc2a92f178830
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett <gpadgett(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Greg Padgett <gpadgett(a)redhat.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-HasComments: No

Greg Padgett has abandoned this change.

Change subject: storage: refactor nfsSD.py for posix storage
......................................................................


Abandoned

This patch is rather stale; we can revive it later if desired but it would need some
overhaul...

-- 
To view, visit http://gerrit.ovirt.org/9088
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: Ic5788925f9c11b417aba713c652fc2a92f178830
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett <gpadgett(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Greg Padgett <gpadgett(a)redhat.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;