Merge lp:~nuclearbob/utah/isoclass into lp:utah

[Max Brustkern]

I created an ISO class and moved many of the existing utah.iso calls into it. I haven't tried to convert the stuff in isotesting yet, but the stuff in provisioning should be moved over.

[Javier Collado (javier.collado)]

Looked at the changes and they look nice. Also provisioned a VM from scratch using this branch and worked fine. Thanks.

My only comment is that please try to avoid unrelated changes in merge requests that fix a bug. I small refactoring is nice, but I think that removing a whole file and adding a lot of documentation strings should be part of a separate merge request.

review: Approve

[Javier Collado (javier.collado)]

Anyway, I've to say that changes were properly separated in different commits, so it wasn't so difficult to review them (maybe I was a little bit picky in my comment above).

[Max Brustkern (nuclearbob)]

I meant to do those separately, I must have just based this branch on my working copy or something instead of the branch in launchpad. I'll keep better track of that in the future to keep them separated.

=== modified file 'utah/config.py'
--- utah/config.py	2012-08-13 20:37:09 +0000
+++ utah/config.py	2012-08-24 20:25:19 +0000
@@ -75,8 +75,10 @@
 CONFIG = {}
 CONFIG.update(DEFAULTS)
 
+"""
+Process config files.
+"""
 files = ['/etc/utah/config', '~/.utah/config', os.getenv('UTAH_CONFIG_FILE')]
-
 for conffile in files:
     if conffile is not None:
         myfile = os.path.expanduser(conffile)
 
=== modified file 'utah/exceptions.py'
--- utah/exceptions.py	2012-07-19 13:25:30 +0000
+++ utah/exceptions.py	2012-08-24 20:25:19 +0000
@@ -1,4 +1,6 @@
-#!/usr/bin/python
+"""
+Provide exceptions for the rest of the tool.
+"""
 
 
 class UTAHException(Exception):
 
=== modified file 'utah/iso.py'
--- utah/iso.py	2012-08-13 20:37:09 +0000
+++ utah/iso.py	2012-08-24 20:25:19 +0000
@@ -1,12 +1,17 @@
 """
 All commands use bsdtar and accept a logmethod to use for logging.
-TODO: Make an iso class that can return things like arch, series, type, etc.
-TODO: Add the existing functions to the iso class.
-TODO: Convert the code to use the class instead of the existing functions.
 """
+#TODO: Make an iso class that can return things like arch, series, type, etc.
+#TODO: Add the existing functions to the iso class.
+#TODO: Convert the code to use the class instead of the existing functions.
+import logging
+import logging.handlers
 import os
 import subprocess
+import sys
+import urllib
 
+from utah import config
 from utah.exceptions import UTAHException
 
 
