Change in vdsm[master]: Related to 846307 - Simplifying clientIF._cleanOldFiles().

Eduardo has uploaded a new change for review.

Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................

Related to 846307 - Simplifying clientIF._cleanOldFiles().

Change-Id: Ifd63a1f4b1c64f84d5734da033a2420b97ce0ada
Bug-Id: https://bugzilla.redhat.com/show_bug.cgi?id=846307
Signed-off-by: Eduardo 
---
M vdsm/clientIF.py
1 file changed, 15 insertions(+), 20 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/26/8626/1

diff --git a/vdsm/clientIF.py b/vdsm/clientIF.py
index e9ca7c9..669c5be 100644
--- a/vdsm/clientIF.py
+++ b/vdsm/clientIF.py
@@ -1,3 +1,4 @@
+import os.path
 #
 # Copyright 2011 Red Hat, Inc.
 #
@@ -453,24 +454,18 @@
         return None

def _cleanOldFiles(self):
-        for f in os.listdir(constants.P_VDSM_RUN):
+        for fName in os.listdir(constants.P_VDSM_RUN):
+            if fName in ('vdsmd.pid', 'respawn.pid', 'svdsm.pid',
+                        'svdsm.sock'):
+                continue
             try:
-                vmId, fileType = f.split(".", 1)
-                if fileType in ["guest.socket", "monitor.socket", "pid",
-                                    "stdio.dump", "recovery"]:
-                    if vmId in self.vmContainer:
-                        continue
-                    if f == 'vdsmd.pid':
-                        continue
-                    if f == 'respawn.pid':
-                        continue
-                    if f == 'svdsm.pid':
-                        continue
-                    if f == 'svdsm.sock':
-                        continue
-                else:
-                    continue
-                self.log.debug("removing old file " + f)
-                utils.rmFile(constants.P_VDSM_RUN + f)
-            except:
-                pass
+                vmId, fileType = fName.split('.', 1)
+            except ValueError:
+                pass  # fName does not contains '.'
+            if not fileType in ("guest.socket", "monitor.socket", "pid",
+                                "stdio.dump", "recovery"):
+                continue
+            if vmId in self.vmContainer:
+                continue
+            self.log.debug("Removing old file %s", fName)
+            utils.rmFile(constants.P_VDSM_RUN + fName)

--
To view, visit http://gerrit.ovirt.org/8626
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifd63a1f4b1c64f84d5734da033a2420b97ce0ada
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................

Patch Set 3: I would prefer that you didn't submit this

(2 inline comments)

thanks for the bravery of touch this old sh*t. few comments.

