Merge lp:~mpontillo/maas/bug-1458894-ioerror-during-image-download into lp:~maas-committers/maas/trunk

[Mike Pontillo]

Log a single message if SimpleStreams throws an IOError while syncing images, rather than leaving the IOError uncaught and logging a stack trace.

[Blake Rouse (blake-rouse)]

I don't really like this as it affects the region calling the cluster to update the boot images. I don't know if waiting in the thread for 30 seconds is the best thing to do as it will hold the import lock and it will look like from the region that the cluster is importing images, but really it is waiting 30 seconds to try again.

I think this retry logic should be handled in the ImageDownloadService, and can be used to pause inside the reactor. Causing it to not block a thread.

review: Needs Fixing

[Mike Pontillo (mpontillo)]

I talked with Raphael this morning and we changed the approach.

I had done most of the work yesterday to move the retry mechanism to the ImageDownloadService, and it seemed to basically be working, apart from some Twisted-induced pain in suffering while writting the tests.

We decided on a simpler approach: just catch the IOError (as close as possible to the SimpleStreams function that throws it, which means I simply inherited another method from the BasicMirrorWriter class and checked it there) and log a single message rather than an ugly stack trace.

Can you take another look?

[Blake Rouse (blake-rouse)]

Looks good.

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~mpontillo/maas/bug-1458894-ioerror-during-image-download into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm pep8 phantomjs postgresql pyflakes python-apt python-bson python-bzrlib python-convoy python-coverage python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-iscpy python-jinja2 python-jsonschema python-lockfile python-lxml python-mock python-netaddr python-netifaces python-nose python-oauth python-openssl python-paramiko python-pexpect python-pip python-pocket-lint python-psycopg2 python-pyinotify python-pyparsing ...

[Mike Pontillo (mpontillo)]

The exception from the lander seems unrelated to this change. I'm going to try again...

=== modified file 'src/provisioningserver/import_images/download_descriptions.py'
--- src/provisioningserver/import_images/download_descriptions.py	2015-05-07 18:14:38 +0000
+++ src/provisioningserver/import_images/download_descriptions.py	2015-05-28 19:36:05 +0000
@@ -29,6 +29,7 @@
     get_os_from_product,
     get_signing_policy,
     ImageSpec,
+    maaslog,
 )
 from simplestreams.mirrors import (
     BasicMirrorWriter,
@@ -104,6 +105,14 @@
             self.boot_images_dict.set(
                 base_image._replace(subarch='generic'), compact_item)
 
+    def sync(self, reader, path):
+        try:
+            super(RepoDumper, self).sync(reader, path)
+        except IOError:
+            maaslog.warning(
+                "I/O error while syncing boot images. If this problem "
+                "persists, verify network connectivity and disk usage.")
+
 
 def value_passes_filter_list(filter_list, property_value):
     """Does the given property of a boot image pass the given filter list?

=== modified file 'src/provisioningserver/import_images/tests/test_download_descriptions.py'
--- src/provisioningserver/import_images/tests/test_download_descriptions.py	2015-05-07 18:14:38 +0000
+++ src/provisioningserver/import_images/tests/test_download_descriptions.py	2015-05-28 19:36:05 +0000
@@ -14,7 +14,11 @@
 __metaclass__ = type
 __all__ = []
 
+import logging
+
+from fixtures import FakeLogger
 from maastesting.factory import factory
+from maastesting.matchers import MockCalledOnceWith
 from maastesting.testcase import MAASTestCase
 from mock import sentinel
 from provisioningserver.import_images import download_descriptions
@@ -334,3 +338,20 @@
             os=os, release=release, arch=arch, subarch='generic',
             label=label)
         self.assertEqual(compat_item, boot_images_dict.mapping[image_spec])
+
+    def test_sync_does_not_propagate_ioerror(self):
+        mock_sync = self.patch(download_descriptions.BasicMirrorWriter, "sync")
+        mock_sync.side_effect = IOError()
+
+        boot_images_dict = BootImageMapping()
+        dumper = RepoDumper(boot_images_dict)
+
+        with FakeLogger("maas.import-images", level=logging.INFO) as maaslog:
+            # What we're testing here is that sync() doesn't raise IOError...
+            dumper.sync(sentinel.reader, sentinel.path)
+            # ... but we'll validate that we properly called the [mock]
+            # superclass method, and logged something, as well.
+            self.assertThat(
+                mock_sync, MockCalledOnceWith(sentinel.reader, sentinel.path))
+            self.assertDocTestMatches(
+                "...error...syncing boot images...", maaslog.output)