From 2f6c37569aae281c233934c544b3080f2ef75d55 Mon Sep 17 00:00:00 2001 From: Vladimir Kozhukalov Date: Wed, 14 Jan 2015 10:20:50 +0300 Subject: [PATCH] fuel_agent: removed reread_partitions method This reread_partitions method was a desperate attempt to work around udev related "device is busy" error. The correct way to deal with that stuff is to use udevadm --settle which is to block thread until udev is ready to handle events. Closes-Bug: 1410471 Change-Id: Idb0dccb35aab10d02c5ad942fd30d52a461e1a0e --- fuel_agent/manager.py | 32 +++---- fuel_agent/tests/test_partition_utils.py | 110 ++++++++++------------- fuel_agent/utils/partition_utils.py | 33 ++----- 3 files changed, 61 insertions(+), 114 deletions(-) diff --git a/fuel_agent/manager.py b/fuel_agent/manager.py index 4e766c7..51ff376 100644 --- a/fuel_agent/manager.py +++ b/fuel_agent/manager.py @@ -13,7 +13,6 @@ # limitations under the License. import os -import time from oslo.config import cfg @@ -80,30 +79,19 @@ class Manager(object): lu.pvremove_all() for parted in self.partition_scheme.parteds: + for prt in parted.partitions: + # We wipe out the beginning of every new partition + # even before creating it. It allows us to avoid possible + # interactive dialog if some data (metadata or file system) + # present on this new partition and it also allows udev not + # hanging trying to parse this data. + utils.execute('dd', 'if=/dev/zero', 'bs=1M', + 'seek=%s' % max(prt.begin - 1, 0), 'count=2', + 'of=%s' % prt.device, check_exit_code=[0]) + pu.make_label(parted.name, parted.label) for prt in parted.partitions: pu.make_partition(prt.device, prt.begin, prt.end, prt.type) - # We wipe out the beginning of every new partition - # right after creating it. It allows us to avoid possible - # interactive dialog if some data (metadata or file system) - # present on this new partition. - timestamp = time.time() - while 1: - if time.time() > timestamp + 30: - raise errors.PartitionNotFoundError( - 'Error while wiping data on partition %s.' - 'Partition not found' % prt.name) - try: - utils.execute('test', '-e', prt.name, - check_exit_code=[0]) - except errors.ProcessExecutionError: - time.sleep(1) - continue - else: - utils.execute('dd', 'if=/dev/zero', 'bs=1M', 'count=1', - 'of=%s' % prt.name, check_exit_code=[0]) - break - for flag in prt.flags: pu.set_partition_flag(prt.device, prt.count, flag) if prt.guid: diff --git a/fuel_agent/tests/test_partition_utils.py b/fuel_agent/tests/test_partition_utils.py index 447cc05..64f8571 100644 --- a/fuel_agent/tests/test_partition_utils.py +++ b/fuel_agent/tests/test_partition_utils.py @@ -14,7 +14,6 @@ import mock from oslotest import base as test_base -import time from fuel_agent import errors from fuel_agent.utils import partition_utils as pu @@ -30,28 +29,30 @@ class TestPartitionUtils(test_base.BaseTestCase): pu.wipe('/dev/fake') mock_label.assert_called_once_with('/dev/fake') - @mock.patch.object(pu, 'reread_partitions') @mock.patch.object(utils, 'execute') - def test_make_label(self, mock_exec, mock_rerd): + def test_make_label(self, mock_exec): # should run parted OS command # in order to create label on a device mock_exec.return_value = ('out', '') # gpt by default + expected_calls = [ + mock.call('udevadm', 'settle', '--quiet', check_exit_code=[0]), + mock.call('parted', '-s', '/dev/fake', + 'mklabel', 'gpt', check_exit_code=[0, 1])] + pu.make_label('/dev/fake') - mock_exec.assert_called_once_with( - 'parted', '-s', '/dev/fake', - 'mklabel', 'gpt', check_exit_code=[0, 1]) - mock_rerd.assert_called_once_with('/dev/fake', out='out') + self.assertEqual(mock_exec.call_args_list, expected_calls) mock_exec.reset_mock() - mock_rerd.reset_mock() # label is set explicitly + expected_calls = [ + mock.call('udevadm', 'settle', '--quiet', check_exit_code=[0]), + mock.call('parted', '-s', '/dev/fake', + 'mklabel', 'msdos', check_exit_code=[0, 1])] + pu.make_label('/dev/fake', label='msdos') - mock_exec.assert_called_once_with( - 'parted', '-s', '/dev/fake', - 'mklabel', 'msdos', check_exit_code=[0, 1]) - mock_rerd.assert_called_once_with('/dev/fake', out='out') + self.assertEqual(mock_exec.call_args_list, expected_calls) def test_make_label_wrong_label(self): # should check if label is valid @@ -59,28 +60,28 @@ class TestPartitionUtils(test_base.BaseTestCase): self.assertRaises(errors.WrongPartitionLabelError, pu.make_label, '/dev/fake', 'wrong') - @mock.patch.object(pu, 'reread_partitions') @mock.patch.object(utils, 'execute') - def test_set_partition_flag(self, mock_exec, mock_rerd): + def test_set_partition_flag(self, mock_exec): # should run parted OS command # in order to set flag on a partition mock_exec.return_value = ('out', '') # default state is 'on' + expected_calls = [ + mock.call('udevadm', 'settle', '--quiet', check_exit_code=[0]), + mock.call('parted', '-s', '/dev/fake', 'set', '1', 'boot', 'on', + check_exit_code=[0, 1])] pu.set_partition_flag('/dev/fake', 1, 'boot') - mock_exec.assert_called_once_with( - 'parted', '-s', '/dev/fake', 'set', '1', 'boot', 'on', - check_exit_code=[0, 1]) - mock_rerd.assert_called_once_with('/dev/fake', out='out') + self.assertEqual(mock_exec.call_args_list, expected_calls) mock_exec.reset_mock() - mock_rerd.reset_mock() # if state argument is given use it + expected_calls = [ + mock.call('udevadm', 'settle', '--quiet', check_exit_code=[0]), + mock.call('parted', '-s', '/dev/fake', 'set', '1', 'boot', 'off', + check_exit_code=[0, 1])] pu.set_partition_flag('/dev/fake', 1, 'boot', state='off') - mock_exec.assert_called_once_with( - 'parted', '-s', '/dev/fake', 'set', '1', 'boot', 'off', - check_exit_code=[0, 1]) - mock_rerd.assert_called_once_with('/dev/fake', out='out') + self.assertEqual(mock_exec.call_args_list, expected_calls) @mock.patch.object(utils, 'execute') def test_set_partition_flag_wrong_flag(self, mock_exec): @@ -98,10 +99,9 @@ class TestPartitionUtils(test_base.BaseTestCase): pu.set_partition_flag, '/dev/fake', 1, 'boot', state='wrong') - @mock.patch.object(pu, 'reread_partitions') @mock.patch.object(pu, 'info') @mock.patch.object(utils, 'execute') - def test_make_partition(self, mock_exec, mock_info, mock_rerd): + def test_make_partition(self, mock_exec, mock_info): # should run parted OS command # in order to create new partition mock_exec.return_value = ('out', '') @@ -111,15 +111,16 @@ class TestPartitionUtils(test_base.BaseTestCase): {'begin': 0, 'end': 1000, 'fstype': 'free'}, ] } + expected_calls = [ + mock.call('udevadm', 'settle', '--quiet', check_exit_code=[0]), + mock.call('parted', + '-a', 'optimal', + '-s', '/dev/fake', + 'unit', 'MiB', + 'mkpart', 'primary', '100', '200', + check_exit_code=[0, 1])] pu.make_partition('/dev/fake', 100, 200, 'primary') - mock_exec.assert_called_once_with( - 'parted', - '-a', 'optimal', - '-s', '/dev/fake', - 'unit', 'MiB', - 'mkpart', 'primary', '100', '200', - check_exit_code=[0, 1]) - mock_rerd.assert_called_once_with('/dev/fake', out='out') + self.assertEqual(mock_exec.call_args_list, expected_calls) @mock.patch.object(utils, 'execute') def test_make_partition_wrong_ptype(self, mock_exec): @@ -157,10 +158,9 @@ class TestPartitionUtils(test_base.BaseTestCase): self.assertEqual(mock_info.call_args_list, [mock.call('/dev/fake')] * 3) - @mock.patch.object(pu, 'reread_partitions') @mock.patch.object(pu, 'info') @mock.patch.object(utils, 'execute') - def test_remove_partition(self, mock_exec, mock_info, mock_rerd): + def test_remove_partition(self, mock_exec, mock_info): # should run parted OS command # in order to remove partition mock_exec.return_value = ('out', '') @@ -182,10 +182,12 @@ class TestPartitionUtils(test_base.BaseTestCase): } ] } + expected_calls = [ + mock.call('udevadm', 'settle', '--quiet', check_exit_code=[0]), + mock.call('parted', '-s', '/dev/fake', 'rm', '1', + check_exit_code=[0])] pu.remove_partition('/dev/fake', 1) - mock_exec.assert_called_once_with( - 'parted', '-s', '/dev/fake', 'rm', '1', check_exit_code=[0]) - mock_rerd.assert_called_once_with('/dev/fake', out='out') + self.assertEqual(mock_exec.call_args_list, expected_calls) @mock.patch.object(pu, 'info') @mock.patch.object(utils, 'execute') @@ -216,9 +218,12 @@ class TestPartitionUtils(test_base.BaseTestCase): @mock.patch.object(utils, 'execute') def test_set_gpt_type(self, mock_exec): pu.set_gpt_type('dev', 'num', 'type') - mock_exec.assert_called_once_with('sgdisk', - '--typecode=%s:%s' % ('num', 'type'), - 'dev', check_exit_code=[0]) + expected_calls = [ + mock.call('udevadm', 'settle', '--quiet', check_exit_code=[0]), + mock.call('sgdisk', + '--typecode=%s:%s' % ('num', 'type'), + 'dev', check_exit_code=[0])] + self.assertEqual(mock_exec.call_args_list, expected_calls) @mock.patch.object(utils, 'execute') def test_info(self, mock_exec): @@ -250,26 +255,3 @@ class TestPartitionUtils(test_base.BaseTestCase): mock_exec.assert_called_once_with('parted', '-s', '/dev/fake', '-m', 'unit', 'MiB', 'print', 'free', check_exit_code=[0, 1]) - - @mock.patch.object(utils, 'execute') - def test_reread_partitions_ok(self, mock_exec): - pu.reread_partitions('/dev/fake', out='') - self.assertEqual(mock_exec.call_args_list, []) - - @mock.patch.object(time, 'sleep') - @mock.patch.object(utils, 'execute') - def test_reread_partitions_device_busy(self, mock_exec, mock_sleep): - mock_exec.return_value = ('', '') - pu.reread_partitions('/dev/fake', out='_Device or resource busy_') - mock_exec_expected = [ - mock.call('partprobe', '/dev/fake', check_exit_code=[0, 1]), - mock.call('partx', '-a', '/dev/fake', check_exit_code=[0, 1]) - ] - self.assertEqual(mock_exec.call_args_list, mock_exec_expected) - mock_sleep.assert_called_once_with(1) - - @mock.patch.object(utils, 'execute') - def test_reread_partitions_timeout(self, mock_exec): - self.assertRaises(errors.BaseError, pu.reread_partitions, - '/dev/fake', out='Device or resource busy', - timeout=-40) diff --git a/fuel_agent/utils/partition_utils.py b/fuel_agent/utils/partition_utils.py index f911d89..6db8cc8 100644 --- a/fuel_agent/utils/partition_utils.py +++ b/fuel_agent/utils/partition_utils.py @@ -12,8 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -import time - from fuel_agent import errors from fuel_agent.openstack.common import log as logging from fuel_agent.utils import utils @@ -79,10 +77,10 @@ def make_label(dev, label='gpt'): if label not in ('gpt', 'msdos'): raise errors.WrongPartitionLabelError( 'Wrong partition label type: %s' % label) + utils.execute('udevadm', 'settle', '--quiet', check_exit_code=[0]) out, err = utils.execute('parted', '-s', dev, 'mklabel', label, check_exit_code=[0, 1]) LOG.debug('Parted output: \n%s' % out) - reread_partitions(dev, out=out) def set_partition_flag(dev, num, flag, state='on'): @@ -107,10 +105,10 @@ def set_partition_flag(dev, num, flag, state='on'): if state not in ('on', 'off'): raise errors.WrongPartitionSchemeError( 'Wrong partition flag state: %s' % state) + utils.execute('udevadm', 'settle', '--quiet', check_exit_code=[0]) out, err = utils.execute('parted', '-s', dev, 'set', str(num), flag, state, check_exit_code=[0, 1]) LOG.debug('Parted output: \n%s' % out) - reread_partitions(dev, out=out) def set_gpt_type(dev, num, type_guid): @@ -127,6 +125,7 @@ def set_gpt_type(dev, num, type_guid): # TODO(kozhukalov): check whether type_guid is valid LOG.debug('Setting partition GUID: dev=%s num=%s guid=%s' % (dev, num, type_guid)) + utils.execute('udevadm', 'settle', '--quiet', check_exit_code=[0]) utils.execute('sgdisk', '--typecode=%s:%s' % (num, type_guid), dev, check_exit_code=[0]) @@ -150,11 +149,11 @@ def make_partition(dev, begin, end, ptype): 'Invalid boundaries: begin and end ' 'are not inside available free space') + utils.execute('udevadm', 'settle', '--quiet', check_exit_code=[0]) out, err = utils.execute( 'parted', '-a', 'optimal', '-s', dev, 'unit', 'MiB', 'mkpart', ptype, str(begin), str(end), check_exit_code=[0, 1]) LOG.debug('Parted output: \n%s' % out) - reread_partitions(dev, out=out) def remove_partition(dev, num): @@ -162,28 +161,6 @@ def remove_partition(dev, num): if not any(x['fstype'] != 'free' and x['num'] == num for x in info(dev)['parts']): raise errors.PartitionNotFoundError('Partition %s not found' % num) + utils.execute('udevadm', 'settle', '--quiet', check_exit_code=[0]) out, err = utils.execute('parted', '-s', dev, 'rm', str(num), check_exit_code=[0]) - reread_partitions(dev, out=out) - - -def reread_partitions(dev, out='Device or resource busy', timeout=30): - # The reason for this method to exist is that old versions of parted - # use ioctl(fd, BLKRRPART, NULL) to tell Linux to re-read partitions. - # This system call does not work sometimes. So we try to re-read partition - # table several times. Besides partprobe uses BLKPG instead, which - # is better than BLKRRPART for this case. BLKRRPART tells Linux to re-read - # partitions while BLKPG tells Linux which partitions are available - # BLKPG is usually used as a fallback system call. - begin = time.time() - while 'Device or resource busy' in out: - if time.time() > begin + timeout: - raise errors.BaseError('Unable to re-read partition table on' - 'device %s' % dev) - LOG.debug('Last time output contained "Device or resource busy". ' - 'Trying to re-read partition table on device %s' % dev) - out, err = utils.execute('partprobe', dev, check_exit_code=[0, 1]) - LOG.debug('Partprobe output: \n%s' % out) - pout, perr = utils.execute('partx', '-a', dev, check_exit_code=[0, 1]) - LOG.debug('Partx output: \n%s' % pout) - time.sleep(1)