Change in vdsm[master]: hba: Rescan using SCSI layer

smizrahi at redhat.com smizrahi at redhat.com
Sun Nov 23 21:06:19 UTC 2014


Saggi Mizrahi has posted comments on this change.

Change subject: hba: Rescan using SCSI layer
......................................................................


Patch Set 5: Code-Review-1

(5 comments)

http://gerrit.ovirt.org/#/c/34245/5/vdsm/storage/fc-scan
File vdsm/storage/fc-scan:

Line 63:     def run(self):
Line 64:         try:
Line 65:             path = "/sys/class/scsi_host/%s/scan" % self.host
Line 66:             log.debug("Scanning %s", path)
Line 67:             start = time.time()
use the monotnic system timer (os.times()[4])
Line 68:             fd = os.open(path, os.O_WRONLY)
Line 69:             try:
Line 70:                 os.write(fd, "- - -")
Line 71:             finally:


Line 66:             log.debug("Scanning %s", path)
Line 67:             start = time.time()
Line 68:             fd = os.open(path, os.O_WRONLY)
Line 69:             try:
Line 70:                 os.write(fd, "- - -")
> If I recall correctly, we have seen this write causing the process to get i
Depends on the python version. There isn't really a way of knowing what would happen on stuck IO with python.

In any case, what we loose is this process and all open FDs.
Assuming os.write() releases the GIL we would at least close all other files. The current version of os.write() in 2.7 at least releases the GIL during the write so we should be OK.
Line 71:             finally:
Line 72:                 os.close(fd)
Line 73:             self.succeeded = True
Line 74:             elapsed = time.time() - start


Line 81: 
Line 82: def main(args):
Line 83:     if '-h' in args:
Line 84:         print __doc__
Line 85:         sys.exit(0)
I much rather having main() return a value that you
pass to sys.exit().

Makes it look more standard and has the added benefit of doing sys.exit() in a single place.
Line 86: 
Line 87:     logging.basicConfig(
Line 88:         level=logging.DEBUG if '-v' in args else logging.INFO,
Line 89:         format="%(name)s: %(message)s")


Line 87:     logging.basicConfig(
Line 88:         level=logging.DEBUG if '-v' in args else logging.INFO,
Line 89:         format="%(name)s: %(message)s")
Line 90: 
Line 91:     if os.geteuid() != 0:
Don't check for that. We'll just get let it return permission denied. Assuming that you have to be root doesn't add anything and might cause us problem if we ever use kernel capabilities(7).
Line 92:         log.error("Must run as root")
Line 93:         sys.exit(2)
Line 94: 
Line 95:     hosts = [os.path.basename(path)


http://gerrit.ovirt.org/#/c/34245/5/vdsm/storage/hba.py
File vdsm/storage/hba.py:

Line 50:     """
Line 51:     timeout = config.getint('irs', 'scsi_rescan_maximal_timeout')
Line 52: 
Line 53:     log.debug("Starting scan")
Line 54:     proc = utils.execCmd([constants.EXT_FC_SCAN], sync=False, sudo=True,
We are trying to stop using SUDO.
I know a lot of things still use it but please don't add more
stuff that use it.

Have supervdsm run that command.
Line 55:                          execCmdLogger=log)
Line 56:     try:
Line 57:         proc.wait(timeout)
Line 58:     finally:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7deb3047f1b75c4c65d59602b908835515290993
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list