From f3279dffe2993b1821dae396d4defd3f026abd3b Mon Sep 17 00:00:00 2001 From: "John L. Villalovos" Date: Wed, 17 Jan 2018 10:09:02 -0800 Subject: [PATCH] Simplify logic in wait_for_disk_to_become_available * Simplify the logic in utils.py/wait_for_disk_to_become_available * Correct unit tests to not use None as a return value for calls to utils.execute() that return no output. In reality utils.execute() will return empty strings for example: ('', '') * Add unit tests to test 'psmisc' and 'busybox' versions of output data from 'fuser' * Add a shorter check device interval as test was taking over 2 seconds and showing up in longest test run time list. Change-Id: Ibc481e4a76c63612e1774b5c7a8144ee934eb2f5 --- ironic_lib/tests/test_disk_partitioner.py | 16 ++--- ironic_lib/tests/test_disk_utils.py | 10 +-- ironic_lib/tests/test_utils.py | 75 ++++++++++++++++++++++- ironic_lib/utils.py | 17 +++-- 4 files changed, 99 insertions(+), 19 deletions(-) diff --git a/ironic_lib/tests/test_disk_partitioner.py b/ironic_lib/tests/test_disk_partitioner.py index ea74e0aa..c63b6a84 100644 --- a/ironic_lib/tests/test_disk_partitioner.py +++ b/ironic_lib/tests/test_disk_partitioner.py @@ -70,7 +70,7 @@ class DiskPartitionerTestCase(base.IronicLibTestCase): 'size': 1})] with mock.patch.object(dp, 'get_partitions', autospec=True) as mock_gp: mock_gp.return_value = fake_parts - mock_utils_exc.return_value = (None, None) + mock_utils_exc.return_value = ('', '') dp.commit() mock_disk_partitioner_exec.assert_called_once_with( @@ -99,10 +99,8 @@ class DiskPartitionerTestCase(base.IronicLibTestCase): 'fs_type': 'fake-fs-type', 'type': 'fake-type', 'size': 1})] - # TODO(TheJulia): fuser man page indicates only pids are returned via - # stdout. Meaning tests that put the device on stdout need to be - # corrected. - fuser_outputs = iter([("/dev/fake: 10000 10001", None), (None, None)]) + # Test as if the 'psmisc' version of 'fuser' which has stderr output + fuser_outputs = iter([(" 10000 10001", '/dev/fake:\n'), ('', '')]) with mock.patch.object(dp, 'get_partitions', autospec=True) as mock_gp: mock_gp.return_value = fake_parts @@ -137,7 +135,9 @@ class DiskPartitionerTestCase(base.IronicLibTestCase): with mock.patch.object(dp, 'get_partitions', autospec=True) as mock_gp: mock_gp.return_value = fake_parts - mock_utils_exc.return_value = ("/dev/fake: 10000 10001", None) + # Test as if the 'busybox' version of 'fuser' which does not have + # stderr output + mock_utils_exc.return_value = ("10000 10001", '') self.assertRaises(exception.InstanceDeployFailure, dp.commit) mock_disk_partitioner_exec.assert_called_once_with( @@ -169,8 +169,8 @@ class DiskPartitionerTestCase(base.IronicLibTestCase): with mock.patch.object(dp, 'get_partitions', autospec=True) as mock_gp: mock_gp.return_value = fake_parts - mock_utils_exc.return_value = (None, "Specified filename /dev/fake" - " does not exist.") + mock_utils_exc.return_value = ('', "Specified filename /dev/fake" + " does not exist.") self.assertRaises(exception.InstanceDeployFailure, dp.commit) mock_disk_partitioner_exec.assert_called_once_with( diff --git a/ironic_lib/tests/test_disk_utils.py b/ironic_lib/tests/test_disk_utils.py index 2433a087..9134c9e8 100644 --- a/ironic_lib/tests/test_disk_utils.py +++ b/ironic_lib/tests/test_disk_utils.py @@ -315,7 +315,7 @@ class MakePartitionsTestCase(base.IronicLibTestCase): def _test_make_partitions(self, mock_exc, boot_option, boot_mode='bios', disk_label=None): - mock_exc.return_value = (None, None) + mock_exc.return_value = ('', '') disk_utils.make_partitions(self.dev, self.root_mb, self.swap_mb, self.ephemeral_mb, self.configdrive_mb, self.node_uuid, boot_option=boot_option, @@ -386,7 +386,7 @@ class MakePartitionsTestCase(base.IronicLibTestCase): 'mkpart', 'primary', '', '2561', '3585'] self.dev = 'fake-dev' cmd = self._get_parted_cmd(self.dev) + expected_mkpart - mock_exc.return_value = (None, None) + mock_exc.return_value = ('', '') disk_utils.make_partitions(self.dev, self.root_mb, self.swap_mb, self.ephemeral_mb, self.configdrive_mb, self.node_uuid) @@ -410,7 +410,7 @@ class MakePartitionsTestCase(base.IronicLibTestCase): 'swap': swap, 'root': root} cmd = self._get_parted_cmd(self.dev) + expected_mkpart - mock_exc.return_value = (None, None) + mock_exc.return_value = ('', '') result = disk_utils.make_partitions( self.dev, self.root_mb, self.swap_mb, self.ephemeral_mb, self.configdrive_mb, self.node_uuid) @@ -433,7 +433,7 @@ class MakePartitionsTestCase(base.IronicLibTestCase): 'swap': swap, 'root': root} cmd = self._get_parted_cmd(self.dev) + expected_mkpart - mock_exc.return_value = (None, None) + mock_exc.return_value = ('', '') result = disk_utils.make_partitions( self.dev, self.root_mb, self.swap_mb, self.ephemeral_mb, self.configdrive_mb, self.node_uuid) @@ -453,7 +453,7 @@ class MakePartitionsTestCase(base.IronicLibTestCase): 'swap': 'fake-dev2', 'root': 'fake-dev3'} cmd = self._get_parted_cmd(self.dev) + expected_mkpart - mock_exc.return_value = (None, None) + mock_exc.return_value = ('', '') result = disk_utils.make_partitions( self.dev, self.root_mb, self.swap_mb, self.ephemeral_mb, self.configdrive_mb, self.node_uuid) diff --git a/ironic_lib/tests/test_utils.py b/ironic_lib/tests/test_utils.py index 9848f40b..d95112ad 100644 --- a/ironic_lib/tests/test_utils.py +++ b/ironic_lib/tests/test_utils.py @@ -481,6 +481,7 @@ class WaitForDisk(base.IronicLibTestCase): side_effect=processutils.ProcessExecutionError( stderr='fake')) def test_wait_for_disk_to_become_available_no_fuser(self, mock_exc): + CONF.disk_partitioner.check_device_interval = .01 CONF.disk_partitioner.check_device_max_retries = 2 self.assertRaises(exception.IronicException, utils.wait_for_disk_to_become_available, @@ -492,13 +493,17 @@ class WaitForDisk(base.IronicLibTestCase): mock_exc.assert_has_calls([fuser_call, fuser_call]) @mock.patch.object(utils, 'execute', autospec=True) - def test_wait_for_disk_to_become_available_device_in_use(self, mock_exc): + def test_wait_for_disk_to_become_available_device_in_use_psmisc( + self, mock_exc): + # Test that the device is not available. This version has the 'psmisc' + # version of 'fuser' values for stdout and stderr. # NOTE(TheJulia): Looks like fuser returns the actual list of pids # in the stdout output, where as all other text is returned in # stderr. CONF.disk_partitioner.check_device_interval = .01 CONF.disk_partitioner.check_device_max_retries = 2 - + # The 'psmisc' version has a leading space character in stdout. The + # filename is output to stderr mock_exc.side_effect = [(' 1234 ', 'fake-dev: '), (' 15503 3919 15510 15511', 'fake-dev:')] expected_error = ('Processes with the following PIDs are ' @@ -515,6 +520,34 @@ class WaitForDisk(base.IronicLibTestCase): self.assertEqual(2, mock_exc.call_count) mock_exc.assert_has_calls([fuser_call, fuser_call]) + @mock.patch.object(utils, 'execute', autospec=True) + def test_wait_for_disk_to_become_available_device_in_use_busybox( + self, mock_exc): + # Test that the device is not available. This version has the 'busybox' + # version of 'fuser' values for stdout and stderr. + # NOTE(TheJulia): Looks like fuser returns the actual list of pids + # in the stdout output, where as all other text is returned in + # stderr. + CONF.disk_partitioner.check_device_interval = .01 + CONF.disk_partitioner.check_device_max_retries = 2 + # The 'busybox' version does not have a leading space character in + # stdout. Also nothing is output to stderr. + mock_exc.side_effect = [('1234', ''), + ('15503 3919 15510 15511', '')] + expected_error = ('Processes with the following PIDs are ' + 'holding device fake-dev: 15503, 3919, 15510, ' + '15511. Timed out waiting for completion.') + self.assertRaisesRegex( + exception.IronicException, + expected_error, + utils.wait_for_disk_to_become_available, + 'fake-dev') + fuser_cmd = ['fuser', 'fake-dev'] + fuser_call = mock.call(*fuser_cmd, run_as_root=True, + check_exit_code=[0, 1]) + self.assertEqual(2, mock_exc.call_count) + mock_exc.assert_has_calls([fuser_call, fuser_call]) + @mock.patch.object(utils, 'execute', autospec=True) def test_wait_for_disk_to_become_available_no_device(self, mock_exc): # NOTE(TheJulia): Looks like fuser returns the actual list of pids @@ -539,3 +572,41 @@ class WaitForDisk(base.IronicLibTestCase): check_exit_code=[0, 1]) self.assertEqual(2, mock_exc.call_count) mock_exc.assert_has_calls([fuser_call, fuser_call]) + + @mock.patch.object(utils, 'execute', autospec=True) + def test_wait_for_disk_to_become_available_dev_becomes_avail_psmisc( + self, mock_exc): + # Test that initially device is not available but then becomes + # available. This version has the 'psmisc' version of 'fuser' values + # for stdout and stderr. + CONF.disk_partitioner.check_device_interval = .01 + CONF.disk_partitioner.check_device_max_retries = 2 + # The 'psmisc' version has a leading space character in stdout. The + # filename is output to stderr + mock_exc.side_effect = [(' 1234 ', 'fake-dev: '), + ('', '')] + utils.wait_for_disk_to_become_available('fake-dev') + fuser_cmd = ['fuser', 'fake-dev'] + fuser_call = mock.call(*fuser_cmd, run_as_root=True, + check_exit_code=[0, 1]) + self.assertEqual(2, mock_exc.call_count) + mock_exc.assert_has_calls([fuser_call, fuser_call]) + + @mock.patch.object(utils, 'execute', autospec=True) + def test_wait_for_disk_to_become_available_dev_becomes_avail_busybox( + self, mock_exc): + # Test that initially device is not available but then becomes + # available. This version has the 'busybox' version of 'fuser' values + # for stdout and stderr. + CONF.disk_partitioner.check_device_interval = .01 + CONF.disk_partitioner.check_device_max_retries = 2 + # The 'busybox' version does not have a leading space character in + # stdout. Also nothing is output to stderr. + mock_exc.side_effect = [('1234 5895', ''), + ('', '')] + utils.wait_for_disk_to_become_available('fake-dev') + fuser_cmd = ['fuser', 'fake-dev'] + fuser_call = mock.call(*fuser_cmd, run_as_root=True, + check_exit_code=[0, 1]) + self.assertEqual(2, mock_exc.call_count) + mock_exc.assert_has_calls([fuser_call, fuser_call]) diff --git a/ironic_lib/utils.py b/ironic_lib/utils.py index 68f3febb..023b680d 100644 --- a/ironic_lib/utils.py +++ b/ironic_lib/utils.py @@ -450,6 +450,13 @@ def wait_for_disk_to_become_available(device): if retries[0] > max_retries: raise loopingcall.LoopingCallDone() + # There are 'psmisc' and 'busybox' versions of the 'fuser' program. The + # 'fuser' programs differ in how they output data to stderr. The + # busybox version does not output the filename to stderr, while the + # standard 'psmisc' version does output the filename to stderr. How + # they output to stdout is almost identical in that only the PIDs are + # output to stdout, with the 'psmisc' version adding a leading space + # character to the list of PIDs. try: # NOTE(ifarkas): fuser returns a non-zero return code if none of # the specified files is accessed. @@ -466,12 +473,14 @@ def wait_for_disk_to_become_available(device): out, err = execute('fuser', device, check_exit_code=[0, 1], run_as_root=True) - if err: - stderr[0] = err - if out: - pids[0] = fuser_pids_re.findall(out) if not out and not err: raise loopingcall.LoopingCallDone() + + stderr[0] = err + # NOTE: findall() returns a list of matches, or an empty list if no + # matches + pids[0] = fuser_pids_re.findall(out) + except processutils.ProcessExecutionError as exc: LOG.warning('Failed to check the device %(device)s with fuser:' ' %(err)s', {'device': device, 'err': exc})