@@ -104,3 +109,134 @@
     """
     cmd = getrealfile(image, filename, logmethod=logmethod)
     return subprocess.Popen(cmd, stdout=subprocess.PIPE, **kw)
+
+
+class ISO(object):
+    """
+    Provide a simplified method of interfacing with images.
+    """
+    def __init__(self, image, dlpercentincrement=1, logger=None):
+        self.dlpercentincrement = dlpercentincrement
+        if logger is None:
+            self._loggersetup()
+        else:
+            self.logger = logger
+        if image.startswith('~'):
+            path = os.path.expanduser(image)
+            self.logger.debug('Rewriting ~-based path: ' + image + ' to: ' + path)
+        else:
+            path = image
+
+        self.percent = 0
+        self.logger.info('Preparing image: ' + path)
+        self.image = urllib.urlretrieve(path, reporthook=self.dldisplay)[0]
+        self.logger.debug(path + ' is locally available as ' + self.image)
+        self.installtype = self.getinstalltype()
+        if self.installtype == 'mini':
+            self.logger.debug('Using mini image')
+            self.infofile = '.disk/mini-info'
+        else:
+            self.logger.debug('Using normal image')
+            self.infofile = './.disk/info'
+        # Info file looks like this:
+        # Ubuntu 12.04 LTS "Precise Pangolin" - Release i386 (20120423)
+        # or this:
+        # Ubuntu 12.10 "Quantal Quetzal" - Alpha amd64 (20120820)
+        # i.e. Ubuntu Version (LTS) "Series Codename" - Alpha/Beta/Release
+        # arch (buildnumber)
+        self.arch = self.getarch()
+        self.series = self.getseries()
+
+    def _loggersetup(self):
+        """
+        Initialize the logging for the image.
+        """
+        self.logger = logging.getLogger('utah.iso')
+        self.logger.propagate = False
+        self.logger.setLevel(logging.INFO)
+        for handler in list(self.logger.handlers):
+            self.logger.removeHandler(handler)
+            del handler
+        self.consolehandler = logging.StreamHandler(stream=sys.stderr)
+        console_formatter = logging.Formatter('%(levelname)s: %(message)s')
+        self.consolehandler.setFormatter(console_formatter)
+        self.consolehandler.setLevel(config.consoleloglevel)
+        self.logger.addHandler(self.consolehandler)
+        self.filehandler = logging.handlers.WatchedFileHandler(config.logfile)
+        file_formatter = logging.Formatter('%(asctime)s iso '
+                                           + ' %(levelname)s: %(message)s')
+        self.filehandler.setFormatter(file_formatter)
+        self.filehandler.setLevel(config.fileloglevel)
+        self.logger.addHandler(self.filehandler)
+        if config.debuglog is not None:
+            self.logger.setLevel(logging.DEBUG)
+            self.debughandler = (
+                    logging.handlers.WatchedFileHandler(config.debuglog))
+            self.debughandler.setFormatter(file_formatter)
+            self.debughandler.setLevel(logging.DEBUG)
+            self.logger.addHandler(self.debughandler)
+
+    def getinstalltype(self):
+        """
+        Inspect the image's files to get the image type.
+        If .disk/mini-info exists, it's mini.
+        If the casper directory exists, it's desktop.
+        If ubuntu-server.seed exists in the preseeds directory, it's server.
+        """
+        self.logger.info('Getting image type of ' + self.image)
+        files = self.listfiles(returnlist=True)
+        installtype = 'alternate'
+        if '.disk/mini-info' in files:
+            installtype = 'mini'
+        elif './casper' in files:
+            installtype = 'desktop'
+        elif './preseed/ubuntu-server.seed' in files:
+            installtype = 'server'
+        self.logger.info('Image type is: ' + installtype)
+        return installtype
+
+    def getarch(self):
+        """
+        Unpack the image's info file to get the arch.
+        """
+        stdout = self.dump(self.infofile).communicate()[0]
+        arch = stdout.split()[-2]
+        self.logger.info('Arch is: ' + arch)
+        return arch
+
+    def getseries(self):
+        """
+        Unpack the image's info file to get the series.
+        """
+        stdout = self.dump(self.infofile).communicate()[0]
+        for word in stdout.split():
+            if word.startswith('"'):
+                series = word.strip('"').lower()
+                break
+        self.logger.info('Series is ' + series)
+        return series
+
+    def dldisplay(self, blocks, size, total):
+        read = blocks * size
+        percent = 100 * read / total
+        if percent >= self.percent:
+            self.logger.info('File ' + str(percent) + '% downloaded')
+            self.percent += self.dlpercentincrement
+        self.logger.debug("%s read, %s%% of %s total" % (read, percent, total))
+
+    def listfiles(self, returnlist=False):
+        return listfiles(self.image, self.logger.debug, returnlist=returnlist)
+
+    def getrealfile(self, filename, outdir=None):
+        return getrealfile(self.image, filename, outdir=outdir, logmethod=self.logger.debug)
+
+    def extract(self, filename, outdir=None, outfile=None, **kw):
+        return extract(self.image, filename, outdir, outfile, logmethod=self.logger.debug, **kw)
+
+    def dump(self, filename, **kw):
+        return dump(self.image, filename, logmethod=self.logger.debug, **kw)
+
+    def extractall(self):
+        for myfile in self.listfiles(returnlist=True):
+            self.extract(myfile)
+
 
=== modified file 'utah/provisioning/baremetal/cobbler.py'
--- utah/provisioning/baremetal/cobbler.py	2012-08-20 10:20:44 +0000
+++ utah/provisioning/baremetal/cobbler.py	2012-08-24 20:25:19 +0000
@@ -1,4 +1,6 @@
-#!/usr/bin/python
+"""
+Support bare metal provisioning through cobbler.
+"""
 
 import os
 import shutil
@@ -12,23 +14,22 @@
         Machine,
         SSHMixin,
         )
-from utah.provisioning.exceptions import UTAHProvisioningException
+from utah.provisioning.baremetal.exceptions import UTAHBMProvisioningException
 from utah.retry import retry
 
 
 class CobblerMachine(CustomInstallMixin, SSHMixin, Machine):
     """
     Provide a class to provision a machine via cobbler.
