Merge lp:~allenap/maas/repo-dumper-do-not-suppress-io-errors--2.1 into lp:maas/2.1

Proposed by Gavin Panella on 2017-01-25
Status: Merged
Approved by: Gavin Panella on 2017-01-26
Approved revision: 5583
Merged at revision: 5582
Proposed branch: lp:~allenap/maas/repo-dumper-do-not-suppress-io-errors--2.1
Merge into: lp:maas/2.1
Diff against target: 47 lines (+13/-8)
2 files modified
src/provisioningserver/import_images/download_descriptions.py (+7/-0)
src/provisioningserver/import_images/tests/test_download_descriptions.py (+6/-8)
To merge this branch: bzr merge lp:~allenap/maas/repo-dumper-do-not-suppress-io-errors--2.1
Reviewer Review Type Date Requested Status
Mike Pontillo (community) 2017-01-25 Approve on 2017-01-26
Review via email: mp+315587@code.launchpad.net

Commit message

Backport r5635 and r5636 from trunk: Don't suppress IOError in RepoDumper.sync.

This was allowing garbage to reach cache_boot_sources.update_cache. This most often resulted in BootSourceCache being emptied in full or in part.

To post a comment you must log in.
Mike Pontillo (mpontillo) wrote :

LGTM.

review: Approve
5583. By Gavin Panella on 2017-01-26

Link to bug #1659511.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/provisioningserver/import_images/download_descriptions.py'
--- src/provisioningserver/import_images/download_descriptions.py 2016-09-30 13:56:04 +0000
+++ src/provisioningserver/import_images/download_descriptions.py 2017-01-26 09:43:15 +0000
@@ -117,6 +117,13 @@
117 maaslog.warning(117 maaslog.warning(
118 "I/O error while syncing boot images. If this problem "118 "I/O error while syncing boot images. If this problem "
119 "persists, verify network connectivity and disk usage.")119 "persists, verify network connectivity and disk usage.")
120 # This MUST NOT suppress the I/O error because callers use
121 # self.boot_images_dict as the "return" value. Suppressing
122 # exceptions here gives the caller no reason to doubt that
123 # boot_images_dict is not utter garbage and so pass it up the
124 # stack where it is then acted upon, to empty out BootSourceCache
125 # for example. True story.
126 raise
120127
121128
122def value_passes_filter_list(filter_list, property_value):129def value_passes_filter_list(filter_list, property_value):
123130
=== modified file 'src/provisioningserver/import_images/tests/test_download_descriptions.py'
--- src/provisioningserver/import_images/tests/test_download_descriptions.py 2016-09-30 19:08:51 +0000
+++ src/provisioningserver/import_images/tests/test_download_descriptions.py 2017-01-26 09:43:15 +0000
@@ -393,20 +393,18 @@
393 ]393 ]
394 self.assertItemsEqual(image_specs, list(boot_images_dict.mapping))394 self.assertItemsEqual(image_specs, list(boot_images_dict.mapping))
395395
396 def test_sync_does_not_propagate_ioerror(self):396 def test_sync_does_propagate_ioerror(self):
397 io_error = factory.make_exception_type(bases=(IOError,))
398
397 mock_sync = self.patch(download_descriptions.BasicMirrorWriter, "sync")399 mock_sync = self.patch(download_descriptions.BasicMirrorWriter, "sync")
398 mock_sync.side_effect = IOError()400 mock_sync.side_effect = io_error()
399401
400 boot_images_dict = BootImageMapping()402 boot_images_dict = BootImageMapping()
401 dumper = RepoDumper(boot_images_dict)403 dumper = RepoDumper(boot_images_dict)
402404
403 with FakeLogger("maas.import-images", level=logging.INFO) as maaslog:405 with FakeLogger("maas.import-images", level=logging.INFO) as maaslog:
404 # What we're testing here is that sync() doesn't raise IOError...406 self.assertRaises(
405 dumper.sync(sentinel.reader, sentinel.path)407 io_error, dumper.sync, sentinel.reader, sentinel.path)
406 # ... but we'll validate that we properly called the [mock]
407 # superclass method, and logged something, as well.
408 self.assertThat(
409 mock_sync, MockCalledOnceWith(sentinel.reader, sentinel.path))
410 self.assertDocTestMatches(408 self.assertDocTestMatches(
411 "...error...syncing boot images...", maaslog.output)409 "...error...syncing boot images...", maaslog.output)
412410

Subscribers

People subscribed via source and target branches

to all changes: