Ayal Baron has posted comments on this change.

Change subject: Prefetch domains when connecting a storage server.
......................................................................


Patch Set 6: I would prefer that you didn't submit this

(6 inline comments)

....................................................
File vdsm/storage/hsm.py
Line 2238: 
Line 2239:     def __prefetchDomains(self, domType, conObj):
Line 2240:         uuidPatern = "????????-????-????-????-????????????"
Line 2241: 
Line 2242:         if domType in (sd.FCP_DOMAIN, sd.ISCSI_DOMAIN):
1. s/.*/if domType in sd.BLOCK_DOMAIN_TYPES:/

2. connectStorageServer is not called for FCP so need to make sure this won't 
mess anything up.

3. in case of iSCSI for direct lun this will cause a vgs for every connection 
even though we don't even expect there to be a storage domain there.

Because of 2,3, I think that we should not prefetch for block domains at all 
and then we'd have:
if domType in sd.FILE_DOMAIN_TYPES:
   ...
else:
   ...

And significantly simplify the code here.
Line 2243:             uuids = tuple(blockSD.getStorageDomainsList())
Line 2244:         elif domType is sd.NFS_DOMAIN:
Line 2245:             lPath = conObj._mountCon._getLocalPath()
Line 2246:             self.log.debug("nfs local path: %s", lPath)


Line 2240:         uuidPatern = "????????-????-????-????-????????????"
Line 2241: 
Line 2242:         if domType in (sd.FCP_DOMAIN, sd.ISCSI_DOMAIN):
Line 2243:             uuids = tuple(blockSD.getStorageDomainsList())
Line 2244:         elif domType is sd.NFS_DOMAIN:
elif domType in FILE_DOMAIN_TYPES: (see below for comments on this)
Line 2245:             lPath = conObj._mountCon._getLocalPath()
Line 2246:             self.log.debug("nfs local path: %s", lPath)
Line 2247:             goop = oop.getGlobalProcPool()
Line 2248:             uuids = tuple(os.path.basename(d) for d in


Line 2241: 
Line 2242:         if domType in (sd.FCP_DOMAIN, sd.ISCSI_DOMAIN):
Line 2243:             uuids = tuple(blockSD.getStorageDomainsList())
Line 2244:         elif domType is sd.NFS_DOMAIN:
Line 2245:             lPath = conObj._mountCon._getLocalPath()
instead of going into _mountCon, expose a getLocalPath method in NFSConnection 
(and make the one in LocalDirectoryConnection public).
Line 2246:             self.log.debug("nfs local path: %s", lPath)
Line 2247:             goop = oop.getGlobalProcPool()
Line 2248:             uuids = tuple(os.path.basename(d) for d in
Line 2249:                           goop.glob.glob(os.path.join(lPath, 
uuidPatern)))


Line 2256:         elif domType is sd.GLUSTERFS_DOMAIN:
Line 2257:             glusterDomPath = os.path.join(sd.GLUSTERSD_DIR, "*")
Line 2258:             self.log.debug("glusterDomPath: %s", glusterDomPath)
Line 2259:             uuids = tuple(sdUUID for sdUUID, domainPath in
Line 2260:                           nfsSD.fileSD.scanDomains(glusterDomPath))
why are we scanning here and not using the path same as above?
if we need to do this differently then I see no reason to get fileSD from nfsSD 
and not import fileSD here explicitly.
Line 2261:         elif domType is sd.LOCALFS_DOMAIN:
Line 2262:             lPath = conObj._path
Line 2263:             self.log.debug("local _path: %s", lPath)
Line 2264:             uuids = tuple(os.path.basename(d) for d in


Line 2258:             self.log.debug("glusterDomPath: %s", glusterDomPath)
Line 2259:             uuids = tuple(sdUUID for sdUUID, domainPath in
Line 2260:                           nfsSD.fileSD.scanDomains(glusterDomPath))
Line 2261:         elif domType is sd.LOCALFS_DOMAIN:
Line 2262:             lPath = conObj._path
use same getLocalPath here.
also use goop and make all these cases exactly the same and collapse them into 
1.
Line 2263:             self.log.debug("local _path: %s", lPath)
Line 2264:             uuids = tuple(os.path.basename(d) for d in
Line 2265:                           glob.glob(os.path.join(lPath, uuidPatern)))
Line 2266:         else:


Line 2307:                     "Could not connect to storageServer", 
exc_info=True)
Line 2308:                 status, _ = self._translateConnectionError(err)
Line 2309:             else:
Line 2310:                 status = 0
Line 2311:                 doms = self.__prefetchDomains(domType, conObj)
1. failure here should not cause connect method to fail as connection has 
already succeeded.

2. domType should probably be renamed connType since this can be used for 
non-domain related purposes as well (e.g. direct lun).
Line 2312:                 sdCache.knownSDs.update(doms)
Line 2313: 
Line 2314:             self.log.debug("knownSDs: %s", sdCache.knownSDs)
Line 2315: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8f2b19a566c659bac30a318d17cef929e0b2575b
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewars...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Daniel Paikov <pai...@gmail.com>
Gerrit-Reviewer: Eduardo <ewars...@redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Haim Ateya <hat...@redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykap...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to