-
     """
     # TODO: may need to fix DHCP/hostname issues, may just be magners
     def __init__(self, boot=None, cargs=None, inventory=None,
                  name=None, preseed=None, *args, **kw):
         if name is None:
-            raise UTAHProvisioningException(
+            raise UTAHBMProvisioningException(
                     'Machine name reqired for cobbler machine')
         if cargs is None:
-            raise UTAHProvisioningException(
+            raise UTAHBMProvisioningException(
                     'No cobbler arguments given for machine creation')
         else:
             self.cargs = cargs
@@ -38,7 +39,7 @@
         super(CobblerMachine, self).__init__(*args, name=name,
                                              preseed=preseed, **kw)
         if self.image is None:
-            raise UTAHProvisioningException(
+            raise UTAHBMProvisioningException(
                     'Image file required for cobbler installation')
         self._custominit(arch=self.arch, boot=boot,
                          installtype=self.installtype, series=self.series)
@@ -50,7 +51,7 @@
                                         'ubuntu-installer', self.arch,
                                         'initrd.gz')
         else:
-            raise UTAHProvisioningException(
+            raise UTAHBMProvisioningException(
                     'Only alternate and server images currently supported '
                     'for cobbler provisioning')
         if self.arch == 'amd64':
@@ -63,16 +64,17 @@
 
     def _provision(self, checktimeout=config.checktimeout,
                    installtimeout=config.installtimeout):
+        """
+        Install a machine.
+        """
+        # TODO: support for reusing existing machines
         self.tmpdir = tempfile.mkdtemp(prefix='/tmp/' + self.name + '_')
         os.chdir(self.tmpdir)
 
         # TODO: try to remove this step, mount the iso and import that
         # with a specified kernel/preseed/initrd from the tmpdir
         self.logger.info('Extracting image to ' + self.tmpdir)
-        files = utah.iso.listfiles(self.image, logmethod=self.logger.debug,
-                                   returnlist=True)
-        for myfile in files:
-            utah.iso.extract(self.image, myfile, logmethod=self.logger.debug)
+        self.image.extractall()
 
         self._preparekernel()
         cinitrd = os.path.join(self.tmpdir, self.cinitrd)
@@ -149,7 +151,7 @@
                               .format(uuid=self.uuid))
         if self.run(uuid_check_command)[0] != 0:
             self.provisioned = False
-            raise UTAHProvisioningException(
+            raise UTAHBMProvisioningException(
                     'Installed UUID differs from CobblerMachine UUID; '
                     'installation probably failed. '
                     'Try restarting cobbler or running cobbler sync')
@@ -157,6 +159,11 @@
         return True
 
     def destroy(self, *args, **kw):
+        """
+        Destroy the machine, and call super to release it in the inventory.
+        """
+        # TODO: Remove the machine from cobbler
+        # TODO: Consider actually wrecking the install
         super(CobblerMachine, self).destroy(*args, **kw)
         self.__del__()
         del self
@@ -185,6 +192,9 @@
         self.active = False
 
     def restart(self):
+        """
+        Stop the machine, wait powertimeout, then start it/
+        """
         self.logger.info('Restarting system')
         self.stop()
         self.logger.debug('Waiting ' + str(config.powertimeout) + ' seconds')
@@ -192,6 +202,10 @@
         self._start()
 
     def __del__(self):
+        """
+        If the machine has an inventory, tell the inventory to release the
+        machine when the object is deleted.
+        """
         if self.inventory is not None:
             self.inventory.release(machine=self)
 
@@ -199,6 +213,7 @@
         """
         Start the machine if needed, and check for SSH login.
         """
+        # TODO: Consider moving this to SSHMixin
         self.logger.debug('Checking if machine is active')
         self.provisioncheck()
         if not self.active:
 
=== modified file 'utah/provisioning/baremetal/exceptions.py'
--- utah/provisioning/baremetal/exceptions.py	2012-07-19 09:36:34 +0000
+++ utah/provisioning/baremetal/exceptions.py	2012-08-24 20:25:19 +0000
@@ -1,4 +1,6 @@
-#!/usr/bin/python
+"""
+Provide exceptions specific to bare metal provisioning.
+"""
 
 from utah.provisioning.exceptions import UTAHProvisioningException
 
 
=== modified file 'utah/provisioning/exceptions.py'
--- utah/provisioning/exceptions.py	2012-07-24 11:08:49 +0000
+++ utah/provisioning/exceptions.py	2012-08-24 20:25:19 +0000
@@ -1,5 +1,6 @@
-#!/usr/bin/python
-
+"""
+Provide exceptions specific to provisioning.
+"""
 from utah.exceptions import UTAHException
 
 
 
=== removed file 'utah/provisioning/inventory/bootspeed.py'
--- utah/provisioning/inventory/bootspeed.py	2012-08-20 10:20:44 +0000
+++ utah/provisioning/inventory/bootspeed.py	1970-01-01 00:00:00 +0000
@@ -1,116 +0,0 @@
-#!/usr/bin/python
-
-import os
-from utah.provisioning.inventory.exceptions import \
-        UTAHProvisioningInventoryException
-from utah.provisioning.inventory.sqlite import SQLiteInventory
-from utah.provisioning.baremetal.cobbler import CobblerMachine
-
-
-class BootspeedInventory(SQLiteInventory):
-    """
-    Keep an inventory of manually entered machines for bootspeed testing.
-    All columns other than machineid, name, and state are assumed to be
-    arguments for cobbler system creation.
-    """
-    def __init__(self, db='~/.utah-bootspeed-inventory',
-                 lockfile='~/.utah-bootspeed-lock', *args, **kw):
-        db = os.path.expanduser(db)
-        lockfile = os.path.expanduser(lockfile)
-        if not os.path.isfile(db):
-            raise UTAHProvisioningInventoryException(
-                    'No machine database found at ' + db)
-        super(BootspeedInventory, self).__init__(*args, db=db,
-                                                 lockfile=lockfile, **kw)
-        machines_count = (self.connection
-                          .execute('SELECT COUNT(*) FROM machines')
-                          .fetchall()[0][0])
-        if machines_count == 0:
-            raise UTAHProvisioningInventoryException('No machines in database')
-        self.machines = []
-
-    def request(self, name=None, *args, **kw):
-        query = 'SELECT * FROM machines'
-        queryvars = []
-        if name is not None:
-            query += ' WHERE name=?'
-            queryvars.append(name)
-        result = self.connection.execute(query, queryvars).fetchall()
-        if result is None:
-            raise UTAHProvisioningInventoryException(
-                    'No machines meet criteria')
-        else:
-            for machineinfo in result:
-                cargs = dict(machineinfo)
-                machineid = cargs.pop('machineid')
-                name = cargs.pop('name')
-                state = cargs.pop('state')
-                if state == 'available':
-                    update = self.connection.execute(
-                            "UPDATE machines SET state='provisioned' "
-                            "WHERE machineid=? AND state='available'",
-                            [machineid]).rowcount
-                    if update == 1:
-                        machine = CobblerMachine(*args, cargs=cargs,
-                                inventory=self, name=name, **kw)
-                        self.machines.append(machine)
-                        return machine
-                    elif update == 0:
-                        raise UTAHProvisioningInventoryException(
-                                'Machine was requested by another process '
-                                'before we could request it')
-                    elif update > 1:
-                        raise UTAHProvisioningInventoryException(
-                                'Multiple machines exist '
-                                'matching those criteria; '
-                                'database ' + self.db + ' may be corrupt')
-                    else:
-                        raise UTAHProvisioningInventoryException(
-                                'Negative rowcount returned '
-                                'when attempting to request machine')
-        raise UTAHProvisioningInventoryException(
-                'All machines meeting criteria are currently unavailable')
-
-    def release(self, machine=None, name=None):
-        if machine is not None:
-            name = machine.name
-        if name is None:
-            raise UTAHProvisioningInventoryException(
-                    'name required to release a machine')
-        query = "UPDATE machines SET state='available' WHERE name=?"
-        queryvars = [name]
-        update = self.connection.execute(query, queryvars).rowcount
-        if update == 1:
-            if machine is not None:
-                if machine in self.machines:
-                    self.machines.remove(machine)
-        elif update == 0:
-            raise UTAHProvisioningInventoryException(
-                    'SERIOUS ERROR: Another process released this machine '
-                    'before we could, which means two processes provisioned '
-                    'the same machine simultaneously')
-        elif update > 1:
-            raise UTAHProvisioningInventoryException(
-                    'Multiple machines exist matching those criteria; '
-                    'database ' + self.db + ' may be corrupt')
-        else:
-            raise UTAHProvisioningInventoryException(
-                    'Negative rowcount returned '
-                    'when attempting to request machine')
-
-# Here is how I currently create the database:
-# CREATE TABLE machines (machineid INTEGER PRIMARY KEY,
-#                        name TEXT NOT NULL UNIQUE,
-#                        state TEXT default 'available',
-#                        [mac-address] TEXT NOT NULL UNIQUE,
-#                        [power-address] TEXT DEFAULT '10.97.0.13',
-#                        [power-id] TEXT,
-#                        [power-user] TEXT DEFAULT 'ubuntu',
-#                        [power-pass] TEXT DEFAULT 'ubuntu',
-#                        [power-type] DEFAULT 'sentryswitch_cdu');
-
-# Here is how I currently populate the database:
-# INSERT INTO machines (name, [mac-address], [power-id])
-# VALUES ('acer-veriton-01-Pete', 'd0:27:88:9f:73:ce', 'Veriton_1');
-# INSERT INTO machines (name, [mac-address], [power-id])
-# VALUES ('acer-veriton-02-Pete', 'd0:27:88:9b:84:5b', 'Veriton_2');
 
=== modified file 'utah/provisioning/inventory/exceptions.py'
--- utah/provisioning/inventory/exceptions.py	2012-07-19 09:36:34 +0000
+++ utah/provisioning/inventory/exceptions.py	2012-08-24 20:25:19 +0000
@@ -1,4 +1,6 @@
-#!/usr/bin/python
+"""
+Provide excpetions specific to inventory issues.
+"""
 
 from utah.provisioning.exceptions import UTAHProvisioningException
 
 
=== modified file 'utah/provisioning/inventory/inventory.py'
--- utah/provisioning/inventory/inventory.py	2012-07-24 14:07:41 +0000
+++ utah/provisioning/inventory/inventory.py	2012-08-24 20:25:19 +0000
@@ -63,7 +63,7 @@
 
     def write(self):
         """
