Merge lp:~nuclearbob/utah/cobbler-cleanup into lp:utah

Proposed by Max Brustkern
Status: Merged
Approved by: Javier Collado
Approved revision: 705
Merged at revision: 705
Proposed branch: lp:~nuclearbob/utah/cobbler-cleanup
Merge into: lp:utah
Diff against target: 62 lines (+20/-13)
1 file modified
utah/provisioning/baremetal/cobbler.py (+20/-13)
To merge this branch: bzr merge lp:~nuclearbob/utah/cobbler-cleanup
Reviewer Review Type Date Requested Status
Javier Collado (community) Approve
Max Brustkern (community) Needs Resubmitting
Review via email: mp+128792@code.launchpad.net

Description of the change

This branch should more aggressively clean up tmp directories in case of failures so we don't get left with tons of directories cluttering up the system.

To post a comment you must log in.
Revision history for this message
Javier Collado (javier.collado) wrote :

I think that in the _cleanuptmpdir method is confusing to use a variable
that is named `filename` when it could be both a filename or a dirname.
I'd prefer something like `name`, but since that's already used in the
loop for the relative name, maybe `absolute_name` is a good choice
(also `path`, `absolute_path` might be good alternatives).

lp:~nuclearbob/utah/cobbler-cleanup updated
704. By Max Brustkern

Changed variable name per Javier's comments, and added a check to make sure the directory exists

Revision history for this message
Max Brustkern (nuclearbob) wrote :

Good point. I've also added a check to make sure the directory exists so that if the method gets called during provisioning and during __del__, it doesn't blow up.

review: Needs Resubmitting
Revision history for this message
Javier Collado (javier.collado) wrote :

The variable renaming introduced a few errors that I'm pasting below:
./utah/provisioning/baremetal/cobbler.py:281: W806 local variable 'absolute_name' is assigned to but never used
./utah/provisioning/baremetal/cobbler.py:282: W802 undefined name 'absoulte_name'
./utah/provisioning/baremetal/cobbler.py:284: W802 undefined name 'filename'
./utah/provisioning/baremetal/cobbler.py:285: W802 undefined name 'absoulte_name'

review: Needs Fixing
lp:~nuclearbob/utah/cobbler-cleanup updated
705. By Nuclear Bob <max@daedelus>

Fixed typos

Revision history for this message
Max Brustkern (nuclearbob) wrote :

Thanks, I really should be checking these better. New changes should be pushed now.

review: Needs Resubmitting
Revision history for this message
Javier Collado (javier.collado) wrote :

Looks good now. Merging.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'utah/provisioning/baremetal/cobbler.py'
2--- utah/provisioning/baremetal/cobbler.py 2012-10-04 15:14:41 +0000
3+++ utah/provisioning/baremetal/cobbler.py 2012-10-10 20:36:19 +0000
4@@ -182,24 +182,16 @@
5 + ' seconds')
6 retry(self.sshcheck, checktimeout, logmethod=self.logger.info)
7
8+ if self.installtype == 'desktop':
9+ self.logger.info('Removing NFS share')
10+ self._removenfs()
11+
12 if self.debug:
13 self.logger.info('Leaving temp directory '
14 'because debug is enabled: ' + self.tmpdir)
15 else:
16 self.logger.info('Cleaning up temp directory')
17- if self.installtype == 'desktop':
18- self.logger.info('Removing NFS share')
19- self._removenfs()
20- # Cribbed from http://svn.python.org
21- # /projects/python/trunk/Mac/BuildScript/build-installer.py
22- for dirpath, dirnames, filenames in os.walk(self.tmpdir):
23- for name in (dirnames + filenames):
24- filename = os.path.join(dirpath, name)
25- if not os.path.islink(filename):
26- self.logger.debug('Changing permissions of '
27- + filename)
28- os.chmod(filename, 0775)
29- shutil.rmtree(self.tmpdir)
30+ self._cleanuptmpdir()
31
32 self.provisioned = True
33 self.active = True
34@@ -268,6 +260,8 @@
35 """
36 if self.installtype == 'desktop':
37 self._removenfs()
38+ if not self.debug:
39+ self._cleanuptmpdir()
40 if self.inventory is not None:
41 self.inventory.release(machine=self)
42
43@@ -277,6 +271,19 @@
44 '/d', '-i', config.nfsconfigfile])
45 self.logger.debug('Reloading NFS config')
46 self._runargs(config.nfscommand)
47+
48+ def _cleanuptmpdir(self):
49+ if os.path.isdir(self.tmpdir):
50+ # Cribbed from http://svn.python.org
51+ # /projects/python/trunk/Mac/BuildScript/build-installer.py
52+ for dirpath, dirnames, filenames in os.walk(self.tmpdir):
53+ for name in (dirnames + filenames):
54+ absolute_name = os.path.join(dirpath, name)
55+ if not os.path.islink(absolute_name):
56+ self.logger.debug('Changing permissions of '
57+ + absolute_name)
58+ os.chmod(absolute_name, 0775)
59+ shutil.rmtree(self.tmpdir)
60
61 def activecheck(self):
62 """

Subscribers

People subscribed via source and target branches