Ayal Baron has posted comments on this change.
Change subject: Prefetch domains when connecting a storage server.
......................................................................
Patch Set 6: (5 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):
What you said has absolutely nothing to do with my comment.
direct files will either require a 'file' type connect so won't go in this 'if'
or no connect and still not go here.
Again, I see no reason to prefetch on block domains.
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 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()
No, in composition it is common to wrap and expose methods of the internal
objects as is already done there. It's adding another such method.
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))
?
I have no idea what this has to do with domain definition, but what I'm saying
here would unify the code to look the same for all types and get rid of most of
the differences in this flow...
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
a different path than what? point was that getLocalPath in
LocalDirectoryConnection would return the correct path and then you can unify
code paths here.
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)
It's impossible to determine that, it goes through too many sub functions.
You're adding logic here that is not strictly related to the connect and you'd
fail connect on it. That is not acceptable.
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