-        Force a commit all current inventory data in memory to internal or
+        Force a commit of all current inventory data in memory to internal or
         external storage.
         """
         return False
 
=== modified file 'utah/provisioning/inventory/sqlite.py'
--- utah/provisioning/inventory/sqlite.py	2012-08-20 10:20:44 +0000
+++ utah/provisioning/inventory/sqlite.py	2012-08-24 20:25:19 +0000
@@ -177,7 +177,7 @@
     #                        [power-id] TEXT,
     #                        [power-user] TEXT DEFAULT 'ubuntu',
     #                        [power-pass] TEXT DEFAULT 'ubuntu',
-    #                        [power-type] DEFAULT 'sentryswitch_cdu');
+    #                        [power-type] TEXT DEFAULT 'sentryswitch_cdu');
 
     # Here is how I currently populate the database:
     # INSERT INTO machines (name, [mac-address], [power-id])
 
=== modified file 'utah/provisioning/provisioning.py'
--- utah/provisioning/provisioning.py	2012-08-20 19:56:21 +0000
+++ utah/provisioning/provisioning.py	2012-08-24 20:25:19 +0000
@@ -18,10 +18,11 @@
 import re
 import apt.cache
 
+from utah.iso import ISO
 from utah.provisioning.exceptions import UTAHProvisioningException
 from utah.retry import retry
 import utah.timeout
-from utah import config, iso
+from utah import config
 
 
 class Machine(object):
@@ -94,7 +95,14 @@
         self.active = False
         self._loggersetup()
 
-        for item in ['preseed', 'xml', 'kernel', 'initrd', 'image']:
+        if image is None:
+            self.image = None
+        else:
+            self.image = ISO(image,
+                    dlpercentincrement=self.dlpercentincrement,
+                    logger=self.logger)
+
+        for item in ['preseed', 'xml', 'kernel', 'initrd']:
             # Ensure every file/url type argument is available locally
             arg = locals()[item]
             if arg is None:
@@ -634,87 +642,12 @@
     rewriting them to our liking, and repacking them.
     """
     # TODO: setup variable(s) to suppress adding and rewriting
