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

Proposed by Gavin Panella on 2017-01-11
Status: Merged
Approved by: Gavin Panella on 2017-01-11
Approved revision: 5636
Merged at revision: 5636
Proposed branch: lp:~allenap/maas/repo-dumper-do-not-suppress-io-errors
Merge into: lp:maas/trunk
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
Reviewer Review Type Date Requested Status
Brendan Donegan (community) Needs Information on 2017-01-11
Mike Pontillo (community) 2017-01-11 Approve on 2017-01-11
Review via email: mp+314524@code.launchpad.net

Commit message

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.
Brendan Donegan (brendan-donegan) wrote :

I'm not going to fully review this, but will say that CI tests point to it alleviating the issues mentioned above - notably we don't randomly end up with an empty boot source cache.

Mike Pontillo (mpontillo) wrote :

Nice find. Thanks for the fix.

review: Approve
Brendan Donegan (brendan-donegan) wrote :

Erp, I just noticed one thing, but it's so minor it won't matter if it lands

review: Needs Information
5636. By Gavin Panella on 2017-01-11

Fix name in comment.

Gavin Panella (allenap) wrote :

> Erp, I just noticed one thing, but it's so minor it
> won't matter if it lands

Fixed!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/provisioningserver/import_images/download_descriptions.py'
2--- src/provisioningserver/import_images/download_descriptions.py 2016-09-30 13:56:04 +0000
3+++ src/provisioningserver/import_images/download_descriptions.py 2017-01-11 17:28:28 +0000
4@@ -117,6 +117,13 @@
5 maaslog.warning(
6 "I/O error while syncing boot images. If this problem "
7 "persists, verify network connectivity and disk usage.")
8+ # This MUST NOT suppress the I/O error because callers use
9+ # self.boot_images_dict as the "return" value. Suppressing
10+ # exceptions here gives the caller no reason to doubt that
11+ # boot_images_dict is not utter garbage and so pass it up the
12+ # stack where it is then acted upon, to empty out BootSourceCache
13+ # for example. True story.
14+ raise
15
16
17 def value_passes_filter_list(filter_list, property_value):
18
19=== modified file 'src/provisioningserver/import_images/tests/test_download_descriptions.py'
20--- src/provisioningserver/import_images/tests/test_download_descriptions.py 2016-09-30 19:08:51 +0000
21+++ src/provisioningserver/import_images/tests/test_download_descriptions.py 2017-01-11 17:28:28 +0000
22@@ -393,20 +393,18 @@
23 ]
24 self.assertItemsEqual(image_specs, list(boot_images_dict.mapping))
25
26- def test_sync_does_not_propagate_ioerror(self):
27+ def test_sync_does_propagate_ioerror(self):
28+ io_error = factory.make_exception_type(bases=(IOError,))
29+
30 mock_sync = self.patch(download_descriptions.BasicMirrorWriter, "sync")
31- mock_sync.side_effect = IOError()
32+ mock_sync.side_effect = io_error()
33
34 boot_images_dict = BootImageMapping()
35 dumper = RepoDumper(boot_images_dict)
36
37 with FakeLogger("maas.import-images", level=logging.INFO) as maaslog:
38- # What we're testing here is that sync() doesn't raise IOError...
39- dumper.sync(sentinel.reader, sentinel.path)
40- # ... but we'll validate that we properly called the [mock]
41- # superclass method, and logged something, as well.
42- self.assertThat(
43- mock_sync, MockCalledOnceWith(sentinel.reader, sentinel.path))
44+ self.assertRaises(
45+ io_error, dumper.sync, sentinel.reader, sentinel.path)
46 self.assertDocTestMatches(
47 "...error...syncing boot images...", maaslog.output)
48