Merge lp:~raharper/curtin/trunk.bcache-toctou-v2 into lp:~curtin-dev/curtin/trunk

[Ryan Harper]

clear_holders: bcache log IO/OS exceptions but do not raise

While stopping bcache devices, curtin races with the kernel w.r.t how
quickly it removes the bcache device (and the sysfs tree related).
The result is that as curtin attempts to write "1" into the sysfs tree
of bcache device to stop it, the range of errors changes depending on
when we race with the kernel asynchronously removing the sysfs path.
Therefore we log the exception errno we got but do not re-raise. The
the calling process is watching whether the same sysfs path is being
removed; if the bcache device fails to go away then curtin will have a
log of the exceptions to debug.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:542
https://jenkins.ubuntu.com/server/job/curtin-ci/672/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/672
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/672
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/672
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/672

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/672/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

Thanks Ryan

review: Approve

[Ryan Harper (raharper)]

vmtest on jenkins approves as well

https://jenkins.ubuntu.com/server/view/curtin/job/curtin-vmtest-devel-debug/17/console

----------------------------------------------------------------------
Ran 2219 tests in 8642.899s

OK (SKIP=162)
Thu, 02 Nov 2017 16:43:35 +0000: vmtest end [0] in 8646s

Merging to trunk now.

=== modified file 'curtin/block/clear_holders.py'
--- curtin/block/clear_holders.py	2017-10-23 16:49:46 +0000
+++ curtin/block/clear_holders.py	2017-11-01 20:23:47 +0000
@@ -99,14 +99,16 @@
     try:
         util.write_file(bcache_stop, '1', mode=None)
     except (IOError, OSError) as e:
-        # Note: if we get a write failure and the exception is that the
-        # file is not found; we log this expected behavior; any other
-        # exception is raised
-        if e.errno == errno.ENOENT:
-            LOG.debug('bcache stop file %s missing, device removed: %s',
-                      bcache_stop, e)
-        else:
-            raise e
+        # Note: if we get any exceptions in the above exception classes
+        # it is a result of attempting to write "1" into the sysfs path
+        # The range of errors changes depending on when we race with
+        # the kernel asynchronously removing the sysfs path. Therefore
+        # we log the exception errno we got, but do not re-raise as
+        # the calling process is watching whether the same sysfs path
+        # is being removed;  if it fails to go away then we'll have
+        # a log of the exceptions to debug.
+        LOG.debug('Error writing to bcache stop file %s, device removed: %s',
+                  bcache_stop, e)
 
 
 def shutdown_bcache(device):

=== modified file 'tests/unittests/test_clear_holders.py'
--- tests/unittests/test_clear_holders.py	2017-10-13 17:48:29 +0000
+++ tests/unittests/test_clear_holders.py	2017-11-01 20:23:47 +0000
@@ -580,11 +580,11 @@
 
     @mock.patch('curtin.block.clear_holders.util.write_file')
     def test_maybe_stop_bcache_device_raises_errors(self, m_write_file):
-        """Unexpected exceptions are raised by maybe_stop_bcache_device."""
-        m_write_file.side_effect = OSError(errno.EAGAIN, 'Unexpected errno')
-        with self.assertRaises(OSError) as cm:
+        """Non-IO/OS exceptions are raised by maybe_stop_bcache_device."""
+        m_write_file.side_effect = ValueError('Crazy Value Error')
+        with self.assertRaises(ValueError) as cm:
             clear_holders.maybe_stop_bcache_device('does/not/matter')
-        self.assertEqual('[Errno 11] Unexpected errno', str(cm.exception))
+        self.assertEqual('Crazy Value Error', str(cm.exception))
         self.assertEqual(
             mock.call('does/not/matter/stop', '1', mode=None),
             m_write_file.call_args)
@@ -597,7 +597,7 @@
         m_write_file.side_effect = OSError(errno.ENOENT, 'Expected oserror')
         clear_holders.maybe_stop_bcache_device('does/not/matter')
         self.assertEqual(
-            'bcache stop file %s missing, device removed: %s',
+            'Error writing to bcache stop file %s, device removed: %s',
             m_log.debug.call_args[0][0])
         self.assertEqual('does/not/matter/stop', m_log.debug.call_args[0][1])
 
@@ -609,7 +609,7 @@
         m_write_file.side_effect = IOError(errno.ENOENT, 'Expected ioerror')
         clear_holders.maybe_stop_bcache_device('does/not/matter')
         self.assertEqual(
-            'bcache stop file %s missing, device removed: %s',
+            'Error writing to bcache stop file %s, device removed: %s',
             m_log.debug.call_args[0][0])
         self.assertEqual('does/not/matter/stop', m_log.debug.call_args[0][1])