-
-    def _getinstalltypefromimage(self, image=None):
-        """
-        Inspect the image's files to get the image type.
-        If .disk/mini-info exists, it's mini.
-        If the casper directory exists, it's desktop.
-        If ubuntu-server.seed exists in the preseeds directory, it's server.
-        """
-        # TODO: move this into the iso class after it exists
-        if image is None:
-            image = self.image
-        self.logger.info('Getting image type of ' + image)
-        files = iso.listfiles(image,
-                              logmethod=self.logger.debug, returnlist=True)
-        installtype = 'alternate'
-        if '.disk/mini-info' in files:
-            installtype = 'mini'
-        elif './casper' in files:
-            installtype = 'desktop'
-        elif './preseed/ubuntu-server.seed' in files:
-            installtype = 'server'
-        self.logger.info('Image type is: ' + installtype)
-        return installtype
-
-    def _getarchfromimage(self, image=None, installtype=None):
-        """
-        Unpack the image's info file to get the arch.
-        Mini images use .disk/mini-info, everything else uses .disk/info.
-        """
-        # TODO: document the format of the info file
-        # TODO: move this into the iso class after it exists
-        if image is None:
-            image = self.image
-        if installtype is None:
-            installtype = self.installtype
-        self.logger.info('Getting arch of ' + image)
-        if installtype == 'mini':
-            self.logger.debug('Checking mini image')
-            infofile = '.disk/mini-info'
-        else:
-            self.logger.debug('Checking normal image')
-            infofile = './.disk/info'
-        stdout = (iso.dump(image, infofile, logmethod=logging.debug)
-                  .communicate()[0])
-        arch = stdout.split()[-2]
-        self.logger.info('Arch is: ' + arch)
-        return arch
-
-    def _getseriesfromimage(self, image=None, installtype=None):
-        """
-        Unpack the image's info file to get the series.
-        Mini images use .disk/mini-info, everything else uses .disk/info.
-        """
-        # TODO: document the format of the info file
-        # TODO: move this into the iso class after it exists
-        if image is None:
-            image = self.image
-        if installtype is None:
-            installtype = self.installtype
-        self.logger.info('Getting series of ' + image)
-        if installtype == 'mini':
-            self.logger.debug('Checking mini image')
-            infofile = '.disk/mini-info'
-        else:
-            self.logger.debug('Checking normal image')
-            infofile = './.disk/info'
-        stdout = (iso.dump(image, infofile, logmethod=logging.debug)
-                  .communicate()[0])
-        for word in stdout.split():
-            if word.startswith('"'):
-                series = word.strip('"').lower()
-                break
-        self.logger.info('Series is ' + series)
-        return series
-
     def _preparekernel(self, kernel=None, tmpdir=None):
         """
         If we haven't been given a kernel file, unpack one from the image.
         """
         # TODO: document this better
