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
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-26 09:43:15 +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-26 09:43:15 +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

Subscribers

People subscribed via source and target branches

to all changes: