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