-        # TODO: consider moving this into the iso class when it exists
+        # TODO: consider moving this into the ISO class when it exists
         self.logger.info('Preparing kernel')
         if kernel is None:
             kernel = self.kernel
@@ -730,8 +663,7 @@
                 self.logger.debug('Desktop image detected')
                 kernelpath = './casper/vmlinuz'
             kernel = os.path.join(tmpdir, 'kernel')
-            iso.extract(self.image, kernelpath,
-                        outfile=kernel, logmethod=logging.debug)
+            self.image.extract(kernelpath, outfile=kernel)
         else:
             self.logger.info('Using local kernel: ' + kernel)
         return kernel
@@ -741,7 +673,7 @@
         If we haven't been given an initrd file, unpack one from the image.
         """
         # TODO: document this better
-        # TODO: consider moving this into the iso class when it exists
+        # TODO: consider moving this into the ISO class when it exists
         self.logger.info('Preparing initrd')
         if initrd is None:
             initrd = self.initrd
@@ -757,8 +689,7 @@
                 self.logger.debug('Desktop image detected')
                 initrdpath = './casper/initrd.lz'
             initrd = os.path.join(tmpdir, os.path.basename(initrdpath))
-            iso.extract(self.image, initrdpath,
-                        outfile=initrd, logmethod=logging.debug)
+            self.image.extract(initrdpath, outfile=initrd)
         else:
             self.logger.info('Using local initrd: ' + initrd)
         return initrd
@@ -874,7 +805,7 @@
                                          .format(pkgname))
                         line = line.replace('string ',
                                             'string {} '.format(pkgname))
-                        line = line.rstrip()
+                        line = line.rstrip() + "\n"
             if 'netcfg/get_hostname' in line:
                 self.logger.info('Rewriting hostname to ' + self.name)
                 line = 'd-i netcfg/get_hostname string ' + self.name + "\n"
@@ -1018,13 +949,15 @@
 
     def _custominit(self, arch=None, boot=None, installtype=None, series=None):
         """
