Federico Simoncelli has uploaded a new change for review.
Change subject: storage: kill ongoing operations on exit ......................................................................
storage: kill ongoing operations on exit
Some storage operations must be forcibly interrupted when the vdsm (parent) process exits.
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=713434 Change-Id: Ic6ff67cb687494099e332d2d7b1eb3a293d1b4e4 Signed-off-by: Federico Simoncelli fsimonce@redhat.com --- M lib/vdsm/utils.py M vdsm/storage/blockSD.py M vdsm/storage/curlImgWrap.py M vdsm/storage/misc.py M vdsm/storage/sp.py M vdsm/storage/storage_mailbox.py M vdsm/storage/volume.py 7 files changed, 35 insertions(+), 21 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/74/18074/1
diff --git a/lib/vdsm/utils.py b/lib/vdsm/utils.py index 538498f..8890160 100644 --- a/lib/vdsm/utils.py +++ b/lib/vdsm/utils.py @@ -458,7 +458,8 @@
def execCmd(command, sudo=False, cwd=None, data=None, raw=False, logErr=True, printable=None, env=None, sync=True, nice=None, ioclass=None, - ioclassdata=None, setsid=False, execCmdLogger=logging.root): + ioclassdata=None, setsid=False, execCmdLogger=logging.root, + killOnExit=True): """ Executes an external command, optionally via sudo. """ @@ -485,7 +486,8 @@ cmdline = repr(subprocess.list2cmdline(printable)) execCmdLogger.debug("%s (cwd %s)", cmdline, cwd)
- p = BetterPopen(command, close_fds=True, cwd=cwd, env=env) + p = BetterPopen(command, close_fds=True, cwd=cwd, env=env, + deathSignal=(signal.SIGKILL if killOnExit else 0)) p = AsyncProc(p) if not sync: if data is not None: @@ -516,12 +518,14 @@
def watchCmd(command, stop, cwd=None, data=None, recoveryCallback=None, - nice=None, ioclass=None, execCmdLogger=logging.root): + nice=None, ioclass=None, execCmdLogger=logging.root, + killOnExit=False): """ Executes an external command, optionally via sudo with stop abilities. """ proc = execCmd(command, sudo=False, cwd=cwd, data=data, sync=False, - nice=nice, ioclass=ioclass, execCmdLogger=execCmdLogger) + nice=nice, ioclass=ioclass, execCmdLogger=execCmdLogger, + killOnExit=killOnExit) if recoveryCallback: recoveryCallback(proc)
diff --git a/vdsm/storage/blockSD.py b/vdsm/storage/blockSD.py index e8b2980..baca437 100644 --- a/vdsm/storage/blockSD.py +++ b/vdsm/storage/blockSD.py @@ -204,7 +204,8 @@ "of=%s" % lvm.lvPath(sdUUID, volUUID), "bs=%s" % BS, "count=%s" % count] p = misc.execCmd(cmd, sudo=False, sync=False, - nice=utils.NICENESS.HIGH, ioclass=utils.IOCLASS.IDLE) + nice=utils.NICENESS.HIGH, ioclass=utils.IOCLASS.IDLE, + killOnExit=True) return p
@@ -1084,7 +1085,7 @@ # If there is a journal already tune2fs will do nothing, indicating # this condition only with exit code. However, we do not really care. cmd = [constants.EXT_TUNE2FS, "-j", masterfsdev] - misc.execCmd(cmd, sudo=True) + misc.execCmd(cmd, sudo=True, killOnExit=True)
masterMount = mount.Mount(masterfsdev, masterDir)
@@ -1274,7 +1275,7 @@ Create a special file system to store VM data """ cmd = [constants.EXT_MKFS, "-q", "-j", "-E", "nodiscard", dev] - rc = misc.execCmd(cmd, sudo=True)[0] + rc = misc.execCmd(cmd, sudo=True, killOnExit=True)[0] if rc != 0: raise se.MkfsError(dev)
diff --git a/vdsm/storage/curlImgWrap.py b/vdsm/storage/curlImgWrap.py index 2f63311..e9b574e 100644 --- a/vdsm/storage/curlImgWrap.py +++ b/vdsm/storage/curlImgWrap.py @@ -63,7 +63,7 @@ cmd = [constants.EXT_CURL_IMG_WRAP, "--download"] cmd.extend(_headersToOptions(headers) + [path, url])
- rc, out, err = utils.execCmd(cmd) + rc, out, err = utils.execCmd(cmd, killOnExit=True)
if rc != 0: raise CurlError(rc, out, err) @@ -73,7 +73,7 @@ cmd = [constants.EXT_CURL_IMG_WRAP, "--upload"] cmd.extend(_headersToOptions(headers) + [path, url])
- rc, out, err = utils.execCmd(cmd) + rc, out, err = utils.execCmd(cmd, killOnExit=True)
if rc != 0: raise CurlError(rc, out, err) diff --git a/vdsm/storage/misc.py b/vdsm/storage/misc.py index fe3a870..013eded 100644 --- a/vdsm/storage/misc.py +++ b/vdsm/storage/misc.py @@ -367,12 +367,14 @@
if not stop: (rc, out, err) = execCmd(cmd, sudo=False, nice=utils.NICENESS.HIGH, - ioclass=utils.IOCLASS.IDLE) + ioclass=utils.IOCLASS.IDLE, + killOnExit=True) else: (rc, out, err) = watchCmd(cmd, stop=stop, recoveryCallback=recoveryCallback, nice=utils.NICENESS.HIGH, - ioclass=utils.IOCLASS.IDLE) + ioclass=utils.IOCLASS.IDLE, + killOnExit=True)
if rc: raise se.MiscBlockWriteException(dst, offset, size) diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py index 3f3c1bf..90c6313 100644 --- a/vdsm/storage/sp.py +++ b/vdsm/storage/sp.py @@ -2077,7 +2077,7 @@ try: if method.lower() == "wget": cmd = [constants.EXT_WGET, "-O", targetPath, srcPath] - (rc, out, err) = misc.execCmd(cmd, sudo=False) + (rc, out, err) = misc.execCmd(cmd, sudo=False, killOnExit=True)
if rc: self.log.error("uploadVolume - error while trying to " @@ -2086,7 +2086,7 @@ raise se.VolumeCopyError(vol, err) elif method.lower() == "rsync": cmd = [constants.EXT_RSYNC, "-aq", srcPath, targetPath] - (rc, out, err) = misc.execCmd(cmd, sudo=False) + (rc, out, err) = misc.execCmd(cmd, sudo=False, killOnExit=True)
if rc: self.log.error("uploadVolume - error while trying to copy:" diff --git a/vdsm/storage/storage_mailbox.py b/vdsm/storage/storage_mailbox.py index af3ccb8..38dff90 100644 --- a/vdsm/storage/storage_mailbox.py +++ b/vdsm/storage/storage_mailbox.py @@ -269,7 +269,8 @@
def _initMailbox(self): # Sync initial incoming mail state with storage view - (rc, out, err) = misc.execCmd(self._inCmd, sudo=False, raw=True) + (rc, out, err) = misc.execCmd(self._inCmd, sudo=False, raw=True, + killOnExit=True) if rc == 0: self._incomingMail = out self._init = True @@ -385,7 +386,8 @@ pChk = struct.pack('<l', chk) # Assumes CHECKSUM_BYTES equals 4!!! self._outgoingMail = \ self._outgoingMail[0:MAILBOX_SIZE - CHECKSUM_BYTES] + pChk - misc.execCmd(self._outCmd, data=self._outgoingMail, sudo=False) + misc.execCmd(self._outCmd, data=self._outgoingMail, sudo=False, + killOnExit=True)
def _handleMessage(self, message): # TODO: add support for multiple mailboxes @@ -569,7 +571,8 @@ self.log.debug("SPM_MailMonitor - clearing outgoing mail, command is: " "%s", self._outCmd) cmd = self._outCmd + ['bs=' + str(self._outMailLen)] - (rc, out, err) = misc.execCmd(cmd, sudo=False, data=self._outgoingMail) + (rc, out, err) = misc.execCmd(cmd, sudo=False, data=self._outgoingMail, + killOnExit=True) if rc: self.log.warning("SPM_MailMonitor couldn't clear outgoing mail, " "dd failed") @@ -742,7 +745,8 @@ try: cmd = self._outCmd + ['bs=' + str(self._outMailLen)] (rc, out, err) = misc.execCmd(cmd, sudo=False, - data=self._outgoingMail) + data=self._outgoingMail, + killOnExit=True) if rc: self.log.warning("SPM_MailMonitor couldn't write " "outgoing mail, dd failed") @@ -767,7 +771,8 @@ 'seek=' + str(mailboxOffset / MAILBOX_SIZE)] #self.log.debug("Running command: %s, for message id: %s", # str(cmd), str(msgID)) - (rc, out, err) = misc.execCmd(cmd, sudo=False, data=mailbox) + (rc, out, err) = misc.execCmd(cmd, sudo=False, data=mailbox, + killOnExit=True) if rc: self.log.error("SPM_MailMonitor: sendReply - couldn't send " "reply, dd failed") diff --git a/vdsm/storage/volume.py b/vdsm/storage/volume.py index 10b3363..3c3b378 100644 --- a/vdsm/storage/volume.py +++ b/vdsm/storage/volume.py @@ -1057,7 +1057,7 @@ # +1 is so that odd numbers will round upwards. cmd += [volume, "%uK" % ((size + 1) / 2)]
- (rc, out, err) = misc.execCmd(cmd, sudo=False, cwd=cwd) + (rc, out, err) = misc.execCmd(cmd, sudo=False, cwd=cwd, killOnExit=True) if rc: raise se.VolumeCreationError(out) return True @@ -1096,7 +1096,8 @@ (rc, out, err) = misc.watchCmd(cmd, stop=stop, cwd=cwd, recoveryCallback=recoveryCallback, ioclass=utils.IOCLASS.IDLE, - nice=utils.NICENESS.HIGH) + nice=utils.NICENESS.HIGH, + killOnExit=True)
log.debug('(qemuRebase): REBASE %s DONE' % (src)) return (rc, out, err) @@ -1124,7 +1125,8 @@ (rc, out, err) = misc.watchCmd(cmd, stop=stop, recoveryCallback=baseAsyncTasksRollback, ioclass=utils.IOCLASS.IDLE, - nice=utils.NICENESS.HIGH) + nice=utils.NICENESS.HIGH, + killOnExit=True)
log.debug('(qemuConvert): COPY %s to %s DONE' % (src, dst)) return (rc, out, err)
oVirt Jenkins CI Server has posted comments on this change.
Change subject: storage: kill ongoing operations on exit ......................................................................
Patch Set 1:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3933/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3850/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3044/ : SUCCESS
Ayal Baron has posted comments on this change.
Change subject: storage: kill ongoing operations on exit ......................................................................
Patch Set 1:
(1 comment)
.................................................... File lib/vdsm/utils.py Line 517: return [l[:-1] if l.endswith('\n') else l for l in lines] Line 518: Line 519: Line 520: def watchCmd(command, stop, cwd=None, data=None, recoveryCallback=None, Line 521: nice=None, ioclass=None, execCmdLogger=logging.root, why is the default here False ? Line 522: killOnExit=False): Line 523: """ Line 524: Executes an external command, optionally via sudo with stop abilities. Line 525: """
Federico Simoncelli has posted comments on this change.
Change subject: storage: kill ongoing operations on exit ......................................................................
Patch Set 1:
(1 comment)
.................................................... File lib/vdsm/utils.py Line 517: return [l[:-1] if l.endswith('\n') else l for l in lines] Line 518: Line 519: Line 520: def watchCmd(command, stop, cwd=None, data=None, recoveryCallback=None, Line 521: nice=None, ioclass=None, execCmdLogger=logging.root, right, but the other mistake is in 'execCmd' where it should be False by default. Line 522: killOnExit=False): Line 523: """ Line 524: Executes an external command, optionally via sudo with stop abilities. Line 525: """
oVirt Jenkins CI Server has posted comments on this change.
Change subject: storage: kill ongoing operations on exit ......................................................................
Patch Set 2:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4019/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3130/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3937/ : SUCCESS
Ayal Baron has posted comments on this change.
Change subject: storage: kill ongoing operations on exit ......................................................................
Patch Set 2:
(4 comments)
.................................................... File vdsm/storage/blockSD.py Line 1059: fileUtils.createdir(masterDir) Line 1060: Line 1061: masterfsdev = lvm.lvPath(self.sdUUID, MASTERLV) Line 1062: cmd = [constants.EXT_FSCK, "-p", masterfsdev] Line 1063: (rc, out, err) = misc.execCmd(cmd, sudo=True) why not kill on exit here as well? Line 1064: # fsck exit codes Line 1065: # 0 - No errors Line 1066: # 1 - File system errors corrected Line 1067: # 2 - File system errors corrected, system should
Line 1096: raise se.BlockStorageDomainMasterMountError(masterfsdev, rc, out) Line 1097: Line 1098: cmd = [constants.EXT_CHOWN, "%s:%s" % Line 1099: (constants.METADATA_USER, constants.METADATA_GROUP), masterDir] Line 1100: (rc, out, err) = misc.execCmd(cmd, sudo=True) and here Line 1101: if rc != 0: Line 1102: self.log.error("failed to chown %s", masterDir) Line 1103: Line 1104: @classmethod
.................................................... File vdsm/storage/sp.py Line 2076: vol.prepare(rw=True, setrw=False) Line 2077: try: Line 2078: if method.lower() == "wget": Line 2079: cmd = [constants.EXT_WGET, "-O", targetPath, srcPath] Line 2080: (rc, out, err) = misc.execCmd(cmd, sudo=False, killOnExit=True) I wonder if we should wrap execCmd in sp.py to change the default. Line 2081: Line 2082: if rc: Line 2083: self.log.error("uploadVolume - error while trying to " Line 2084: "retrieve: %s into: %s, stderr: %s" %
.................................................... File vdsm/storage/storage_mailbox.py Line 268: self.start() Line 269: Line 270: def _initMailbox(self): Line 271: # Sync initial incoming mail state with storage view Line 272: (rc, out, err) = misc.execCmd(self._inCmd, sudo=False, raw=True, here also I wonder if we shouldn't just wrap execCmd Line 273: killOnExit=True) Line 274: if rc == 0: Line 275: self._incomingMail = out Line 276: self._init = True
Federico Simoncelli has posted comments on this change.
Change subject: storage: kill ongoing operations on exit ......................................................................
Patch Set 2:
(4 comments)
.................................................... File vdsm/storage/blockSD.py Line 1059: fileUtils.createdir(masterDir) Line 1060: Line 1061: masterfsdev = lvm.lvPath(self.sdUUID, MASTERLV) Line 1062: cmd = [constants.EXT_FSCK, "-p", masterfsdev] Line 1063: (rc, out, err) = misc.execCmd(cmd, sudo=True) I can't recall what was the reasoning here. Probably I was just dubious about SIGKILLing a fsck process.
As far as we know at the moment we never had a corruption here (even if it is feasible). We don't know how's going to be SIGKILLing the process (even though it's correct).
Anyway it makes sense to hope that fsck is robust enough to handle a SIGKILL, crash, power down, etc. Line 1064: # fsck exit codes Line 1065: # 0 - No errors Line 1066: # 1 - File system errors corrected Line 1067: # 2 - File system errors corrected, system should
Line 1096: raise se.BlockStorageDomainMasterMountError(masterfsdev, rc, out) Line 1097: Line 1098: cmd = [constants.EXT_CHOWN, "%s:%s" % Line 1099: (constants.METADATA_USER, constants.METADATA_GROUP), masterDir] Line 1100: (rc, out, err) = misc.execCmd(cmd, sudo=True) This is a chown only (really short). We might rely on this to complete regardless. Anyway there's no race here (worst case chown will fail if the file/dir has been removed). Line 1101: if rc != 0: Line 1102: self.log.error("failed to chown %s", masterDir) Line 1103: Line 1104: @classmethod
.................................................... File vdsm/storage/sp.py Line 2076: vol.prepare(rw=True, setrw=False) Line 2077: try: Line 2078: if method.lower() == "wget": Line 2079: cmd = [constants.EXT_WGET, "-O", targetPath, srcPath] Line 2080: (rc, out, err) = misc.execCmd(cmd, sudo=False, killOnExit=True) As far as the current code is concerned, uploadVolume looks like a special and obsolete verb. It's the only one doing something directly (execCmd). I wouldn't mind leaving it as it is and even remove it in the future. I don't think sp.py is the place to have a special (wrapper) execCmd. Line 2081: Line 2082: if rc: Line 2083: self.log.error("uploadVolume - error while trying to " Line 2084: "retrieve: %s into: %s, stderr: %s" %
.................................................... File vdsm/storage/storage_mailbox.py Line 268: self.start() Line 269: Line 270: def _initMailbox(self): Line 271: # Sync initial incoming mail state with storage view Line 272: (rc, out, err) = misc.execCmd(self._inCmd, sudo=False, raw=True, Done Line 273: killOnExit=True) Line 274: if rc == 0: Line 275: self._incomingMail = out Line 276: self._init = True
oVirt Jenkins CI Server has posted comments on this change.
Change subject: storage: kill ongoing operations on exit ......................................................................
Patch Set 3:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4178/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3283/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4099/ : SUCCESS
Ayal Baron has posted comments on this change.
Change subject: storage: kill ongoing operations on exit ......................................................................
Patch Set 3: Code-Review+2
Federico Simoncelli has posted comments on this change.
Change subject: storage: kill ongoing operations on exit ......................................................................
Patch Set 3: Verified+1
Verified using this patch regularly for more than a week. I also verified that a (long) running dd command (zeroImage) is now terminated when vdsm is killed.
Saggi Mizrahi has posted comments on this change.
Change subject: storage: kill ongoing operations on exit ......................................................................
Patch Set 3: Code-Review-1
(2 comments)
.................................................... File lib/vdsm/utils.py Line 458: Line 459: def execCmd(command, sudo=False, cwd=None, data=None, raw=False, logErr=True, Line 460: printable=None, env=None, sync=True, nice=None, ioclass=None, Line 461: ioclassdata=None, setsid=False, execCmdLogger=logging.root, Line 462: killOnExit=False): instead of killOnExist change to deathSignal
Also deathsignal is sent when the current thread dies and not when the process dies. I would rather you block the use of sync=false and a death signal together to prevent people from making stupid mistakes. Line 463: """ Line 464: Executes an external command, optionally via sudo. Line 465: """ Line 466: if ioclass is not None:
.................................................... File vdsm/storage/storage_mailbox.py Line 79: vars.task = ctask Line 80: ctask.prepare(cmd, *args) Line 81: Line 82: Line 83: def mboxExecCmd(*args, **kwargs): I assume this is module private _mbox... Line 84: kwargs['killOnExit'] = True Line 85: return misc.execCmd(*args, **kwargs) Line 86: Line 87:
Federico Simoncelli has posted comments on this change.
Change subject: storage: kill ongoing operations on exit ......................................................................
Patch Set 3:
(1 comment)
.................................................... File lib/vdsm/utils.py Line 458: Line 459: def execCmd(command, sudo=False, cwd=None, data=None, raw=False, logErr=True, Line 460: printable=None, env=None, sync=True, nice=None, ioclass=None, Line 461: ioclassdata=None, setsid=False, execCmdLogger=logging.root, Line 462: killOnExit=False): Why do you think "death" is more thread-specific than "exit"? After all the pthread lib has pthread_exit not pthread_die. If you think that "death" is better because it both includes exits and, well, process deaths (crashes) then we could consider:
* killOnDeath * stopOnExit/stopOnDeath * terminateOnExit/terminateOnDeath
Anyway my point here is that "deathSignal" really looks confusing (let's leave it for BetterPopen where in fact we specify a signal to use). Line 463: """ Line 464: Executes an external command, optionally via sudo. Line 465: """ Line 466: if ioclass is not None:
Saggi Mizrahi has posted comments on this change.
Change subject: storage: kill ongoing operations on exit ......................................................................
Patch Set 3:
(1 comment)
.................................................... File lib/vdsm/utils.py Line 458: Line 459: def execCmd(command, sudo=False, cwd=None, data=None, raw=False, logErr=True, Line 460: printable=None, env=None, sync=True, nice=None, ioclass=None, Line 461: ioclassdata=None, setsid=False, execCmdLogger=logging.root, Line 462: killOnExit=False): death is not more thread spcific. I spoke about two things
1. Let the user pass the signal since we might want to pass different signals to other processes 2. Make sure you can't specify this when sync=False Line 463: """ Line 464: Executes an external command, optionally via sudo. Line 465: """ Line 466: if ioclass is not None:
Federico Simoncelli has posted comments on this change.
Change subject: storage: kill ongoing operations on exit ......................................................................
Patch Set 4: Verified+1
Quickly re-verified.
oVirt Jenkins CI Server has posted comments on this change.
Change subject: storage: kill ongoing operations on exit ......................................................................
Patch Set 4: Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4266/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3369/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4185/ : SUCCESS
Federico Simoncelli has posted comments on this change.
Change subject: storage: kill ongoing operations on exit ......................................................................
Patch Set 5: Verified+1
Minor change
oVirt Jenkins CI Server has posted comments on this change.
Change subject: storage: kill ongoing operations on exit ......................................................................
Patch Set 5:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4267/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3370/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4186/ : SUCCESS
Saggi Mizrahi has posted comments on this change.
Change subject: storage: kill ongoing operations on exit ......................................................................
Patch Set 5: Code-Review+2
Dan Kenigsberg has submitted this change and it was merged.
Change subject: storage: kill ongoing operations on exit ......................................................................
storage: kill ongoing operations on exit
Some storage operations must be forcibly interrupted when the vdsm (parent) process exits.
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=713434 Change-Id: Ic6ff67cb687494099e332d2d7b1eb3a293d1b4e4 Signed-off-by: Federico Simoncelli fsimonce@redhat.com Reviewed-on: http://gerrit.ovirt.org/18074 Reviewed-by: Saggi Mizrahi smizrahi@redhat.com --- M lib/vdsm/utils.py M vdsm/storage/blockSD.py M vdsm/storage/curlImgWrap.py M vdsm/storage/misc.py M vdsm/storage/sp.py M vdsm/storage/storage_mailbox.py M vdsm/storage/volume.py 7 files changed, 41 insertions(+), 19 deletions(-)
Approvals: Saggi Mizrahi: Looks good to me, approved Federico Simoncelli: Verified
vdsm-patches@lists.fedorahosted.org