From 6f3c24eacefa82c3a9024e702d974d404d349c6f Mon Sep 17 00:00:00 2001 From: Dmitry Bilunov Date: Fri, 26 Feb 2016 17:07:37 +0300 Subject: [PATCH] Avoid swapspace uuid's time_mid collision with minix fs magic mkswap defaults to generating a random UUID. The second field of that UUID, time_mid, is written at offset 0x410. If a random generated value yields to have 0x8f13 in this very location, blkid mistakenly treats the block device as a minix filesystem superblock and then fails reading the structure. This bug is triggered with probability of 2^-16. We can work around this bug by re-running mkfs several times unless blkid is able to examine the newly-created filesystem. Change-Id: I13209faeb4e1d2798b7b0e0e8382e43803998d36 Closes-Bug: #1550227 --- bareon/tests/test_fs_utils.py | 68 ++++++++++++++++++++++++++++++----- bareon/utils/fs.py | 25 ++++++++++++- 2 files changed, 84 insertions(+), 9 deletions(-) diff --git a/bareon/tests/test_fs_utils.py b/bareon/tests/test_fs_utils.py index 1742c8a..b6d1761 100644 --- a/bareon/tests/test_fs_utils.py +++ b/bareon/tests/test_fs_utils.py @@ -25,22 +25,40 @@ class TestFSUtils(unittest2.TestCase): def test_make_xfs_add_f_flag(self, mock_exec): fu.make_fs('xfs', '--other-options --passed', '', '/dev/fake') - mock_exec.assert_called_once_with('mkfs.xfs', '--other-options', - '--passed', '-f', '/dev/fake') + expected_calls = [ + mock.call('mkfs.xfs', '--other-options', '--passed', + '-f', '/dev/fake'), + mock.call('blkid', '-c', '/dev/null', '-o', 'value', + '-s', 'UUID', '/dev/fake') + ] + self.assertEqual(mock_exec.call_args_list, expected_calls) def test_make_xfs_empty_options(self, mock_exec): fu.make_fs('xfs', '', '', '/dev/fake') - mock_exec.assert_called_once_with('mkfs.xfs', '-f', '/dev/fake') + expected_calls = [ + mock.call('mkfs.xfs', '-f', '/dev/fake'), + mock.call('blkid', '-c', '/dev/null', '-o', 'value', + '-s', 'UUID', '/dev/fake') + ] + self.assertEqual(mock_exec.call_args_list, expected_calls) def test_make_fs(self, mock_exec): fu.make_fs('ext4', '-F', 'fake_label', '/dev/fake') - mock_exec.assert_called_once_with('mkfs.ext4', '-F', '-L', - 'fake_label', '/dev/fake') + expected_calls = [ + mock.call('mkfs.ext4', '-F', '-L', 'fake_label', '/dev/fake'), + mock.call('blkid', '-c', '/dev/null', '-o', 'value', + '-s', 'UUID', '/dev/fake') + ] + self.assertEqual(mock_exec.call_args_list, expected_calls) def test_make_fs_swap(self, mock_exec): - fu.make_fs('swap', '-f', 'fake_label', '/dev/fake') - mock_exec.assert_called_once_with('mkswap', '-f', '-L', 'fake_label', - '/dev/fake') + fu.make_fs('swap', '', 'fake_label', '/dev/fake') + expected_calls = [ + mock.call('mkswap', '-f', '-L', 'fake_label', '/dev/fake'), + mock.call('blkid', '-c', '/dev/null', '-o', 'value', + '-s', 'UUID', '/dev/fake') + ] + self.assertEqual(mock_exec.call_args_list, expected_calls) def test_extend_fs_ok_ext2(self, mock_exec): fu.extend_fs('ext2', '/dev/fake') @@ -142,3 +160,37 @@ class TestFSUtils(unittest2.TestCase): self.assertEqual(fu.format_fs_label(long_label), template.format(long_label_trimmed)) + + +class TestFSRetry(unittest2.TestCase): + + def test_make_fs_bad_swap_retry(self): + # We mock utils.execute to throw an exception on first two + # invocations of blkid to test the retry loop. + rvs = [ + None, errors.ProcessExecutionError(), + None, errors.ProcessExecutionError(), + None, None + ] + with mock.patch.object(utils, 'execute', side_effect=rvs) as mock_exec: + fu.make_fs('swap', '', 'fake_label', '/dev/fake') + expected_calls = 3 * [ + mock.call('mkswap', '-f', '-L', 'fake_label', '/dev/fake'), + mock.call('blkid', '-c', '/dev/null', '-o', 'value', + '-s', 'UUID', '/dev/fake') + ] + self.assertEqual(mock_exec.call_args_list, expected_calls) + + def test_make_fs_bad_swap_failure(self): + # We mock utils.execute to throw an exception on invocations + # of blkid (MAX_MKFS_TRIES times) to see if it fails. + rvs = fu.MAX_MKFS_TRIES * [None, errors.ProcessExecutionError()] + with mock.patch.object(utils, 'execute', side_effect=rvs) as mock_exec: + with self.assertRaises(errors.FsUtilsError): + fu.make_fs('swap', '', 'fake_label', '/dev/fake') + expected_calls = 3 * [ + mock.call('mkswap', '-f', '-L', 'fake_label', '/dev/fake'), + mock.call('blkid', '-c', '/dev/null', '-o', 'value', + '-s', 'UUID', '/dev/fake') + ] + self.assertEqual(mock_exec.call_args_list, expected_calls) diff --git a/bareon/utils/fs.py b/bareon/utils/fs.py index e92e1fb..6cb8e9d 100644 --- a/bareon/utils/fs.py +++ b/bareon/utils/fs.py @@ -19,7 +19,10 @@ from bareon import errors from bareon.openstack.common import log as logging from bareon.utils import utils +import six + LOG = logging.getLogger(__name__) +MAX_MKFS_TRIES = 5 def format_fs_label(label): @@ -45,11 +48,31 @@ def make_fs(fs_type, fs_options, fs_label, dev): # NOTE(agordeev): force xfs creation. # Othwerwise, it will fail to proceed if filesystem exists. fs_options += ' -f ' + if fs_type == 'swap': + fs_options += ' -f ' cmd_line.append(cmd_name) for opt in (fs_options, format_fs_label(fs_label)): cmd_line.extend([s for s in opt.split(' ') if s]) cmd_line.append(dev) - utils.execute(*cmd_line) + + # NOTE(dbilunov): make sure the newly-created fs can + # be observed by blkid. Currently known problem is + # that generated UUID could possibly collide with + # minix filesystem magic (0x8f13) + mkfs_ok = False + for _ in six.moves.range(MAX_MKFS_TRIES): + utils.execute(*cmd_line) + try: + utils.execute('blkid', '-c', '/dev/null', '-o', 'value', + '-s', 'UUID', dev) + except errors.ProcessExecutionError: + LOG.warning('blkid has failed on %s, retrying...', dev) + else: + mkfs_ok = True + break + if not mkfs_ok: + raise errors.FsUtilsError('Cannot get UUID of a newly-created ' + + '{0} on {1}'.format(fs_type, dev)) def extend_fs(fs_type, fs_dev):