-        TODO: Make this a proper __init__ method
-        by adding super call to Machine.
+        Setup installtype, arch, series and commandline.
+        Everything but commandline setup may be replaced by ISO class later.
         """
+        #TODO: Make this a proper __init__ method
+        #by adding super call to Machine.
         if installtype is None:
-            self.installtype = self._getinstalltypefromimage()
+            self.installtype = self.image.installtype
         if arch is None:
-            self.arch = self._getarchfromimage()
+            self.arch = self.image.arch
         if series is None:
-            self.series = self._getseriesfromimage()
+            self.series = self.image.series
         self._cmdlinesetup(boot=boot)
 
=== modified file 'utah/provisioning/vm/exceptions.py'
--- utah/provisioning/vm/exceptions.py	2012-07-19 09:36:34 +0000
+++ utah/provisioning/vm/exceptions.py	2012-08-24 20:25:19 +0000
@@ -1,4 +1,6 @@
-#!/usr/bin/python
+"""
+Provide excpetions specific to virtual machines.
+"""
 
 from utah.provisioning.exceptions import UTAHProvisioningException
 
 
=== removed file 'utah/provisioning/vm/isotest.py'
--- utah/provisioning/vm/isotest.py	2012-07-12 11:06:21 +0000
+++ utah/provisioning/vm/isotest.py	1970-01-01 00:00:00 +0000
@@ -1,15 +0,0 @@
-#!/usr/bin/python
-
-import os
-
-import utah.provisioning
-
-isodir = '/var/cache/uath/iso'
-
-for iso in os.listdir(isodir):
-    isofile = os.path.join(isodir, iso)
-    m = utah.provisioning.vm.CustomVM(image=isofile)
-    print('File: ' + isofile)
-    print('Type: ' + m.installtype)
-    print('Arch: ' + m.arch)
-    print('Series: ' + m.series)
 
=== modified file 'utah/provisioning/vm/libvirtvm.py'
--- utah/provisioning/vm/libvirtvm.py	2012-08-20 10:20:44 +0000
+++ utah/provisioning/vm/libvirtvm.py	2012-08-24 20:25:19 +0000
@@ -1,4 +1,6 @@
-#!/usr/bin/python
+"""
+Provide classes to provision libvirt-based virtual machines.
+"""
 
 import os
 import random
@@ -16,7 +18,6 @@
         SSHMixin,
         CustomInstallMixin,
         )
-from utah.provisioning.exceptions import UTAHProvisioningException
 from utah.provisioning.vm.vm import VM
 from utah.provisioning.vm.exceptions import UTAHVMProvisioningException
 from utah import config
@@ -40,7 +41,7 @@
 
     def _load(self):
         """
