Merge lp:~nuclearbob/utah/iso-static-analysis-updates into lp:utah

[Max Brustkern]

This branch addresses lp:1086772. It also changes an exception in iso.py to give a clearer indication of what failed.

[Dimitri John Ledkov (xnox)]

The 12.04.2 images will also not have vmlinuz, and instead have vmlinuz.efi.
So please remove release guard.
It might make sense to pass a pattern like "bsdtar -x -f bla.iso 'casper/vmlinuz.*'" and then check error code from bsdtar / whether any casper/vmlinuz.* got extracted and what it's name is.

review: Needs Fixing

[lp:~nuclearbob/utah/iso-static-analysis-updates]

772. By Max Brustkern on 2012-12-05

Moved method to detect kernel path into ISO class

773. By Max Brustkern on 2012-12-05

Updated the test to use the kernelpath function of the ISO class, which allows it to work on server images as well

[Max Brustkern (nuclearbob)]

We have a method to find a kernel in an ISO by checking the bootloader configuration. I moved that from the main provisioning library into the ISO class, and changed the test to use that instead. This also allows the test to work on server images, so I updated it to do so.

review: Needs Resubmitting

[Dimitri John Ledkov (xnox)]

Looks good to me.

review: Approve

[Javier Collado (javier.collado)]

It looks to me too.

Anyway, I was wondering if we could make the kernel detection more solid using
a regular expression.

That is, instead of this:

if 'kernel' in line or 'linux' in line:
    newkernel = line.split()[1].strip('./')

maybe something like this:

import re

...

match = re.match('^\s+(kernel|linux)\s+(?P<path>[^\s]+).*', line)
if match:
    newkernel = match.group('path')

[Dimitri John Ledkov (xnox)]

I like the readable way (as is) vs (proposed) cryptic regexp =)
It's not like this is a performance bottle neck, is it?

[Javier Collado (javier.collado)]

My concern is that the words kernel or linux may appear anywhere in a line and,
if that happens, that will be be wrongly identified as a kernel path.

[Javier Collado (javier.collado)]

I'm merging this since the bug is critical. We can discuss my comment in the
next meeting.

review: Approve

[Javier Collado (javier.collado)]

I've created bug1087237 regarding to follow up the conversation about the
kernel path.

=== modified file 'utah/iso.py'
--- utah/iso.py	2012-11-30 12:05:01 +0000
+++ utah/iso.py	2012-12-05 19:45:25 +0000
@@ -10,6 +10,7 @@
 import urllib
 import re
 
+from collections import defaultdict
 from hashlib import md5
 
 from utah import config
@@ -191,7 +192,8 @@
                     cmd = ['bsdtar', '-x', '-f', self.image, '-C', outdir,
                            '-O', realfile]
         except IndexError:
-            raise UTAHException('Cannot unpack image: ' + self.image)
+            raise UTAHException('Cannot unpack {path} from {image}'
+                                .format(path=filename, image=self.image))
         self.logger.debug('bsdtar extract command: ' + ' '.join(cmd))
         return cmd
 
@@ -244,7 +246,7 @@
         """
         for myfile in self.listfiles(returnlist=True):
             self.extract(myfile)
-            
+
     def getmd5(self, path):
         """
         Return the md5 checksum of a file.
@@ -260,7 +262,7 @@
         filemd5 = isohash.hexdigest()
         self.logger.debug('md5 of ' + path + ' is ' + filemd5)
         return filemd5
