6 a.m.
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 <ewarszaw(a)redhat.com>
---
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 <ewarszaw(a)redhat.com>
4:38 a.m.
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 <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: oVirt Jenkins CI Server
4:34 a.m.
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 <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: oVirt Jenkins CI Server
4:32 a.m.
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 <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: oVirt Jenkins CI Server
4:32 a.m.
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 <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: oVirt Jenkins CI Server
5:56 p.m.
Itamar Heim has abandoned this change.
Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................
Abandoned
no reply - abandoning - please restore if still relevant
--
To view, visit http://gerrit.ovirt.org/8626
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ifd63a1f4b1c64f84d5734da033a2420b97ce0ada
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)gmail.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)gmail.com>
Gerrit-Reviewer: Itamar Heim <iheim(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
11:31 p.m.
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 <ewarszaw(a)redhat.com>
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 <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
2:12 p.m.
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 <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>
5:45 p.m.
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 <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>
3:01 a.m.
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 <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>
5:32 p.m.
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 <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>
4:17 p.m.
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 <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>
2:59 p.m.
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 <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>
1:59 p.m.
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 <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>
12:17 p.m.
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 <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>
noon
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 <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: Yaniv Bronhaim <ybronhei(a)redhat.com>
8:38 a.m.
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 <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
3:42 p.m.
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 <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: oVirt Jenkins CI Server
7:45 a.m.
Yaniv Bronhaim has posted comments on this change.
Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................
Patch Set 10: Code-Review-1
no
--
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 <alitke(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Itamar Heim <iheim(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
Gerrit-HasComments: No
4:22 a.m.
Itamar Heim has posted comments on this change.
Change subject: Related to 846307 - Simplifying clientIF._cleanOldFiles().
......................................................................
Patch Set 10:
still relevant?
--
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 <alitke(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Itamar Heim <iheim(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
Gerrit-HasComments: No
11:27 p.m.
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
3637
days inactive
4208
days old
vdsm-patches@lists.fedorahosted.org
20 comments
8 participants
participants (8)
-
agl＠us.ibm.com -
Dan Kenigsberg -
ewarszaw＠redhat.com -
iheim＠redhat.com -
Jenkins CI RO -
smizrahi＠redhat.com -
ybronhei＠redhat.com -
zhshzhou＠linux.vnet.ibm.com