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

[Max Brustkern]

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.

[Javier Collado (javier.collado)]

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]

704. By Max Brustkern on 2012-10-10

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

[Max Brustkern (nuclearbob)]

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

[Javier Collado (javier.collado)]

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]

705. By Nuclear Bob <max@daedelus> on 2012-10-10

Fixed typos

[Max Brustkern (nuclearbob)]

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

review: Needs Resubmitting

[Javier Collado (javier.collado)]

Looks good now. Merging.

review: Approve

=== modified file 'utah/provisioning/baremetal/cobbler.py'
--- utah/provisioning/baremetal/cobbler.py	2012-10-04 15:14:41 +0000
+++ utah/provisioning/baremetal/cobbler.py	2012-10-10 20:36:19 +0000
@@ -182,24 +182,16 @@
                          + ' seconds')
         retry(self.sshcheck, checktimeout, logmethod=self.logger.info)
 
+        if self.installtype == 'desktop':
+            self.logger.info('Removing NFS share')
+            self._removenfs()
+
         if self.debug:
             self.logger.info('Leaving temp directory '
                              'because debug is enabled: ' + self.tmpdir)
         else:
             self.logger.info('Cleaning up temp directory')
-            if self.installtype == 'desktop':
-                self.logger.info('Removing NFS share')
-                self._removenfs()
-            # Cribbed from http://svn.python.org
-            # /projects/python/trunk/Mac/BuildScript/build-installer.py
-            for dirpath, dirnames, filenames in os.walk(self.tmpdir):
-                for name in (dirnames + filenames):
-                    filename = os.path.join(dirpath, name)
-                    if not os.path.islink(filename):
-                        self.logger.debug('Changing permissions of '
-                                          + filename)
-                        os.chmod(filename, 0775)
-            shutil.rmtree(self.tmpdir)
+            self._cleanuptmpdir()
 
         self.provisioned = True
         self.active = True
@@ -268,6 +260,8 @@
         """
         if self.installtype == 'desktop':
             self._removenfs()
+        if not self.debug:
+            self._cleanuptmpdir()
         if self.inventory is not None:
             self.inventory.release(machine=self)
 
@@ -277,6 +271,19 @@
                        '/d', '-i', config.nfsconfigfile])
         self.logger.debug('Reloading NFS config')
         self._runargs(config.nfscommand)
+        
+    def _cleanuptmpdir(self):
+        if os.path.isdir(self.tmpdir):
+            # Cribbed from http://svn.python.org
+            # /projects/python/trunk/Mac/BuildScript/build-installer.py
+            for dirpath, dirnames, filenames in os.walk(self.tmpdir):
+                for name in (dirnames + filenames):
+                    absolute_name = os.path.join(dirpath, name)
+                    if not os.path.islink(absolute_name):
+                        self.logger.debug('Changing permissions of '
+                                          + absolute_name)
+                        os.chmod(absolute_name, 0775)
+            shutil.rmtree(self.tmpdir)
 
     def activecheck(self):
         """