Merge lp:~rvb/maas/reporting-bug-1301809 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Change list_boot_images() so that it can cope with a missing boot images directory: this happens if the reporting task runs before the images have been imported.

[Jeroen T. Vermeulen (jtv)]

That's a nice touch — it fixes an unforeseen consequence of the TFTP root moving to a new directory hierarchy.

The comment on the new code reads a bit weird:

        # Return an empty list only if the exception is a "No such file
        # or directory" exception.

Which is the main new fact in that sentence — that we should return an empty list, or that sometimes we shouldn't return an empty list? I think it might come out clearer as a matter of course if you commented on the two possible cases _inside_ the if/else bodies: “Directory does not exist, so return empty list.” / “Other error. Propagate.”

Right below that, the log message seems useful, but unnecessarily verbose for its context. This is an automatically repeating job, not something that ever happens in response to a human interaction. We already have helpful messages in the UI for the situation it signals. If a user is savvy enough to operate without ever looking at the UI even when things aren't working, then I think we can expect them to understand a much terser message such as “No boot images have been imported yet.”

Keeping the message short helps keep the log files readable. The message is likely to be logged many times before the images have been imported, and even while they are importing. Also, if a user does see the repeating “push the import button!” message at that stage, they might misinterpret it as meaning that the original import didn't work and they need to start a new one.

review: Approve

[Raphaël Badin (rvb)]

> That's a nice touch — it fixes an unforeseen consequence of the TFTP root
> moving to a new directory hierarchy.
> [...]

Both very good points, both well made :). Fixed.

=== modified file 'src/provisioningserver/boot/tests/test_tftppath.py'
--- src/provisioningserver/boot/tests/test_tftppath.py	2014-03-28 04:43:57 +0000
+++ src/provisioningserver/boot/tests/test_tftppath.py	2014-04-03 16:36:28 +0000
@@ -14,10 +14,13 @@
 __metaclass__ = type
 __all__ = []
 
+import errno
 import os.path
 
 from maastesting.factory import factory
 from maastesting.testcase import MAASTestCase
+from mock import Mock
+from provisioningserver.boot import tftppath
 from provisioningserver.boot.tftppath import (
     compose_image_path,
     drill_down,
@@ -35,6 +38,7 @@
     Not,
     StartsWith,
     )
+from testtools.testcase import ExpectedException
 
 
 def make_image(params, purpose):
@@ -95,6 +99,15 @@
         self.assertEqual(
             self.tftproot, locate_tftp_path(None, tftproot=self.tftproot))
 
+    def test_list_boot_images_copes_with_missing_directory(self):
+        self.assertEqual([], list_boot_images(factory.getRandomString()))
+
+    def test_list_boot_images_passes_on_other_exceptions(self):
+        error = OSError(errno.EACCES, "Deliberate error for testing.")
+        self.patch(tftppath, 'list_subdirs', Mock(side_effect=error))
+        with ExpectedException(OSError):
+            list_boot_images(factory.getRandomString())
+
     def test_list_boot_images_copes_with_empty_directory(self):
         self.assertEqual([], list_boot_images(self.tftproot))
 

=== modified file 'src/provisioningserver/boot/tftppath.py'
--- src/provisioningserver/boot/tftppath.py	2014-03-28 04:36:31 +0000
+++ src/provisioningserver/boot/tftppath.py	2014-04-03 16:36:28 +0000
@@ -20,10 +20,15 @@
     'locate_tftp_path',
     ]
 
+import errno
 from itertools import chain
+from logging import getLogger
 import os.path
 
 
+logger = getLogger(__name__)
+
+
 def compose_image_path(arch, subarch, release, label):
     """Compose the TFTP path for a PXE kernel/initrd directory.
 
@@ -135,7 +140,16 @@
     """
     # The sub-directories directly under tftproot, if they contain
     # images, represent architectures.
-    potential_archs = list_subdirs(tftproot)
+    try:
+        potential_archs = list_subdirs(tftproot)
+    except OSError as exception:
+        if exception.errno == errno.ENOENT:
+            # Directory does not exist, so return empty list.
+            logger.warning("No boot images have been imported yet.")
+            return []
+        else:
+            # Other error. Propagate.
+            raise
 
     # Starting point for iteration: paths that contain only the
     # top-level subdirectory of tftproot, i.e. the architecture name.