-        Load an existing VM
+        Load an existing VM.
         """
         self.logger.info('Loading VM')
         self.vm = self.lv.lookupByName(self.name)
@@ -121,6 +122,9 @@
         self.active = False
 
     def libvirterrorhandler(self, context, err):
+        """
+        Log libvirt errors instead of sending them directly to the console.
+        """
         errorcode = err.get_error_code()
         if errorcode in [9, 42]:
             # We see these as part of normal operations,
@@ -236,8 +240,7 @@
                 self.logger.info("\tThe kvm and processor-specific "
                                  "kvm kernel modules are installed and loaded")
                 self.logger.error('Software virtual machine support '
-                                  'will be implemented '
-                                  'in a future version of UTAH')
+                                  'is implemented in the CustomVM class')
                 raise UTAHVMProvisioningException(
                         'No hardware virtual machine support available')
             if ('Could not find' in line
@@ -259,7 +262,7 @@
                                  "'s/^vm_extra_packages=\""
                                  "/vm_extra_packages=\"python-yaml /' "
                                  "-i ~/.uqt_vm_tools.conf")
-                raise UTAHProvisioningException(
+                raise UTAHVMProvisioningException(
                         'No vm-tools config file available; '
                         'more info available in '
                         + self.filehandler.baseFilename)
@@ -344,6 +347,9 @@
 
 
 class CustomVM(CustomInstallMixin, SSHMixin, LibvirtVM):
+    """
+    Install a VM from an image using libvirt direct kernel booting.
+    """
     def __init__(self, arch=None, boot=None, diskbus=None, disksizes=None,
                  emulator=None, image=None, initrd=None, installtype=None,
                  kernel=None, machineid=None, macs=[], name=None,
@@ -417,6 +423,9 @@
         self.logger.debug('CustomVM init finished')
 
     def _createdisks(self, disksizes=None):
+        """
+        Create disk files if needed and build a list of them.
+        """
         self.logger.info('Creating disks')
         if disksizes is None:
             disksizes = self.disksizes
@@ -440,6 +449,9 @@
             self.logger.debug('Adding disk to list')
 
     def _supportsdomaintype(self, domaintype):
+        """
+        Check emulator support in libvirt capabilities.
+        """
         capabilities = ElementTree.fromstring(self.lv.getCapabilities())
         for guest in capabilities.iterfind('guest'):
             for arch in guest.iterfind('arch'):
@@ -450,6 +462,9 @@
 
     def _installxml(self, cmdline=None, initrd=None,
                     kernel=None, tmpdir=None, xml=None):
+        """
+        Return the XML tree to be passed to libvirt for VM installation.
+        """
         self.logger.info('Creating installation XML')
         if cmdline is None:
             cmdline = self.cmdline
@@ -502,11 +517,11 @@
             #TODO: Add a cdrom if none exists
             if disk.get('device') == 'cdrom':
                 if disk.find('source') is not None:
-                    disk.find('source').set('file', self.image)
+                    disk.find('source').set('file', self.image.image)
                     self.logger.debug('Rewrote existing CD-ROM')
                 else:
                     source = ElementTree.Element('source')
-                    source.set('file', self.image)
+                    source.set('file', self.image.image)
                     disk.append(source)
                     self.logger.debug('Added source to existing CD-ROM')
         for disk in self.disks:
@@ -559,6 +574,10 @@
         return xmlt
 
     def _installvm(self, lv=None, tmpdir=None, xml=None):
+        """
+        Install a VM, then undefine it in libvirt.
+        The final installation will recreate the VM using the existing disks.
+        """
         self.logger.info('Creating VM')
         if lv is None:
             lv = self.lv
@@ -581,6 +600,10 @@
         self.logger.info('Installation complete')
 
     def _finalxml(self, tmpdir=None, xml=None):
+        """
+        Create the XML to be used for the post-installation VM.
+        This may be a transformation of the installation XML.
+        """
         self.logger.info('Creating final VM XML')
         if xml is None:
             xml = ElementTree.ElementTree(file=self.xml)
@@ -601,6 +624,10 @@
         return xml
 
     def _create(self):
+        """
+        Create the VM, install the system, and prepare it to boot.
+        This primarily calls functions from CustomInstallMixin and CustomVM.
+        """
         self.logger.info('Creating custom virtual machine')
 
         tmpdir = tempfile.mkdtemp(prefix='/tmp/' + self.name + '_')
@@ -653,6 +680,9 @@
         self.active = True
 
     def destroy(self, *args, **kw):
+        """
+        Remove the machine from libvirt and remove all the disk files.
+        """
         super(CustomVM, self).destroy(*args, **kw)
         self.stop(force=True)
         self.vm.undefine()
 
=== modified file 'utah/provisioning/vm/vm.py'
--- utah/provisioning/vm/vm.py	2012-08-20 10:20:44 +0000
+++ utah/provisioning/vm/vm.py	2012-08-24 20:25:19 +0000
@@ -1,13 +1,14 @@
-#!/usr/bin/python
+"""
+Consolidate functions specific to virtual machine provisioning,
+but not specific to any type of virtual machine.
+"""
 
 from utah.provisioning.provisioning import Machine
 
 
 class VM(Machine):
     """
-    Provide a generic class to provision an arbitrary VM.
-
-    Raise exceptions for most methods.
+    Provide a class to consolidate methods specific to virtual machines.
     """
     def __init__(self, *args, **kw):
         super(VM, self).__init__(*args, **kw)
 
=== modified file 'utah/run.py'
--- utah/run.py	2012-08-16 13:12:51 +0000
+++ utah/run.py	2012-08-24 20:25:19 +0000
@@ -11,6 +11,9 @@
 
 
 def run_tests(args, machine):
+    """
+    Run some tests and retrieve results.
+    """
     # Since we're running from a script, output INFO
     for logger in [machine.logger] + machine.logger.handlers:
         if logger.level > logging.INFO:
 
=== modified file 'utah/timeout.py'
--- utah/timeout.py	2012-08-13 20:37:09 +0000
+++ utah/timeout.py	2012-08-24 20:25:19 +0000
@@ -22,8 +22,8 @@
     """
     Run command with all other arguments for up to timeout seconds, after with
     a UTAHTimeout exception is returned.
-    TODO: Better support for nested timeouts.
     """
+    #TODO: Better support for nested timeouts.
     if command is None:
         return
 
@@ -101,6 +101,9 @@
 
 
 def get_process_children(pid):
+    """
+    Find process children so they can be killed when the timeout expires.
+    """
     p = subprocess.Popen('ps --no-headers -o pid --ppid %d' % pid, shell=True,
                          stdout=subprocess.PIPE, stderr=subprocess.PIPE)
     stdout, stderr = p.communicate()