[node-patches] Change in ovirt-node[master]: ovirt-node-upgrade: add lock file check to prevent parallel ...

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: ovirt-node-upgrade: add lock file check to prevent parallel runs
......................................................................


Patch Set 5: (7 inline comments)

....................................................
File scripts/ovirt-node-upgrade.py
Line 81:         lock_fd, self._lock_file = tempfile.mkstemp(prefix=".ovirtupgrade.")
Line 82:         os.close(lock_fd)
Line 83: 
Line 84:     def __enter__(self):
Line 85:         return
here you should lock.
Line 86: 
Line 87:     def __exit__(self, exc_type, exc_value, traceback):
Line 88:         return
Line 89: 


Line 84:     def __enter__(self):
Line 85:         return
Line 86: 
Line 87:     def __exit__(self, exc_type, exc_value, traceback):
Line 88:         return
here you should unlock.
Line 89: 
Line 90:     def _acquire(self):
Line 91:         self._logger.info("Acquiring Lock")
Line 92:         if os.path.exists(OVIRT_UPGRADE_LOCK):


Line 100:             pidfile = open(self._lock_file, "w")
Line 101:             pidfile.write("%s" % os.getpid())
Line 102:             pidfile.close
Line 103:             os.symlink(self._lock_file, OVIRT_UPGRADE_LOCK)
Line 104:         except Exception as e:
you should remove your self._lock_file here
Line 105:             self._logger.exception('Error: Upgrade Failed: %s', e)
Line 106:             raise RuntimeError("Unable to write lockfile")
Line 107: 
Line 108:     def _remove(self):


Line 106:             raise RuntimeError("Unable to write lockfile")
Line 107: 
Line 108:     def _remove(self):
Line 109:         try:
Line 110:             os.remove(OVIRT_UPGRADE_LOCK)
if os.path.exists...
Line 111:             os.remove(self._lock_file)
Line 112:         except Exception as e:
Line 113:             self._logger.exception('Error: Upgrade Failed: %s', e)
Line 114:             raise RuntimeError("Unable to remove lockfile")


Line 120:         self._options = None
Line 121:         self._python_lib = None
Line 122:         self._tmp_python_path = None
Line 123:         self.iso_tmp = None
Line 124:         self.lock = LockFile()
no need.
Line 125:         self._tmp_dir = tempfile.mkdtemp(dir="/data")
Line 126:         self._chroot_path = os.path.join(self._tmp_dir, "rootfs")
Line 127:         self._squashfs_dir = os.path.join(self._tmp_dir, "squashfs")
Line 128:         self._ovirtnode_dir = os.path.join(self._tmp_dir, "ovirtnode")


Line 248:         try:
Line 249:             for dir in [self._chroot_path, self._squashfs_dir, "/live"]:
Line 250:                 self._system(which("umount"), dir)
Line 251:             shutil.rmtree(self._tmp_dir)
Line 252:             self.lock._remove()
no need.
Line 253:             if self.iso_tmp and os.path.exists(self.iso_tmp):
Line 254:                 os.remove(self.iso_tmp)
Line 255:         except:
Line 256:             self._logger.warning("Cleanup Failed")


Line 318:         if os.geteuid() != 0:
Line 319:             raise RuntimeError("You must run as root")
Line 320: 
Line 321:         with LockFile():
Line 322:             self.lock._acquire()
The with statement should have acquired the lock. No need for this statement.
Line 323:             if not self._options.iso_file:
Line 324:                 raise RuntimeError("iso file not defined")
Line 325:             elif self._options.iso_file == "-":
Line 326:                 iso_fd, self._options.iso_file = tempfile.mkstemp(


-- 
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: 5
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: Fabian Deutsch <fabiand at fedoraproject.org>
Gerrit-Reviewer: Joey Boggs <jboggs at redhat.com>
Gerrit-Reviewer: Michael Burns <mburns at redhat.com>