openstack/ironic-lib
Code Issues Proposed changes

Simplify logic in wait_for_disk_to_become_available

Browse Source 
* 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
This commit is contained in:
John L. Villalovos 2018-01-17 10:09:02 -08:00
parent 0d65451cfc
commit f3279dffe2
4 changed files with 99 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
ironic_lib/tests/test_disk_partitioner.py
View File

@ -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(

10
ironic_lib/tests/test_disk_utils.py
View File

@ -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)

75
ironic_lib/tests/test_utils.py
View File

@ -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])

17
ironic_lib/utils.py
View File

@ -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})