Zhou Zheng Sheng has posted comments on this change.
Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................
Patch Set 10: I would prefer that you didn't submit this
(2 inline comments)
A cleaner solution maybe to move all the VM related runtime files into a sub-dir. It's
OK if you keep the current solution, just make sure we do not delete unrelated files.
....................................................
File vdsm/clientIF.py
Line 466: orphanVMFile = lambda vmFileName: vmFileName.split('.', 1)[0] \
Line 467: not in self.vmContainer
Line 468: dontDel = ("vdsmd.pid", "respawn.pid",
"svdsm.pid")
Line 469:
Line 470: fNames = tuple(fName for fName in os.listdir(constants.P_VDSM_RUN)
Saggi's idea is good, but I think the resulting set of files to delete is too big.
toDel = set(os.listdir(constants.P_VDSM_RUN))
The above will include all the sub-dirs, "lvm.env" and "svdsm.time"
and other files if added by other programmer in future. Files like "lvm.env" and
"svdsm.time" will be deleted, and unlink a sub-dir like "lvm" will
raise OSError. _cleanOldFiles is called by _recoverExistingVms, it should not delete any
files other than old VM related ones. Programmers have to remember to update the white
list if they add files under constants.P_VDSM_RUN, and this is not obvious because the
white list is hidden in the VM recovery logic. It's error prone.
In the original code, files like "svdsm.time" will not be deleted because its
extension name is not in ["guest.socket", "monitor.socket",
"pid", "stdio.dump", "recovery"], but "x.pid" will
be deleted if we forget to skip it.
I think "toDel" can be calculated as follow
hex = "[0-9a-f]"
# pattern for uuid
vmidPattern = "-".join([hex * 8] + [hex * 4] * 3 + [hex * 12])
knownExt = [".guest.socket", ".pid", ".recovery"]
patterns = [id + ext for id in [vmidPattern] for ext in knownExt]
toDel = set(f for f in os.listdir('/var/run/vdsm') if
any(fnmatch.fnmatch(f, pattern) for pattern in patterns) and
orphanVMFile(f))
or
hex = "[0-9a-f]"
# pattern for uuid
vmidPattern = "-".join([hex * 8] + [hex * 4] * 3 + [hex * 12])
knownExt = [".guest.socket", ".pid", ".recovery"]
patterns = [id + ext for id in [vmidPattern] for ext in knownExt]
toDel = set(f for f in os.listdir('/var/run/vdsm') if
any(fnmatch.fnmatch(f, pattern) for pattern in patterns))
for vm in self.vmContainer:
for suffix in knownExt:
toDel.discard(vm + suffix)
Now we do not have to maintain a white list. If someone adds a "x.pid" file,
because "x.pid" does not match the calculated "vmUUID.ext" pattern, it
is skipped. "svdsm.time" and "lvm.env" will be skipped as well.
A better solution maybe to move all the VM related runtime files into a sub-dir.
Line 471: if fName not in dontDel)
Line 472: toDel = [f for f in fnmatch.filter(fNames, "*.guest.socket")
Line 473: if orphanVMFile(f)]
Line 474: toDel.extend(f for f in fnmatch.filter(fNames, "*.pid")
Line 473: if orphanVMFile(f)]
Line 474: toDel.extend(f for f in fnmatch.filter(fNames, "*.pid")
Line 475: if orphanVMFile(f))
Line 476: toDel.extend(f for f in fnmatch.filter(fNames, "*.recovery")
Line 477: if orphanVMFile(f))
I think we can use built-in function "any" to express "at least one of the
predicate must be true" and merge the predicates into one big list to use them in
"any".
exts = [ "*.guest.socket", "*.pid", "*.recovery"]
toDel = [f for f in fnames if
any(fnmatch.fnmatch(f, ext) for ext in exts) and
orphanVMFile(f)]
In this way we just traverse the fNames for 1 time.
Line 478:
Line 479: for fName in toDel:
Line 480: fPath = os.path.join(constants.P_VDSM_RUN, fName)
Line 481: try:
--
To view, visit
http://gerrit.ovirt.org/8626
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd63a1f4b1c64f84d5734da033a2420b97ce0ada
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server