....................................................
File vdsm/clientIF.py
Line 452:             self.log.debug("Error recovering VM", exc_info=True)
Line 453:         return None
Line 454: 
Line 455:     def _cleanOldFiles(self):
Line 456:         known_exts = ("guest.socket", "monitor.socket", "pid", "stdio.dump",
"monitor.socket" and "stdio.dump" are from el5 days. Long deceased.
Line 457:                       "recovery")
Line 458:         dontDel = ("vdsmd.pid", "respawn.pid", "svdsm.pid", "svdsm.sock")
Line 459: 
Line 460:         fNames = [fName for fName in os.listdir(constants.P_VDSM_RUN)

Line 454: 
Line 455:     def _cleanOldFiles(self):
Line 456:         known_exts = ("guest.socket", "monitor.socket", "pid", "stdio.dump",
Line 457:                       "recovery")
Line 458:         dontDel = ("vdsmd.pid", "respawn.pid", "svdsm.pid", "svdsm.sock")
set()s would be nicer.
Line 459: 
Line 460:         fNames = [fName for fName in os.listdir(constants.P_VDSM_RUN)
Line 461:                                                     if fName not in dontDel]
Line 462:         for fName in fNames:

--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo

Adam Litke has posted comments on this change.

Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................

Patch Set 6: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File vdsm/clientIF.py
Line 466: 
Line 467:             if fileExt in known_exts and vmId not in self.vmContainer:
Line 468:                 fPath = os.path.join(constants.P_VDSM_RUN, fName)
Line 469:                 try:
Line 470:                     utils.rmFile(constants.P_VDSM_RUN + fName)
you just calculated fPath above.  Why not use it here instead of '+'?
Line 471:                 except OSError:

--
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: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Yaniv Bronhaim

Saggi Mizrahi has posted comments on this change.

Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................

Patch Set 6: (1 inline comment)

....................................................
File vdsm/clientIF.py
Line 456:         known_exts = ("guest.socket", "pid", "recovery")
Line 457:         dontDel = ("vdsmd.pid", "respawn.pid", "svdsm.pid", "svdsm.sock")
Line 458: 
Line 459:         fNames = [fName for fName in os.listdir(constants.P_VDSM_RUN)
Line 460:                                                     if fName not in dontDel]
Could be a generator instead of a list
Line 461:         for fName in fNames:
Line 462:             try:
Line 463:                 vmId, fileExt = fName.split(".", 1)
Line 464:             except ValueError:

--
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: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim

Adam Litke has posted comments on this change.

Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................

Patch Set 7: Looks good to me, but someone else must approve

--
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: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................

Patch Set 7: (1 inline comment)

....................................................
File vdsm/clientIF.py
Line 453:         return None
Line 454: 
Line 455:     def _cleanOldFiles(self):
Line 456:         known_exts = ("guest.socket", "pid", "recovery")
Line 457:         dontDel = ("vdsmd.pid", "respawn.pid", "svdsm.pid", "svdsm.sock")
I believe that "svdsm.sock" is superfluous here: it does not have one of the known_exts.

P.S. why not set()s? (I know that the tuples are short)
Line 458: 
Line 459:         fNames = [fName for fName in os.listdir(constants.P_VDSM_RUN)
Line 460:                                                     if fName not in dontDel]
Line 461:         for fName in fNames:

--
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: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim

Adam Litke has posted comments on this change.

Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................

Patch Set 8: Looks good to me, but someone else must approve

--
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: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................

Patch Set 8: I would prefer that you didn't submit this

(2 inline comments)

....................................................
File vdsm/clientIF.py
Line 453:         return None
Line 454: 
Line 455:     def _cleanOldFiles(self):
Line 456:         known_exts = ("guest.socket", "pid", "recovery")
Line 457:         dontDel = ("vdsmd.pid", "respawn.pid", "svdsm.pid", "svdsm.sock")
please drop "svdsm.sock", it does not have one of the known_exts.
Line 458: 
Line 459:         fNames = [fName for fName in os.listdir(constants.P_VDSM_RUN)
Line 460:                                                     if fName not in dontDel]
Line 461:         for fName in fNames:

Line 466: 
Line 467:             if fileExt in known_exts and vmId not in self.vmContainer:
Line 468:                 fPath = os.path.join(constants.P_VDSM_RUN, fName)
Line 469:                 try:
Line 470:                     utils.rmFile(constants.P_VDSM_RUN + fName)
fpath was already created one line above.
Line 471:                 except OSError:

--
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: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim

Eduardo has posted comments on this change.

Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................

Patch Set 8: (1 inline comment)

....................................................
File vdsm/clientIF.py
Line 466: 
Line 467:             if fileExt in known_exts and vmId not in self.vmContainer:
Line 468:                 fPath = os.path.join(constants.P_VDSM_RUN, fName)
Line 469:                 try:
Line 470:                     utils.rmFile(constants.P_VDSM_RUN + fName)
Please compare with patch set 7.
"They are between us".
Line 471:                 except OSError:

--
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: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim

Yaniv Bronhaim has posted comments on this change.

Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................

Patch Set 9: Looks good to me, but someone else must approve

--
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim

Adam Litke has posted comments on this change.

Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................

Patch Set 9: Looks good to me, but someone else must approve

--
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................

Patch Set 10:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/473/ (2/2)

--
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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................

Patch Set 10:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/438/ (1/2)

--
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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server 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

Build Unstable

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/438/ : UNSTABLE

http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/473/ : SUCCESS

--
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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg 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

please fix the reported indentation issues.

--
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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

Saggi Mizrahi 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

(1 inline comment)

....................................................
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)
This is much simpler and faster

# Get all potential files
toDel = set(os.listdir(constants.P_VDSM_RUN))
# Don't include files that are in the whitelist
toDel.difference_update(("vdsmd.pid", "respawn.pid", "svdsm.pid"))

# Don't include files related to running VMs
for vm in self.vmContainer:
  for suffix in ['.guest.socket', 'pid', 'recovery']:
      toDel.discard(vm + suffix)

# Delete all remaining files.
for fName in toDel:
....

This is better because:
- no lambdas
- It's clear that when you add a file not related to VMs it should be added to
  the whitelist
- It's clear that when adding a new file related to VMs it should be named
  <VM>.<suffix> and it's clear how to add the suffix
- Because all running VMs have files on the directory just running over that list is
  simpler and faster.
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")

--
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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Zhou Zheng Sheng has posted comments on this change.

Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................

Patch Set 10: (1 inline comment)

....................................................
Commit Message
Line 3: AuthorDate: 2012-10-17 12:54:09 +0200
Line 4: Commit:     Eduardo Warszawski 
Line 5: CommitDate: 2012-12-19 12:29:37 +0200
Line 6: 
Line 7: Related to 846307 - Simplifying clientIF._cleanOldFiles().
It's not obvious to me how this patch is related to https://bugzilla.redhat.com/show_bug.cgi?id=846307.
Line 8: 
Line 9: Change-Id: Ifd63a1f4b1c64f84d5734da033a2420b97ce0ada
Line 10: Bug-Id: https://bugzilla.redhat.com/show_bug.cgi?id=846307

--
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 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

Itamar Heim has posted comments on this change.

Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................

Patch Set 10:

still relevant?

Yaniv Bronhaim has posted comments on this change.

Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................

Patch Set 10: Code-Review-1

no

Itamar Heim has abandoned this change.

Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................

Abandoned

no reply - abandoning - please restore if still relevant