[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 4: (8 inline comments)

....................................................
File scripts/ovirt-node-upgrade.py
Line 71:         self._logger = logging.getLogger(
Line 72:             '%s.%s' % (LOG_PREFIX, self.__class__.__name__))
Line 73: 
Line 74: 
Line 75: class LockFile(Base):
please add __enter__ and __exit__ so it can be used within with statement
Line 76: 
Line 77:     def __init__(self):
Line 78:         super(LockFile, self).__init__()
Line 79:         self.pidfile = None


Line 80:         self.oldpid = None
Line 81: 
Line 82:     def _acquire(self):
Line 83:         self._logger.info("Acquiring Lock")
Line 84:         if os.access(OVIRT_UPGRADE_LOCK, os.F_OK):
please use os,path.exists
Line 85:             self.pidfile = open(OVIRT_UPGRADE_LOCK, "r")
Line 86:             self.pidfile.seek(0)
Line 87:             self.oldpid = self.pidfile.readline()
Line 88:             if os.path.exists("/proc/%s" % self.oldpid):


Line 82:     def _acquire(self):
Line 83:         self._logger.info("Acquiring Lock")
Line 84:         if os.access(OVIRT_UPGRADE_LOCK, os.F_OK):
Line 85:             self.pidfile = open(OVIRT_UPGRADE_LOCK, "r")
Line 86:             self.pidfile.seek(0)
after open you do not need seek.

please use

 with open() as f:
Line 87:             self.oldpid = self.pidfile.readline()
Line 88:             if os.path.exists("/proc/%s" % self.oldpid):
Line 89:                 raise RuntimeError("You already have an instance of the" +
Line 90:                                    "program running")


Line 87:             self.oldpid = self.pidfile.readline()
Line 88:             if os.path.exists("/proc/%s" % self.oldpid):
Line 89:                 raise RuntimeError("You already have an instance of the" +
Line 90:                                    "program running")
Line 91:             else:
no need for else, we already discussed that 1000 times.
Line 92:                 self._logger.info("File is there but the program is not" +
Line 93:                                   "running")
Line 94:                 os.remove(OVIRT_UPGRADE_LOCK)
Line 95:         try:


Line 92:                 self._logger.info("File is there but the program is not" +
Line 93:                                   "running")
Line 94:                 os.remove(OVIRT_UPGRADE_LOCK)
Line 95:         try:
Line 96:             pidfile = open(OVIRT_UPGRADE_LOCK, "w")
you need to open other file and perform symlink.
Line 97:             pidfile.write("%s" % os.getpid())
Line 98:             pidfile.close
Line 99:         except:
Line 100:             raise RuntimeError("Unable to write lockfile")


Line 95:         try:
Line 96:             pidfile = open(OVIRT_UPGRADE_LOCK, "w")
Line 97:             pidfile.write("%s" % os.getpid())
Line 98:             pidfile.close
Line 99:         except:
please log original exception
Line 100:             raise RuntimeError("Unable to write lockfile")
Line 101: 
Line 102:     def _remove(self):
Line 103:         try:


Line 103:         try:
Line 104:             os.remove(OVIRT_UPGRADE_LOCK)
Line 105:         except:
Line 106:             raise RuntimeError("Unable to remove lockfile")
Line 107: 
shell code for that with wait, you can skip the wait.

        #!/bin/sh

        LOCK_REMOVE=""
        lock() {
                local file="$1"
                local timeout="$2"

                local mytmp="$(mktemp)"

                LOCK_REMOVE="${mytmp}"
                echo "$$" > "${mytmp}"

                while ! ln -s "${mytmp}" "${file}" > /dev/null 2>&1; do
                        sleep 1
                        timeout="$(($timeout-1))"
                        [ "${timeout}" = 0 ] && return 1

                        # link exists
                        if [ -L "${file}" ]; then
                                if [ -s "${file}" ]; then
                                        # check if dead process
                                        local curpid="$(<"${file}")"
                                        [ -d "/proc/${curpid}" ] || rm "${file}"
                                else
                                        # dead link
                                        rm -f "${file}"
                                fi
                        fi
                done

                LOCK_REMOVE="${LOCK_REMOVE}
        ${file}"
                return 0
        }

        unlock() {
                echo "${LOCK_REMOVE}" | while read f; do
                        rm -f "${f}"
                done
        }

        (
                trap unlock 0
                lock /tmp/mylock 2 || exit 1
                sleep 10
        )
Line 108: 
Line 109: class UpgradeTool(Base):
Line 110:     def __init__(self):
Line 111:         super(UpgradeTool, self).__init__()


Line 308:         self._parse_options()
Line 309:         self._logger.debug(self._options)
Line 310:         if os.geteuid() != 0:
Line 311:             raise RuntimeError("You must run as root")
Line 312:         self.lock._acquire()
please use with

 self.LockFile(file=xxxx):
     bla
Line 313:         if not self._options.iso_file:
Line 314:             raise RuntimeError("iso file not defined")
Line 315:         elif self._options.iso_file == "-":
Line 316:             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: 4
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>
Gerrit-Reviewer: Michael Burns <mburns at redhat.com>