-        
+
     def downloadiso(self, arch=None, installtype=None, series=None):
         """
         Download an ISO given series, type, and arch.
@@ -339,7 +341,8 @@
             isopath = os.path.join(remotepath, isofile)
             self.percent = 0
             self.logger.info('Attempting to download ' + isopath)
-            temppath = urllib.urlretrieve(isopath, reporthook=self.dldisplay)[0]
+            temppath = urllib.urlretrieve(isopath,
+                                          reporthook=self.dldisplay)[0]
 
             # Try to copy it into our cache
             self.logger.debug('Copying ' + temppath + ' to ' + path)
@@ -350,3 +353,34 @@
             else:
                 raise UTAHException('Image failed to download after ' +
                                     config.dlretries + ' tries')
+
+    def kernelpath(self):
+        kernelpath = './install/vmlinuz'
+        if self.installtype == 'mini':
+            self.logger.debug('Getting kernel for mini image')
+            kernelpath = 'linux'
+        elif self.installtype == 'desktop':
+            self.logger.debug('Getting kernel for desktop image')
+            # Setup the old default kernel path in case none are found
+            kernels = defaultdict(int, {'casper/vmlinuz': 0})
+            # Scan bootloader config files to find kernel images
+            cfgfiles = ['isolinux/txt.cfg', 'isolinux/rqtxt.cfg',
+                        'boot/grub/grub.cfg', 'boot/grub/loopback.cfg']
+            for cfgfile in cfgfiles:
+                if cfgfile in self.listfiles(returnlist=True):
+                    stdout = self.dump(cfgfile).communicate()[0]
+                    for line in stdout.splitlines():
+                        if 'kernel' in line or 'linux' in line:
+                            newkernel = line.split()[1].strip('./')
+                            if 'mt86plus' in newkernel:
+                                self.logger.debug('Rejecting '
+                                                  'memtest kernel')
+                            else:
+                                self.logger.debug('Found kernel: '
+                                                  + newkernel)
+                                kernels[newkernel] += 1
+            # Now we have a list of kernel paths and the number of times
+            # each once occurs. We'll use the one that occurs most.
+            kernelpath = max(kernels.iteritems(),
+                             key=lambda (_path, count): count)[0]
+        return kernelpath

=== modified file 'utah/isotest/iso_static_validation.py'
--- utah/isotest/iso_static_validation.py	2012-11-27 18:11:39 +0000
+++ utah/isotest/iso_static_validation.py	2012-12-05 19:45:25 +0000
@@ -37,7 +37,6 @@
 lib_path = os.path.abspath('../')
 sys.path.append(lib_path)
 from utah.iso import ISO
-from utah import config
 
 
 # Defaults
@@ -76,7 +75,7 @@
     ubiquity_image = True
 else:
     ubiquity_image = False
-    
+
 #Setup the path for data files
 DATA_PATH = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'data')
 
@@ -248,7 +247,7 @@
 
     # Test if  wubi is present for desktop or dvd and it is a valid PE
     @unittest.skipUnless(st_variant == 'desktop' and
-                         st_arch != 'powerpc' and st_release < 'raring', 
+                         st_arch != 'powerpc' and st_release < 'raring',
                          "Skipping images that aren't ubiquity-based and "
                          "older than raring")
     def test_wubi(self):
@@ -320,7 +319,7 @@
         if self.st_release >= 'q':
             # cdromupgrade is not shipped in quantal
             exclude_files.append('cdromupgrade')
-            
+
         for list_server in files_list:
             logging.debug('check if important d-i files are present in iso')
             path = list_server.rstrip()
@@ -330,14 +329,15 @@
                 self.assertIn(path, stdout)
 
     # Test if vmlinuz is a valid executable
-    @unittest.skipUnless(ubiquity_image and st_arch != 'powerpc',
-                         "vmlinuz is present only for ubiquity based images")
+    @unittest.skipUnless(st_arch != 'powerpc',
+                         "vmlinuz is not present only for powerpc images")
     def test_vmlinuz(self):
-        (stdout, stderr) = self.iso.extract('casper/vmlinuz', self.temp_dir)
+        path = iso.kernelpath()
+        (stdout, stderr) = self.iso.extract(path, self.temp_dir)
         logging.debug('Asserting that vmlinuz is present in the iso')
         self.assertEqual(stderr, '')
 
-        vmlinuz_path = os.path.join(self.temp_dir, 'casper/vmlinuz')
+        vmlinuz_path = os.path.join(self.temp_dir, path)
 
         cmd = ["file", vmlinuz_path]
         output = subprocess.Popen(cmd, stdout=subprocess.PIPE,

=== modified file 'utah/provisioning/provisioning.py'
--- utah/provisioning/provisioning.py	2012-11-23 13:08:36 +0000
+++ utah/provisioning/provisioning.py	2012-12-05 19:45:25 +0000
@@ -20,7 +20,6 @@
 
 from glob import glob
 from stat import S_ISDIR
-from collections import defaultdict
 
 import utah.timeout
 
@@ -888,34 +887,7 @@
             tmpdir = self.tmpdir
         if kernel is None:
             self.logger.info('Unpacking kernel from image')
-            kernelpath = './install/vmlinuz'
-            if self.installtype == 'mini':
-                self.logger.debug('Mini image detected')
-                kernelpath = 'linux'
-            elif self.installtype == 'desktop':
-                self.logger.debug('Desktop image detected')
-                # Setup the old default kernel path in case none are found
-                kernels = defaultdict(int, {'casper/vmlinuz': 0})
-                # Scan bootloader config files to find kernel images
-                cfgfiles = ['isolinux/txt.cfg', 'isolinux/rqtxt.cfg',
-                            'boot/grub/grub.cfg', 'boot/grub/loopback.cfg']
-                for cfgfile in cfgfiles:
-                    if cfgfile in self.image.listfiles(returnlist=True):
-                        stdout = self.image.dump(cfgfile).communicate()[0]
-                        for line in stdout.splitlines():
-                            if 'kernel' in line or 'linux' in line:
-                                newkernel = line.split()[1].strip('./')
-                                if 'mt86plus' in newkernel:
-                                    self.logger.debug('Rejecting '
-                                                      'memtest kernel')
-                                else:
-                                    self.logger.debug('Found kernel: '
-                                                      + newkernel)
-                                    kernels[newkernel] += 1
-                # Now we have a list of kernel paths and the number of times
-                # each once occurs. We'll use the one that occurs most.
-                kernelpath = max(kernels.iteritems(),
-                                 key=lambda (_path, count): count)[0]
+            kernelpath = self.image.kernelpath()
             kernel = os.path.join(tmpdir, 'kernel')
             self.image.extract(kernelpath, outfile=kernel)
         else: