[node-patches] Change in ovirt-node[master]: ovirt-node-upgrade: add lock file check to prevent parallel ...
Alon Bar-Lev
alonbl at redhat.com
Wed May 29 21:23:15 UTC 2013
Alon Bar-Lev has posted comments on this change.
Change subject: ovirt-node-upgrade: add lock file check to prevent parallel runs
......................................................................
Patch Set 2: (12 inline comments)
What I expect is:
with LockFile(LOCKFILE):
do logic
At entry LockFile class should acquire the lock or exit with exception if lock exist and pid within the lock refer to running process, if not remove the LOCKFILE.
To make it atomic, you write the pid to LOCKFILE.pid and try to create symlink to LOCKFILE, if success you got the lock.
At exit remove LOCKFILE and LOCKFILE.pid.
....................................................
File scripts/ovirt-node-upgrade.py
Line 66: console.setFormatter(conformatter)
Line 67: logger.addHandler(console)
Line 68:
Line 69:
Line 70: def read_pid_from_pidfile(pidfile_path):
please move all these function to an object PidFile or similar, inherit from Base, so you have logger.
Line 71: pid = None
Line 72: try:
Line 73: pidfile = open(pidfile_path, 'r')
Line 74: except IOError:
Line 69:
Line 70: def read_pid_from_pidfile(pidfile_path):
Line 71: pid = None
Line 72: try:
Line 73: pidfile = open(pidfile_path, 'r')
should be:
pid = None
try:
with open(pidfile_path, 'r') as pid_file:
pid = int(pidfile.readline())
except:
self.logger.debug('exception', exc_info=True)
return pid
Line 74: except IOError:
Line 75: pass
Line 76: else:
Line 77: line = pidfile.readline().strip()
Line 92:
Line 93: pid = os.getpid()
Line 94: line = "%(pid)d\n" % vars()
Line 95: pidfile.write(line)
Line 96: pidfile.close()
Hmmm... why not just:
with open(pidfile_path, 'w') as f:
f.write('{pid}\n'.format(pid=os.getpid())
Line 97:
Line 98:
Line 99: def remove_existing_pidfile(pidfile_path):
Line 100: try:
Line 95: pidfile.write(line)
Line 96: pidfile.close()
Line 97:
Line 98:
Line 99: def remove_existing_pidfile(pidfile_path):
Hmmm....
if os.path.exists(pidfile_path):
os.unlink(pidfile_path)
Line 100: try:
Line 101: os.remove(pidfile_path)
Line 102: except OSError as exc:
Line 103: if exc.errno == errno.ENOENT:
Line 111: self._logger = logging.getLogger(
Line 112: '%s.%s' % (LOG_PREFIX, self.__class__.__name__))
Line 113:
Line 114:
Line 115: class LockFile():
inherit from Base
Line 116: def __init__(self):
Line 117: self.timeout = 5
Line 118: self.path = OVIRT_LOCKFILE
Line 119:
Line 112: '%s.%s' % (LOG_PREFIX, self.__class__.__name__))
Line 113:
Line 114:
Line 115: class LockFile():
Line 116: def __init__(self):
If you have object, please accept setting via arguments, constants should be used by the caller.
Line 117: self.timeout = 5
Line 118: self.path = OVIRT_LOCKFILE
Line 119:
Line 120: def read_pid(self):
Line 116: def __init__(self):
Line 117: self.timeout = 5
Line 118: self.path = OVIRT_LOCKFILE
Line 119:
Line 120: def read_pid(self):
why the duplication?
Line 121: return read_pid_from_pidfile(self.path)
Line 122:
Line 123: def is_locked(self):
Line 124: return os.path.exists(self.path)
Line 119:
Line 120: def read_pid(self):
Line 121: return read_pid_from_pidfile(self.path)
Line 122:
Line 123: def is_locked(self):
this does not mean that it is locked.
Line 124: return os.path.exists(self.path)
Line 125:
Line 126: def i_am_locking(self):
Line 127: return self.is_locked() and os.getpid() == self.read_pid()
Line 122:
Line 123: def is_locked(self):
Line 124: return os.path.exists(self.path)
Line 125:
Line 126: def i_am_locking(self):
I don't see the need...
Line 127: return self.is_locked() and os.getpid() == self.read_pid()
Line 128:
Line 129: def acquire(self, timeout=None):
Line 130: timeout = self.timeout
Line 133: end_time += timeout
Line 134:
Line 135: while True:
Line 136: try:
Line 137: write_pid_to_pidfile(self.path)
just write? without check?
Line 138: except OSError as exc:
Line 139: if exc.errno == errno.EEXIST:
Line 140: # The lock creation failed. Maybe sleep a bit.
Line 141: if timeout is not None and time.time() > end_time:
Line 136: try:
Line 137: write_pid_to_pidfile(self.path)
Line 138: except OSError as exc:
Line 139: if exc.errno == errno.EEXIST:
Line 140: # The lock creation failed. Maybe sleep a bit.
I don't understand when it can fail, and waiting will be good, you should not assume O_EXCL is working.
Line 141: if timeout is not None and time.time() > end_time:
Line 142: if timeout > 0:
Line 143: raise RuntimeError("Timeout waiting to acquire"
Line 144: " lock for %s" %
Line 286: self._logger.info("Cleaning up temporary directory")
Line 287: try:
Line 288: for dir in [self._chroot_path, self._squashfs_dir, "/live"]:
Line 289: self._system(which("umount"), dir)
Line 290: remove_existing_pidfile(OVIRT_LOCKFILE)
you should decide if you work with pid or lock interface, don't mix up.
Line 291: shutil.rmtree(self._tmp_dir)
Line 292: except:
Line 293: self._logger.warning("Cleanup Failed")
Line 294: self._logger.debug('exception', exc_info=True)
--
To view, visit http://gerrit.ovirt.org/14960
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifdd6553046d99cf9c9db29c1c899dc93e8ad733a
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: Joey Boggs <jboggs at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Joey Boggs <jboggs at redhat.com>
More information about the node-patches
mailing list